CB-633 round 2: the scrub ran on macOS and did nothing on Linux
Six review findings, from the peer lead `vms` and a reviewer worker. The first one is a real defect that would have shipped as a dead control. 1. The scrub only ran in a login shell. It lived in the generated `.zlogin`, and zsh reads `.zlogin` only for a login shell. herdr does not open the same kind of shell everywhere: measured on herdr 0.8.0, a macOS pane runs `-zsh` (login) while a Linux pane runs a plain `/usr/bin/zsh`. So on the vhost this was being built for, `.zlogin` never ran and every member kept the whole secret store, in silence. The scrub body now lives in a generated `scrub.zsh` that BOTH `.zshrc` and `.zlogin` source, each after sourcing its own `$HOME` counterpart. Linux runs the first, macOS runs both, and the second pass is not merely harmless -- it re-scrubs anything the operator's `~/.zlogin` exported after `~/.zshrc` had finished. Re-running is idempotent. 2. `INFRASTRUCTURE_PASSTHROUGH` listed names that are not infrastructure: ANTHROPIC_AUTH_TOKEN, GITEA_TOKEN, GITEA_HOST, ANTHROPIC_BASE_URL, ANTHROPIC_MODEL, CLAUDE_CONFIG_DIR, OPENCODE_CONFIG, BRIDGED_MEMBER. I read each injection point and confirmed every one of them reaches `launch.env()` only when actually injected, so `allowed.addAll(launch.env().keySet())` already covers the legitimate case. As static entries they were pure leak surface: a host that happened to export ANTHROPIC_AUTH_TOKEN would have had it passed straight through. 3. A missing `scrub-report.txt` at teardown was logged at debug. The report is the only evidence the scrub ran at all. Its absence has an innocent reading and a serious one, and we cannot tell them apart from the daemon -- so it is now a WARN that says exactly that. Logging it at debug is how a control that quietly stopped working stays unnoticed. 4. Nothing tested that the control was wired in. Deleting the single `applyEnvironmentAllowListPolicy(cfg, launch)` line left all 896 tests green while turning the feature completely off -- the CB-586/CB-611 shape again. `HerdrPeerLauncherAllowListWiringTest` starts a real spawn and asserts on the env that reached herdr. Mutation-checked: unwiring that line fails it. 5. `EnvAllowListScrubTest` now also runs `zsh -i` with no `-l`, which is the Linux pane shape, so finding 1 is tested from a Mac. Mutation-checked: putting the scrub back in `.zlogin` alone fails that test alone, while the login-shell test still passes -- which is exactly the blind spot that let the bug through. 6. Two ZDOTDIR leaks closed. A failed spawn has no pane id, so its directory was never keyed for teardown; it is now removed on the way out. And `deleteOnExit` covers a clean shutdown and nothing else, so `generate` now reaps sibling directories older than 24h left by a killed daemon. Also: `policy:` is lowercased with Locale.ROOT, and the `.zlogin`-only claim is corrected in fleetd.example.yaml, FleetConfig and HerdrPeerLauncher. 901 tests, 0 failures, `mvn clean install` green.
This commit is contained in:
@@ -510,11 +510,14 @@ guard:
|
||||
# where that shell does not re-export the name (see ROUND-2 CORRECTION above). An
|
||||
# unrecognized value refuses to start, naming it.
|
||||
# policy → "allow-list" (CB-633) moves the control to a per-spawn ZDOTDIR directory the daemon
|
||||
# generates and passes through tab.create's env map. The pane's zsh startup order is
|
||||
# .zshenv → .zprofile → .zshrc → .zlogin, and the operator's whole chain runs inside the
|
||||
# first three — so the generated .zlogin, which sources ~/.zlogin first and THEN blanks
|
||||
# every exported variable not on the derived allow-list, runs after everything the
|
||||
# operator sourced. No sourced file can undo it. The allow-list is DERIVED, never typed:
|
||||
# generates and passes through tab.create's env map. Each generated startup file sources
|
||||
# its ~/ counterpart FIRST and then runs the scrub, so the scrub happens after the
|
||||
# operator's whole chain and no sourced file can undo it.
|
||||
# The scrub is sourced from BOTH the generated .zshrc and the generated .zlogin, because
|
||||
# herdr does not open the same kind of shell everywhere: macOS panes run a LOGIN zsh (so
|
||||
# .zlogin runs), Linux panes run a plain interactive zsh (so .zlogin never runs at all).
|
||||
# A scrub in .zlogin alone would be a control that silently does nothing on Linux.
|
||||
# The allow-list is DERIVED, never typed:
|
||||
# every profile's tokenEnv/gitTokenEnv/gitHostEnv values and env-map keys, plus an
|
||||
# infrastructure set (PATH HOME SHELL TERM LANG LC_* TMPDIR USER LOGNAME PWD SHLVL EDITOR
|
||||
# PAGER JAVA_HOME XDG_* ZDOTDIR), plus whatever keys this spawn's own env overlay carries.
|
||||
@@ -522,6 +525,10 @@ guard:
|
||||
# Under this policy `known`/`allow` below become REPORTING ONLY — they feed the gap WARN,
|
||||
# they are no longer a control. If the member's login shell is NOT zsh, the daemon logs a
|
||||
# loud WARN saying protection is off and falls back to deny-by-default's overlay.
|
||||
# Each pane writes a scrub-report.txt naming how many variables it kept of how many it
|
||||
# saw; the daemon logs that "allowed N of M" line when the pane stops. If the report is
|
||||
# MISSING the daemon logs a WARN instead — the scrub cannot then be confirmed to have
|
||||
# run, and a silently-dead control is exactly what this policy exists to prevent.
|
||||
# allow → credential names a member legitimately needs. Under deny-by-default, left OUT of the
|
||||
# pane's env overlay entirely, so the value the pane's own (login) shell exports passes
|
||||
# through untouched. Under allow-list: reporting only.
|
||||
|
||||
@@ -959,8 +959,8 @@ public record FleetConfig(
|
||||
*
|
||||
* <p><b>CB-633: allow-list.</b> Deny-by-default's overlay is applied BEFORE the pane's login
|
||||
* shell runs, so any file that chain sources can re-export over it — and did. The allow-list
|
||||
* policy moves the control to a generated ZDOTDIR whose {@code .zlogin} runs LAST, after the
|
||||
* whole operator chain, and blanks every exported variable not on an allow-list DERIVED from
|
||||
* policy moves the control to a generated ZDOTDIR whose startup files run the scrub LAST, after
|
||||
* the whole operator chain, and blank every exported variable not on an allow-list DERIVED from
|
||||
* what the launcher itself injects (profiles' tokenEnv/gitTokenEnv/gitHostEnv/env keys plus an
|
||||
* infrastructure set) — never hand-typed, so adding a profile cannot break a spawn. Under this
|
||||
* policy {@link #allow} and {@link #known} stop being a control and become reporting only.
|
||||
@@ -1005,8 +1005,10 @@ public record FleetConfig(
|
||||
|
||||
/**
|
||||
* CB-633: derive the kept-name set from what the launcher itself injects, generate a
|
||||
* per-spawn ZDOTDIR whose {@code .zlogin} blanks every exported variable not on it AFTER the
|
||||
* pane's login shell has finished sourcing the operator's chain.
|
||||
* per-spawn ZDOTDIR whose startup files blank every exported variable not on it AFTER the
|
||||
* pane's shell has finished sourcing the operator's chain. The scrub is sourced from both
|
||||
* the generated {@code .zshrc} and {@code .zlogin}, because herdr opens a LOGIN zsh on macOS
|
||||
* and a plain interactive one on Linux — see {@code EnvAllowListScrub}.
|
||||
*/
|
||||
public static final String POLICY_ALLOW_LIST = "allow-list";
|
||||
|
||||
@@ -1017,7 +1019,7 @@ public record FleetConfig(
|
||||
|
||||
public MemberCredentials {
|
||||
String normalizedPolicy = (policy == null || policy.isBlank())
|
||||
? POLICY_DENY_BY_DEFAULT : policy.toLowerCase();
|
||||
? POLICY_DENY_BY_DEFAULT : policy.toLowerCase(java.util.Locale.ROOT);
|
||||
// deny-list is an alias of deny-by-default, not a third behaviour — normalize to one
|
||||
// spelling so every isDenyList()-style check has one value to compare against.
|
||||
policy = POLICY_DENY_LIST.equals(normalizedPolicy) ? POLICY_DENY_BY_DEFAULT : normalizedPolicy;
|
||||
|
||||
@@ -1,9 +1,14 @@
|
||||
package dev.ltms.fleet.member;
|
||||
|
||||
import org.slf4j.Logger;
|
||||
import org.slf4j.LoggerFactory;
|
||||
|
||||
import java.io.IOException;
|
||||
import java.io.UncheckedIOException;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.time.Duration;
|
||||
import java.time.Instant;
|
||||
import java.util.ArrayList;
|
||||
import java.util.List;
|
||||
import java.util.Set;
|
||||
@@ -13,12 +18,27 @@ import java.util.stream.Stream;
|
||||
* CB-633: generates the per-spawn {@code ZDOTDIR} directory whose startup files enforce
|
||||
* {@code memberCredentials.policy: allow-list}.
|
||||
*
|
||||
* <p>The seam: a login interactive zsh sources {@code $ZDOTDIR/.zshenv}, then {@code .zprofile},
|
||||
* then {@code .zshrc}, then {@code .zlogin} — in that order, LAST first-named-last. The operator's
|
||||
* whole chain ({@code ~/.zshrc} → secret store) runs inside the first three, so a scrub placed in
|
||||
* the generated {@code .zlogin} runs after everything the operator sourced, and nothing later can
|
||||
* re-export over it. This is the property CB-596's env-overlay control lacked: herdr applies that
|
||||
* overlay BEFORE the login shell starts, so any sourced file can undo it — and did.
|
||||
* <p>The seam: zsh reads its startup files from {@code $ZDOTDIR}, and the daemon puts that variable
|
||||
* in the pane-creation env map. The operator's whole chain ({@code ~/.zshrc} → secret store) runs
|
||||
* inside those files, so a scrub appended to the LAST one runs after everything the operator
|
||||
* sourced, and nothing later can re-export over it. This is the property CB-596's env-overlay
|
||||
* control lacked: herdr applies that overlay BEFORE the shell starts, so any sourced file can undo
|
||||
* it — and did.
|
||||
*
|
||||
* <p><b>Which file is last depends on the platform, so the scrub runs from two of them.</b> zsh
|
||||
* reads {@code .zshenv} always, {@code .zprofile} and {@code .zlogin} only for a LOGIN shell, and
|
||||
* {@code .zshrc} only for an INTERACTIVE one. herdr does not open the same kind of shell
|
||||
* everywhere — measured on herdr 0.8.0: macOS panes run {@code -zsh} (login, so {@code .zlogin}
|
||||
* runs), Linux panes run a plain {@code /usr/bin/zsh} (interactive but NOT login, so
|
||||
* {@code .zlogin} never runs at all). A scrub in {@code .zlogin} alone is therefore a control that
|
||||
* silently does nothing on Linux — the exact failure this class exists to remove, one platform
|
||||
* over.
|
||||
*
|
||||
* <p>So both {@code .zshrc} and {@code .zlogin} source the same generated {@code scrub.zsh} after
|
||||
* sourcing their {@code $HOME} counterpart. On Linux only the first fires; on macOS both do, and
|
||||
* the second pass is deliberate rather than merely harmless — it re-scrubs anything the operator's
|
||||
* own {@code ~/.zlogin} exported after {@code .zshrc} had finished. Re-running is idempotent: a
|
||||
* name already blank is blanked again, and the report is rewritten with the same counts.
|
||||
*
|
||||
* <p>Each generated file sources its {@code $HOME} counterpart FIRST, so {@code PATH} and every
|
||||
* toolchain binary still resolve exactly as the operator configured them; only afterwards does
|
||||
@@ -35,9 +55,27 @@ import java.util.stream.Stream;
|
||||
*/
|
||||
public final class EnvAllowListScrub {
|
||||
|
||||
private static final Logger log = LoggerFactory.getLogger(EnvAllowListScrub.class);
|
||||
|
||||
/** Name of the report file written into the generated directory by the scrub itself. */
|
||||
static final String REPORT_FILE = "scrub-report.txt";
|
||||
|
||||
/** The scrub body, generated once and sourced from both {@code .zshrc} and {@code .zlogin}. */
|
||||
static final String SCRUB_FILE = "scrub.zsh";
|
||||
|
||||
/** Prefix of every generated directory — also what {@link #reapOrphans} matches on. */
|
||||
static final String DIR_PREFIX = "bridged-zdotdir-";
|
||||
|
||||
/**
|
||||
* How old an orphan must be before {@link #reapOrphans} removes it. Comfortably longer than any
|
||||
* spawn takes, so a directory belonging to a pane that is still starting is never removed.
|
||||
*/
|
||||
private static final Duration ORPHAN_AGE = Duration.ofHours(24);
|
||||
|
||||
/** Appended to the two startup files that must run the scrub, after their {@code $HOME} source. */
|
||||
private static final String SOURCE_SCRUB =
|
||||
"source \"$ZDOTDIR/" + SCRUB_FILE + "\"\n";
|
||||
|
||||
private EnvAllowListScrub() {
|
||||
}
|
||||
|
||||
@@ -61,12 +99,17 @@ public final class EnvAllowListScrub {
|
||||
*/
|
||||
public static Path generate(Path parentDir, Set<String> allowedNames) {
|
||||
try {
|
||||
Path dir = Files.createTempDirectory(parentDir, "bridged-zdotdir-");
|
||||
reapOrphans(parentDir);
|
||||
Path dir = Files.createTempDirectory(parentDir, DIR_PREFIX);
|
||||
dir.toFile().deleteOnExit();
|
||||
// The report is written by zsh, after these hooks are registered, so register its path
|
||||
// too — otherwise the directory is non-empty at JVM exit and cannot be removed at all.
|
||||
dir.resolve(REPORT_FILE).toFile().deleteOnExit();
|
||||
write(dir, SCRUB_FILE, scrubScript(allowedNames));
|
||||
write(dir, ".zshenv", homeSourcingFile(".zshenv"));
|
||||
write(dir, ".zprofile", homeSourcingFile(".zprofile"));
|
||||
write(dir, ".zshrc", homeSourcingFile(".zshrc"));
|
||||
write(dir, ".zlogin", zloginScript(allowedNames));
|
||||
write(dir, ".zshrc", homeSourcingFile(".zshrc") + SOURCE_SCRUB);
|
||||
write(dir, ".zlogin", homeSourcingFile(".zlogin") + SOURCE_SCRUB);
|
||||
return dir;
|
||||
} catch (IOException e) {
|
||||
throw new UncheckedIOException("cannot generate ZDOTDIR scrub files under " + parentDir, e);
|
||||
@@ -83,12 +126,13 @@ public final class EnvAllowListScrub {
|
||||
}
|
||||
|
||||
/**
|
||||
* The generated {@code .zlogin}: source the operator's own {@code ~/.zlogin}, then run the scrub.
|
||||
* Package-private so tests can assert on the exact script handed to zsh — the artefact here IS a
|
||||
* shell file, and a test that checks only the Java string assembly proves nothing about whether
|
||||
* zsh accepts it.
|
||||
* The generated {@code scrub.zsh} — the scrub body on its own, so the two startup files that
|
||||
* must run it ({@code .zshrc} and {@code .zlogin}) hold one copy between them rather than two
|
||||
* that can drift. Package-private so tests can assert on the exact script handed to zsh — the
|
||||
* artefact here IS a shell file, and a test that checks only the Java string assembly proves
|
||||
* nothing about whether zsh accepts it.
|
||||
*/
|
||||
static String zloginScript(Set<String> allowedNames) {
|
||||
static String scrubScript(Set<String> allowedNames) {
|
||||
StringBuilder names = new StringBuilder();
|
||||
for (String n : allowedNames.stream().sorted().toList()) {
|
||||
if (names.length() > 0) {
|
||||
@@ -100,8 +144,11 @@ public final class EnvAllowListScrub {
|
||||
}
|
||||
return """
|
||||
# generated by fleetd (CB-633 memberCredentials policy=allow-list) — do not edit.
|
||||
# Runs LAST in the login-shell order, after everything the operator sourced.
|
||||
[ -r "$HOME/.zlogin" ] && source "$HOME/.zlogin"
|
||||
# Sourced from .zshrc and again from .zlogin, each time AFTER that file has sourced
|
||||
# its $HOME counterpart — so this runs after everything the operator sourced, on a
|
||||
# login shell (macOS panes) and on a plain interactive one (Linux panes) alike.
|
||||
# Running twice is idempotent and deliberate: the second pass catches anything
|
||||
# ~/.zlogin exported after ~/.zshrc had finished.
|
||||
|
||||
typeset -A _cb633_allowed
|
||||
for _cb633_n in %s; do _cb633_allowed[$_cb633_n]=1; done
|
||||
@@ -174,6 +221,42 @@ public final class EnvAllowListScrub {
|
||||
}
|
||||
|
||||
/** Best-effort recursive delete; failures are swallowed — JVM-exit cleanup is the backstop. */
|
||||
/**
|
||||
* Remove generated directories left behind by an earlier daemon process.
|
||||
*
|
||||
* <p>{@link #generate} registers each directory for deletion at JVM exit, which covers a clean
|
||||
* shutdown and covers nothing else. A {@code kill -9}, a crash, or a host reboot leaves the
|
||||
* directory in the temp dir for good, and the daemon is restarted often enough that these
|
||||
* accumulate. They hold no secrets — the generated files contain variable NAMES and a report of
|
||||
* names, never a value — but an unbounded pile of them in {@code /tmp} is still our mess to
|
||||
* clear.
|
||||
*
|
||||
* <p>Called from {@link #generate}, so it runs on the path that creates them and needs no
|
||||
* separate wiring or scheduler. Only directories older than {@link #ORPHAN_AGE} are touched,
|
||||
* which keeps it clear of any pane that is merely still starting, including one belonging to a
|
||||
* different daemon instance running right now. Best-effort: every failure is ignored, because
|
||||
* tidying temp files must never be the reason a spawn fails.
|
||||
*/
|
||||
static void reapOrphans(Path parentDir) {
|
||||
Instant cutoff = Instant.now().minus(ORPHAN_AGE);
|
||||
try (Stream<Path> entries = Files.list(parentDir)) {
|
||||
entries.filter(d -> d.getFileName().toString().startsWith(DIR_PREFIX))
|
||||
.filter(Files::isDirectory)
|
||||
.filter(d -> olderThan(d, cutoff))
|
||||
.forEach(EnvAllowListScrub::deleteRecursively);
|
||||
} catch (IOException | RuntimeException e) {
|
||||
log.debug("could not scan {} for orphaned ZDOTDIRs: {}", parentDir, e.toString());
|
||||
}
|
||||
}
|
||||
|
||||
private static boolean olderThan(Path dir, Instant cutoff) {
|
||||
try {
|
||||
return Files.getLastModifiedTime(dir).toInstant().isBefore(cutoff);
|
||||
} catch (IOException e) {
|
||||
return false; // unreadable timestamp ⇒ leave it alone
|
||||
}
|
||||
}
|
||||
|
||||
static void deleteRecursively(Path dir) {
|
||||
if (dir == null || !Files.exists(dir)) {
|
||||
return;
|
||||
|
||||
@@ -438,9 +438,20 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
// the exact env-map keys this launch injects (ANTHROPIC_*, OPENCODE_CONFIG, GITEA_TOKEN, …)
|
||||
// — anything the daemon deliberately sets must survive its own control.
|
||||
Path zdotdir = applyEnvironmentAllowListPolicy(cfg, launch);
|
||||
Agent agent = cfg.tabPlacement()
|
||||
? spawnInTab(cfg, launch.env(), launch.argv(), cwd, role, liveFleet)
|
||||
: spawnAsPane(cfg, launch.env(), launch.argv(), cwd, charter);
|
||||
Agent agent;
|
||||
try {
|
||||
agent = cfg.tabPlacement()
|
||||
? spawnInTab(cfg, launch.env(), launch.argv(), cwd, role, liveFleet)
|
||||
: spawnAsPane(cfg, launch.env(), launch.argv(), cwd, charter);
|
||||
} catch (RuntimeException e) {
|
||||
// A failed spawn has no pane id, so nothing would ever key this directory for
|
||||
// teardown and it would sit in the temp dir until the JVM exits cleanly — which,
|
||||
// for a daemon, may be never. Remove it on the way out.
|
||||
if (zdotdir != null) {
|
||||
EnvAllowListScrub.deleteRecursively(zdotdir);
|
||||
}
|
||||
throw e;
|
||||
}
|
||||
if (zdotdir != null) {
|
||||
zdotdirByPane.put(agent.paneId(), zdotdir);
|
||||
}
|
||||
@@ -1013,9 +1024,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
|
||||
/**
|
||||
* CB-633: under {@code memberCredentials.policy: allow-list}, generate the per-spawn ZDOTDIR
|
||||
* directory whose {@code .zlogin} blanks every exported variable not on the DERIVED allow-list —
|
||||
* running AFTER the pane's login shell has finished sourcing the operator's chain, which is what
|
||||
* no pre-shell env overlay can achieve. Mutates {@code launch.env()} to carry
|
||||
* directory whose startup files blank every exported variable not on the DERIVED allow-list —
|
||||
* running AFTER the pane's shell has finished sourcing the operator's chain, which is what no
|
||||
* pre-shell env overlay can achieve. {@code EnvAllowListScrub} sources the scrub from both the
|
||||
* generated {@code .zshrc} and {@code .zlogin}, since a herdr pane is a login shell on macOS
|
||||
* and a plain interactive one on Linux. Mutates {@code launch.env()} to carry
|
||||
* {@code ZDOTDIR=<dir>}, so both placement paths ({@link #spawnInTab}, {@link #spawnAsPane})
|
||||
* pass it through {@code tab.create}/{@code pane.split}. Returns the directory for teardown
|
||||
* registration, or {@code null} when the policy does not apply.
|
||||
@@ -1075,9 +1088,17 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
|
||||
/**
|
||||
* CB-633 teardown half: read the pane's scrub report (the denominator report the generated
|
||||
* {@code .zlogin} wrote) and delete the directory. Called from {@link #stop}, which is the one
|
||||
* funnel every teardown exit already goes through. Best-effort throughout: a missing report is
|
||||
* logged at debug, never an error — the pane may be gone before its shell reached the scrub.
|
||||
* scrub wrote) and delete the directory. Called from {@link #stop}, which is the one funnel
|
||||
* every teardown exit already goes through.
|
||||
*
|
||||
* <p><b>A missing report is a WARN, not a debug line.</b> The report is the only evidence that
|
||||
* the scrub ran at all in that pane. Its absence has an innocent reading — the pane died before
|
||||
* its shell finished starting — and a serious one: the shell was not zsh, or it read its
|
||||
* startup files from somewhere other than the directory we generated, in which case the member
|
||||
* ran for its whole life with the operator's full secret store in its environment and nothing
|
||||
* said so. We cannot tell those two apart from here, so the line says what is and is not known
|
||||
* rather than picking one. Logging this at debug is how a control that silently stopped working
|
||||
* stays unnoticed — the failure mode this whole class exists to remove.
|
||||
*/
|
||||
private void releaseZdotdir(String paneId) {
|
||||
Path dir = zdotdirByPane.remove(paneId);
|
||||
@@ -1086,7 +1107,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
|
||||
}
|
||||
EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(dir);
|
||||
if (report == null) {
|
||||
log.debug("memberCredentials allow-list: pane {} left no scrub report", paneId);
|
||||
log.warn("memberCredentials allow-list: pane {} left no scrub report in {} — the "
|
||||
+ "environment scrub cannot be confirmed to have run. Either the pane ended "
|
||||
+ "before its shell finished starting, or its shell never read our generated "
|
||||
+ "startup files, in which case that member saw the full host environment.",
|
||||
paneId, dir);
|
||||
} else {
|
||||
log.info("memberCredentials allow-list: pane {} allowed {} of {} environment variables",
|
||||
paneId, report.allowed(), report.total());
|
||||
|
||||
@@ -41,23 +41,27 @@ public final class MemberEnvAllowList {
|
||||
/**
|
||||
* Names that are not credentials and that a login shell or agent binary genuinely needs.
|
||||
*
|
||||
* <p>Deliberately conservative beyond the ticket's named set: {@code ZDOTDIR} must survive or
|
||||
* every later sub-shell loses the scrub; {@code BRIDGED_MEMBER} is the daemon's own marker;
|
||||
* {@code GITEA_TOKEN}/{@code GITEA_HOST} are what {@code applyGitToken} injects by literal name;
|
||||
* the {@code ANTHROPIC_*}/{@code CLAUDE_CONFIG_DIR}/{@code OPENCODE_CONFIG} names are what the
|
||||
* adapters inject by literal name (they are also re-added per-spawn from the env map itself —
|
||||
* listing them here keeps the derived set self-contained for tests and reporting); {@code
|
||||
* JAVA_HOME} and the {@code XDG_*} roots are toolchain locations, not secrets. Everything else a
|
||||
* member needs must arrive via a profile's {@code env:}, which lands on this set automatically.
|
||||
* <p>Every name here is a location or a shell setting, never a credential. That rule is load
|
||||
* bearing, and CB-633's first cut broke it: it also listed {@code ANTHROPIC_AUTH_TOKEN},
|
||||
* {@code GITEA_TOKEN}, {@code GITEA_HOST}, {@code ANTHROPIC_BASE_URL}, {@code ANTHROPIC_MODEL},
|
||||
* {@code CLAUDE_CONFIG_DIR}, {@code OPENCODE_CONFIG} and {@code BRIDGED_MEMBER} "because the
|
||||
* launcher injects them". The launcher does — but only on the spawns where it actually sets
|
||||
* them, and {@code HerdrPeerLauncher} already unions THIS spawn's env-map keys into the
|
||||
* allow-list. So a static entry adds nothing on a spawn that injects the name, and on a spawn
|
||||
* that does not it lets the operator's own value through under exactly the name a member reads.
|
||||
* {@code ANTHROPIC_BASE_URL} is the sharpest case: an inherited one silently moves a member off
|
||||
* the endpoint the profile chose.
|
||||
*
|
||||
* <p>{@code ZDOTDIR} stays because it is this control's own handle — lose it and every later
|
||||
* sub-shell loses the scrub. {@code JAVA_HOME} and the {@code XDG_*} roots are toolchain
|
||||
* locations. Everything else a member needs must arrive via a profile's {@code env:} or the
|
||||
* launcher's own injection, both of which land on the derived set automatically.
|
||||
*/
|
||||
public static final Set<String> INFRASTRUCTURE_PASSTHROUGH = Set.of(
|
||||
"PATH", "HOME", "SHELL", "TERM", "LANG", "TMPDIR",
|
||||
"USER", "LOGNAME", "PWD", "SHLVL", "EDITOR", "PAGER",
|
||||
"_",
|
||||
"ZDOTDIR", "BRIDGED_MEMBER",
|
||||
"GITEA_TOKEN", "GITEA_HOST",
|
||||
"ANTHROPIC_BASE_URL", "ANTHROPIC_AUTH_TOKEN", "ANTHROPIC_MODEL",
|
||||
"CLAUDE_CONFIG_DIR", "OPENCODE_CONFIG",
|
||||
"ZDOTDIR",
|
||||
"JAVA_HOME",
|
||||
"XDG_CONFIG_HOME", "XDG_DATA_HOME", "XDG_CACHE_HOME", "XDG_STATE_HOME");
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@ import java.io.IOException;
|
||||
import java.nio.charset.StandardCharsets;
|
||||
import java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.util.ArrayList;
|
||||
import java.util.HashSet;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
@@ -112,6 +113,54 @@ class EnvAllowListScrubTest {
|
||||
assertNull(EnvAllowListScrub.readReport(dir));
|
||||
}
|
||||
|
||||
/**
|
||||
* The same equality, for a shell that is INTERACTIVE but NOT a login shell — the shape herdr
|
||||
* opens on Linux.
|
||||
*
|
||||
* <p>Why this test exists. The first version of this control put the scrub in {@code .zlogin}
|
||||
* alone. zsh reads {@code .zlogin} only for a login shell, and herdr does not open one
|
||||
* everywhere: measured on herdr 0.8.0, a macOS pane runs {@code -zsh} (login) while a Linux pane
|
||||
* runs a plain {@code /usr/bin/zsh}. So the control would have passed every test on the
|
||||
* developer's Mac and protected nothing at all on the vhost it was being built for, in silence.
|
||||
*
|
||||
* <p>This runs {@code zsh -i} — no {@code -l} — so {@code .zprofile} and {@code .zlogin} are
|
||||
* skipped exactly as they are on Linux. It therefore tests the Linux code path from a Mac,
|
||||
* which is the only place we can currently run it. Reverting the scrub to {@code .zlogin} only
|
||||
* makes this test fail while the login-shell test above still passes.
|
||||
*/
|
||||
@Test
|
||||
void scrubAlsoRunsInAnInteractiveNonLoginShell(@TempDir Path tmp) throws Exception {
|
||||
assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here");
|
||||
Path homeZshrc = Path.of(System.getProperty("user.home"), ".zshrc");
|
||||
assumeTrue(Files.exists(homeZshrc), "$HOME/.zshrc does not exist — no real chain to test against");
|
||||
|
||||
Set<String> allowed = MemberEnvAllowList.derive(List.of());
|
||||
Path zdotdir = EnvAllowListScrub.generate(tmp, allowed);
|
||||
|
||||
Map<String, String> cleanParent = Map.of(
|
||||
"HOME", System.getProperty("user.home"),
|
||||
"PATH", "/usr/bin:/bin",
|
||||
"SHELL", "/bin/zsh",
|
||||
"USER", System.getProperty("user.name", "nobody"),
|
||||
"TMPDIR", tmp.toString());
|
||||
|
||||
List<String> interactiveOnly = List.of("-i");
|
||||
Set<String> baseline = exportedNamesFromCleanParent(cleanParent, null, interactiveOnly);
|
||||
Set<String> scrubbed = exportedNamesFromCleanParent(cleanParent, zdotdir, interactiveOnly);
|
||||
|
||||
Set<String> expected = new TreeSet<>();
|
||||
for (String name : baseline) {
|
||||
if (MemberEnvAllowList.keeps(allowed, name)) {
|
||||
expected.add(name);
|
||||
}
|
||||
}
|
||||
expected.add("ZDOTDIR"); // the harness set it and it is infrastructure, so it must survive
|
||||
assertEquals(expected, scrubbed,
|
||||
"a non-login interactive zsh is what a herdr pane runs on Linux; its surviving "
|
||||
+ "exported names must EQUAL baseline \u2229 allow-list, exactly as for a login "
|
||||
+ "shell. A difference here means the scrub is dead on Linux.");
|
||||
}
|
||||
|
||||
/**
|
||||
* Run {@code /bin/zsh -l -i} from a clean parent and return the NAMES it has exported by prompt
|
||||
* time. With {@code zdotdir} non-null, {@code ZDOTDIR} points at a generated scrub directory, so
|
||||
@@ -119,7 +168,16 @@ class EnvAllowListScrubTest {
|
||||
*/
|
||||
private static Set<String> exportedNamesFromCleanParent(Map<String, String> cleanParent,
|
||||
Path zdotdir) throws IOException, InterruptedException {
|
||||
ProcessBuilder pb = new ProcessBuilder("/bin/zsh", "-l", "-i");
|
||||
return exportedNamesFromCleanParent(cleanParent, zdotdir, List.of("-l", "-i"));
|
||||
}
|
||||
|
||||
private static Set<String> exportedNamesFromCleanParent(Map<String, String> cleanParent,
|
||||
Path zdotdir, List<String> shellFlags)
|
||||
throws IOException, InterruptedException {
|
||||
List<String> argv = new ArrayList<>();
|
||||
argv.add("/bin/zsh");
|
||||
argv.addAll(shellFlags);
|
||||
ProcessBuilder pb = new ProcessBuilder(argv);
|
||||
pb.environment().clear();
|
||||
pb.environment().putAll(cleanParent);
|
||||
if (zdotdir != null) {
|
||||
|
||||
+170
@@ -0,0 +1,170 @@
|
||||
package dev.ltms.fleet.member;
|
||||
|
||||
import dev.ltms.fleet.config.FleetConfig;
|
||||
import dev.ltms.fleet.herdr.AgentControl;
|
||||
import dev.ltms.fleet.herdr.FakeHerdr;
|
||||
import dev.ltms.fleet.herdr.WorkspaceControl;
|
||||
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 java.nio.file.Files;
|
||||
import java.nio.file.Path;
|
||||
import java.util.HashMap;
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
import java.util.function.Supplier;
|
||||
|
||||
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;
|
||||
|
||||
/**
|
||||
* CB-633: proves the allow-list scrub is actually WIRED INTO the spawn path — not merely that its
|
||||
* pieces work when a test calls them directly.
|
||||
*
|
||||
* <p>Why this test exists, and why it is separate from {@link EnvAllowListScrubTest}. Every other
|
||||
* test of this feature calls {@code EnvAllowListScrub} or {@code MemberEnvAllowList} itself. Those
|
||||
* prove the scrub is correct. None of them proves anyone runs it: deleting the single
|
||||
* {@code applyEnvironmentAllowListPolicy(cfg, launch)} line from {@code spawnInternal} left all 896
|
||||
* tests green while turning the control completely off. That is the recurring shape in this
|
||||
* codebase — a feature behind one call, with every test on the far side of it (CB-586, CB-611).
|
||||
*
|
||||
* <p>So this test starts a real spawn through {@link HerdrPeerLauncher#spawn} and asserts on what
|
||||
* reached herdr. It deliberately checks the pane-creation parameters rather than the launcher's own
|
||||
* map, because the map is an intermediate: {@code ZDOTDIR} only protects anything if it is in the
|
||||
* env herdr uses to create the pane, and that is the last point we can observe before the shell
|
||||
* starts.
|
||||
*/
|
||||
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";
|
||||
|
||||
@Test
|
||||
void spawningUnderAllowListPolicyGivesThePaneAGeneratedZdotdir() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
WiringLauncher launcher = new WiringLauncher(herdr, allowList());
|
||||
|
||||
launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV));
|
||||
|
||||
String paneParams = String.valueOf(herdr.lastCall("pane.split").params());
|
||||
assertTrue(paneParams.contains("ZDOTDIR"),
|
||||
"the spawn must hand herdr a ZDOTDIR so the pane's zsh reads our generated startup "
|
||||
+ "files; without it the scrub never runs and the member inherits the whole "
|
||||
+ "host environment. pane.split params were: " + paneParams);
|
||||
|
||||
Path dir = Path.of(launcher.env.get("ZDOTDIR"));
|
||||
assertTrue(Files.isDirectory(dir), "ZDOTDIR must point at a directory that exists: " + dir);
|
||||
// Both startup files must exist and both must source the scrub: .zlogin covers macOS panes
|
||||
// (login shells), .zshrc covers Linux panes (interactive, NOT login). Checking only one
|
||||
// would pass on the platform it was written for and ship a dead control on the other.
|
||||
for (String file : List.of(".zshrc", ".zlogin")) {
|
||||
Path f = dir.resolve(file);
|
||||
assertTrue(Files.isRegularFile(f), file + " must be generated: " + f);
|
||||
assertTrue(readAll(f).contains(EnvAllowListScrub.SCRUB_FILE),
|
||||
file + " must source " + EnvAllowListScrub.SCRUB_FILE + " — a scrub only one of "
|
||||
+ "them runs is dead on the platform that reads the other");
|
||||
}
|
||||
assertTrue(readAll(dir.resolve(EnvAllowListScrub.SCRUB_FILE)).contains(INJECTED),
|
||||
"the allow-list must include the names this very launch injects (" + INJECTED
|
||||
+ "), or the daemon's own configuration is blanked by its own control");
|
||||
}
|
||||
|
||||
/** The default policy must not generate anything — an upgrade changes nothing until asked. */
|
||||
@Test
|
||||
void spawningUnderTheDefaultPolicyGeneratesNoZdotdir() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
WiringLauncher launcher = new WiringLauncher(herdr, () -> new FleetConfig.MemberCredentials(
|
||||
null, List.of(), List.of(), null));
|
||||
|
||||
launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV));
|
||||
|
||||
assertFalse(launcher.env.containsKey("ZDOTDIR"),
|
||||
"policy=deny-by-default is the shipped default; it must not silently start "
|
||||
+ "rewriting members' shell startup files");
|
||||
}
|
||||
|
||||
/**
|
||||
* A non-zsh shell cannot read {@code ZDOTDIR} at all. The launcher must fall back rather than
|
||||
* generate a directory nothing will ever read — a directory that would look like protection.
|
||||
*/
|
||||
@Test
|
||||
void aNonZshShellGeneratesNothingAndFallsBack() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/bash");
|
||||
|
||||
launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV));
|
||||
|
||||
assertFalse(launcher.env.containsKey("ZDOTDIR"),
|
||||
"bash ignores ZDOTDIR; setting it would be protection theatre");
|
||||
}
|
||||
|
||||
private static Supplier<FleetConfig.MemberCredentials> allowList() {
|
||||
return () -> new FleetConfig.MemberCredentials(
|
||||
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of(), null);
|
||||
}
|
||||
|
||||
private static String readAll(Path p) {
|
||||
try {
|
||||
return Files.readString(p);
|
||||
} catch (java.io.IOException e) {
|
||||
throw new AssertionError("cannot read " + p, e);
|
||||
}
|
||||
}
|
||||
|
||||
private static FleetConfig.Profile profile() {
|
||||
return new FleetConfig.Profile("test", "http://gx00.gw:8000", null, null,
|
||||
"BRIDGED_WORKER_TOKEN", List.of("test"), "pane", null, null, null, null, null);
|
||||
}
|
||||
|
||||
/**
|
||||
* A minimal peer launcher whose {@code buildLaunch} returns a MUTABLE env map holding one name
|
||||
* the daemon injects. Mutable on purpose: the policy adds {@code ZDOTDIR} to this very map, so
|
||||
* an immutable one would throw and the test would pass for the wrong reason.
|
||||
*/
|
||||
private static final class WiringLauncher extends HerdrPeerLauncher {
|
||||
private final Map<String, String> env = new HashMap<>(Map.of(INJECTED, "http://gateway"));
|
||||
|
||||
WiringLauncher(FakeHerdr herdr, Supplier<FleetConfig.MemberCredentials> creds) {
|
||||
this(herdr, creds, "/bin/zsh");
|
||||
}
|
||||
|
||||
WiringLauncher(FakeHerdr herdr, Supplier<FleetConfig.MemberCredentials> creds, String shell) {
|
||||
super("test", new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
Map.of("test", profile()), "test",
|
||||
name -> "SHELL".equals(name) ? shell : null,
|
||||
0, () -> 0L, () -> { }, null, creds);
|
||||
}
|
||||
|
||||
@Override
|
||||
protected Launch buildLaunch(FleetConfig.Profile cfg, LaunchSpec spec) {
|
||||
return new Launch(env, List.of("test"));
|
||||
}
|
||||
|
||||
@Override
|
||||
public Set<Capability> capabilities() {
|
||||
return Set.of();
|
||||
}
|
||||
}
|
||||
|
||||
/** The generated directory is a temp directory; make sure the test does not leave a pile. */
|
||||
@Test
|
||||
void theGeneratedDirectoryIsRemovedWhenThePaneIsStopped() {
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
WiringLauncher launcher = new WiringLauncher(herdr, allowList());
|
||||
|
||||
var spawned = launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV));
|
||||
Path dir = Path.of(launcher.env.get("ZDOTDIR"));
|
||||
assertNotNull(spawned, "spawn returned nothing");
|
||||
assertTrue(Files.isDirectory(dir));
|
||||
|
||||
launcher.stop(spawned.id());
|
||||
|
||||
assertEquals(false, Files.exists(dir),
|
||||
"stopping the pane must remove its generated ZDOTDIR: " + dir);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user