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

Allow specifying CA chain when not using Fulcio #1554

Closed
bburky opened this issue Mar 4, 2022 · 33 comments
Closed

Allow specifying CA chain when not using Fulcio #1554

bburky opened this issue Mar 4, 2022 · 33 comments
Labels
enhancement New feature or request

Comments

@bburky
Copy link
Contributor

bburky commented Mar 4, 2022

When using a local PEM key file, a PKCS#11 key, or a KMS key a user should be able to specify --cert and something like --chain to specify the dev.sigstore.cosign/certificate and dev.sigstore.cosign/chain.

The following already works to specify the certificate, and correctly adds it to the cosign signature:

cosign sign --key awskms:///... --cert cert.pem example.com/foo

However, --cert is silently ignored in the following command (the certificate from the token is used instead of the provided one): (this is a separate bug really, should I create a new issue for it?)

cosign sign --key pkcs11:... --cert cert.pem example.com/foo

It is not possible with either of these to specify the CA chain. If my certificate is signed by existing PKI, I would like to include the intermediate certificates and root in the signature.

Two possible implementations would be:

  1. Add a new argument like --chain which includes a chain of certificates.
  2. Allow the --cert file to include both the leaf cert, and a chain of intermediates.
    • One issue with this is that a user may want to use the public key on the PKCS#11 token and provide a chain to include in the signature. This option would force them to extract the public key from their token to include in this file. If the chain is supplied separately with --chain they do not need to.

Providing the certificate chain is incompatible with using Fulcio, and should return an error.

@bburky bburky added the enhancement New feature or request label Mar 4, 2022
@dlorenc
Copy link
Member

dlorenc commented Mar 4, 2022

+1 to allowing this somehow. I've been meaning to think more about it forever but haven't had the time. Thanks for opening it!

@bburky
Copy link
Contributor Author

bburky commented Mar 4, 2022

Also, cosign verify should support verification. I think the go api already should "just work" if you specify the roots correctly.

Something like:

# possibly somewhat confusing:
cosign verify --cert ca-bundle.pem
# or a new flag, something like this which more closely matches the go api:
cosign verify --roots ca-bundle.pem

Currently cosign verify --cert ca-bundle.pem with a root bundle seems to result in a crash. :(

@bburky
Copy link
Contributor Author

bburky commented Mar 4, 2022

@dlorenc Do you have a preference for new flags like --chain or putting a concatenated cert chain in the --cert PEM file?

@dlorenc
Copy link
Member

dlorenc commented Mar 5, 2022

I'd probably defer to @haydentherapper on that!

@haydentherapper
Copy link
Contributor

I was interested in making changes around this, thanks for bringing this up!

I'd prefer a new flag --chain to specify a certificate chain. That can be a concatenated chain of PEM encoded certificates.

Note that we need to change how Cosign verifies a certificate chain. Currently Cosign is only expecting a leaf certificate without a chain, because the hosted Fulcio infrastructure issues leaf certificates directly off a root CA. We'll need to also pass in intermediate certificates to Verify

Something to discuss further is specifying the root via flag for verification. In order to trust a certificate, Cosign currently expects the root CA certificate to be signed via a mechanism called TUF (tl;dr is securing trusted files), and you can provide your own CA roots signed through TUF via the cosign initialize command. This effectively securely pins the CA roots. We allow you to also specify trusted roots via the SIGSTORE_ROOT_FILE flag, but I assume this was meant for testing (@dlorenc, @asraa, was this flag meant just for testing?). I'd prefer that we not specify root CA certificates via the verify command, and that these root certs either be specified with TUF or some out-of-band mechanism to pin trusted roots. @dlorenc, @asraa, did y'all have more thoughts on this?

@bburky
Copy link
Contributor Author

bburky commented Mar 7, 2022

For actual verification in a Kubernetes cluster I was looking at Kyverno's image verification support. It doesn't appear to be documented yet, but its policies now allow specifying roots:. Currently the code suggests that roots: implies Rekor, but it could make sense to perform validation against a CA bundle when not using Rekor. (I haven't tested any of that yet though.)

What you're describing with TUF makes sense for command line verification. However for something like kyverno policy, I'm not entirely sure it's needed. The ability to provide policy configuration is privileged. "Enterprise" use cases could use an existing organizational CA, but may not use TUF, Fulcio or Rekor.

What happens if I specify RootCerts in cosign.CheckOpts, does this go through TUF? I think verification may "just work" if I provide roots here?

@haydentherapper
Copy link
Contributor

What you're describing with TUF makes sense for command line verification. However for something like kyverno policy, I'm not entirely sure it's needed. The ability to provide policy configuration is privileged. "Enterprise" use cases could use an existing organizational CA, but may not use TUF, Fulcio or Rekor.

That's a good point, I was considering the command line flags. I agree that it's fine that the Cosign package API supports specifying roots without TUF, but the Cosign flags should not.

What happens if I specify RootCerts in cosign.CheckOpts, does this go through TUF? I think verification may "just work" if I provide roots here?

Correct, if roots are provided here directly, this does not go through TUF. For Cosign, in verify.go, co.RootCerts is populated by a call that will end up verifying the roots with TUF (see the code). As you mentioned, RootCerts can be populated without going through TUF.

@haydentherapper
Copy link
Contributor

I can work on this later this week, unless @bburky was planning to work on this.

@bburky
Copy link
Contributor Author

bburky commented Mar 8, 2022

@haydentherapper I was thinking of working on it some, but I'm still not really too familiar with the codebase yet, please go ahead and start it. I'll see if I can review your PR.

I hacked in a cosign verify --roots flag to test verifying with a custom chain in RootCerts via cosign.CheckOpts... but it seems I only have RSA CAs/leaf certs and none of mine have the right key usage. I also ran into #1566 along the way while testing.

@haydentherapper
Copy link
Contributor

I'll split this change into two - The first will add support for intermediate certs for those using the Cosign library.

For adding support for verification using flags, I'd like to think through this a bit more, in particular for those who want to bring their own PKI but not use TUF. For example, currently, --cert is only used for providing the public key, Cosign doesn't actually verify the signature on the certificate.

@haydentherapper
Copy link
Contributor

I'm walking back the earlier comment about not allowing the root certificate to be specified. With the assumptions the code currently makes, it's assumed that Fulcio, Rekor and TUF usage go hand in hand. It would require a bit of refactoring to allow a root CA specified by TUF and used with a provided certificate. I think there is still value in allowing a root to be pinned by TUF, but this can be done separately.

I'm going to allow the full certificate chain to be specified in the --chain flag. It will verify the certificate provided in --cert.

@haydentherapper
Copy link
Contributor

All PRs have been submitted now. If there's any issues, in particular with PKCS11 tokens, let me know!

@bburky
Copy link
Contributor Author

bburky commented Mar 29, 2022

Thanks for working on all the changes to implement this! I think this should enable cosign to support a lot of organizational use cases where internal PKI exists.

I left comments on a few of the issues, let me know if anything needs clarification.

@znewman01
Copy link
Contributor

znewman01 commented Apr 15, 2022

I think we also need to support this for the sign-* commands, since they perform a verification of the SCT unless you pass --insecure-skip-verify.

EDIT: maybe this is not the right issue for that, because the above only applies when you are using Fulcio. But if you're using Fulcio without a corresponding TUF instance (e.g., in local dev) it's quite useful.

@haydentherapper
Copy link
Contributor

I can add support to sign-blob too, but it would just be for verifying the certificate chain. For sign with a custom certificate chain, we verify the chain, but also persist it in the OCI image. For sign-blob, there's no Cosign support for storing the certificate or chain alongside the blob.

Something to note is we now support uploading certificate chains to Rekor, but Cosign does not support reading entries with a chain uploaded.

@znewman01
Copy link
Contributor

I can add support to sign-blob too, but it would just be for verifying the certificate chain.

I think that's okay. Here's the issue I've been running into (with Fulcio running in docker-compose fulcio@d834c5):

$ cosign sign-blob --fulcio-url=http://localhost:5555 /dev/null
Using payload from: /dev/null
Generating ephemeral keys...
Retrieving signed certificate...
Your browser will now be opened to:
https://oauth2.sigstore.dev/auth/auth?access_type=online&client_id=sigstore&...
Error: signing /dev/null: getting key from Fulcio: verifying SCT: failed to verify ECDSA signature
main.go:46: error during command execution: signing /dev/null: getting key from Fulcio: verifying SCT: failed to verify ECDSA signature

I'd like to manually grab the root CA from Fulcio (curl http://localhost:5555/api/v1/rootCert using legacy API) and specify that using --cert. This use case may be specific to local development and testing; I agree that in the long run you always want to grab these certs from a TUF root.

@haydentherapper
Copy link
Contributor

Totally reasonable! I'll take a look at adding this. When I've been testing out Fulcio locally, I either provide the insecure flag, or set up a local TUF repository.

@haydentherapper
Copy link
Contributor

Looked into adding this - A couple of thoughts:

  • Mirroring cosign sign, we could add support for --cert and --chain, but that would only be for a key delivery mechanism effectively, because the certificate and chain aren't bundled with the blob. At some point, once we have a specification around blobs, then it'd be more useful to add these flags, but until then, I'd just recommend using --key.
  • cosign sign also has no support for providing just a certificate chain while the cert is issued from Fulcio. Besides local testing, I don't think there's another use-case for this to justify adding it.
  • I think providing --insecure-skip-verify is a reasonable step for local testing. Coupled with the SIGSTORE_ROOT_FILE flag, you can avoid setting up a local TUF repo.

@znewman01
Copy link
Contributor

I think the "BYO-root" use case is broken.

Imagine I have some certs (root -> intermediate -> foo):

$ step certificate create \
    --no-password \
    --insecure \
    --profile root-ca \
    root-ca root-ca.crt root-ca.key
$ step certificate create \
    --no-password \
    --insecure \
    --profile intermediate-ca \
    --ca ./root-ca.crt \
    --ca-key ./root-ca.key \
    intermediate-ca intermediate-ca.crt intermediate-ca.key
$ # Need to use a template to make a code signing cert
$ cat > cert.tpl <<-EOF
{
  "subject": {{ toJson .Subject }},
  "sans": {{ toJson .SANs }},
  "keyUsage": ["digitalSignature"],
  "extKeyUsage": ["codeSigning"]
}
EOF
$ step certificate create \
    --no-password \
    --insecure \
    --template cert.tpl \
    --ca ./intermediate-ca.crt \
    --ca-key ./intermediate-ca.key \
    foo foo.crt foo.key 

I can use them to sign an image:

$ REPO="ttl.sh/$(head -c 8 /dev/urandom | xxd -p)"
$ TAG="5m"
$ SRC_IMAGE=gcr.io/distroless/static-debian11
$ export COSIGN_EXPERIMENTAL=1
$
$ # Copy an image to a short-lived repo
$ crane copy $SRC_IMAGE "$REPO:$TAG"
$
$ # Sign
$ COSIGN_PASSWORD=password cosign import-key-pair --key foo.key
$ cat intermediate-ca.crt root-ca.crt > chain.crt
$ COSIGN_PASSWORD=password cosign sign  --key import-cosign.key \
    --certificate-chain chain.crt \
    --certificate foo.crt \
    "$REPO:$TAG"
$ # Verify with the cert/chain from above, provided explicitly:
$ cosign verify --certificate-chain chain.crt --certificate foo.crt "$REPO:$TAG"

That works. I'd expect to be able to verify using SIGSTORE_ROOT_FILE:

$ SIGSTORE_ROOT_FILE=root-ca.crt cosign verify "$REPO:$TAG"
Error: no matching signatures:
x509: certificate signed by unknown authority
main.go:62: error during command execution: no matching signatures:
x509: certificate signed by unknown authority

But that fails. I can do it myself in a pretty hacky way:

$ SIG=$(cosign triangulate "$REPO:$TAG")
$ crane manifest $SIG > manifest.json
$ cat manifest.json \
    | jq '.layers[0].annotations."dev.sigstore.cosign/certificate"' -r \
    | grep . \ # https://github.com/sigstore/cosign/issues/2237
    > registry.crt
$ cat manifest.json \
    | jq '.layers[0].annotations."dev.sigstore.cosign/chain"' -r \
    | grep . \ # https://github.com/sigstore/cosign/issues/2237
    > registry.chain.crt
$ # Pull out the provided root cert from the chain and check that it matches.
$ sed -n '/END CERTIFICATE/,$p' registry.chain.crt | tail -n +2 > registry.root.crt
$ diff root-ca.crt registry.root.crt || echo "no match!"
$ cosign verify --certificate-chain registry.chain.crt --certificate registry.crt "$REPO:$TAG"

This feels like a use case that should be supported; I'd argue for a first-class --certificate-root flag that we could use for verification.

@haydentherapper
Copy link
Contributor

Will take a look! At a glance, my guess is the correct intermediate is not being found, but it might be something else.

@haydentherapper
Copy link
Contributor

Ah ha, I think I figured out what's happening:

  • Experimental is enabled, so the Fulcio roots and intermediates are fetched here
    • This calls initRoots once
    • Pools are initialized - remember this seemingly unimportant step :)
    • Since you provided SIGSTORE_ROOT_FILE, parse the PEM-encoded certificates in the file here (Note that it doesn't initialize roots from TUF using this code)
  • No verifier is set because no flags like cert or key are provided
  • Eventually call VerifySignatureImage (code)
  • Here, check if either there is no chain annotation (there is) or if the intermediate certificates is not set (then populate the intermediate from the chain annotation) <- Ah ha
  • Verify the cert, and then fail

The step where I said "ah ha" is where I expect the intermediates to be populated when not explicitly provided. This was intentionally added to avoid intermediates always needing to be specified, that you'd just need to pin the root. I then realized that because we always initialize a CertPool, even if it's empty, it's still non-nil, so we never populate the intermediate from the chain.

So, two ways to fix this:

  1. Require that you specify the intermediate in your SIGSTORE_ROOT_FILE, just concatenate another PEM encoded certificate and it'll work. You can try this out, this should work.
  2. Lazily initialize the intermediate pool (and probably in sigstore/sigstore too?). Pull the intermediates from the chain annotation when not specified. The only issue I can think of with this is there's no way to "revoke" an intermediate then without using TUF, but maybe that should just be out of scope, if you're provided your own root via flag.

@haydentherapper
Copy link
Contributor

re: --certificate-root, I would like to avoid more flags that make it easy to provide your own root. We should be encouraging TUF usage. I've always said that SIGSTORE_ROOT_FILE is a testing flag.

@haydentherapper
Copy link
Contributor

Looks like this was an accidental regression, at one point we were lazily loading the intermediate pool - https://github.com/sigstore/cosign/pull/1804/files

I think this change occurred when we moved over code from sigstore/sigstore. I'll fix this!

@znewman01
Copy link
Contributor

re: --certificate-root, I would like to avoid more flags that make it easy to provide your own root. We should be encouraging TUF usage.

I agree with you in principle, but as a matter of pragmatism, using an existing root is a real use case, and setting up TUF is a hassle. Even if the tooling were polished, you have to stand up a new repository, and manage the keys. Think about migrating from a traditional code-signing PKI to Sigstore (like your Adopting Sigstore Incrementally post, the "Self-managed keys in identity-based code signing certificate with auditability" section).

I've always said that SIGSTORE_ROOT_FILE is a testing flag.

Agreed, which is why we should promote it to a real flag and test/document it 😛

haydentherapper added a commit to haydentherapper/cosign that referenced this issue Sep 13, 2022
This fixes a regression where intermediates, when not present in the
intermediate certificate pool, would be fetched from the OCI chain
annotation.

Ref sigstore#1554, which includes more details

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
haydentherapper added a commit to haydentherapper/cosign that referenced this issue Sep 13, 2022
This fixes a regression where intermediates, when not present in the
intermediate certificate pool, would be fetched from the OCI chain
annotation.

Ref sigstore#1554, which includes more details

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
haydentherapper added a commit to haydentherapper/cosign that referenced this issue Sep 19, 2022
This fixes a regression where intermediates, when not present in the
intermediate certificate pool, would be fetched from the OCI chain
annotation.

Ref sigstore#1554, which includes more details

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
dlorenc pushed a commit that referenced this issue Sep 20, 2022
…#2244)

This fixes a regression where intermediates, when not present in the
intermediate certificate pool, would be fetched from the OCI chain
annotation.

Ref #1554, which includes more details

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@maxking
Copy link

maxking commented Sep 23, 2022

I am trying to setup container image signing without having to setup and maintain a service for TL. We already have a PKI system with identity certificates available that I want to use for signing the container images.

So, far, I have been able to use cosign import-key-pair to import the key/cert, use them to sign and attach both certificate and certificate chain to the image using cosign sign --key import-cosign.key --cert key.cert --cert-chain ca-chain.crt $IMAGE:$TAG. I am also able to verify the signature by passing in the cert (cosign verify --cert key.cert $IMAGE:$TAG). However, that would require me to figure out a way to distribute the certificate and have policy-controller use it for verification. But, the certificate & certificate chain both are already available as annotations in the manifest.json as @znewman01 mentioned above and I am able to verify as well.

I am not sure if I am trying to do something stupid over here, but is there some reason why we can't get the certificate & chain from the manifest and validate the signature only with the ROOT ca being provided to the verify command? Is the proposal for --certificate-root to serve the same purpose? or would it only work with Signature fetched from TL and not the annotations in OCI manifest?

@haydentherapper
Copy link
Contributor

Can you try SIGSTORE_ROOT_FILE=root.cert cosign verify $IMAGE:$TAG? This should pick up the certificate and chain from the annotations on the image automatically.

@maxking
Copy link

maxking commented Sep 24, 2022

@haydentherapper ^ That doesn't work unless you have COSIGN_EXPERIMENTAL=1 turned on, which as I understand, is going to try to talk to TL?

$ SIGSTORE_ROOT_FILE=Root_CA.crt.pem cosign verify $IMAGE:$TAG
Error: exactly one of: key reference (--key), certificate (--cert) or hardware token (--sk) must be provided
main.go:62: error during command execution: exactly one of: key reference (--key), certificate (--cert) or hardware token (--sk) must be provided

@maxking
Copy link

maxking commented Sep 24, 2022

FWIW, i tried verification using Kyverno and it seems to work by just adding the cert chain in the ClusterPolicy. This is the verification code in Kyverno.

So, it seems like this is just a matter of allowing verification (and perhaps a better way to pass the root-cert/chain to the CLI) in the CLI? I'll try to spend some time digging through the code.

@haydentherapper
Copy link
Contributor

Ah yes, that would require checking the transparency log by default. We should be supporting this use case of automatically fetching the certificate and chain from the annotations. We are working on removing the experimental flag and will be providing a flag to explicitly not check the tlog.

For now, I believe you can set “rekor-url” to an empty string, which should disable tlog checks even when the experimental flag is turned on. If it doesn’t, let us know, we can push a fix to support that case.

@maxking
Copy link

maxking commented Sep 25, 2022

Aha, you are absolutely right! Setting the COSIGN_EXPERIMENTAL=1 and --rekor-url="" allows it to fetch the cert and chain from the annotations. Thanks a lot!

@znewman01
Copy link
Contributor

Glad you got it working! Apologies that this UX is...unfortunate right now. Normally I would insist that we clean it up, but I think in this case there's going to be some unification where we flip COSIGN_EXPERIMENTAL behavior on by default over the next couple of months and it'll get resolved on its own.

@maxking
Copy link

maxking commented Sep 26, 2022

In terms UX, my initial expectation was to use --cert-chain flag to pass in the certificate chain/root CA to the verify command similar to how we pass in to sign command.

I agree that removing the COSIGN_EXPERIMENTAL flag (with some way to skip talking to TL) would fix the other parts.

@haydentherapper
Copy link
Contributor

haydentherapper commented Jan 11, 2023

This should be complete now. There's some lingering issues like supporting attach and clarifying that chain included a root of trust, but they're tracked in other issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants