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 keyless verification without Fulcio roots #2631

Closed
wants to merge 1 commit into from

Conversation

nsmith5
Copy link
Contributor

@nsmith5 nsmith5 commented Jan 16, 2023

Adds support for verifying keyless signatures from a specificied CA bundle instead of assuming Fulcio and loading its roots.

Fixes #2630

Summary

This changes allows folks to more easily verify signatures from a custom CA (see #2630 for additional discussion)

Release Note

  • Allow verifying signatures from a custom CA without needing leaf certificate

Documentation

Will attach an PR for docs here if folks agree on the parent issue that its worth doing

TODO

The TODO list as this idea is still in discussion etc.

  • Add PR for docs with examples of this kind of verification
  • Add some end to end tests of this (where are these now?)

Fixes sigstore#2630

Signed-off-by: Nathan Smith <nathan@nfsmith.ca>
@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2023

Codecov Report

Merging #2631 (722d104) into main (29360f6) will increase coverage by 2.24%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2631      +/-   ##
==========================================
+ Coverage   30.03%   32.27%   +2.24%     
==========================================
  Files         146      151       +5     
  Lines        9283    10428    +1145     
==========================================
+ Hits         2788     3366     +578     
- Misses       6065     6579     +514     
- Partials      430      483      +53     
Impacted Files Coverage Δ
cmd/cosign/cli/verify/verify.go 25.64% <0.00%> (+3.15%) ⬆️

... and 37 files with indirect coverage changes

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

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

LGTM. Do we also need to update the other verify- commands?

@haydentherapper
Copy link
Contributor

For tests, verify_blob tests are really thorough and should be pretty easy to add this case. You could try to add them in e2e_tests too, but it’s not always clear what’s being tested.

@nsmith5
Copy link
Contributor Author

nsmith5 commented Jan 16, 2023

Yeah limme take a look at the other verify commands and see if they work. If this sound reasonable I'll add e2e tests and some example docs and open this up for review :)

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions
Copy link

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Feb 26, 2023
@dmitris
Copy link
Contributor

dmitris commented Mar 27, 2023

@nsmith5 could we reopen this? (Or would need to open a new PR?). Currently we are using an internal copy of cosign with the code of this PR, would be great to get it merged 😄 Let me know if I can help to move this forward (do some testing etc.)

@haydentherapper
Copy link
Contributor

@dmitris Ive reopened it, would you like to work on it? The remaining changes are adding support for all verify commands and testing

@dmitris
Copy link
Contributor

dmitris commented Mar 27, 2023

@dmitris Ive reopened it, would you like to work on it? The remaining changes are adding support for all verify commands and testing

thanks @haydentherapper I will take a look. Since this PR is off @nsmith5's branch, I'd need to create a new PR to make additional changes, right?

@haydentherapper
Copy link
Contributor

I believe so

Note you should be able to work around the lack of this feature in a number of ways. You can either set up a TUF repository to distribute the chain, or provide both the leaf and the chain.

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@haydentherapper
Copy link
Contributor

Superseded by #2845

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Support "keyless" verification non-Fulcio certificate authorities
4 participants