Fix verifying when a certificate is provided #2633

haydentherapper · 2023-01-16T03:28:08Z

Keys were preferred over certificates. This caused an issue, because in the CLI functions that call verify, if you pass a certificate to verify with, we extract the public key from it and set that to co.SigVerifier while also setting sig.Cert. Checking if a certificate was provided first should mitigate this.

Fixes: #2632

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

Summary

Release Note

Documentation

Keys were preferred over certificates. This caused an issue, because in the CLI functions that call verify, if you pass a certificate to verify with, we extract the public key from it and set that to co.SigVerifier while also setting sig.Cert. Checking if a certificate was provided first should mitigate this. Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper · 2023-01-16T03:29:01Z

@asraa, can you check this is correct, since you did the refactor for this?

cc @nsmith5

haydentherapper · 2023-01-16T03:38:23Z

Looking into unit test failures now

haydentherapper · 2023-01-16T03:43:10Z

The failure is when you provide both a key (which you want to verify with) and a bundle with a certificate. I think the fix is just to make this an error when taking in input. We don't allow you to specify more than one of (cert, security key, public key), so we shouldn't allow you to specify a bundle with a verifier (pub key or cert) if you also specified the verifier. (Note we would still allow a bundle + verifier if the bundle only contains the Rekor entry) (also note that we should still error if the rekor entry contains the wrong verifier, which I will make sure there’s a test for that case)

Will follow up on Tuesday.

nsmith5 · 2023-01-16T03:49:28Z

pkg/cosign/verify.go

-	// We have a public key.
-	if co.SigVerifier != nil {
+	switch {
+	// certificate provided from the OCI image, or via flag (--certificate or --bundle)


I'm not 100% sure I understand the logic here, but I'm worried that this bit might have a subtle verification bug. Let me know if this makes sense:

If the user has specified --certificate at the command line my understanding is that it get parsed and its public key is used in initialize co.SigVerifier. This function appears to then return a certificate PEM if found on the OCI signature (which may be different than --certificate 😬 ). The function calling this one

cosign/pkg/cosign/verify.go

Lines 1078 to 1081 in 29360f6

if !bytes.Equal(pemFirst.Bytes, pemSecond.Bytes) {

return fmt.Errorf("comparing public key PEMs, expected %s, got %s",

pemBytes, decodeSecond)

}

then appears to compare the returned certificate to the one it finds in the rekor bundle.

My worry here is that the certificate compared to the rekor bundle might not be the one the user actually specified at the command line, unless that bit of logic is lurking in a different place?

I think we have this issue now also, although it doesn’t surface any error. If you provide a certificate that certified the same public key as a certificate on the OCI imagine, but the rest of the cert differs, we wouldn’t notice.

Note this isn’t an issue for verify-blob since we construct a static OCI struct from input. For verify on OCI, theres no way to pass the certificate passed in from input

Sigh…maybe we could add a check that a provided cert matches what’s on the image annotations? Or disallow providing a certificate when the image annotations contain a certificate already?

Sigh…maybe we could add a check that a provided cert matches what’s on the image annotations? Or disallow providing a certificate when the image annotations contain a certificate already?

+1, see my comment below. I think --certificate is a little misleading here. The --certificate flag for OCI image never "makes" it into the pkg verification code (just the pub key does). In the CLI layer there's some verification of it, but once we get into pkg/verify, we rely on sig.Cert().

haydentherapper · 2023-01-16T04:20:39Z

Oof my brain

I was confused why we hadn't caught this for verify-blob. Turns out, it's because we don't set co.SigVerifier in verify-blob:

https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify/verify_blob.go#L193-L197

Contrast this to verify:

https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify/verify.go#L214-L243

This means that the certificate is used when comparing to the Rekor entry, because SigVerifier == nil.

Not sure if this was intentional or accidental. I think it wasn't intentional because if a bundle is provided with a certificate in the bundle, we do set the verifier: https://github.com/sigstore/cosign/blob/main/cmd/cosign/cli/verify/verify_blob.go#L220

I think we should pick one place to set the verifier. Either require a verifier always provided (and figure out how to pass a certificate value into VerifyImageSignature, or always leverage

cosign/pkg/cosign/verify.go

Lines 638 to 670 in 29360f6

    
           verifier := co.SigVerifier 
        
           if verifier == nil { 
        
           	// If we don't have a public key to check against, we can try a root cert. 
        
           	cert, err := sig.Cert() 
        
           	if err != nil { 
        
           		return false, err 
        
           	} 
        
           	if cert == nil { 
        
           		return false, &VerificationError{"no certificate found on signature"} 
        
           	} 
        
           	// Create a certificate pool for intermediate CA certificates, excluding the root 
        
           	chain, err := sig.Chain() 
        
           	if err != nil { 
        
           		return false, err 
        
           	} 
        
           	// If there is no chain annotation present, we preserve the pools set in the CheckOpts. 
        
           	if len(chain) > 0 { 
        
           		if len(chain) == 1 { 
        
           			co.IntermediateCerts = nil 
        
           		} else if co.IntermediateCerts == nil { 
        
           			// If the intermediate certs have not been loaded in by TUF 
        
           			pool := x509.NewCertPool() 
        
           			for _, cert := range chain[:len(chain)-1] { 
        
           				pool.AddCert(cert) 
        
           			} 
        
           			co.IntermediateCerts = pool 
        
           		} 
        
           	} 
        
           	verifier, err = ValidateAndUnpackCert(cert, co) 
        
           	if err != nil { 
        
           		return false, err 
        
           	} 
        
           }

to set the verifier.

asraa

Thanks! Yes this should have been the correct behavior -- chose cert, or choose public key PEM for the verifier.

One comment -- I think @nsmith5 identified the problem about the comparison in pkg/cosign/verify.go linked in the comment. The more weird problem is in my comment, but --certificate is never actually used besides public key extraction and verification BEFORE hitting the pkg. I think we either want to set the sig.Cert() with this / override it, or disallow this flag when a sig.Cert() is already specified on the OCI layer. Flag confusion..

asraa · 2023-01-17T16:12:55Z

pkg/cosign/verify.go

-	// We have a public key.
-	if co.SigVerifier != nil {
+	switch {
+	// certificate provided from the OCI image, or via flag (--certificate or --bundle)


is this comment accurate / misleading? If it's OCI it will always take it in from the OCI image, NOT from the --certificate flag. I think @nsmith5 issue shows a "Red herring" like you mentioned. I think the issue should have happened if the --certificate was specified or not since it's comparing sig.Cert() form the OCI annotation. The OCI verify CLI never sets that. So maybe

// certificate provided from the OCI image, or, in the case of a blob, via flag (--certificate or --bundle)

asraa · 2023-01-17T16:14:59Z

pkg/cosign/verify.go

-	// We have a public key.
-	if co.SigVerifier != nil {
+	switch {
+	// certificate provided from the OCI image, or via flag (--certificate or --bundle)


Sigh…maybe we could add a check that a provided cert matches what’s on the image annotations? Or disallow providing a certificate when the image annotations contain a certificate already?

+1, see my comment below. I think --certificate is a little misleading here. The --certificate flag for OCI image never "makes" it into the pkg verification code (just the pub key does). In the CLI layer there's some verification of it, but once we get into pkg/verify, we rely on sig.Cert().

haydentherapper · 2023-01-18T00:07:03Z

After trying a few different approaches, the cleanest is to override sig.Cert() (if not set). We don't have any issues in verify-blob because we are setting sig.Cert() based on --certificate. We should be doing the same for the OCI case, just with an additional check if there's any difference. Then the logic in verifyInternal doesn't have to change. Adding a field to CheckOpts like Nathan suggested for passing the certificate. (This struct is TOO BIG)

haydentherapper · 2023-01-18T00:45:27Z

OK, so after repeatedly typing lines and deleting them, here's what I think I'll do:

Add VerifierCert to CheckOpts (co)
We need to predictably set co.SigVerifier. I'm going to remove setting SigVerifier when a cert is passed, so that SigVerifier == nil means verify with a certificate. Effectively now co.SigVerifier is just a provided public key. (maybe even rename it to PublicKey)
We will pull the cert from either sig.Cert or co.VerifierCert, and pull chain from either sig.Chain or co.Roots/co.Intermediates (which will have been initialized with either a provided chain or the Fulcio/TUF certs). It will prefer co over sig for chain (current behavior), and check for equality for cert.
We will make the change here to read either SigVerifier or the parsed cert (What if Rekor has the full chain uploaded? Currently Cosign uploads only the cert, not chain, so I think this is out of scope)

@asraa @nsmith5 Lemme know if you see any issues

nsmith5 · 2023-01-18T01:05:16Z

Yup, this sounds like the right approach to me!

github-actions · 2023-02-19T02:03:39Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2023-03-22T01:55:40Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2023-04-27T01:54:23Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

github-actions · 2023-05-07T01:58:10Z

This PR was closed because it has been stalled for 10 days with no activity.

haydentherapper requested a review from asraa January 16, 2023 03:28

haydentherapper mentioned this pull request Jan 16, 2023

bug: x509 verification broken #2632

Open

nsmith5 reviewed Jan 16, 2023

View reviewed changes

haydentherapper marked this pull request as draft January 16, 2023 04:05

asraa reviewed Jan 17, 2023

View reviewed changes

nsmith5 mentioned this pull request Jan 17, 2023

feature: support sign-blob with certificate #2636

Closed

znewman01 mentioned this pull request Jan 25, 2023

Make sure a cert passed in via --cert matches the bundle cert #2652

Merged

github-actions bot added the no-pr-activity label Feb 19, 2023

haydentherapper removed the no-pr-activity label Feb 19, 2023

github-actions bot added the no-pr-activity label Mar 22, 2023

haydentherapper removed the no-pr-activity label Mar 27, 2023

github-actions bot added the no-pr-activity label Apr 27, 2023

github-actions bot closed this May 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix verifying when a certificate is provided #2633

Fix verifying when a certificate is provided #2633

haydentherapper commented Jan 16, 2023 •

edited

Loading

haydentherapper commented Jan 16, 2023

haydentherapper commented Jan 16, 2023

haydentherapper commented Jan 16, 2023 •

edited

Loading

nsmith5 Jan 16, 2023

haydentherapper Jan 16, 2023

asraa Jan 17, 2023

haydentherapper commented Jan 16, 2023 •

edited

Loading

asraa left a comment

asraa Jan 17, 2023

asraa Jan 17, 2023

haydentherapper commented Jan 18, 2023

haydentherapper commented Jan 18, 2023

nsmith5 commented Jan 18, 2023

github-actions bot commented Feb 19, 2023

github-actions bot commented Mar 22, 2023

github-actions bot commented Apr 27, 2023

github-actions bot commented May 7, 2023

	if !bytes.Equal(pemFirst.Bytes, pemSecond.Bytes) {
	return fmt.Errorf("comparing public key PEMs, expected %s, got %s",
	pemBytes, decodeSecond)
	}

Fix verifying when a certificate is provided #2633

Fix verifying when a certificate is provided #2633

Conversation

haydentherapper commented Jan 16, 2023 • edited Loading

Summary

Release Note

Documentation

haydentherapper commented Jan 16, 2023

haydentherapper commented Jan 16, 2023

haydentherapper commented Jan 16, 2023 • edited Loading

nsmith5 Jan 16, 2023

Choose a reason for hiding this comment

haydentherapper Jan 16, 2023

Choose a reason for hiding this comment

asraa Jan 17, 2023

Choose a reason for hiding this comment

haydentherapper commented Jan 16, 2023 • edited Loading

asraa left a comment

Choose a reason for hiding this comment

asraa Jan 17, 2023

Choose a reason for hiding this comment

asraa Jan 17, 2023

Choose a reason for hiding this comment

haydentherapper commented Jan 18, 2023

haydentherapper commented Jan 18, 2023

nsmith5 commented Jan 18, 2023

github-actions bot commented Feb 19, 2023

github-actions bot commented Mar 22, 2023

github-actions bot commented Apr 27, 2023

github-actions bot commented May 7, 2023

haydentherapper commented Jan 16, 2023 •

edited

Loading

haydentherapper commented Jan 16, 2023 •

edited

Loading

haydentherapper commented Jan 16, 2023 •

edited

Loading