-
Notifications
You must be signed in to change notification settings - Fork 64
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
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 7442b8a
mod: add const for refresherTypes
duffney aeaeacc
mod: Add cast checks & refresherType constants
duffney 3b7e47c
style: rename kmp.spec.refreshinterval to refreshInterval json tag
duffney b566b2d
fix: update samples with correct refreshInterval name
duffney 8cb3f0d
fix: new error for refresher.GetResult() cast check
duffney b6d3b60
chore: Bump anchore/sbom-action from 0.17.0 to 0.17.1
dependabot[bot] 5893c69
chore: Bump github/codeql-action from 3.26.0 to 3.26.1
dependabot[bot] bed493d
chore: update helm charts (#1702)
junczhu 2a36491
feat: add timestamp and traceId to verification response (#1697)
binbin-li f470985
chore: Bump github/codeql-action from 3.26.1 to 3.26.2
dependabot[bot] 7a89eac
chore: add the governance doc link to readme.md (#1713)
yizha1 a8177a5
Merge branch 'dev' into issue-1131/refresher
binbin-li ad92eb8
Merge pull request #1625 from duffney/issue-1131/refresher
binbin-li 78ea559
refactor: refactor error messages
binbin-li 7ff80d1
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li fa90085
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li dfaaa0d
test: fix broken tests
binbin-li b737156
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li c3112d7
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li 8a010b1
chore: address comments
binbin-li aaba189
chore: remove ending period from error details
binbin-li 83be113
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li 8e58648
chore: refactor msg
binbin-li eb1148c
chore: address comments
binbin-li 65617a1
chore: address comments
binbin-li 242be02
Merge remote-tracking branch 'upstream/dev' into refactor-error-msg
binbin-li d821a90
chore: address comments
binbin-li fe92a9e
docs: address comments
binbin-li ebb2217
Merge branch 'dev' into refactor-error-msg
binbin-li 4bf283d
Merge branch 'dev' into refactor-error-msg
binbin-li 45c29a2
Merge branch 'dev' into refactor-error-msg
binbin-li 4f48d2f
chore: address comments
binbin-li 8893dac
feat: refactor error messages for cosign verification
binbin-li 51d56e5
chore: address comments
binbin-li 96c2bf8
Merge branch 'dev' into refactor-cosign-msg
binbin-li 0a4cc40
chore: address comments
binbin-li b10327e
chore: address comments
binbin-li File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 | ||||||
|
@@ -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 | ||||||
} | ||||||
|
@@ -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 | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
// 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) | ||||||
|
@@ -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 | ||||||
} | ||||||
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 { | ||||||
|
@@ -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")) | ||||||
errorResult.Extensions = Extension{SignatureExtension: sigExtensions, TrustPolicy: trustPolicy.GetName()} | ||||||
return errorResult, nil | ||||||
} | ||||||
|
@@ -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, | ||||||
|
@@ -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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
} | ||||||
|
||||||
// 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) | ||||||
} | ||||||
cosignOpts.SigVerifier = verifier | ||||||
// verify signature with cosign options + perform bundle verification | ||||||
|
@@ -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) | ||||||
} | ||||||
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) | ||||||
} | ||||||
case *ecdsa.PublicKey: | ||||||
switch keyType.Curve { | ||||||
|
@@ -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 | ||||||
} | ||||||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 addingfailed to validate existence of cosign signature
instead?