move verify-blob function to "pkg/cosign" to be consistent with verify
#2223
Conversation
Signed-off-by: Hirokuni-Kitahara1 <[email protected]>
Signed-off-by: Hirokuni-Kitahara1 <[email protected]>
Signed-off-by: Hirokuni-Kitahara1 <[email protected]>
Signed-off-by: Hirokuni-Kitahara1 <[email protected]>
Signed-off-by: Hirokuni-Kitahara1 <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2223 +/- ##
==========================================
+ Coverage 26.62% 26.64% +0.01%
==========================================
Files 131 132 +1
Lines 7680 7676 -4
==========================================
Hits 2045 2045
+ Misses 5376 5372 -4
Partials 259 259
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Hirokuni-Kitahara1 <[email protected]>
Signed-off-by: Hirokuni-Kitahara1 <[email protected]>
|
|
||
| var validSigExists bool | ||
| for _, verifier := range verifiers { | ||
| if err := verifyOCISignature(ctx, verifier, sig); err != nil { |
There was a problem hiding this comment.
What happens to intoto attestations?
There was a problem hiding this comment.
Thank you for your reviewing! The intoto DSSE verifier will only be available in keyless verification mode with my current codes, and this is simply a mistake.
I will fix this.
| checkedSignatures = append(checkedSignatures, sig) | ||
| } | ||
|
|
||
| if !validSigExists && co.RekorClient != nil { |
There was a problem hiding this comment.
How come the TLOG entry time will only be checked if a valid sig does not exist? verifyOCISignature does not check cert expiry time.
There was a problem hiding this comment.
Thank you for the information, I think we should have VerifyBundle() after verifyOCISignature() as VerifyImageSignature does. I will fix this.
|
We should hold off on moving this code until a) the refactor we are working on is done b) there are most tests so we have confidence in moving code. |
|
Another meta comment, seems like there are functional changes mixed into this. A refactor should be as minimal as possible, so if we did want to move this, it should be clear that we are just moving the functions, not making any changes in them. |
|
@asraa Thank you for your reviewing and your comments, I will update the changes to fix some problems you mentioned in the comments. @haydentherapper Thank you for your comment, I totally agree with that we should not mix a refactor and functional changes in 1 PR. I will remove functional changes from this PR and will create another PR for it when the timing is good. |
|
We'll hopefully have the refactor in by either end of this week or next week. |
|
That's great! Thank you very much @haydentherapper |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR was closed because it has been stalled for 10 days with no activity. |
Signed-off-by: Hirokuni-Kitahara1 [email protected]
Summary
This PR solves the issue #2222 by moving the core function of
verify-blobfrom "cmd/cosign/cli/verify" package to "pkg/cosign" package.The new function
VerifyBlobSignature()is based on basically the same design asVerifyImageSignature(), which is the core function ofverifycommand.Also the arguments of
VerifyBlobSignature()are all non-filepath information such as []byte, so it is easy for developers to invoke this function even on some read-only environment.Release Note
Documentation