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: adds NonExistentTag Exit Code to Error #2766

Merged
merged 1 commit into from
Mar 12, 2023

Conversation

ChrisJBurns
Copy link
Contributor

@ChrisJBurns ChrisJBurns commented Mar 4, 2023

Fixes rest of of #948

As described in #948 "Currently if you run cosign verify against a non existing image, against a not signed image, against a signed image with a different key, the exit status is the same (1)" . #2673 implements exit codes for the scenario of an image being signed with a different key, this PR addresses the non existing image and no signatures found.

Summary

  • adds the exit code to when cosign throws an error due to a user trying to verify an image tag that doesn't exist.
  • adds functionality with associated exit code for when there are no signatures found for an image

@codecov
Copy link

codecov bot commented Mar 4, 2023

Codecov Report

Merging #2766 (1977cc8) into main (f61cdf0) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2766      +/-   ##
==========================================
- Coverage   29.54%   29.51%   -0.04%     
==========================================
  Files         151      151              
  Lines        9646     9657      +11     
==========================================
  Hits         2850     2850              
- Misses       6357     6368      +11     
  Partials      439      439              
Impacted Files Coverage Δ
cmd/cosign/errors/exit_code_lookup.go 100.00% <ø> (ø)
pkg/cosign/errors.go 60.00% <ø> (ø)
pkg/cosign/verify.go 36.79% <0.00%> (-0.44%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ChrisJBurns
Copy link
Contributor Author

ChrisJBurns commented Mar 4, 2023

@znewman01 Here is the additional exit code functionality for verifying a non existing image and an image with no signatures. There's less code than #2673 because the main wrapping code has been done previous. So this PR just adds the exit codes error type for a non existing image and image without any signatures and throwing it where it errors.

cpanato
cpanato previously approved these changes Mar 5, 2023
@cpanato cpanato requested a review from znewman01 March 5, 2023 17:17
pkg/cosign/verify.go Outdated Show resolved Hide resolved
@ChrisJBurns
Copy link
Contributor Author

Someone might need to rerun the E2E tests?

@@ -484,6 +484,12 @@ func VerifyImageSignatures(ctx context.Context, signedImgRef name.Reference, co
// entity that minimizes registry requests when supplied with a digest input
digest, err := ociremote.ResolveDigest(signedImgRef, co.RegistryClientOpts...)
if err != nil {
if strings.Contains(err.Error(), "MANIFEST_UNKNOWN") {
return nil, false, &VerificationError{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think best practice for wrapping errors is to store the inner error inside the struct and format the message inside the Error function.

See: https://go.dev/blog/go1.13-errors

znewman01
znewman01 previously approved these changes Mar 12, 2023
Copy link
Contributor

@znewman01 znewman01 left a comment

Choose a reason for hiding this comment

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

LGTM, your call as to whether you want to extend VerificationError for better wrapping.

@ChrisJBurns
Copy link
Contributor Author

ChrisJBurns commented Mar 12, 2023

@znewman01 I tell you what I can do, if we merge this PR, I can get another one up that wraps the errors a bit more nicely. As I'll have to do the same to the other errors we're wrapping as part of the previous exit codes anyways, worth having a single PR to address them all in one go.

e2e tests still seem to be failing with same error, not entirely sure if it's an intermittent error or if there is a genuine problem I've introduced as part of this PR.

@znewman01
Copy link
Contributor

I tell you what I can do, if we merge this PR, I can get another one up that wraps the errors a bit more nicely.

Works for me 👍

e2e tests still seem to be failing with same error, not entirely sure if it's an intermittent error or if there is a genuine problem I've introduced as part of this PR.

Filed #2786

@znewman01
Copy link
Contributor

e2e tests still seem to be failing with same error, not entirely sure if it's an intermittent error or if there is a genuine problem I've introduced as part of this PR.

Unfortunately, this is blocking all PR submissions! We'll get right on it

@znewman01
Copy link
Contributor

Rebase and try again?

- adds the exit code to when cosign throws an error due to a user trying to verify an image tag that doesn't exist.
- adds functionality with associated exit code for when there are no signatures found for an image

Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com>
@ChrisJBurns
Copy link
Contributor Author

@znewman01 Accidental close there from me, tests are running, fingers crossed!

@znewman01 znewman01 enabled auto-merge (squash) March 12, 2023 19:32
@znewman01 znewman01 merged commit 3238ae9 into sigstore:main Mar 12, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone Mar 12, 2023
dmitris pushed a commit to dmitris/cosign that referenced this pull request Mar 24, 2023
- adds the exit code to when cosign throws an error due to a user trying to verify an image tag that doesn't exist.
- adds functionality with associated exit code for when there are no signatures found for an image

Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants