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,9 @@ 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},
 	} {
 		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

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
 		}

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, possiblyUnqualifiedNamedReference, err := normalizeTaggedDigestedString(name)
 	if err != nil {
 		return nil, "", err
 	}
@@ -259,7 +259,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 
 	// If the name clearly refers to a local image, try to look it up.
 	if byFullID || byDigest {
-		img, err := r.lookupImageInLocalStorage(originalName, name, options)
+		img, err := r.lookupImageInLocalStorage(originalName, name, nil, options)
 		if err != nil {
 			return nil, "", err
 		}
@@ -297,7 +297,7 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 	}
 
 	for _, candidate := range candidates {
-		img, err := r.lookupImageInLocalStorage(name, candidate.String(), options)
+		img, err := r.lookupImageInLocalStorage(name, candidate.String(), candidate, options)
 		if err != nil {
 			return nil, "", err
 		}
@@ -308,29 +308,53 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 
 	// The specified name may refer to a short ID. Note that this *must*
 	// happen after the short-name expansion as done above.
-	img, err := r.lookupImageInLocalStorage(name, name, options)
+	img, err := r.lookupImageInLocalStorage(name, name, nil, options)
 	if err != nil {
 		return nil, "", err
 	}
 	if img != nil {
 		return img, name, err
 	}
 
-	return r.lookupImageInDigestsAndRepoTags(name, options)
+	return r.lookupImageInRepoTags(name, possiblyUnqualifiedNamedReference, 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) {
+func (r *Runtime) lookupImageInLocalStorage(name, candidate string, namedCandidate reference.Named, options *LookupImageOptions) (*Image, error) {
 	logrus.Debugf("Trying %q ...", candidate)
-	img, err := r.store.Image(candidate)
-	if err != nil && !errors.Is(err, storage.ErrImageUnknown) {
-		return nil, err
-	}
-	if img == nil {
-		return nil, nil
+
+	var err error
+	var img *storage.Image
+	var ref types.ImageReference
+
+	// for images pulled by tag, Image.Names does not currently contain a
+	// repo@digest value, so such an input would not match.
+	if namedCandidate != nil {
+		ref, err = storageTransport.Transport.NewStoreReference(r.store, namedCandidate, "")
+		if err != nil {
+			return nil, err
+		}
+		img, err = storageTransport.Transport.GetStoreImage(r.store, ref)
+		if err != nil {
+			if errors.Is(err, storage.ErrImageUnknown) {
+				return nil, nil
+			}
+			return nil, err
+		}
+		// NOTE: we must reparse the reference another time below since
+		// an ordinary image may have been referenced via its parent
+		// (manifest list) digest.
+	} else {
+		img, err = r.store.Image(candidate)
+		if err != nil {
+			if errors.Is(err, storage.ErrImageUnknown) {
+				return nil, nil
+			}
+			return nil, err
+		}
 	}
-	ref, err := storageTransport.Transport.ParseStoreReference(r.store, img.ID)
+	ref, err = storageTransport.Transport.ParseStoreReference(r.store, img.ID)
 	if err != nil {
 		return nil, err
 	}
@@ -414,59 +438,40 @@ 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
-	}
+// 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, possiblyUnqualifiedNamedReference reference.Named, options *LookupImageOptions) (*Image, string, error) {
+	originalName := name // we may change name below
 
-	ref, err := reference.Parse(name) // Warning! This is not ParseNormalizedNamed
-	if err != nil {
-		return nil, "", err
-	}
-	named, isNamed := ref.(reference.Named)
-	if !isNamed {
-		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
+	if possiblyUnqualifiedNamedReference == nil {
+		return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown)
 	}
 
-	digested, isDigested := named.(reference.Digested)
+	// In case of a digested reference, we strip off the digest and require
+	// any image matching the repo/tag to also match the specified digest.
+	var digestStr string
+	digested, isDigested := possiblyUnqualifiedNamedReference.(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
-
-			}
-		}
-		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
+		digestStr = digested.Digest().String()
+		possiblyUnqualifiedNamedReference = reference.TrimNamed(possiblyUnqualifiedNamedReference)
+		name = possiblyUnqualifiedNamedReference.String()
 	}
 
 	if !shortnames.IsShortName(name) {
-		return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
+		return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown)
+	}
+
+	possiblyUnqualifiedNamedReference = reference.TagNameOnly(possiblyUnqualifiedNamedReference) // Docker compat: make sure to add ":latest" if needed
+	namedTagged, isNamedTagged := possiblyUnqualifiedNamedReference.(reference.NamedTagged)
+	if !isNamedTagged {
+		// NOTE: this should never happen since we already stripped off
+		// the digest.
+		return nil, "", fmt.Errorf("%s: %w (could not cast to tagged)", originalName, storage.ErrImageUnknown)
 	}
 
-	named = reference.TagNameOnly(named) // Make sure to add ":latest" if needed
-	namedTagged, isNammedTagged := named.(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 {
@@ -477,16 +482,19 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, options *LookupIm
 		if named == nil {
 			continue
 		}
-		img, err := r.lookupImageInLocalStorage(name, named.String(), options)
+		img, err := r.lookupImageInLocalStorage(name, named.String(), named, options)
 		if err != nil {
 			return nil, "", err
 		}
 		if img != nil {
-			return img, named.String(), err
+			if isDigested && !img.hasDigest(digestStr) {
+				continue
+			}
+			return img, named.String(), nil
 		}
 	}
 
-	return nil, "", fmt.Errorf("%s: %w", name, storage.ErrImageUnknown)
+	return nil, "", fmt.Errorf("%s: %w", originalName, storage.ErrImageUnknown)
 }
 
 // ResolveName resolves the specified name.  If the name resolves to a local