Merge 21bf7d8592 into a477063650

cmd/skopeo/proxy.go

@@ -73,6 +73,7 @@ import (
 	"github.com/containers/image/v5/image"
 	"github.com/containers/image/v5/manifest"
 	"github.com/containers/image/v5/pkg/blobinfocache"
+	"github.com/containers/image/v5/signature"
 	"github.com/containers/image/v5/transports"
 	"github.com/containers/image/v5/transports/alltransports"
 	"github.com/containers/image/v5/types"
@@ -152,9 +153,9 @@ type activePipe struct {
 // openImage is an opened image reference
 type openImage struct {
 	// id is an opaque integer handle
-	id        uint64
+	id  uint64
-	src       types.ImageSource
+	src types.ImageSource
-	cachedimg types.Image
+	img types.Image
 }
 
 // proxyHandler is the state associated with our socket.
@@ -162,9 +163,10 @@ type proxyHandler struct {
 	// lock protects everything else in this structure.
 	lock sync.Mutex
 	// opts is CLI options
-	opts   *proxyOptions
+	opts      *proxyOptions
-	sysctx *types.SystemContext
+	sysctx    *types.SystemContext
-	cache  types.BlobInfoCache
+	policyctx *signature.PolicyContext
+	cache     types.BlobInfoCache
 
 	// imageSerial is a counter for open images
 	imageSerial uint64
@@ -204,6 +206,12 @@ func (h *proxyHandler) Initialize(args []any) (replyBuf, error) {
 	h.sysctx = sysctx
 	h.cache = blobinfocache.DefaultCache(sysctx)
 
+	policyContext, err := h.opts.global.getPolicyContext()
+	if err != nil {
+		return ret, err
+	}
+	h.policyctx = policyContext
+
 	r := replyBuf{
 		value: protocolVersion,
 	}
@@ -217,6 +225,7 @@ func (h *proxyHandler) OpenImage(args []any) (replyBuf, error) {
 }
 
 func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (retReplyBuf replyBuf, retErr error) {
+	ctx := context.Background()
 	h.lock.Lock()
 	defer h.lock.Unlock()
 	var ret replyBuf
@@ -245,23 +254,54 @@ func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (retReplyBu
 		return ret, err
 	}
 
-	policyContext, err := h.opts.global.getPolicyContext()
-	if err != nil {
-		return ret, err
-	}
-	defer func() {
-		if err := policyContext.Destroy(); err != nil {
-			retErr = noteCloseFailure(retErr, "tearing down policy context", err)
-		}
-	}()
-
 	unparsedTopLevel := image.UnparsedInstance(imgsrc, nil)
-	allowed, err := policyContext.IsRunningImageAllowed(context.Background(), unparsedTopLevel)
+	// Check the signature on the toplevel (possibly multi-arch) manifest, but we don't
+	// yet propagate the error here.
+	allowed, toplevelVerificationErr := h.policyctx.IsRunningImageAllowed(context.Background(), unparsedTopLevel)
+	if toplevelVerificationErr == nil && !allowed {
+		return ret, fmt.Errorf("internal inconsistency: policy verification failed without returning an error")
+	}
+
+	mfest, manifestType, err := unparsedTopLevel.Manifest(ctx)
 	if err != nil {
 		return ret, err
 	}
-	if !allowed {
+	var target *image.UnparsedImage
-		return ret, fmt.Errorf("internal inconsistency: policy verification failed without returning an error")
+	if manifest.MIMETypeIsMultiImage(manifestType) {
+		manifestList, err := manifest.ListFromBlob(mfest, manifestType)
+		if err != nil {
+			return ret, err
+		}
+		instanceDigest, err := manifestList.ChooseInstance(h.sysctx)
+		if err != nil {
+			return ret, err
+		}
+		target = image.UnparsedInstance(imgsrc, &instanceDigest)
+
+		allowed, targetVerificationErr := h.policyctx.IsRunningImageAllowed(context.Background(), target)
+		if targetVerificationErr == nil && !allowed {
+			return ret, fmt.Errorf("internal inconsistency: policy verification failed without returning an error")
+		}
+
+		// Now, we only error if *both* the toplevel and target verification failed.
+		// If either succeeded, that's OK. We want to support a case where the manifest
+		// list is signed, but the target is not (because we previously supported that behavior),
+		// and we want to support the case where only the target is signed (consistent with what c/image enforces).
+		if targetVerificationErr != nil && toplevelVerificationErr != nil {
+			return ret, targetVerificationErr
+		}
+	} else {
+		target = unparsedTopLevel
+
+		// We're not using a manifest list, so require verification of the single arch manifest.
+		if toplevelVerificationErr != nil {
+			return ret, toplevelVerificationErr
+		}
+	}
+
+	img, err := image.FromUnparsedImage(ctx, h.sysctx, target)
+	if err != nil {
+		return ret, err
 	}
 
 	// Note that we never return zero as an imageid; this code doesn't yet
@@ -270,6 +310,7 @@ func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (retReplyBu
 	openimg := &openImage{
 		id:  h.imageSerial,
 		src: imgsrc,
+		img: img,
 	}
 	h.images[openimg.id] = openimg
 	ret.value = openimg.id
@@ -369,44 +410,6 @@ func (h *proxyHandler) returnBytes(retval any, buf []byte) (replyBuf, error) {
 	return ret, nil
 }
 
-// cacheTargetManifest is invoked when GetManifest or GetConfig is invoked
-// the first time for a given image.  If the requested image is a manifest
-// list, this function resolves it to the image matching the calling process'
-// operating system and architecture.
-//
-// TODO: Add GetRawManifest or so that exposes manifest lists
-func (h *proxyHandler) cacheTargetManifest(img *openImage) error {
-	ctx := context.Background()
-	if img.cachedimg != nil {
-		return nil
-	}
-	unparsedToplevel := image.UnparsedInstance(img.src, nil)
-	mfest, manifestType, err := unparsedToplevel.Manifest(ctx)
-	if err != nil {
-		return err
-	}
-	var target *image.UnparsedImage
-	if manifest.MIMETypeIsMultiImage(manifestType) {
-		manifestList, err := manifest.ListFromBlob(mfest, manifestType)
-		if err != nil {
-			return err
-		}
-		instanceDigest, err := manifestList.ChooseInstance(h.sysctx)
-		if err != nil {
-			return err
-		}
-		target = image.UnparsedInstance(img.src, &instanceDigest)
-	} else {
-		target = unparsedToplevel
-	}
-	cachedimg, err := image.FromUnparsedImage(ctx, h.sysctx, target)
-	if err != nil {
-		return err
-	}
-	img.cachedimg = cachedimg
-	return nil
-}
-
 // GetManifest returns a copy of the manifest, converted to OCI format, along with the original digest.
 // Manifest lists are resolved to the current operating system and architecture.
 func (h *proxyHandler) GetManifest(args []any) (replyBuf, error) {
@@ -426,14 +429,8 @@ func (h *proxyHandler) GetManifest(args []any) (replyBuf, error) {
 		return ret, err
 	}
 
-	err = h.cacheTargetManifest(imgref)
-	if err != nil {
-		return ret, err
-	}
-	img := imgref.cachedimg
-
 	ctx := context.Background()
-	rawManifest, manifestType, err := img.Manifest(ctx)
+	rawManifest, manifestType, err := imgref.img.Manifest(ctx)
 	if err != nil {
 		return ret, err
 	}
@@ -462,7 +459,7 @@ func (h *proxyHandler) GetManifest(args []any) (replyBuf, error) {
 	// docker schema and MIME types.
 	if manifestType != imgspecv1.MediaTypeImageManifest {
 		manifestUpdates := types.ManifestUpdateOptions{ManifestMIMEType: imgspecv1.MediaTypeImageManifest}
-		ociImage, err := img.UpdatedImage(ctx, manifestUpdates)
+		ociImage, err := imgref.img.UpdatedImage(ctx, manifestUpdates)
 		if err != nil {
 			return ret, err
 		}
@@ -496,14 +493,9 @@ func (h *proxyHandler) GetFullConfig(args []any) (replyBuf, error) {
 	if err != nil {
 		return ret, err
 	}
-	err = h.cacheTargetManifest(imgref)
-	if err != nil {
-		return ret, err
-	}
-	img := imgref.cachedimg
 
 	ctx := context.TODO()
-	config, err := img.OCIConfig(ctx)
+	config, err := imgref.img.OCIConfig(ctx)
 	if err != nil {
 		return ret, err
 	}
@@ -533,14 +525,9 @@ func (h *proxyHandler) GetConfig(args []any) (replyBuf, error) {
 	if err != nil {
 		return ret, err
 	}
-	err = h.cacheTargetManifest(imgref)
-	if err != nil {
-		return ret, err
-	}
-	img := imgref.cachedimg
 
 	ctx := context.TODO()
-	config, err := img.OCIConfig(ctx)
+	config, err := imgref.img.OCIConfig(ctx)
 	if err != nil {
 		return ret, err
 	}
@@ -644,19 +631,13 @@ func (h *proxyHandler) GetLayerInfo(args []any) (replyBuf, error) {
 
 	ctx := context.TODO()
 
-	err = h.cacheTargetManifest(imgref)
+	layerInfos, err := imgref.img.LayerInfosForCopy(ctx)
-	if err != nil {
-		return ret, err
-	}
-	img := imgref.cachedimg
-
-	layerInfos, err := img.LayerInfosForCopy(ctx)
 	if err != nil {
 		return ret, err
 	}
 
 	if layerInfos == nil {
-		layerInfos = img.LayerInfos()
+		layerInfos = imgref.img.LayerInfos()
 	}
 
 	layers := make([]convertedLayerInfo, 0, len(layerInfos))
@@ -756,9 +737,13 @@ func (h *proxyHandler) close() {
 		err := image.src.Close()
 		if err != nil {
 			// This shouldn't be fatal
-			logrus.Warnf("Failed to close image %s: %v", transports.ImageName(image.cachedimg.Reference()), err)
+			logrus.Warnf("Failed to close image %s: %v", transports.ImageName(image.img.Reference()), err)
 		}
 	}
+
+	if err := h.policyctx.Destroy(); err != nil {
+		logrus.Warnf("tearing down policy context: %v", err)
+	}
 }
 
 // send writes a reply buffer to the socket

integration/proxy_test.go

@@ -21,8 +21,9 @@ import (
 	"github.com/stretchr/testify/suite"
 )
 
-// This image is known to be x86_64 only right now
+// This image is known to be x86_64 only right now; TODO push
-const knownNotManifestListedImageX8664 = "docker://quay.io/coreos/11bot"
+// a non-manifest-listed fixture to quay.io/libpod
+const knownNotManifestListedImageX8664 = "docker://quay.io/cgwalters/ostest"
 
 // knownNotExtantImage would be very surprising if it did exist
 const knownNotExtantImage = "docker://quay.io/centos/centos:opensusewindowsubuntu"
@@ -175,7 +176,12 @@ func (p *proxy) callReadAllBytes(method string, args []any) (rval any, buf []byt
 	return
 }
 
-func newProxy() (*proxy, error) {
+type proxyConfig struct {
+	// policyFilePath is equivalent to skopeo --policy, an empty value is treated as unset
+	policyFilePath string
+}
+
+func newProxy(config proxyConfig) (*proxy, error) {
 	fds, err := syscall.Socketpair(syscall.AF_LOCAL, syscall.SOCK_SEQPACKET, 0)
 	if err != nil {
 		return nil, err
@@ -191,7 +197,12 @@ func newProxy() (*proxy, error) {
 	}
 
 	// Note ExtraFiles starts at 3
-	proc := exec.Command("skopeo", "experimental-image-proxy", "--sockfd", "3")
+	args := []string{}
+	if config.policyFilePath != "" {
+		args = append(args, "--policy", config.policyFilePath)
+	}
+	args = append(args, "experimental-image-proxy", "--sockfd", "3")
+	proc := exec.Command("skopeo", args...)
 	proc.Stderr = os.Stderr
 	cmdLifecycleToParentIfPossible(proc)
 	proc.ExtraFiles = append(proc.ExtraFiles, theirfd)
@@ -350,7 +361,10 @@ func runTestOpenImageOptionalNotFound(p *proxy, img string) error {
 
 func (s *proxySuite) TestProxy() {
 	t := s.T()
-	p, err := newProxy()
+	config := proxyConfig{
+		policyFilePath: "",
+	}
+	p, err := newProxy(config)
 	require.NoError(t, err)
 
 	err = runTestMetadataAPIs(p, knownNotManifestListedImageX8664)
@@ -371,3 +385,30 @@ func (s *proxySuite) TestProxy() {
 	}
 	assert.NoError(t, err)
 }
+
+// Verify that a policy that denies all images is correctly rejected
+// for both manifest listed and direct per-arch images.
+func (s *proxySuite) TestProxyPolicyRejectAll() {
+	t := s.T()
+	tempd := t.TempDir()
+	config := proxyConfig{
+		policyFilePath: tempd + "/policy.json",
+	}
+	err := os.WriteFile(config.policyFilePath, []byte("{ \"default\": [ { \"type\":\"reject\"} ] }"), 0o644)
+	require.NoError(t, err)
+	p, err := newProxy(config)
+	require.NoError(t, err)
+
+	err = runTestGetManifestAndConfig(p, knownNotManifestListedImageX8664)
+	assert.ErrorContains(t, err, "is rejected by policy")
+
+	err = runTestGetManifestAndConfig(p, knownListImage)
+	assert.ErrorContains(t, err, "is rejected by policy")
+
+	// This one should continue to be fine.
+	err = runTestOpenImageOptionalNotFound(p, knownNotExtantImage)
+	if err != nil {
+		err = fmt.Errorf("Testing optional image %s: %v", knownNotExtantImage, err)
+	}
+	assert.NoError(t, err)
+}