Merge pull request #15933 from vrothberg/fix-15879

auto-update: validate container image

6 files changed, 67 insertions, 91 deletions

diff --git a/libpod/container_validate.go b/libpod/container_validate.go
index f4611ecce..7224ec7db 100644
--- a/libpod/container_validate.go
+++ b/libpod/container_validate.go
@@ -3,6 +3,9 @@ package libpod
 import (
 	"fmt"
 
+	"github.com/containers/image/v5/docker"
+	"github.com/containers/image/v5/pkg/shortnames"
+	"github.com/containers/image/v5/transports/alltransports"
 	"github.com/containers/podman/v4/libpod/define"
 	spec "github.com/opencontainers/runtime-spec/specs-go"
 )
@@ -141,5 +144,35 @@ func (c *Container) validate() error {
 	if c.config.HealthCheckOnFailureAction != define.HealthCheckOnFailureActionNone && c.config.HealthCheckConfig == nil {
 		return fmt.Errorf("cannot set on-failure action to %s without a health check", c.config.HealthCheckOnFailureAction.String())
 	}
+
+	if value, exists := c.config.Labels[define.AutoUpdateLabel]; exists {
+		// TODO: we cannot reference pkg/autoupdate here due to
+		// circular dependencies.  It's worth considering moving the
+		// auto-update logic into the libpod package.
+		if value == "registry" || value == "image" {
+			if err := validateAutoUpdateImageReference(c.config.RawImageName); err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
+// validateAutoUpdateImageReference checks if the specified imageName is a
+// fully-qualified image reference to the docker transport. Such a reference
+// includes a domain, name and tag (e.g., quay.io/podman/stable:latest).  The
+// reference may also be prefixed with "docker://" explicitly indicating that
+// it's a reference to the docker transport.
+func validateAutoUpdateImageReference(imageName string) error {
+	// Make sure the input image is a docker.
+	imageRef, err := alltransports.ParseImageName(imageName)
+	if err == nil && imageRef.Transport().Name() != docker.Transport.Name() {
+		return fmt.Errorf("auto updates require the docker image transport but image is of transport %q", imageRef.Transport().Name())
+	} else if err != nil {
+		if shortnames.IsShortName(imageName) {
+			return fmt.Errorf("short name: auto updates require fully-qualified image reference: %q", imageName)
+		}
+	}
 	return nil
 }
diff --git a/libpod/define/autoupdate.go b/libpod/define/autoupdate.go
new file mode 100644
index 000000000..7c278c3c5
--- /dev/null
+++ b/libpod/define/autoupdate.go
@@ -0,0 +1,9 @@
+package define
+
+// AutoUpdateLabel denotes the container/pod label key to specify auto-update
+// policies in container labels.
+const AutoUpdateLabel = "io.containers.autoupdate"
+
+// AutoUpdateAuthfileLabel denotes the container label key to specify authfile
+// in container labels.
+const AutoUpdateAuthfileLabel = "io.containers.autoupdate.authfile"
diff --git a/pkg/autoupdate/autoupdate.go b/pkg/autoupdate/autoupdate.go
index 9cf77d135..a0ed8ccba 100644
--- a/pkg/autoupdate/autoupdate.go
+++ b/pkg/autoupdate/autoupdate.go
@@ -9,8 +9,6 @@ import (
 	"github.com/containers/common/libimage"
 	"github.com/containers/common/pkg/config"
 	"github.com/containers/image/v5/docker"
-	"github.com/containers/image/v5/docker/reference"
-	"github.com/containers/image/v5/transports/alltransports"
 	"github.com/containers/podman/v4/libpod"
 	"github.com/containers/podman/v4/libpod/define"
 	"github.com/containers/podman/v4/libpod/events"
@@ -21,14 +19,6 @@ import (
 	"github.com/sirupsen/logrus"
 )
 
-// Label denotes the container/pod label key to specify auto-update policies in
-// container labels.
-const Label = "io.containers.autoupdate"
-
-// Label denotes the container label key to specify authfile in
-// container labels.
-const AuthfileLabel = "io.containers.autoupdate.authfile"
-
 // Policy represents an auto-update policy.
 type Policy string
 
@@ -102,32 +92,7 @@ func LookupPolicy(s string) (Policy, error) {
 	return "", fmt.Errorf("invalid auto-update policy %q: valid policies are %+q", s, keys)
 }
 
-// ValidateImageReference checks if the specified imageName is a fully-qualified
-// image reference to the docker transport (without digest).  Such a reference
-// includes a domain, name and tag (e.g., quay.io/podman/stable:latest).  The
-// reference may also be prefixed with "docker://" explicitly indicating that
-// it's a reference to the docker transport.
-func ValidateImageReference(imageName string) error {
-	// Make sure the input image is a docker.
-	imageRef, err := alltransports.ParseImageName(imageName)
-	if err == nil && imageRef.Transport().Name() != docker.Transport.Name() {
-		return fmt.Errorf("auto updates require the docker image transport but image is of transport %q", imageRef.Transport().Name())
-	} else if err != nil {
-		repo, err := reference.Parse(imageName)
-		if err != nil {
-			return fmt.Errorf("enforcing fully-qualified docker transport reference for auto updates: %w", err)
-		}
-		if _, ok := repo.(reference.NamedTagged); !ok {
-			return fmt.Errorf("auto updates require fully-qualified image references (no tag): %q", imageName)
-		}
-		if _, ok := repo.(reference.Digested); ok {
-			return fmt.Errorf("auto updates require fully-qualified image references without digest: %q", imageName)
-		}
-	}
-	return nil
-}
-
-// AutoUpdate looks up containers with a specified auto-update policy and acts
+/// AutoUpdate looks up containers with a specified auto-update policy and acts
 // accordingly.
 //
 // If the policy is set to PolicyRegistryImage, it checks if the image
@@ -418,7 +383,7 @@ func (u *updater) assembleTasks(ctx context.Context) []error {
 		// Check the container's auto-update policy which is configured
 		// as a label.
 		labels := ctr.Labels()
-		value, exists := labels[Label]
+		value, exists := labels[define.AutoUpdateLabel]
 		if !exists {
 			continue
 		}
@@ -454,7 +419,7 @@ func (u *updater) assembleTasks(ctx context.Context) []error {
 		}
 
 		t := task{
-			authfile:     labels[AuthfileLabel],
+			authfile:     labels[define.AutoUpdateAuthfileLabel],
 			auto:         u,
 			container:    ctr,
 			policy:       policy,
diff --git a/pkg/autoupdate/autoupdate_test.go b/pkg/autoupdate/autoupdate_test.go
deleted file mode 100644
index 7a5da5bb0..000000000
--- a/pkg/autoupdate/autoupdate_test.go
+++ /dev/null
@@ -1,50 +0,0 @@
-package autoupdate
-
-import (
-	"testing"
-)
-
-func TestValidateImageReference(t *testing.T) {
-	tests := []struct {
-		input string
-		valid bool
-	}{
-		{ // Fully-qualified reference
-			input: "quay.io/foo/bar:tag",
-			valid: true,
-		},
-		{ // Fully-qualified reference in transport notation
-			input: "docker://quay.io/foo/bar:tag",
-			valid: true,
-		},
-		{ // Fully-qualified reference but with digest
-			input: "quay.io/foo/bar@sha256:c9b1b535fdd91a9855fb7f82348177e5f019329a58c53c47272962dd60f71fc9",
-			valid: false,
-		},
-		{ // Reference with missing tag
-			input: "quay.io/foo/bar",
-			valid: false,
-		},
-		{ // Short name
-			input: "alpine",
-			valid: false,
-		},
-		{ // Short name with repo
-			input: "library/alpine",
-			valid: false,
-		},
-		{ // Wrong transport
-			input: "docker-archive:/some/path.tar",
-			valid: false,
-		},
-	}
-
-	for _, test := range tests {
-		err := ValidateImageReference(test.input)
-		if test.valid && err != nil {
-			t.Fatalf("parsing %q should have succeeded: %v", test.input, err)
-		} else if !test.valid && err == nil {
-			t.Fatalf("parsing %q should have failed", test.input)
-		}
-	}
-}
diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go
index bd9117f72..4a83cb464 100644
--- a/pkg/domain/infra/abi/play.go
+++ b/pkg/domain/infra/abi/play.go
@@ -20,7 +20,6 @@ import (
 	"github.com/containers/image/v5/types"
 	"github.com/containers/podman/v4/libpod"
 	"github.com/containers/podman/v4/libpod/define"
-	"github.com/containers/podman/v4/pkg/autoupdate"
 	"github.com/containers/podman/v4/pkg/domain/entities"
 	v1apps "github.com/containers/podman/v4/pkg/k8s.io/api/apps/v1"
 	v1 "github.com/containers/podman/v4/pkg/k8s.io/api/core/v1"
@@ -816,8 +815,8 @@ func (ic *ContainerEngine) getImageAndLabelInfo(ctx context.Context, cwd string,
 		}
 	}
 
-	setLabel(autoupdate.Label)
-	setLabel(autoupdate.AuthfileLabel)
+	setLabel(define.AutoUpdateLabel)
+	setLabel(define.AutoUpdateAuthfileLabel)
 
 	return pulledImage, labels, nil
 }
diff --git a/test/system/255-auto-update.bats b/test/system/255-auto-update.bats
index 76f6b02e8..6f5113779 100644
--- a/test/system/255-auto-update.bats
+++ b/test/system/255-auto-update.bats
@@ -133,6 +133,26 @@ function _confirm_update() {
     die "Timed out waiting for $cname to update; old IID=$old_iid"
 }
 
+@test "podman auto-update - validate input" {
+    # Fully-qualified image reference is required
+    run_podman create --label io.containers.autoupdate=registry $IMAGE
+    run_podman rm -f "$output"
+
+    # Short name does not work
+    shortname="shortname:latest"
+    run_podman image tag $IMAGE $shortname
+    run_podman 125 create --label io.containers.autoupdate=registry $shortname
+    is "$output" "Error: short name: auto updates require fully-qualified image reference: \"$shortname\""
+
+    # Requires docker (or no) transport
+    archive=$PODMAN_TMPDIR/archive.tar
+    run_podman save -o $archive $IMAGE
+    run_podman 125 create --label io.containers.autoupdate=registry docker-archive:$archive
+    is "$output" ".*Error: auto updates require the docker image transport but image is of transport \"docker-archive\""
+
+    run_podman rmi $shortname
+}
+
 # This test can fail in dev. environment because of SELinux.
 # quick fix: chcon -t container_runtime_exec_t ./bin/podman
 @test "podman auto-update - label io.containers.autoupdate=image" {