Verify the certificate chain against the Fulcio root trust by default #2139

wata727 · 2022-08-06T09:13:18Z

Summary

Currently, the verification performed by cosign verify-blob --cert does not verify the certificate chain, so you cannot guarantee that the certificate was correctly issued by Fulcio. So you need to explicitly pass Fulcio's root certificate via the --cert-chain option.

cosign initialize
cat ~/.sigstore/root/targets/fulcio_intermediate_v1.crt.pem > fulcio_chained.crt.pem
echo >> fulcio_chained.crt.pem
cat ~/.sigstore/root/targets/fulcio.crt.pem >> fulcio_chained.crt.pem
echo >> fulcio_chained.crt.pem
cat ~/.sigstore/root/targets/fulcio_v1.crt.pem >> fulcio_chained.crt.pem

cosign verify-blob --cert checksum.txt.pem --cert-chanin fulcio_chained.crt.pem ...

However, this way of getting root certificates is not secure. Ideally, it should be retrieved by a TUF client. After discussing this matter in #2134, @haydentherapper says that there is room for changing the default behavior.

This PR will change the default behavior from not verifying the certificate chain if --cert-chain is not passed to verifying the certificate chain against the Fulcio root trust getting by a TUF client.

Release Note

Changed the default behavior from not verifying the certificate chain if --cert-chain is not passed to verifying the certificate chain against the Fulcio root trust getting by a TUF client.

Documentation

The description of the --certificate option needs to be changed.

wata727 · 2022-08-06T09:19:49Z

I have two things to discuss:

Should this change be followed by similar commands such as cosign verify?
Is there a good way to add tests for this implementation? verify_blob_test.go didn't seem to have any tests for verification.

Signed-off-by: Kazuma Watanabe <[email protected]>

codecov-commenter · 2022-08-06T15:32:27Z

Codecov Report

Merging #2139 (b4887ea) into main (128f8fb) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2139      +/-   ##
==========================================
- Coverage   26.28%   26.23%   -0.06%     
==========================================
  Files         130      130              
  Lines        7602     7617      +15     
==========================================
  Hits         1998     1998              
- Misses       5347     5362      +15     
  Partials      257      257

Impacted Files	Coverage Δ
cmd/cosign/cli/options/certificate.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/verify/verify.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/verify/verify_attestation.go	`0.00% <0.00%> (ø)`
cmd/cosign/cli/verify/verify_blob.go	`9.72% <0.00%> (-0.16%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

haydentherapper

The same changes need to be made in https://cs.github.com/sigstore/cosign/blob/c5fda01a8ff33ca981f45a9f13e7fb6bd2080b94/cmd/cosign/cli/verify/verify_attestation.go?q=org%3Asigstore+CheckCertificatePolicy#L141 and https://cs.github.com/sigstore/cosign/blob/95b74db89941e8ec85e768f639efd4d948db06cd/cmd/cosign/cli/verify/verify.go?q=org%3Asigstore+CheckCertificatePolicy#L162

cmd/cosign/cli/verify/verify_blob.go

haydentherapper · 2022-08-06T16:54:44Z

Testing is unfortunately a bit rough. Check out e2e_test.go, but it might be hard to fit in a test for this, especially with TUF. We may need a TODO.

haydentherapper · 2022-08-06T16:55:01Z

/hold, I want to give a bit more thought to this and will get back to you on Monday.

cc @znewman01

…default Signed-off-by: Kazuma Watanabe <[email protected]>

…ust by default Signed-off-by: Kazuma Watanabe <[email protected]>

wata727 · 2022-08-07T16:28:57Z

The same changes need to be made

Fixed eda267b

Testing is unfortunately a bit rough. Check out e2e_test.go, but it might be hard to fit in a test for this, especially with TUF. We may need a TODO.

Understood. I'll take a little deeper look at the e2e_test.go implementation.

znewman01 · 2022-08-08T21:54:27Z

Hey, thanks for this! (and thanks for looping me in, @haydentherapper)

I haven't looked too closely at the code but this change seems like much better/safer default behavior.

I just want to think through workarounds for anyone who was relying on the old behavior. The right thing to do in such a case is probably to provide the public key directly, rather than via a cert; this makes it much clearer what you're asking for. Maybe we should refer users to the step certificate key command? IIUC it should Just Work™

Was there anything specific you wanted my feedback on?

patflynn · 2022-08-09T00:30:32Z

I'm probably mis-interpreting this, but for cases where the user has signed a blob with a user provided custom key, why would we want to verify against the fulcio root?

haydentherapper · 2022-08-09T00:41:40Z

@znewman01 Just looking for a second opinion! Generally this looked good to me, beyond the potentially breaking change.

@patflynn One thought is a user is very unlikely to provide a certificate in that case (I think), they'd be providing a key. We've got a few options for providing verification material:

No key or cert - Either:
- Fetch the certificate+chain from OCI. Verify the certificate+chain against the TUF root
- Fetch the certificate from Rekor for blob signing. Extract the verification key. (I don't actually like this since Rekor is meant to be a witness to an event and not storage, but let's discuss that more in another issue) <-- Note that this doesn't verify the cert against a chain because previously Rekor didn't support uploading the full cert chain. This option is just kinda messy.
A key - Use the key to verify
A certificate+chain - Verify the chain, and extract the verification key from the cert. Don't verify against TUF.
A certificate w/o chain - Current behavior is to just extract the verification key. My question is, is anyone providing a certificate that's not from Fulcio in this case? If the vast majority of users provide a Fulcio certificate, then we should verify against the chain from TUF.

The workaround would either be extracting the key themselves (Like Zach mentioned) or providing the full certificate chain.

patflynn · 2022-08-09T00:49:27Z

@haydentherapper got it. that makes sense. Thanks for clarifying.

asraa · 2022-08-09T13:32:23Z

cmd/cosign/cli/options/certificate.go

@@ -37,7 +37,7 @@ var _ Interface = (*RekorOptions)(nil)
 // AddFlags implements Interface
 func (o *CertVerifyOptions) AddFlags(cmd *cobra.Command) {
 	cmd.Flags().StringVar(&o.Cert, "certificate", "",
-		"path to the public certificate")
+		"path to the public certificate. The certificate will be verified against the Fulcio roots if the --certificate-chain option is not passed.")


nit: Fulcio root provided in the TUF root?

I didn't mention it to TUF here because I have a concern about the description would be too long. If we should mention it, please let me know so I fix it.

haydentherapper · 2022-08-09T13:43:58Z

LGTM, just need to fix the failing test. Thanks!

dlorenc · 2022-08-09T15:51:38Z

The failure is unrelated and the fix is over here: sigstore/scaffolding#277

dlorenc · 2022-08-09T15:51:46Z

I'll merge while we wait for the fix.

wata727 mentioned this pull request Aug 6, 2022

How to get the Fulcio root certificate securely in verify-blob? #2134

Closed

Verify the certificate chain against the Fulcio root trust by default

ce81d79

Signed-off-by: Kazuma Watanabe <[email protected]>

wata727 force-pushed the verify_cert_chain_by_default branch from d43c022 to ce81d79 Compare August 6, 2022 09:26

haydentherapper requested changes Aug 6, 2022

View reviewed changes

cmd/cosign/cli/verify/verify_blob.go Show resolved Hide resolved

wata727 added 2 commits August 7, 2022 16:10

fixup! Verify the certificate chain against the Fulcio root trust by …

eda267b

…default Signed-off-by: Kazuma Watanabe <[email protected]>

fixup! fixup! Verify the certificate chain against the Fulcio root tr…

b4887ea

…ust by default Signed-off-by: Kazuma Watanabe <[email protected]>

wata727 changed the title ~~verify-blob: Verify the certificate chain against the Fulcio root trust by default~~ Verify the certificate chain against the Fulcio root trust by default Aug 7, 2022

wata727 marked this pull request as ready for review August 7, 2022 16:31

asraa approved these changes Aug 9, 2022

View reviewed changes

dlorenc merged commit fdceee4 into sigstore:main Aug 9, 2022

github-actions bot added this to the v1.11.0 milestone Aug 9, 2022

wata727 deleted the verify_cert_chain_by_default branch August 10, 2022 11:59

cpanato mentioned this pull request Aug 16, 2022

fix oidc post-merge job #2164

Merged

wata727 mentioned this pull request Aug 20, 2022

Revert "Add a notice about verification of keyless signing" terraform-linters/tflint#1487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify the certificate chain against the Fulcio root trust by default #2139

Verify the certificate chain against the Fulcio root trust by default #2139

wata727 commented Aug 6, 2022 •

edited

Loading

wata727 commented Aug 6, 2022

codecov-commenter commented Aug 6, 2022 •

edited

Loading

haydentherapper left a comment

haydentherapper commented Aug 6, 2022

haydentherapper commented Aug 6, 2022

wata727 commented Aug 7, 2022

znewman01 commented Aug 8, 2022

patflynn commented Aug 9, 2022

haydentherapper commented Aug 9, 2022 •

edited

Loading

patflynn commented Aug 9, 2022

asraa Aug 9, 2022

wata727 Aug 10, 2022

haydentherapper commented Aug 9, 2022

dlorenc commented Aug 9, 2022

dlorenc commented Aug 9, 2022

Verify the certificate chain against the Fulcio root trust by default #2139

Verify the certificate chain against the Fulcio root trust by default #2139

Conversation

wata727 commented Aug 6, 2022 • edited Loading

Summary

Release Note

Documentation

wata727 commented Aug 6, 2022

codecov-commenter commented Aug 6, 2022 • edited Loading

Codecov Report

haydentherapper left a comment

Choose a reason for hiding this comment

haydentherapper commented Aug 6, 2022

haydentherapper commented Aug 6, 2022

wata727 commented Aug 7, 2022

znewman01 commented Aug 8, 2022

patflynn commented Aug 9, 2022

haydentherapper commented Aug 9, 2022 • edited Loading

patflynn commented Aug 9, 2022

asraa Aug 9, 2022

Choose a reason for hiding this comment

wata727 Aug 10, 2022

Choose a reason for hiding this comment

haydentherapper commented Aug 9, 2022

dlorenc commented Aug 9, 2022

dlorenc commented Aug 9, 2022

wata727 commented Aug 6, 2022 •

edited

Loading

codecov-commenter commented Aug 6, 2022 •

edited

Loading

haydentherapper commented Aug 9, 2022 •

edited

Loading