update: logs and error messages (#235)

This PR adds more logs to the library and improves error messages.

This PR intends to resolve following issues:
notaryproject/notation#462
notaryproject/notation#128 -> comments related
to trust policy

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

notation.go

@@ -98,6 +98,7 @@ func Sign(ctx context.Context, signer Signer, repo registry.Repository, opts Sig
 	logger.Debugf("Pushing signature of artifact descriptor: %+v, signature media type: %v", targetDesc, opts.SignatureMediaType)
 	_, _, err = repo.PushSignature(ctx, opts.SignatureMediaType, sig, targetDesc, annotations)
 	if err != nil {
+		logger.Error("Failed to push the signature")
 		return ocispec.Descriptor{}, err
 	}
 
@@ -159,7 +160,8 @@ type VerifyOptions struct {
 
 // Verifier is a generic interface for verifying an artifact.
 type Verifier interface {
-	// Verify verifies the signature blob and returns the outcome upon
+	// Verify verifies the signature blob `signature` against the target OCI
+	// artifact with manifest descriptor `desc`, and returns the outcome upon
 	// successful verification.
 	// If nil signature is present and the verification level is not 'skip',
 	// an error will be returned.
@@ -188,7 +190,7 @@ type skipVerifier interface {
 
 // Verify performs signature verification on each of the notation supported
 // verification types (like integrity, authenticity, etc.) and return the
-// successful signature verification outcomes.
+// successful signature verification outcome.
 // For more details on signature verification, see
 // https://github.com/notaryproject/notaryproject/blob/main/specs/trust-store-trust-policy.md#signature-verification
 func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, remoteOpts RemoteVerifyOptions) (ocispec.Descriptor, []*VerificationOutcome, error) {
@@ -263,8 +265,9 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, re
 			// verify each signature
 			outcome, err := verifier.Verify(ctx, artifactDescriptor, sigBlob, opts)
 			if err != nil {
+				logger.Infof("Signature %v failed verification with error: %v", sigManifestDesc.Digest, err)
 				if outcome == nil {
-					// TODO: log fatal error
+					logger.Error("Got nil outcome. Expecting non-nil outcome on verification failure")
 					return err
 				}
 				continue

signer/plugin.go

@@ -230,7 +230,7 @@ func areUnknownAttributesAdded(content []byte) []string {
 
 func getKeySet(inputMap map[string]interface{}) []string {
 	keySet := make([]string, 0, len(inputMap))
-	for k, _ := range inputMap {
+	for k := range inputMap {
 		keySet = append(keySet, k)
 	}
 	return keySet

verifier/trustpolicy/trustpolicy.go

@@ -150,10 +150,18 @@ type SignatureVerification struct {
 	Override          map[ValidationType]ValidationAction `json:"override,omitempty"`
 }
 
-// Validate validates a policy document according to it's version's rule set.
+// Validate validates a policy document according to its version's rule set.
 // if any rule is violated, returns an error
 func (policyDoc *Document) Validate() error {
+	// sanity check
+	if policyDoc == nil {
+		return errors.New("trust policy document cannot be nil")
+	}
+
 	// Validate Version
+	if policyDoc.Version == "" {
+		return errors.New("trust policy document is missing or has empty version, it must be specified")
+	}
 	if !slices.Contains(supportedPolicyVersions, policyDoc.Version) {
 		return fmt.Errorf("trust policy document uses unsupported version %q", policyDoc.Version)
 	}
@@ -173,10 +181,10 @@ func (policyDoc *Document) Validate() error {
 		}
 		policyStatementNameCount[statement.Name]++
 
-		// Verify signature verification level is valid
+		// Verify signature verification is valid
 		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)
+			return fmt.Errorf("trust policy statement %q has invalid signatureVerification. Error: %w", statement.Name, err)
 		}
 
 		// Any signature verification other than "skip" needs a trust store and
@@ -263,22 +271,26 @@ func LoadDocument() (*Document, error) {
 	policyDocument := &Document{}
 	err = json.NewDecoder(jsonFile).Decode(policyDocument)
 	if err != nil {
-		return nil, err
+		return nil, errors.New("malformed trustpolicy.json file")
 	}
 	return policyDocument, nil
 }
 
 // GetVerificationLevel returns VerificationLevel struct for the given
 // SignatureVerification struct throws error if SignatureVerification is invalid
 func (signatureVerification *SignatureVerification) GetVerificationLevel() (*VerificationLevel, error) {
+	if signatureVerification.VerificationLevel == "" {
+		return nil, errors.New("trust policy statement is missing or has empty signature verification level, it must be specified")
+	}
+
 	var baseLevel *VerificationLevel
 	for _, l := range VerificationLevels {
 		if l.Name == signatureVerification.VerificationLevel {
 			baseLevel = l
 		}
 	}
 	if baseLevel == nil {
-		return nil, fmt.Errorf("invalid signature verification %q", signatureVerification.VerificationLevel)
+		return nil, fmt.Errorf("invalid signature verification level %q", signatureVerification.VerificationLevel)
 	}
 
 	if len(signatureVerification.Override) == 0 {
@@ -287,7 +299,7 @@ func (signatureVerification *SignatureVerification) GetVerificationLevel() (*Ver
 	}
 
 	if baseLevel == LevelSkip {
-		return nil, fmt.Errorf("signature verification %q can't be used to customize signature verification", baseLevel.Name)
+		return nil, fmt.Errorf("signature verification level %q can't be used to customize signature verification", baseLevel.Name)
 	}
 
 	customVerificationLevel := &VerificationLevel{
@@ -353,13 +365,13 @@ func validateTrustStore(statement TrustPolicy) error {
 	for _, trustStore := range statement.TrustStores {
 		storeType, namedStore, found := strings.Cut(trustStore, ":")
 		if !found {
-			return fmt.Errorf("trust policy statement %q is missing separator in trust store value %q", statement.Name, trustStore)
+			return fmt.Errorf("trust policy statement %q has malformed trust store value %q. Format <TrustStoreType>:<TrustStoreName> is required", statement.Name, trustStore)
 		}
 		if !isValidTrustStoreType(storeType) {
 			return fmt.Errorf("trust policy statement %q uses an unsupported trust store type %q in trust store value %q", statement.Name, storeType, trustStore)
 		}
 		if !file.IsValidFileName(namedStore) {
-			return errors.New("named store name needs to follow [a-zA-Z0-9_.-]+ format")
+			return fmt.Errorf("trust policy statement %q uses an unsupported trust store name %q in trust store value %q. Named store name needs to follow [a-zA-Z0-9_.-]+ format", statement.Name, namedStore, trustStore)
 		}
 	}
 

verifier/trustpolicy/trustpolicy_test.go

@@ -220,11 +220,17 @@ func TestValidRegistryScopes(t *testing.T) {
 // TestValidatePolicyDocument calls policyDoc.Validate()
 // and tests various validations on policy eliments
 func TestValidateInvalidPolicyDocument(t *testing.T) {
+	// Sanity check
+	var nilPolicyDoc *Document
+	err := nilPolicyDoc.Validate()
+	if err == nil || err.Error() != "trust policy document cannot be nil" {
+		t.Fatalf("nil policyDoc should return error")
+	}
 
 	// Invalid Version
 	policyDoc := dummyPolicyDocument()
 	policyDoc.Version = "invalid"
-	err := policyDoc.Validate()
+	err = policyDoc.Validate()
 	if err == nil || err.Error() != "trust policy document uses unsupported version \"invalid\"" {
 		t.Fatalf("invalid version should return error")
 	}
@@ -284,7 +290,7 @@ func TestValidateInvalidPolicyDocument(t *testing.T) {
 	policyStatement.SignatureVerification = SignatureVerification{VerificationLevel: "invalid"}
 	policyDoc.TrustPolicies = []TrustPolicy{policyStatement}
 	err = policyDoc.Validate()
-	if err == nil || err.Error() != "trust policy statement \"test-statement-name\" uses invalid signatureVerification value \"invalid\"" {
+	if err == nil || err.Error() != "trust policy statement \"test-statement-name\" has invalid signatureVerification. Error: invalid signature verification level \"invalid\"" {
 		t.Fatalf("policy statement with invalid SignatureVerification should return error")
 	}
 
@@ -365,7 +371,7 @@ func TestValidateInvalidPolicyDocument(t *testing.T) {
 	policyStatement.TrustStores = []string{"ca"}
 	policyDoc.TrustPolicies = []TrustPolicy{policyStatement}
 	err = policyDoc.Validate()
-	if err == nil || err.Error() != "trust policy statement \"test-statement-name\" is missing separator in trust store value \"ca\"" {
+	if err == nil || err.Error() != "trust policy statement \"test-statement-name\" has malformed trust store value \"ca\". Format <TrustStoreType>:<TrustStoreName> is required" {
 		t.Fatalf("policy statement with trust store missing separator should return error")
 	}
 
@@ -385,7 +391,7 @@ func TestValidateInvalidPolicyDocument(t *testing.T) {
 	policyStatement.TrustStores = []string{"ca:"}
 	policyDoc.TrustPolicies = []TrustPolicy{policyStatement}
 	err = policyDoc.Validate()
-	if err == nil || err.Error() != "named store name needs to follow [a-zA-Z0-9_.-]+ format" {
+	if err == nil || err.Error() != "trust policy statement \"test-statement-name\" uses an unsupported trust store name \"\" in trust store value \"ca:\". Named store name needs to follow [a-zA-Z0-9_.-]+ format" {
 		t.Fatalf("policy statement with trust store missing named store should return error")
 	}
 

verifier/verifier.go

@@ -27,7 +27,7 @@ import (
 	"oras.land/oras-go/v2/content"
 )
 
-// verifier implements notation.Verifier
+// verifier implements notation.Verifier and notation.skipVerifier
 type verifier struct {
 	trustPolicyDoc *trustpolicy.Document
 	trustStore     truststore.X509TrustStore
@@ -83,8 +83,11 @@ func (v *verifier) SkipVerify(ctx context.Context, artifactRef string) (bool, *t
 	return false, verificationLevel, nil
 }
 
-// Verify verifies the signature blob and returns the verified descriptor
-// upon successful verification.
+// Verify verifies the signature blob `signature` against the target OCI
+// artifact with manifest descriptor `desc`, and returns the outcome upon
+// successful verification.
+// If nil signature is present and the verification level is not 'skip',
+// an error will be returned.
 func (v *verifier) Verify(ctx context.Context, desc ocispec.Descriptor, signature []byte, opts notation.VerifyOptions) (*notation.VerificationOutcome, error) {
 	artifactRef := opts.ArtifactReference
 	envelopeMediaType := opts.SignatureMediaType
@@ -119,11 +122,14 @@ func (v *verifier) Verify(ctx context.Context, desc ocispec.Descriptor, signatur
 	payload := &envelope.Payload{}
 	err = json.Unmarshal(outcome.EnvelopeContent.Payload.Content, payload)
 	if err != nil {
+		logger.Error("Failed to unmarshal the payload content in the signature blob to envelope.Payload")
 		outcome.Error = err
 		return outcome, err
 	}
 
 	if !content.Equal(payload.TargetArtifact, desc) {
+		logger.Infof("payload.TargetArtifact in signature: %+v", payload.TargetArtifact)
+		logger.Infof("Target artifact that want to be verified: %+v", desc)
 		outcome.Error = errors.New("content descriptor mismatch")
 	}
 	return outcome, outcome.Error