fix: added truststore.ValidateCerts (notaryproject#285)

This PR aims to resolve notaryproject#284.

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

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{}

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
 }
 

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)
 	}
 }