From 576742dfebc7078030483c0dfdced2acc2bcd9c2 Mon Sep 17 00:00:00 2001 From: brynmailer-mdc Date: Mon, 3 Aug 2026 12:44:10 +1000 Subject: [PATCH] fix(migrate): keep predastore's data dir owned by its service user The v4 to v5 topology migration runs as root under `spx admin upgrade`, but predastore runs as spinifex-storage with an empty capability bounding set. Two paths left it locked out of its own state. The migration created the cluster data dir as root:root 0750, so spinifex-storage could not traverse into the node dirs the rename had placed under it, and clusterrun could not create new ones. Separately, the raft recovery that follows each state replica move reopens badger and writes a fresh snapshot as root, leaving root-owned files inside a node dir that the rename had otherwise left correctly owned. Both now take their ownership from the data being moved rather than from a service user name, so a dev install running as the invoking user needs no special case. The data dir chown is unconditional so a re-run repairs a dir left behind by an earlier failed run, and the tree chown runs even when there was nothing to recover, since that path still opens the stores. --- .../004_predastore_topology.go | 32 ++++++- .../migrate/predastoretopology/ownership.go | 66 +++++++++++++ .../predastoretopology/ownership_test.go | 94 +++++++++++++++++++ 3 files changed, 190 insertions(+), 2 deletions(-) create mode 100644 spinifex/migrate/predastoretopology/ownership.go create mode 100644 spinifex/migrate/predastoretopology/ownership_test.go diff --git a/spinifex/migrate/predastoretopology/004_predastore_topology.go b/spinifex/migrate/predastoretopology/004_predastore_topology.go index ef4844441..e5eb82aa7 100644 --- a/spinifex/migrate/predastoretopology/004_predastore_topology.go +++ b/spinifex/migrate/predastoretopology/004_predastore_topology.go @@ -261,6 +261,13 @@ 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. @@ -268,6 +275,13 @@ 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 @@ -292,9 +306,23 @@ 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) } - if err := os.MkdirAll(filepath.Dir(m.To), 0750); err != nil { - return false, fmt.Errorf("create %s: %w", filepath.Dir(m.To), err) + // 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 } + + 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 new file mode 100644 index 000000000..15b1c0ccd --- /dev/null +++ b/spinifex/migrate/predastoretopology/ownership.go @@ -0,0 +1,66 @@ +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 new file mode 100644 index 000000000..eba53b515 --- /dev/null +++ b/spinifex/migrate/predastoretopology/ownership_test.go @@ -0,0 +1,94 @@ +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) + } +}