libimage: image lookup: fix ID vs short name

When looking up an image by a short name that prefixes another image's
ID, the one matching the short name should be returned.

This means that we need to do a final lookup in the storage with the
specified name (without normalization) to continue matching short IDs.

Since it's common that users of libimage (e.g., Buildah) internally
refer to images by full ID, let's make sure that we check for that
first.  This way, we'll match full IDs on first lookup and keep the
expected performance.

Note that a name starting with `sha2556:` must be followed by a 64-byte
hex value; something we didn't check for before.

Fixes: containers/podman/issues/12761
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

common/libimage/image_test.go

@@ -22,6 +22,11 @@ func TestImageFunctions(t *testing.T) {
 	defer cleanup()
 	ctx := context.Background()
 
+	// Looking up image by invalid sha.
+	_, _, err := runtime.LookupImage("sha256:aa", nil)
+	require.Error(t, err, "invalid hex value")
+	require.Contains(t, err.Error(), errNoHexValue.Error())
+
 	// Pull busybox:latest, get its digest and then perform a digest pull.
 	// It's effectively pulling the same image twice but we need the digest
 	// for some of the tests below.

common/libimage/pull_test.go

@@ -187,3 +187,37 @@ func TestPullPolicy(t *testing.T) {
 	require.NotNil(t, pulledImages, "lookup alpine")
 
 }
+
+func TestShortNameAndIDconflict(t *testing.T) {
+	// Regression test for https://github.com/containers/podman/issues/12761
+	runtime, cleanup := testNewRuntime(t)
+	defer cleanup()
+	ctx := context.Background()
+	pullOptions := &PullOptions{}
+	pullOptions.Writer = os.Stdout
+
+	busybox, err := runtime.Pull(ctx, "busybox", config.PullPolicyAlways, pullOptions)
+	require.NoError(t, err)
+	require.Len(t, busybox, 1)
+
+	alpine, err := runtime.Pull(ctx, "alpine", config.PullPolicyAlways, pullOptions)
+	require.NoError(t, err)
+	require.Len(t, alpine, 1)
+
+	// Tag the alpine image with the first character of busybox's ID to
+	// cause a conflict when looking up the image. The expected outcome is
+	// that short names always have precedence of IDs.
+	c := busybox[0].ID()[0:1]
+	err = alpine[0].Tag(c)
+	require.NoError(t, err, "tag")
+
+	// Short name is selected over ID.
+	img, _, err := runtime.LookupImage(c, nil)
+	require.NoError(t, err)
+	require.Equal(t, alpine[0].ID(), img.ID())
+
+	// Not matching short name, so ID is selected.
+	img, _, err = runtime.LookupImage(busybox[0].ID()[0:2], nil)
+	require.NoError(t, err)
+	require.Equal(t, busybox[0].ID(), img.ID())
+}

common/libimage/runtime.go

@@ -190,6 +190,8 @@ type LookupImageOptions struct {
 	returnManifestIfNoInstance bool
 }
 
+var errNoHexValue = errors.New("invalid format: no 64-byte hexadecimal value")
+
 // Lookup Image looks up `name` in the local container storage.  Returns the
 // image and the name it has been found with.  Note that name may also use the
 // `containers-storage:` prefix used to refer to the containers-storage
@@ -233,13 +235,29 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 		name = normalizedName
 	}
 
+	byDigest := false
 	originalName := name
-	idByDigest := false
 	if strings.HasPrefix(name, "sha256:") {
-		// Strip off the sha256 prefix so it can be parsed later on.
-		idByDigest = true
+		byDigest = true
 		name = strings.TrimPrefix(name, "sha256:")
 	}
+	byFullID := reference.IsFullIdentifier(name)
+
+	if byDigest && !byFullID {
+		return nil, "", fmt.Errorf("%s: %v", originalName, errNoHexValue)
+	}
+
+	// If the name clearly refers to a local image, try to look it up.
+	if byFullID || byDigest {
+		img, err := r.lookupImageInLocalStorage(originalName, name, options)
+		if err != nil {
+			return nil, "", err
+		}
+		if img != nil {
+			return img, originalName, nil
+		}
+		return nil, "", errors.Wrap(storage.ErrImageUnknown, originalName)
+	}
 
 	// Unless specified, set the platform specified in the system context
 	// for later platform matching.  Builder likes to set these things via
@@ -256,27 +274,11 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 	// Normalize platform to be OCI compatible (e.g., "aarch64" -> "arm64").
 	options.OS, options.Architecture, options.Variant = NormalizePlatform(options.OS, options.Architecture, options.Variant)
 
-	// First, check if we have an exact match in the storage. Maybe an ID
-	// or a fully-qualified image name.
-	img, err := r.lookupImageInLocalStorage(name, name, options)
-	if err != nil {
-		return nil, "", err
-	}
-	if img != nil {
-		return img, originalName, nil
-	}
-
-	// If the name clearly referred to a local image, there's nothing we can
-	// do anymore.
-	if storageRef != nil || idByDigest {
-		return nil, "", errors.Wrap(storage.ErrImageUnknown, originalName)
-	}
-
 	// Second, try out the candidates as resolved by shortnames. This takes
 	// "localhost/" prefixed images into account as well.
 	candidates, err := shortnames.ResolveLocally(&r.systemContext, name)
 	if err != nil {
-		return nil, "", errors.Wrap(storage.ErrImageUnknown, originalName)
+		return nil, "", errors.Wrap(storage.ErrImageUnknown, name)
 	}
 	// Backwards compat: normalize to docker.io as some users may very well
 	// rely on that.
@@ -294,7 +296,17 @@ func (r *Runtime) LookupImage(name string, options *LookupImageOptions) (*Image,
 		}
 	}
 
-	return r.lookupImageInDigestsAndRepoTags(originalName, options)
+	// 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)
+	if err != nil {
+		return nil, "", err
+	}
+	if img != nil {
+		return img, name, err
+	}
+
+	return r.lookupImageInDigestsAndRepoTags(name, options)
 }
 
 // lookupImageInLocalStorage looks up the specified candidate for name in the