Reapply "fix(migrate): keep predastore's data dir owned by its service user"

This reverts commit 317f714355.

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

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

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