amadan
isolated exec: plan 2 final-review fixes (probe cgroup-ro, seed template, control dir hidden, seed cleanup, probe names)
Six items from the final whole-branch review of plan 2 (ledger: .superpowers/sdd/2026-09-11-isolated-exec-2-control/progress.md, "Final review (fable) on 08a0ed8"). 1. The cgroup-ro probe check targeted the probe's own cgroup.procs, root 0644: the kernel checks DAC before the read-only mount flag, so uid carlos got EACCES on a hardened box whether or not ProtectControlGroups=yes was in force, and /ready never recovered. It now targets the edge's Delegate=-chowned cgroup.procs (EROFS is then evidence; EACCES means not delegated, ENOENT means the edge is not loaded) and a new cgroup-ro-mount line asserts /sys/fs/cgroup itself carries the ro mount option. 2. The seed unit template lost KillMode=mixed and TimeoutStopSec=20 somewhere in an earlier round; restored, matching the run template. 3. /run/carlos-control was reachable from inside a nested tenant's namespace (a connect would succeed; only the peer check refused it). InaccessiblePaths= in the shared namespace block now covers it, and a new control-unreachable probe line asserts an Lstat from inside fails. 4. /seed's cleanup stop ran on the request context: a disconnected client left the oneshot's cgroup running behind a response nobody read, and the next /seed could read a stale result. Every stop now runs on a context detached from the request, and the handler compares InvocationID before and after start to detect and clear an abandoned run before reading its result. 5. The probe's symlink check used fixed names in a tenant-shared directory. It now plants both the target and the link under random 16-hex names via instancefs.OpenBeneath, and refuses (without removing) a colliding name. 6. Three cheap minors: productionPeerOK's user.Lookup/Atoi failure is now log.Fatalf instead of cached forever (socket activation restarts on the next connection); /ready before the first probe names the reason instead of an empty Lines; the exit record is created 0600. Spec bumped to revision 13: §8.5 names the edge's delegated cgroup.procs and the ro mountinfo line, §5.2 names InaccessiblePaths= as what hides the control socket, §5.3's /seed row describes the new order, and §13 gets one review-log line.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YZ1ZvWj7naf73GjTX8MBv5
16 files changed,
+510
−64
docs/superpowers/specs/2026-09-11-isolated-exec-design.md+18 −8internal/host/units/carlos-instance-probe.service+6 −2internal/host/units/carlos-instance-seed@.service+10 −2internal/host/units/carlos-instance-sidecar@.service+6 −2internal/host/units/carlos-instance@.service+6 −2internal/host/units_isolation_test.go+2 −1internal/instancectl/peer_linux.go+12 −2internal/instancectl/server.go+60 −12internal/instancectl/server_test.go+140 −0internal/instanceexec/exit.go+5 −1internal/instanceexec/exit_test.go+6 −0internal/instanceexec/probe.go+119 −17internal/instanceexec/probe_test.go+73 −1internal/instancefs/instancefs_linux.go+19 −11internal/instancefs/instancefs_linux_test.go+23 −2internal/instancefs/instancefs_other.go+5 −1
diff --git a/docs/superpowers/specs/2026-09-11-isolated-exec-design.md b/docs/superpowers/specs/2026-09-11-isolated-exec-design.md| index 463dea34d..33dac7ded 100644 |
| --- a/docs/superpowers/specs/2026-09-11-isolated-exec-design.md |
| +++ b/docs/superpowers/specs/2026-09-11-isolated-exec-design.md |
| @@ -1,9 +1,10 @@ |
| # Isolated exec — every exec-backed instance in its own namespace |
| -**Date:** 2026-09-11 · **Status:** revision 12; codex verdict after |
| +**Date:** 2026-09-11 · **Status:** revision 13; codex verdict after |
| round 11 is **accept with corrections**, and the one correction is |
| -applied here (54 findings over 11 rounds, all accepted; §13); awaiting |
| -Paul's review · **Requested by:** discussion |
| +applied here (54 findings over 11 rounds, all accepted; §13); a |
| +whole-branch final review (plan 2) found six further items, fixed in |
| +revision 13 (§13); awaiting Paul's review · **Requested by:** discussion |
| #126 (Tito, with box evidence), approved in direction by Paul on |
| 2026-09-11 |
| @@ -876,9 +877,11 @@ MemoryMax=64M |
| The socket lives in `/run/carlos-control`, `0750 root:carlos`: any |
| `carlos` process can connect (search on the directory), none can create, |
| rename or replace anything in it. The unit is socket-activated and runs |
| -only while there is work. It is not in any instance's namespace: |
| +only while there is work. It is hidden from every instance's namespace |
| +by `InaccessiblePaths=` in the shared block (§4.2): a connect from |
| +inside one is impossible, and the peer check refuses everything else. |
| `/run/carlos` inside an instance is an empty tmpfs with the instance's |
| -own directory bound in, and `/run/carlos-control` is not under it. |
| +own directory bound in. |
| ### 5.3 Protocol |
| @@ -892,7 +895,7 @@ itself: |
| | Verb | What it runs | Returns | Agent deadline | |
| |---|---|---|---| |
| | `POST /start?host=H&role=run\|sidecar&memory_max=N` | `systemctl set-property --runtime carlos-instances-<esc>.slice MemoryMax=N MemoryHigh=0.9N MemorySwapMax=N`; `systemctl start <unit>` (blocking: the start job completes) | 200 with `{invocation_id}` from `systemctl show -p InvocationID <unit>`, read immediately after the start returns; or 502 with systemctl's stderr | 30s | |
| -| `POST /seed?host=H&memory_max=N` | the same `set-property`; `systemctl start` on `carlos-instance-seed@<esc>.service` (blocking: a oneshot's start job completes when `ExecStart` exits, or fails at `TimeoutStartSec`); the unit is now `active (exited)` or `failed` and **stays loaded** (`RemainAfterExit=yes`, §4.2); `systemctl show -p InvocationID,Result,ExecMainStatus,ExecMainCode`; wait for the cgroup to be empty (§6.5's rule, applied by the service as root); `systemctl stop` (instant for an exited oneshot; clears it) | 200 with `{invocation_id, result, status, code}`, always complete | 150s: 120s seed budget plus `TimeoutStopSec` and slack (R3-5) | |
| +| `POST /seed?host=H&memory_max=N` | the same `set-property`; `systemctl start` on `carlos-instance-seed@<esc>.service` (blocking: a oneshot's start job completes when `ExecStart` exits, or fails at `TimeoutStartSec`); if the InvocationID it left is the SAME one that was running before this start, an earlier request's stop never ran, so the stale unit is stopped and started again; the unit is now `active (exited)` or `failed` and **stays loaded** (`RemainAfterExit=yes`, §4.2); `systemctl show -p InvocationID,Result,ExecMainStatus,ExecMainCode` reads the retained result; `systemctl stop` (instant for an exited oneshot; clears it), on a context detached from the request so a disconnected client cannot leave it running; then wait for the cgroup to be empty (§6.5's rule, applied by the service as root) | 200 with `{invocation_id, result, status, code}`, always complete | 150s: 120s seed budget plus `TimeoutStopSec` and slack (R3-5) | |
| | `POST /stop?host=H&role=run\|seed\|sidecar` | `systemctl stop <unit>` (blocking; also cancels a pending start job) | 204 | 30s | |
| | `POST /reset?host=H&role=…` | `systemctl reset-failed <unit>`, tolerating "not loaded" | 204 | 30s | |
| | `POST /sweep` | `systemctl list-units --all` and `list-jobs` over the three templates' instances; `systemctl stop` each that is not `inactive` or has a job; wait for terminal state and empty cgroup on each | 200 with the list of unit names swept, or 502 naming the unit that would not terminate | 60s | |
| @@ -1662,8 +1665,14 @@ verdict line: |
| - `/data` and `/run/carlos` list nothing but the probe's own bound |
| directories, and `/var/cache` and `/var/lib` list nothing at all; |
| -- a write to `cgroup.kill` under any cgroup directory it can name |
| - (its own included) fails with `EROFS`; |
| +- `access(W_OK)` on the edge's own delegated `cgroup.procs` |
| + (`/sys/fs/cgroup/carlos.slice/carlos-edge.service/cgroup.procs`, chowned |
| + to carlos by `Delegate=` on `carlos-edge.service`) answers `EROFS`, |
| + never `EACCES` (the kernel checks DAC before the mount flag, so `EACCES` |
| + there means the file is not delegated, not a pass) or `ENOENT` (the edge |
| + unit is not loaded); and `/proc/self/mountinfo` shows `/sys/fs/cgroup` |
| + itself carrying `ro`, so the `EROFS` answer is the mount flag's doing |
| + and not some other cause; |
| - `/etc/carlos` cannot be opened (`EACCES` or `ENOENT`, never a |
| listing), and the fixture `/etc/carlos/probe-secret` cannot be read. |
| The fixture is **not** written by the control service, whose |
| @@ -2106,3 +2115,4 @@ rollback runs the old binary). |
| finding, every prior item resolved, **verdict accept with |
| corrections**). The correction is applied in this revision: |
| R11-1 the probe fixture comes from tmpfiles at bootstrap, and the control service verifies its ownership and mode before accepting a verdict (§3.2, §8.5). |
| +- R12 (final review, plan 2): cgroup-ro targets the edge's delegated cgroup.procs; DAC precedes the mount flag so a root-owned file answers EACCES. |
diff --git a/internal/host/units/carlos-instance-probe.service b/internal/host/units/carlos-instance-probe.service| index 5ab462f10..f8878fe1e 100644 |
| --- a/internal/host/units/carlos-instance-probe.service |
| +++ b/internal/host/units/carlos-instance-probe.service |
| @@ -37,8 +37,12 @@ TemporaryFileSystem=/var/lib:ro |
| # /etc/carlos/relay-masters is root:carlos 0640 for carlos-relay.service, |
| # and an instance's Group=carlos would read it too (codex R10-1). An |
| # instance gets its configuration through its launch file and needs |
| -# nothing from /etc/carlos. |
| -InaccessiblePaths=/etc/carlos |
| +# nothing from /etc/carlos. /run/carlos-control is the root control |
| +# service's socket directory (design §5.2): hiding it here too means a |
| +# connect from inside any instance's namespace is impossible outright, |
| +# not merely refused by the peer check on the far end (final review, |
| +# plan 2, item 3). |
| +InaccessiblePaths=/etc/carlos /run/carlos-control |
| ProtectHome=yes |
| PrivateTmp=yes |
| PrivateDevices=yes |
diff --git a/internal/host/units/carlos-instance-seed@.service b/internal/host/units/carlos-instance-seed@.service| index 50a15df7e..711b51b24 100644 |
| --- a/internal/host/units/carlos-instance-seed@.service |
| +++ b/internal/host/units/carlos-instance-seed@.service |
| @@ -38,6 +38,10 @@ Group=carlos |
| # carries the tier (§4.3, §6.6). Slices referenced here are created on |
| # demand; no per-host unit file exists. |
| Slice=carlos-instances-%i.slice |
| +# SIGTERM to the app, SIGKILL to anything it left behind after |
| +# TimeoutStopSec; the activator's stop budget is 20s. |
| +KillMode=mixed |
| +TimeoutStopSec=20 |
| # The log stream: stdout and stderr connect to the agent's per-role |
| # listener (§6.4). PID 1 opens this as root, outside the namespace, on the |
| # nosymfollow logs mount, at a path no tenant mount contains. |
| @@ -60,8 +64,12 @@ TemporaryFileSystem=/var/lib:ro |
| # /etc/carlos/relay-masters is root:carlos 0640 for carlos-relay.service, |
| # and an instance's Group=carlos would read it too (codex R10-1). An |
| # instance gets its configuration through its launch file and needs |
| -# nothing from /etc/carlos. |
| -InaccessiblePaths=/etc/carlos |
| +# nothing from /etc/carlos. /run/carlos-control is the root control |
| +# service's socket directory (design §5.2): hiding it here too means a |
| +# connect from inside any instance's namespace is impossible outright, |
| +# not merely refused by the peer check on the far end (final review, |
| +# plan 2, item 3). |
| +InaccessiblePaths=/etc/carlos /run/carlos-control |
| ProtectHome=yes |
| PrivateTmp=yes |
| PrivateDevices=yes |
diff --git a/internal/host/units/carlos-instance-sidecar@.service b/internal/host/units/carlos-instance-sidecar@.service| index 711336448..8b65c85f0 100644 |
| --- a/internal/host/units/carlos-instance-sidecar@.service |
| +++ b/internal/host/units/carlos-instance-sidecar@.service |
| @@ -60,8 +60,12 @@ TemporaryFileSystem=/var/lib:ro |
| # /etc/carlos/relay-masters is root:carlos 0640 for carlos-relay.service, |
| # and an instance's Group=carlos would read it too (codex R10-1). An |
| # instance gets its configuration through its launch file and needs |
| -# nothing from /etc/carlos. |
| -InaccessiblePaths=/etc/carlos |
| +# nothing from /etc/carlos. /run/carlos-control is the root control |
| +# service's socket directory (design §5.2): hiding it here too means a |
| +# connect from inside any instance's namespace is impossible outright, |
| +# not merely refused by the peer check on the far end (final review, |
| +# plan 2, item 3). |
| +InaccessiblePaths=/etc/carlos /run/carlos-control |
| ProtectHome=yes |
| PrivateTmp=yes |
| PrivateDevices=yes |
diff --git a/internal/host/units/carlos-instance@.service b/internal/host/units/carlos-instance@.service| index 38ade8b5b..01b3063a1 100644 |
| --- a/internal/host/units/carlos-instance@.service |
| +++ b/internal/host/units/carlos-instance@.service |
| @@ -60,8 +60,12 @@ TemporaryFileSystem=/var/lib:ro |
| # /etc/carlos/relay-masters is root:carlos 0640 for carlos-relay.service, |
| # and an instance's Group=carlos would read it too (codex R10-1). An |
| # instance gets its configuration through its launch file and needs |
| -# nothing from /etc/carlos. |
| -InaccessiblePaths=/etc/carlos |
| +# nothing from /etc/carlos. /run/carlos-control is the root control |
| +# service's socket directory (design §5.2): hiding it here too means a |
| +# connect from inside any instance's namespace is impossible outright, |
| +# not merely refused by the peer check on the far end (final review, |
| +# plan 2, item 3). |
| +InaccessiblePaths=/etc/carlos /run/carlos-control |
| ProtectHome=yes |
| PrivateTmp=yes |
| PrivateDevices=yes |
diff --git a/internal/host/units_isolation_test.go b/internal/host/units_isolation_test.go| index 1fd4a43b2..25dd4d829 100644 |
| --- a/internal/host/units_isolation_test.go |
| +++ b/internal/host/units_isolation_test.go |
| @@ -234,7 +234,8 @@ func TestInstanceTemplatesShareOneNamespace(t *testing.T) { |
| "Slice=carlos-instances-%i.slice", "Restart=no", "KillMode=mixed", "TimeoutStopSec=20", "BindsTo=carlos-edge.service", |
| "Requires=data-instances.mount run-carlos-instances.mount run-carlos-logs.mount", "OOMScoreAdjust=500", "OOMPolicy=kill", "TasksMax=256", "Type=simple"}, |
| "carlos-instance-seed@.service": {"Type=oneshot", "RemainAfterExit=yes", "TimeoutStartSec=120", "ExecStart=/opt/carlos/carlos instance-exec seed %I", |
| - "ExecStopPost=/opt/carlos/carlos instance-exit seed %I", "StandardOutput=file:/run/carlos/logs/%I.seed.sock"}, |
| + "ExecStopPost=/opt/carlos/carlos instance-exit seed %I", "StandardOutput=file:/run/carlos/logs/%I.seed.sock", |
| + "KillMode=mixed", "TimeoutStopSec=20"}, |
| "carlos-instance-sidecar@.service": {"Type=simple", "ExecStart=/opt/carlos/carlos instance-exec sidecar %I", |
| "ExecStopPost=/opt/carlos/carlos instance-exit sidecar %I", "StandardOutput=file:/run/carlos/logs/%I.sidecar.sock"}, |
| "carlos-instance-probe.service": {"Type=oneshot", "ExecStart=/opt/carlos/carlos instance-probe", |
diff --git a/internal/instancectl/peer_linux.go b/internal/instancectl/peer_linux.go| index 1c3190f18..af226bb21 100644 |
| --- a/internal/instancectl/peer_linux.go |
| +++ b/internal/instancectl/peer_linux.go |
| @@ -6,6 +6,7 @@ import ( |
| "context" |
| "errors" |
| "fmt" |
| + "log" |
| "net" |
| "os" |
| "os/exec" |
| @@ -45,11 +46,20 @@ func peerCred(conn net.Conn) (*unix.Ucred, error) { |
| // reach the socket at all. MainPID is asked of systemctl and cached for one |
| // second, because the agent restarts and the pid changes. |
| func productionPeerOK() func(net.Conn) error { |
| + // A missing carlos user or an unparsable uid is fatal at startup, not a |
| + // permanent refusal cached into the returned closure: socket activation |
| + // restarts this service on the very next connection attempt, so a fix |
| + // to the box (the user provisioned, say) takes effect on the next |
| + // accept instead of requiring every already-running process to be |
| + // found and killed by hand (final review, plan 2, item 6). |
| u, err := user.Lookup("carlos") |
| if err != nil { |
| - return func(net.Conn) error { return fmt.Errorf("no carlos user: %v", err) } |
| + log.Fatalf("instance-control: no carlos user: %v", err) |
| + } |
| + carlosUID, err := strconv.Atoi(u.Uid) |
| + if err != nil { |
| + log.Fatalf("instance-control: carlos uid %q: %v", u.Uid, err) |
| } |
| - carlosUID, _ := strconv.Atoi(u.Uid) |
| var mu sync.Mutex |
| var mainPID int |
| var lastErr error |
diff --git a/internal/instancectl/server.go b/internal/instancectl/server.go| index 65a35a256..9558ed1f6 100644 |
| --- a/internal/instancectl/server.go |
| +++ b/internal/instancectl/server.go |
| @@ -287,6 +287,20 @@ func parseShow(out string) map[string]string { |
| return m |
| } |
| +// handleSeed runs the seed oneshot once and returns its retained result. |
| +// Every cleanup `stop` runs on a context detached from the request (never |
| +// ctx itself): a client that disconnects, or whose request context is |
| +// cancelled for any other reason, must not leave the oneshot's cgroup |
| +// running behind a response nobody reads (final review, plan 2, item 4). |
| +// Before starting, and again right after, the unit's InvocationID is |
| +// read: if start left the SAME invocation in place, an earlier request's |
| +// stop never ran (its client context was cancelled before this fix |
| +// existed, or the agent restarted mid-seed), so this handler stops the |
| +// stale run and starts again rather than reading and returning someone |
| +// else's result. The order after that is show (the fields RemainAfterExit |
| +// retains), then stop, then wait for the cgroup to empty: a populated |
| +// cgroup after the wait is still reported as a 502 naming the unit, the |
| +// same as before this reordering. |
| func (s *Server) handleSeed(w http.ResponseWriter, r *http.Request) { |
| req, err := parseRequest(r, nil, true) |
| if err != nil { |
| @@ -299,30 +313,34 @@ func (s *Server) handleSeed(w http.ResponseWriter, r *http.Request) { |
| http.Error(w, "set-property: "+err.Error(), http.StatusBadGateway) |
| return |
| } |
| + |
| // A oneshot's start job completes when ExecStart exits; RemainAfterExit |
| // keeps the unit loaded with its result until the stop below. |
| + invBefore := s.showValue(ctx, "InvocationID", unit) |
| startOut, startErr := s.Systemctl(ctx, "start", unit) |
| + invAfter := s.showValue(ctx, "InvocationID", unit) |
| + if invBefore != "" && invBefore == invAfter { |
| + s.stopDetached(ctx, unit) |
| + startOut, startErr = s.Systemctl(ctx, "start", unit) |
| + if retry := s.showValue(ctx, "InvocationID", unit); retry == "" || retry == invAfter { |
| + http.Error(w, "seed: "+unit+" would not leave its stale invocation "+invAfter, http.StatusBadGateway) |
| + return |
| + } |
| + } |
| + |
| out, err := s.Systemctl(ctx, "show", "-p", "InvocationID,Result,ExecMainStatus,ExecMainCode", unit) |
| if err != nil { |
| http.Error(w, "show: "+err.Error(), http.StatusBadGateway) |
| return |
| } |
| m := parseShow(out) |
| + s.stopDetached(ctx, unit) |
| wctx, cancel := context.WithTimeout(ctx, s.waitBudget()) |
| defer cancel() |
| if err := WaitCgroupEmpty(wctx, s.CgroupRoot, SliceName(req.host), unit, 50*time.Millisecond); err != nil { |
| - // The cgroup never emptied: stop is the action that actually kills |
| - // the survivors, so issue it before reporting the failure rather |
| - // than leaving them running behind a 502. |
| - if _, stopErr := s.Systemctl(ctx, "stop", unit); stopErr != nil { |
| - s.logf("instance-control: %s: stop after seed timeout: %v", req.host, stopErr) |
| - } |
| http.Error(w, "seed: "+err.Error(), http.StatusBadGateway) |
| return |
| } |
| - if _, err := s.Systemctl(ctx, "stop", unit); err != nil { |
| - s.logf("instance-control: %s: stop after seed: %v", req.host, err) |
| - } |
| if startErr != nil && m["Result"] == "" { |
| http.Error(w, "start: "+strings.TrimSpace(startOut+" "+startErr.Error()), http.StatusBadGateway) |
| return |
| @@ -330,6 +348,30 @@ func (s *Server) handleSeed(w http.ResponseWriter, r *http.Request) { |
| writeJSON(w, http.StatusOK, SeedResponse{InvocationID: m["InvocationID"], Result: m["Result"], Status: m["ExecMainStatus"], Code: m["ExecMainCode"]}) |
| } |
| +// showValue reads one systemd property's --value for unit, returning "" on |
| +// any error. It is a probe, not a hard failure path: the caller's own |
| +// later show call is what surfaces a real systemctl error to the client. |
| +func (s *Server) showValue(ctx context.Context, prop, unit string) string { |
| + out, err := s.Systemctl(ctx, "show", "-p", prop, "--value", unit) |
| + if err != nil { |
| + return "" |
| + } |
| + return strings.TrimSpace(out) |
| +} |
| + |
| +// stopDetached issues `systemctl stop` for unit on a context that outlives |
| +// the request: context.WithoutCancel(ctx) keeps ctx's values (so logging |
| +// and tracing still see the request that triggered this) but not its |
| +// cancellation, bounded instead by its own 30-second timeout (final |
| +// review, plan 2, item 4). |
| +func (s *Server) stopDetached(ctx context.Context, unit string) { |
| + dctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) |
| + defer cancel() |
| + if _, err := s.Systemctl(dctx, "stop", unit); err != nil { |
| + s.logf("instance-control: stop %s: %v", unit, err) |
| + } |
| +} |
| + |
| func (s *Server) handleStop(w http.ResponseWriter, r *http.Request) { |
| req, err := parseRequest(r, []Role{RoleRun, RoleSeed, RoleSidecar}, false) |
| if err != nil { |
| @@ -462,12 +504,18 @@ func (s *Server) handleReady(w http.ResponseWriter, r *http.Request) { |
| writeJSON(w, http.StatusServiceUnavailable, rep) |
| } |
| -// Ready returns the last probe verdict. Before the first probe, or when |
| -// the last one could not be read, the box is not ready. |
| +// Ready returns the last probe verdict. Before the first probe has ever |
| +// run, there is no verdict to report, so a synthetic line names that |
| +// reason explicitly rather than answering with an empty Lines (final |
| +// review, plan 2, item 6): a caller reading /ready during startup should |
| +// see why, not a report that looks like every check silently passed. |
| func (s *Server) Ready() (ProbeReport, bool) { |
| s.mu.Lock() |
| defer s.mu.Unlock() |
| - return s.report, s.probed && s.report.Ready |
| + if !s.probed { |
| + return ProbeReport{Lines: []VerdictLine{{Check: "probe", Verdict: "fail", Detail: "not yet run"}}}, false |
| + } |
| + return s.report, s.report.Ready |
| } |
| func (s *Server) waitBudget() time.Duration { |
diff --git a/internal/instancectl/server_test.go b/internal/instancectl/server_test.go| index f8683d879..a79af5173 100644 |
| --- a/internal/instancectl/server_test.go |
| +++ b/internal/instancectl/server_test.go |
| @@ -6,6 +6,7 @@ import ( |
| "errors" |
| "net" |
| "net/http" |
| + "net/http/httptest" |
| "os" |
| "path/filepath" |
| "strings" |
| @@ -192,6 +193,125 @@ func TestSeedFailsWhenTheCgroupStaysPopulated(t *testing.T) { |
| if resp.StatusCode != 502 { |
| t.Errorf("status %d, want 502 while processes remain", resp.StatusCode) |
| } |
| + // stop is issued (on the detached context) before the wait gives up, not |
| + // only after: it is the action that actually kills the survivors (final |
| + // review, plan 2, item 4). |
| + if !strings.Contains(ctl.joined(), "stop carlos-instance-seed@h.example.com.service") { |
| + t.Errorf("stop was not issued while the cgroup stayed populated: %s", ctl.joined()) |
| + } |
| +} |
| + |
| +// TestSeedStopRunsDespiteACancelledClientContext proves the cleanup stop |
| +// uses a context detached from the request: even when the client's |
| +// context is already cancelled by the time systemctl would run "stop", |
| +// the stop call still reaches systemctl with a context that is NOT |
| +// cancelled (final review, plan 2, item 4). A fake that ignored ctx |
| +// entirely could not tell the two cases apart, so this fake asserts on |
| +// the context it is handed. |
| +func TestSeedStopRunsDespiteACancelledClientContext(t *testing.T) { |
| + ctx, cancel := context.WithCancel(context.Background()) |
| + ctl := &fakeCtl{answers: map[string]string{ |
| + "show -p InvocationID,Result,ExecMainStatus,ExecMainCode": "InvocationID=feed\nResult=success\nExecMainStatus=0\nExecMainCode=exited\n", |
| + }} |
| + root := t.TempDir() |
| + stoppedWithLiveContext := false |
| + s := &Server{ |
| + Systemctl: func(c context.Context, args ...string) (string, error) { |
| + out, err := ctl.run(c, args...) |
| + if len(args) > 0 && args[0] == "start" { |
| + cancel() // the client disconnected the instant start answered |
| + } |
| + if len(args) > 0 && args[0] == "stop" && c.Err() == nil { |
| + stoppedWithLiveContext = true |
| + } |
| + return out, err |
| + }, |
| + PeerOK: func(net.Conn) error { return nil }, |
| + CgroupRoot: filepath.Join(root, "cgroup"), |
| + Logf: t.Logf, |
| + } |
| + s.cgroupWaitBudget = time.Second |
| + ev := filepath.Join(s.CgroupRoot, "carlos.slice", "carlos-instances.slice", "carlos-instances-h.example.com.slice", "carlos-instance-seed@h.example.com.service", "cgroup.events") |
| + if err := os.MkdirAll(filepath.Dir(ev), 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := os.WriteFile(ev, []byte("populated 0\n"), 0o644); err != nil { |
| + t.Fatal(err) |
| + } |
| + r := httptest.NewRequest("POST", "/seed?host=h.example.com&memory_max=536870912", nil).WithContext(ctx) |
| + w := httptest.NewRecorder() |
| + s.handleSeed(w, r) |
| + if !stoppedWithLiveContext { |
| + t.Errorf("stop did not run on a context detached from the (cancelled) request:\n%s", ctl.joined()) |
| + } |
| +} |
| + |
| +// TestSeedStopsAndRestartsAStaleInvocation proves the stale-result guard: |
| +// when start leaves the SAME (non-empty) InvocationID in place, the |
| +// handler stops the abandoned run and starts again so it reads its own |
| +// result, never one left behind by a prior request (final review, plan 2, |
| +// item 4). |
| +func TestSeedStopsAndRestartsAStaleInvocation(t *testing.T) { |
| + var mu sync.Mutex |
| + var calls []string |
| + invocation := "stale00000000000000000000000001" |
| + starts := 0 |
| + root := t.TempDir() |
| + systemctl := func(_ context.Context, args ...string) (string, error) { |
| + mu.Lock() |
| + calls = append(calls, strings.Join(args, " ")) |
| + mu.Unlock() |
| + switch args[0] { |
| + case "start": |
| + starts++ |
| + if starts == 2 { |
| + // The retry actually moves the invocation on, as a real |
| + // systemd start would after the stale unit was stopped. |
| + invocation = "fresh0000000000000000000000002" |
| + } |
| + return "", nil |
| + case "show": |
| + if len(args) >= 3 && args[2] == "InvocationID" { |
| + return invocation + "\n", nil |
| + } |
| + return "InvocationID=" + invocation + "\nResult=success\nExecMainStatus=0\nExecMainCode=exited\n", nil |
| + default: |
| + return "", nil |
| + } |
| + } |
| + s := &Server{ |
| + Systemctl: systemctl, |
| + PeerOK: func(net.Conn) error { return nil }, |
| + CgroupRoot: filepath.Join(root, "cgroup"), |
| + Logf: t.Logf, |
| + } |
| + s.cgroupWaitBudget = time.Second |
| + ev := filepath.Join(s.CgroupRoot, "carlos.slice", "carlos-instances.slice", "carlos-instances-h.example.com.slice", "carlos-instance-seed@h.example.com.service", "cgroup.events") |
| + if err := os.MkdirAll(filepath.Dir(ev), 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := os.WriteFile(ev, []byte("populated 0\n"), 0o644); err != nil { |
| + t.Fatal(err) |
| + } |
| + r := httptest.NewRequest("POST", "/seed?host=h.example.com&memory_max=536870912", nil) |
| + w := httptest.NewRecorder() |
| + s.handleSeed(w, r) |
| + if w.Code != http.StatusOK { |
| + t.Fatalf("status %d, body %s", w.Code, w.Body.String()) |
| + } |
| + if starts != 2 { |
| + t.Errorf("starts = %d, want 2 (a stale invocation must trigger a stop and retry)", starts) |
| + } |
| + mu.Lock() |
| + joined := strings.Join(calls, "\n") |
| + mu.Unlock() |
| + if !strings.Contains(joined, "stop carlos-instance-seed@h.example.com.service") { |
| + t.Errorf("no stop issued for the stale invocation:\n%s", joined) |
| + } |
| + var out SeedResponse |
| + if err := json.NewDecoder(w.Body).Decode(&out); err != nil || out.InvocationID != invocation { |
| + t.Errorf("response = %+v, %v, want the FRESH invocation %s", out, err, invocation) |
| + } |
| } |
| func TestStopResetAndTheirTolerances(t *testing.T) { |
| @@ -291,6 +411,26 @@ func TestSweepStopsOthersAndReportsAStuckUnitAsFailed(t *testing.T) { |
| } |
| } |
| +// TestReadyBeforeTheFirstProbeNamesTheReason pins that /ready answers with |
| +// an explicit "not yet run" line before RunProbe has ever completed, not |
| +// an empty Lines that could be misread as every check having passed |
| +// (final review, plan 2, item 6). |
| +func TestReadyBeforeTheFirstProbeNamesTheReason(t *testing.T) { |
| + ctl := &fakeCtl{} |
| + _, c := newServer(t, ctl) |
| + resp, err := c.Get("http://unix/ready") |
| + if err != nil || resp.StatusCode != 503 { |
| + t.Fatalf("status: %v, %v", resp, err) |
| + } |
| + var got ProbeReport |
| + if err := json.NewDecoder(resp.Body).Decode(&got); err != nil { |
| + t.Fatal(err) |
| + } |
| + if got.Ready || len(got.Lines) != 1 || got.Lines[0].Check != "probe" || got.Lines[0].Verdict != "fail" || got.Lines[0].Detail != "not yet run" { |
| + t.Errorf("body = %+v", got) |
| + } |
| +} |
| + |
| func TestReadyReflectsTheProbeVerdict(t *testing.T) { |
| ctl := &fakeCtl{} |
| s, c := newServer(t, ctl) |
diff --git a/internal/instanceexec/exit.go b/internal/instanceexec/exit.go| index 930933c6e..cf32a4a04 100644 |
| --- a/internal/instanceexec/exit.go |
| +++ b/internal/instanceexec/exit.go |
| @@ -72,7 +72,11 @@ func writeExitRecord(runDir string, role instancectl.Role, iid string, env func( |
| } |
| raw = append(raw, '\n') |
| dir := filepath.Join(runDir, "instances", iid) |
| - f, err := instancefs.OpenBeneath(dir, instancectl.ExitRecordName(role, inv), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) |
| + // 0600: this record carries SERVICE_RESULT/EXIT_CODE/EXIT_STATUS from |
| + // systemd, which is diagnostic detail about the instance's own process, |
| + // not something every reader of the instance directory needs (final |
| + // review, plan 2, item 6). |
| + f, err := instancefs.OpenBeneath(dir, instancectl.ExitRecordName(role, inv), os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o600) |
| if err != nil { |
| if errors.Is(err, os.ErrExist) { |
| return fmt.Errorf("record for invocation %s already exists", inv) |
diff --git a/internal/instanceexec/exit_test.go b/internal/instanceexec/exit_test.go| index e3c8183d0..de580d64d 100644 |
| --- a/internal/instanceexec/exit_test.go |
| +++ b/internal/instanceexec/exit_test.go |
| @@ -38,6 +38,12 @@ func TestWriteExitRecordWritesTheFourVariables(t *testing.T) { |
| if rec.Invocation != "deadbeefdeadbeefdeadbeefdeadbeef" || rec.Result != "exit-code" || rec.Code != "exited" || rec.Status != "3" || !rec.At.Equal(now) { |
| t.Errorf("record = %+v", rec) |
| } |
| + // 0600: the record is diagnostic detail about this instance's own exit, |
| + // not something every reader of the instance directory needs (final |
| + // review, plan 2, item 6). |
| + if fi, err := os.Stat(filepath.Join(run, "instances", iid, "exit.run.deadbeefdeadbeefdeadbeefdeadbeef.json")); err != nil || fi.Mode().Perm() != 0o600 { |
| + t.Errorf("mode = %v, %v, want 0600", fi.Mode(), err) |
| + } |
| // O_EXCL: a second write for the same invocation is refused and the |
| // first record stands. |
| env2 := envOf(map[string]string{"INVOCATION_ID": "deadbeefdeadbeefdeadbeefdeadbeef", "SERVICE_RESULT": "success", "EXIT_CODE": "exited", "EXIT_STATUS": "0"}) |
diff --git a/internal/instanceexec/probe.go b/internal/instanceexec/probe.go| index a8edba65f..21d41e3e4 100644 |
| --- a/internal/instanceexec/probe.go |
| +++ b/internal/instanceexec/probe.go |
| @@ -1,6 +1,8 @@ |
| package instanceexec |
| import ( |
| + "crypto/rand" |
| + "encoding/hex" |
| "encoding/json" |
| "errors" |
| "flag" |
| @@ -27,12 +29,18 @@ type probeEnv struct { |
| varLib string // /var/lib |
| etcCarlos string // /etc/carlos |
| procRoot string // /proc |
| - cgroupProcs string // a cgroupfs file to test writability on |
| + controlDir string // /run/carlos-control, which must be invisible from in here |
| + cgroupProcs string // the edge's delegated cgroup.procs, to test writability on |
| pid func() int |
| writable func(path string) error |
| sendUDP func(network, addr string) error |
| connectTCP func(addr string, timeout time.Duration) error |
| mountNoSym func(mountpoint string) (bool, error) |
| + // mountOption asks whether a mountpoint carries a given per-mount |
| + // option; wired to instancefs.MountOptionOf in production. It backs |
| + // the cgroup-ro-mount check, which asserts /sys/fs/cgroup itself |
| + // carries "ro" rather than trusting cgroup-ro's access(W_OK) alone. |
| + mountOption func(mountpoint, opt string) (bool, error) |
| // symlinkCheck plants a symlink under dir and reports whether it was |
| // refused. Production wires this to realSymlinkCheck; the test fixture |
| // stubs it because the fixture's tree is a plain temp dir with no |
| @@ -41,6 +49,12 @@ type probeEnv struct { |
| symlinkCheck func(dir string) error |
| } |
| +// cgroupMountpoint is the fixed system path the cgroup-ro-mount check |
| +// asks instancefs.MountOptionOf about. It is not a probeEnv field: every |
| +// test that cares stubs the mountOption function itself, so no fixture |
| +// needs a real mount at this path. |
| +const cgroupMountpoint = "/sys/fs/cgroup" |
| + |
| var errNotELOOP = errors.New("symlink was followed") |
| // RunProbe is `carlos instance-probe`: it runs inside the probe unit's |
| @@ -62,12 +76,21 @@ func RunProbe(args []string) { |
| func productionProbeEnv() probeEnv { |
| return probeEnv{ |
| dataRoot: "/data", runRoot: "/run/carlos", varCache: "/var/cache", varLib: "/var/lib", |
| - etcCarlos: "/etc/carlos", procRoot: "/proc", cgroupProcs: "/sys/fs/cgroup/cgroup.procs", |
| + etcCarlos: "/etc/carlos", procRoot: "/proc", controlDir: "/run/carlos-control", |
| + // The edge's own delegated cgroup.procs, not the probe's: Delegate= |
| + // on carlos-edge.service chowns this file to carlos, so DAC passes |
| + // and only the mount's ro flag can refuse the write, making EROFS |
| + // evidence rather than an artifact of ownership (final review, |
| + // plan 2, item 1). This depends on the edge being loaded, which |
| + // every instance unit's BindsTo=carlos-edge.service already |
| + // assumes. |
| + cgroupProcs: "/sys/fs/cgroup/carlos.slice/carlos-edge.service/cgroup.procs", |
| pid: os.Getpid, |
| writable: accessWritable, |
| sendUDP: sendOneDatagram, |
| connectTCP: connectWithTimeout, |
| mountNoSym: instancefs.MountNoSymfollow, |
| + mountOption: instancefs.MountOptionOf, |
| symlinkCheck: realSymlinkCheck, |
| } |
| } |
| @@ -138,9 +161,26 @@ func runChecks(e probeEnv) instancectl.ProbeReport { |
| pid := e.pid() |
| pass("pid1", fmt.Sprintf("pid=%d others=[%s] err=%v", pid, others, err), err == nil && pid == 1 && others == "") |
| - // cgroupfs is read-only: access(W_OK) answers EROFS without writing. |
| + // cgroupfs is read-only, checked two ways. access(W_OK) on the edge's |
| + // delegated cgroup.procs must answer EROFS: DAC is checked before the |
| + // mount's read-only flag, so EACCES here means the file is not |
| + // delegated to carlos (not a pass, whatever the mount says) and ENOENT |
| + // means the edge unit is not loaded at all, neither of which the mount |
| + // flag could produce on its own. And /sys/fs/cgroup itself must carry |
| + // the "ro" mount option, so the EROFS above is the mount flag's doing |
| + // and not some other cause (final review, plan 2, item 1). |
| werr := e.writable(e.cgroupProcs) |
| - pass("cgroup-ro", fmt.Sprintf("access(W_OK) = %v", werr), errors.Is(werr, syscall.EROFS)) |
| + detail, ok := cgroupROVerdict(werr) |
| + pass("cgroup-ro", detail, ok) |
| + mro, mroErr := e.mountOption(cgroupMountpoint, "ro") |
| + pass("cgroup-ro-mount", fmt.Sprintf("MountOptionOf(%s, ro) = %v, %v", cgroupMountpoint, mro, mroErr), mro && mroErr == nil) |
| + |
| + // The control socket directory is invisible from in here: InaccessiblePaths= |
| + // in the shared namespace block (design §5.2) makes a connect from |
| + // inside any instance impossible outright, not merely refused by the |
| + // peer check on the other end. |
| + _, cerr := os.Lstat(e.controlDir) |
| + pass("control-unreachable", "control socket directory visible", cerr != nil) |
| // /etc/carlos cannot be opened and its fixture cannot be read. |
| _, derr := os.ReadDir(e.etcCarlos) |
| @@ -185,25 +225,87 @@ func runChecks(e probeEnv) instancectl.ProbeReport { |
| return r |
| } |
| -// realSymlinkCheck plants a symlink inside dir and tries to open it through |
| -// its name; on a nosymfollow mount the open fails with ELOOP. It returns nil |
| -// for ELOOP, errNotELOOP if the link was followed, and any other error. |
| +// cgroupROVerdict turns access(W_OK) on the edge's delegated cgroup.procs |
| +// into the cgroup-ro check's detail and pass/fail: EROFS is the only |
| +// answer the read-only mount flag alone can produce, since the kernel |
| +// checks DAC first and Delegate= has already given carlos write access on |
| +// the file itself (final review, plan 2, item 1). |
| +func cgroupROVerdict(werr error) (detail string, ok bool) { |
| + switch { |
| + case errors.Is(werr, syscall.EROFS): |
| + return "", true |
| + case errors.Is(werr, syscall.EACCES): |
| + return "not delegated", false |
| + case errors.Is(werr, syscall.ENOENT): |
| + return "edge not loaded", false |
| + case werr == nil: |
| + return "writable", false |
| + default: |
| + return werr.Error(), false |
| + } |
| +} |
| + |
| +// randomName returns a 16-hex-digit name. A test overrides it to force a |
| +// name collision; production draws 8 random bytes so two probe runs, or a |
| +// probe run and a flat-tenant's own writes, never collide by accident. |
| +var randomName = func() string { |
| + var b [8]byte |
| + if _, err := rand.Read(b[:]); err != nil { |
| + // crypto/rand.Read only fails if the OS's random source is |
| + // unavailable, which is a box-level emergency worth stopping the |
| + // probe over rather than silently falling back to a fixed name |
| + // that reintroduces the property this check exists to catch. |
| + panic(fmt.Sprintf("instance-probe: crypto/rand: %v", err)) |
| + } |
| + return hex.EncodeToString(b[:]) |
| +} |
| + |
| +// realSymlinkCheck plants a symlink under a random name inside dir, |
| +// pointing at a real file under another random name, and tries to open |
| +// the link through its name; on a nosymfollow mount the open fails with |
| +// ELOOP. Random names mean a same-uid tenant that shares this directory |
| +// cannot pre-plant either one (final review, plan 2, item 5); an EEXIST |
| +// planting either name is reported as a failure and nothing at that name |
| +// is removed, since it was not ours to remove. It returns nil for ELOOP, |
| +// errNotELOOP if the link was followed, and any other error. |
| func realSymlinkCheck(dir string) error { |
| - target := filepath.Join(dir, "probe-target") |
| - link := filepath.Join(dir, "probe-link") |
| - os.Remove(link) |
| - os.Remove(target) |
| - if err := os.WriteFile(target, []byte("x"), 0o600); err != nil { |
| + target := randomName() |
| + f, err := instancefs.OpenBeneath(dir, target, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) |
| + if err != nil { |
| + if errors.Is(err, os.ErrExist) { |
| + return fmt.Errorf("a planted entry already occupies %s", filepath.Join(dir, target)) |
| + } |
| return err |
| } |
| - defer os.Remove(target) |
| - if err := os.Symlink(target, link); err != nil { |
| + _, werr := f.Write([]byte("x")) |
| + cerr := f.Close() |
| + targetPath := filepath.Join(dir, target) |
| + defer os.Remove(targetPath) |
| + if werr != nil { |
| + return werr |
| + } |
| + if cerr != nil { |
| + return cerr |
| + } |
| + |
| + link := randomName() |
| + linkPath := filepath.Join(dir, link) |
| + if err := os.Symlink(targetPath, linkPath); err != nil { |
| + if errors.Is(err, os.ErrExist) { |
| + return fmt.Errorf("a planted entry already occupies %s", linkPath) |
| + } |
| return err |
| } |
| - defer os.Remove(link) |
| - f, err := os.Open(link) |
| + defer os.Remove(linkPath) |
| + |
| + // A plain open, deliberately: the whole point of this check is that |
| + // the MOUNT's nosymfollow option refuses the follow. A resolve flag of |
| + // our own (RESOLVE_NO_SYMLINKS) would answer ELOOP whether or not the |
| + // mount carries the option, making the line vacuous either way (ledger |
| + // ruling, final review, plan 2, item 5). |
| + lf, err := os.Open(linkPath) |
| if err == nil { |
| - f.Close() |
| + lf.Close() |
| return errNotELOOP |
| } |
| if errors.Is(err, syscall.ELOOP) { |
diff --git a/internal/instanceexec/probe_test.go b/internal/instanceexec/probe_test.go| index 89062a4ea..e03e54f12 100644 |
| --- a/internal/instanceexec/probe_test.go |
| +++ b/internal/instanceexec/probe_test.go |
| @@ -6,6 +6,7 @@ import ( |
| "errors" |
| "os" |
| "path/filepath" |
| + "strings" |
| "syscall" |
| "testing" |
| "time" |
| @@ -38,6 +39,7 @@ func goodEnv(t *testing.T) probeEnv { |
| mk("proc/self") |
| return probeEnv{ |
| dataRoot: data, runRoot: run, varCache: varCache, varLib: varLib, etcCarlos: etc, procRoot: proc, |
| + controlDir: filepath.Join(root, "run", "carlos-control"), // deliberately absent: Lstat fails |
| cgroupProcs: filepath.Join(root, "sys", "cgroup.procs"), |
| pid: func() int { return 1 }, |
| writable: func(string) error { return syscall.EROFS }, |
| @@ -50,6 +52,7 @@ func goodEnv(t *testing.T) probeEnv { |
| }, |
| connectTCP: func(string, time.Duration) error { return syscall.ETIMEDOUT }, |
| mountNoSym: func(string) (bool, error) { return true, nil }, |
| + mountOption: func(string, string) (bool, error) { return true, nil }, |
| symlinkCheck: func(string) error { return nil }, |
| } |
| } |
| @@ -68,7 +71,7 @@ func TestProbePassesOnAnIsolatedNamespace(t *testing.T) { |
| if !r.Ready { |
| t.Fatalf("not ready: %+v", r.Lines) |
| } |
| - for _, c := range []string{"data-tree", "run-tree", "var-empty", "pid1", "cgroup-ro", "etc-carlos", "symlink-eloop", "nosymfollow", "imds-v4", "imds-v6", "loopback", "imds-tcp"} { |
| + for _, c := range []string{"data-tree", "run-tree", "var-empty", "pid1", "cgroup-ro", "cgroup-ro-mount", "control-unreachable", "etc-carlos", "symlink-eloop", "nosymfollow", "imds-v4", "imds-v6", "loopback", "imds-tcp"} { |
| if v := verdict(r, c); v != "ok" { |
| t.Errorf("%s = %s", c, v) |
| } |
| @@ -95,6 +98,50 @@ func TestProbeFailsWhenCgroupfsIsWritable(t *testing.T) { |
| } |
| } |
| +// TestCgroupROVerdictDistinguishesTheThreeAnswers pins that DAC (checked |
| +// first by the kernel) and the mount flag are told apart, not merged into |
| +// one pass/fail: EACCES means the file is not delegated to carlos, ENOENT |
| +// means the edge unit is not loaded, and neither is the mount flag's own |
| +// answer (final review, plan 2, item 1). |
| +func TestCgroupROVerdictDistinguishesTheThreeAnswers(t *testing.T) { |
| + cases := []struct { |
| + err error |
| + detail string |
| + ok bool |
| + }{ |
| + {syscall.EROFS, "", true}, |
| + {syscall.EACCES, "not delegated", false}, |
| + {syscall.ENOENT, "edge not loaded", false}, |
| + {nil, "writable", false}, |
| + } |
| + for _, c := range cases { |
| + detail, ok := cgroupROVerdict(c.err) |
| + if detail != c.detail || ok != c.ok { |
| + t.Errorf("cgroupROVerdict(%v) = %q, %v, want %q, %v", c.err, detail, ok, c.detail, c.ok) |
| + } |
| + } |
| +} |
| + |
| +func TestProbeFailsWhenTheCgroupMountLacksRO(t *testing.T) { |
| + e := goodEnv(t) |
| + e.mountOption = func(string, string) (bool, error) { return false, nil } |
| + r := runChecks(e) |
| + if r.Ready || verdict(r, "cgroup-ro-mount") != "fail" { |
| + t.Errorf("cgroup mount without ro yet ready=%v", r.Ready) |
| + } |
| +} |
| + |
| +func TestProbeFailsWhenTheControlSocketDirectoryIsVisible(t *testing.T) { |
| + e := goodEnv(t) |
| + if err := os.MkdirAll(e.controlDir, 0o750); err != nil { |
| + t.Fatal(err) |
| + } |
| + r := runChecks(e) |
| + if r.Ready || verdict(r, "control-unreachable") != "fail" { |
| + t.Errorf("visible control socket directory yet ready=%v", r.Ready) |
| + } |
| +} |
| + |
| func TestProbeFailsWhenEtcCarlosIsReadable(t *testing.T) { |
| e := goodEnv(t) |
| if err := os.MkdirAll(e.etcCarlos, 0o755); err != nil { |
| @@ -168,3 +215,28 @@ func TestProbeSymlinkCheckUsesARealSymlink(t *testing.T) { |
| t.Error("a followable symlink was not reported") |
| } |
| } |
| + |
| +// TestRealSymlinkCheckReportsAPlantedName pins the review fix: a name the |
| +// check would have used is refused (EEXIST), reported as a failure naming |
| +// the planted entry, and left in place, rather than removed. A same-uid |
| +// tenant sharing this directory could otherwise have its own file deleted |
| +// by a probe run (final review, plan 2, item 5). |
| +func TestRealSymlinkCheckReportsAPlantedName(t *testing.T) { |
| + dir := t.TempDir() |
| + orig := randomName |
| + t.Cleanup(func() { randomName = orig }) |
| + randomName = func() string { return "planted" } |
| + if err := os.MkdirAll(filepath.Join(dir, "planted"), 0o700); err != nil { |
| + t.Fatal(err) |
| + } |
| + err := realSymlinkCheck(dir) |
| + if err == nil { |
| + t.Fatal("a colliding name was not reported") |
| + } |
| + if !strings.Contains(err.Error(), filepath.Join(dir, "planted")) { |
| + t.Errorf("error does not name the planted entry: %v", err) |
| + } |
| + if fi, statErr := os.Stat(filepath.Join(dir, "planted")); statErr != nil || !fi.IsDir() { |
| + t.Errorf("the planted entry was removed: %v, %v", fi, statErr) |
| + } |
| +} |
diff --git a/internal/instancefs/instancefs_linux.go b/internal/instancefs/instancefs_linux.go| index 468a5510c..12fd93c5c 100644 |
| --- a/internal/instancefs/instancefs_linux.go |
| +++ b/internal/instancefs/instancefs_linux.go |
| @@ -125,29 +125,37 @@ func WriteAtomic(dir, name string, data []byte, perm os.FileMode) error { |
| return nil |
| } |
| -// MountNoSymfollow reports whether mountpoint is a mountpoint whose |
| -// options include nosymfollow, by reading /proc/self/mountinfo. A path |
| -// that is not a mountpoint is (false, nil). |
| -func MountNoSymfollow(mountpoint string) (bool, error) { |
| +// MountOptionOf reports whether mountpoint is a mountpoint whose per-mount |
| +// options include opt, by reading /proc/self/mountinfo. A path that is not |
| +// a mountpoint is (false, nil). |
| +func MountOptionOf(mountpoint, opt string) (bool, error) { |
| f, err := os.Open("/proc/self/mountinfo") |
| if err != nil { |
| return false, err |
| } |
| defer f.Close() |
| - return mountNoSymfollow(f, mountpoint) |
| + return mountOptionOf(f, mountpoint, opt) |
| +} |
| + |
| +// MountNoSymfollow reports whether mountpoint carries nosymfollow. It |
| +// predates MountOptionOf and stays as a thin wrapper because most callers |
| +// only ever ask this one question and naming it directly reads better at |
| +// the call site than spelling the option out every time. |
| +func MountNoSymfollow(mountpoint string) (bool, error) { |
| + return MountOptionOf(mountpoint, "nosymfollow") |
| } |
| -// mountNoSymfollow is MountNoSymfollow's body, taking its mountinfo source |
| -// as an io.Reader so a test can feed it a fixture with stacked mount lines |
| +// mountOptionOf is MountOptionOf's body, taking its mountinfo source as an |
| +// io.Reader so a test can feed it a fixture with stacked mount lines |
| // instead of the live /proc/self/mountinfo. |
| // |
| // Stacked mounts at one mountpoint (a bind mount, then a later remount) |
| // list oldest first in mountinfo, and the LAST line for a mountpoint is |
| // the one actually in effect. Every line is therefore scanned to the end |
| // and the last matching line's verdict remembered, rather than returning |
| -// on the first match: an earlier stacked mount carrying nosymfollow must |
| -// not outvote a later one that dropped it, or vice versa. |
| -func mountNoSymfollow(r io.Reader, mountpoint string) (bool, error) { |
| +// on the first match: an earlier stacked mount carrying opt must not |
| +// outvote a later one that dropped it, or vice versa. |
| +func mountOptionOf(r io.Reader, mountpoint, opt string) (bool, error) { |
| want := filepath.Clean(mountpoint) |
| sc := bufio.NewScanner(r) |
| verdict := false |
| @@ -157,7 +165,7 @@ func mountNoSymfollow(r io.Reader, mountpoint string) (bool, error) { |
| if !ok || mp != want { |
| continue |
| } |
| - verdict = mountinfoLineHas(line, want, "nosymfollow") |
| + verdict = mountinfoLineHas(line, want, opt) |
| } |
| return verdict, sc.Err() |
| } |
diff --git a/internal/instancefs/instancefs_linux_test.go b/internal/instancefs/instancefs_linux_test.go| index 1c7a3d679..cf702a578 100644 |
| --- a/internal/instancefs/instancefs_linux_test.go |
| +++ b/internal/instancefs/instancefs_linux_test.go |
| @@ -161,14 +161,35 @@ func TestMountNoSymfollowReadsMountinfo(t *testing.T) { |
| first := "36 35 98:0 /.instances /data/instances rw,nosuid,nodev,noexec,relatime - ext4 /dev/root rw" |
| last := "37 35 98:0 /.instances /data/instances rw,nosuid,nodev,noexec,nosymfollow,relatime - ext4 /dev/root rw" |
| stacked := strings.NewReader(first + "\n" + last + "\n") |
| - got, err = mountNoSymfollow(stacked, "/data/instances") |
| + got, err = mountOptionOf(stacked, "/data/instances", "nosymfollow") |
| if err != nil || !got { |
| t.Errorf("stacked mounts, last with nosymfollow: got %v, %v, want true, nil", got, err) |
| } |
| stackedReversed := strings.NewReader(last + "\n" + first + "\n") |
| - got, err = mountNoSymfollow(stackedReversed, "/data/instances") |
| + got, err = mountOptionOf(stackedReversed, "/data/instances", "nosymfollow") |
| if err != nil || got { |
| t.Errorf("stacked mounts, last WITHOUT nosymfollow: got %v, %v, want false, nil (the earlier nosymfollow line must not outvote the effective one)", got, err) |
| } |
| } |
| + |
| +// TestMountOptionOfIsTheGeneralForm pins that MountNoSymfollow is now a |
| +// thin wrapper over MountOptionOf, not a separate implementation: asking |
| +// for an option other than nosymfollow (here "ro", the option the |
| +// cgroup-ro-mount probe check asks about) reads the same field. |
| +func TestMountOptionOfIsTheGeneralForm(t *testing.T) { |
| + line := "36 35 98:0 / /sys/fs/cgroup ro,nosuid,nodev,noexec - cgroup2 cgroup2 rw" |
| + got, err := mountOptionOf(strings.NewReader(line+"\n"), "/sys/fs/cgroup", "ro") |
| + if err != nil || !got { |
| + t.Errorf("MountOptionOf(/sys/fs/cgroup, ro): got %v, %v, want true, nil", got, err) |
| + } |
| + got, err = mountOptionOf(strings.NewReader(line+"\n"), "/sys/fs/cgroup", "nosymfollow") |
| + if err != nil || got { |
| + t.Errorf("MountOptionOf(/sys/fs/cgroup, nosymfollow) on a line without it: got %v, %v, want false, nil", got, err) |
| + } |
| + got, err = MountOptionOf("/", "ro") |
| + if err != nil { |
| + t.Fatalf("MountOptionOf(/, ro) against the live mountinfo: %v", err) |
| + } |
| + _ = got // / is not reliably ro or rw across test hosts; only the absence of an error matters here. |
| +} |
diff --git a/internal/instancefs/instancefs_other.go b/internal/instancefs/instancefs_other.go| index 8c8e31d1b..5df5e10d5 100644 |
| --- a/internal/instancefs/instancefs_other.go |
| +++ b/internal/instancefs/instancefs_other.go |
| @@ -21,6 +21,10 @@ func WriteAtomic(dir, name string, data []byte, perm os.FileMode) error { |
| return errors.ErrUnsupported |
| } |
| -func MountNoSymfollow(mountpoint string) (bool, error) { return false, errors.ErrUnsupported } |
| +func MountOptionOf(mountpoint, opt string) (bool, error) { return false, errors.ErrUnsupported } |
| + |
| +func MountNoSymfollow(mountpoint string) (bool, error) { |
| + return MountOptionOf(mountpoint, "nosymfollow") |
| +} |
| func mountinfoLineHas(line, mountpoint, option string) bool { return false } |