Bound and drain hand-rolled HTTP transport pools (#6495)

* Bound and drain hand-rolled HTTP transport pools

#6480 bounded and drained only the clients built by
networking.HttpClientBuilder.Build. Several hand-rolled
&http.Transport{} literals elsewhere left IdleConnTimeout at zero, so a
pooled idle connection never expired and a dropped client pinned a
socket plus its goroutine pair for the process lifetime. Separately,
several wrapping RoundTrippers did not implement CloseIdleConnections,
so http.Client.CloseIdleConnections type-asserted the outermost
transport, missed the method, and silently became a no-op.

Implements changes for issue #6483:
- Bound the transport literals in pkg/auth/discovery, pkg/auth/oauth,
  and pkg/oauthproto (discovery, dcr) with the same host-scoped pool
  bounds Build applies
- Add networking.ForwardCloseIdle so a wrapping RoundTripper forwards
  CloseIdleConnections without an ad-hoc anonymous assertion; use it
  from ValidatingTransport and the vmcp wrappers
- Forward CloseIdleConnections from bearerTokenTransport,
  registry/auth.Transport, and oauthproto.UserAgentTransport (the last
  inline, since pkg/oauthproto cannot import pkg/networking)

pkg/authz/authorizers/http builds no wrapper and uses http.DefaultTransport
(or its Clone), which is already bounded and forwards the call, so it needs
no change.

* Centralize idle-pool bounds and cover them with tests

Addresses #6495 review comments:
- MEDIUM pkg/oauthproto/dcr.go (3925404488): the 90s/100/4 pool values
  were duplicated across five transport literals with only a comment to
  keep them in sync. Add networking.SetIdleConnBounds as the single
  source (Build now uses it too); the three cycle-free sites call it, and
  the two oauthproto leaf sites reference a local const block since
  pkg/networking imports pkg/oauthproto.
- MEDIUM pkg/auth/discovery/discovery.go (3925404500): the two
  discovery.go transports and the oidc.go transport shipped untested.
  Extract each into a small package-private builder that calls
  SetIdleConnBounds, and assert the three bounds (and that blockPrivateIPs
  disables keep-alives) in unit tests.

Also add compile-time IdleConnectionCloser assertions to networking's own
ValidatingTransport and closeIdlerTransport wrappers.

* Forward and assert CloseIdleConnections on all wrappers

Addresses #6495 review comments:
- MEDIUM pkg/vmcp/client/client.go (3925404525): the session-backed twins
  authRoundTripper and identityRoundTripper in mcp_session.go implemented
  RoundTrip but not CloseIdleConnections, so they swallowed the drain the
  canonical twins forward. Their doc requires the invariant be kept in sync
  until #5333; mirror the forward with networking.ForwardCloseIdle.
- LOW pkg/registry/auth/transport.go (3925404536): none of the new
  forwarding wrappers carried a compile-time assertion, so a rename or typo
  of CloseIdleConnections would compile and re-hide the pool. Add
  var _ networking.IdleConnectionCloser assertions to the wrappers
  (registry/auth.Transport, bearerTokenTransport, the vmcp wrappers, and
  the twins); UserAgentTransport asserts the local shape since pkg/oauthproto
  cannot import pkg/networking.

* Strengthen UserAgentTransport drain tests

Addresses #6495 review comments:
- MEDIUM pkg/oauthproto/useragent_test.go (3925404517): the nil-Base test
  claimed to verify the DefaultTransport fallback forwards, but a type
  assertion on a nil interface returns ok==false without panicking, so the
  test would still pass if the guard were removed. Soften the comment to
  state accurately that it only checks the nil-Base path does not panic.
- LOW pkg/oauthproto/useragent_test.go (3925404540): the ok==false arm
  (a non-nil Base lacking CloseIdleConnections) was never exercised. Add a
  plainRoundTripper sub-case asserting a safe no-op.

* Fix codespell: keep-alives -> keep-alive

Codespell flags the plural "keep-alives"; the rest of the tree uses the
singular. Reword the comments and test messages added in this branch to
match, unblocking the Spellcheck CI job.

Author: tgrunnagle

pkg/auth/dcr/resolver.go

@@ -1401,6 +1401,11 @@ type bearerTokenTransport struct {
 	next  http.RoundTripper
 }
 
+// Compile-time assertion: a rename or typo of CloseIdleConnections would
+// otherwise silently make http.Client.CloseIdleConnections a no-op on any
+// client using this transport (see networking.IdleConnectionCloser).
+var _ networking.IdleConnectionCloser = (*bearerTokenTransport)(nil)
+
 // RoundTrip implements http.RoundTripper.
 func (t *bearerTokenTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	// Clone per http.RoundTripper contract: implementations must not modify
@@ -1410,6 +1415,13 @@ func (t *bearerTokenTransport) RoundTrip(req *http.Request) (*http.Response, err
 	return t.next.RoundTrip(cp)
 }
 
+// CloseIdleConnections forwards to the wrapped RoundTripper so
+// http.Client.CloseIdleConnections reaches the underlying connection pool
+// instead of stopping at this wrapper (see networking.IdleConnectionCloser).
+func (t *bearerTokenTransport) CloseIdleConnections() {
+	networking.ForwardCloseIdle(t.next)
+}
+
 // errDCRRedirectRefused is returned when a DCR registration endpoint
 // responds with a 30x. Net/http surfaces it via *url.Error so callers
 // observe a clear failure mode instead of a confusing JSON decode error.

pkg/auth/dcr/resolver_test.go

@@ -2363,3 +2363,25 @@ func TestResolveDCRCredentials_MetadataSelfNamedRegistrationEndpointAllowed(t *t
 	require.NotNil(t, res)
 	assert.NotEmpty(t, res.ClientID)
 }
+
+// closeIdleSpy records CloseIdleConnections calls; RoundTrip exists only to
+// satisfy http.RoundTripper.
+type closeIdleSpy struct{ closed int }
+
+func (*closeIdleSpy) RoundTrip(*http.Request) (*http.Response, error) {
+	return nil, errors.New("unused")
+}
+func (s *closeIdleSpy) CloseIdleConnections() { s.closed++ }
+
+// TestBearerTokenTransport_CloseIdleConnections verifies the wrapper forwards
+// CloseIdleConnections to its next RoundTripper rather than silently swallowing
+// it, which would make http.Client.CloseIdleConnections a no-op and leave the
+// pool pinned.
+func TestBearerTokenTransport_CloseIdleConnections(t *testing.T) {
+	t.Parallel()
+
+	spy := &closeIdleSpy{}
+	tr := &bearerTokenTransport{token: "t", next: spy}
+	tr.CloseIdleConnections()
+	assert.Equal(t, 1, spy.closed)
+}

pkg/auth/discovery/discovery.go

@@ -79,6 +79,24 @@ func DefaultDiscoveryConfig() *Config {
 	}
 }
 
+// newDetectionClient builds the HTTP client used to probe a target server for
+// WWW-Authenticate. The remote MCP server is untrusted, so it refuses
+// cross-host / scheme-downgrade redirects to prevent the server driving the
+// host into an SSRF (CWE-918). Extracted so the pool bounds
+// networking.SetIdleConnBounds applies stay unit-testable.
+func newDetectionClient(config *Config) *http.Client {
+	transport := &http.Transport{
+		TLSHandshakeTimeout:   config.TLSHandshakeTimeout,
+		ResponseHeaderTimeout: config.ResponseHeaderTimeout,
+	}
+	networking.SetIdleConnBounds(transport)
+	return &http.Client{
+		Timeout:       config.Timeout,
+		Transport:     transport,
+		CheckRedirect: networking.SameHostRedirectPolicy(),
+	}
+}
+
 // DetectAuthenticationFromServer attempts to detect authentication requirements from the target server
 func DetectAuthenticationFromServer(ctx context.Context, targetURI string, config *Config) (*AuthInfo, error) {
 	if config == nil {
@@ -90,16 +108,7 @@ func DetectAuthenticationFromServer(ctx context.Context, targetURI string, confi
 	defer cancel()
 
 	// Make a test request to the target server to see if it returns WWW-Authenticate.
-	// The remote MCP server is untrusted, so refuse cross-host / scheme-downgrade
-	// redirects to prevent it driving the host into an SSRF (CWE-918).
-	client := &http.Client{
-		Timeout: config.Timeout,
-		Transport: &http.Transport{
-			TLSHandshakeTimeout:   config.TLSHandshakeTimeout,
-			ResponseHeaderTimeout: config.ResponseHeaderTimeout,
-		},
-		CheckRedirect: networking.SameHostRedirectPolicy(),
-	}
+	client := newDetectionClient(config)
 
 	// First try a GET request
 	authInfo, err := detectAuthWithRequest(detectCtx, client, targetURI, http.MethodGet, nil)
@@ -1020,6 +1029,26 @@ func buildOAuthFlowResult(
 	}
 }
 
+// newResourceMetadataTransport builds the transport for RFC 9728 metadata
+// fetches. The metadataURL is server-supplied, so the caller pairs it with a
+// same-host redirect policy; when blockPrivateIPs is true it also refuses to
+// dial private/loopback/link-local addresses on every hop (and disables
+// keep-alive so a pooled connection cannot skip the per-dial check).
+// Extracted so the pool bounds networking.SetIdleConnBounds applies stay
+// unit-testable.
+func newResourceMetadataTransport(blockPrivateIPs bool) *http.Transport {
+	transport := &http.Transport{
+		TLSHandshakeTimeout:   5 * time.Second,
+		ResponseHeaderTimeout: 5 * time.Second,
+	}
+	networking.SetIdleConnBounds(transport)
+	if blockPrivateIPs {
+		transport.DialContext = networking.NewPrivateIPBlockingDialContext()
+		transport.DisableKeepAlives = true
+	}
+	return transport
+}
+
 // FetchResourceMetadata fetches RFC 9728 protected-resource metadata from a
 // server-supplied URL.
 //
@@ -1046,17 +1075,7 @@ func FetchResourceMetadata(ctx context.Context, metadataURL string, blockPrivate
 		return nil, fmt.Errorf("metadata URL must use HTTPS: %s", metadataURL)
 	}
 
-	// The HTTPS check above runs once on the initial URL only, so refuse
-	// cross-host and scheme-downgrade redirects to stop a 30x reaching an
-	// internal address; optionally block private dials on every hop.
-	transport := &http.Transport{
-		TLSHandshakeTimeout:   5 * time.Second,
-		ResponseHeaderTimeout: 5 * time.Second,
-	}
-	if blockPrivateIPs {
-		transport.DialContext = networking.NewPrivateIPBlockingDialContext()
-		transport.DisableKeepAlives = true
-	}
+	transport := newResourceMetadataTransport(blockPrivateIPs)
 	client := &http.Client{
 		Timeout:       DefaultHTTPTimeout,
 		Transport:     transport,

pkg/auth/discovery/discovery_test.go

@@ -1474,3 +1474,36 @@ func TestCreateOAuthConfig_DiscoveredTokenEndpoint(t *testing.T) {
 		})
 	}
 }
+
+// TestNewDetectionClient_BoundsIdleConnectionPool pins that the auth-detection
+// client bounds its idle-connection pool. This is one of the exact sites #6483
+// names; a zero IdleConnTimeout would pin a socket and its goroutine pair for
+// the process lifetime.
+func TestNewDetectionClient_BoundsIdleConnectionPool(t *testing.T) {
+	t.Parallel()
+
+	client := newDetectionClient(DefaultDiscoveryConfig())
+	transport, ok := client.Transport.(*http.Transport)
+	require.True(t, ok, "detection client must use an *http.Transport")
+
+	assert.Equal(t, 90*time.Second, transport.IdleConnTimeout)
+	assert.Equal(t, 100, transport.MaxIdleConns)
+	assert.Equal(t, 4, transport.MaxIdleConnsPerHost)
+}
+
+// TestNewResourceMetadataTransport_BoundsIdleConnectionPool pins that the RFC
+// 9728 metadata transport bounds its pool in both dial modes. The bounds are
+// set regardless of blockPrivateIPs (they are moot only when keep-alive is
+// disabled, which is a separate field).
+func TestNewResourceMetadataTransport_BoundsIdleConnectionPool(t *testing.T) {
+	t.Parallel()
+
+	for _, blockPrivateIPs := range []bool{false, true} {
+		transport := newResourceMetadataTransport(blockPrivateIPs)
+		assert.Equal(t, 90*time.Second, transport.IdleConnTimeout)
+		assert.Equal(t, 100, transport.MaxIdleConns)
+		assert.Equal(t, 4, transport.MaxIdleConnsPerHost)
+		assert.Equal(t, blockPrivateIPs, transport.DisableKeepAlives,
+			"blockPrivateIPs must disable keep-alive so pooling cannot skip the per-dial SSRF check")
+	}
+}

pkg/auth/oauth/oidc.go

@@ -64,6 +64,26 @@ func discoverOIDCEndpointsWithClient(
 	return discoverOIDCEndpointsWithClientAndValidation(ctx, issuer, client, true, insecureAllowHTTP, blockPrivateIPs)
 }
 
+// newOIDCDiscoveryTransport builds the transport used to fetch OIDC/OAuth
+// discovery documents from an untrusted, remote-server-supplied issuer. The
+// caller pairs it with a same-host redirect policy to prevent a 30x driving the
+// host into an SSRF (CWE-918); when blockPrivateIPs is true it also refuses to
+// dial private/loopback/link-local addresses on every hop (and disables
+// keep-alive so a pooled connection cannot skip the per-dial check). Extracted
+// so the pool bounds networking.SetIdleConnBounds applies stay unit-testable.
+func newOIDCDiscoveryTransport(blockPrivateIPs bool) *http.Transport {
+	transport := &http.Transport{
+		TLSHandshakeTimeout:   10 * time.Second,
+		ResponseHeaderTimeout: 10 * time.Second,
+	}
+	networking.SetIdleConnBounds(transport)
+	if blockPrivateIPs {
+		transport.DialContext = networking.NewPrivateIPBlockingDialContext()
+		transport.DisableKeepAlives = true
+	}
+	return transport
+}
+
 // discoverOIDCEndpointsWithClientAndValidation discovers OAuth endpoints with optional issuer validation
 //
 //nolint:gocyclo // Function complexity justified by comprehensive OIDC discovery logic
@@ -82,21 +102,9 @@ func discoverOIDCEndpointsWithClientAndValidation(
 	}
 
 	if client == nil {
-		// The issuer/metadata URL originates from untrusted remote-server
-		// discovery, so refuse cross-host and scheme-downgrade redirects to
-		// prevent a 30x from driving the host into an SSRF (CWE-918), and
-		// optionally block private dials on every hop.
-		transport := &http.Transport{
-			TLSHandshakeTimeout:   10 * time.Second,
-			ResponseHeaderTimeout: 10 * time.Second,
-		}
-		if blockPrivateIPs {
-			transport.DialContext = networking.NewPrivateIPBlockingDialContext()
-			transport.DisableKeepAlives = true
-		}
 		client = &http.Client{
 			Timeout:       30 * time.Second,
-			Transport:     transport,
+			Transport:     newOIDCDiscoveryTransport(blockPrivateIPs),
 			CheckRedirect: networking.SameHostRedirectPolicy(),
 		}
 	}

pkg/auth/oauth/oidc_test.go

@@ -1532,3 +1532,20 @@ func TestBuildWellKnownURLs(t *testing.T) {
 		})
 	}
 }
+
+// TestNewOIDCDiscoveryTransport_BoundsIdleConnectionPool pins that the OIDC
+// discovery transport bounds its pool in both dial modes. This is one of the
+// exact sites #6483 names; a zero IdleConnTimeout would pin a socket and its
+// goroutine pair for the process lifetime.
+func TestNewOIDCDiscoveryTransport_BoundsIdleConnectionPool(t *testing.T) {
+	t.Parallel()
+
+	for _, blockPrivateIPs := range []bool{false, true} {
+		transport := newOIDCDiscoveryTransport(blockPrivateIPs)
+		assert.Equal(t, 90*time.Second, transport.IdleConnTimeout)
+		assert.Equal(t, 100, transport.MaxIdleConns)
+		assert.Equal(t, 4, transport.MaxIdleConnsPerHost)
+		assert.Equal(t, blockPrivateIPs, transport.DisableKeepAlives,
+			"blockPrivateIPs must disable keep-alive so pooling cannot skip the per-dial SSRF check")
+	}
+}

pkg/networking/http_client.go

@@ -137,12 +137,48 @@ type IdleConnectionCloser interface {
 	CloseIdleConnections()
 }
 
+// SetIdleConnBounds applies Build's idle-connection-pool bounds to a
+// hand-constructed *http.Transport — the transports this package cannot build
+// through Build because they need their own DialContext, CheckRedirect, or TLS
+// config. A zero IdleConnTimeout never expires a pooled connection, so a client
+// that performs one request and is dropped pins a socket and its goroutine pair
+// for the process lifetime; this is the leak Build fixes for its own clients
+// (see the constant block above). Setting all three fields together keeps a
+// call site from bounding two of the three by mistake.
+//
+// pkg/networking imports pkg/oauthproto, so leaf packages that oauthproto's
+// import graph reaches cannot call this without a cycle; those mirror the
+// values with a local const block instead.
+func SetIdleConnBounds(t *http.Transport) {
+	t.IdleConnTimeout = idleConnTimeout
+	t.MaxIdleConns = maxIdleConns
+	t.MaxIdleConnsPerHost = maxIdleConnsPerHost
+}
+
+// ForwardCloseIdle drains the idle-connection pool reachable through rt when rt
+// implements IdleConnectionCloser — as *http.Transport, and every wrapping
+// RoundTripper in this repo that forwards the call, do. A wrapping RoundTripper
+// must call this from its own CloseIdleConnections; otherwise
+// http.Client.CloseIdleConnections stops at the wrapper and the pool underneath
+// is never drained (see IdleConnectionCloser). Using this helper keeps the
+// wrapper half from silently regressing into an anonymous
+// `interface{ CloseIdleConnections() }` assertion that drifts out of sync.
+func ForwardCloseIdle(rt http.RoundTripper) {
+	if closer, ok := rt.(IdleConnectionCloser); ok {
+		closer.CloseIdleConnections()
+	}
+}
+
 // ValidatingTransport is for validating URLs prior to request
 type ValidatingTransport struct {
 	Transport         http.RoundTripper
 	InsecureAllowHTTP bool
 }
 
+// Compile-time assertion: a rename or typo of CloseIdleConnections would
+// otherwise silently re-hide the pool it wraps (see IdleConnectionCloser).
+var _ IdleConnectionCloser = (*ValidatingTransport)(nil)
+
 // RoundTrip validates the request URL prior to forwarding
 func (t *ValidatingTransport) RoundTrip(req *http.Request) (*http.Response, error) {
 	// Skip validation if INSECURE_DISABLE_URL_VALIDATION is set or if InsecureAllowHTTP is true
@@ -172,9 +208,7 @@ func (t *ValidatingTransport) RoundTrip(req *http.Request) (*http.Response, erro
 // IdleConnectionCloser. The assertion is required because the field is an
 // http.RoundTripper; it holds an *http.Transport for every client Build produces.
 func (t *ValidatingTransport) CloseIdleConnections() {
-	if closer, ok := t.Transport.(IdleConnectionCloser); ok {
-		closer.CloseIdleConnections()
-	}
+	ForwardCloseIdle(t.Transport)
 }
 
 // closeIdlerTransport wraps a RoundTripper that does not implement
@@ -185,6 +219,8 @@ type closeIdlerTransport struct {
 	pool *http.Transport
 }
 
+var _ IdleConnectionCloser = (*closeIdlerTransport)(nil)
+
 // CloseIdleConnections closes the idle connections held by the underlying pool.
 func (t *closeIdlerTransport) CloseIdleConnections() {
 	t.pool.CloseIdleConnections()
@@ -335,10 +371,8 @@ func (b *HttpClientBuilder) Build() (*http.Client, error) {
 	transport := &http.Transport{
 		TLSHandshakeTimeout:   b.tlsHandshakeTimeout,
 		ResponseHeaderTimeout: b.responseHeaderTimeout,
-		IdleConnTimeout:       idleConnTimeout,
-		MaxIdleConns:          maxIdleConns,
-		MaxIdleConnsPerHost:   maxIdleConnsPerHost,
 	}
+	SetIdleConnBounds(transport)
 	transport.DisableKeepAlives = b.disableKeepAlives
 
 	if !b.allowPrivate {

pkg/networking/http_client_test.go

@@ -11,6 +11,7 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/pem"
+	"errors"
 	"io"
 	"math/big"
 	"net/http"
@@ -954,6 +955,20 @@ func TestBuild_BoundsIdleConnectionPool(t *testing.T) {
 	assert.Equal(t, 4, transport.MaxIdleConnsPerHost)
 }
 
+// TestSetIdleConnBounds pins the pool bounds the helper applies. These are the
+// single source of truth the hand-rolled transports outside Build mirror, so a
+// retune here must be a deliberate, visible change.
+func TestSetIdleConnBounds(t *testing.T) {
+	t.Parallel()
+
+	transport := &http.Transport{}
+	SetIdleConnBounds(transport)
+
+	assert.Equal(t, 90*time.Second, transport.IdleConnTimeout)
+	assert.Equal(t, 100, transport.MaxIdleConns)
+	assert.Equal(t, 4, transport.MaxIdleConnsPerHost)
+}
+
 // TestBuild_CloseIdleConnectionsReachesPool pins that
 // http.Client.CloseIdleConnections is not a silent no-op on a built client. The
 // client discovers the capability by asserting the outermost transport, so every
@@ -999,6 +1014,38 @@ func TestBuild_CloseIdleConnectionsReachesPool(t *testing.T) {
 	}
 }
 
+// forwardCloseIdleSpy records CloseIdleConnections calls; the RoundTrip method
+// exists only to satisfy http.RoundTripper.
+type forwardCloseIdleSpy struct{ closed int }
+
+func (*forwardCloseIdleSpy) RoundTrip(*http.Request) (*http.Response, error) {
+	return nil, errors.New("unused")
+}
+func (s *forwardCloseIdleSpy) CloseIdleConnections() { s.closed++ }
+
+// plainRoundTripper implements http.RoundTripper but not IdleConnectionCloser.
+type plainRoundTripper struct{}
+
+func (plainRoundTripper) RoundTrip(*http.Request) (*http.Response, error) {
+	return nil, errors.New("unused")
+}
+
+func TestForwardCloseIdle(t *testing.T) {
+	t.Parallel()
+
+	t.Run("forwards to a RoundTripper that implements the capability", func(t *testing.T) {
+		t.Parallel()
+		spy := &forwardCloseIdleSpy{}
+		ForwardCloseIdle(spy)
+		assert.Equal(t, 1, spy.closed)
+	})
+
+	t.Run("is a safe no-op when the RoundTripper does not implement it", func(t *testing.T) {
+		t.Parallel()
+		assert.NotPanics(t, func() { ForwardCloseIdle(plainRoundTripper{}) })
+	})
+}
+
 // getReusedConn issues a GET and reports whether it was served from the
 // client's idle connection pool.
 func getReusedConn(t *testing.T, client *http.Client, target string) bool {