From 184b810c0581a9c835e22a61a1d49a3dbee37d30 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 20 Jan 2017 08:22:59 +0100 Subject: [PATCH] BEHAVIOR CHANGE: Do not re-construct the reference in XParseNamed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of rebuilding it as name/name+digest/name+tag, just use the return value from distreference.ParseNormalizedName without modification. THIS CHANGES BEHAVIOR: before, name@tag:digest inputs were silently trated as name:digest, dropping the tag; now the semantics is correctly preserved. We already anticipate such strings as references in docker: and docker-daemon: (where they are now rejected) and in signature verification (where, unless we check repository names only, they must match exactly). Signed-off-by: Miloslav Trmač --- docker/daemon/daemon_transport.go | 3 ++- docker/daemon/daemon_transport_test.go | 9 +++------ docker/docker_transport.go | 3 ++- docker/docker_transport_test.go | 12 +++++------- docker/policyconfiguration/naming_test.go | 14 +++++--------- docker/reference/reference.go | 16 +--------------- docker/reference/reference_test.go | 16 ---------------- signature/policy_reference_match_test.go | 22 +++++++++------------- 8 files changed, 27 insertions(+), 68 deletions(-) diff --git a/docker/daemon/daemon_transport.go b/docker/daemon/daemon_transport.go index a80225ad..5b629347 100644 --- a/docker/daemon/daemon_transport.go +++ b/docker/daemon/daemon_transport.go @@ -81,7 +81,8 @@ func NewReference(id digest.Digest, ref distreference.Named) (types.ImageReferen return nil, errors.Errorf("docker-daemon: reference %s has neither a tag nor a digest", distreference.FamiliarString(ref)) } // A github.com/distribution/reference value can have a tag and a digest at the same time! - // docker/reference does not handle that, so fail. + // Most versions of docker/reference do not handle that (ignoring the tag), so reject such input. + // This MAY be accepted in the future. _, isTagged := ref.(distreference.NamedTagged) _, isDigested := ref.(distreference.Canonical) if isTagged && isDigested { diff --git a/docker/daemon/daemon_transport_test.go b/docker/daemon/daemon_transport_test.go index 09f94aba..d6b4186e 100644 --- a/docker/daemon/daemon_transport_test.go +++ b/docker/daemon/daemon_transport_test.go @@ -54,12 +54,9 @@ func testParseReference(t *testing.T, fn func(string) (types.ImageReference, err {"busybox:latest", "", "docker.io/library/busybox:latest"}, // Explicit tag {"busybox@" + sha256digest, "", "docker.io/library/busybox@" + sha256digest}, // Explicit digest // A github.com/distribution/reference value can have a tag and a digest at the same time! - // github.com/docker/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. - // FIXME? Instead work extra hard to reject such input? - {"busybox:latest@" + sha256digest, "", "docker.io/library/busybox@" + sha256digest}, // Both tag and digest - {"docker.io/library/busybox:latest", "", "docker.io/library/busybox:latest"}, // All implied values explicitly specified + // Most versions of docker/reference do not handle that (ignoring the tag), so we reject such input. + {"busybox:latest@" + sha256digest, "", ""}, // Both tag and digest + {"docker.io/library/busybox:latest", "", "docker.io/library/busybox:latest"}, // All implied values explicitly specified } { ref, err := fn(c.input) if c.expectedID == "" && c.expectedRef == "" { diff --git a/docker/docker_transport.go b/docker/docker_transport.go index 9fe352d2..2fd6f008 100644 --- a/docker/docker_transport.go +++ b/docker/docker_transport.go @@ -60,7 +60,8 @@ func NewReference(ref distreference.Named) (types.ImageReference, error) { return nil, errors.Errorf("Docker reference %s has neither a tag nor a digest", distreference.FamiliarString(ref)) } // A github.com/distribution/reference value can have a tag and a digest at the same time! - // docker/reference does not handle that, so fail. + // The docker/distribution API does not really support that (we can’t ask for an image with a specific + // tag and digest), so fail. This MAY be accepted in the future. // (Even if it were supported, the semantics of policy namespaces are unclear - should we drop // the tag or the digest first?) _, isTagged := ref.(distreference.NamedTagged) diff --git a/docker/docker_transport_test.go b/docker/docker_transport_test.go index e3916421..91ca1488 100644 --- a/docker/docker_transport_test.go +++ b/docker/docker_transport_test.go @@ -48,13 +48,11 @@ func testParseReference(t *testing.T, fn func(string) (types.ImageReference, err {"//busybox" + sha256digest, "docker.io/library/busybox" + sha256digest}, // Explicit digest {"//busybox", "docker.io/library/busybox:latest"}, // Default tag // A github.com/distribution/reference value can have a tag and a digest at the same time! - // github.com/docker/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. - // FIXME? Instead work extra hard to reject such input? - {"//busybox:latest" + sha256digest, "docker.io/library/busybox" + sha256digest}, // Both tag and digest - {"//docker.io/library/busybox:latest", "docker.io/library/busybox:latest"}, // All implied values explicitly specified - {"//UPPERCASEISINVALID", ""}, // Invalid input + // The docker/distribution API does not really support that (we can’t ask for an image with a specific + // tag and digest), so fail. This MAY be accepted in the future. + {"//busybox:latest" + sha256digest, ""}, // Both tag and digest + {"//docker.io/library/busybox:latest", "docker.io/library/busybox:latest"}, // All implied values explicitly specified + {"//UPPERCASEISINVALID", ""}, // Invalid input } { ref, err := fn(c.input) if c.expected == "" { diff --git a/docker/policyconfiguration/naming_test.go b/docker/policyconfiguration/naming_test.go index 71290d6c..db5c6ecd 100644 --- a/docker/policyconfiguration/naming_test.go +++ b/docker/policyconfiguration/naming_test.go @@ -36,11 +36,6 @@ func TestDockerReference(t *testing.T) { 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/docker/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.XParseNamed(fullInput) @@ -81,12 +76,13 @@ func TestDockerReferenceIdentity(t *testing.T) { assert.Error(t, err) // A github.com/distribution/reference value can have a tag and a digest at the same time! - parsed, err = reference.XParseNamed("busybox@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef") + parsed, err = reference.XParseNamed("busybox:notlatest@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef") require.NoError(t, err) - refDigested, ok := parsed.(distreference.Canonical) + _, ok := parsed.(distreference.Canonical) require.True(t, ok) - tagDigestRef := refWithTagAndDigest{refDigested} - id, err = DockerReferenceIdentity(tagDigestRef) + _, ok = parsed.(distreference.NamedTagged) + require.True(t, ok) + id, err = DockerReferenceIdentity(parsed) assert.Equal(t, "", id) assert.Error(t, err) } diff --git a/docker/reference/reference.go b/docker/reference/reference.go index f8547b87..340f2f98 100644 --- a/docker/reference/reference.go +++ b/docker/reference/reference.go @@ -17,21 +17,7 @@ import ( // returned. // If an error was encountered it is returned, along with a nil Reference. func XParseNamed(s string) (distreference.Named, error) { - named, err := distreference.ParseNormalizedNamed(s) - if err != nil { - return nil, errors.Wrapf(err, "Error parsing reference: %q is not a valid repository/tag", s) - } - r, err := distreference.WithName(named.Name()) - if err != nil { - return nil, err - } - if canonical, isCanonical := named.(distreference.Canonical); isCanonical { - return distreference.WithDigest(r, canonical.Digest()) - } - if tagged, isTagged := named.(distreference.NamedTagged); isTagged { - return distreference.WithTag(r, tagged.Tag()) - } - return r, nil + return distreference.ParseNormalizedNamed(s) } // XParseIDOrReference parses string for an image ID or a reference. ID can be diff --git a/docker/reference/reference_test.go b/docker/reference/reference_test.go index 0db49b7f..9153b4c6 100644 --- a/docker/reference/reference_test.go +++ b/docker/reference/reference_test.go @@ -243,19 +243,3 @@ func TestParseRepositoryInfo(t *testing.T) { } } } - -func TestParseReferenceWithTagAndDigest(t *testing.T) { - ref, err := XParseNamed("busybox:latest@sha256:86e0e091d0da6bde2456dbb48306f3956bbeb2eae1b5b9a43045843f69fe4aaa") - if err != nil { - t.Fatal(err) - } - if _, isTagged := ref.(distreference.NamedTagged); isTagged { - t.Fatalf("Reference from %q should not support tag", ref) - } - if _, isCanonical := ref.(distreference.Canonical); !isCanonical { - t.Fatalf("Reference from %q should not support digest", ref) - } - if expected, actual := "busybox@sha256:86e0e091d0da6bde2456dbb48306f3956bbeb2eae1b5b9a43045843f69fe4aaa", distreference.FamiliarString(ref); actual != expected { - t.Fatalf("Invalid parsed reference for %q: expected %q, got %q", ref, expected, actual) - } -} diff --git a/signature/policy_reference_match_test.go b/signature/policy_reference_match_test.go index b7e4194c..413b11a5 100644 --- a/signature/policy_reference_match_test.go +++ b/signature/policy_reference_match_test.go @@ -148,14 +148,12 @@ var prmExactMatchTestTable = []prmSymmetricTableTest{ {"busybox", "busybox:latest", false}, {"busybox", "busybox" + digestSuffix, false}, {"busybox", "busybox", false}, - // References with both tags and digests: `reference.XParseNamed` essentially drops the tag. - // This is not _particularly_ desirable but it is the semantics used throughout containers/image; at least, with the digest it is clear which image the reference means, - // even if the tag may reflect a different user intent. + // References with both tags and digests: We match them exactly (requiring BOTH to match) // NOTE: Again, this is not documented behavior; the recommendation is to sign tags, not digests, and then tag-and-digest references won’t match the signed identity. {"busybox:latest" + digestSuffix, "busybox:latest" + digestSuffix, true}, {"busybox:latest" + digestSuffix, "busybox:latest" + digestSuffixOther, false}, - {"busybox:latest" + digestSuffix, "busybox:notlatest" + digestSuffix, true}, // Ugly. Do not rely on this. - {"busybox:latest" + digestSuffix, "busybox" + digestSuffix, true}, // Ugly. Do not rely on this. + {"busybox:latest" + digestSuffix, "busybox:notlatest" + digestSuffix, false}, + {"busybox:latest" + digestSuffix, "busybox" + digestSuffix, false}, {"busybox:latest" + digestSuffix, "busybox:latest", false}, // Invalid format {"UPPERCASE_IS_INVALID_IN_DOCKER_REFERENCES", "busybox:latest", false}, @@ -194,7 +192,7 @@ var prmRepositoryMatchTestTable = []prmSymmetricTableTest{ {"hostname/library/busybox:latest", "busybox:notlatest", false}, {"busybox:latest", fullRHELRef, false}, {"busybox" + digestSuffix, "notbusybox" + digestSuffix, false}, - // References with both tags and digests: `reference.XParseNamed` essentially drops the tag, and we ignore both anyway. + // References with both tags and digests: We ignore both anyway. {"busybox:latest" + digestSuffix, "busybox:latest" + digestSuffix, true}, {"busybox:latest" + digestSuffix, "busybox:latest" + digestSuffixOther, true}, {"busybox:latest" + digestSuffix, "busybox:notlatest" + digestSuffix, true}, @@ -272,14 +270,12 @@ func TestPMMMatchRepoDigestOrExactMatchesDockerReference(t *testing.T) { // Digest references accept any signature with matching repository. {"busybox" + digestSuffix, "busybox:latest", true}, {"busybox" + digestSuffix, "busybox" + digestSuffixOther, true}, // Even this is accepted here. (This could more reasonably happen with two different digest algorithms.) - // References with both tags and digests: `reference.XParseNamed` essentially drops the tag. - // This is not _particularly_ desirable but it is the semantics used throughout containers/image; at least, with the digest it is clear which image the reference means, - // even if the tag may reflect a different user intent. - {"busybox:latest" + digestSuffix, "busybox:latest", true}, - {"busybox:latest" + digestSuffix, "busybox:notlatest", true}, + // References with both tags and digests: We match them exactly (requiring BOTH to match). + {"busybox:latest" + digestSuffix, "busybox:latest", false}, + {"busybox:latest" + digestSuffix, "busybox:notlatest", false}, {"busybox:latest", "busybox:latest" + digestSuffix, false}, - {"busybox:latest" + digestSuffix, "busybox:latest" + digestSuffixOther, true}, // Even this is accepted here. (This could more reasonably happen with two different digest algorithms.) - {"busybox:latest" + digestSuffix, "busybox:notlatest" + digestSuffixOther, true}, // Ugly. Do not rely on this. + {"busybox:latest" + digestSuffix, "busybox:latest" + digestSuffixOther, false}, + {"busybox:latest" + digestSuffix, "busybox:notlatest" + digestSuffixOther, false}, } { testImageAndSig(t, prm, test.imageRef, test.sigRef, test.result) }