diff --git a/.changeset/mcp-pool-key-credential-retention.md b/.changeset/mcp-pool-key-credential-retention.md new file mode 100644 index 000000000..9506f5d5f --- /dev/null +++ b/.changeset/mcp-pool-key-credential-retention.md @@ -0,0 +1,11 @@ +--- +"executor": patch +--- + +**The MCP connection pool no longer keeps credentials in its cache key** + +A pooled remote MCP session is looked up by a key describing the connection's identity, and that key included the connection's resolved credential values — plus the headers and query params those same secrets had already been rendered into. The key is retained as a `Map` key for the pool's lifetime, so the secret stayed readable in process memory long after the call that needed it had finished, with nothing left to read it. + +The key is now the SHA-256 digest of that identity rather than the identity itself. Reuse is unchanged, because equal identities still produce equal keys, and separation is unchanged too: a rotated access token, a different rendered auth header and a credential carried in a query param each still dial a fresh session instead of reusing one authenticated as somebody else. Hashing the whole identity rather than only the fields known to be sensitive means a field added later is covered without anyone having to remember it carries a secret. + +Nothing reads the key back — the pool only compares it, and it reaches no log, span or error message — so nothing observable changes. diff --git a/packages/plugins/mcp/src/sdk/connection-pool-key.test.ts b/packages/plugins/mcp/src/sdk/connection-pool-key.test.ts new file mode 100644 index 000000000..3b4db9ff0 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/connection-pool-key.test.ts @@ -0,0 +1,154 @@ +// --------------------------------------------------------------------------- +// MCP connection-pool key +// +// The key decides which pooled session a call may reuse, and it is retained as +// a `Map` key for the POOL's lifetime — much longer than the call that produced +// it. Two things therefore have to hold at once, and they pull in opposite +// directions: +// +// * it must still SEPARATE identities — a different credential value, or a +// different rendered auth header, must never reuse somebody else's +// authenticated session; +// * it must not RETAIN the credential — the secret that distinguishes two +// identities must not survive in the key that distinguishes them. +// +// A digest satisfies both. These tests pin both halves, because a change that +// satisfied only the second (say, dropping the credential from the key) would +// look like a privacy improvement and be a session-hijack bug. +// +// The pool itself composes on top: it is a `Map` keyed by this string, so +// "different key" ⇒ "different session" is the pool's own property, covered in +// `connection-pool.test.ts`. +// --------------------------------------------------------------------------- + +import { describe, expect, it } from "@effect/vitest"; +import { Effect } from "effect"; + +import { connectionPoolKey } from "./plugin"; +import type { ConnectorInput } from "./connection"; + +const SECRET = "sk-live-poolkey-Zq7!x-SECRET"; +const OTHER_SECRET = "sk-live-poolkey-Zq7!x-ROTATED"; + +type RemoteInput = Extract; + +const remoteInput = (overrides: Partial = {}): RemoteInput => ({ + transport: "remote", + endpoint: "https://mcp.example.com/sse", + remoteTransport: "streamable-http", + headers: { authorization: `Bearer ${SECRET}` }, + ...overrides, +}); + +describe("MCP connection-pool key", () => { + it.effect("is a bare SHA-256 digest — no plaintext rides along", () => + Effect.gen(function* () { + const key = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET }); + + // Asserted positively as well as negatively: "does not contain the + // secret" alone would still pass for a key that appended the digest to + // the plaintext identity. + expect(key).toMatch(/^[0-9a-f]{64}$/); + expect(key).not.toContain(SECRET); + expect(key).not.toContain(`Bearer ${SECRET}`); + expect(key).not.toContain("mcp.example.com"); + }), + ); + + it.effect("the same identity keeps producing the same key, so reuse is unchanged", () => + Effect.gen(function* () { + const first = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET }); + const second = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET }); + + expect(first).toBe(second); + }), + ); + + it.effect("a rotated credential value produces a different key", () => + Effect.gen(function* () { + // The case this field exists for: a refreshed access token must dial a + // fresh session rather than reuse one authenticated with the old token. + const before = yield* connectionPoolKey(remoteInput({ headers: {} }), "bearer", { + token: SECRET, + }); + const after = yield* connectionPoolKey(remoteInput({ headers: {} }), "bearer", { + token: OTHER_SECRET, + }); + + expect(after).not.toBe(before); + }), + ); + + it.effect("a different rendered auth header produces a different key", () => + Effect.gen(function* () { + // `buildConnectorInput` renders apikey placements onto `headers`, so the + // same secret reaches the key by a second route. Separation has to hold + // there too. + const mine = yield* connectionPoolKey( + remoteInput({ headers: { authorization: `Bearer ${SECRET}` } }), + "bearer", + {}, + ); + const theirs = yield* connectionPoolKey( + remoteInput({ headers: { authorization: `Bearer ${OTHER_SECRET}` } }), + "bearer", + {}, + ); + + expect(theirs).not.toBe(mine); + }), + ); + + it.effect("a credential carried in a query param separates too", () => + Effect.gen(function* () { + // Servers that authenticate via `?token=` put the secret here instead. + const mine = yield* connectionPoolKey( + remoteInput({ headers: {}, queryParams: { token: SECRET } }), + "query", + {}, + ); + const theirs = yield* connectionPoolKey( + remoteInput({ headers: {}, queryParams: { token: OTHER_SECRET } }), + "query", + {}, + ); + + expect(theirs).not.toBe(mine); + expect(mine).not.toContain(SECRET); + }), + ); + + it.effect("insertion order does not split one identity into two", () => + Effect.gen(function* () { + // `sortedRecord` exists for this: a key that changed with property order + // would silently dial a new session per call and never reuse anything. + const oneWay = yield* connectionPoolKey( + remoteInput({ headers: { authorization: `Bearer ${SECRET}`, "x-team": "acme" } }), + "bearer", + { token: SECRET, region: "eu" }, + ); + const otherWay = yield* connectionPoolKey( + remoteInput({ headers: { "x-team": "acme", authorization: `Bearer ${SECRET}` } }), + "bearer", + { region: "eu", token: SECRET }, + ); + + expect(otherWay).toBe(oneWay); + }), + ); + + it.effect("a different endpoint or template separates identities", () => + Effect.gen(function* () { + const base = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET }); + const otherEndpoint = yield* connectionPoolKey( + remoteInput({ endpoint: "https://mcp.other.example.com/sse" }), + "bearer", + { token: SECRET }, + ); + const otherTemplate = yield* connectionPoolKey(remoteInput(), "apikey", { token: SECRET }); + + expect(otherEndpoint).not.toBe(base); + expect(otherTemplate).not.toBe(base); + }), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index e3b7a6857..da198b065 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -14,6 +14,7 @@ import { IntegrationSlug, mergeAuthTemplates, OAuthClientSlug, + sha256Hex, tool, ToolResult, type AuthMethodDescriptor, @@ -628,20 +629,45 @@ const sortedRecord = ( .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)), ); -const connectionPoolKey = ( +/** The pooled remote connection's identity, as an opaque digest. + * + * HASHED, not carried in the clear, because three of the fields below hold a + * live credential: `values` is the connection's resolved secret inputs, and + * `headers` / `queryParams` are those SAME secrets already rendered onto the + * outbound request by `buildConnectorInput`. The result is retained as a `Map` + * key for the POOL's lifetime (`connection-pool.ts`), which outlives by far the + * call that needed the secret — so a plaintext key leaves credentials sitting + * in process memory with no reader. + * + * Hashing the WHOLE serialized identity rather than only the fields known to + * be sensitive keeps equality exactly (same identity → same digest, so reuse is + * unchanged) and keeps any field added later covered without anyone having to + * remember it carries a secret. Nothing reads the key back: the pool only ever + * compares it, and it reaches no log, span or error message. + * + * SHA-256 rather than a cheap non-cryptographic hash on purpose. A collision + * means reusing a connection authenticated as somebody else, so the hash has to + * be one an attacker who controls their own credential values cannot aim. + * + * Exported for tests (not re-exported from `sdk/index.ts`, so this widens no + * public API): the retention property is a property of the KEY, and asserting + * it through pool behaviour alone would not see it. */ +export const connectionPoolKey = ( input: Extract, template: string, values: Record, -): string => - JSON.stringify({ - endpoint: input.endpoint, - transport: input.transport, - remoteTransport: input.remoteTransport, - headers: sortedRecord(input.headers), - queryParams: sortedRecord(input.queryParams), - template, - values: sortedRecord(values), - }); +): Effect.Effect => + sha256Hex( + JSON.stringify({ + endpoint: input.endpoint, + transport: input.transport, + remoteTransport: input.remoteTransport, + headers: sortedRecord(input.headers), + queryParams: sortedRecord(input.queryParams), + template, + values: sortedRecord(values), + }), + ); // --------------------------------------------------------------------------- // Declared auth methods — project the stored MCP config into the catalog's @@ -1306,7 +1332,11 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { const connector: McpConnector = createMcpConnector(connectorInput); const poolKey = connectorInput.transport === "remote" - ? connectionPoolKey(connectorInput, String(credential.template), credential.values) + ? yield* connectionPoolKey( + connectorInput, + String(credential.template), + credential.values, + ) : undefined; const connectionRef = {