CB-633 follow-up: only log allowed N of M when the scrub actually runs
The coverage line was logged before the zsh gate, so a non-zsh spawn (where nothing is scrubbed — overlayBlockedCredentials is the fallback instead) printed 'allowed N of M' as if the derived allow-list scrub had run. Move the log after the gate so it only fires on the path that actually generates the ZDOTDIR scrub; the non-zsh fallback keeps logCredentialGap's WARN as its only signal. Added a test proving no 'allowed N of M' line is emitted on the non-zsh fallback, through the real HerdrPeerLauncher#spawn path.
This commit is contained in:
@@ -1049,19 +1049,24 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
return null;
|
||||
}
|
||||
Set<String> allowed = derivedAllowedNames(creds, launch);
|
||||
logAllowListCoverage(allowed);
|
||||
String loginShell = resolveEnv("SHELL");
|
||||
boolean zsh = loginShell != null && (loginShell.endsWith("/zsh") || loginShell.equals("zsh"));
|
||||
if (!zsh) {
|
||||
// A non-zsh login shell ignores ZDOTDIR entirely: NO scrub would run, so pretending
|
||||
// otherwise would be worse than saying so. Warn loudly and fall back to the CB-596
|
||||
// sentinel overlay over the enumerated known: names — weaker (a sourced file can undo
|
||||
// it), but strictly better than nothing.
|
||||
// it), but strictly better than nothing. Deliberately no "allowed N of M" line here: the
|
||||
// scrub this count describes does not run on this path, so printing it would tell an
|
||||
// operator that a fraction of names were blocked when the real number blocked is zero.
|
||||
// logCredentialGap's WARN (below) is the only signal for this path.
|
||||
warnNonZsh(loginShell);
|
||||
overlayBlockedCredentials(launch.env(), creds);
|
||||
logCredentialGap(creds);
|
||||
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).
|
||||
logAllowListCoverage(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 "
|
||||
@@ -1086,13 +1091,17 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
}
|
||||
|
||||
/**
|
||||
* CB-633 follow-up: one INFO line per allow-list spawn, so an operator can read a single log
|
||||
* line and know the scrub ran and how much of the visible environment it will keep. {@code M} is
|
||||
* {@link #hostEnvNames}' size (the daemon's own environment — see that field's javadoc for why it
|
||||
* stands in for the pane's, which the daemon has no channel to inspect at spawn time) and
|
||||
* {@code N} is how many of those names survive {@code allowed} (including the {@code LC_*}
|
||||
* prefix rule). Neither number is a constant: both come from the actual derived set and the
|
||||
* actual environment this spawn sees. Never logs a variable NAME or VALUE — only the counts.
|
||||
* CB-633 follow-up: one INFO line per allow-list spawn WHOSE SCRUB ACTUALLY RUNS, so an operator
|
||||
* can read a single log line and know the scrub ran and how much of the visible environment it
|
||||
* will keep. Callable ONLY from the zsh branch of {@link #applyEnvironmentAllowListPolicy}, after
|
||||
* the shell gate — logging it before that gate (or on the non-zsh fallback, where nothing is
|
||||
* scrubbed) would tell an operator a fraction of names were blocked when the real number blocked
|
||||
* is zero, which is worse than not logging at all. {@code M} is {@link #hostEnvNames}' size (the
|
||||
* daemon's own environment — see that field's javadoc for why it stands in for the pane's, which
|
||||
* the daemon has no channel to inspect at spawn time) and {@code N} is how many of those names
|
||||
* survive {@code allowed} (including the {@code LC_*} prefix rule). Neither number is a constant:
|
||||
* both come from the actual derived set and the actual environment this spawn sees. Never logs a
|
||||
* variable NAME or VALUE — only the counts.
|
||||
*/
|
||||
private void logAllowListCoverage(Set<String> allowed) {
|
||||
Set<String> hostNames = hostEnvNames.get();
|
||||
|
||||
@@ -196,6 +196,39 @@ class HerdrPeerLauncherAllowListWiringTest {
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
/**
|
||||
* Lead-review fix: on a NON-zsh shell no scrub ever runs (bash ignores {@code ZDOTDIR}), so the
|
||||
* "allowed N of M" line — which describes what the scrub does — must not be printed there either.
|
||||
* Before this fix the line was logged BEFORE the zsh gate, so a non-zsh host printed e.g.
|
||||
* "allowed 1 of 3" while blocking nothing at all, telling an operator a control ran when it did
|
||||
* not. Real path: goes through {@link HerdrPeerLauncher#spawn}, same as the sibling test above,
|
||||
* with the shell fixed to bash so the fallback branch is the one exercised.
|
||||
*/
|
||||
@Test
|
||||
void noAllowedCountLineIsEmittedOnTheNonZshFallbackPath() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
Set<String> hostEnvNames = Set.of(INJECTED, "SOME_UNRELATED_NAME", "ANOTHER_UNRELATED_NAME");
|
||||
WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/bash", () -> hostEnvNames);
|
||||
|
||||
Logger logger = (Logger) LoggerFactory.getLogger(HerdrPeerLauncher.class);
|
||||
Level original = logger.getLevel();
|
||||
logger.setLevel(Level.INFO);
|
||||
ListAppender<ILoggingEvent> appender = new ListAppender<>();
|
||||
appender.start();
|
||||
logger.addAppender(appender);
|
||||
try {
|
||||
launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV));
|
||||
} finally {
|
||||
logger.detachAppender(appender);
|
||||
logger.setLevel(original);
|
||||
}
|
||||
|
||||
assertFalse(appender.list.stream()
|
||||
.anyMatch(e -> e.getFormattedMessage().startsWith("member credentials: allowed ")),
|
||||
"no scrub runs on a non-zsh shell, so no 'allowed N of M' count may be printed — got: "
|
||||
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
|
||||
}
|
||||
|
||||
private static String readAll(Path p) {
|
||||
try {
|
||||
return Files.readString(p);
|
||||
|
||||
Reference in New Issue
Block a user