Fix bug in attest-blob when using a timestamp authority with new bundles

[steiza]

Summary

Fix bug in #3752

When adding bundle support to attest-blob, we sent the wrong data to the timestamp authority to sign.

You can test this by first attesting to a blob with a timestamp authority and a predicate with something like:

$ go run cmd/cosign/main.go attest-blob --bundle="attest-pgi.sigstore.json" --new-bundle-format=true --key=cosign.key --tlog-upload=false --timestamp-server-url="https://timestamp.githubapp.com/api/v1/timestamp" --tlog-upload=false --type=something --predicate="../sigstore-go/examples/sigstore-go-signing/intoto.txt" ../sigstore-go/examples/sigstore-go-signing/hello_world.txt

Then the verification which previously failed (and now succeeds) looks like:

$ go run cmd/cosign/main.go verify-blob-attestation --trusted-root=trusted_root_with_timestamps.public.json --use-signed-timestamps=true --insecure-ignore-tlog=true --key=cosign.pub --bundle="attest-pgi.sigstore.json" --new-bundle-format=true --type=something ../sigstore-go/examples/sigstore-go-signing/hello_world.txt

Release Note

• Fixed bug that created bundles with incorrect signed timestamps when attest-blob was used with new bundles

Documentation

N/A

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 36 lines in your changes missing coverage. Please review.

Project coverage is 36.60%. Comparing base (2ef6022) to head (004061c).
Report is 217 commits behind head on main.

Files with missing lines	Patch %	Lines
cmd/cosign/cli/attest/attest_blob.go	0.00%	28 Missing ⚠️
cmd/cosign/cli/attest/attest.go	0.00%	8 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3877      +/-   ##
==========================================
- Coverage   40.10%   36.60%   -3.50%     
==========================================
  Files         155      203      +48     
  Lines       10044    12811    +2767     
==========================================
+ Hits         4028     4690     +662     
- Misses       5530     7538    +2008     
- Partials      486      583      +97     

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

[haydentherapper]

Can you also fix

cosign/cmd/cosign/cli/attest/attest.go

Line 179 in 780780b

responseBytes, err := tsa.GetTimestampedSignature(signedPayload, tsaclient.NewTSAClient(c.KeyOpts.TSAServerURL))

, which I believe has the same issue?

[haydentherapper]

Could you test that this doesn't break the existing verification flows for verify-attestation and verify-blob-attestation without using sigstore-go? I think we might have overloaded sig in a few places such that the verifier is expecting the DSSE envelope to have been signed over rather than the DSSE's sig, but I haven't dug in closely yet.

[haydentherapper]

Grumble grumble, this will affect verification for non-bundle flows, see

cosign/pkg/cosign/verify.go

Lines 1140 to 1154 in 780780b

	if len(b64Sig) == 0 {
	// For attestations, the Base64Signature is not set, therefore we rely on the signed payload
	signedPayload, err := sig.Payload()
	if err != nil {
	return nil, fmt.Errorf("reading the payload: %w", err)
	}
	tsBytes = signedPayload
	} else {
	// create timestamp over raw bytes of signature
	rawSig, err := base64.StdEncoding.DecodeString(b64Sig)
	if err != nil {
	return nil, err
	}
	tsBytes = rawSig
	}

. We verify using either the signature or the signed payload.

I think the cleanest solution would be to change this only for the case where you provide the new bundle flag.

[haydentherapper]

LGTM, thanks! Just a comment about a test.

[haydentherapper]

Can we add a test under e2e_test as well? A copy of

cosign/test/e2e_test.go

Line 771 in 081dea1

func TestAttestationRFC3161Timestamp(t *testing.T) {

that uses AttestBlobCommand?

[steiza]

Sure! This would have been a lot harder if I had not written #3876 recently 😅

[haydentherapper]

Thanks!