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) + } +}