amadan

carlos / platform Public

layout: EnsureDirs relies on inheritance and verifies, never chmods or chowns (plan 4 final review)

The pass runs as carlos, which is not in carlos-sockets. Its fchmod of
<run>/.instances cleared setgid, and its chown into the sockets group
failed with EPERM, so every migrate was refused after damaging the parent.
The instance directories now come from mkdirat alone; both parents and
the new or existing instance directory are checked and refused, never
repaired. MkdirBeneath and ChownBeneath go, replaced by EnsureDirBeneath
and StatDir, which only create and report.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude ee6f2a6f67bc2c40e6439aca05926c9ab451ffca parent fa3c25f
6 files changed, +417 −146
  • internal/instancefs/dirs_linux.go +49 −56
  • internal/instancefs/dirs_linux_test.go +67 −43
  • internal/instancefs/instancefs.go +7 −0
  • internal/instancefs/instancefs_other.go +5 −10
  • internal/layout/preflight.go +88 −36
  • internal/layout/preflight_test.go +201 −1
diff --git a/internal/instancefs/dirs_linux.go b/internal/instancefs/dirs_linux.go
index 16d184efc..5129fa4ed 100644
--- a/internal/instancefs/dirs_linux.go
+++ b/internal/instancefs/dirs_linux.go
@@ -18,44 +18,70 @@ import (
// directory fd that openDir opened without following any symlink, and names
// exactly one entry beneath it. The *at calls do not follow a symlink at
// that last component either: mkdirat answers EEXIST, renameat and unlinkat
-// act on the link itself, and fchownat is told not to.
+// act on the link itself, and fstatat is told not to.
-// MkdirBeneath makes sure name, directly under dir, is a real directory
-// with exactly perm (setgid and sticky included): it creates it when it is
-// absent and accepts it when it is already a directory, then sets the mode
-// through an fd on the directory itself. mkdir(2) masks the mode with the
-// umask and ignores setgid, so the explicit fchmod is what makes the mode
-// exact. Anything else at name, a symlink or a file, is refused and left
-// untouched.
-func MkdirBeneath(dir, name string, perm os.FileMode) error {
+// EnsureDirBeneath makes sure name, directly under dir, is a real
+// directory and reports what is there. It creates it with mkdirat when it
+// is absent, so its group and setgid come from dir the way the kernel
+// hands them out, and leaves an existing directory exactly as it is. It
+// never changes a mode or an owner: a chmod by a caller outside the
+// directory's group silently clears setgid, and a chown into a group the
+// caller is not in fails, so the caller checks the returned stat and
+// refuses what it cannot vouch for. Anything else at name, a symlink or a
+// file, is refused and left untouched.
+func EnsureDirBeneath(dir, name string, perm os.FileMode) (DirStat, error) {
if err := bareName(name); err != nil {
- return err
+ return DirStat{}, err
}
dfd, err := openDir(dir)
if err != nil {
- return err
+ return DirStat{}, err
}
defer unix.Close(dfd)
path := filepath.Join(dir, name)
if err := unix.Mkdirat(dfd, name, uint32(perm.Perm())); err != nil && !errors.Is(err, unix.EEXIST) {
- return &os.PathError{Op: "mkdirat", Path: path, Err: err}
+ return DirStat{}, &os.PathError{Op: "mkdirat", Path: path, Err: err}
}
- how := unix.OpenHow{
- Flags: unix.O_RDONLY | unix.O_DIRECTORY | unix.O_CLOEXEC,
- Resolve: resolveFlags,
+ var st unix.Stat_t
+ if err := unix.Fstatat(dfd, name, &st, unix.AT_SYMLINK_NOFOLLOW); err != nil {
+ return DirStat{}, &os.PathError{Op: "fstatat", Path: path, Err: err}
}
- fd, err := unix.Openat2(dfd, name, &how)
+ switch st.Mode & unix.S_IFMT {
+ case unix.S_IFDIR:
+ return dirStatOf(&st), nil
+ case unix.S_IFLNK:
+ return DirStat{}, fmt.Errorf("%w: %s", ErrSymlink, path)
+ }
+ return DirStat{}, fmt.Errorf("instancefs: %s is not a directory", path)
+}
+
+// StatDir reports dir itself, opened without following a symlink at any
+// component.
+func StatDir(dir string) (DirStat, error) {
+ fd, err := openDir(dir)
if err != nil {
- if errors.Is(err, unix.ELOOP) {
- return fmt.Errorf("%w: %s", ErrSymlink, path)
- }
- return &os.PathError{Op: "openat2", Path: path, Err: err}
+ return DirStat{}, err
}
defer unix.Close(fd)
- if err := unix.Fchmod(fd, unixMode(perm)); err != nil {
- return &os.PathError{Op: "fchmod", Path: path, Err: err}
+ var st unix.Stat_t
+ if err := unix.Fstat(fd, &st); err != nil {
+ return DirStat{}, &os.PathError{Op: "fstat", Path: dir, Err: err}
}
- return nil
+ return dirStatOf(&st), nil
+}
+
+func dirStatOf(st *unix.Stat_t) DirStat {
+ mode := os.ModeDir | os.FileMode(st.Mode&0o777)
+ if st.Mode&unix.S_ISUID != 0 {
+ mode |= os.ModeSetuid
+ }
+ if st.Mode&unix.S_ISGID != 0 {
+ mode |= os.ModeSetgid
+ }
+ if st.Mode&unix.S_ISVTX != 0 {
+ mode |= os.ModeSticky
+ }
+ return DirStat{Mode: mode, UID: int(st.Uid), GID: int(st.Gid)}
}
// RenameBetween renames srcName in srcDir to dstName in dstDir. Both
@@ -103,36 +129,3 @@ func RemoveBeneath(dir, name string) error {
}
return nil
}
-
-// ChownBeneath changes the owner and group of name directly under dir
-// without following a symlink there. -1 leaves either unchanged.
-func ChownBeneath(dir, name string, uid, gid int) error {
- if err := bareName(name); err != nil {
- return err
- }
- dfd, err := openDir(dir)
- if err != nil {
- return err
- }
- defer unix.Close(dfd)
- if err := unix.Fchownat(dfd, name, uid, gid, unix.AT_SYMLINK_NOFOLLOW); err != nil {
- return &os.PathError{Op: "fchownat", Path: filepath.Join(dir, name), Err: err}
- }
- return nil
-}
-
-// unixMode converts an os.FileMode to the mode bits chmod(2) takes; Go's
-// setgid and sticky flags are not the kernel's bit positions.
-func unixMode(m os.FileMode) uint32 {
- mode := uint32(m.Perm())
- if m&os.ModeSetuid != 0 {
- mode |= unix.S_ISUID
- }
- if m&os.ModeSetgid != 0 {
- mode |= unix.S_ISGID
- }
- if m&os.ModeSticky != 0 {
- mode |= unix.S_ISVTX
- }
- return mode
-}
diff --git a/internal/instancefs/dirs_linux_test.go b/internal/instancefs/dirs_linux_test.go
index eb2a24364..46b2a3aba 100644
--- a/internal/instancefs/dirs_linux_test.go
+++ b/internal/instancefs/dirs_linux_test.go
@@ -10,41 +10,56 @@ import (
"testing"
)
-func TestMkdirBeneathCreatesWithTheExactMode(t *testing.T) {
+// A new directory takes its mode from mkdirat and its group and setgid
+// from its parent, the way the kernel hands them out; the stat returned is
+// what is on disk.
+func TestEnsureDirBeneathCreatesAndInheritsSetgid(t *testing.T) {
dir := t.TempDir()
- if err := MkdirBeneath(dir, "run", 0o750|os.ModeSetgid); err != nil {
- t.Fatalf("MkdirBeneath: %v", err)
+ if err := os.Chmod(dir, 0o750|os.ModeSetgid); err != nil {
+ t.Fatal(err)
+ }
+ st, err := EnsureDirBeneath(dir, "run", 0o700)
+ if err != nil {
+ t.Fatalf("EnsureDirBeneath: %v", err)
}
fi, err := os.Lstat(filepath.Join(dir, "run"))
if err != nil {
t.Fatal(err)
}
- if !fi.IsDir() || fi.Mode().Perm() != 0o750 || fi.Mode()&os.ModeSetgid == 0 {
- t.Fatalf("mode = %v, want a 2750 directory", fi.Mode())
+ if !fi.IsDir() || fi.Mode().Perm() != 0o700 || fi.Mode()&os.ModeSetgid == 0 {
+ t.Fatalf("mode = %v, want a directory 0700 with setgid inherited", fi.Mode())
+ }
+ if st.Mode != fi.Mode() || st.UID != os.Geteuid() || st.GID != int(fi.Sys().(*syscall.Stat_t).Gid) {
+ t.Fatalf("stat = %+v, disk says %v", st, fi.Mode())
}
}
-// An existing real directory is accepted and brought to the requested
-// mode: the pass runs again and again over the same host, and a directory
-// a same-uid process loosened must not stay loose.
-func TestMkdirBeneathAcceptsAnExistingDirectoryAndResetsItsMode(t *testing.T) {
+// An existing directory is reported as it stands and never changed: a
+// chmod by a caller outside the directory's group clears setgid, so the
+// caller verifies instead.
+func TestEnsureDirBeneathLeavesAnExistingDirectoryAlone(t *testing.T) {
dir := t.TempDir()
- if err := os.Mkdir(filepath.Join(dir, "d"), 0o777); err != nil {
+ d := filepath.Join(dir, "d")
+ if err := os.Mkdir(d, 0o700); err != nil {
t.Fatal(err)
}
- if err := os.Chmod(filepath.Join(dir, "d"), 0o777); err != nil {
+ if err := os.Chmod(d, 0o777|os.ModeSetgid); err != nil {
t.Fatal(err)
}
- if err := MkdirBeneath(dir, "d", 0o700); err != nil {
- t.Fatalf("MkdirBeneath over an existing directory: %v", err)
+ st, err := EnsureDirBeneath(dir, "d", 0o700)
+ if err != nil {
+ t.Fatalf("EnsureDirBeneath over an existing directory: %v", err)
+ }
+ want := os.ModeDir | os.ModeSetgid | 0o777
+ if st.Mode != want {
+ t.Errorf("stat mode = %v, want %v", st.Mode, want)
}
- fi, _ := os.Lstat(filepath.Join(dir, "d"))
- if fi.Mode().Perm() != 0o700 {
- t.Fatalf("mode = %v, want 0700", fi.Mode())
+ if fi, _ := os.Lstat(d); fi.Mode() != want {
+ t.Errorf("mode changed to %v", fi.Mode())
}
}
-func TestMkdirBeneathRefusesASymlinkAndLeavesItsTargetAlone(t *testing.T) {
+func TestEnsureDirBeneathRefusesASymlinkAndLeavesItsTargetAlone(t *testing.T) {
dir, elsewhere := t.TempDir(), t.TempDir()
if err := os.Chmod(elsewhere, 0o755); err != nil {
t.Fatal(err)
@@ -52,38 +67,38 @@ func TestMkdirBeneathRefusesASymlinkAndLeavesItsTargetAlone(t *testing.T) {
if err := os.Symlink(elsewhere, filepath.Join(dir, "d")); err != nil {
t.Fatal(err)
}
- if err := MkdirBeneath(dir, "d", 0o700); !errors.Is(err, ErrSymlink) {
+ if _, err := EnsureDirBeneath(dir, "d", 0o700); !errors.Is(err, ErrSymlink) {
t.Fatalf("err = %v, want ErrSymlink", err)
}
fi, _ := os.Stat(elsewhere)
if fi.Mode().Perm() != 0o755 {
- t.Fatalf("the symlink's target was chmodded to %v", fi.Mode())
+ t.Fatalf("the symlink's target was changed to %v", fi.Mode())
}
}
-func TestMkdirBeneathRefusesAFileAndAPathName(t *testing.T) {
+func TestEnsureDirBeneathRefusesAFileAndAPathName(t *testing.T) {
dir := t.TempDir()
if err := os.WriteFile(filepath.Join(dir, "f"), nil, 0o600); err != nil {
t.Fatal(err)
}
- if err := MkdirBeneath(dir, "f", 0o700); err == nil {
+ if _, err := EnsureDirBeneath(dir, "f", 0o700); err == nil {
t.Fatal("a regular file accepted as a directory")
}
- if err := MkdirBeneath(dir, "a/b", 0o700); !errors.Is(err, ErrEscapes) {
+ if _, err := EnsureDirBeneath(dir, "a/b", 0o700); !errors.Is(err, ErrEscapes) {
t.Fatalf("a multi-component name: err = %v, want ErrEscapes", err)
}
- if err := MkdirBeneath(dir, "..", 0o700); !errors.Is(err, ErrEscapes) {
+ if _, err := EnsureDirBeneath(dir, "..", 0o700); !errors.Is(err, ErrEscapes) {
t.Fatalf("..: err = %v, want ErrEscapes", err)
}
}
-func TestMkdirBeneathRefusesASymlinkedParent(t *testing.T) {
+func TestEnsureDirBeneathRefusesASymlinkedParent(t *testing.T) {
base, real := t.TempDir(), t.TempDir()
link := filepath.Join(base, "link")
if err := os.Symlink(real, link); err != nil {
t.Fatal(err)
}
- if err := MkdirBeneath(link, "d", 0o700); !errors.Is(err, ErrSymlink) {
+ if _, err := EnsureDirBeneath(link, "d", 0o700); !errors.Is(err, ErrSymlink) {
t.Fatalf("err = %v, want ErrSymlink", err)
}
if _, err := os.Lstat(filepath.Join(real, "d")); !errors.Is(err, os.ErrNotExist) {
@@ -91,6 +106,33 @@ func TestMkdirBeneathRefusesASymlinkedParent(t *testing.T) {
}
}
+func TestStatDirReportsModeAndGroupAndRefusesASymlink(t *testing.T) {
+ base := t.TempDir()
+ d := filepath.Join(base, "d")
+ if err := os.Mkdir(d, 0o700); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Chmod(d, 0o750|os.ModeSetgid); err != nil {
+ t.Fatal(err)
+ }
+ st, err := StatDir(d)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if st.Mode != os.ModeDir|os.ModeSetgid|0o750 || st.GID != os.Getegid() {
+ t.Fatalf("stat = %+v", st)
+ }
+ if err := os.Symlink(d, filepath.Join(base, "link")); err != nil {
+ t.Fatal(err)
+ }
+ if _, err := StatDir(filepath.Join(base, "link")); !errors.Is(err, ErrSymlink) {
+ t.Fatalf("err = %v, want ErrSymlink", err)
+ }
+ if _, err := StatDir(filepath.Join(base, "absent")); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("err = %v, want ErrNotExist", err)
+ }
+}
+
func TestRenameBetweenMovesFilesAndDirectories(t *testing.T) {
src, dst := t.TempDir(), t.TempDir()
if err := os.WriteFile(filepath.Join(src, "h.db"), []byte("db"), 0o600); err != nil {
@@ -187,21 +229,3 @@ func TestRemoveBeneathRemovesTheEntryNotItsTarget(t *testing.T) {
t.Fatalf("second remove: err = %v, want ErrNotExist", err)
}
}
-
-func TestChownBeneathChangesTheGroupWithoutFollowing(t *testing.T) {
- dir := t.TempDir()
- if err := os.Mkdir(filepath.Join(dir, "d"), 0o700); err != nil {
- t.Fatal(err)
- }
- gid := os.Getgid()
- if err := ChownBeneath(dir, "d", -1, gid); err != nil {
- t.Fatalf("ChownBeneath to our own group: %v", err)
- }
- fi, _ := os.Lstat(filepath.Join(dir, "d"))
- if st := fi.Sys().(*syscall.Stat_t); int(st.Gid) != gid {
- t.Fatalf("gid = %d, want %d", st.Gid, gid)
- }
- if err := ChownBeneath(dir, "a/b", -1, gid); !errors.Is(err, ErrEscapes) {
- t.Fatalf("path name: err = %v, want ErrEscapes", err)
- }
-}
diff --git a/internal/instancefs/instancefs.go b/internal/instancefs/instancefs.go
index 3e07bc427..ca7e4b667 100644
--- a/internal/instancefs/instancefs.go
+++ b/internal/instancefs/instancefs.go
@@ -40,6 +40,13 @@ var ErrEscapes = errors.New("instancefs: path escapes its directory")
// the files this package reads on a tenant's behalf.
var ErrNotRegular = errors.New("instancefs: not a regular file")
+// DirStat is what EnsureDirBeneath and StatDir found: the mode in
+// os.FileMode form (setgid included) and the owning uid and gid.
+type DirStat struct {
+ Mode os.FileMode
+ UID, GID int
+}
+
// cleanRel validates name as a relative path that stays inside dir.
func cleanRel(name string) (string, error) {
if name == "" || filepath.IsAbs(name) {
diff --git a/internal/instancefs/instancefs_other.go b/internal/instancefs/instancefs_other.go
index 8ffe79b90..41464f3be 100644
--- a/internal/instancefs/instancefs_other.go
+++ b/internal/instancefs/instancefs_other.go
@@ -29,13 +29,15 @@ func MountNoSymfollow(mountpoint string) (bool, error) {
func mountinfoLineHas(line, mountpoint, option string) bool { return false }
-func MkdirBeneath(dir, name string, perm os.FileMode) error {
+func EnsureDirBeneath(dir, name string, perm os.FileMode) (DirStat, error) {
if err := bareName(name); err != nil {
- return err
+ return DirStat{}, err
}
- return errors.ErrUnsupported
+ return DirStat{}, errors.ErrUnsupported
}
+func StatDir(dir string) (DirStat, error) { return DirStat{}, errors.ErrUnsupported }
+
func RenameBetween(srcDir, srcName, dstDir, dstName string) error {
if err := bareName(srcName); err != nil {
return err
@@ -52,10 +54,3 @@ func RemoveBeneath(dir, name string) error {
}
return errors.ErrUnsupported
}
-
-func ChownBeneath(dir, name string, uid, gid int) error {
- if err := bareName(name); err != nil {
- return err
- }
- return errors.ErrUnsupported
-}
diff --git a/internal/layout/preflight.go b/internal/layout/preflight.go
index 0cdeaa2db..38593b5d2 100644
--- a/internal/layout/preflight.go
+++ b/internal/layout/preflight.go
@@ -99,46 +99,98 @@ func DestinationCheck(p *Plan, now func() time.Time) (aside []string, err error)
return aside, nil
}
-// EnsureDirs creates the raw directories the bind mounts show: the data
-// directory 0700 (the tenant's own ground) and the run directory 2750 with
-// the carlos-sockets group, so an enrolled consumer can reach app.sock. The
-// raw parents get the modes bootstrap's tmpfiles lines give them, in case
-// the box has not run those yet. The directories must exist before the row
-// flips, because Restore and prepare both assume them.
+// EnsureDirs makes the raw directories the bind mounts show: the data
+// directory 0700 (the tenant's own ground) and the run directory 2750 in
+// the carlos-sockets group, so an enrolled consumer can dial app.sock
+// (design §3.1). The directories must exist before the row flips, because
+// Restore and prepare both assume them.
//
-// Each directory is made from an fd on its parent that followed no symlink,
-// and is then required to be a real directory: a same-uid tenant can plant
-// a symlink at <data>/.instances/<iid> before the pass arrives, and a move
-// into it would land the database wherever the link points.
+// The pass runs as carlos, which is not a member of carlos-sockets. It
+// therefore cannot chown anything into that group, and any chmod it makes
+// on a directory of that group has the kernel clear setgid, which on the
+// run parent would hand every later instance directory the wrong group.
+// So nothing here changes a mode or an owner. The run directory gets its
+// group and setgid by inheritance from <run>/.instances, which tmpfiles
+// owns, exactly as prepare relies on; both parents are checked and never
+// repaired, and a new or existing instance directory that is not what
+// inheritance should have made is refused as it stands.
+//
+// Each directory is made and checked through an fd on its parent that
+// followed no symlink: a same-uid tenant can plant a symlink at
+// <data>/.instances/<iid> before the pass arrives, and a move into it
+// would land the database wherever the link points.
func EnsureDirs(dataDir, socketDir, host string, socketsGID int) error {
data := rawInstanceDir(dataDir, host)
run := rawRunDir(socketDir, host)
- for _, d := range []struct {
- path string
- perm os.FileMode
- gid int
- }{
- {filepath.Dir(data), 0o700, -1},
- {data, 0o700, -1},
- {filepath.Dir(run), 0o750 | os.ModeSetgid, socketsGID},
- {run, 0o750 | os.ModeSetgid, socketsGID},
- } {
- parent, name := filepath.Dir(d.path), filepath.Base(d.path)
- if err := instancefs.MkdirBeneath(parent, name, d.perm); err != nil {
- return fmt.Errorf("layout: %s: %w", host, err)
- }
- if d.gid >= 0 {
- if err := instancefs.ChownBeneath(parent, name, -1, d.gid); err != nil {
- return fmt.Errorf("layout: %s: %w", host, err)
- }
- }
- fi, err := os.Lstat(d.path)
- if err != nil {
- return fmt.Errorf("layout: %s: %w", host, err)
- }
- if !fi.IsDir() {
- return fmt.Errorf("layout: %s: %s is not a directory (%v)", host, d.path, fi.Mode().Type())
- }
+ // The run parent is checked before anything is created, so a refusal
+ // leaves nothing behind.
+ runParent, err := checkRunParent(filepath.Dir(run), socketsGID)
+ if err != nil {
+ return fmt.Errorf("layout: %s: %w", host, err)
+ }
+ // The data parent is tmpfiles' too, but carlos:carlos 0700 is exactly
+ // what mkdir as carlos makes, so a missing one is created, never
+ // changed.
+ if _, err := instancefs.EnsureDirBeneath(dataDir, filepath.Base(filepath.Dir(data)), 0o700); err != nil {
+ return fmt.Errorf("layout: %s: %w", host, err)
+ }
+ st, err := instancefs.EnsureDirBeneath(filepath.Dir(data), filepath.Base(data), 0o700)
+ if err != nil {
+ return fmt.Errorf("layout: %s: %w", host, err)
+ }
+ if err := checkInstanceDir(data, st, 0o700, false, -1); err != nil {
+ return fmt.Errorf("layout: %s: %w", host, err)
+ }
+ st, err = instancefs.EnsureDirBeneath(filepath.Dir(run), filepath.Base(run), 0o750)
+ if err != nil {
+ return fmt.Errorf("layout: %s: %w", host, err)
+ }
+ if err := checkInstanceDir(run, st, 0o750, runParent.Mode&os.ModeSetgid != 0, socketsGID); err != nil {
+ return fmt.Errorf("layout: %s: %w", host, err)
+ }
+ return nil
+}
+
+// checkRunParent reads <run>/.instances. With the sockets group known it
+// must be there already, in that group and setgid, because only tmpfiles,
+// running as root, can make it so. Without the group (a box that has none,
+// or a test) a missing parent is simply created.
+func checkRunParent(parent string, socketsGID int) (instancefs.DirStat, error) {
+ if socketsGID < 0 {
+ return instancefs.EnsureDirBeneath(filepath.Dir(parent), filepath.Base(parent), 0o750)
+ }
+ st, err := instancefs.StatDir(parent)
+ if errors.Is(err, os.ErrNotExist) {
+ return st, fmt.Errorf("%s is missing; tmpfiles should create it, and the pass does not", parent)
+ }
+ if err != nil {
+ return st, err
+ }
+ if st.GID != socketsGID {
+ return st, fmt.Errorf("%s has group %d, not carlos-sockets (%d); tmpfiles should restore it, and the pass does not repair it", parent, st.GID, socketsGID)
+ }
+ if st.Mode&os.ModeSetgid == 0 {
+ return st, fmt.Errorf("%s has lost its setgid bit (mode %v); tmpfiles should restore it, and the pass does not repair it", parent, st.Mode)
+ }
+ return st, nil
+}
+
+// checkInstanceDir refuses an instance directory the pass cannot vouch
+// for: not its own, the wrong permission bits, no setgid under a setgid
+// parent, or not the sockets group. Each would need a chmod or chown to
+// fix, which is exactly what the pass must not do.
+func checkInstanceDir(path string, st instancefs.DirStat, perm os.FileMode, wantSetgid bool, gid int) error {
+ if st.UID != os.Geteuid() {
+ return fmt.Errorf("%s is owned by uid %d, not the agent's %d; refusing to use it", path, st.UID, os.Geteuid())
+ }
+ if st.Mode.Perm() != perm {
+ return fmt.Errorf("%s has mode %v, not %v; refusing to use it", path, st.Mode.Perm(), perm)
+ }
+ if wantSetgid && st.Mode&os.ModeSetgid == 0 {
+ return fmt.Errorf("%s has no setgid bit though its parent has; refusing to use it", path)
+ }
+ if gid >= 0 && st.GID != gid {
+ return fmt.Errorf("%s has group %d, not carlos-sockets (%d); refusing to use it", path, st.GID, gid)
}
return nil
}
diff --git a/internal/layout/preflight_test.go b/internal/layout/preflight_test.go
index dcd4d4ddb..b066db997 100644
--- a/internal/layout/preflight_test.go
+++ b/internal/layout/preflight_test.go
@@ -8,6 +8,7 @@ import (
"path/filepath"
"strconv"
"strings"
+ "syscall"
"testing"
"time"
@@ -161,7 +162,7 @@ func TestEnsureDirsCreatesTheRawDirectoriesWithTheRightModes(t *testing.T) {
t.Errorf("data dir: %v %v", fi, err)
}
fi, err = os.Lstat(rawRunDir(rd, "h.example.com"))
- if err != nil || !fi.IsDir() || fi.Mode().Perm() != 0o750 || fi.Mode()&os.ModeSetgid == 0 {
+ if err != nil || !fi.IsDir() || fi.Mode().Perm() != 0o750 {
t.Errorf("run dir: %v %v", fi, err)
}
// Idempotent: the pass calls it on every move.
@@ -184,3 +185,202 @@ func TestEnsureDirsRefusesASymlinkAtTheInstanceDirectory(t *testing.T) {
t.Fatalf("err = %v, want ErrSymlink", err)
}
}
+
+// supplementaryGroup is a group the test runner is in other than its
+// effective one, so a directory can be given a group that inheritance
+// alone explains. The runner is not assumed to be root.
+func supplementaryGroup(t *testing.T) int {
+ t.Helper()
+ groups, err := os.Getgroups()
+ if err != nil {
+ t.Fatal(err)
+ }
+ probe := t.TempDir()
+ for _, g := range groups {
+ // A group the runner is listed in can still be unusable: inside a
+ // user namespace an unmapped gid is refused with EINVAL.
+ if g != os.Getegid() && os.Chown(probe, -1, g) == nil {
+ return g
+ }
+ }
+ t.Skip("the test runner is in no usable supplementary group, so a setgid parent with a group other than its own cannot be built")
+ return -1
+}
+
+func gidOf(t *testing.T, path string) int {
+ t.Helper()
+ fi, err := os.Lstat(path)
+ if err != nil {
+ t.Fatal(err)
+ }
+ return int(fi.Sys().(*syscall.Stat_t).Gid)
+}
+
+// runParentFixture makes <rd>/.instances with group gid and mode, the way
+// tmpfiles leaves it on a box, and the data parent 0700.
+func runParentFixture(t *testing.T, dd, rd string, gid int, mode os.FileMode) string {
+ t.Helper()
+ host := "h.example.com"
+ parent := filepath.Dir(rawRunDir(rd, host))
+ if err := os.Mkdir(parent, 0o700); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Chown(parent, -1, gid); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Chmod(parent, mode); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Mkdir(filepath.Dir(rawInstanceDir(dd, host)), 0o700); err != nil {
+ t.Fatal(err)
+ }
+ return parent
+}
+
+// The pass runs as carlos, which is not in carlos-sockets, so it can
+// neither chown into that group nor chmod a directory of it without the
+// kernel clearing setgid. The instance's run directory gets the group and
+// setgid by inheritance alone, and the tmpfiles-owned parents are never
+// touched. The parent's mode here is deliberately not the 2750 tmpfiles
+// gives it, so any chmod of it shows.
+func TestEnsureDirsInheritsTheSocketsGroupAndLeavesTheParentsAlone(t *testing.T) {
+ g := supplementaryGroup(t)
+ dd, rd := t.TempDir(), t.TempDir()
+ host := "h.example.com"
+ runParent := runParentFixture(t, dd, rd, g, 0o770|os.ModeSetgid)
+ dataParent := filepath.Dir(rawInstanceDir(dd, host))
+ if err := os.Chmod(dataParent, 0o710); err != nil {
+ t.Fatal(err)
+ }
+ before := map[string]os.FileMode{}
+ for _, d := range []string{runParent, dataParent} {
+ fi, err := os.Lstat(d)
+ if err != nil {
+ t.Fatal(err)
+ }
+ before[d] = fi.Mode()
+ }
+ for i := 0; i < 2; i++ {
+ if err := EnsureDirs(dd, rd, host, g); err != nil {
+ t.Fatalf("call %d: %v", i+1, err)
+ }
+ }
+ run := rawRunDir(rd, host)
+ fi, err := os.Lstat(run)
+ if err != nil || !fi.IsDir() || fi.Mode().Perm() != 0o750 || fi.Mode()&os.ModeSetgid == 0 {
+ t.Fatalf("run dir: %v %v, want a 2750 directory", fi, err)
+ }
+ if got := gidOf(t, run); got != g {
+ t.Errorf("run dir group = %d, want the inherited %d", got, g)
+ }
+ for d, mode := range before {
+ fi, err := os.Lstat(d)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if fi.Mode() != mode {
+ t.Errorf("%s mode changed from %v to %v; the pass must never chmod a tmpfiles-owned parent", d, mode, fi.Mode())
+ }
+ }
+ if got := gidOf(t, runParent); got != g {
+ t.Errorf("run parent group changed to %d", got)
+ }
+}
+
+// A run parent that has lost setgid, or carries the wrong group, would
+// hand every new instance directory the wrong group. The pass refuses,
+// names the parent, repairs nothing and creates nothing.
+func TestEnsureDirsRefusesARunParentWithoutSetgidOrTheSocketsGroup(t *testing.T) {
+ g := supplementaryGroup(t)
+ for _, tc := range []struct {
+ name string
+ gid int
+ mode os.FileMode
+ }{
+ {"no setgid", g, 0o750},
+ {"wrong group", os.Getegid(), 0o750 | os.ModeSetgid},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ dd, rd := t.TempDir(), t.TempDir()
+ host := "h.example.com"
+ parent := runParentFixture(t, dd, rd, tc.gid, tc.mode)
+ err := EnsureDirs(dd, rd, host, g)
+ if err == nil || !strings.Contains(err.Error(), parent) || !strings.Contains(err.Error(), "tmpfiles should restore it") {
+ t.Fatalf("err = %v, want a refusal naming %s", err, parent)
+ }
+ for _, d := range []string{rawInstanceDir(dd, host), rawRunDir(rd, host)} {
+ if _, err := os.Lstat(d); !errors.Is(err, os.ErrNotExist) {
+ t.Errorf("%s was created by a refused call", d)
+ }
+ }
+ fi, _ := os.Lstat(parent)
+ if fi.Mode() != os.ModeDir|tc.mode || gidOf(t, parent) != tc.gid {
+ t.Errorf("parent repaired to %v group %d", fi.Mode(), gidOf(t, parent))
+ }
+ })
+ }
+}
+
+// An instance directory already there with the wrong group, without the
+// setgid its parent has, or with the wrong mode is refused as it stands:
+// changing it is exactly the chmod or chown the pass cannot do safely.
+func TestEnsureDirsRefusesAnExistingInstanceDirectoryItCannotVouchFor(t *testing.T) {
+ g := supplementaryGroup(t)
+ host := "h.example.com"
+ for _, tc := range []struct {
+ name string
+ setup func(t *testing.T, dd, rd string) string
+ }{
+ {"run dir with the wrong group", func(t *testing.T, dd, rd string) string {
+ d := rawRunDir(rd, host)
+ if err := os.Mkdir(d, 0o750); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Chown(d, -1, os.Getegid()); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Chmod(d, 0o750|os.ModeSetgid); err != nil {
+ t.Fatal(err)
+ }
+ return d
+ }},
+ {"run dir without setgid", func(t *testing.T, dd, rd string) string {
+ d := rawRunDir(rd, host)
+ if err := os.Mkdir(d, 0o750); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Chmod(d, 0o750); err != nil {
+ t.Fatal(err)
+ }
+ return d
+ }},
+ {"data dir opened up", func(t *testing.T, dd, rd string) string {
+ d := rawInstanceDir(dd, host)
+ if err := os.Mkdir(d, 0o700); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Chmod(d, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ return d
+ }},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ dd, rd := t.TempDir(), t.TempDir()
+ runParentFixture(t, dd, rd, g, 0o750|os.ModeSetgid)
+ d := tc.setup(t, dd, rd)
+ before, err := os.Lstat(d)
+ if err != nil {
+ t.Fatal(err)
+ }
+ gid := gidOf(t, d)
+ if err := EnsureDirs(dd, rd, host, g); err == nil || !strings.Contains(err.Error(), d) {
+ t.Fatalf("err = %v, want a refusal naming %s", err, d)
+ }
+ after, _ := os.Lstat(d)
+ if after.Mode() != before.Mode() || gidOf(t, d) != gid {
+ t.Errorf("%s changed from %v group %d to %v group %d", d, before.Mode(), gid, after.Mode(), gidOf(t, d))
+ }
+ })
+ }
+}