-
Notifications
You must be signed in to change notification settings - Fork 545
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
Fix e2e test failure, add test for local bundle without rekor bundle #2248
Conversation
Signed-off-by: Hayden Blauzvern <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2248 +/- ##
==========================================
+ Coverage 28.35% 28.39% +0.03%
==========================================
Files 131 131
Lines 7839 7839
==========================================
+ Hits 2223 2226 +3
+ Misses 5315 5313 -2
+ Partials 301 300 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@asraa I'm not 100% this is the right behavior. This is the case where we provide a local bundle (without experimental, so no rekor lookup) with just a signature (and it could optionally contain a cert too, but in the test, we pass a key), and the local bundle has no rekor bundle. This is probably actually a valid use-case, that you use the local bundle just to hold the verification material. |
Ugh. I see this case. cosign/cmd/cosign/cli/sign/sign_blob.go Line 86 in fb51ab0
So I think this should technically pass? |
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.
Fortunately this is a correctness issue, not a security concern...
But this really clobbers the verify Rekor Bundle logic. That I guess, should actually refer to the rekor bundle inside the local bundle.
Can we just add a check for experimental flag when verifying with a Rekor bundle? I think that’s the simplest fix. |
Simplest fix would be that verifyRekorBundle only executes when there is a bundle inside the local bundle, IMO! |
That sounds good! |
Signed-off-by: Asra Ali <[email protected]>
Done! Updated description and release note @cpanato @haydentherapper |
going to merge this to check the post submit jobs and if they are green will release it |
Signed-off-by: Hayden Blauzvern [email protected]
Summary
This fixes the behavior of
verify-blob
when the local bundle does not contain a Rekor bundle (for example, signed without experimental mode). In this case,verify-blob
will still run using the local bundle for the cert/sig information.If it does not contain one and we require a timestamp for cert validity, we fail. Adds a test for a short-lived cert with a bundle missing rekor bundle and no experimental fails. But if experimental is on, we can succeed.
Release Note
verify-blob
to handle local bundles that do not contain Rekor bundles.Documentation