From 51148574cbd079df309b52f05ceea891ca9ceb5d Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Wed, 12 Jul 2017 17:06:03 -0400 Subject: [PATCH] Fix handling of references with digests References can contain digests or tags, but not both, so fix our parser to handle either type of name for use as image names. Update PolicyConfigurationNamespaces() to discard tags and digests when computing the list of alternate namespaces which can match an image. Signed-off-by: Nalin Dahyabhai --- storage/storage_reference.go | 48 +++++++--- storage/storage_reference_test.go | 6 +- storage/storage_transport.go | 143 +++++++++++++++++++++--------- storage/storage_transport_test.go | 8 +- 4 files changed, 145 insertions(+), 60 deletions(-) diff --git a/storage/storage_reference.go b/storage/storage_reference.go index 9aee75be02..905b8501a3 100644 --- a/storage/storage_reference.go +++ b/storage/storage_reference.go @@ -7,6 +7,7 @@ import ( "github.com/containers/image/docker/reference" "github.com/containers/image/types" "github.com/containers/storage" + digest "github.com/opencontainers/go-digest" "github.com/pkg/errors" ) @@ -17,10 +18,12 @@ type storageReference struct { transport storageTransport reference string id string - name reference.Named + name reference.Reference + tag string + digest digest.Digest } -func newReference(transport storageTransport, reference, id string, name reference.Named) *storageReference { +func newReference(transport storageTransport, reference, id string, name reference.Reference, tag string, digest digest.Digest) *storageReference { // We take a copy of the transport, which contains a pointer to the // store that it used for resolving this reference, so that the // transport that we'll return from Transport() won't be affected by @@ -30,6 +33,8 @@ func newReference(transport storageTransport, reference, id string, name referen reference: reference, id: id, name: name, + tag: tag, + digest: digest, } } @@ -76,9 +81,26 @@ func (s storageReference) Transport() types.ImageTransport { } } -// Return a name with a tag, if we have a name to base them on. +// Return a name with a tag or digest, if we have either, else return it bare. func (s storageReference) DockerReference() reference.Named { - return s.name + if s.name == nil { + return nil + } + name, ok := s.name.(reference.Named) + if !ok { + return nil + } + if s.tag != "" { + if namedTagged, err := reference.WithTag(name, s.tag); err == nil { + return namedTagged + } + } + if s.digest != "" { + if canonical, err := reference.WithDigest(name, s.digest); err == nil { + return canonical + } + } + return name } // Return a name with a tag, prefixed with the graph root and driver name, to @@ -91,7 +113,7 @@ func (s storageReference) StringWithinTransport() string { optionsList = ":" + strings.Join(options, ",") } storeSpec := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "+" + s.transport.store.RunRoot() + optionsList + "]" - if s.name == nil { + if s.reference == "" { return storeSpec + "@" + s.id } if s.id == "" { @@ -120,14 +142,14 @@ func (s storageReference) PolicyConfigurationNamespaces() []string { driverlessStoreSpec := "[" + s.transport.store.GraphRoot() + "]" namespaces := []string{} if s.name != nil { - if s.id != "" { - // The reference without the ID is also a valid namespace. - namespaces = append(namespaces, storeSpec+s.reference) - } - components := strings.Split(s.name.Name(), "/") - for len(components) > 0 { - namespaces = append(namespaces, storeSpec+strings.Join(components, "/")) - components = components[:len(components)-1] + name, ok := s.name.(reference.Named) + if ok { + name = reference.TrimNamed(name) + components := strings.Split(name.String(), "/") + for len(components) > 0 { + namespaces = append(namespaces, storeSpec+strings.Join(components, "/")) + components = components[:len(components)-1] + } } } namespaces = append(namespaces, storeSpec) diff --git a/storage/storage_reference_test.go b/storage/storage_reference_test.go index 37ddcf3364..bc7100bbe6 100644 --- a/storage/storage_reference_test.go +++ b/storage/storage_reference_test.go @@ -52,7 +52,11 @@ var validReferenceTestCases = []struct { }, { "busybox@" + sha256digestHex, "docker.io/library/busybox:latest@" + sha256digestHex, - []string{"docker.io/library/busybox:latest", "docker.io/library/busybox", "docker.io/library", "docker.io"}, + []string{"docker.io/library/busybox", "docker.io/library", "docker.io"}, + }, + { + "busybox@sha256:" + sha256digestHex, "docker.io/library/busybox@sha256:" + sha256digestHex, + []string{"docker.io/library/busybox", "docker.io/library", "docker.io"}, }, } diff --git a/storage/storage_transport.go b/storage/storage_transport.go index 336dd814a4..8abeacd903 100644 --- a/storage/storage_transport.go +++ b/storage/storage_transport.go @@ -12,8 +12,7 @@ import ( "github.com/containers/image/types" "github.com/containers/storage" "github.com/containers/storage/pkg/idtools" - "github.com/opencontainers/go-digest" - ddigest "github.com/opencontainers/go-digest" + digest "github.com/opencontainers/go-digest" ) func init() { @@ -101,8 +100,6 @@ func (s *storageTransport) DefaultGIDMap() []idtools.IDMap { // relative to the given store, and returns it in a reference object. func (s storageTransport) ParseStoreReference(store storage.Store, ref string) (*storageReference, error) { var name reference.Named - var sum digest.Digest - var err error if ref == "" { return nil, ErrInvalidReference } @@ -110,51 +107,95 @@ func (s storageTransport) ParseStoreReference(store storage.Store, ref string) ( // Ignore the store specifier. closeIndex := strings.IndexRune(ref, ']') if closeIndex < 1 { - return nil, ErrInvalidReference + return nil, errors.Wrapf(ErrInvalidReference, "store specifier in %q did not end", ref) } ref = ref[closeIndex+1:] } - refInfo := strings.SplitN(ref, "@", 2) - if len(refInfo) == 1 { - // A name. - name, err = reference.ParseNormalizedNamed(refInfo[0]) - if err != nil { - return nil, err + + // The last segment, if there's more than one, is either a digest from a reference, or an image ID. + split := strings.LastIndex(ref, "@") + idOrDigest := "" + if split != -1 { + // Peel off that last bit so that we can work on the rest. + idOrDigest = ref[split+1:] + if idOrDigest == "" { + return nil, errors.Wrapf(ErrInvalidReference, "%q does not look like a digest or image ID", idOrDigest) } - } else if len(refInfo) == 2 { - // An ID, possibly preceded by a name. - if refInfo[0] != "" { - name, err = reference.ParseNormalizedNamed(refInfo[0]) - if err != nil { - return nil, err - } + ref = ref[:split] + } + + // The middle segment (now the last segment), if there is one, is a digest. + split = strings.LastIndex(ref, "@") + sum := digest.Digest("") + if split != -1 { + sum = digest.Digest(ref[split+1:]) + if sum == "" { + return nil, errors.Wrapf(ErrInvalidReference, "%q does not look like an image digest", sum) } - sum, err = digest.Parse(refInfo[1]) - if err != nil || sum.Validate() != nil { - sum, err = digest.Parse("sha256:" + refInfo[1]) - if err != nil || sum.Validate() != nil { - return nil, err - } + ref = ref[:split] + } + + // If we have something that unambiguously should be a digest, validate it, and then the third part, + // if we have one, as an ID. + id := "" + if sum != "" { + if idSum, err := digest.Parse("sha256:" + idOrDigest); err != nil || idSum.Validate() != nil { + return nil, errors.Wrapf(ErrInvalidReference, "%q does not look like an image ID", idOrDigest) + } + if err := sum.Validate(); err != nil { + return nil, errors.Wrapf(ErrInvalidReference, "%q does not look like an image digest", sum) + } + id = idOrDigest + } else if idOrDigest != "" { + // There was no middle portion, so the final portion could be either a digest or an ID. + if idSum, err := digest.Parse("sha256:" + idOrDigest); err == nil && idSum.Validate() == nil { + id = idOrDigest + } else if idSum, err := digest.Parse(idOrDigest); err == nil && idSum.Validate() == nil { + sum = idSum + } else { + return nil, errors.Wrapf(ErrInvalidReference, "%q does not look like a digest or image ID", idOrDigest) } - } else { // Coverage: len(refInfo) is always 1 or 2 - // Anything else: store specified in a form we don't - // recognize. - return nil, ErrInvalidReference } + + // The initial portion is a name, possibly with a tag. + if ref != "" { + var err error + name, err = reference.ParseNormalizedNamed(ref) + if err != nil { + return nil, errors.Wrapf(err, "error parsing named reference %q", ref) + } + } + if name == nil && sum == "" && id == "" { + return nil, errors.Errorf("error parsing reference") + } + + // Construct a copy of the store spec. optionsList := "" options := store.GraphOptions() if len(options) > 0 { optionsList = ":" + strings.Join(options, ",") } storeSpec := "[" + store.GraphDriverName() + "@" + store.GraphRoot() + "+" + store.RunRoot() + optionsList + "]" - id := "" - if sum.Validate() == nil { - id = sum.Hex() - } + + // Convert the name back into a reference string, if we got a name. refname := "" + tag := "" if name != nil { - name = reference.TagNameOnly(name) - refname = verboseName(name) + if sum.Validate() == nil { + canonical, err := reference.WithDigest(name, sum) + if err != nil { + return nil, errors.Wrapf(err, "error mixing name %q with digest %q", name, sum) + } + refname = verboseName(canonical) + } else { + name = reference.TagNameOnly(name) + tagged, ok := name.(reference.Tagged) + if !ok { + return nil, errors.Errorf("error parsing possibly-tagless name %q", ref) + } + refname = verboseName(name) + tag = tagged.Tag() + } } if refname == "" { logrus.Debugf("parsed reference into %q", storeSpec+"@"+id) @@ -163,7 +204,7 @@ func (s storageTransport) ParseStoreReference(store storage.Store, ref string) ( } else { logrus.Debugf("parsed reference into %q", storeSpec+refname+"@"+id) } - return newReference(storageTransport{store: store, defaultUIDMap: s.defaultUIDMap, defaultGIDMap: s.defaultGIDMap}, refname, id, name), nil + return newReference(storageTransport{store: store, defaultUIDMap: s.defaultUIDMap, defaultGIDMap: s.defaultGIDMap}, refname, id, name, tag, sum), nil } func (s *storageTransport) GetStore() (storage.Store, error) { @@ -182,7 +223,8 @@ func (s *storageTransport) GetStore() (storage.Store, error) { return s.store, nil } -// ParseReference takes a name and/or an ID ("_name_"/"@_id_"/"_name_@_id_"), +// ParseReference takes a name and a tag or digest and/or ID +// ("_name_"/"@_id_"/"_name_:_tag_"/"_name_:_tag_@_id_"/"_name_@_digest_"/"_name_@_digest_@_id_"), // possibly prefixed with a store specifier in the form "[_graphroot_]" or // "[_driver_@_graphroot_]" or "[_driver_@_graphroot_+_runroot_]" or // "[_driver_@_graphroot_:_options_]" or "[_driver_@_graphroot_+_runroot_:_options_]", @@ -335,7 +377,7 @@ func (s storageTransport) ValidatePolicyConfigurationScope(scope string) error { if err != nil { return err } - _, err = ddigest.Parse("sha256:" + scopeInfo[1]) + _, err = digest.Parse("sha256:" + scopeInfo[1]) if err != nil { return err } @@ -345,11 +387,28 @@ func (s storageTransport) ValidatePolicyConfigurationScope(scope string) error { return nil } -func verboseName(name reference.Named) string { - name = reference.TagNameOnly(name) +func verboseName(r reference.Reference) string { + if r == nil { + return "" + } + named, isNamed := r.(reference.Named) + digested, isDigested := r.(reference.Digested) + tagged, isTagged := r.(reference.Tagged) + name := "" tag := "" - if tagged, ok := name.(reference.NamedTagged); ok { - tag = ":" + tagged.Tag() + sum := "" + if isNamed { + name = (reference.TrimNamed(named)).String() + } + if isTagged { + if tagged.Tag() != "" { + tag = ":" + tagged.Tag() + } + } + if isDigested { + if digested.Digest().Validate() == nil { + sum = "@" + digested.Digest().String() + } } - return name.Name() + tag + return name + tag + sum } diff --git a/storage/storage_transport_test.go b/storage/storage_transport_test.go index bcccfcf45d..c5a4a6db15 100644 --- a/storage/storage_transport_test.go +++ b/storage/storage_transport_test.go @@ -26,7 +26,7 @@ func TestTransportParseStoreReference(t *testing.T) { {"[garbage]busybox", "docker.io/library/busybox:latest", ""}, // Store specifier is overridden by the store we pass to ParseStoreReference {"UPPERCASEISINVALID", "", ""}, // Invalid single-component name - {"sha256:" + sha256digestHex, "docker.io/library/sha256:" + sha256digestHex, ""}, // Valid single-component name; the hex part is not an ID unless it has a "@" prefix + {"sha256:" + sha256digestHex, "docker.io/library/sha256:" + sha256digestHex, ""}, // Valid single-component name; the hex part is not an ID unless it has a "@" prefix, so it looks like a tag {sha256digestHex, "", ""}, // Invalid single-component ID; not an ID without a "@" prefix, so it's parsed as a name, but names aren't allowed to look like IDs {"@" + sha256digestHex, "", sha256digestHex}, // Valid single-component ID {"sha256:ab", "docker.io/library/sha256:ab", ""}, // Valid single-component name, explicit tag @@ -37,8 +37,7 @@ func TestTransportParseStoreReference(t *testing.T) { {"UPPERCASEISINVALID@" + sha256digestHex, "", ""}, // Invalid name in name@ID {"busybox@ab", "", ""}, // Invalid ID in name@ID {"busybox@", "", ""}, // Empty ID in name@ID - {"busybox@sha256:" + sha256digestHex, "docker.io/library/busybox:latest", sha256digestHex}, // Valid two-component name, with ID using "sha256:" prefix - {"@" + sha256digestHex, "", sha256digestHex}, // Valid two-component name, with ID only + {"busybox@sha256:" + sha256digestHex, "docker.io/library/busybox@sha256:" + sha256digestHex, ""}, // Valid two-component name, with a digest and no tag {"busybox@" + sha256digestHex, "docker.io/library/busybox:latest", sha256digestHex}, // Valid two-component name, implicit tag {"busybox:notlatest@" + sha256digestHex, "docker.io/library/busybox:notlatest", sha256digestHex}, // Valid two-component name, explicit tag {"docker.io/library/busybox:notlatest@" + sha256digestHex, "docker.io/library/busybox:notlatest", sha256digestHex}, // Valid two-component name, everything explicit @@ -57,7 +56,8 @@ func TestTransportParseStoreReference(t *testing.T) { dockerRef, err := reference.ParseNormalizedNamed(c.expectedRef) require.NoError(t, err) require.NotNil(t, storageRef.name, c.input) - assert.Equal(t, dockerRef.String(), storageRef.name.String()) + assert.Equal(t, dockerRef.String(), storageRef.reference) + assert.Equal(t, dockerRef.String(), storageRef.DockerReference().String()) } } }