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

@@ -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,20 +255,58 @@ 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 (as some signing
+		// systems are known to do this).
+		if targetVerificationErr != nil && toplevelVerificationErr != nil {
+			return ret, toplevelVerificationErr
+		}
+	} else {
+		target = unparsedTopLevel
+	}
+
+	cachedimg, 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
 	// handle overflow though.
 	h.imageSerial++
 	openimg := &openImage{
-		id:  h.imageSerial,
-		src: imgsrc,
+		id:        h.imageSerial,
+		src:       imgsrc,
+		cachedimg: cachedimg,
 	}
 	h.images[openimg.id] = openimg
 	ret.value = openimg.id
@@ -367,44 +406,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,10 +425,6 @@ 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()
@@ -494,10 +491,6 @@ 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()
@@ -531,10 +524,6 @@ 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()
@@ -641,10 +630,6 @@ 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)