CB-633 follow-up: union memberCredentials.allow into the derived env allow-list
CI / build (pull_request) Successful in 1m7s
CI / contract (pull_request) Successful in 1m16s

MemberEnvAllowList.derive only ever looked at profile fields, so
memberCredentials.allow: was silently ignored under
policy: allow-list — turning the policy on would have blanked
credentials working members already depended on.

- derive(profiles, configuredAllow) unions memberCredentials.allow
  into the derived set, with SSH_AUTH_SOCK explicitly excluded from
  that union (it stays governed only by sshAuthSock: allow).
- HerdrPeerLauncher threads MemberCredentials.allowSet() into the
  derivation instead of calling the profiles-only overload.
- Added a per-spawn INFO log 'member credentials: allowed N of M'
  (N/M from the daemon's own env, the existing hostEnvNames proxy),
  never logging a blocked name or a value.
This commit is contained in:
Dai Ha
2026-08-28 05:56:27 +07:00
parent 7c4170ff6d
commit 82e7be564c
4 changed files with 208 additions and 13 deletions
@@ -1033,17 +1033,23 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
* pass it through {@code tab.create}/{@code pane.split}. Returns the directory for teardown
* registration, or {@code null} when the policy does not apply.
*
* <p>The allow-list handed to the generator is the derived profile set ({@link
* MemberEnvAllowList#derive}) UNIONed with the exact keys of THIS launch's env map — names the
* daemon itself injects must survive its own control. {@code SSH_AUTH_SOCK} is added ONLY when
* the config explicitly allows it; by default it is absent, so the scrub blanks it like any
* other non-derived name.
* <p>The allow-list handed to the generator is the derived profile set UNIONed with the
* operator's own {@code memberCredentials.allow:} names ({@link MemberEnvAllowList#derive(
* Collection, Set)} — CB-633 follow-up) and with the exact keys of THIS launch's env map —
* names the daemon itself injects must survive its own control. {@code SSH_AUTH_SOCK} is added
* ONLY when the config explicitly allows it, EVEN IF the operator also listed it under
* {@code allow:}; by default it is absent, so the scrub blanks it like any other non-derived
* name. It stays a one-off decision because it is a live handle to the operator's ssh-agent, not
* a value — a member holding it can sign with every key the agent holds, so letting it ride in
* on the generic {@code allow:} list would hand that out for an unrelated reason.
*/
private Path applyEnvironmentAllowListPolicy(FleetConfig.Profile cfg, Launch launch) {
FleetConfig.MemberCredentials creds = memberCredentials == null ? null : memberCredentials.get();
if (creds == null || !creds.isAllowList()) {
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) {
@@ -1056,11 +1062,6 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
logCredentialGap(creds);
return null;
}
Set<String> allowed = new java.util.TreeSet<>(MemberEnvAllowList.derive(profiles.values()));
if (creds.sshAuthSockAllowed()) {
allowed.add(SSH_AUTH_SOCK);
} // blocked by default: absent from the set ⇒ blanked by the scrub like any other name
allowed.addAll(launch.env().keySet());
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 "
@@ -1069,8 +1070,38 @@ public abstract class HerdrPeerLauncher implements PeerLauncher {
return dir;
}
/**
* The full kept-name set for this spawn: the profile-derived names, unioned with {@code
* memberCredentials.allow:} (CB-633 follow-up — previously ignored by this whole policy), the
* ssh-agent handle when explicitly allowed, and the exact keys of THIS launch's own env map.
*/
private Set<String> derivedAllowedNames(FleetConfig.MemberCredentials creds, Launch launch) {
Set<String> allowed = new java.util.TreeSet<>(
MemberEnvAllowList.derive(profiles.values(), creds.allowSet()));
if (creds.sshAuthSockAllowed()) {
allowed.add(SSH_AUTH_SOCK);
} // blocked by default: absent from the set ⇒ blanked by the scrub like any other name
allowed.addAll(launch.env().keySet());
return allowed;
}
/**
* 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.
*/
private void logAllowListCoverage(Set<String> allowed) {
Set<String> hostNames = hostEnvNames.get();
long kept = hostNames.stream().filter(name -> MemberEnvAllowList.keeps(allowed, name)).count();
log.info("member credentials: allowed {} of {}", kept, hostNames.size());
}
/** The operator ssh-agent handle — kept ONLY by explicit config decision, never by default. */
private static final String SSH_AUTH_SOCK = "SSH_AUTH_SOCK";
private static final String SSH_AUTH_SOCK = MemberEnvAllowList.SSH_AUTH_SOCK;
/**
* CB-633: a non-zsh login shell means the allow-list control CANNOT run — say so once per
@@ -35,9 +35,27 @@ import java.util.TreeSet;
* <p>{@code SSH_AUTH_SOCK} is deliberately NOT here. It is a handle to the operator's ssh-agent — a
* member holding it can sign with the operator's keys — so keeping it is a config decision
* ({@code memberCredentials.sshAuthSock: allow}), not a derivation default.
*
* <p><b>CB-633 follow-up:</b> the union also includes {@code memberCredentials.allow:} — the
* operator's own explicit list. Before this, {@code policy: allow-list} silently ignored every name
* an operator wrote under {@code allow:} unless a profile happened to carry it too, which meant
* turning the policy on could blank credentials working members already depended on. {@code
* SSH_AUTH_SOCK} is the one exception: even when the operator lists it under {@code allow:}, it is
* excluded here and added back ONLY by the caller when {@code sshAuthSock: allow} is explicitly set
* (see {@link #SSH_AUTH_SOCK}'s javadoc) — it is a live handle to the operator's own ssh-agent, not
* a value, so treating it like any other allow-listed name would hand a member every key the
* operator's agent holds the moment they typed the name under {@code allow:} for an unrelated
* reason.
*/
public final class MemberEnvAllowList {
/**
* The operator's ssh-agent socket path. Deliberately excluded from {@link #derive}'s union of
* {@code memberCredentials.allow:} — see the class javadoc's CB-633 follow-up note. Governed
* ONLY by {@code memberCredentials.sshAuthSock}, never by appearing in {@code allow:}.
*/
public static final String SSH_AUTH_SOCK = "SSH_AUTH_SOCK";
/**
* Names that are not credentials and that a login shell or agent binary genuinely needs.
*
@@ -73,9 +91,21 @@ public final class MemberEnvAllowList {
/**
* Derive the allowed NAME set from the given profiles plus {@link #INFRASTRUCTURE_PASSTHROUGH}.
* Deterministic (sorted) so generated scrub files are diffable run-to-run.
* Equivalent to {@link #derive(Collection, Set)} with no operator-configured names — kept for
* callers (and existing tests) that only care about the profile-derived half.
*/
public static Set<String> derive(Collection<FleetConfig.Profile> profiles) {
return derive(profiles, Set.of());
}
/**
* Derive the allowed NAME set: the profile-derived union above, PLUS {@code configuredAllow} —
* the operator's own {@code memberCredentials.allow:} list (CB-633 follow-up). {@code
* SSH_AUTH_SOCK} is dropped from {@code configuredAllow} even if the operator listed it there;
* see the class javadoc for why. Deterministic (sorted) so generated scrub files are diffable
* run-to-run.
*/
public static Set<String> derive(Collection<FleetConfig.Profile> profiles, Set<String> configuredAllow) {
Set<String> derived = new TreeSet<>(INFRASTRUCTURE_PASSTHROUGH);
if (profiles != null) {
for (FleetConfig.Profile p : profiles) {
@@ -87,6 +117,13 @@ public final class MemberEnvAllowList {
}
}
}
if (configuredAllow != null) {
for (String name : configuredAllow) {
if (name != null && !name.isBlank() && !SSH_AUTH_SOCK.equals(name)) {
derived.add(name);
}
}
}
return Set.copyOf(derived);
}
@@ -1,5 +1,9 @@
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;
import dev.ltms.fleet.config.FleetConfig;
import dev.ltms.fleet.herdr.AgentControl;
import dev.ltms.fleet.herdr.FakeHerdr;
@@ -8,6 +12,7 @@ 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 org.slf4j.LoggerFactory;
import java.nio.file.Files;
import java.nio.file.Path;
@@ -108,6 +113,89 @@ class HerdrPeerLauncherAllowListWiringTest {
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, List.of(), List.of(), null);
}
/** Same as {@link #allowList()} but with an operator-configured {@code allow:} list. */
private static Supplier<FleetConfig.MemberCredentials> allowListWithAllow(List<String> allow) {
return () -> new FleetConfig.MemberCredentials(
FleetConfig.MemberCredentials.POLICY_ALLOW_LIST, allow, List.of(), null);
}
/**
* CB-633 follow-up: a name that lives ONLY in {@code memberCredentials.allow:} — no profile
* mentions it — must survive the scrub the real spawn path generates. Calling {@code
* MemberEnvAllowList.derive} directly (as {@link MemberEnvAllowListTest} does) would pass even
* if {@code HerdrPeerLauncher} never threaded {@code allow:} into the derivation at all; this
* test goes through {@link HerdrPeerLauncher#spawn}, the method the daemon actually calls at
* spawn time, so it proves the union is wired in, not just correct in isolation.
*/
@Test
void spawningUnderAllowListPolicyIncludesAnOperatorConfiguredAllowName() {
FakeHerdr herdr = new FakeHerdr();
WiringLauncher launcher = new WiringLauncher(herdr,
allowListWithAllow(List.of("OPERATOR_ONLY_NAME")));
launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV));
Path dir = Path.of(launcher.env.get("ZDOTDIR"));
assertTrue(readAll(dir.resolve(EnvAllowListScrub.SCRUB_FILE)).contains("OPERATOR_ONLY_NAME"),
"a name only in memberCredentials.allow: must reach the generated scrub through the "
+ "real launcher spawn path");
}
/**
* {@code SSH_AUTH_SOCK} is a live ssh-agent handle, not a value — it must stay blocked under
* {@code allow-list} even when the operator lists it under {@code allow:}, because {@code
* sshAuthSock} defaults to blocked. Governed ONLY by {@code memberCredentials.sshAuthSock}.
*/
@Test
void sshAuthSockStaysBlockedEvenWhenListedInMemberCredentialsAllow() {
FakeHerdr herdr = new FakeHerdr();
WiringLauncher launcher = new WiringLauncher(herdr,
allowListWithAllow(List.of("SSH_AUTH_SOCK")));
launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV));
Path dir = Path.of(launcher.env.get("ZDOTDIR"));
String scrub = readAll(dir.resolve(EnvAllowListScrub.SCRUB_FILE));
assertFalse(scrub.contains("'SSH_AUTH_SOCK'"),
"SSH_AUTH_SOCK must not be on the derived allow-list just because the operator put "
+ "it under allow: — sshAuthSock is unset here, so it defaults to block");
}
/**
* CB-633 follow-up criterion 3: on every allow-list spawn the daemon logs one INFO line, shaped
* "member credentials: allowed N of M", with real counts — not constants. Real path: the count
* is asserted after a real {@link HerdrPeerLauncher#spawn} call, reading the log the production
* code actually emits.
*/
@Test
void logsAnAllowedCountLineAgainstTheHostEnvironmentOnEverySpawn() {
FakeHerdr herdr = new FakeHerdr();
// INJECTED is a key of this launch's own env map, so it always survives; the other two are
// neither derived from the profile nor configured anywhere, so they are blanked. Real
// N=1 (INJECTED), real M=3 (all three names) — neither number is hardcoded in the assertion
// by coincidence, they follow directly from this fixture.
Set<String> hostEnvNames = Set.of(INJECTED, "SOME_UNRELATED_NAME", "ANOTHER_UNRELATED_NAME");
WiringLauncher launcher = new WiringLauncher(herdr, allowList(), "/bin/zsh", () -> 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);
}
assertTrue(appender.list.stream()
.anyMatch(e -> "member credentials: allowed 1 of 3".equals(e.getFormattedMessage())),
"expected 'member credentials: allowed 1 of 3', got: "
+ appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList());
}
private static String readAll(Path p) {
try {
return Files.readString(p);
@@ -134,10 +222,16 @@ class HerdrPeerLauncherAllowListWiringTest {
}
WiringLauncher(FakeHerdr herdr, Supplier<FleetConfig.MemberCredentials> creds, String shell) {
this(herdr, creds, shell, null);
}
/** Plus an injectable {@code hostEnvNames} source, for the "allowed N of M" log line test. */
WiringLauncher(FakeHerdr herdr, Supplier<FleetConfig.MemberCredentials> creds, String shell,
Supplier<Set<String>> hostEnvNames) {
super("test", new AgentControl(herdr), new WorkspaceControl(herdr),
Map.of("test", profile()), "test",
name -> "SHELL".equals(name) ? shell : null,
0, () -> 0L, () -> { }, null, creds);
0, () -> 0L, () -> { }, null, creds, hostEnvNames);
}
@Override
@@ -88,6 +88,39 @@ class MemberEnvAllowListTest {
assertTrue(after.containsAll(Set.of("TOKEN_SECOND", "GIT_TOK", "SECOND_KEY")));
}
/**
* CB-633 follow-up: a name that appears ONLY in {@code memberCredentials.allow:} — no profile
* mentions it at all — must still survive the derivation. Before this fix {@code derive} never
* saw {@code allow:}, so setting {@code policy: allow-list} silently blanked exactly this name.
*/
@Test
void aNameOnlyInMemberCredentialsAllowSurvivesDerivation() {
FleetConfig.Profile p = profile("p", "TOKEN_A", null, null, Map.of("KEY_A", "v"));
Set<String> derived = MemberEnvAllowList.derive(List.of(p), Set.of("OPERATOR_ONLY_NAME"));
assertTrue(derived.contains("OPERATOR_ONLY_NAME"),
"memberCredentials.allow: must be unioned in, not ignored");
// and the profile-derived half must still be present — this is a union, not a replacement.
assertTrue(derived.contains("KEY_A"));
assertTrue(derived.contains("TOKEN_A"));
}
/**
* {@code SSH_AUTH_SOCK} is a live handle to the operator's ssh-agent, never a value — so it must
* stay excluded from the derived set even when the operator lists it under {@code allow:} for an
* unrelated reason. It is governed ONLY by {@code memberCredentials.sshAuthSock}, applied
* separately by the caller ({@code HerdrPeerLauncher}).
*/
@Test
void sshAuthSockInMemberCredentialsAllowIsStillExcluded() {
Set<String> derived = MemberEnvAllowList.derive(List.of(), Set.of("SSH_AUTH_SOCK", "OTHER_NAME"));
assertFalse(derived.contains("SSH_AUTH_SOCK"),
"SSH_AUTH_SOCK must never ride in on the generic allow: list");
assertTrue(derived.contains("OTHER_NAME"), "other allow: names are unaffected");
}
/** {@code LC_*} categories are infrastructure by prefix; everything else needs an exact match. */
@Test
void keepsMatchesExactlyPlusTheLocalePrefixRule() {