proxy: Verify *either* toplevel or target

(Only compile tested locally)

An issue was raised in that a current Red Hat internal build system
was performing a signature just on the per-architecture manifest,
but the current proxy code is expecting a signature on the manifest
list.

To quote Miloslav from that bug:

> Podman signs both the per-platform items and the top level,
> and enforces signatures only on per-platform items. cosign,
> by default, signs only the top level (and has an option to
> sign everything), I’m not sure which one it enforces.
> I don’t immediately recall other platforms.

We believe the current proxy code is secure since
we always require signatures (if configured) on the manifest
list, and the manifest list covers the individual manifests
via sha256 digest.

However, we want to support signatures only being present
on the per-arch manifest too in order to be flexible.

Yet, we can't hard switch to requiring signatures on the
per-arch manifests as that would immediately break anyone
relying on the current behavior of validating the toplevel.

Change the logic here to:

- Verify signature on the manifest list, and cache the error (if any)
- Fetch the manifest
- Verify signature on the manifest
- Allow if *either* signature was accepted; conversely, only error
  if signature validation failed on *both* the manifest list and
  manifest

This also switches things to cache the manifest upfront instead
of doing it lazily on `GetManifest/GetConfig`; in practice
callers were always immediately requesting those anyways.
The use case of just fetching blobs exists (e.g. to resume
an interrupted fetch), but is relatively obscure and
in general I think it's good to re-verify signatures on
each operation.

Signed-off-by: Colin Walters <[email protected]>

Author: cgwalters

cmd/skopeo/proxy.go

@@ -153,9 +153,9 @@ type activePipe struct {
 // openImage is an opened image reference
 type openImage struct {
 	// id is an opaque integer handle
-	id        uint64
-	src       types.ImageSource
-	cachedimg types.Image
+	id  uint64
+	src types.ImageSource
+	img types.Image
 }
 
 // proxyHandler is the state associated with our socket.
@@ -225,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
@@ -254,12 +255,53 @@ func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (retReplyBu
 	}
 
 	unparsedTopLevel := image.UnparsedInstance(imgsrc, nil)
-	allowed, err := h.policyctx.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 {
-		return ret, fmt.Errorf("internal inconsistency: policy verification failed without returning an error")
+	var target *image.UnparsedImage
+	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
@@ -268,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
@@ -367,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) {
@@ -424,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
 	}
@@ -460,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
 		}
@@ -494,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
 	}
@@ -531,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
 	}
@@ -641,19 +630,13 @@ func (h *proxyHandler) GetLayerInfo(args []any) (replyBuf, error) {
 
 	ctx := context.TODO()
 
-	err = h.cacheTargetManifest(imgref)
-	if err != nil {
-		return ret, err
-	}
-	img := imgref.cachedimg
-
-	layerInfos, err := img.LayerInfosForCopy(ctx)
+	layerInfos, err := imgref.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))
@@ -699,7 +682,7 @@ 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)
 		}
 	}
 

integration/proxy_test.go

@@ -19,8 +19,9 @@ import (
 	"github.com/stretchr/testify/suite"
 )
 
-// This image is known to be x86_64 only right now
-const knownNotManifestListedImageX8664 = "docker://quay.io/coreos/11bot"
+// This image is known to be x86_64 only right now; TODO push
+// 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"
@@ -173,7 +174,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
@@ -189,7 +195,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)
@@ -334,7 +345,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 = runTestGetManifestAndConfig(p, knownNotManifestListedImageX8664)
@@ -355,3 +369,29 @@ 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",
+	}
+	os.WriteFile(config.policyFilePath, []byte("{ \"default\": [ { \"type\":\"reject\"} ] }"), 0o644)
+	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)
+}