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

@@ -397,7 +397,12 @@ func filterID(value string) filterFunc {
 // filterDigest creates an digest filter for matching the specified value.
 func filterDigest(value string) filterFunc {
 	return func(img *Image) (bool, error) {
-		return string(img.Digest()) == value, nil
+		for _, d := range img.Digests() {
+			if string(d) == value {
+				return true, nil
+			}
+		}
+		return false, nil
 	}
 }
 

libimage/filters_test.go

@@ -62,7 +62,8 @@ func TestFilterReference(t *testing.T) {
 		{"quay.io/libpod/*", 2},
 		{"busybox", 1},
 		{"alpine", 1},
-		{"alpine@" + alpine.Digest().String(), 1},
+		{"quay.io/libpod/alpine@" + alpine.Digest().String(), 1},
+		{"quay.io/libpod/alpine:latest@" + alpine.Digest().String(), 1},
 	} {
 		listOptions := &ListImagesOptions{
 			Filters: []string{"reference=" + test.filter},

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,7 +132,7 @@ 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, _, err := normalizeTaggedDigestedString(test.input)
 		if test.expected == "" {
 			assert.Error(t, err, "%v", test)
 		} else {

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
 		}

libimage/runtime.go

@@ -239,7 +239,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 	// 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, err := normalizeTaggedDigestedString(name)
+	normalizedName, namedName, err := normalizeTaggedDigestedString(name)
 	if err != nil {
 		return nil, "", err
 	}
@@ -316,19 +316,36 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 		return img, name, err
 	}
 
-	return r.lookupImageInDigestsAndRepoTags(name, options)
+	return r.lookupImageInRepoTags(name, namedName, options)
 }
 
 // lookupImageInLocalStorage looks up the specified candidate for name in the
 // storage and checks whether it's matching the system context.
 func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *LookupImageOptions) (*Image, error) {
 	logrus.Debugf("Trying %q ...", candidate)
+
+	// First, try store.Image() which will properly work on IDs (but not on
+	// digests).
 	img, err := r.store.Image(candidate)
 	if err != nil && !errors.Is(err, storage.ErrImageUnknown) {
 		return nil, err
 	}
 	if img == nil {
-		return nil, nil
+		// Second, try parsing the candidate into a storage reference
+		// which will work with digests.  However, we must reparse the
+		// reference another time below since an ordinary image may
+		// have been referenced via its parent (manifest list) digest.
+		ref, err := storageTransport.Transport.ParseStoreReference(r.store, candidate)
+		if err != nil {
+			return nil, nil
+		}
+		img, err = storageTransport.Transport.GetStoreImage(r.store, ref)
+		if err != nil && !errors.Is(err, storage.ErrImageUnknown) {
+			return nil, err
+		}
+		if img == nil {
+			return nil, nil
+		}
 	}
 	ref, err := storageTransport.Transport.ParseStoreReference(r.store, img.ID)
 	if err != nil {
@@ -414,61 +431,34 @@ func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *Loo
 	return image, nil
 }
 
-// lookupImageInDigestsAndRepoTags attempts to match name against any image in
-// the local containers storage.  If name is digested, it will be compared
-// against image digests.  Otherwise, it will be looked up in the repo tags.
-func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, options *LookupImageOptions) (*Image, string, error) {
-	// Until now, we've tried very hard to find an image but now it is time
-	// for limbo.  If the image includes a digest that we couldn't detect
-	// verbatim in the storage, we must have a look at all digests of all
-	// images.  Those may change over time (e.g., via manifest lists).
-	// Both Podman and Buildah want us to do that dance.
-	allImages, err := r.ListImages(context.Background(), nil, nil)
-	if err != nil {
-		return nil, "", err
-	}
-
-	ref, err := reference.Parse(name) // Warning! This is not ParseNormalizedNamed
-	if err != nil {
-		return nil, "", err
-	}
-	named, isNamed := ref.(reference.Named)
-	if !isNamed {
+// lookupImageInRepoTags attempts to match named against any image in the local
+// containers storage by comparing the name to all known repo tags.
+func (r *Runtime) lookupImageInRepoTags(name string, namedName reference.Named, options *LookupImageOptions) (*Image, string, error) {
+	if namedName == nil {
 		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
 	}
 
-	digested, isDigested := named.(reference.Digested)
-	if isDigested {
-		logrus.Debug("Looking for image with matching recorded digests")
-		digest := digested.Digest()
-		for _, image := range allImages {
-			for _, d := range image.Digests() {
-				if d != digest {
-					continue
-				}
-				// Also make sure that the matching image fits all criteria (e.g., manifest list).
-				if _, err := r.lookupImageInLocalStorage(name, image.ID(), options); err != nil {
-					return nil, "", err
-				}
-				return image, name, nil
-
-			}
-		}
+	if _, isDigested := namedName.(reference.Digested); isDigested {
 		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
 	}
 
 	if !shortnames.IsShortName(name) {
 		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
 	}
 
-	named = reference.TagNameOnly(named) // Make sure to add ":latest" if needed
-	namedTagged, isNammedTagged := named.(reference.NamedTagged)
+	namedName = reference.TagNameOnly(namedName) // Docker compat: make sure to add ":latest" if needed
+	namedTagged, isNammedTagged := namedName.(reference.NamedTagged)
 	if !isNammedTagged {
 		// NOTE: this should never happen since we already know it's
 		// not a digested reference.
 		return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", name, storage.ErrImageUnknown)
 	}
 
+	allImages, err := r.ListImages(context.Background(), nil, nil)
+	if err != nil {
+		return nil, "", err
+	}
+
 	for _, image := range allImages {
 		named, err := image.inRepoTags(namedTagged)
 		if err != nil {