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/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/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/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..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 @@ -76,10 +76,24 @@ 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 + * 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; } @@ -95,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); @@ -199,39 +225,63 @@ 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; } } /** - * 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. + * 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: + * + * 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(); 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)) { - 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; " @@ -243,11 +293,28 @@ 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)); + 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) { - log.error("[McpIdentity] failed to parse private-key-pem (expect PKCS#8 RSA): {}", e.getMessage()); + // 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. + 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 f4c41288d..8a0536977 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: @@ -509,12 +555,40 @@ 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") -user = claims["sub"] # trusted only after signature verification + 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": + 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 ``` +> **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 b0500f830..aae5f4b31 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）只透传、不验签： @@ -480,12 +504,35 @@ 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") -user = claims["sub"] # 验签通过才相信 + issuer="mateclaw", audience="https://api.internal", leeway=10) +# 验签通过只证明 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 ``` +> **时钟偏差。** `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 2a053f042..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 @@ -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 { @@ -58,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() { @@ -69,4 +105,72 @@ void optInMatching() { p.setServers(Set.of("42")); 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() { + 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..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 @@ -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(McpSchema.CallToolRequest.class))) + .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(McpSchema.CallToolRequest.class))) + .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(McpSchema.CallToolRequest.class))) + .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..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 @@ -12,10 +12,14 @@ import java.security.KeyPair; import java.security.KeyPairGenerator; +import java.time.Instant; import java.util.Base64; 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 +163,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() { @@ -230,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. @@ -250,6 +286,158 @@ 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. + // 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. + // 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); + 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(goodPem); + McpIdentityForwardService service = svc(p); + + // 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(); + + // 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 = + 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 { + logger.detachAppender(appender); + } + } + @Test @DisplayName("token mode: mints RS256 JWT that verifies with trust/channel_type claims") void tokenMintAndVerify() throws Exception { @@ -273,10 +461,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 { @@ -298,11 +530,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"); } }