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

add --ca-roots and --ca-intermediates flags to 'cosign verify' #3464

Merged
merged 13 commits into from
Jul 1, 2024
17 changes: 16 additions & 1 deletion cmd/cosign/cli/options/certificate.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ type CertVerifyOptions struct {
CertGithubWorkflowName string
CertGithubWorkflowRepository string
CertGithubWorkflowRef string
CAIntermediates string
CARoots string
CertChain string
SCT string
IgnoreSCT bool
Expand Down Expand Up @@ -75,12 +77,25 @@ func (o *CertVerifyOptions) AddFlags(cmd *cobra.Command) {
cmd.Flags().StringVar(&o.CertGithubWorkflowRef, "certificate-github-workflow-ref", "",
"contains the ref claim from the GitHub OIDC Identity token that contains the git ref that the workflow run was based upon.")
// -- Cert extensions end --
cmd.Flags().StringVar(&o.CAIntermediates, "ca-intermediates", "",
"path to a file of intermediate CA certificates in PEM format which will be needed "+
"when building the certificate chains for the signing certificate. "+
"The flag is optional and must be used together with --ca-roots, conflicts with "+
"--certificate-chain.")
_ = cmd.Flags().SetAnnotation("ca-intermediates", cobra.BashCompFilenameExt, []string{"cert"})
cmd.Flags().StringVar(&o.CARoots, "ca-roots", "",
dmitris marked this conversation as resolved.
Show resolved Hide resolved
"path to a bundle file of CA certificates in PEM format which will be needed "+
"when building the certificate chains for the signing certificate. Conflicts with --certificate-chain.")
dmitris marked this conversation as resolved.
Show resolved Hide resolved
_ = cmd.Flags().SetAnnotation("ca-roots", cobra.BashCompFilenameExt, []string{"cert"})

Comment on lines +80 to +90
Copy link
Member

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:

  1. Technically speaking, there is no real distinction between a "root" and an "intermediate" certificate: some people use "root" to mean "self-signed," but it's common to have both non-self-signed and cross-issued roots in real PKIs. In other contexts "intermediate" means "part of the untrusted set", but it's not immediately clear to me whether intermediates are trusted or untrusted in cosign (CC @haydentherapper)
  2. Long term, I believe the plan is to have most Sigstore clients use a common --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).
  3. Similarly long-term, I believe the plan is to have most Sigstore clients (including 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 😅

Copy link
Member

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.

Copy link
Contributor Author

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!

Technically speaking, there is no real distinction between a "root" and an "intermediate" certificate

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 for intermediate brings up over 140 hits in the sigstore/cosign codebase, including in the existing options for cosign verify:

doc/cosign_verify.md: --certificate-chain string path to a list of CA certificates in PEM format which will be needed when building the certificate chain for the signing certificate. Must start with the parent intermediate CA certificate of the signing certificate and end with the root certificate

also over 60 matches have both intermediate and root in the same line so these terms seems both well established and useful in the cosign 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.

In other contexts "intermediate" means "part of the untrusted set", but it's not immediately clear to me whether intermediates are trusted or untrusted in cosign
[...]
But if cosign is to be used with other deployments, they can be untrusted.

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 on untrusted 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 our TheCompanyCA 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 @haydentherapper

But 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 providing Roots and optionally Intermediates in the VerifyOptions mentioned above - to which the two new suggested flags would correspond.)

Copy link
Contributor

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:

  • We know --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.
  • The current trust root spec supports chains rather than pools, as noted in this issue. We need to add support for pools to match this PR.

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.

cmd.Flags().StringVar(&o.CertChain, "certificate-chain", "",
"path to a list of CA certificates in PEM format which will be needed "+
"when building the certificate chain for the signing certificate. "+
"Must start with the parent intermediate CA certificate of the "+
"signing certificate and end with the root certificate")
"signing certificate and end with the root certificate. Conflicts with --ca-roots and --ca-intermediates.")
_ = cmd.Flags().SetAnnotation("certificate-chain", cobra.BashCompFilenameExt, []string{"cert"})
cmd.MarkFlagsMutuallyExclusive("ca-roots", "certificate-chain")
cmd.MarkFlagsMutuallyExclusive("ca-intermediates", "certificate-chain")

cmd.Flags().StringVar(&o.SCT, "sct", "",
"path to a detached Signed Certificate Timestamp, formatted as a RFC6962 AddChainResponse struct. "+
Expand Down
6 changes: 6 additions & 0 deletions cmd/cosign/cli/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ against the transparency log.`,
# verify image with local certificate and certificate chain
cosign verify --cert cosign.crt --cert-chain chain.crt <IMAGE>

# verify image with local certificate and certificate bundles of CA roots
# and (optionally) CA intermediates
cosign verify --cert cosign.crt --ca-roots ca-roots.pem --ca-intermediates ca-intermediates.pem <IMAGE>

# verify image using keyless verification with the given certificate
# chain and identity parameters, without Fulcio roots (for BYO PKI):
cosign verify --cert-chain chain.crt --certificate-oidc-issuer https://issuer.example.com --certificate-identity [email protected] <IMAGE>
Expand Down Expand Up @@ -115,6 +119,8 @@ against the transparency log.`,
CertGithubWorkflowName: o.CertVerify.CertGithubWorkflowName,
CertGithubWorkflowRepository: o.CertVerify.CertGithubWorkflowRepository,
CertGithubWorkflowRef: o.CertVerify.CertGithubWorkflowRef,
CAIntermediates: o.CertVerify.CAIntermediates,
CARoots: o.CertVerify.CARoots,
CertChain: o.CertVerify.CertChain,
IgnoreSCT: o.CertVerify.IgnoreSCT,
SCTRef: o.CertVerify.SCT,
Expand Down
53 changes: 46 additions & 7 deletions cmd/cosign/cli/verify/verify.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ type VerifyCommand struct {
CertGithubWorkflowName string
CertGithubWorkflowRepository string
CertGithubWorkflowRef string
CAIntermediates string
dmitris marked this conversation as resolved.
Show resolved Hide resolved
CARoots string
CertChain string
CertOidcProvider string
IgnoreSCT bool
Expand Down Expand Up @@ -173,7 +175,8 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
}
}
if keylessVerification(c.KeyRef, c.Sk) {
dmitris marked this conversation as resolved.
Show resolved Hide resolved
if c.CertChain != "" {
switch {
case c.CertChain != "":
chain, err := loadCertChainFromFileOrURL(c.CertChain)
if err != nil {
return err
Expand All @@ -186,9 +189,32 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
co.IntermediateCerts.AddCert(cert)
}
}
} else {
// This performs an online fetch of the Fulcio roots. This is needed
// for verifying keyless certificates (both online and offline).
case c.CARoots != "":
caRoots, err := loadCertChainFromFileOrURL(c.CARoots)
if err != nil {
return err
}
co.RootCerts = x509.NewCertPool()
if len(caRoots) > 0 {
dmitris marked this conversation as resolved.
Show resolved Hide resolved
for _, cert := range caRoots {
co.RootCerts.AddCert(cert)
}
}
if c.CAIntermediates != "" {
caIntermediates, err := loadCertChainFromFileOrURL(c.CAIntermediates)
if err != nil {
return err
}
if len(caIntermediates) > 0 {
co.IntermediateCerts = x509.NewCertPool()
for _, cert := range caIntermediates {
co.IntermediateCerts.AddCert(cert)
}
}
}
default:
// This performs an online fetch of the Fulcio roots from a TUF repository.
// This is needed for verifying keyless certificates (both online and offline).
co.RootCerts, err = fulcio.GetRoots()
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
Expand Down Expand Up @@ -237,8 +263,9 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
if err != nil {
return err
}
if c.CertChain == "" {
// If no certChain is passed, the Fulcio root certificate will be used
switch {
case c.CertChain == "" && co.RootCerts == nil:
// If no certChain and no CARoots are passed, the Fulcio root certificate will be used
co.RootCerts, err = fulcio.GetRoots()
if err != nil {
return fmt.Errorf("getting Fulcio roots: %w", err)
Expand All @@ -251,7 +278,7 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
if err != nil {
return err
}
} else {
case c.CertChain != "":
// Verify certificate with chain
chain, err := loadCertChainFromFileOrURL(c.CertChain)
if err != nil {
Expand All @@ -261,14 +288,26 @@ func (c *VerifyCommand) Exec(ctx context.Context, images []string) (err error) {
if err != nil {
return err
}
case co.RootCerts != nil:
// Verify certificate with root (and if given, intermediate) certificate
pubKey, err = cosign.ValidateAndUnpackCert(cert, co)
if err != nil {
return err
}
default:
return errors.New("internal error in handling CertChain and RootCerts - default case should never happen")
dmitris marked this conversation as resolved.
Show resolved Hide resolved
}

if c.SCTRef != "" {
sct, err := os.ReadFile(filepath.Clean(c.SCTRef))
if err != nil {
return fmt.Errorf("reading sct from file: %w", err)
}
co.SCT = sct
}
default:
// Do nothing. Neither keyRef, c.Sk, nor certRef were set - can happen for example when using Fulcio and TSA.
// For an example see the TestAttachWithRFC3161Timestamp test in test/e2e_test.go.
}
co.SigVerifier = pubKey

Expand Down
4 changes: 3 additions & 1 deletion doc/cosign_dockerfile_verify.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion doc/cosign_manifest_verify.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion doc/cosign_verify-attestation.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion doc/cosign_verify-blob-attestation.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion doc/cosign_verify-blob.md

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading