fix(proxy): unpinned upstream servers pooled behind one shared HTTP client - #587
fix(proxy): unpinned upstream servers pooled behind one shared HTTP client#587rajnisht7 wants to merge 5 commits into
Conversation
Signed-off-by: rajnisht7 <rajnishtiwari9787@gmail.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
imran-siddique
left a comment
There was a problem hiding this comment.
The fix is right and the reasoning in the docstring is the part I want kept: two entries sharing a real pinned fingerprint are the same verified peer and should share a client; two that merely share the property of being unpinned are not, and pooling them hands one dev upstream another's cookie jar and connection-pool budget. _server_execution_key already existed in this file for exactly this, and _stdio_for already used it, so this was the one caller out of step.
The conflict is mine, not yours. I merged #587's sibling, #589, a few minutes ago and both append to CHANGELOG.md. Apologies for the ordering.
I ran the merge locally so you do not have to discover the shape of it:
- Only
CHANGELOG.mdconflicts.src/cmcp_runtime/mcp/proxy.pyandtests/unit/test_mcp_proxy.pymerge clean. - The conflict is your two entries landing adjacent. Keep both, yours above the
tools/callone. Nothing needs rewording. - Resolved that way the branch is
+81/-3across the three files, which is your diff unchanged.
So: merge origin/main (currently 92c22e2bd02c) into cmcp-tls, keep both CHANGELOG entries, push. I will merge on the green.
One non-blocking observation for whenever you touch this again. _server_execution_key returns a tuple, so f"unpinned:{identity}" is formatting a tuple through its repr. It is deterministic and unique per server, so it is correct, but it does lean on tuple repr staying stable, where _stdio_for uses the tuple itself as the key. If _http_clients ever widens its key type, keying on the tuple directly would match the pattern more exactly. Not worth a round trip on its own.
What
CMCPProxy._client_for_upstream() pooled every unpinned upstream MCP server in a session behind a single shared httpx.AsyncClient same cookie jar, same connection-pool budget regardless of how unrelated those servers actually were. Now keys the client cache on _server_execution_key(entry) for both unpinned branches, matching the pattern _stdio_for() already uses for spawned children.
Why
The pinned branch already keys its cached client on the TLS fingerprint (correct: a matching pin is the same verified peer). Both unpinned branches (http://, and the PLACEHOLDER_FINGERPRINT dev-mode pin) instead collapsed to the literal string "unpinned". A session whose catalog lists two or more unrelated unpinned upstreams the supported, if discouraged, dev/demo path this method's own docstring describes and got exactly one httpx.AsyncClient for all of them.
_server_execution_key(entry) already exists in this exact file for precisely this purpose ("the security-relevant identity used to pool one upstream"), and _stdio_for() already keys spawned stdio children on it, _client_for_upstream() was the one caller that didn't follow the established pattern.
Reproduced before fixing: two CatalogEntry objects for different hosts, both PLACEHOLDER_FINGERPRINT, returned object-identical AsyncClient instances (and cookie jars) from _client_for_upstream().
Security impact
Two or more distinct, unpinned upstream MCP servers configured in the same gateway session shared one httpx.AsyncClient, and therefore one cookie jar and one connection-pool budget. This affects only the already-lower-trust "unpinned" path (plain HTTP, or the placeholder-pin dev mode) pinned upstreams were never affected, since a matching TLS fingerprint correctly identifies the same peer regardless of URL. Within that unpinned path: a cookie set by one unpinned upstream could be sent to another if their domains overlapped under standard cookie-jar domain-matching rules, and a heavily-loaded or slow unpinned upstream could exhaust the shared connection pool, delaying or blocking requests to an unrelated unpinned upstream in the same session (a cross-tenant resource-starvation/isolation gap between untrusted servers a session operator explicitly chose to run without pinning).
Test plan
pytestpassesruff checkpassesmypypassesDCO sign-off
Developer Certificate of Origin (https://developercertificate.org).