diff --git a/REPORT-cb161.md b/REPORT-cb161.md new file mode 100644 index 0000000..f223d9e --- /dev/null +++ b/REPORT-cb161.md @@ -0,0 +1,131 @@ +# CB-161: fix the pane-ownership ancestry gap in PaneLocator + +## The bug + +`PaneLocator.paneOwnsPid` matched a pid against only two things: a pane's `shell_pid`, and +the pids in `foreground_processes`. A grandchild process a member spawns (for example a +`python3` or `curl` helper that opens its own connection to `127.0.0.1:8765`) matched no +pane. `ConnectionIdentity.resolve` then fell through to loopback-trust and resolved that +connection as the **primary** — a worker→primary privilege escalation. + +## The fix + +`fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java`: + +- `terminalForPid` now builds the caller's **ancestor pid set once** (the pid itself, then its + parents up the tree) before scanning any pane, and reuses that same set across every `HerdrClient` + searched (the CB-185 two-daemon path). It does not walk the tree once per pane. +- The walk is bounded at 32 generations (`MAX_ANCESTRY_DEPTH`) and also stops the moment it revisits + a pid it has already recorded (cycle guard), so a cycle or a pathologically deep tree cannot hang + identity resolution, which runs on every MCP call. +- `paneOwnsPid` (renamed `paneOwnsAnyOf`) now checks whether a pane's `shell_pid` or any + `foreground_processes` pid is a **member of that ancestor set**, instead of comparing the raw pid + directly. A pid that IS the shell pid or a foreground pid still matches (ancestry sets always + contain the pid itself) — no regression there. +- A vanished ancestor (`ParentResolver.parentOf` returning empty) ends the walk quietly, not as an + error — matches the brief. + +### The seam: `ParentResolver` + +New file `fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java` — a one-method interface +(`OptionalLong parentOf(long pid)`) so the ancestry walk is testable without spawning real OS +processes. `ParentResolver.PROCESS_HANDLE` is the production implementation, backed by +`ProcessHandle.of(pid)` / `.parent()`. `PaneLocator` gained two new constructor overloads +(`PaneLocator(HerdrClient, ParentResolver)` and `PaneLocator(HerdrClient, HerdrClient, +ParentResolver)`) that accept an injected resolver; the existing constructors delegate to these with +`ParentResolver.PROCESS_HANDLE`, so every existing call site (`Fleetd.java`, `ConnectionIdentity` +tests, the contract test) is unchanged and compiles as-is. + +## Tests + +New file `fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java` — a fake `ParentResolver` +backed by an explicit pid→parent `Map`. + +New tests added to `fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java` (using a new +small `OnePaneHerdr` fake nested in that test class, purpose-built so shell_pid and a foreground pid +can be given distinct values — the shared `FakeHerdr` fixture happens to use the same pid for both, +which would not isolate the two cases): + +1. `stillResolvesAPidThatIsExactlyThePaneShellPid` — regression, no ancestry walk needed. +2. `stillResolvesAPidThatIsExactlyAForegroundPid` — regression, via the foreground-processes list. +3. `resolvesAGrandchildPidTwoLevelsBelowTheShellPid` — **the bug.** grandchild → child → shell_pid. +4. `nullForAPidWhoseAncestryMatchesNoPane` — a pid whose ancestry never reaches any pane still + resolves to `null`, so the real primary is not demoted to a worker. +5. `ancestryWalkTerminatesOnACycleInsteadOfHanging` — a 2-cycle fake parent map (100→101→100) + terminates and returns `null` instead of hanging. +6. `ancestrySetIsComputedOnceAcrossBothClientsInTheTwoDaemonConstructor` — wraps the fake resolver + with a call counter, uses the two-arg (CB-185) constructor with one client reporting no panes, + and asserts exactly 3 `parentOf` calls (not 6) — the ancestor set is walked once and reused + across both clients, not recomputed per client. + +All existing `PaneLocatorTest` tests (the CB-185 two-daemon fallback tests) still pass unchanged. + +### Proof that tests 3 and 5 fail without the fix + +I temporarily reverted `ancestorsOf` twice, ran the specific test, then restored the real fix +(`git diff` is clean of these — they were never committed): + +**Test 3, reverted to "no ancestry walk at all" (`return Set.of(pid);`):** + +``` +mvn -Dtest=PaneLocatorTest#resolvesAGrandchildPidTwoLevelsBelowTheShellPid test +... +org.opentest4j.AssertionFailedError: expected: but was: +[ERROR] PaneLocatorTest.resolvesAGrandchildPidTwoLevelsBelowTheShellPid:100 expected: but was: +[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0 +[INFO] BUILD FAILURE +``` + +**Test 5, reverted to a naive walk (no `MAX_ANCESTRY_DEPTH` bound, no "already seen" cycle guard — +just `while (current > 0) { ancestry.add(current); ... }`):** + +Ran `mvn -Dtest=PaneLocatorTest#ancestryWalkTerminatesOnACycleInsteadOfHanging test` in the +background and polled it. It was still running (stuck inside `PaneLocatorTest`, no test-run summary +line ever printed) after 20 seconds and had to be force-killed — confirming the naive walk hangs on +the 100↔101 cycle fixture. The real fix's depth bound and cycle guard are what make this test pass +in ~0.25s instead. + +Both reverts were made only in-memory in my worktree (I kept a copy of the correct file at +`/tmp/PaneLocator.java.fixed` beforehand) and restored via `cp` before running the full build below. +`git status` / `git diff` at commit time show only the intended fix, not either revert. + +## Full build + +``` +cd fleetd && mvn clean install +``` + +Ran to completion, unpiped. Final lines: + +``` +[INFO] Tests run: 1043, Failures: 0, Errors: 0, Skipped: 0 +[INFO] BUILD SUCCESS +[INFO] Total time: 35.061 s +``` + +(main was 1037 tests; this branch adds 6 new tests in `PaneLocatorTest` = 1043.) + +## Scope discipline — what I did NOT touch + +Per the brief: only `PaneLocator`, its tests, and the minimal `ParentResolver` seam + its fake. +`Authz`, `ConnectionIdentity`'s fallthrough, and the loopback-trust policy are unchanged — those +stay the deliberate primary-only decision they were. + +One thing spotted but out of scope, noted only: `terminalForPid` still issues one +`pane.process_info` herdr call **per pane** on every resolution (no caching) — the class javadoc +already flags this as "the obvious optimization once wired into `ClaudeCodeLauncher`." Not touched. + +## What I could NOT check + +- No IDE tooling (no `ide_diagnostics` / IntelliJ inspections) — `mvn clean install` is my only + verification, as the worker charter expects. +- Cannot restart or query the live `fleetd` daemon — this fix is unverified against a real herdr + process tree. The pre-existing `PaneLocatorContractTest` (tagged `contract`, run with + `-Pcontract`) exercises `PaneLocator` against a real herdr daemon and a real shell pid (not a + grandchild), but I did not run it — it requires a live herdr socket, which I don't have in this + worktree, and it wasn't part of the standard `mvn clean install` run above (it's a separate + Maven profile). I did not add a grandchild-pid variant of it, since the brief scoped tests to + unit tests against a fake resolver. +- I did not attempt to reproduce the original privilege-escalation scenario end-to-end (spawning a + real grandchild process against a live daemon) — the ticket says that half is already measured + and confirmed, and I have no way to drive a live daemon from this worktree. diff --git a/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java b/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java index f2a0af4..7c5c89f 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java @@ -2,8 +2,11 @@ package dev.ltms.fleet.herdr; import com.fasterxml.jackson.databind.JsonNode; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.OptionalLong; +import java.util.Set; /** * Resolves which herdr pane a process belongs to — the herdr half of connection-based MCP @@ -12,23 +15,46 @@ import java.util.Map; * is calling without the worker sending anything spoofable. * *

herdr owns the PID→pane truth: {@code pane.process_info} reports each pane's {@code shell_pid} - * and foreground process PIDs. This scans agent panes; a spawn-time {@code pid→terminal} cache is - * the obvious optimization once wired into {@code ClaudeCodeLauncher}. + * and foreground process PIDs. A pid that is neither of those directly — e.g. a grandchild a + * worker spawned, such as a {@code python3} or {@code curl} helper that opens its own MCP + * connection — is resolved by walking its ancestry (via {@link ParentResolver}) up to the root and + * matching any ancestor against a pane's {@code shell_pid} or foreground pids (CB-161). Without + * this walk such a pid matches no pane, and the caller falls through to loopback-trust and is + * resolved as the primary — a worker→primary privilege escalation. + * + *

This scans agent panes; a spawn-time {@code pid→terminal} cache is the obvious optimization + * once wired into {@code ClaudeCodeLauncher}. * *

CB-185 split the fleet across two herdr daemons — lead operations on one, members on the * other ({@code memberHerdrSocket}). A caller's pane can live on either daemon (a lead's * MCP connection resolves against the lead daemon; a member's against the member daemon), so this * must be able to search more than one client. {@link #PaneLocator(HerdrClient, HerdrClient)} * searches the lead client first, then the member client, and collapses to a single scan when the - * two are the same object (the historical single-daemon deployment). + * two are the same object (the historical single-daemon deployment). The caller's ancestor set is + * computed once per {@link #terminalForPid} call and reused across every client searched — it + * does not depend on which daemon a pane happens to live on. */ public final class PaneLocator { + /** + * Bound on how many ancestor generations {@link #ancestorsOf} walks. This runs on every MCP + * call, so a cycle or a pathologically deep process tree must not hang identity resolution; + * 32 generations is far more than any real worker→helper process tree needs. + */ + private static final int MAX_ANCESTRY_DEPTH = 32; + private final List herdrs; + private final ParentResolver parentResolver; /** Search only this client — the single-daemon deployment. */ public PaneLocator(HerdrClient herdr) { + this(herdr, ParentResolver.PROCESS_HANDLE); + } + + /** Search only this client, resolving ancestry through {@code parentResolver} — for tests. */ + public PaneLocator(HerdrClient herdr, ParentResolver parentResolver) { this.herdrs = List.of(herdr); + this.parentResolver = parentResolver; } /** @@ -37,7 +63,13 @@ public final class PaneLocator { * collapses to one client and one scan, exactly {@link #PaneLocator(HerdrClient)}'s behaviour. */ public PaneLocator(HerdrClient lead, HerdrClient member) { + this(lead, member, ParentResolver.PROCESS_HANDLE); + } + + /** Two-daemon deployment, resolving ancestry through {@code parentResolver} — for tests. */ + public PaneLocator(HerdrClient lead, HerdrClient member, ParentResolver parentResolver) { this.herdrs = lead == member ? List.of(lead) : List.of(lead, member); + this.parentResolver = parentResolver; } /** @@ -49,8 +81,9 @@ public final class PaneLocator { if (pid <= 0) { return null; } + Set ancestry = ancestorsOf(pid); for (HerdrClient herdr : herdrs) { - String terminal = terminalForPid(herdr, pid); + String terminal = terminalForPid(herdr, ancestry); if (terminal != null) { return terminal; } @@ -58,28 +91,54 @@ public final class PaneLocator { return null; } - private static String terminalForPid(HerdrClient herdr, long pid) { + /** + * {@code pid} itself plus its ancestor chain, walked through {@link #parentResolver} up to + * {@link #MAX_ANCESTRY_DEPTH} generations or pid 1, whichever comes first. A vanished ancestor + * ({@link ParentResolver#parentOf} returning empty) ends the walk without error — it just means + * the chain is shorter than the bound. A cycle in a fake resolver is caught by the "already + * seen" check and also ends the walk, so this can never loop. + */ + private Set ancestorsOf(long pid) { + Set ancestry = new LinkedHashSet<>(); + long current = pid; + for (int depth = 0; depth < MAX_ANCESTRY_DEPTH; depth++) { + if (current <= 0 || !ancestry.add(current)) { + break; // vanished/invalid pid, or a cycle back to a pid already recorded + } + if (current == 1) { + break; // reached the root of the process tree + } + OptionalLong parent = parentResolver.parentOf(current); + if (parent.isEmpty()) { + break; // vanished ancestor — not an error, just the end of the chain + } + current = parent.getAsLong(); + } + return ancestry; + } + + private static String terminalForPid(HerdrClient herdr, Set ancestry) { for (JsonNode pane : herdr.call("pane.list", Map.of()).path("panes")) { String paneId = pane.path("pane_id").asText(null); - if (paneId != null && paneOwnsPid(herdr, paneId, pid)) { + if (paneId != null && paneOwnsAnyOf(herdr, paneId, ancestry)) { return pane.path("terminal_id").asText(null); } } return null; } - private static boolean paneOwnsPid(HerdrClient herdr, String paneId, long pid) { + private static boolean paneOwnsAnyOf(HerdrClient herdr, String paneId, Set ancestry) { JsonNode info; try { info = herdr.call("pane.process_info", Map.of("pane_id", paneId)).path("process_info"); } catch (HerdrException e) { return false; // pane vanished mid-scan — just skip it } - if (info.path("shell_pid").asLong(-1) == pid) { + if (ancestry.contains(info.path("shell_pid").asLong(-1))) { return true; } for (JsonNode p : info.path("foreground_processes")) { - if (p.path("pid").asLong(-1) == pid) { + if (ancestry.contains(p.path("pid").asLong(-1))) { return true; } } diff --git a/fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java b/fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java new file mode 100644 index 0000000..6cc8149 --- /dev/null +++ b/fleetd/src/main/java/dev/ltms/fleet/herdr/ParentResolver.java @@ -0,0 +1,21 @@ +package dev.ltms.fleet.herdr; + +import java.util.OptionalLong; + +/** + * Resolves a pid's parent pid — the seam {@link PaneLocator} walks a process's ancestry through, + * so its tests can drive the walk from a fake pid→parent map instead of spawning real processes. + * + *

{@link #PROCESS_HANDLE} is the production implementation, backed by {@link ProcessHandle}. + */ +public interface ParentResolver { + + /** The parent pid of {@code pid}, or empty if {@code pid} is gone or has no known parent. */ + OptionalLong parentOf(long pid); + + /** Production resolver: asks the JVM's {@link ProcessHandle} view of the OS process tree. */ + ParentResolver PROCESS_HANDLE = pid -> ProcessHandle.of(pid) + .flatMap(ProcessHandle::parent) + .map(parent -> OptionalLong.of(parent.pid())) + .orElse(OptionalLong.empty()); +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java b/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java new file mode 100644 index 0000000..0dcf0a1 --- /dev/null +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/FakeParentResolver.java @@ -0,0 +1,27 @@ +package dev.ltms.fleet.herdr; + +import java.util.HashMap; +import java.util.Map; +import java.util.OptionalLong; + +/** + * Fake {@link ParentResolver} backed by an explicit pid→parent map — lets {@link PaneLocatorTest} + * drive {@link PaneLocator}'s ancestry walk (grandchild pids, cycles) without spawning real OS + * processes. + */ +final class FakeParentResolver implements ParentResolver { + + private final Map parents = new HashMap<>(); + + /** {@code pid}'s parent is {@code parentPid}. A pid with no entry here has no known parent. */ + FakeParentResolver parent(long pid, long parentPid) { + parents.put(pid, parentPid); + return this; + } + + @Override + public OptionalLong parentOf(long pid) { + Long parent = parents.get(pid); + return parent == null ? OptionalLong.empty() : OptionalLong.of(parent); + } +} diff --git a/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java b/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java index 929ddc1..7208f05 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/herdr/PaneLocatorTest.java @@ -1,7 +1,11 @@ package dev.ltms.fleet.herdr; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; import org.junit.jupiter.api.Test; +import java.util.concurrent.atomic.AtomicInteger; + import static org.junit.jupiter.api.Assertions.*; /** Unit tests for PID → pane resolution (the herdr half of connection-based MCP identity). */ @@ -63,4 +67,123 @@ class PaneLocatorTest { long paneListCalls = shared.calls.stream().filter(c -> c.method().equals("pane.list")).count(); assertEquals(1, paneListCalls, "same-object lead/member must scan exactly once, not twice"); } + + // --- ancestry walk (CB-161: grandchild pids matched no pane, resolving as primary) -------- + + @Test + void stillResolvesAPidThatIsExactlyThePaneShellPid() { + // Regression: a pid with no parent chain at all — no ancestry walk is needed to match it. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + PaneLocator loc = new PaneLocator(pane, new FakeParentResolver()); + assertEquals("term_x", loc.terminalForPid(5000)); + } + + @Test + void stillResolvesAPidThatIsExactlyAForegroundPid() { + // Regression: same as above, but matching via the foreground-processes list. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + PaneLocator loc = new PaneLocator(pane, new FakeParentResolver()); + assertEquals("term_x", loc.terminalForPid(6000)); + } + + @Test + void resolvesAGrandchildPidTwoLevelsBelowTheShellPid() { + // The bug: a helper process a worker spawns (python3, curl, ...) is a grandchild of the + // pane's shell — not the shell_pid and not a foreground pid directly. Before the fix, + // paneOwnsPid only checked direct pid equality, so this pid matched no pane and the + // caller fell through to loopback-trust as the primary. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(7002, 7001) // grandchild -> child + .parent(7001, 5000); // child -> shell (the pane's shell_pid) + PaneLocator loc = new PaneLocator(pane, parents); + assertEquals("term_x", loc.terminalForPid(7002)); + } + + @Test + void nullForAPidWhoseAncestryMatchesNoPane() { + // Must not break the other direction: a pid that truly belongs to nothing here (e.g. the + // real primary) must still resolve to null. Resolving everything to a worker would demote + // the actual lead and refuse every orchestration call. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(9002, 9001) + .parent(9001, 9000); // chain never reaches 5000 or 6000 + PaneLocator loc = new PaneLocator(pane, parents); + assertNull(loc.terminalForPid(9002)); + } + + @Test + void ancestryWalkTerminatesOnACycleInsteadOfHanging() { + // A fake (or corrupted) parent map that cycles must not hang identity resolution, which + // runs on every MCP call. The walk must still terminate and correctly resolve to null. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(100, 101) + .parent(101, 100); // cycle, never reaches the pane's pids + PaneLocator loc = new PaneLocator(pane, parents); + assertNull(loc.terminalForPid(100)); + } + + @Test + void ancestrySetIsComputedOnceAcrossBothClientsInTheTwoDaemonConstructor() { + // CB-185: the two-daemon constructor searches lead then member. The ancestor set is + // per-caller, not per-client — it must be walked once and reused, not recomputed for + // each client searched. + OnePaneHerdr pane = new OnePaneHerdr("term_x", "pX", 5000, 6000); + FakeParentResolver parents = new FakeParentResolver() + .parent(7002, 7001) + .parent(7001, 5000); + AtomicInteger calls = new AtomicInteger(); + ParentResolver counting = pid -> { + calls.incrementAndGet(); + return parents.parentOf(pid); + }; + HerdrClient noPanes = new FakeHerdr().withNoPanes(); + PaneLocator two = new PaneLocator(noPanes, pane, counting); + assertEquals("term_x", two.terminalForPid(7002)); + assertEquals(3, calls.get(), "ancestry must be walked once (3 lookups: 7002, 7001, 5000), " + + "not re-walked per herdr client"); + } + + /** Minimal single-pane {@link HerdrClient} fake, purpose-built for the ancestry tests above. */ + private static final class OnePaneHerdr implements HerdrClient { + private final ObjectMapper mapper = new ObjectMapper(); + private final String terminalId; + private final String paneId; + private final long shellPid; + private final long foregroundPid; + + OnePaneHerdr(String terminalId, String paneId, long shellPid, long foregroundPid) { + this.terminalId = terminalId; + this.paneId = paneId; + this.shellPid = shellPid; + this.foregroundPid = foregroundPid; + } + + @Override + public JsonNode call(String method, Object params) { + try { + return switch (method) { + case "pane.list" -> mapper.readTree((""" + {"type":"pane_list","panes":[ + {"pane_id":"%s","terminal_id":"%s","workspace_id":"w1","tab_id":"w1:t1","agent":"claude"}]}""") + .formatted(paneId, terminalId)); + case "pane.process_info" -> mapper.readTree((""" + {"type":"pane_process_info","process_info":{"pane_id":"%s","shell_pid":%d, + "foreground_processes":[{"pid":%d,"name":"node","argv0":"claude"}]}}""") + .formatted(paneId, shellPid, foregroundPid)); + default -> throw new HerdrException("OnePaneHerdr has no canned response for " + method); + }; + } catch (HerdrException e) { + throw e; + } catch (Exception e) { + throw new HerdrException("OnePaneHerdr decode failed for " + method, e); + } + } + + @Override + public void close() { + } + } }