diff --git a/revocation/ocsp/ocsp_test.go b/revocation/ocsp/ocsp_test.go index 86e42ecc..8c84ef5d 100644 --- a/revocation/ocsp/ocsp_test.go +++ b/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")} diff --git a/revocation/revocation_test.go b/revocation/revocation_test.go index 77b1e70c..25558b60 100644 --- a/revocation/revocation_test.go +++ b/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")} diff --git a/x509/codesigning_cert_validations.go b/x509/codesigning_cert_validations.go index f0e6ed90..06c6a1a6 100644 --- a/x509/codesigning_cert_validations.go +++ b/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) } diff --git a/x509/codesigning_cert_validations_test.go b/x509/codesigning_cert_validations_test.go index 4fd74d78..dfc80e2c 100644 --- a/x509/codesigning_cert_validations_test.go +++ b/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) } } diff --git a/x509/helper.go b/x509/helper.go index a99cfa2b..5020a922 100644 --- a/x509/helper.go +++ b/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 diff --git a/x509/helper_test.go b/x509/helper_test.go index a6357395..c8b9cc11 100644 --- a/x509/helper_test.go +++ b/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) } diff --git a/x509/timestamp_cert_validations.go b/x509/timestamp_cert_validations.go index 738d1b1a..50fa43a4 100644 --- a/x509/timestamp_cert_validations.go +++ b/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) } diff --git a/x509/timestamp_cert_validations_test.go b/x509/timestamp_cert_validations_test.go index 2314e03d..a446d4c3 100644 --- a/x509/timestamp_cert_validations_test.go +++ b/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)