Add dial-control option to vMCP session factory - #6547
Conversation
Session-init backend connections opened by MultiSessionFactory used http.DefaultTransport with no net.Dialer.Control hook, and NewSessionFactory exposed no way to inject one -- unlike the per-call backend client (pkg/vmcp/client), which already offers WithDialControl. An embedder guarding backend-client dials against SSRF / DNS-rebinding therefore could not guard the session-init dials. Implements changes for issue #6545: - Add session.WithDialControl MultiSessionFactoryOption, wired into NewSessionFactory - Add backend.WithDialControl HTTPConnectorOption and a backendBaseTransport helper; thread the hook through createMCPClient for both streamable-http and sse transports - A nil control returns http.DefaultTransport unchanged, keeping the dial path byte-for-byte identical - Tests: a deny-all control yields zero backend requests at session init for both transports; a nil control leaves DefaultTransport untouched Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address code review: the dial-control base transport was duplicated between pkg/vmcp/client and the new session connector, kept in sync only by a comment (review M1), risking drift of the shared dial timeouts and clone logic. - Add networking.CloneDefaultTransportWithDialControl as the single construction point for the vMCP backend transport (clone-or-reconstruct plus the optional dial-control dialer) - Have both pkg/vmcp/client.newBackendTransport and the session connector's backendBaseTransport call it; delete the now-redundant backendDialer in the client - Document the pooled-connection safety argument in the session option doc for parity with the client option (review L1) - Cover the shared helper directly in pkg/networking Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6547 +/- ##
==========================================
+ Coverage 78.69% 78.71% +0.02%
==========================================
Files 777 777
Lines 76797 76834 +37
==========================================
+ Hits 60432 60483 +51
+ Misses 16360 16346 -14
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tgrunnagle
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: security-reviewer, go-correctness-reviewer, test-coverage-reviewer, general-code-quality-reviewer
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | WithDialControl caveat docs drop the OWASP citation + re-classification sentence present in the original |
9/10 | MEDIUM | Fix |
| 2 | Guarded-dial tests prove "error + zero hits" without confirming the specific hook fired | 8/10 | MEDIUM | Fix |
| 3 | RestoreSession's dial-control wiring has no direct test |
8/10 | MEDIUM | Fix |
| 4 | createMCPClient's adjacent nil-typed positional params risk future transposition |
7/10 | MEDIUM | Fix |
| 5 | CloneDefaultTransportWithDialControl's doc names vMCP specifically inside a generic package |
7/10 | MEDIUM | Fix |
| 6 | Reconstructed-transport fallback branch (DefaultTransport replaced) is entirely untested | 7/10 | MEDIUM | Fix |
| 7 | No multi-backend test for mixed allow/block dial control | 7/10 | MEDIUM | Fix |
Overall
This PR closes a real coverage gap: session-init backend dials in pkg/vmcp/session previously had no way to install a net.Dialer.Control hook, so an SSRF/DNS-rebinding guard installed on the per-call client (pkg/vmcp/client) did not cover the session-init handshake. The approach is sound — a single shared transport constructor (networking.CloneDefaultTransportWithDialControl) is now used by both the per-call client and the persistent session connector, the hook is verified to fire for both streamable-http and sse (both branches build off the same base transport before the transport-type switch), and the no-hook path is deliberately pinned to be byte-for-byte identical to before (assert.Same(t, http.DefaultTransport, backendBaseTransport(nil), ...)).
The findings below are all polish and rigor items, not correctness defects. The biggest is that the new caveat documentation drops two sentences (the OWASP SSRF Prevention Cheat Sheet citation and the "no per-request re-classification" clarification) present in the pkg/vmcp/client.WithDialControl original it's meant to mirror — worth fixing since the closed issue's acceptance criteria call for the same caveat documentation. The rest are test-coverage gaps: the new guarded/unguarded tests prove "error + zero backend hits" without confirming the specific hook fired (an unrelated failure earlier in the chain would look identical), RestoreSession's dial-control wiring and a mixed allow/block multi-backend scenario are both untested, and the http.DefaultTransport-replaced fallback branch in the new shared transport helper has no coverage at all.
None of this should block merge. Build and the full affected-package test suite pass, lint is clean on every touched file (two pre-existing gci failures in pkg/authserver predate this branch and are unrelated to this diff), and the change stays well inside the repo's PR-size budget.
Documentation
pkg/vmcp/session/internal/backend/mcp_session.go:85-97—WithDialControl's caveat doc drops the OWASP SSRF Prevention Cheat Sheet citation and the "no per-request re-classification" sentence present inpkg/vmcp/client.WithDialControl(see inline comment for a suggested fix).pkg/networking/http_client.go:151-153—CloneDefaultTransportWithDialControl's doc names vMCP specifically inside a generic, dependency-free package (see inline comment for a suggested rewording).
Generated with Claude Code
Addresses #6547 review comments: - MEDIUM mcp_session.go (3960204428): fold createMCPClient's trailing option params (sink, dialControl, requestTimeout) into an mcpClientParams struct so the two adjacent nil-able func members cannot be silently transposed at call sites - MEDIUM mcp_session.go (3960204343): restore the "no per-request re-classification" sentence and the OWASP SSRF Cheat Sheet citation in WithDialControl's caveat docs, for parity with pkg/vmcp/client - MEDIUM http_client.go (3960204434): reword CloneDefaultTransportWithDialControl's doc generically instead of naming vMCP inside the dependency-free package Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses #6547 review comments: - MEDIUM mcp_session_dialcontrol_test.go (3960204415): record that the dial-control hook fired (atomic.Bool) so an unrelated earlier failure cannot pass the guarded test for the wrong reason - MEDIUM factory_dialcontrol_test.go (3960204421): add a RestoreSession dial-control test, covering its distinct filtering/identity-binding path - MEDIUM factory_dialcontrol_test.go (3960204451): add a two-backend test proving the control is applied per-backend (one blocked, one allowed) - MEDIUM backend_transport_test.go (3960204445): cover the reconstruction branch taken when http.DefaultTransport is not a *http.Transport, via a non-parallel test that saves/restores the global Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jhrozek
left a comment
There was a problem hiding this comment.
Reviewed with parallel code-quality, security, and architecture passes.
Clean, well-scoped refactor: centralizes backend HTTP transport construction (previously duplicated between pkg/vmcp/client and pkg/vmcp/session/internal/backend) into networking.CloneDefaultTransportWithDialControl, and threads an optional dial-control hook through the vMCP session factory. Constants preserved exactly, option-application order is correct, and the new tests actually assert the hook fires rather than just checking for an error.
One thing worth tracking as a follow-up (non-blocking): the new WithDialControl option isn't wired to any real policy in production yet (no caller in serve.go passes it), same as the existing client-side WithDialControl from #5551. Not a regression, just worth an issue so the SSRF-blocking policy actually gets connected at some point.
Approving.
) * Generalize vMCP session dial-control into a per-workload resolver PR #6547 added session.WithDialControl / backend.WithDialControl: a single, address-blind net.Dialer.Control hook applied to every backend's session-init dials. Because the hook receives only (network, address, RawConn), an embedder cannot vary the dial policy per backend. Enterprise connector-gateway needs a per-backend dial policy at session init (some backends opt into private-IP dialing, most don't), which the single hook cannot express. #6547 is merged but not yet in any release tag, so this generalizes that option in place rather than adding a second one alongside — one option, not two. - Replace session.WithDialControl with WithDialControlResolver, a func(workloadID string) func(network, address string, c syscall.RawConn) error, mirroring the sibling WithRevisionLookup / WithRequestTimeoutResolver options. - Replace backend.WithDialControl (HTTPConnectorOption) likewise; the connector resolves the per-workload hook in NewHTTPConnector's closure (where the target is known) and threads the already-resolved hook through mcpClientParams, so createMCPClient and backendBaseTransport are unchanged. - Preserve the no-hook invariant exactly: a nil resolver, or a resolver that returns nil for a workload, leaves that backend's transport on http.DefaultTransport — byte-for-byte identical to the no-hook path. Required by enterprise connector-gateway (stacklok/stacklok-enterprise-platform#3313). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Clarify WithDialControlResolver docs and align param name Addresses #6567 review comments: - MEDIUM mcp_session.go (3970572745): document that the resolver only selects a hook; the returned hook must inspect the resolved address or it gives no SSRF/DNS-rebinding protection (mirrored in factory.go) - MEDIUM mcp_session.go (3970572769): scope the client.WithDialControl "twin"/"counterpart" framing to the returned hook's signature and note the client option is not yet per-backend (mirrored in factory.go) - LOW mcp_session.go / factory.go (3970572781): rename the option parameter resolve -> resolver to match the sibling WithRequestTimeoutResolver * Isolate a panicking dial-control resolver; cover restore + concurrency Addresses #6567 review comments: - MEDIUM mcp_session.go (3970572761): recover a panicking per-workload resolver via resolveDialControl so it is isolated to that backend (excluded like any init failure) instead of crashing the per-backend init goroutine and the process; covered by a factory test proving the surviving backend still connects - MEDIUM factory_dialcontrol_test.go (3970572752): add a RestoreSession per-backend-keying test (deny one of two workloads; assert the allowed one survives restore and the denied one is excluded) - LOW factory_dialcontrol_test.go (3970572776): add a -race barrier test proving the resolver is invoked concurrently across the per-backend init goroutines --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
The vMCP
MultiSessionFactoryopens a persistent connection to every backendduring session
initialize(the MCP handshake plus capability listing), butthose session-init dials used
http.DefaultTransportwith nonet.Dialer.Controlhook, andNewSessionFactoryexposed no way to injectone. This is asymmetric with the per-call backend client in
pkg/vmcp/client,which already offers
WithDialControl. As a result, an embedder that installsa dial guard on the backend client (for example, an SSRF / DNS-rebinding guard
that refuses dials resolving into private IP ranges) covered the aggregation
and tool-call paths but could not cover the session-init dials — so
initializewould still dial and handshake against an operator- orattacker-influenceable backend endpoint.
session.WithDialControl(MultiSessionFactoryOption) and itscounterpart
backend.WithDialControl(HTTPConnectorOption), threadedthrough
createMCPClientso the hook fires on session-init dials for boththe
streamable-httpandssetransports, on both theMakeSessionWithIDand
RestoreSessionpaths. The signature matchesnet.Dialer.Controlandvmcpclient.WithDialControlexactly and carries the same security caveats(per-TCP-dial not per-request, proxy transparency, both IP families, OWASP
range list).
networking.CloneDefaultTransportWithDialControlas the single sharedconstruction point used by both the per-call client and the session
connector, removing the duplicated clone-or-reconstruct + dialer logic that
previously lived in
pkg/vmcp/client.createMCPClient's option parameters into anmcpClientParamsstruct so the two adjacent, frequently-nil func members (
sink,dialControl) cannot be silently transposed at a call site.path byte-for-byte identical to
http.DefaultTransport— the sessionconnector returns
http.DefaultTransportunchanged when no hook is set.Closes #6545
Type of change
Test plan
task test)Ran the affected packages (
pkg/networking,pkg/vmcp/client,pkg/vmcp/session, andpkg/vmcp/session/internal/backend) with the Taskfileflags. Coverage added:
initializefor both thestreamable-httpandssetransports, and thecontrol's hook is asserted to have actually fired (so an unrelated earlier
failure cannot pass the test for the wrong reason).
RestoreSessionpath (its distinctfiltering / identity-binding restore logic).
the control is applied per-backend: the allowed backend connects and the
blocked one is excluded.
http.DefaultTransportunchanged.networking.CloneDefaultTransportWithDialControlhelper is covereddirectly, including the reconstruction branch taken when
http.DefaultTransporthas been replaced by a non-
*http.Transport.Changes
pkg/networking/http_client.goCloneDefaultTransportWithDialControl, the single shared backend-transport construction point (clone-or-reconstruct + optional dial-control dialer).pkg/networking/backend_transport_test.goDefaultTransport-replaced reconstruction branch.pkg/vmcp/client/client.gobackendDialer/ transport construction with the shared helper.pkg/vmcp/session/factory.goWithDialControlfactory option; thread it into the HTTP connector.pkg/vmcp/session/factory_dialcontrol_test.goMakeSessionWithID,RestoreSession, and the mixed per-backend allow/block scenario.pkg/vmcp/session/internal/backend/mcp_session.goWithDialControlconnector option andbackendBaseTransport; groupcreateMCPClient's option params intomcpClientParams; thread the hook through for streamable-http and sse.pkg/vmcp/session/internal/backend/mcp_session_dialcontrol_test.gopkg/vmcp/session/internal/backend/mcp_session_test.gocreateMCPClientsignature.Does this introduce a user-facing change?
No end-user behavior changes by default. This adds an opt-in embedder API: a
new
session.WithDialControloption that lets a deployment enforce aper-connection dial policy on the connections opened at vMCP session
initialization, closing a gap relative to the existing backend-client guard.
Special notes for reviewers
backendBaseTransport(nil)returnshttp.DefaultTransportdirectly ratherthan a clone, so nothing changes for deployments that do not opt in.
(
pkg/networking), so the client and session-connector dial paths can nolonger drift.
http.DefaultTransportand is therefore non-parallel (restored viat.Cleanup); it carries a justified//nolint:paralleltest.option into
pkg/vmcp/cli/serve.goso a production deployment can opt into aguard. This PR adds the plumbing; the CLI wiring can land separately.
Generated with Claude Code