Fix OCI verification with local cert - old bundle

`SigVerifier` is set on `CheckOpts` no matter whether the verifying
material is a public key or a certificate. The `keyBytes` function used
for comparing the verifier object was incorrectly assuming that
`SigVerifier` being non-nil meant that a local certificate was not
present and was returning the certificate's public key instead of the
certificate. This change fixes the function to check for a certificate
before returning the bytes from the public key.

It is also possible to provide a public key as an argument when a
certificate is in the signature bundle. It does not make sense to try to
provide both, but this has been allowed and asserted in
verify_blob_test.go. In this case, the comparer should ensure that the
certificate and public key are related to each other. The alternative is
to disallow providing a key as a command argument when a certificate is
present in the signature, but this could be considered a breaking
change.

This change only applies to verifying images using the old signature
format. For the new bundle format, the certificate verification goes
through a different path.

Signed-off-by: Colleen Murphy <[email protected]>

Author: cmurphy

cmd/cosign/cli/verify/verify_blob_test.go

@@ -162,6 +162,18 @@ func TestVerifyBlob(t *testing.T) {
 		time.Now().Add(-time.Hour), leafPriv, rootCert, rootPriv)
 	expiredLeafPem, _ := cryptoutils.MarshalCertificateToPEM(expiredLeafCert)
 
+	unrelatedPriv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatal(err)
+	}
+	unrelatedSigner, err := signature.LoadECDSASignerVerifier(unrelatedPriv, crypto.SHA256)
+	if err != nil {
+		t.Fatal(err)
+	}
+	unrelatedLeafCert, _ := test.GenerateLeafCertWithExpiration(identity, issuer,
+		time.Now(), unrelatedPriv, rootCert, rootPriv)
+	unrelatedCertPem, _ := cryptoutils.MarshalCertificateToPEM(unrelatedLeafCert)
+
 	// Make rekor signer
 	rekorPriv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 	if err != nil {
@@ -178,18 +190,20 @@ func TestVerifyBlob(t *testing.T) {
 	tmpRekorPubFile := writeBlobFile(t, td, string(pemRekor), "rekor_pub.key")
 	t.Setenv("SIGSTORE_REKOR_PUBLIC_KEY", tmpRekorPubFile)
 
-	var makeSignature = func(blob []byte) string {
+	var makeSignature = func(blob []byte, signer signature.SignerVerifier) string {
 		sig, err := signer.SignMessage(bytes.NewReader(blob))
 		if err != nil {
 			t.Fatal(err)
 		}
 		return string(sig)
 	}
 	blobBytes := []byte("foo")
-	blobSignature := makeSignature(blobBytes)
+	blobSignature := makeSignature(blobBytes, signer)
 
 	otherBytes := []byte("bar")
-	otherSignature := makeSignature(otherBytes)
+	otherSignature := makeSignature(otherBytes, signer)
+
+	unrelatedSignature := makeSignature(blobBytes, unrelatedSigner)
 
 	// initialize timestamp for expired and unexpired certificates
 	expiredTSAOpts := mock.TSAClientOptions{Time: time.Now().Add(-time.Hour), Message: []byte(blobSignature)}
@@ -300,18 +314,27 @@ func TestVerifyBlob(t *testing.T) {
 		{
 			name:      "valid signature with public key - bad bundle cert mismatch",
 			blob:      blobBytes,
+			signature: unrelatedSignature,
+			key:       pubKeyBytes,
+			bundlePath: makeLocalBundle(t, *rekorSigner, blobBytes, []byte(unrelatedSignature),
+				unrelatedCertPem, true),
+			shouldErr: true,
+		},
+		{
+			name:      "valid signature with public key and bundle cert derived from public key",
+			blob:      blobBytes,
 			signature: blobSignature,
 			key:       pubKeyBytes,
 			bundlePath: makeLocalBundle(t, *rekorSigner, blobBytes, []byte(blobSignature),
 				unexpiredCertPem, true),
-			shouldErr: true,
+			shouldErr: false,
 		},
 		{
 			name:      "valid signature with public key - bad bundle signature mismatch",
 			blob:      blobBytes,
 			signature: blobSignature,
 			key:       pubKeyBytes,
-			bundlePath: makeLocalBundle(t, *rekorSigner, blobBytes, []byte(makeSignature(blobBytes)),
+			bundlePath: makeLocalBundle(t, *rekorSigner, blobBytes, []byte(makeSignature(blobBytes, signer)),
 				pubKeyBytes, true),
 			shouldErr: true,
 		},
@@ -365,21 +388,12 @@ func TestVerifyBlob(t *testing.T) {
 			cert:      unexpiredLeafCert,
 			shouldErr: true,
 		},
-		{
-			name:      "valid signature with unexpired certificate - bad bundle cert mismatch",
-			blob:      blobBytes,
-			signature: blobSignature,
-			key:       pubKeyBytes,
-			bundlePath: makeLocalBundle(t, *rekorSigner, blobBytes, []byte(blobSignature),
-				unexpiredCertPem, true),
-			shouldErr: true,
-		},
 		{
 			name:      "valid signature with unexpired certificate - bad bundle signature mismatch",
 			blob:      blobBytes,
 			signature: blobSignature,
 			cert:      unexpiredLeafCert,
-			bundlePath: makeLocalBundle(t, *rekorSigner, blobBytes, []byte(makeSignature(blobBytes)),
+			bundlePath: makeLocalBundle(t, *rekorSigner, blobBytes, []byte(makeSignature(blobBytes, signer)),
 				unexpiredCertPem, true),
 			shouldErr: true,
 		},

pkg/cosign/verify.go

@@ -945,15 +945,23 @@ func keyBytes(sig oci.Signature, co *CheckOpts) ([]byte, error) {
 	if err != nil {
 		return nil, err
 	}
-	// We have a public key.
+	var pub crypto.PublicKey
 	if co.SigVerifier != nil {
-		pub, err := co.SigVerifier.PublicKey(co.PKOpts...)
+		pub, err = co.SigVerifier.PublicKey(co.PKOpts...)
 		if err != nil {
 			return nil, err
 		}
-		return cryptoutils.MarshalPublicKeyToPEM(pub)
 	}
-	return cryptoutils.MarshalCertificateToPEM(cert)
+	if cert != nil && co.SigVerifier != nil {
+		if err := cryptoutils.EqualKeys(cert.PublicKey, pub); err != nil {
+			return nil, fmt.Errorf("both public key and certificate were provided but did not match")
+		}
+	}
+
+	if cert != nil {
+		return cryptoutils.MarshalCertificateToPEM(cert)
+	}
+	return cryptoutils.MarshalPublicKeyToPEM(pub)
 }
 
 // VerifyBlobSignature verifies a blob signature.

pkg/cosign/verify_test.go

@@ -326,6 +326,57 @@ func TestVerifyImageSignatureWithNoChain(t *testing.T) {
 		t.Fatalf("expected verified=true, got verified=false")
 	}
 }
+
+func TestVerifyImageSignatureWithKeyAndCert(t *testing.T) {
+	ctx := context.Background()
+	rootCert, rootKey, _ := test.GenerateRootCa()
+	sv, _, err := signature.NewECDSASignerVerifier(elliptic.P256(), rand.Reader, crypto.SHA256)
+	if err != nil {
+		t.Fatalf("creating signer: %v", err)
+	}
+
+	leafCert, privKey, _ := test.GenerateLeafCert("[email protected]", "oidc-issuer", rootCert, rootKey)
+	pemLeaf := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: leafCert.Raw})
+
+	rootPool := x509.NewCertPool()
+	rootPool.AddCert(rootCert)
+
+	payload := []byte{1, 2, 3, 4}
+	h := sha256.Sum256(payload)
+	sig, _ := privKey.Sign(rand.Reader, h[:], crypto.SHA256)
+
+	// Create a fake bundle
+	pe, _ := proposedEntries(base64.StdEncoding.EncodeToString(sig), payload, pemLeaf)
+	entry, _ := rtypes.UnmarshalEntry(pe[0])
+	leaf, _ := entry.Canonicalize(ctx)
+	rekorBundle := CreateTestBundle(ctx, t, sv, leaf)
+	pemBytes, _ := cryptoutils.MarshalPublicKeyToPEM(sv.Public())
+	rekorPubKeys := NewTrustedTransparencyLogPubKeys()
+	rekorPubKeys.AddTransparencyLogPubKey(pemBytes, tuf.Active)
+
+	opts := []static.Option{static.WithCertChain(pemLeaf, []byte{}), static.WithBundle(rekorBundle)}
+	ociSig, _ := static.NewSignature(payload, base64.StdEncoding.EncodeToString(sig), opts...)
+
+	leafSV, err := signature.LoadECDSASignerVerifier(privKey, crypto.SHA256)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	verified, err := VerifyImageSignature(context.TODO(), ociSig, v1.Hash{},
+		&CheckOpts{
+			SigVerifier:  leafSV,
+			RootCerts:    rootPool,
+			IgnoreSCT:    true,
+			Identities:   []Identity{{Subject: "[email protected]", Issuer: "oidc-issuer"}},
+			RekorPubKeys: &rekorPubKeys})
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+	if verified == false {
+		t.Fatalf("expected verified=true, got verified=false")
+	}
+}
+
 func TestVerifyImageSignatureWithInvalidPublicKeyType(t *testing.T) {
 	ctx := context.Background()
 	rootCert, rootKey, _ := test.GenerateRootCa()