From e2839c38c5733e5fa9a6186dafbb87dbf70e6896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 30 May 2016 20:41:49 +0200 Subject: [PATCH 1/6] Add a test for valid signature using an unknown public key (The key was one-time-generated in a temporary directory, and is, intentionally, not available.) This is not conceptually related to the rest of the PR, just adding a missing case to the test, except that the added fixture will be reused in a prSignedBy test. --- signature/fixtures/unknown-key.signature | Bin 0 -> 558 bytes signature/mechanism_test.go | 7 +++++++ 2 files changed, 7 insertions(+) create mode 100644 signature/fixtures/unknown-key.signature diff --git a/signature/fixtures/unknown-key.signature b/signature/fixtures/unknown-key.signature new file mode 100644 index 0000000000000000000000000000000000000000..2277b130dab415c59a76679a5ad1b1d252e98908 GIT binary patch literal 558 zcmV+}0@3}W0h_?f%)r6;)IC>F;6ep6`5C#FEmYRFI(=Id~ zsyD2^7m{SL`O>k)B`-?0L@ZL}y()d};O`xMqGhb%F57k~E|suqk3T$<)gj}0*ap{D zP2IHKDMlR5Gxub_Kl@<)%f}1!o6TNIx?F!IJ5E z){OaV0yD&Jzxz99M$RSs71wGNzAxRsP|#lH=4m$Oh*Jw+sOCSFsqK$otN&uzd8ao0 zYvO Date: Wed, 1 Jun 2016 19:16:08 +0200 Subject: [PATCH 2/6] 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 | 33 +++++++++--- signature/docker_test.go | 5 ++ signature/signature.go | 31 +++++++---- signature/signature_test.go | 100 ++++++++++++++++++++++++++++++++---- 4 files changed, 142 insertions(+), 27 deletions(-) diff --git a/signature/docker.go b/signature/docker.go index eb575c06..223385c5 100644 --- a/signature/docker.go +++ b/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 } diff --git a/signature/docker_test.go b/signature/docker_test.go index ec4c858d..766fb036 100644 --- a/signature/docker_test.go +++ b/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) diff --git a/signature/signature.go b/signature/signature.go index b59209ac..bf1c63e9 100644 --- a/signature/signature.go +++ b/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 -// using mech for expectedDockerReference, and returns it (without matching its contents to an image). -func verifyAndExtractSignature(mech SigningMechanism, unverifiedSignature []byte, - expectedKeyIdentity, expectedDockerReference string) (*Signature, error) { +// signatureAcceptanceRules specifies how to decide whether an untrusted signature is acceptable. +// We centralize the actual parsing and data extraction in verifyAndExtractSignature; this supplies +// the policy. We use an object instead of supplying func parameters to verifyAndExtractSignature +// 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 { - return nil, InvalidSignatureError{msg: fmt.Sprintf("Signature by %s does not match expected fingerprint %s", keyIdentity, expectedKeyIdentity)} + if err := rules.validateKeyIdentity(keyIdentity); err != nil { + return nil, err } var unmatchedSignature privateSignature if err := json.Unmarshal(signed, &unmatchedSignature); err != nil { return nil, InvalidSignatureError{msg: err.Error()} } - - if unmatchedSignature.DockerReference != expectedDockerReference { - return nil, InvalidSignatureError{msg: fmt.Sprintf("Docker reference %s does not match %s", - unmatchedSignature.DockerReference, expectedDockerReference)} + if err := rules.validateSignedDockerManifestDigest(unmatchedSignature.DockerManifestDigest); err != nil { + return nil, err + } + if err := rules.validateSignedDockerReference(unmatchedSignature.DockerReference); err != nil { + return nil, err } signature := unmatchedSignature.Signature // Policy OK. return &signature, nil diff --git a/signature/signature_test.go b/signature/signature_test.go index 471e8b9f..db63abee 100644 --- a/signature/signature_test.go +++ b/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 - sig, err = verifyAndExtractSignature(mech, signature, "unexpected fingerprint", TestImageSignatureReference) + // Valid signature with a wrong key: asked for keyIdentity, only + 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) } From 677f711c6c8610c8c449566467de5811249a44ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 31 May 2016 02:12:28 +0200 Subject: [PATCH 3/6] Redefine Policy.Specific scopes to use fully expanded hostname/namespace/repo format Using the canonical minimized format of Docker references introduces too many ambiguities. This also removes some validation of the scope string, but all that was really doing was rejecting completely invalid input like uppercase. Sadly it is not qutie obvious that we can detect and reject mistakes like using "busybox" as a scope instead of the correct "docker.io/library/busybox". Perhaps require at least one dot or port number in the host name? --- signature/policy_config.go | 7 +++---- signature/policy_config_test.go | 7 ++----- signature/policy_types.go | 4 +++- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/signature/policy_config.go b/signature/policy_config.go index fd634783..7fc5a382 100644 --- a/signature/policy_config.go +++ b/signature/policy_config.go @@ -88,10 +88,9 @@ func (m *policySpecificMap) UnmarshalJSON(data []byte) error { // So, use a temporary map of pointers-to-slices and convert. tmpMap := map[string]*PolicyRequirements{} if err := paranoidUnmarshalJSONObject(data, func(key string) interface{} { - // Check that the scope format is at least plausible. - if _, err := reference.ParseNamed(key); err != nil { - return nil // FIXME? This returns an "Unknown key" error instead of saying that the format is invalid. - } + // FIXME? We might want to validate the scope format. + // Note that reference.ParseNamed is unsuitable; it would understand "example.com" as + // "docker.io/library/example.com" // paranoidUnmarshalJSONObject detects key duplication for us, check just to be safe. if _, ok := tmpMap[key]; ok { return nil diff --git a/signature/policy_config_test.go b/signature/policy_config_test.go index 63fd8c74..23c2f1a6 100644 --- a/signature/policy_config_test.go +++ b/signature/policy_config_test.go @@ -160,7 +160,7 @@ func TestPolicyUnmarshalJSON(t *testing.T) { xNewPRSignedByKeyData(SBKeyTypeGPGKeys, []byte("abc"), NewPRMMatchExact()), }, Specific: map[string]PolicyRequirements{ - "library/busybox": []PolicyRequirement{ + "docker.io/library/busybox": []PolicyRequirement{ xNewPRSignedByKeyData(SBKeyTypeGPGKeys, []byte("def"), NewPRMMatchExact()), }, "registry.access.redhat.com": []PolicyRequirement{ @@ -191,11 +191,8 @@ func TestPolicyUnmarshalJSON(t *testing.T) { func(v mSI) { v["specific"] = []string{} }, // "default" is an invalid PolicyRequirements func(v mSI) { v["default"] = PolicyRequirements{} }, - // Invalid scope name in "specific". Uppercase is invalid in Docker reference components. - // Get valid PolicyRequirements by copying them from "library/buxybox". - func(v mSI) { x(v, "specific")["INVALIDUPPERCASE"] = x(v, "specific")["library/busybox"] }, // A field in "specific" is an invalid PolicyRequirements - func(v mSI) { x(v, "specific")["library/busybox"] = PolicyRequirements{} }, + func(v mSI) { x(v, "specific")["docker.io/library/busybox"] = PolicyRequirements{} }, } for _, fn := range breakFns { err = tryUnmarshalModifiedPolicy(t, &p, validJSON, fn) diff --git a/signature/policy_types.go b/signature/policy_types.go index c38b1e76..c57fa34d 100644 --- a/signature/policy_types.go +++ b/signature/policy_types.go @@ -11,7 +11,9 @@ type Policy struct { // Default applies to any image which does not have a matching policy in Specific. Default PolicyRequirements `json:"default"` // Specific applies to images matching scope, the map key. - // Scope is registry/server, namespace in a registry, single repository. + // Scope is hostname[/zero/or/more/namespaces[/repository[:tag|@digest]]]; note that in order to be + // unambiguous, this must use a fully expanded format, e.g. "docker.io/library/busybox" or + // "docker.io/library", not "busybox" or "library". // FIXME: Scope syntax - should it be namespaced docker:something ? Or, in the worst case, a composite object (we couldn't use a JSON map) // Most specific scope wins, duplication is prohibited (hard failure). // Defaults to an empty map if not specified. From 90361256bcadd3823ab1055b483706417b06d2e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 25 May 2016 22:28:14 +0200 Subject: [PATCH 4/6] Add PolicyReferenceMatch implementations Also move the declaration of the type from the mostly-public policy_types.go to policy_eval.go. --- signature/policy_eval.go | 11 ++ signature/policy_reference_match.go | 61 +++++++ signature/policy_reference_match_test.go | 202 +++++++++++++++++++++++ signature/policy_types.go | 1 - 4 files changed, 274 insertions(+), 1 deletion(-) create mode 100644 signature/policy_eval.go create mode 100644 signature/policy_reference_match.go create mode 100644 signature/policy_reference_match_test.go diff --git a/signature/policy_eval.go b/signature/policy_eval.go new file mode 100644 index 00000000..ec9d5c0e --- /dev/null +++ b/signature/policy_eval.go @@ -0,0 +1,11 @@ +package signature + +import "github.com/projectatomic/skopeo/types" + +// PolicyReferenceMatch specifies a set of image identities accepted in PolicyRequirement. +// The type is public, but its implementation is private. +type PolicyReferenceMatch interface { + // matchesDockerReference decides whether a specific image identity is accepted for an image + // (or, usually, for the image's IntendedDockerReference()), + matchesDockerReference(image types.Image, signatureDockerReference string) bool +} diff --git a/signature/policy_reference_match.go b/signature/policy_reference_match.go new file mode 100644 index 00000000..b1d2caea --- /dev/null +++ b/signature/policy_reference_match.go @@ -0,0 +1,61 @@ +// PolicyReferenceMatch implementations. + +package signature + +import ( + "github.com/projectatomic/skopeo/reference" + "github.com/projectatomic/skopeo/types" +) + +// parseDockerReferences converts two reference strings into parsed entities, failing on any error +func parseDockerReferences(s1, s2 string) (reference.Named, reference.Named, error) { + r1, err := reference.ParseNamed(s1) + if err != nil { + return nil, nil, err + } + r2, err := reference.ParseNamed(s2) + if err != nil { + return nil, nil, err + } + return r1, r2, nil +} + +func (prm *prmMatchExact) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(image.IntendedDockerReference(), signatureDockerReference) + if err != nil { + return false + } + // Do not add default tags: image.IntendedDockerReference() has it added already per its construction, and signatureDockerReference should be exact; so, verify that now. + if reference.IsNameOnly(intended) || reference.IsNameOnly(signature) { + return false + } + return signature.String() == intended.String() +} + +func (prm *prmMatchRepository) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(image.IntendedDockerReference(), signatureDockerReference) + if err != nil { + return false + } + return signature.Name() == intended.Name() +} + +func (prm *prmExactReference) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(prm.DockerReference, signatureDockerReference) + if err != nil { + return false + } + // prm.DockerReference and signatureDockerReference should be exact; so, verify that now. + if reference.IsNameOnly(intended) || reference.IsNameOnly(signature) { + return false + } + return signature.String() == intended.String() +} + +func (prm *prmExactRepository) matchesDockerReference(image types.Image, signatureDockerReference string) bool { + intended, signature, err := parseDockerReferences(prm.DockerRepository, signatureDockerReference) + if err != nil { + return false + } + return signature.Name() == intended.Name() +} diff --git a/signature/policy_reference_match_test.go b/signature/policy_reference_match_test.go new file mode 100644 index 00000000..0b00214b --- /dev/null +++ b/signature/policy_reference_match_test.go @@ -0,0 +1,202 @@ +package signature + +import ( + "fmt" + "testing" + + "github.com/projectatomic/skopeo/types" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + fullRHELRef = "registry.access.redhat.com/rhel7/rhel:7.2.3" + untaggedRHELRef = "registry.access.redhat.com/rhel7/rhel" +) + +func TestParseDockerReferences(t *testing.T) { + const ( + ok1 = "busybox" + ok2 = fullRHELRef + bad1 = "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES" + bad2 = "" + ) + + // Success + r1, r2, err := parseDockerReferences(ok1, ok2) + require.NoError(t, err) + assert.Equal(t, ok1, r1.String()) + assert.Equal(t, ok2, r2.String()) + + // Failures + for _, refs := range [][]string{ + {bad1, ok2}, + {ok1, bad2}, + {bad1, bad2}, + } { + _, _, err := parseDockerReferences(refs[0], refs[1]) + assert.Error(t, err) + } +} + +// refImageMock is a mock of types.Image which returns itself in IntendedDockerReference. +type refImageMock string + +func (ref refImageMock) IntendedDockerReference() string { + return string(ref) +} +func (ref refImageMock) Manifest() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) ManifestMatchesDigest(expectedDigest string) (bool, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) Signatures() ([][]byte, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) Layers(layers ...string) error { + panic("unexpected call to a mock function") +} +func (ref refImageMock) Inspect() (*types.ImageInspectInfo, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) DockerTar() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref refImageMock) GetRepositoryTags() ([]string, error) { + panic("unexpected call to a mock function") +} + +type prmTableTest struct { + imageRef, sigRef string + result bool +} + +// Test cases for exact reference match +var prmExactMatchTestTable = []prmTableTest{ + // Success, simple matches + {"busybox:latest", "busybox:latest", true}, + {fullRHELRef, fullRHELRef, true}, + // Non-canonical reference format is canonicalized + {"library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "library/busybox:latest", true}, + {"docker.io/library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "docker.io/library/busybox:latest", true}, + // Mismatch + {"busybox:latest", "busybox:notlatest", false}, + {"busybox:latest", "notbusybox:latest", false}, + {"busybox:latest", "hostname/library/busybox:notlatest", false}, + {"hostname/library/busybox:latest", "busybox:notlatest", false}, + {"busybox:latest", fullRHELRef, false}, + // Missing tags + {"busybox", "busybox:latest", false}, + {"busybox:latest", "busybox", false}, + {"busybox", "busybox", false}, + // Invalid format + {"UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", "busybox:latest", false}, + {"busybox:latest", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + {"", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + // Even if they are exactly equal, invalid values are rejected. + {"INVALID", "INVALID", false}, +} + +// Test cases for repository-only reference match +var prmRepositoryMatchTestTable = []prmTableTest{ + // Success, simple matches + {"busybox:latest", "busybox:latest", true}, + {fullRHELRef, fullRHELRef, true}, + // Non-canonical reference format is canonicalized + {"library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "library/busybox:latest", true}, + {"docker.io/library/busybox:latest", "busybox:latest", true}, + {"busybox:latest", "docker.io/library/busybox:latest", true}, + // The same as above, but with mismatching tags + {"busybox:latest", "busybox:notlatest", true}, + {fullRHELRef + "tagsuffix", fullRHELRef, true}, + {"library/busybox:latest", "busybox:notlatest", true}, + {"busybox:latest", "library/busybox:notlatest", true}, + {"docker.io/library/busybox:notlatest", "busybox:latest", true}, + {"busybox:notlatest", "docker.io/library/busybox:latest", true}, + // The same as above, but with defaulted tags (should not actually happen) + {"busybox", "busybox:notlatest", true}, + {fullRHELRef, untaggedRHELRef, true}, + {"library/busybox", "busybox", true}, + {"busybox", "library/busybox", true}, + {"docker.io/library/busybox", "busybox", true}, + {"busybox", "docker.io/library/busybox", true}, + // Mismatch + {"busybox:latest", "notbusybox:latest", false}, + {"hostname/library/busybox:latest", "busybox:notlatest", false}, + {"busybox:latest", fullRHELRef, false}, + // Invalid format + {"UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", "busybox:latest", false}, + {"busybox:latest", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + {"", "UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", false}, + // Even if they are exactly equal, invalid values are rejected. + {"INVALID", "INVALID", false}, +} + +func TestPRMMatchExactMatchesDockerReference(t *testing.T) { + prm := NewPRMMatchExact() + for _, test := range prmExactMatchTestTable { + res := prm.matchesDockerReference(refImageMock(test.imageRef), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} + +func TestPRMMatchRepositoryMatchesDockerReference(t *testing.T) { + prm := NewPRMMatchRepository() + for _, test := range prmRepositoryMatchTestTable { + res := prm.matchesDockerReference(refImageMock(test.imageRef), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} + +// forbiddenImageMock is a mock of types.Image which ensures IntendedDockerReference is not called +type forbiddenImageMock string + +func (ref forbiddenImageMock) IntendedDockerReference() string { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Manifest() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) ManifestMatchesDigest(expectedDigest string) (bool, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Signatures() ([][]byte, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Layers(layers ...string) error { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) Inspect() (*types.ImageInspectInfo, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) DockerTar() ([]byte, error) { + panic("unexpected call to a mock function") +} +func (ref forbiddenImageMock) GetRepositoryTags() ([]string, error) { + panic("unexpected call to a mock function") +} + +func TestPRMExactReferenceMatchesDockerReference(t *testing.T) { + for _, test := range prmExactMatchTestTable { + // Do not use NewPRMExactReference, we want to also test the case with an invalid DockerReference, + // even though NewPRMExactReference should never let it happen. + prm := prmExactReference{DockerReference: test.imageRef} + res := prm.matchesDockerReference(forbiddenImageMock(""), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} + +func TestPRMExactRepositoryMatchesDockerReference(t *testing.T) { + for _, test := range prmRepositoryMatchTestTable { + // Do not use NewPRMExactRepository, we want to also test the case with an invalid DockerReference, + // even though NewPRMExactRepository should never let it happen. + prm := prmExactRepository{DockerRepository: test.imageRef} + res := prm.matchesDockerReference(forbiddenImageMock(""), test.sigRef) + assert.Equal(t, test.result, res, fmt.Sprintf("%s vs. %s", test.imageRef, test.sigRef)) + } +} diff --git a/signature/policy_types.go b/signature/policy_types.go index c57fa34d..3a231669 100644 --- a/signature/policy_types.go +++ b/signature/policy_types.go @@ -101,7 +101,6 @@ type prSignedBaseLayer struct { // PolicyReferenceMatch specifies a set of image identities accepted in PolicyRequirement. // The type is public, but its implementation is private. -type PolicyReferenceMatch interface{} // Will be expanded and moved elsewhere later. // prmCommon is the common type field in a JSON encoding of PolicyReferenceMatch. type prmCommon struct { From fd9c615d88b12c09be7f4a95153a4649c547931c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 30 May 2016 22:38:51 +0200 Subject: [PATCH 5/6] Add PolicyRequirement implementations Also move the declaration of the type from the mostly-public policy_types.go to policy_eval.go. --- .../manifest.json | 1 + .../dir-img-manifest-digest-error/signature-1 | 1 + .../fixtures/dir-img-mixed/manifest.json | 1 + signature/fixtures/dir-img-mixed/signature-1 | 1 + signature/fixtures/dir-img-mixed/signature-2 | 1 + .../dir-img-modified-manifest/manifest.json | 27 ++ .../dir-img-modified-manifest/signature-1 | 1 + .../fixtures/dir-img-no-manifest/signature-1 | 1 + .../fixtures/dir-img-unsigned/manifest.json | 1 + .../fixtures/dir-img-valid-2/manifest.json | 1 + .../fixtures/dir-img-valid-2/signature-1 | 1 + .../fixtures/dir-img-valid-2/signature-2 | Bin 0 -> 425 bytes .../fixtures/dir-img-valid/manifest.json | 1 + signature/fixtures/dir-img-valid/signature-1 | Bin 0 -> 427 bytes signature/policy_eval.go | 50 ++++ signature/policy_eval_baselayer.go | 18 ++ signature/policy_eval_baselayer_test.go | 24 ++ signature/policy_eval_signedby.go | 137 ++++++++++ signature/policy_eval_signedby_test.go | 239 ++++++++++++++++++ signature/policy_eval_simple.go | 25 ++ signature/policy_eval_simple_test.go | 32 +++ signature/policy_eval_test.go | 59 +++++ signature/policy_types.go | 1 - 23 files changed, 622 insertions(+), 1 deletion(-) create mode 120000 signature/fixtures/dir-img-manifest-digest-error/manifest.json create mode 120000 signature/fixtures/dir-img-manifest-digest-error/signature-1 create mode 120000 signature/fixtures/dir-img-mixed/manifest.json create mode 120000 signature/fixtures/dir-img-mixed/signature-1 create mode 120000 signature/fixtures/dir-img-mixed/signature-2 create mode 100644 signature/fixtures/dir-img-modified-manifest/manifest.json create mode 120000 signature/fixtures/dir-img-modified-manifest/signature-1 create mode 120000 signature/fixtures/dir-img-no-manifest/signature-1 create mode 120000 signature/fixtures/dir-img-unsigned/manifest.json create mode 120000 signature/fixtures/dir-img-valid-2/manifest.json create mode 120000 signature/fixtures/dir-img-valid-2/signature-1 create mode 100644 signature/fixtures/dir-img-valid-2/signature-2 create mode 120000 signature/fixtures/dir-img-valid/manifest.json create mode 100644 signature/fixtures/dir-img-valid/signature-1 create mode 100644 signature/policy_eval_baselayer.go create mode 100644 signature/policy_eval_baselayer_test.go create mode 100644 signature/policy_eval_signedby.go create mode 100644 signature/policy_eval_signedby_test.go create mode 100644 signature/policy_eval_simple.go create mode 100644 signature/policy_eval_simple_test.go create mode 100644 signature/policy_eval_test.go diff --git a/signature/fixtures/dir-img-manifest-digest-error/manifest.json b/signature/fixtures/dir-img-manifest-digest-error/manifest.json new file mode 120000 index 00000000..3dee14b4 --- /dev/null +++ b/signature/fixtures/dir-img-manifest-digest-error/manifest.json @@ -0,0 +1 @@ +../v2s1-invalid-signatures.manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-manifest-digest-error/signature-1 b/signature/fixtures/dir-img-manifest-digest-error/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-manifest-digest-error/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-mixed/manifest.json b/signature/fixtures/dir-img-mixed/manifest.json new file mode 120000 index 00000000..ff7d2ffa --- /dev/null +++ b/signature/fixtures/dir-img-mixed/manifest.json @@ -0,0 +1 @@ +../dir-img-valid/manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-mixed/signature-1 b/signature/fixtures/dir-img-mixed/signature-1 new file mode 120000 index 00000000..b27cdc45 --- /dev/null +++ b/signature/fixtures/dir-img-mixed/signature-1 @@ -0,0 +1 @@ +../invalid-blob.signature \ No newline at end of file diff --git a/signature/fixtures/dir-img-mixed/signature-2 b/signature/fixtures/dir-img-mixed/signature-2 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-mixed/signature-2 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-modified-manifest/manifest.json b/signature/fixtures/dir-img-modified-manifest/manifest.json new file mode 100644 index 00000000..82fde381 --- /dev/null +++ b/signature/fixtures/dir-img-modified-manifest/manifest.json @@ -0,0 +1,27 @@ +{ + "schemaVersion": 2, + "mediaType": "application/vnd.docker.distribution.manifest.v2+json", + "config": { + "mediaType": "application/vnd.docker.container.image.v1+json", + "size": 7023, + "digest": "sha256:b5b2b2c507a0944348e0303114d8d93aaaa081732b86451d9bce1f432a537bc7" + }, + "layers": [ + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 32654, + "digest": "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f" + }, + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 16724, + "digest": "sha256:3c3a4604a545cdc127456d94e421cd355bca5b528f4a9c1905b15da2eb4a4c6b" + }, + { + "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip", + "size": 73109, + "digest": "sha256:ec4b8955958665577945c89419d1af06b5f7636b4ac3da7f12184802ad867736" + } + ], + "extra": "this manifest has been modified" +} diff --git a/signature/fixtures/dir-img-modified-manifest/signature-1 b/signature/fixtures/dir-img-modified-manifest/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-modified-manifest/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-no-manifest/signature-1 b/signature/fixtures/dir-img-no-manifest/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-no-manifest/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-unsigned/manifest.json b/signature/fixtures/dir-img-unsigned/manifest.json new file mode 120000 index 00000000..ff7d2ffa --- /dev/null +++ b/signature/fixtures/dir-img-unsigned/manifest.json @@ -0,0 +1 @@ +../dir-img-valid/manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid-2/manifest.json b/signature/fixtures/dir-img-valid-2/manifest.json new file mode 120000 index 00000000..ff7d2ffa --- /dev/null +++ b/signature/fixtures/dir-img-valid-2/manifest.json @@ -0,0 +1 @@ +../dir-img-valid/manifest.json \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid-2/signature-1 b/signature/fixtures/dir-img-valid-2/signature-1 new file mode 120000 index 00000000..f010fd4c --- /dev/null +++ b/signature/fixtures/dir-img-valid-2/signature-1 @@ -0,0 +1 @@ +../dir-img-valid/signature-1 \ No newline at end of file diff --git a/signature/fixtures/dir-img-valid-2/signature-2 b/signature/fixtures/dir-img-valid-2/signature-2 new file mode 100644 index 0000000000000000000000000000000000000000..dbba8f422811e20ff5077fb33bed1c7b0c49484c GIT binary patch literal 425 zcmV;a0apH_0h_?f%)r5TyXccd_m-R!jHeGICNYHjEHbWEN-oMQ$xKenQL?I5%1lYk zE6FUW1hG@{le1Hcbc<5cQj1dal2eteluA;IOEUA)^>Y*RGSh%;tDHoTkW#IVQf6*q zdMc7Om=fKT%yf_vE2ZL$L?cr(DuwBQr}&!?ct{GXvvPgA@b9B$HJ0G&2JeBlA>KQ;=mPl?5Q@B$njoW+p2n=jW9q zX6B_9DHLa>=Ovbu7Nvp|=I25Dm0XkxR9uA6W1wfKXK1XOl3Jz&G%hn2=)}a_ z0wpU$6Eh$*FfcW&t?ih@!obMEz{bi13Md9HP61f(DZgc5SRvv3>p{5-e{}20e`Y_P zE4`Nw^4q4zQny`lU4iZWtgzB)=G`B(-b`J^858E^%~GoIY*RGSh%;tDHoTkW#IVQf6*q zdMc7Om=fKT%yf_vE2ZL$L?cr(DuwBQr}&!?ct{GXvvPgA@b9B$HJ0G&2JeBlA>KQ;=mPl?5Q@B$njoW+p2n=jW9q zX6B_9DHLa>=Ovbu7Nvp|=I25Dm0XkxR9uA6W1wfKXK1XOl3Jz&G%hn2=)}a_ z0wpU$6EhQIOAAX=liJ#jIV=o}91Lu%OrU^b;Nld31z)%@^M8+d`p;)&aHfmy<#WsS zIK8RVeU8%X-WmB$J1dW|Oj*WaDl(;Svh=AfLN^vEid+ Date: Tue, 31 May 2016 17:09:43 +0200 Subject: [PATCH 6/6] Add PolicyContext, with GetSignaturesWithAcceptedAuthor and IsRunningImageAllowed PolicyContext is intended to be the primary API for skopeo/signature: supply a policy and an image, and ask specific, well-defined (preferably yes/no) questions. --- signature/policy_eval.go | 275 ++++++++++++++++++++- signature/policy_eval_test.go | 445 ++++++++++++++++++++++++++++++++++ 2 files changed, 719 insertions(+), 1 deletion(-) diff --git a/signature/policy_eval.go b/signature/policy_eval.go index a6a863a3..c62328ed 100644 --- a/signature/policy_eval.go +++ b/signature/policy_eval.go @@ -1,6 +1,19 @@ +// This defines the top-level policy evaluation API. +// To the extent possible, the interface of the fuctions provided +// here is intended to be completely unambiguous, and stable for users +// to rely on. + package signature -import "github.com/projectatomic/skopeo/types" +import ( + "fmt" + "strings" + + "github.com/Sirupsen/logrus" + distreference "github.com/docker/distribution/reference" + "github.com/projectatomic/skopeo/reference" + "github.com/projectatomic/skopeo/types" +) // PolicyRequirementError is an explanatory text for rejecting a signature or an image. type PolicyRequirementError string @@ -49,6 +62,8 @@ type PolicyRequirement interface { // isRunningImageAllowed returns true if the requirement allows running an image. // If it returns false, err must be non-nil, and should be an PolicyRequirementError if evaluation // succeeded but the result was rejection. + // WARNING: This validates signatures and the manifest, but does not download or validate the + // layers. Users must validate that the layers match their expected digests. isRunningImageAllowed(image types.Image) (bool, error) } @@ -59,3 +74,261 @@ type PolicyReferenceMatch interface { // (or, usually, for the image's IntendedDockerReference()), matchesDockerReference(image types.Image, signatureDockerReference string) bool } + +// PolicyContext encapsulates a policy and possible cached state +// for speeding up its evaluation. +type PolicyContext struct { + Policy *Policy + state policyContextState // Internal consistency checking +} + +// policyContextState is used internally to verify the users are not misusing a PolicyContext. +type policyContextState string + +const ( + pcInvalid policyContextState = "" + pcInitializing policyContextState = "Initializing" + pcReady policyContextState = "Ready" + pcInUse policyContextState = "InUse" + pcDestroying policyContextState = "Destroying" + pcDestroyed policyContextState = "Destroyed" +) + +// changeContextState changes pc.state, or fails if the state is unexpected +func (pc *PolicyContext) changeState(expected, new policyContextState) error { + if pc.state != expected { + return fmt.Errorf(`"Invalid PolicyContext state, expected "%s", found "%s"`, expected, pc.state) + } + pc.state = new + return nil +} + +// NewPolicyContext sets up and initializes a context for the specified policy. +// The policy must not be modified while the context exists. FIXME: make a deep copy? +// If this function succeeds, the caller should call PolicyContext.Destroy() when done. +func NewPolicyContext(policy *Policy) (*PolicyContext, error) { + pc := &PolicyContext{Policy: policy, state: pcInitializing} + // FIXME: initialize + if err := pc.changeState(pcInitializing, pcReady); err != nil { + // Huh?! This should never fail, we didn't give the pointer to anybody. + // Just give up and leave unclean state around. + return nil, err + } + return pc, nil +} + +// Destroy should be called when the user of the context is done with it. +func (pc *PolicyContext) Destroy() error { + if err := pc.changeState(pcReady, pcDestroying); err != nil { + return err + } + // FIXME: destroy + return pc.changeState(pcDestroying, pcDestroyed) +} + +// fullyExpandedDockerReference converts a reference.Named into a fully expanded format; +// i.e. soft of an opposite to ref.String(), which is a fully canonicalized/minimized format. +// This is guaranteed to be the same as reference.FullName(), with a tag or digest appended, if available. +// FIXME? This feels like it should be provided by skopeo/reference. +func fullyExpandedDockerReference(ref reference.Named) (string, error) { + res := ref.FullName() + tagged, isTagged := ref.(distreference.Tagged) + digested, isDigested := ref.(distreference.Digested) + // A github.com/distribution/reference value can have a tag and a digest at the same time! + // skopeo/reference does not handle that, so fail. + // FIXME? Should we support that? + switch { + case isTagged && isDigested: + // Coverage: This should currently not happen, the way skopeo/reference sets up types, + // isTagged and isDigested is mutually exclusive. + return "", fmt.Errorf("Names with both a tag and digest are not currently supported") + case isTagged: + res = res + ":" + tagged.Tag() + case isDigested: + res = res + "@" + digested.Digest().String() + default: + // res is already OK. + } + return res, nil +} + +// requirementsForImage selects the appropriate requirements for image. +func (pc *PolicyContext) requirementsForImage(image types.Image) (PolicyRequirements, error) { + imageIdentity := image.IntendedDockerReference() + // We don't technically need to parse it first in order to match the full name:tag, + // but do so anyway to ensure that the intended identity really does follow that + // format, or at least that it is not demonstrably wrong. + ref, err := reference.ParseNamed(imageIdentity) + if err != nil { + return nil, err + } + ref = reference.WithDefaultTag(ref) + + // Look for a full match. + fullyExpanded, err := fullyExpandedDockerReference(ref) + if err != nil { // Coverage: This cannot currently happen. + return nil, err + } + if req, ok := pc.Policy.Specific[fullyExpanded]; ok { + logrus.Debugf(" Using specific policy section %s", fullyExpanded) + return req, nil + } + + // Look for a match of the repository, and then of the possible parent + // namespaces. Note that this only happens on the expanded host names + // and repository names, i.e. "busybox" is looked up as "docker.io/library/busybox", + // then in its parent "docker.io/library"; in none of "busybox", + // un-namespaced "library" nor in "" implicitly representing "library/". + // + // ref.FullName() == ref.Hostname() + "/" + ref.RemoteName(), so the last + // iteration matches the host name (for any namespace). + name := ref.FullName() + for { + if req, ok := pc.Policy.Specific[name]; ok { + logrus.Debugf(" Using specific policy section %s", name) + return req, nil + } + + lastSlash := strings.LastIndex(name, "/") + if lastSlash == -1 { + break + } + name = name[:lastSlash] + } + + logrus.Debugf(" Using default policy section") + return pc.Policy.Default, nil +} + +// GetSignaturesWithAcceptedAuthor returns those signatures from an image +// for which the policy accepts the author (and which have been successfully +// verified). +// NOTE: This may legitimately return an empty list and no error, if the image +// has no signatures or only invalid signatures. +// WARNING: This makes the signature contents acceptable for futher processing, +// but it does not necessarily mean that the contents of the signature are +// consistent with local policy. +// For example: +// - Do not use a an existence of an accepted signature to determine whether to run +// a container based on this image; use IsRunningImageAllowed instead. +// - Just because a signature is accepted does not automatically mean the contents of the +// signature are authorized to run code as root, or to affect system or cluster configuration. +func (pc *PolicyContext) GetSignaturesWithAcceptedAuthor(image types.Image) (sigs []*Signature, finalErr error) { + if err := pc.changeState(pcReady, pcInUse); err != nil { + return nil, err + } + defer func() { + if err := pc.changeState(pcInUse, pcReady); err != nil { + sigs = nil + finalErr = err + } + }() + + logrus.Debugf("GetSignaturesWithAcceptedAuthor for image %s", image.IntendedDockerReference()) + + reqs, err := pc.requirementsForImage(image) + if err != nil { + return nil, err + } + + // FIXME: rename Signatures to UnverifiedSignatures + unverifiedSignatures, err := image.Signatures() + if err != nil { + return nil, err + } + + res := make([]*Signature, 0, len(unverifiedSignatures)) + for sigNumber, sig := range unverifiedSignatures { + var acceptedSig *Signature // non-nil if accepted + rejected := false + // FIXME? Say more about the contents of the signature, i.e. parse it even before verification?! + logrus.Debugf("Evaluating signature %d:", sigNumber) + interpretingReqs: + for reqNumber, req := range reqs { + // FIXME: Log the requirement itself? For now, we use just the number. + // FIXME: supply state + switch res, as, err := req.isSignatureAuthorAccepted(image, sig); res { + case sarAccepted: + if as == nil { // Coverage: this should never happen + logrus.Debugf(" Requirement %d: internal inconsistency: sarAccepted but no parsed contents", reqNumber) + rejected = true + break interpretingReqs + } + logrus.Debugf(" Requirement %d: signature accepted", reqNumber) + if acceptedSig == nil { + acceptedSig = as + } else if *as != *acceptedSig { // Coverage: this should never happen + // Huh?! Two ways of verifying the same signature blob resulted in two different parses of its already accepted contents? + logrus.Debugf(" Requirement %d: internal inconsistency: sarAccepted but different parsed contents", reqNumber) + rejected = true + acceptedSig = nil + break interpretingReqs + } + case sarRejected: + logrus.Debugf(" Requirement %d: signature rejected: %s", reqNumber, err.Error()) + rejected = true + break interpretingReqs + case sarUnknown: + if err != nil { // Coverage: this should never happen + logrus.Debugf(" Requirement %d: internal inconsistency: sarUnknown but an error message %s", reqNumber, err.Error()) + rejected = true + break interpretingReqs + } + logrus.Debugf(" Requirement %d: signature state unknown, continuing", reqNumber) + default: // Coverage: this should never happen + logrus.Debugf(" Requirement %d: internal inconsistency: unknown result %#v", reqNumber, string(res)) + rejected = true + break interpretingReqs + } + } + // This also handles the (invalid) case of empty reqs, by rejecting the signature. + if acceptedSig != nil && !rejected { + logrus.Debugf(" Overall: OK, signature accepted") + res = append(res, acceptedSig) + } else { + logrus.Debugf(" Overall: Signature not accepted") + } + } + return res, nil +} + +// IsRunningImageAllowed returns true iff the policy allows running the image. +// If it returns false, err must be non-nil, and should be an PolicyRequirementError if evaluation +// succeeded but the result was rejection. +// WARNING: This validates signatures and the manifest, but does not download or validate the +// layers. Users must validate that the layers match their expected digests. +func (pc *PolicyContext) IsRunningImageAllowed(image types.Image) (res bool, finalErr error) { + if err := pc.changeState(pcReady, pcInUse); err != nil { + return false, err + } + defer func() { + if err := pc.changeState(pcInUse, pcReady); err != nil { + res = false + finalErr = err + } + }() + + logrus.Debugf("IsRunningImageAllowed for image %s", image.IntendedDockerReference()) + + reqs, err := pc.requirementsForImage(image) + if err != nil { + return false, err + } + + if len(reqs) == 0 { + return false, PolicyRequirementError("List of verification policy requirements must not be empty") + } + + for reqNumber, req := range reqs { + // FIXME: supply state + allowed, err := req.isRunningImageAllowed(image) + if !allowed { + logrus.Debugf("Requirement %d: denied, done", reqNumber) + return false, err + } + logrus.Debugf(" Requirement %d: allowed", reqNumber) + } + // We have tested that len(reqs) != 0, so at least one req must have explicitly allowed this image. + logrus.Debugf("Overall: allowed") + return true, nil +} diff --git a/signature/policy_eval_test.go b/signature/policy_eval_test.go index 4e1e0ca2..254b285d 100644 --- a/signature/policy_eval_test.go +++ b/signature/policy_eval_test.go @@ -1,11 +1,456 @@ package signature import ( + "fmt" + "os" "testing" + "github.com/projectatomic/skopeo/reference" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func TestPolicyRequirementError(t *testing.T) { + // A stupid test just to keep code coverage + s := "test" + err := PolicyRequirementError(s) + assert.Equal(t, s, err.Error()) +} + +func TestPolicyContextChangeState(t *testing.T) { + pc, err := NewPolicyContext(&Policy{Default: PolicyRequirements{NewPRReject()}}) + require.NoError(t, err) + defer pc.Destroy() + + require.Equal(t, pcReady, pc.state) + err = pc.changeState(pcReady, pcInUse) + require.NoError(t, err) + + err = pc.changeState(pcReady, pcInUse) + require.Error(t, err) + + // Return state to pcReady to allow pc.Destroy to clean up. + err = pc.changeState(pcInUse, pcReady) + require.NoError(t, err) +} + +func TestPolicyContextNewDestroy(t *testing.T) { + pc, err := NewPolicyContext(&Policy{Default: PolicyRequirements{NewPRReject()}}) + require.NoError(t, err) + assert.Equal(t, pcReady, pc.state) + + err = pc.Destroy() + require.NoError(t, err) + assert.Equal(t, pcDestroyed, pc.state) + + // Trying to destroy when not pcReady + pc, err = NewPolicyContext(&Policy{Default: PolicyRequirements{NewPRReject()}}) + require.NoError(t, err) + err = pc.changeState(pcReady, pcInUse) + require.NoError(t, err) + err = pc.Destroy() + require.Error(t, err) + assert.Equal(t, pcInUse, pc.state) // The state, and hopefully nothing else, has changed. + + err = pc.changeState(pcInUse, pcReady) + require.NoError(t, err) + err = pc.Destroy() + assert.NoError(t, err) +} + +func TestFullyExpandedDockerReference(t *testing.T) { + sha256Digest := "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" + // Test both that fullyExpandedDockerReference returns the expected value (fullName+suffix), + // and that .FullName returns the expected value (fullName), i.e. that the two functions are + // consistent. + for inputName, fullName := range map[string]string{ + "example.com/ns/repo": "example.com/ns/repo", + "example.com/repo": "example.com/repo", + "localhost/ns/repo": "localhost/ns/repo", + // Note that "localhost" is special here: notlocalhost/repo is be parsed as docker.io/notlocalhost.repo: + "localhost/repo": "localhost/repo", + "notlocalhost/repo": "docker.io/notlocalhost/repo", + "docker.io/ns/repo": "docker.io/ns/repo", + "docker.io/library/repo": "docker.io/library/repo", + "docker.io/repo": "docker.io/library/repo", + "ns/repo": "docker.io/ns/repo", + "library/repo": "docker.io/library/repo", + "repo": "docker.io/library/repo", + } { + for inputSuffix, mappedSuffix := range map[string]string{ + ":tag": ":tag", + sha256Digest: sha256Digest, + "": "", + // A github.com/distribution/reference value can have a tag and a digest at the same time! + // github.com/skopeo/reference handles that by dropping the tag. That is not obviously the + // right thing to do, but it is at least reasonable, so test that we keep behaving reasonably. + // This test case should not be construed to make this an API promise. + ":tag" + sha256Digest: sha256Digest, + } { + fullInput := inputName + inputSuffix + ref, err := reference.ParseNamed(fullInput) + require.NoError(t, err) + assert.Equal(t, fullName, ref.FullName(), fullInput) + expanded, err := fullyExpandedDockerReference(ref) + require.NoError(t, err) + assert.Equal(t, fullName+mappedSuffix, expanded, fullInput) + } + } +} + +func TestPolicyContextRequirementsForImage(t *testing.T) { + ktGPG := SBKeyTypeGPGKeys + prm := NewPRMMatchExact() + + policy := &Policy{ + Default: PolicyRequirements{NewPRReject()}, + Specific: map[string]PolicyRequirements{}, + } + // Just put _something_ into the Specific map for the keys we care about, and make it pairwise + // distinct so that we can compare the values and show them when debugging the tests. + for _, scope := range []string{ + "unmatched", + "hostname.com", + "hostname.com/namespace", + "hostname.com/namespace/repo", + "hostname.com/namespace/repo:latest", + "hostname.com/namespace/repo:tag2", + "localhost", + "localhost/namespace", + "localhost/namespace/repo", + "localhost/namespace/repo:latest", + "localhost/namespace/repo:tag2", + "deep.com", + "deep.com/n1", + "deep.com/n1/n2", + "deep.com/n1/n2/n3", + "deep.com/n1/n2/n3/repo", + "deep.com/n1/n2/n3/repo:tag2", + "docker.io", + "docker.io/library", + "docker.io/library/busybox", + "docker.io/namespaceindocker", + "docker.io/namespaceindocker/repo", + "docker.io/namespaceindocker/repo:tag2", + // Note: these non-fully-expanded repository names are not matched against canonical (shortened) + // Docker names; they are instead parsed as starting with hostnames. + "busybox", + "library/busybox", + "namespaceindocker", + "namespaceindocker/repo", + "namespaceindocker/repo:tag2", + } { + policy.Specific[scope] = PolicyRequirements{xNewPRSignedByKeyData(ktGPG, []byte(scope), prm)} + } + + pc, err := NewPolicyContext(policy) + require.NoError(t, err) + + for input, matched := range map[string]string{ + // Full match + "hostname.com/namespace/repo:latest": "hostname.com/namespace/repo:latest", + "hostname.com/namespace/repo:tag2": "hostname.com/namespace/repo:tag2", + "hostname.com/namespace/repo": "hostname.com/namespace/repo:latest", + "localhost/namespace/repo:latest": "localhost/namespace/repo:latest", + "localhost/namespace/repo:tag2": "localhost/namespace/repo:tag2", + "localhost/namespace/repo": "localhost/namespace/repo:latest", + "deep.com/n1/n2/n3/repo:tag2": "deep.com/n1/n2/n3/repo:tag2", + // Repository match + "hostname.com/namespace/repo:notlatest": "hostname.com/namespace/repo", + "localhost/namespace/repo:notlatest": "localhost/namespace/repo", + "deep.com/n1/n2/n3/repo:nottag2": "deep.com/n1/n2/n3/repo", + // Namespace match + "hostname.com/namespace/notrepo:latest": "hostname.com/namespace", + "localhost/namespace/notrepo:latest": "localhost/namespace", + "deep.com/n1/n2/n3/notrepo:tag2": "deep.com/n1/n2/n3", + "deep.com/n1/n2/notn3/repo:tag2": "deep.com/n1/n2", + "deep.com/n1/notn2/n3/repo:tag2": "deep.com/n1", + // Host name match + "hostname.com/notnamespace/repo:latest": "hostname.com", + "localhost/notnamespace/repo:latest": "localhost", + "deep.com/notn1/n2/n3/repo:tag2": "deep.com", + // Default + "this.doesnt/match:anything": "", + "this.doesnt/match-anything/defaulttag": "", + + // docker.io canonizalication effects + "docker.io/library/busybox": "docker.io/library/busybox", + "library/busybox": "docker.io/library/busybox", + "busybox": "docker.io/library/busybox", + "docker.io/library/somethinginlibrary": "docker.io/library", + "library/somethinginlibrary": "docker.io/library", + "somethinginlibrary": "docker.io/library", + "docker.io/namespaceindocker/repo:tag2": "docker.io/namespaceindocker/repo:tag2", + "namespaceindocker/repo:tag2": "docker.io/namespaceindocker/repo:tag2", + "docker.io/namespaceindocker/repo:nottag2": "docker.io/namespaceindocker/repo", + "namespaceindocker/repo:nottag2": "docker.io/namespaceindocker/repo", + "docker.io/namespaceindocker/notrepo:tag2": "docker.io/namespaceindocker", + "namespaceindocker/notrepo:tag2": "docker.io/namespaceindocker", + "docker.io/notnamespaceindocker/repo:tag2": "docker.io", + "notnamespaceindocker/repo:tag2": "docker.io", + } { + var expected PolicyRequirements + if matched != "" { + e, ok := policy.Specific[matched] + require.True(t, ok, fmt.Sprintf("case %s: expected reqs not found", input)) + expected = e + } else { + expected = policy.Default + } + + reqs, err := pc.requirementsForImage(refImageMock(input)) + require.NoError(t, err) + comment := fmt.Sprintf("case %s: %#v", input, reqs[0]) + // Do not sue assert.Equal, which would do a deep contents comparison; we want to compare + // the pointers. Also, == does not work on slices; so test that the slices start at the + // same element and have the same length. + assert.True(t, &(reqs[0]) == &(expected[0]), comment) + assert.True(t, len(reqs) == len(expected), comment) + } + + // Invalid reference format + _, err = pc.requirementsForImage(refImageMock("UPPERCASEISINVALID")) + assert.Error(t, err) +} + +func TestPolicyContextGetSignaturesWithAcceptedAuthor(t *testing.T) { + expectedSig := &Signature{ + DockerManifestDigest: TestImageManifestDigest, + DockerReference: "testing/manifest:latest", + } + + pc, err := NewPolicyContext(&Policy{ + Default: PolicyRequirements{NewPRReject()}, + Specific: map[string]PolicyRequirements{ + "docker.io/testing/manifest:latest": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchExact()), + }, + "docker.io/testing/manifest:twoAccepts": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:acceptReject": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + NewPRReject(), + }, + "docker.io/testing/manifest:acceptUnknown": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + xNewPRSignedBaseLayer(NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:rejectUnknown": { + NewPRReject(), + xNewPRSignedBaseLayer(NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:unknown": { + xNewPRSignedBaseLayer(NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:unknown2": { + NewPRInsecureAcceptAnything(), + }, + "docker.io/testing/manifest:invalidEmptyRequirements": {}, + }, + }) + require.NoError(t, err) + defer pc.Destroy() + + // Success + image := dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + sigs, err := pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // Two signatures + // FIXME? Use really different signatures for this? + image = dirImageMock("fixtures/dir-img-valid-2", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig, expectedSig}, sigs) + + // No signatures + image = dirImageMock("fixtures/dir-img-unsigned", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // Only invalid signatures + image = dirImageMock("fixtures/dir-img-modified-manifest", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // 1 invalid, 1 valid signature (in this order) + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // Two sarAccepted results for one signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:twoAccepts") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // sarAccepted+sarRejected for a signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:acceptReject") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // sarAccepted+sarUnknown for a signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:acceptUnknown") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Equal(t, []*Signature{expectedSig}, sigs) + + // sarRejected+sarUnknown for a signature + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:rejectUnknown") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // sarUnknown only + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:unknown") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:unknown2") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // Empty list of requirements (invalid) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + require.NoError(t, err) + assert.Empty(t, sigs) + + // Failures: Make sure we return nil sigs. + + // Unexpected state (context already destroyed) + destroyedPC, err := NewPolicyContext(pc.Policy) + require.NoError(t, err) + err = destroyedPC.Destroy() + require.NoError(t, err) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + sigs, err = destroyedPC.GetSignaturesWithAcceptedAuthor(image) + assert.Error(t, err) + assert.Nil(t, sigs) + // Not testing the pcInUse->pcReady transition, that would require custom PolicyRequirement + // implementations meddling with the state, or threads. This is for catching trivial programmer + // mistakes only, anyway. + + // Invalid IntendedDockerReference value + image = dirImageMock("fixtures/dir-img-valid", "UPPERCASEISINVALID") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + assert.Error(t, err) + assert.Nil(t, sigs) + + // Error reading signatures. + invalidSigDir := createInvalidSigDir(t) + defer os.RemoveAll(invalidSigDir) + image = dirImageMock(invalidSigDir, "testing/manifest:latest") + sigs, err = pc.GetSignaturesWithAcceptedAuthor(image) + assert.Error(t, err) + assert.Nil(t, sigs) +} + +func TestPolicyContextIsRunningImageAllowed(t *testing.T) { + pc, err := NewPolicyContext(&Policy{ + Default: PolicyRequirements{NewPRReject()}, + Specific: map[string]PolicyRequirements{ + "docker.io/testing/manifest:latest": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchExact()), + }, + "docker.io/testing/manifest:twoAllows": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + }, + "docker.io/testing/manifest:allowDeny": { + xNewPRSignedByKeyPath(SBKeyTypeGPGKeys, "fixtures/public-key.gpg", NewPRMMatchRepository()), + NewPRReject(), + }, + "docker.io/testing/manifest:reject": { + NewPRReject(), + }, + "docker.io/testing/manifest:acceptAnything": { + NewPRInsecureAcceptAnything(), + }, + "docker.io/testing/manifest:invalidEmptyRequirements": {}, + }, + }) + require.NoError(t, err) + defer pc.Destroy() + + // Success + image := dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + res, err := pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Two signatures + // FIXME? Use really different signatures for this? + image = dirImageMock("fixtures/dir-img-valid-2", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // No signatures + image = dirImageMock("fixtures/dir-img-unsigned", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // Only invalid signatures + image = dirImageMock("fixtures/dir-img-modified-manifest", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // 1 invalid, 1 valid signature (in this order) + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:latest") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Two allowed results + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:twoAllows") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Allow + deny results + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:allowDeny") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // prReject works + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:reject") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // prInsecureAcceptAnything works + image = dirImageMock("fixtures/dir-img-mixed", "testing/manifest:acceptAnything") + res, err = pc.IsRunningImageAllowed(image) + assertRunningAllowed(t, res, err) + + // Empty list of requirements (invalid) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:invalidEmptyRequirements") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejectedPolicyRequirement(t, res, err) + + // Unexpected state (context already destroyed) + destroyedPC, err := NewPolicyContext(pc.Policy) + require.NoError(t, err) + err = destroyedPC.Destroy() + require.NoError(t, err) + image = dirImageMock("fixtures/dir-img-valid", "testing/manifest:latest") + res, err = destroyedPC.IsRunningImageAllowed(image) + assertRunningRejected(t, res, err) + // Not testing the pcInUse->pcReady transition, that would require custom PolicyRequirement + // implementations meddling with the state, or threads. This is for catching trivial programmer + // mistakes only, anyway. + + // Invalid IntendedDockerReference value + image = dirImageMock("fixtures/dir-img-valid", "UPPERCASEISINVALID") + res, err = pc.IsRunningImageAllowed(image) + assertRunningRejected(t, res, err) +} + // Helpers for validating PolicyRequirement.isSignatureAuthorAccepted results: // assertSARRejected verifies that isSignatureAuthorAccepted returns a consistent sarRejected result