fix: updated error messages for certificate chain validation in x509 …

…package (#120)

Signed-off-by: Patrick Zheng <[email protected]>

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 {

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 {

x509/cert_validations_test.go

@@ -225,31 +225,44 @@ 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) {
 	certChain := []*x509.Certificate{codeSigningCert, intermediateCert1, intermediateCert2, rootCert}
 	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) {
 	certChain := []*x509.Certificate{codeSigningCert, unrelatedCert, intermediateCert1, rootCert}
 	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 ----------------