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 support for cert and cert chain flags with PKCS11 tokens #1671

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

haydentherapper
Copy link
Contributor

Currently, Cosign attempts to automatically extract the certificate from
the PKCS11 token, and there's no option to provide your own certificate
chain or different certificate. Now, Cosign will respect those flags.

Note that the chain must be valid, and the public key from the token
must match the provided certificate from either the token or flag.

Note that if a cert flag is specified, it will override the certificate
fetched from the PKCS11 token if one is present, and log a warning.

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

Summary

Ticket Link

Ref #1554

Release Note

Added support for cert and cert-chain flags for PKCS11 tokens

Currently, Cosign attempts to automatically extract the certificate from
the PKCS11 token, and there's no option to provide your own certificate
chain or different certificate. Now, Cosign will respect those flags.

Note that the chain must be valid, and the public key from the token
must match the provided certificate from either the token or flag.

Note that if a cert flag is specified, it will override the certificate
fetched from the PKCS11 token if one is present, and log a warning.

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

@bburky, would you be able to verify that this is working as expected? I don't have a setup with a PKCS11 token currently.

Can you check that:

  • You can pass a certificate chain by flag, where the certificate comes from the token
  • You can pass both a certificate chain and certificate by flag

@codecov-commenter
Copy link

Codecov Report

Merging #1671 (bc03eba) into main (69fc348) will increase coverage by 0.06%.
The diff coverage is 37.03%.

@@            Coverage Diff             @@
##             main    #1671      +/-   ##
==========================================
+ Coverage   29.19%   29.26%   +0.06%     
==========================================
  Files         140      140              
  Lines        8343     8348       +5     
==========================================
+ Hits         2436     2443       +7     
+ Misses       5642     5638       -4     
- Partials      265      267       +2     
Impacted Files Coverage Δ
cmd/cosign/cli/sign/sign.go 14.57% <37.03%> (+2.74%) ⬆️
pkg/cosign/tuf/client.go 62.34% <0.00%> (-0.95%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69fc348...bc03eba. Read the comment docs.

@dlorenc dlorenc merged commit e596625 into sigstore:main Mar 29, 2022
@github-actions github-actions bot added this to the v1.7.0 milestone Mar 29, 2022
@haydentherapper haydentherapper deleted the pkcs11 branch March 29, 2022 04:47
@bburky
Copy link
Contributor

bburky commented Mar 29, 2022

My current smart card appears to have the key usage options incompatible with cosign and I only have an RSA key. With some quick patches from signature.LoadECDSAVerifier() to signature.LoadVerifier() I was able to test. The full chain (multiple certs) is successfully added as a chain annotation when signing.

# The following worked before, and still works. No chain is included in the signature.
./cosign sign  "localhost:5000/$image"  --key "pkcs11:$key"
# The following succeeds verification, like before
./cosign verify  "localhost:5000/$image"  --cert cert.pem

However, I'm seeing both sign and verify reject my certificate if I provide a full chain. Or if I provide the leaf certificate as the root:

# The following is sort of saying "verify this certificate as a self signed root" I think? I think I would expect this to be have identically to `--cert cert.pem`
# This returns the error `error during command execution: x509: certificate specifies an incompatible key usage`
./cosign verify  "localhost:5000/$image"  --cert cert.pem --cert-chain cert.pem

# This error also appears when correctly passing the full chain to the root:
# `error during command execution: x509: certificate specifies an incompatible key usage`
./cosign verify  "localhost:5000/$image"  --cert-chain chain.pem

# And can be triggered with `sign` if a --cert-chain is passed.
# `signer: unable to validate certificate chain: x509: certificate specifies an incompatible key usage`
./cosign sign "localhost:5000/$image" --key "pkcs11:$key" --cert-chain chain.pem

This may not be a bug, but the above behavior is a little bit inconsistent (if this is intended, then verify --cert cert.pem should fail the same as verify --cert cert.pem --cert-chain chain.pem?).

It seems like the required key usage is Code Signing? I was testing with a critical "Digital Signature, Non Repudiation" certificate instead. For production use, I plan to have a Code Signing certificate issued, but do not currently have one available to test. If I replace this with x509.ExtKeyUsageAny, both sign and verify succeed:

cosign/pkg/cosign/verify.go

Lines 846 to 848 in ba50ee0

KeyUsages: []x509.ExtKeyUsage{
x509.ExtKeyUsageCodeSigning,
},

I think this feature is working correctly, but we have no documentation about the required key usage.

@bburky
Copy link
Contributor

bburky commented Mar 29, 2022

# The following is correctly rejected with `getting signer: public key in certificate does not match the provided public key`
./cosign sign  "localhost:5000/$image"  --key "pkcs11:$key" --cert wrong-cert.pem
# The following is correctly rejected with `getting signer: unable to validate certificate chain: x509: certificate signed by unknown authority`
./cosign sign  "localhost:5000/$image"  --key "pkcs11:$key" --cert-chain wrong-chain.pem

# The following works fine, but is the same cert as is on the smartcard. I don't have an alternate valid cert to use.
./cosign sign  "localhost:5000/$image"  --key "pkcs11:$key" --cert cert.pem

@haydentherapper
Copy link
Contributor Author

Thanks so much for testing this out. The error you saw, x509: certificate specifies an incompatible key usage, is working as intended. This is from EKU chaining or constrained EKUs, where the intermediate CA certs' EKUs constrain the leaf certificates'. We could relax this in Cosign and accept any EKU, like what you did in that patch, but it's a best practice to enforce this. Enforcement differs by x509 client - I know that Windows expects intermediates to contain the EKUs of the children certs. The reason it worked when you just provided a cert vs the cert chain is because the verification library is checking that the EKUs in a cert chain match the leaf cert and match the value provided to thecert.Verify function - Without a chain, it passed.

I'll write up some documentation on this.

mlieberman85 pushed a commit to mlieberman85/cosign that referenced this pull request May 6, 2022
…e#1671)

Currently, Cosign attempts to automatically extract the certificate from
the PKCS11 token, and there's no option to provide your own certificate
chain or different certificate. Now, Cosign will respect those flags.

Note that the chain must be valid, and the public key from the token
must match the provided certificate from either the token or flag.

Note that if a cert flag is specified, it will override the certificate
fetched from the PKCS11 token if one is present, and log a warning.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants