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

CB-633 follow-up: the credential-gap WARN claims names are inherited unblocked when the scrub does blank them, and one guard can hide the real report #192

New Issue
Closed
opened 2026-08-31 03:47:56 +02:00 by ltms · 1 comment
ltms commented 2026-08-31 03:47:56 +02:00
Owner
Copy Link
Copy Source

Salvaged from PRs #174 and #191, both closed as superseded. These two are what main (23ada19)
still does not have. Both are log-accuracy problems, not exposure problems — nothing leaks. But an
operator reads these lines to decide whether members are contained, so a line that is wrong is worse
than no line.

1. The WARN wording is false on the allow-list path

HerdrPeerLauncher.logCredentialGap(creds) has one wording for every caller:

memberCredentials gap: N credential-shaped env var name(s) are on neither known: nor allow: —
every member pane inherits them UNBLOCKED — [...]

That is true on the deny-by-default path and on the non-zsh allow-list fallback, where the only
protection is the CB-596 overlay. It is false under policy: allow-list on a zsh login shell,
which is the live configuration on the Mac: there the generated ZDOTDIR scrub blanks exactly those
names. The line reports the control working as though it were a hole.

Fix: choose the wording from whether a scrub-derived allow-list set was actually computed, not
from creds.isAllowList() alone. PR #191 had this right — it keyed on effectiveAllowed != null,
so the non-zsh fallback (which computes no such set) correctly keeps the WARN while the zsh path
gets an INFO saying the name will be blanked.

The trap to avoid, and PR #174 fell into it: do not key on the policy. Under allow-list with a
non-zsh shell nothing is scrubbed, so an INFO saying "will be blanked by the scrub" is a lie in the
more dangerous direction. Whatever emits the INFO must be reachable only from the branch where the
scrub really runs — the same gate logAllowListCoverage already sits behind
(HerdrPeerLauncher.java:1112, and read its javadoc, it explains this exact reasoning).

2. One AtomicBoolean can suppress the report that matters

HerdrPeerLauncher.java:1181:

private final AtomicBoolean credentialGapLogged = new AtomicBoolean();

One flag per launcher instance, for all report kinds. memberCredentials is live-reloadable and
Fleetd supplies config.get().memberCredentials(), so the policy really can change under a single
launcher. Once the wording splits (item 1), the ordering below becomes possible:

  1. First spawn: allow-list + zsh → logs the harmless INFO, sets the flag.
  2. Operator reloads config to deny-by-default.
  3. Second spawn: names now genuinely inherited unblocked → the WARN is suppressed for the life of
    the daemon    .

Fix: one guard per report kind. PR #191 split it into allowListGapLogged / unprotectedGapLogged
and that was a few lines.

Testing note, carried over from #191

HerdrPeerLauncherAllowListWiringTest.WiringLauncher overrides buildLaunch and so bypasses
baseEnv() entirely. The deny-by-default WARN path in applyMemberCredentialPolicy never fires
through that fixture, so item 2 cannot be proved there — a test written against it would pass with
the fix removed. Put that proof in ClaudeCodeLauncherTest, which exercises the real baseEnv()
path: spawn twice on one launcher with a live supplier that changes policy between spawns, and assert
the second WARN still appears.

This is the recurring shape here (CB-586, CB-611, and the four CB-185 routing defects that all passed
the full suite): a test on the wrong side of the gate proves nothing. Prove each fix by removing it
and watching its test fail.

Acceptance

  • allow-list + zsh: the unkept name is reported, and the message does not claim it is inherited
    unblocked.
  • allow-list + non-zsh: the WARN wording is kept, and the message does not claim a scrub blanks
    anything.
  • deny-by-default: the existing WARN text is unchanged, byte for byte — ClaudeCodeLauncherTest
    already asserts on the exact string.
  • Two spawns on one launcher, allow-list then deny-by-default: the second WARN still fires.
  • Each of the above fails when its fix is reverted, and the failure text is quoted in the PR.
Salvaged from PRs #174 and #191, both closed as superseded. These two are what `main` (`23ada19`) still does not have. Both are log-accuracy problems, not exposure problems — nothing leaks. But an operator reads these lines to decide whether members are contained, so a line that is wrong is worse than no line. ## 1. The WARN wording is false on the allow-list path `HerdrPeerLauncher.logCredentialGap(creds)` has one wording for every caller: > memberCredentials gap: N credential-shaped env var name(s) are on neither known: nor allow: — > **every member pane inherits them UNBLOCKED** — [...] That is true on the deny-by-default path and on the non-zsh allow-list fallback, where the only protection is the CB-596 overlay. It is **false** under `policy: allow-list` on a zsh login shell, which is the live configuration on the Mac: there the generated ZDOTDIR scrub blanks exactly those names. The line reports the control working as though it were a hole. **Fix:** choose the wording from whether a scrub-derived allow-list set was actually computed, not from `creds.isAllowList()` alone. PR #191 had this right — it keyed on `effectiveAllowed != null`, so the non-zsh fallback (which computes no such set) correctly keeps the WARN while the zsh path gets an INFO saying the name will be blanked. **The trap to avoid**, and PR #174 fell into it: do not key on the policy. Under `allow-list` with a non-zsh shell nothing is scrubbed, so an INFO saying "will be blanked by the scrub" is a lie in the more dangerous direction. Whatever emits the INFO must be reachable only from the branch where the scrub really runs — the same gate `logAllowListCoverage` already sits behind (`HerdrPeerLauncher.java:1112`, and read its javadoc, it explains this exact reasoning). ## 2. One `AtomicBoolean` can suppress the report that matters `HerdrPeerLauncher.java:1181`: ```java private final AtomicBoolean credentialGapLogged = new AtomicBoolean(); ``` One flag per launcher instance, for all report kinds. `memberCredentials` is live-reloadable and `Fleetd` supplies `config.get().memberCredentials()`, so the policy really can change under a single launcher. Once the wording splits (item 1), the ordering below becomes possible: 1. First spawn: `allow-list` + zsh → logs the harmless INFO, sets the flag. 2. Operator reloads config to `deny-by-default`. 3. Second spawn: names now genuinely inherited unblocked → the WARN is **suppressed for the life of the daemon**. **Fix:** one guard per report kind. PR #191 split it into `allowListGapLogged` / `unprotectedGapLogged` and that was a few lines. ## Testing note, carried over from #191 `HerdrPeerLauncherAllowListWiringTest.WiringLauncher` overrides `buildLaunch` and so **bypasses `baseEnv()` entirely**. The deny-by-default WARN path in `applyMemberCredentialPolicy` never fires through that fixture, so item 2 cannot be proved there — a test written against it would pass with the fix removed. Put that proof in `ClaudeCodeLauncherTest`, which exercises the real `baseEnv()` path: spawn twice on one launcher with a live supplier that changes policy between spawns, and assert the second WARN still appears. This is the recurring shape here (CB-586, CB-611, and the four CB-185 routing defects that all passed the full suite): a test on the wrong side of the gate proves nothing. **Prove each fix by removing it and watching its test fail.** ## Acceptance - allow-list + zsh: the unkept name is reported, and the message does **not** claim it is inherited unblocked. - allow-list + non-zsh: the WARN wording is kept, and the message does **not** claim a scrub blanks anything. - deny-by-default: the existing WARN text is unchanged, byte for byte — `ClaudeCodeLauncherTest` already asserts on the exact string. - Two spawns on one launcher, allow-list then deny-by-default: the second WARN still fires. - Each of the above fails when its fix is reverted, and the failure text is quoted in the PR.
ltms changed title from CB-633 follow-up: the credential-gap WARN says "inherited UNBLOCKED" about names the scrub does blank, and one guard can hide the real one to CB-633 follow-up: the credential-gap WARN claims names are inherited unblocked when the scrub does blank them, and one guard can hide the real report 2026-08-31 03:48:02 +02:00
ltms commented 2026-08-31 04:33:56 +02:00
Author
Owner
Copy Link
Copy Source

Fixed and merged as 2823349 (PR #194). Lead-verified: merged with #196 onto an integration branch off main, full mvn clean install green at 1025 tests.

Both reported defects fixed:

  1. The WARN no longer fires on the allow-list zsh path claiming names are "inherited UNBLOCKED" when the scrub does blank them.
  2. credentialGapLogged is split into unprotectedGapLogged and allowListGapLogged, so a benign informational report can no longer suppress the serious one for the life of the daemon.

A third defect turned up in review, in the fix itself — worth recording, because it is the same shape as the original bug.

The first cut branched the wording on effectiveAllowed != null but still computed the gap from known ∪ allow, then asserted "they are not on the derived allow-list, so no member pane keeps them". Nothing checked that. effectiveAllowed is a superset of known ∪ allow: MemberEnvAllowList.derive also unions in every profile's gitTokenEnv / gitHostEnv / tokenEnv / env: keys, and derivedAllowedNames unions in the spawn's own env keys on top.

So a profile with tokenEnv: SOME_TOKEN, where that name is in the daemon's environment and the operator forgot to list it, lands in the gap and in the derived allow-list. The scrub keeps it, the member inherits it — and the log would have said it was blanked. A real credential leak reported as safe, which is precisely the inversion this ticket exists to remove.

Now split per name with MemberEnvAllowList.keeps(effectiveAllowed, name) — the same predicate the generated scrub itself evaluates, so the report cannot drift from what the scrub actually does:

  • kept by the derived list → WARN, same severity and same guard as deny-by-default, and the text names why it is kept (a profile setting derives it, or the spawn injects it).
  • blanked by the scrub → INFO, wording unchanged.
  • mixed gap → both lines fire.

Checked against the live fleetd.yaml: all three profile-derived names (AI_GATEWAY_TOKEN, WORKER_GITEA_TOKEN, GITEA_HOST) are on allow:, so this never fired in production. It was latent, and it would have fired on exactly the misconfiguration the log line exists to catch.

The deny-by-default WARN string was confirmed byte-identical to main.

One accepted behaviour change, flagged rather than fixed. The report now happens inside applyEnvironmentAllowListPolicy (after buildLaunch) instead of inside baseEnv. If buildLaunch throws after baseEnv returns, that one spawn attempt logs no gap report. It is self-healing — the guard flags are only consumed when a line actually logs, so the next spawn that completes normally still reports. Accepted: the gap report is a config-health advisory, not a per-spawn control.

Fixed and merged as `2823349` (PR #194). Lead-verified: merged with #196 onto an integration branch off main, full `mvn clean install` green at 1025 tests. **Both reported defects fixed:** 1. The WARN no longer fires on the allow-list zsh path claiming names are "inherited UNBLOCKED" when the scrub does blank them. 2. `credentialGapLogged` is split into `unprotectedGapLogged` and `allowListGapLogged`, so a benign informational report can no longer suppress the serious one for the life of the daemon. **A third defect turned up in review, in the fix itself — worth recording, because it is the same shape as the original bug.** The first cut branched the *wording* on `effectiveAllowed != null` but still computed the gap from `known ∪ allow`, then asserted "they are not on the derived allow-list, so no member pane keeps them". Nothing checked that. `effectiveAllowed` is a **superset** of `known ∪ allow`: `MemberEnvAllowList.derive` also unions in every profile's `gitTokenEnv` / `gitHostEnv` / `tokenEnv` / `env:` keys, and `derivedAllowedNames` unions in the spawn's own env keys on top. So a profile with `tokenEnv: SOME_TOKEN`, where that name is in the daemon's environment and the operator forgot to list it, lands in the gap **and** in the derived allow-list. The scrub keeps it, the member inherits it — and the log would have said it was blanked. **A real credential leak reported as safe**, which is precisely the inversion this ticket exists to remove. Now split per name with `MemberEnvAllowList.keeps(effectiveAllowed, name)` — the same predicate the generated scrub itself evaluates, so the report cannot drift from what the scrub actually does: - kept by the derived list → **WARN**, same severity and same guard as deny-by-default, and the text names *why* it is kept (a profile setting derives it, or the spawn injects it). - blanked by the scrub → **INFO**, wording unchanged. - mixed gap → both lines fire. Checked against the live `fleetd.yaml`: all three profile-derived names (`AI_GATEWAY_TOKEN`, `WORKER_GITEA_TOKEN`, `GITEA_HOST`) are on `allow:`, so this never fired in production. It was latent, and it would have fired on exactly the misconfiguration the log line exists to catch. The deny-by-default WARN string was confirmed byte-identical to main. **One accepted behaviour change, flagged rather than fixed.** The report now happens inside `applyEnvironmentAllowListPolicy` (after `buildLaunch`) instead of inside `baseEnv`. If `buildLaunch` throws after `baseEnv` returns, that one spawn attempt logs no gap report. It is self-healing — the guard flags are only consumed when a line actually logs, so the next spawn that completes normally still reports. Accepted: the gap report is a config-health advisory, not a per-spawn control.
ltms closed this issue 2026-08-31 04:34:01 +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#192
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?