From dabd8aa5d7ede1da26563c69762822d116918be7 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 16 Jun 2023 09:07:12 +0200 Subject: [PATCH] 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 #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: #1248 Signed-off-by: Valentin Rothberg --- libimage/filters.go | 7 +++- libimage/filters_test.go | 3 +- libimage/normalize.go | 10 +++--- libimage/normalize_test.go | 2 +- libimage/pull.go | 2 +- libimage/runtime.go | 74 +++++++++++++++++--------------------- 6 files changed, 47 insertions(+), 51 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index ff50321b7..202cb5514 100644 --- a/libimage/filters.go +++ b/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 } } diff --git a/libimage/filters_test.go b/libimage/filters_test.go index 12091b130..13773018d 100644 --- a/libimage/filters_test.go +++ b/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}, diff --git a/libimage/normalize.go b/libimage/normalize.go index bb3cdbc7c..9619b1a0d 100644 --- a/libimage/normalize.go +++ b/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 diff --git a/libimage/normalize_test.go b/libimage/normalize_test.go index 206c0eb35..3e2049634 100644 --- a/libimage/normalize_test.go +++ b/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 { diff --git a/libimage/pull.go b/libimage/pull.go index 188ecb5ef..296d003a8 100644 --- a/libimage/pull.go +++ b/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 } diff --git a/libimage/runtime.go b/libimage/runtime.go index 95da83bb9..d6b254488 100644 --- a/libimage/runtime.go +++ b/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,46 +431,14 @@ 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) } @@ -461,14 +446,19 @@ func (r *Runtime) lookupImageInDigestsAndRepoTags(name string, options *LookupIm 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 {