updated per code review

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

Author: Two-Hearts

internal/slice/slice.go

@@ -1,10 +1,5 @@
 package slice
 
-const (
-	Wildcard    = "*"
-	X509Subject = "x509.subject"
-)
-
 // ContainsString is a utility function to check if a string exists in an array
 func ContainsString(val string, values []string) bool {
 	for _, v := range values {

internal/trustpolicy/trustpolicy.go

@@ -0,0 +1,6 @@
+package trustpolicy
+
+const (
+	Wildcard    = "*"
+	X509Subject = "x509.subject"
+)

verification/trustpolicy/trustpolicy.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/notaryproject/notation-go/internal/pkix"
 	"github.com/notaryproject/notation-go/internal/slice"
+	"github.com/notaryproject/notation-go/internal/trustpolicy"
 	"github.com/notaryproject/notation-go/verification/truststore"
 )
 
@@ -168,7 +169,7 @@ func (policyDoc *Document) Validate() error {
 		policyStatementNameCount[statement.Name]++
 
 		// Verify signature verification level is valid
-		verificationLevel, err := GetVerificationLevel(statement.SignatureVerification)
+		verificationLevel, err := statement.SignatureVerification.GetVerificationLevel()
 		if err != nil {
 			return fmt.Errorf("trust policy statement %q uses invalid signatureVerification value %q", statement.Name, statement.SignatureVerification.VerificationLevel)
 		}
@@ -213,9 +214,43 @@ func (policyDoc *Document) Validate() error {
 	return nil
 }
 
+// GetApplicableTrustPolicy returns a pointer to the deep copied TrustPolicy
+// statement that applies to the given registry URI. If no applicable trust
+// policy is found, returns an error
+// see https://github.com/notaryproject/notaryproject/blob/main/trust-store-trust-policy-specification.md#selecting-a-trust-policy-based-on-artifact-uri
+func (trustPolicyDoc *Document) GetApplicableTrustPolicy(artifactReference string) (*TrustPolicy, error) {
+
+	artifactPath, err := getArtifactPathFromReference(artifactReference)
+	if err != nil {
+		return nil, err
+	}
+
+	var wildcardPolicy *TrustPolicy
+	var applicablePolicy *TrustPolicy
+	for _, policyStatement := range trustPolicyDoc.TrustPolicies {
+		if slice.ContainsString(trustpolicy.Wildcard, policyStatement.RegistryScopes) {
+			// we need to deep copy because we can't use the loop variable
+			// address. see https://stackoverflow.com/a/45967429
+			wildcardPolicy = (&policyStatement).clone()
+		} else if slice.ContainsString(artifactPath, policyStatement.RegistryScopes) {
+			applicablePolicy = (&policyStatement).clone()
+		}
+	}
+
+	if applicablePolicy != nil {
+		// a policy with exact match for registry URI takes precedence over
+		// a wildcard (*) policy.
+		return applicablePolicy, nil
+	} else if wildcardPolicy != nil {
+		return wildcardPolicy, nil
+	} else {
+		return nil, fmt.Errorf("artifact %q has no applicable trust policy", artifactReference)
+	}
+}
+
 // GetVerificationLevel returns VerificationLevel struct for the given
 // SignatureVerification struct throws error if SignatureVerification is invalid
-func GetVerificationLevel(signatureVerification SignatureVerification) (*VerificationLevel, error) {
+func (signatureVerification *SignatureVerification) GetVerificationLevel() (*VerificationLevel, error) {
 	var baseLevel *VerificationLevel
 	for _, l := range VerificationLevels {
 		if l.Name == signatureVerification.VerificationLevel {
@@ -281,6 +316,17 @@ func GetVerificationLevel(signatureVerification SignatureVerification) (*Verific
 	return customVerificationLevel, nil
 }
 
+// clone returns a pointer to the deeply copied TrustPolicy
+func (t *TrustPolicy) clone() *TrustPolicy {
+	return &TrustPolicy{
+		Name:                  t.Name,
+		SignatureVerification: t.SignatureVerification,
+		RegistryScopes:        append([]string(nil), t.RegistryScopes...),
+		TrustedIdentities:     append([]string(nil), t.TrustedIdentities...),
+		TrustStores:           append([]string(nil), t.TrustStores...),
+	}
+}
+
 // validateTrustStore validates if the policy statement is following the
 // Notary V2 spec rules for truststores
 func validateTrustStore(statement TrustPolicy) error {
@@ -300,7 +346,7 @@ func validateTrustedIdentities(statement TrustPolicy) error {
 
 	// If there is a wildcard in trusted identies, there shouldn't be any other
 	//identities
-	if len(statement.TrustedIdentities) > 1 && slice.ContainsString(slice.Wildcard, statement.TrustedIdentities) {
+	if len(statement.TrustedIdentities) > 1 && slice.ContainsString(trustpolicy.Wildcard, statement.TrustedIdentities) {
 		return fmt.Errorf("trust policy statement %q uses a wildcard trusted identity '*', a wildcard identity cannot be used in conjunction with other values", statement.Name)
 	}
 
@@ -311,7 +357,7 @@ func validateTrustedIdentities(statement TrustPolicy) error {
 			return fmt.Errorf("trust policy statement %q has an empty trusted identity", statement.Name)
 		}
 
-		if identity != slice.Wildcard {
+		if identity != trustpolicy.Wildcard {
 			i := strings.Index(identity, ":")
 			if i < 0 {
 				return fmt.Errorf("trust policy statement %q has trusted identity %q without an identity prefix", statement.Name, identity)
@@ -321,7 +367,7 @@ func validateTrustedIdentities(statement TrustPolicy) error {
 			identityValue := identity[i+1:]
 
 			// notation natively supports x509.subject identities only
-			if identityPrefix == slice.X509Subject {
+			if identityPrefix == trustpolicy.X509Subject {
 				dn, err := pkix.ParseDistinguishedName(identityValue)
 				if err != nil {
 					return err
@@ -332,7 +378,7 @@ func validateTrustedIdentities(statement TrustPolicy) error {
 	}
 
 	// Verify there are no overlapping DNs
-	if err := hasOverlappingDNs(statement.Name, parsedDNs); err != nil {
+	if err := validateOverlappingDNs(statement.Name, parsedDNs); err != nil {
 		return err
 	}
 
@@ -350,11 +396,11 @@ func validateRegistryScopes(policyDoc *Document) error {
 		if len(statement.RegistryScopes) == 0 {
 			return fmt.Errorf("trust policy statement %q has zero registry scopes, it must specify registry scopes with at least one value", statement.Name)
 		}
-		if len(statement.RegistryScopes) > 1 && slice.ContainsString(slice.Wildcard, statement.RegistryScopes) {
+		if len(statement.RegistryScopes) > 1 && slice.ContainsString(trustpolicy.Wildcard, statement.RegistryScopes) {
 			return fmt.Errorf("trust policy statement %q uses wildcard registry scope '*', a wildcard scope cannot be used in conjunction with other scope values", statement.Name)
 		}
 		for _, scope := range statement.RegistryScopes {
-			if scope != slice.Wildcard {
+			if scope != trustpolicy.Wildcard {
 				if err := validateRegistryScopeFormat(scope); err != nil {
 					return err
 				}
@@ -374,7 +420,7 @@ func validateRegistryScopes(policyDoc *Document) error {
 	return nil
 }
 
-func hasOverlappingDNs(policyName string, parsedDNs []parsedDN) error {
+func validateOverlappingDNs(policyName string, parsedDNs []parsedDN) error {
 	for i, dn1 := range parsedDNs {
 		for j, dn2 := range parsedDNs {
 			if i != j && pkix.IsSubsetDN(dn1.ParsedMap, dn2.ParsedMap) {
@@ -397,51 +443,6 @@ func isValidTrustStoreType(s string) bool {
 	return false
 }
 
-// GetApplicableTrustPolicy returns a pointer to the deep copied TrustPolicy
-// statement that applies to the given registry URI. If no applicable trust
-// policy is found, returns an error
-// see https://github.com/notaryproject/notaryproject/blob/main/trust-store-trust-policy-specification.md#selecting-a-trust-policy-based-on-artifact-uri
-func GetApplicableTrustPolicy(trustPolicyDoc *Document, artifactReference string) (*TrustPolicy, error) {
-
-	artifactPath, err := getArtifactPathFromReference(artifactReference)
-	if err != nil {
-		return nil, err
-	}
-
-	var wildcardPolicy *TrustPolicy
-	var applicablePolicy *TrustPolicy
-	for _, policyStatement := range trustPolicyDoc.TrustPolicies {
-		if slice.ContainsString(slice.Wildcard, policyStatement.RegistryScopes) {
-			// we need to deep copy because we can't use the loop variable
-			// address. see https://stackoverflow.com/a/45967429
-			wildcardPolicy = (&policyStatement).clone()
-		} else if slice.ContainsString(artifactPath, policyStatement.RegistryScopes) {
-			applicablePolicy = (&policyStatement).clone()
-		}
-	}
-
-	if applicablePolicy != nil {
-		// a policy with exact match for registry URI takes precedence over
-		// a wildcard (*) policy.
-		return applicablePolicy, nil
-	} else if wildcardPolicy != nil {
-		return wildcardPolicy, nil
-	} else {
-		return nil, fmt.Errorf("artifact %q has no applicable trust policy", artifactReference)
-	}
-}
-
-// clone returns a pointer to the deeply copied TrustPolicy
-func (t *TrustPolicy) clone() *TrustPolicy {
-	return &TrustPolicy{
-		Name:                  t.Name,
-		SignatureVerification: t.SignatureVerification,
-		RegistryScopes:        append([]string(nil), t.RegistryScopes...),
-		TrustedIdentities:     append([]string(nil), t.TrustedIdentities...),
-		TrustStores:           append([]string(nil), t.TrustStores...),
-	}
-}
-
 func getArtifactPathFromReference(artifactReference string) (string, error) {
 	// TODO support more types of URI like "domain.com/repository",
 	// "domain.com/repository:tag"

verification/trustpolicy/trustpolicy_test.go

@@ -381,7 +381,7 @@ func TestGetVerificationLevel(t *testing.T) {
 	for i, tt := range tests {
 		t.Run(strconv.Itoa(i), func(t *testing.T) {
 
-			level, err := GetVerificationLevel(tt.verificationLevel)
+			level, err := tt.verificationLevel.GetVerificationLevel()
 
 			if tt.wantErr != (err != nil) {
 				t.Fatalf("TestFindVerificationLevel Error: %q WantErr: %v", err, tt.wantErr)
@@ -428,7 +428,7 @@ func TestCustomVerificationLevel(t *testing.T) {
 	}
 	for i, tt := range tests {
 		t.Run(strconv.Itoa(i), func(t *testing.T) {
-			level, err := GetVerificationLevel(tt.customVerification)
+			level, err := tt.customVerification.GetVerificationLevel()
 
 			if tt.wantErr != (err != nil) {
 				t.Fatalf("TestCustomVerificationLevel Error: %q WantErr: %v", err, tt.wantErr)
@@ -461,13 +461,13 @@ func TestApplicableTrustPolicy(t *testing.T) {
 		policyStatement,
 	}
 	// existing Registry Scope
-	policy, err := GetApplicableTrustPolicy(&policyDoc, registryUri)
+	policy, err := (&policyDoc).GetApplicableTrustPolicy(registryUri)
 	if policy.Name != policyStatement.Name || err != nil {
 		t.Fatalf("getApplicableTrustPolicy should return %q for registry scope %q", policyStatement.Name, registryScope)
 	}
 
 	// non-existing Registry Scope
-	policy, err = GetApplicableTrustPolicy(&policyDoc, "non.existing.scope/repo@sha256:hash")
+	policy, err = (&policyDoc).GetApplicableTrustPolicy("non.existing.scope/repo@sha256:hash")
 	if policy != nil || err == nil || err.Error() != "artifact \"non.existing.scope/repo@sha256:hash\" has no applicable trust policy" {
 		t.Fatalf("getApplicableTrustPolicy should return nil for non existing registry scope")
 	}
@@ -484,7 +484,7 @@ func TestApplicableTrustPolicy(t *testing.T) {
 		policyStatement,
 		wildcardStatement,
 	}
-	policy, err = GetApplicableTrustPolicy(&policyDoc, "some.registry.that/has.no.policy@sha256:hash")
+	policy, err = (&policyDoc).GetApplicableTrustPolicy("some.registry.that/has.no.policy@sha256:hash")
 	if policy.Name != wildcardStatement.Name || err != nil {
 		t.Fatalf("getApplicableTrustPolicy should return wildcard policy for registry scope \"some.registry.that/has.no.policy\"")
 	}

verification/truststore/truststore.go

@@ -9,6 +9,7 @@ import (
 	"io/fs"
 	"os"
 	"path/filepath"
+	"regexp"
 
 	corex509 "github.com/notaryproject/notation-core-go/x509"
 	"github.com/notaryproject/notation-go/dir"
@@ -36,20 +37,23 @@ type X509TrustStore interface {
 	GetCertificates(ctx context.Context, storeType Type, namedStore string) ([]*x509.Certificate, error)
 }
 
+// NewX509TrustStore generates a new X509TrustStore
+func NewX509TrustStore(trustStorefs dir.SysFS) X509TrustStore {
+	return &x509TrustStore{trustStorefs}
+}
+
 // x509TrustStore implements X509TrustStore
 type x509TrustStore struct {
 	trustStorefs dir.SysFS
 }
 
-// NewX509TrustStore generates a new X509TrustStore
-func NewX509TrustStore(trustStorefs dir.SysFS) X509TrustStore {
-	return x509TrustStore{trustStorefs}
-}
-
 // GetCertificates returns certificates under storeType/namedStore
-func (trustStore x509TrustStore) GetCertificates(ctx context.Context, storeType Type, namedStore string) ([]*x509.Certificate, error) {
-	if storeType == "" || namedStore == "" {
-		return nil, errors.New("storeType and namedStore cannot be empty")
+func (trustStore *x509TrustStore) GetCertificates(ctx context.Context, storeType Type, namedStore string) ([]*x509.Certificate, error) {
+	if !isValidStoreType(storeType) {
+		return nil, fmt.Errorf("unsupported store type: %s", storeType)
+	}
+	if !isValidNamedStore(namedStore) {
+		return nil, errors.New("named store name needs to follow [a-zA-Z0-9_.-]+ format")
 	}
 	path, err := trustStore.trustStorefs.SysPath(dir.X509TrustStoreDir(string(storeType), namedStore))
 	if err != nil {
@@ -123,3 +127,18 @@ func validateCerts(certs []*x509.Certificate, path string) error {
 
 	return nil
 }
+
+// isValidStoreType checks if storeType is supported
+func isValidStoreType(storeType Type) bool {
+	for _, t := range Types {
+		if storeType == t {
+			return true
+		}
+	}
+	return false
+}
+
+// isValidFileName checks if a file name is cross-platform compatible
+func isValidNamedStore(namedStore string) bool {
+	return regexp.MustCompile(`^[a-zA-Z0-9_.-]+$`).MatchString(namedStore)
+}