sigstore / cosign

Code signing and transparency for containers and binaries
Apache License 2.0
4.38k stars 537 forks source link

Allow specifying CA chain when not using Fulcio #1554

Closed bburky closed 1 year ago

bburky commented 2 years ago

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.

dlorenc commented 2 years ago

+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 commented 2 years ago

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 commented 2 years ago

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

dlorenc commented 2 years ago

I'd probably defer to @haydentherapper on that!

haydentherapper commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

bburky commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

bburky commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Looked into adding this - A couple of thoughts:

znewman01 commented 2 years ago

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 commented 2 years ago

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

haydentherapper commented 2 years ago

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

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 😛

maxking commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.