Use callbacks instead of single expected values in verifyAndExtractSignature

To support verification of signatures when more than one key, or more
than one identity, are accepted, have verifyAndExtract signature accept
callbacks (in a struct so that they are explicitly named).

verifyAndExtractSignature now also validates the manifest digest.  It is
intended to become THE SINGLE PLACE where untrusted signature blobs
have signatures verified, are validated against other expectations, and
parsed, and converted into internal data structures available to other
code.

Also:
- Modifies VerifyDockerManifestSignature to use utils.ManifestMatchesDigest.
- Adds a test for Docker reference mismatch in VerifyDockerManifestSignature.

signature/docker.go

@@ -28,16 +28,33 @@ func SignDockerManifest(manifest []byte, dockerReference string, mech SigningMec
 // using mech.
 func VerifyDockerManifestSignature(unverifiedSignature, unverifiedManifest []byte,
 	expectedDockerReference string, mech SigningMechanism, expectedKeyIdentity string) (*Signature, error) {
-	expectedManifestDigest, err := utils.ManifestDigest(unverifiedManifest)
+	sig, err := verifyAndExtractSignature(mech, unverifiedSignature, signatureAcceptanceRules{
+		validateKeyIdentity: func(keyIdentity string) error {
+			if keyIdentity != expectedKeyIdentity {
+				return InvalidSignatureError{msg: fmt.Sprintf("Signature by %s does not match expected fingerprint %s", keyIdentity, expectedKeyIdentity)}
+			}
+			return nil
+		},
+		validateSignedDockerReference: func(signedDockerReference string) error {
+			if signedDockerReference != expectedDockerReference {
+				return InvalidSignatureError{msg: fmt.Sprintf("Docker reference %s does not match %s",
+					signedDockerReference, expectedDockerReference)}
+			}
+			return nil
+		},
+		validateSignedDockerManifestDigest: func(signedDockerManifestDigest string) error {
+			matches, err := utils.ManifestMatchesDigest(unverifiedManifest, signedDockerManifestDigest)
+			if err != nil {
+				return err
+			}
+			if !matches {
+				return InvalidSignatureError{msg: fmt.Sprintf("Signature for docker digest %s does not match", signedDockerManifestDigest, signedDockerManifestDigest)}
+			}
+			return nil
+		},
+	})
 	if err != nil {
 		return nil, err
 	}
-	sig, err := verifyAndExtractSignature(mech, unverifiedSignature, expectedKeyIdentity, expectedDockerReference)
-	if err != nil {
-		return nil, err
-	}
-	if sig.DockerManifestDigest != expectedManifestDigest {
-		return nil, InvalidSignatureError{msg: fmt.Sprintf("Docker manifest digest %s does not match %s", sig.DockerManifestDigest, expectedManifestDigest)}
-	}
 	return sig, nil
 }

signature/docker_test.go

@@ -72,6 +72,11 @@ func TestVerifyDockerManifestSignature(t *testing.T) {
 	assert.Error(t, err)
 	assert.Nil(t, sig)
 
+	// Docker reference mismatch
+	sig, err = VerifyDockerManifestSignature(signature, manifest, "example.com/doesnt/match", mech, TestKeyFingerprint)
+	assert.Error(t, err)
+	assert.Nil(t, sig)
+
 	// Docker manifest digest mismatch
 	sig, err = VerifyDockerManifestSignature(signature, []byte("unexpected manifest"), TestImageSignatureReference, mech, TestKeyFingerprint)
 	assert.Error(t, err)

signature/signature.go

@@ -155,26 +155,37 @@ func (s privateSignature) sign(mech SigningMechanism, keyIdentity string) ([]byt
 	return mech.Sign(json, keyIdentity)
 }
 
-// verifyAndExtractSignature verifies that signature has been signed by expectedKeyIdentity
+// signatureAcceptanceRules specifies how to decide whether an untrusted signature is acceptable.
-// using mech for expectedDockerReference, and returns it (without matching its contents to an image).
+// We centralize the actual parsing and data extraction in verifyAndExtractSignature; this supplies
-func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte,
+// the policy.  We use an object instead of supplying func parameters to verifyAndExtractSignature
-	expectedKeyIdentity, expectedDockerReference string) (*Signature, error) {
+// because all of the functions have the same type, so there is a risk of exchanging the functions;
+// named members of this struct are more explicit.
+type signatureAcceptanceRules struct {
+	validateKeyIdentity                func(string) error
+	validateSignedDockerReference      func(string) error
+	validateSignedDockerManifestDigest func(string) error
+}
+
+// verifyAndExtractSignature verifies that unverifiedSignature has been signed, and that its principial components
+// match expected values, both as specified by rules, and returns it
+func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte, rules signatureAcceptanceRules) (*Signature, error) {
 	signed, keyIdentity, err := mech.Verify(unverifiedSignature)
 	if err != nil {
 		return nil, err
 	}
-	if keyIdentity != expectedKeyIdentity {
+	if err := rules.validateKeyIdentity(keyIdentity); err != nil {
-		return nil, InvalidSignatureError{msg: fmt.Sprintf("Signature by %s does not match expected fingerprint %s", keyIdentity, expectedKeyIdentity)}
+		return nil, err
 	}
 
 	var unmatchedSignature privateSignature
 	if err := json.Unmarshal(signed, &unmatchedSignature); err != nil {
 		return nil, InvalidSignatureError{msg: err.Error()}
 	}
-
+	if err := rules.validateSignedDockerManifestDigest(unmatchedSignature.DockerManifestDigest); err != nil {
-	if unmatchedSignature.DockerReference != expectedDockerReference {
+		return nil, err
-		return nil, InvalidSignatureError{msg: fmt.Sprintf("Docker reference %s does not match %s",
+	}
-			unmatchedSignature.DockerReference, expectedDockerReference)}
+	if err := rules.validateSignedDockerReference(unmatchedSignature.DockerReference); err != nil {
+		return nil, err
 	}
 	signature := unmatchedSignature.Signature // Policy OK.
 	return &signature, nil

signature/signature_test.go

@@ -2,6 +2,7 @@ package signature
 
 import (
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"testing"
 
@@ -149,7 +150,26 @@ func TestSign(t *testing.T) {
 	signature, err := sig.sign(mech, TestKeyFingerprint)
 	require.NoError(t, err)
 
-	verified, err := verifyAndExtractSignature(mech, signature, TestKeyFingerprint, sig.DockerReference)
+	verified, err := verifyAndExtractSignature(mech, signature, signatureAcceptanceRules{
+		validateKeyIdentity: func(keyIdentity string) error {
+			if keyIdentity != TestKeyFingerprint {
+				return fmt.Errorf("Unexpected keyIdentity")
+			}
+			return nil
+		},
+		validateSignedDockerReference: func(signedDockerReference string) error {
+			if signedDockerReference != sig.DockerReference {
+				return fmt.Errorf("Unexpected signedDockerReference")
+			}
+			return nil
+		},
+		validateSignedDockerManifestDigest: func(signedDockerManifestDigest string) error {
+			if signedDockerManifestDigest != sig.DockerManifestDigest {
+				return fmt.Errorf("Unexpected signedDockerManifestDigest")
+			}
+			return nil
+		},
+	})
 	require.NoError(t, err)
 
 	assert.Equal(t, sig.Signature, *verified)
@@ -167,40 +187,102 @@ func TestVerifyAndExtractSignature(t *testing.T) {
 	mech, err := newGPGSigningMechanismInDirectory(testGPGHomeDirectory)
 	require.NoError(t, err)
 
+	type triple struct{ keyIdentity, signedDockerReference, signedDockerManifestDigest string }
+	var wanted, recorded triple
+	// recordingRules are a plausible signatureAcceptanceRules implementations, but equally
+	// importantly record that we are passing the correct values to the rule callbacks.
+	recordingRules := signatureAcceptanceRules{
+		validateKeyIdentity: func(keyIdentity string) error {
+			recorded.keyIdentity = keyIdentity
+			if keyIdentity != wanted.keyIdentity {
+				return fmt.Errorf("keyIdentity mismatch")
+			}
+			return nil
+		},
+		validateSignedDockerReference: func(signedDockerReference string) error {
+			recorded.signedDockerReference = signedDockerReference
+			if signedDockerReference != wanted.signedDockerReference {
+				return fmt.Errorf("signedDockerReference mismatch")
+			}
+			return nil
+		},
+		validateSignedDockerManifestDigest: func(signedDockerManifestDigest string) error {
+			recorded.signedDockerManifestDigest = signedDockerManifestDigest
+			if signedDockerManifestDigest != wanted.signedDockerManifestDigest {
+				return fmt.Errorf("signedDockerManifestDigest mismatch")
+			}
+			return nil
+		},
+	}
+
 	signature, err := ioutil.ReadFile("./fixtures/image.signature")
 	require.NoError(t, err)
+	signatureData := triple{
+		keyIdentity:                TestKeyFingerprint,
+		signedDockerReference:      TestImageSignatureReference,
+		signedDockerManifestDigest: TestImageManifestDigest,
+	}
 
 	// Successful verification
-	sig, err := verifyAndExtractSignature(mech, signature, TestKeyFingerprint, TestImageSignatureReference)
+	wanted = signatureData
+	recorded = triple{}
+	sig, err := verifyAndExtractSignature(mech, signature, recordingRules)
 	require.NoError(t, err)
 	assert.Equal(t, TestImageSignatureReference, sig.DockerReference)
 	assert.Equal(t, TestImageManifestDigest, sig.DockerManifestDigest)
+	assert.Equal(t, signatureData, recorded)
 
 	// For extra paranoia, test that we return a nil signature object on error.
 
 	// Completely invalid signature.
-	sig, err = verifyAndExtractSignature(mech, []byte{}, TestKeyFingerprint, TestImageSignatureReference)
+	recorded = triple{}
+	sig, err = verifyAndExtractSignature(mech, []byte{}, recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, sig)
+	assert.Equal(t, triple{}, recorded)
 
-	sig, err = verifyAndExtractSignature(mech, []byte("invalid signature"), TestKeyFingerprint, TestImageSignatureReference)
+	recorded = triple{}
+	sig, err = verifyAndExtractSignature(mech, []byte("invalid signature"), recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, sig)
+	assert.Equal(t, triple{}, recorded)
 
-	// Valid signature of non-JSON
+	// Valid signature of non-JSON: asked for keyIdentity, only
 	invalidBlobSignature, err := ioutil.ReadFile("./fixtures/invalid-blob.signature")
 	require.NoError(t, err)
-	sig, err = verifyAndExtractSignature(mech, invalidBlobSignature, TestKeyFingerprint, TestImageSignatureReference)
+	recorded = triple{}
+	sig, err = verifyAndExtractSignature(mech, invalidBlobSignature, recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, sig)
+	assert.Equal(t, triple{keyIdentity: signatureData.keyIdentity}, recorded)
 
-	// Valid signature with a wrong key
+	// Valid signature with a wrong key: asked for keyIdentity, only
-	sig, err = verifyAndExtractSignature(mech, signature, "unexpected fingerprint", TestImageSignatureReference)
+	wanted = signatureData
+	wanted.keyIdentity = "unexpected fingerprint"
+	recorded = triple{}
+	sig, err = verifyAndExtractSignature(mech, signature, recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, sig)
+	assert.Equal(t, triple{keyIdentity: signatureData.keyIdentity}, recorded)
+
+	// Valid signature with a wrong manifest digest: asked for keyIdentity and signedDockerManifestDigest
+	wanted = signatureData
+	wanted.signedDockerManifestDigest = "invalid digest"
+	recorded = triple{}
+	sig, err = verifyAndExtractSignature(mech, signature, recordingRules)
+	assert.Error(t, err)
+	assert.Nil(t, sig)
+	assert.Equal(t, triple{
+		keyIdentity:                signatureData.keyIdentity,
+		signedDockerManifestDigest: signatureData.signedDockerManifestDigest,
+	}, recorded)
 
 	// Valid signature with a wrong image reference
-	sig, err = verifyAndExtractSignature(mech, signature, TestKeyFingerprint, "unexpected docker reference")
+	wanted = signatureData
+	wanted.signedDockerReference = "unexpected docker reference"
+	recorded = triple{}
+	sig, err = verifyAndExtractSignature(mech, signature, recordingRules)
 	assert.Error(t, err)
 	assert.Nil(t, sig)
+	assert.Equal(t, signatureData, recorded)
 }