fix for issue 3476 #3486

Mukuls77 · 2024-01-18T07:02:37Z

Summary

The PR fixes the issue "Closes #3476"
Use case Impacted:
A company sign the artifact using its PKI using
companyRootCA->companyIntermediateCrt->CompanyLeafcert
Client download the software and want to sign it again using client PKI before it can be used using
ClientRootCA->clientIntermediatecrt->Clientleafcrt
so in this case we will have multiple signatures attached to the artifact with different Root->intermediate->leaf chains.
Now if client verify the artifact just passing the client RootCA, the verification fails randomly.

The main issue identified is that
func verifySignatures(ctx context.Context, sigs oci.Signatures, h v1.Hash, co *CheckOpts) (checkedSignatures []oci.Signature, bundleVerified bool, err error)
function invokes multiple worker thread and call the function VerifyImageSignature() by passing common CheckOpts reference.
now in downstream the function
func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash,
verifyFn signatureVerificationFn, co *CheckOpts)
try to update the co.IntermediateCerts if it is null. ( which will be the case in this scenario as we are just passing root certificate for verification)
since this function is being executed by multiple threads so it can corrupt the co.IntermediateCerts (which ever thread find it Null)
This can corrupt the verification chain , so sometime verification fails.
To solve this issue a new function is introduced
func ValidateAndUnpackCertWithIntermediates(cert *x509.Certificate, co *CheckOpts, intermediateCerts *x509.CertPool) (signature.Verifier, error)
this has intermediate as separate argument. this new function is now called inside verifyInternal()

Release Note

File modified:
cosign/pkg/cosign/verify.go

Documentation

no change in existing API

Signed-off-by: Mukuls77 <[email protected]>

codecov · 2024-01-18T07:05:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (26e8440) 40.07% compared to head (0aea637) 40.10%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3486      +/-   ##
==========================================
+ Coverage   40.07%   40.10%   +0.02%     
==========================================
  Files         155      155              
  Lines       10040    10044       +4     
==========================================
+ Hits         4024     4028       +4     
  Misses       5530     5530              
  Partials      486      486

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cpanato · 2024-01-18T08:14:46Z

pkg/cosign/verify.go

+// as it uses a common reference of CheckOpts so in multi thread invocation co.intermediateCert object
+// can get overwritten my multiple threads. So to solve this ValidateAndUnpackCertWithIntermediates
+// is created which has a separate argument for Intermediate certs.
+


nit: remove this blank line

Thanks for the review i will remove the empty line

cpanato

there is any tests that we can add or the existing ones cover this change?

Mukuls77 · 2024-01-18T08:50:45Z

The new function written ValidateAndUnpackCertWithIntermediates()
is covered by already existing verify testing function like
TestVerifyImageSignature()
TestVerifyImageSignatureMultipleSubs()
TestVerifyImageSignatureWithNoChain()
TestVerifyImageSignatureWithOnlyRoot()
TestVerifyImageSignatureWithMissingSub ()

also test function TestVerifyImageSignature()
create an oci.sig with Root->Intermediate->leaf cert
and verify it using only root cert.

The only logic which existing function can not test is that we need to invoke verification on Multiple Signature case. i am not sure if we can create a unit test for the same. But if required i can create an end to end test where we can attach multiple signatures with different root chains and do verification using any of the root certificate. Pls suggest shall we create an end to end test case for this.

Mukuls77 · 2024-01-18T09:28:34Z

i propose to enhance existing end to end test case
cosign/test/e2e_test_attach.sh
to attach another signature to the artifact with different root chain and verify it using root certificate only.

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 · 2024-01-18T11:31:11Z

I have enhanced end to end attach test case to attach two signatures with different root chains and tested the verification using root certificate only. attached are the logs of the testing done.
e2e_attach_test_logs.txt

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 · 2024-01-18T13:15:21Z

@haydentherapper ; @cpanato
I have done the required changes and added the end to end test case to test multiple signatures with different root chains. kindly review

pkg/cosign/verify.go

haydentherapper · 2024-01-18T18:56:55Z

pkg/cosign/verify.go

+	}
+
+	// Now verify the cert, then the signature.
+	if intermediateCerts == nil {


I would remove this conditional, as the purpose of this function is to pass in the intermediates that should be used for chain building.

This check is important and should be present.
we will reach the new function ValidateAndUnpackCertWithIntermediates() via two paths

ValidateAndUnpackCert()

verifyInternal()

verifyInternal is called in some cases where we dont have an intermediate present in signature but we pass that as an argument in verification in those cases co.IntermediateCerts will have a valid intermediate cert but we will not be able to set the intermediatePool argument. one scenario is verification of Blob signatures.
so we need to have this condition if the intermediate Argument is Null that set the value from co.IntermediateCerts
I tested this by removing this condition and i could see verify blob signature test cases were failing so i put this condition back.

Ah I think I understand the issue. Correct me if I misunderstand, you're saying that co.IntermediateCerts may have been set by one of the functions that calls verifyInternal.

I think the fix would just be to pass co.IntermediateCerts to ValidateAndUnpackCertWithIntermediates on line 745 if co.IntermediateCerts != nil. Then you could remove this check.

haydentherapper · 2024-01-18T18:58:52Z

pkg/cosign/verify.go

@@ -721,19 +803,19 @@ func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash,
 			return false, err
 		}
 		// If there is no chain annotation present, we preserve the pools set in the CheckOpts.
+		var pool = (*x509.CertPool)(nil)
 		if len(chain) > 0 {
 			if len(chain) == 1 {
 				co.IntermediateCerts = nil


This line needs to be updated too, we can't overwrite co.IntermediateCerts

Agreed i will incorporate this comment

haydentherapper · 2024-01-18T18:59:01Z

pkg/cosign/verify.go

@@ -721,19 +803,19 @@ func verifyInternal(ctx context.Context, sig oci.Signature, h v1.Hash,
 			return false, err
 		}
 		// If there is no chain annotation present, we preserve the pools set in the CheckOpts.
+		var pool = (*x509.CertPool)(nil)


Suggested change

var pool = (*x509.CertPool)(nil)

var pool *x509.CertPool

This should be sufficient to create a pointer.

Agreed i will incorporate this comment

pkg/cosign/verify.go

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 · 2024-01-19T10:53:00Z

@haydentherapper i have implemented the review comments. kindly review

haydentherapper · 2024-01-19T22:29:41Z

pkg/cosign/verify.go

@@ -216,9 +216,16 @@ func verifyOCISignature(ctx context.Context, verifier signature.Verifier, sig pa
 	return verifier.VerifySignature(bytes.NewReader(signature), bytes.NewReader(payload), options.WithContext(ctx))
 }

-// ValidateAndUnpackCert creates a Verifier from a certificate. Veries that the certificate
-// chains up to a trusted root. Optionally verifies the subject and issuer of the certificate.
+// ValidateAndUnpackCert calls ValidateAndUnpackCertWithIntermediates() by passing intermediate


I'd keep the previous comment, just adding the detail that the intermediate certificate chain comes from CheckOpts. You can omit the detail that it calls another function.

haydentherapper · 2024-01-19T22:46:19Z

pkg/cosign/verify.go

+	}
+
+	// Now verify the cert, then the signature.
+	if intermediateCerts == nil {


Ah I think I understand the issue. Correct me if I misunderstand, you're saying that co.IntermediateCerts may have been set by one of the functions that calls verifyInternal.

I think the fix would just be to pass co.IntermediateCerts to ValidateAndUnpackCertWithIntermediates on line 745 if co.IntermediateCerts != nil. Then you could remove this check.

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 · 2024-01-20T05:45:26Z

@haydentherapper i have incorporated the review comments pls review

haydentherapper

Thanks for the fix!

pkg/cosign/verify_test.go

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 · 2024-01-23T06:34:53Z

@haydentherapper i have incorporated the review comment. please check

haydentherapper · 2024-01-23T22:34:06Z

test/e2e_test_attach.sh

@@ -25,6 +25,10 @@ cp ./test/testdata/test_attach_private_key $tmp/private_key
 cp ./test/testdata/test_attach_leafcert.pem $tmp/leafcert.pem
 cp ./test/testdata/test_attach_certchain.pem $tmp/certchain.pem
 cp ./test/testdata/test_attach_rootcert.pem $tmp/rootcert.pem
+cp ./test/testdata/test_attach_second_private_key $tmp/secondprivate_key


@Mukuls77 What is the purpose of this extra set of certificates? It looks like this is testing the same behavior, are these e2e test changes needed?

This test case creates the scenario when multiple signatures are attached to a artifact with different root chains and we want to verify the artifact with any one of the root certificate. because there are more than one signatures attached so multiple threads will be created during verification and each thread will verify the allocated signature. During this operation previously when threads were modifying co.intermediates sometime verification use to fail, now with this fix it will not fail. so in my view this is an important test cases of verifying multiple signatures.

Thanks, that makes sense. Can you add a comment about this in the test file?

haydentherapper · 2024-01-23T22:34:24Z

LGTM, just one comment about tests

hectorj2f

lgtm

Mukuls77 · 2024-01-25T07:16:35Z

@haydentherapper @hectorj2f
if there are no further comments i request to merge the pull request.
Thanks

haydentherapper · 2024-01-25T08:11:22Z

test/e2e_test_attach.sh

@@ -25,6 +25,10 @@ cp ./test/testdata/test_attach_private_key $tmp/private_key
 cp ./test/testdata/test_attach_leafcert.pem $tmp/leafcert.pem
 cp ./test/testdata/test_attach_certchain.pem $tmp/certchain.pem
 cp ./test/testdata/test_attach_rootcert.pem $tmp/rootcert.pem
+cp ./test/testdata/test_attach_second_private_key $tmp/secondprivate_key


Thanks, that makes sense. Can you add a comment about this in the test file?

haydentherapper · 2024-01-25T08:12:10Z

test/e2e_test_attach.sh

@@ -59,24 +67,22 @@ echo "Payload: $PAYLOAD"

 ## Attach Signature, payload, cert and cert-chain
 ./cosign attach signature --signature ./payloadbase64.sig --payload ./payload.json --cert ./leafcert.pem --cert-chain ./certchain.pem $IMAGE_URI_DIGEST
+./cosign attach signature --signature ./secondpayloadbase64.sig --payload ./payload.json --cert ./secondleafcert.pem --cert-chain ./secondcertchain.pem $IMAGE_URI_DIGEST


Can we modify this test so that we still test verification with a single attached cert, then after verification, attach another and try verification?

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 · 2024-01-25T09:42:29Z

@haydentherapper i have incorporated the review comments. kindly check

haydentherapper

Thanks @Mukuls77!

ret2libc · 2024-01-26T13:45:32Z

What is the difference between the new ValidateAndUnpackCertWithIntermediates and ValidateAndUnpackCertWithChain?

Mukuls77 · 2024-01-26T13:58:45Z

Main difference is Function ValidateAndUnpackCertWithChain() uses intermediate cert from co.Intermediatecerts to construct the verification chain to verify the certificates. Function ValidateAndUnpackCertWithIntermediates() provide the caller to pass intermediate cert as a separate argument and use that for constructing the certificate chain for cert verification. Function ValidateAndUnpackCertWithChain() internally calls ValidateAndUnpackCertWithIntermediates() by passing co.Intermediatecerts in the Intermediatecert Argument of the function.

…

On Fri, Jan 26, 2024 at 7:15 PM Riccardo Schirone ***@***.***> wrote: What is the difference between the new ValidateAndUnpackCertWithIntermediates and ValidateAndUnpackCertWithChain? — Reply to this email directly, view it on GitHub <#3486 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A46KPVI6BITZUKXWUGGBZWTYQOXQRAVCNFSM6AAAAABB7YF77KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMJSGA4TGMJWG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

haydentherapper · 2024-01-26T17:05:02Z

Purpose was a nonbreaking API change. Long term there's a larger refactor to do to pass a set of read-only CA cert metadata to the verification function.

Mukuls77 · 2024-01-27T05:12:41Z

@haydentherapper thanks for Merging the pull request. I suppose this fix will be available in 2.3.0 release, i wanted to check what is the expected timeframe of next cosign release.
Also i have one query if i see the cosign main page it shows

when i checked further i see cross build to macOS is failing in upload artifact step

so wanted to check if there is some action i can do to solve this issue,

haydentherapper · 2024-01-27T06:07:52Z

I'll likely cut a release next week or so, there's a few open PRs we should get in first.

I've been noticing that failure too, I'm not sure the root cause, but it's not related to this PR specifically.

fix for issue 3476

c3db7a8

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 mentioned this pull request Jan 18, 2024

cosign verify fails randomly when we have multiple signatures attached to image #3476

Closed

cpanato reviewed Jan 18, 2024

View reviewed changes

fix for issue 3476

f69f351

Signed-off-by: Mukuls77 <[email protected]>

fix for whitespace in e2e attach tet script

5860b37

Signed-off-by: Mukuls77 <[email protected]>

haydentherapper requested changes Jan 18, 2024

View reviewed changes

Mukuls77 added 2 commits January 19, 2024 14:18

Added Unit test and incorporated review comments

f4f31d0

Signed-off-by: Mukuls77 <[email protected]>

incorporated review comments

ce38bd7

Signed-off-by: Mukuls77 <[email protected]>

haydentherapper reviewed Jan 19, 2024

View reviewed changes

incorporated review comments

8743a71

Signed-off-by: Mukuls77 <[email protected]>

haydentherapper previously approved these changes Jan 22, 2024

View reviewed changes

haydentherapper reviewed Jan 22, 2024

View reviewed changes

pkg/cosign/verify_test.go Show resolved Hide resolved

Mukuls77 dismissed haydentherapper’s stale review via 63a9897 January 23, 2024 05:45

incorporated review comments

63a9897

Signed-off-by: Mukuls77 <[email protected]>

haydentherapper reviewed Jan 23, 2024

View reviewed changes

hectorj2f previously approved these changes Jan 24, 2024

View reviewed changes

haydentherapper requested changes Jan 25, 2024

View reviewed changes

incorporated review comments

d6a95df

Signed-off-by: Mukuls77 <[email protected]>

Mukuls77 dismissed hectorj2f’s stale review via d6a95df January 25, 2024 09:05

incorporated review comments

0aea637

Signed-off-by: Mukuls77 <[email protected]>

haydentherapper approved these changes Jan 25, 2024

View reviewed changes

haydentherapper merged commit f43eb6b into sigstore:main Jan 25, 2024
28 checks passed

github-actions bot added this to the v2.3.0 milestone Jan 25, 2024

fix for issue 3476 #3486

fix for issue 3476 #3486

Conversation

Mukuls77 commented Jan 18, 2024

Summary

Release Note

Documentation

codecov bot commented Jan 18, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpanato left a comment

Choose a reason for hiding this comment

Mukuls77 commented Jan 18, 2024

Mukuls77 commented Jan 18, 2024

Mukuls77 commented Jan 18, 2024

Mukuls77 commented Jan 18, 2024

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

Mukuls77 commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mukuls77 commented Jan 20, 2024

haydentherapper left a comment

Choose a reason for hiding this comment

Mukuls77 commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haydentherapper commented Jan 23, 2024

hectorj2f left a comment

Choose a reason for hiding this comment

Mukuls77 commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mukuls77 commented Jan 25, 2024

haydentherapper left a comment

Choose a reason for hiding this comment

ret2libc commented Jan 26, 2024

Mukuls77 commented Jan 26, 2024 via email

haydentherapper commented Jan 26, 2024

Mukuls77 commented Jan 27, 2024

haydentherapper commented Jan 27, 2024

codecov bot commented Jan 18, 2024 •

edited

Loading