fleetd#341: a per-name guard so a later spawn's different unprotected name still warns
unprotectedGapLogged was one AtomicBoolean guarding two WARN branches in logCredentialGap that name different env var names (the allow-list keptByDerivedList branch, and warnGapUnprotected's deny-by-default / non-zsh-fallback branch). memberCredentials is a live, re-read-per-spawn supplier, so between two spawns a policy reload can change which names are in the gap: spawn 1 warns about name A and trips the shared flag, and spawn 2's gap containing a different name B never gets its WARN. Replace the AtomicBoolean with unprotectedGapNamesWarned, a ConcurrentHashMap-backed Set<String> guard keyed per name (same shape as OpenCodeLauncher.modelCheckSkippedWarned), so each distinct credential-shaped name is warned about exactly once, ever, regardless of which branch or which spawn first reports it. allowListGapLogged (the separate INFO guard, #192) is untouched. Neither WARN's wording changed.
This commit is contained in:
@@ -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.
|
||||
*
|
||||
* <p>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<String>} 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".
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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<String> 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<String> 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<String> 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<String> 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<String> 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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<FleetConfig.MemberCredentials> credsState = new AtomicReference<>(
|
||||
new FleetConfig.MemberCredentials(null, List.of(), List.of(), null)); // deny-by-default
|
||||
AtomicReference<Set<String>> 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<ILoggingEvent> 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<String> 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
|
||||
|
||||
Reference in New Issue
Block a user