Updates to Timestamp signing and verification #2499

haydentherapper · 2022-11-30T21:45:45Z

Switch to using the raw signature rather than base64 signature for OCI and blob signing
For blob signing, write only the timestamp to disk, not the LocalSignedPayload (since that's already written with the bundle. I want to avoid conflating the Rekor bundle with the TS)
For blob verification, expect only a timestamp in the file. If you don't pass a bundle, you'll need to also pass the signature by flag
Some nits from the previous PR

Signed-off-by: Hayden Blauzvern [email protected]

Summary

Release Note

Documentation

haydentherapper · 2022-11-30T21:46:20Z

Still working on e2e testing and some more unit tests, but just wanted to get this up early for feedback

codecov-commenter · 2022-11-30T21:48:53Z

Codecov Report

Merging #2499 (a4eb7ba) into main (f7f7f3d) will increase coverage by 0.31%.
The diff coverage is 38.46%.

@@            Coverage Diff             @@
##             main    #2499      +/-   ##
==========================================
+ Coverage   29.62%   29.94%   +0.31%     
==========================================
  Files         139      139              
  Lines        8558     8557       -1     
==========================================
+ Hits         2535     2562      +27     
+ Misses       5666     5640      -26     
+ Partials      357      355       -2

Impacted Files	Coverage Δ
cmd/cosign/cli/options/signblob.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/options/verify.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/policy_init.go	`1.25% <0.00%> (+<0.01%)`	⬆️
cmd/cosign/cli/sign/sign_blob.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/verify.go	`0.00% <0.00%> (ø)`
pkg/cosign/bundle/tsa.go	`100.00% <ø> (ø)`
pkg/cosign/fetch.go	`0.00% <ø> (ø)`
internal/pkg/cosign/tsa/signer.go	`51.42% <50.00%> (-2.31%)`	⬇️
cmd/cosign/cli/verify/verify_blob.go	`49.09% <55.55%> (+3.52%)`	⬆️
pkg/cosign/verify.go	`38.73% <70.83%> (+2.69%)`	⬆️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

haydentherapper · 2022-11-30T22:56:08Z

Now it should be good to go, would like to add some more tests in verify-blob for completeness, but might do that in a follow up.

cmd/cosign/cli/sign/sign_blob.go

hectorj2f · 2022-12-01T16:40:41Z

cmd/cosign/cli/verify/verify_blob_test.go

@@ -552,6 +525,10 @@ func TestVerifyBlob(t *testing.T) {
 				expiredLeafPem, true)},
 			shouldErr: true,
 		},
+		// TODO: Add tests for TSA:


This basically removes tests that we had in place. I am not sure we want to change functionality and remove tests. How does it work if I only sign/verify a blob without using tlog ?

I plan to add some more tests in, but the presence of the bundle is just a way to get the signature and/or other timestamp. If you don't use the tlog, then you must provide the cert/sig the same way you currently do, via --certificate/--signature flags.

This PR should only be adding in tests, and refactoring tests based on the proposed changes.

With these changes, it actually requires less tests since this is only an additional flag that affects verification, rather than modifying codepaths that read in the cert/signature.

internal/pkg/cosign/tsa/signer.go

cmd/cosign/cli/verify/verify_blob.go

haydentherapper · 2022-12-02T20:19:39Z

Verified with a local TSA instance:

# Sign blob and upload to Rekor
cosign sign-blob --timestamp-server-url http://localhost:3000 --rfc3161-timestamp tsaresponse --tlog-upload=true --output-certificate=blob.cert --output-signature=blob.sig --bundle blob.bundle blob

# Verify with local Rekor bundle
cosign verify-blob --rfc3161-timestamp tsaresponse --bundle blob.bundle --timestamp-cert-chain tsacertchain blob

# Verify with skipping checking Rekor
cosign verify-blob --rfc3161-timestamp tsaresponse --timestamp-cert-chain tsacertchain --certificate blob.cert --signature blob.sig --insecure-skip-tlog-verify --offline blob

# Sign blob without Rekor upload
cosign sign-blob --timestamp-server-url http://localhost:3000 --rfc3161-timestamp tsaresponse --tlog-upload=false --output-certificate=blob.cert --output-signature=blob.sig  blob

An unrelated issue is if I skip tlog-upload, then I can't output a certificate (cosign sign-blob --timestamp-server-url http://localhost:3000 --rfc3161-timestamp tsaresponse --tlog-upload=false --output-certificate=blob.cert --output-signature=blob.sig blob fails to output a cert), so I can't verify. I'll fix this separately.

hectorj2f

LGTM, I added some nit comments. Ideally, I'd like to see some tests to validate the usage of the rfc3161timestamp with blobs.

cmd/cosign/cli/options/verify.go

cmd/cosign/cli/sign/sign_blob.go

specs/SIGNATURE_SPEC.md

cmd/cosign/cli/verify/verify_blob.go

znewman01

Some nits but overall great changes

cmd/cosign/cli/sign/sign_blob.go

internal/pkg/cosign/tsa/signer.go

pkg/cosign/verify.go

cmd/cosign/cli/sign/sign_blob.go

cmd/cosign/cli/verify/verify_blob.go

cmd/cosign/cli/sign/sign_blob.go

znewman01 · 2022-12-06T21:58:47Z

cmd/cosign/cli/sign/sign_blob.go

+		// TODO: Consider uploading RFC3161 TS to Rekor
+
+		if rfc3161Timestamp == nil {
+			return nil, fmt.Errorf("rfc3161 timestamp is nil")


Should we do these checks before signing etc.?

This is a just-in-case check, since TimestampToRFC3161Timestamp might return nil if respBytes is empty for some reason.

Yeah. Maybe both wouldn't be a bad idea, up to you

znewman01 · 2022-12-06T22:21:27Z

cmd/cosign/cli/sign/sign_blob.go

+		// TODO: Consider uploading RFC3161 TS to Rekor
+
+		if rfc3161Timestamp == nil {
+			return nil, fmt.Errorf("rfc3161 timestamp is nil")


Yeah. Maybe both wouldn't be a bad idea, up to you

* Switch to using the raw signature rather than base64 signature for OCI and blob signing * For blob signing, write only the timestamp to disk, not the LocalSignedPayload (since that's already written with the bundle) * For blob verification, expect only a timestamp in the file. If you don't pass a bundle, you'll need to also pass the signature by flag * Some nits from the previous PR Signed-off-by: Hayden Blauzvern <[email protected]>

Signed-off-by: Hayden Blauzvern <[email protected]>

hectorj2f

lgtm

haydentherapper requested review from znewman01, hectorj2f and asraa November 30, 2022 21:45

haydentherapper changed the title ~~WIP: Updates to Timestamp signing and verification~~ Updates to Timestamp signing and verification Nov 30, 2022

haydentherapper mentioned this pull request Nov 30, 2022

Attestation/Blob signing and verification using a RFC3161 time-stamping server #2464

Merged

hectorj2f reviewed Dec 1, 2022

View reviewed changes

cmd/cosign/cli/sign/sign_blob.go Show resolved Hide resolved

hectorj2f reviewed Dec 1, 2022

View reviewed changes

internal/pkg/cosign/tsa/signer.go Show resolved Hide resolved

hectorj2f reviewed Dec 1, 2022

View reviewed changes

cmd/cosign/cli/verify/verify_blob.go Show resolved Hide resolved

haydentherapper force-pushed the fix-tsa branch from 2ee4d62 to a0888e1 Compare December 2, 2022 20:06

hectorj2f reviewed Dec 4, 2022

View reviewed changes

cmd/cosign/cli/options/verify.go Outdated Show resolved Hide resolved

cmd/cosign/cli/sign/sign_blob.go Show resolved Hide resolved

specs/SIGNATURE_SPEC.md Outdated Show resolved Hide resolved

cmd/cosign/cli/verify/verify_blob.go Show resolved Hide resolved

haydentherapper mentioned this pull request Dec 5, 2022

Cosign 2.0 Tracking #2376

Closed

3 tasks

znewman01 reviewed Dec 5, 2022

View reviewed changes

cmd/cosign/cli/sign/sign_blob.go Outdated Show resolved Hide resolved

internal/pkg/cosign/tsa/signer.go Show resolved Hide resolved

internal/pkg/cosign/tsa/signer.go Show resolved Hide resolved

pkg/cosign/verify.go Show resolved Hide resolved

haydentherapper commented Dec 6, 2022

View reviewed changes

cmd/cosign/cli/sign/sign_blob.go Outdated Show resolved Hide resolved

haydentherapper mentioned this pull request Dec 6, 2022

Add oci.Signature() method to expose raw signature #2517

Closed

haydentherapper force-pushed the fix-tsa branch from a0888e1 to 5dd2425 Compare December 6, 2022 21:52

znewman01 previously approved these changes Dec 6, 2022

View reviewed changes

haydentherapper dismissed znewman01’s stale review via f529c74 December 6, 2022 23:04

znewman01 previously approved these changes Dec 6, 2022

View reviewed changes

haydentherapper added 7 commits December 6, 2022 23:18

Add testing in verify, update e2e test

cb9c9cb

Signed-off-by: Hayden Blauzvern <[email protected]>

Fix test

56fb38c

Signed-off-by: Hayden Blauzvern <[email protected]>

Add missing setting of SkipTlogVerify

5f8e8bd

Signed-off-by: Hayden Blauzvern <[email protected]>

Address comments

5acd7f6

Signed-off-by: Hayden Blauzvern <[email protected]>

Fix docs

6fb95c9

Signed-off-by: Hayden Blauzvern <[email protected]>

Address nits

ee0f511

Signed-off-by: Hayden Blauzvern <[email protected]>

Fix e2e test

ae14708

Signed-off-by: Hayden Blauzvern <[email protected]>

haydentherapper dismissed znewman01’s stale review via ae14708 December 6, 2022 23:20

haydentherapper force-pushed the fix-tsa branch from f529c74 to ae14708 Compare December 6, 2022 23:20

znewman01 approved these changes Dec 6, 2022

View reviewed changes

hectorj2f approved these changes Dec 7, 2022

View reviewed changes

hectorj2f merged commit e9e9cb2 into sigstore:main Dec 7, 2022

github-actions bot added this to the v1.14.0 milestone Dec 7, 2022

haydentherapper deleted the fix-tsa branch December 7, 2022 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates to Timestamp signing and verification #2499

Updates to Timestamp signing and verification #2499

haydentherapper commented Nov 30, 2022 •

edited

Loading

haydentherapper commented Nov 30, 2022

codecov-commenter commented Nov 30, 2022 •

edited

Loading

haydentherapper commented Nov 30, 2022 •

edited

Loading

hectorj2f Dec 1, 2022

haydentherapper Dec 1, 2022 •

edited

Loading

haydentherapper Dec 1, 2022

haydentherapper commented Dec 2, 2022

hectorj2f left a comment

znewman01 left a comment

znewman01 Dec 6, 2022

haydentherapper Dec 6, 2022

znewman01 Dec 6, 2022

znewman01 Dec 6, 2022

hectorj2f left a comment

Updates to Timestamp signing and verification #2499

Updates to Timestamp signing and verification #2499

Conversation

haydentherapper commented Nov 30, 2022 • edited Loading

Summary

Release Note

Documentation

haydentherapper commented Nov 30, 2022

codecov-commenter commented Nov 30, 2022 • edited Loading

Codecov Report

haydentherapper commented Nov 30, 2022 • edited Loading

hectorj2f Dec 1, 2022

Choose a reason for hiding this comment

haydentherapper Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

haydentherapper Dec 1, 2022

Choose a reason for hiding this comment

haydentherapper commented Dec 2, 2022

hectorj2f left a comment

Choose a reason for hiding this comment

znewman01 left a comment

Choose a reason for hiding this comment

znewman01 Dec 6, 2022

Choose a reason for hiding this comment

haydentherapper Dec 6, 2022

Choose a reason for hiding this comment

znewman01 Dec 6, 2022

Choose a reason for hiding this comment

znewman01 Dec 6, 2022

Choose a reason for hiding this comment

hectorj2f left a comment

Choose a reason for hiding this comment

haydentherapper commented Nov 30, 2022 •

edited

Loading

codecov-commenter commented Nov 30, 2022 •

edited

Loading

haydentherapper commented Nov 30, 2022 •

edited

Loading

haydentherapper Dec 1, 2022 •

edited

Loading