containers
/
image
mirror of https://github.com/containers/image.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

BEHAVIOR CHANGE: Do not re-construct the reference in XParseNamed 

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č <mitr@redhat.com>
This commit is contained in:
Miloslav Trmač 2017-01-20 08:22:59 +01:00
parent e928b402a5
commit 184b810c05
8 changed files with 27 additions and 68 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
docker/daemon/daemon_transport.go
View File

@ -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 {

7
docker/daemon/daemon_transport_test.go
View File

@ -54,11 +54,8 @@ 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
// 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)

3
docker/docker_transport.go
View File

@ -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)

8
docker/docker_transport_test.go
View File

@ -48,11 +48,9 @@ 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
// 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
} {

14
docker/policyconfiguration/naming_test.go
View File

@ -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)
}

16
docker/reference/reference.go
View File

@ -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

16
docker/reference/reference_test.go
View File

@ -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)
}
}

22
signature/policy_reference_match_test.go
View File

@ -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)
}