Merge pull request moby#3671 from sipsma/fix-merge-file-caps

diffapply: do chown before xattrs

Author: sipsma

go.mod

@@ -87,6 +87,7 @@ require (
 	google.golang.org/genproto v0.0.0-20230131230820-1c016267d619
 	google.golang.org/grpc v1.52.3
 	google.golang.org/protobuf v1.28.1
+	kernel.org/pub/linux/libs/security/libcap/cap v1.2.67
 )
 
 require (
@@ -156,4 +157,5 @@ require (
 	golang.org/x/text v0.7.0 // indirect
 	golang.org/x/tools v0.5.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
+	kernel.org/pub/linux/libs/security/libcap/psx v1.2.67 // indirect
 )

go.sum

@@ -1890,6 +1890,10 @@ k8s.io/legacy-cloud-providers v0.17.4/go.mod h1:FikRNoD64ECjkxO36gkDgJeiQWwyZTuB
 k8s.io/utils v0.0.0-20191114184206-e782cd3c129f/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
 k8s.io/utils v0.0.0-20200729134348-d5654de09c73/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
 k8s.io/utils v0.0.0-20201110183641-67b214c5f920/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
+kernel.org/pub/linux/libs/security/libcap/cap v1.2.67 h1:sPQ9qlSNR26fToTKbxe/HDWJlXvBLqGmt84LGCQkOy0=
+kernel.org/pub/linux/libs/security/libcap/cap v1.2.67/go.mod h1:GkntoBuwffz19qtdFVB+k2NtWNN+yCKnC/Ykv/hMiTU=
+kernel.org/pub/linux/libs/security/libcap/psx v1.2.67 h1:NxbXJ7pDVq0FKBsqjieT92QDXI2XaqH2HAi4QcCOHt8=
+kernel.org/pub/linux/libs/security/libcap/psx v1.2.67/go.mod h1:+l6Ee2F59XiJ2I6WR5ObpC1utCQJZ/VLsEbQCD8RG24=
 modernc.org/cc v1.0.0/go.mod h1:1Sk4//wdnYJiUIxnW8ddKpaOJCF37yAdqYnkxUpaYxw=
 modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
 modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k=

snapshot/diffapply_unix.go

@@ -379,6 +379,18 @@ func (a *applier) applyCopy(ctx context.Context, ca *changeApply) error {
 		return errors.Errorf("unhandled file type %d during merge at path %q", ca.srcStat.Mode&unix.S_IFMT, ca.srcPath)
 	}
 
+	// NOTE: it's important that chown happens before setting xattrs due to the fact that chown will
+	// reset the security.capabilities xattr which results in file capabilities being lost.
+	if err := os.Lchown(ca.dstPath, int(ca.srcStat.Uid), int(ca.srcStat.Gid)); err != nil {
+		return errors.Wrap(err, "failed to chown during apply")
+	}
+
+	if ca.srcStat.Mode&unix.S_IFMT != unix.S_IFLNK {
+		if err := unix.Chmod(ca.dstPath, ca.srcStat.Mode); err != nil {
+			return errors.Wrapf(err, "failed to chmod path %q during apply", ca.dstPath)
+		}
+	}
+
 	if ca.srcPath != "" {
 		xattrs, err := sysx.LListxattr(ca.srcPath)
 		if err != nil {
@@ -410,16 +422,6 @@ func (a *applier) applyCopy(ctx context.Context, ca *changeApply) error {
 		}
 	}
 
-	if err := os.Lchown(ca.dstPath, int(ca.srcStat.Uid), int(ca.srcStat.Gid)); err != nil {
-		return errors.Wrap(err, "failed to chown during apply")
-	}
-
-	if ca.srcStat.Mode&unix.S_IFMT != unix.S_IFLNK {
-		if err := unix.Chmod(ca.dstPath, ca.srcStat.Mode); err != nil {
-			return errors.Wrapf(err, "failed to chmod path %q during apply", ca.dstPath)
-		}
-	}
-
 	atimeSpec := unix.Timespec{Sec: ca.srcStat.Atim.Sec, Nsec: ca.srcStat.Atim.Nsec}
 	mtimeSpec := unix.Timespec{Sec: ca.srcStat.Mtim.Sec, Nsec: ca.srcStat.Mtim.Nsec}
 	if ca.srcStat.Mode&unix.S_IFMT != unix.S_IFDIR {

snapshot/snapshotter_test.go

@@ -26,6 +26,7 @@ import (
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 	bolt "go.etcd.io/bbolt"
+	libcap "kernel.org/pub/linux/libs/security/libcap/cap"
 )
 
 func newSnapshotter(ctx context.Context, t *testing.T, snapshotterName string) (_ context.Context, _ *mergeSnapshotter, rerr error) {
@@ -372,6 +373,41 @@ func TestHardlinks(t *testing.T) {
 	}
 }
 
+func TestMergeFileCapabilities(t *testing.T) {
+	requireRoot(t)
+	for _, snName := range []string{"overlayfs", "native", "native-nohardlink"} {
+		snName := snName
+		t.Run(snName, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, sn, err := newSnapshotter(context.Background(), t, snName)
+			require.NoError(t, err)
+
+			setCaps := "cap_net_bind_service=+ep"
+			base1Snap := committedKey(ctx, t, sn, identity.NewID(), "",
+				fstest.CreateFile("hasCaps", []byte("capable"), 0700),
+				fstest.Chown("hasCaps", 1000, 1000),
+				setFileCap("hasCaps", setCaps),
+			)
+			base2Snap := committedKey(ctx, t, sn, identity.NewID(), "",
+				fstest.CreateFile("foo", []byte("bar"), 0600),
+			)
+
+			mergeSnap := mergeKey(ctx, t, sn, identity.NewID(), []Diff{
+				{"", base1Snap.Name},
+				{"", base2Snap.Name},
+			})
+
+			actualCaps := getFileCap(ctx, t, sn, mergeSnap.Name, "hasCaps")
+			require.Equal(t, "cap_net_bind_service=ep", actualCaps)
+			stat := statPath(ctx, t, sn, mergeSnap.Name, "hasCaps")
+			require.EqualValues(t, 1000, stat.Uid)
+			require.EqualValues(t, 1000, stat.Gid)
+			require.EqualValues(t, 0700, stat.Mode&0777)
+		})
+	}
+}
+
 func TestUsage(t *testing.T) {
 	for _, snName := range []string{"overlayfs", "native", "native-nohardlink"} {
 		snName := snName
@@ -559,7 +595,7 @@ func requireMtime(t *testing.T, path string, mtime time.Time) {
 	require.Equal(t, mtime.UnixNano(), stat.Mtim.Nano())
 }
 
-func tryStatPath(ctx context.Context, t *testing.T, sn *mergeSnapshotter, key, path string) (st *syscall.Stat_t) {
+func pathCallback[T any](ctx context.Context, t *testing.T, sn *mergeSnapshotter, key, path string, cb func(t *testing.T, path string) *T) *T {
 	t.Helper()
 	mounts, cleanup := getMounts(ctx, t, sn, key)
 	defer cleanup()
@@ -577,24 +613,32 @@ func tryStatPath(ctx context.Context, t *testing.T, sn *mergeSnapshotter, key, p
 			}
 		}
 		if upperdir != "" {
-			st = trySyscallStat(t, filepath.Join(upperdir, path))
-			if st != nil {
-				return st
+			r := cb(t, filepath.Join(upperdir, path))
+			if r != nil {
+				return r
 			}
 		}
 		for _, lowerdir := range lowerdirs {
-			st = trySyscallStat(t, filepath.Join(lowerdir, path))
-			if st != nil {
-				return st
+			r := cb(t, filepath.Join(lowerdir, path))
+			if r != nil {
+				return r
 			}
 		}
 		return nil
 	}
 
+	var r *T
 	withMount(ctx, t, sn, key, func(root string) {
-		st = trySyscallStat(t, filepath.Join(root, path))
+		r = cb(t, filepath.Join(root, path))
+	})
+	return r
+}
+
+func tryStatPath(ctx context.Context, t *testing.T, sn *mergeSnapshotter, key, path string) *syscall.Stat_t {
+	t.Helper()
+	return pathCallback(ctx, t, sn, key, path, func(t *testing.T, path string) *syscall.Stat_t {
+		return trySyscallStat(t, path)
 	})
-	return st
 }
 
 func statPath(ctx context.Context, t *testing.T, sn *mergeSnapshotter, key, path string) (st *syscall.Stat_t) {
@@ -610,3 +654,36 @@ func requireRoot(t *testing.T) {
 		t.Skip("test requires root")
 	}
 }
+
+func setFileCap(path string, caps string) fstest.Applier {
+	return applyFn(func(root string) error {
+		path := filepath.Join(root, path)
+		capSet, err := libcap.FromText(caps)
+		if err != nil {
+			return err
+		}
+		return capSet.SetFile(path)
+	})
+}
+
+func getFileCap(ctx context.Context, t *testing.T, sn *mergeSnapshotter, key, path string) string {
+	t.Helper()
+	caps := pathCallback(ctx, t, sn, key, path, func(t *testing.T, path string) *string {
+		t.Helper()
+		capSet, err := libcap.GetFile(path)
+		if err != nil {
+			require.ErrorIs(t, err, os.ErrNotExist)
+			return nil
+		}
+		caps := capSet.String()
+		return &caps
+	})
+	require.NotNil(t, caps)
+	return *caps
+}
+
+type applyFn func(root string) error
+
+func (a applyFn) Apply(root string) error {
+	return a(root)
+}