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
CI / build (pull_request) Successful in 1m39s

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.
This commit is contained in:
Dai Ha
2026-08-31 08:44:30 +07:00
parent f6c150e99a
commit 5da912ae8e
3 changed files with 228 additions and 21 deletions
@@ -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<String> 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<String> effectiveAllowed) {
Set<String> 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);
}
}
@@ -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<FleetConfig.MemberCredentials> 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<ILoggingEvent> 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
@@ -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<ILoggingEvent> 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<FleetConfig.MemberCredentials> 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<FleetConfig.MemberCredentials> 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<FleetConfig.MemberCredentials> allowList() {
return allowList(List.of());
}
@@ -180,10 +287,15 @@ class HerdrPeerLauncherAllowListWiringTest {
}
WiringLauncher(FakeHerdr herdr, Supplier<FleetConfig.MemberCredentials> creds, String shell) {
this(herdr, creds, shell, null);
}
WiringLauncher(FakeHerdr herdr, Supplier<FleetConfig.MemberCredentials> creds, String shell,
Supplier<Set<String>> 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