From 78f2ec9165c9999ad2c5c749c97059f2ba94b7e7 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Mon, 13 Feb 2023 22:41:42 +0800 Subject: [PATCH] fix: updated error messages for certificate chain validation in x509 package (#120) Signed-off-by: Patrick Zheng --- x509/cert.go | 2 +- x509/cert_validations.go | 58 +++++++++++++++++++++++++---------- x509/cert_validations_test.go | 34 +++++++++++++++----- 3 files changed, 68 insertions(+), 26 deletions(-) diff --git a/x509/cert.go b/x509/cert.go index 20e11a41..65385927 100644 --- a/x509/cert.go +++ b/x509/cert.go @@ -6,7 +6,7 @@ import ( "os" ) -// ReadCertificateFile reads a certificate PEM file. +// ReadCertificateFile reads a certificate PEM or DER file. func ReadCertificateFile(path string) ([]*x509.Certificate, error) { data, err := os.ReadFile(path) if err != nil { diff --git a/x509/cert_validations.go b/x509/cert_validations.go index d7d5035c..dda44e1c 100644 --- a/x509/cert_validations.go +++ b/x509/cert_validations.go @@ -21,14 +21,16 @@ var kuLeafCertBlocked = x509.KeyUsageContentCommitment | var kuLeafCertBlockedString = "ContentCommitment, KeyEncipherment, DataEncipherment, KeyAgreement, " + "CertSign, CRLSign, EncipherOnly, DecipherOnly" -// ValidateCodeSigningCertChain takes an ordered code-signing certificate chain and validates issuance from leaf to root +// ValidateCodeSigningCertChain takes an ordered code-signing certificate chain +// and validates issuance from leaf to root // Validates certificates according to this spec: // https://github.com/notaryproject/notaryproject/blob/main/signature-specification.md#certificate-requirements func ValidateCodeSigningCertChain(certChain []*x509.Certificate, signingTime *time.Time) error { return validateCertChain(certChain, 0, signingTime) } -// ValidateTimeStampingCertChain takes an ordered time-stamping certificate chain and validates issuance from leaf to root +// ValidateTimeStampingCertChain takes an ordered time-stamping certificate +// chain and validates issuance from leaf to root // Validates certificates according to this spec: // https://github.com/notaryproject/notaryproject/blob/main/signature-specification.md#certificate-requirements func ValidateTimeStampingCertChain(certChain []*x509.Certificate, signingTime *time.Time) error { @@ -46,28 +48,48 @@ func validateCertChain(certChain []*x509.Certificate, expectedLeafEku x509.ExtKe if signingTime != nil && (signingTime.Before(cert.NotBefore) || signingTime.After(cert.NotAfter)) { return fmt.Errorf("certificate with subject %q was not valid at signing time of %s", cert.Subject, signingTime.UTC()) } - if err := cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature); err != nil { - return err + return fmt.Errorf("invalid self-signed certificate. subject: %q. Error: %w", cert.Subject, err) + } + if err := validateLeafCertificate(cert, expectedLeafEku); err != nil { + return fmt.Errorf("invalid self-signed certificate. Error: %w", err) } - return validateLeafCertificate(cert, expectedLeafEku) + return nil } for i, cert := range certChain { if signingTime != nil && (signingTime.Before(cert.NotBefore) || signingTime.After(cert.NotAfter)) { return fmt.Errorf("certificate with subject %q was not valid at signing time of %s", cert.Subject, signingTime.UTC()) } - if i == len(certChain)-1 { - if !isSelfSigned(cert) { - return errors.New("certificate chain must end with a root certificate (root certificates are self-signed)") + selfSigned, selfSignedError := isSelfSigned(cert) + if selfSignedError != nil { + return fmt.Errorf("root certificate with subject %q is invalid or not self-signed. Certificate chain must end with a valid self-signed root certificate. Error: %v", cert.Subject, selfSignedError) + } + if !selfSigned { + return fmt.Errorf("root certificate with subject %q is not self-signed. Certificate chain must end with a valid self-signed root certificate", cert.Subject) } } else { - // This is to avoid extra/redundant multiple root cert at the end of certificate-chain - if isSelfSigned(cert) { - return errors.New("certificate chain must not contain self-signed intermediate certificates") - } else if nextCert := certChain[i+1]; !isIssuedBy(cert, nextCert) { - return fmt.Errorf("certificate with subject %q is not issued by %q", cert.Subject, nextCert.Subject) + // This is to avoid extra/redundant multiple root cert at the end + // of certificate-chain + selfSigned, selfSignedError := isSelfSigned(cert) + // not checking selfSignedError != nil here because we expect + // a non-nil err. For a non-root certificate, it shouldn't be + // self-signed, hence CheckSignatureFrom would return a non-nil + // error. + if selfSignedError == nil && selfSigned { + if i == 0 { + return fmt.Errorf("leaf certificate with subject %q is self-signed. Certificate chain must not contain self-signed leaf certificate", cert.Subject) + } + return fmt.Errorf("intermediate certificate with subject %q is self-signed. Certificate chain must not contain self-signed intermediate certificate", cert.Subject) + } + parentCert := certChain[i+1] + issuedBy, issuedByError := isIssuedBy(cert, parentCert) + if issuedByError != nil { + return fmt.Errorf("invalid certificates or certificate with subject %q is not issued by %q. Error: %v", cert.Subject, parentCert.Subject, issuedByError) + } + if !issuedBy { + return fmt.Errorf("certificate with subject %q is not issued by %q", cert.Subject, parentCert.Subject) } } @@ -84,13 +106,15 @@ func validateCertChain(certChain []*x509.Certificate, expectedLeafEku x509.ExtKe return nil } -func isSelfSigned(cert *x509.Certificate) bool { +func isSelfSigned(cert *x509.Certificate) (bool, error) { return isIssuedBy(cert, cert) } -func isIssuedBy(subject *x509.Certificate, issuer *x509.Certificate) bool { - err := subject.CheckSignatureFrom(issuer) - return err == nil && bytes.Equal(issuer.RawSubject, subject.RawIssuer) +func isIssuedBy(subject *x509.Certificate, issuer *x509.Certificate) (bool, error) { + if err := subject.CheckSignatureFrom(issuer); err != nil { + return false, err + } + return bytes.Equal(issuer.RawSubject, subject.RawIssuer), nil } func validateCACertificate(cert *x509.Certificate, expectedPathLen int) error { diff --git a/x509/cert_validations_test.go b/x509/cert_validations_test.go index 4b680876..be8978ce 100644 --- a/x509/cert_validations_test.go +++ b/x509/cert_validations_test.go @@ -225,7 +225,8 @@ func TestFailChainNotEndingInRoot(t *testing.T) { signingTime := time.Now() err := ValidateCodeSigningCertChain(certChain, &signingTime) - assertErrorEqual("certificate chain must end with a root certificate (root certificates are self-signed)", err, t) + expected := "root certificate with subject \"CN=Intermediate1\" is invalid or not self-signed. Certificate chain must end with a valid self-signed root certificate. Error: crypto/rsa: verification error" + assertErrorEqual(expected, err, t) } func TestFailChainNotOrdered(t *testing.T) { @@ -233,7 +234,8 @@ func TestFailChainNotOrdered(t *testing.T) { signingTime := time.Now() err := ValidateCodeSigningCertChain(certChain, &signingTime) - assertErrorEqual("certificate with subject \"CN=CodeSigningLeaf\" is not issued by \"CN=Intermediate1\"", err, t) + expected := "invalid certificates or certificate with subject \"CN=CodeSigningLeaf\" is not issued by \"CN=Intermediate1\". Error: crypto/rsa: verification error" + assertErrorEqual(expected, err, t) } func TestFailChainWithUnrelatedCert(t *testing.T) { @@ -241,15 +243,26 @@ func TestFailChainWithUnrelatedCert(t *testing.T) { signingTime := time.Now() err := ValidateCodeSigningCertChain(certChain, &signingTime) - assertErrorEqual("certificate with subject \"CN=CodeSigningLeaf\" is not issued by \"CN=Hello\"", err, t) + expected := "invalid certificates or certificate with subject \"CN=CodeSigningLeaf\" is not issued by \"CN=Hello\". Error: x509: invalid signature: parent certificate cannot sign this kind of certificate" + assertErrorEqual(expected, err, t) } -func TestFailChainWithDuplicateRepeatedRoots(t *testing.T) { +func TestFailChainWithSelfSignedLeafCertificate(t *testing.T) { certChain := []*x509.Certificate{rootCert, rootCert, rootCert} signingTime := time.Now() err := ValidateCodeSigningCertChain(certChain, &signingTime) - assertErrorEqual("certificate chain must not contain self-signed intermediate certificates", err, t) + expected := "leaf certificate with subject \"CN=Root\" is self-signed. Certificate chain must not contain self-signed leaf certificate" + assertErrorEqual(expected, err, t) +} + +func TestFailChainWithSelfSignedIntermediateCertificate(t *testing.T) { + certChain := []*x509.Certificate{codeSigningCert, intermediateCert2, intermediateCert1, rootCert, rootCert} + signingTime := time.Now() + + err := ValidateCodeSigningCertChain(certChain, &signingTime) + expected := "intermediate certificate with subject \"CN=Root\" is self-signed. Certificate chain must not contain self-signed intermediate certificate" + assertErrorEqual(expected, err, t) } func TestFailInvalidPathLen(t *testing.T) { @@ -261,8 +274,12 @@ func TestFailInvalidPathLen(t *testing.T) { } func TestRootCertIdentified(t *testing.T) { - if isSelfSigned(codeSigningCert) || isSelfSigned(intermediateCert1) || - isSelfSigned(intermediateCert2) || !isSelfSigned(rootCert) { + selfSignedCodeSigning, _ := isSelfSigned(codeSigningCert) + selfSignedIntermediateCert1, _ := isSelfSigned(intermediateCert1) + selfSignedIntermediateCert2, _ := isSelfSigned(intermediateCert2) + selfSignedRootCert, _ := isSelfSigned(rootCert) + if selfSignedCodeSigning || selfSignedIntermediateCert1 || + selfSignedIntermediateCert2 || !selfSignedRootCert { t.Fatal("Root cert was not correctly identified") } } @@ -271,7 +288,8 @@ func TestInvalidSelfSignedSigningCertificate(t *testing.T) { certChain := []*x509.Certificate{testhelper.GetRSARootCertificate().Cert} signingTime := time.Now() err := ValidateCodeSigningCertChain(certChain, &signingTime) - assertErrorEqual("certificate with subject \"CN=Notation Test RSA Root,O=Notary,L=Seattle,ST=WA,C=US\": if the basic constraints extension is present, the ca field must be set to false", err, t) + expected := "invalid self-signed certificate. Error: certificate with subject \"CN=Notation Test RSA Root,O=Notary,L=Seattle,ST=WA,C=US\": if the basic constraints extension is present, the ca field must be set to false" + assertErrorEqual(expected, err, t) } // ---------------- CA Validations ----------------