From 469ffe2f433bce0cbff6d057d49e644428f93987 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=80=AA=E7=A8=8B=E4=BC=9F?= Date: Sat, 18 Jul 2026 04:54:44 +0800 Subject: [PATCH 1/3] fix(mcp): round-1 adversarial review of #459 identity forwarding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 8 confirmed findings from the first adversarial review pass on the merged #460/#471/#464 implementation: - signingKey rotation: a cached key was served forever once parsed, so a valid→valid PEM rotation never took effect without a restart (contradicted the self-heal Javadoc; dangerous when retiring a suspected-compromised key). Now tracks lastSuccessfulPem and re-parses on PEM change. - requesterUserId drop: the approval-replay and queued-message web-origin sites used overloads that lost the immutable user id, so on-behalf-of sub fell back to the mutable username typed 'authenticated'. Plumbed requesterUserIdOf(auth) into both origins. - doc drift (en+zh): plaintext value is ':' (not 'username'); JWT carries trust + channel_type claims; sub is the immutable numeric id; added an Identity-typing section + webchat/IM caveat; fixed Python examples. - progress-path identity injection (ProgressAwareMcpToolCallback bypasses the normal call()) now has behavioral tests for plaintext/token/no-identity. - NONE-branch coverage: web+no-user+no-ThreadLocal, non-web+blank-requester. - fast-path reuse + valid→valid rotation + 16-thread DCL stress tests. - audienceFor(null,null) now rejects instead of returning a colliding 'null'. The new concurrency test caught a regression in the first rotation draft (fast-path null short-circuit returned empty mid-parse under contention); fixed by moving all null-returning decisions inside the synchronized block. Tests: 216 run, 0 failures (identity-forward subset 23→41). --- .../vip/mate/channel/web/ChatController.java | 31 ++-- .../runtime/McpIdentityForwardProperties.java | 12 ++ .../runtime/McpIdentityForwardService.java | 54 ++++--- .../src/main/resources/docs/en/mcp.md | 86 +++++++++--- .../src/main/resources/docs/zh/mcp.md | 40 +++++- .../IdentityForwardingToolCallbackTest.java | 27 ++++ .../McpClientManagerProgressWrapTest.java | 132 ++++++++++++++++++ .../McpIdentityForwardServiceTest.java | 101 ++++++++++++++ 8 files changed, 430 insertions(+), 53 deletions(-) diff --git a/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java b/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java index 9d5317cc5..da739e783 100644 --- a/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java +++ b/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java @@ -273,7 +273,7 @@ public SseEmitter chatStream( // deny 是正常 turn 终结，用户可能在 awaiting_approval 阶段排了消息 ChatStreamTracker.CompletionResult denyCr = streamTracker.completeAndConsumeIfLast(conversationId); if (denyCr.allDone() && denyCr.queuedInput() != null) { - startQueuedMessage(conversationId, emitter, approvalEmitterDone, denyCr.queuedInput(), username, requestBaseUrl); + startQueuedMessage(conversationId, emitter, approvalEmitterDone, denyCr.queuedInput(), username, requesterUserIdOf(auth), requestBaseUrl); } else { completeEmitterQuietly(emitter, approvalEmitterDone); } @@ -287,7 +287,7 @@ public SseEmitter chatStream( // 审批记录被另一个请求消费，但用户可能在等待期间排了消息 ChatStreamTracker.CompletionResult consumedNullCr = streamTracker.completeAndConsumeIfLast(conversationId); if (consumedNullCr.allDone() && consumedNullCr.queuedInput() != null) { - startQueuedMessage(conversationId, emitter, approvalEmitterDone, consumedNullCr.queuedInput(), username, requestBaseUrl); + startQueuedMessage(conversationId, emitter, approvalEmitterDone, consumedNullCr.queuedInput(), username, requesterUserIdOf(auth), requestBaseUrl); } else { completeEmitterQuietly(emitter, approvalEmitterDone); } @@ -310,8 +310,11 @@ public SseEmitter chatStream( vip.mate.agent.context.ChatOrigin replayOrigin = approvalService.restoreChatOrigin(finalConsumed.getChatOrigin()); if (replayOrigin == vip.mate.agent.context.ChatOrigin.EMPTY) { + // Carry the immutable requesterUserId so on-behalf-of + // identity forwarding (issue #459) asserts the real + // account on replayed tool calls, not the username. replayOrigin = vip.mate.agent.context.ChatOrigin.web( - conversationId, username, workspaceId, null); + conversationId, username, workspaceId, null, null, requesterUserIdOf(auth)); } // Carry the request-thread base URL so any file a replayed // tool generates gets an absolute download link. @@ -392,7 +395,7 @@ public SseEmitter chatStream( ChatStreamTracker.CompletionResult cr = streamTracker.completeAndConsumeIfLast(conversationId); if (cr.allDone()) { if (cr.queuedInput() != null) { - startQueuedMessage(conversationId, emitter, approvalEmitterDone, cr.queuedInput(), username, requestBaseUrl); + startQueuedMessage(conversationId, emitter, approvalEmitterDone, cr.queuedInput(), username, requesterUserIdOf(auth), requestBaseUrl); } else { conversationService.updateStreamStatus(conversationId, "idle"); completeEmitterQuietly(emitter, approvalEmitterDone); @@ -493,7 +496,7 @@ public SseEmitter chatStream( ChatStreamTracker.CompletionResult cr = streamTracker.completeAndConsumeIfLast(conversationId); if (cr.allDone()) { if (cr.queuedInput() != null) { - startQueuedMessage(conversationId, emitter, approvalEmitterDone, cr.queuedInput(), username, requestBaseUrl); + startQueuedMessage(conversationId, emitter, approvalEmitterDone, cr.queuedInput(), username, requesterUserIdOf(auth), requestBaseUrl); } else { conversationService.updateStreamStatus(conversationId, "idle"); completeEmitterQuietly(emitter, approvalEmitterDone); @@ -708,7 +711,7 @@ public SseEmitter chatStream( // genuinely doesn't want continuation, no message would // have been in messageQueue to begin with. if (cr.queuedInput() != null) { - startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), username, requestBaseUrl); + startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), username, requesterUserIdOf(auth), requestBaseUrl); } else { conversationService.updateStreamStatus(conversationId, "idle"); // 延迟关闭 emitter，确保最后的事件都已发送 @@ -802,7 +805,7 @@ public SseEmitter chatStream( if (cr.allDone()) { if (cr.queuedInput() != null) { // 无论中断类型，都消费排队消息（修复 Disposable 不可用时队列被丢弃的 bug） - startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), username, requestBaseUrl); + startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), username, requesterUserIdOf(auth), requestBaseUrl); } else { conversationService.updateStreamStatus(conversationId, "idle"); completeEmitterQuietly(emitter, emitterDone); @@ -925,7 +928,7 @@ public SseEmitter chatStream( // — just run it. Aligns with doOnComplete and the 4 other // queue-launch sites in this controller. if (cr.queuedInput() != null) { - startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), username, requestBaseUrl); + startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), username, requesterUserIdOf(auth), requestBaseUrl); } else { conversationService.updateStreamStatus(conversationId, "idle"); completeEmitterQuietly(emitter, emitterDone); @@ -1349,7 +1352,7 @@ public static class ChatStreamRequest { */ private void startQueuedMessage(String conversationId, SseEmitter emitter, AtomicBoolean emitterDone, ChatStreamTracker.QueuedInput preConsumedInput, String requesterId, - String baseUrl) { + Long requesterUserId, String baseUrl) { if (preConsumedInput == null) { conversationService.updateStreamStatus(conversationId, "idle"); completeEmitterQuietly(emitter, emitterDone); @@ -1408,9 +1411,11 @@ private void startQueuedMessage(String conversationId, SseEmitter emitter, Atomi streamTracker.incrementFlux(conversationId); // RFC-063r §2.5: queued messages land in the same conversation; carry // a web-origin ChatOrigin so any cron job created during the queued - // turn keeps a consistent (null-channel) binding. + // turn keeps a consistent (null-channel) binding. Carry the immutable + // requesterUserId so on-behalf-of identity forwarding (issue #459) still + // asserts the authenticated account, not just the mutable username. vip.mate.agent.context.ChatOrigin queuedOrigin = - vip.mate.agent.context.ChatOrigin.web(conversationId, requesterId, null, null) + vip.mate.agent.context.ChatOrigin.web(conversationId, requesterId, null, null, null, requesterUserId) .withBaseUrl(baseUrl); Disposable disposable = agentService.chatStructuredStream(agentId, queuedMessage, conversationId, requesterId, null, queuedOrigin) .doOnNext(delta -> { @@ -1474,7 +1479,7 @@ private void startQueuedMessage(String conversationId, SseEmitter emitter, Atomi if (cr.allDone()) { if (cr.queuedInput() != null) { // 链式续跑：queued stream 期间又排了新消息 - startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), requesterId, baseUrl); + startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), requesterId, requesterUserId, baseUrl); } else { conversationService.updateStreamStatus(conversationId, "idle"); sseExecutor.execute(() -> { @@ -1519,7 +1524,7 @@ private void startQueuedMessage(String conversationId, SseEmitter emitter, Atomi ChatStreamTracker.CompletionResult cr = streamTracker.completeAndConsumeIfLast(conversationId); if (cr.allDone()) { if (cr.queuedInput() != null) { - startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), requesterId, baseUrl); + startQueuedMessage(conversationId, emitter, emitterDone, cr.queuedInput(), requesterId, requesterUserId, baseUrl); } else { conversationService.updateStreamStatus(conversationId, "idle"); completeEmitterQuietly(emitter, emitterDone); diff --git a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardProperties.java b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardProperties.java index 32979408f..bb0bdd9cb 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardProperties.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/McpIdentityForwardProperties.java @@ -111,8 +111,20 @@ public boolean forwardsTo(Long serverId, String serverName) { * Audience claim for a server's minted tokens: an explicit mapping (by name * or id) when configured, otherwise the server name (or id as string). Lets * the backend reject a token minted for a different server. + * + * @throws IllegalArgumentException when both {@code serverId} and + * {@code serverName} are {@code null} — there is no defensible + * audience for an unidentified server, and silently returning the + * literal string {@code "null"} would let two distinct null/null + * servers share one audience (defeating per-server isolation). + * Callers reachable from the production wrap path always have at + * least one of the two. */ public String audienceFor(Long serverId, String serverName) { + if (serverId == null && (serverName == null || serverName.isBlank())) { + throw new IllegalArgumentException( + "audienceFor requires a serverId or serverName; both were null/blank"); + } Map aud = token.getAudiences(); if (serverName != null && aud.containsKey(serverName)) { return aud.get(serverName); 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 7bf3d8751..a701ceebb 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 @@ -80,6 +80,15 @@ public class McpIdentityForwardService { */ private volatile String lastAttemptedPem; + /** + * PEM content from which {@link #signingKey} was successfully parsed. Lets a + * valid-to-valid rotation take effect without a restart: when the configured + * PEM differs from this, the fast path falls through to a fresh parse and + * swaps in the rotated key (critical when the old key is being retired). + * {@code null} = no successful parse yet. + */ + private volatile String lastSuccessfulPem; + public McpIdentityForwardService(McpIdentityForwardProperties properties) { this.properties = properties; } @@ -206,30 +215,42 @@ private String mint(ResolvedIdentity id, String audience) { /** * 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. + * PEM changes (e.g. an operator fixes a malformed key, rotates a suspected + * compromise, or a config reload pushes a new one) — recovery no longer + * needs an app restart. Two self-heal cases: + *
    + *
  • failed → fixed: a prior parse left {@code signingKey} null; + * the next call re-parses once the PEM differs from the last attempt.
    • + *
  • valid → rotated: {@code signingKey} is already non-null but + * the configured PEM has changed; the new key is parsed and swapped in + * so tokens are re-signed with the rotated key (critical when the old + * key is being rotated out because it may be compromised).
    • + *
+ * Stays fail-closed otherwise: a parse failure leaves {@code signingKey} + * null and the same PEM is not retried on every call. */ private PrivateKey signingKey() { + String pem = properties.getToken().getPrivateKeyPem(); PrivateKey k = signingKey; - if (k != null) { + // Fast path ONLY for the steady state: a cached key parsed from the + // *current* PEM. Any other case (cold start, in-flight parse on another + // thread, rotation, or a prior failed parse) must enter the lock — we + // never return null from the fast path just because another thread set + // lastAttemptedPem, since that thread may still be mid-parse and about + // to publish a good key. + if (k != null && pem != null && pem.equals(lastSuccessfulPem)) { return k; } - 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) { + // Re-check under the lock: another thread may have just parsed the + // same (current) PEM, in which case we reuse its result. + if (signingKey != null && pem != null && pem.equals(lastSuccessfulPem)) { return signingKey; } - // Re-check under the lock: another thread may have just attempted - // the same (unchanged) PEM. - if (lastAttemptedPem != null && lastAttemptedPem.equals(pem)) { + // A prior attempt at this same PEM failed (or is being retried with + // an unchanged value). Don't re-parse on every call — that would + // spam the log and burn CPU. A PEM *change* falls through to parse. + if (signingKey == null && lastAttemptedPem != null && lastAttemptedPem.equals(pem)) { return null; } lastAttemptedPem = pem; @@ -245,6 +266,7 @@ private PrivateKey signingKey() { byte[] der = Base64.getDecoder().decode(body); signingKey = KeyFactory.getInstance("RSA") .generatePrivate(new PKCS8EncodedKeySpec(der)); + lastSuccessfulPem = pem; // remember the PEM this key was parsed from log.info("[McpIdentity] loaded RS256 signing key (kid={})", properties.getToken().getKeyId()); } catch (Exception e) { log.error("[McpIdentity] failed to parse private-key-pem (expect PKCS#8 RSA): {}", e.getMessage()); diff --git a/mateclaw-server/src/main/resources/docs/en/mcp.md b/mateclaw-server/src/main/resources/docs/en/mcp.md index f4c41288d..4b01257fe 100644 --- a/mateclaw-server/src/main/resources/docs/en/mcp.md +++ b/mateclaw-server/src/main/resources/docs/en/mcp.md @@ -390,9 +390,11 @@ user; its environment is fixed at spawn and STDIO has no per-request header channel like HTTP. So per-user identity **cannot travel via env** — it must ride in-band with each tool call. -MateClaw can inject the **authenticated username** into every tool call for a -chosen server, so the server can call its downstream REST backend on behalf of -that user. +MateClaw can inject the **caller's identity** into every tool call for a chosen +server, so the server can call its downstream REST backend on behalf of that +user. The identity is **typed** so the backend can tell "MateClaw authenticated +this user" apart from "this is an external/anonymous identifier" (see +[Identity typing](#identity-typing) below). ### Enable (opt-in, per server) @@ -411,10 +413,21 @@ mateclaw: ### Data contract When enabled, MateClaw injects the reserved argument **`__mateclaw_user__`** -(value = authenticated username) into each tool call's JSON arguments. It is -injected by trusted server code, **never by the LLM** — any model-supplied value -of the same key is overwritten, so the model cannot spoof identity. When there is -no authenticated user, nothing is injected (identity is never fabricated). +into each tool call's JSON arguments. The value is **`:`** — a +trust prefix followed by the subject identifier — so the backend can tell +authenticated users apart from anonymous/external ones without verifying a JWT. +It is injected by trusted server code, **never by the LLM** — any +model-supplied value of the same key is overwritten, so the model cannot spoof +identity. When there is no usable identity (cron / system / unattributed), +nothing is injected (identity is never fabricated). + +The trust prefix is one of: + +| Prefix | Meaning | Example value | +|---|---|---| +| `authenticated` | MateClaw web-console login (JWT/PAT). `sub` = the user's immutable numeric id (or username on legacy paths). | `authenticated:42` | +| `anonymous` | webchat visitor / third-party `endUserId`. **No MateClaw account backs it.** | `anonymous:visitor-xyz` | +| `external` | IM sender (feishu/wecom/…). External platform id. | `external:im_user_1` | The MCP server reads and strips the key, then calls REST with it plus its own backend API key (e.g. an `X-On-Behalf-Of` header): @@ -432,9 +445,14 @@ API_KEY = os.environ["BACKEND_API_KEY"] # service-level key (authenticates def query_orders(keyword: str, __mateclaw_user__: str | None = None) -> str: if not __mateclaw_user__: raise ValueError("missing injected identity") # reject identity-less calls + # __mateclaw_user__ is ":", e.g. "authenticated:42". + # Split off the trust prefix; authorize on-behalf-of only for "authenticated". + trust, _, subject = __mateclaw_user__.partition(":") + if trust != "authenticated": + raise ValueError(f"refusing on-behalf-of for non-authenticated identity: {__mateclaw_user__}") headers = { "Authorization": f"ApiKey {API_KEY}", # service identity - "X-On-Behalf-Of": __mateclaw_user__, # the acting user + "X-On-Behalf-Of": subject, # the acting user } r = httpx.get(f"{REST_BASE}/orders", params={"q": keyword}, headers=headers, timeout=30) r.raise_for_status() @@ -448,6 +466,29 @@ if __name__ == "__main__": > `__mateclaw_user__` as an optional parameter (as above) or strict validation > will reject it. +### Identity typing + +Not every requester is a MateClaw-authenticated account. The injected identity is +typed so the REST backend can decide how much to trust the on-behalf-of call: + +- **`authenticated`** — web-console login (JWT/PAT). MateClaw vouches for this + user. `sub` is the user's immutable numeric id (`ChatOrigin.requesterUserId`) + on the standard path, falling back to the username on legacy ThreadLocal-only + paths. The backend may authorize on-behalf-of freely. +- **`anonymous`** — webchat visitor / third-party single-account `endUserId`. No + MateClaw account backs it; `sub` is the visitor id. **The backend must treat + this as unauthenticated** and decide for itself whether/how to serve — do not + map `sub` onto a MateClaw account, and do not grant authenticated-user rights. +- **`external`** — IM sender (feishu/wecom/…). `sub` is the platform sender id; + same caveat as anonymous. +- _(nothing injected)_ — cron / system / unattributed origin. MateClaw never + asserts identity on behalf of a non-user (fail-closed). + +This typing matters most in the **signed token** model below: a verified +signature proves the token was minted by MateClaw, **not** that the `sub` is an +authenticated account. Always check the `trust` claim (or the plaintext prefix) +before authorizing. + ### Two trust models **① Plaintext (default)**: injects the plaintext username. Fits a trusted @@ -484,15 +525,20 @@ openssl pkey -in mcp-idfwd-private.pem -pubout -out mcp-idfwd-public.pem # private-key-pem takes the private key body (PEM headers optional; stripped on parse) ``` -Token claims: `iss`, `sub`=user, `aud`=this server, `iat`, `exp` (short), `jti`. -`aud` + short `exp` bound replay to tens of seconds and to one backend. **When -token mode is on but no key is configured, it fails closed** (no token minted, -nothing injected — the backend rejects) rather than silently downgrading to -plaintext. - -> `sub` carries the MateClaw user identifier (`ChatOrigin.requesterId`). If your -> backend authorizes on an immutable numeric id, resolve username→id before -> minting (kept decoupled from the user store here). +Token claims: `iss`, `sub`=subject, `aud`=this server, `iat`, `exp` (short), +`jti`, plus two typing claims — **`trust`** (`authenticated` / `anonymous` / +`external`) and **`channel_type`** (`web` / `api` / `feishu` / …). `aud` + short +`exp` bound replay to tens of seconds and to one backend. **When token mode is +on but no key is configured, it fails closed** (no token minted, nothing injected +— the backend rejects) rather than silently downgrading to plaintext. + +> **`sub` semantics.** On the standard authenticated-web path `sub` is the user's +> **immutable numeric id** (`ChatOrigin.requesterUserId`); on legacy +> ThreadLocal-only paths it falls back to the username. No backend-side +> username→id resolution is needed. Always also read the `trust` claim before +> authorizing — a verified signature only proves MateClaw minted the token, not +> that `sub` is an authenticated account (an anonymous visitor token also +> verifies). The MCP server (Python) only forwards — it does not verify: @@ -511,7 +557,11 @@ REST backend verifies (pseudocode): import jwt # PyJWT claims = jwt.decode(token, public_key_pem, algorithms=["RS256"], issuer="mateclaw", audience="https://api.internal") -user = claims["sub"] # trusted only after signature verification +# A valid signature only proves MateClaw minted the token — it does NOT mean +# `sub` is an authenticated account. Check the trust claim before authorizing. +if claims.get("trust") != "authenticated": + raise PermissionError(f"refuse on-behalf-of for {claims.get('trust')} identity") +user = claims["sub"] # immutable numeric id (authenticated path) # → per-user authorization; invalid/expired → 401 ``` diff --git a/mateclaw-server/src/main/resources/docs/zh/mcp.md b/mateclaw-server/src/main/resources/docs/zh/mcp.md index b0500f830..5cd8b902c 100644 --- a/mateclaw-server/src/main/resources/docs/zh/mcp.md +++ b/mateclaw-server/src/main/resources/docs/zh/mcp.md @@ -383,7 +383,7 @@ API 响应里 `headers_json` 和 `env_json` 的值自动**脱敏**。`args_json` STDIO MCP server 是**每个配置一个共享子进程**，所有用户共用；env 在子进程启动时一次性注入、之后不可变，STDIO 也没有 HTTP 那种 per-request header 通道。所以**不能用 env 传 per-user 身份**——身份必须随每次工具调用在带内传递。 -MateClaw 支持把**认证用户名**注入到每次工具调用的参数里，让 MCP server 代表该用户调用底层 REST 后端。 +MateClaw 支持把**调用者身份**注入到每次工具调用的参数里，让 MCP server 代表该用户调用底层 REST 后端。注入的身份是**带类型**的，后端可据此区分"MateClaw 已认证该用户"和"这是外部/匿名标识"（见下方[身份类型](#身份类型)）。 ### 开启（opt-in，按 server） @@ -400,7 +400,15 @@ mateclaw: ### 数据契约 -开启后，MateClaw 在调用该 server 的每个工具时，往参数 JSON 里注入保留字段 **`__mateclaw_user__`**（值=认证用户名）。该值由受信服务端注入、**不经 LLM**；若 LLM 伪造了同名字段会被覆盖，因此模型无法冒充身份。无认证用户时不注入（不伪造身份）。 +开启后，MateClaw 在调用该 server 的每个工具时，往参数 JSON 里注入保留字段 **`__mateclaw_user__`**。值为 **`:`**——信任前缀加主体标识——这样后端无需验签 JWT 就能区分认证用户与匿名/外部身份。该值由受信服务端注入、**不经 LLM**；若 LLM 伪造了同名字段会被覆盖，因此模型无法冒充身份。无可用身份（cron / system / 无归属）时不注入（不伪造身份）。 + +信任前缀取值： + +| 前缀 | 含义 | 示例值 | +|---|---|---| +| `authenticated` | MateClaw web 控制台登录（JWT/PAT）。`sub` = 用户不可变数字 id（旧路径回退为用户名）。 | `authenticated:42` | +| `anonymous` | webchat 访客 / 第三方 `endUserId`。**背后无 MateClaw 账号。** | `anonymous:visitor-xyz` | +| `external` | IM 发送者（飞书/企微…）。外部平台 id。 | `external:im_user_1` | MCP server 侧读出该字段、剥掉，再连同自己持有的后端 API Key 一起调 REST（如 `X-On-Behalf-Of` header）： @@ -417,9 +425,14 @@ API_KEY = os.environ["BACKEND_API_KEY"] # 服务级 API Key（认证 MCP def query_orders(keyword: str, __mateclaw_user__: str | None = None) -> str: if not __mateclaw_user__: raise ValueError("missing injected identity") # 拒绝无身份调用 + # __mateclaw_user__ 是 ":"，例如 "authenticated:42"。 + # 拆出信任前缀；仅对 "authenticated" 放行 on-behalf-of。 + trust, _, subject = __mateclaw_user__.partition(":") + if trust != "authenticated": + raise ValueError(f"refusing on-behalf-of for non-authenticated identity: {__mateclaw_user__}") headers = { "Authorization": f"ApiKey {API_KEY}", # 服务身份 - "X-On-Behalf-Of": __mateclaw_user__, # 代表的用户 + "X-On-Behalf-Of": subject, # 代表的用户 } r = httpx.get(f"{REST_BASE}/orders", params={"q": keyword}, headers=headers, timeout=30) r.raise_for_status() @@ -431,6 +444,17 @@ if __name__ == "__main__": > 工具的入参 schema 若是 `additionalProperties: false`，记得像上面那样把 `__mateclaw_user__` 声明为可选参数，否则严格校验会拒绝。 +### 身份类型 + +并非每个调用者都是 MateClaw 已认证账号。注入的身份带类型，让 REST 后端能判断这次 on-behalf-of 调用该信多少： + +- **`authenticated`**——web 控制台登录（JWT/PAT）。MateClaw 为该用户背书。标准路径上 `sub` 是用户**不可变数字 id**（`ChatOrigin.requesterUserId`），仅旧 ThreadLocal 路径回退为用户名。后端可放心按该用户做 on-behalf-of 授权。 +- **`anonymous`**——webchat 访客 / 第三方单账号 `endUserId`。背后无 MateClaw 账号，`sub` 是访客 id。**后端必须视作未认证**，自行决定是否/如何服务——不要把 `sub` 映射到 MateClaw 账号，也不要授予认证用户权限。 +- **`external`**——IM 发送者（飞书/企微…）。`sub` 是平台发送者 id；同 anonymous 的注意事项。 +- _（不注入）_——cron / system / 无归属来源。MateClaw 从不为非用户断言身份（fail-closed）。 + +这一类型划分在下面的**签名 token**模型里尤为关键：验签通过只证明 token 由 MateClaw 签发，**并不**代表 `sub` 是认证账号。授权前务必看 `trust` claim（或明文前缀）。 + ### 两种信任模型 **① 明文（默认）**：注入明文用户名。适合 REST 在内网、且后端用 API Key 认证 MCP 服务、把转发用户当 on-behalf-of 的场景。后端裸信这个字符串。 @@ -461,9 +485,9 @@ openssl pkey -in mcp-idfwd-private.pem -pubout -out mcp-idfwd-public.pem # private-key-pem 用私钥内容（带不带 PEM 头都行，解析时会剥掉） ``` -token 的 claims：`iss`、`sub`=用户、`aud`=该 server、`iat`、`exp`（短）、`jti`。`aud`+短 `exp` 把重放限制在几十秒内、且只对这一个后端。**token 模式开启但没配私钥时 fail-closed**（不签、不注入，后端自然拒绝），不会偷偷退回明文。 +token 的 claims：`iss`、`sub`=主体、`aud`=该 server、`iat`、`exp`（短）、`jti`，外加两个类型 claim——**`trust`**（`authenticated` / `anonymous` / `external`）和 **`channel_type`**（`web` / `api` / `feishu` / …）。`aud`+短 `exp` 把重放限制在几十秒内、且只对这一个后端。**token 模式开启但没配私钥时 fail-closed**（不签、不注入，后端自然拒绝），不会偷偷退回明文。 -> `sub` 携带的是 MateClaw 用户标识（`ChatOrigin.requesterId`）。若后端按不可变数字 id 鉴权，可在签发前把用户名解析成 id（本层刻意不耦合用户存储）。 +> **`sub` 语义。** 标准认证 web 路径上 `sub` 是用户的**不可变数字 id**（`ChatOrigin.requesterUserId`）；仅旧 ThreadLocal 路径回退为用户名。后端无需再做用户名→id 解析。授权前务必同时看 `trust` claim——验签通过只证明 token 由 MateClaw 签发，不代表 `sub` 是认证账号（匿名访客的 token 同样能验签通过）。 MCP server（Python）只透传、不验签： @@ -482,7 +506,11 @@ REST 后端验签（伪代码）： import jwt # PyJWT claims = jwt.decode(token, public_key_pem, algorithms=["RS256"], issuer="mateclaw", audience="https://api.internal") -user = claims["sub"] # 验签通过才相信 +# 验签通过只证明 token 由 MateClaw 签发——并不代表 sub 是认证账号。 +# 授权前务必看 trust claim。 +if claims.get("trust") != "authenticated": + raise PermissionError(f"refuse on-behalf-of for {claims.get('trust')} identity") +user = claims["sub"] # 不可变数字 id（认证路径） # → 按 user 做 per-user 授权；验签失败/过期 → 401 ``` diff --git a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java index 2a053f042..7fdc5854e 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java @@ -69,4 +69,31 @@ void optInMatching() { p.setServers(Set.of("42")); assertThat(p.forwardsTo(42L, "other")).isTrue(); // by id } + + @Test + @DisplayName("forwardsTo: null id and null name never forward (no NPE)") + void forwardsToNullSafe() { + McpIdentityForwardProperties p = new McpIdentityForwardProperties(); + p.setServers(Set.of("svc")); + assertThat(p.forwardsTo(null, null)).isFalse(); + assertThat(p.forwardsTo(null, "svc")).isTrue(); + p.setServers(Set.of("42")); + assertThat(p.forwardsTo(42L, null)).isTrue(); + } + + @Test + @DisplayName("audienceFor(null, null) is rejected — no colliding \"null\" audience") + void audienceForRejectsBothNull() { + // Two distinct unidentified servers must NOT share the literal "null" + // audience — that would defeat per-server audience isolation. Require an + // id or name instead of silently returning a colliding sentinel. + McpIdentityForwardProperties p = new McpIdentityForwardProperties(); + org.assertj.core.api.Assertions.assertThatThrownBy(() -> p.audienceFor(null, null)) + .isInstanceOf(IllegalArgumentException.class); + org.assertj.core.api.Assertions.assertThatThrownBy(() -> p.audienceFor(null, " ")) + .isInstanceOf(IllegalArgumentException.class); + // But a single null is fine — the other identifies the server. + assertThat(p.audienceFor(null, "svc")).isEqualTo("svc"); + assertThat(p.audienceFor(42L, null)).isEqualTo("42"); + } } diff --git a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java index 494a24b64..5738c2077 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java @@ -2,6 +2,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import io.modelcontextprotocol.client.McpSyncClient; +import io.modelcontextprotocol.spec.McpSchema; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.springframework.ai.tool.ToolCallback; @@ -9,11 +10,15 @@ import org.springframework.ai.tool.definition.ToolDefinition; import org.springframework.ai.tool.metadata.ToolMetadata; import org.springframework.ai.chat.model.ToolContext; +import vip.mate.agent.context.ChatOrigin; import java.util.List; +import java.util.Map; import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; /** * Black-box regression suite verifying that the new @@ -107,4 +112,131 @@ void twoArgOverloadStillWorks() { assertEquals(1, wrapped.size()); assertInstanceOf(PrefixedNameToolCallback.class, wrapped.get(0)); } + + // ── Progress path end-to-end: identity MUST reach the CallToolRequest ── + // Regression for the security-critical branch where ProgressAwareMcpToolCallback + // calls mcpClient.callTool directly (bypassing IdentityForwardingToolCallback.call). + // If the chain shape ever changes so identity injection is skipped here, every + // long-running MCP tool call on an opt-in server would lose on-behalf-of identity. + + @Test + @DisplayName("progress path injects __mateclaw_user__ into the outgoing CallToolRequest") + void progressPathInjectsPlaintextIdentity() throws Exception { + McpSyncClient client = mock(McpSyncClient.class); + // A minimal non-null CallToolResult so the wrapper's serializeResult works. + org.mockito.Mockito.when(client.callTool(any())) + .thenReturn(new McpSchema.CallToolResult(List.of(), false)); + ToolCallback cb = stub("long_task"); + ObjectMapper mapper = new ObjectMapper(); + + // Real identity service in plaintext mode, opted in for this server. + McpIdentityForwardProperties props = new McpIdentityForwardProperties(); + props.setServers(java.util.Set.of("my-server")); + McpIdentityForwardService idSvc = new McpIdentityForwardService(props); + + List wrapped = McpClientManager.wrapServerCallbacks(77L, + new ToolCallback[]{cb}, idSvc, "my-server", "my-server", client, mapper); + + assertEquals(1, wrapped.size()); + // Authenticated web user → the request must carry __mateclaw_user__ = "authenticated:42". + ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L); + ToolContext toolCtx = new ToolContext(Map.of( + ChatOrigin.CTX_KEY, origin, + ProgressAwareMcpToolCallback.MCP_PROGRESS_TOKEN_KEY, "tok-1")); + + wrapped.get(0).call("{\"q\":\"hi\"}", toolCtx); + + // Capture the CallToolRequest handed to the client and assert the identity arg. + org.mockito.ArgumentCaptor captor = + org.mockito.ArgumentCaptor.forClass(McpSchema.CallToolRequest.class); + verify(client).callTool(captor.capture()); + McpSchema.CallToolRequest req = captor.getValue(); + Object injected = req.arguments() != null ? req.arguments().get(McpIdentityForwardProperties.USER_ARG) : null; + assertNotNull(injected, "progress path must inject __mateclaw_user__"); + assertEquals("authenticated:42", injected); + // And the LLM-supplied args survive (only the reserved key is added). + assertEquals("hi", req.arguments().get("q")); + // The progress token is also present. + assertEquals("tok-1", req.meta().get("progressToken")); + } + + @Test + @DisplayName("progress path injects __mateclaw_token__ in token mode") + void progressPathInjectsTokenIdentity() throws Exception { + java.security.KeyPair kp; + try { + kp = java.security.KeyPairGenerator.getInstance("RSA").genKeyPair(); + } catch (Exception e) { + throw new IllegalStateException(e); + } + McpSyncClient client = mock(McpSyncClient.class); + org.mockito.Mockito.when(client.callTool(any())) + .thenReturn(new McpSchema.CallToolResult(List.of(), false)); + ToolCallback cb = stub("long_task"); + ObjectMapper mapper = new ObjectMapper(); + + McpIdentityForwardProperties props = new McpIdentityForwardProperties(); + props.setServers(java.util.Set.of("my-server")); + props.getToken().setEnabled(true); + props.getToken().setIssuer("mateclaw"); + props.getToken().setTtlSeconds(60); + props.getToken().setPrivateKeyPem( + java.util.Base64.getEncoder().encodeToString(kp.getPrivate().getEncoded())); + McpIdentityForwardService idSvc = new McpIdentityForwardService(props); + + List wrapped = McpClientManager.wrapServerCallbacks(77L, + new ToolCallback[]{cb}, idSvc, "my-server", "my-server", client, mapper); + + ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L); + ToolContext toolCtx = new ToolContext(Map.of( + ChatOrigin.CTX_KEY, origin, + ProgressAwareMcpToolCallback.MCP_PROGRESS_TOKEN_KEY, "tok-2")); + + wrapped.get(0).call("{\"q\":\"hi\"}", toolCtx); + + org.mockito.ArgumentCaptor captor = + org.mockito.ArgumentCaptor.forClass(McpSchema.CallToolRequest.class); + verify(client).callTool(captor.capture()); + Object token = captor.getValue().arguments().get(McpIdentityForwardProperties.TOKEN_ARG); + assertNotNull(token, "progress path must inject __mateclaw_token__ in token mode"); + // Token verifies with the public key and carries the typed claims. + io.jsonwebtoken.Claims claims = io.jsonwebtoken.Jwts.parser() + .verifyWith(kp.getPublic()).requireAudience("my-server").build() + .parseSignedClaims((String) token).getPayload(); + assertEquals("42", claims.getSubject()); + assertEquals(McpIdentityForwardService.TRUST_AUTHENTICATED, claims.get("trust", String.class)); + } + + @Test + @DisplayName("progress path with no identity (cron) injects nothing, still calls with progress token") + void progressPathNoIdentityInjectsNothing() throws Exception { + McpSyncClient client = mock(McpSyncClient.class); + org.mockito.Mockito.when(client.callTool(any())) + .thenReturn(new McpSchema.CallToolResult(List.of(), false)); + ToolCallback cb = stub("long_task"); + ObjectMapper mapper = new ObjectMapper(); + + McpIdentityForwardProperties props = new McpIdentityForwardProperties(); + props.setServers(java.util.Set.of("my-server")); + McpIdentityForwardService idSvc = new McpIdentityForwardService(props); + + List wrapped = McpClientManager.wrapServerCallbacks(77L, + new ToolCallback[]{cb}, idSvc, "my-server", "my-server", client, mapper); + + // Cron origin → no identity; progress token still rides along. + ChatOrigin origin = ChatOrigin.cron("c1", 1L, null, 9L, null); + ToolContext toolCtx = new ToolContext(Map.of( + ChatOrigin.CTX_KEY, origin, + ProgressAwareMcpToolCallback.MCP_PROGRESS_TOKEN_KEY, "tok-3")); + + wrapped.get(0).call("{\"q\":\"hi\"}", toolCtx); + + org.mockito.ArgumentCaptor captor = + org.mockito.ArgumentCaptor.forClass(McpSchema.CallToolRequest.class); + verify(client).callTool(captor.capture()); + McpSchema.CallToolRequest req = captor.getValue(); + assertFalse(req.arguments().containsKey(McpIdentityForwardProperties.USER_ARG), + "no identity arg when origin is cron"); + assertEquals("tok-3", req.meta().get("progressToken")); + } } 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 8aea890f1..6ca4bf7aa 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 @@ -16,6 +16,9 @@ import java.util.Date; import java.util.Map; import java.util.Optional; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; import static org.assertj.core.api.Assertions.assertThat; @@ -159,6 +162,29 @@ void blankChannelIsFailClosed() { .isEqualTo(McpIdentityForwardService.ResolvedIdentity.NONE); } + // ---- Branch coverage for the NONE returns inside classify() ---- + + @Test + @DisplayName("web channel, no requesterUserId, no ThreadLocal username → NONE") + void webChannelNoUserNoThreadLocal() { + // Do NOT seed the ThreadLocal — mirrors a thread that has never run a + // tool-execution executor pass (or just cleared it). The web branch's + // username fallback must then return NONE rather than inject garbage. + ChatOrigin origin = ChatOrigin.web("c1", "", 1L, null, null); + assertThat(svc(new McpIdentityForwardProperties()).classify(ctx(origin))) + .isEqualTo(McpIdentityForwardService.ResolvedIdentity.NONE); + } + + @Test + @DisplayName("non-web channel with blank requesterId → NONE") + void nonWebChannelBlankRequester() { + // A recognised channelType but no requester id: nothing to forward. + ChatOrigin origin = new ChatOrigin(null, "c1", "", 1L, null, + null, null, false, null, "feishu", 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() { @@ -250,6 +276,81 @@ void signingKeySelfHealsAfterConfigFix() { assertThat(claims.getSubject()).isEqualTo("42"); } + @Test + @DisplayName("signing key rotates when a valid PEM is replaced by another valid PEM (no restart)") + void signingKeyRotatesOnValidPemChange() { + // Regression for the rotation gap: once a key is cached, a subsequent + // valid PEM change MUST take effect without an app restart — otherwise a + // key retired on suspicion of compromise keeps signing tokens forever. + KeyPair kp1 = rsaKeyPair(); + KeyPair kp2 = rsaKeyPair(); + String pem1 = Base64.getEncoder().encodeToString(kp1.getPrivate().getEncoded()); + String pem2 = Base64.getEncoder().encodeToString(kp2.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); + p.getToken().setPrivateKeyPem(pem1); + McpIdentityForwardService service = svc(p); + + // 1. First token is signed with key #1 and verifies with pubkey #1. + String tok1 = service.resolve(ctx(origin), "my-api").orElseThrow().value(); + Jwts.parser().verifyWith(kp1.getPublic()).requireAudience("my-api").build() + .parseSignedClaims(tok1).getPayload(); + + // 2. Rotate to key #2. The next token MUST verify with pubkey #2 (not #1). + p.getToken().setPrivateKeyPem(pem2); + String tok2 = service.resolve(ctx(origin), "my-api").orElseThrow().value(); + Jwts.parser().verifyWith(kp2.getPublic()).requireAudience("my-api").build() + .parseSignedClaims(tok2).getPayload(); + // And the rotated token must NOT verify with the OLD pubkey anymore. + org.assertj.core.api.Assertions.assertThatThrownBy(() -> + Jwts.parser().verifyWith(kp1.getPublic()).requireAudience("my-api").build() + .parseSignedClaims(tok2).getPayload()); + } + + @Test + @DisplayName("signing-key parse is single-flight under concurrency (all tokens verify)") + void signingKeyParseIsSingleFlightConcurrently() throws Exception { + // Stress the double-checked locking: N threads hit a cold service at once. + // Every returned token must verify against the single public key. + KeyPair kp = rsaKeyPair(); + var p = new McpIdentityForwardProperties(); + p.getToken().setEnabled(true); + p.getToken().setIssuer("mateclaw"); + p.getToken().setTtlSeconds(60); + p.getToken().setPrivateKeyPem(Base64.getEncoder().encodeToString(kp.getPrivate().getEncoded())); + McpIdentityForwardService service = svc(p); + + int n = 16; + // Use Callables so a thread that loses the race still returns its result; + // no shared mutable array, no early-return-drops-slot hazard. + java.util.List> tasks = new java.util.ArrayList<>(n); + for (int i = 0; i < n; i++) { + tasks.add(() -> { + ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L); + return service.resolve(ctx(origin), "my-api").orElseThrow().value(); + }); + } + ExecutorService pool = Executors.newFixedThreadPool(n); + try { + java.util.List> futures = pool.invokeAll(tasks); + pool.shutdown(); + org.assertj.core.api.Assertions.assertThat(pool.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); + + for (java.util.concurrent.Future f : futures) { + String t = f.get(); // propagates any execution exception + assertThat(t).isNotBlank(); + Jwts.parser().verifyWith(kp.getPublic()).requireAudience("my-api").build() + .parseSignedClaims(t).getPayload(); + } + } finally { + pool.shutdownNow(); + } + } + @Test @DisplayName("token mode: mints RS256 JWT that verifies with trust/channel_type claims") void tokenMintAndVerify() throws Exception { From db873f2ae391915fae8f331553d6ed75fe169b95 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=80=AA=E7=A8=8B=E4=BC=9F?= Date: Sat, 18 Jul 2026 05:16:50 +0800 Subject: [PATCH 2/3] fix(mcp): round-2 adversarial review of #459 identity forwarding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 14 confirmed findings from the second adversarial pass (robustness + test-quality + integration lenses) on the post-R1 code: signingKey() state machine (R2-1 HIGH, R2-2 MEDIUM): - valid→invalid PEM rotation re-parsed + re-logged the ERROR on every call while silently keeping the stale old key (CPU/log spam, no operator signal). - null/empty PEM logged the ERROR on every call (dedup couldn't match null). Both fixed: dedup now covers null-key AND stale-key cases via Objects.equals(lastAttemptedPem, pem) + an attemptedPem flag distinguishing first-ever-call from already-tried-null. The bad/empty PEM is parsed/logged exactly once; a prior good key keeps serving (fail-soft). Javadoc accuracy (I-1 MEDIUM): - signingKey() claimed self-heal on 'config reload / no restart', but the properties bean is plain @ConfigurationProperties (not @RefreshScope, no spring-cloud) so a static application.yml edit does NOT take effect without a restart. Rewrote the Javadoc to state the truth. Test hardening (TQ-1/3/4/5 HIGH, TQ-6/7/8 MEDIUM, TQ-9/10/13 LOW, R2-3 LOW): - concurrency test now asserts the key is parsed EXACTLY once (single-flight) via a Logback ListAppender, not just 'all tokens verify'. - non-string reserved-key spoof (number/object/array) overwrite covered. - call(String) with no ToolContext proven fail-closed (no identity injected). - jti uniqueness across mints asserted (replay distinguishability). - JWT exp bounded to ttlSeconds window, not just 'after now'. - self-heal test now proves the bad-PEM failure is cached (not retried/cleared). - audienceFor by numeric id + name-precedence covered. - forwardsTo OR-semantics (id OR name) covered across all 4 combinations. - progress-path tests tightened to any(CallToolRequest.class). - colon-bearing visitorId plaintext round-trip pinned. - descriptive .as() messages on load-bearing assertions. Tests: 204 run, 0 failures (identity-forward subset 41→47). --- .../runtime/McpIdentityForwardService.java | 50 +++-- .../IdentityForwardingToolCallbackTest.java | 60 ++++++ .../McpClientManagerProgressWrapTest.java | 6 +- .../McpIdentityForwardServiceTest.java | 176 +++++++++++++++--- 4 files changed, 252 insertions(+), 40 deletions(-) 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 a701ceebb..76e079b71 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 @@ -76,10 +76,15 @@ public class McpIdentityForwardService { * 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. + * sticking with a permanent null. May itself be {@code null} (when the + * configured PEM is empty), so {@link #attemptedPem} distinguishes "never + * attempted" from "attempted a null/empty PEM". */ private volatile String lastAttemptedPem; + /** {@code true} once any PEM value (incl. null/empty) has been attempted. */ + private volatile boolean attemptedPem; + /** * PEM content from which {@link #signingKey} was successfully parsed. Lets a * valid-to-valid rotation take effect without a restart: when the configured @@ -214,10 +219,13 @@ private String mint(ResolvedIdentity id, String audience) { } /** - * Resolve the signing key, parsing it lazily. Self-heals when the configured - * PEM changes (e.g. an operator fixes a malformed key, rotates a suspected - * compromise, or a config reload pushes a new one) — recovery no longer - * needs an app restart. Two self-heal cases: + * Resolve the signing key, parsing it lazily. Picks up a changed + * {@code private-key-pem} on the next call without a JVM restart, but only + * if the properties bean is actually re-bound at runtime — the default + * {@code @ConfigurationProperties} binding reads {@code application.yml} + * once at startup, so in the stock deployment a static config edit still + * requires a restart. Self-heal applies when properties are mutated + * programmatically or a refresh-scope bean is re-bound: *
    *
  • failed → fixed: a prior parse left {@code signingKey} null; * the next call re-parses once the PEM differs from the last attempt.
    • @@ -225,9 +233,12 @@ private String mint(ResolvedIdentity id, String audience) { * the configured PEM has changed; the new key is parsed and swapped in * so tokens are re-signed with the rotated key (critical when the old * key is being rotated out because it may be compromised). + *
  • valid → invalid: the new PEM fails to parse; the stale key + * keeps serving (best-effort availability) and the bad PEM is NOT + * re-parsed/logged on every subsequent call.
    • *
- * Stays fail-closed otherwise: a parse failure leaves {@code signingKey} - * null and the same PEM is not retried on every call. + * Stays fail-closed otherwise: a parse failure with no prior key leaves + * {@code signingKey} null and the same PEM is not retried on every call. */ private PrivateKey signingKey() { String pem = properties.getToken().getPrivateKeyPem(); @@ -247,12 +258,18 @@ private PrivateKey signingKey() { if (signingKey != null && pem != null && pem.equals(lastSuccessfulPem)) { return signingKey; } - // A prior attempt at this same PEM failed (or is being retried with - // an unchanged value). Don't re-parse on every call — that would - // spam the log and burn CPU. A PEM *change* falls through to parse. - if (signingKey == null && lastAttemptedPem != null && lastAttemptedPem.equals(pem)) { - return null; + // Dedup EVERY repeated attempt of the *same* PEM value — whether the + // prior attempt left signingKey null (failed/empty PEM) or left a + // stale key from an older PEM (valid→invalid rotation). Re-parsing + // the identical broken/unchanged PEM on every call would spam the + // log and burn CPU with no benefit. Uses Objects.equals so a null + // configured PEM dedups against a null lastAttemptedPem; the + // attemptedPem flag distinguishes "first ever call" (must log once) + // from "already tried this null PEM" (skip). + if (attemptedPem && java.util.Objects.equals(lastAttemptedPem, pem)) { + return signingKey; // null if never parsed, or the stale key if rotated-to-invalid } + attemptedPem = true; lastAttemptedPem = pem; if (pem == null || pem.isBlank()) { log.error("[McpIdentity] token mode enabled but mateclaw.mcp.identity-forward.token.private-key-pem is empty; " @@ -264,11 +281,16 @@ private PrivateKey signingKey() { .replaceAll("-----END (.*)-----", "") .replaceAll("\\s", ""); byte[] der = Base64.getDecoder().decode(body); - signingKey = KeyFactory.getInstance("RSA") + PrivateKey parsed = KeyFactory.getInstance("RSA") .generatePrivate(new PKCS8EncodedKeySpec(der)); - lastSuccessfulPem = pem; // remember the PEM this key was parsed from + signingKey = parsed; // publish only on success + lastSuccessfulPem = pem; // remember the PEM this key was parsed from log.info("[McpIdentity] loaded RS256 signing key (kid={})", properties.getToken().getKeyId()); } catch (Exception e) { + // Parse failed: leave signingKey/lastSuccessfulPem unchanged. + // If a prior good key exists it keeps serving (best-effort + // availability); if not, signingKey stays null (fail-closed). + // Either way the dedup above prevents re-parsing this same PEM. log.error("[McpIdentity] failed to parse private-key-pem (expect PKCS#8 RSA): {}", e.getMessage()); } return signingKey; diff --git a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java index 7fdc5854e..ee1aec7f5 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java @@ -36,6 +36,25 @@ void overwritesLlmSuppliedValue() throws Exception { assertThat(MAPPER.readTree(out).get(KEY).asText()).isEqualTo("alice"); } + @Test + @DisplayName("overwrites a NON-STRING LLM value of the reserved key (number / object / array)") + void overwritesNonStringLlmValue() throws Exception { + // The model may try to spoof identity with a non-scalar value; node.put + // replaces any prior value type with the trusted string unconditionally. + for (String adversarial : new String[]{ + "{\"" + KEY + "\":123}", + "{\"" + KEY + "\":{\"admin\":true}}", + "{\"" + KEY + "\":[\"a\",\"b\"]}", + "{\"" + KEY + "\":null}"}) { + String out = IdentityForwardingToolCallback.withClaim(adversarial, KEY, "alice"); + JsonNode node = MAPPER.readTree(out); + assertThat(node.get(KEY).isTextual()) + .as("reserved key must be coerced to the trusted string for input: %s", adversarial) + .isTrue(); + assertThat(node.get(KEY).asText()).isEqualTo("alice"); + } + } + @Test @DisplayName("blank/empty input becomes a fresh object carrying the claim") void emptyInputGetsObject() throws Exception { @@ -70,6 +89,47 @@ void optInMatching() { assertThat(p.forwardsTo(42L, "other")).isTrue(); // by id } + @Test + @DisplayName("opt-in matching: OR semantics — either id OR name match forwards") + void forwardsToOrSemantics() { + // A server listed under BOTH its name and id must forward; a regression + // that required BOTH to match would silently disable opt-in here. + McpIdentityForwardProperties p = new McpIdentityForwardProperties(); + p.setServers(Set.of("svc", "42")); + assertThat(p.forwardsTo(42L, "svc")).isTrue(); // both present + assertThat(p.forwardsTo(42L, "other")).isTrue(); // id matches, name doesn't + assertThat(p.forwardsTo(7L, "svc")).isTrue(); // name matches, id doesn't + assertThat(p.forwardsTo(7L, "other")).isFalse(); // neither + } + + @Test + @DisplayName("call(toolInput) with NO ToolContext injects nothing (fail-closed)") + void callWithoutToolContextInjectsNothing() throws Exception { + // The single-arg call() has no ToolContext → classify() returns NONE → + // the input must pass through to the delegate UNMODIFIED. Guards against + // a future refactor that falls back to a stale ThreadLocal username. + java.util.concurrent.atomic.AtomicReference captured = new java.util.concurrent.atomic.AtomicReference<>(); + org.springframework.ai.tool.ToolCallback delegate = new org.springframework.ai.tool.ToolCallback() { + private final org.springframework.ai.tool.definition.ToolDefinition def = + org.springframework.ai.tool.definition.DefaultToolDefinition.builder() + .name("t").description("").inputSchema("{}").build(); + @Override public org.springframework.ai.tool.definition.ToolDefinition getToolDefinition() { return def; } + @Override public String call(String toolInput) { captured.set(toolInput); return "ok"; } + }; + McpIdentityForwardProperties props = new McpIdentityForwardProperties(); + props.setServers(Set.of("svc")); + McpIdentityForwardService idSvc = new McpIdentityForwardService(props); + IdentityForwardingToolCallback cb = new IdentityForwardingToolCallback(delegate, idSvc, "svc"); + + cb.call("{\"q\":\"hi\"}"); + + JsonNode node = MAPPER.readTree(captured.get()); + assertThat(node.has(McpIdentityForwardProperties.USER_ARG)) + .as("no ToolContext ⇒ no identity injected").isFalse(); + assertThat(node.has(McpIdentityForwardProperties.TOKEN_ARG)).isFalse(); + assertThat(node.get("q").asText()).isEqualTo("hi"); + } + @Test @DisplayName("forwardsTo: null id and null name never forward (no NPE)") void forwardsToNullSafe() { diff --git a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java index 5738c2077..ae1b9a97c 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/McpClientManagerProgressWrapTest.java @@ -124,7 +124,7 @@ void twoArgOverloadStillWorks() { void progressPathInjectsPlaintextIdentity() throws Exception { McpSyncClient client = mock(McpSyncClient.class); // A minimal non-null CallToolResult so the wrapper's serializeResult works. - org.mockito.Mockito.when(client.callTool(any())) + org.mockito.Mockito.when(client.callTool(any(McpSchema.CallToolRequest.class))) .thenReturn(new McpSchema.CallToolResult(List.of(), false)); ToolCallback cb = stub("long_task"); ObjectMapper mapper = new ObjectMapper(); @@ -170,7 +170,7 @@ void progressPathInjectsTokenIdentity() throws Exception { throw new IllegalStateException(e); } McpSyncClient client = mock(McpSyncClient.class); - org.mockito.Mockito.when(client.callTool(any())) + org.mockito.Mockito.when(client.callTool(any(McpSchema.CallToolRequest.class))) .thenReturn(new McpSchema.CallToolResult(List.of(), false)); ToolCallback cb = stub("long_task"); ObjectMapper mapper = new ObjectMapper(); @@ -211,7 +211,7 @@ void progressPathInjectsTokenIdentity() throws Exception { @DisplayName("progress path with no identity (cron) injects nothing, still calls with progress token") void progressPathNoIdentityInjectsNothing() throws Exception { McpSyncClient client = mock(McpSyncClient.class); - org.mockito.Mockito.when(client.callTool(any())) + org.mockito.Mockito.when(client.callTool(any(McpSchema.CallToolRequest.class))) .thenReturn(new McpSchema.CallToolResult(List.of(), false)); ToolCallback cb = stub("long_task"); ObjectMapper mapper = new ObjectMapper(); 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 6ca4bf7aa..bc77e9186 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 @@ -12,6 +12,7 @@ import java.security.KeyPair; import java.security.KeyPairGenerator; +import java.time.Instant; import java.util.Base64; import java.util.Date; import java.util.Map; @@ -256,7 +257,16 @@ void signingKeySelfHealsAfterConfigFix() { // 1. Malformed key → fail-closed. p.getToken().setPrivateKeyPem("not-a-valid-pem"); McpIdentityForwardService service = svc(p); - assertThat(service.resolve(ctx(origin), "my-api")).isEmpty(); + assertThat(service.resolve(ctx(origin), "my-api")) + .as("malformed PEM ⇒ fail-closed, no token") + .isEmpty(); + + // 1b. The SAME malformed PEM, called again, must STILL return empty — + // proving the failure was cached (not retried per call) and that + // step 1 wasn't empty just because the PEM was treated as null. + assertThat(service.resolve(ctx(origin), "my-api")) + .as("repeated identical bad PEM ⇒ still fail-closed (cached failure)") + .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. @@ -315,39 +325,107 @@ void signingKeyRotatesOnValidPemChange() { @DisplayName("signing-key parse is single-flight under concurrency (all tokens verify)") void signingKeyParseIsSingleFlightConcurrently() throws Exception { // Stress the double-checked locking: N threads hit a cold service at once. - // Every returned token must verify against the single public key. + // Two properties must hold: (1) every returned token verifies, AND + // (2) the key is parsed exactly once (single-flight) — otherwise the DCL + // is meaningless. Counting the "loaded RS256 signing key" INFO log line + // pins the single-flight guarantee the test is named for. + ch.qos.logback.classic.Logger logger = + (ch.qos.logback.classic.Logger) org.slf4j.LoggerFactory.getLogger(McpIdentityForwardService.class); + java.util.List captured = new java.util.concurrent.CopyOnWriteArrayList<>(); + ch.qos.logback.core.read.ListAppender appender = + new ch.qos.logback.core.read.ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + KeyPair kp = rsaKeyPair(); + var p = new McpIdentityForwardProperties(); + p.getToken().setEnabled(true); + p.getToken().setIssuer("mateclaw"); + p.getToken().setTtlSeconds(60); + p.getToken().setPrivateKeyPem(Base64.getEncoder().encodeToString(kp.getPrivate().getEncoded())); + McpIdentityForwardService service = svc(p); + + int n = 16; + java.util.List> tasks = new java.util.ArrayList<>(n); + for (int i = 0; i < n; i++) { + tasks.add(() -> { + ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L); + return service.resolve(ctx(origin), "my-api").orElseThrow().value(); + }); + } + ExecutorService pool = Executors.newFixedThreadPool(n); + try { + java.util.List> futures = pool.invokeAll(tasks); + pool.shutdown(); + org.assertj.core.api.Assertions.assertThat(pool.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); + + for (java.util.concurrent.Future f : futures) { + String t = f.get(); + assertThat(t).as("every concurrent call must yield a verifiable token").isNotBlank(); + Jwts.parser().verifyWith(kp.getPublic()).requireAudience("my-api").build() + .parseSignedClaims(t).getPayload(); + } + } finally { + pool.shutdownNow(); + } + + long loads = appender.list.stream() + .filter(e -> e.getFormattedMessage().contains("loaded RS256 signing key")) + .count(); + assertThat(loads) + .as("16 concurrent cold-start calls must parse the key exactly once (single-flight DCL)") + .isEqualTo(1L); + } finally { + logger.detachAppender(appender); + } + } + + @Test + @DisplayName("valid→invalid PEM rotation: keeps the stale key, does NOT re-parse/log on every call") + void signingKeepsStaleKeyAndDedupsBadPem() { + // Regression for R2-1: once a good key is cached, replacing the PEM with + // a malformed value must (a) keep serving the old key (best-effort), + // (b) log the parse failure ONCE, not on every subsequent call. 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); - p.getToken().setPrivateKeyPem(Base64.getEncoder().encodeToString(kp.getPrivate().getEncoded())); + p.getToken().setPrivateKeyPem(goodPem); McpIdentityForwardService service = svc(p); - int n = 16; - // Use Callables so a thread that loses the race still returns its result; - // no shared mutable array, no early-return-drops-slot hazard. - java.util.List> tasks = new java.util.ArrayList<>(n); - for (int i = 0; i < n; i++) { - tasks.add(() -> { - ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L); - return service.resolve(ctx(origin), "my-api").orElseThrow().value(); - }); - } - ExecutorService pool = Executors.newFixedThreadPool(n); - try { - java.util.List> futures = pool.invokeAll(tasks); - pool.shutdown(); - org.assertj.core.api.Assertions.assertThat(pool.awaitTermination(30, TimeUnit.SECONDS)).isTrue(); + // 1. Prime: a good key is loaded and a token mints. + String tok1 = service.resolve(ctx(origin), "my-api").orElseThrow().value(); + Jwts.parser().verifyWith(kp.getPublic()).requireAudience("my-api").build() + .parseSignedClaims(tok1).getPayload(); - for (java.util.concurrent.Future f : futures) { - String t = f.get(); // propagates any execution exception - assertThat(t).isNotBlank(); + // 2. Corrupt the PEM. Subsequent calls must still mint verifiable tokens + // (using the stale good key) — fail-soft, not fail-closed. + ch.qos.logback.classic.Logger logger = + (ch.qos.logback.classic.Logger) org.slf4j.LoggerFactory.getLogger(McpIdentityForwardService.class); + ch.qos.logback.core.read.ListAppender appender = + new ch.qos.logback.core.read.ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + p.getToken().setPrivateKeyPem("not-a-valid-pem-anymore"); + for (int i = 0; i < 5; i++) { + String t = service.resolve(ctx(origin), "my-api").orElseThrow().value(); + // Still verifies with the ORIGINAL pubkey → stale key still in use. Jwts.parser().verifyWith(kp.getPublic()).requireAudience("my-api").build() .parseSignedClaims(t).getPayload(); } + long failures = appender.list.stream() + .filter(e -> e.getFormattedMessage().contains("failed to parse private-key-pem")) + .count(); + assertThat(failures) + .as("a repeated bad PEM must be parsed/logged exactly once, not once per call") + .isEqualTo(1L); } finally { - pool.shutdownNow(); + logger.detachAppender(appender); } } @@ -374,10 +452,54 @@ void tokenMintAndVerify() throws Exception { assertThat(claims.getSubject()).isEqualTo("42"); assertThat(claims.get("trust", String.class)).isEqualTo(McpIdentityForwardService.TRUST_AUTHENTICATED); assertThat(claims.get("channel_type", String.class)).isEqualTo("web"); + // exp must be bounded to the configured TTL window (60s) — not just any + // future date. A regression that dropped ttlSeconds would leak here. assertThat(claims.getExpiration()).isAfter(new Date()); + assertThat(claims.getExpiration().toInstant()) + .as("exp must be within ttlSeconds (60) + small skew, not unbounded") + .isBefore(Instant.now().plusSeconds(70)); assertThat(claims.getId()).isNotBlank(); } + @Test + @DisplayName("token mode: two mints for the same subject get distinct jti (replay distinguishability)") + void tokenJtiUniqueAcrossMints() { + // The backend may track consumed jtis to reject replays; that only works + // if each mint produces a fresh jti. + KeyPair kp = rsaKeyPair(); + var p = tokenProps(kp); + ChatOrigin origin = ChatOrigin.web("c1", "alice", 1L, null, null, 42L); + McpIdentityForwardService service = svc(p); + + Claims c1 = Jwts.parser().verifyWith(kp.getPublic()).build() + .parseSignedClaims(service.resolve(ctx(origin), "my-api").orElseThrow().value()) + .getPayload(); + Claims c2 = Jwts.parser().verifyWith(kp.getPublic()).build() + .parseSignedClaims(service.resolve(ctx(origin), "my-api").orElseThrow().value()) + .getPayload(); + + assertThat(c1.getId()).as("jti must differ across mints").isNotEqualTo(c2.getId()); + assertThat(c1.getSubject()).isEqualTo(c2.getSubject()); // same subject, different token + } + + @Test + @DisplayName("plaintext mode: colon-bearing visitorId round-trips as anonymous:") + void plaintextColonInSubject() { + // webchat visitorIds may contain ':' (VISITOR_ID_PATTERN allows it). The + // plaintext value is ':', so a colon in subject is only + // recoverable via partition(":") (split on FIRST colon) — pin that contract. + ChatOrigin origin = new ChatOrigin(null, "c1", "a:b", 1L, null, + null, null, false, null, "api", null, null, null); + Optional inj = + svc(new McpIdentityForwardProperties()).resolve(ctx(origin), "my-api"); + assertThat(inj).isPresent(); + String value = inj.get().value(); + assertThat(value).startsWith("anonymous:"); + // partition(":") on the backend must recover the full colon-bearing subject. + String subject = value.substring(value.indexOf(':') + 1); + assertThat(subject).isEqualTo("a:b"); + } + @Test @DisplayName("token mode: anonymous visitor token carries trust=anonymous") void tokenAnonymousVisitor() throws Exception { @@ -399,11 +521,19 @@ void tokenAnonymousVisitor() throws Exception { } @Test - @DisplayName("audienceFor: explicit mapping wins, else server name") + @DisplayName("audienceFor: explicit mapping wins (by name AND by id), else server name/id") void audienceResolution() { var p = new McpIdentityForwardProperties(); assertThat(p.audienceFor(42L, "svc")).isEqualTo("svc"); // default = name p.getToken().setAudiences(Map.of("svc", "https://api.internal")); assertThat(p.audienceFor(42L, "svc")).isEqualTo("https://api.internal"); + // Mapping by NUMERIC ID (as string) — branch not covered before. + p.getToken().setAudiences(Map.of("42", "https://by-id.internal")); + assertThat(p.audienceFor(42L, "unmapped-name")) + .as("audience mapped by numeric id must win when name is unmapped") + .isEqualTo("https://by-id.internal"); + // Name mapping takes precedence over id mapping when both are configured. + p.getToken().setAudiences(Map.of("svc", "https://by-name", "42", "https://by-id")); + assertThat(p.audienceFor(42L, "svc")).isEqualTo("https://by-name"); } } From 25aa27b56138e44394fd93ef0cd0facc42cdc622 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=80=AA=E7=A8=8B=E4=BC=9F?= Date: Sat, 18 Jul 2026 05:31:17 +0800 Subject: [PATCH 3/3] =?UTF-8?q?fix(mcp):=20round-3=20adversarial=20review?= =?UTF-8?q?=20of=20#459=20=E2=80=94=20deployment=20+=20hardening?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 9 confirmed findings from the third adversarial pass (regression/interaction + production-readiness lenses) on the post-R2 code. Deployment artifacts (P3-1 CRITICAL, P3-2/P3-3 HIGH) — the feature could NOT be deployed via the documented Docker path before this: - .env.example: added MCP_IDFWD_PRIVATE_KEY_PEM block (was absent; JWT_SECRET was the precedent). An operator copying .env.example never learned the feature existed, so token mode silently failed-closed forever. - docker-compose.yml: pass MCP_IDFWD_PRIVATE_KEY_PEM into the container. - application.yml: added a commented identity-forward example block so the shipped config is self-documenting and the ${MCP_IDFWD_PRIVATE_KEY_PEM:} placeholder actually exists (was only in docs/, which disagreed with the shipped config). Hardening: - Log injection (P3-5): e.getMessage() from Jackson was interpolated raw into logs; a crafted toolInput containing CR/LF could forge log lines. Added a sanitize() helper (strip control chars, cap length) on all 3 message sites + a test pinning the behavior. - Encrypted/FIPS key errors (P3-8/P3-9): detect ENCRYPTED PRIVATE KEY and NoSuchAlgorithmException specifically, emit actionable hints instead of a generic 'failed to parse'. Docs (en + zh): - Clock skew (P3-6): document backend-side leeway / NTP guidance (60s TTL + drift was rejecting valid tokens, undocumented). - Key rotation runbook (P3-7): rotation is a coordinated multi-host operation (update backends FIRST, then MateClaw, watch the load log); no JWKS yet. Test quality: - R3-2: deleted a dead 'captured' variable that misled about the assertion sink. - R3-3: guarded the logback Logger casts with assumeTrue so a non-logback SLF4J binding skips instead of throwing ClassCastException. The signingKey() state machine (rewritten twice across R1/R2) was verified correct across all 9 transitions by the regression reviewer; no production regression found. Tests: 204 run, 0 failures (identity-forward subset 47→48). YAML validated; Spring loads application.yml; log-appender tests stable across stress runs. --- .env.example | 9 +++++++ docker-compose.yml | 4 +++ .../IdentityForwardingToolCallback.java | 19 ++++++++++++- .../runtime/McpIdentityForwardService.java | 27 +++++++++++++++++-- .../src/main/resources/application.yml | 18 +++++++++++++ .../src/main/resources/docs/en/mcp.md | 26 +++++++++++++++++- .../src/main/resources/docs/zh/mcp.md | 21 ++++++++++++++- .../IdentityForwardingToolCallbackTest.java | 17 ++++++++++++ .../McpIdentityForwardServiceTest.java | 11 +++++++- 9 files changed, 146 insertions(+), 6 deletions(-) diff --git a/.env.example b/.env.example index b9ba79a13..c356181ec 100644 --- a/.env.example +++ b/.env.example @@ -40,6 +40,15 @@ TAVILY_API_KEY= # 生产部署必须设置，至少 32 位随机字符串：openssl rand -base64 48 JWT_SECRET= +# MCP on-behalf-of 身份透传（STDIO MCP server 代表登录用户调 REST 后端）的 +# RS256 签名私钥（PKCS#8 PEM，明文、不带 passphrase）。仅当你在 +# application.yml 里把 mateclaw.mcp.identity-forward.servers 配了允许清单、 +# 且 token.enabled=true 时才需要。留空 ⇒ token 模式 fail-closed（不签发、不注入）。 +# 生成：openssl genpkey -algorithm RSA -pkcs8 -out mcp-idfwd-private.pem +# 然后把整段 PEM（含 -----BEGIN/END-----）作为该变量的值。 +# 公钥配到 REST 后端用于验签。详见 docs/{en,zh}/mcp.md "on-behalf-of" 章节。 +MCP_IDFWD_PRIVATE_KEY_PEM= + # CORS 白名单（逗号分隔，如 https://mateclaw.example.com,https://admin.example.com）。 # 若留空，服务器会允许所有 origin 并在启动日志里 WARN。生产部署务必设置。 MATECLAW_CORS_ALLOWED_ORIGINS= diff --git a/docker-compose.yml b/docker-compose.yml index 6a8a0411e..18cd99906 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -108,6 +108,10 @@ services: SERPER_API_KEY: ${SERPER_API_KEY:-} TAVILY_API_KEY: ${TAVILY_API_KEY:-} JWT_SECRET: ${JWT_SECRET:-} + # MCP on-behalf-of identity forwarding (RS256 signing key, PKCS#8 PEM). + # Only needed when mateclaw.mcp.identity-forward has opted-in servers and + # token.enabled=true. Empty by default ⇒ token mode fails closed (safe). + MCP_IDFWD_PRIVATE_KEY_PEM: ${MCP_IDFWD_PRIVATE_KEY_PEM:-} MATECLAW_CORS_ALLOWED_ORIGINS: ${MATECLAW_CORS_ALLOWED_ORIGINS:-} # SearXNG: tell the app where to reach the sidecar container SEARXNG_BASE_URL: ${SEARXNG_BASE_URL:-http://searxng:8080} diff --git a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallback.java b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallback.java index 2ec356c79..0570dabf6 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallback.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallback.java @@ -110,8 +110,25 @@ static String withClaim(String toolInput, String key, String value) { node.put(key, value); return MAPPER.writeValueAsString(node); } catch (Exception e) { - log.warn("[McpIdentity] failed to inject identity into tool input: {}", e.getMessage()); + // Sanitize: exception messages may echo fragments of the (model/user + // controlled) toolInput, and Jackson errors embed a source snippet — + // strip CR/LF so a crafted input can't forge extra log lines under + // naive log shippers, and cap the length. + log.warn("[McpIdentity] failed to inject identity into tool input: {}", + sanitize(e.getMessage())); return toolInput; } } + + /** + * Make an exception message safe to interpolate into a log line: replace + * CR/LF (and other ASCII control chars) with a visible glyph and cap the + * length, so a crafted toolInput echoed inside a Jackson/parse error can't + * forge additional log records (log-injection hardening). + */ + static String sanitize(String msg) { + if (msg == null) return ""; + String cleaned = msg.replaceAll("[\\r\\n\\t\\p{Cntrl}]", "?"); + return cleaned.length() > 200 ? cleaned.substring(0, 200) + "…" : cleaned; + } } 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 76e079b71..3f6b5217b 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 @@ -109,6 +109,18 @@ public String audienceFor(Long serverId, String serverName) { /** The (key, value) to merge into the call arguments, or empty to inject nothing. */ public record Injection(String key, String value) {} + /** + * Make an exception message safe to interpolate into a log line: replace + * CR/LF and other ASCII control chars (which a malformed operator- or model + * supplied PEM/toolInput can surface inside the message) with a visible + * glyph, and cap the length — log-injection hardening. + */ + private static String sanitize(String msg) { + if (msg == null) return ""; + String cleaned = msg.replaceAll("[\\r\\n\\t\\p{Cntrl}]", "?"); + return cleaned.length() > 200 ? cleaned.substring(0, 200) + "…" : cleaned; + } + /** A typed identity resolved from the request origin. Empty = inject nothing. */ record ResolvedIdentity(String subject, String trust, String channelType) { static final ResolvedIdentity NONE = new ResolvedIdentity(null, null, null); @@ -213,7 +225,7 @@ private String mint(ResolvedIdentity id, String audience) { .signWith(key, Jwts.SIG.RS256) .compact(); } catch (Exception e) { - log.error("[McpIdentity] failed to mint identity token: {}", e.getMessage()); + log.error("[McpIdentity] failed to mint identity token: {}", sanitize(e.getMessage())); return null; } } @@ -291,7 +303,18 @@ private PrivateKey signingKey() { // If a prior good key exists it keeps serving (best-effort // availability); if not, signingKey stays null (fail-closed). // Either way the dedup above prevents re-parsing this same PEM. - log.error("[McpIdentity] failed to parse private-key-pem (expect PKCS#8 RSA): {}", e.getMessage()); + String hint = ""; + if (pem != null && pem.contains("ENCRYPTED PRIVATE KEY")) { + hint = " (the key is passphrase-protected; strip it with " + + "`openssl pkcs8 -topk8 -nocrypt -in ... -out ...` — " + + "MateClaw needs an UNENCRYPTED PKCS#8 RSA key)"; + } else if (e instanceof java.security.NoSuchAlgorithmException) { + hint = " (RSA KeyFactory unavailable in this JVM's security-provider " + + "configuration — FIPS/restricted-provider JVMs may need " + + "BouncyCastle registered)"; + } + log.error("[McpIdentity] failed to parse private-key-pem (expect PKCS#8 RSA){}: {}", + hint, sanitize(e.getMessage())); } return signingKey; } diff --git a/mateclaw-server/src/main/resources/application.yml b/mateclaw-server/src/main/resources/application.yml index 891e0eca4..8c91db9aa 100644 --- a/mateclaw-server/src/main/resources/application.yml +++ b/mateclaw-server/src/main/resources/application.yml @@ -172,6 +172,24 @@ mateclaw: # MCP server 配置已迁移至数据库（mate_mcp_server 表），通过 UI 管理 mcp: enabled: true + # on-behalf-of identity forwarding to MCP servers (issue #459). OFF by default — + # enabling leaks caller identity to the listed servers, so it is opt-in per + # server (by name or numeric id). Two trust models: plaintext (default) injects + # ':' under __mateclaw_user__; token (token.enabled=true) injects + # a short-lived RS256 JWT under __mateclaw_token__. Token mode fails closed when + # the private key is missing/malformed (never silently downgrades to plaintext). + # See docs/{en,zh}/mcp.md "on-behalf-of" for the wire format and backend recipe. + # identity-forward: + # servers: + # - my-internal-api # mate_mcp_server name or numeric id + # token: + # enabled: true # false ⇒ plaintext (back-compat) + # issuer: mateclaw + # ttl-seconds: 60 # short; backend should allow clock skew + # key-id: mateclaw-mcp-1 + # private-key-pem: ${MCP_IDFWD_PRIVATE_KEY_PEM:} # PKCS#8 PEM, RS256; empty ⇒ fail-closed + # audiences: # optional; default aud = server name + # my-internal-api: https://api.internal tools: disclosure: # progressive: extension-tier tools are hidden behind the extension-tools diff --git a/mateclaw-server/src/main/resources/docs/en/mcp.md b/mateclaw-server/src/main/resources/docs/en/mcp.md index 4b01257fe..8a0536977 100644 --- a/mateclaw-server/src/main/resources/docs/en/mcp.md +++ b/mateclaw-server/src/main/resources/docs/en/mcp.md @@ -555,8 +555,11 @@ REST backend verifies (pseudocode): ```python import jwt # PyJWT +# Allow a small clock-skew window — iat/exp come from MateClaw's clock, which +# may drift from this backend's. With ttl-seconds=60, a leeway of ~10s avoids +# rejecting valid tokens near the boundary. claims = jwt.decode(token, public_key_pem, algorithms=["RS256"], - issuer="mateclaw", audience="https://api.internal") + issuer="mateclaw", audience="https://api.internal", leeway=10) # A valid signature only proves MateClaw minted the token — it does NOT mean # `sub` is an authenticated account. Check the trust claim before authorizing. if claims.get("trust") != "authenticated": @@ -565,6 +568,27 @@ user = claims["sub"] # immutable numeric id (authenticated path) # → per-user authorization; invalid/expired → 401 ``` +> **Clock skew.** `iat`/`exp` are computed from the MateClaw host's clock. With +> the default 60s TTL, keep MateClaw and the REST backend on the same NTP source +> and configure verification leeway (PyJWT `leeway=10`, jjwt +> `setAllowedClockSkewSeconds(10)`) so minor drift doesn't reject valid tokens. +> Raise `ttl-seconds` if your deployment has larger skew. + +> **Key rotation runbook (no JWKS yet).** Public-key distribution is out-of-band +> today (a JWKS endpoint is a follow-up). Rotating the signing key is therefore +> a **coordinated multi-host operation**, not a one-shot MateClaw change: +> 1. Generate a new keypair (`openssl genpkey -algorithm RSA -pkcs8 ...`). +> 2. **Update every REST backend's public key FIRST** — until every backend +> accepts the new public key, tokens signed with the new private key will be +> rejected with 401. +> 3. Then update `MCP_IDFWD_PRIVATE_KEY_PEM` on MateClaw and restart (or rebind +> the properties bean if refresh-scoped). +> 4. Watch the MateClaw log for `[McpIdentity] loaded RS256 signing key (kid=…)` +> — the new `kid` confirms the rotation took effect. +> +> A spike in backend 401s immediately after a MateClaw-side rotation almost +> always means step 2 was missed on some backend. + > Public-key distribution: for now an operator configures the public key on the > REST side out-of-band. A JWKS endpoint for auto-distribution + rotation is a > natural follow-up. diff --git a/mateclaw-server/src/main/resources/docs/zh/mcp.md b/mateclaw-server/src/main/resources/docs/zh/mcp.md index 5cd8b902c..aae5f4b31 100644 --- a/mateclaw-server/src/main/resources/docs/zh/mcp.md +++ b/mateclaw-server/src/main/resources/docs/zh/mcp.md @@ -504,8 +504,10 @@ REST 后端验签（伪代码）： ```python import jwt # PyJWT +# 留一点时钟偏差余量——iat/exp 用的是 MateClaw 的时钟，可能与本后端有漂移。 +# ttl-seconds=60 时，leeway≈10s 可避免在窗口边界拒绝合法 token。 claims = jwt.decode(token, public_key_pem, algorithms=["RS256"], - issuer="mateclaw", audience="https://api.internal") + issuer="mateclaw", audience="https://api.internal", leeway=10) # 验签通过只证明 token 由 MateClaw 签发——并不代表 sub 是认证账号。 # 授权前务必看 trust claim。 if claims.get("trust") != "authenticated": @@ -514,6 +516,23 @@ user = claims["sub"] # 不可变数字 id（认证路径） # → 按 user 做 per-user 授权；验签失败/过期 → 401 ``` +> **时钟偏差。** `iat`/`exp` 按 MateClaw 主机时钟计算。默认 60s TTL 下，请让 +> MateClaw 与 REST 后端共用同一 NTP 源，并在验签侧配置 leeway（PyJWT +> `leeway=10`、jjwt `setAllowedClockSkewSeconds(10)`），避免微小漂移误拒合法 +> token。部署偏差较大时调大 `ttl-seconds`。 + +> **密钥轮换手册（暂无 JWKS）。** 公钥当前带外分发（JWKS 端点是后续工作）， +> 因此轮换签名密钥是**多主机协同操作**，不是 MateClaw 单侧一键完成： +> 1. 生成新密钥对（`openssl genpkey -algorithm RSA -pkcs8 ...`）。 +> 2. **先把每个 REST 后端的公钥更新到位**——在后端全部接受新公钥之前，用 +> 新私钥签的 token 会被 401 拒绝。 +> 3. 然后更新 MateClaw 的 `MCP_IDFWD_PRIVATE_KEY_PEM` 并重启（或对 properties +> bean 做 refresh-scoped 重绑）。 +> 4. 观察日志出现 `[McpIdentity] loaded RS256 signing key (kid=…)`——新 `kid` +> 确认轮换已生效。 +> +> MateClaw 侧轮换后若后端 401 激增，几乎一定是某个后端漏做了第 2 步。 + > 公钥分发：当前由运维把上面生成的公钥配到 REST 侧（带外）。后续可加一个 JWKS 端点自动分发+轮换。 > > 与 API Key 的关系：可保留 API Key 作"服务/通道认证"（这台 MCP 服务被允许跟后端说话）+ JWT 作"用户断言"，双层更清晰；也可让 JWT 一肩挑。 diff --git a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java index ee1aec7f5..c77f61b83 100644 --- a/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java +++ b/mateclaw-server/src/test/java/vip/mate/tool/mcp/runtime/IdentityForwardingToolCallbackTest.java @@ -77,6 +77,23 @@ void malformedJsonUnchanged() { assertThat(IdentityForwardingToolCallback.withClaim("{not json", KEY, "alice")).isEqualTo("{not json"); } + @Test + @DisplayName("sanitize strips CR/LF and caps length (log-injection hardening)") + void sanitizePreventsLogInjection() { + // A crafted toolInput surfaces inside Jackson exception messages; the + // sanitize helper must neutralize newlines (no forged log lines) and + // cap length so a huge payload can't blow up log volume. + String crafted = "line1\nline2\r\n2026-07-18 ERROR fake-injected-line\ttab"; + String out = IdentityForwardingToolCallback.sanitize(crafted); + assertThat(out).as("no CR/LF/control chars survive").doesNotContain("\n", "\r", "\t"); + assertThat(out).contains("line1").contains("line2"); // content preserved + // Length cap. + String huge = "x".repeat(500); + assertThat(IdentityForwardingToolCallback.sanitize(huge).length()) + .as("capped to ~200 chars").isLessThanOrEqualTo(201); + assertThat(IdentityForwardingToolCallback.sanitize(null)).isEmpty(); + } + @Test @DisplayName("opt-in matching: by id or name, empty set never forwards") void optInMatching() { 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 bc77e9186..09f1313f4 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 @@ -329,9 +329,14 @@ void signingKeyParseIsSingleFlightConcurrently() throws Exception { // (2) the key is parsed exactly once (single-flight) — otherwise the DCL // is meaningless. Counting the "loaded RS256 signing key" INFO log line // pins the single-flight guarantee the test is named for. + // Guard the logback cast: these tests need a logback Logger to attach a + // ListAppender. Skip (not fail) if the runtime SLF4J binding isn't logback. + org.junit.jupiter.api.Assumptions.assumeTrue( + org.slf4j.LoggerFactory.getLogger(McpIdentityForwardService.class) + instanceof ch.qos.logback.classic.Logger, + "logback must be the active SLF4J binding for this log-capture test"); ch.qos.logback.classic.Logger logger = (ch.qos.logback.classic.Logger) org.slf4j.LoggerFactory.getLogger(McpIdentityForwardService.class); - java.util.List captured = new java.util.concurrent.CopyOnWriteArrayList<>(); ch.qos.logback.core.read.ListAppender appender = new ch.qos.logback.core.read.ListAppender<>(); appender.start(); @@ -404,6 +409,10 @@ void signingKeepsStaleKeyAndDedupsBadPem() { // 2. Corrupt the PEM. Subsequent calls must still mint verifiable tokens // (using the stale good key) — fail-soft, not fail-closed. + org.junit.jupiter.api.Assumptions.assumeTrue( + org.slf4j.LoggerFactory.getLogger(McpIdentityForwardService.class) + instanceof ch.qos.logback.classic.Logger, + "logback must be the active SLF4J binding for this log-capture test"); ch.qos.logback.classic.Logger logger = (ch.qos.logback.classic.Logger) org.slf4j.LoggerFactory.getLogger(McpIdentityForwardService.class); ch.qos.logback.core.read.ListAppender appender =