Add new bundle support to `verify-blob` and `verify-blob-attestation` #3796

steiza · 2024-07-23T21:14:50Z

Summary

This pull request is for the second part of #3139: adding protobuf bundle support for verify-blob and verify-blob-attestation.

Now that #3752 has landed, you can first generate bundles with something like:

$ go run cmd/cosign/main.go sign-blob --bundle="test.sigstore.json" --key="cosign.key" --new-bundle-format=true --tlog-upload=false ../sigstore-go/examples/sigstore-go-signing/hello_world.txt

And verify it (with these changes) with something like:

$ go run cmd/cosign/main.go verify-blob --bundle=test.sigstore.json --new-bundle-format=true --key=cosign.pub --use-signed-timestamps=false --insecure-ignore-tlog=true ../sigstore-go/examples/sigstore-go-signing/hello_world.txt

Or do an attestation with something like:

$ cat predicate.txt 
{}
$ go run cmd/cosign/main.go attest-blob --bundle="attest-pgi.sigstore.json" --new-bundle-format=true --identity-token="..." --type=something --predicate="predicate.txt" ../sigstore-go/examples/sigstore-go-signing/hello_world.txt

And then verify that attestation with these changes (assuming your identity token came from https://github.com/sigstore-conformance/extremely-dangerous-public-oidc-beacon/), with:

$ go run cmd/cosign/main.go verify-blob-attestation --bundle attest-pgi.sigstore.json --new-bundle-format=true --certificate-oidc-issuer="https://token.actions.githubusercontent.com" --certificate-identity="https://github.com/sigstore-conformance/extremely-dangerous-public-oidc-beacon/.github/workflows/extremely-dangerous-oidc-beacon.yml@refs/heads/main" --type=something ../sigstore-go/examples/sigstore-go-signing/hello_world.txt

Release Note

NONE - we probably want to finish #3139 (especially the more comprehensive conformance testing!) before we announce this as released.

Documentation

N/A - same as above

Part of sigstore#3139 Signed-off-by: Zach Steindler <steiza@github.com>

steiza · 2024-07-23T21:16:32Z

cmd/cosign/cli/verify/verify_bundle.go

+
+func verifyNewBundle(ctx context.Context, bundlePath, trustedRootPath, keyRef, slot, certOIDCIssuer, certOIDCIssuerRegex, certIdentity, certIdentityRegexp, githubWorkflowTrigger, githubWorkflowSHA, githubWorkflowName, githubWorkflowRepository, githubWorkflowRef, artifactRef string, sk, ignoreTlog, useSignedTimestamps, ignoreSCT bool) error {
+	if certOIDCIssuerRegex != "" {
+		return fmt.Errorf("--new-bundle-format does not support --certificate-oidc-issuer-regexp")


Although support is coming soon! sigstore/sigstore-go#236

steiza · 2024-07-23T21:17:50Z

go.mod

 	github.com/sigstore/rekor v1.3.6
 	github.com/sigstore/sigstore v1.8.7
+	github.com/sigstore/sigstore-go v0.4.1-0.20240717174219-8554eb6de5ac


To make use of sigstore/sigstore-go#235. Once sigstore/sigstore-go#236 also lands we should consider cutting a numbered release of sigstore-go to include these two changes.

Approved - shall we merge all the open dependabot PRs and then cut a new release? (Edit, I created a PR to bump all the deps)

codecov · 2024-07-23T21:18:42Z

Codecov Report

Attention: Patch coverage is 37.68116% with 86 lines in your changes missing coverage. Please review.

Project coverage is 37.65%. Comparing base (2ef6022) to head (c3c07dc).
Report is 167 commits behind head on main.

Files	Patch %	Lines
cmd/cosign/cli/verify/verify_bundle.go	35.92%	56 Missing and 10 partials ⚠️
cmd/cosign/cli/options/verify.go	0.00%	14 Missing ⚠️
cmd/cosign/cli/verify.go	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3796      +/-   ##
==========================================
- Coverage   40.10%   37.65%   -2.45%     
==========================================
  Files         155      202      +47     
  Lines       10044    12571    +2527     
==========================================
+ Hits         4028     4734     +706     
- Misses       5530     7251    +1721     
- Partials      486      586     +100

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Zach Steindler <steiza@github.com>

haydentherapper · 2024-07-23T21:43:59Z

cmd/cosign/cli/verify/verify_bundle.go

+			return err
+		}
+
+		newExpiringKey := root.NewExpiringKey(signatureVerifier, time.Time{}, time.Time{})


Out of scope for this, should we create a non-expiring key as well?

The terminology here is confusing - despite the name NewExpiringKey, since the validityPeriodStart and validityPeriodEnd will return true for IsZero(), this key won't actually be treated as expiring.

In the future, if there are cosign CLI flags for specifying the validity period of the key, we could make use of them here!

haydentherapper · 2024-07-23T21:50:20Z

cmd/cosign/cli/verify/verify_bundle.go

+	}
+	buf := bytes.NewBuffer(payload)
+
+	sev, err := verify.NewSignedEntityVerifier(trustedmaterial, verifierConfig...)


Originally, what I had in mind was decomposing the bundle into the CheckOpts and OCI structs so that we could call VerifyBlobSignature. The concern was a lack of parity between the two verification policies.

Given that this is guarded behind a flag, it wouldn't be a breaking change to have a different verification path. And maybe I'm overthinking this, that there isn't actually a significant gap between the two verification policies - keys was one concern, but it seems like you've fully supported that here.

WDYT, between either using sigstore-go's verification library or continuing to use Cosign's with bundle decomposition?

That was the path I originally started going down - decomposing the bundle and using cosign's existing VerifyBlobSignature() - similar to the technique we used to add signing support. But it's a fair amount of work to decompose both the bundle and the trusted root into the format cosign is expecting, which would look very similar to code that's already in sigstore-go. Since I think we want to eventually get sigstore-go into cosign (and have more parts of cosign use sigstore-go in the future) I jumped ahead a bit in this pull request.

I agree, I'm happy to see us cut over now. I've been looking over VerifyBlobSignature to see if there are any differences that we'd be missing, but I don't see anything concerning.

Online verification for Rekor is missing, but that's a good thing, I want to remove this. Bundles always have inclusion proofs and online lookups by default isn't a great design.

There is no support for verifying with certificate chains bundled in the verification material, but I know that's blocked on support in sigstore-go, and it's simple enough to work around with a trust root file.

Signed-off-by: Zach Steindler <steiza@github.com>

haydentherapper

This is great, thanks for your work on this! Just one tiny comment.

cmd/cosign/cli/verify/verify_bundle.go

haydentherapper · 2024-07-25T22:24:36Z

cmd/cosign/cli/verify/verify_bundle.go

+	}
+	buf := bytes.NewBuffer(payload)
+
+	sev, err := verify.NewSignedEntityVerifier(trustedmaterial, verifierConfig...)


I agree, I'm happy to see us cut over now. I've been looking over VerifyBlobSignature to see if there are any differences that we'd be missing, but I don't see anything concerning.

Online verification for Rekor is missing, but that's a good thing, I want to remove this. Bundles always have inclusion proofs and online lookups by default isn't a great design.

There is no support for verifying with certificate chains bundled in the verification material, but I know that's blocked on support in sigstore-go, and it's simple enough to work around with a trust root file.

Signed-off-by: Zach Steindler <steiza@github.com>

haydentherapper

I'm good to merge as-is!
I'd like a little more testing ideally, could we add an e2e test under https://github.com/sigstore/cosign/blob/main/test/e2e_test.go as well? Can do as a follow up if you prefer.

steiza · 2024-07-29T13:51:41Z

I would say land as-is. I'm already working on a conformance pull request (which has an open question), which we can roll additional e2e tests into as well.

Add new bundle support to verify-blob and verify-blob-attestation

da2d86b

Part of sigstore#3139 Signed-off-by: Zach Steindler <steiza@github.com>

steiza commented Jul 23, 2024

View reviewed changes

fix error message

cd8d985

Signed-off-by: Zach Steindler <steiza@github.com>

haydentherapper reviewed Jul 23, 2024

View reviewed changes

Use sigstore-go v0.5.1 for cert issuer regex support

cad0dde

Signed-off-by: Zach Steindler <steiza@github.com>

steiza force-pushed the verify-new-bundle branch from 21d35fb to cad0dde Compare July 25, 2024 14:26

haydentherapper reviewed Jul 25, 2024

View reviewed changes

Use more specific WithIntegratedTimestamps with tlog verification

c3c07dc

Signed-off-by: Zach Steindler <steiza@github.com>

bdehamer mentioned this pull request Jul 26, 2024

Add instructions for verifying the attestations using cosign actions/attest-build-provenance#162

Open

steiza mentioned this pull request Jul 26, 2024

Support the protobuf bundle format in Cosign #3139

Open

7 tasks

haydentherapper approved these changes Jul 26, 2024

View reviewed changes

haydentherapper merged commit b4cf37b into sigstore:main Jul 29, 2024
22 checks passed

steiza mentioned this pull request Sep 16, 2024

Support trusted root in cosign verification #3854

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new bundle support to `verify-blob` and `verify-blob-attestation` #3796

Add new bundle support to `verify-blob` and `verify-blob-attestation` #3796

steiza commented Jul 23, 2024

steiza Jul 23, 2024

steiza Jul 23, 2024

haydentherapper Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

haydentherapper Jul 23, 2024

steiza Jul 24, 2024

haydentherapper Jul 23, 2024

steiza Jul 24, 2024

haydentherapper Jul 25, 2024

haydentherapper left a comment

haydentherapper Jul 25, 2024

haydentherapper left a comment •

edited

Loading

steiza commented Jul 29, 2024

Add new bundle support to verify-blob and verify-blob-attestation #3796

Add new bundle support to verify-blob and verify-blob-attestation #3796

Conversation

steiza commented Jul 23, 2024

Summary

Release Note

Documentation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment • edited Loading

Choose a reason for hiding this comment

steiza commented Jul 29, 2024

Add new bundle support to `verify-blob` and `verify-blob-attestation` #3796

Add new bundle support to `verify-blob` and `verify-blob-attestation` #3796

haydentherapper Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading

haydentherapper left a comment •

edited

Loading