-
Notifications
You must be signed in to change notification settings - Fork 116
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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. | ||||||||||||||||||||||||||
if err := validateCertSignatureAlgorithms(certs); err != nil { | ||||||||||||||||||||||||||
result = append(result, field.Invalid(tlsFieldPath.Child("caCertificate"), "redacted ca certificate data", err.Error())) | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This message gets written to the route's |
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
verifyOptions = &x509.VerifyOptions{ | ||||||||||||||||||||||||||
|
@@ -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 { | ||||||||||||||||||||||||||
|
@@ -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. | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does HAProxy refusing them at runtime improve user experience? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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())) | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
@@ -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, | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
// 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 { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this mapping necessary for the validation process? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
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 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, | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||||||||||||||||||||||||||
|
@@ -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 { | ||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||||||||||||||||||||||||||
|
There was a problem hiding this comment.
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 withinvalidateCertSignatureAlgorithms
? In other words, is a self-signed cert okay to be ignored for aDestinationCACertificate
orCertificate
, which are checked byvalidateCertSignatureAlgorithm
orvalidateCertificatePEM
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 unlikeDestinationCACertificate
andCACertificate
. But two thoughts:Open to other opinions.