From e3e403e5c887402531a1223d3024ca9a5da715c4 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 10 Sep 2026 08:08:41 +0700 Subject: [PATCH] #394: contain a fatal export error instead of letting it abort the scrub EnvAllowListScrub's blanking loop used a plain `export "$n="` on every name not on the allow-list. For a zsh read-only/special parameter (e.g. UID) that is a FATAL parameter error, and since the loop runs inside the sourced startup file, the error aborts the whole file: every name still to come is never blanked, and scrub-report.txt is never written at all -- silently, because 2>/dev/null on the group swallows it. Route each blanking attempt through `eval` instead, which contains the error to that one iteration. The loop always finishes; a name it could not blank is now counted separately ("failed" on the report's first line) and listed !-prefixed rather than disappearing. No skip-list of known-bad names is added -- every enumerated name is still attempted, so a name nobody has thought of is still tried and, if it fails, still counted. HerdrPeerLauncher: log a WARN when a pane's report carries a nonzero failed count, and reword the "no report at all" WARN so it no longer claims the daemon knows the member "saw the full host environment" -- a partial vs. a missing scrub are different situations and only the first is now distinguishable from the report alone. --- .../ltms/fleet/member/EnvAllowListScrub.java | 73 +++++++++++++++---- .../ltms/fleet/member/HerdrPeerLauncher.java | 10 ++- .../fleet/member/EnvAllowListScrubTest.java | 71 ++++++++++++++++++ 3 files changed, 139 insertions(+), 15 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java index d56105b..b2ba33e 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java @@ -75,9 +75,19 @@ import java.util.stream.Stream; * never to the control. * *

The scrub also writes {@code scrub-report.txt} into its own directory: one {@code allowed N of - * M} line (N = exports left untouched, M = exports present when the scrub ran), then the blanked - * NAMES — never values. The launcher reads this back at teardown and logs it, because a blocked - * count next to an unknown denominator is not a finding. + * M failed F} line (N = exports left untouched, M = exports present when the scrub ran, F = names + * the scrub attempted to blank but could not), then the NAMES — blanked ones bare, unblankable ones + * {@code !}-prefixed — never values. The launcher reads this back at teardown and logs it, because a + * blocked count next to an unknown denominator is not a finding. + * + *

fleetd #394: plain {@code export "$n="} is a FATAL error for a zsh read-only or special + * parameter (for example {@code UID}) — it aborts the whole sourced file, so every name still to + * come is never blanked and the report above is never written at all. The blanking loop instead + * routes each attempt through {@code eval}, which contains that error to the single iteration: the + * loop always finishes, and a name that could not be blanked is counted as {@code failed} and + * listed {@code !}-prefixed rather than silently disappearing. This is deliberately not a skip-list + * of known-bad names — every enumerated name is still attempted, so a name nobody has thought of + * yet still gets tried and, if it fails, still gets counted. */ public final class EnvAllowListScrub { @@ -132,10 +142,16 @@ public final class EnvAllowListScrub { } /** - * A parsed {@code scrub-report.txt}: how many exported variables existed when the scrub ran, - * how many were left untouched (allowed), and the NAMES that were blanked. Values never appear. + * A parsed {@code scrub-report.txt}: how many exported variables existed when the scrub ran + * ({@code total}), how many were left untouched ({@code allowed}), how many the scrub attempted + * to blank but could not ({@code failed} — fleetd #394: a zsh read-only/special parameter such + * as {@code UID} fatally errors on plain {@code export NAME=}, so those attempts go through + * {@code eval} instead so the loop keeps going and the failure is counted rather than left + * invisible), and the NAMES in each of the latter two categories. {@code allowed + + * blanked.size() + unblankable.size() == total}, and {@code unblankable.size() == failed}. + * Values never appear. */ - record ScrubReport(int allowed, int total, List blanked) { + record ScrubReport(int allowed, int total, int failed, List blanked, List unblankable) { } /** @@ -334,15 +350,32 @@ public final class EnvAllowListScrub { _cb633_blank+=("$_cb633_n") done - { for _cb633_n in "${_cb633_blank[@]}"; do export "$_cb633_n="; done; } 2>/dev/null + # fleetd #394: plain `export "$n="` is FATAL for a zsh read-only/special parameter + # (e.g. UID) and aborts this whole sourced file — every name still to come is never + # blanked, and the report below is never written, silently. `eval` contains that + # error to the single iteration instead: it still fails for that one name, but the + # loop continues and we can tell allowed / blanked / unblankable apart afterwards. + # This is not a skip-list of known-bad names (that would miss the next one nobody + # thought of) — every name in _cb633_blank is still attempted, unconditionally. + typeset -a _cb633_ok _cb633_unblankable + _cb633_ok=() + _cb633_unblankable=() + for _cb633_n in "${_cb633_blank[@]}"; do + if eval "export ${_cb633_n}=" 2>/dev/null; then + _cb633_ok+=("$_cb633_n") + else + _cb633_unblankable+=("$_cb633_n") + fi + done integer _cb633_kept=$(( _cb633_total - ${#_cb633_blank} )) { - print -r -- "allowed $_cb633_kept of $_cb633_total" - for _cb633_n in "${_cb633_blank[@]}"; do print -r -- "$_cb633_n"; done + print -r -- "allowed $_cb633_kept of $_cb633_total failed ${#_cb633_unblankable}" + for _cb633_n in "${_cb633_ok[@]}"; do print -r -- "$_cb633_n"; done + for _cb633_n in "${_cb633_unblankable[@]}"; do print -r -- "!$_cb633_n"; done } > "$ZDOTDIR/%s" 2>/dev/null - unset _cb633_allowed _cb633_names _cb633_blank _cb633_n _cb633_total _cb633_kept + unset _cb633_allowed _cb633_names _cb633_blank _cb633_ok _cb633_unblankable _cb633_n _cb633_total _cb633_kept """.formatted(names, MemberEnvAllowList.zshCasePattern(), REPORT_FILE); } @@ -356,6 +389,11 @@ public final class EnvAllowListScrub { * Read and parse {@link #REPORT_FILE} out of a generated ZDOTDIR directory. Returns {@code null} * when absent or unreadable (the pane may have been torn down before its login shell ever got to * the scrub) — callers treat that as "no measurement available", never as success. + * + *

First line is {@code "allowed of failed "} (fleetd #394 added the trailing + * {@code failed } — a count of names the scrub attempted to blank but could not, e.g. a zsh + * read-only/special parameter). Every following non-blank line is a name: a bare name was + * blanked, a {@code !}-prefixed name was attempted and failed. Values never appear on either. */ static ScrubReport readReport(Path zdotdir) { Path report = zdotdir.resolve(REPORT_FILE); @@ -368,17 +406,24 @@ public final class EnvAllowListScrub { return null; } String[] parts = lines.getFirst().substring("allowed ".length()).trim().split("\\s+"); - if (parts.length != 3 || !"of".equals(parts[1])) { + if (parts.length != 5 || !"of".equals(parts[1]) || !"failed".equals(parts[3])) { return null; } List blanked = new ArrayList<>(); + List unblankable = new ArrayList<>(); for (int i = 1; i < lines.size(); i++) { - if (!lines.get(i).isBlank()) { - blanked.add(lines.get(i)); + String line = lines.get(i); + if (line.isBlank()) { + continue; + } + if (line.startsWith("!")) { + unblankable.add(line.substring(1)); + } else { + blanked.add(line); } } return new ScrubReport(Integer.parseInt(parts[0]), Integer.parseInt(parts[2]), - List.copyOf(blanked)); + Integer.parseInt(parts[4]), List.copyOf(blanked), List.copyOf(unblankable)); } catch (IOException | NumberFormatException e) { return null; } 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 3d87531..c445715 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java @@ -1643,11 +1643,19 @@ public abstract class HerdrPeerLauncher implements PeerLauncher { 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.", + + "startup files. Either way, we cannot tell from here whether the scrub ran, " + + "so we do not know what that member's environment contained.", paneId, dir); } else { log.info("memberCredentials allow-list: pane {} allowed {} of {} environment variables", paneId, report.allowed(), report.total()); + if (report.failed() > 0) { + log.warn("memberCredentials allow-list: pane {} could not blank {} environment " + + "variable(s) — {} (likely a zsh read-only/special parameter) — those " + + "names were left in the member's environment. Confirm none of them is a " + + "credential.", + paneId, report.failed(), report.unblankable()); + } List shaped = report.blanked().stream() .filter(name -> CREDENTIAL_SHAPED_NAME.matcher(name).matches()) .toList(); diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java index 6d16bba..e967b16 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java @@ -118,6 +118,77 @@ class EnvAllowListScrubTest { "allowed N of M with N <= M — the denominator is always reported"); } + /** + * fleetd #394: the actual defect. Plain {@code export "$n="} is FATAL for a zsh read-only or + * special parameter (e.g. {@code UID}) and aborts the whole sourced file — every name still to + * come is never blanked, and the {@code scrub-report.txt} below is never written at all, + * silently ({@code 2>/dev/null} swallows the error). This plants an unblankable, exported, + * read-only variable in the MIDDLE of the names the scrub attempts to blank, with two more + * names after it, and asserts that both of those later names are STILL blanked and the report + * is STILL written with the failure counted — a test that only checked names BEFORE the failure + * point would pass today and prove nothing. + * + *

The planted name is a made-up one ({@code FLEETD_TEST_UNBLANKABLE}), not {@code UID} or + * any other name a skip-list might already know about — invariant 1 is that the loop survives + * ANY unblankable name, not a known one, so the test must not lean on one either. + * + *

Exercises the real artefact: {@link EnvAllowListScrub#scrubScript} is run verbatim under a + * real {@code /bin/zsh}, not just asserted on as a Java string. The four planted names are + * exported one at a time via {@code typeset -x}/{@code typeset -rx} immediately before the + * script runs, in a fixed order — zsh's {@code export}/{@code typeset -x} appends to the + * process's environment table in call order (verified empirically: a freshly-exported name + * always sorts after every inherited one and after every earlier freshly-exported name in + * {@code command env}'s own output), which is what makes the "middle" position deterministic + * here, unlike relying on the OS's own inherited-environment order. + */ + @Test + void unblankableNameInTheMiddleDoesNotAbortNamesAfterIt(@TempDir Path tmp) throws Exception { + assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here"); + + // Only ZDOTDIR is allowed — it must survive the scrub itself, since the report is written + // to "$ZDOTDIR/..." AFTER the blanking loop runs; if ZDOTDIR were blanked as a side effect, + // the report write would silently go to the wrong place instead of testing anything. + String script = EnvAllowListScrub.scrubScript(Set.of("ZDOTDIR")); + String setup = """ + typeset -x FLEETD_TEST_BEFORE=1 + typeset -rx FLEETD_TEST_UNBLANKABLE=1 + typeset -x FLEETD_TEST_AFTER_A=1 + typeset -x FLEETD_TEST_AFTER_B=1 + """; + + ProcessBuilder pb = new ProcessBuilder("/bin/zsh"); + pb.environment().clear(); + pb.environment().put("PATH", "/usr/bin:/bin"); + pb.environment().put("ZDOTDIR", tmp.toAbsolutePath().toString()); + pb.redirectError(ProcessBuilder.Redirect.DISCARD); + Process zsh = pb.start(); + zsh.getOutputStream().write((setup + script).getBytes(StandardCharsets.UTF_8)); + zsh.getOutputStream().flush(); + zsh.getOutputStream().close(); + assertTrue(zsh.waitFor(60, java.util.concurrent.TimeUnit.SECONDS), + "the scrub script did not exit within 60s"); + assertEquals(0, zsh.exitValue(), + "the scrub script itself must never abort — an unblankable name must not kill the " + + "sourced file"); + + EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(tmp); + assertNotNull(report, "the report must still be written even though one name could not be " + + "blanked — a report that silently never appears is the #394 bug"); + assertTrue(report.blanked().contains("FLEETD_TEST_BEFORE"), + "sanity: the name before the unblankable one must be blanked"); + assertTrue(report.blanked().contains("FLEETD_TEST_AFTER_A"), + "the FIRST name AFTER the unblankable one must still be blanked — before the fix, " + + "the whole loop aborted at the unblankable name and every later name was " + + "silently left untouched"); + assertTrue(report.blanked().contains("FLEETD_TEST_AFTER_B"), + "the SECOND name after the unblankable one must also still be blanked"); + assertEquals(1, report.failed(), + "exactly one attempted name could not be blanked, and that count must be visible " + + "without reading the member's environment"); + assertEquals(List.of("FLEETD_TEST_UNBLANKABLE"), report.unblankable(), + "the unblankable name is reported by name, not silently dropped"); + } + /** A group-shared ZDOTDIR still lets the member truncate and write its pre-created receipt. */ @Test void groupSharedScrubWritesAndReadsItsReport(@TempDir Path tmp) throws Exception {