diff --git a/cmd/skopeo/proxy.go b/cmd/skopeo/proxy.go index 9396f42273..a4a90898c6 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" @@ -152,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. @@ -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, } @@ -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,24 +254,55 @@ func (h *proxyHandler) openImageImpl(args []any, allowNotFound bool) (retReplyBu return ret, err } - policyContext, err := h.opts.global.getPolicyContext() + unparsedTopLevel := image.UnparsedInstance(imgsrc, nil) + // 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 } - defer func() { - if err := policyContext.Destroy(); err != nil { - retErr = noteCloseFailure(retErr, "tearing down policy context", err) + 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) - unparsedTopLevel := image.UnparsedInstance(imgsrc, nil) - allowed, err := policyContext.IsRunningImageAllowed(context.Background(), unparsedTopLevel) + 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 } - if !allowed { - return ret, fmt.Errorf("internal inconsistency: policy verification failed without returning an error") - } // Note that we never return zero as an imageid; this code doesn't yet // handle overflow though. @@ -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 } @@ -643,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)) @@ -701,9 +682,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 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) +}