From 895ca87c6902a4c398896a1a8cdbd49ca8361678 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 15 Jul 2026 20:45:45 -0700 Subject: [PATCH 1/7] Guard upstream-DCR HTTP calls against SSRF The DCR resolver dialed both the discovery URL and the registration endpoint with HTTP clients that had no private-IP protection, and gave callers no way to supply a guarded client. A DCR upstream was therefore an SSRF vector (CWE-918) even when the caller validated the configured URL up front: a discovery document could point registration_endpoint at an in-cluster service or a link-local metadata address, and a validated endpoint could rebind to a private address at dial time. Mirror the OAuth2/OIDC upstream posture (safe-by-default): - Add networking.NewHostScopedClientBuilder as the single source of truth for the host-scoped guard policy, and refactor the upstream provider's newHTTPClientForHost to delegate to it so the two paths cannot drift. - Dial both resolver outbound calls through a private-IP-guarded client with keep-alives disabled, preserving the existing redirect-refusal and bearer-token layering on the registration client. - Add AllowPrivateIPs to dcr.Request as the explicit opt-in for in-cluster upstreams, wired from OAuth2UpstreamRunConfig.AllowPrivateIPs so DCR shares the upstream's private-IP posture. Closes #5825 Co-Authored-By: Claude Opus 4.8 --- pkg/auth/dcr/request.go | 24 +++++ pkg/auth/dcr/resolver.go | 96 ++++++++++++++++--- pkg/auth/dcr/resolver_test.go | 133 +++++++++++++++++++++++++++ pkg/authserver/runner/dcr_adapter.go | 4 + pkg/authserver/upstream/oauth2.go | 12 +-- pkg/networking/http_client.go | 25 +++++ 6 files changed, 271 insertions(+), 23 deletions(-) diff --git a/pkg/auth/dcr/request.go b/pkg/auth/dcr/request.go index 055056c160..07c3db4d04 100644 --- a/pkg/auth/dcr/request.go +++ b/pkg/auth/dcr/request.go @@ -132,4 +132,28 @@ type Request struct { // code_challenge_methods_supported the S256 gate cannot be evaluated, // so the resolver refuses to register as a public client. PublicClient bool + + // AllowPrivateIPs permits both of the resolver's outbound calls — the + // discovery fetch to DiscoveryURL and the registration POST to the + // resolved registration endpoint — to connect to private IP ranges + // (RFC-1918, loopback, link-local). Both calls are otherwise dialed + // through a private-IP-guarded client that refuses such addresses at + // connect time, closing the CWE-918 SSRF vectors that a DCR upstream + // would otherwise open: a discovery document that points + // registration_endpoint at an in-cluster service or a link-local + // metadata address, and DNS rebinding of an endpoint that was public + // when the caller validated it. Loopback hosts remain permitted for + // development/testing regardless of this flag; the check runs on the + // address actually dialed, after DNS resolution, so it also defends + // against rebinding. + // + // Set this to true only when the upstream authorization server is + // reachable solely over a private address (e.g. an in-cluster IdP with + // no public endpoint). HTTPS-scheme enforcement is unchanged — HTTPS is + // still required for non-loopback hosts. Defaults to false. + // + // Mirrors the AllowPrivateIPs posture already carried by the OAuth2 and + // OIDC upstream configs so a DCR upstream is not the one outbound-facing + // path without an SSRF guard. + AllowPrivateIPs bool } diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index 6ffba819f0..5ba7a0dc3b 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -750,7 +750,10 @@ func performRegistration( registrationEndpoint, redirectURI, authMethod string, scopes []string, ) (*oauthproto.DynamicClientRegistrationResponse, error) { - httpClient := newDCRHTTPClient(req.InitialAccessToken) + httpClient, err := newDCRHTTPClient(req.InitialAccessToken, registrationEndpoint, req.AllowPrivateIPs) + if err != nil { + return nil, fmt.Errorf("dcr: build registration http client: %w", err) + } clientName := req.ClientName if clientName == "" { @@ -891,7 +894,21 @@ func resolveDCREndpoints( return nil, err } - metadata, err := oauthproto.FetchAuthorizationServerMetadataFromURL(ctx, req.DiscoveryURL, upstreamIssuer, nil) + // Dial the discovery fetch through the private-IP-guarded client so a + // DiscoveryURL that resolves to a private/loopback/link-local address — + // or rebinds to one after the caller validated it — is refused at connect + // time (CWE-918). Guarded by default; req.AllowPrivateIPs opts in to + // private ranges for an in-cluster upstream. + discoveryHost, err := hostFromURL(req.DiscoveryURL) + if err != nil { + return nil, err + } + discoveryClient, err := newGuardedDCRClient(discoveryHost, req.AllowPrivateIPs) + if err != nil { + return nil, fmt.Errorf("dcr: build discovery http client: %w", err) + } + + metadata, err := oauthproto.FetchAuthorizationServerMetadataFromURL(ctx, req.DiscoveryURL, upstreamIssuer, discoveryClient) return endpointsFromMetadata(metadata, err, upstreamIssuer) } @@ -1263,27 +1280,33 @@ var errDCRRedirectRefused = errors.New( "to avoid forwarding the RFC 7591 initial access token to a foreign origin") // newDCRHTTPClient returns the http.Client to pass to -// oauthproto.RegisterClientDynamically. The client always blocks HTTP -// redirects so that an upstream cannot use a 30x to coerce us into +// oauthproto.RegisterClientDynamically for a registration POST to +// registrationEndpoint. The client dials through the private-IP-guarded +// transport built by newGuardedDCRClient (CWE-918 SSRF protection) and always +// blocks HTTP redirects so that an upstream cannot use a 30x to coerce us into // re-issuing the registration request (and any attached // Authorization: Bearer header) against a different origin. RFC 7591 §3 // does not require redirect support, so refusing them is safe. // -// When initialAccessToken is non-empty the client also wraps the canonical -// DCR client's transport with a bearerTokenTransport that injects the -// Authorization header. The combination of the bearer transport plus the -// redirect block is what prevents the token-leak class of bug. -// -// The timeout policy is sourced from oauthproto.NewDefaultDCRClient so -// future tightening of those bounds propagates automatically. -func newDCRHTTPClient(initialAccessToken string) *http.Client { - client := oauthproto.NewDefaultDCRClient() +// When initialAccessToken is non-empty the client also wraps the guarded +// transport with a bearerTokenTransport that injects the Authorization header. +// The combination of the bearer transport plus the redirect block is what +// prevents the token-leak class of bug. +func newDCRHTTPClient(initialAccessToken, registrationEndpoint string, allowPrivateIPs bool) (*http.Client, error) { + host, err := hostFromURL(registrationEndpoint) + if err != nil { + return nil, err + } + client, err := newGuardedDCRClient(host, allowPrivateIPs) + if err != nil { + return nil, err + } client.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { return errDCRRedirectRefused } if initialAccessToken == "" { - return client + return client, nil } next := client.Transport @@ -1294,5 +1317,48 @@ func newDCRHTTPClient(initialAccessToken string) *http.Client { token: initialAccessToken, next: next, } - return client + return client, nil +} + +// newGuardedDCRClient builds the private-IP-guarded *http.Client used for both +// of the resolver's outbound calls — the discovery fetch and the registration +// POST. It dials through networking's protected dialer so a host that resolves +// to a private, loopback, or link-local address is refused at connect time +// (CWE-918), closing both the discovery-indirection and DNS-rebinding SSRF +// vectors: the check runs on the address actually dialed, after DNS +// resolution, and — with keep-alives disabled — on every request rather than +// being bypassed by a pooled connection. +// +// allowPrivateIPs widens only the private-IP gate (for an in-cluster upstream +// reachable solely over an RFC-1918 address); loopback hosts remain permitted +// for development regardless, matching networking.NewHostScopedClientBuilder +// and the AllowPrivateIPs posture of the OAuth2/OIDC upstream configs. HTTP +// scheme enforcement is left to the resolver's URL validation and the builder's +// ValidatingTransport (HTTPS-except-loopback). +// +// The builder's 30 s overall / 10 s TLS / 10 s response-header default +// timeouts match the bounds previously sourced from +// oauthproto.NewDefaultDCRClient. +func newGuardedDCRClient(host string, allowPrivateIPs bool) (*http.Client, error) { + return networking.NewHostScopedClientBuilder(host, allowPrivateIPs, false). + WithDisableKeepAlives(true). + Build() +} + +// hostFromURL extracts the host[:port] component used to scope the guarded +// HTTP client. Every URL reaching this helper has already passed +// scheme-and-host validation at the resolver's entry points +// (validateUpstreamEndpointURL for the registration endpoint, +// FetchAuthorizationServerMetadataFromURL for the discovery URL), so a parse +// failure or empty host here signals an internal inconsistency rather than +// untrusted input. +func hostFromURL(rawURL string) (string, error) { + u, err := url.Parse(rawURL) + if err != nil { + return "", fmt.Errorf("dcr: parse url for http client host: %w", err) + } + if u.Host == "" { + return "", fmt.Errorf("dcr: url missing host: %q", rawURL) + } + return u.Host, nil } diff --git a/pkg/auth/dcr/resolver_test.go b/pkg/auth/dcr/resolver_test.go index 2c163e110d..9367c9bbe8 100644 --- a/pkg/auth/dcr/resolver_test.go +++ b/pkg/auth/dcr/resolver_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/require" "github.com/stacklok/toolhive/pkg/authserver/storage" + "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" ) @@ -343,6 +344,138 @@ func TestResolveDCRCredentials_DoesNotForwardBearerOnRedirect(t *testing.T) { atomic.LoadInt32(&foreignHits), foreignAuthHeaders) } +// TestResolveDCRCredentials_BlocksPrivateIPTargets pins the CWE-918 SSRF guard +// added for issue #5825: both of the resolver's outbound calls — the discovery +// fetch and the registration POST — are dialed through a private-IP-guarded +// client, so a registration endpoint that resolves to a private or link-local +// address is refused at connect time. The guard fires before any bytes leave +// the host, so these cases need no live server behind the private target. +// AllowPrivateIPs defaults to false; loopback stays permitted for development. +func TestResolveDCRCredentials_BlocksPrivateIPTargets(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + newReq func(t *testing.T) *Request + }{ + { + // Direct RegistrationEndpoint branch: an operator-configured + // endpoint that resolves (or rebinds) to an RFC-1918 address. + name: "direct registration endpoint on a private IP", + newReq: func(_ *testing.T) *Request { + // Issuer is unique per test: the process-global dcrFlight + // singleflight keys on (Issuer, RedirectURI, ScopesHash), so a + // shared synthetic issuer would coalesce this call with another + // parallel test and return that test's result. + return &Request{ + Issuer: "https://block-direct.example.test", + Scopes: []string{"openid"}, + RegistrationEndpoint: "https://10.255.255.1/register", + } + }, + }, + { + // Discovery-indirection branch: the discovery document is served + // from an allowed (loopback) host, but it points + // registration_endpoint at a link-local metadata address the + // metadata itself controls. HTTPS so it clears scheme validation + // and reaches the guarded dial. + name: "registration endpoint from discovery on a link-local IP", + newReq: func(t *testing.T) *Request { + t.Helper() + mux := http.NewServeMux() + var server *httptest.Server + mux.HandleFunc("/.well-known/oauth-authorization-server", + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(oauthproto.AuthorizationServerMetadata{ + Issuer: server.URL, + AuthorizationEndpoint: server.URL + "/authorize", + TokenEndpoint: server.URL + "/token", + JWKSURI: server.URL + "/jwks", + RegistrationEndpoint: "https://169.254.169.254/register", + }) + }) + server = httptest.NewServer(mux) + t.Cleanup(server.Close) + return &Request{ + Issuer: server.URL, + Scopes: []string{"openid"}, + DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server", + } + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + _, err := ResolveCredentials(context.Background(), tc.newReq(t), newMemoryDCRStore(t)) + require.Error(t, err) + assert.ErrorContains(t, err, networking.ErrPrivateIpAddress, + "a private/link-local registration target must be refused at connect time") + }) + } +} + +// TestResolveDCRCredentials_AllowPrivateIPsHonored proves req.AllowPrivateIPs +// is threaded through to the guarded client: with it set, the resolver dials +// the private target instead of refusing it at the guard. The target is a +// non-routable RFC 5737 documentation address (TEST-NET-1), so the dial fails +// with a network error rather than the guard error — and can never reach a +// real host. +func TestResolveDCRCredentials_AllowPrivateIPsHonored(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + // Issuer is unique per test so the process-global dcrFlight singleflight + // (keyed on Issuer, RedirectURI, ScopesHash) cannot coalesce this call + // with another parallel test and hand back that test's result. + req := &Request{ + Issuer: "https://allow-private.example.test", + Scopes: []string{"openid"}, + RegistrationEndpoint: "https://192.0.2.1/register", + AllowPrivateIPs: true, + } + + _, err := ResolveCredentials(ctx, req, newMemoryDCRStore(t)) + require.Error(t, err, "the dead documentation address cannot complete registration") + assert.NotContains(t, err.Error(), networking.ErrPrivateIpAddress, + "AllowPrivateIPs=true must lift the guard so the dial is attempted, not refused") +} + +func TestHostFromURL(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rawURL string + want string + wantErr bool + }{ + {name: "https with port", rawURL: "https://idp.example.com:8443/register", want: "idp.example.com:8443"}, + {name: "https without port", rawURL: "https://idp.example.com/register", want: "idp.example.com"}, + {name: "loopback with port", rawURL: "http://127.0.0.1:5000/register", want: "127.0.0.1:5000"}, + {name: "missing host", rawURL: "/register", wantErr: true}, + {name: "malformed url", rawURL: "://bad", wantErr: true}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := hostFromURL(tc.rawURL) + if tc.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} + func TestResolveDCRCredentials_AuthMethodPreference(t *testing.T) { t.Parallel() diff --git a/pkg/authserver/runner/dcr_adapter.go b/pkg/authserver/runner/dcr_adapter.go index 37dfd54507..dd214d3aa8 100644 --- a/pkg/authserver/runner/dcr_adapter.go +++ b/pkg/authserver/runner/dcr_adapter.go @@ -79,6 +79,10 @@ func newDCRRequest(rc *authserver.OAuth2UpstreamRunConfig, localIssuer string) ( AuthorizationEndpoint: rc.AuthorizationEndpoint, TokenEndpoint: rc.TokenEndpoint, InitialAccessToken: initialAccessToken, + // Reuse the upstream's private-IP policy so the DCR discovery and + // registration calls share the same SSRF posture as its token and + // userinfo calls (see upstream.OAuth2Config.AllowPrivateIPs). + AllowPrivateIPs: rc.AllowPrivateIPs, }, nil } diff --git a/pkg/authserver/upstream/oauth2.go b/pkg/authserver/upstream/oauth2.go index 11153f9dd9..32369043a6 100644 --- a/pkg/authserver/upstream/oauth2.go +++ b/pkg/authserver/upstream/oauth2.go @@ -28,7 +28,6 @@ import ( "maps" "net/http" "net/url" - "os" "strings" "golang.org/x/oauth2" @@ -830,12 +829,9 @@ func formatOAuth2Error(err error, prefix string) error { // allowPrivateIPs widens only the private-IP gate: it permits connections to // RFC-1918/link-local addresses (e.g. in-cluster providers) without enabling // the HTTP scheme for non-localhost hosts. +// +// The host-scoped guard policy lives in networking.NewHostScopedClientBuilder +// so this provider path and the DCR resolver share one implementation. func newHTTPClientForHost(host string, allowPrivateIPs, insecureAllowHTTP bool) (*http.Client, error) { - allowInsecure := networking.IsLocalhost(host) || - insecureAllowHTTP || - strings.EqualFold(os.Getenv("INSECURE_DISABLE_URL_VALIDATION"), "true") - return networking.NewHttpClientBuilder(). - WithInsecureAllowHTTP(allowInsecure). - WithPrivateIPs(allowInsecure || allowPrivateIPs). - Build() + return networking.NewHostScopedClientBuilder(host, allowPrivateIPs, insecureAllowHTTP).Build() } diff --git a/pkg/networking/http_client.go b/pkg/networking/http_client.go index 86332c50bb..cef216617b 100644 --- a/pkg/networking/http_client.go +++ b/pkg/networking/http_client.go @@ -176,6 +176,31 @@ func NewHttpClientBuilder() *HttpClientBuilder { } } +// NewHostScopedClientBuilder returns an HttpClientBuilder pre-configured with +// the SSRF-guard policy (CWE-918) appropriate for dialing host. By default the +// returned builder blocks plain HTTP and connections to private/loopback/ +// link-local IP ranges. Both gates are relaxed automatically for loopback +// hosts (development/testing) and when INSECURE_DISABLE_URL_VALIDATION is set. +// +// allowPrivateIPs widens only the private-IP gate — for example an in-cluster +// provider reachable solely over an RFC-1918 address — without enabling plain +// HTTP for non-loopback hosts. insecureAllowHTTP additionally permits +// plain-HTTP for non-loopback hosts and must never be set in production. +// +// The returned builder is not yet built: callers may chain further options +// (e.g. WithTimeout, WithDisableKeepAlives) before calling Build. This is the +// single source of truth for the host-scoped guard policy shared by the +// upstream OAuth2/OIDC providers and the DCR resolver so the two paths cannot +// drift. +func NewHostScopedClientBuilder(host string, allowPrivateIPs, insecureAllowHTTP bool) *HttpClientBuilder { + allowInsecure := IsLocalhost(host) || + insecureAllowHTTP || + strings.EqualFold(os.Getenv("INSECURE_DISABLE_URL_VALIDATION"), "true") + return NewHttpClientBuilder(). + WithInsecureAllowHTTP(allowInsecure). + WithPrivateIPs(allowInsecure || allowPrivateIPs) +} + // WithCABundle sets the CA certificate bundle path func (b *HttpClientBuilder) WithCABundle(path string) *HttpClientBuilder { b.caCertPath = path From 8b0774a69b7de69f33762adfa0ac93aa8fe45c41 Mon Sep 17 00:00:00 2001 From: Trey Date: Wed, 15 Jul 2026 21:03:52 -0700 Subject: [PATCH 2/7] Address code review feedback Fixed issues from code review: - MEDIUM: Install SameHostRedirectPolicy on the DCR discovery client so a malicious upstream 30x cannot walk the metadata fetch onto another host (the registration client already refuses all redirects). - MEDIUM: Thread the private-IP decision into the CLI DCR consumer via OAuthFlowConfig.AllowPrivateIPs, wired from the existing TargetIsPrivate model at both CLI entrypoints, so a CLI pointed at a private remote is no longer newly refused on DCR while a public remote stays guarded. Co-Authored-By: Claude Opus 4.8 --- cmd/thv/app/proxy.go | 38 +++++++++++++++++--------------- pkg/auth/dcr/resolver.go | 15 +++++++++++++ pkg/auth/dcr/resolver_test.go | 35 +++++++++++++++++++++++++++++ pkg/auth/discovery/discovery.go | 15 +++++++++++++ pkg/auth/remote/handler.go | 39 ++++++++++++++++++++------------- 5 files changed, 109 insertions(+), 33 deletions(-) diff --git a/cmd/thv/app/proxy.go b/cmd/thv/app/proxy.go index 46e5df577a..7a55fa0e01 100644 --- a/cmd/thv/app/proxy.go +++ b/cmd/thv/app/proxy.go @@ -391,15 +391,16 @@ func handleOutgoingAuthentication(ctx context.Context) (*discovery.OAuthFlowResu } flowConfig := &discovery.OAuthFlowConfig{ - ClientID: remoteAuthFlags.RemoteAuthClientID, - ClientSecret: clientSecret, - AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL, - TokenURL: remoteAuthFlags.RemoteAuthTokenURL, - Scopes: remoteAuthFlags.RemoteAuthScopes, - CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort, - Timeout: remoteAuthFlags.RemoteAuthTimeout, - SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser, - ScopeParamName: remoteAuthFlags.RemoteAuthScopeParamName, + ClientID: remoteAuthFlags.RemoteAuthClientID, + ClientSecret: clientSecret, + AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL, + TokenURL: remoteAuthFlags.RemoteAuthTokenURL, + Scopes: remoteAuthFlags.RemoteAuthScopes, + CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort, + Timeout: remoteAuthFlags.RemoteAuthTimeout, + SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser, + ScopeParamName: remoteAuthFlags.RemoteAuthScopeParamName, + AllowPrivateIPs: networking.TargetIsPrivate(ctx, proxyTargetURI), } result, err := discovery.PerformOAuthFlow(ctx, remoteAuthFlags.RemoteAuthIssuer, flowConfig) @@ -422,15 +423,16 @@ func handleOutgoingAuthentication(ctx context.Context) (*discovery.OAuthFlowResu // Perform OAuth flow with discovered configuration flowConfig := &discovery.OAuthFlowConfig{ - ClientID: remoteAuthFlags.RemoteAuthClientID, - ClientSecret: clientSecret, - AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL, - TokenURL: remoteAuthFlags.RemoteAuthTokenURL, - Scopes: remoteAuthFlags.RemoteAuthScopes, - CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort, - Timeout: remoteAuthFlags.RemoteAuthTimeout, - SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser, - ScopeParamName: remoteAuthFlags.RemoteAuthScopeParamName, + ClientID: remoteAuthFlags.RemoteAuthClientID, + ClientSecret: clientSecret, + AuthorizeURL: remoteAuthFlags.RemoteAuthAuthorizeURL, + TokenURL: remoteAuthFlags.RemoteAuthTokenURL, + Scopes: remoteAuthFlags.RemoteAuthScopes, + CallbackPort: remoteAuthFlags.RemoteAuthCallbackPort, + Timeout: remoteAuthFlags.RemoteAuthTimeout, + SkipBrowser: remoteAuthFlags.RemoteAuthSkipBrowser, + ScopeParamName: remoteAuthFlags.RemoteAuthScopeParamName, + AllowPrivateIPs: networking.TargetIsPrivate(ctx, proxyTargetURI), } result, err := discovery.PerformOAuthFlow(ctx, authInfo.Realm, flowConfig) diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index 5ba7a0dc3b..9f2f2b033a 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -907,6 +907,15 @@ func resolveDCREndpoints( if err != nil { return nil, fmt.Errorf("dcr: build discovery http client: %w", err) } + // The discovery URL is operator-configured, but the document it returns — + // and any 30x the upstream serves in response — is upstream-controlled. + // Restrict the fetch to same-host redirects so a malicious redirect cannot + // walk the request onto another host (CWE-918); this matters even when the + // dial guard is relaxed for a loopback discovery host. The registration + // client refuses redirects outright because it carries the bearer token; + // the discovery GET carries no secret, so same-host redirects are allowed — + // matching the CLI discovery clients in pkg/auth/discovery. + discoveryClient.CheckRedirect = networking.SameHostRedirectPolicy() metadata, err := oauthproto.FetchAuthorizationServerMetadataFromURL(ctx, req.DiscoveryURL, upstreamIssuer, discoveryClient) return endpointsFromMetadata(metadata, err, upstreamIssuer) @@ -1339,6 +1348,12 @@ func newDCRHTTPClient(initialAccessToken, registrationEndpoint string, allowPriv // The builder's 30 s overall / 10 s TLS / 10 s response-header default // timeouts match the bounds previously sourced from // oauthproto.NewDefaultDCRClient. +// +// The returned client has no CheckRedirect policy; each caller layers its own +// (the registration client refuses all redirects to protect the bearer token; +// the discovery client restricts them to the same host). The dial guard alone +// does not stop a redirect to a different public host, so the redirect policy +// is a required complement, not an optional one. func newGuardedDCRClient(host string, allowPrivateIPs bool) (*http.Client, error) { return networking.NewHostScopedClientBuilder(host, allowPrivateIPs, false). WithDisableKeepAlives(true). diff --git a/pkg/auth/dcr/resolver_test.go b/pkg/auth/dcr/resolver_test.go index 9367c9bbe8..bedc084e21 100644 --- a/pkg/auth/dcr/resolver_test.go +++ b/pkg/auth/dcr/resolver_test.go @@ -446,6 +446,41 @@ func TestResolveDCRCredentials_AllowPrivateIPsHonored(t *testing.T) { "AllowPrivateIPs=true must lift the guard so the dial is attempted, not refused") } +// TestResolveDCRCredentials_DiscoveryRefusesCrossHostRedirect pins that the +// discovery fetch installs SameHostRedirectPolicy: a discovery endpoint that +// 30x-redirects to a different host must not be followed, so a malicious +// upstream cannot walk the metadata fetch onto an unintended origin (CWE-918). +// A different port counts as a different host, so two loopback httptest servers +// suffice. +func TestResolveDCRCredentials_DiscoveryRefusesCrossHostRedirect(t *testing.T) { + t.Parallel() + + var foreignHits int32 + foreign := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&foreignHits, 1) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(foreign.Close) + + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/oauth-authorization-server", func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, foreign.URL+"/.well-known/oauth-authorization-server", http.StatusFound) + }) + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + req := &Request{ + Issuer: server.URL, + Scopes: []string{"openid"}, + DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server", + } + + _, err := ResolveCredentials(context.Background(), req, newMemoryDCRStore(t)) + require.Error(t, err, "discovery must fail when the endpoint redirects cross-host") + assert.EqualValues(t, 0, atomic.LoadInt32(&foreignHits), + "the discovery client must not follow a cross-host redirect") +} + func TestHostFromURL(t *testing.T) { t.Parallel() diff --git a/pkg/auth/discovery/discovery.go b/pkg/auth/discovery/discovery.go index d487cc0ce7..416e57e9da 100644 --- a/pkg/auth/discovery/discovery.go +++ b/pkg/auth/discovery/discovery.go @@ -519,6 +519,16 @@ type OAuthFlowConfig struct { Resource string // RFC 8707 resource indicator (optional) OAuthParams map[string]string ScopeParamName string // Override scope query parameter name (e.g., "user_scope" for Slack) + + // AllowPrivateIPs permits the Dynamic Client Registration calls (discovery + // fetch and registration POST) to reach private/loopback/link-local + // addresses. Callers set it from networking.TargetIsPrivate on the remote + // target: when ToolHive is pointed at a private remote the upstream IdP may + // legitimately be private too, so its DCR calls must be allowed; otherwise + // they are guarded to contain SSRF (CWE-918). This mirrors the + // blockPrivateIPs decision the flow already applies to its other discovery + // fetches. Defaults to false (guarded). + AllowPrivateIPs bool } // OAuthFlowResult contains the result of an OAuth flow @@ -720,6 +730,11 @@ func resolveDCRCredentials( AuthorizationEndpoint: discoveredDoc.AuthorizationEndpoint, TokenEndpoint: discoveredDoc.TokenEndpoint, PublicClient: true, + // Carry the flow's private-IP decision so DCR shares the same SSRF + // posture as the flow's other discovery fetches; without this the DCR + // calls would always be guarded and a legitimately private upstream + // would be refused. See OAuthFlowConfig.AllowPrivateIPs. + AllowPrivateIPs: config.AllowPrivateIPs, } // Fetch AS metadata using the multi-URL fallback so non-root issuers diff --git a/pkg/auth/remote/handler.go b/pkg/auth/remote/handler.go index f92e7a04ec..c5ce0c848c 100644 --- a/pkg/auth/remote/handler.go +++ b/pkg/auth/remote/handler.go @@ -103,8 +103,11 @@ func (h *Handler) Authenticate(ctx context.Context, remoteURL string) (oauth2.To } } - // Priority 3: Fresh OAuth authentication flow - return h.performOAuthFlow(ctx, issuer, scopes, authServerInfo) + // Priority 3: Fresh OAuth authentication flow. Pass the same target-private + // decision the discovery fetches use so DCR (discovery + registration) is + // allowed to reach a private upstream only when the operator-configured + // remote is itself private, and guarded otherwise (CWE-918). + return h.performOAuthFlow(ctx, issuer, scopes, authServerInfo, networking.TargetIsPrivate(ctx, remoteURL)) } // validateBearerRequirement checks if Bearer auth is required without OAuth fallback @@ -132,6 +135,7 @@ func (h *Handler) performOAuthFlow( issuer string, scopes []string, authServerInfo *discovery.AuthServerInfo, + allowPrivateIPs bool, ) (oauth2.TokenSource, error) { slog.Debug("Starting OAuth authentication flow", "issuer", issuer) @@ -139,7 +143,7 @@ func (h *Handler) performOAuthFlow( // Priority 1: Pre-configured credentials — set by buildOAuthFlowConfig from h.config.ClientID/ClientSecret. // Priority 2: CIMD — AS advertises support and no credentials are set; use metadata URL as client_id. // Priority 3: DCR — PerformOAuthFlow handles this when ClientID is still empty after the above. - flowConfig := h.buildOAuthFlowConfig(scopes, authServerInfo) + flowConfig := h.buildOAuthFlowConfig(scopes, authServerInfo, allowPrivateIPs) if shouldUseCIMD(authServerInfo, flowConfig) { flowConfig.ClientID = oauthproto.ToolHiveClientMetadataDocumentURL slog.Debug("Using CIMD client_id", "url", oauthproto.ToolHiveClientMetadataDocumentURL) @@ -162,19 +166,24 @@ func (h *Handler) performOAuthFlow( } // buildOAuthFlowConfig creates the OAuth flow configuration -func (h *Handler) buildOAuthFlowConfig(scopes []string, authServerInfo *discovery.AuthServerInfo) *discovery.OAuthFlowConfig { +func (h *Handler) buildOAuthFlowConfig( + scopes []string, + authServerInfo *discovery.AuthServerInfo, + allowPrivateIPs bool, +) *discovery.OAuthFlowConfig { flowConfig := &discovery.OAuthFlowConfig{ - ClientID: h.config.ClientID, - ClientSecret: h.config.ClientSecret, - AuthorizeURL: h.config.AuthorizeURL, - TokenURL: h.config.TokenURL, - Scopes: scopes, - CallbackPort: h.config.CallbackPort, - Timeout: h.config.Timeout, - SkipBrowser: h.config.SkipBrowser, - Resource: h.config.Resource, - OAuthParams: h.config.OAuthParams, - ScopeParamName: h.config.ScopeParamName, + ClientID: h.config.ClientID, + ClientSecret: h.config.ClientSecret, + AuthorizeURL: h.config.AuthorizeURL, + TokenURL: h.config.TokenURL, + Scopes: scopes, + CallbackPort: h.config.CallbackPort, + Timeout: h.config.Timeout, + SkipBrowser: h.config.SkipBrowser, + Resource: h.config.Resource, + OAuthParams: h.config.OAuthParams, + ScopeParamName: h.config.ScopeParamName, + AllowPrivateIPs: allowPrivateIPs, } // If we have discovered endpoints from the authorization server metadata, From 5cae777149c39c0030782c0728078209eda80887 Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 16 Jul 2026 08:52:29 -0700 Subject: [PATCH 3/7] Close remaining SSRF guard gaps in CLI DCR flow Addresses stacklok/toolhive#5826 review comments: - HIGH pkg/auth/discovery/discovery.go:746 (3594945993, 3596751548): guard the metadata re-fetch that resolves registration_endpoint; it was dialing through a nil (unguarded) client, reopening the CWE-918 vectors this PR otherwise closes, on a fetch reachable from untrusted server input - MEDIUM pkg/auth/dcr/resolver.go (3596751560): validate a discovery-derived registration_endpoint the same as authorization_endpoint/token_endpoint before it reaches the guarded client - MEDIUM pkg/auth/oauth/oidc.go (body:oidc-blockprivate): thread a real blockPrivateIPs decision through DiscoverOIDCEndpoints instead of hardcoding false, so the CLI's OIDC-discovery fallback ahead of DCR shares the flow's SSRF posture - MEDIUM pkg/auth/remote/handler.go:110 (3596751566): compute TargetIsPrivate once and thread it through instead of a second, later DNS lookup that could disagree with the first - MEDIUM pkg/authserver/upstream/oauth2.go:835 (3596751571): document why this long-lived provider client intentionally keeps keep-alives enabled, unlike the DCR resolver's guarded client Co-Authored-By: Claude Sonnet 5 --- pkg/auth/dcr/resolver.go | 17 +++++++--- pkg/auth/discovery/dcr_resolver_test.go | 45 ++++++++++++++++++++----- pkg/auth/discovery/discovery.go | 27 +++++++++++++-- pkg/auth/discovery/discovery_test.go | 5 +-- pkg/auth/oauth/oidc.go | 32 ++++++++++++------ pkg/auth/oauth/oidc_test.go | 2 +- pkg/auth/remote/handler.go | 35 +++++++++++-------- pkg/auth/remote/handler_test.go | 16 ++++----- pkg/authserver/upstream/oauth2.go | 11 ++++++ pkg/registry/auth/login.go | 5 ++- 10 files changed, 144 insertions(+), 51 deletions(-) diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index 9f2f2b033a..809d19d84f 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -1055,6 +1055,12 @@ func endpointsFromMetadata( return nil, fmt.Errorf("synthesise registration endpoint: %w", err) } registrationEndpoint = synth + } else if err := validateUpstreamEndpointURL(registrationEndpoint, "registration_endpoint"); err != nil { + // Unlike the synthesised branch above, this value came straight from + // the discovery document — validate it the same as + // authorization_endpoint/token_endpoint rather than letting it reach + // hostFromURL unvalidated. + return nil, fmt.Errorf("dcr: discovered %w", err) } return &dcrEndpoints{ @@ -1363,10 +1369,13 @@ func newGuardedDCRClient(host string, allowPrivateIPs bool) (*http.Client, error // hostFromURL extracts the host[:port] component used to scope the guarded // HTTP client. Every URL reaching this helper has already passed // scheme-and-host validation at the resolver's entry points -// (validateUpstreamEndpointURL for the registration endpoint, -// FetchAuthorizationServerMetadataFromURL for the discovery URL), so a parse -// failure or empty host here signals an internal inconsistency rather than -// untrusted input. +// (validateUpstreamEndpointURL for the registration endpoint — both the +// caller-supplied case and the metadata-discovered case handled in +// endpointsFromMetadata — and FetchAuthorizationServerMetadataFromURL for the +// discovery URL; the synthesised-endpoint case derives its host from an +// upstream issuer that already passed the RFC 8414 §3.3 issuer-match check), +// so a parse failure or empty host here signals an internal inconsistency +// rather than untrusted input. func hostFromURL(rawURL string) (string, error) { u, err := url.Parse(rawURL) if err != nil { diff --git a/pkg/auth/discovery/dcr_resolver_test.go b/pkg/auth/discovery/dcr_resolver_test.go index 2886d78ac2..2e4c289632 100644 --- a/pkg/auth/discovery/dcr_resolver_test.go +++ b/pkg/auth/discovery/dcr_resolver_test.go @@ -17,6 +17,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" ) @@ -172,6 +173,8 @@ func TestHandleDynamicRegistration_InheritsS256Gating(t *testing.T) { config := &OAuthFlowConfig{ Scopes: []string{"openid", "profile"}, CallbackPort: 8765, + // Loopback test server: guard would otherwise refuse to dial it. + AllowPrivateIPs: true, } err := handleDynamicRegistration(context.Background(), server.URL, config) @@ -206,8 +209,9 @@ func TestHandleDynamicRegistration_InheritsRedirectRefusal(t *testing.T) { }) config := &OAuthFlowConfig{ - Scopes: []string{"openid", "profile"}, - CallbackPort: 8765, + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + AllowPrivateIPs: true, // loopback test server; guard would otherwise refuse to dial it } err := handleDynamicRegistration(context.Background(), server.URL, config) @@ -218,6 +222,28 @@ func TestHandleDynamicRegistration_InheritsRedirectRefusal(t *testing.T) { "foreign origin must receive zero requests; the redirect refusal prevents the leak") } +// TestHandleDynamicRegistration_MetadataRefetchBlocksPrivateIP pins the +// CWE-918 guard on resolveDCRCredentials's own metadata re-fetch (used to +// recover code_challenge_methods_supported on the pre-discovered path): an +// issuer that resolves to a private IP must be refused at connect time, the +// same as the resolver's own outbound calls in pkg/auth/dcr. +func TestHandleDynamicRegistration_MetadataRefetchBlocksPrivateIP(t *testing.T) { + t.Parallel() + + config := &OAuthFlowConfig{ + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + AuthorizeURL: "https://10.255.255.1/authorize", + TokenURL: "https://10.255.255.1/token", + RegistrationEndpoint: "https://10.255.255.1/register", + } + + err := handleDynamicRegistration(context.Background(), "https://10.255.255.1", config) + require.Error(t, err, "an issuer resolving to a private IP must be refused at connect time") + assert.ErrorContains(t, err, networking.ErrPrivateIpAddress, + "the metadata re-fetch must be guarded the same as the resolver's own outbound calls") +} + // TestHandleDynamicRegistration_InheritsSingleflightDedup verifies that // N concurrent handleDynamicRegistration calls for the same (issuer, // scopes, redirectURI) tuple coalesce into exactly one upstream /register @@ -256,8 +282,9 @@ func TestHandleDynamicRegistration_InheritsSingleflightDedup(t *testing.T) { // Each goroutine gets its own OAuthFlowConfig so we can assert // the resolution was applied to every caller's config. cfg := &OAuthFlowConfig{ - Scopes: []string{"openid", "profile"}, - CallbackPort: 8765, + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + AllowPrivateIPs: true, // loopback test server; guard would otherwise refuse to dial it } results[i] = cfg go func(idx int) { @@ -336,8 +363,9 @@ func TestHandleDynamicRegistration_NonRootIssuerRFC8414PathInsertion(t *testing. t.Cleanup(server.Close) config := &OAuthFlowConfig{ - Scopes: []string{"openid", "profile"}, - CallbackPort: 8765, + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + AllowPrivateIPs: true, // loopback test server; guard would otherwise refuse to dial it } err := handleDynamicRegistration(context.Background(), server.URL+"/oauth", config) @@ -487,8 +515,9 @@ func TestHandleDynamicRegistration_PopulatesEndpoints(t *testing.T) { }) config := &OAuthFlowConfig{ - Scopes: []string{"openid", "profile"}, - CallbackPort: 8765, + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + AllowPrivateIPs: true, // loopback test server; guard would otherwise refuse to dial it } err := handleDynamicRegistration(context.Background(), server.URL, config) diff --git a/pkg/auth/discovery/discovery.go b/pkg/auth/discovery/discovery.go index 416e57e9da..a35c0136d6 100644 --- a/pkg/auth/discovery/discovery.go +++ b/pkg/auth/discovery/discovery.go @@ -743,7 +743,26 @@ func resolveDCRCredentials( // defence-in-depth for a future refactor that produces an empty-issuer // doc and preserves the pre-existing RegistrationEndpoint-direct path. if discoveredDoc.Issuer != "" { - fullMeta, metaErr := oauthproto.FetchAuthorizationServerMetadata(ctx, discoveredDoc.Issuer, nil) + // Guard this fetch the same way pkg/auth/dcr's own outbound calls are + // guarded (CWE-918): discoveredDoc.Issuer can be untrusted server input + // on some discovery branches (see discoverIssuerAndScopes in + // pkg/auth/remote/handler.go), so a nil client here would reopen the + // discovery-indirection and DNS-rebinding vectors this PR closes + // elsewhere. See networking.NewHostScopedClientBuilder for the guard + // policy and pkg/auth/dcr's newGuardedDCRClient for the same pattern. + metaHost, parseErr := url.Parse(discoveredDoc.Issuer) + if parseErr != nil { + return nil, fmt.Errorf("dynamic client registration failed: parse issuer for http client: %w", parseErr) + } + metaClient, clientErr := networking.NewHostScopedClientBuilder(metaHost.Host, config.AllowPrivateIPs, false). + WithDisableKeepAlives(true). + Build() + if clientErr != nil { + return nil, fmt.Errorf("dynamic client registration failed: build metadata http client: %w", clientErr) + } + metaClient.CheckRedirect = networking.SameHostRedirectPolicy() + + fullMeta, metaErr := oauthproto.FetchAuthorizationServerMetadata(ctx, discoveredDoc.Issuer, metaClient) if metaErr != nil && !errors.Is(metaErr, oauthproto.ErrRegistrationEndpointMissing) { return nil, fmt.Errorf("dynamic client registration failed: discover authorization server metadata: %w", metaErr) } @@ -825,8 +844,10 @@ func getDiscoveryDocument( }, nil } - // Fall back to discovering endpoints - return oauth.DiscoverOIDCEndpoints(ctx, issuer) + // Fall back to discovering endpoints. This fetch precedes the DCR + // registration this function's callers are about to perform, so it shares + // the same private-IP policy (CWE-918) rather than defaulting to unguarded. + return oauth.DiscoverOIDCEndpoints(ctx, issuer, !config.AllowPrivateIPs) } // createOAuthConfig creates the OAuth configuration based on available endpoints diff --git a/pkg/auth/discovery/discovery_test.go b/pkg/auth/discovery/discovery_test.go index 70f9c03305..a3e6af4b97 100644 --- a/pkg/auth/discovery/discovery_test.go +++ b/pkg/auth/discovery/discovery_test.go @@ -1324,8 +1324,9 @@ func TestHandleDynamicRegistration_MissingRegistrationEndpoint(t *testing.T) { t.Cleanup(server.Close) config := &OAuthFlowConfig{ - Scopes: []string{"openid", "profile"}, - CallbackPort: 8765, + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + AllowPrivateIPs: true, // loopback test server; guard would otherwise refuse to dial it } err := handleDynamicRegistration(context.Background(), server.URL, config) diff --git a/pkg/auth/oauth/oidc.go b/pkg/auth/oauth/oidc.go index d4de7156e9..08fed930d4 100644 --- a/pkg/auth/oauth/oidc.go +++ b/pkg/auth/oauth/oidc.go @@ -20,9 +20,18 @@ import ( "github.com/stacklok/toolhive/pkg/oauthproto" ) -// DiscoverOIDCEndpoints discovers OAuth endpoints from an OIDC issuer -func DiscoverOIDCEndpoints(ctx context.Context, issuer string) (*oauthproto.OIDCDiscoveryDocument, error) { - return discoverOIDCEndpointsWithClient(ctx, issuer, nil, false) +// DiscoverOIDCEndpoints discovers OAuth endpoints from an OIDC issuer. +// +// issuer originates from untrusted remote-server discovery in some callers +// (e.g. the CLI DCR flow's well-known fallback) and from an operator-supplied +// flag in others; blockPrivateIPs lets each caller apply its own SSRF policy +// (CWE-918) rather than this function silently choosing one for everyone. +func DiscoverOIDCEndpoints( + ctx context.Context, + issuer string, + blockPrivateIPs bool, +) (*oauthproto.OIDCDiscoveryDocument, error) { + return discoverOIDCEndpointsWithClient(ctx, issuer, nil, false, blockPrivateIPs) } // DiscoverActualIssuer discovers the actual issuer from a URL that might be different from the issuer itself @@ -41,17 +50,18 @@ func DiscoverActualIssuer( return discoverOIDCEndpointsWithClientAndValidation(ctx, metadataURL, nil, false, false, blockPrivateIPs) } -// discoverOIDCEndpointsWithClient discovers OAuth endpoints from an OIDC issuer with a custom HTTP client (private for testing) +// discoverOIDCEndpointsWithClient discovers OAuth endpoints from an OIDC +// issuer with a custom HTTP client (private for testing). blockPrivateIPs is +// only consulted when client is nil — a caller-supplied client carries its +// own dial policy. func discoverOIDCEndpointsWithClient( ctx context.Context, issuer string, client networking.HTTPClient, insecureAllowHTTP bool, + blockPrivateIPs bool, ) (*oauthproto.OIDCDiscoveryDocument, error) { - // A caller-supplied client carries its own dial policy; the default-client - // private-IP guard only applies when client is nil, so blockPrivateIPs is - // irrelevant here. - return discoverOIDCEndpointsWithClientAndValidation(ctx, issuer, client, true, insecureAllowHTTP, false) + return discoverOIDCEndpointsWithClientAndValidation(ctx, issuer, client, true, insecureAllowHTTP, blockPrivateIPs) } // discoverOIDCEndpointsWithClientAndValidation discovers OAuth endpoints with optional issuer validation @@ -242,8 +252,10 @@ func createOAuthConfigFromOIDCWithClient( resource string, client networking.HTTPClient, ) (*Config, error) { - // Discover OIDC endpoints (insecureAllowHTTP is false for OAuth config creation) - doc, err := discoverOIDCEndpointsWithClient(ctx, issuer, client, false) + // Discover OIDC endpoints (insecureAllowHTTP is false for OAuth config creation). + // blockPrivateIPs=false here preserves this call path's existing behavior; + // it is unrelated to the CLI DCR fallback fixed in DiscoverOIDCEndpoints. + doc, err := discoverOIDCEndpointsWithClient(ctx, issuer, client, false, false) if err != nil { return nil, fmt.Errorf("failed to discover OIDC endpoints: %w", err) } diff --git a/pkg/auth/oauth/oidc_test.go b/pkg/auth/oauth/oidc_test.go index 4a523abbb7..b3a51be416 100644 --- a/pkg/auth/oauth/oidc_test.go +++ b/pkg/auth/oauth/oidc_test.go @@ -1123,7 +1123,7 @@ func TestDiscoverOIDCEndpoints_Production(t *testing.T) { }, } } - doc, err := discoverOIDCEndpointsWithClient(ctx, issuer, client, false) + doc, err := discoverOIDCEndpointsWithClient(ctx, issuer, client, false, false) if tt.expectError { require.Error(t, err) diff --git a/pkg/auth/remote/handler.go b/pkg/auth/remote/handler.go index c5ce0c848c..7786c465b3 100644 --- a/pkg/auth/remote/handler.go +++ b/pkg/auth/remote/handler.go @@ -85,7 +85,7 @@ func (h *Handler) Authenticate(ctx context.Context, remoteURL string) (oauth2.To } // Discover OAuth endpoints once (used by both cached token restore and fresh OAuth) - issuer, scopes, authServerInfo, err := h.discoverIssuerAndScopes(ctx, authInfo, remoteURL) + issuer, scopes, authServerInfo, allowPrivateIPs, err := h.discoverIssuerAndScopes(ctx, authInfo, remoteURL) if err != nil { return nil, err } @@ -103,11 +103,13 @@ func (h *Handler) Authenticate(ctx context.Context, remoteURL string) (oauth2.To } } - // Priority 3: Fresh OAuth authentication flow. Pass the same target-private - // decision the discovery fetches use so DCR (discovery + registration) is - // allowed to reach a private upstream only when the operator-configured - // remote is itself private, and guarded otherwise (CWE-918). - return h.performOAuthFlow(ctx, issuer, scopes, authServerInfo, networking.TargetIsPrivate(ctx, remoteURL)) + // Priority 3: Fresh OAuth authentication flow. Reuse the same target-private + // decision the discovery fetches above already computed, so DCR (discovery + + // registration) is allowed to reach a private upstream only when the + // operator-configured remote is itself private, and guarded otherwise + // (CWE-918) — without a second, later TargetIsPrivate DNS lookup that could + // disagree with the first. + return h.performOAuthFlow(ctx, issuer, scopes, authServerInfo, allowPrivateIPs) } // validateBearerRequirement checks if Bearer auth is required without OAuth fallback @@ -361,13 +363,18 @@ func (h *Handler) discoverIssuerAndScopes( ctx context.Context, authInfo *discovery.AuthInfo, remoteURL string, -) (string, []string, *discovery.AuthServerInfo, error) { +) (string, []string, *discovery.AuthServerInfo, bool, error) { // Decide once whether discovery fetches derived from untrusted server input // (realm, resource_metadata, authorization_servers) may reach private // addresses. If the operator-configured target is itself internal they may; // otherwise block them to contain SSRF (CWE-918). The configured-issuer path // below is exempt because that URL comes from the operator, not the server. - blockPrivateIPs := !networking.TargetIsPrivate(ctx, remoteURL) + // Callers also thread this decision into the DCR flow (see performOAuthFlow), + // so it's computed exactly once per Authenticate call rather than being + // re-derived from a second, later TargetIsPrivate DNS lookup that could + // disagree with this one. + allowPrivateIPs := networking.TargetIsPrivate(ctx, remoteURL) + blockPrivateIPs := !allowPrivateIPs // Priority 1: Use configured issuer if available. Fetch discovery to populate // AuthServerInfo (including ClientIDMetadataDocumentSupported) even when the @@ -375,7 +382,7 @@ func (h *Handler) discoverIssuerAndScopes( if h.config.Issuer != "" { slog.Debug("Using configured issuer", "issuer", h.config.Issuer) authServerInfo, _ := discovery.ValidateAndDiscoverAuthServer(ctx, h.config.Issuer, false) - return h.config.Issuer, h.config.Scopes, authServerInfo, nil + return h.config.Issuer, h.config.Scopes, authServerInfo, allowPrivateIPs, nil } // Priority 2: Try to derive from realm (RFC 8414). Fetch discovery for the @@ -385,7 +392,7 @@ func (h *Handler) discoverIssuerAndScopes( if derivedIssuer != "" { slog.Debug("Derived issuer from realm", "issuer", derivedIssuer) authServerInfo, _ := discovery.ValidateAndDiscoverAuthServer(ctx, derivedIssuer, blockPrivateIPs) - return derivedIssuer, h.config.Scopes, authServerInfo, nil + return derivedIssuer, h.config.Scopes, authServerInfo, allowPrivateIPs, nil } } @@ -393,7 +400,7 @@ func (h *Handler) discoverIssuerAndScopes( if authInfo.ResourceMetadata != "" { issuer, scopes, authServerInfo, err := h.tryDiscoverFromResourceMetadata(ctx, authInfo.ResourceMetadata, blockPrivateIPs) if err == nil { - return issuer, scopes, authServerInfo, nil + return issuer, scopes, authServerInfo, allowPrivateIPs, nil } slog.Debug("Resource metadata discovery failed, falling through to well-known discovery", "error", err) } @@ -402,7 +409,7 @@ func (h *Handler) discoverIssuerAndScopes( // This handles cases where the issuer differs from the server URL (e.g., Atlassian) issuer, scopes, authServerInfo, err := h.tryDiscoverFromWellKnown(ctx, remoteURL, blockPrivateIPs) if err == nil { - return issuer, scopes, authServerInfo, nil + return issuer, scopes, authServerInfo, allowPrivateIPs, nil } slog.Debug("Could not discover from well-known endpoint", "error", err) @@ -410,11 +417,11 @@ func (h *Handler) discoverIssuerAndScopes( derivedIssuer := discovery.DeriveIssuerFromURL(remoteURL) if derivedIssuer != "" { slog.Debug("Using derived issuer from URL", "issuer", derivedIssuer) - return derivedIssuer, h.config.Scopes, nil, nil + return derivedIssuer, h.config.Scopes, nil, allowPrivateIPs, nil } // No issuer could be determined - return "", nil, nil, fmt.Errorf("could not determine OAuth issuer. Please provide issuer in configuration, " + + return "", nil, nil, false, fmt.Errorf("could not determine OAuth issuer. Please provide issuer in configuration, " + "or ensure the server provides a valid realm parameter or resource_metadata URL in the WWW-Authenticate header") } diff --git a/pkg/auth/remote/handler_test.go b/pkg/auth/remote/handler_test.go index 2aa4000139..ae7bfe58c4 100644 --- a/pkg/auth/remote/handler_test.go +++ b/pkg/auth/remote/handler_test.go @@ -274,7 +274,7 @@ func TestDiscoverIssuerAndScopes(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) defer cancel() - issuer, scopes, authServerInfo, err := handler.discoverIssuerAndScopes( + issuer, scopes, authServerInfo, _, err := handler.discoverIssuerAndScopes( ctx, authInfo, remoteURL, @@ -388,7 +388,7 @@ func TestDiscoverIssuerAndScopes_Security(t *testing.T) { } ctx := t.Context() - issuer, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") + issuer, _, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") require.NoError(t, err) // The path traversal should be normalized @@ -410,7 +410,7 @@ func TestDiscoverIssuerAndScopes_Security(t *testing.T) { defer mockServer.Close() ctx := t.Context() - issuer, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, mockServer.URL) + issuer, _, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, mockServer.URL) require.NoError(t, err) // Should not use the insecure realm, should fall through @@ -444,7 +444,7 @@ func TestDiscoverIssuerAndScopes_Security(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 1*time.Second) defer cancel() - issuer, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") + issuer, _, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") // Should not hang or crash; Priority 3 fails gracefully and falls through to URL-derived issuer require.NoError(t, err) @@ -532,7 +532,7 @@ func TestDiscoveryPriorityChain(t *testing.T) { } ctx := context.Background() - issuer, scopes, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") + issuer, scopes, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") require.NoError(t, err) assert.Equal(t, "https://configured.example.com", issuer) @@ -551,7 +551,7 @@ func TestDiscoveryPriorityChain(t *testing.T) { } ctx := context.Background() - issuer, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") + issuer, _, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") require.NoError(t, err) assert.Equal(t, "https://realm.example.com/oauth", issuer) @@ -569,7 +569,7 @@ func TestDiscoveryPriorityChain(t *testing.T) { } ctx := context.Background() - issuer, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") + issuer, _, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com") require.NoError(t, err) // Should fall through to URL-derived issuer @@ -587,7 +587,7 @@ func TestDiscoveryPriorityChain(t *testing.T) { } ctx := context.Background() - issuer, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com/path") + issuer, _, _, _, err := handler.discoverIssuerAndScopes(ctx, authInfo, "https://server.example.com/path") require.NoError(t, err) assert.Equal(t, "https://server.example.com", issuer) diff --git a/pkg/authserver/upstream/oauth2.go b/pkg/authserver/upstream/oauth2.go index 32369043a6..db9ba54cd5 100644 --- a/pkg/authserver/upstream/oauth2.go +++ b/pkg/authserver/upstream/oauth2.go @@ -832,6 +832,17 @@ func formatOAuth2Error(err error, prefix string) error { // // The host-scoped guard policy lives in networking.NewHostScopedClientBuilder // so this provider path and the DCR resolver share one implementation. +// +// Unlike the DCR resolver's guarded client, this one deliberately leaves +// keep-alives enabled: the returned client is stored on BaseOAuth2Provider and +// reused for many token-refresh/userinfo calls over the provider's lifetime +// against a single operator-configured host, so disabling keep-alives here +// would pay a fresh TCP+TLS handshake on every call. This trades a narrower +// window — a DNS change for that fixed host between connection reuses is not +// re-checked mid-lifetime — for avoiding that cost on a hot path; the DCR +// resolver's per-request-host, low-frequency calls don't have the same +// trade-off, hence the difference. If this provider's threat model changes +// (e.g. it starts dialing caller-varying hosts), revisit this decision. func newHTTPClientForHost(host string, allowPrivateIPs, insecureAllowHTTP bool) (*http.Client, error) { return networking.NewHostScopedClientBuilder(host, allowPrivateIPs, insecureAllowHTTP).Build() } diff --git a/pkg/registry/auth/login.go b/pkg/registry/auth/login.go index 3bb4447c09..b952d78470 100644 --- a/pkg/registry/auth/login.go +++ b/pkg/registry/auth/login.go @@ -290,7 +290,10 @@ func ConfigureOAuth( discoveryCtx, cancel := context.WithTimeout(ctx, 10*time.Second) defer cancel() - if _, err := oauth.DiscoverOIDCEndpoints(discoveryCtx, issuer); err != nil { + // blockPrivateIPs=false preserves this call path's existing behavior: + // issuer is an operator-supplied flag here, not remote-server-derived + // discovery input, unlike the CLI DCR flow's use of DiscoverOIDCEndpoints. + if _, err := oauth.DiscoverOIDCEndpoints(discoveryCtx, issuer, false); err != nil { return nil, fmt.Errorf("OIDC discovery failed for issuer %s: %w", issuer, err) } From 06a954e48fde6d85079a016679492d5fa3454927 Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 16 Jul 2026 08:58:01 -0700 Subject: [PATCH 4/7] Add test coverage for SSRF guard threading and discovery path Addresses stacklok/toolhive#5826 review comments: - MEDIUM pkg/auth/dcr/resolver_test.go (3594946001): prove the discovery client's own dial guard directly (a DiscoveryURL pointing at a private IP), not just inferred from the registration-endpoint case - MEDIUM pkg/auth/dcr/resolver_test.go (3596751597): prove AllowPrivateIPs=true also lifts the guard via the discovery-indirection path, not just the direct RegistrationEndpoint branch - MEDIUM pkg/authserver/runner/dcr_adapter_test.go, pkg/auth/remote/handler_test.go, pkg/auth/discovery/dcr_resolver_test.go (3596751603): assert AllowPrivateIPs propagates correctly at each threading point (dcr_adapter's newDCRRequest, handler's buildOAuthFlowConfig, and discovery's metadata re-fetch), so a future refactor that drops it fails a test instead of silently regressing - MEDIUM pkg/networking/http_client_test.go (3596751608): add a direct table-driven test for NewHostScopedClientBuilder's own decision table Co-Authored-By: Claude Sonnet 5 --- pkg/auth/dcr/resolver_test.go | 62 ++++++++++++++++++++ pkg/auth/discovery/dcr_resolver_test.go | 26 +++++++++ pkg/auth/remote/handler_test.go | 18 ++++++ pkg/authserver/runner/dcr_adapter_test.go | 20 +++++++ pkg/networking/http_client_test.go | 69 +++++++++++++++++++++++ 5 files changed, 195 insertions(+) diff --git a/pkg/auth/dcr/resolver_test.go b/pkg/auth/dcr/resolver_test.go index bedc084e21..0f380157bd 100644 --- a/pkg/auth/dcr/resolver_test.go +++ b/pkg/auth/dcr/resolver_test.go @@ -405,6 +405,24 @@ func TestResolveDCRCredentials_BlocksPrivateIPTargets(t *testing.T) { } }, }, + { + // Discovery-fetch branch itself: DiscoveryURL points directly at a + // private IP, so the *discovery* client's own dial guard + // (newGuardedDCRClient(discoveryHost, ...) in resolveDCREndpoints) + // must refuse it. The two cases above only prove the guard on the + // registration client; this proves it on the discovery client. No + // live server is needed — the guard fires before any request is + // sent, and deriveExpectedIssuerFromDiscoveryURL resolves the + // issuer from the URL string alone. + name: "discovery URL itself on a private IP", + newReq: func(_ *testing.T) *Request { + return &Request{ + Issuer: "https://block-discovery.example.test", + Scopes: []string{"openid"}, + DiscoveryURL: "https://10.255.255.1/.well-known/oauth-authorization-server", + } + }, + }, } for _, tc := range tests { @@ -446,6 +464,50 @@ func TestResolveDCRCredentials_AllowPrivateIPsHonored(t *testing.T) { "AllowPrivateIPs=true must lift the guard so the dial is attempted, not refused") } +// TestResolveDCRCredentials_AllowPrivateIPsHonoredViaDiscovery proves +// req.AllowPrivateIPs also lifts the guard on the discovery-indirection +// path — not just the direct RegistrationEndpoint branch covered by +// TestResolveDCRCredentials_AllowPrivateIPsHonored above: with it set, a +// registration_endpoint the discovery document points at a private IP is +// dialed instead of refused at the guard. +func TestResolveDCRCredentials_AllowPrivateIPsHonoredViaDiscovery(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + mux := http.NewServeMux() + var server *httptest.Server + mux.HandleFunc("/.well-known/oauth-authorization-server", + func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(oauthproto.AuthorizationServerMetadata{ + Issuer: server.URL, + AuthorizationEndpoint: server.URL + "/authorize", + TokenEndpoint: server.URL + "/token", + JWKSURI: server.URL + "/jwks", + // Non-routable RFC 5737 documentation address (TEST-NET-1): the + // dial fails with a network error rather than reaching a real + // host, same as TestResolveDCRCredentials_AllowPrivateIPsHonored. + RegistrationEndpoint: "https://192.0.2.1/register", + }) + }) + server = httptest.NewServer(mux) + t.Cleanup(server.Close) + + req := &Request{ + Issuer: server.URL, + Scopes: []string{"openid"}, + DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server", + AllowPrivateIPs: true, + } + + _, err := ResolveCredentials(ctx, req, newMemoryDCRStore(t)) + require.Error(t, err, "the dead documentation address cannot complete registration") + assert.NotContains(t, err.Error(), networking.ErrPrivateIpAddress, + "AllowPrivateIPs=true must lift the guard on the discovery-indirection path too") +} + // TestResolveDCRCredentials_DiscoveryRefusesCrossHostRedirect pins that the // discovery fetch installs SameHostRedirectPolicy: a discovery endpoint that // 30x-redirects to a different host must not be followed, so a malicious diff --git a/pkg/auth/discovery/dcr_resolver_test.go b/pkg/auth/discovery/dcr_resolver_test.go index 2e4c289632..d5227de914 100644 --- a/pkg/auth/discovery/dcr_resolver_test.go +++ b/pkg/auth/discovery/dcr_resolver_test.go @@ -244,6 +244,32 @@ func TestHandleDynamicRegistration_MetadataRefetchBlocksPrivateIP(t *testing.T) "the metadata re-fetch must be guarded the same as the resolver's own outbound calls") } +// TestHandleDynamicRegistration_MetadataRefetchAllowPrivateIPsHonored proves +// OAuthFlowConfig.AllowPrivateIPs lifts the guard on the same metadata +// re-fetch pinned as blocked-by-default above. The target is a non-routable +// RFC 5737 documentation address (TEST-NET-1), so the dial fails with a +// network error rather than the guard error — and can never reach a real host. +func TestHandleDynamicRegistration_MetadataRefetchAllowPrivateIPsHonored(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + config := &OAuthFlowConfig{ + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + AuthorizeURL: "https://192.0.2.1/authorize", + TokenURL: "https://192.0.2.1/token", + RegistrationEndpoint: "https://192.0.2.1/register", + AllowPrivateIPs: true, + } + + err := handleDynamicRegistration(ctx, "https://192.0.2.1", config) + require.Error(t, err, "the dead documentation address cannot complete registration") + assert.NotContains(t, err.Error(), networking.ErrPrivateIpAddress, + "AllowPrivateIPs=true must lift the guard on the metadata re-fetch") +} + // TestHandleDynamicRegistration_InheritsSingleflightDedup verifies that // N concurrent handleDynamicRegistration calls for the same (issuer, // scopes, redirectURI) tuple coalesce into exactly one upstream /register diff --git a/pkg/auth/remote/handler_test.go b/pkg/auth/remote/handler_test.go index ae7bfe58c4..3a98f0cd14 100644 --- a/pkg/auth/remote/handler_test.go +++ b/pkg/auth/remote/handler_test.go @@ -1011,3 +1011,21 @@ func TestResolveClientCredentials(t *testing.T) { }) } } + +// TestBuildOAuthFlowConfig_ThreadsAllowPrivateIPs pins that performOAuthFlow's +// allowPrivateIPs parameter reaches the built OAuthFlowConfig. Without this, +// a future refactor could silently drop the SSRF-guard decision computed in +// Authenticate/discoverIssuerAndScopes without any test failing. +func TestBuildOAuthFlowConfig_ThreadsAllowPrivateIPs(t *testing.T) { + t.Parallel() + + h := &Handler{config: &Config{}} + + flowConfig := h.buildOAuthFlowConfig([]string{"openid"}, nil, true) + assert.True(t, flowConfig.AllowPrivateIPs, + "allowPrivateIPs=true must reach OAuthFlowConfig.AllowPrivateIPs") + + flowConfig = h.buildOAuthFlowConfig([]string{"openid"}, nil, false) + assert.False(t, flowConfig.AllowPrivateIPs, + "allowPrivateIPs=false must reach OAuthFlowConfig.AllowPrivateIPs") +} diff --git a/pkg/authserver/runner/dcr_adapter_test.go b/pkg/authserver/runner/dcr_adapter_test.go index 3a4e7815c1..d9126b9091 100644 --- a/pkg/authserver/runner/dcr_adapter_test.go +++ b/pkg/authserver/runner/dcr_adapter_test.go @@ -247,6 +247,7 @@ func TestNewDCRRequest(t *testing.T) { wantInitialAccessToken string wantDiscoveryURL string wantRegistration string + wantAllowPrivateIPs bool }{ { name: "discovery_url branch resolves file-based initial access token", @@ -274,6 +275,24 @@ func TestNewDCRRequest(t *testing.T) { wantIssuer: "https://thv.example.com", wantRegistration: "https://idp.example.com/register", }, + { + // Pins the AllowPrivateIPs one-line copy from + // OAuth2UpstreamRunConfig onto dcr.Request: without this test, a + // future refactor could silently drop or invert the SSRF-guard + // decision without any test failing. + name: "AllowPrivateIPs propagates from run-config", + rc: &authserver.OAuth2UpstreamRunConfig{ + Scopes: []string{"openid"}, + DCRConfig: &authserver.DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + }, + AllowPrivateIPs: true, + }, + localIssuer: "https://thv.example.com", + wantIssuer: "https://thv.example.com", + wantRegistration: "https://idp.example.com/register", + wantAllowPrivateIPs: true, + }, { name: "nil run-config rejected", rc: nil, @@ -302,6 +321,7 @@ func TestNewDCRRequest(t *testing.T) { assert.Equal(t, tc.wantInitialAccessToken, req.InitialAccessToken) assert.Equal(t, tc.wantDiscoveryURL, req.DiscoveryURL) assert.Equal(t, tc.wantRegistration, req.RegistrationEndpoint) + assert.Equal(t, tc.wantAllowPrivateIPs, req.AllowPrivateIPs) }) } } diff --git a/pkg/networking/http_client_test.go b/pkg/networking/http_client_test.go index 7cbc818aa4..fb6f2c85c7 100644 --- a/pkg/networking/http_client_test.go +++ b/pkg/networking/http_client_test.go @@ -33,6 +33,75 @@ func TestNewHttpClientBuilder(t *testing.T) { assert.False(t, builder.allowPrivate) } +func TestNewHostScopedClientBuilder(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + host string + allowPrivateIPs bool + insecureAllowHTTP bool + wantAllowPrivate bool + wantInsecureHTTP bool + }{ + { + name: "external host, guarded by default", + host: "idp.example.com", + }, + { + name: "external host, allowPrivateIPs widens only the private-IP gate", + host: "idp.example.com", + allowPrivateIPs: true, + wantAllowPrivate: true, + wantInsecureHTTP: false, + }, + { + name: "external host, insecureAllowHTTP widens both gates", + host: "idp.example.com", + insecureAllowHTTP: true, + wantAllowPrivate: true, + wantInsecureHTTP: true, + }, + { + name: "loopback host is exempted from both gates regardless of flags", + host: "127.0.0.1:8443", + wantAllowPrivate: true, + wantInsecureHTTP: true, + }, + { + name: "loopback host stays exempted even with allowPrivateIPs also set", + host: "localhost", + allowPrivateIPs: true, + wantAllowPrivate: true, + wantInsecureHTTP: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + builder := NewHostScopedClientBuilder(tt.host, tt.allowPrivateIPs, tt.insecureAllowHTTP) + + assert.Equal(t, tt.wantAllowPrivate, builder.allowPrivate, "allowPrivate mismatch") + assert.Equal(t, tt.wantInsecureHTTP, builder.insecureAllowHTTP, "insecureAllowHTTP mismatch") + }) + } +} + +// TestNewHostScopedClientBuilder_InsecureDisableURLValidationEnvVar pins that +// the env var widens both gates the same way a loopback host does. Kept as a +// standalone test (not a table case) because t.Setenv is incompatible with +// t.Parallel. +func TestNewHostScopedClientBuilder_InsecureDisableURLValidationEnvVar(t *testing.T) { + t.Setenv("INSECURE_DISABLE_URL_VALIDATION", "true") + + builder := NewHostScopedClientBuilder("idp.example.com", false, false) + + assert.True(t, builder.allowPrivate, "env var must widen the private-IP gate") + assert.True(t, builder.insecureAllowHTTP, "env var must widen the HTTP scheme gate") +} + func TestHttpClientBuilder_WithCABundle(t *testing.T) { t.Parallel() From c6d348648a5cce31c8bc453cc0745e77937dcca5 Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 16 Jul 2026 09:01:03 -0700 Subject: [PATCH 5/7] Tidy duplicated/stale SSRF-guard documentation Addresses stacklok/toolhive#5826 review comments: - LOW pkg/auth/dcr/request.go, pkg/auth/dcr/resolver.go (3596751588): trim the CWE-918 rationale restated across dcr.Request.AllowPrivateIPs and newGuardedDCRClient's doc comments; networking.NewHostScopedClientBuilder is now the single pointed-to explanation of the guard policy itself - INFO pkg/authserver/config.go (body:doc-allowprivate): note that OAuth2UpstreamRunConfig.AllowPrivateIPs now also gates DCR discovery and registration calls, not just the token/userinfo client Co-Authored-By: Claude Sonnet 5 --- pkg/auth/dcr/request.go | 28 ++++++++-------------------- pkg/auth/dcr/resolver.go | 23 +++++------------------ pkg/authserver/config.go | 12 ++++++++---- 3 files changed, 21 insertions(+), 42 deletions(-) diff --git a/pkg/auth/dcr/request.go b/pkg/auth/dcr/request.go index 07c3db4d04..1f4b84edc1 100644 --- a/pkg/auth/dcr/request.go +++ b/pkg/auth/dcr/request.go @@ -135,25 +135,13 @@ type Request struct { // AllowPrivateIPs permits both of the resolver's outbound calls — the // discovery fetch to DiscoveryURL and the registration POST to the - // resolved registration endpoint — to connect to private IP ranges - // (RFC-1918, loopback, link-local). Both calls are otherwise dialed - // through a private-IP-guarded client that refuses such addresses at - // connect time, closing the CWE-918 SSRF vectors that a DCR upstream - // would otherwise open: a discovery document that points - // registration_endpoint at an in-cluster service or a link-local - // metadata address, and DNS rebinding of an endpoint that was public - // when the caller validated it. Loopback hosts remain permitted for - // development/testing regardless of this flag; the check runs on the - // address actually dialed, after DNS resolution, so it also defends - // against rebinding. - // - // Set this to true only when the upstream authorization server is - // reachable solely over a private address (e.g. an in-cluster IdP with - // no public endpoint). HTTPS-scheme enforcement is unchanged — HTTPS is - // still required for non-loopback hosts. Defaults to false. - // - // Mirrors the AllowPrivateIPs posture already carried by the OAuth2 and - // OIDC upstream configs so a DCR upstream is not the one outbound-facing - // path without an SSRF guard. + // resolved registration endpoint — to connect to private IP ranges. + // See networking.NewHostScopedClientBuilder for the CWE-918 guard policy + // this widens (loopback exemption, dial-time re-check, etc.). Set this to + // true only when the upstream authorization server is reachable solely + // over a private address (e.g. an in-cluster IdP with no public + // endpoint); defaults to false. Mirrors the AllowPrivateIPs posture + // already carried by the OAuth2 and OIDC upstream configs so a DCR + // upstream is not the one outbound-facing path without an SSRF guard. AllowPrivateIPs bool } diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index 809d19d84f..baa9423b79 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -1337,25 +1337,12 @@ func newDCRHTTPClient(initialAccessToken, registrationEndpoint string, allowPriv // newGuardedDCRClient builds the private-IP-guarded *http.Client used for both // of the resolver's outbound calls — the discovery fetch and the registration -// POST. It dials through networking's protected dialer so a host that resolves -// to a private, loopback, or link-local address is refused at connect time -// (CWE-918), closing both the discovery-indirection and DNS-rebinding SSRF -// vectors: the check runs on the address actually dialed, after DNS -// resolution, and — with keep-alives disabled — on every request rather than -// being bypassed by a pooled connection. +// POST. See networking.NewHostScopedClientBuilder for the CWE-918 guard policy +// this applies (allowPrivateIPs semantics, loopback exemption, HTTPS +// enforcement). Keep-alives are disabled so the dial-time check re-runs on +// every request rather than being bypassed by a pooled connection. // -// allowPrivateIPs widens only the private-IP gate (for an in-cluster upstream -// reachable solely over an RFC-1918 address); loopback hosts remain permitted -// for development regardless, matching networking.NewHostScopedClientBuilder -// and the AllowPrivateIPs posture of the OAuth2/OIDC upstream configs. HTTP -// scheme enforcement is left to the resolver's URL validation and the builder's -// ValidatingTransport (HTTPS-except-loopback). -// -// The builder's 30 s overall / 10 s TLS / 10 s response-header default -// timeouts match the bounds previously sourced from -// oauthproto.NewDefaultDCRClient. -// -// The returned client has no CheckRedirect policy; each caller layers its own +// The returned client has no CheckRedirect policy: each caller layers its own // (the registration client refuses all redirects to protect the bearer token; // the discovery client restricts them to the same host). The dial guard alone // does not stop a redirect to a different public host, so the redirect policy diff --git a/pkg/authserver/config.go b/pkg/authserver/config.go index 92ff27a8e2..7a2d073575 100644 --- a/pkg/authserver/config.go +++ b/pkg/authserver/config.go @@ -394,10 +394,14 @@ type OAuth2UpstreamRunConfig struct { DCRConfig *DCRUpstreamConfig `json:"dcr_config,omitempty" yaml:"dcr_config,omitempty"` // AllowPrivateIPs permits the upstream provider's HTTP client to connect to - // private IP ranges (RFC-1918, link-local). Use only when the upstream is - // hosted inside the same cluster and has no public endpoint. HTTP-scheme - // restrictions are unchanged — HTTPS is still required for non-localhost hosts. - // Defaults to false. + // private IP ranges (RFC-1918, link-local). When DCRConfig is set, this + // also gates the DCR discovery and registration calls made on this + // upstream's behalf (see pkg/authserver/runner/dcr_adapter.go), so a + // single flag covers the whole upstream rather than needing a separate + // DCR-specific setting. Use only when the upstream is hosted inside the + // same cluster and has no public endpoint. HTTP-scheme restrictions are + // unchanged — HTTPS is still required for non-localhost hosts. Defaults + // to false. AllowPrivateIPs bool `json:"allow_private_ips,omitempty" yaml:"allow_private_ips,omitempty"` // InsecureAllowHTTP permits plain-HTTP authorization and token endpoint URLs From b682e18a1d4be3cffcd0378b49ac2e62ce59b11b Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 16 Jul 2026 09:08:19 -0700 Subject: [PATCH 6/7] Fix codespell keep-alives typo flagged by CI Codespell flags "keep-alives" as a misspelling of "keep-alive"; reword the two doc comments added in the SSRF-guard fix to use the singular form instead. Co-Authored-By: Claude Sonnet 5 --- pkg/auth/dcr/resolver.go | 2 +- pkg/authserver/upstream/oauth2.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index baa9423b79..519d9f23d5 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -1339,7 +1339,7 @@ func newDCRHTTPClient(initialAccessToken, registrationEndpoint string, allowPriv // of the resolver's outbound calls — the discovery fetch and the registration // POST. See networking.NewHostScopedClientBuilder for the CWE-918 guard policy // this applies (allowPrivateIPs semantics, loopback exemption, HTTPS -// enforcement). Keep-alives are disabled so the dial-time check re-runs on +// enforcement). Keep-alive is disabled so the dial-time check re-runs on // every request rather than being bypassed by a pooled connection. // // The returned client has no CheckRedirect policy: each caller layers its own diff --git a/pkg/authserver/upstream/oauth2.go b/pkg/authserver/upstream/oauth2.go index db9ba54cd5..dd545829eb 100644 --- a/pkg/authserver/upstream/oauth2.go +++ b/pkg/authserver/upstream/oauth2.go @@ -834,9 +834,9 @@ func formatOAuth2Error(err error, prefix string) error { // so this provider path and the DCR resolver share one implementation. // // Unlike the DCR resolver's guarded client, this one deliberately leaves -// keep-alives enabled: the returned client is stored on BaseOAuth2Provider and +// keep-alive enabled: the returned client is stored on BaseOAuth2Provider and // reused for many token-refresh/userinfo calls over the provider's lifetime -// against a single operator-configured host, so disabling keep-alives here +// against a single operator-configured host, so disabling keep-alive here // would pay a fresh TCP+TLS handshake on every call. This trades a narrower // window — a DNS change for that fixed host between connection reuses is not // re-checked mid-lifetime — for avoiding that cost on a hot path; the DCR From a16730ae979790954e5a01d8818383c7968b9c76 Mon Sep 17 00:00:00 2001 From: Trey Date: Thu, 16 Jul 2026 09:14:39 -0700 Subject: [PATCH 7/7] Regenerate swagger docs for AllowPrivateIPs doc comment task docs was missed after updating the OAuth2UpstreamRunConfig.AllowPrivateIPs doc comment, which swag embeds verbatim as the allow_private_ips schema description in docs/server/. Co-Authored-By: Claude Sonnet 5 --- docs/server/docs.go | 2 +- docs/server/swagger.json | 2 +- docs/server/swagger.yaml | 12 ++++++++---- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/docs/server/docs.go b/docs/server/docs.go index 267f322c79..1b56ae03b3 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -423,7 +423,7 @@ const docTemplate = `{ "type": "object" }, "allow_private_ips": { - "description": "AllowPrivateIPs permits the upstream provider's HTTP client to connect to\nprivate IP ranges (RFC-1918, link-local). Use only when the upstream is\nhosted inside the same cluster and has no public endpoint. HTTP-scheme\nrestrictions are unchanged — HTTPS is still required for non-localhost hosts.\nDefaults to false.", + "description": "AllowPrivateIPs permits the upstream provider's HTTP client to connect to\nprivate IP ranges (RFC-1918, link-local). When DCRConfig is set, this\nalso gates the DCR discovery and registration calls made on this\nupstream's behalf (see pkg/authserver/runner/dcr_adapter.go), so a\nsingle flag covers the whole upstream rather than needing a separate\nDCR-specific setting. Use only when the upstream is hosted inside the\nsame cluster and has no public endpoint. HTTP-scheme restrictions are\nunchanged — HTTPS is still required for non-localhost hosts. Defaults\nto false.", "type": "boolean" }, "authorization_endpoint": { diff --git a/docs/server/swagger.json b/docs/server/swagger.json index a68ec0aa8c..7baa02d831 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -416,7 +416,7 @@ "type": "object" }, "allow_private_ips": { - "description": "AllowPrivateIPs permits the upstream provider's HTTP client to connect to\nprivate IP ranges (RFC-1918, link-local). Use only when the upstream is\nhosted inside the same cluster and has no public endpoint. HTTP-scheme\nrestrictions are unchanged — HTTPS is still required for non-localhost hosts.\nDefaults to false.", + "description": "AllowPrivateIPs permits the upstream provider's HTTP client to connect to\nprivate IP ranges (RFC-1918, link-local). When DCRConfig is set, this\nalso gates the DCR discovery and registration calls made on this\nupstream's behalf (see pkg/authserver/runner/dcr_adapter.go), so a\nsingle flag covers the whole upstream rather than needing a separate\nDCR-specific setting. Use only when the upstream is hosted inside the\nsame cluster and has no public endpoint. HTTP-scheme restrictions are\nunchanged — HTTPS is still required for non-localhost hosts. Defaults\nto false.", "type": "boolean" }, "authorization_endpoint": { diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index b22c644e5e..a8c0a7ff31 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -469,10 +469,14 @@ components: allow_private_ips: description: |- AllowPrivateIPs permits the upstream provider's HTTP client to connect to - private IP ranges (RFC-1918, link-local). Use only when the upstream is - hosted inside the same cluster and has no public endpoint. HTTP-scheme - restrictions are unchanged — HTTPS is still required for non-localhost hosts. - Defaults to false. + private IP ranges (RFC-1918, link-local). When DCRConfig is set, this + also gates the DCR discovery and registration calls made on this + upstream's behalf (see pkg/authserver/runner/dcr_adapter.go), so a + single flag covers the whole upstream rather than needing a separate + DCR-specific setting. Use only when the upstream is hosted inside the + same cluster and has no public endpoint. HTTP-scheme restrictions are + unchanged — HTTPS is still required for non-localhost hosts. Defaults + to false. type: boolean authorization_endpoint: description: AuthorizationEndpoint is the URL for the OAuth authorization