From 5e88bb0d2ac8ae2f82a1427e915eab48e896175e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 14 Aug 2024 21:53:02 -0400 Subject: [PATCH 1/2] proxy: Move policycontext into global state I am not aware of a reason not to just cache this for the life of the proxy, like we do other global state. Prep for further changes. Signed-off-by: Colin Walters --- cmd/skopeo/proxy.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 9396f42273..1d5ff6bd5f 100644 --- a/cmd/skopeo/proxy.go +++ b/cmd/skopeo/proxy.go @@ -74,6 +74,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" @@ -162,9 +163,10 @@ type proxyHandler struct { // lock protects everything else in this structure. lock sync.Mutex // opts is CLI options - opts *proxyOptions - sysctx *types.SystemContext - cache types.BlobInfoCache + opts *proxyOptions + sysctx *types.SystemContext + 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, } @@ -245,18 +253,8 @@ 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) + allowed, err := h.policyctx.IsRunningImageAllowed(context.Background(), unparsedTopLevel) if err != nil { return ret, err } @@ -704,6 +702,10 @@ func (h *proxyHandler) close() { logrus.Warnf("Failed to close image %s: %v", transports.ImageName(image.cachedimg.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 From 21bf7d8592290995a461a7e0f5a1eec44835554f Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 14 Aug 2024 22:03:58 -0400 Subject: [PATCH 2/2] proxy: Verify *either* toplevel or target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (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 --- cmd/skopeo/proxy.go | 129 +++++++++++++++++--------------------- integration/proxy_test.go | 51 +++++++++++++-- 2 files changed, 102 insertions(+), 78 deletions(-) diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 1d5ff6bd5f..a4a90898c6 100644 --- a/cmd/skopeo/proxy.go +++ b/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) } } diff --git a/integration/proxy_test.go b/integration/proxy_test.go index 826128da64..bcf45185b1 100644 --- a/integration/proxy_test.go +++ b/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,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) +}