From 67a477f0c6d0054cb54f3062fe62a76de6882a63 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 20 Apr 2023 13:22:48 +0800 Subject: [PATCH] fix: added truststore.ValidateCerts (#285) This PR aims to resolve #284. Signed-off-by: Patrick Zheng --- notation.go | 2 +- verifier/truststore/truststore.go | 23 ++++---------- verifier/truststore/truststore_test.go | 44 ++++++++++++++++++++++---- 3 files changed, 46 insertions(+), 23 deletions(-) diff --git a/notation.go b/notation.go index e8a1c905..0ec45662 100644 --- a/notation.go +++ b/notation.go @@ -327,7 +327,7 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, ve } var verificationOutcomes []*VerificationOutcome - errExceededMaxVerificationLimit := ErrorVerificationFailed{Msg: fmt.Sprintf("total number of signatures associated with an artifact should be less than: %d", verifyOpts.MaxSignatureAttempts)} + errExceededMaxVerificationLimit := ErrorVerificationFailed{Msg: fmt.Sprintf("total number of signatures associated with an artifact should be at most: %d", verifyOpts.MaxSignatureAttempts)} numOfSignatureProcessed := 0 var verificationFailedErr error = ErrorVerificationFailed{} diff --git a/verifier/truststore/truststore.go b/verifier/truststore/truststore.go index 6f2e8404..878847ba 100644 --- a/verifier/truststore/truststore.go +++ b/verifier/truststore/truststore.go @@ -60,7 +60,6 @@ func (trustStore *x509TrustStore) GetCertificates(ctx context.Context, storeType if err != nil { return nil, err } - // throw error if path is not a directory or is a symlink or does not exist. fileInfo, err := os.Lstat(path) if err != nil { @@ -73,7 +72,6 @@ func (trustStore *x509TrustStore) GetCertificates(ctx context.Context, storeType if !mode.IsDir() || mode&fs.ModeSymlink != 0 { return nil, fmt.Errorf("%q is not a regular directory (symlinks are not supported)", path) } - files, err := os.ReadDir(path) if err != nil { return nil, err @@ -89,40 +87,33 @@ func (trustStore *x509TrustStore) GetCertificates(ctx context.Context, storeType if err != nil { return nil, fmt.Errorf("error while reading certificates from %q: %w", joinedPath, err) } - - if err := validateCerts(certs, joinedPath); err != nil { - return nil, err + if err := ValidateCertificates(certs); err != nil { + return nil, fmt.Errorf("error while validating certificates from %q: %w", joinedPath, err) } - certificates = append(certificates, certs...) } - if len(certificates) < 1 { return nil, fmt.Errorf("trust store %q has no x509 certificates", path) } - return certificates, nil } -func validateCerts(certs []*x509.Certificate, path string) error { - // to prevent any trust store misconfigurations, ensure there is at least - // one certificate from each file. +// ValidateCertificates ensures certificates from trust store are +// CA certificates or self-signed. +func ValidateCertificates(certs []*x509.Certificate) error { if len(certs) < 1 { - return fmt.Errorf("could not parse a certificate from %q, every file in a trust store must have a PEM or DER certificate in it", path) + return errors.New("input certs cannot be empty") } - for _, cert := range certs { if !cert.IsCA { if err := cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature); err != nil { return fmt.Errorf( - "certificate with subject %q from file %q is not a CA certificate or self-signed signing certificate", + "certificate with subject %q is not a CA certificate or self-signed signing certificate", cert.Subject, - path, ) } } } - return nil } diff --git a/verifier/truststore/truststore_test.go b/verifier/truststore/truststore_test.go index b7d46195..92a85267 100644 --- a/verifier/truststore/truststore_test.go +++ b/verifier/truststore/truststore_test.go @@ -2,10 +2,12 @@ package truststore import ( "context" + "errors" "fmt" "path/filepath" "testing" + corex509 "github.com/notaryproject/notation-core-go/x509" "github.com/notaryproject/notation-go/dir" ) @@ -35,24 +37,54 @@ func TestLoadValidTrustStoreWithSelfSignedSigningCertificate(t *testing.T) { func TestLoadTrustStoreWithInvalidCerts(t *testing.T) { failurePath := filepath.FromSlash("../testdata/truststore/x509/ca/trust-store-with-invalid-certs/invalid") + expectedErr := fmt.Errorf("error while reading certificates from %q: x509: malformed certificate", failurePath) _, err := trustStore.GetCertificates(context.Background(), "ca", "trust-store-with-invalid-certs") - if err == nil || err.Error() != fmt.Sprintf("error while reading certificates from %q: x509: malformed certificate", failurePath) { - t.Fatalf("invalid certs should return error : %q", err) + if err == nil || err.Error() != expectedErr.Error() { + t.Fatalf("invalid certs should return error: %q", expectedErr) } } func TestLoadTrustStoreWithLeafCerts(t *testing.T) { failurePath := filepath.FromSlash("../testdata/truststore/x509/ca/trust-store-with-leaf-certs/non-ca.crt") + expectedErrMsg := fmt.Sprintf("error while validating certificates from %q: certificate with subject \"CN=wabbit-networks.io,O=Notary,L=Seattle,ST=WA,C=US\" is not a CA certificate or self-signed signing certificate", failurePath) _, err := trustStore.GetCertificates(context.Background(), "ca", "trust-store-with-leaf-certs") - if err == nil || err.Error() != fmt.Sprintf("certificate with subject \"CN=wabbit-networks.io,O=Notary,L=Seattle,ST=WA,C=US\" from file %q is not a CA certificate or self-signed signing certificate", failurePath) { - t.Fatalf("leaf cert in a trust store should return error : %q", err) + if err == nil || err.Error() != expectedErrMsg { + t.Fatalf("leaf cert in a trust store should return error: %s, got: %v", expectedErrMsg, err) } } func TestLoadTrustStoreWithLeafCertsInSingleFile(t *testing.T) { failurePath := filepath.FromSlash("../testdata/truststore/x509/ca/trust-store-with-leaf-certs-in-single-file/RootAndLeafCerts.crt") + expectedErrMsg := fmt.Sprintf("error while validating certificates from %q: certificate with subject \"CN=wabbit-networks.io,O=Notary,L=Seattle,ST=WA,C=US\" is not a CA certificate or self-signed signing certificate", failurePath) _, err := trustStore.GetCertificates(context.Background(), "ca", "trust-store-with-leaf-certs-in-single-file") - if err == nil || err.Error() != fmt.Sprintf("certificate with subject \"CN=wabbit-networks.io,O=Notary,L=Seattle,ST=WA,C=US\" from file %q is not a CA certificate or self-signed signing certificate", failurePath) { - t.Fatalf("leaf cert in a trust store should return error : %q", err) + if err == nil || err.Error() != expectedErrMsg { + t.Fatalf("leaf cert in a trust store should return error: %s, got: %v", expectedErrMsg, err) + } +} + +// TestValidCerts tests valid trust store cert +func TestValidateCerts(t *testing.T) { + joinedPath := filepath.FromSlash("../testdata/truststore/x509/ca/valid-trust-store/GlobalSign.der") + certs, err := corex509.ReadCertificateFile(joinedPath) + if err != nil { + t.Fatalf("error while reading certificates from %q: %q", joinedPath, err) + } + err = ValidateCertificates(certs) + if err != nil { + t.Fatalf("expected to get nil err, got %v", err) + } +} + +// TestValidateCertsWithLeafCert tests invalid trust store leaf cert +func TestValidateCertsWithLeafCert(t *testing.T) { + failurePath := filepath.FromSlash("../testdata/truststore/x509/ca/trust-store-with-leaf-certs/non-ca.crt") + certs, err := corex509.ReadCertificateFile(failurePath) + if err != nil { + t.Fatalf("error while reading certificates from %q: %q", failurePath, err) + } + expectedErr := errors.New("certificate with subject \"CN=wabbit-networks.io,O=Notary,L=Seattle,ST=WA,C=US\" is not a CA certificate or self-signed signing certificate") + err = ValidateCertificates(certs) + if err == nil || err.Error() != expectedErr.Error() { + t.Fatalf("leaf cert in a trust store should return error %q, got: %v", expectedErr, err) } }