From ffc7d8e68f1fbd091dabc796cd2d2240637d0a1a 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 | 13 ++++ libimage/image.go | 52 ++++++++++---- libimage/image_test.go | 58 +++++++++++++++ libimage/manifest_list.go | 5 ++ libimage/normalize.go | 10 +-- libimage/normalize_test.go | 4 +- libimage/pull.go | 2 +- libimage/runtime.go | 142 ++++++++++++++++++++++--------------- 9 files changed, 210 insertions(+), 82 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..6315c5a8b 100644 --- a/libimage/filters_test.go +++ b/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}, diff --git a/libimage/image.go b/libimage/image.go index da4ff8b7a..86c1c08d7 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 @@ -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 } } diff --git a/libimage/image_test.go b/libimage/image_test.go index c22cec6bc..bc671684e 100644 --- a/libimage/image_test.go +++ b/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() diff --git a/libimage/manifest_list.go b/libimage/manifest_list.go index 3a75709e0..0223fb355 100644 --- a/libimage/manifest_list.go +++ b/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 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..7707d2e3b 100644 --- a/libimage/runtime.go +++ b/libimage/runtime.go @@ -16,6 +16,7 @@ import ( "github.com/containers/storage" deepcopy "github.com/jinzhu/copier" jsoniter "github.com/json-iterator/go" + "github.com/opencontainers/go-digest" "github.com/sirupsen/logrus" ) @@ -239,7 +240,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 +260,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 +298,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 +309,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 +317,51 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image, return img, name, err } - return r.lookupImageInDigestsAndRepoTags(name, options) + return r.lookupImageInDigestsAndRepoTags(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 + + // FIXME: the lookup logic for manifest lists needs improvement. + // See https://github.com/containers/common/pull/1505#discussion_r1242677279 + // for details. + + // For images pulled by tag, Image.Names does not currently contain a + // repo@digest value, so such an input would not match directly in + // c/storage. + if namedCandidate != nil { + namedCandidate = reference.TagNameOnly(namedCandidate) + 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 resolved into a per-platform image + // without any regard to options.{Architecture,OS,Variant}. + } 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 } @@ -417,76 +448,71 @@ func (r *Runtime) lookupImageInLocalStorage(name, candidate string, options *Loo // 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 - } +func (r *Runtime) lookupImageInDigestsAndRepoTags(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 requiredDigest digest.Digest + 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) + requiredDigest = digested.Digest() + 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) + // Docker compat: make sure to add the "latest" tag if needed. The tag + // will be ignored if we're looking for a digest match. + possiblyUnqualifiedNamedReference = reference.TagNameOnly(possiblyUnqualifiedNamedReference) + 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 { - named, err := image.inRepoTags(namedTagged) + named, err := image.inRepoTags(namedTagged, isDigested) if err != nil { return nil, "", err } 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 { + if !img.hasDigest(requiredDigest.String()) { + continue + } + named = reference.TrimNamed(named) + canonical, err := reference.WithDigest(named, requiredDigest) + if err != nil { + return nil, "", fmt.Errorf("building canonical reference with digest %q and matched %q: %w", requiredDigest.String(), named.String(), err) + } + return img, canonical.String(), nil + } + 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