Skip to content
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

Support non-Sigstore TSA requests #2708

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

haydentherapper
Copy link
Contributor

This changes the client to instead simply exchange a TSQ for a TSR given a URL to the API. This will mean that for clients using timestamping currently, they will need to update their Cosign calls to use the full path (e.g. tsa.sigstore/api/v1/timestamp)

Fixes #2704

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

Summary

Release Note

Documentation

This changes the client to instead simply exchange a TSQ for a TSR given
a URL to the API. This will mean that for clients using timestamping
currently, they will need to update their Cosign calls to use the full
path (e.g. tsa.sigstore/api/v1/timestamp)

Fixes sigstore#2704

Signed-off-by: Hayden Blauzvern <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #2708 (339934e) into main (01bd21d) will increase coverage by 0.01%.
The diff coverage is 21.42%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #2708      +/-   ##
==========================================
+ Coverage   30.15%   30.16%   +0.01%     
==========================================
  Files         146      150       +4     
  Lines        9363     9438      +75     
==========================================
+ Hits         2823     2847      +24     
- Misses       6103     6156      +53     
+ Partials      437      435       -2     
Impacted Files Coverage Δ
cmd/cosign/cli/attest/attest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/attest/attest_blob.go 30.12% <0.00%> (+0.75%) ⬆️
cmd/cosign/cli/options/attest.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/attest_blob.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/policy.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/sign.go 0.00% <0.00%> (ø)
cmd/cosign/cli/options/signblob.go 0.00% <0.00%> (ø)
cmd/cosign/cli/policy_init.go 1.24% <0.00%> (+0.01%) ⬆️
cmd/cosign/cli/sign/sign.go 15.00% <0.00%> (+0.11%) ⬆️
cmd/cosign/cli/sign/sign_blob.go 0.00% <0.00%> (ø)
... and 11 more

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

@haydentherapper
Copy link
Contributor Author

Will test PR in the morning

@haydentherapper haydentherapper marked this pull request as draft February 10, 2023 09:21
Copy link
Contributor

@hectorj2f hectorj2f left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add tests against these non-sigstore supported TSAs ?

internal/pkg/cosign/tsa/client/client.go Outdated Show resolved Hide resolved
internal/pkg/cosign/tsa/client/client.go Outdated Show resolved Hide resolved
Signed-off-by: Hayden Blauzvern <[email protected]>
@haydentherapper haydentherapper marked this pull request as ready for review February 10, 2023 19:46
@haydentherapper
Copy link
Contributor Author

Tested with Sigstore TSA:

<started up local TSA, copied cert chain>

cosign sign-blob --timestamp-server-url http://localhost:3000/api/v1/timestamp --rfc3161-timestamp tsaresponse --tlog-upload=false --bundle=blob.bundle --output-certificate=blob.cert  blob --yes

cosign verify-blob --rfc3161-timestamp tsaresponse --timestamp-certificate-chain tsacertchain  --certificate-identity <email> --certificate-oidc-issuer https://accounts.google.com --insecure-ignore-tlog --bundle blob.bundle --offline --certificate blob.cert blob

Tested with FreeTSA:

cosign sign-blob --timestamp-server-url https://freetsa.org/tsr --rfc3161-timestamp tsaresponse --tlog-upload=false --bundle=blob.bundle --output-certificate=blob.cert  blob --yes

# Grabbed the tsa chain from the website

cosign verify-blob --rfc3161-timestamp tsaresponse --timestamp-certificate-chain freetsachain  --certificate-identity <email> --certificate-oidc-issuer https://accounts.google.com --insecure-ignore-tlog --bundle blob.bundle --offline --certificate blob.cert blob

@hectorj2f I don't think we should add a non-sigstore TSA to the e2e tests because that's adding an external dependency.

@hectorj2f
Copy link
Contributor

@hectorj2f I don't think we should add a non-sigstore TSA to the e2e tests because that's adding an external dependency.

How would we know we didn't break it ?

Also, should we document the list of supported non-TSAs ?

@haydentherapper
Copy link
Contributor Author

We just send the timestamp query to the timestamp authority, any TSA should work. We are no longer treating the sigstore TSA as different, so if we break something, we'll notice it in the e2e tests.

@haydentherapper
Copy link
Contributor Author

I also want to make sure we're not describing the sigstore implementation of a TSA as something special. It's just an server implementation of rfc3161, there should be no differences in the accepted API requests and responses to any TSA implementation. This PR just changes the client so that you have to pass the path to fetch a TSR, whereas previously the sigstore tsa client knew the path.

@hectorj2f
Copy link
Contributor

Sgtm

@haydentherapper
Copy link
Contributor Author

This should be good to merge.

@hectorj2f
Copy link
Contributor

Yes, let's merge it. We need to change the clients to adapt to these changes :).

@haydentherapper
Copy link
Contributor Author

Do we know who's using this? This is just in the 2.0 RC so I assume we only need to update the blog post you published?

@hectorj2f
Copy link
Contributor

@haydentherapper I was thinking the policy-controller, but the verification isn't affected by this. On the other hand, we use the signing piece in a different component, so whenever we release a new RC, I'll change our client calls.

@haydentherapper
Copy link
Contributor Author

Ah, didn't realize it was used there. Would this only affect deployments of policy-controller? I assume there's no hardcoded TSA APIs, no other code should need to be updated

@haydentherapper
Copy link
Contributor Author

Bump, should be ready for merging.

@priyawadhwa priyawadhwa merged commit d15ed74 into sigstore:main Feb 16, 2023
@haydentherapper haydentherapper deleted the tsa-change branch February 16, 2023 17:00
@github-actions github-actions bot added this to the v1.14.0 milestone Feb 16, 2023
dmitris pushed a commit to dmitris/cosign that referenced this pull request Mar 24, 2023
* Support non-Sigstore TSA requests

This changes the client to instead simply exchange a TSQ for a TSR given
a URL to the API. This will mean that for clients using timestamping
currently, they will need to update their Cosign calls to use the full
path (e.g. tsa.sigstore/api/v1/timestamp)

Fixes sigstore#2704

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

* Address comments

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

---------

Signed-off-by: Hayden Blauzvern <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support non-Sigstore TSAs
5 participants