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

feat: refactor cosign verification error messages #1750

Merged
merged 38 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 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
8893dac
feat: refactor error messages for cosign verification
binbin-li Aug 23, 2024
51d56e5
chore: address comments
binbin-li Sep 9, 2024
96c2bf8
Merge branch 'dev' into refactor-cosign-msg
binbin-li Sep 9, 2024
0a4cc40
chore: address comments
binbin-li Sep 10, 2024
b10327e
chore: address comments
binbin-li Sep 10, 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 pkg/referrerstore/oras/cosign.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func getCosignReferences(ctx context.Context, subjectReference common.Reference,
return nil, nil
}
evictOnError(ctx, err, subjectReference.Original)
return nil, re.ErrorCodeRepositoryOperationFailure.WithError(err).WithComponentType(re.ReferrerStore)
return nil, re.ErrorCodeRepositoryOperationFailure.WithDetail(fmt.Sprintf("Failed to validate the signature of the artifact: %+v", subjectReference)).WithError(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: consider adding cosign in the error message since it's co-sign specific. Also maybe adding failed to validate existence of cosign signature instead?

}

references = append(references, ocispecs.ReferenceDescriptor{
Expand All @@ -64,7 +64,7 @@ func getCosignReferences(ctx context.Context, subjectReference common.Reference,
func attachedImageTag(subjectReference common.Reference, tagSuffix string) (string, error) {
// sha256:d34db33f -> sha256-d34db33f.suffix
if subjectReference.Digest.String() == "" {
return "", re.ErrorCodeReferenceInvalid.WithComponentType(re.ReferrerStore).WithDetail("Cosign subject digest is empty")
return "", re.ErrorCodeReferenceInvalid.WithDetail("The digest of the artifact is empty")
}
tagStr := strings.ReplaceAll(subjectReference.Digest.String(), ":", "-") + tagSuffix
return fmt.Sprintf("%s:%s", subjectReference.Path, tagStr), nil
Expand Down
8 changes: 4 additions & 4 deletions pkg/referrerstore/oras/oras.go
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@
func (store *orasStore) ListReferrers(ctx context.Context, subjectReference common.Reference, _ []string, _ string, subjectDesc *ocispecs.SubjectDescriptor) (referrerstore.ListReferrersResult, error) {
repository, err := store.createRepository(ctx, store, subjectReference)
if err != nil {
return referrerstore.ListReferrersResult{}, re.ErrorCodeCreateRepositoryFailure.WithError(err).WithComponentType(re.ReferrerStore)
return referrerstore.ListReferrersResult{}, re.ErrorCodeRepositoryOperationFailure.WithDetail("Failed to connect to the remote registry").WithError(err)

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

View check run for this annotation

Codecov / codecov/patch

pkg/referrerstore/oras/oras.go#L211

Added line #L211 was not covered by tests
}

// resolve subject descriptor if not provided
Expand Down Expand Up @@ -260,7 +260,7 @@

repository, err := store.createRepository(ctx, store, subjectReference)
if err != nil {
return nil, err
return nil, re.ErrorCodeGetBlobContentFailure.WithDetail("Failed to connect to the remote registry").WithError(err)
}

// create a dummy Descriptor to check the local store cache
Expand Down Expand Up @@ -292,10 +292,10 @@
blobDesc, rc, err := repository.Blobs().FetchReference(ctx, ref)
if err != nil {
evictOnError(ctx, err, subjectReference.Original)
return nil, err
return nil, re.ErrorCodeRepositoryOperationFailure.WithDetail("Failed to fetch the artifact metadata from the registry").WithError(err)
}
if blobContent, err = io.ReadAll(rc); err != nil {
return nil, re.ErrorCodeGetBlobContentFailure.WithError(err)
return nil, re.ErrorCodeRepositoryOperationFailure.WithDetail("Failed to parse the artifact metadata").WithError(err)
}

// push fetched content to local ORAS cache
Expand Down
40 changes: 20 additions & 20 deletions pkg/verifier/cosign/cosign.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,17 +143,17 @@
logger.GetLogger(context.Background(), logOpt).Debugf("creating cosign verifier with config %v, namespace '%v'", verifierConfig, namespace)
verifierName, hasName := verifierConfig[types.Name].(string)
if !hasName {
return nil, re.ErrorCodeConfigInvalid.WithComponentType(re.Verifier).WithPluginName(verifierName).WithDetail("missing name in verifier config")
return nil, re.ErrorCodeConfigInvalid.WithDetail("The name field is required in the Cosign Verifier configuration")
}

config, err := parseVerifierConfig(verifierConfig)
if err != nil {
return nil, re.ErrorCodeConfigInvalid.WithComponentType(re.Verifier).WithPluginName(verifierName)
return nil, re.ErrorCodeConfigInvalid.WithDetail("Failed to create the Cosign Verifier").WithError(err)
}

// if key or rekorURL is provided, trustPolicies should not be provided
if (config.KeyRef != "" || config.RekorURL != "") && len(config.TrustPolicies) > 0 {
return nil, re.ErrorCodeConfigInvalid.WithComponentType(re.Verifier).WithPluginName(verifierName).WithDetail("'key' and 'rekorURL' are part of cosign legacy configuration and cannot be used with `trustPolicies`")
return nil, re.ErrorCodeConfigInvalid.WithDetail("'key' and 'rekorURL' are part of Cosign legacy configuration and cannot be used with `trustPolicies` parameter")
}

var trustPolicies *TrustPolicies
Expand All @@ -163,7 +163,7 @@
logger.GetLogger(context.Background(), logOpt).Debugf("legacy cosign verifier configuration not found, creating trust policies")
trustPolicies, err = CreateTrustPolicies(config.TrustPolicies, verifierName)
if err != nil {
return nil, err
return nil, re.ErrorCodePluginInitFailure.WithDetail("Failed to create the Cosign Verifier").WithError(err)
}
legacy = false
}
Expand Down Expand Up @@ -224,18 +224,18 @@
// get the reference manifest (cosign oci image)
referenceManifest, err := referrerStore.GetReferenceManifest(ctx, subjectReference, referenceDescriptor)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to get reference manifest: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail(fmt.Sprintf("Failed to get artifact metadata for %s", referenceDescriptor.Digest)).WithError(err)), nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail(fmt.Sprintf("Failed to get artifact metadata for %s", referenceDescriptor.Digest)).WithError(err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail(fmt.Sprintf("Failed to get cosign signature metadata for %s", referenceDescriptor.Digest)).WithError(err)), nil

}

// manifest must be an OCI Image
if referenceManifest.MediaType != imgspec.MediaTypeImageManifest {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("reference manifest is not an image")), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("The artifact metadata is not an OCI image")), nil
}

// get the subject image descriptor
subjectDesc, err := referrerStore.GetSubjectDescriptor(ctx, subjectReference)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to create subject hash: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyReferenceFailure.WithDetail(fmt.Sprintf("Failed to validate the Cosign signature of the artifact: %+v", subjectReference)).WithError(err)), nil
}

// create the hash of the subject image descriptor (used as the hashed payload)
Expand All @@ -255,23 +255,23 @@
// fetch the blob content of the signature from the referrer store
blobBytes, err := referrerStore.GetBlobContent(ctx, subjectReference, blob.Digest)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to get blob content: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeGetBlobContentFailure.WithDetail(fmt.Sprintf("Failed to get Cosign signature with digest %s", blob.Digest)).WithError(err)), nil
}
// convert the blob to a static signature
staticOpts, err := staticLayerOpts(blob)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to parse static signature opts: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail(fmt.Sprintf("Failed to parse Cosign signature with digest %s", blob.Digest)).WithError(err)), nil
}
sig, err := static.NewSignature(blobBytes, blob.Annotations[static.SignatureAnnotationKey], staticOpts...)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to generate static signature: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to validate the Cosign signature").WithError(err)), nil

Check warning on line 267 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L267

Added line #L267 was not covered by tests
}
if len(keysMap) > 0 {
// if keys are found, perform verification with keys
var verifications []cosignExtension
verifications, hasValidSignature, err = verifyWithKeys(ctx, keysMap, sig, blob.Annotations[static.SignatureAnnotationKey], blobBytes, staticOpts, &cosignOpts, subjectDescHash)
if err != nil {
return errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("failed to verify with keys: %w", err)), nil
return errorToVerifyResult(v.name, v.verifierType, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to validate the Cosign signature with keys").WithError(err)), nil
}
extensionListEntry.Verifications = append(extensionListEntry.Verifications, verifications...)
} else {
Expand All @@ -295,7 +295,7 @@
), nil
}

errorResult := errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("no valid signatures found"))
errorResult := errorToVerifyResult(v.name, v.verifierType, fmt.Errorf("no valid Cosign signatures found"))

Check warning on line 298 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L298

Added line #L298 was not covered by tests
errorResult.Extensions = Extension{SignatureExtension: sigExtensions, TrustPolicy: trustPolicy.GetName()}
return errorResult, nil
}
Expand Down Expand Up @@ -485,7 +485,7 @@

// ErrorToVerifyResult returns a verifier result with the error message and isSuccess set to false
func errorToVerifyResult(name string, verifierType string, err error) verifier.VerifierResult {
verifierErr := re.ErrorCodeVerifyReferenceFailure.WithDetail("Verification failed").WithError(err)
verifierErr := re.ErrorCodeVerifyReferenceFailure.WithDetail("Failed to validate the Cosign signature").WithError(err)
return verifier.NewVerifierResult(
"",
name,
Expand Down Expand Up @@ -540,14 +540,14 @@
if pubKey.ProviderType == azurekeyvault.ProviderName {
hashType, sig, err = processAKVSignature(sigEncoded, sig, pubKey.Key, payload, staticOpts)
if err != nil {
return verifications, false, fmt.Errorf("failed to process AKV signature: %w", err)
return verifications, false, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to validate the Cosign signature generated by AKV").WithError(err)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return verifications, false, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to validate the Cosign signature generated by AKV").WithError(err)
return verifications, false, re.ErrorCodeVerifyPluginFailure.WithDetail("Failed to validate the Cosign signature generated by Azure Key Vault").WithError(err)

}
}

// return the correct verifier based on public key type and bytes
verifier, err := signature.LoadVerifier(pubKey.Key, hashType)
if err != nil {
return verifications, false, fmt.Errorf("failed to load public key from provider [%s] name [%s] version [%s]: %w", mapKey.Provider, mapKey.Name, mapKey.Version, err)
return verifications, false, re.ErrorCodeVerifyPluginFailure.WithDetail(fmt.Sprintf("Failed to load public key from provider [%s] name [%s] version [%s]", mapKey.Provider, mapKey.Name, mapKey.Version)).WithError(err)

Check warning on line 550 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L550

Added line #L550 was not covered by tests
}
cosignOpts.SigVerifier = verifier
// verify signature with cosign options + perform bundle verification
Expand Down Expand Up @@ -627,17 +627,17 @@
// EC verifiers in cosign have built in ASN.1 decoding, but RSA verifiers do not
base64DecodedBytes, err := base64.StdEncoding.DecodeString(sigEncoded)
if err != nil {
return crypto.SHA256, nil, fmt.Errorf("RSA key check: failed to decode base64 signature: %w", err)
return crypto.SHA256, nil, re.ErrorCodeVerifyPluginFailure.WithDetail("RSA key check: failed to decode base64 signature").WithError(err)
}
// decode ASN.1 signature to raw signature if it is ASN.1 encoded
decodedSigBytes, err := decodeASN1Signature(base64DecodedBytes)
if err != nil {
return crypto.SHA256, nil, fmt.Errorf("RSA key check: failed to decode ASN.1 signature: %w", err)
return crypto.SHA256, nil, re.ErrorCodeVerifyPluginFailure.WithDetail("RSA key check: failed to decode ASN.1 signature").WithError(err)

Check warning on line 635 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L635

Added line #L635 was not covered by tests
}
encodedBase64SigBytes := base64.StdEncoding.EncodeToString(decodedSigBytes)
staticSig, err = static.NewSignature(payloadBytes, encodedBase64SigBytes, staticOpts...)
if err != nil {
return crypto.SHA256, nil, fmt.Errorf("RSA key check: failed to generate static signature: %w", err)
return crypto.SHA256, nil, re.ErrorCodeVerifyPluginFailure.WithDetail("RSA key check: failed to generate static signature").WithError(err)

Check warning on line 640 in pkg/verifier/cosign/cosign.go

View check run for this annotation

Codecov / codecov/patch

pkg/verifier/cosign/cosign.go#L640

Added line #L640 was not covered by tests
}
case *ecdsa.PublicKey:
switch keyType.Curve {
Expand All @@ -648,10 +648,10 @@
case elliptic.P521():
hashType = crypto.SHA512
default:
return crypto.SHA256, nil, fmt.Errorf("ECDSA key check: unsupported key curve: %s", keyType.Params().Name)
return crypto.SHA256, nil, fmt.Errorf("ECDSA key check: unsupported key curve [%s]", keyType.Params().Name)
}
default:
return crypto.SHA256, nil, fmt.Errorf("unsupported public key type: %T", publicKey)
return crypto.SHA256, nil, fmt.Errorf("unsupported public key type [%T]", publicKey)
}
return hashType, staticSig, nil
}
Expand Down
Loading
Loading