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 eaaf1ae..fc78d26 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -1000,8 +1000,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { * *

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 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. * *

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 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 covered = new HashSet<>(creds.known()); - covered.addAll(creds.allow()); + private void logCredentialGap(FleetConfig.MemberCredentials creds, Set effectiveAllowed) { + Set covered = effectiveAllowed == null + ? new HashSet<>(creds.known()) : effectiveAllowed; + if (effectiveAllowed == null) { + covered.addAll(creds.allow()); + } List 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); + } } } diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java b/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java index 8b7138a..b4670bd 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java @@ -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. * - *

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: + *

A hand-typed list that replaces 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: * *

* - *

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. + *

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. * *

{@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 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 12508ec..780c268 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java @@ -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 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 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 7b85062..2b53417 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java @@ -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 allowList() { + return allowList(List.of()); + } + + private static Supplier allowList(List 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) {