From 4846e49a0ef3638b52791a824872708c7cfabfa3 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 7 Jun 2024 15:31:55 +0900 Subject: [PATCH 1/6] overlay: Always enforce force mask Force mask is not being enforced when there is a lower layer so correct it. Signed-off-by: Akihiko Odaki --- drivers/overlay/overlay.go | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index 679d64c1bd..d2a36c33f4 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -1049,6 +1049,9 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl if err := idtools.MkdirAllAndChownNew(path.Dir(dir), 0o755, idPair); err != nil { return err } + + perms := defaultPerms + if parent != "" { parentBase := d.dir(parent) st, err := system.Stat(filepath.Join(parentBase, "diff")) @@ -1057,6 +1060,7 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl } rootUID = int(st.UID()) rootGID = int(st.GID()) + perms = os.FileMode(st.Mode()) } if err := fileutils.Lexists(dir); err == nil { @@ -1102,20 +1106,10 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl } } - perms := defaultPerms if d.options.forceMask != nil { perms = *d.options.forceMask } - if parent != "" { - parentBase := d.dir(parent) - st, err := system.Stat(filepath.Join(parentBase, "diff")) - if err != nil { - return err - } - perms = os.FileMode(st.Mode()) - } - if err := idtools.MkdirAs(path.Join(dir, "diff"), perms, rootUID, rootGID); err != nil { return err } From 97cf580abacd44152c3bc73d3ad5874962e45792 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 7 Jun 2024 15:48:06 +0900 Subject: [PATCH 2/6] overlay: Override mode when forcing mask Naively forcing mask for the root directory corrupts its mode in the containers. Set the overriding extended attribute to fix the mode. Signed-off-by: Akihiko Odaki --- drivers/overlay/overlay.go | 28 ++++++++++++++++++---------- pkg/idtools/idtools.go | 12 ++++++++++++ 2 files changed, 30 insertions(+), 10 deletions(-) diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index d2a36c33f4..83f482c895 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -1050,17 +1050,17 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl return err } - perms := defaultPerms + st := idtools.Stat{IDs: idPair, Mode: defaultPerms} if parent != "" { parentBase := d.dir(parent) - st, err := system.Stat(filepath.Join(parentBase, "diff")) + systemSt, err := system.Stat(filepath.Join(parentBase, "diff")) if err != nil { return err } - rootUID = int(st.UID()) - rootGID = int(st.GID()) - perms = os.FileMode(st.Mode()) + st.IDs.UID = int(systemSt.UID()) + st.IDs.GID = int(systemSt.GID()) + st.Mode = os.FileMode(systemSt.Mode()) } if err := fileutils.Lexists(dir); err == nil { @@ -1106,14 +1106,22 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl } } + forcedPerms := st.Mode if d.options.forceMask != nil { - perms = *d.options.forceMask + forcedPerms = *d.options.forceMask } - if err := idtools.MkdirAs(path.Join(dir, "diff"), perms, rootUID, rootGID); err != nil { + diff := path.Join(dir, "diff") + if err := idtools.MkdirAs(diff, forcedPerms, st.IDs.UID, st.IDs.GID); err != nil { return err } + if d.options.forceMask != nil { + if err := idtools.SetContainersOverrideXattr(diff, st); err != nil { + return err + } + } + lid := generateID(idLength) linkBase := path.Join("..", id, "diff") @@ -1126,16 +1134,16 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl return err } - if err := idtools.MkdirAs(path.Join(dir, "work"), 0o700, rootUID, rootGID); err != nil { + if err := idtools.MkdirAs(path.Join(dir, "work"), 0o700, st.IDs.UID, st.IDs.GID); err != nil { return err } - if err := idtools.MkdirAs(path.Join(dir, "merged"), 0o700, rootUID, rootGID); err != nil { + if err := idtools.MkdirAs(path.Join(dir, "merged"), 0o700, st.IDs.UID, st.IDs.GID); err != nil { return err } // if no parent directory, create a dummy lower directory and skip writing a "lowers" file if parent == "" { - return idtools.MkdirAs(path.Join(dir, "empty"), 0o700, rootUID, rootGID) + return idtools.MkdirAs(path.Join(dir, "empty"), 0o700, st.IDs.UID, st.IDs.GID) } lower, err := d.getLower(parent) diff --git a/pkg/idtools/idtools.go b/pkg/idtools/idtools.go index ef5a952546..1657e9258d 100644 --- a/pkg/idtools/idtools.go +++ b/pkg/idtools/idtools.go @@ -367,6 +367,18 @@ func checkChownErr(err error, name string, uid, gid int) error { return err } +// Stat contains file states that can be overriden with ContainersOverrideXattr. +type Stat struct { + IDs IDPair + Mode os.FileMode +} + +// SetContainersOverrideXattr will encode and set ContainersOverrideXattr. +func SetContainersOverrideXattr(path string, stat Stat) error { + value := fmt.Sprintf("%d:%d:0%o", stat.IDs.UID, stat.IDs.GID, stat.Mode) + return system.Lsetxattr(path, ContainersOverrideXattr, []byte(value), 0) +} + func SafeChown(name string, uid, gid int) error { if runtime.GOOS == "darwin" { var mode uint64 = 0o0700 From ce3f9d3bd44c238f9f6aa02e6cf38baff474d3fc Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 7 Jun 2024 15:52:15 +0900 Subject: [PATCH 3/6] overlay: Fix owner when forcing mask Forcing mask implies the owner is fixed. Signed-off-by: Akihiko Odaki --- drivers/overlay/overlay.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index 83f482c895..2ecc2575bb 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -1106,13 +1106,14 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl } } - forcedPerms := st.Mode + forcedSt := st if d.options.forceMask != nil { - forcedPerms = *d.options.forceMask + forcedSt.IDs = idPair + forcedSt.Mode = *d.options.forceMask } diff := path.Join(dir, "diff") - if err := idtools.MkdirAs(diff, forcedPerms, st.IDs.UID, st.IDs.GID); err != nil { + if err := idtools.MkdirAs(diff, forcedSt.Mode, forcedSt.IDs.UID, forcedSt.IDs.GID); err != nil { return err } @@ -1134,16 +1135,16 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl return err } - if err := idtools.MkdirAs(path.Join(dir, "work"), 0o700, st.IDs.UID, st.IDs.GID); err != nil { + if err := idtools.MkdirAs(path.Join(dir, "work"), 0o700, forcedSt.IDs.UID, forcedSt.IDs.GID); err != nil { return err } - if err := idtools.MkdirAs(path.Join(dir, "merged"), 0o700, st.IDs.UID, st.IDs.GID); err != nil { + if err := idtools.MkdirAs(path.Join(dir, "merged"), 0o700, forcedSt.IDs.UID, forcedSt.IDs.GID); err != nil { return err } // if no parent directory, create a dummy lower directory and skip writing a "lowers" file if parent == "" { - return idtools.MkdirAs(path.Join(dir, "empty"), 0o700, st.IDs.UID, st.IDs.GID) + return idtools.MkdirAs(path.Join(dir, "empty"), 0o700, forcedSt.IDs.UID, forcedSt.IDs.GID) } lower, err := d.getLower(parent) From f462f4248d7897ab8f6e6f1a70fe2410ff71d9eb Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 7 Jun 2024 15:54:28 +0900 Subject: [PATCH 4/6] overlay: Respect overriding extended attributes Signed-off-by: Akihiko Odaki --- drivers/overlay/overlay.go | 17 +++++++++++------ pkg/idtools/idtools.go | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/drivers/overlay/overlay.go b/drivers/overlay/overlay.go index 2ecc2575bb..4b5bbca883 100644 --- a/drivers/overlay/overlay.go +++ b/drivers/overlay/overlay.go @@ -1054,13 +1054,18 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl if parent != "" { parentBase := d.dir(parent) - systemSt, err := system.Stat(filepath.Join(parentBase, "diff")) - if err != nil { - return err + parentDiff := filepath.Join(parentBase, "diff") + if xSt, err := idtools.GetContainersOverrideXattr(parentDiff); err == nil { + st = xSt + } else { + systemSt, err := system.Stat(parentDiff) + if err != nil { + return err + } + st.IDs.UID = int(systemSt.UID()) + st.IDs.GID = int(systemSt.GID()) + st.Mode = os.FileMode(systemSt.Mode()) } - st.IDs.UID = int(systemSt.UID()) - st.IDs.GID = int(systemSt.GID()) - st.Mode = os.FileMode(systemSt.Mode()) } if err := fileutils.Lexists(dir); err == nil { diff --git a/pkg/idtools/idtools.go b/pkg/idtools/idtools.go index 1657e9258d..e9a3f6d434 100644 --- a/pkg/idtools/idtools.go +++ b/pkg/idtools/idtools.go @@ -373,6 +373,44 @@ type Stat struct { Mode os.FileMode } +// GetContainersOverrideXattr will get and decode ContainersOverrideXattr. +func GetContainersOverrideXattr(path string) (Stat, error) { + var stat Stat + xstat, err := system.Lgetxattr(path, ContainersOverrideXattr) + if err != nil { + return stat, err + } + + attrs := strings.Split(string(xstat), ":") + if len(attrs) != 3 { + return stat, fmt.Errorf("The number of clons in %s does not equal to 3", + ContainersOverrideXattr) + } + + value, err := strconv.ParseUint(attrs[0], 10, 32) + if err != nil { + return stat, fmt.Errorf("Failed to parse UID: %w", err) + } + + stat.IDs.UID = int(value) + + value, err = strconv.ParseUint(attrs[0], 10, 32) + if err != nil { + return stat, fmt.Errorf("Failed to parse GID: %w", err) + } + + stat.IDs.GID = int(value) + + value, err = strconv.ParseUint(attrs[2], 8, 32) + if err != nil { + return stat, fmt.Errorf("Failed to parse mode: %w", err) + } + + stat.Mode = os.FileMode(value) + + return stat, nil +} + // SetContainersOverrideXattr will encode and set ContainersOverrideXattr. func SetContainersOverrideXattr(path string, stat Stat) error { value := fmt.Sprintf("%d:%d:0%o", stat.IDs.UID, stat.IDs.GID, stat.Mode) From b3a2f6a07a5c166a6050e33cf5370702c0f5a289 Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Fri, 7 Jun 2024 15:55:30 +0900 Subject: [PATCH 5/6] idtools: Use SetContainersOverrideXattr() Signed-off-by: Akihiko Odaki --- pkg/archive/archive.go | 15 ++++++++++----- pkg/chunked/storage_linux.go | 9 ++++++--- pkg/idtools/idtools.go | 16 ++++++++-------- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index 906079395b..5ebcefce1c 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -701,8 +701,11 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L } if forceMask != nil && (hdr.Typeflag != tar.TypeSymlink || runtime.GOOS == "darwin") { - value := fmt.Sprintf("%d:%d:0%o", hdr.Uid, hdr.Gid, hdrInfo.Mode()&0o7777) - if err := system.Lsetxattr(path, idtools.ContainersOverrideXattr, []byte(value), 0); err != nil { + value := idtools.Stat{ + IDs: idtools.IDPair{UID: hdr.Uid, GID: hdr.Gid}, + Mode: hdrInfo.Mode() & 0o7777, + } + if err := idtools.SetContainersOverrideXattr(path, value); err != nil { return err } } @@ -1114,11 +1117,13 @@ loop: } if options.ForceMask != nil { - value := "0:0:0755" + value := idtools.Stat{Mode: 0o755} if rootHdr != nil { - value = fmt.Sprintf("%d:%d:0%o", rootHdr.Uid, rootHdr.Gid, rootHdr.Mode) + value.IDs.UID = rootHdr.Uid + value.IDs.GID = rootHdr.Gid + value.Mode = os.FileMode(rootHdr.Mode) } - if err := system.Lsetxattr(dest, idtools.ContainersOverrideXattr, []byte(value), 0); err != nil { + if err := idtools.SetContainersOverrideXattr(dest, value); err != nil { return err } } diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index d10bf2fe20..1f65beed8a 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -1263,9 +1263,12 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff if options.ForceMask != nil { uid, gid, mode, err := archive.GetFileOwner(dest) if err == nil { - value := fmt.Sprintf("%d:%d:0%o", uid, gid, mode) - if err := unix.Setxattr(dest, containersOverrideXattr, []byte(value), 0); err != nil { - return output, &fs.PathError{Op: "setxattr", Path: dest, Err: err} + value := idtools.Stat{ + IDs: idtools.IDPair{UID: int(uid), GID: int(gid)}, + Mode: os.FileMode(mode), + } + if err := idtools.SetContainersOverrideXattr(dest, value); err != nil { + return output, err } } } diff --git a/pkg/idtools/idtools.go b/pkg/idtools/idtools.go index e9a3f6d434..035e24ebbd 100644 --- a/pkg/idtools/idtools.go +++ b/pkg/idtools/idtools.go @@ -419,19 +419,19 @@ func SetContainersOverrideXattr(path string, stat Stat) error { func SafeChown(name string, uid, gid int) error { if runtime.GOOS == "darwin" { - var mode uint64 = 0o0700 + var mode os.FileMode = 0o0700 xstat, err := system.Lgetxattr(name, ContainersOverrideXattr) if err == nil { attrs := strings.Split(string(xstat), ":") if len(attrs) == 3 { val, err := strconv.ParseUint(attrs[2], 8, 32) if err == nil { - mode = val + mode = os.FileMode(val) } } } - value := fmt.Sprintf("%d:%d:0%o", uid, gid, mode) - if err = system.Lsetxattr(name, ContainersOverrideXattr, []byte(value), 0); err != nil { + value := Stat{IDPair{uid, gid}, mode} + if err = SetContainersOverrideXattr(name, value); err != nil { return err } uid = os.Getuid() @@ -447,19 +447,19 @@ func SafeChown(name string, uid, gid int) error { func SafeLchown(name string, uid, gid int) error { if runtime.GOOS == "darwin" { - var mode uint64 = 0o0700 + var mode os.FileMode = 0o0700 xstat, err := system.Lgetxattr(name, ContainersOverrideXattr) if err == nil { attrs := strings.Split(string(xstat), ":") if len(attrs) == 3 { val, err := strconv.ParseUint(attrs[2], 8, 32) if err == nil { - mode = val + mode = os.FileMode(val) } } } - value := fmt.Sprintf("%d:%d:0%o", uid, gid, mode) - if err = system.Lsetxattr(name, ContainersOverrideXattr, []byte(value), 0); err != nil { + value := Stat{IDPair{uid, gid}, mode} + if err = SetContainersOverrideXattr(name, value); err != nil { return err } uid = os.Getuid() From 18136c586b3a9a7692ec6494935a36faa5e6981a Mon Sep 17 00:00:00 2001 From: Akihiko Odaki Date: Sun, 9 Jun 2024 17:36:14 +0900 Subject: [PATCH 6/6] overlay: Test user.fuseoverlayfs.override_stat handling Signed-off-by: Akihiko Odaki --- drivers/graphtest/graphtest_unix.go | 27 +++++++++++++++++++-------- drivers/overlay/overlay_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/drivers/graphtest/graphtest_unix.go b/drivers/graphtest/graphtest_unix.go index 85b4777dfc..17415bd992 100644 --- a/drivers/graphtest/graphtest_unix.go +++ b/drivers/graphtest/graphtest_unix.go @@ -44,13 +44,7 @@ type Driver struct { refCount int } -func newDriver(t testing.TB, name string, options []string) *Driver { - root, err := os.MkdirTemp("", "storage-graphtest-") - require.NoError(t, err) - runroot, err := os.MkdirTemp("", "storage-graphtest-") - require.NoError(t, err) - - require.NoError(t, os.MkdirAll(root, 0o755)) +func newGraphDriver(t testing.TB, name string, options []string, root string, runroot string) graphdriver.Driver { d, err := graphdriver.GetDriver(name, graphdriver.Options{DriverOptions: options, Root: root, RunRoot: runroot}) if err != nil { t.Logf("graphdriver: %v\n", err) @@ -63,7 +57,17 @@ func newDriver(t testing.TB, name string, options []string) *Driver { } t.Fatal(err) } - return &Driver{d, root, runroot, 1} + return d +} + +func newDriver(t testing.TB, name string, options []string) *Driver { + root, err := os.MkdirTemp("", "storage-graphtest-") + require.NoError(t, err) + runroot, err := os.MkdirTemp("", "storage-graphtest-") + require.NoError(t, err) + + require.NoError(t, os.MkdirAll(root, 0o755)) + return &Driver{newGraphDriver(t, name, options, root, runroot), root, runroot, 1} } func cleanup(t testing.TB, d *Driver) { @@ -84,6 +88,13 @@ func GetDriver(t testing.TB, name string, options ...string) graphdriver.Driver return drv } +func ReconfigureDriver(t testing.TB, name string, options ...string) { + if err := drv.Cleanup(); err != nil { + t.Fatal(err) + } + drv.Driver = newGraphDriver(t, name, options, drv.root, drv.runroot) +} + // PutDriver removes the driver if it is no longer used and updates the reference count. func PutDriver(t testing.TB) { if drv == nil { diff --git a/drivers/overlay/overlay_test.go b/drivers/overlay/overlay_test.go index 439ac264a3..e7589622ff 100644 --- a/drivers/overlay/overlay_test.go +++ b/drivers/overlay/overlay_test.go @@ -4,12 +4,16 @@ package overlay import ( + "os" + "os/exec" "testing" graphdriver "github.com/containers/storage/drivers" "github.com/containers/storage/drivers/graphtest" "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/reexec" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) const driverName = "overlay" @@ -31,6 +35,31 @@ func skipIfNaive(t *testing.T) { } } +// Ensure that a layer created with force_mask will keep the root directory mode +// with user.containers.override_stat. This preserved mode should also be +// inherited by the upper layer, whether force_mask is set or not. +// +// This test is placed before TestOverlaySetup() because it uses driver options +// different from the other tests. +func TestContainersOverlayXattr(t *testing.T) { + path, err := exec.LookPath("fuse-overlayfs") + if err != nil { + t.Skipf("fuse-overlayfs unavailable") + } + + driver := graphtest.GetDriver(t, driverName, "force_mask=700", "mount_program="+path) + defer graphtest.PutDriver(t) + require.NoError(t, driver.Create("lower", "", nil)) + graphtest.ReconfigureDriver(t, driverName, "mount_program="+path) + require.NoError(t, driver.Create("upper", "lower", nil)) + + root, err := driver.Get("upper", graphdriver.MountOpts{}) + require.NoError(t, err) + fi, err := os.Stat(root) + require.NoError(t, err) + assert.Equal(t, 0o555&os.ModePerm, fi.Mode()&os.ModePerm, root) +} + // This avoids creating a new driver for each test if all tests are run // Make sure to put new tests between TestOverlaySetup and TestOverlayTeardown func TestOverlaySetup(t *testing.T) {