fix: converge the logic for self-signed certificate

Signed-off-by: Junjie Gao <[email protected]>

revocation/ocsp/ocsp_test.go

@@ -128,7 +128,7 @@ func TestCheckStatusForNonSelfSignedSingleCert(t *testing.T) {
 	}
 
 	certResults, err := CheckStatus(opts)
-	expectedErr := result.InvalidChainError{Err: errors.New("invalid self-signed leaf certificate. subject: \"CN=Notation Test RSA Leaf Cert,O=Notary,L=Seattle,ST=WA,C=US\". Error: crypto/rsa: verification error")}
+	expectedErr := result.InvalidChainError{Err: errors.New("invalid self-signed leaf certificate. Subject: \"CN=Notation Test RSA Leaf Cert,O=Notary,L=Seattle,ST=WA,C=US\". Error: crypto/rsa: verification error")}
 	if err == nil || err.Error() != expectedErr.Error() {
 		t.Errorf("Expected CheckStatus to fail with %v, but got: %v", expectedErr, err)
 	}
@@ -421,7 +421,7 @@ func TestCheckStatusErrors(t *testing.T) {
 
 	timestampSigningCertErr := result.InvalidChainError{Err: errors.New("timestamp signing certificate with subject \"CN=Notation Test Revokable RSA Chain Cert 3,O=Notary,L=Seattle,ST=WA,C=US\" must have and only have Timestamping as extended key usage")}
 	backwardsChainErr := result.InvalidChainError{Err: errors.New("leaf certificate with subject \"CN=Notation Test Revokable RSA Chain Cert Root,O=Notary,L=Seattle,ST=WA,C=US\" is self-signed. Certificate chain must not contain self-signed leaf certificate")}
-	chainRootErr := result.InvalidChainError{Err: errors.New("root certificate with subject \"CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US\" is not self-signed. Certificate chain must end with a valid self-signed root certificate")}
+	chainRootErr := result.InvalidChainError{Err: errors.New("self-signed certificate with subject \"CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US\" is not self-issued. Certificate chain must end with a valid self-signed root certificate")}
 	expiredRespErr := GenericError{Err: errors.New("expired OCSP response")}
 	noHTTPErr := GenericError{Err: errors.New("OCSPServer protocol ldap is not supported")}
 

revocation/revocation_test.go

@@ -833,7 +833,7 @@ func TestCheckRevocationErrors(t *testing.T) {
 	noHTTPChain := []*x509.Certificate{noHTTPLeaf, revokableTuples[1].Cert, revokableTuples[2].Cert}
 
 	backwardsChainErr := result.InvalidChainError{Err: errors.New("leaf certificate with subject \"CN=Notation Test Revokable RSA Chain Cert Root,O=Notary,L=Seattle,ST=WA,C=US\" is self-signed. Certificate chain must not contain self-signed leaf certificate")}
-	chainRootErr := result.InvalidChainError{Err: errors.New("root certificate with subject \"CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US\" is not self-signed. Certificate chain must end with a valid self-signed root certificate")}
+	chainRootErr := result.InvalidChainError{Err: errors.New("self-signed certificate with subject \"CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US\" is not self-issued. Certificate chain must end with a valid self-signed root certificate")}
 	expiredRespErr := revocationocsp.GenericError{Err: errors.New("expired OCSP response")}
 	noHTTPErr := revocationocsp.GenericError{Err: errors.New("OCSPServer protocol ldap is not supported")}
 

x509/codesigning_cert_validations.go

@@ -34,7 +34,7 @@ func ValidateCodeSigningCertChain(certChain []*x509.Certificate, signingTime *ti
 	// For self-signed signing certificate (not a CA)
 	if len(certChain) == 1 {
 		cert := certChain[0]
-		if err := validateSelfSignedLeaf(cert); err != nil {
+		if err := validateSelfSignedCert(cert, true); err != nil {
 			return err
 		}
 		if signedTimeError := validateSigningTime(cert, signingTime); signedTimeError != nil {
@@ -51,22 +51,18 @@ func ValidateCodeSigningCertChain(certChain []*x509.Certificate, signingTime *ti
 			return signedTimeError
 		}
 		if i == len(certChain)-1 {
-			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)
+			if err := validateSelfSignedCert(cert, false); err != nil {
+				return err
 			}
 		} else {
 			// This is to avoid extra/redundant multiple root cert at the end
 			// of certificate-chain
-			selfSigned, selfSignedError := isSelfSigned(cert)
+			selfSignedError := validateSelfSignedCert(cert, false)
 			// 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 selfSignedError == nil {
 				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)
 				}

x509/codesigning_cert_validations_test.go

@@ -210,7 +210,7 @@ func TestInvalidSelfSignedLeaf(t *testing.T) {
 	signingTime := time.Now()
 
 	err = ValidateCodeSigningCertChain(certChain, &signingTime)
-	assertErrorEqual("invalid self-signed leaf certificate. subject: \"CN=valid cert\". Error: issuer and subject are not the same", err, t)
+	assertErrorEqual("self-signed certificate with subject \"CN=valid cert\" is not self-issued. Certificate chain must end with a valid self-signed root certificate", err, t)
 }
 
 func TestInvalidCodeSigningCertSigningTime(t *testing.T) {
@@ -320,13 +320,17 @@ func TestFailInvalidPathLen(t *testing.T) {
 }
 
 func TestRootCertIdentified(t *testing.T) {
-	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")
+	if err := validateSelfSignedCert(codeSigningCert, false); err == nil {
+		t.Error("expected error as leaf certificate is not self-signed")
+	}
+	if err := validateSelfSignedCert(intermediateCert1, false); err == nil {
+		t.Error("expected error as intermediate certificate is not self-signed")
+	}
+	if err := validateSelfSignedCert(intermediateCert2, false); err == nil {
+		t.Error("expected error as intermediate certificate is not self-signed")
+	}
+	if err := validateSelfSignedCert(rootCert, false); err != nil {
+		t.Errorf("unexpected error: %v", err)
 	}
 }
 

x509/helper.go

@@ -23,23 +23,33 @@ import (
 	"github.com/notaryproject/notation-core-go/signature"
 )
 
-// validateSelfSignedLeaf validates a self-signed leaf certificate.
+// validateSelfSignedCert validates a self-signed certificate.
 //
-// A self-signed leaf certificate must have the same subject and issuer.
-func validateSelfSignedLeaf(cert *x509.Certificate) error {
-	if err := cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature); err != nil {
-		return fmt.Errorf("invalid self-signed leaf certificate. subject: %q. Error: %w", cert.Subject, err)
+// This function handles 2 cases for isSingleCertChian:
+//  1. True: The certificate is a self-signed leaf certificate. It is both
+//     a leaf and root certificate.
+//  2. False: The certificate is a self-signed root CA certificate.
+//
+// A self-signed certificate must have the same subject and issuer.
+func validateSelfSignedCert(cert *x509.Certificate, isSingleCertChain bool) error {
+	if isSingleCertChain {
+		// only check signature
+		if err := cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature); err != nil {
+			return fmt.Errorf("invalid self-signed leaf certificate. Subject: %q. Error: %w", cert.Subject, err)
+		}
+	} else {
+		// check root CA
+		if err := cert.CheckSignatureFrom(cert); err != 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, err)
+		}
 	}
+
 	if !bytes.Equal(cert.RawSubject, cert.RawIssuer) {
-		return fmt.Errorf("invalid self-signed leaf certificate. subject: %q. Error: issuer and subject are not the same", cert.Subject)
+		return fmt.Errorf("self-signed certificate with subject %q is not self-issued. Certificate chain must end with a valid self-signed root certificate", cert.Subject)
 	}
 	return nil
 }
 
-func isSelfSigned(cert *x509.Certificate) (bool, error) {
-	return isIssuedBy(cert, cert)
-}
-
 func isIssuedBy(subject *x509.Certificate, issuer *x509.Certificate) (bool, error) {
 	if err := subject.CheckSignatureFrom(issuer); err != nil {
 		return false, err

x509/helper_test.go

@@ -82,30 +82,34 @@ func TestValidateSelfSignedLeaf(t *testing.T) {
 	}
 
 	tests := []struct {
-		name    string
-		cert    *x509.Certificate
-		wantErr bool
+		name         string
+		cert         *x509.Certificate
+		isSingleCert bool
+		wantErr      bool
 	}{
 		{
-			name:    "Valid Self-Signed Certificate",
-			cert:    selfSignedCert,
-			wantErr: false,
+			name:         "Valid Self-Signed Certificate",
+			cert:         selfSignedCert,
+			isSingleCert: true,
+			wantErr:      false,
 		},
 		{
-			name:    "Empty Certificate",
-			cert:    emptyCert,
-			wantErr: true,
+			name:         "Empty Certificate",
+			cert:         emptyCert,
+			isSingleCert: true,
+			wantErr:      true,
 		},
 		{
-			name:    "Not Self-Issued Certificate",
-			cert:    notSelfIssuedCert,
-			wantErr: true,
+			name:         "Not Self-Issued Certificate",
+			cert:         notSelfIssuedCert,
+			isSingleCert: true,
+			wantErr:      true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			err := validateSelfSignedLeaf(tt.cert)
+			err := validateSelfSignedCert(tt.cert, tt.isSingleCert)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("validateSelfSignedLeaf() error = %v, wantErr %v", err, tt.wantErr)
 			}

x509/timestamp_cert_validations.go

@@ -33,7 +33,7 @@ func ValidateTimestampingCertChain(certChain []*x509.Certificate) error {
 	// For self-signed signing certificate (not a CA)
 	if len(certChain) == 1 {
 		cert := certChain[0]
-		if err := validateSelfSignedLeaf(cert); err != nil {
+		if err := validateSelfSignedCert(cert, true); err != nil {
 			return err
 		}
 		if err := validateTimestampingLeafCertificate(cert); err != nil {
@@ -44,22 +44,18 @@ func ValidateTimestampingCertChain(certChain []*x509.Certificate) error {
 
 	for i, cert := range certChain {
 		if i == len(certChain)-1 {
-			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)
+			if err := validateSelfSignedCert(cert, false); err != nil {
+				return err
 			}
 		} else {
 			// This is to avoid extra/redundant multiple root cert at the end
 			// of certificate-chain
-			selfSigned, selfSignedError := isSelfSigned(cert)
+			selfSignedError := validateSelfSignedCert(cert, false)
 			// 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 selfSignedError == nil {
 				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)
 				}

x509/timestamp_cert_validations_test.go

@@ -91,7 +91,7 @@ func TestInvalidTimestampingChain(t *testing.T) {
 	assertErrorEqual(expectedErr, err, t)
 
 	certChain = []*x509.Certificate{timestamp_leaf}
-	expectedErr = "invalid self-signed leaf certificate. subject: \"CN=DigiCert Timestamp 2023,O=DigiCert\\\\, Inc.,C=US\". Error: crypto/rsa: verification error"
+	expectedErr = "invalid self-signed leaf certificate. Subject: \"CN=DigiCert Timestamp 2023,O=DigiCert\\\\, Inc.,C=US\". Error: crypto/rsa: verification error"
 	err = ValidateTimestampingCertChain(certChain)
 	assertErrorEqual(expectedErr, err, t)