Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Refactor error messages for Notation signature verification #1730

Merged
merged 34 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
48ac346
Mod: Add interval and refreshable to spec
duffney Jul 10, 2024
7442b8a
mod: add const for refresherTypes
duffney Aug 14, 2024
aeaeacc
mod: Add cast checks & refresherType constants
duffney Aug 14, 2024
3b7e47c
style: rename kmp.spec.refreshinterval to refreshInterval json tag
duffney Aug 14, 2024
b566b2d
fix: update samples with correct refreshInterval name
duffney Aug 14, 2024
8cb3f0d
fix: new error for refresher.GetResult() cast check
duffney Aug 14, 2024
b6d3b60
chore: Bump anchore/sbom-action from 0.17.0 to 0.17.1
dependabot[bot] Aug 14, 2024
5893c69
chore: Bump github/codeql-action from 3.26.0 to 3.26.1
dependabot[bot] Aug 14, 2024
bed493d
chore: update helm charts (#1702)
junczhu Aug 15, 2024
2a36491
feat: add timestamp and traceId to verification response (#1697)
binbin-li Aug 15, 2024
f470985
chore: Bump github/codeql-action from 3.26.1 to 3.26.2
dependabot[bot] Aug 15, 2024
7a89eac
chore: add the governance doc link to readme.md (#1713)
yizha1 Aug 15, 2024
a8177a5
Merge branch 'dev' into issue-1131/refresher
binbin-li Aug 16, 2024
ad92eb8
Merge pull request #1625 from duffney/issue-1131/refresher
binbin-li Aug 16, 2024
78ea559
refactor: refactor error messages
binbin-li Aug 19, 2024
7ff80d1
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li Aug 20, 2024
fa90085
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li Aug 21, 2024
dfaaa0d
test: fix broken tests
binbin-li Aug 21, 2024
b737156
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li Aug 22, 2024
c3112d7
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li Aug 23, 2024
8a010b1
chore: address comments
binbin-li Aug 23, 2024
aaba189
chore: remove ending period from error details
binbin-li Aug 23, 2024
83be113
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li Aug 29, 2024
8e58648
chore: refactor msg
binbin-li Aug 29, 2024
eb1148c
chore: address comments
binbin-li Aug 30, 2024
65617a1
chore: address comments
binbin-li Sep 1, 2024
242be02
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li Sep 2, 2024
d821a90
chore: address comments
binbin-li Sep 4, 2024
fe92a9e
docs: address comments
binbin-li Sep 9, 2024
ebb2217
Merge branch 'dev' into refactor-error-msg
binbin-li Sep 9, 2024
4bf283d
Merge branch 'dev' into refactor-error-msg
binbin-li Sep 9, 2024
45c29a2
Merge branch 'dev' into refactor-error-msg
binbin-li Sep 9, 2024
4f48d2f
chore: address comments
binbin-li Sep 9, 2024
06faa58
Merge branch 'dev' into refactor-error-msg
binbin-li Sep 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
binbin-li marked this conversation as resolved.
Show resolved Hide resolved
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
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 @@
// 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 @@
}
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 @@
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 @@
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 @@
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 @@
// 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 @@
// 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) 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 @@
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 @@
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) 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
Loading