Add field KeyPaths and KeyDatas to prSigstoreSigned

The new fields `KeyPaths` and `KeyDatas` is taken directly from `/etc/containers/policy.json` and allows users to provide multiple signature keys to be used to verify images. Only one of the keys has to verify, thereby this mechanism allows us to have support seamless key rotation on a registry. This fixes #2319 Signed-off-by: Dan Čermák <[email protected]> Co-authored-by: Danish Prakash <[email protected]> Signed-off-by: Miloslav Trmač <[email protected]>
containers · Aug 17, 2024 · 981cf64 · 981cf64
1 parent c7b781a
commit 981cf64
Show file tree

Hide file tree

Showing 10 changed files with 457 additions and 117 deletions.
diff --git a/docs/containers-policy.json.5.md b/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,

diff --git a/signature/internal/sigstore_payload.go b/signature/internal/sigstore_payload.go
@@ -7,6 +7,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/containers/image/v5/version"
@@ -172,35 +173,60 @@ type SigstorePayloadAcceptanceRules struct {
 }
 
 // verifySigstorePayloadBlobSignature verifies verifies unverifiedSignature of unverifiedPayload was correctly created
-// by publicKey.
+// 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(publicKey crypto.PublicKey, unverifiedPayload []byte, unverifiedSignature []byte) error {
-	verifier, err := sigstoreSignature.LoadVerifier(publicKey, sigstoreHarcodedHashAlgorithm)
-	if err != nil {
-		return err
+func verifySigstorePayloadBlobSignature(publicKeys []crypto.PublicKey, unverifiedPayload []byte, unverifiedSignature []byte) error {
+	if len(publicKeys) == 0 {
+		return fmt.Errorf("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())
 	}
 
-	// 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 NewInvalidSignatureError(fmt.Sprintf("cryptographic signature verification failed: %v", err))
+	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 nil
+	return NewInvalidSignatureError("cryptographic signature verification failed: " + strings.Join(failures, ", "))
 }
 
-// VerifySigstorePayload verifies unverifiedBase64Signature of unverifiedPayload was correctly created by publicKey, and that its principal components
+// 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) {
+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))
 	}
 
-	if err := verifySigstorePayloadBlobSignature(publicKey, unverifiedPayload, unverifiedSignature); err != nil {
+	if err := verifySigstorePayloadBlobSignature(publicKeys, unverifiedPayload, unverifiedSignature); err != nil {
 		return nil, err
 	}
 

diff --git a/signature/internal/sigstore_payload_test.go b/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)

diff --git a/signature/internal/testdata/cosign2.pub b/signature/internal/testdata/cosign2.pub
@@ -0,0 +1 @@
+../../fixtures/cosign2.pub
diff --git a/signature/policy_config_sigstore.go b/signature/policy_config_sigstore.go
@@ -21,6 +21,20 @@ func PRSigstoreSignedWithKeyPath(keyPath string) PRSigstoreSignedOption {
 	}
 }
 
+// PRSigstoreSignedWithKeyPaths specifies a value for the "keyPaths" field when calling NewPRSigstoreSigned.
+func PRSigstoreSignedWithKeyPaths(keyPaths []string) PRSigstoreSignedOption {
+	return func(pr *prSigstoreSigned) error {
+		if pr.KeyPaths != nil {
+			return InvalidPolicyFormatError(`"keyPaths" already specified`)
+		}
+		if len(keyPaths) == 0 {
+			return InvalidPolicyFormatError(`"keyPaths" contains no entries`)
+		}
+		pr.KeyPaths = keyPaths
+		return nil
+	}
+}
+
 // PRSigstoreSignedWithKeyData specifies a value for the "keyData" field when calling NewPRSigstoreSigned.
 func PRSigstoreSignedWithKeyData(keyData []byte) PRSigstoreSignedOption {
 	return func(pr *prSigstoreSigned) error {
@@ -32,6 +46,20 @@ func PRSigstoreSignedWithKeyData(keyData []byte) PRSigstoreSignedOption {
 	}
 }
 
+// PRSigstoreSignedWithKeyDatas specifies a value for the "keyDatas" field when calling NewPRSigstoreSigned.
+func PRSigstoreSignedWithKeyDatas(keyDatas [][]byte) PRSigstoreSignedOption {
+	return func(pr *prSigstoreSigned) error {
+		if pr.KeyDatas != nil {
+			return InvalidPolicyFormatError(`"keyDatas" already specified`)
+		}
+		if len(keyDatas) == 0 {
+			return InvalidPolicyFormatError(`"keyDatas" contains no entries`)
+		}
+		pr.KeyDatas = keyDatas
+		return nil
+	}
+}
+
 // PRSigstoreSignedWithFulcio specifies a value for the "fulcio" field when calling NewPRSigstoreSigned.
 func PRSigstoreSignedWithFulcio(fulcio PRSigstoreSignedFulcio) PRSigstoreSignedOption {
 	return func(pr *prSigstoreSigned) error {
@@ -91,14 +119,20 @@ func newPRSigstoreSigned(options ...PRSigstoreSignedOption) (*prSigstoreSigned,
 	if res.KeyPath != "" {
 		keySources++
 	}
+	if res.KeyPaths != nil {
+		keySources++
+	}
 	if res.KeyData != nil {
 		keySources++
 	}
+	if res.KeyDatas != nil {
+		keySources++
+	}
 	if res.Fulcio != nil {
 		keySources++
 	}
 	if keySources != 1 {
-		return nil, InvalidPolicyFormatError("exactly one of keyPath, keyData and fulcio must be specified")
+		return nil, InvalidPolicyFormatError("exactly one of keyPath, keyPaths, keyData, keyDatas and fulcio must be specified")
 	}
 
 	if res.RekorPublicKeyPath != "" && res.RekorPublicKeyData != nil {
@@ -143,7 +177,7 @@ var _ json.Unmarshaler = (*prSigstoreSigned)(nil)
 func (pr *prSigstoreSigned) UnmarshalJSON(data []byte) error {
 	*pr = prSigstoreSigned{}
 	var tmp prSigstoreSigned
-	var gotKeyPath, gotKeyData, gotFulcio, gotRekorPublicKeyPath, gotRekorPublicKeyData bool
+	var gotKeyPath, gotKeyPaths, gotKeyData, gotKeyDatas, gotFulcio, gotRekorPublicKeyPath, gotRekorPublicKeyData bool
 	var fulcio prSigstoreSignedFulcio
 	var signedIdentity json.RawMessage
 	if err := internal.ParanoidUnmarshalJSONObject(data, func(key string) any {
@@ -153,9 +187,15 @@ func (pr *prSigstoreSigned) UnmarshalJSON(data []byte) error {
 		case "keyPath":
 			gotKeyPath = true
 			return &tmp.KeyPath
+		case "keyPaths":
+			gotKeyPaths = true
+			return &tmp.KeyPaths
 		case "keyData":
 			gotKeyData = true
 			return &tmp.KeyData
+		case "keyDatas":
+			gotKeyDatas = true
+			return &tmp.KeyDatas
 		case "fulcio":
 			gotFulcio = true
 			return &fulcio
@@ -191,9 +231,15 @@ func (pr *prSigstoreSigned) UnmarshalJSON(data []byte) error {
 	if gotKeyPath {
 		opts = append(opts, PRSigstoreSignedWithKeyPath(tmp.KeyPath))
 	}
+	if gotKeyPaths {
+		opts = append(opts, PRSigstoreSignedWithKeyPaths(tmp.KeyPaths))
+	}
 	if gotKeyData {
 		opts = append(opts, PRSigstoreSignedWithKeyData(tmp.KeyData))
 	}
+	if gotKeyDatas {
+		opts = append(opts, PRSigstoreSignedWithKeyDatas(tmp.KeyDatas))
+	}
 	if gotFulcio {
 		opts = append(opts, PRSigstoreSignedWithFulcio(&fulcio))
 	}