From 91a784239371dc89d8827a78e62b993ee66cbc62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=80=AA=E7=A8=8B=E4=BC=9F?= Date: Thu, 2 Jul 2026 11:13:27 +0800 Subject: [PATCH] fix(mcp): fail-closed on unknown channel + signing-key self-heal (#471) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of PR #464 found that classify() promoted an absent channelType to the 'authenticated' trust branch, stamping an untrusted ThreadLocal username (e.g. stale value on a reused thread, or internal tasks like SkillConsolidation/Reflection that carry no channel) with authenticated trust — contradicting the fail-closed contract the service documents. - classify(): channel==null/blank now resolves to NONE (no injection); only the explicit 'web' channel may yield authenticated. Unrecognised non-web channels downgrade to external, never authenticated. - signingKey(): replace the one-shot keyParseAttempted latch with lastAttemptedPem so a corrected/hot-reloaded PEM re-parses on the next call without an app restart. Still fail-closed when PEM is unchanged. - Tests: 4 new cases lock the regression (null+dirty-ThreadLocal->NONE, blank->NONE, novel channel->external, self-heal after config fix). - .gitignore: exclude local .codebase-memory/ agent index. MCP+identity suite: 93/93 green. --- .gitignore | 3 + .../runtime/McpIdentityForwardService.java | 61 +++++++++++--- .../McpIdentityForwardServiceTest.java | 80 +++++++++++++++++++ 3 files changed, 133 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index f93f1342..5e5ab1e8 100644 --- a/.gitignore +++ b/.gitignore @@ -105,6 +105,9 @@ CLAUDE.md # Codex CLI local artifacts .codex/ +# Codebase memory (local agent index / graph artifact; do not commit) +.codebase-memory/ + # Sync tooling local state (generated each run; report is intentionally tracked) scripts/.*-sync-state.json diff --git a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardService.java b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardService.java index 07690792..7bf3d875 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardService.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardService.java @@ -64,9 +64,21 @@ public class McpIdentityForwardService { private final McpIdentityForwardProperties properties; - /** Lazily parsed signing key; {@code null} until first use / when unavailable. */ + /** + * Lazily parsed signing key; {@code null} until first successful parse or + * while a parse is pending. Guarded by {@code this} (the parse block) and + * safe to read outside the lock because the field is volatile and the + * {@link PrivateKey} is published safely after construction. + */ private volatile PrivateKey signingKey; - private volatile boolean keyParseAttempted; + + /** + * PEM content last handed to the parser. Lets the cache self-heal when the + * operator fixes the config (or a config reload pushes a new key) without an + * app restart: if the PEM changed since the last attempt we retry instead of + * sticking with a permanent null. {@code null} = never attempted. + */ + private volatile String lastAttemptedPem; public McpIdentityForwardService(McpIdentityForwardProperties properties) { this.properties = properties; @@ -94,10 +106,13 @@ public class McpIdentityForwardService { * {@link ResolvedIdentity#NONE} for cron / system / unattributed origins * (never assert identity on behalf of a non-user). * - *

Source channel is read from {@link ChatOrigin} (carried on the - * {@link ToolContext}); when the context carries no origin it falls back to - * the legacy {@link ToolExecutionContext} username (treated as web/authenticated - * only if non-blank — preserves the original contract for the ThreadLocal path). + *

Trust is keyed off {@link ChatOrigin#channelType()}: only the explicit + * {@code "web"} channel (built by {@code ChatOrigin.web()}) may resolve to + * {@code authenticated} — every other channel is typed as {@code anonymous} + * / {@code external}, and an unknown (null/blank) channel resolves + * to {@link ResolvedIdentity#NONE} (fail-closed). The ThreadLocal + * {@link ToolExecutionContext} username is consulted only inside the web + * branch and only when no immutable user id is present. */ ResolvedIdentity classify(ToolContext ctx) { ChatOrigin origin = ChatOrigin.from(ctx); @@ -106,10 +121,19 @@ public class McpIdentityForwardService { return ResolvedIdentity.NONE; } String channel = origin.channelType(); + // Unknown / unattributed channel: never assert identity. An absent + // channelType must NOT be promoted to authenticated — only the explicit + // "web" channel (built by ChatOrigin.web()) carries MateClaw's assertion + // that a real account backs this request. Treating null/blank as web + // would silently stamp an untrusted ThreadLocal value with authenticated + // trust, violating the fail-closed contract this service guarantees. + if (channel == null || channel.isBlank()) { + return ResolvedIdentity.NONE; + } // Authenticated web account: MateClaw vouches for this user. Prefer the // immutable numeric id when available (web-console login); fall back to // the username when only the ThreadLocal path supplied identity. - if (channel == null || channel.isBlank() || "web".equals(channel)) { + if ("web".equals(channel)) { if (origin.requesterUserId() != null) { return new ResolvedIdentity(String.valueOf(origin.requesterUserId()), TRUST_AUTHENTICATED, "web"); @@ -180,20 +204,35 @@ public class McpIdentityForwardService { } } + /** + * Resolve the signing key, parsing it lazily. Self-heals when the configured + * PEM changes (e.g. an operator fixes a malformed key or a config reload + * pushes a new one) — a prior failed parse is retried on the next call once + * {@code private-key-pem} differs from what was last attempted, so recovery + * no longer needs an app restart. Stays fail-closed otherwise. + */ private PrivateKey signingKey() { PrivateKey k = signingKey; if (k != null) { return k; } - if (keyParseAttempted) { - return null; // already tried and failed; don't spam parsing + String pem = properties.getToken().getPrivateKeyPem(); + // Skip only while the PEM is unchanged since the last attempt — that + // avoids re-parsing (and re-logging) on every call. A changed PEM clears + // the way for a fresh parse, which is the self-healing path. + if (lastAttemptedPem != null && lastAttemptedPem.equals(pem)) { + return null; } synchronized (this) { if (signingKey != null) { return signingKey; } - keyParseAttempted = true; - String pem = properties.getToken().getPrivateKeyPem(); + // Re-check under the lock: another thread may have just attempted + // the same (unchanged) PEM. + if (lastAttemptedPem != null && lastAttemptedPem.equals(pem)) { + return null; + } + lastAttemptedPem = pem; if (pem == null || pem.isBlank()) { log.error("[McpIdentity] token mode enabled but mateclaw.mcp.identity-forward.token.private-key-pem is empty; " + "identity tokens will NOT be issued (fail-closed)"); diff --git a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpIdentityForwardServiceTest.java b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpIdentityForwardServiceTest.java index e012a7df..8aea890f 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpIdentityForwardServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpIdentityForwardServiceTest.java @@ -129,6 +129,51 @@ class McpIdentityForwardServiceTest { assertThat(id.subject()).isEqualTo("alice"); assertThat(id.trust()).isEqualTo(McpIdentityForwardService.TRUST_AUTHENTICATED); } + + // ---- Fail-closed: an unknown / unattributed channel must NEVER be + // promoted to authenticated, even when a stale ThreadLocal username + // is present. This is the regression for the privilege-escalation + // gap where channel==null used to fall into the web branch and + // stamp an untrusted identifier with authenticated trust. ---- + + @Test + @DisplayName("unknown channelType (null) → NONE, even with a polluted ThreadLocal (no privilege escalation)") + void unknownChannelIsFailClosedEvenWithThreadLocal() { + // A stale username from a prior request reused this thread. + ToolExecutionContext.set("c1", "attacker"); + // System-style origin built with no channelType (e.g. SkillConsolidation + // / SkillReflection internal tasks): requesterId="", channelType=null. + ChatOrigin origin = new ChatOrigin(null, "c1", "", 1L, null, + null, null, false, null, null, null, null, null); + assertThat(svc(new McpIdentityForwardProperties()).classify(ctx(origin))) + .isEqualTo(McpIdentityForwardService.ResolvedIdentity.NONE); + } + + @Test + @DisplayName("blank channelType → NONE (fail-closed, not authenticated)") + void blankChannelIsFailClosed() { + ToolExecutionContext.set("c1", "attacker"); + ChatOrigin origin = new ChatOrigin(null, "c1", "someone", 1L, null, + null, null, false, null, " ", null, null, null); + assertThat(svc(new McpIdentityForwardProperties()).classify(ctx(origin))) + .isEqualTo(McpIdentityForwardService.ResolvedIdentity.NONE); + } + + @Test + @DisplayName("unrecognised non-web channel → downgraded to external (never authenticated)") + void novelChannelIsDowngradedNotAuthenticated() { + // A channel the classifier doesn't recognise is treated as external + // (explicit trust downgrade), never as authenticated. The backend sees + // an untrusted id and decides for itself — this is the key guarantee: + // no unknown channel can ever acquire authenticated trust. + ChatOrigin origin = new ChatOrigin(null, "c1", "u1", 1L, null, + null, null, false, null, "future-net", null, null, null); + McpIdentityForwardService.ResolvedIdentity id = + svc(new McpIdentityForwardProperties()).classify(ctx(origin)); + assertThat(id.subject()).isEqualTo("u1"); + assertThat(id.trust()).isEqualTo(McpIdentityForwardService.TRUST_EXTERNAL); + assertThat(id.channelType()).isEqualTo("future-net"); + } } // ==================== Resolution: plaintext & token modes ==================== @@ -170,6 +215,41 @@ class McpIdentityForwardServiceTest { assertThat(svc(p).resolve(ctx(origin), "my-api")).isEmpty(); } + @Test + @DisplayName("signing key self-heals after a bad PEM is corrected (no restart needed)") + void signingKeySelfHealsAfterConfigFix() { + KeyPair kp = rsaKeyPair(); + String goodPem = Base64.getEncoder().encodeToString(kp.getPrivate().getEncoded()); + ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L); + + var p = new McpIdentityForwardProperties(); + p.getToken().setEnabled(true); + p.getToken().setIssuer("mateclaw"); + p.getToken().setTtlSeconds(60); + + // 1. Malformed key → fail-closed. + p.getToken().setPrivateKeyPem("not-a-valid-pem"); + McpIdentityForwardService service = svc(p); + assertThat(service.resolve(ctx(origin), "my-api")).isEmpty(); + + // 2. Operator fixes the config (or a reload pushes a good key) → next + // call re-parses and issues a token, without needing an app restart. + p.getToken().setPrivateKeyPem(goodPem); + Optional inj = service.resolve(ctx(origin), "my-api"); + assertThat(inj).isPresent(); + assertThat(inj.get().key()).isEqualTo(McpIdentityForwardProperties.TOKEN_ARG); + + // The minted token verifies against the matching public key. + Claims claims = Jwts.parser() + .verifyWith(kp.getPublic()) + .requireIssuer("mateclaw") + .requireAudience("my-api") + .build() + .parseSignedClaims(inj.get().value()) + .getPayload(); + assertThat(claims.getSubject()).isEqualTo("42"); + } + @Test @DisplayName("token mode: mints RS256 JWT that verifies with trust/channel_type claims") void tokenMintAndVerify() throws Exception {