diff --git a/bridged/bridged.example.yaml b/bridged/bridged.example.yaml index 2912a60..405c903 100644 --- a/bridged/bridged.example.yaml +++ b/bridged/bridged.example.yaml @@ -455,7 +455,23 @@ guard: # the operator's shell holds, not just the ones bridged means to give it. Measured on this host: # 31 credential names, all set, with only ONE (GITEA_ACCESS_TOKEN) blocked before this — and that # block was a single name hardcoded in HerdrPeerLauncher.java, not driven by this file. This block -# replaces that hardcoded shadow. +# replaces that hardcoded shadow with a config-driven list of names. +# +# ROUND-2 CORRECTION, measured live: the pane-creation env overlay below (applied at tab.create / +# pane.split, BEFORE the pane's login shell runs) does NOT survive that login shell for any name +# secrets.sh actually exports — the shell re-exports it afterwards and overwrites the sentinel. +# Proof: GITEA_ACCESS_TOKEN comes back blocked only because secrets.sh itself carries a guarded +# export (`[ -n "${BRIDGED_MEMBER:-}" ] || export GITEA_ACCESS_TOKEN=...`) — that guard, not this +# file, is what wins. No other name in `known` below has a matching guard in secrets.sh yet (1 +# guard measured against 33 export lines there). So today this block's overlay is REAL protection +# only for a name secrets.sh does not export, or a peer kind whose pane never runs a login shell — +# for everything secrets.sh exports and guards, the guard in secrets.sh (out of scope for this +# ticket) is what actually blocks it, not this list. An exec-time fix (winning after the login +# shell finishes, before the agent process starts) was attempted and found to have no seam in the +# current herdr protocol — AgentControl.start takes a fixed `kind` (herdr resolves the executable) +# plus trailing CLI args for that binary, not an arbitrary argv or an env map; only tab.create / +# pane.split accept `env`, and that is this same pane-creation overlay. See gitea #82 for the open +# design question this leaves. # # DENY-BY-DEFAULT, NOT A DENY-LIST. A deny-list (name the bad ones, let everything else through) is # silently wrong the moment the operator's store gains a new secret — nothing would ever report it. @@ -471,8 +487,9 @@ guard: # allow → credential names a member legitimately needs. Left OUT of the pane's env overlay # entirely, so the value the pane's own (login) shell exports passes through untouched. # known → every credential name the operator's store is known to export. Every name here NOT -# also in `allow` is overlaid with a non-secret sentinel value before the member process -# starts, shadowing whatever the login shell would otherwise put there. +# also in `allow` is overlaid with a non-secret sentinel value before the pane's login +# shell runs — real protection only for names the login shell does not itself re-export +# (see the ROUND-2 CORRECTION note above for the ones it does). # # HOT-RELOADABLE the same way `fleet:` is (CB-559): read fresh on every spawn, so editing this list # and reloading config (or restarting) changes what the NEXT spawn inherits; already-running members diff --git a/bridged/src/main/java/dev/ltms/bridged/Bridged.java b/bridged/src/main/java/dev/ltms/bridged/Bridged.java index 73d33d9..4a62794 100644 --- a/bridged/src/main/java/dev/ltms/bridged/Bridged.java +++ b/bridged/src/main/java/dev/ltms/bridged/Bridged.java @@ -87,6 +87,11 @@ public final class Bridged { // else can fail on a silently-empty one. A daemon started without a login shell (launchd) // boots fine either way — this is the only thing that says so out loud. reportRequiredSecrets(cfg); + // CB-596: an absent (or empty) memberCredentials: block blocks NOTHING — no credential + // name is hardcoded any more to fall back on. Say so loudly, the same way a missing + // secret is reported above, so upgrading past this commit never silently drops CB-592's + // protection. + reportMemberCredentialsGap(cfg); // CB-559: `cfg` stays the startup snapshot — every validation and every piece of one-time // wiring below reads it, and must, because those decisions cannot be unmade. `config` is the // live reference the hot paths read per use. Which keys can actually move is ConfigRef's @@ -615,6 +620,31 @@ public final class Bridged { }); } + /** + * CB-596: {@code known:} empty (block absent entirely, or present but empty) means {@link + * BridgedConfig.MemberCredentials#blockedSet()} is empty too — every member pane inherits the + * operator's whole secret store, unblocked, exactly the defect this ticket fixes. Unlike a + * missing token ({@link #reportRequiredSecrets}), there is no name to point at: the point is + * that the block itself is missing. Warn once at startup and say what to add; never refuse to + * start over it — see {@link #reportRequiredSecrets} for why a daemon that boots and says + * what is wrong beats one that will not boot at all. + * + *

Package-private so the test can capture the log directly, the same way {@link + * #requiredSecretEnvVars} is exposed for {@link #reportRequiredSecrets}'s own test. + */ + static void reportMemberCredentialsGap(BridgedConfig cfg) { + BridgedConfig.MemberCredentials creds = cfg.memberCredentials(); + if (creds != null && !creds.known().isEmpty()) { + log.info("memberCredentials: {} known name(s), {} allowed — blocking {} on every spawn", + creds.known().size(), creds.allow().size(), creds.blockedSet().size()); + return; + } + log.warn("memberCredentials: absent or empty — the daemon will start anyway, and every " + + "member pane inherits the operator's WHOLE secret store, unblocked (CB-592's " + + "protection is lost). Add a memberCredentials: block (policy/allow/known) to " + + "bridged.yaml — see bridged.example.yaml — and restart."); + } + /** * Poll herdr's {@code ping} until it answers or {@link #HERDR_WAIT_SECONDS} elapses (CB-504). * diff --git a/bridged/src/test/java/dev/ltms/bridged/MemberCredentialsGapReportTest.java b/bridged/src/test/java/dev/ltms/bridged/MemberCredentialsGapReportTest.java new file mode 100644 index 0000000..579019e --- /dev/null +++ b/bridged/src/test/java/dev/ltms/bridged/MemberCredentialsGapReportTest.java @@ -0,0 +1,108 @@ +package dev.ltms.bridged; + +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; +import dev.ltms.bridged.config.BridgedConfig; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.slf4j.LoggerFactory; + +import java.nio.file.Files; +import java.nio.file.Path; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * CB-596: an absent (or empty) {@code memberCredentials:} block blocks nothing — no name is + * hardcoded any more to fall back on, so the daemon must say so out loud at startup rather than + * silently dropping CB-592's protection. Mirrors {@link RequiredSecretEnvVarsTest}'s pattern for + * the CB-594 startup-secrets report, capturing the real log via a {@link ListAppender}. + */ +class MemberCredentialsGapReportTest { + + private static BridgedConfig load(Path dir, String yaml) throws Exception { + Path f = dir.resolve("bridged.yaml"); + Files.writeString(f, yaml); + return BridgedConfig.load(f); + } + + private static ListAppender attach() { + Logger logger = (Logger) LoggerFactory.getLogger(Bridged.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + return appender; + } + + private static void detach(ListAppender appender) { + ((Logger) LoggerFactory.getLogger(Bridged.class)).detachAppender(appender); + } + + @Test + void anAbsentBlockWarnsThatEveryMemberInheritsTheWholeStore(@TempDir Path dir) throws Exception { + BridgedConfig cfg = load(dir, "bind:\n host: 127.0.0.1\n port: 8765\n"); + + ListAppender appender = attach(); + try { + Bridged.reportMemberCredentialsGap(cfg); + } finally { + detach(appender); + } + + assertTrue(appender.list.stream().anyMatch(e -> + e.getLevel() == ch.qos.logback.classic.Level.WARN + && e.getFormattedMessage().contains("memberCredentials") + && e.getFormattedMessage().contains("WHOLE secret store")), + "an absent block must WARN that protection is lost, not stay silent"); + } + + @Test + void anEmptyKnownListWarnsTheSameAsAbsent(@TempDir Path dir) throws Exception { + BridgedConfig cfg = load(dir, """ + memberCredentials: + policy: deny-by-default + """); + + ListAppender appender = attach(); + try { + Bridged.reportMemberCredentialsGap(cfg); + } finally { + detach(appender); + } + + assertTrue(appender.list.stream().anyMatch(e -> + e.getLevel() == ch.qos.logback.classic.Level.WARN + && e.getFormattedMessage().contains("memberCredentials")), + "policy: with no known: names still blocks nothing and must warn the same way"); + } + + @Test + void aPopulatedKnownListLogsInfoNotWarn(@TempDir Path dir) throws Exception { + BridgedConfig cfg = load(dir, """ + memberCredentials: + policy: deny-by-default + allow: [AI_GATEWAY_TOKEN] + known: [AI_GATEWAY_TOKEN, GITEA_ACCESS_TOKEN] + """); + + // logback-test.xml pins dev.ltms.bridged to WARN (see its own comment); raise it here so + // the INFO line this test asserts on actually reaches the appender, and restore after. + Logger logger = (Logger) LoggerFactory.getLogger(Bridged.class); + ch.qos.logback.classic.Level original = logger.getLevel(); + logger.setLevel(ch.qos.logback.classic.Level.INFO); + ListAppender appender = attach(); + try { + Bridged.reportMemberCredentialsGap(cfg); + } finally { + detach(appender); + logger.setLevel(original); + } + + assertFalse(appender.list.stream().anyMatch(e -> e.getLevel() == ch.qos.logback.classic.Level.WARN), + "a configured, non-empty known: list must not warn — the block is doing its job"); + assertTrue(appender.list.stream().anyMatch(e -> e.getFormattedMessage().contains("blocking 1")), + "the INFO line should say how many names are actually blocked (known minus allow)"); + } +}