fix: fix cert chain validation for verify-blob in non-experimental mode #2256

asraa · 2022-09-15T16:30:36Z

Signed-off-by: Asra Ali [email protected]

Summary

fix: Pulls Fulcio root and intermediate when --certificate-chain is not passed into verify-blob.

In non-experimental mode, we incorrectly only loaded Fulcio roots to verify against when certRef == "".

When a certRef != "" in non-experimental mode, like, user provides cert with offline bundle without specifying a custom cert chain, then we would NOT load in the Fulcio roots. This would cause certificate validation failure.

Instead, whenever certChain == "" we will load in fulcio roots in non-experimental mode, either if you provide a cert or a bundle.

Release Note

fix: Pulls Fulcio root and intermediate when --certificate-chain is not passed into verify-blob. The v1.12.0 release introduced a regression: when COSIGN_EXPERIMENTAL was not set, cosign verify-blob would check a --certificate (without a --certificate-chain provided) against the operating system root CA bundle. In this release, Cosign checks the certificate against Fulcio's CA root instead (restoring the earlier behavior).

Documentation

Signed-off-by: Asra Ali <[email protected]>

cmd/cosign/cli/verify/verify_blob.go

asraa · 2022-09-15T16:33:28Z

Should add test validating that we will succeed when certRef != "" in non-experimental mode. I think that's currently missing. One sec.

haydentherapper · 2022-09-15T16:42:42Z

cmd/cosign/cli/verify/verify_blob.go

+				if err != nil {
+					return fmt.Errorf("getting Fulcio intermediates: %w", err)
+				}
+			}
 			co.SigVerifier, err = cosign.ValidateAndUnpackCert(cert, co)


Should we handle else for when the cert chain is set? Otherwise, the certificate wouldn't validate if it's been signed by a non-fulcio pki

good catch!

lemme see if I can also add a test here.

Hm...I think it's pretty unexpected that --certificate-chain would mean "accept whatever cert was at the root of the chain."

This is part of why I think an explicit --certificate-root flag would be a good idea.

We do document the the chain is the intermediates and root -

cosign/cmd/cosign/cli/options/certificate.go

Lines 67 to 70 in 04f509c

"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")

Yea, let's. It's a bit unclear to me what should be the right behavior here, especially if I provide both a sigstore root and certificate chain.

The behavior here should match 1.11.x (the more significant change is around checking expiration)

If someone could help me update the release notes to close this out, that'd be great. I added one in. I'm not sure if anything breaks.

Hm I don't see anything under "Release Notes" in the PR description.

Maybe:

The v1.12.0 release introduced a regression: when COSIGN_EXPERIMENTAL was not set, cosign verify-blob would check a --certificate (without a --certificate-chain provided) against the operating system root CA bundle. In this release, Cosign checks the certificate against Fulcio's CA root instead (restoring the earlier behavior).

I think we should have a summary of the current behavior (as opposed to a diff, in the release note) somewhere too; maybe that's what we can hash out in #2254

Sorry, I put it in the summary instead. Moved, but feel free to edit.

Sg, I'll add a longer explanation

Signed-off-by: Asra Ali <[email protected]>

asraa · 2022-09-15T16:44:58Z

Just added a test verifying that certRef provided without explicit intermediate succeeds against the fulcio roots.

codecov-commenter · 2022-09-15T16:45:25Z

Codecov Report

Merging #2256 (0a2e6e7) into main (f43839b) will increase coverage by 0.15%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##             main    #2256      +/-   ##
==========================================
+ Coverage   28.40%   28.56%   +0.15%     
==========================================
  Files         131      131              
  Lines        7832     7855      +23     
==========================================
+ Hits         2225     2244      +19     
+ Misses       5309     5305       -4     
- Partials      298      306       +8

Impacted Files	Coverage Δ
cmd/cosign/cli/verify/verify_blob.go	`45.26% <42.85%> (+0.35%)`	⬆️
cmd/cosign/cli/verify/verify.go	`5.97% <0.00%> (+2.61%)`	⬆️

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

cmd/cosign/cli/verify/verify_blob.go

cmd/cosign/cli/verify/verify_blob_test.go

znewman01 · 2022-09-15T16:47:32Z

cmd/cosign/cli/verify/verify_blob.go

+				if err != nil {
+					return fmt.Errorf("getting Fulcio intermediates: %w", err)
+				}
+			}
 			co.SigVerifier, err = cosign.ValidateAndUnpackCert(cert, co)


Hm...I think it's pretty unexpected that --certificate-chain would mean "accept whatever cert was at the root of the chain."

This is part of why I think an explicit --certificate-root flag would be a good idea.

Signed-off-by: Asra Ali <[email protected]>

cmd/cosign/cli/verify/verify_blob_test.go

znewman01 · 2022-09-15T16:58:53Z

cmd/cosign/cli/verify/verify_blob.go

+				if err != nil {
+					return fmt.Errorf("getting Fulcio intermediates: %w", err)
+				}
+			}
 			co.SigVerifier, err = cosign.ValidateAndUnpackCert(cert, co)


If we're sure that this matches the behavior from v1.11.x, and we explain it more clearly in the help text, I'm okay with dealing with this problem later.

Signed-off-by: Asra Ali <[email protected]>

haydentherapper

thanks!

cpanato · 2022-09-16T07:22:48Z

@asraa @dlorenc should we release a 1.12.1 patch?

haydentherapper · 2022-09-16T11:55:06Z

@cpanato as soon as possible I think. Release notes should be specified in the PR description

fix: fix cert chain validation

7f50741

Signed-off-by: Asra Ali <[email protected]>

asraa commented Sep 15, 2022

View reviewed changes

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

haydentherapper reviewed Sep 15, 2022

View reviewed changes

one more test

e641243

Signed-off-by: Asra Ali <[email protected]>

znewman01 reviewed Sep 15, 2022

View reviewed changes

asraa added 2 commits September 15, 2022 11:50

add one more test

1405355

Signed-off-by: Asra Ali <[email protected]>

address comments

0a2e6e7

Signed-off-by: Asra Ali <[email protected]>

znewman01 reviewed Sep 15, 2022

View reviewed changes

update

4f1ae75

Signed-off-by: Asra Ali <[email protected]>

znewman01 approved these changes Sep 15, 2022

View reviewed changes

haydentherapper approved these changes Sep 15, 2022

View reviewed changes

znewman01 approved these changes Sep 15, 2022

View reviewed changes

asraa enabled auto-merge (squash) September 15, 2022 21:44

dlorenc approved these changes Sep 16, 2022

View reviewed changes

asraa merged commit 63fe875 into sigstore:main Sep 16, 2022

github-actions bot added this to the v1.13.0 milestone Sep 16, 2022

wlynch mentioned this pull request Sep 23, 2022

tuf: warning using deprecated ecdsa hex-encoded keys sigstore/gitsign#143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix cert chain validation for verify-blob in non-experimental mode #2256

fix: fix cert chain validation for verify-blob in non-experimental mode #2256

asraa commented Sep 15, 2022 •

edited

Loading

asraa commented Sep 15, 2022

haydentherapper Sep 15, 2022

asraa Sep 15, 2022

asraa Sep 15, 2022

znewman01 Sep 15, 2022

haydentherapper Sep 15, 2022

haydentherapper Sep 15, 2022

haydentherapper Sep 15, 2022

asraa Sep 15, 2022

znewman01 Sep 15, 2022

asraa Sep 15, 2022

asraa commented Sep 15, 2022

codecov-commenter commented Sep 15, 2022 •

edited

Loading

znewman01 Sep 15, 2022

znewman01 Sep 15, 2022

haydentherapper left a comment

cpanato commented Sep 16, 2022

haydentherapper commented Sep 16, 2022

	"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")

fix: fix cert chain validation for verify-blob in non-experimental mode #2256

fix: fix cert chain validation for verify-blob in non-experimental mode #2256

Conversation

asraa commented Sep 15, 2022 • edited Loading

Summary

Release Note

Documentation

asraa commented Sep 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Sep 15, 2022

codecov-commenter commented Sep 15, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper left a comment

Choose a reason for hiding this comment

cpanato commented Sep 16, 2022

haydentherapper commented Sep 16, 2022

asraa commented Sep 15, 2022 •

edited

Loading

codecov-commenter commented Sep 15, 2022 •

edited

Loading