From ad587eafa3bbf759277692b673075abd7dddc462 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Mon, 31 Aug 2026 14:21:48 +0700 Subject: [PATCH] #172: keep the broker URI, password and all, out of every member pane broker.uriEnv names an environment variable holding amqp://user:password@host, and it was reaching every member. Its name is not credential-shaped -- no TOKEN, KEY or SECRET in it -- so every name-pattern heuristic missed it, and it sat on neither credential list. fleetd already knows the name: the operator wrote it in broker.uriEnv. So derive the exclusion from the config rather than hoping an operator also remembers to deny it. coordinator.uriEnv has the same shape and is excluded too; on this host both resolve to the same variable. Excluded even when the operator lists the name under memberCredentials.allow:, following the SSH_AUTH_SOCK precedent. There is no override, because a member has no legitimate use for the broker password. Reviewer finding, recorded rather than overstated: this is only a hard guarantee under policy: allow-list, where the ZDOTDIR scrub runs after the pane's shell has sourced the operator's chain. Under deny-list the name is removed from the pre-shell env only, and a login shell re-exports it. That is deny-list's existing weakness rather than a regression here, but the javadoc now says so plainly instead of implying a guarantee that path cannot give. Co-authored-by: fleetd worker --- .../src/main/java/dev/ltms/fleet/Fleetd.java | 4 +- .../ltms/fleet/member/ClaudeCodeLauncher.java | 41 +++++++++++--- .../ltms/fleet/member/HerdrPeerLauncher.java | 32 +++++++++-- .../ltms/fleet/member/MemberEnvAllowList.java | 55 ++++++++++++++++++- .../ltms/fleet/member/OpenCodeLauncher.java | 33 +++++++++-- .../HerdrPeerLauncherAllowListWiringTest.java | 47 +++++++++++++++- .../fleet/member/MemberEnvAllowListTest.java | 28 ++++++++++ 7 files changed, 216 insertions(+), 24 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java index c63aa07..4988c95 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java +++ b/fleetd/src/main/java/dev/ltms/fleet/Fleetd.java @@ -177,14 +177,14 @@ public final class Fleetd { claudeProfiles, cfg.effectiveDefaultProfile(), System::getenv, cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), () -> config.get().fleet(), - () -> config.get().memberCredentials())); + () -> config.get().memberCredentials(), null, config::get)); } if (!opencodeProfiles.isEmpty()) { adapters.add(new OpenCodeLauncher(router.memberAgents(), router.memberSpaces(), opencodeProfiles, cfg.effectiveDefaultProfile(), System::getenv, cfg.spawnReadyTimeoutMs(), cfg.spawnReadyPollMs(), () -> config.get().fleet(), - () -> config.get().memberCredentials())); + () -> config.get().memberCredentials(), config::get)); } AtomicReference> liveCountRef = new AtomicReference<>(_ -> 0); // CB-578 stage B: one quarantine tracker for the whole daemon, shared between the launcher diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java index 3079c0e..0e55579 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java @@ -94,13 +94,26 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { public ClaudeCodeLauncher(AgentControl agents, WorkspaceControl spaces, SubscriptionGuard guard, Map profiles, String defaultProfile, Function env, - long spawnReadyTimeoutMs, long spawnReadyPollMs, - Supplier fleet, - Supplier memberCredentials) { + long spawnReadyTimeoutMs, long spawnReadyPollMs, + Supplier fleet, + Supplier memberCredentials) { + this(agents, spaces, guard, profiles, defaultProfile, env, spawnReadyTimeoutMs, spawnReadyPollMs, + fleet, memberCredentials, null, null); + } + + /** Production constructor, plus the live config for URI environment exclusions. */ + public ClaudeCodeLauncher(AgentControl agents, WorkspaceControl spaces, SubscriptionGuard guard, + Map profiles, String defaultProfile, + Function env, + long spawnReadyTimeoutMs, long spawnReadyPollMs, + Supplier fleet, + Supplier memberCredentials, + Supplier> hostEnvNames, + Supplier config) { this(agents, spaces, guard, profiles, defaultProfile, env, spawnReadyTimeoutMs, System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), - fleet, memberCredentials); + fleet, memberCredentials, hostEnvNames, config); } /** @@ -153,10 +166,22 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { Function env, long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper, - Supplier fleet, - Supplier memberCredentials) { + Supplier fleet, + Supplier memberCredentials) { + this(agents, spaces, guard, profiles, defaultProfile, env, spawnReadyTimeoutMs, nowMillis, sleeper, + fleet, memberCredentials, null, null); + } + + /** Full testability constructor, plus the live config for URI environment exclusions. */ + public ClaudeCodeLauncher(AgentControl agents, WorkspaceControl spaces, SubscriptionGuard guard, + Map profiles, String defaultProfile, + Function env, long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, Supplier fleet, + Supplier memberCredentials, + Supplier> hostEnvNames, + Supplier config) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, - spawnReadyTimeoutMs, nowMillis, sleeper, fleet, memberCredentials); + spawnReadyTimeoutMs, nowMillis, sleeper, fleet, memberCredentials, hostEnvNames, config); this.guard = guard; } @@ -174,7 +199,7 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { Supplier memberCredentials, Supplier> hostEnvNames) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, - spawnReadyTimeoutMs, nowMillis, sleeper, fleet, memberCredentials, hostEnvNames); + spawnReadyTimeoutMs, nowMillis, sleeper, fleet, memberCredentials, hostEnvNames, null); this.guard = guard; } 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 113fb22..25fd025 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -170,6 +170,8 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { /** Guards {@link #warnNonZsh} to one WARN per launcher instance, not one per spawn. */ private final AtomicBoolean nonZshShellWarned = new AtomicBoolean(); + /** Live config provides URI environment names that must never enter member panes. */ + private final Supplier config; /** * @param namePrefix label prefix for this peer kind (drives naming and reap) @@ -238,8 +240,19 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { long spawnReadyTimeoutMs, LongSupplier nowMillis, Runnable sleeper, Supplier fleet, + Supplier memberCredentials, + Supplier> hostEnvNames) { + this(namePrefix, agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, nowMillis, + sleeper, fleet, memberCredentials, hostEnvNames, null); + } + + /** As above, plus the live full config for secret-bearing URI environment names. */ + protected HerdrPeerLauncher(String namePrefix, AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, Supplier fleet, Supplier memberCredentials, - Supplier> hostEnvNames) { + Supplier> hostEnvNames, Supplier config) { this.fleet = fleet; this.namePrefix = namePrefix; this.agents = agents; @@ -252,6 +265,7 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { this.sleeper = sleeper; this.memberCredentials = memberCredentials; this.hostEnvNames = hostEnvNames != null ? hostEnvNames : () -> System.getenv().keySet(); + this.config = config; } // --- adapter seams ------------------------------------------------------------------------- @@ -1028,9 +1042,11 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { } /** Put {@link #BLOCKED_CREDENTIAL_SENTINEL} over every blocked name in the pane-creation env map. */ - private static void overlayBlockedCredentials(Map workerEnv, - FleetConfig.MemberCredentials creds) { - for (String name : creds.blockedSet()) { + private void overlayBlockedCredentials(Map workerEnv, + FleetConfig.MemberCredentials creds) { + Set blocked = new java.util.TreeSet<>(creds.blockedSet()); + blocked.addAll(brokerUriEnvNames()); + for (String name : blocked) { workerEnv.put(name, BLOCKED_CREDENTIAL_SENTINEL); } } @@ -1097,15 +1113,21 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { * ssh-agent handle when explicitly allowed, and the exact keys of THIS launch's own env map. */ private Set derivedAllowedNames(FleetConfig.MemberCredentials creds, Launch launch) { + Set brokerUriEnvNames = brokerUriEnvNames(); Set allowed = new java.util.TreeSet<>( - MemberEnvAllowList.derive(profiles.values(), creds.allowSet())); + MemberEnvAllowList.derive(profiles.values(), creds.allowSet(), brokerUriEnvNames)); 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()); + allowed.removeAll(brokerUriEnvNames); return allowed; } + private Set brokerUriEnvNames() { + return MemberEnvAllowList.brokerUriEnvNames(config == null ? null : config.get()); + } + /** * 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 diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java b/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java index 8ed0872..07d927b 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/MemberEnvAllowList.java @@ -40,8 +40,9 @@ import java.util.TreeSet; * 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 + * SSH_AUTH_SOCK} and configured broker URI environment names are exceptions: even when the operator + * lists them under {@code allow:}, they are excluded here. {@code SSH_AUTH_SOCK} is 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 @@ -106,6 +107,16 @@ public final class MemberEnvAllowList { * run-to-run. */ public static Set derive(Collection profiles, Set configuredAllow) { + return derive(profiles, configuredAllow, Set.of()); + } + + /** + * As {@link #derive(Collection, Set)}, while excluding names that fleetd knows carry credentials. + * A configured broker URI contains its AMQP password inline, so it must never reach a member, + * even when an operator put its variable name in {@code memberCredentials.allow:}. + */ + public static Set derive(Collection profiles, Set configuredAllow, + Set excludedNames) { Set derived = new TreeSet<>(INFRASTRUCTURE_PASSTHROUGH); if (profiles != null) { for (FleetConfig.Profile p : profiles) { @@ -124,9 +135,37 @@ public final class MemberEnvAllowList { } } } + if (excludedNames != null) { + derived.removeAll(excludedNames); + } return Set.copyOf(derived); } + /** + * The host environment names whose values are AMQP URIs with inline passwords. Both broker + * connections belong to fleetd, never to a member pane. Blank and absent configuration changes + * nothing. + * + *

How strong this exclusion is depends on the policy, and the difference matters. + * Under {@code policy: allow-list} it is enforced by the generated ZDOTDIR scrub, which runs + * AFTER the pane's shell has sourced the operator's chain — so a login shell that re-exports the + * name is still blanked. Under the deny-list policy there is no scrub: the name is only removed + * from the pre-shell env map, and a login shell that sources the operator's secret store + * re-exports it. That is the long-standing weakness of deny-list (a sourced file can undo it), + * not something this exclusion introduces, but it means deny-list deployments do NOT get this + * guarantee. The same caveat applies to the non-zsh path, which has no scrub at all — see + * {@code HerdrPeerLauncher#applyEnvironmentAllowListPolicy}. + */ + public static Set brokerUriEnvNames(FleetConfig config) { + if (config == null) { + return Set.of(); + } + Set names = new TreeSet<>(); + addUriEnvIfPresent(names, config.broker()); + addUriEnvIfPresent(names, config.coordinator()); + return Set.copyOf(names); + } + /** * Whether {@code name} survives the scrub when {@code allowedNames} is the derived set: an exact * match, or an infrastructure-prefixed name ({@code LC_*}). Prefix rules live ONLY here and in @@ -146,4 +185,16 @@ public final class MemberEnvAllowList { into.add(name); } } + + private static void addUriEnvIfPresent(Set into, FleetConfig.Broker broker) { + if (broker != null && broker.hasUriEnv()) { + into.add(broker.uriEnv()); + } + } + + private static void addUriEnvIfPresent(Set into, FleetConfig.Coordinator coordinator) { + if (coordinator != null && coordinator.hasUriEnv()) { + into.add(coordinator.uriEnv()); + } + } } diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java index 0401d07..7f75538 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java @@ -116,12 +116,23 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { public OpenCodeLauncher(AgentControl agents, WorkspaceControl spaces, Map profiles, String defaultProfile, Function env, - long spawnReadyTimeoutMs, long spawnReadyPollMs, - Supplier fleet, - Supplier memberCredentials) { + long spawnReadyTimeoutMs, long spawnReadyPollMs, + Supplier fleet, + Supplier memberCredentials) { + this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, spawnReadyPollMs, + fleet, memberCredentials, null); + } + + /** Production constructor, plus the live config for URI environment exclusions. */ + public OpenCodeLauncher(AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, long spawnReadyTimeoutMs, long spawnReadyPollMs, + Supplier fleet, + Supplier memberCredentials, + Supplier config) { this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, System::currentTimeMillis, () -> sleepUninterruptibly(spawnReadyPollMs), - defaultConfigRoot(), defaultDiscoveryRoot(), fleet, memberCredentials); + defaultConfigRoot(), defaultDiscoveryRoot(), fleet, memberCredentials, config); } /** @@ -182,8 +193,20 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { Path configRoot, Path discoveryRoot, Supplier fleet, Supplier memberCredentials) { + this(agents, spaces, profiles, defaultProfile, env, spawnReadyTimeoutMs, nowMillis, sleeper, + configRoot, discoveryRoot, fleet, memberCredentials, null); + } + + /** Full testability constructor, plus the live config for URI environment exclusions. */ + public OpenCodeLauncher(AgentControl agents, WorkspaceControl spaces, + Map profiles, String defaultProfile, + Function env, long spawnReadyTimeoutMs, + LongSupplier nowMillis, Runnable sleeper, Path configRoot, Path discoveryRoot, + Supplier fleet, + Supplier memberCredentials, + Supplier config) { super(NAME_PREFIX, agents, spaces, profiles, defaultProfile, env, - spawnReadyTimeoutMs, nowMillis, sleeper, fleet, memberCredentials); + spawnReadyTimeoutMs, nowMillis, sleeper, fleet, memberCredentials, null, config); this.configRoot = configRoot; this.discovery = new OpenCodeSessionDiscovery(discoveryRoot); } 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 2e926b2..e68d729 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/HerdrPeerLauncherAllowListWiringTest.java @@ -161,6 +161,34 @@ class HerdrPeerLauncherAllowListWiringTest { + "it under allow: — sshAuthSock is unset here, so it defaults to block"); } + @Test + void brokerUriEnvStaysBlockedWhenListedInMemberCredentialsAllow() { + FakeHerdr herdr = new FakeHerdr(); + WiringLauncher launcher = new WiringLauncher(herdr, + allowListWithAllow(List.of("BROKER_CONNECTION_URI")), "/bin/zsh", null, + () -> config("BROKER_CONNECTION_URI")); + + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + + Path dir = Path.of(launcher.env.get("ZDOTDIR")); + assertFalse(readAll(dir.resolve(EnvAllowListScrub.SCRUB_FILE)).contains("'BROKER_CONNECTION_URI'"), + "broker.uriEnv must not reach a member even when listed in memberCredentials.allow:"); + } + + @Test + void brokerUriEnvIsDeniedUnderTheDenyListPolicyEvenWhenAllowed() { + FakeHerdr herdr = new FakeHerdr(); + WiringLauncher launcher = new WiringLauncher(herdr, + () -> new FleetConfig.MemberCredentials(null, List.of("BROKER_CONNECTION_URI"), List.of(), null), + "/bin/bash", null, + () -> config("BROKER_CONNECTION_URI")); + + launcher.spawn(new SpawnRequest("test", null, null, null, null, MemberRole.DEV)); + + assertEquals("blocked-by-fleetd-cb596-see-gitea-issue-82", launcher.env.get("BROKER_CONNECTION_URI"), + "the deny-list overlay must deny broker.uriEnv even when allow: names it"); + } + /** * 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 @@ -260,15 +288,24 @@ class HerdrPeerLauncherAllowListWiringTest { /** Plus an injectable {@code hostEnvNames} source, for the "allowed N of M" log line test. */ WiringLauncher(FakeHerdr herdr, Supplier creds, String shell, - Supplier> hostEnvNames) { + Supplier> hostEnvNames) { + this(herdr, creds, shell, hostEnvNames, null); + } + + WiringLauncher(FakeHerdr herdr, Supplier creds, String shell, + Supplier> hostEnvNames, Supplier config) { super("test", new AgentControl(herdr), new WorkspaceControl(herdr), Map.of("test", profile()), "test", name -> "SHELL".equals(name) ? shell : null, - 0, () -> 0L, () -> { }, null, creds, hostEnvNames); + 0, () -> 0L, () -> { }, null, creds, hostEnvNames, config); } @Override protected Launch buildLaunch(FleetConfig.Profile cfg, LaunchSpec spec) { + Map launchEnv = baseEnv(cfg); + launchEnv.putAll(env); + env.clear(); + env.putAll(launchEnv); return new Launch(env, List.of("test")); } @@ -278,6 +315,12 @@ class HerdrPeerLauncherAllowListWiringTest { } } + private static FleetConfig config(String brokerUriEnv) { + return new FleetConfig(null, null, null, Map.of(), null, null, null, null, null, + new FleetConfig.Broker(null, brokerUriEnv, null), null, null, null, null, null, + null, null, null, null, null).withDefaults(); + } + /** The generated directory is a temp directory; make sure the test does not leave a pile. */ @Test void theGeneratedDirectoryIsRemovedWhenThePaneIsStopped() { diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java index 3cbb1b3..bc0f9a7 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/MemberEnvAllowListTest.java @@ -121,6 +121,34 @@ class MemberEnvAllowListTest { assertTrue(derived.contains("OTHER_NAME"), "other allow: names are unaffected"); } + @Test + void configuredBrokerAndCoordinatorUriEnvNamesAreExcludedEvenWhenAllowed() { + FleetConfig config = config("BROKER_CONNECTION_URI", "COORDINATOR_CONNECTION_URI"); + Set excluded = MemberEnvAllowList.brokerUriEnvNames(config); + + Set derived = MemberEnvAllowList.derive(List.of(), + Set.of("BROKER_CONNECTION_URI", "COORDINATOR_CONNECTION_URI", "OTHER_NAME"), excluded); + + assertFalse(derived.contains("BROKER_CONNECTION_URI"), + "broker.uriEnv is secret-bearing and must not ride in on allow:"); + assertFalse(derived.contains("COORDINATOR_CONNECTION_URI"), + "coordinator.uriEnv has the same inline-password shape"); + assertTrue(derived.contains("OTHER_NAME"), "unrelated allow: entries are unaffected"); + } + + @Test + void absentOrBlankBrokerUriEnvAddsNoExclusions() { + assertTrue(MemberEnvAllowList.brokerUriEnvNames(config(null, null)).isEmpty()); + assertTrue(MemberEnvAllowList.brokerUriEnvNames(config(" ", "")).isEmpty()); + } + + private static FleetConfig config(String brokerUriEnv, String coordinatorUriEnv) { + return new FleetConfig(null, null, null, Map.of(), null, null, null, null, null, + new FleetConfig.Broker(null, brokerUriEnv, null), null, null, null, null, null, + null, null, null, null, + new FleetConfig.Coordinator(null, coordinatorUriEnv, null, null)).withDefaults(); + } + /** {@code LC_*} categories are infrastructure by prefix; everything else needs an exact match. */ @Test void keepsMatchesExactlyPlusTheLocalePrefixRule() {