Skip to content

Respect multiple TSA certificate chains in trust root #4174

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bkabrda
Copy link
Contributor

@bkabrda bkabrda commented Apr 24, 2025

Note: right now the CI is failing, because this PR needs updating sigstore/sigstore to a version containing sigstore/sigstore#2068 which wasn't released yet. I'm not sure if the best course is to update the dependency to the specific commit or if it's better to wait until a new version is released - please let me know.

Summary

Fixes #4098 - cosign will now respect multiple TSA certificate chains provided through the trust root and will try to verify the signed timestamp with all of them (and fail if none of the TSA chains verify the signed timestamp).

Release Note

Support for multiple TSA certificate chains in the trust root was added.

Documentation

I don't believe this requires a documentation change.

@bkabrda bkabrda requested a review from a team as a code owner April 24, 2025 09:49
@bkabrda bkabrda force-pushed the tsa-multiple-certchains branch 2 times, most recently from 126a3fc to 608b4cb Compare April 24, 2025 09:51
@haydentherapper
Copy link
Contributor

You can use the sigstore/sigstore version at the commit.

@haydentherapper
Copy link
Contributor

@bkabrda bkabrda requested a review from a team as a code owner April 25, 2025 07:54
@bkabrda bkabrda force-pushed the tsa-multiple-certchains branch from 77145cf to 73d343d Compare April 25, 2025 07:57
Copy link

codecov bot commented Apr 25, 2025

Codecov Report

Attention: Patch coverage is 54.38596% with 26 lines in your changes missing coverage. Please review.

Project coverage is 36.46%. Comparing base (2ef6022) to head (6a23a02).
Report is 371 commits behind head on main.

Files with missing lines Patch % Lines
pkg/cosign/tsa.go 44.73% 18 Missing and 3 partials ⚠️
pkg/cosign/verify.go 86.66% 2 Missing ⚠️
cmd/cosign/cli/verify/verify.go 0.00% 1 Missing ⚠️
cmd/cosign/cli/verify/verify_attestation.go 0.00% 1 Missing ⚠️
cmd/cosign/cli/verify/verify_blob_attestation.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4174      +/-   ##
==========================================
- Coverage   40.10%   36.46%   -3.64%     
==========================================
  Files         155      210      +55     
  Lines       10044    13762    +3718     
==========================================
+ Hits         4028     5019     +991     
- Misses       5530     8120    +2590     
- Partials      486      623     +137     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Slavek Kabrda <[email protected]>
@bkabrda
Copy link
Contributor Author

bkabrda commented Apr 25, 2025

@haydentherapper thanks for the quick release of the sigstore library. I updated the PR to incorporate that, but I noticed that one e2e test is failing; it looks like it might be related to my change, so I'll try to take a look at it later today to see what's going wrong.

@bkabrda
Copy link
Contributor Author

bkabrda commented Apr 25, 2025

Ok, I understand what's going wrong; in the "nonstandard key names with valid usage", it is assumed that even if multiple targets with "Usage: TSA" are used, they are still just parts of one certificate chain, but my code change expects these to be full certificate chains and therefore it fails on parsing them as such.

Even though I dislike the behavior, I think technically my change is breaking it and so probably shouldn't be merged... WDYT @haydentherapper? I currently don't see a fully backwards compatible way of making this work with the "old-style" tuf targets and so probably the best way to go would be to wait for the trusted_root.json support to get merged.

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.

cosign doesn't correctly respect individual TSA certificate chains
2 participants