Merge pull request #1505 from vrothberg/bye-bye-digest-dance

libimage: harden lookup by digest

libimage/filters.go

@@ -394,10 +394,12 @@ func filterID(value string) filterFunc {
 	}
 }
 
-// filterDigest creates an digest filter for matching the specified value.
+// filterDigest creates a digest filter for matching the specified value.
 func filterDigest(value string) filterFunc {
+	// TODO: return an error if value is not a digest
+	// if _, err := digest.Parse(value); err != nil {...}
 	return func(img *Image) (bool, error) {
-		return string(img.Digest()) == value, nil
+		return img.hasDigest(value), nil
 	}
 }
 

libimage/filters_test.go

@@ -63,6 +63,19 @@ func TestFilterReference(t *testing.T) {
 		{"busybox", 1},
 		{"alpine", 1},
 		{"alpine@" + alpine.Digest().String(), 1},
+		{"alpine:latest@" + alpine.Digest().String(), 1},
+		{"quay.io/libpod/alpine@" + alpine.Digest().String(), 1},
+		{"quay.io/libpod/alpine:latest@" + alpine.Digest().String(), 1},
+		// Make sure that tags are ignored
+		{"alpine:ignoreme@" + alpine.Digest().String(), 1},
+		{"alpine:123@" + alpine.Digest().String(), 1},
+		{"quay.io/libpod/alpine:hurz@" + alpine.Digest().String(), 1},
+		{"quay.io/libpod/alpine:456@" + alpine.Digest().String(), 1},
+		// Make sure that repo and digest must match
+		{"alpine:busyboxdigest@" + busybox.Digest().String(), 0},
+		{"alpine:busyboxdigest@" + busybox.Digest().String(), 0},
+		{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String(), 0},
+		{"quay.io/libpod/alpine:busyboxdigest@" + busybox.Digest().String(), 0},
 	} {
 		listOptions := &ListImagesOptions{
 			Filters: []string{"reference=" + test.filter},

libimage/image.go

@@ -144,6 +144,9 @@ func (i *Image) ID() string {
 // possibly many digests that we have stored for the image, so many
 // applications are better off using the entire list returned by Digests().
 func (i *Image) Digest() digest.Digest {
+	// TODO: we return the image digest or the one of the manifest list
+	// which can lead to issues depending on the callers' assumptions.
+	// Hence, deprecate in favor of Digest_s_.
 	return i.storageImage.Digest
 }
 
@@ -154,6 +157,18 @@ func (i *Image) Digests() []digest.Digest {
 	return i.storageImage.Digests
 }
 
+// hasDigest returns whether the specified value matches any digest of the
+// image.
+func (i *Image) hasDigest(value string) bool {
+	// TODO: change the argument to a typed digest.Digest
+	for _, d := range i.Digests() {
+		if string(d) == value {
+			return true
+		}
+	}
+	return false
+}
+
 // IsReadOnly returns whether the image is set read only.
 func (i *Image) IsReadOnly() bool {
 	return i.storageImage.ReadOnly
@@ -656,6 +671,8 @@ func (i *Image) NamedTaggedRepoTags() ([]reference.NamedTagged, error) {
 // NamedRepoTags returns the repotags associated with the image as a
 // slice of reference.Named.
 func (i *Image) NamedRepoTags() ([]reference.Named, error) {
+	// FIXME: the NamedRepoTags name is a bit misleading as it can return
+	// repo@digest values if that’s how an image was pulled.
 	var repoTags []reference.Named
 	for _, name := range i.Names() {
 		parsed, err := reference.Parse(name)
@@ -669,32 +686,37 @@ func (i *Image) NamedRepoTags() ([]reference.Named, error) {
 	return repoTags, nil
 }
 
-// inRepoTags looks for the specified name/tag pair in the image's repo tags.
-func (i *Image) inRepoTags(namedTagged reference.NamedTagged) (reference.Named, error) {
+// inRepoTags looks for the specified name/tag in the image's repo tags.  If
+// `ignoreTag` is set, only the repo must match and the tag is ignored.
+func (i *Image) inRepoTags(namedTagged reference.NamedTagged, ignoreTag bool) (reference.Named, error) {
 	repoTags, err := i.NamedRepoTags()
 	if err != nil {
 		return nil, err
 	}
 
-	pairs, err := ToNameTagPairs(repoTags)
-	if err != nil {
-		return nil, err
-	}
-
 	name := namedTagged.Name()
 	tag := namedTagged.Tag()
-	for _, pair := range pairs {
-		if tag != pair.Tag {
-			continue
+	for _, r := range repoTags {
+		if !ignoreTag {
+			var repoTag string
+			tagged, isTagged := r.(reference.NamedTagged)
+			if isTagged {
+				repoTag = tagged.Tag()
+			}
+			if !isTagged || tag != repoTag {
+				continue
+			}
 		}
-		if !strings.HasSuffix(pair.Name, name) {
+
+		repoName := r.Name()
+		if !strings.HasSuffix(repoName, name) {
 			continue
 		}
-		if len(pair.Name) == len(name) { // full match
-			return pair.named, nil
+		if len(repoName) == len(name) { // full match
+			return r, nil
 		}
-		if pair.Name[len(pair.Name)-len(name)-1] == '/' { // matches at repo
-			return pair.named, nil
+		if repoName[len(repoName)-len(name)-1] == '/' { // matches at repo
+			return r, nil
 		}
 	}
 

libimage/image_test.go

@@ -182,6 +182,64 @@ func TestImageFunctions(t *testing.T) {
 	require.Equal(t, image.NamesHistory(), imageData.NamesHistory, "inspect data should match")
 }
 
+func TestLookupImage(t *testing.T) {
+	alpineNoTag := "quay.io/libpod/alpine"
+	alpineLatest := alpineNoTag + ":latest"
+
+	runtime, cleanup := testNewRuntime(t)
+	defer cleanup()
+	ctx := context.Background()
+
+	pullOptions := &PullOptions{}
+	pullOptions.Writer = os.Stdout
+
+	pulledImages, err := runtime.Pull(ctx, alpineLatest, config.PullPolicyMissing, pullOptions)
+	require.NoError(t, err)
+	require.Len(t, pulledImages, 1)
+	alpine := pulledImages[0]
+
+	digestStr := alpine.Digest().String()
+	alpineDigest := alpineNoTag + "@" + digestStr
+
+	for _, test := range []struct {
+		input        string
+		expectedName string
+		mustFail     bool
+	}{
+		// Name only
+		{"alpine", alpineLatest, false},
+		{"alpine:latest", alpineLatest, false},
+		{"alpine:wrongtag", "", true},
+		{"alpine@" + digestStr, alpineDigest, false},
+		{"alpine:latest@" + digestStr, alpineDigest, false},   // Tag will be trimmed
+		{"alpine:wrongtag@" + digestStr, alpineDigest, false}, // Tag will be ignored and trimmed
+		// Repo + name
+		{"libpod/alpine", alpineLatest, false},
+		{"libpod/alpine:latest", alpineLatest, false},
+		{"libpod/alpine:wrongtag", "", true},
+		{"libpod/alpine@" + digestStr, alpineDigest, false},
+		{"libpod/alpine:latest@" + digestStr, alpineDigest, false},   // Tag will be trimmed
+		{"libpod/alpine:wrongtag@" + digestStr, alpineDigest, false}, // Tag will be ignored and trimmed
+		// Domain + repo + name
+		{alpineNoTag, alpineLatest, false},
+		{alpineLatest, alpineLatest, false},
+		{alpineNoTag + ":wrongtag", "", true},
+		{alpineDigest, alpineDigest, false},
+		{alpineNoTag + ":latest@" + digestStr, alpineDigest, false},   // Tag will be trimmed
+		{alpineNoTag + ":wrongtag@" + digestStr, alpineDigest, false}, // Tag will be ignored and trimmed
+	} {
+		resolvedImage, resolvedName, err := runtime.LookupImage(test.input, nil)
+		if test.mustFail {
+			require.Error(t, err)
+			continue
+		}
+		require.NoError(t, err)
+		require.NotNil(t, resolvedImage)
+		require.Equal(t, alpine.ID(), resolvedImage.ID())
+		require.Equal(t, test.expectedName, resolvedName, "input resolved to the expected name")
+	}
+}
+
 func TestInspectHealthcheck(t *testing.T) {
 	runtime, cleanup := testNewRuntime(t)
 	defer cleanup()

libimage/manifest_list.go

@@ -217,6 +217,11 @@ func (i *Image) getManifestList() (manifests.List, error) {
 // image index (OCI).  This information may be critical to make certain
 // execution paths more robust (e.g., suppress certain errors).
 func (i *Image) IsManifestList(ctx context.Context) (bool, error) {
+	// FIXME: due to `ImageDigestBigDataKey` we'll always check the
+	// _last-written_ manifest which is causing issues for multi-arch image
+	// pulls.
+	//
+	// See https://github.com/containers/common/pull/1505#discussion_r1242677279.
 	ref, err := i.StorageReference()
 	if err != nil {
 		return false, err

libimage/normalize.go

@@ -100,22 +100,22 @@ func ToNameTagPairs(repoTags []reference.Named) ([]NameTagPair, error) {
 // normalizeTaggedDigestedString strips the tag off the specified string iff it
 // is tagged and digested. Note that the tag is entirely ignored to match
 // Docker behavior.
-func normalizeTaggedDigestedString(s string) (string, error) {
+func normalizeTaggedDigestedString(s string) (string, reference.Named, error) {
 	// Note that the input string is not expected to be parseable, so we
 	// return it verbatim in error cases.
 	ref, err := reference.Parse(s)
 	if err != nil {
-		return "", err
+		return "", nil, err
 	}
 	named, ok := ref.(reference.Named)
 	if !ok {
-		return s, nil
+		return s, nil, nil
 	}
 	named, err = normalizeTaggedDigestedNamed(named)
 	if err != nil {
-		return "", err
+		return "", nil, err
 	}
-	return named.String(), nil
+	return named.String(), named, nil
 }
 
 // normalizeTaggedDigestedNamed strips the tag off the specified named

libimage/normalize_test.go

@@ -132,12 +132,14 @@ func TestNormalizeTaggedDigestedString(t *testing.T) {
 		{"localhost/fedora:anothertag" + digestSuffix, "localhost/fedora" + digestSuffix},
 		{"localhost:5000/fedora:v1.2.3.4.5" + digestSuffix, "localhost:5000/fedora" + digestSuffix},
 	} {
-		res, err := normalizeTaggedDigestedString(test.input)
+		res, named, err := normalizeTaggedDigestedString(test.input)
 		if test.expected == "" {
 			assert.Error(t, err, "%v", test)
 		} else {
 			assert.NoError(t, err, "%v", test)
 			assert.Equal(t, test.expected, res, "%v", test)
+			assert.NotNil(t, named, "%v", test)
+			assert.Equal(t, res, named.String(), "%v", test)
 		}
 	}
 }

libimage/pull.go

@@ -86,7 +86,7 @@ func (r *Runtime) Pull(ctx context.Context, name string, pullPolicy config.PullP
 		// Docker compat: strip off the tag iff name is tagged and digested
 		// (e.g., fedora:latest@sha256...).  In that case, the tag is stripped
 		// off and entirely ignored.  The digest is the sole source of truth.
-		normalizedName, normalizeError := normalizeTaggedDigestedString(name)
+		normalizedName, _, normalizeError := normalizeTaggedDigestedString(name)
 		if normalizeError != nil {
 			return nil, normalizeError
 		}