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

CB-633: SSH_AUTH_SOCK block wins over allow:, non-zsh fallback reports its gap #191

Closed
agent wants to merge 2 commits from worker/cb-633-fix-5f4396-3 into main
pull from: worker/cb-633-fix-5f4396-3
merge into: fleet:main
Conversation 1 Commits 2 Files Changed 4
+352 -39
agent commented 2026-08-31 03:44:56 +02:00
Member
Copy Link
Copy Source

Supersedes #174.

Fixes two defects found in review of #174, both in HerdrPeerLauncher.applyEnvironmentAllowListPolicy / applyMemberCredentialPolicy:

Defect 1 (high): allowed.addAll(creds.allow()) unioned memberCredentials.allow into the derived allow-list before the sshAuthSockAllowed() check. The live operator fleetd.yaml has sshAuthSock: block AND SSH_AUTH_SOCK on allow: (for other reasons) — under #174 that combination silently defeated the block, so a member kept the operator's ssh-agent socket. Fixed: SSH_AUTH_SOCK is now excluded from the allow: union and is governed only by sshAuthSock:; a conflicting allow: entry gets a one-time WARN naming which control won, with no value printed.

Defect 2 (medium): #174 removed the logCredentialGap call from the non-zsh login-shell fallback branch (warnNonZsh + overlayBlockedCredentials). Under policy: allow-list with a non-zsh login shell — the weakest path, since the overlay can be undone by a sourced startup file — no gap report was produced at all. Restored the call, and logCredentialGap now picks WARN vs INFO wording from whether a scrub-derived allow-list set was actually computed (effectiveAllowed != null) instead of from the policy alone, so this path correctly gets the WARN ("inherited UNBLOCKED") wording rather than the allow-list INFO wording (which would falsely claim a scrub blanks it).

Defect 3 (low, fixed): credentialGapLogged was one shared AtomicBoolean guarding both report kinds. Since memberCredentials is live-reloadable, an early INFO on an allow-list spawn could permanently suppress a later, more serious WARN from a deny-by-default spawn on the same launcher instance. Split into allowListGapLogged/unprotectedGapLogged.

Tests added (HerdrPeerLauncherAllowListWiringTest, ClaudeCodeLauncherTest):

  • sshAuthSockBlockWinsOverAnAllowListEntry — the live-config probe reproducing defect 1; sshAuthSock: block wins even with SSH_AUTH_SOCK on allow:.
  • sshAuthSockAllowKeepsTheVariable — mirror case: sshAuthSock: allow keeps it.
  • nonZshAllowListFallbackStillReportsTheGapAsAWarn — the non-zsh fallback path still reports a WARN gap.
  • anEarlierInfoGapDoesNotSuppressALaterWarnGapOnTheSameLauncher — proves the per-kind guard split.

Each fix was proven by reverting it locally and watching its test fail, then restoring it (see PR description / commit message for the failure text observed).

mvn clean install: Tests run: 976, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.

Supersedes #174. Fixes two defects found in review of #174, both in `HerdrPeerLauncher.applyEnvironmentAllowListPolicy` / `applyMemberCredentialPolicy`: **Defect 1 (high):** `allowed.addAll(creds.allow())` unioned `memberCredentials.allow` into the derived allow-list before the `sshAuthSockAllowed()` check. The live operator `fleetd.yaml` has `sshAuthSock: block` AND `SSH_AUTH_SOCK` on `allow:` (for other reasons) — under #174 that combination silently defeated the block, so a member kept the operator's ssh-agent socket. Fixed: `SSH_AUTH_SOCK` is now excluded from the `allow:` union and is governed only by `sshAuthSock:`; a conflicting `allow:` entry gets a one-time WARN naming which control won, with no value printed. **Defect 2 (medium):** #174 removed the `logCredentialGap` call from the non-zsh login-shell fallback branch (`warnNonZsh` + `overlayBlockedCredentials`). Under `policy: allow-list` with a non-zsh login shell — the weakest path, since the overlay can be undone by a sourced startup file — no gap report was produced at all. Restored the call, and `logCredentialGap` now picks WARN vs INFO wording from whether a scrub-derived allow-list set was actually computed (`effectiveAllowed != null`) instead of from the policy alone, so this path correctly gets the WARN ("inherited UNBLOCKED") wording rather than the allow-list INFO wording (which would falsely claim a scrub blanks it). **Defect 3 (low, fixed):** `credentialGapLogged` was one shared `AtomicBoolean` guarding both report kinds. Since `memberCredentials` is live-reloadable, an early INFO on an allow-list spawn could permanently suppress a later, more serious WARN from a deny-by-default spawn on the same launcher instance. Split into `allowListGapLogged`/`unprotectedGapLogged`. **Tests added** (`HerdrPeerLauncherAllowListWiringTest`, `ClaudeCodeLauncherTest`): - `sshAuthSockBlockWinsOverAnAllowListEntry` — the live-config probe reproducing defect 1; `sshAuthSock: block` wins even with `SSH_AUTH_SOCK` on `allow:`. - `sshAuthSockAllowKeepsTheVariable` — mirror case: `sshAuthSock: allow` keeps it. - `nonZshAllowListFallbackStillReportsTheGapAsAWarn` — the non-zsh fallback path still reports a WARN gap. - `anEarlierInfoGapDoesNotSuppressALaterWarnGapOnTheSameLauncher` — proves the per-kind guard split. Each fix was proven by reverting it locally and watching its test fail, then restoring it (see PR description / commit message for the failure text observed). `mvn clean install`: `Tests run: 976, Failures: 0, Errors: 0, Skipped: 0`, BUILD SUCCESS.
agent added 2 commits 2026-08-31 03:44:56 +02:00
CB-633: honor explicit member credential keeps
CI / build (pull_request) Successful in 1m0s
Details
CI / contract (pull_request) Successful in 1m19s
Details
f6c150e99a
CB-633: sshAuthSock: block must win over an SSH_AUTH_SOCK allow: entry, and report the credential gap on the non-zsh fallback path
CI / contract (pull_request) Successful in 1m14s
Details
CI / build (pull_request) Successful in 1m39s
Details
5da912ae8e 
Fixes two defects in PR #174:

1. applyEnvironmentAllowListPolicy unioned memberCredentials.allow directly into the
   derived allow-list, so an operator who wrote both `sshAuthSock: block` and
   SSH_AUTH_SOCK on `allow:` (the live fleetd.yaml shape) got the block silently
   defeated. SSH_AUTH_SOCK is now excluded from that union and governed only by
   sshAuthSock:, with a one-time WARN when the two controls conflict.

2. The non-zsh login-shell fallback branch dropped its logCredentialGap call, so an
   allow-list spawn on a non-zsh shell produced no gap report at all — exactly the
   weakest, overlay-only path that most needs one. The call is restored, and
   logCredentialGap now picks its WARN/INFO wording from whether a scrub-derived
   allow-list set was actually computed (effectiveAllowed != null) rather than from
   the policy alone, so this path correctly gets the WARN ("inherited UNBLOCKED")
   wording instead of the allow-list INFO wording.

3. credentialGapLogged was one shared AtomicBoolean guarding both report kinds;
   split into allowListGapLogged/unprotectedGapLogged so an early INFO on one spawn
   can no longer suppress a later WARN on a live-reloaded policy.

Each fix is proven by reverting it and observing the corresponding test fail, then
restoring it.
ltms commented 2026-08-31 03:47:30 +02:00
Owner
Copy Link
Copy Source

Closing — my brief was wrong, not your work

I asked for this fix without first diffing #174 against current main. main had already
superseded it, so two of the three defects I sent you do not exist there:

defect I briefed state on main
1 — SSH_AUTH_SOCK widened in by allow: already fixed — MemberEnvAllowList.java:122 excludes it inside derive(profiles, allowSet)
2 — non-zsh fallback lost its gap report already fixed — HerdrPeerLauncher.java:1070 calls it
3 — one-shot guard hides a later WARN still open — main still has a single credentialGapLogged

That is my error and it cost you a full run. The work itself was good: you started from the PR
branch as asked, you did the revert-and-watch-it-fail proof on all three, and you reported the exact
failure text for each. You also flagged honestly that the WiringLauncher fixture bypasses
baseEnv() so defect 3 could not be proved through it, and moved that proof to ClaudeCodeLauncherTest
instead of quietly asserting on the seam. That is the right instinct and it is the thing this repo
keeps getting wrong.

Your caveat about launch.env().keySet() reintroducing SSH_AUTH_SOCK after the exclusion: I
checked it, and no adapter puts that name into the launch env, so it cannot happen today. Worth
having asked.

Two things survive into a ticket against current main: the one-shot guard split (defect 3), and
the INFO/WARN wording split, which main does not have — it uses the single-argument
logCredentialGap everywhere, so under allow-list on zsh it still says names are "inherited
UNBLOCKED" when the scrub does blank them.

Closing this branch because rebasing it onto main would mean reverting the two fixes that are
already there.

## Closing — my brief was wrong, not your work I asked for this fix without first diffing #174 against **current** `main`. `main` had already superseded it, so two of the three defects I sent you do not exist there: | defect I briefed | state on `main` | |---|---| | 1 — `SSH_AUTH_SOCK` widened in by `allow:` | **already fixed** — `MemberEnvAllowList.java:122` excludes it inside `derive(profiles, allowSet)` | | 2 — non-zsh fallback lost its gap report | **already fixed** — `HerdrPeerLauncher.java:1070` calls it | | 3 — one-shot guard hides a later WARN | **still open** — `main` still has a single `credentialGapLogged` | That is my error and it cost you a full run. The work itself was good: you started from the PR branch as asked, you did the revert-and-watch-it-fail proof on all three, and you reported the exact failure text for each. You also flagged honestly that the `WiringLauncher` fixture bypasses `baseEnv()` so defect 3 could not be proved through it, and moved that proof to `ClaudeCodeLauncherTest` instead of quietly asserting on the seam. That is the right instinct and it is the thing this repo keeps getting wrong. Your caveat about `launch.env().keySet()` reintroducing `SSH_AUTH_SOCK` after the exclusion: I checked it, and no adapter puts that name into the launch env, so it cannot happen today. Worth having asked. Two things survive into a ticket against current `main`: the one-shot guard split (defect 3), and the INFO/WARN wording split, which `main` does not have — it uses the single-argument `logCredentialGap` everywhere, so under allow-list on zsh it still says names are "inherited UNBLOCKED" when the scrub does blank them. Closing this branch because rebasing it onto `main` would mean reverting the two fixes that are already there.
ltms closed this pull request 2026-08-31 03:47:35 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 1m14s
Details
CI / build (pull_request) Successful in 1m39s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#191
Delete Branch "worker/cb-633-fix-5f4396-3"

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?