carlos / platform Public

review fixes: boot-safe group ordering, converging chowns, only-our-child chmod guard

Opus review findings on the #103 branch, all applied:
- groupadd runs before the tmpfiles conf lands, and a sysusers.d snippet
  (g carlos-sockets -) makes the conf's group resolve at boot even when it
  arrived via a staged-tree unit refresh — systemd-tmpfiles DROPS a line
  whose group is unknown, and carlos-edge's ReadWritePaths=/run/carlos
  (no '-') would then fail the unit forever at the next reboot.
- privilegedPlan chowns converge toward the target: /run/carlos recursively
  carlos:carlos-sockets (a re-run is the documented enrollment path and
  must not reset live sockets' group), apps/ back to carlos:carlos after.
- Start skips normalization when the child exited during the health wait
  (squatter answered — not our socket to widen) and Lstat-guards the chmod
  against planted non-sockets; both pinned by real-spawn tests.
- Consumer users validated before any mutation; runPrivileged guards keyed
  off argv, not package constants.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QytpMG8wVhBmWwz3uohtc7
Paul Campbell 5e64f881e3aef9ddf7dfa78647ab87352a7ba967 parent ab4ac51
5 files changed, +260 −46
  • docs/superpowers/specs/2026-08-08-instance-socket-consumers-design.md +45 −15
  • internal/activator/activator_test.go +110 −2
  • internal/activator/backend_exec.go +20 −8
  • internal/host/bootstrap.go +64 −19
  • internal/host/host_test.go +21 −2
diff --git a/docs/superpowers/specs/2026-08-08-instance-socket-consumers-design.md b/docs/superpowers/specs/2026-08-08-instance-socket-consumers-design.md
index 33aab1a96..08616e3a9 100644
--- a/docs/superpowers/specs/2026-08-08-instance-socket-consumers-design.md
+++ b/docs/superpowers/specs/2026-08-08-instance-socket-consumers-design.md
@@ -43,7 +43,27 @@ So the blessed consumers group is a dedicated system group,
`-socket-consumer <user>` flag runs `usermod -aG carlos-sockets <user>` on a
live root (printed under a staging root). Idempotent — `usermod -aG` on an
existing member is a no-op, so re-running bootstrap with the same flags
-converges. A missing user fails loudly via usermod's own error.
+converges. Consumer users are validated (`user.Lookup`) before any mutation:
+a typo leaves the box untouched rather than aborting bootstrap halfway.
+
+**Boot-ordering safety** (review finding, empirically verified):
+systemd-tmpfiles *drops* a line whose group doesn't resolve, and
+`carlos-edge.service` has `ReadWritePaths=/run/carlos` with no `-` prefix —
+so a tmpfiles line naming a group that doesn't exist yet turns the next
+reboot into a permanent edge-start failure. Two mechanisms close this:
+on a live root the privileged plan (including `groupadd`) runs *before* the
+tmpfiles conf is written, and bootstrap also ships
+`/etc/sysusers.d/carlos.conf` (`g carlos-sockets -`) —
+`systemd-sysusers.service` is ordered `Before=systemd-tmpfiles-setup.service`,
+so the group exists at boot even when the conf arrived via a staged-tree
+unit refresh that never ran a live bootstrap.
+
+**Re-run convergence** (review finding): every chown in the privileged plan
+converges *toward* the target state — `/run/carlos` is recursively
+`carlos:carlos-sockets` (so a re-run, the documented enrollment path, also
+converges live instance sockets' group instead of resetting it and
+stranding already-enrolled consumers), with `apps/` put back to
+`carlos:carlos` right after.
**Scope of the grant, stated plainly:** membership in `carlos-sockets` is
box-wide — a member may dial *every* exec-backed instance socket on the box.
@@ -65,9 +85,12 @@ state #103 asks us out of.
So: **`ExecBackend.Start` chmods the instance socket to `0660` immediately
after the health wait succeeds.** Properties that make this the right spot:
-- It runs only for children the activator itself spawned — it can never
- touch a socket an external process is serving (the #17 external-socket
- advisory's territory).
+- It runs only on a socket the activator's own child bound — enforced, not
+ assumed: the health wait tolerates a squatter (it only dials), so `Start`
+ skips normalization when the child has already exited (the lost-bind-race
+ shape — that socket is the #17 external-socket advisory's territory), and
+ an `Lstat` confirms the path is a socket before the chmod, so a planted
+ symlink can never redirect the mode change elsewhere.
- The agent and the child run as the same user, so the chmod is on a socket
we own; failure is essentially impossible, and is logged loudly rather
than failing the wake (a box with no consumers must not lose a healthy
@@ -111,20 +134,27 @@ No registry, wire, or storage shape changes. No console surface.
## Deployment notes
-Per box, after adopting a binary with this change:
+Per box, after adopting a binary with this change. **Step 1 starts a
+consumer outage window** on a box using the interim grant: the moment
+`/run/carlos`'s group flips to `carlos-sockets`, a running consumer whose
+access came from group `carlos` (supplementary groups are fixed at process
+start) loses traversal — so run steps 1–3 as one uninterrupted sequence and
+the window is seconds.
1. Re-run `carlos bootstrap -socket-consumer <user> ...` (live root). On
**tito**: enroll the titogo router/relay users (the ones titogo#535 put
- in group `carlos`); afterwards remove them from group `carlos`
- (`gpasswd -d <user> carlos`) — the interim grant carries /data read and
- the new group replaces it. On **flagship**: no consumers today; plain
- re-run just converges the dir.
-2. `systemctl restart` the consumer processes (group membership is read at
- process start).
-3. Already-live instance sockets keep their old group/mode until their next
- wake; converge immediately with one `chgrp carlos-sockets /run/carlos/*.sock
- && chmod 0660 /run/carlos/*.sock`, or just let the next hibernate/wake
- cycle do it.
+ in group `carlos`). On **flagship**: no consumers today; a plain re-run
+ just converges the dir. Bootstrap's recursive chown also converges the
+ group on every live instance socket.
+2. Converge live sockets' *mode* (group is already done):
+ `chmod g+rw /run/carlos/*.sock` — or skip and let each instance's next
+ wake normalize it.
+3. `systemctl restart` the consumer processes (picks up the new group
+ membership; ends the outage window).
+4. Only then revoke the interim grant: `gpasswd -d <user> carlos` per
+ enrolled user, plus a restart of each — the group-`carlos` membership
+ carries /data group-read (every instance DB and admin token) and the new
+ group replaces it.
Follow-ups owed elsewhere: website docs batch (standing rule) — the
app-author contract line belongs on the public app-contract page when that
diff --git a/internal/activator/activator_test.go b/internal/activator/activator_test.go
index 59e9566d5..e861d928a 100644
--- a/internal/activator/activator_test.go
+++ b/internal/activator/activator_test.go
@@ -3071,7 +3071,8 @@ func TestMain(m *testing.M) {
// fakeAppMain is the app half: bind the --socket, tighten it to 0600 — the
// exact titogo habit that stranded the sibling router in issue #103 — and
-// serve 200s until killed.
+// serve 200s until killed (500s under ACTIVATOR_FAKE_APP_UNHEALTHY, the
+// never-becomes-healthy app).
func fakeAppMain() {
var socket string
for i, a := range os.Args {
@@ -3089,8 +3090,12 @@ func fakeAppMain() {
if err := os.Chmod(socket, 0o600); err != nil {
os.Exit(1)
}
+ status := http.StatusOK
+ if os.Getenv("ACTIVATOR_FAKE_APP_UNHEALTHY") == "1" {
+ status = http.StatusInternalServerError
+ }
_ = http.Serve(ln, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
- w.WriteHeader(http.StatusOK)
+ w.WriteHeader(status)
}))
}
@@ -3140,3 +3145,106 @@ func TestExecBackendStartNormalizesSocketMode(t *testing.T) {
t.Fatalf("socket mode after healthy Start = %o, want 660 — an app's own chmod must not strand a sibling consumer", fi.Mode().Perm())
}
}
+
+// TestExecBackendFailedWakeDoesNotNormalizeSocketMode pins the other half of
+// the contract: normalization happens only AFTER a successful health wait —
+// a wake that fails must leave the socket exactly as the app left it. The
+// fake app binds and tightens to 0600 but never answers healthily.
+func TestExecBackendFailedWakeDoesNotNormalizeSocketMode(t *testing.T) {
+ requireUnixSockets(t)
+ b := testExecBackend(t)
+ host := "unhealthy.example.com"
+ stageServingLive(t, b, host)
+ t.Setenv("ACTIVATOR_FAKE_APP_UNHEALTHY", "1")
+
+ old := spawnHealthTimeout
+ spawnHealthTimeout = 500 * time.Millisecond
+ defer func() { spawnHealthTimeout = old }()
+
+ rt := registry.Route{Host: host, Kind: "instance", Socket: tmpSock(t)}
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ if proc, _, err := b.Start(ctx, rt); err == nil {
+ defer proc.Stop(context.Background())
+ t.Fatal("Start succeeded for a never-healthy app")
+ }
+ fi, err := os.Stat(rt.Socket)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if fi.Mode().Perm() != 0o600 {
+ t.Fatalf("socket mode after failed wake = %o, want the app's own 600 — a failed wake must not widen anything", fi.Mode().Perm())
+ }
+}
+
+// TestExecBackendDoesNotNormalizeSquatterSocket pins the only-our-child
+// guard: a stray supervisor squatting the route's socket (the #17 advisory's
+// subject) can answer the health probe while the spawned child — which lost
+// the bind race and exited — cannot. Start must not widen the squatter's
+// socket. The squatter here holds its 200 until the child has verifiably
+// exited, making the race the guard closes deterministic.
+func TestExecBackendDoesNotNormalizeSquatterSocket(t *testing.T) {
+ requireUnixSockets(t)
+ b := testExecBackend(t)
+ host := "squatted.example.com"
+ marker := filepath.Join(t.TempDir(), "child-ran")
+ // The child: proof-of-life marker, then immediate exit — the shape of a
+ // child that could not bind.
+ script := "#!/bin/sh\ntouch '" + marker + "'\nexit 0\n"
+ vdir := filepath.Join(b.OptDir, "versions", "v1")
+ if err := os.MkdirAll(vdir, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ bin := filepath.Join(vdir, "app")
+ if err := os.WriteFile(bin, []byte(script), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.MkdirAll(filepath.Join(b.OptDir, "live"), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Symlink(bin, b.liveLink(host)); err != nil {
+ t.Fatal(err)
+ }
+
+ sock := tmpSock(t)
+ ln, err := net.Listen("unix", sock)
+ if err != nil {
+ t.Fatal(err)
+ }
+ defer ln.Close()
+ if err := os.Chmod(sock, 0o600); err != nil {
+ t.Fatal(err)
+ }
+ srv := &http.Server{Handler: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+ // Answer only once the child is provably dead: marker written, plus
+ // a grace for the exit itself to be reaped.
+ deadline := time.Now().Add(5 * time.Second)
+ for time.Now().Before(deadline) {
+ if _, err := os.Stat(marker); err == nil {
+ break
+ }
+ time.Sleep(10 * time.Millisecond)
+ }
+ time.Sleep(300 * time.Millisecond)
+ w.WriteHeader(http.StatusOK)
+ })}
+ go srv.Serve(ln)
+ defer srv.Close()
+
+ rt := registry.Route{Host: host, Kind: "instance", Socket: sock}
+ ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
+ defer cancel()
+ proc, _, err := b.Start(ctx, rt)
+ if err != nil {
+ t.Fatalf("Start: %v (the squatter answers healthily; the wake itself succeeds)", err)
+ }
+ defer proc.Stop(context.Background())
+
+ fi, err := os.Stat(rt.Socket)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if fi.Mode().Perm() != 0o600 {
+ t.Fatalf("squatter socket mode after Start = %o, want its own 600 — a socket our child did not bind is never ours to widen", fi.Mode().Perm())
+ }
+}
diff --git a/internal/activator/backend_exec.go b/internal/activator/backend_exec.go
index a3916b7ca..c8514652e 100644
--- a/internal/activator/backend_exec.go
+++ b/internal/activator/backend_exec.go
@@ -494,14 +494,26 @@ func (b *ExecBackend) Start(ctx context.Context, rt registry.Route) (Proc, strin
// see host.tmpfilesConf) can dial, no matter what the child's umask or
// its own post-bind chmod produced — titogo tightened its socket to 0600
// and stranded the sibling router that proxies to it. After the health
- // wait, so the app's own bind/chmod has already happened and this wins;
- // and only here, on a child THIS backend spawned — an externally-served
- // socket (the #17 advisory's territory) is never touched. On a box with
- // no consumers group the socket's group is just `carlos`, so the wider
- // mode grants nobody anything. Logged, never fatal: a box with no
- // consumers must not lose a healthy wake to a normalization it doesn't
- // need.
- if err := os.Chmod(rt.Socket, 0o660); err != nil {
+ // wait, so the app's own bind/chmod has already happened and this wins.
+ //
+ // The two guards make "only a socket THIS backend's child bound" true
+ // rather than merely intended. waitHealthy tolerates a squatter (its own
+ // contract — it only dials), so a child that lost the bind race with a
+ // stray supervisor (EADDRINUSE, exits) can still see the health wait
+ // succeed off the SQUATTER's answers; a dead child means the socket is
+ // not ours to widen (that route is the #17 external-socket advisory's
+ // territory). The Lstat keeps the chmod from ever following a
+ // non-socket — a symlink planted at the route's path must not become a
+ // mode change somewhere else. On a box with no consumers group the
+ // socket's group is just `carlos`, so the wider mode grants nobody
+ // anything. All three branches log and never fail the wake: a box with
+ // no consumers must not lose a healthy wake to a normalization it
+ // doesn't need.
+ if p.Exited() {
+ log.Printf("activator: %s: child exited during the health wait (external process serving its socket?) — not normalizing the socket mode", host)
+ } else if fi, statErr := os.Lstat(rt.Socket); statErr != nil || fi.Mode()&os.ModeSocket == 0 {
+ log.Printf("activator: %s: %s is not a socket we can normalize (stat: %v)", host, rt.Socket, statErr)
+ } else if err := os.Chmod(rt.Socket, 0o660); err != nil {
log.Printf("activator: normalize socket mode for %s: %v (enrolled consumers would get permission denied)", host, err)
}
return p, spawnVer, nil
diff --git a/internal/host/bootstrap.go b/internal/host/bootstrap.go
index a4dfebe9d..cf5cb53d0 100644
--- a/internal/host/bootstrap.go
+++ b/internal/host/bootstrap.go
@@ -174,6 +174,17 @@ func bootstrapOffcloud(root string, consumers []string) error {
// steps printed instead of run.
func bootstrap(root string, consumers []string) error {
live := filepath.Clean(root) == "/"
+ if live {
+ // Validate consumer users BEFORE any mutation (the offcloud
+ // precondition's discipline): a typo in -socket-consumer must leave
+ // the box untouched, not abort bootstrap halfway with units written
+ // but timers never armed.
+ for _, u := range consumers {
+ if _, err := user.Lookup(u); err != nil {
+ return fmt.Errorf("-socket-consumer %s: no such user (create the host app's user first): %w", u, err)
+ }
+ }
+ }
unitDir := filepath.Join(root, "etc", "systemd", "system")
dataDir := filepath.Join(root, "data")
runDir := filepath.Join(root, "run", "carlos")
@@ -204,6 +215,7 @@ func bootstrap(root string, consumers []string) error {
{filepath.Join(root, "etc", "carlos", "apps"), 0o755},
{unitDir, 0o755},
{filepath.Join(root, "etc", "tmpfiles.d"), 0o755},
+ {filepath.Join(root, "etc", "sysusers.d"), 0o755},
}
for _, d := range dirs {
if err := os.MkdirAll(d.path, d.mode); err != nil {
@@ -232,6 +244,35 @@ func bootstrap(root string, consumers []string) error {
fmt.Printf(" wrote %s\n", path)
}
+ // On a live root the privileged plan runs BEFORE the tmpfiles conf is
+ // written: the conf's /run/carlos line names socketsGroup, and
+ // systemd-tmpfiles DROPS a line whose group doesn't resolve — so a
+ // bootstrap interrupted between "conf written" and "groupadd ran" would
+ // leave a box that looks healthy until its next reboot, when /run/carlos
+ // simply isn't created and carlos-edge (ReadWritePaths=/run/carlos, no
+ // "-" prefix) fails its namespace setup forever. Group first, then the
+ // conf that names it.
+ if live {
+ for _, args := range privilegedPlan(consumers) {
+ if err := runPrivileged(args); err != nil {
+ return err
+ }
+ }
+ }
+
+ // The sysusers.d snippet closes the same hole for confs that arrive
+ // WITHOUT a live bootstrap run: a staging root is how a live box
+ // refreshes its unit files (pinfra's update-platform-binary.sh), and a
+ // refresh that installs the staged etc/ tree ships the tmpfiles line to
+ // a box that never ran groupadd. systemd-sysusers.service is ordered
+ // Before=systemd-tmpfiles-setup.service, so the group exists by the time
+ // the tmpfiles line is parsed, however the conf got there.
+ sysusersPath := filepath.Join(root, "etc", "sysusers.d", "carlos.conf")
+ if err := os.WriteFile(sysusersPath, []byte("g "+socketsGroup+" -\n"), 0o644); err != nil {
+ return fmt.Errorf("write %s: %v", sysusersPath, err)
+ }
+ fmt.Printf(" wrote %s\n", sysusersPath)
+
tmpfilesPath := filepath.Join(root, "etc", "tmpfiles.d", "carlos.conf")
if err := os.WriteFile(tmpfilesPath, []byte(tmpfilesConf), 0o644); err != nil {
return fmt.Errorf("write %s: %v", tmpfilesPath, err)
@@ -254,11 +295,6 @@ func bootstrap(root string, consumers []string) error {
return nil
}
- for _, args := range privilegedPlan(consumers) {
- if err := runPrivileged(args); err != nil {
- return err
- }
- }
for _, args := range systemctlPlan(enabledUnits) {
if err := systemctl(args...); err != nil {
return err
@@ -288,22 +324,27 @@ func unitNames() ([]string, error) {
// issue #103 added the consumers group): the two can never drift into
// describing different behaviour.
//
-// The sequence: service user, consumers group, ownership (the recursive
-// chown gives serviceUser everything it reads/writes — and resets
-// /run/carlos's group in passing, which is why the dir-only chown to the
-// consumers group comes AFTER it, never before). The chmod re-asserts the
-// setgid bit the tmpfiles line promises; consumers enroll last, one usermod
-// each (`-aG` is append — an existing member is a no-op, so re-runs
-// converge). It deliberately does NOT touch group `carlos` memberships: an
-// interim grant made there (the tito workaround this replaces) is the
-// operator's to revoke, per the deployment notes in the #103 spec.
+// The sequence: service user, consumers group, ownership. Every chown
+// converges TOWARD the target state, never through an intermediate that
+// undoes a previous run's work: /run/carlos is recursively
+// serviceUser:socketsGroup — recursive on purpose, so a re-run (the
+// documented way to enroll another consumer) also converges the group on
+// every LIVE instance socket instead of resetting it to serviceUser's group
+// and stranding the already-enrolled consumers until each instance rebinds —
+// and apps/ (unit-tenant sockets, systemd-bound, no path consumers) is then
+// put back to serviceUser:serviceUser. The chmod re-asserts the setgid bit
+// the tmpfiles line promises; consumers enroll last, one usermod each
+// (`-aG` is append — an existing member is a no-op, so re-runs converge).
+// It deliberately does NOT touch group `carlos` memberships: an interim
+// grant made there (the tito workaround this replaces) is the operator's to
+// revoke, per the deployment notes in the #103 spec.
func privilegedPlan(consumers []string) [][]string {
plan := [][]string{
{"useradd", "--system", "--user-group", "--no-create-home", "--shell", "/usr/sbin/nologin", serviceUser},
{"groupadd", "--system", socketsGroup},
{"chown", "-R", serviceUser + ":" + serviceUser, "/data"},
- {"chown", "-R", serviceUser + ":" + serviceUser, "/run/carlos"},
- {"chown", serviceUser + ":" + socketsGroup, "/run/carlos"},
+ {"chown", "-R", serviceUser + ":" + socketsGroup, "/run/carlos"},
+ {"chown", "-R", serviceUser + ":" + serviceUser, "/run/carlos/apps"},
{"chmod", "2750", "/run/carlos"},
}
for _, u := range consumers {
@@ -318,13 +359,17 @@ func privilegedPlan(consumers []string) [][]string {
// bootstrap's normal re-run state, not an error. Everything else in the plan
// is idempotent as-is.
func runPrivileged(args []string) error {
- switch args[0] {
+ // The subject is derived from the command line itself (both commands
+ // take it as their final argument), never from a package constant — a
+ // guard keyed to a constant would silently skip any SECOND useradd or
+ // groupadd a later plan revision adds.
+ switch subject := args[len(args)-1]; args[0] {
case "useradd":
- if _, err := user.Lookup(serviceUser); err == nil {
+ if _, err := user.Lookup(subject); err == nil {
return nil
}
case "groupadd":
- if _, err := user.LookupGroup(socketsGroup); err == nil {
+ if _, err := user.LookupGroup(subject); err == nil {
return nil
}
}
diff --git a/internal/host/host_test.go b/internal/host/host_test.go
index e365770cd..5734e25d2 100644
--- a/internal/host/host_test.go
+++ b/internal/host/host_test.go
@@ -684,6 +684,19 @@ func TestBootstrapTmpfilesBlessesConsumersGroup(t *testing.T) {
if afi.Mode()&os.ModeSetgid != 0 {
t.Errorf("/run/carlos/apps mode %v: setgid must not leak onto the tenant socket dir", afi.Mode())
}
+
+ // The sysusers.d snippet is what makes the tmpfiles line safe at boot on
+ // a box whose conf arrived via a staged-tree refresh (never a live
+ // groupadd): systemd-sysusers runs Before=systemd-tmpfiles-setup, and a
+ // tmpfiles line whose group doesn't resolve is DROPPED — /run/carlos then
+ // isn't created and carlos-edge (ReadWritePaths, no "-") can't start.
+ su, err := os.ReadFile(filepath.Join(root, "etc", "sysusers.d", "carlos.conf"))
+ if err != nil {
+ t.Fatalf("sysusers conf: %v", err)
+ }
+ if want := "g carlos-sockets -"; !strings.Contains(string(su), want) {
+ t.Errorf("sysusers conf %q missing %q", su, want)
+ }
}
// TestBootstrapPrivilegedPlanEnrollsConsumers pins the printed-equals-run
@@ -694,12 +707,18 @@ func TestBootstrapTmpfilesBlessesConsumersGroup(t *testing.T) {
// dir convergence always run.
func TestBootstrapPrivilegedPlanEnrollsConsumers(t *testing.T) {
got := privilegedPlan([]string{"titogo", "titogo-relay"})
+ // The /run/carlos chown is recursive AND targets the consumers group: a
+ // re-run is the documented enrollment path, and a recursive chown back
+ // to carlos:carlos would reset every live instance socket's group,
+ // stranding already-enrolled consumers until each instance rebinds.
+ // apps/ is put back to carlos:carlos right after (unit tenants stay out
+ // of consumer reach).
want := [][]string{
{"useradd", "--system", "--user-group", "--no-create-home", "--shell", "/usr/sbin/nologin", "carlos"},
{"groupadd", "--system", "carlos-sockets"},
{"chown", "-R", "carlos:carlos", "/data"},
- {"chown", "-R", "carlos:carlos", "/run/carlos"},
- {"chown", "carlos:carlos-sockets", "/run/carlos"},
+ {"chown", "-R", "carlos:carlos-sockets", "/run/carlos"},
+ {"chown", "-R", "carlos:carlos", "/run/carlos/apps"},
{"chmod", "2750", "/run/carlos"},
{"usermod", "-aG", "carlos-sockets", "titogo"},
{"usermod", "-aG", "carlos-sockets", "titogo-relay"},