updated per code review

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

Author: Two-Hearts

internal/slice/slice.go renamed to internal/slices/slices.go

@@ -1,4 +1,4 @@
-package slice
+package slices
 
 // Contains reports whether v is present in s.
 func Contains[E comparable](s []E, v E) bool {

notation.go

@@ -20,8 +20,6 @@ import (
 
 const annotationX509ChainThumbprint = "io.cncf.notary.x509chain.thumbprint#S256"
 
-const maxVerificationLimitDefault = 50
-
 var errDoneVerification = errors.New("done verification")
 
 // SignOptions contains parameters for Signer.Sign.
@@ -90,7 +88,7 @@ type VerifyOptions struct {
 
 	// MaxSignatureAttempts is the maximum number of signature envelopes that
 	// can be associated with the target artifact. If set to less than or equals
-	// to zero, value defaults to 50.
+	// to zero, an error will be returned.
 	// Note: this option is scoped to notation.Verify(). verifier.Verify() is
 	// for signle signature verification, and therefore, does not use it.
 	MaxSignatureAttempts int
@@ -167,8 +165,7 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, op
 
 	var verificationOutcomes []*VerificationOutcome
 	if opts.MaxSignatureAttempts <= 0 {
-		// Set MaxVerificationLimit to 50 as default
-		opts.MaxSignatureAttempts = maxVerificationLimitDefault
+		return ocispec.Descriptor{}, verificationOutcomes, ErrorSignatureRetrievalFailed{Msg: fmt.Sprintf("verifyOptions.MaxSignatureAttempts expects a positive number, got %d", opts.MaxSignatureAttempts)}
 	}
 	errExceededMaxVerificationLimit := ErrorVerificationFailed{Msg: fmt.Sprintf("total number of signatures associated with an artifact should be less than: %d", opts.MaxSignatureAttempts)}
 	numOfSignatureProcessed := 0

notation_test.go

@@ -22,7 +22,7 @@ func TestRegistryResolveError(t *testing.T) {
 
 	// mock the repository
 	repo.ResolveError = errors.New(errorMessage)
-	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri}
+	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri, MaxSignatureAttempts: 50}
 	_, _, err := Verify(context.Background(), &verifier, repo, opts)
 
 	if err == nil || !errors.Is(err, expectedErr) {
@@ -35,7 +35,7 @@ func TestSkippedSignatureVerification(t *testing.T) {
 	repo := mock.NewRepository()
 	verifier := dummyVerifier{&policyDocument, mock.PluginManager{}, false, *trustpolicy.LevelSkip}
 
-	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri}
+	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri, MaxSignatureAttempts: 50}
 	_, outcomes, err := Verify(context.Background(), &verifier, repo, opts)
 
 	if err != nil || outcomes[0].VerificationLevel.Name != trustpolicy.LevelSkip.Name {
@@ -52,7 +52,7 @@ func TestRegistryNoSignatureManifests(t *testing.T) {
 
 	// mock the repository
 	repo.ListSignaturesResponse = []ocispec.Descriptor{}
-	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri}
+	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri, MaxSignatureAttempts: 50}
 	_, _, err := Verify(context.Background(), &verifier, repo, opts)
 
 	if err == nil || !errors.Is(err, expectedErr) {
@@ -69,7 +69,7 @@ func TestRegistryFetchSignatureBlobError(t *testing.T) {
 
 	// mock the repository
 	repo.FetchSignatureBlobError = errors.New("network error")
-	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri}
+	opts := VerifyOptions{ArtifactReference: mock.SampleArtifactUri, MaxSignatureAttempts: 50}
 	_, _, err := Verify(context.Background(), &verifier, repo, opts)
 
 	if err == nil || !errors.Is(err, expectedErr) {

plugin/proto/proto.go

@@ -63,14 +63,3 @@ const (
 	// capability for a plugin to support verifying revocation checks.
 	CapabilityRevocationCheckVerifier Capability = "SIGNATURE_VERIFIER.REVOCATION_CHECK"
 )
-
-// In returns true if the Capability is present in the given array of
-// capabilities.
-func (c Capability) In(capabilities []Capability) bool {
-	for _, capability := range capabilities {
-		if c == capability {
-			return true
-		}
-	}
-	return false
-}

verifier/helpers.go

@@ -11,9 +11,9 @@ import (
 	"github.com/notaryproject/notation-core-go/signature"
 	"github.com/notaryproject/notation-go"
 	"github.com/notaryproject/notation-go/dir"
-	"github.com/notaryproject/notation-go/internal/pkix"
-	"github.com/notaryproject/notation-go/internal/slice"
-	trustpolicyInternal "github.com/notaryproject/notation-go/internal/trustpolicy"
+	"github.com/notaryproject/notation-go/internal/slices"
+	"github.com/notaryproject/notation-go/plugin"
+	"github.com/notaryproject/notation-go/plugin/proto"
 	"github.com/notaryproject/notation-go/verifier/trustpolicy"
 	"github.com/notaryproject/notation-go/verifier/truststore"
 )
@@ -82,51 +82,11 @@ func isCriticalFailure(result *notation.ValidationResult) bool {
 	return result.Action == trustpolicy.ActionEnforce && result.Error != nil
 }
 
-func verifyX509TrustedIdentitiesCore(certs []*x509.Certificate, trustPolicy *trustpolicy.TrustPolicy) error {
-	if slice.Contains(trustPolicy.TrustedIdentities, trustpolicyInternal.Wildcard) {
-		return nil
-	}
-
-	var trustedX509Identities []map[string]string
-	for _, identity := range trustPolicy.TrustedIdentities {
-		i := strings.Index(identity, ":")
-
-		identityPrefix := identity[:i]
-		identityValue := identity[i+1:]
-
-		if identityPrefix == trustpolicyInternal.X509Subject {
-			parsedSubject, err := pkix.ParseDistinguishedName(identityValue)
-			if err != nil {
-				return err
-			}
-			trustedX509Identities = append(trustedX509Identities, parsedSubject)
-		}
-	}
-
-	if len(trustedX509Identities) == 0 {
-		return fmt.Errorf("no x509 trusted identities are configured in the trust policy %q", trustPolicy.Name)
-	}
-
-	leafCert := certs[0] // trusted identities only supported on the leaf cert
-
-	leafCertDN, err := pkix.ParseDistinguishedName(leafCert.Subject.String()) // parse the certificate subject following rfc 4514 DN syntax
-	if err != nil {
-		return fmt.Errorf("error while parsing the certificate subject from the digital signature. error : %q", err)
-	}
-	for _, trustedX509Identity := range trustedX509Identities {
-		if pkix.IsSubsetDN(trustedX509Identity, leafCertDN) {
-			return nil
-		}
-	}
-
-	return fmt.Errorf("signing certificate from the digital signature does not match the X.509 trusted identities %q defined in the trust policy %q", trustedX509Identities, trustPolicy.Name)
-}
-
 func getNonPluginExtendedCriticalAttributes(signerInfo *signature.SignerInfo) []signature.Attribute {
 	var criticalExtendedAttrs []signature.Attribute
 	for _, attr := range signerInfo.SignedAttributes.ExtendedAttributes {
 		attrStrKey, ok := attr.Key.(string)
-		if ok && !slice.Contains(VerificationPluginHeaders, attrStrKey) { // filter the plugin extended attributes
+		if ok && !slices.Contains(VerificationPluginHeaders, attrStrKey) { // filter the plugin extended attributes
 			// TODO support other attribute types (COSE attribute keys can be numbers)
 			criticalExtendedAttrs = append(criticalExtendedAttrs, attr)
 		}
@@ -181,3 +141,22 @@ func getVerificationPluginMinVersion(signerInfo *signature.SignerInfo) (string,
 	}
 	return version, nil
 }
+
+// getPluginMetadata gets metadata of the plugin
+func getPluginMetadata(ctx context.Context, installedPlugin plugin.Plugin, pluginConfig map[string]string) (*proto.GetMetadataResponse, error) {
+	req := &proto.GetMetadataRequest{
+		PluginConfig: pluginConfig,
+	}
+	metadata, err := installedPlugin.GetMetadata(ctx, req)
+	if err != nil {
+		return nil, err
+	}
+	if !metadata.SupportsContract(proto.ContractVersion) {
+		return nil, fmt.Errorf(
+			"contract version %q is not in the list of the plugin supported versions %v",
+			proto.ContractVersion, metadata.SupportedContractVersions,
+		)
+	}
+
+	return metadata, nil
+}

verifier/trustpolicy/trustpolicy.go

@@ -11,7 +11,7 @@ import (
 
 	"github.com/notaryproject/notation-go/dir"
 	"github.com/notaryproject/notation-go/internal/pkix"
-	"github.com/notaryproject/notation-go/internal/slice"
+	"github.com/notaryproject/notation-go/internal/slices"
 	"github.com/notaryproject/notation-go/internal/trustpolicy"
 	"github.com/notaryproject/notation-go/verifier/truststore"
 )
@@ -153,7 +153,7 @@ type SignatureVerification struct {
 // if any rule is violated, returns an error
 func (policyDoc *Document) Validate() error {
 	// Validate Version
-	if !slice.Contains(supportedPolicyVersions, policyDoc.Version) {
+	if !slices.Contains(supportedPolicyVersions, policyDoc.Version) {
 		return fmt.Errorf("trust policy document uses unsupported version %q", policyDoc.Version)
 	}
 
@@ -232,11 +232,11 @@ func (trustPolicyDoc *Document) GetApplicableTrustPolicy(artifactReference strin
 	var wildcardPolicy *TrustPolicy
 	var applicablePolicy *TrustPolicy
 	for _, policyStatement := range trustPolicyDoc.TrustPolicies {
-		if slice.Contains(policyStatement.RegistryScopes, trustpolicy.Wildcard) {
+		if slices.Contains(policyStatement.RegistryScopes, trustpolicy.Wildcard) {
 			// 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.Contains(policyStatement.RegistryScopes, artifactPath) {
+		} else if slices.Contains(policyStatement.RegistryScopes, artifactPath) {
 			applicablePolicy = (&policyStatement).clone()
 		}
 	}
@@ -365,7 +365,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.Contains(statement.TrustedIdentities, trustpolicy.Wildcard) {
+	if len(statement.TrustedIdentities) > 1 && slices.Contains(statement.TrustedIdentities, trustpolicy.Wildcard) {
 		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)
 	}
 
@@ -415,7 +415,7 @@ 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.Contains(statement.RegistryScopes, trustpolicy.Wildcard) {
+		if len(statement.RegistryScopes) > 1 && slices.Contains(statement.RegistryScopes, trustpolicy.Wildcard) {
 			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 {

verifier/trustpolicy/trustpolicy_test.go

@@ -495,7 +495,7 @@ func TestApplicableTrustPolicy(t *testing.T) {
 	}
 }
 
-func TestLoadPolicyDocument(t *testing.T) {
+func TestLoadDocument(t *testing.T) {
 	// non-existing policy file
 	tempRoot := t.TempDir()
 	dir.UserConfigDir = tempRoot

verifier/truststore/truststore.go

@@ -13,7 +13,7 @@ import (
 
 	corex509 "github.com/notaryproject/notation-core-go/x509"
 	"github.com/notaryproject/notation-go/dir"
-	"github.com/notaryproject/notation-go/internal/slice"
+	"github.com/notaryproject/notation-go/internal/slices"
 )
 
 // Type is an enum for trust store types supported such as
@@ -128,7 +128,7 @@ func validateCerts(certs []*x509.Certificate, path string) error {
 
 // isValidStoreType checks if storeType is supported
 func isValidStoreType(storeType Type) bool {
-	return slice.Contains(Types, storeType)
+	return slices.Contains(Types, storeType)
 }
 
 // isValidFileName checks if a file name is cross-platform compatible