From cf5a3bc53134db29e04fe038fa4e5ed7929a7676 Mon Sep 17 00:00:00 2001 From: Djordje Lukic Date: Thu, 14 Dec 2023 16:15:35 +0100 Subject: [PATCH] c8d: Fix image commit with userns mapping The remapping in the commit code was in the wrong place, we would create a diff and then remap the snapshot, but the descriptor created in "CreateDiff" was still pointing to the old snapshot, we now remap the snapshot before creating a diff. Also make sure we don't lose any capabilities, they used to be lost after the chown. Signed-off-by: Djordje Lukic --- daemon/containerd/image_commit.go | 66 ++++++++++---------- daemon/containerd/image_snapshot.go | 2 - daemon/containerd/image_snapshot_unix.go | 76 +++++++++++++++++------- 3 files changed, 89 insertions(+), 55 deletions(-) diff --git a/daemon/containerd/image_commit.go b/daemon/containerd/image_commit.go index c6f08fd253..0c6677cc0c 100644 --- a/daemon/containerd/image_commit.go +++ b/daemon/containerd/image_commit.go @@ -29,6 +29,7 @@ import ( "github.com/opencontainers/image-spec/identity" "github.com/opencontainers/image-spec/specs-go" ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/pkg/errors" ) /* @@ -81,7 +82,7 @@ func (i *ImageService) CommitImage(ctx context.Context, cc backend.CommitConfig) } }() - diffLayerDesc, diffID, err := createDiff(ctx, cc.ContainerID, sn, cs, differ) + diffLayerDesc, diffID, err := i.createDiff(ctx, cc.ContainerID, sn, cs, differ) if err != nil { return "", fmt.Errorf("failed to export layer: %w", err) } @@ -249,10 +250,38 @@ func writeContentsForImage(ctx context.Context, snName string, cs content.Store, // createDiff creates a layer diff into containerd's content store. // If the diff is empty it returns nil empty digest and no error. -func createDiff(ctx context.Context, name string, sn snapshots.Snapshotter, cs content.Store, comparer diff.Comparer) (*ocispec.Descriptor, digest.Digest, error) { +func (i *ImageService) createDiff(ctx context.Context, name string, sn snapshots.Snapshotter, cs content.Store, comparer diff.Comparer) (*ocispec.Descriptor, digest.Digest, error) { + if !i.idMapping.Empty() { + // The rootfs of the container is remapped if an id mapping exists, we + // need to "unremap" it before committing the snapshot + rootPair := i.idMapping.RootPair() + usernsID := fmt.Sprintf("%s-%d-%d", name, rootPair.UID, rootPair.GID) + remappedID := usernsID + remapSuffix + + err := sn.Commit(ctx, name+"-pre", name) + if err != nil && !cerrdefs.IsAlreadyExists(err) { + return nil, "", err + } + + if !cerrdefs.IsAlreadyExists(err) { + mounts, err := sn.Prepare(ctx, remappedID, name+"-pre") + if err != nil { + return nil, "", err + } + + if err := i.unremapRootFS(ctx, mounts); err != nil { + return nil, "", err + } + + if err := sn.Commit(ctx, name, remappedID); err != nil { + return nil, "", err + } + } + } + newDesc, err := rootfs.CreateDiff(ctx, name, sn, comparer) if err != nil { - return nil, "", err + return nil, "", errors.Wrap(err, "CreateDiff") } ra, err := cs.ReaderAt(ctx, newDesc) @@ -311,7 +340,7 @@ func (i *ImageService) applyDiffLayer(ctx context.Context, name string, containe defer func() { if retErr != nil { - // NOTE: the snapshotter should be hold by lease. Even + // NOTE: the snapshotter should be held by lease. Even // if the cleanup fails, the containerd gc can delete it. if err := sn.Remove(ctx, key); err != nil { log.G(ctx).Warnf("failed to cleanup aborted apply %s: %s", key, err) @@ -323,35 +352,6 @@ func (i *ImageService) applyDiffLayer(ctx context.Context, name string, containe return err } - if !i.idMapping.Empty() { - // The rootfs of the container is remapped if an id mapping exists, we - // need to "unremap" it before committing the snapshot - rootPair := i.idMapping.RootPair() - usernsID := fmt.Sprintf("%s-%d-%d", key, rootPair.UID, rootPair.GID) - remappedID := usernsID + remapSuffix - - if err = sn.Commit(ctx, name+"-pre", key); err != nil { - if cerrdefs.IsAlreadyExists(err) { - return nil - } - return err - } - - mounts, err = sn.Prepare(ctx, remappedID, name+"-pre") - if err != nil { - return err - } - - if err := i.unremapRootFS(ctx, mounts); err != nil { - return err - } - - if err := sn.Commit(ctx, name, remappedID); err != nil { - return err - } - key = remappedID - } - if err = sn.Commit(ctx, name, key); err != nil { if cerrdefs.IsAlreadyExists(err) { return nil diff --git a/daemon/containerd/image_snapshot.go b/daemon/containerd/image_snapshot.go index 190c2e0321..157c550090 100644 --- a/daemon/containerd/image_snapshot.go +++ b/daemon/containerd/image_snapshot.go @@ -17,8 +17,6 @@ import ( "github.com/pkg/errors" ) -const remapSuffix = "-remap" - // PrepareSnapshot prepares a snapshot from a parent image for a container func (i *ImageService) PrepareSnapshot(ctx context.Context, id string, parentImage string, platform *ocispec.Platform, setupInit func(string) error) error { var parentSnapshot string diff --git a/daemon/containerd/image_snapshot_unix.go b/daemon/containerd/image_snapshot_unix.go index 491034aabc..99cbe3c12f 100644 --- a/daemon/containerd/image_snapshot_unix.go +++ b/daemon/containerd/image_snapshot_unix.go @@ -11,38 +11,33 @@ import ( "github.com/containerd/containerd/mount" "github.com/containerd/containerd/snapshots" - "github.com/containerd/log" + "github.com/containerd/continuity/sysx" "github.com/docker/docker/pkg/idtools" ) -func (i *ImageService) remapSnapshot(ctx context.Context, snapshotter snapshots.Snapshotter, id string, parentSnapshot string) error { - rootPair := i.idMapping.RootPair() - usernsID := fmt.Sprintf("%s-%d-%d", parentSnapshot, rootPair.UID, rootPair.GID) - remappedID := usernsID + remapSuffix +const ( + // Values based on linux/include/uapi/linux/capability.h + xattrCapsSz2 = 20 + versionOffset = 3 + vfsCapRevision2 = 2 + vfsCapRevision3 = 3 + remapSuffix = "-remap" +) - // If the remapped snapshot already exist we only need to prepare the new snapshot - if _, err := snapshotter.Stat(ctx, usernsID); err == nil { - _, err = snapshotter.Prepare(ctx, id, usernsID) +func (i *ImageService) remapSnapshot(ctx context.Context, snapshotter snapshots.Snapshotter, id string, parentSnapshot string) error { + _, err := snapshotter.Prepare(ctx, id, parentSnapshot) + if err != nil { return err } - - mounts, err := snapshotter.Prepare(ctx, remappedID, parentSnapshot) + mounts, err := snapshotter.Mounts(ctx, id) if err != nil { return err } if err := i.remapRootFS(ctx, mounts); err != nil { - if rmErr := snapshotter.Remove(ctx, usernsID); rmErr != nil { - log.G(ctx).WithError(rmErr).Warn("failed to remove snapshot after remap error") - } return err } - if err := snapshotter.Commit(ctx, usernsID, remappedID); err != nil { - return err - } - - _, err = snapshotter.Prepare(ctx, id, usernsID) return err } @@ -63,7 +58,7 @@ func (i *ImageService) remapRootFS(ctx context.Context, mounts []mount.Mount) er return err } - return os.Lchown(path, ids.UID, ids.GID) + return chownWithCaps(path, ids.UID, ids.GID) }) }) } @@ -85,7 +80,48 @@ func (i *ImageService) unremapRootFS(ctx context.Context, mounts []mount.Mount) return err } - return os.Lchown(path, uid, gid) + return chownWithCaps(path, uid, gid) }) }) } + +// chownWithCaps will chown path and preserve the extended attributes. +// chowning a file will remove the capabilities, so we need to first get all of +// them, chown the file, and then set the extended attributes +func chownWithCaps(path string, uid int, gid int) error { + xattrKeys, err := sysx.LListxattr(path) + if err != nil { + return err + } + + xattrs := make(map[string][]byte, len(xattrKeys)) + + for _, xattr := range xattrKeys { + data, err := sysx.LGetxattr(path, xattr) + if err != nil { + return err + } + xattrs[xattr] = data + } + + if err := os.Lchown(path, uid, gid); err != nil { + return err + } + + for xattrKey, xattrValue := range xattrs { + length := len(xattrValue) + // make sure the capabilities are version 2, + // capabilities version 3 also store the root uid of the namespace, + // we don't want this when we are in userns-remap mode + // see: https://github.com/moby/moby/pull/41724 + if xattrKey == "security.capability" && xattrValue[versionOffset] == vfsCapRevision3 { + xattrValue[versionOffset] = vfsCapRevision2 + length = xattrCapsSz2 + } + if err := sysx.LSetxattr(path, xattrKey, xattrValue[:length], 0); err != nil { + return err + } + } + + return nil +}