From ccf06edaef984dba2f9eb71c05103d15eec0fa3f 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 | 6 +- libimage/filters_test.go | 3 + libimage/image.go | 15 +++++ libimage/normalize.go | 10 +-- libimage/normalize_test.go | 4 +- libimage/pull.go | 2 +- libimage/runtime.go | 131 ++++++++++++++++++++----------------- 7 files changed, 102 insertions(+), 69 deletions(-) diff --git a/libimage/filters.go b/libimage/filters.go index ff50321b7..995f89c78 100644 --- a/libimage/filters.go +++ b/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 } } diff --git a/libimage/filters_test.go b/libimage/filters_test.go index 12091b130..94e513596 100644 --- a/libimage/filters_test.go +++ b/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}, diff --git a/libimage/image.go b/libimage/image.go index da4ff8b7a..d8ffb2e1a 100644 --- a/libimage/image.go +++ b/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 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..9b9dfb464 100644 --- a/libimage/normalize_test.go +++ b/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) } } } 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..0d39d77ff 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, 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,7 +308,7 @@ 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 } @@ -316,21 +316,45 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image, 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 } @@ -352,6 +376,9 @@ func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *Loo } if options.lookupManifest || options.ManifestList { if isManifestList { + if _, err := image.ToManifestList(); err != nil { + return nil, err + } return image, nil } // return ErrNotAManifestList if lookupManifest is set otherwise try resolving image. @@ -414,59 +441,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) } - 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) + 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) + } + + allImages, err := r.ListImages(context.Background(), nil, nil) + if err != nil { + return nil, "", err } for _, image := range allImages { @@ -477,16 +485,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