Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
add --ca-roots and --ca-intermediates flags to 'cosign verify' #3464
add --ca-roots and --ca-intermediates flags to 'cosign verify' #3464
Changes from 11 commits
3663f23
b6b9371
e333eb8
2ecf4c3
c5ca7b4
8131308
701bad7
da3779d
ee0ece0
70ed9f9
c437f56
ec1abdf
f1be17d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm less familiar with cosign so please take this with a grain of salt, but: I'm personally -1 on separate state flags for intermediate and root CA certificates.
My rationale:
--trusted-root
or similar flag, which will load a shared format for the roots of trust for an "entire" Sigstore deployment (including the Rekor key, CTFE key, etc.). While this doesn't preclude that effort, it does add another set of knobs that will eventually need to be deprecated (or treated as another source of state).cosign
) use Sigstore bundles as their interchange format. Bundles in turn support untrusted intermediates for the BYO PKI case, and in an unambiguous fashion (everything in the bundle is presumed untrusted).So, TL;DR: if the idea behind
--ca-intermediates
is for it to pass trusted intermediates, then IMO it should be removed and collapsed into--ca-roots
(which IMO could be called--trusted-certs
to minimize confusion).On the other hand, if these certificates are meant to be untrusted I can see the value of this flag, but I think it might be somewhat short-lived given the trustroot + bundle progression. But maybe that isn't as far along as I think 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For public good Sigstore, the intermediate is "trusted", as it is provided via the TUF trust root, and it's part of the trusted_root.json, and for public good we do not allow intermediates not provided direct via the trust root.
But if
cosign
is to be used with other deployments, they can be untrusted. So with that in mind, and the bundle format is not yet supported in cosign (to carry untrusted intermediates) I think the current proposal makes sense, but it can feel a bit awkward for the public good instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@woodruffw thanks for your review and suggestions!
Well, for example looking at the Fulcio Certificate Specification, it has subsections for Root, Intermediate, and Issued Certificates, with specifications of what each type of certificate
MUST
/MUST NOT
/SHOULD
have. Also, for better or worth, a search forintermediate
brings up over 140 hits in thesigstore/cosign
codebase, including in the existing options forcosign verify
:also over 60 matches have both
intermediate
androot
in the same line so these terms seems both well established and useful in thecosign
repo and code. Maybe it is due to my 'surrounding environment' but I don't remember any cases where I or people around would have difficulties distinguishing which (CA) certificates are root ones, and which - intermediates.I have to admit having difficulties understanding which certificates are (or should be) considered "trusted" and "untrusted" in cosign (there is only one search hit on
untrusted
in sigstore/cosign and it is to a CHANGELOG in #2124; the only search hit onuntrusted
in https://docs.sigstore.dev also is not very helpful). I think the certificate trust is so much context-dependant. For example, I'm used to think of the certificates issues by ourTheCompanyCA
as both fully trusted and more trustworthy than those from Digicert & Co. 😄 - while in an environment oriented to the public websites and Certificate Authorities it would likely be considered as "Self-Signed" and untrusted.Regarding the points 2. and 3. ("use a common --trusted-root flag" and Sigstore bundles as the interchange format) - in the long term, I have some reservations regarding this in terms of the ease of use and Developer / DevOps Experience. I worry that they would become custom requirements and make hurdles to using Sigstore for signing artifacts. PEM certificate bundles are "universal", ubiquitous and extremely familiar to everyone working with them and also deeply interwined with the existing scripts, processes etc. The TUF roots / Sigstore bundles are (IMO) in comparison relatively new, more complex (PEM -> JSON -> ...), "special purpose" and require custom handling, adoption steps / timeframe and increase the adoption and operational friction, especially for large users. Undoubtedly, the change will make life easier for us as the sigstore developers and the codebase probably nicer ("just one file/format to rule them all!") but possibly at the expense of placing some extra burden on the end users ("in addition to distributing the
ca-bundle.pem
to 100,000 endpoints that you are doing now and must continue to do for many years, you also need to create the file and distribute it to all the endpoints that will or might need to verify cosign signatures"). So I would recommend and plead 😄 to consider the trade-off carefully and if decided, let the users know well in advance about the upcoming change(s) and provide for an ample transition period (maybe something like one major version during which one could use both old and new ways of supplying certificates) 🙏 /cc @haydentherapperBut as you said, this would be long-term changes - and currently we have an "acute" issue of not being able to (properly) verify cosign signatures with
cosign verify
and through the cosign Go library if there is more than one CA authority issuing certificates - namely using a certificate bundle PEM file that contains multiple CA Root certificates. (And we would like to start issuing and verifying lots of Sigstore/Cosign signatures in Q1 '24! 😁).The suggested
cosign verify --ca-roots <certbundle.pem>
and the expanded CertVerifyOptions and VerifyCommand in the Go library would solve that problem. If you have other ideas how we could do this (in the near term), please let me know! (I thought about expanding the current--certificate-chain
format or adding--certificate-chains
[plural], but couldn't figure out how to handle the file format - with the single chain, it is easy to parse the file [<Intermediate>*<Root>
] but if there are multiple roots, it gets very messy.... I also didn't want to get into the business of building multiple certificate chains from a set of certificates - the Go stdlib already does it very well but it requires providingRoots
and optionallyIntermediates
in the VerifyOptions mentioned above - to which the two new suggested flags would correspond.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for raising this issue back up, but to summarize how I'm looking at this:
--trusted-root
is the route we eventually want to take. We are slowly adding sigstore-go dependencies in Cosign. Once we add support for trusted-root, we will deprecate all other options for providing trust root information, including the one proposed here. With that said, deprecation will take time.I'm fine moving forward with this because it unblocks @dmitris's use-case. While it is adding more complexity around how roots are provided, there are already a number of ways that adding one more doesn't add much more complexity. We should concurrently discuss moving sigstore/protobuf-specs#249 forward.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.