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 <djordje.lukic@docker.com>

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

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

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
+}