Guard upstream-DCR HTTP calls against SSRF

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)

pkg/auth/dcr/request.go

@@ -132,4 +132,16 @@ 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.
+	// 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
 }

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,30 @@ 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)
+	}
+	// 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)
 }
 
@@ -1029,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{
@@ -1263,27 +1295,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 +1332,44 @@ 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. See networking.NewHostScopedClientBuilder for the CWE-918 guard policy
+// this applies (allowPrivateIPs semantics, loopback exemption, HTTPS
+// 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
+// (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).
+		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 — 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 {
+		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
 }