Keep credentials out of the MCP connection pool's cache key (#1573)

The pool key describing a remote MCP session's identity embedded the connection's resolved credential values, and the headers and query params those same secrets had already been rendered into. That 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, with nothing left to read it. Hash the whole serialised identity instead. Equal identities still produce equal keys, so reuse is unchanged, and a rotated token, a different rendered auth header or a query-param credential each still dial a fresh session. Hashing everything rather than the fields known to be sensitive means a field added later is covered without anyone having to remember it carries a secret. SHA-256 rather than a cheap hash on purpose: a collision would mean reusing a connection authenticated as somebody else.

Sergio Fernández committed 2 months ago

Commit 45ba1411 parent e8e97c43 files · +207 −12Browse files

Unified · Split

3 files

.changeset/mcp-pool-key-credential-retention.md +11 −0
packages/plugins/mcp/src/sdk/connection-pool-key.test.ts +154 −0
packages/plugins/mcp/src/sdk/plugin.ts +42 −12

.changeset/mcp-pool-key-credential-retention.md+11−0

@@ -0,0 +1,11 @@
	1	---
	2	"executor": patch
	3	---
	4
	5	The MCP connection pool no longer keeps credentials in its cache key
	6
	7	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.
	8
	9	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.
	10
	11	Nothing reads the key back — the pool only compares it, and it reaches no log, span or error message — so nothing observable changes.

packages/plugins/mcp/src/sdk/connection-pool-key.test.ts+154−0

@@ -0,0 +1,154 @@
	1	// ---------------------------------------------------------------------------
	2	// MCP connection-pool key
	3	//
	4	// The key decides which pooled session a call may reuse, and it is retained as
	5	// a `Map` key for the POOL's lifetime — much longer than the call that produced
	6	// it. Two things therefore have to hold at once, and they pull in opposite
	7	// directions:
	8	//
	9	// * it must still SEPARATE identities — a different credential value, or a
	10	// different rendered auth header, must never reuse somebody else's
	11	// authenticated session;
	12	// * it must not RETAIN the credential — the secret that distinguishes two
	13	// identities must not survive in the key that distinguishes them.
	14	//
	15	// A digest satisfies both. These tests pin both halves, because a change that
	16	// satisfied only the second (say, dropping the credential from the key) would
	17	// look like a privacy improvement and be a session-hijack bug.
	18	//
	19	// The pool itself composes on top: it is a `Map` keyed by this string, so
	20	// "different key" ⇒ "different session" is the pool's own property, covered in
	21	// `connection-pool.test.ts`.
	22	// ---------------------------------------------------------------------------
	23
	24	import { describe, expect, it } from "@effect/vitest";
	25	import { Effect } from "effect";
	26
	27	import { connectionPoolKey } from "./plugin";
	28	import type { ConnectorInput } from "./connection";
	29
	30	const SECRET = "sk-live-poolkey-Zq7!x-SECRET";
	31	const OTHER_SECRET = "sk-live-poolkey-Zq7!x-ROTATED";
	32
	33	type RemoteInput = Extract<ConnectorInput, { readonly transport: "remote" }>;
	34
	35	const remoteInput = (overrides: Partial<RemoteInput> = {}): RemoteInput => ({
	36	transport: "remote",
	37	endpoint: "https://mcp.example.com/sse",
	38	remoteTransport: "streamable-http",
	39	headers: { authorization: `Bearer ${SECRET}` },
	40	...overrides,
	41	});
	42
	43	describe("MCP connection-pool key", () => {
	44	it.effect("is a bare SHA-256 digest — no plaintext rides along", () =>
	45	Effect.gen(function* () {
	46	const key = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
	47
	48	// Asserted positively as well as negatively: "does not contain the
	49	// secret" alone would still pass for a key that appended the digest to
	50	// the plaintext identity.
	51	expect(key).toMatch(/^[0-9a-f]{64}$/);
	52	expect(key).not.toContain(SECRET);
	53	expect(key).not.toContain(`Bearer ${SECRET}`);
	54	expect(key).not.toContain("mcp.example.com");
	55	}),
	56	);
	57
	58	it.effect("the same identity keeps producing the same key, so reuse is unchanged", () =>
	59	Effect.gen(function* () {
	60	const first = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
	61	const second = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
	62
	63	expect(first).toBe(second);
	64	}),
	65	);
	66
	67	it.effect("a rotated credential value produces a different key", () =>
	68	Effect.gen(function* () {
	69	// The case this field exists for: a refreshed access token must dial a
	70	// fresh session rather than reuse one authenticated with the old token.
	71	const before = yield* connectionPoolKey(remoteInput({ headers: {} }), "bearer", {
	72	token: SECRET,
	73	});
	74	const after = yield* connectionPoolKey(remoteInput({ headers: {} }), "bearer", {
	75	token: OTHER_SECRET,
	76	});
	77
	78	expect(after).not.toBe(before);
	79	}),
	80	);
	81
	82	it.effect("a different rendered auth header produces a different key", () =>
	83	Effect.gen(function* () {
	84	// `buildConnectorInput` renders apikey placements onto `headers`, so the
	85	// same secret reaches the key by a second route. Separation has to hold
	86	// there too.
	87	const mine = yield* connectionPoolKey(
	88	remoteInput({ headers: { authorization: `Bearer ${SECRET}` } }),
	89	"bearer",
	90	{},
	91	);
	92	const theirs = yield* connectionPoolKey(
	93	remoteInput({ headers: { authorization: `Bearer ${OTHER_SECRET}` } }),
	94	"bearer",
	95	{},
	96	);
	97
	98	expect(theirs).not.toBe(mine);
	99	}),
	100	);
	101
	102	it.effect("a credential carried in a query param separates too", () =>
	103	Effect.gen(function* () {
	104	// Servers that authenticate via `?token=` put the secret here instead.
	105	const mine = yield* connectionPoolKey(
	106	remoteInput({ headers: {}, queryParams: { token: SECRET } }),
	107	"query",
	108	{},
	109	);
	110	const theirs = yield* connectionPoolKey(
	111	remoteInput({ headers: {}, queryParams: { token: OTHER_SECRET } }),
	112	"query",
	113	{},
	114	);
	115
	116	expect(theirs).not.toBe(mine);
	117	expect(mine).not.toContain(SECRET);
	118	}),
	119	);
	120
	121	it.effect("insertion order does not split one identity into two", () =>
	122	Effect.gen(function* () {
	123	// `sortedRecord` exists for this: a key that changed with property order
	124	// would silently dial a new session per call and never reuse anything.
	125	const oneWay = yield* connectionPoolKey(
	126	remoteInput({ headers: { authorization: `Bearer ${SECRET}`, "x-team": "acme" } }),
	127	"bearer",
	128	{ token: SECRET, region: "eu" },
	129	);
	130	const otherWay = yield* connectionPoolKey(
	131	remoteInput({ headers: { "x-team": "acme", authorization: `Bearer ${SECRET}` } }),
	132	"bearer",
	133	{ region: "eu", token: SECRET },
	134	);
	135
	136	expect(otherWay).toBe(oneWay);
	137	}),
	138	);
	139
	140	it.effect("a different endpoint or template separates identities", () =>
	141	Effect.gen(function* () {
	142	const base = yield* connectionPoolKey(remoteInput(), "bearer", { token: SECRET });
	143	const otherEndpoint = yield* connectionPoolKey(
	144	remoteInput({ endpoint: "https://mcp.other.example.com/sse" }),
	145	"bearer",
	146	{ token: SECRET },
	147	);
	148	const otherTemplate = yield* connectionPoolKey(remoteInput(), "apikey", { token: SECRET });
	149
	150	expect(otherEndpoint).not.toBe(base);
	151	expect(otherTemplate).not.toBe(base);
	152	}),
	153	);
	154	});

packages/plugins/mcp/src/sdk/plugin.ts+42−12

   IntegrationSlug,
   mergeAuthTemplates,
   OAuthClientSlug,
+  sha256Hex,
   tool,
   ToolResult,
   type AuthMethodDescriptor,
       .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<ConnectorInput, { readonly transport: "remote" }>,
   template: string,
   values: Record<string, string | null>,
-): string =>
+): Effect.Effect<string> =>
-  JSON.stringify({
+  sha256Hex(
-    endpoint: input.endpoint,
+    JSON.stringify({
-    transport: input.transport,
+      endpoint: input.endpoint,
-    remoteTransport: input.remoteTransport,
+      transport: input.transport,
-    headers: sortedRecord(input.headers),
+      remoteTransport: input.remoteTransport,
-    queryParams: sortedRecord(input.queryParams),
+      headers: sortedRecord(input.headers),
-    template,
+      queryParams: sortedRecord(input.queryParams),
-    values: sortedRecord(values),
+      template,
-  });
+      values: sortedRecord(values),
+    }),
+  );
 // ---------------------------------------------------------------------------
 // Declared auth methods — project the stored MCP config into the catalog's
         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 = {