From d89ae94a2ea407d1af72b50828b5b70b0861cb47 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 09:17:20 +0700 Subject: [PATCH] CB-192: fix false credential-gap WARN under allow-list+zsh, split its log guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit logCredentialGap(creds) always emitted the WARN wording ("every member pane inherits them UNBLOCKED"), even under memberCredentials.policy: allow-list on a zsh login shell, where the generated ZDOTDIR scrub genuinely blanks the name. The line reported the control working as though it were a hole. Pass an effectiveAllowed set instead: null keeps the WARN (deny-by-default, and the allow-list non-zsh fallback, where nothing is ever scrubbed); the derived allow-list set (only reachable after applyEnvironmentAllowListPolicy's own zsh gate) selects a new INFO wording that says the scrub will blank the name instead of claiming it is inherited unblocked. Also split the single credentialGapLogged AtomicBoolean into two guards (unprotectedGapLogged / allowListGapLogged) — one per report kind. Since memberCredentials is a live, re-read-per-spawn supplier, a shared flag let a harmless allow-list INFO on one spawn permanently suppress a later spawn's real deny-by-default WARN after a policy reload. Fixes gitea #192. --- .../ltms/fleet/member/HerdrPeerLauncher.java | 66 ++++++- .../fleet/member/ClaudeCodeLauncherTest.java | 181 ++++++++++++++++++ 2 files changed, 238 insertions(+), 9 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 76c8f40..fda9c4d 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -1008,6 +1008,13 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { * 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}. + * + *

CB-633 follow-up (#192): under {@code allow-list} this method does NOT call {@link + * #logCredentialGap} itself — at this point (called from {@link #baseEnv}, before {@link + * #applyEnvironmentAllowListPolicy} runs) we do not yet know whether the pane's shell is zsh, so + * we cannot yet pick correct wording. That decision, and the call, are deferred entirely to + * {@link #applyEnvironmentAllowListPolicy}, which knows by then whether the scrub will actually + * run. */ private void applyMemberCredentialPolicy(Map workerEnv) { FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get(); @@ -1016,8 +1023,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. */ @@ -1067,12 +1074,15 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { // logCredentialGap's WARN (below) is the only signal for this path. warnNonZsh(loginShell); overlayBlockedCredentials(launch.env(), creds); - logCredentialGap(creds); + logCredentialGap(creds, null); return null; } // Only reached when the scrub is actually about to run — the count below describes that - // scrub, so it must not be logged before this gate (see the non-zsh branch above). + // scrub, so it must not be logged before this gate (see the non-zsh branch above). Same + // reasoning gates logCredentialGap's wording: passing the derived `allowed` set (non-null) + // here, and ONLY here, is what tells it the scrub will really blank an unkept name — #192. logAllowListCoverage(allowed); + 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 " @@ -1177,18 +1187,46 @@ 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. */ - private final AtomicBoolean credentialGapLogged = new AtomicBoolean(); + /** + * Guards the {@code effectiveAllowed == null} branch of {@link #logCredentialGap} — the + * genuinely-unprotected report (deny-by-default, and the allow-list non-zsh fallback) — to one + * WARN per launcher instance, not one per spawn. + * + *

CB-633 follow-up (#192): kept SEPARATE from {@link #allowListGapLogged} on purpose. + * {@code memberCredentials} is a live, re-read-per-spawn supplier, so the policy can change + * between two spawns on the same launcher. A single shared flag would let a harmless allow-list + * INFO on spawn 1 permanently suppress the real deny-by-default WARN a later spawn deserves — + * the report that matters most getting hidden by the report that doesn't. Two flags mean each + * report kind fires exactly once, independent of what the other kind already logged. + */ + private final AtomicBoolean unprotectedGapLogged = new AtomicBoolean(); + + /** + * Guards the {@code effectiveAllowed != null} branch of {@link #logCredentialGap} — the + * allow-list-scrub-covered report — to one INFO per launcher instance. See {@link + * #unprotectedGapLogged}'s javadoc for why this is a separate flag rather than a shared one. + */ + private final AtomicBoolean allowListGapLogged = 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. + * every such NAME — never a value, a prefix of a value, or a hash of a value, so the log itself + * cannot leak anything. + * + *

CB-633 follow-up (#192): {@code effectiveAllowed} picks the wording, and it must NOT be + * picked from {@code creds.isAllowList()} — see {@link #applyEnvironmentAllowListPolicy}'s + * javadoc for the reasoning this mirrors. {@code null} means no scrub-derived allow-list was + * computed for this call — true on the deny-by-default path AND on the allow-list non-zsh + * fallback, where nothing is ever scrubbed — so the gap is real and gets the WARN, unchanged + * from before this fix. Non-null means this call came from the zsh branch of {@link + * #applyEnvironmentAllowListPolicy}, reachable ONLY after that method's own zsh gate — so the + * generated scrub genuinely will blank these names, and the INFO says so instead of claiming + * they are inherited unblocked. */ - private void logCredentialGap(FleetConfig.MemberCredentials creds) { + private void logCredentialGap(FleetConfig.MemberCredentials creds, Set effectiveAllowed) { Set covered = new HashSet<>(creds.known()); covered.addAll(creds.allow()); List gap = hostEnvNames.get().stream() @@ -1199,7 +1237,17 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { if (gap.isEmpty()) { return; } - if (credentialGapLogged.compareAndSet(false, true)) { + if (effectiveAllowed != null) { + if (allowListGapLogged.compareAndSet(false, true)) { + log.info("memberCredentials gap: {} credential-shaped env var name(s) are on neither " + + "known: nor allow: — {}. The allow-list scrub blanks them anyway (they " + + "are not on the derived allow-list), so no member pane keeps them; add " + + "each to memberCredentials.known or .allow to make that explicit.", + gap.size(), gap); + } + return; + } + 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 " 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..209d938 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java @@ -1,5 +1,6 @@ package dev.ltms.fleet.member; +import ch.qos.logback.classic.Level; import ch.qos.logback.classic.Logger; import ch.qos.logback.classic.spi.ILoggingEvent; import ch.qos.logback.core.read.ListAppender; @@ -1055,6 +1056,186 @@ class ClaudeCodeLauncherTest { "a name already on allow: is covered, not a gap"); } + /** + * CB-633 follow-up (#192): the deny-by-default WARN wording is a promise an operator relies on — + * pinned byte-for-byte so a future edit cannot drift it (e.g. while picking wording for the + * allow-list path) without a test noticing. + */ + @Test + void denyByDefaultKeepsTheExactCredentialGapWarn() { + 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); + ClaudeCodeLauncher svc = new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), + _ -> null, 0, System::currentTimeMillis, () -> {}, null, () -> TEST_MEMBER_CREDENTIALS, + () -> Set.of("A_BRAND_NEW_SECRET_TOKEN")); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + svc.spawn(); + } finally { + logger.detachAppender(appender); + } + + assertTrue(appender.list.stream().anyMatch(e -> + ("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).") + .equals(e.getFormattedMessage())), + "the deny-by-default WARN text must not drift — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + + /** + * CB-633 follow-up (#192), defect 1: under {@code policy: allow-list} on a zsh login shell the + * generated ZDOTDIR scrub genuinely blanks an unkept credential-shaped name, so the report must + * not say the pane inherits it UNBLOCKED — that claim is exactly what PR #174 got wrong. This + * goes through the real spawn path (not {@code HerdrPeerLauncherAllowListWiringTest}'s fixture, + * which overrides {@code buildLaunch} and bypasses none of the logic under test here — the + * shell-dependent branch lives in {@code applyEnvironmentAllowListPolicy}, which every spawn + * still passes through). + */ + @Test + void allowListPolicyOnZshReportsTheGapWithoutClaimingItIsUnblocked() { + 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("AI_GATEWAY_TOKEN"), List.of()); + 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("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN")); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + Level original = logger.getLevel(); + logger.setLevel(Level.INFO); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + svc.spawn(); + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + assertTrue(appender.list.stream().anyMatch(e -> + e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")), + "the unkept name must still be reported — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + assertFalse(appender.list.stream().anyMatch(e -> + e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("UNBLOCKED")), + "on zsh the scrub genuinely blanks the name, so the report must not claim it is " + + "inherited unblocked — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + + /** + * CB-633 follow-up (#192): the mirror of the zsh test above. On a non-zsh login shell {@code + * ZDOTDIR} is ignored, so no scrub ever runs — the report must keep the WARN wording (a name here + * really is inherited unblocked) rather than claiming a scrub protects it. This is the trap PR + * #174 fell into the other direction: keying the wording on the shell, not on {@code + * creds.isAllowList()}, is what keeps this branch correct. + */ + @Test + void allowListPolicyOnNonZshKeepsTheWarnWording() { + 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("AI_GATEWAY_TOKEN"), List.of()); + 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/bash" : null, + 0, System::currentTimeMillis, () -> {}, null, () -> creds, + () -> Set.of("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN")); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + svc.spawn(); + } finally { + logger.detachAppender(appender); + } + + assertTrue(appender.list.stream().anyMatch(e -> + e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("UNBLOCKED") + && e.getFormattedMessage().contains("A_BRAND_NEW_SECRET_TOKEN")), + "no scrub runs on a non-zsh shell, so the WARN wording must be kept — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + assertFalse(appender.list.stream().anyMatch(e -> + e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().toLowerCase(java.util.Locale.ROOT).contains("scrub")), + "nothing is scrubbed on this path, so the report must not claim otherwise — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + + /** + * CB-633 follow-up (#192), defect 2: {@code memberCredentials} is a live, re-read-per-spawn + * supplier, so the policy can change between two spawns on the same launcher. Before this fix a + * single {@code AtomicBoolean} guarded both report kinds, so the harmless allow-list INFO on the + * first spawn would permanently suppress the real deny-by-default WARN a later spawn deserves. + * This goes through {@link ClaudeCodeLauncher#buildLaunch}'s real {@code baseEnv()} path — the + * WARN this test pins fires from {@code applyMemberCredentialPolicy}, which {@code + * HerdrPeerLauncherAllowListWiringTest}'s fixture never reaches at all (see its class javadoc). + */ + @Test + void secondSpawnStillWarnsAfterPolicyChangesFromAllowListToDenyByDefault() { + 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("AI_GATEWAY_TOKEN"), List.of())); + 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("AI_GATEWAY_TOKEN", "A_BRAND_NEW_SECRET_TOKEN")); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + Level original = logger.getLevel(); + logger.setLevel(Level.INFO); + ListAppender appender = new ListAppender<>(); + appender.start(); + try { + logger.addAppender(appender); + svc.spawn(); // allow-list + zsh: harmless INFO, sets the allow-list guard only + + creds.set(new FleetConfig.MemberCredentials(FleetConfig.MemberCredentials.POLICY_DENY_BY_DEFAULT, + List.of("AI_GATEWAY_TOKEN"), List.of())); + svc.spawn(); // policy reloaded to deny-by-default: this WARN must NOT be suppressed + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + assertTrue(appender.list.stream().anyMatch(e -> + e.getFormattedMessage().contains("memberCredentials gap") + && e.getFormattedMessage().contains("UNBLOCKED")), + "the second spawn's deny-by-default WARN must still fire even though the first " + + "spawn's allow-list INFO already logged the same underlying gap — got: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + /** * 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