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 980d885..c33863b 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -1665,30 +1665,59 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** * 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. + * genuinely-unprotected report (deny-by-default, and the allow-list non-zsh fallback) — AND + * the {@code effectiveAllowed != null} / {@code keptByDerivedList} branch, the allow-list case + * where a name is in the gap but the derived allow-list keeps it anyway. Both branches log the + * same severity (WARN) about the same fact — a name genuinely reaching a member pane + * unprotected — so they share this one guard, keyed per NAME rather than per launcher instance: + * each credential-shaped name that is ever reported unprotected gets exactly one WARN, however + * many spawns see it and whichever of the two branches first reports it. + * + *

fleetd #341: {@code memberCredentials} is a live, re-read-per-spawn supplier, so the + * policy — and so the gap's actual member names — can change between two spawns on the same + * launcher. Before this fix the guard was a single {@code AtomicBoolean} tripped by either + * branch: spawn 1 could warn about name A and trip the flag, and a later spawn's gap containing + * a different name B would never be reported, even though B is just as unprotected as A was. + * {@code AtomicBoolean} could not express "once per distinct name" at all — only "once, ever, + * for whichever name got there first" — so this is a {@code Set} guard instead, the + * same shape {@link OpenCodeLauncher#modelCheckSkippedWarned} already uses for its own + * once-per-distinct-thing WARN. {@link #add}'s return value (true only the first time a name is + * added) is what turns "log the whole gap" into "log only the names never warned about before". + * + *

Bounded by construction: every name added here first passed {@link + * #CREDENTIAL_SHAPED_NAME}'s filter over {@link #hostEnvNames}, i.e. it is an actual + * environment variable name from the daemon's own process — a small, OS-bounded set (the host + * environment has, in practice, tens to a few hundred entries), not an attacker- or + * request-controlled input. So this set cannot grow past "however many distinct credential- + * shaped names this host's environment has ever held across this launcher's lifetime," which is + * effectively fixed for the life of one daemon process — no separate cap is needed. * *

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. + * report kind (WARN vs. INFO) fires independently of what the other kind already logged; within + * the WARN kind itself, the set above further separates by name, for the same reason. */ - private final AtomicBoolean unprotectedGapLogged = new AtomicBoolean(); + private final Set unprotectedGapNamesWarned = ConcurrentHashMap.newKeySet(); /** * 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. + * #unprotectedGapNamesWarned}'s javadoc for why this is a separate flag rather than a shared + * one; unlike that guard it stays a per-instance {@code AtomicBoolean}, not a per-name set — + * fleetd #341 fixed the WARN-vs-WARN suppression, not this INFO's own one-shot shape, which was + * not reported as broken and is out of that ticket's scope. */ private final AtomicBoolean allowListGapLogged = new AtomicBoolean(); /** * fleetd #185 stage 2: guards {@link #warnUnknownMemberEnvironment} to one WARN per launcher - * instance, not one per spawn — the same one-per-instance shape as {@link #unprotectedGapLogged} - * and {@link #allowListGapLogged}, kept as its own flag for the same reason those two are split: - * this mode is orthogonal to which of the other two branches would otherwise have fired. + * instance, not one per spawn — the same one-shot shape {@link #unprotectedGapNamesWarned} and + * {@link #allowListGapLogged} guard their own branches with, kept as its own flag for the same + * reason those two are split: this mode is orthogonal to which of the other two branches would + * otherwise have fired. */ private final AtomicBoolean unknownMemberEnvironmentWarned = new AtomicBoolean(); @@ -1816,14 +1845,21 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { List blankedByScrub = gap.stream() .filter(name -> !MemberEnvAllowList.keeps(effectiveAllowed, name)) .toList(); - if (!keptByDerivedList.isEmpty() && unprotectedGapLogged.compareAndSet(false, true)) { + // fleetd #341: filter to names this guard has never warned about before — not just + // "isEmpty" on the whole branch — so a name this spawn's gap shares with an EARLIER + // spawn's (already-warned) gap does not re-print, while a name unique to THIS gap still + // does, whichever of the two WARN branches reported it first. + List newlyUnprotected = keptByDerivedList.stream() + .filter(unprotectedGapNamesWarned::add) + .toList(); + if (!newlyUnprotected.isEmpty()) { log.warn("memberCredentials gap: {} credential-shaped env var name(s) are on neither " + "known: nor allow: — the derived allow-list keeps them anyway (a profile's " + "gitTokenEnv/gitHostEnv/tokenEnv/env: names one, or this spawn injects it), " + "so every member pane inherits them UNBLOCKED — {}. Add each to " + "memberCredentials.known (or .allow if a member legitimately needs it), or " + "remove it from whatever profile setting derives it in.", - keptByDerivedList.size(), keptByDerivedList); + newlyUnprotected.size(), newlyUnprotected); } if (!blankedByScrub.isEmpty() && allowListGapLogged.compareAndSet(false, true)) { log.info("memberCredentials gap: {} credential-shaped env var name(s) are on neither " @@ -1836,12 +1872,18 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** The deny-by-default (and allow-list non-zsh fallback) WARN — unchanged byte-for-byte by #192. */ private void warnGapUnprotected(List gap) { - if (unprotectedGapLogged.compareAndSet(false, true)) { + // fleetd #341: same "only the names never warned before" filter as the sibling branch in + // logCredentialGap above — see unprotectedGapNamesWarned's javadoc. Both branches share + // this one guard because both report the exact same fact (a name reaching a member pane + // unprotected) at the exact same severity; keying it by name is what lets a later spawn's + // DIFFERENT name still get its own WARN after an earlier spawn's already fired. + List newlyUnprotected = gap.stream().filter(unprotectedGapNamesWarned::add).toList(); + if (!newlyUnprotected.isEmpty()) { 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); + newlyUnprotected.size(), newlyUnprotected); } } 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 50611d3..2340878 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.function.Supplier; @@ -370,6 +371,69 @@ class HerdrPeerLauncherAllowListWiringTest { "expected the pre-existing 'scrub blanks them' INFO unchanged, got: " + messages); } + /** + * fleetd #341: {@code unprotectedGapLogged} guarded TWO WARN branches that name DIFFERENT env + * var names — the allow-list branch ({@code keptByDerivedList}, below) and the deny-by-default + * / non-zsh-fallback branch ({@link HerdrPeerLauncher#warnGapUnprotected}). {@code + * memberCredentials} is a live, re-read-per-spawn supplier, so the policy can change between + * two spawns on the same launcher instance — a config reload needs no restart. Spawn 1 runs + * under {@code deny-by-default} with a gap of {@code SPAWN_ONE_UNCOVERED_TOKEN}, which trips + * the (before this fix) SHARED one-shot flag. The policy is then reloaded to {@code + * allow-list}; spawn 2's gap is {@code FLEETD_WORKER_TOKEN} instead — the test profile's own + * {@code tokenEnv}, which the derived allow-list keeps even though it is on neither {@code + * known:} nor {@code allow:}, so it is genuinely unprotected and deserves its own WARN. Before + * this fix that WARN never fires, because the shared flag was already {@code true} — the + * operator is never told {@code FLEETD_WORKER_TOKEN} reaches every member pane unblocked. Real + * path: two real {@link HerdrPeerLauncher#spawn} calls on ONE launcher instance, with mutable + * {@code memberCredentials}/host-env suppliers standing in for a live config reload between + * spawns. + */ + @Test + void aDifferentUnprotectedGapOnALaterSpawnIsNotSuppressedByAnEarlierSpawnsWarn() { + FakeHerdr herdr = new FakeHerdr(); + AtomicReference credsState = new AtomicReference<>( + new FleetConfig.MemberCredentials(null, List.of(), List.of(), null)); // deny-by-default + AtomicReference> hostEnvState = + new AtomicReference<>(Set.of("SPAWN_ONE_UNCOVERED_TOKEN")); + WiringLauncher launcher = new WiringLauncher(herdr, credsState::get, "/bin/zsh", hostEnvState::get); + + Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class); + Level original = logger.getLevel(); + logger.setLevel(Level.WARN); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + // Spawn 1: deny-by-default, gap = {SPAWN_ONE_UNCOVERED_TOKEN} — the effectiveAllowed == + // null branch, via warnGapUnprotected. + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + + // Live policy reload to allow-list, with a DIFFERENT gap name. + credsState.set(new FleetConfig.MemberCredentials( + FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of(), null)); + hostEnvState.set(Set.of("FLEETD_WORKER_TOKEN")); + // Spawn 2: allow-list, gap = {FLEETD_WORKER_TOKEN} — kept by the derived allow-list + // (the profile's own tokenEnv), so it is the effectiveAllowed != null / keptByDerivedList + // branch, at the SAME log line HerdrPeerLauncher:1819 guards with the shared flag. + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + } finally { + logger.detachAppender(appender); + logger.setLevel(original); + } + + List messages = appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList(); + assertTrue(messages.stream().anyMatch( + m -> m.contains("UNBLOCKED") && m.contains("SPAWN_ONE_UNCOVERED_TOKEN")), + "spawn 1's deny-by-default gap must still warn — got: " + messages); + assertTrue(messages.stream().anyMatch( + m -> m.contains("UNBLOCKED") && m.contains("FLEETD_WORKER_TOKEN")), + "spawn 2's gap names a DIFFERENT env var than spawn 1 (FLEETD_WORKER_TOKEN, not " + + "SPAWN_ONE_UNCOVERED_TOKEN) — it must still be warned about even though a " + + "flag already fired once for spawn 1's unrelated name. Before fleetd #341's " + + "fix this WARN never fires because unprotectedGapLogged was already true. " + + "Got: " + messages); + } + /** * fleetd #185 stage 2: with {@code memberHerdrSocket:} configured, member panes run under a * different OS user — {@link HerdrPeerLauncher#hostEnvNames} describes fleetd's own process, not