Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release-4.15] OCPBUGS-49392: Block Upgrades for CA-Signed Certs Using SHA1 #1172

Open
wants to merge 2 commits into
base: release-4.15
Choose a base branch
from
Open

[release-4.15] OCPBUGS-49392: Block Upgrades for CA-Signed Certs Using SHA1 #1172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0e1ce8
OCPBUGS-45290: Block Upgrades for CA-Signed Certs Using SHA1
gcs278 Dec 4, 2024
b457c4a
OCPBUGS-49392: Add Upgrade Blocker for DSA SHA1 Default Certificates
gcs278 Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 50 additions & 5 deletions pkg/operator/controller/ingress/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ingress

import (
"bytes"
"context"
"crypto/x509"
"encoding/pem"
Expand Down Expand Up @@ -728,19 +729,63 @@ func checkDefaultCertificate(secret *corev1.Secret, domain string) error {
if cert.Subject.CommonName == domain && !foundSAN {
return fmt.Errorf("certificate in secret %s/%s has legacy Common Name (CN) but has no Subject Alternative Name (SAN) for domain: %s", secret.Namespace, secret.Name, domain)
}
// Prevent the upgrade only if the leaf certificate (the first certificate) uses a SHA1 signature algorithm.
// SHA1 can still be used by other certificates in the chain, such as the root and intermediate certificates.
if !cert.IsCA {
// Block the upgrade only if CA-signed certificates (not self-signed) uses a SHA1 signature algorithm.
// SHA1 can still be used by the root CA certificate.
if !isSelfSignedCert(cert) {
switch cert.SignatureAlgorithm {
case x509.SHA1WithRSA, x509.ECDSAWithSHA1:
return fmt.Errorf("certificate in secret %s/%s has weak SHA1 signature algorithm: %s (see https://docs.openshift.com/container-platform/4.16/release_notes/ocp-4-16-release-notes.html#ocp-4-16-sha-haproxy-support-removed_release-notes for more details)", secret.Namespace, secret.Name, cert.SignatureAlgorithm)
case x509.SHA1WithRSA, x509.ECDSAWithSHA1, x509.DSAWithSHA1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message doesn't say anything about adding x509.DSAWithSHA1. Is this PR actually fixing two separate defects (first, the use of cert.IsCA instead of checking the signer; and second, the missing x509.DSAWithSHA1 case expression)?

Copy link
Contributor Author

@gcs278 gcs278 Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EDIT: Sorry, I thought I was in the router repo not CIO....let me rethink/adjust my answer. Keeping the one below because it actually applies to openshift/router#641

I added per this comment thread for the 4.19 fix. It wasn't really fixing anything new, as DSA was already rejected by the router. I decided to add it to provide a more precise error message about SHA1 not being supported rather than a more obscure Invalid value: "redacted key data": block PRIVATE KEY is not valid.

For this 4.15 upgrade blocker, I carried it over for completeness/consistency, but I suppose that since it's already rejected, it's a bit pointless to block upgrades for it and may cause an unnecessary upgrade blocker for someone that already has a DSA SHA1 cert that is rejected. I don't think anyone should be using DSA SHA1 at this point, but I suppose its better to be safe.

I'll remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: Sorry for the confusion here. My answer above actually applies to openshift/router#641 (I'll follow up over there). Here's what I should have said...

Is this PR actually fixing two separate defects (first, the use of cert.IsCA instead of checking the signer; and second, the missing x509.DSAWithSHA1 case expression)?

Short Answer: Yes, upon further reflection, it is fixing two things, which I should mention in the commit message(s).

Long Answer: Originally, for this PR, I added DSA SHA1 without thinking much about it. However, I did some testing for DSA SHA1, and it's nuanced, but we DO actually need to block upgrades with DSA SHA1 for CIO. Here's why:

  • While openshift-router (not HAProxy) rejects DSA certificates because it doesn't have logic to parse them, the CIO allows cluster admins to create DSA SHA1 default certificates
  • However, you can't use the K8S TLS secret object because it can't parse the DSA key and you get an error (I guess K8S doesn't support it?) but you can still use an opaque/generic secret:
    • oc create secret generic router-cert -n openshift-ingress --from-file=tls.key=dsa.key --from-file=tls.crt=dsa-combo.crt
  • I tested a 4.15 router image with a DSA-SHA1 default cert, and it worked
  • I tested a 4.16 router image with a DSA-SHA1 default cert, and it failed

In summary, I was unaware there was a path for users to use DSA certs in the default certificate on the IngressController. I do think we need to block upgrades in CIO with DSA-SHA1 in this PR, but not in the router PR because the router has always rejected DSA certs. It's an odd situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the fixes two commit so I can clearly document each.

I'm not against splitting it out into another PR and/or bug if recommended, but it feels closely related enough I thought to keep it together for now.

return fmt.Errorf("a CA-signed certificate in secret %s/%s has weak SHA1 signature algorithm: %s (see https://docs.openshift.com/container-platform/4.16/release_notes/ocp-4-16-release-notes.html#ocp-4-16-sha-haproxy-support-removed_release-notes for more details)", secret.Namespace, secret.Name, cert.SignatureAlgorithm)
}
}
}

return nil
}

// isSelfSignedCert determines if a certificate is self-signed by verifying that the issuer matches the subject,
// the authority key identifier matches the subject key identifier, and the public key algorithm matches the
// signature algorithm. This logic mirrors the approach that OpenSSL uses to set the EXFLAG_SS flag, which
// indicates a certificate is self-signed.
// Ref: https://github.com/openssl/openssl/blob/b85e6f534906f0bf9114386d227e481d2336a0ff/crypto/x509/v3_purp.c#L557
func isSelfSignedCert(cert *x509.Certificate) bool {
issuerIsEqualToSubject := bytes.Equal(cert.RawIssuer, cert.RawSubject)
authorityKeyIsEqualToSubjectKey := bytes.Equal(cert.AuthorityKeyId, cert.SubjectKeyId)
algorithmIsConsistent := signatureAlgorithmToPublicKeyAlgorithm(cert.SignatureAlgorithm) == cert.PublicKeyAlgorithm
return issuerIsEqualToSubject &&
(cert.AuthorityKeyId == nil || authorityKeyIsEqualToSubjectKey) &&
algorithmIsConsistent
}

// signatureAlgorithmToPublicKeyAlgorithm maps a SignatureAlgorithm to its corresponding PublicKeyAlgorithm.
// Unfortunately, the x509 library does not expose a public mapping function for this.
// Returns UnknownPublicKeyAlgorithm if the mapping is not recognized.
func signatureAlgorithmToPublicKeyAlgorithm(sigAlgo x509.SignatureAlgorithm) x509.PublicKeyAlgorithm {
switch sigAlgo {
case x509.MD2WithRSA,
x509.MD5WithRSA,
x509.SHA1WithRSA,
x509.SHA256WithRSA,
x509.SHA384WithRSA,
x509.SHA512WithRSA,
x509.SHA256WithRSAPSS,
x509.SHA384WithRSAPSS,
x509.SHA512WithRSAPSS:
return x509.RSA
case x509.DSAWithSHA1,
x509.DSAWithSHA256:
return x509.DSA
case x509.ECDSAWithSHA1,
x509.ECDSAWithSHA256,
x509.ECDSAWithSHA384,
x509.ECDSAWithSHA512:
return x509.ECDSA
case x509.PureEd25519:
return x509.Ed25519
default:
return x509.UnknownPublicKeyAlgorithm
}
}

func formatConditions(conditions []*operatorv1.OperatorCondition) string {
var formatted string
if len(conditions) == 0 {
Expand Down
44 changes: 33 additions & 11 deletions pkg/operator/controller/ingress/status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2246,6 +2246,11 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) {
// If root cert is not provided, use the certTemplate to create a self-signed cert.
if rootCert == nil {
rootCert = certTemplate
} else {
// Go's x509.CreateCertificate assumes that if subject == issuer, the certificate is self-signed and does
// not add AuthorityKeyId. This fallback ensures AuthorityKeyId is included in cases where subject == issuer
// but the certificate is NOT self-signed. See https://github.com/golang/go/issues/62060.
certTemplate.AuthorityKeyId = rootCert.SubjectKeyId
}
var certBytes []byte
var privateKey crypto.PrivateKey
Expand Down Expand Up @@ -2297,14 +2302,21 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) {
sha256ECDSACertWithSans, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.ECDSAWithSHA256, nil, nil, false)
sha256ECDSACertWithNoSans, _ := makeCertificate(wildcardDomain, []string{}, x509.ECDSAWithSHA256, nil, nil, false)

sha1ECDSACert, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.ECDSAWithSHA1, nil, nil, false)
sha1RSACert, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA1WithRSA, nil, nil, false)
sha1ECDSACertSelfSigned, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.ECDSAWithSHA1, nil, nil, false)
sha1RSACertSelfSigned, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA1WithRSA, nil, nil, false)

sha1RSARootCA, sha1RSARootCAKey := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA1WithRSA, nil, nil, true)
// CA's need to use different common name to make the subject != issuer.
sha1RSARootCA, sha1RSARootCAKey := makeCertificate("sha1RootCA.com", []string{wildcardDomain}, x509.SHA1WithRSA, nil, nil, true)
sha256RSACertSignedBySha1RSA, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA256WithRSA, sha1RSARootCA, sha1RSARootCAKey, false)

sha256RSARootCA, sha256RSARootCAKey := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA256WithRSA, nil, nil, true)
sha1RSACertSignedBySha256RSA, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA1WithRSA, sha256RSARootCA, sha256RSARootCAKey, false)
sha256RSARootCA, sha256RSARootCAKey := makeCertificate("sha256RootCA.com", []string{wildcardDomain}, x509.SHA256WithRSA, nil, nil, true)
sha1RSACertSignedBySha256RSARootCA, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA1WithRSA, sha256RSARootCA, sha256RSARootCAKey, false)

sha1RSAIntCASignedBySha256RSARootCA, sha1RSAIntCASignedBySha256RSARootCAKey := makeCertificate("sha1IntCA.com", []string{wildcardDomain}, x509.SHA1WithRSA, sha256RSARootCA, sha256RSARootCAKey, true)
sha256RSACertSignedBySha1RSAIntCA, _ := makeCertificate(wildcardDomain, []string{wildcardDomain}, x509.SHA256WithRSA, sha1RSAIntCASignedBySha256RSARootCA, sha1RSAIntCASignedBySha256RSARootCAKey, false)

// Intentionally making the subject == issuer.
sha1RSACertSignedBySha256RSARootCASameSubjectIssuer, _ := makeCertificate("sha256RootCA.com", []string{wildcardDomain}, x509.SHA1WithRSA, sha256RSARootCA, sha256RSARootCAKey, false)

testCases := []struct {
description string
Expand Down Expand Up @@ -2352,13 +2364,13 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) {
},
{
description: "if the self-signed default certificate uses ECDSA SHA1",
secret: makeCertificateSecret(sha1ECDSACert),
expect: false,
secret: makeCertificateSecret(sha1ECDSACertSelfSigned),
expect: true,
},
{
description: "if the self-signed default certificate uses RSA SHA1",
secret: makeCertificateSecret(sha1RSACert),
expect: false,
secret: makeCertificateSecret(sha1RSACertSelfSigned),
expect: true,
},
{
description: "if the default leaf certificate uses RSA SHA256 included with its root certificate that uses RSA SHA1",
Expand All @@ -2367,12 +2379,22 @@ func Test_computeIngressUpgradeableCondition(t *testing.T) {
},
{
description: "if the default leaf certificate uses RSA SHA1 included with its root certificate that uses RSA SHA256",
secret: makeCertificateSecret(sha1RSACertSignedBySha256RSA, sha256RSARootCA),
secret: makeCertificateSecret(sha1RSACertSignedBySha256RSARootCA, sha256RSARootCA),
expect: false,
},
{
description: "if the default leaf certificate uses RSA SHA1 included with its root certificate that uses RSA SHA256, but the order is reversed",
secret: makeCertificateSecret(sha256RSARootCA, sha1RSACertSignedBySha256RSA),
secret: makeCertificateSecret(sha256RSARootCA, sha1RSACertSignedBySha256RSARootCA),
expect: false,
},
{
description: "if the default root and leaf certificate uses RSA SHA256, but the intermediate certificate uses RSA SHA1",
secret: makeCertificateSecret(sha256RSARootCA, sha1RSAIntCASignedBySha256RSARootCA, sha256RSACertSignedBySha1RSAIntCA),
expect: false,
},
{
description: "if the default leaf certificate uses RSA SHA1, but the subject is the same as issuer's subject",
secret: makeCertificateSecret(sha1RSACertSignedBySha256RSARootCASameSubjectIssuer),
expect: false,
},
}
Expand Down