-
Notifications
You must be signed in to change notification settings - Fork 658
Default to using the new protobuf format #4318
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #4318 +/- ##
==========================================
- Coverage 40.10% 34.32% -5.78%
==========================================
Files 155 217 +62
Lines 10044 15549 +5505
==========================================
+ Hits 4028 5337 +1309
- Misses 5530 9520 +3990
- Partials 486 692 +206 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Overall LGTM. Let's hold off on this PR until we land #4316 and #4319, do a pass over the other open PRs, and review the Cosign v3 brainstorm to see if there's anything else we want to get in. Then let's cut a new release of Cosign, hopefully the last v2 release, and then start merging these breaking changes. |
1326825 to
307f358
Compare
|
So, in some senses this is not a breaking change, in that we've made it so that Verifies if you explicitly say it's the old bundle format: ... but also verifies if you omit the bundle format (defaults to true): I spent some time looking into providing this fallback behavior for OCI verify commands ( |
|
There's some issues with how the trusted root file is working in |
|
I couldn't get the scaffolding working with the new bundle format - I think there was some issue with how the trusted root was constructed - but we can update it when we overhaul the scaffolding test. |
1fb6393 to
936e3a3
Compare
|
Okay! With #4346 I think the old scaffolding is working with the new protobuf bundle format. I also added code for |
a453e1d to
f6640a5
Compare
f6640a5 to
8b15976
Compare
|
The one downside with this is that while it's not a breaking change for verification of old metadata, it is a breaking change for verifying new metadata, in that you can't use an old client to verify new signatures. Are we OK with that? |
I think so. I would hope that an organization would upgrade it's toolchain (for signing and verifying) to a new version at the same time. If we don't think that's reasonable, the alternative (which I'm okay with) is cutting a last cosign 2.x release before this lands and have this be the start of cosign v3! |
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
…ml; until cosign sign supports new bundle format Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Have `cosign sign` default to true Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
Signed-off-by: Zach Steindler <[email protected]>
8b15976 to
8f4cc24
Compare
Signed-off-by: Zach Steindler <[email protected]>
haydentherapper
left a comment
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.
Fantastic!
| if !c.LocalImage { | ||
| ref, err := name.ParseReference(images[0], c.NameOptions...) | ||
| if err == nil && c.NewBundleFormat { | ||
| newBundles, _, err := cosign.GetBundles(ctx, ref, co) |
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.
We'll make an extra network call with GetBundles - do you think that's fine? Or should we store the bundles it finds and pass those to the verification function? Not sure if that's straightforward though.
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.
Yeah, I mention this above:
I spent some time looking into providing this fallback behavior for OCI verify commands (
verify,verify-attestation) as well. It's technical possible of course, but it would either add quite a few requests to your registry for each verify (essentially doubling the requests) or require a somewhat substantial refactor in order to query once and then pass the results along topkg/cosign/verify.gofunctions - what do you think?
As you can see, I opted to implement increasing the number of requests 😆 While testing / debugging these changes though, I discovered there was already another place in cmd/cosign/cli/verify/verify_attestation.go where we were doing this, in addition to pkg/cosign/verify.go and pkg/oci/remote/write.go. In short, I don't think it's an issue.
haydentherapper
left a comment
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.
Meant to approve, just one comment!
| } | ||
|
|
||
| // Check to see if we are using the new bundle format or not | ||
| if !c.LocalImage { |
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.
Just to confirm, this should handle backwards compatibility for verification of either the new or old bundle format?
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.
Yes, so this version of the code has c.NewBundleFormat default to true, so it tests to make sure you aren't using an old format bundle.
The only case that doesn't work here is if you explicitly specify --new-bundle-format=false while pointing it at a new bundle, but I'm not sure how you'd end up in that case.
| - name: Verify with cosign | ||
| run: | | ||
| ./cosign verify --rekor-url ${REKOR_URL} --allow-insecure-registry ${demoimage} --certificate-identity https://kubernetes.io/namespaces/default/serviceaccounts/default --certificate-oidc-issuer "https://kubernetes.default.svc.cluster.local" | ||
| ./cosign verify --trusted-root=${trustedroot} --allow-insecure-registry ${demoimage} --certificate-identity https://kubernetes.io/namespaces/default/serviceaccounts/default --certificate-oidc-issuer "https://kubernetes.default.svc.cluster.local" |
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.
--trusted-root shouldn't be necessary here, it's already set for these local services in the cosign initialize step (both the air-gapped version and remote version)
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.
I think that now that #4347 landed (which I wrote after this pull request) you're right!
I'm going to leave this as-is since this pull request has been going for a month and a half, but I support testing / removing --trusted-root from kind-verify-attestation.yaml.
|
|
||
| // TODO: have this default to true as a breaking change | ||
| cmd.Flags().BoolVar(&o.NewBundleFormat, "new-bundle-format", false, "expect the signature/attestation to be packaged in a Sigstore bundle") | ||
| cmd.Flags().BoolVar(&o.NewBundleFormat, "new-bundle-format", true, "expect the signature/attestation to be packaged in a Sigstore bundle") |
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.
Not your change, but why is the word "expect" here? The sign command is creating the bundle in a format it decides, not expecting anything of an existing bundle.
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.
Yeah, I think this is just copy/pasted from verify commands - we should change this! But I'm going to leave this as-is to not further increase the scope of this pull request.
Summary
This is a first step towards #4221, where we default
--new-bundle-formattotrueinstead offalse.We never did add protobuf support to cosign sign (see #3927 and #3139) - maybe we want to wait until that is done? Otherwise we could just have
--new-bundle-formatdefault totruewhen we add it to cosign sign.Release Note
attest,attest-blob,sign,sign-blob,dockerfile-verify,manifest-verify,verify,verify-attestation,verify-blob,verify-blob-attestationso that--new-bundle-formatdefaults totrueinstead offalse.Documentation
Ran
make docgenas part of this PR.