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.

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

OCPBUGS-45290: Reject All CA-Signed Certs Using SHA1 #642

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
85 changes: 77 additions & 8 deletions pkg/router/routeapihelpers/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,11 @@ func ExtendedValidateRoute(route *routev1.Route) field.ErrorList {
} else {
tlsConfig.CACertificate = string(data)
}
// HAProxy will fail to start if intermediate CA certs use unsupported signature algorithms.
// However, root CAs can still use unsupported algorithms since they are self-signed.
Copy link
Contributor

@candita candita Dec 11, 2024

Choose a reason for hiding this comment

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

Should you only check the CACertificate for whether it's self-signed, instead of within validateCertSignatureAlgorithms? In other words, is a self-signed cert okay to be ignored for a DestinationCACertificate or Certificate, which are checked by validateCertSignatureAlgorithm or validateCertificatePEM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In other words, is a self-signed cert okay to be ignored for a DestinationCACertificate or Certificate

DestinationCACertificate needs to allow SHA1 self-signed certs (a lot of root CAs use SHA1). So it is required for us to ignore self-signed certs in our SHA1 validation here.

As for Certificate, there isn't a critical business need to allow SHA1 self-signed leaf certs unlike DestinationCACertificate and CACertificate. But two thoughts:

  1. I think the router should be accurate in what it's rejecting, and shouldn't reject things that HAProxy actually supports.
  2. 4.15 dev clusters (I hope not prod...) may be using self-signed SHA1 certs for testing. When they upgrade, we are currently forcing them to recreate all of them, even though 4.16 HAProxy supports them. Unnecessary upgrade pains for customers.

Open to other opinions.

if err := validateCertSignatureAlgorithms(certs); err != nil {
result = append(result, field.Invalid(tlsFieldPath.Child("caCertificate"), "redacted ca certificate data", err.Error()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this log message identify the route, so that the invalid certificate can be found/fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This message gets written to the route's Admitted condition type with the reason ExtendedValidationFailed. It is directly written on the affected route, which makes it clear of which route is impacted.

}
}

verifyOptions = &x509.VerifyOptions{
Expand Down Expand Up @@ -254,7 +259,7 @@ func ExtendedValidateRoute(route *routev1.Route) field.ErrorList {
}

if len(tlsConfig.DestinationCACertificate) > 0 {
if _, err := cert.ParseCertsPEM([]byte(tlsConfig.DestinationCACertificate)); err != nil {
if certs, err := cert.ParseCertsPEM([]byte(tlsConfig.DestinationCACertificate)); err != nil {
errmsg := fmt.Sprintf("failed to parse destination CA certificate: %v", err)
result = append(result, field.Invalid(tlsFieldPath.Child("destinationCACertificate"), "redacted destination ca certificate data", errmsg))
} else {
Expand All @@ -263,6 +268,11 @@ func ExtendedValidateRoute(route *routev1.Route) field.ErrorList {
} else {
tlsConfig.DestinationCACertificate = string(data)
}
// Unsupported destinationCACertificates algorithms won't prevent HAProxy from starting.
// However, HAProxy will quietly refuse to use them at runtime. Rejecting here improves UX.

Choose a reason for hiding this comment

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

How does HAProxy refusing them at runtime improve user experience?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rejecting the certs "here" (in this function) improves UX. I don't mean HAProxy refusing them improves UX.

If we don't reject CA-signed destinationCACertificates with SHA1, then HAProxy will quietly fail (connections will just stop working with an SSL error), which is almost certainly going to cause users to get confused. Loud/Verbose failure better than quiet/subtle failure.

if err := validateCertSignatureAlgorithms(certs); err != nil {
result = append(result, field.Invalid(tlsFieldPath.Child("destinationCACertificate"), "redacted ca certificate data", err.Error()))
}
}
}

Expand Down Expand Up @@ -353,6 +363,70 @@ func validateInsecureEdgeTerminationPolicy(tls *routev1.TLSConfig, fldPath *fiel
return nil
}

// isSelfSignedCert determines if a certificate is self-signed by verifying that the issuer matches the subject,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related thread describing my thought process here: #642 (comment)

// 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 {
isIssuerEqualToSubject := bytes.Equal(cert.RawIssuer, cert.RawSubject)
isAuthorityKeyEqualToSubjectKey := bytes.Equal(cert.AuthorityKeyId, cert.SubjectKeyId)
isAlgorithmConsistent := signatureAlgorithmToPublicKeyAlgorithm(cert.SignatureAlgorithm) == cert.PublicKeyAlgorithm
return isIssuerEqualToSubject &&
(cert.AuthorityKeyId == nil || isAuthorityKeyEqualToSubjectKey) &&
isAlgorithmConsistent
Comment on lines +372 to +377
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nit. Change if there ends up being other things to change. The variable names could be a little more clear since they are bool:

Suggested change
isIssuerEqualToSubject := bytes.Equal(cert.RawIssuer, cert.RawSubject)
isAuthorityKeyEqualToSubjectKey := bytes.Equal(cert.AuthorityKeyId, cert.SubjectKeyId)
isAlgorithmConsistent := signatureAlgorithmToPublicKeyAlgorithm(cert.SignatureAlgorithm) == cert.PublicKeyAlgorithm
return isIssuerEqualToSubject &&
(cert.AuthorityKeyId == nil || isAuthorityKeyEqualToSubjectKey) &&
isAlgorithmConsistent
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 {

Choose a reason for hiding this comment

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

Why is this mapping necessary for the validation process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea it's a good question. I explain it a bit in the go doc of isSelfSignedCert:

// 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.

The goal is to mirror what HAProxy/OpenSSL will reject. And to do that with 100% accuracy, we need to mirror what OpenSSL rejects. This line of code in the OpenSSL library compares the signature algorithm of the public key to the cert to determine if it's self-signed.

In order to mirror that OpenSSL algorithm comparison, we must add this function signatureAlgorithmToPublicKeyAlgorithm to convert the cert's signature algorithm to a public key algorithm type.

I did a lot of digging in Github to figure out exactly why they compare these two algorithms, and the best I could find was that it helps establish that the public key of the cert was used to sign the certificate (aka it's self signed).

switch sigAlgo {
case x509.SHA1WithRSA,
Copy link
Contributor

@candita candita Jan 10, 2025

Choose a reason for hiding this comment

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

So SHA1 with RSA,DSA, or ECDSA sig algorithm is valid in our case for an RSA public key algorithm, but invalid for the CA-signed certs? In other words, I guess I expected us not to return a valid mapping for the SHA1 sig algorithms. I'm just thinking of the intention of the title: "Reject all CA-Signed Certs using SHA1".

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
}
}

// validateCertSignatureAlgorithms checks if the certificate list has any certs that use a
// signature algorithm that the router no longer supports. If an unsupported cert is present, HAProxy
// may refuse to start (server & CA certs) or may start but reject connections (destination CA certs).
func validateCertSignatureAlgorithms(certs []*x509.Certificate) error {
for _, cert := range certs {
// Verify the signature algorithms only for certs signed by a CA.
// Since OpenSSL doesn't validate self-signed certificates, the signature algorithm check can be skipped.
// It's important that we do NOT reject self-signed certificates, as many root CAs still utilize SHA1.
if !isSelfSignedCert(cert) {
switch certs[0].SignatureAlgorithm {
Copy link
Contributor

@candita candita Jan 10, 2025

Choose a reason for hiding this comment

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

We don't need to check certs[0].SignatureAlgorithm's value on every iteration, because it doesn't change. Also, shouldn't we collect the errors on all certs before returning?

case x509.SHA1WithRSA, x509.ECDSAWithSHA1:
Copy link
Contributor

@candita candita Jan 10, 2025

Choose a reason for hiding this comment

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

What about DSAWithSHA1?

return fmt.Errorf("router does not support CA-signed certs using SHA1")
case x509.MD5WithRSA:
return fmt.Errorf("router does not support CA-signed certs using MD5")
default:
// Acceptable algorithm
}
}
}
return nil
}

// validateCertificatePEM checks if a certificate PEM is valid and
// optionally verifies the certificate using the options.
func validateCertificatePEM(certPEM string, options *x509.VerifyOptions) ([]*x509.Certificate, error) {
Expand All @@ -366,13 +440,8 @@ func validateCertificatePEM(certPEM string, options *x509.VerifyOptions) ([]*x50
}

// Reject any unsupported cert algorithms as HaProxy will refuse to start with them.
switch certs[0].SignatureAlgorithm {
case x509.SHA1WithRSA, x509.ECDSAWithSHA1:
return certs, fmt.Errorf("router does not support certs using SHA1")
case x509.MD5WithRSA:
return certs, fmt.Errorf("router does not support certs using MD5")
default:
// Acceptable algorithm
if err := validateCertSignatureAlgorithms(certs); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned, this allows the cert to be self-signed now - introducing new behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Our validation was invalid previously, rejecting valid scenarios, so this fixes that.

Do you think that will be a problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm starting to get it - we want to reject certs using SHA1 if they are CA-signed, but not if they are self-signed.

return certs, err
}

if options != nil {
Expand Down
Loading