fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

A herdr error during the pane scan makes a worker's connection resolve as the primary — #317's escalation, through the door #317 did not close #505

New Issue
Closed
opened 2026-09-12 05:13:45 +02:00 by ltms · 1 comment
ltms commented 2026-09-12 05:13:45 +02:00
Owner
Copy Link
Copy Source

Found by the #492-followup worker as one line in a same-shape sweep. I checked this one myself in
the tree at 136312f, because it is an authorization path. The reading holds, and it is a
different hole from fleetd #317 — #317's fix does not cover it.

Severity: this is the worker→primary privilege escalation PaneLocator's own javadoc names. I am
describing the class of defect only, not how to trigger it.

The chain

fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java:129-135:

private static boolean paneOwnsAnyOf(HerdrClient herdr, String paneId, Set<Long> 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
    }

One false carries two different facts: "this pane does not own the pid" and "I could not
find out whether this pane owns the pid". The comment names only the first reading.

Follow it out:

  1. terminalForPid(herdr, ancestry) (:119-127) skips that pane and keeps scanning. If the pane it
    could not read was the caller's own, the scan ends and returns null.

  2. ConnectionIdentity.resolve (:60-67) builds new Caller(panes.terminalForPid(pid), pid) — a
    real pid, a null terminal.

  3. CallerResolver (:247):

    return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous();

    c.resolved() is pid > 0 (ConnectionIdentity.java:56). The pid did resolve — lsof
    worked fine. So the guard passes and the caller is granted Principal.primary.

A worker that should resolve as Principal.worker resolves as the primary, and the primary can
spawn, stop and drain.

Why #317's fix does not reach this

#317 is about the other input to the same decision. Its fix added c.resolved() so that an
unresolvable pid — LsofPeerPidLookup's -1 sentinel — could no longer be promoted. The
comment at CallerResolver.java:239-246 states the reasoning, and it is correct for what it covers.

But resolved() tests one thing only: whether the lsof lookup succeeded. It says nothing about
whether the pane scan succeeded. So the two failure sources are not symmetric:

what failed pid terminal resolved() outcome
lsof lookup (#317) -1 null false anonymous — refused, correct
herdr pane.process_info (this ticket) real null true primary — promoted

The row #317 closed and the row this ticket opens land on opposite sides of the same ternary. That
is why a sweep for "#317 again" would not find it: nothing here is a sentinel, and the value that
goes wrong is a boolean deep inside a helper three calls away from the decision.

LsofPeerPidLookup.java:45-56 itself is fine and should not be changed. Its comment already
says both branches return the same sentinel on purpose, and that the direction is refusal, never
promotion. The worker listed it as a defect; it is not one. Recording that here so the next reader
does not re-open it.

The fix, in the shape this codebase already uses

Do not widen resolved(). It is documented as testing the lsof sentinel and is centralised
next to that sentinel for the reason ConnectionIdentity.java:48-53 gives.

Add the missing third state to the scan instead, exactly as fleetd #497's family prescribes: one
sentinel for two states that need opposite handling is fixed by a third state, not a better
boolean.

  1. Let the scan report that it was incomplete. paneOwnsAnyOf returning a three-valued answer
    (owns / does not own / could not tell), or terminalForPid distinguishing "scanned every pane,
    no match" from "could not scan every pane", is enough. Keep the existing "pane vanished
    mid-scan" behaviour for a pane that genuinely went away — that case is real and skipping it is
    right. What must change is that an incomplete scan stops looking like a complete one.
  2. Have CallerResolver refuse on an incomplete scan, returning Principal.anonymous() — the same
    clean, already-tested "authenticated as nothing" outcome #317 chose. Fail toward the
    recoverable error    : a refused primary sees a loud refusal and retries; a promoted worker sees
    nothing at all.
  3. Log it at the scan, naming the pane and the herdr daemon. #317's own lesson was that the silent
    path is what let the escalation go unnoticed; LsofPeerPidLookup.java:45-50 records that in a
    comment. Do not repeat the omission one layer up.

Note PaneLocator searches more than one HerdrClient (:84-90, the lead/member split from
CB-185). An error on one daemon must not be reported as a clean negative for the whole scan.

Acceptance

  • A test where pane.process_info throws HerdrException for the pane that does own the
    caller's pid, asserting the caller resolves to anonymous and not to primary. This is the
    discriminating case: a test where the error hits some other pane passes either way and proves
    nothing.
  • A test that a genuinely vanished pane still leaves an otherwise-successful scan able to find the
    right terminal — so the fix does not turn every mid-scan teardown into a refusal.
  • Keep the existing primary case green: a real primary owns no pane, its scan completes, and it
    still resolves as primary.
  • Mutation: revert the production change and confirm the first test goes red. A test that still
    passes with the fix removed is pinning nothing — that exact mistake was caught on PR #496 and is
    written up there.

Related

  • fleetd #317 — the same decision, the other input. Read its fix before starting.
  • fleetd #305 — the loopback definition that drifted into an escalation; why isLoopback and
    resolved() are centralised. The fix here must not add a fourth private copy of a rule.
  • fleetd #497 — the family: one sentinel, two states, opposite handling. This is the highest-stakes
    member of it found so far.
  • fleetd #408 — trusting an exit code as proof of an effect. Same instinct, different surface.
Found by the #492-followup worker as one line in a same-shape sweep. **I checked this one myself in the tree at `136312f`**, because it is an authorization path. The reading holds, and it is a different hole from fleetd #317 — #317's fix does not cover it. Severity: this is the worker→primary privilege escalation `PaneLocator`'s own javadoc names. I am describing the class of defect only, not how to trigger it. ## The chain `fleetd/src/main/java/dev/ltms/fleet/herdr/PaneLocator.java:129-135`: ```java private static boolean paneOwnsAnyOf(HerdrClient herdr, String paneId, Set<Long> 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 } ``` One `false` carries two different facts: **"this pane does not own the pid"** and **"I could not find out whether this pane owns the pid"**. The comment names only the first reading. Follow it out: 1. `terminalForPid(herdr, ancestry)` (`:119-127`) skips that pane and keeps scanning. If the pane it could not read was the caller's own, the scan ends and returns `null`. 2. `ConnectionIdentity.resolve` (`:60-67`) builds `new Caller(panes.terminalForPid(pid), pid)` — a real pid, a `null` terminal. 3. `CallerResolver` (`:247`): ```java return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous(); ``` `c.resolved()` is `pid > 0` (`ConnectionIdentity.java:56`). The pid **did** resolve — `lsof` worked fine. So the guard passes and the caller is granted `Principal.primary`. A worker that should resolve as `Principal.worker` resolves as the primary, and the primary can spawn, stop and drain. ## Why #317's fix does not reach this #317 is about the **other** input to the same decision. Its fix added `c.resolved()` so that an **unresolvable pid** — `LsofPeerPidLookup`'s `-1` sentinel — could no longer be promoted. The comment at `CallerResolver.java:239-246` states the reasoning, and it is correct for what it covers. But `resolved()` tests one thing only: whether the **lsof** lookup succeeded. It says nothing about whether the **pane scan** succeeded. So the two failure sources are not symmetric: | what failed | `pid` | `terminal` | `resolved()` | outcome | |---|---|---|---|---| | lsof lookup (#317) | `-1` | `null` | `false` | `anonymous` — refused, correct | | herdr `pane.process_info` (this ticket) | real | `null` | **`true`** | **`primary`** — promoted | The row #317 closed and the row this ticket opens land on opposite sides of the same ternary. That is why a sweep for "#317 again" would not find it: nothing here is a sentinel, and the value that goes wrong is a `boolean` deep inside a helper three calls away from the decision. `LsofPeerPidLookup.java:45-56` itself is **fine** and should not be changed. Its comment already says both branches return the same sentinel on purpose, and that the direction is refusal, never promotion. The worker listed it as a defect; it is not one. Recording that here so the next reader does not re-open it. ## The fix, in the shape this codebase already uses Do **not** widen `resolved()`. It is documented as testing the lsof sentinel and is centralised next to that sentinel for the reason `ConnectionIdentity.java:48-53` gives. Add the missing third state to the scan instead, exactly as fleetd #497's family prescribes: one sentinel for two states that need opposite handling is fixed by a **third state**, not a better boolean. 1. Let the scan report that it was **incomplete**. `paneOwnsAnyOf` returning a three-valued answer (owns / does not own / could not tell), or `terminalForPid` distinguishing "scanned every pane, no match" from "could not scan every pane", is enough. Keep the existing "pane vanished mid-scan" behaviour for a pane that genuinely went away — that case is real and skipping it is right. What must change is that an incomplete scan stops looking like a complete one. 2. Have `CallerResolver` refuse on an incomplete scan, returning `Principal.anonymous()` — the same clean, already-tested "authenticated as nothing" outcome #317 chose. **Fail toward the recoverable error**: a refused primary sees a loud refusal and retries; a promoted worker sees nothing at all. 3. Log it at the scan, naming the pane and the herdr daemon. #317's own lesson was that the silent path is what let the escalation go unnoticed; `LsofPeerPidLookup.java:45-50` records that in a comment. Do not repeat the omission one layer up. Note `PaneLocator` searches more than one `HerdrClient` (`:84-90`, the lead/member split from CB-185). An error on one daemon must not be reported as a clean negative for the whole scan. ## Acceptance - A test where `pane.process_info` throws `HerdrException` for the pane that **does** own the caller's pid, asserting the caller resolves to `anonymous` and **not** to `primary`. This is the discriminating case: a test where the error hits some *other* pane passes either way and proves nothing. - A test that a genuinely vanished pane still leaves an otherwise-successful scan able to find the right terminal — so the fix does not turn every mid-scan teardown into a refusal. - Keep the existing primary case green: a real primary owns no pane, its scan completes, and it still resolves as `primary`. - Mutation: revert the production change and confirm the first test goes red. A test that still passes with the fix removed is pinning nothing — that exact mistake was caught on PR #496 and is written up there. ## Related - fleetd #317 — the same decision, the other input. Read its fix before starting. - fleetd #305 — the loopback definition that drifted into an escalation; why `isLoopback` and `resolved()` are centralised. The fix here must not add a fourth private copy of a rule. - fleetd #497 — the family: one sentinel, two states, opposite handling. This is the highest-stakes member of it found so far. - fleetd #408 — trusting an exit code as proof of an effect. Same instinct, different surface.
ltms commented 2026-09-12 05:30:18 +02:00
Author
Owner
Copy Link
Copy Source

Why fleetd #415's antidote does not cover this, and where the guard actually belongs

The fleet01 lead made this structural point from an older revision and flagged their premise as
unverified. I checked both halves at 136312f. Both hold, and this changes where a worker should
put the fix.

Their premise, confirmed

They assumed the scan's error path produces a Principal rather than refusing. It does —
CallerResolver.java:247:

return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous();

A failed pane scan leaves a real pid and a null terminal, so c.resolved() is true and this
returns Principal.primary.

The structural point

Authz.permits is a default-less switch over Action — the exhaustive shape #415 recommends, and
it is the right shape:

return switch (action) {
    case SPAWN, STOP, DRAIN, HANDOVER -> caller.isPrimary();
    case SEND                         -> caller.isPrimary() || caller.isArchitect();
    case REPLY, ASK                   -> caller.ownsSession(targetSession);
    case READ, METRICS                -> caller.isPrimary() || caller.isWorker() || caller.isArchitect();
    case COORD_READ                   -> caller.isPrimary();
};

Exhaustiveness at the decision point cannot protect against a wrong principal. The switch is
total over Action and says nothing about whether caller was resolved correctly. Hand it a
Principal that is primary-by-error and every branch votes yes, with the compiler satisfied.

I also checked whether anything downstream could notice. It cannot: Principal carries Role,
terminal and name, and no record of how it was resolved. There is no field an authorization
check could consult even if it wanted to.

So the #415 mechanism — make the decision total, so a new case is a compile error — is orthogonal
to this defect. #415 guards against a decision nobody wrote. This is a decision written correctly
and fed a bad input.

What that means for the fix

Do not add an arm to the switch, and do not add a Role. The guard belongs at the resolver: an
explicit "could not tell" that cannot become a Principal at all. That is the same third-state fix
fleetd #497's family prescribes, applied one layer earlier than the acceptance criteria above put it.

Concretely, this sharpens ask 2 in the ticket body. CallerResolver returning
Principal.anonymous() on an incomplete scan is still correct, but the reason is now explicit: the
unknown state must be unrepresentable as a principal, not merely mapped to a safe one further
down. If a future caller of the resolver forgets the check, it should not compile or should get
anonymous by construction — not silently get primary again.

Severity, stated the way they put it and I agree with

detect_supervisor returning none and this are the same shape — a detection failure resolving to
a definite answer instead of to unknown. The difference is which answer it picks. none falls back
toward kill, which destroys state. This one falls back toward primary, which grants authority.

A detector whose failure mode grants authority is a different severity class from one whose failure
mode destroys state, because it does not need anything else to go wrong.

That sentence belongs on this ticket and I am recording it as theirs.

## Why fleetd #415's antidote does not cover this, and where the guard actually belongs The fleet01 lead made this structural point from an older revision and flagged their premise as unverified. I checked both halves at `136312f`. **Both hold**, and this changes where a worker should put the fix. ### Their premise, confirmed They assumed the scan's error path produces a `Principal` rather than refusing. It does — `CallerResolver.java:247`: ```java return isLoopback(remoteAddr) && c.resolved() ? Principal.primary(c.pid()) : Principal.anonymous(); ``` A failed pane scan leaves a real pid and a `null` terminal, so `c.resolved()` is `true` and this returns `Principal.primary`. ### The structural point `Authz.permits` is a default-less `switch` over `Action` — the exhaustive shape #415 recommends, and it is the right shape: ```java return switch (action) { case SPAWN, STOP, DRAIN, HANDOVER -> caller.isPrimary(); case SEND -> caller.isPrimary() || caller.isArchitect(); case REPLY, ASK -> caller.ownsSession(targetSession); case READ, METRICS -> caller.isPrimary() || caller.isWorker() || caller.isArchitect(); case COORD_READ -> caller.isPrimary(); }; ``` **Exhaustiveness at the decision point cannot protect against a wrong principal.** The switch is total over `Action` and says nothing about whether `caller` was resolved correctly. Hand it a `Principal` that is primary-by-error and every branch votes yes, with the compiler satisfied. I also checked whether anything downstream could notice. It cannot: `Principal` carries `Role`, terminal and name, and **no record of how it was resolved**. There is no field an authorization check could consult even if it wanted to. So the #415 mechanism — make the decision total, so a new case is a compile error — is **orthogonal** to this defect. #415 guards against a decision nobody wrote. This is a decision written correctly and fed a bad input. ### What that means for the fix Do not add an arm to the switch, and do not add a `Role`. **The guard belongs at the resolver**: an explicit "could not tell" that cannot become a `Principal` at all. That is the same third-state fix fleetd #497's family prescribes, applied one layer earlier than the acceptance criteria above put it. Concretely, this sharpens ask 2 in the ticket body. `CallerResolver` returning `Principal.anonymous()` on an incomplete scan is still correct, but the reason is now explicit: the unknown state must be **unrepresentable as a principal**, not merely mapped to a safe one further down. If a future caller of the resolver forgets the check, it should not compile or should get `anonymous` by construction — not silently get `primary` again. ### Severity, stated the way they put it and I agree with `detect_supervisor` returning `none` and this are the same shape — a **detection failure resolving to a definite answer instead of to unknown**. The difference is which answer it picks. `none` falls back toward `kill`, which destroys state. This one falls back toward **primary**, which grants authority. A detector whose failure mode grants authority is a different severity class from one whose failure mode destroys state, **because it does not need anything else to go wrong**. That sentence belongs on this ticket and I am recording it as theirs.
ltms closed this issue 2026-09-12 05:36:17 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#505
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?