Skip to content

Commit

Permalink
chore: Refactor error messages for Notation signature verification (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
binbin-li authored Sep 9, 2024
1 parent d89400e commit ab8d001
Show file tree
Hide file tree
Showing 14 changed files with 165 additions and 62 deletions.
4 changes: 2 additions & 2 deletions cmd/ratify/cmd/cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,8 @@ func TestVerify(t *testing.T) {

// TODO: make ratify cli more unit testable
// unit test should not have dependency for real image
if !strings.Contains(err.Error(), "plugin not found") {
t.Errorf("error expected")
if !strings.Contains(err.Error(), "PLUGIN_NOT_FOUND") {
t.Fatalf("expected containing: %s, but got: %s", "PLUGIN_NOT_FOUND", err.Error())
}
}

Expand Down
8 changes: 5 additions & 3 deletions pkg/controllers/clusterresource/verifier_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package clusterresource

import (
"context"
"fmt"

configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
re "github.com/ratify-project/ratify/errors"
Expand Down Expand Up @@ -82,12 +83,13 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
return ctrl.Result{}, nil
}

// creates a verifier reference from CRD spec and add store to map
// creates a verifier reference from CR spec and add verifier to map
func verifierAddOrReplace(spec configv1beta1.VerifierSpec, objectName string) error {
verifierConfig, err := cutils.SpecToVerifierConfig(spec.Parameters.Raw, objectName, spec.Name, spec.ArtifactTypes, spec.Source)
if err != nil {
logrus.Error(err)
return err
errMsg := fmt.Sprintf("Unable to apply cluster-wide resource %s of Verifier kind", objectName)
logrus.Error(err, errMsg)
return re.ErrorCodeConfigInvalid.WithDetail(errMsg).WithError(err)
}

return cutils.UpsertVerifier(spec.Version, spec.Address, constants.EmptyNamespace, objectName, verifierConfig)
Expand Down
7 changes: 3 additions & 4 deletions pkg/controllers/clusterresource/verifier_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"context"
"errors"
"os"
"strings"
"testing"

configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
Expand Down Expand Up @@ -124,12 +123,12 @@ func TestVerifierAdd_WithParameters(t *testing.T) {

func TestVerifierAddOrReplace_PluginNotFound(t *testing.T) {
resetVerifierMap()
var resource = "invalidplugin"
expectedMsg := "plugin not found"
resource := "invalidplugin"
expectedMsg := "PLUGIN_NOT_FOUND: Verifier plugin pluginnotfound not found: failed to find plugin \"pluginnotfound\" in paths [test/path]: Please ensure that the correct type is specified for the built-in Verifier configuration or the custom Verifier plugin is configured."
var testVerifierSpec = getInvalidVerifierSpec()
err := verifierAddOrReplace(testVerifierSpec, resource)

if !strings.Contains(err.Error(), expectedMsg) {
if err.Error() != expectedMsg {
t.Fatalf("TestVerifierAddOrReplace_PluginNotFound expected msg: '%v', actual %v", expectedMsg, err.Error())
}
}
Expand Down
6 changes: 4 additions & 2 deletions pkg/controllers/namespaceresource/verifier_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package namespaceresource

import (
"context"
"fmt"

configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
"github.com/ratify-project/ratify/internal/constants"
Expand Down Expand Up @@ -85,8 +86,9 @@ func (r *VerifierReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
func verifierAddOrReplace(spec configv1beta1.NamespacedVerifierSpec, objectName string, namespace string) error {
verifierConfig, err := cutils.SpecToVerifierConfig(spec.Parameters.Raw, objectName, spec.Name, spec.ArtifactTypes, spec.Source)
if err != nil {
logrus.Error(err)
return err
errMsg := fmt.Sprintf("Unable to apply the resource %s of NamespacedVerifier kind in the namespace %s", objectName, namespace)
logrus.Error(err, errMsg)
return re.ErrorCodeConfigInvalid.WithDetail(errMsg).WithError(err)
}

return cutils.UpsertVerifier(spec.Version, spec.Address, namespace, objectName, verifierConfig)
Expand Down
7 changes: 3 additions & 4 deletions pkg/controllers/namespaceresource/verifier_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"context"
"errors"
"os"
"strings"
"testing"

configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
Expand Down Expand Up @@ -142,12 +141,12 @@ func TestVerifierAdd_WithParameters(t *testing.T) {

func TestVerifierAddOrReplace_PluginNotFound(t *testing.T) {
resetVerifierMap()
var resource = "invalidplugin"
expectedMsg := "plugin not found"
resource := "invalidplugin"
expectedMsg := "PLUGIN_NOT_FOUND: Verifier plugin pluginnotfound not found: failed to find plugin \"pluginnotfound\" in paths [test/path]: Please ensure that the correct type is specified for the built-in Verifier configuration or the custom Verifier plugin is configured."
var testVerifierSpec = getInvalidVerifierSpec()
err := verifierAddOrReplace(testVerifierSpec, resource, testNamespace)

if !strings.Contains(err.Error(), expectedMsg) {
if err.Error() != expectedMsg {
t.Fatalf("TestVerifierAddOrReplace_PluginNotFound expected msg: '%v', actual %v", expectedMsg, err.Error())
}
}
Expand Down
7 changes: 5 additions & 2 deletions pkg/controllers/utils/verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ package utils

import (
"encoding/json"
"fmt"

configv1beta1 "github.com/ratify-project/ratify/api/v1beta1"
re "github.com/ratify-project/ratify/errors"
vc "github.com/ratify-project/ratify/pkg/verifier/config"
vf "github.com/ratify-project/ratify/pkg/verifier/factory"
"github.com/ratify-project/ratify/pkg/verifier/types"
Expand Down Expand Up @@ -56,8 +58,9 @@ func SpecToVerifierConfig(raw []byte, verifierName, verifierType, artifactTypes

if string(raw) != "" {
if err := json.Unmarshal(raw, &verifierConfig); err != nil {
logrus.Error(err, "unable to decode verifier parameters", "Parameters.Raw", raw)
return vc.VerifierConfig{}, err
errMsg := fmt.Sprintf("Unable to recognize the parameters of the Verifier resource %s", string(raw))
logrus.Error(err, errMsg)
return vc.VerifierConfig{}, re.ErrorCodeConfigInvalid.WithError(err).WithDetail(errMsg).WithRemediation("Please update the Verifier parameters and try again. Refer to the Verifier configuration guide: https://ratify.dev/docs/reference/custom%20resources/verifiers")
}
}
verifierConfig[types.Name] = verifierName
Expand Down
10 changes: 5 additions & 5 deletions pkg/executor/core/executor.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ type Executor struct {
// VerifySubject verifies the subject and returns results.
func (executor Executor) VerifySubject(ctx context.Context, verifyParameters e.VerifyParameters) (types.VerifyResult, error) {
if executor.PolicyEnforcer == nil {
return types.VerifyResult{}, errors.ErrorCodePolicyProviderNotFound.WithComponentType(errors.Executor)
return types.VerifyResult{}, errors.ErrorCodePolicyProviderNotFound.WithDetail("Policy configuration not found")

Check warning on line 64 in pkg/executor/core/executor.go

View check run for this annotation

Codecov / codecov/patch

pkg/executor/core/executor.go#L64

Added line #L64 was not covered by tests
}
result, err := executor.verifySubjectInternal(ctx, verifyParameters)
if err != nil {
Expand All @@ -83,7 +83,7 @@ func (executor Executor) verifySubjectInternal(ctx context.Context, verifyParame
}
if executor.PolicyEnforcer.GetPolicyType(ctx) == pt.ConfigPolicy {
if len(verifierReports) == 0 {
return types.VerifyResult{}, errors.ErrorCodeNoVerifierReport.WithComponentType(errors.Executor).WithDescription()
return types.VerifyResult{}, errors.ErrorCodeNoVerifierReport.WithDetail(fmt.Sprintf("No verification results for the artifact %s. Ensure verifiers are properly configured and that artifact metadata is attached", verifyParameters.Subject))
}
}
// If it requires embedded Rego Policy Engine make the decision, execute
Expand Down Expand Up @@ -176,7 +176,7 @@ func (executor Executor) verifyReferenceForJSONPolicy(ctx context.Context, subje
verifierStartTime := time.Now()
verifyResult, err := verifier.Verify(ctx, subjectRef, referenceDesc, referrerStore)
if err != nil {
verifierErr := errors.ErrorCodeVerifyReferenceFailure.NewError(errors.Verifier, verifier.Name(), errors.EmptyLink, err, nil, errors.HideStackTrace)
verifierErr := errors.ErrorCodeVerifyReferenceFailure.WithError(err)
verifyResult = vr.NewVerifierResult("", verifier.Name(), verifier.Type(), "", false, &verifierErr, nil)
}

Expand Down Expand Up @@ -224,7 +224,7 @@ func (executor Executor) verifyReferenceForRegoPolicy(ctx context.Context, subje
verifierStartTime := time.Now()
verifierResult, err := verifier.Verify(errCtx, subjectRef, referenceDesc, referrerStore)
if err != nil {
verifierErr := errors.ErrorCodeVerifyReferenceFailure.NewError(errors.Verifier, verifier.Name(), errors.EmptyLink, err, nil, errors.HideStackTrace)
verifierErr := errors.ErrorCodeVerifyReferenceFailure.WithError(err)
verifierReport = vt.CreateVerifierResult(verifier.Name(), verifier.Type(), "", false, &verifierErr)
} else {
verifierReport = vt.NewVerifierResult(verifierResult)
Expand Down Expand Up @@ -288,7 +288,7 @@ func (executor Executor) addNestedReports(ctx context.Context, referenceDes ocis
for _, report := range reports.VerifierReports {
nestedReport, err := types.NewNestedVerifierReport(report)
if err != nil {
return errors.ErrorCodeExecutorFailure.WithError(err).WithComponentType(errors.Executor)
return errors.ErrorCodeExecutorFailure.WithError(err)

Check warning on line 291 in pkg/executor/core/executor.go

View check run for this annotation

Codecov / codecov/patch

pkg/executor/core/executor.go#L291

Added line #L291 was not covered by tests
}
nestedReports = append(nestedReports, nestedReport)
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/keymanagementprovider/keymanagementprovider.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,15 @@ func setCertificatesInMap(resource string, certs map[KMPMapKey][]*x509.Certifica
// GetCertificatesFromMap gets the certificates from the map and returns an empty map of certificate arrays if not found or an error happened.
func GetCertificatesFromMap(ctx context.Context, resource string) (map[KMPMapKey][]*x509.Certificate, error) {
if !hasAccessToProvider(ctx, resource) {
return map[KMPMapKey][]*x509.Certificate{}, errors.ErrorCodeForbidden.WithDetail(fmt.Sprintf("namespace: [%s] does not have access to key management provider: %s", ctxUtils.GetNamespace(ctx), resource)).WithRemediation(fmt.Sprintf("Ensure the key management provider: %s is created under namespace: [%s] or as a cluster-wide resource.", resource, ctxUtils.GetNamespace(ctx)))
return map[KMPMapKey][]*x509.Certificate{}, errors.ErrorCodeForbidden.WithDetail(fmt.Sprintf("Resources in namespace [%s] do not have access to key management provider [%s]", ctxUtils.GetNamespace(ctx), resource)).WithRemediation(fmt.Sprintf("Make sure the key management provider: %s is created in the namespace: [%s] or as a cluster-wide resource.", resource, ctxUtils.GetNamespace(ctx)))

Check warning on line 139 in pkg/keymanagementprovider/keymanagementprovider.go

View check run for this annotation

Codecov / codecov/patch

pkg/keymanagementprovider/keymanagementprovider.go#L139

Added line #L139 was not covered by tests
}
if err, ok := certificateErrMap.Load(resource); ok && err != nil {
return map[KMPMapKey][]*x509.Certificate{}, err.(error)
}
if certs, ok := certificatesMap.Load(resource); ok {
return certs.(map[KMPMapKey][]*x509.Certificate), nil
}
return map[KMPMapKey][]*x509.Certificate{}, errors.ErrorCodeNotFound.WithDetail(fmt.Sprintf("failed to access non-existent key management provider: %s", resource))
return map[KMPMapKey][]*x509.Certificate{}, errors.ErrorCodeNotFound.WithDetail(fmt.Sprintf("The key management provider [%s] does not exist", resource)).WithRemediation(fmt.Sprintf("Make sure the key management provider: %s is created in the namespace: [%s] or as a cluster-wide resource.", resource, ctxUtils.GetNamespace(ctx)))
}

// DeleteResourceFromMap deletes the certificates, keys and errors from the map
Expand Down Expand Up @@ -186,15 +186,15 @@ func GetKeysFromMap(ctx context.Context, resource string) (map[KMPMapKey]PublicK
// A cluster-wide operation can access cluster-wide provider
// A namespaced operation can only fetch the provider in the same namespace or cluster-wide provider.
if !hasAccessToProvider(ctx, resource) {
return map[KMPMapKey]PublicKey{}, fmt.Errorf("namespace: [%s] does not have access to key management provider: %s", ctxUtils.GetNamespace(ctx), resource)
return map[KMPMapKey]PublicKey{}, errors.ErrorCodeForbidden.WithDetail(fmt.Sprintf("The resources in namespace [%s] do not have access to key management provider [%s]", ctxUtils.GetNamespace(ctx), resource)).WithRemediation(fmt.Sprintf("Make sure the key management provider [%s] is created in the namespace [%s] or as a cluster-wide resource.", resource, ctxUtils.GetNamespace(ctx)))
}
if err, ok := keyErrMap.Load(resource); ok && err != nil {
return map[KMPMapKey]PublicKey{}, err.(error)
}
if keys, ok := keyMap.Load(resource); ok {
return keys.(map[KMPMapKey]PublicKey), nil
}
return map[KMPMapKey]PublicKey{}, fmt.Errorf("failed to access non-existent key management provider: %s", resource)
return map[KMPMapKey]PublicKey{}, errors.ErrorCodeNotFound.WithDetail(fmt.Sprintf("The key management provider [%s] does not exist", resource)).WithRemediation(fmt.Sprintf("Make sure the key management provider: %s is created in the namespace: [%s] or as a cluster-wide resource.", resource, ctxUtils.GetNamespace(ctx)))
}

// SetCertificateError sets the error while fetching certificates from key management provider.
Expand Down
16 changes: 8 additions & 8 deletions pkg/referrerstore/oras/oras.go
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ func (store *orasStore) GetBlobContent(ctx context.Context, subjectReference com
func (store *orasStore) GetReferenceManifest(ctx context.Context, subjectReference common.Reference, referenceDesc ocispecs.ReferenceDescriptor) (ocispecs.ReferenceManifest, error) {
repository, err := store.createRepository(ctx, store, subjectReference)
if err != nil {
return ocispecs.ReferenceManifest{}, re.ErrorCodeCreateRepositoryFailure.NewError(re.ReferrerStore, storeName, re.EmptyLink, err, nil, re.HideStackTrace)
return ocispecs.ReferenceManifest{}, re.ErrorCodeRepositoryOperationFailure.WithDetail("Failed to connect to the remote registry").WithError(err)
}
var manifestBytes []byte
// check if manifest exists in local ORAS cache
Expand All @@ -336,12 +336,12 @@ func (store *orasStore) GetReferenceManifest(ctx context.Context, subjectReferen
manifestReader, err := repository.Fetch(ctx, referenceDesc.Descriptor)
if err != nil {
evictOnError(ctx, err, subjectReference.Original)
return ocispecs.ReferenceManifest{}, re.ErrorCodeRepositoryOperationFailure.NewError(re.ReferrerStore, storeName, re.EmptyLink, err, nil, re.HideStackTrace)
return ocispecs.ReferenceManifest{}, re.ErrorCodeRepositoryOperationFailure.WithDetail("Failed to fetch the artifact metadata from the registry").WithError(err)
}

manifestBytes, err = io.ReadAll(manifestReader)
if err != nil {
return ocispecs.ReferenceManifest{}, re.ErrorCodeManifestInvalid.WithError(err).WithPluginName(storeName).WithComponentType(re.ReferrerStore)
return ocispecs.ReferenceManifest{}, re.ErrorCodeManifestInvalid.WithDetail("Failed to parse the artifact metadata").WithError(err)
}

// push fetched manifest to local ORAS cache
Expand All @@ -360,15 +360,15 @@ func (store *orasStore) GetReferenceManifest(ctx context.Context, subjectReferen
if referenceDesc.Descriptor.MediaType == oci.MediaTypeImageManifest {
var imageManifest oci.Manifest
if err := json.Unmarshal(manifestBytes, &imageManifest); err != nil {
return ocispecs.ReferenceManifest{}, re.ErrorCodeDataDecodingFailure.WithError(err).WithComponentType(re.ReferrerStore)
return ocispecs.ReferenceManifest{}, re.ErrorCodeDataDecodingFailure.WithDetail("Failed to parse artifact metadata of mediatype `application/vnd.oci.image.manifest.v1+json`").WithError(err).WithRemediation("Please check if the artifact metadata was created correctly.")
}
referenceManifest = commonutils.OciManifestToReferenceManifest(imageManifest)
} else if referenceDesc.Descriptor.MediaType == ocispecs.MediaTypeArtifactManifest {
if err := json.Unmarshal(manifestBytes, &referenceManifest); err != nil {
return ocispecs.ReferenceManifest{}, re.ErrorCodeDataDecodingFailure.WithError(err).WithComponentType(re.ReferrerStore)
return ocispecs.ReferenceManifest{}, re.ErrorCodeDataDecodingFailure.WithDetail("Failed to parse artifact metadata of mediatype `application/vnd.oci.artifact.manifest.v1+json`").WithError(err).WithRemediation("Please check if the artifact metadata was created correctly.")
}
} else {
return ocispecs.ReferenceManifest{}, fmt.Errorf("unsupported manifest media type: %s", referenceDesc.Descriptor.MediaType)
return ocispecs.ReferenceManifest{}, re.ErrorCodeGetReferenceManifestFailure.WithDetail(fmt.Sprintf("Unsupported artifact metadata of media type %s", referenceDesc.Descriptor.MediaType)).WithRemediation("Please check if the artifact metadata was created correctly.")
}

return referenceManifest, nil
Expand All @@ -377,13 +377,13 @@ func (store *orasStore) GetReferenceManifest(ctx context.Context, subjectReferen
func (store *orasStore) GetSubjectDescriptor(ctx context.Context, subjectReference common.Reference) (*ocispecs.SubjectDescriptor, error) {
repository, err := store.createRepository(ctx, store, subjectReference)
if err != nil {
return nil, re.ErrorCodeCreateRepositoryFailure.WithError(err).WithComponentType(re.ReferrerStore).WithPluginName(storeName)
return nil, re.ErrorCodeRepositoryOperationFailure.WithDetail("Failed to connect to remote registry").WithError(err)

Check warning on line 380 in pkg/referrerstore/oras/oras.go

View check run for this annotation

Codecov / codecov/patch

pkg/referrerstore/oras/oras.go#L380

Added line #L380 was not covered by tests
}

desc, err := repository.Resolve(ctx, subjectReference.Original)
if err != nil {
evictOnError(ctx, err, subjectReference.Original)
return nil, re.ErrorCodeRepositoryOperationFailure.WithError(err).WithPluginName(storeName)
return nil, re.ErrorCodeRepositoryOperationFailure.WithDetail(fmt.Sprintf("Unable to resolve the reference: %s", subjectReference.Original)).WithError(err)
}

return &ocispecs.SubjectDescriptor{Descriptor: desc}, nil
Expand Down
Loading

0 comments on commit ab8d001

Please sign in to comment.