libimage: harden lookup by digest

When looking up an image by digest, make sure that the entire repository
of the specified value is considered.  Previously, both the repository
and the tag have been ignored and we looked for _some_ image with a
matching digest.

As outlined in containers#1248, Docker stopped ignoring the repository with
version v20.10.20 (Oct '22) which is a compelling reason to do the same.

To be clear, previously `something@digest` would look for any image with
`digest` while `something` is entirely ignored.  With this change, both
`something` and `digest` must match the image.

This change breaks two e2e tests in Podman CI which relied on the
previous behavior.  There is a risk of breaking users but there is a
strong security argument to perform this change:  if the repository does
not match the (previously) returned issue, there is a fair chance of a
user error.

Fixes: containers#1248
Signed-off-by: Valentin Rothberg <[email protected]>

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
 		}