From 53a533afb4f1fcc5baebd9d7c3cec735d3684280 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 13:58:14 +0700 Subject: [PATCH] #317: refuse an unresolved caller instead of promoting it to primary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ConnectionIdentity.resolve() called pids.pidForLocalPort(remotePort), which returns -1 both on a real failure and (silently, no log line) when lsof just finds no matching process. terminalForPid(-1) then matches no pane, so CallerResolver's loopback-trust fallback could not tell that caller apart from a genuine primary and handed it Principal.primary(...) — granting SPAWN, STOP, SEND and DRAIN to a worker whose PID lookup failed. This is the escalation PaneLocator's own javadoc already names; CB-161's ancestry walk only helps once a candidate pid exists, and a failed lookup has none. Fix: ConnectionIdentity.Caller gets a resolved() predicate (pid > 0), centralised next to the -1 sentinel it tests for the same reason isLoopback() is centralised (fleetd #305: two independent copies of one rule already drifted once). CallerResolver's loopback-trust fallback now requires c.resolved() before granting PRIMARY; an unresolved caller gets Principal.anonymous() — the same already-tested "authenticated as nothing" outcome used everywhere else in that method, so the refusal is a clean, named, unsurprising result rather than something that looks like a bug. Also logs the previously-silent "lsof ran clean, found no match" case in LsofPeerPidLookup at DEBUG, since that (not a slow lsof — the waitFor result was already discarded) is the likelier real trigger. loopbackTrustTreatsANonWorkerLoopbackCallerAsThePrimary is untouched and still green: a real pid that owns no pane (the actual primary) is still resolved() and still PRIMARY. Token mode is unaffected — it never consults c.pid() at all. Mutation-tested: reverting only the CallerResolver.java guard reproduces the escalation exactly (aFailedPeerPidLookupIsRefusedNotPromotedToPrimary fails with "expected: but was: "). --- .../dev/ltms/fleet/auth/CallerResolver.java | 10 +++- .../ltms/fleet/mcp/ConnectionIdentity.java | 19 ++++++++ .../dev/ltms/fleet/mcp/LsofPeerPidLookup.java | 8 ++++ .../ltms/fleet/auth/CallerResolverTest.java | 48 +++++++++++++++++++ .../fleet/mcp/ConnectionIdentityTest.java | 16 +++++++ 5 files changed, 100 insertions(+), 1 deletion(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/auth/CallerResolver.java b/fleetd/src/main/java/dev/ltms/fleet/auth/CallerResolver.java index e9012f1..a358be9 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/auth/CallerResolver.java +++ b/fleetd/src/main/java/dev/ltms/fleet/auth/CallerResolver.java @@ -236,7 +236,15 @@ public final class CallerResolver { // loopback-trust: same-host callers that are not workers are the primary. A non-loopback // caller is anonymous even here — and startup refuses that combination anyway // (FleetConfig.validateAuthExposure), so this is defence in depth, not the control. - return isLoopback(remoteAddr) ? Principal.primary(c.pid()) : Principal.anonymous(); + // + // fleetd #317: "not a worker" must not be conflated with "identity unresolved". The real + // primary is a real process — its pid resolves (c.resolved()), it just owns no herdr pane. + // A caller whose peer-PID lookup failed (LsofPeerPidLookup's -1 sentinel — on any failure, + // silently including "lsof found no match") has no such pid, and PaneLocator's own javadoc + // already names what happens if that case is handed the primary role: a worker→primary + // escalation. So an unresolved caller is refused (ANONYMOUS — the same clean, already-tested + // "authenticated as nothing" outcome used everywhere else in this method), never promoted. + return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous(); } private boolean presentedTokenMatches(String authorizationHeader) { diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/ConnectionIdentity.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/ConnectionIdentity.java index 16986bb..8f9e453 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/ConnectionIdentity.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/ConnectionIdentity.java @@ -36,6 +36,25 @@ public final class ConnectionIdentity { * primary / an off-host client) and its {@code pid} (or {@code -1} if not resolvable). */ public record Caller(String terminal, long pid) { + + /** + * Whether the OS peer-PID lookup actually succeeded — {@code false} means {@code pid} is + * the {@code -1} sentinel, not a real process id, so this caller's identity could not be + * established at all. That is a different fact from a real pid that simply owns no worker + * pane (the primary's own connection): the primary is {@code resolved()} and has a + * {@code null terminal}; an unresolvable caller is {@code !resolved()} and also has a + * {@code null terminal}. The two look identical through {@link #terminal} alone, which is + * exactly how fleetd #317 happened — a failed {@code lsof} lookup and a genuine primary both + * fell through to {@code Principal.primary(...)}. + * + *

Centralised here, next to the sentinel it tests, for the same reason + * {@link ConnectionIdentity#isLoopback} is centralised rather than left for each caller to + * reimplement: a raw {@code pid > 0} check duplicated at every call site is precisely the + * "one rule, two copies" shape that let #305 drift. + */ + public boolean resolved() { + return pid > 0; + } } /** Resolve the caller's terminal and PID from one peer-PID lookup. */ diff --git a/fleetd/src/main/java/dev/ltms/fleet/mcp/LsofPeerPidLookup.java b/fleetd/src/main/java/dev/ltms/fleet/mcp/LsofPeerPidLookup.java index 79da40d..83cab49 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/mcp/LsofPeerPidLookup.java +++ b/fleetd/src/main/java/dev/ltms/fleet/mcp/LsofPeerPidLookup.java @@ -41,6 +41,14 @@ public final class LsofPeerPidLookup implements PeerPidLookup { if (!p.waitFor(2, TimeUnit.SECONDS)) { p.destroyForcibly(); } + if (found < 0) { + // fleetd #317: this is the silent path — lsof ran clean and simply reported no + // matching process (e.g. queried before the OS socket table settles). Previously + // this logged nothing at all, which is exactly why the escalation went unnoticed; + // the exception path below already logs. A caller now refused because of this is + // still refused (never promoted) — this line only makes the refusal diagnosable. + log.debug("lsof peer-pid lookup for port {} found no matching process", port); + } return found; } catch (Exception e) { log.debug("lsof peer-pid lookup for port {} failed: {}", port, e.getMessage()); diff --git a/fleetd/src/test/java/dev/ltms/fleet/auth/CallerResolverTest.java b/fleetd/src/test/java/dev/ltms/fleet/auth/CallerResolverTest.java index c0d4755..e8d7016 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/auth/CallerResolverTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/auth/CallerResolverTest.java @@ -103,6 +103,54 @@ class CallerResolverTest { assertEquals(Role.PRIMARY, p.role(), "the historical behaviour, now an explicit choice"); } + // ── fleetd #317: an unresolvable caller must never be promoted to the primary ────────────────── + // #305 closed the trigger where a resolved pid matched no pane *and* had no ancestry walk to + // save it. This is the other trigger PaneLocator's javadoc names: the pid never resolves at + // all — LsofPeerPidLookup returns -1 on any failure, including (silently) "lsof found no + // match" — so there is no candidate pid for an ancestry walk to even attempt. + + /** + * The failing-without-the-fix case. Before #317's fix, {@code c.terminal() == null} was the + * only test in the loopback-trust fallback, and an unresolved pid produces exactly that same + * {@code null} terminal as a genuine primary — so this caller was handed + * {@code Principal.primary(...)}, a real worker's failed lookup becoming indistinguishable from + * the lead. + */ + @Test + void aFailedPeerPidLookupIsRefusedNotPromotedToPrimary() { + ConnectionIdentity unresolved = new ConnectionIdentity(new PaneLocator(herdr), _ -> -1); + Principal p = new CallerResolver(unresolved).resolve("127.0.0.1", 55555, null); + + assertEquals(Role.ANONYMOUS, p.role(), + "an unresolvable caller must never be silently promoted to the primary"); + } + + /** + * The companion invariant #317 must not break: a caller whose lookup genuinely succeeded, and + * who simply owns no herdr pane — the real primary's own connection — is still the primary. + * This is {@link #loopbackTrustTreatsANonWorkerLoopbackCallerAsThePrimary} pinned again here, + * named for #317 and placed next to the test it must be distinguished from: same {@code null} + * terminal, opposite verdict, because {@code Caller.resolved()} tells them apart. + */ + @Test + void aRealPidThatOwnsNoPaneIsStillThePrimaryNotRefused() { + Principal p = new CallerResolver(nonWorkerIdentity()).resolve("127.0.0.1", 55555, null); + + assertEquals(Role.PRIMARY, p.role()); + } + + /** #317 point 4: token mode never consults {@code c.pid()}, so a failed lookup must not change it. */ + @Test + void tokenModeIsUndisturbedByAnUnresolvedLookup() { + ConnectionIdentity unresolved = new ConnectionIdentity(new PaneLocator(herdr), _ -> -1); + CallerResolver r = new CallerResolver(unresolved, true, "s3cret"); + + assertEquals(Role.ANONYMOUS, r.resolve("127.0.0.1", 55555, null).role(), + "no credential is still just ANONYMOUS, as before #317 — unchanged by the lookup failing"); + assertEquals(Role.PRIMARY, r.resolve("127.0.0.1", 55555, "Bearer s3cret").role(), + "a valid token still authenticates the primary even though the peer-pid lookup failed"); + } + @Test void tokenModeRefusesANonWorkerCallerThatPresentsNoToken() { Principal p = new CallerResolver(nonWorkerIdentity(), true, "s3cret") diff --git a/fleetd/src/test/java/dev/ltms/fleet/mcp/ConnectionIdentityTest.java b/fleetd/src/test/java/dev/ltms/fleet/mcp/ConnectionIdentityTest.java index 94ef7c8..af4139c 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/mcp/ConnectionIdentityTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/mcp/ConnectionIdentityTest.java @@ -43,6 +43,22 @@ class ConnectionIdentityTest { assertNull(with(_ -> 999_999).callerTerminal("127.0.0.1", 55555)); } + @Test + void callerIsUnresolvedWhenThePeerPidLookupFails() { + // fleetd #317: LsofPeerPidLookup returns -1 on any failure — a fork error, or (silently) + // simply no matching lsof line. Caller.resolved() is the one place that sentinel is tested. + ConnectionIdentity.Caller c = with(_ -> -1).resolve("127.0.0.1", 55555); + assertFalse(c.resolved(), "a -1 pid means the lookup failed, not that this pid owns no pane"); + } + + @Test + void callerIsResolvedWhenThePidIsRealEvenThoughItOwnsNoPane() { + // The primary's own connection: a real, lsof-found pid that just isn't a worker pane. This + // must read as "resolved" — the distinction #317 turns on. + ConnectionIdentity.Caller c = with(_ -> 999_999).resolve("127.0.0.1", 55555); + assertTrue(c.resolved()); + } + @Test void resolvesTheCallersPidAndCwd() { // CB-112: the primary maps to no pane, but its PID and cwd are still readable.