CB-633: honor explicit member credential keeps #174

Closed
agent wants to merge 1 commits from worker/cb-633-allow-list-union-ed374b-1 into main
4 changed files with 148 additions and 42 deletions
@@ -1000,8 +1000,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
*
* <p>CB-633: under {@code policy: allow-list} this overlay is NOT the control anymore — it is
* applied before the login shell runs and a sourced file can (and did) undo it. The control is
* the ZDOTDIR scrub ({@link #applyEnvironmentAllowListPolicy}); {@code known}/{@code allow}
* remain as reporting only via {@link #logCredentialGap}.
* the ZDOTDIR scrub ({@link #applyEnvironmentAllowListPolicy}); {@code allow} adds explicit
* keeps to its derived base, while {@code known} remains reporting metadata.
*/
private void applyMemberCredentialPolicy(Map<String, String> workerEnv) {
FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get();
@@ -1010,8 +1010,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
}
if (!creds.isAllowList()) {
overlayBlockedCredentials(workerEnv, creds);
logCredentialGap(creds, null);
}
logCredentialGap(creds);
}
/** Put {@link #BLOCKED_CREDENTIAL_SENTINEL} over every blocked name in the pane-creation env map. */
@@ -1034,10 +1034,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
* registration, or {@code null} when the policy does not apply.
*
* <p>The allow-list handed to the generator is the derived profile set ({@link
* MemberEnvAllowList#derive}) UNIONed with the exact keys of THIS launch's env map — names the
* daemon itself injects must 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.
* 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.
*/
private Path applyEnvironmentAllowListPolicy(FleetConfig.Profile cfg, Launch launch) {
FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get();
@@ -1053,17 +1054,18 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
// it), but strictly better than nothing.
warnNonZsh(loginShell);
overlayBlockedCredentials(launch.env(), creds);
logCredentialGap(creds);
return null;
}
Set<String> allowed = new java.util.TreeSet<>(MemberEnvAllowList.derive(profiles.values()));
allowed.addAll(creds.allow());
if (creds.sshAuthSockAllowed()) {
allowed.add(SSH_AUTH_SOCK);
} // blocked by default: absent from the set ⇒ blanked by the scrub like any other name
allowed.addAll(launch.env().keySet());
logCredentialGap(creds, allowed);
Path dir = EnvAllowListScrub.generate(Path.of(System.getProperty("java.io.tmpdir")), allowed);
launch.env().put("ZDOTDIR", dir.toAbsolutePath().toString());
log.info("memberCredentials policy=allow-list: profile={} generated ZDOTDIR {} — derived "
log.info("memberCredentials policy=allow-list: profile={} generated ZDOTDIR {} — effective "
+ "allow-list holds {} name(s); the pane reports allowed N of M at release",
cfg.profile(), dir.getFileName(), allowed.size());
return dir;
@@ -1131,34 +1133,47 @@ 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 WARN per launcher instance, not one per spawn. */
/** Guards {@link #logCredentialGap} to one report per launcher instance, not one per spawn. */
private final AtomicBoolean credentialGapLogged = new AtomicBoolean();
/**
* CB-596 criterion 4: a credential-shaped host env var name on neither {@code known} nor
* {@code allow} is not silently allowed — it is reported. {@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, which the daemon has no channel to inspect at spawn time); this logs
* every such NAME, at WARN, 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.
* 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.
*/
private void logCredentialGap(FleetConfig.MemberCredentials creds) {
Set<String> covered = new HashSet<>(creds.known());
covered.addAll(creds.allow());
private void logCredentialGap(FleetConfig.MemberCredentials creds, Set<String> effectiveAllowed) {
Set<String> covered = effectiveAllowed == null
? new HashSet<>(creds.known()) : effectiveAllowed;
if (effectiveAllowed == null) {
covered.addAll(creds.allow());
}
List<String> gap = hostEnvNames.get().stream()
.filter(name -> CREDENTIAL_SHAPED_NAME.matcher(name).matches())
.filter(name -> !covered.contains(name))
.filter(name -> effectiveAllowed == null
? !covered.contains(name) : !MemberEnvAllowList.keeps(covered, name))
.sorted()
.toList();
if (gap.isEmpty()) {
return;
}
if (credentialGapLogged.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);
if (creds.isAllowList()) {
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);
}
}
}
@@ -7,14 +7,16 @@ import java.util.Set;
import java.util.TreeSet;
/**
* CB-633: the set of environment variable NAMES a spawned member is allowed to keep under
* {@code memberCredentials.policy: allow-list} — DERIVED from what the launcher itself injects,
* never hand-typed.
* CB-633: the base set of environment variable NAMES a spawned member is allowed to keep under
* {@code memberCredentials.policy: allow-list}. This class derives the base from what the launcher
* itself injects. The launcher then adds the operator's explicitly named {@code
* memberCredentials.allow} keeps and the exact keys of this launch's env map.
*
* <p>A hand-typed allow-list is the defect this class exists to prevent: a name an operator forgets
* to type is a credential that passes through to every member, and a profile added to config later
* would silently break spawns whose scrub did not know its names. Derivation closes both ends. The
* kept-name set is the union of:
* <p>A hand-typed list that <em>replaces</em> derivation is the defect this class exists to prevent:
* a profile added to config later would silently break spawns whose scrub did not know its names.
* An explicit list that only adds keeps is safe because adding a name can only widen the set; it
* cannot make another spawn lose a name that derivation already kept. The derived base is the union
* of:
*
* <ul>
* <li>every configured {@link FleetConfig.Profile profile}'s {@code gitTokenEnv},
@@ -26,11 +28,11 @@ import java.util.TreeSet;
* or the agent binary genuinely needs to function.</li>
* </ul>
*
* <p>Because the union spans EVERY profile (not just the one spawning), adding a new profile can
* only ever widen the list — it cannot break another spawn's scrub. And because the launcher also
* unions in the exact keys of each spawn's own env map at generation time (see {@code
* HerdrPeerLauncher}), anything the daemon deliberately injects for THIS spawn survives its own
* control.
* <p>Because the base spans EVERY profile (not just the one spawning), adding a new profile can only
* ever widen the list — it cannot break another spawn's scrub. The launcher then adds the operator's
* explicit keeps and the exact keys of each spawn's own env map at generation time (see {@code
* HerdrPeerLauncher}), so member binaries can keep named inherited variables without putting their
* secret values in config, and anything the daemon injects for THIS spawn survives its own control.
*
* <p>{@code SSH_AUTH_SOCK} is deliberately NOT here. It is a handle to the operator's ssh-agent — a
* member holding it can sign with the operator's keys — so keeping it is a config decision
@@ -1025,7 +1025,7 @@ class ClaudeCodeLauncherTest {
* {@code System.getenv()} so the test is deterministic.
*/
@Test
void aCredentialShapedNameOnNeitherListIsLoggedAsAGap() {
void denyByDefaultKeepsTheExactCredentialGapWarn() {
FakeHerdr herdr = new FakeHerdr();
FleetConfig.Profile cfg = new FleetConfig.Profile(
"ltms-local", "http://gx00.gw:8000", "coder", null, "FLEETD_WORKER_TOKEN",
@@ -1045,16 +1045,59 @@ class ClaudeCodeLauncherTest {
logger.detachAppender(appender);
}
String expected = "memberCredentials gap: 1 credential-shaped env var name(s) are on neither "
+ "known: nor allow: — every member pane inherits them UNBLOCKED — "
+ "[A_BRAND_NEW_SECRET_TOKEN]. Add each to memberCredentials.known (blocked by default) "
+ "or .allow (if a member legitimately needs it).";
assertTrue(appender.list.stream().anyMatch(e ->
e.getFormattedMessage().contains("memberCredentials gap")
&& e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")),
"the gap must name the unrecognized credential-shaped var, never a value");
e.getLevel() == ch.qos.logback.classic.Level.WARN
&& e.getFormattedMessage().equals(expected)),
"deny-by-default must keep the exact existing WARN text");
assertNotNull(startEnv(herdr).get("GITEA_ACCESS_TOKEN"),
"deny-by-default must still overlay a known blocked credential");
assertFalse(appender.list.stream().anyMatch(e -> e.getFormattedMessage().contains("PATH")),
"PATH/HOME are not credential-shaped and must not be reported as a gap");
assertFalse(appender.list.stream().anyMatch(e -> e.getFormattedMessage().contains("AI_GATEWAY_TOKEN")),
"a name already on allow: is covered, not a gap");
}
@Test
void allowListReportsAnUnkeptCredentialAsBlankedWithoutSayingUnblocked() {
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);
FleetConfig.MemberCredentials creds = 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,
() -> 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();
} finally {
logger.detachAppender(appender);
logger.setLevel(original);
}
ILoggingEvent gap = appender.list.stream()
.filter(e -> e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN"))
.findFirst().orElseThrow(() -> new AssertionError("allow-list must report the blanked name"));
assertEquals(ch.qos.logback.classic.Level.INFO, gap.getLevel(),
"a scrubbed credential name is the allow-list control working, not a WARN");
assertTrue(gap.getFormattedMessage().contains("will be blanked by the scrub"));
assertFalse(gap.getFormattedMessage().contains("UNBLOCKED"),
"the allow-list report must not claim that a blanked name is exposed");
}
/**
* 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
@@ -8,7 +8,9 @@ import dev.ltms.fleet.peer.Capability;
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 java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashMap;
@@ -21,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
/**
* CB-633: proves the allow-list scrub is actually WIRED INTO the spawn path — not merely that its
@@ -43,6 +46,8 @@ class HerdrPeerLauncherAllowListWiringTest {
/** A name the daemon itself injects — it must survive its own scrub, so it must be allowed. */
private static final String INJECTED = "ANTHROPIC_BASE_URL";
private static final String OPERATOR_KEEP = "CONTEXT7_TOKEN";
private static final String UNLISTED_CREDENTIAL = "A_BRAND_NEW_SECRET_TOKEN";
@Test
void spawningUnderAllowListPolicyGivesThePaneAGeneratedZdotdir() {
@@ -74,6 +79,43 @@ class HerdrPeerLauncherAllowListWiringTest {
+ "), or the daemon's own configuration is blanked by its own control");
}
/** Explicit operator keeps widen the derived base, but all other exported names stay blocked. */
@Test
void operatorKeepSurvivesWhileAnUnlistedCredentialIsBlanked(@TempDir Path home) throws Exception {
Path zsh = Path.of("/bin/zsh");
assumeTrue(Files.isExecutable(zsh), "/bin/zsh not present — nothing to prove here");
FakeHerdr herdr = new FakeHerdr();
WiringLauncher launcher = new WiringLauncher(herdr, allowList(List.of(OPERATOR_KEEP)));
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 \"$" + OPERATOR_KEEP + "\" ]] && print -r -- kept || print -r -- missing; "
+ "[[ -z \"$" + UNLISTED_CREDENTIAL
+ "\" ]] && print -r -- blanked || print -r -- leaked");
probe.environment().clear();
probe.environment().putAll(Map.of(
"HOME", home.toString(),
"PATH", "/usr/bin:/bin",
"SHELL", zsh.toString(),
"ZDOTDIR", zdotdir.toString(),
OPERATOR_KEEP, "needed-by-member-binary",
UNLISTED_CREDENTIAL, "must-not-survive"));
probe.redirectError(ProcessBuilder.Redirect.DISCARD);
Process process = probe.start();
String output = new String(process.getInputStream().readAllBytes(), StandardCharsets.UTF_8);
assertTrue(process.waitFor(60, java.util.concurrent.TimeUnit.SECONDS),
"the zsh scrub probe did not exit within 60 seconds");
assertEquals(0, process.exitValue(), "the zsh scrub probe must exit cleanly");
assertEquals("kept\nblanked\n", output,
"memberCredentials.allow must add a keep, while an unlisted credential stays blanked");
} finally {
launcher.stop(spawned.id());
}
}
/** The default policy must not generate anything — an upgrade changes nothing until asked. */
@Test
void spawningUnderTheDefaultPolicyGeneratesNoZdotdir() {
@@ -104,8 +146,12 @@ class HerdrPeerLauncherAllowListWiringTest {
}
private static Supplier<FleetConfig.MemberCredentials> allowList() {
return allowList(List.of());
}
private static Supplier<FleetConfig.MemberCredentials> allowList(List<String> allow) {
return () -> new FleetConfig.MemberCredentials(
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of(), null);
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, allow, List.of(), null);
}
private static String readAll(Path p) {