Merge pull request #2524 from mtrmac/sigstore-multi

Add  `keyPaths`, `keyDatas` to `prSigstoreSigned`

docs/containers-policy.json.5.md

@@ -320,7 +320,9 @@ This requirement requires an image to be signed using a sigstore signature with
 {
     "type":    "sigstoreSigned",
     "keyPath": "/path/to/local/public/key/file",
+    "keyPaths": ["/path/to/first/public/key/one", "/path/to/first/public/key/two"],
     "keyData": "base64-encoded-public-key-data",
+    "keyDatas": ["base64-encoded-public-key-one-data", "base64-encoded-public-key-two-data"]
     "fulcio": {
         "caPath": "/path/to/local/CA/file",
         "caData": "base64-encoded-CA-data",
@@ -332,11 +334,14 @@ This requirement requires an image to be signed using a sigstore signature with
     "signedIdentity": identity_requirement
 }
 ```
-Exactly one of `keyPath`, `keyData` and `fulcio` must be present.
+Exactly one of `keyPath`, `keyPaths`, `keyData`, `keyDatas` and `fulcio` must be present.
 
 If `keyPath` or `keyData` is present, it contains a sigstore public key.
 Only signatures made by this key are accepted.
 
+If `keyPaths` or `keyDatas` is present, it contains sigstore public keys.
+Only signatures made by any key in the list are accepted.
+
 If `fulcio` is present, the signature must be based on a Fulcio-issued certificate.
 One of `caPath` and `caData` must be specified, containing the public key of the Fulcio instance.
 Both `oidcIssuer` and `subjectEmail` are mandatory,

signature/internal/sigstore_payload.go

@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/containers/image/v5/version"
@@ -165,24 +166,62 @@ type SigstorePayloadAcceptanceRules struct {
 	ValidateSignedDockerManifestDigest func(digest.Digest) error
 }
 
-// VerifySigstorePayload verifies unverifiedBase64Signature of unverifiedPayload was correctly created by publicKey, and that its principal components
+// verifySigstorePayloadBlobSignature verifies unverifiedSignature of unverifiedPayload was correctly created
+// by any of the public keys in publicKeys.
+//
+// This is an internal implementation detail of VerifySigstorePayload and should have no other callers.
+// It is INSUFFICIENT alone to consider the signature acceptable.
+func verifySigstorePayloadBlobSignature(publicKeys []crypto.PublicKey, unverifiedPayload, unverifiedSignature []byte) error {
+	if len(publicKeys) == 0 {
+		return errors.New("Need at least one public key to verify the sigstore payload, but got 0")
+	}
+
+	verifiers := make([]sigstoreSignature.Verifier, 0, len(publicKeys))
+	for _, key := range publicKeys {
+		// Failing to load a verifier indicates that something is really, really
+		// invalid about the public key; prefer to fail even if the signature might be
+		// valid with other keys, so that users fix their fallback keys before they need them.
+		// For that reason, we even initialize all verifiers before trying to validate the signature
+		// with any key.
+		verifier, err := sigstoreSignature.LoadVerifier(key, sigstoreHarcodedHashAlgorithm)
+		if err != nil {
+			return err
+		}
+		verifiers = append(verifiers, verifier)
+	}
+
+	var failures []string
+	for _, verifier := range verifiers {
+		// github.com/sigstore/cosign/pkg/cosign.verifyOCISignature uses signatureoptions.WithContext(),
+		// which seems to be not used by anything. So we don’t bother.
+		err := verifier.VerifySignature(bytes.NewReader(unverifiedSignature), bytes.NewReader(unverifiedPayload))
+		if err == nil {
+			return nil
+		}
+
+		failures = append(failures, err.Error())
+	}
+
+	if len(failures) == 0 {
+		// Coverage: We have checked there is at least one public key, any success causes an early return,
+		// and any failure adds an entry to failures => there must be at least one error.
+		return fmt.Errorf("Internal error: signature verification failed but no errors have been recorded")
+	}
+	return NewInvalidSignatureError("cryptographic signature verification failed: " + strings.Join(failures, ", "))
+}
+
+// VerifySigstorePayload verifies unverifiedBase64Signature of unverifiedPayload was correctly created by any of the public keys in publicKeys, and that its principal components
 // match expected values, both as specified by rules, and returns it.
 // We return an *UntrustedSigstorePayload, although nothing actually uses it,
 // just to double-check against stupid typos.
-func VerifySigstorePayload(publicKey crypto.PublicKey, unverifiedPayload []byte, unverifiedBase64Signature string, rules SigstorePayloadAcceptanceRules) (*UntrustedSigstorePayload, error) {
-	verifier, err := sigstoreSignature.LoadVerifier(publicKey, sigstoreHarcodedHashAlgorithm)
-	if err != nil {
-		return nil, fmt.Errorf("creating verifier: %w", err)
-	}
-
+func VerifySigstorePayload(publicKeys []crypto.PublicKey, unverifiedPayload []byte, unverifiedBase64Signature string, rules SigstorePayloadAcceptanceRules) (*UntrustedSigstorePayload, error) {
 	unverifiedSignature, err := base64.StdEncoding.DecodeString(unverifiedBase64Signature)
 	if err != nil {
 		return nil, NewInvalidSignatureError(fmt.Sprintf("base64 decoding: %v", err))
 	}
-	// github.com/sigstore/cosign/pkg/cosign.verifyOCISignature uses signatureoptions.WithContext(),
-	// which seems to be not used by anything. So we don’t bother.
-	if err := verifier.VerifySignature(bytes.NewReader(unverifiedSignature), bytes.NewReader(unverifiedPayload)); err != nil {
-		return nil, NewInvalidSignatureError(fmt.Sprintf("cryptographic signature verification failed: %v", err))
+
+	if err := verifySigstorePayloadBlobSignature(publicKeys, unverifiedPayload, unverifiedSignature); err != nil {
+		return nil, err
 	}
 
 	var unmatchedPayload UntrustedSigstorePayload

signature/internal/sigstore_payload_test.go

@@ -2,6 +2,7 @@ package internal
 
 import (
 	"bytes"
+	"crypto"
 	"encoding/base64"
 	"encoding/json"
 	"errors"
@@ -204,11 +205,18 @@ func TestUntrustedSigstorePayloadUnmarshalJSON(t *testing.T) {
 	assert.Equal(t, validSig, s)
 }
 
+// verifySigstorePayloadBlobSignature is tested by TestVerifySigstorePayload
+
 func TestVerifySigstorePayload(t *testing.T) {
 	publicKeyPEM, err := os.ReadFile("./testdata/cosign.pub")
 	require.NoError(t, err)
 	publicKey, err := cryptoutils.UnmarshalPEMToPublicKey(publicKeyPEM)
 	require.NoError(t, err)
+	publicKeyPEM2, err := os.ReadFile("./testdata/cosign2.pub")
+	require.NoError(t, err)
+	publicKey2, err := cryptoutils.UnmarshalPEMToPublicKey(publicKeyPEM2)
+	require.NoError(t, err)
+	singlePublicKey := []crypto.PublicKey{publicKey}
 
 	type acceptanceData struct {
 		signedDockerReference      string
@@ -248,36 +256,55 @@ func TestVerifySigstorePayload(t *testing.T) {
 	}
 
 	// Successful verification
-	wanted = signatureData
-	recorded = acceptanceData{}
-	res, err := VerifySigstorePayload(publicKey, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
-	require.NoError(t, err)
-	assert.Equal(t, res, &UntrustedSigstorePayload{
-		untrustedDockerManifestDigest: TestSigstoreManifestDigest,
-		untrustedDockerReference:      TestSigstoreSignatureReference,
-		untrustedCreatorID:            nil,
-		untrustedTimestamp:            nil,
-	})
-	assert.Equal(t, signatureData, recorded)
+	for _, publicKeys := range [][]crypto.PublicKey{
+		singlePublicKey,
+		{publicKey, publicKey2}, // The matching key is first
+		{publicKey2, publicKey}, // The matching key is second
+	} {
+		wanted = signatureData
+		recorded = acceptanceData{}
+		res, err := VerifySigstorePayload(publicKeys, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
+		require.NoError(t, err)
+		assert.Equal(t, res, &UntrustedSigstorePayload{
+			untrustedDockerManifestDigest: TestSigstoreManifestDigest,
+			untrustedDockerReference:      TestSigstoreSignatureReference,
+			untrustedCreatorID:            nil,
+			untrustedTimestamp:            nil,
+		})
+		assert.Equal(t, signatureData, recorded)
+	}
 
 	// For extra paranoia, test that we return a nil signature object on error.
 
-	// Invalid verifier
-	recorded = acceptanceData{}
-	invalidPublicKey := struct{}{} // crypto.PublicKey is, for some reason, just an any, so this is acceptable.
-	res, err = VerifySigstorePayload(invalidPublicKey, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
-	assert.Error(t, err)
-	assert.Nil(t, res)
-	assert.Equal(t, acceptanceData{}, recorded)
-
 	// Invalid base64 encoding
 	for _, invalidBase64Sig := range []string{
 		"&",                                      // Invalid base64 characters
 		cryptoBase64Sig + "=",                    // Extra padding
 		cryptoBase64Sig[:len(cryptoBase64Sig)-1], // Truncated base64 data
 	} {
 		recorded = acceptanceData{}
-		res, err = VerifySigstorePayload(publicKey, sigstoreSig.UntrustedPayload(), invalidBase64Sig, recordingRules)
+		res, err := VerifySigstorePayload(singlePublicKey, sigstoreSig.UntrustedPayload(), invalidBase64Sig, recordingRules)
+		assert.Error(t, err)
+		assert.Nil(t, res)
+		assert.Equal(t, acceptanceData{}, recorded)
+	}
+
+	// No public keys
+	recorded = acceptanceData{}
+	res, err := VerifySigstorePayload([]crypto.PublicKey{}, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
+	assert.Error(t, err)
+	assert.Nil(t, res)
+	assert.Equal(t, acceptanceData{}, recorded)
+
+	// Invalid verifier:
+	// crypto.PublicKey is, for some reason, just an any, so using a struct{}{} to trigger this code path works.
+	for _, invalidPublicKeys := range [][]crypto.PublicKey{
+		{struct{}{}},            // A single invalid key
+		{struct{}{}, publicKey}, // An invalid key, followed by a matching key
+		{publicKey, struct{}{}}, // A matching key, but the configuration also includes an invalid key
+	} {
+		recorded = acceptanceData{}
+		res, err = VerifySigstorePayload(invalidPublicKeys, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
 		assert.Error(t, err)
 		assert.Nil(t, res)
 		assert.Equal(t, acceptanceData{}, recorded)
@@ -292,22 +319,34 @@ func TestVerifySigstorePayload(t *testing.T) {
 		append(bytes.Clone(validSignatureBytes), validSignatureBytes...),
 	} {
 		recorded = acceptanceData{}
-		res, err = VerifySigstorePayload(publicKey, sigstoreSig.UntrustedPayload(), base64.StdEncoding.EncodeToString(invalidSig), recordingRules)
+		res, err = VerifySigstorePayload(singlePublicKey, sigstoreSig.UntrustedPayload(), base64.StdEncoding.EncodeToString(invalidSig), recordingRules)
+		assert.Error(t, err)
+		assert.Nil(t, res)
+		assert.Equal(t, acceptanceData{}, recorded)
+	}
+
+	// No matching public keys
+	for _, nonmatchingPublicKeys := range [][]crypto.PublicKey{
+		{publicKey2},
+		{publicKey2, publicKey2},
+	} {
+		recorded = acceptanceData{}
+		res, err = VerifySigstorePayload(nonmatchingPublicKeys, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
 		assert.Error(t, err)
 		assert.Nil(t, res)
 		assert.Equal(t, acceptanceData{}, recorded)
 	}
 
 	// Valid signature of non-JSON
 	recorded = acceptanceData{}
-	res, err = VerifySigstorePayload(publicKey, []byte("&"), "MEUCIARnnxZQPALBfqkB4aNAYXad79Qs6VehcrgIeZ8p7I2FAiEAzq2HXwXlz1iJeh+ucUR3L0zpjynQk6Rk0+/gXYp49RU=", recordingRules)
+	res, err = VerifySigstorePayload(singlePublicKey, []byte("&"), "MEUCIARnnxZQPALBfqkB4aNAYXad79Qs6VehcrgIeZ8p7I2FAiEAzq2HXwXlz1iJeh+ucUR3L0zpjynQk6Rk0+/gXYp49RU=", recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, res)
 	assert.Equal(t, acceptanceData{}, recorded)
 
 	// Valid signature of an unacceptable JSON
 	recorded = acceptanceData{}
-	res, err = VerifySigstorePayload(publicKey, []byte("{}"), "MEUCIQDkySOBGxastVP0+koTA33NH5hXjwosFau4rxTPN6g48QIgb7eWKkGqfEpHMM3aT4xiqyP/170jEkdFuciuwN4mux4=", recordingRules)
+	res, err = VerifySigstorePayload(singlePublicKey, []byte("{}"), "MEUCIQDkySOBGxastVP0+koTA33NH5hXjwosFau4rxTPN6g48QIgb7eWKkGqfEpHMM3aT4xiqyP/170jEkdFuciuwN4mux4=", recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, res)
 	assert.Equal(t, acceptanceData{}, recorded)
@@ -316,7 +355,7 @@ func TestVerifySigstorePayload(t *testing.T) {
 	wanted = signatureData
 	wanted.signedDockerManifestDigest = "invalid digest"
 	recorded = acceptanceData{}
-	res, err = VerifySigstorePayload(publicKey, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
+	res, err = VerifySigstorePayload(singlePublicKey, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, res)
 	assert.Equal(t, acceptanceData{
@@ -327,7 +366,7 @@ func TestVerifySigstorePayload(t *testing.T) {
 	wanted = signatureData
 	wanted.signedDockerReference = "unexpected docker reference"
 	recorded = acceptanceData{}
-	res, err = VerifySigstorePayload(publicKey, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
+	res, err = VerifySigstorePayload(singlePublicKey, sigstoreSig.UntrustedPayload(), cryptoBase64Sig, recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, res)
 	assert.Equal(t, signatureData, recorded)

signature/internal/testdata/cosign2.pub

@@ -0,0 +1 @@
+../../fixtures/cosign2.pub