Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

proxy: Verify *either* toplevel or target #2400

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+114 −88
Open

proxy: Verify *either* toplevel or target #2400

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e88bb0
proxy: Move policycontext into global state
cgwalters Aug 15, 2024
21bf7d8
proxy: Verify *either* toplevel or target
cgwalters Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
151 changes: 68 additions & 83 deletions cmd/skopeo/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -152,19 +153,20 @@ 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.
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
Expand Down Expand Up @@ -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()
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return ret, err
}
h.policyctx = policyContext

r := replyBuf{
value: protocolVersion,
}
Expand All @@ -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
Expand Down Expand Up @@ -245,31 +254,63 @@ 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 {
mtrmac marked this conversation as resolved.
Show resolved Hide resolved
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.
h.imageSerial++
openimg := &openImage{
id: h.imageSerial,
src: imgsrc,
img: img,
}
h.images[openimg.id] = openimg
ret.value = openimg.id
Expand Down Expand Up @@ -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) {
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Pre-existing, non-blocking: using image.src.Reference() here would more obviously correct — but it ends up being the same value in the end.)

}
}

if err := h.policyctx.Destroy(); err != nil {
logrus.Warnf("tearing down policy context: %v", err)
}
}

// send writes a reply buffer to the socket
Expand Down
51 changes: 46 additions & 5 deletions integration/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This might be a tiny bit easier to read with raw string literals per https://go.dev/ref/spec#String_literals.)

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)
Comment on lines +394 to +397
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert.NoError(t, err, "testing image %s", knownNotExtantImage)

}