Skip to content

cosign doesn't correctly respect individual TSA certificate chains #4098

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.

Sign up for GitHub

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

Jump to bottom
Open
bkabrda opened this issue Mar 4, 2025 · 3 comments · May be fixed by #4174
Open

cosign doesn't correctly respect individual TSA certificate chains #4098

bkabrda opened this issue Mar 4, 2025 · 3 comments · May be fixed by #4174
Labels
bug Something isn't working

Comments

@bkabrda
Copy link
Contributor

bkabrda commented Mar 4, 2025

Description

I have a private Sigstore instance, in which I attempted to rotate the TSA certificate chain. When trying to verify any blob/image after the rotation, I get the following error:

$ cosign --verbose verify-blob [email protected] --bundle signed.bundle to-sign --rfc3161-timestamp=timestamp.txt --use-signed-timestamps
Error: unable to load TSA certificates: TSA certificate chain must contain exactly one leaf certificate
main.go:74: error during command execution: unable to load TSA certificates: TSA certificate chain must contain exactly one leaf certificate

This is because the GetTSACerts function in pkg/cosign/tsa.go only expects to parse out a single certificate chain, because of the assumption that these individual chains are provided as individual TUF targets.

However in order to be able to properly rotate the TSA certificate chain, one needs to provide the whole TSA certificate chain as a single TUF target (which is possible and I do it). However, when I attempt to introduce a new TSA chain as a new TUF target and keeping the old one as "Expired", I get the above error when verifying any artifact.

I think the GetTSACerts function should behave more like GetRekorPubs, which returns multiple keys and all of them are considered valid for verification.

I think this will actually work fine in the future with #3844 because of how the timestamp verification function from sigstore-go works. I can submit a PR to fix this if you think it's still worth it before #3844 gets merged - I think it's worth because of private Sigstore deployments that might be slower to upgrade.

Version

2.4.1, but applies to latest main branch as well AFAICS
@bkabrda bkabrda added the bug Something isn't working label Mar 4, 2025
@haydentherapper
Copy link
Contributor

A fix would be welcome, though if it’s a significant change, feel free to not and we will fix it indirectly with Sigstore-go.

@bkabrda
Copy link
Contributor Author

bkabrda commented Mar 5, 2025 •
edited
Loading

@haydentherapper I see two ways to fix this basically:

  • Either changing GetTSACerts to return multiple cert chains (easier but would break the API because it's a public function, so probably not great)
  • Or introducing a new function, e.g. GetAllTSACertChains that would return all valid found TSA certificate chains and using that throughout the codebase; AIUI all of this code will get removed eventually after we fully move to trusted_root.json, so adding a small new function temporarily shouldn't be much of a problem.

I think going for the second option makes more sense (a little more code required, but not breaking API), WDYT?

@haydentherapper
Copy link
Contributor

We don't offer any gurantees around API breakages for Cosign so making a breaking change would be fine. I'm OK with either approach.

@bkabrda bkabrda mentioned this issue Apr 22, 2025
Merged
haydentherapper pushed a commit to sigstore/sigstore that referenced this issue Apr 22, 2025
@bkabrda
Add a Name field to the TargetFile struct (#2068)
8f79f87 
This PR is related to sigstore/cosign#4098 - in order to fix that issue, the cosign code has to access the target name to see if it's one of the "fallbacks" or not (and treat it accordingly as explained in the linked issue).

I really wanted to keep this change contained to cosign codebase itself, but I just couldn't find a way to do this; I think adding this small PR here shouldn't hurt, as it is fully backwards compatible.

Signed-off-by: Slavek Kabrda <[email protected]>
@bkabrda bkabrda linked a pull request Apr 24, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Respect multiple TSA certificate chains in trust root bkabrda/cosign
2 participants
@bkabrda @haydentherapper