From 5da912ae8e0d8d9b94b779e666e6dbb029c4d77b Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 08:44:30 +0700 Subject: [PATCH] CB-633: sshAuthSock: block must win over an SSH_AUTH_SOCK allow: entry, and report the credential gap on the non-zsh fallback path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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/fleet/member/HerdrPeerLauncher.java | 90 +++++++++++--- .../fleet/member/ClaudeCodeLauncherTest.java | 45 +++++++ .../HerdrPeerLauncherAllowListWiringTest.java | 114 +++++++++++++++++- 3 files changed, 228 insertions(+), 21 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java index fc78d26..0edc86d 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -1037,8 +1037,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { * MemberEnvAllowList#derive}) UNIONed with {@code memberCredentials.allow} and the exact keys of * THIS launch's env map. This lets the operator keep inherited variables by name without putting * their values in config, and names the daemon itself injects survive its own control. {@code - * SSH_AUTH_SOCK} is added ONLY when the config explicitly allows it; by default it is absent, so - * the scrub blanks it like any other non-derived name. + * SSH_AUTH_SOCK} is EXCLUDED from that union and added ONLY when {@code + * memberCredentials.sshAuthSock: allow} — {@code sshAuthSock} is the sole control for that one + * name, so it cannot be widened back in by naming it on {@code allow:} too; a conflicting entry + * there gets a WARN ({@link #warnSshAuthSockConflict}) and loses. By default {@code + * SSH_AUTH_SOCK} is absent, so the scrub blanks it like any other non-derived name. */ private Path applyEnvironmentAllowListPolicy(FleetConfig.Profile cfg, Launch launch) { FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get(); @@ -1054,12 +1057,25 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { // it), but strictly better than nothing. warnNonZsh(loginShell); overlayBlockedCredentials(launch.env(), creds); + // Nothing is scrubbed on this path — it is the CB-596 overlay only, which a sourced + // startup file can undo. Report the gap with the WARN wording (an inherited-unblocked + // exposure), not the allow-list INFO wording, which would falsely claim a scrub blanks it. + logCredentialGap(creds, null); return null; } Set allowed = new java.util.TreeSet<>(MemberEnvAllowList.derive(profiles.values())); - allowed.addAll(creds.allow()); + for (String name : creds.allow()) { + // SSH_AUTH_SOCK is governed ONLY by sshAuthSock: below, never by allow: — an operator + // who writes both `sshAuthSock: block` and `SSH_AUTH_SOCK` on `allow:` means the block + // to win, not to be silently widened away by the second control. + if (!SSH_AUTH_SOCK.equals(name)) { + allowed.add(name); + } + } if (creds.sshAuthSockAllowed()) { allowed.add(SSH_AUTH_SOCK); + } else if (creds.allow().contains(SSH_AUTH_SOCK)) { + warnSshAuthSockConflict(); } // blocked by default: absent from the set ⇒ blanked by the scrub like any other name allowed.addAll(launch.env().keySet()); logCredentialGap(creds, allowed); @@ -1074,6 +1090,23 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** The operator ssh-agent handle — kept ONLY by explicit config decision, never by default. */ private static final String SSH_AUTH_SOCK = "SSH_AUTH_SOCK"; + /** Guards {@link #warnSshAuthSockConflict} to one WARN per launcher instance, not one per spawn. */ + private final AtomicBoolean sshAuthSockConflictWarned = new AtomicBoolean(); + + /** + * Defect fix: {@code SSH_AUTH_SOCK} named on {@code memberCredentials.allow} while + * {@code sshAuthSock} is not {@code allow} is a conflict between the two controls — say which + * one wins, once per launcher instance, without printing any value. + */ + private void warnSshAuthSockConflict() { + if (sshAuthSockConflictWarned.compareAndSet(false, true)) { + log.warn("memberCredentials policy=allow-list: SSH_AUTH_SOCK is named on allow: but " + + "sshAuthSock is not 'allow' — sshAuthSock: block wins and SSH_AUTH_SOCK " + + "stays blocked. Remove it from allow: or set sshAuthSock: allow if the " + + "member should keep it."); + } + } + /** * CB-633: a non-zsh login shell means the allow-list control CANNOT run — say so once per * launcher instance, naming the shell, instead of failing silently. @@ -1133,18 +1166,30 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { private static final Pattern CREDENTIAL_SHAPED_NAME = Pattern.compile("(?i).*(TOKEN|SECRET|_KEY|APIKEY|PASSWORD|CREDENTIAL|AUTH).*"); - /** Guards {@link #logCredentialGap} to one report per launcher instance, not one per spawn. */ - private final AtomicBoolean credentialGapLogged = new AtomicBoolean(); + /** + * Guards {@link #logCredentialGap}'s two report kinds separately — one flag per wording, not + * one shared flag — so an early INFO ("will be blanked by the scrub") on one spawn can never + * suppress a later, more serious WARN ("inherited UNBLOCKED") on another. {@code + * memberCredentials} is live-reloadable, so the policy really can change between spawns on the + * same launcher instance. + */ + private final AtomicBoolean allowListGapLogged = new AtomicBoolean(); + + /** See {@link #allowListGapLogged} — the WARN-wording counterpart. */ + private final AtomicBoolean unprotectedGapLogged = new AtomicBoolean(); /** * CB-596 criterion 4: report credential-shaped host env var names that the active policy does - * not classify. Under deny-by-default, a name on neither {@code known} nor {@code allow} still - * gets the original WARN because it is inherited unblocked. Under allow-list, a name absent from - * the effective kept-name set gets an INFO stating that the scrub will blank it, because that is - * the control working rather than an exposure. {@link #hostEnvNames} enumerates the daemon's own - * environment (see that field's javadoc for why the daemon's env is read rather than the spawned - * pane's). This logs names only, at most once per launcher instance — never a value, a prefix of - * a value, or a hash of a value, so the log itself cannot leak anything. + * not classify. Callers that pass a non-null {@code effectiveAllowed} are reporting against a + * kept-name set a scrub will actually enforce — an absent name gets an INFO stating that the + * scrub will blank it, because that is the control working rather than an exposure. Every + * other caller — deny-by-default, and an allow-list spawn whose scrub could not run (non-zsh + * login shell, falls back to the overlay) — passes {@code null} and gets the original WARN, + * because in both cases a gap name is inherited unblocked. {@link #hostEnvNames} enumerates the + * daemon's own environment (see that field's javadoc for why the daemon's env is read rather + * than the spawned pane's). This logs names only, at most once per launcher instance per report + * kind — never a value, a prefix of a value, or a hash of a value, so the log itself cannot + * leak anything. */ private void logCredentialGap(FleetConfig.MemberCredentials creds, Set effectiveAllowed) { Set covered = effectiveAllowed == null @@ -1161,19 +1206,24 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { if (gap.isEmpty()) { return; } - if (credentialGapLogged.compareAndSet(false, true)) { - if (creds.isAllowList()) { + // effectiveAllowed != null means a derived allow-list set was actually computed (the + // scrub will run against it) — that is the INFO case. A null effectiveAllowed means no + // scrub protects this spawn (deny-by-default, or an allow-list spawn that fell back to + // the overlay-only path because the login shell is not zsh) — that is the WARN case. + // See #allowListGapLogged for why the two kinds use separate guards. + if (effectiveAllowed != null) { + if (allowListGapLogged.compareAndSet(false, true)) { log.info("memberCredentials allow-list: {} credential-shaped host env var name(s) " + "are not kept and will be blanked by the scrub — {}. Add any name " + "a member legitimately needs to memberCredentials.allow.", gap.size(), gap); - } else { - log.warn("memberCredentials gap: {} credential-shaped env var name(s) are on neither " - + "known: nor allow: — every member pane inherits them UNBLOCKED — {}. " - + "Add each to memberCredentials.known (blocked by default) or .allow " - + "(if a member legitimately needs it).", - gap.size(), gap); } + } else if (unprotectedGapLogged.compareAndSet(false, true)) { + log.warn("memberCredentials gap: {} credential-shaped env var name(s) are on neither " + + "known: nor allow: — every member pane inherits them UNBLOCKED — {}. " + + "Add each to memberCredentials.known (blocked by default) or .allow " + + "(if a member legitimately needs it).", + gap.size(), gap); } } diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java index 780c268..4ec48a8 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java @@ -1098,6 +1098,51 @@ class ClaudeCodeLauncherTest { "the allow-list report must not claim that a blanked name is exposed"); } + /** + * Defect fix: {@code memberCredentials} is live-reloadable, so the policy really can change + * between spawns on the SAME launcher instance. An earlier INFO gap report (allow-list) must + * not permanently suppress a later, more serious WARN gap report (deny-by-default) — the two + * report kinds need separate guards, not one shared flag. + */ + @Test + void anEarlierInfoGapDoesNotSuppressALaterWarnGapOnTheSameLauncher() { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = new FleetConfig.Profile( + "ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN", + List.of("claude"), "tab", "fleetd-workers", "worker: {profile} #{n}", null, null, null); + AtomicReference creds = new AtomicReference<>( + new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of(), null)); + ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), + name -> "SHELL".equals(name) ? "/bin/zsh" : null, + 0, System::currentTimeMillis, () -> {}, null, creds::get, + () -> Set.of("PATH", "HOME", "A_BRAND_NEW_SECRET_TOKEN")); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + ch.qos.logback.classic.Level original = logger.getLevel(); + logger.setLevel(ch.qos.logback.classic.Level.INFO); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + svc.spawn(); // allow-list: logs the INFO gap report first + creds.set(new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_DENY_BY_DEFAULT, List.of(), List.of(), null)); + svc.spawn(); // deny-by-default: must still log its own WARN gap report + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + assertTrue(appender.list.stream().anyMatch(e -> + e.getLevel() == ch.qos.logback.classic.Level.WARN + && e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN") + && e.getFormattedMessage().contains("UNBLOCKED")), + "a prior INFO gap report must not suppress a later WARN gap report on the same " + + "launcher instance"); + } + /** * The half of CB-592 that can actually survive the pane's login shell. BRIDGED_MEMBER is a name * secrets.sh never exports, so nothing overwrites it — measured: GITEA_TOKEN is injected the diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java index 2b53417..a403ac6 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java @@ -1,5 +1,8 @@ package dev.ltms.fleet.member; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import dev.ltms.fleet.config.FleetConfig; import dev.ltms.fleet.herdr.AgentControl; import dev.ltms.fleet.herdr.FakeHerdr; @@ -9,6 +12,7 @@ import dev.ltms.fleet.peer.MemberRole; import dev.ltms.fleet.peer.SpawnRequest; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.slf4j.LoggerFactory; import java.nio.charset.StandardCharsets; import java.nio.file.Files; @@ -145,6 +149,109 @@ class HerdrPeerLauncherAllowListWiringTest { "bash ignores ZDOTDIR; setting it would be protection theatre"); } + /** + * Defect fix: under {@code policy: allow-list} with a non-zsh login shell, nothing is + * scrubbed — the overlay-only fallback is the whole protection. The gap must still be + * reported, and with the deny-by-default WARN wording ("inherited UNBLOCKED"), never the + * allow-list INFO wording ("will be blanked by the scrub"), because nothing is scrubbed here. + */ + @Test + void nonZshAllowListFallbackStillReportsTheGapAsAWarn() { + FakeHerdr herdr = new FakeHerdr(); + WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/bash", + () -> Set.of("PATH", "HOME", UNLISTED_CREDENTIAL)); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + } finally { + logger.detachAppender(appender); + } + + ILoggingEvent gap = appender.list.stream() + .filter(e -> e.getFormattedMessage().contains(UNLISTED_CREDENTIAL)) + .findFirst().orElseThrow(() -> new AssertionError( + "the non-zsh allow-list fallback must still report the credential gap")); + assertEquals(ch.qos.logback.classic.Level.WARN, gap.getLevel(), + "nothing is scrubbed on this path, so the report must use the WARN wording, not " + + "the allow-list INFO wording"); + assertTrue(gap.getFormattedMessage().contains("UNBLOCKED"), + "the fallback path scrubs nothing, so the wording must say so"); + assertFalse(gap.getFormattedMessage().contains("will be blanked by the scrub"), + "nothing is scrubbed on this path — that wording would be false here"); + } + + /** + * The live operator config has {@code SSH_AUTH_SOCK} on BOTH {@code allow:} AND + * {@code sshAuthSock: block}. Defect fix: {@code sshAuthSock: block} must win — the allow: + * entry must not silently widen the derived allow-list to include it. + */ + @Test + void sshAuthSockBlockWinsOverAnAllowListEntry(@TempDir Path home) throws Exception { + Path zsh = Path.of("/bin/zsh"); + assumeTrue(Files.isExecutable(zsh), "/bin/zsh not present"); + FakeHerdr herdr = new FakeHerdr(); + Supplier live = () -> new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, + List.of("AI_GATEWAY_TOKEN", "WORKER_GITEA_TOKEN", "CONTEXT7_TOKEN", "GITEA_HOST", + "OPENCODE_AUTOMODE_MODEL", "SSH_AUTH_SOCK", "CLAUDE_CODE_MESSAGING_TOKEN"), + List.of(), "block"); + WiringLauncher launcher = new WiringLauncher(herdr, live); + var spawned = launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + try { + Path zdotdir = Path.of(launcher.env.get("ZDOTDIR")); + ProcessBuilder probe = new ProcessBuilder(zsh.toString(), "-i", "-c", + "[[ -n \"$SSH_AUTH_SOCK\" ]] && print -r -- LEAKED || print -r -- blanked"); + probe.environment().clear(); + probe.environment().putAll(Map.of( + "HOME", home.toString(), "PATH", "/usr/bin:/bin", "SHELL", zsh.toString(), + "ZDOTDIR", zdotdir.toString(), "SSH_AUTH_SOCK", "/tmp/agent.sock")); + probe.redirectError(ProcessBuilder.Redirect.DISCARD); + Process pr = probe.start(); + String out = new String(pr.getInputStream().readAllBytes(), StandardCharsets.UTF_8); + pr.waitFor(60, java.util.concurrent.TimeUnit.SECONDS); + assertEquals("blanked\n", out, "sshAuthSock: block must win over allow:"); + } finally { + launcher.stop(spawned.id()); + } + } + + /** + * Mirror of {@link #sshAuthSockBlockWinsOverAnAllowListEntry}: with {@code sshAuthSock: allow} + * (no conflict with {@code allow:}), {@code SSH_AUTH_SOCK} IS kept. Without this case, a fix + * that simply deletes {@code SSH_AUTH_SOCK} everywhere would also pass the block test. + */ + @Test + void sshAuthSockAllowKeepsTheVariable(@TempDir Path home) throws Exception { + Path zsh = Path.of("/bin/zsh"); + assumeTrue(Files.isExecutable(zsh), "/bin/zsh not present"); + FakeHerdr herdr = new FakeHerdr(); + Supplier creds = () -> new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, + List.of("SSH_AUTH_SOCK"), List.of(), "allow"); + WiringLauncher launcher = new WiringLauncher(herdr, creds); + var spawned = launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + try { + Path zdotdir = Path.of(launcher.env.get("ZDOTDIR")); + ProcessBuilder probe = new ProcessBuilder(zsh.toString(), "-i", "-c", + "[[ -n \"$SSH_AUTH_SOCK\" ]] && print -r -- kept || print -r -- blanked"); + probe.environment().clear(); + probe.environment().putAll(Map.of( + "HOME", home.toString(), "PATH", "/usr/bin:/bin", "SHELL", zsh.toString(), + "ZDOTDIR", zdotdir.toString(), "SSH_AUTH_SOCK", "/tmp/agent.sock")); + probe.redirectError(ProcessBuilder.Redirect.DISCARD); + Process pr = probe.start(); + String out = new String(pr.getInputStream().readAllBytes(), StandardCharsets.UTF_8); + pr.waitFor(60, java.util.concurrent.TimeUnit.SECONDS); + assertEquals("kept\n", out, "sshAuthSock: allow must keep SSH_AUTH_SOCK"); + } finally { + launcher.stop(spawned.id()); + } + } + private static Supplier allowList() { return allowList(List.of()); } @@ -180,10 +287,15 @@ class HerdrPeerLauncherAllowListWiringTest { } WiringLauncher(FakeHerdr herdr, Supplier creds, String shell) { + this(herdr, creds, shell, null); + } + + WiringLauncher(FakeHerdr herdr, Supplier creds, String shell, + Supplier> hostEnvNames) { super("test", new AgentControl(herdr), new WorkspaceControl(herdr), Map.of("test", profile()), "test", name -> "SHELL".equals(name) ? shell : null, - 0, () -> 0L, () -> { }, null, creds); + 0, () -> 0L, () -> { }, null, creds, hostEnvNames); } @Override