diff --git a/spinifex/migrate/predastoretopology/004_predastore_topology.go b/spinifex/migrate/predastoretopology/004_predastore_topology.go index e5eb82aa7..ef4844441 100644 --- a/spinifex/migrate/predastoretopology/004_predastore_topology.go +++ b/spinifex/migrate/predastoretopology/004_predastore_topology.go @@ -261,13 +261,6 @@ func relocatePredastoreData(ctx migrate.ConfigContext, topo *predastoreTopology) continue } - // The rename preserved this directory's ownership, which is what - // recovery must leave behind on everything it writes into it. - uid, gid, err := dirOwner(m.To) - if err != nil { - return err - } - // The replica set is renumbered, so the configuration persisted here // names servers that no longer exist. Raft will not bootstrap over // existing state to correct that; it has to be rewritten. @@ -275,13 +268,6 @@ func relocatePredastoreData(ctx migrate.ConfigContext, topo *predastoreTopology) if err != nil { return fmt.Errorf("recover raft configuration for state replica %d in %s: %w", m.NodeID, m.To, err) } - - // Recovery reopened badger and wrote a fresh snapshot as root, both of - // which create files. Runs even when there was nothing to recover, - // which still opens the stores. - if err := chownTree(m.To, uid, gid); err != nil { - return err - } ctx.Logger.Info("Migrated predastore state replica", "nodeID", m.NodeID, "dataDir", m.To, "raftRecovered", recovered) } return nil @@ -306,23 +292,9 @@ func movePredastoreNodeDir(ctx migrate.ConfigContext, m predastoreMove) (bool, e return false, fmt.Errorf("cannot move %s to %s: destination already exists", m.From, m.To) } - // Taken before the rename, which carries it to the destination; the data - // dir created below has to match or predastore cannot traverse into it. - uid, gid, err := dirOwner(m.From) - if err != nil { - return false, err + if err := os.MkdirAll(filepath.Dir(m.To), 0750); err != nil { + return false, fmt.Errorf("create %s: %w", filepath.Dir(m.To), err) } - - dataDir := filepath.Dir(m.To) - if err := os.MkdirAll(dataDir, 0750); err != nil { - return false, fmt.Errorf("create %s: %w", dataDir, err) - } - // Unconditional, so a re-run also repairs a data dir left root-owned by an - // earlier run that failed after creating it. - if err := chown(dataDir, uid, gid); err != nil { - return false, err - } - if err := os.Rename(m.From, m.To); err != nil { return false, fmt.Errorf("move %s to %s: %w", m.From, m.To, err) } diff --git a/spinifex/migrate/predastoretopology/ownership.go b/spinifex/migrate/predastoretopology/ownership.go deleted file mode 100644 index 15b1c0ccd..000000000 --- a/spinifex/migrate/predastoretopology/ownership.go +++ /dev/null @@ -1,66 +0,0 @@ -package predastoretopology - -import ( - "fmt" - "io/fs" - "os" - "path/filepath" - "syscall" -) - -// The migration runs as root under `spx admin upgrade`, but predastore runs as -// its own service user. Anything root creates along the way is root-owned and -// unreachable to it, so the migration has to hand each path back. - -// dirOwner reports the uid and gid owning path. The migration takes the -// ownership it applies from the data it is moving rather than from a service -// user name, so a dev install running as the invoking user needs no special -// case. -func dirOwner(path string) (int, int, error) { - info, err := os.Stat(path) - if err != nil { - return 0, 0, fmt.Errorf("stat %s: %w", path, err) - } - st, ok := info.Sys().(*syscall.Stat_t) - if !ok { - return 0, 0, fmt.Errorf("cannot read ownership of %s", path) - } - return int(st.Uid), int(st.Gid), nil -} - -// chown gives path to uid:gid. -// -// Only root can hand a file to another user, and only root ever creates one -// owned by the wrong user, so an unprivileged run has nothing to correct. -func chown(path string, uid, gid int) error { - if os.Geteuid() != 0 { - return nil - } - if err := os.Lchown(path, uid, gid); err != nil { - return fmt.Errorf("chown %s to %d:%d: %w", path, uid, gid, err) - } - return nil -} - -// chownTree gives path and everything beneath it to uid:gid. -// -// os.Root scopes the walk to the tree, so a symlink planted inside it cannot -// redirect a chown running as root. -func chownTree(path string, uid, gid int) error { - if os.Geteuid() != 0 { - return nil - } - - root, err := os.OpenRoot(path) - if err != nil { - return fmt.Errorf("open %s: %w", path, err) - } - defer root.Close() - - return fs.WalkDir(root.FS(), ".", func(p string, _ fs.DirEntry, err error) error { - if err != nil { - return fmt.Errorf("walk %s: %w", filepath.Join(path, p), err) - } - return chown(filepath.Join(path, p), uid, gid) - }) -} diff --git a/spinifex/migrate/predastoretopology/ownership_test.go b/spinifex/migrate/predastoretopology/ownership_test.go deleted file mode 100644 index eba53b515..000000000 --- a/spinifex/migrate/predastoretopology/ownership_test.go +++ /dev/null @@ -1,94 +0,0 @@ -package predastoretopology - -import ( - "fmt" - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// nobodyUID is an unprivileged id the root-only assertions move data to, so -// "inherited the owner" cannot be confused with "happened to already match". -const nobodyUID, nobodyGID = 65534, 65534 - -// TestPredastoreDataDirInheritsNodeOwnership pins the ownership of the data -// dir the migration creates. Predastore runs as its own service user while the -// migration runs as root, and cannot traverse a root-owned parent. -func TestPredastoreDataDirInheritsNodeOwnership(t *testing.T) { - nodeDirs := []string{ - "distributed/db/node-1", "distributed/db/node-2", "distributed/db/node-3", - "distributed/nodes/node-1", "distributed/nodes/node-2", "distributed/nodes/node-3", - } - configDir, dataDir := predastoreFixture(t, "predastore_v4_singlenode.toml", nodeDirs) - - wantUID, wantGID := os.Geteuid(), os.Getegid() - if os.Geteuid() == 0 { - // Stand the fixture up as an unprivileged user owns it, which is the - // shape of a real install the root migration has to preserve. - for _, dir := range nodeDirs { - require.NoError(t, chownTree(filepath.Join(dataDir, "predastore", dir), nobodyUID, nobodyGID)) - } - wantUID, wantGID = nobodyUID, nobodyGID - } - - runPredastoreMigration(t, configDir, dataDir) - - cluster := filepath.Join(dataDir, "predastore", "cluster") - uid, gid, err := dirOwner(cluster) - require.NoError(t, err) - assert.Equal(t, wantUID, uid, "data dir owner") - assert.Equal(t, wantGID, gid, "data dir group") - - // The node dirs arrive by rename, so they keep their owner rather than - // gaining one; assert it anyway, since recovery writes into them. - for id := 1; id <= 6; id++ { - uid, gid, err := dirOwner(filepath.Join(cluster, fmt.Sprintf("node-%d", id))) - require.NoError(t, err) - assert.Equal(t, wantUID, uid, "node-%d owner", id) - assert.Equal(t, wantGID, gid, "node-%d group", id) - } -} - -// TestChownIsNoopWhenUnprivileged covers the guard that lets the migration run -// as an ordinary user, where there is no ownership to correct and every chown -// would fail. -func TestChownIsNoopWhenUnprivileged(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("asserts the unprivileged path") - } - - dir := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(dir, "file"), []byte("x"), 0640)) - - require.NoError(t, chown(dir, nobodyUID, nobodyGID)) - require.NoError(t, chownTree(dir, nobodyUID, nobodyGID)) - - uid, _, err := dirOwner(dir) - require.NoError(t, err) - assert.Equal(t, os.Geteuid(), uid, "ownership must be left alone") -} - -// TestChownTreeCoversNestedContents guards the recursive case: raft recovery -// writes a snapshot directory and badger segments below the node dir. -func TestChownTreeCoversNestedContents(t *testing.T) { - if os.Geteuid() != 0 { - t.Skip("chown to another user requires root") - } - - dir := t.TempDir() - nested := filepath.Join(dir, "snapshots", "1-2-3") - require.NoError(t, os.MkdirAll(nested, 0750)) - require.NoError(t, os.WriteFile(filepath.Join(nested, "state.bin"), []byte("x"), 0640)) - - require.NoError(t, chownTree(dir, nobodyUID, nobodyGID)) - - for _, path := range []string{dir, nested, filepath.Join(nested, "state.bin")} { - uid, gid, err := dirOwner(path) - require.NoError(t, err) - assert.Equal(t, nobodyUID, uid, path) - assert.Equal(t, nobodyGID, gid, path) - } -}