-
Notifications
You must be signed in to change notification settings - Fork 557
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
Add support for new bundle specification for attesting/verifying OCI image attestations #3889
base: main
Are you sure you want to change the base?
Add support for new bundle specification for attesting/verifying OCI image attestations #3889
Conversation
4af8cc0
to
e509ec5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3889 +/- ##
==========================================
- Coverage 40.10% 36.11% -3.99%
==========================================
Files 155 210 +55
Lines 10044 13634 +3590
==========================================
+ Hits 4028 4924 +896
- Misses 5530 8092 +2562
- Partials 486 618 +132 ☔ View full report in Codecov by Sentry. |
af5093d
to
524f558
Compare
cosign verify-attestation
pkg/cosign/verify.go
Outdated
// Wrap TrustedMaterial | ||
vTrustedMaterial := &verifyTrustedMaterial{TrustedMaterial: co.TrustedMaterial} | ||
|
||
// If TrustedMaterial is not set, fetch it from TUF (TODO: should this even be done? Old verifier requires co.RootCerts to be set) |
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.
If they haven't set --trusted-root
then they could be using other flags or relying on the TUF v1 setup which might be pointed to something other than the public good instance. I understand that this is only meant to be called when --new-bundle-format
is set but I'm worried that this would be surprising behavior if the PGI trusted root is used while the cached TUF metadata is pointed somewhere else.
} | ||
|
||
for _, verified := range bundlesVerified { | ||
atLeastOneBundleVerified = atLeastOneBundleVerified || verified |
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.
Could you clarify why it's okay for only one bundle to be verified?
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.
This logic was copied from
Line 1054 in 737c83c
func VerifyImageAttestation(ctx context.Context, atts oci.Signatures, h v1.Hash, co *CheckOpts) (checkedAttestations []oci.Signature, bundleVerified bool, err error) { |
bundleVerified
to atLeastOneBundleVerified
so it's more clear. The effect is that if there are multiple bundles associated with an image, verification will succeed if at least one of them is verified.
I can write a comment about this if it helps!
@@ -361,7 +377,7 @@ func attestVerify(t *testing.T, predicateType, attestation, goodCue, badCue stri | |||
} | |||
|
|||
// Now attest the image | |||
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc} | |||
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc, NewBundleFormat: newBundleFormat} |
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.
Maybe out of scope for this PR but a note for the future, this is setting NewBundleFormat
without setting TrustedRootPath
, which means the TrustedMaterial
will default to using PGI. We're trying to avoid making external network calls to live services. The only reason that isn't a problem here is because this test is using a local key pair and is not uploading to Rekor, so it doesn't matter what verification material is used. But a useful test in the future might be to have this use ephemeral keys from Fulcio (localhost:5555) and upload the entry to Rekor (localhost:3000) so that the full verification path with a locally generated trust root could be tested.
2c27fc5
to
1602cc1
Compare
I rebased and squashed all commits into one as there have been a couple of items refactored into separate PRs (#4006 and #4013) and this ongoing PR was getting a bit messy. I still need to finish #4013 and rebase this one again, at which point I will be ready for another review pass. Thanks everyone who has been patiently reviewing and helping me iterate on this! |
Signed-off-by: Cody Soyland <[email protected]>
Signed-off-by: Cody Soyland <[email protected]>
1602cc1
to
6d7fbf2
Compare
NewBundleFormat bool | ||
TrustedRootPath string |
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.
Note these have moved to CommonVerifyOptions
This has been rebased again and is ready for review. |
Summary
This PR adds support for the new Cosign Bundle Specification in
cosign attest
andcosign verify-attestation
.Related: #3139
To test, run the following (replacing
MY_IDENTITY
,MY_ISSUER
,MY_TRUSTED_ROOT
andMY_IMAGE
as needed -- trusted root is optional). Note that the new OCI support requires passing--new-bundle-format
into both commands.Full example (using
crane
, but can instead usedocker tag
/docker push
):To show that it uses the OCI 1.1 referrers API, you can use
oras
:Release Note
Documentation