feat: refactor cosign verification error messages #1750

binbin-li · 2024-08-26T08:24:40Z

Description

What this PR does / why we need it:

This PR is based on #1730, check the actual diff at: binbin-li#230

It refactors error messages involved in Cosign signature validation, improving the clarity of error messages and providing more intuitive remediation steps for users.

For errors that are self-explanatory, I have not included remediation steps.

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

Part of work for #1321

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Helm Chart Change (any edit/addition/update that is necessary for changes merged to the main branch)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

Test A
Test B

Checklist:

Does the affected code have corresponding tests?
Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
Does this introduce breaking changes that would require an announcement or bumping the major version?
Do all new files have appropriate license header?

Post Merge Requirements

MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates Helm Chart Change

feat: refresher interface fix: formatting and license test: kuberefresher test: kuberefresher helpers refactor: IsRefreshable KMP interface method fix: add IsRefreshable method refactor: kmp spec interval as string mod: rm todo mod: rm comment style: use embedded type feat: namespaced refresher interface test: mock factory & test refreshable logic refactor: use kubeRefreshNamespaced in controller mod: add license blocks refactor: kmp.spec.interval default to empty string disabling refresh tests: increase refresher coverage refactor(kmp): use factory for creating refresher objects This change introduces a RefresherFactory that is responsible for creating the appropriate refresher object based on a configuration that's passed in. This decouples the reconcile logic from the object creation, making the code more modular and easier to test mod: rename interval to refreshinterval & adds crd descriptions mod: remove verbose logging from refresher docs: kmp refresh interval samples doc: corrected resource name of akv samples fix: kmp spec json tag using incorrect values & add refresh interval description fix: add licenses & update interval name in tests mod: comments for exported interfaces test: improve refresher test coverage Signed-off-by: Joshua Duffney <[email protected]>

Signed-off-by: Joshua Duffney <[email protected]>

Bumps [anchore/sbom-action](https://github.com/anchore/sbom-action) from 0.17.0 to 0.17.1. - [Release notes](https://github.com/anchore/sbom-action/releases) - [Commits](anchore/sbom-action@d94f46e...ab9d16d) --- updated-dependencies: - dependency-name: anchore/sbom-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.26.0 to 3.26.1. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@eb055d7...29d86d2) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Signed-off-by: Joshua Duffney <[email protected]>

…ect#1697) Signed-off-by: Joshua Duffney <[email protected]>

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.26.1 to 3.26.2. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@29d86d2...429e197) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Signed-off-by: Yi Zha <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

codecov · 2024-08-26T08:28:09Z

Codecov Report

Attention: Patch coverage is 82.75862% with 10 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/verifier/cosign/cosign.go	75.00%	5 Missing ⚠️
pkg/verifier/cosign/trustpolicies.go	80.00%	2 Missing ⚠️
pkg/verifier/cosign/trustpolicy.go	90.90%	2 Missing ⚠️
pkg/referrerstore/oras/oras.go	75.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
pkg/referrerstore/oras/cosign.go	`100.00% <100.00%> (ø)`
pkg/referrerstore/oras/oras.go	`80.97% <75.00%> (ø)`
pkg/verifier/cosign/trustpolicies.go	`94.52% <80.00%> (ø)`
pkg/verifier/cosign/trustpolicy.go	`90.59% <90.90%> (+8.54%)`	⬆️
pkg/verifier/cosign/cosign.go	`80.97% <75.00%> (+2.30%)`	⬆️

binbin-li · 2024-09-09T08:51:29Z

I've resolved comments from @yizha1 at the PR from my forked repo: binbin-li#230

pkg/verifier/cosign/cosign.go

pkg/verifier/cosign/trustpolicy.go

yizha1

LGTM

FeynmanZhou

LGTM

akashsinghal

Thanks for adding this! Left some non blocking comments.

akashsinghal · 2024-09-10T06:20:54Z

pkg/referrerstore/oras/cosign.go

@@ -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)


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?

akashsinghal · 2024-09-10T06:22:56Z

pkg/verifier/cosign/cosign.go

@@ -224,18 +224,18 @@ func (v *cosignVerifier) verifyInternal(ctx context.Context, subjectReference co
 	// 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


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

akashsinghal · 2024-09-10T06:24:09Z

pkg/verifier/cosign/cosign.go

@@ -540,14 +540,14 @@ func verifyWithKeys(ctx context.Context, keysMap map[PKKey]keymanagementprovider
 		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)


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)

Signed-off-by: akashsinghal <[email protected]>

duffney and others added 22 commits August 16, 2024 05:55

mod: add const for refresherTypes

7442b8a

Signed-off-by: Joshua Duffney <[email protected]>

mod: Add cast checks & refresherType constants

aeaeacc

Signed-off-by: Joshua Duffney <[email protected]>

style: rename kmp.spec.refreshinterval to refreshInterval json tag

3b7e47c

Signed-off-by: Joshua Duffney <[email protected]>

fix: update samples with correct refreshInterval name

b566b2d

Signed-off-by: Joshua Duffney <[email protected]>

fix: new error for refresher.GetResult() cast check

8cb3f0d

Signed-off-by: Joshua Duffney <[email protected]>

chore: update helm charts (ratify-project#1702)

bed493d

Signed-off-by: Joshua Duffney <[email protected]>

feat: add timestamp and traceId to verification response (ratify-proj…

2a36491

…ect#1697) Signed-off-by: Joshua Duffney <[email protected]>

chore: add the governance doc link to readme.md (ratify-project#1713)

7a89eac

Signed-off-by: Yi Zha <[email protected]> Signed-off-by: Joshua Duffney <[email protected]>

Merge branch 'dev' into issue-1131/refresher

a8177a5

Merge pull request ratify-project#1625 from duffney/issue-1131/refresher

ad92eb8

refactor: refactor error messages

78ea559

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

7ff80d1

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

fa90085

test: fix broken tests

dfaaa0d

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

b737156

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

c3112d7

chore: address comments

8a010b1

chore: remove ending period from error details

aaba189

binbin-li requested review from akashsinghal, jimmyraywv, luisdlp, susanshi and toddysm as code owners August 26, 2024 08:24

binbin-li force-pushed the refactor-cosign-msg branch 2 times, most recently from 360d80b to 381d61a Compare August 28, 2024 04:55

binbin-li force-pushed the refactor-cosign-msg branch from 86e32c9 to fac5b5a Compare September 1, 2024 15:17

chore: address comments

65617a1

binbin-li force-pushed the refactor-cosign-msg branch from fac5b5a to 99bd939 Compare September 2, 2024 03:18

Merge remote-tracking branch 'upstream/dev' into refactor-error-msg

242be02

binbin-li force-pushed the refactor-cosign-msg branch from 99bd939 to 7bd896d Compare September 2, 2024 05:34

binbin-li and others added 5 commits September 4, 2024 06:37

chore: address comments

d821a90

docs: address comments

fe92a9e

Merge branch 'dev' into refactor-error-msg

ebb2217

Merge branch 'dev' into refactor-error-msg

4bf283d

Merge branch 'dev' into refactor-error-msg

45c29a2

binbin-li force-pushed the refactor-cosign-msg branch from 7bd896d to 0c99179 Compare September 9, 2024 08:50

binbin-li added 3 commits September 9, 2024 08:54

chore: address comments

4f48d2f

feat: refactor error messages for cosign verification

8893dac

chore: address comments

51d56e5

binbin-li force-pushed the refactor-cosign-msg branch from 0c99179 to 51d56e5 Compare September 9, 2024 11:53

Merge branch 'dev' into refactor-cosign-msg

96c2bf8

yizha1 reviewed Sep 10, 2024

View reviewed changes

pkg/verifier/cosign/cosign.go Outdated Show resolved Hide resolved

pkg/verifier/cosign/trustpolicy.go Outdated Show resolved Hide resolved

pkg/verifier/cosign/trustpolicy.go Outdated Show resolved Hide resolved

pkg/verifier/cosign/trustpolicy.go Outdated Show resolved Hide resolved

binbin-li added 2 commits September 10, 2024 03:26

chore: address comments

0a4cc40

chore: address comments

b10327e

yizha1 approved these changes Sep 10, 2024

View reviewed changes

FeynmanZhou approved these changes Sep 10, 2024

View reviewed changes

binbin-li enabled auto-merge (squash) September 10, 2024 05:36

binbin-li disabled auto-merge September 10, 2024 05:56

binbin-li enabled auto-merge (squash) September 10, 2024 06:05

akashsinghal approved these changes Sep 10, 2024

View reviewed changes

binbin-li merged commit 4d4d00c into ratify-project:dev Sep 10, 2024
19 checks passed

binbin-li mentioned this pull request Sep 10, 2024

chore: update error messages for cosign validation #1792

Merged

12 tasks

akashsinghal pushed a commit to akashsinghal/ratify that referenced this pull request Sep 13, 2024

feat: refactor cosign verification error messages (ratify-project#1750)

e0b6c7c

Signed-off-by: akashsinghal <[email protected]>

binbin-li added a commit to binbin-li/ratify that referenced this pull request Sep 14, 2024

feat: refactor cosign verification error messages (ratify-project#1750)

da04523

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor cosign verification error messages #1750

feat: refactor cosign verification error messages #1750

binbin-li commented Aug 26, 2024 •

edited

Loading

codecov bot commented Aug 26, 2024 •

edited

Loading

binbin-li commented Sep 9, 2024

yizha1 left a comment

FeynmanZhou left a comment

akashsinghal left a comment

akashsinghal Sep 10, 2024

akashsinghal Sep 10, 2024

akashsinghal Sep 10, 2024

	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

	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)

feat: refactor cosign verification error messages #1750

feat: refactor cosign verification error messages #1750

Conversation

binbin-li commented Aug 26, 2024 • edited Loading

Description

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):

Type of change

How Has This Been Tested?

Checklist:

Post Merge Requirements

codecov bot commented Aug 26, 2024 • edited Loading

Codecov Report

binbin-li commented Sep 9, 2024

yizha1 left a comment

Choose a reason for hiding this comment

FeynmanZhou left a comment

Choose a reason for hiding this comment

akashsinghal left a comment

Choose a reason for hiding this comment

akashsinghal Sep 10, 2024

Choose a reason for hiding this comment

akashsinghal Sep 10, 2024

Choose a reason for hiding this comment

akashsinghal Sep 10, 2024

Choose a reason for hiding this comment

binbin-li commented Aug 26, 2024 •

edited

Loading

Which issue(s) this PR fixes (optional, using `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when the PR gets merged):

codecov bot commented Aug 26, 2024 •

edited

Loading