From ec898fc358b22d6a37be49934bd4fa836fd710e0 Mon Sep 17 00:00:00 2001 From: Joris Scharp Date: Mon, 11 May 2026 18:51:30 +0200 Subject: [PATCH 1/3] refactor: centralise outbound URL validation in StrictHTTPClient Move core.ParsePublicURL from per-caller into StrictHTTPClient.Do so every outbound HTTP request gets HTTPS-only + no-IP + no-RFC2606-reserved-host validation in strict mode. Add CheckRedirect to re-validate every redirect target (10-hop cap matches net/http's default). Removes the duplicated validation in auth/openid4vci.Client, auth/client/iam.HTTPClient.ClientMetadata, and the scheme-only check in auth/services/oauth/relying_party.RequestRFC003AccessToken. All production callers route through *StrictHTTPClient. Behavior change: strict mode now rejects IP literals and RFC 2606 reserved hostnames on every outbound HTTP request, not just OpenID4VCI. Breaking API: oauth.NewRelyingParty no longer accepts a strictMode bool parameter; strict-mode behaviour is now controlled exclusively by the http/client.StrictMode flag set by the HTTP engine at startup. Closes #4244 --- auth/auth.go | 4 +- auth/client/iam/client.go | 6 - auth/openid4vci/client.go | 42 +----- auth/openid4vci/client_test.go | 35 ++--- auth/services/oauth/relying_party.go | 11 +- auth/services/oauth/relying_party_test.go | 37 ----- docs/pages/deployment/configuration.rst | 4 +- http/client/client.go | 35 +++-- http/client/client_test.go | 172 +++++++++++++++++++++- vcr/vcr_test.go | 4 +- 10 files changed, 225 insertions(+), 125 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 52528e4f0e..b3fd95d1cb 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -181,13 +181,13 @@ func (auth *Auth) Configure(config core.ServerConfig) error { // auth.http.config got deprecated in favor of httpclient.timeout auth.httpClientTimeout = config.HTTPClient.Timeout } - auth.openID4VCIClient = openid4vci.NewClient(httpclient.NewWithCache(auth.httpClientTimeout), auth.strictMode) + auth.openID4VCIClient = openid4vci.NewClient(httpclient.NewWithCache(auth.httpClientTimeout)) // V1 API related stuff accessTokenLifeSpan := time.Duration(auth.config.AccessTokenLifeSpan) * time.Second auth.authzServer = oauth.NewAuthorizationServer(auth.vdrInstance.Resolver(), auth.vcr, auth.vcr.Verifier(), auth.serviceResolver, auth.keyStore, auth.contractNotary, auth.jsonldManager, accessTokenLifeSpan) auth.relyingParty = oauth.NewRelyingParty(auth.vdrInstance.Resolver(), auth.serviceResolver, - auth.keyStore, auth.vcr.Wallet(), auth.httpClientTimeout, auth.tlsConfig, config.Strictmode, auth.pkiProvider) + auth.keyStore, auth.vcr.Wallet(), auth.httpClientTimeout, auth.tlsConfig, auth.pkiProvider) if err := auth.authzServer.Configure(auth.config.ClockSkew, config.Strictmode); err != nil { return err diff --git a/auth/client/iam/client.go b/auth/client/iam/client.go index 3112ede9bf..a9d441f862 100644 --- a/auth/client/iam/client.go +++ b/auth/client/iam/client.go @@ -85,12 +85,6 @@ func (hb HTTPClient) OAuthAuthorizationServerMetadata(ctx context.Context, oauth // ClientMetadata retrieves the client metadata from the client metadata endpoint given in the authorization request. // We use the AuthorizationServerMetadata struct since it overlaps greatly with the client metadata. func (hb HTTPClient) ClientMetadata(ctx context.Context, endpoint string) (*oauth.OAuthClientMetadata, error) { - _, err := core.ParsePublicURL(endpoint, hb.strictMode) - if err != nil { - return nil, err - } - - // create a GET request request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return nil, err diff --git a/auth/openid4vci/client.go b/auth/openid4vci/client.go index 23f34b70e0..ac74a8d08d 100644 --- a/auth/openid4vci/client.go +++ b/auth/openid4vci/client.go @@ -73,44 +73,18 @@ type Client interface { } // NewClient returns a Client backed by the provided HTTP request doer. -// In production callers should pass *httpclient.StrictHTTPClient so the -// shared transport policies apply (HTTPS-in-strict, body size limit, -// User-Agent header). -// -// When strictMode is true, target URLs are additionally validated via -// core.ParsePublicURL: HTTPS scheme, no IP hosts, no reserved hostnames. -func NewClient(httpClient core.HTTPRequestDoer, strictMode bool) Client { - return &client{httpClient: httpClient, strictMode: strictMode} +// Production callers should pass *httpclient.StrictHTTPClient so the shared +// transport policies apply: HTTPS-only in strict mode, no IP/reserved hosts +// in strict mode (via core.ParsePublicURL), body size limit, User-Agent. +func NewClient(httpClient core.HTTPRequestDoer) Client { + return &client{httpClient: httpClient} } type client struct { httpClient core.HTTPRequestDoer - strictMode bool -} - -// validateURL guards against SSRF by rejecting target URLs that fail -// core.ParsePublicURL (in strict mode: HTTPS only, no IP/reserved hosts). -// Called at the entry of every method that makes outbound HTTP. -// -// TODO: this validation belongs on httpclient.StrictHTTPClient so every -// outbound HTTP call (not just OpenID4VCI) gets the IP/reserved-host check, -// not only the HTTPS scheme check that StrictHTTPClient.Do enforces today. -// Placed here for now to preserve parity with master, where the equivalent -// caller (auth/client/iam.HTTPClient) validated via oauth.IssuerIdToWellKnown -// → core.ParsePublicURL before issuing the request, and to address a CodeQL -// SSRF finding on this PR. Tracked as a follow-up to consolidate the check -// in the shared HTTP client. -func (c *client) validateURL(name, target string) error { - if _, err := core.ParsePublicURL(target, c.strictMode); err != nil { - return fmt.Errorf("openid4vci: invalid %s URL: %w", name, err) - } - return nil } func (c *client) OpenIDCredentialIssuerMetadata(ctx context.Context, issuerURL string) (*OpenIDCredentialIssuerMetadata, error) { - if err := c.validateURL("issuer", issuerURL); err != nil { - return nil, err - } // Per §12.2.1, the Credential Issuer Identifier MUST NOT contain query // or fragment components. if parsed, _ := url.Parse(issuerURL); parsed != nil && (parsed.RawQuery != "" || parsed.Fragment != "") { @@ -145,9 +119,6 @@ func (c *client) OpenIDCredentialIssuerMetadata(ctx context.Context, issuerURL s } func (c *client) RequestNonce(ctx context.Context, nonceEndpoint string) (string, error) { - if err := c.validateURL("nonce endpoint", nonceEndpoint); err != nil { - return "", err - } req, err := http.NewRequestWithContext(ctx, http.MethodPost, nonceEndpoint, http.NoBody) if err != nil { return "", err @@ -171,9 +142,6 @@ func (c *client) RequestNonce(ctx context.Context, nonceEndpoint string) (string } func (c *client) RequestCredential(ctx context.Context, opts RequestCredentialOpts) (*CredentialResponse, error) { - if err := c.validateURL("credential endpoint", opts.CredentialEndpoint); err != nil { - return nil, err - } body := CredentialRequest{ Proofs: &CredentialRequestProofs{ JWT: []string{opts.ProofJWT}, diff --git a/auth/openid4vci/client_test.go b/auth/openid4vci/client_test.go index c88034d64e..a094eb0867 100644 --- a/auth/openid4vci/client_test.go +++ b/auth/openid4vci/client_test.go @@ -42,7 +42,7 @@ func TestClient_RequestNonce(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) nonce, err := client.RequestNonce(context.Background(), srv.URL) require.NoError(t, err) assert.Equal(t, "test-nonce-123", nonce) @@ -54,7 +54,7 @@ func TestClient_RequestNonce(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.RequestNonce(context.Background(), srv.URL) require.Error(t, err) assert.Contains(t, err.Error(), "500") @@ -67,7 +67,7 @@ func TestClient_RequestNonce(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.RequestNonce(context.Background(), srv.URL) require.Error(t, err) assert.Contains(t, err.Error(), "empty c_nonce") @@ -91,7 +91,7 @@ func TestClient_OpenIDCredentialIssuerMetadata(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) metadata, err := client.OpenIDCredentialIssuerMetadata(context.Background(), srv.URL) require.NoError(t, err) require.NotNil(t, metadata) @@ -110,7 +110,7 @@ func TestClient_OpenIDCredentialIssuerMetadata(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), srv.URL+"/oauth2/alice") require.NoError(t, err) assert.Equal(t, "/.well-known/openid-credential-issuer/oauth2/alice", capturedPath) @@ -126,7 +126,7 @@ func TestClient_OpenIDCredentialIssuerMetadata(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), srv.URL+"/foo%2Fbar") require.NoError(t, err) assert.Equal(t, "/.well-known/openid-credential-issuer/foo%2Fbar", capturedRawPath) @@ -141,7 +141,7 @@ func TestClient_OpenIDCredentialIssuerMetadata(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), srv.URL) require.Error(t, err) assert.Contains(t, err.Error(), "credential_issuer") @@ -154,21 +154,14 @@ func TestClient_OpenIDCredentialIssuerMetadata(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), srv.URL) require.Error(t, err) assert.Contains(t, err.Error(), "404") }) - t.Run("rejects non-https issuer URL in strict mode", func(t *testing.T) { - client := NewClient(http.DefaultClient, true) - _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), "http://issuer.example/") - require.Error(t, err) - assert.Contains(t, err.Error(), "invalid issuer URL") - }) - t.Run("rejects issuer URL with query or fragment per §12.2.1", func(t *testing.T) { - client := NewClient(http.DefaultClient, false) + client := NewClient(http.DefaultClient) _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), "https://issuer.example/?foo=bar") require.Error(t, err) assert.Contains(t, err.Error(), "query and fragment") @@ -185,7 +178,7 @@ func TestClient_OpenIDCredentialIssuerMetadata(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), srv.URL) require.Error(t, err) assert.Contains(t, err.Error(), "decoding issuer metadata") @@ -216,7 +209,7 @@ func TestClient_RequestCredential(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) resp, err := client.RequestCredential(context.Background(), RequestCredentialOpts{ CredentialEndpoint: srv.URL, AccessToken: "test-token", @@ -240,7 +233,7 @@ func TestClient_RequestCredential(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.RequestCredential(context.Background(), RequestCredentialOpts{ CredentialEndpoint: srv.URL, AccessToken: "t", @@ -261,7 +254,7 @@ func TestClient_RequestCredential(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.RequestCredential(context.Background(), RequestCredentialOpts{ CredentialEndpoint: srv.URL, AccessToken: "test-token", @@ -279,7 +272,7 @@ func TestClient_RequestCredential(t *testing.T) { })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.RequestCredential(context.Background(), RequestCredentialOpts{ CredentialEndpoint: srv.URL, AccessToken: "test-token", diff --git a/auth/services/oauth/relying_party.go b/auth/services/oauth/relying_party.go index e78160a86c..48d967e49d 100644 --- a/auth/services/oauth/relying_party.go +++ b/auth/services/oauth/relying_party.go @@ -24,7 +24,6 @@ import ( "fmt" "github.com/nuts-foundation/nuts-node/pki" "net/url" - "strings" "time" "github.com/lestrrat-go/jwx/v2/jwt" @@ -47,24 +46,23 @@ type relyingParty struct { keyResolver resolver.KeyResolver privateKeyStore nutsCrypto.KeyStore serviceResolver didman.CompoundServiceResolver - strictMode bool httpClientTimeout time.Duration httpClientTLS *tls.Config wallet holder.Wallet pkiValidator pki.Validator } -// NewRelyingParty returns an implementation of RelyingParty +// NewRelyingParty returns an implementation of RelyingParty. +// Strict-mode URL validation is centralised in http/client.StrictHTTPClient; this constructor no longer takes a strictMode flag. func NewRelyingParty( didResolver resolver.DIDResolver, serviceResolver didman.CompoundServiceResolver, privateKeyStore nutsCrypto.KeyStore, - wallet holder.Wallet, httpClientTimeout time.Duration, httpClientTLS *tls.Config, strictMode bool, pkiValidator pki.Validator) RelyingParty { + wallet holder.Wallet, httpClientTimeout time.Duration, httpClientTLS *tls.Config, pkiValidator pki.Validator) RelyingParty { return &relyingParty{ keyResolver: resolver.DIDKeyResolver{Resolver: didResolver}, serviceResolver: serviceResolver, privateKeyStore: privateKeyStore, httpClientTimeout: httpClientTimeout, httpClientTLS: httpClientTLS, - strictMode: strictMode, wallet: wallet, pkiValidator: pkiValidator, } @@ -110,9 +108,6 @@ func (s *relyingParty) CreateJwtGrant(ctx context.Context, request services.Crea } func (s *relyingParty) RequestRFC003AccessToken(ctx context.Context, jwtGrantToken string, authorizationServerEndpoint url.URL) (*oauth.TokenResponse, error) { - if s.strictMode && strings.ToLower(authorizationServerEndpoint.Scheme) != "https" { - return nil, fmt.Errorf("authorization server endpoint must be HTTPS when in strict mode: %s", authorizationServerEndpoint.String()) - } httpClient := strictHttp.NewWithTLSConfig(s.httpClientTimeout, s.httpClientTLS) authClient, err := client.NewHTTPClient("", s.httpClientTimeout, client.WithHTTPClient(httpClient), client.WithRequestEditorFn(core.UserAgentRequestEditor)) if err != nil { diff --git a/auth/services/oauth/relying_party_test.go b/auth/services/oauth/relying_party_test.go index 9a3d647ca7..057efbf0c2 100644 --- a/auth/services/oauth/relying_party_test.go +++ b/auth/services/oauth/relying_party_test.go @@ -22,7 +22,6 @@ import ( "context" "crypto/tls" "errors" - "fmt" "net/http" "net/http/httptest" "testing" @@ -76,42 +75,6 @@ func TestRelyingParty_RequestRFC003AccessToken(t *testing.T) { assert.EqualError(t, err, "remote server/nuts node returned error creating access token: server returned HTTP 502 (expected: 200)") }) - t.Run("endpoint security validation (only HTTPS in strict mode)", func(t *testing.T) { - ctx := createRPContext(t, nil) - httpServer := httptest.NewServer(&http2.Handler{ - StatusCode: http.StatusOK, - }) - httpsServer := httptest.NewTLSServer(&http2.Handler{ - StatusCode: http.StatusOK, - }) - t.Cleanup(httpServer.Close) - t.Cleanup(httpsServer.Close) - - t.Run("HTTPS in strict mode", func(t *testing.T) { - ctx.relyingParty.strictMode = true - - response, err := ctx.relyingParty.RequestRFC003AccessToken(context.Background(), bearerToken, *test.MustParseURL(httpsServer.URL)) - - assert.NoError(t, err) - assert.NotNil(t, response) - }) - t.Run("HTTP allowed in non-strict mode", func(t *testing.T) { - ctx.relyingParty.strictMode = false - - response, err := ctx.relyingParty.RequestRFC003AccessToken(context.Background(), bearerToken, *test.MustParseURL(httpServer.URL)) - - assert.NoError(t, err) - assert.NotNil(t, response) - }) - t.Run("HTTP not allowed in strict mode", func(t *testing.T) { - ctx.relyingParty.strictMode = true - - response, err := ctx.relyingParty.RequestRFC003AccessToken(context.Background(), bearerToken, *test.MustParseURL(httpServer.URL)) - - assert.EqualError(t, err, fmt.Sprintf("authorization server endpoint must be HTTPS when in strict mode: %s", httpServer.URL)) - assert.Nil(t, response) - }) - }) } func TestService_CreateJwtBearerToken(t *testing.T) { diff --git a/docs/pages/deployment/configuration.rst b/docs/pages/deployment/configuration.rst index 4385fcb20a..baa604fa4d 100644 --- a/docs/pages/deployment/configuration.rst +++ b/docs/pages/deployment/configuration.rst @@ -95,4 +95,6 @@ requesting an access token from another node on ``/n2n/auth/v1/accesstoken`` doe json-ld context can only be downloaded from trusted domains configured in ``jsonld.contexts.remoteallowlist``, and the ``internalratelimiter`` is always on. -Interacting with remote Nuts nodes requires HTTPS: it will refuse to connect to plain HTTP endpoints when in strict mode. \ No newline at end of file +Interacting with remote Nuts nodes requires HTTPS: it will refuse to connect to plain HTTP endpoints when in strict mode. +Strict mode additionally rejects outbound URLs whose host is an IP literal or an RFC 2606 reserved hostname/TLD (e.g. ``*.localhost``, ``*.test``, ``example.com/net/org``). +This applies to every outbound HTTP call made via the shared HTTP client (OpenID4VCI, OAuth relying-party, IAM, Discovery, did:web resolution, etc.) and to every redirect target along the way. \ No newline at end of file diff --git a/http/client/client.go b/http/client/client.go index 2c9e4308df..2be7a2b25f 100644 --- a/http/client/client.go +++ b/http/client/client.go @@ -21,7 +21,6 @@ package client import ( "bytes" "crypto/tls" - "errors" "fmt" "io" "net/http" @@ -32,6 +31,21 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) +// maxRedirects matches net/http's default redirect cap. +const maxRedirects = 10 + +// checkRedirect re-runs core.ParsePublicURL on every redirect target so a +// validated initial URL cannot be turned into an SSRF via a 3xx response. +func checkRedirect(req *http.Request, via []*http.Request) error { + if len(via) >= maxRedirects { + return fmt.Errorf("stopped after %d redirects", maxRedirects) + } + if _, err := core.ParsePublicURL(req.URL.String(), StrictMode); err != nil { + return fmt.Errorf("httpclient: invalid redirect target: %w", err) + } + return nil +} + // SafeHttpTransport is a http.Transport that can be used as a default transport for HTTP clients. var SafeHttpTransport *http.Transport @@ -75,8 +89,9 @@ func New(timeout time.Duration) *StrictHTTPClient { transport := getTransport(SafeHttpTransport) return &StrictHTTPClient{ client: &http.Client{ - Transport: transport, - Timeout: timeout, + Transport: transport, + Timeout: timeout, + CheckRedirect: checkRedirect, }, } } @@ -98,8 +113,9 @@ func NewWithCache(timeout time.Duration) *StrictHTTPClient { transport := getTransport(DefaultCachingTransport) return &StrictHTTPClient{ client: &http.Client{ - Transport: transport, - Timeout: timeout, + Transport: transport, + Timeout: timeout, + CheckRedirect: checkRedirect, }, } } @@ -112,8 +128,9 @@ func NewWithTLSConfig(timeout time.Duration, tlsConfig *tls.Config) *StrictHTTPC transport.TLSClientConfig = tlsConfig return &StrictHTTPClient{ client: &http.Client{ - Transport: getTransport(transport), - Timeout: timeout, + Transport: getTransport(transport), + Timeout: timeout, + CheckRedirect: checkRedirect, }, } } @@ -123,8 +140,8 @@ type StrictHTTPClient struct { } func (s *StrictHTTPClient) Do(req *http.Request) (*http.Response, error) { - if StrictMode && req.URL.Scheme != "https" { - return nil, errors.New("strictmode is enabled, but request is not over HTTPS") + if _, err := core.ParsePublicURL(req.URL.String(), StrictMode); err != nil { + return nil, fmt.Errorf("httpclient: invalid target URL: %w", err) } req.Header.Set("User-Agent", core.UserAgent()) result, err := s.client.Do(req) diff --git a/http/client/client_test.go b/http/client/client_test.go index 1a1b01366c..5eb261255a 100644 --- a/http/client/client_test.go +++ b/http/client/client_test.go @@ -21,6 +21,7 @@ package client import ( "crypto/tls" "fmt" + "io" "net/http" "net/http/httptest" "strings" @@ -40,12 +41,13 @@ func TestStrictHTTPClient(t *testing.T) { rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true + t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) _, err := client.Do(httpRequest) - assert.EqualError(t, err, "strictmode is enabled, but request is not over HTTPS") + assert.ErrorContains(t, err, "httpclient: invalid target URL") assert.Equal(t, 0, rt.invocations) }) t.Run("strict mode disabled", func(t *testing.T) { @@ -66,12 +68,13 @@ func TestStrictHTTPClient(t *testing.T) { rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true + t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) _, err := client.Do(httpRequest) - assert.EqualError(t, err, "strictmode is enabled, but request is not over HTTPS") + assert.ErrorContains(t, err, "httpclient: invalid target URL") assert.Equal(t, 0, rt.invocations) }) t.Run("sets TLS config", func(t *testing.T) { @@ -89,14 +92,177 @@ func TestStrictHTTPClient(t *testing.T) { rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true + t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) _, err := client.Do(httpRequest) - assert.EqualError(t, err, "strictmode is enabled, but request is not over HTTPS") + assert.ErrorContains(t, err, "httpclient: invalid target URL") assert.Equal(t, 0, rt.invocations) }) + t.Run("strict mode rejects IP host", func(t *testing.T) { + rt := &stubRoundTripper{} + DefaultCachingTransport = rt + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + + client := NewWithCache(time.Second) + httpRequest, _ := http.NewRequest("GET", "https://127.0.0.1/foo", nil) + _, err := client.Do(httpRequest) + + assert.ErrorContains(t, err, "httpclient: invalid target URL") + assert.ErrorContains(t, err, "hostname is IP") + assert.Equal(t, 0, rt.invocations) + }) + t.Run("strict mode rejects RFC2606 reserved host", func(t *testing.T) { + rt := &stubRoundTripper{} + DefaultCachingTransport = rt + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + + client := NewWithCache(time.Second) + httpRequest, _ := http.NewRequest("GET", "https://service.localhost/foo", nil) + _, err := client.Do(httpRequest) + + assert.ErrorContains(t, err, "httpclient: invalid target URL") + assert.ErrorContains(t, err, "hostname is RFC2606 reserved") + assert.Equal(t, 0, rt.invocations) + }) +} + +func TestCheckRedirect(t *testing.T) { + makeReq := func(target string) *http.Request { + req, _ := http.NewRequest("GET", target, nil) + return req + } + t.Run("strict mode rejects http redirect", func(t *testing.T) { + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + err := checkRedirect(makeReq("http://example.org"), nil) + assert.ErrorContains(t, err, "invalid redirect target") + assert.ErrorContains(t, err, "scheme must be https") + }) + t.Run("strict mode rejects redirect to IP host", func(t *testing.T) { + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + err := checkRedirect(makeReq("https://127.0.0.1/x"), nil) + assert.ErrorContains(t, err, "invalid redirect target") + assert.ErrorContains(t, err, "hostname is IP") + }) + t.Run("strict mode rejects redirect to reserved host", func(t *testing.T) { + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + err := checkRedirect(makeReq("https://internal.localhost/x"), nil) + assert.ErrorContains(t, err, "invalid redirect target") + assert.ErrorContains(t, err, "hostname is RFC2606 reserved") + }) + t.Run("non-strict mode allows http redirect", func(t *testing.T) { + StrictMode = false + err := checkRedirect(makeReq("http://example.org"), nil) + assert.NoError(t, err) + }) + t.Run("non-strict mode allows redirect to IP host", func(t *testing.T) { + StrictMode = false + err := checkRedirect(makeReq("http://127.0.0.1/x"), nil) + assert.NoError(t, err) + }) + t.Run("redirect cap stops after 10 hops", func(t *testing.T) { + StrictMode = false + via := make([]*http.Request, maxRedirects) + err := checkRedirect(makeReq("http://example.org"), via) + assert.ErrorContains(t, err, "stopped after 10 redirects") + }) + t.Run("cap checked before URL validation", func(t *testing.T) { + // Even an invalid target should produce the cap error first when via is at the limit. + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + via := make([]*http.Request, maxRedirects) + err := checkRedirect(makeReq("http://example.org"), via) + assert.ErrorContains(t, err, "stopped after 10 redirects") + }) +} + +// redirectOnceTransport is a stub RoundTripper that responds with a 302 to redirectTo +// on the first request, then a 200 OK on subsequent requests. It also counts hops so +// tests can verify the redirected request was (or was not) sent. +type redirectOnceTransport struct { + redirectTo string + requests []*http.Request +} + +func (t *redirectOnceTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.requests = append(t.requests, req) + if len(t.requests) == 1 { + return &http.Response{ + StatusCode: http.StatusFound, + Header: http.Header{"Location": []string{t.redirectTo}}, + Body: io.NopCloser(strings.NewReader("")), + Request: req, + }, nil + } + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader("ok")), + Request: req, + }, nil +} + +// TestStrictHTTPClient_RedirectEndToEnd drives the full net/http redirect path +// through a stub transport so we prove the configured CheckRedirect actually +// fires and blocks the second HTTP request from being issued. +// +// Initial URLs use "nuts.nl" because strict mode rejects RFC 2606 reserved 2LDs +// (example.com/net/org), and the test needs the initial Do() to pass so the +// redirect can be exercised. The hostname is never resolved — the stub +// transport intercepts all requests. +func TestStrictHTTPClient_RedirectEndToEnd(t *testing.T) { + const initialURL = "https://nuts.nl/" + t.Run("strict mode blocks redirect to non-https target", func(t *testing.T) { + rt := &redirectOnceTransport{redirectTo: "http://example.com/x"} + DefaultCachingTransport = rt + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + + c := NewWithCache(time.Second) + req, _ := http.NewRequest("GET", initialURL, nil) + _, err := c.Do(req) + + require.Error(t, err) + assert.ErrorContains(t, err, "invalid redirect target") + assert.ErrorContains(t, err, "scheme must be https") + // only the initial request reached the transport; the redirect was blocked + assert.Len(t, rt.requests, 1, "second request must not be issued") + }) + t.Run("strict mode blocks redirect to IP host", func(t *testing.T) { + rt := &redirectOnceTransport{redirectTo: "https://10.0.0.1/x"} + DefaultCachingTransport = rt + StrictMode = true + t.Cleanup(func() { StrictMode = false }) + + c := NewWithCache(time.Second) + req, _ := http.NewRequest("GET", initialURL, nil) + _, err := c.Do(req) + + require.Error(t, err) + assert.ErrorContains(t, err, "invalid redirect target") + assert.ErrorContains(t, err, "hostname is IP") + assert.Len(t, rt.requests, 1) + }) + t.Run("non-strict mode follows http redirect", func(t *testing.T) { + rt := &redirectOnceTransport{redirectTo: "http://example.com/x"} + DefaultCachingTransport = rt + StrictMode = false + + c := NewWithCache(time.Second) + req, _ := http.NewRequest("GET", "http://example.com/", nil) + resp, err := c.Do(req) + + require.NoError(t, err) + assert.Equal(t, http.StatusOK, resp.StatusCode) + assert.Len(t, rt.requests, 2, "both initial and redirected request should be issued") + }) } func TestLimitedReadAll(t *testing.T) { diff --git a/vcr/vcr_test.go b/vcr/vcr_test.go index d4833943a9..231bc5da7d 100644 --- a/vcr/vcr_test.go +++ b/vcr/vcr_test.go @@ -87,6 +87,7 @@ func TestVCR_Configure(t *testing.T) { t.Run("strictmode passed to client APIs", func(t *testing.T) { ctx := newMockContext(t) client.StrictMode = true + t.Cleanup(func() { client.StrictMode = false }) testVC := test.ValidNutsOrganizationCredential(t) issuerDID := did.MustParseDID(testVC.Issuer.String()) testDirectory := io.TestDirectory(t) @@ -106,7 +107,8 @@ func TestVCR_Configure(t *testing.T) { require.NoError(t, err) err = issuer.OfferCredential(context.Background(), testVC, "http://example.com") - assert.ErrorContains(t, err, "http request error: strictmode is enabled, but request is not over HTTPS") + assert.ErrorContains(t, err, "httpclient: invalid target URL") + assert.ErrorContains(t, err, "scheme must be https") }) } From 3ae15497618a0aab5afb4d68790b67a5a7726f0c Mon Sep 17 00:00:00 2001 From: Joris Scharp Date: Mon, 11 May 2026 19:47:38 +0200 Subject: [PATCH 2/3] test: capture and restore package globals in client tests Apply the capture-and-restore t.Cleanup pattern to tests that mutate the package-level StrictMode and DefaultCachingTransport globals so they no longer leak state into subsequent tests. Adds a withClientGlobals helper in http/client/client_test.go to avoid repeating the boilerplate. Also fixes a pre-existing DefaultCachingTransport leak in TestCaching. --- http/client/client_test.go | 46 +++++++++++++++++++++++++------------- vcr/vcr_test.go | 3 ++- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/http/client/client_test.go b/http/client/client_test.go index 5eb261255a..de11e0075e 100644 --- a/http/client/client_test.go +++ b/http/client/client_test.go @@ -35,13 +35,26 @@ import ( "github.com/stretchr/testify/require" ) +// withClientGlobals captures the package-level globals StrictMode and +// DefaultCachingTransport at call time and restores them via t.Cleanup, +// so subtests can mutate them without leaking state into other tests. +func withClientGlobals(t *testing.T) { + t.Helper() + oldStrict := StrictMode + oldTransport := DefaultCachingTransport + t.Cleanup(func() { + StrictMode = oldStrict + DefaultCachingTransport = oldTransport + }) +} + func TestStrictHTTPClient(t *testing.T) { t.Run("caching transport", func(t *testing.T) { t.Run("strict mode enabled", func(t *testing.T) { + withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true - t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) @@ -51,6 +64,7 @@ func TestStrictHTTPClient(t *testing.T) { assert.Equal(t, 0, rt.invocations) }) t.Run("strict mode disabled", func(t *testing.T) { + withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = false @@ -65,10 +79,10 @@ func TestStrictHTTPClient(t *testing.T) { }) t.Run("TLS transport", func(t *testing.T) { t.Run("strict mode enabled", func(t *testing.T) { + withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true - t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) @@ -89,10 +103,10 @@ func TestStrictHTTPClient(t *testing.T) { }) }) t.Run("error on HTTP call when strictmode is enabled", func(t *testing.T) { + withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true - t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) @@ -102,10 +116,10 @@ func TestStrictHTTPClient(t *testing.T) { assert.Equal(t, 0, rt.invocations) }) t.Run("strict mode rejects IP host", func(t *testing.T) { + withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true - t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "https://127.0.0.1/foo", nil) @@ -116,10 +130,10 @@ func TestStrictHTTPClient(t *testing.T) { assert.Equal(t, 0, rt.invocations) }) t.Run("strict mode rejects RFC2606 reserved host", func(t *testing.T) { + withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true - t.Cleanup(func() { StrictMode = false }) client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "https://service.localhost/foo", nil) @@ -137,37 +151,40 @@ func TestCheckRedirect(t *testing.T) { return req } t.Run("strict mode rejects http redirect", func(t *testing.T) { + withClientGlobals(t) StrictMode = true - t.Cleanup(func() { StrictMode = false }) err := checkRedirect(makeReq("http://example.org"), nil) assert.ErrorContains(t, err, "invalid redirect target") assert.ErrorContains(t, err, "scheme must be https") }) t.Run("strict mode rejects redirect to IP host", func(t *testing.T) { + withClientGlobals(t) StrictMode = true - t.Cleanup(func() { StrictMode = false }) err := checkRedirect(makeReq("https://127.0.0.1/x"), nil) assert.ErrorContains(t, err, "invalid redirect target") assert.ErrorContains(t, err, "hostname is IP") }) t.Run("strict mode rejects redirect to reserved host", func(t *testing.T) { + withClientGlobals(t) StrictMode = true - t.Cleanup(func() { StrictMode = false }) err := checkRedirect(makeReq("https://internal.localhost/x"), nil) assert.ErrorContains(t, err, "invalid redirect target") assert.ErrorContains(t, err, "hostname is RFC2606 reserved") }) t.Run("non-strict mode allows http redirect", func(t *testing.T) { + withClientGlobals(t) StrictMode = false err := checkRedirect(makeReq("http://example.org"), nil) assert.NoError(t, err) }) t.Run("non-strict mode allows redirect to IP host", func(t *testing.T) { + withClientGlobals(t) StrictMode = false err := checkRedirect(makeReq("http://127.0.0.1/x"), nil) assert.NoError(t, err) }) t.Run("redirect cap stops after 10 hops", func(t *testing.T) { + withClientGlobals(t) StrictMode = false via := make([]*http.Request, maxRedirects) err := checkRedirect(makeReq("http://example.org"), via) @@ -175,8 +192,8 @@ func TestCheckRedirect(t *testing.T) { }) t.Run("cap checked before URL validation", func(t *testing.T) { // Even an invalid target should produce the cap error first when via is at the limit. + withClientGlobals(t) StrictMode = true - t.Cleanup(func() { StrictMode = false }) via := make([]*http.Request, maxRedirects) err := checkRedirect(makeReq("http://example.org"), via) assert.ErrorContains(t, err, "stopped after 10 redirects") @@ -220,10 +237,10 @@ func (t *redirectOnceTransport) RoundTrip(req *http.Request) (*http.Response, er func TestStrictHTTPClient_RedirectEndToEnd(t *testing.T) { const initialURL = "https://nuts.nl/" t.Run("strict mode blocks redirect to non-https target", func(t *testing.T) { + withClientGlobals(t) rt := &redirectOnceTransport{redirectTo: "http://example.com/x"} DefaultCachingTransport = rt StrictMode = true - t.Cleanup(func() { StrictMode = false }) c := NewWithCache(time.Second) req, _ := http.NewRequest("GET", initialURL, nil) @@ -236,10 +253,10 @@ func TestStrictHTTPClient_RedirectEndToEnd(t *testing.T) { assert.Len(t, rt.requests, 1, "second request must not be issued") }) t.Run("strict mode blocks redirect to IP host", func(t *testing.T) { + withClientGlobals(t) rt := &redirectOnceTransport{redirectTo: "https://10.0.0.1/x"} DefaultCachingTransport = rt StrictMode = true - t.Cleanup(func() { StrictMode = false }) c := NewWithCache(time.Second) req, _ := http.NewRequest("GET", initialURL, nil) @@ -251,6 +268,7 @@ func TestStrictHTTPClient_RedirectEndToEnd(t *testing.T) { assert.Len(t, rt.requests, 1) }) t.Run("non-strict mode follows http redirect", func(t *testing.T) { + withClientGlobals(t) rt := &redirectOnceTransport{redirectTo: "http://example.com/x"} DefaultCachingTransport = rt StrictMode = false @@ -283,9 +301,8 @@ func TestLimitedReadAll(t *testing.T) { } func TestMaxConns(t *testing.T) { - oldStrictMode := StrictMode + withClientGlobals(t) StrictMode = false - t.Cleanup(func() { StrictMode = oldStrictMode }) // Our safe http Transport has MaxConnsPerHost = 5 // if we request 6 resources multiple times, we expect a max connection usage of 5 @@ -328,9 +345,8 @@ func TestMaxConns(t *testing.T) { } func TestCaching(t *testing.T) { - oldStrictMode := StrictMode + withClientGlobals(t) StrictMode = false - t.Cleanup(func() { StrictMode = oldStrictMode }) // counter for the number of concurrent requests var total atomic.Int32 diff --git a/vcr/vcr_test.go b/vcr/vcr_test.go index 231bc5da7d..29700f3be2 100644 --- a/vcr/vcr_test.go +++ b/vcr/vcr_test.go @@ -86,8 +86,9 @@ func TestVCR_Configure(t *testing.T) { }) t.Run("strictmode passed to client APIs", func(t *testing.T) { ctx := newMockContext(t) + oldStrict := client.StrictMode client.StrictMode = true - t.Cleanup(func() { client.StrictMode = false }) + t.Cleanup(func() { client.StrictMode = oldStrict }) testVC := test.ValidNutsOrganizationCredential(t) issuerDID := did.MustParseDID(testVC.Issuer.String()) testDirectory := io.TestDirectory(t) From fb8ca6515a6c688d253192539545a640a0139ea5 Mon Sep 17 00:00:00 2001 From: Rein Krul Date: Mon, 17 Aug 2026 15:00:07 +0200 Subject: [PATCH 3/3] refactor: centralise outbound URL validation in StrictHTTPClient StrictHTTPClient.Do and its CheckRedirect now run core.ParsePublicURLAllowIP (HTTPS-only + RFC 2606 reserved-hostname rejection in strict mode) on every outbound request and every redirect hop, instead of each caller duplicating that check. Removes the duplicated validation from auth/openid4vci.Client, auth/client/iam.HTTPClient.ClientMetadata, auth/client/iam.OpenID4VPClient, auth/oauth.FetchMetadata/IssuerIdToWellKnown, and auth/services/oauth.relyingParty, dropping the strictMode parameter each of them threaded through solely for that purpose. IP-literal hosts are deliberately left unchecked by this centralised validation: http/client's dial-time SSRF guard (added in #4420) already polices connecting addresses and honors http.client.allowedinternalcidrs/ deniedcidrs, which a blanket string-level IP check has no visibility into and would make unreachable for IP-literal targets. core.ParsePublicURLAllowIP is a new variant of ParsePublicURL that keeps the HTTPS/reserved-hostname checks but exempts IP literals for this reason. auth.Auth.strictMode was a dead field (never assigned, always false), so the openid4vci and IAM client-metadata paths that read it were effectively never strict-mode-validated at the URL layer regardless of the real strictmode setting. Removing it in favour of the correctly-wired http/client.StrictMode global fixes that as a side effect. Closes #4244. --- auth/auth.go | 2 - auth/client/iam/client.go | 6 +- auth/client/iam/client_test.go | 4 +- auth/client/iam/openid4vp.go | 31 +--- auth/client/iam/openid4vp_test.go | 5 +- auth/oauth/metadata.go | 16 +- auth/oauth/metadata_test.go | 39 ++--- auth/oauth/types.go | 8 +- auth/oauth/types_test.go | 25 +-- auth/services/oauth/relying_party.go | 4 +- auth/services/oauth/relying_party_test.go | 2 + core/url.go | 18 ++- core/url_test.go | 23 +++ docs/pages/deployment/configuration.rst | 2 +- docs/pages/release_notes.rst | 1 + http/client/client.go | 37 ++--- http/client/client_test.go | 179 +++++----------------- 17 files changed, 152 insertions(+), 250 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index 4acff0e079..742dbef9f9 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -67,7 +67,6 @@ type Auth struct { shutdownFunc func() vdrInstance vdr.VDR publicURL *url.URL - strictMode bool httpClientTimeout time.Duration tlsConfig *tls.Config subjectManager didsubject.Manager @@ -139,7 +138,6 @@ func (auth *Auth) IAMClient() iam.Client { JWTSigner: auth.keyStore, LDDocumentLoader: auth.jsonldManager.DocumentLoader(), PolicyBackend: auth.policyBackend, - StrictMode: auth.strictMode, HTTPClientTimeout: auth.httpClientTimeout, ExperimentalJwtBearerClient: auth.config.Experimental.JwtBearerClient, }) diff --git a/auth/client/iam/client.go b/auth/client/iam/client.go index a80bb2595b..a510b37ae6 100644 --- a/auth/client/iam/client.go +++ b/auth/client/iam/client.go @@ -50,7 +50,6 @@ var ErrBadGateway = errors.New("upstream returned unexpected result") // HTTPClient holds the server address and other basic settings for the http client type HTTPClient struct { - strictMode bool keyResolver resolver.KeyResolver httpClient core.HTTPRequestDoer } @@ -70,7 +69,7 @@ func (hb HTTPClient) OAuthAuthorizationServerMetadata(ctx context.Context, oauth // 1. insert (RFC 8414): https://host/.well-known/oauth-authorization-server/ // 2. append (OIDC Disc): https://host//.well-known/oauth-authorization-server // Many authorization servers publish metadata only under the append convention. - metadata, err := oauth.FetchMetadata[oauth.AuthorizationServerMetadata](ctx, hb.httpClient, oauthIssuer, hb.strictMode) + metadata, err := oauth.FetchMetadata[oauth.AuthorizationServerMetadata](ctx, hb.httpClient, oauthIssuer) if err != nil && errors.Is(err, oauth.ErrAllCandidates4xx) { // Every candidate rejected the request outright (no 5xx, no network/decode failure, no // identifier mismatch): the identifier itself is most likely wrong. @@ -82,6 +81,7 @@ func (hb HTTPClient) OAuthAuthorizationServerMetadata(ctx context.Context, oauth // ClientMetadata retrieves the client metadata from the client metadata endpoint given in the authorization request. // We use the AuthorizationServerMetadata struct since it overlaps greatly with the client metadata. func (hb HTTPClient) ClientMetadata(ctx context.Context, endpoint string) (*oauth.OAuthClientMetadata, error) { + // create a GET request request, err := http.NewRequestWithContext(ctx, http.MethodGet, endpoint, nil) if err != nil { return nil, err @@ -231,7 +231,7 @@ func (hb HTTPClient) PostAuthorizationResponse(ctx context.Context, vp vc.Verifi } func (hb HTTPClient) OpenIDConfiguration(ctx context.Context, issuerURL string) (*oauth.OpenIDConfiguration, error) { - metadataURL, err := oauth.IssuerIdToWellKnown(issuerURL, oauth.OpenIdConfigurationWellKnown, hb.strictMode) + metadataURL, err := oauth.IssuerIdToWellKnown(issuerURL, oauth.OpenIdConfigurationWellKnown) if err != nil { return nil, err } diff --git a/auth/client/iam/client_test.go b/auth/client/iam/client_test.go index 3e09f2727d..2b4b758a47 100644 --- a/auth/client/iam/client_test.go +++ b/auth/client/iam/client_test.go @@ -80,8 +80,8 @@ func TestHTTPClient_OAuthAuthorizationServerMetadata(t *testing.T) { // The insert/append fallback, identifier-match, and error-joining behavior is exhaustively // covered by oauth.FetchMetadata's own tests; this wraps it with no extra logic, so these - // tests only need to confirm the wiring (well-known constant, httpClient, strictMode) and - // that this specific bug (rewriting an upstream status code) doesn't reappear. + // tests only need to confirm the wiring (well-known constant, httpClient) and that this + // specific bug (rewriting an upstream status code) doesn't reappear. t.Run("ok - append form when insert 404s", func(t *testing.T) { tlsServer, client, requested := metadataServer(t, http.StatusNotFound, "/iam/123", "/iam/123/.well-known/oauth-authorization-server") diff --git a/auth/client/iam/openid4vp.go b/auth/client/iam/openid4vp.go index 34522f3191..55ae7625d0 100644 --- a/auth/client/iam/openid4vp.go +++ b/auth/client/iam/openid4vp.go @@ -40,7 +40,6 @@ import ( "github.com/nuts-foundation/go-did/vc" "github.com/nuts-foundation/nuts-node/auth/log" "github.com/nuts-foundation/nuts-node/auth/oauth" - "github.com/nuts-foundation/nuts-node/core" nutsCrypto "github.com/nuts-foundation/nuts-node/crypto" "github.com/nuts-foundation/nuts-node/crypto/dpop" nutsHttp "github.com/nuts-foundation/nuts-node/http" @@ -62,7 +61,6 @@ type OpenID4VPClient struct { httpClient HTTPClient jwtSigner nutsCrypto.JWTSigner keyResolver resolver.KeyResolver - strictMode bool wallet holder.Wallet ldDocumentLoader ld.DocumentLoader subjectManager didsubject.Manager @@ -74,8 +72,8 @@ type OpenID4VPClient struct { // ClientConfig groups the dependencies and toggles needed to construct an OpenID4VPClient. // The interface fields (Wallet, KeyResolver, SubjectManager, JWTSigner, LDDocumentLoader, // PolicyBackend) are required; NewClient does not validate them and missing fields will surface as -// nil-pointer panics on first use. Scalar fields default to their zero value (StrictMode=false, -// HTTPClientTimeout=0, ExperimentalJwtBearerClient=false) and the zero values are valid. +// nil-pointer panics on first use. Scalar fields default to their zero value (HTTPClientTimeout=0, +// ExperimentalJwtBearerClient=false) and the zero values are valid. type ClientConfig struct { Wallet holder.Wallet KeyResolver resolver.KeyResolver @@ -83,7 +81,6 @@ type ClientConfig struct { JWTSigner nutsCrypto.JWTSigner LDDocumentLoader ld.DocumentLoader PolicyBackend policy.PDPBackend - StrictMode bool HTTPClientTimeout time.Duration // ExperimentalJwtBearerClient gates the RFC 7523 jwt-bearer two-VP token request flow. // Tracked for replacement by a list-style grant-types config under issue #4231. @@ -93,7 +90,6 @@ type ClientConfig struct { // NewClient returns an OpenID4VPClient configured with the given dependencies. func NewClient(cfg ClientConfig) *OpenID4VPClient { httpClient := HTTPClient{ - strictMode: cfg.StrictMode, httpClient: client.NewWithCache(cfg.HTTPClientTimeout), keyResolver: cfg.KeyResolver, } @@ -103,7 +99,6 @@ func NewClient(cfg ClientConfig) *OpenID4VPClient { jwtSigner: cfg.JWTSigner, ldDocumentLoader: cfg.LDDocumentLoader, subjectManager: cfg.SubjectManager, - strictMode: cfg.StrictMode, wallet: cfg.Wallet, policyBackend: cfg.PolicyBackend, experimentalJwtBearerClient: cfg.ExperimentalJwtBearerClient, @@ -128,7 +123,7 @@ func (c *OpenID4VPClient) ClientMetadata(ctx context.Context, endpoint string) ( func (c *OpenID4VPClient) PostError(ctx context.Context, auth2Error oauth.OAuth2Error, verifierResponseURI string, verifierClientState string) (string, error) { iamClient := c.httpClient - responseURL, err := core.ParsePublicURL(verifierResponseURI, c.strictMode) + responseURL, err := url.Parse(verifierResponseURI) if err != nil { return "", fmt.Errorf("failed to post error to verifier: %w", err) } @@ -149,7 +144,7 @@ func (c *OpenID4VPClient) PostError(ctx context.Context, auth2Error oauth.OAuth2 func (c *OpenID4VPClient) PostAuthorizationResponse(ctx context.Context, vp vc.VerifiablePresentation, presentationSubmission pe.PresentationSubmission, verifierResponseURI string, state string) (string, error) { iamClient := c.httpClient - responseURL, err := core.ParsePublicURL(verifierResponseURI, c.strictMode) + responseURL, err := url.Parse(verifierResponseURI) if err != nil { return "", fmt.Errorf("failed to post error to verifier: %w", err) } @@ -163,7 +158,7 @@ func (c *OpenID4VPClient) PostAuthorizationResponse(ctx context.Context, vp vc.V func (c *OpenID4VPClient) PresentationDefinition(ctx context.Context, endpoint string) (*pe.PresentationDefinition, error) { iamClient := c.httpClient - parsedURL, err := core.ParsePublicURL(endpoint, c.strictMode) + parsedURL, err := url.Parse(endpoint) if err != nil { return nil, fmt.Errorf("failed to retrieve presentation definition: %w", err) } @@ -196,12 +191,7 @@ func (c *OpenID4VPClient) OpenIDConfiguration(ctx context.Context, issuer string func (c *OpenID4VPClient) RequestObjectByGet(ctx context.Context, requestURI string) (string, error) { iamClient := c.httpClient - parsedURL, err := core.ParsePublicURL(requestURI, c.strictMode) - if err != nil { - return "", fmt.Errorf("invalid request_uri: %w", err) - } - - requestObject, err := iamClient.RequestObjectByGet(ctx, parsedURL.String()) + requestObject, err := iamClient.RequestObjectByGet(ctx, requestURI) if err != nil { return "", fmt.Errorf("failed to retrieve JAR Request Object: %w", err) } @@ -209,15 +199,11 @@ func (c *OpenID4VPClient) RequestObjectByGet(ctx context.Context, requestURI str } func (c *OpenID4VPClient) RequestObjectByPost(ctx context.Context, requestURI string, walletMetadata oauth.AuthorizationServerMetadata) (string, error) { iamClient := c.httpClient - parsedURL, err := core.ParsePublicURL(requestURI, c.strictMode) - if err != nil { - return "", fmt.Errorf("invalid request_uri: %w", err) - } // TODO: consider adding a 'wallet_nonce' metadataBytes, _ := json.Marshal(walletMetadata) form := url.Values{oauth.WalletMetadataParam: {string(metadataBytes)}} - requestObject, err := iamClient.RequestObjectByPost(ctx, parsedURL.String(), form) + requestObject, err := iamClient.RequestObjectByPost(ctx, requestURI, form) if err != nil { return "", fmt.Errorf("failed to retrieve JAR Request Object: %w", err) } @@ -226,8 +212,7 @@ func (c *OpenID4VPClient) RequestObjectByPost(ctx context.Context, requestURI st func (c *OpenID4VPClient) AccessToken(ctx context.Context, code string, tokenEndpoint string, callbackURI string, subject string, clientID string, codeVerifier string, useDPoP bool) (*oauth.TokenResponse, error) { iamClient := c.httpClient - // validate tokenEndpoint - parsedURL, err := core.ParsePublicURL(tokenEndpoint, c.strictMode) + parsedURL, err := url.Parse(tokenEndpoint) if err != nil { return nil, err } diff --git a/auth/client/iam/openid4vp_test.go b/auth/client/iam/openid4vp_test.go index 4e53d033cc..52673e681e 100644 --- a/auth/client/iam/openid4vp_test.go +++ b/auth/client/iam/openid4vp_test.go @@ -822,7 +822,7 @@ func TestIAMClient_RequestObjectByGet(t *testing.T) { response, err := ctx.client.RequestObjectByGet(context.Background(), ":") - assert.EqualError(t, err, "invalid request_uri: parse \":\": missing protocol scheme") + assert.EqualError(t, err, "failed to retrieve JAR Request Object: parse \":\": missing protocol scheme") assert.Empty(t, response) }) t.Run("error - failed to get access token", func(t *testing.T) { @@ -853,7 +853,7 @@ func TestIAMClient_RequestObjectByPost(t *testing.T) { response, err := ctx.client.RequestObjectByPost(context.Background(), ":", metadata) - assert.EqualError(t, err, "invalid request_uri: parse \":\": missing protocol scheme") + assert.EqualError(t, err, "failed to retrieve JAR Request Object: parse \":\": missing protocol scheme") assert.Empty(t, response) }) t.Run("error - failed to get access token", func(t *testing.T) { @@ -884,7 +884,6 @@ func createClientTestContext(t *testing.T, tlsConfig *tls.Config) *clientTestCon wallet: wallet, subjectManager: subjectManager, httpClient: HTTPClient{ - strictMode: false, httpClient: client.NewWithTLSConfig(10*time.Second, tlsConfig), }, jwtSigner: jwtSigner, diff --git a/auth/oauth/metadata.go b/auth/oauth/metadata.go index c936ae4227..b6d17dd18c 100644 --- a/auth/oauth/metadata.go +++ b/auth/oauth/metadata.go @@ -24,6 +24,7 @@ import ( "errors" "fmt" "net/http" + "net/url" "strings" "github.com/nuts-foundation/nuts-node/core" @@ -44,8 +45,8 @@ var ErrAllCandidates4xx = errors.New("metadata not found: every candidate return // 2. append (OIDC Disc): https://host//.well-known/ // // When identifier has no path, insert and append collapse to a single URL. Every candidate -// shares identifier's scheme and host, so the single core.ParsePublicURL SSRF check on -// identifier covers them all. +// shares identifier's scheme and host, so the SSRF check that httpClient.Do runs on the first +// candidate it fetches covers them all. // // When every candidate fails, the returned error joins each candidate's failure. A per-candidate // core.HttpError stays recoverable through the join (see errors.AsType), so callers can still @@ -55,9 +56,9 @@ var ErrAllCandidates4xx = errors.New("metadata not found: every candidate return func FetchMetadata[T interface { WellKnownPath() string GetIssuer() string -}](ctx context.Context, httpClient core.HTTPRequestDoer, identifier string, strictMode bool) (*T, error) { +}](ctx context.Context, httpClient core.HTTPRequestDoer, identifier string) (*T, error) { var zero T - candidates, err := wellKnownCandidates(identifier, strictMode, zero.WellKnownPath()) + candidates, err := wellKnownCandidates(identifier, zero.WellKnownPath()) if err != nil { return nil, err } @@ -113,8 +114,11 @@ func fetchMetadataCandidate[T interface{ GetIssuer() string }](ctx context.Conte // wellKnownCandidates returns the metadata URLs to try for identifier, in priority order: // the insert (RFC 8414) placement, then the append (OIDC Discovery) placement. When // identifier has no path, both collapse to a single URL. -func wellKnownCandidates(identifier string, strictMode bool, wellKnown string) ([]string, error) { - identifierURL, err := core.ParsePublicURL(identifier, strictMode) +// +// identifier is not SSRF-checked here: httpClient.Do runs that check on every candidate +// before it connects, so validating it again here would just duplicate that check. +func wellKnownCandidates(identifier string, wellKnown string) ([]string, error) { + identifierURL, err := url.Parse(identifier) if err != nil { return nil, err } diff --git a/auth/oauth/metadata_test.go b/auth/oauth/metadata_test.go index 133bf8971f..8aa7ada77a 100644 --- a/auth/oauth/metadata_test.go +++ b/auth/oauth/metadata_test.go @@ -31,8 +31,11 @@ import ( ) func TestWellKnownCandidates(t *testing.T) { + // SSRF checks (HTTPS-only, no IP hosts, no reserved hosts) are not wellKnownCandidates' + // concern: httpclient.StrictHTTPClient runs them on every candidate before connecting. + // See http/client.TestStrictHTTPClient for that coverage. t.Run("identifier with path returns insert then append", func(t *testing.T) { - candidates, err := wellKnownCandidates("https://nuts.nl/iam/id", true, AuthzServerWellKnown) + candidates, err := wellKnownCandidates("https://nuts.nl/iam/id", AuthzServerWellKnown) require.NoError(t, err) assert.Equal(t, []string{ "https://nuts.nl/.well-known/oauth-authorization-server/iam/id", @@ -40,19 +43,19 @@ func TestWellKnownCandidates(t *testing.T) { }, candidates) }) t.Run("no path collapses to a single candidate", func(t *testing.T) { - candidates, err := wellKnownCandidates("https://nuts.nl", true, AuthzServerWellKnown) + candidates, err := wellKnownCandidates("https://nuts.nl", AuthzServerWellKnown) require.NoError(t, err) assert.Equal(t, []string{"https://nuts.nl/.well-known/oauth-authorization-server"}, candidates) }) t.Run("trailing-slash-only path collapses to a single candidate", func(t *testing.T) { - candidates, err := wellKnownCandidates("https://nuts.nl/", true, AuthzServerWellKnown) + candidates, err := wellKnownCandidates("https://nuts.nl/", AuthzServerWellKnown) require.NoError(t, err) assert.Equal(t, []string{"https://nuts.nl/.well-known/oauth-authorization-server"}, candidates) }) t.Run("terminating slash on the path is removed in both candidates", func(t *testing.T) { // RFC 8414 §3.1 / OIDC Discovery §4.1: any terminating "/" MUST be removed // before inserting/appending the well-known segment. - candidates, err := wellKnownCandidates("https://nuts.nl/iam/id/", true, AuthzServerWellKnown) + candidates, err := wellKnownCandidates("https://nuts.nl/iam/id/", AuthzServerWellKnown) require.NoError(t, err) assert.Equal(t, []string{ "https://nuts.nl/.well-known/oauth-authorization-server/iam/id", @@ -60,16 +63,16 @@ func TestWellKnownCandidates(t *testing.T) { }, candidates) }) t.Run("percent-encoded path is not double-escaped", func(t *testing.T) { - candidates, err := wellKnownCandidates("https://nuts.nl/foo%2Fbar", true, OpenIdCredIssuerWellKnown) + candidates, err := wellKnownCandidates("https://nuts.nl/foo%2Fbar", OpenIdCredIssuerWellKnown) require.NoError(t, err) assert.Equal(t, []string{ "https://nuts.nl/.well-known/openid-credential-issuer/foo%2Fbar", "https://nuts.nl/foo%2Fbar/.well-known/openid-credential-issuer", }, candidates) }) - t.Run("invalid identifier returns the SSRF/parse error", func(t *testing.T) { - candidates, err := wellKnownCandidates("http://nuts.nl/iam/id", true, AuthzServerWellKnown) - assert.ErrorContains(t, err, "scheme must be https") + t.Run("invalid identifier returns the parse error", func(t *testing.T) { + candidates, err := wellKnownCandidates("http:// /iam/id", AuthzServerWellKnown) + assert.ErrorContains(t, err, "invalid character \" \" in host name") assert.Nil(t, candidates) }) } @@ -104,7 +107,7 @@ func TestFetchMetadata(t *testing.T) { t.Run("ok - insert form, root identifier, single request", func(t *testing.T) { srv, requested := metadataServer(t, http.StatusNotFound, "", "/.well-known/oauth-authorization-server") - metadata, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL, false) + metadata, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL) require.NoError(t, err) require.NotNil(t, metadata) @@ -114,7 +117,7 @@ func TestFetchMetadata(t *testing.T) { t.Run("ok - append form when insert 404s", func(t *testing.T) { srv, requested := metadataServer(t, http.StatusNotFound, "/iam/123", "/iam/123/.well-known/oauth-authorization-server") - metadata, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123", false) + metadata, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123") require.NoError(t, err) require.NotNil(t, metadata) @@ -140,7 +143,7 @@ func TestFetchMetadata(t *testing.T) { t.Cleanup(srv.Close) issuer = srv.URL + "/iam/123" - metadata, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), issuer, false) + metadata, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), issuer) require.NoError(t, err) require.NotNil(t, metadata) @@ -160,7 +163,7 @@ func TestFetchMetadata(t *testing.T) { t.Cleanup(srv.Close) issuer = srv.URL + "/oauth" - _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), issuer, false) + _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), issuer) require.Error(t, err) assert.Contains(t, err.Error(), "does not match requested identifier") @@ -168,7 +171,7 @@ func TestFetchMetadata(t *testing.T) { t.Run("error - all candidates 404 names the identifier and the tried locations", func(t *testing.T) { srv, requested := metadataServer(t, http.StatusNotFound, "/iam/123") - _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123", false) + _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123") require.Error(t, err) assert.Contains(t, err.Error(), "failed to retrieve metadata") @@ -183,7 +186,7 @@ func TestFetchMetadata(t *testing.T) { t.Run("error - a candidate's core.HttpError stays recoverable through the join", func(t *testing.T) { srv, requested := metadataServer(t, http.StatusInternalServerError, "/iam/123") - _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123", false) + _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123") require.Error(t, err) var httpErr core.HttpError @@ -208,7 +211,7 @@ func TestFetchMetadata(t *testing.T) { })) t.Cleanup(srv.Close) - _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123", false) + _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL+"/iam/123") require.Error(t, err) assert.NotErrorIs(t, err, ErrAllCandidates4xx) @@ -220,15 +223,15 @@ func TestFetchMetadata(t *testing.T) { })) t.Cleanup(srv.Close) - _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL, false) + _, err := FetchMetadata[AuthorizationServerMetadata](ctx, srv.Client(), srv.URL) require.Error(t, err) assert.Contains(t, err.Error(), "decoding metadata") }) t.Run("error - invalid identifier is rejected before any request", func(t *testing.T) { - _, err := FetchMetadata[AuthorizationServerMetadata](ctx, http.DefaultClient, "http://nuts.nl/iam/id", true) + _, err := FetchMetadata[AuthorizationServerMetadata](ctx, http.DefaultClient, "http:// /iam/id") require.Error(t, err) - assert.ErrorContains(t, err, "scheme must be https") + assert.ErrorContains(t, err, "invalid character \" \" in host name") }) } diff --git a/auth/oauth/types.go b/auth/oauth/types.go index 97be477427..d36fe6dccd 100644 --- a/auth/oauth/types.go +++ b/auth/oauth/types.go @@ -24,7 +24,6 @@ import ( "net/url" "github.com/lestrrat-go/jwx/v3/jwk" - "github.com/nuts-foundation/nuts-node/core" ) // this file contains constants, variables and helper functions for OAuth related code @@ -248,8 +247,11 @@ const ( // IssuerIdToWellKnown converts the OAuth2 Issuer identity to the specified well-known endpoint by inserting the well-known at the root of the path. // It returns no url and an error when issuer is not a valid URL. -func IssuerIdToWellKnown(issuer string, wellKnown string, strictmode bool) (*url.URL, error) { - issuerURL, err := core.ParsePublicURL(issuer, strictmode) +// +// issuer is not SSRF-checked here: the returned URL is only fetched through httpclient.StrictHTTPClient, +// which runs that check before connecting. +func IssuerIdToWellKnown(issuer string, wellKnown string) (*url.URL, error) { + issuerURL, err := url.Parse(issuer) if err != nil { return nil, err } diff --git a/auth/oauth/types_test.go b/auth/oauth/types_test.go index 9e6f22351a..df5fc8c8e6 100644 --- a/auth/oauth/types_test.go +++ b/auth/oauth/types_test.go @@ -28,41 +28,30 @@ import ( ) func TestIssuerIdToWellKnown(t *testing.T) { + // SSRF checks (HTTPS-only, no IP hosts, no reserved hosts) are not IssuerIdToWellKnown's + // concern: httpclient.StrictHTTPClient runs them on the returned URL before connecting. + // See http/client.TestStrictHTTPClient for that coverage. t.Run("ok", func(t *testing.T) { issuer := "https://nuts.nl/iam/id" - u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown, true) + u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown) require.NoError(t, err) assert.Equal(t, "https://nuts.nl/.well-known/oauth-authorization-server/iam/id", u.String()) }) t.Run("no path in issuer", func(t *testing.T) { issuer := "https://nuts.nl" - u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown, true) + u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown) require.NoError(t, err) assert.Equal(t, "https://nuts.nl/.well-known/oauth-authorization-server", u.String()) }) t.Run("don't unescape path", func(t *testing.T) { issuer := "https://nuts.nl/iam/%2E%2E/still-has-iam" - u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown, true) + u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown) require.NoError(t, err) assert.Equal(t, "https://nuts.nl/.well-known/oauth-authorization-server/iam/%2E%2E/still-has-iam", u.String()) }) - t.Run("https in strictmode", func(t *testing.T) { - issuer := "http://nuts.nl/iam/id" - u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown, true) - assert.ErrorContains(t, err, "scheme must be https") - assert.Nil(t, u) - }) - t.Run("no IP allowed", func(t *testing.T) { - issuer := "https://127.0.0.1/iam/id" - - u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown, true) - - assert.ErrorContains(t, err, "hostname is IP") - assert.Nil(t, u) - }) t.Run("invalid URL", func(t *testing.T) { issuer := "http:// /iam/id" - u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown, true) + u, err := IssuerIdToWellKnown(issuer, AuthzServerWellKnown) assert.ErrorContains(t, err, "invalid character \" \" in host name") assert.Nil(t, u) }) diff --git a/auth/services/oauth/relying_party.go b/auth/services/oauth/relying_party.go index c3a36ba911..8b2393167e 100644 --- a/auth/services/oauth/relying_party.go +++ b/auth/services/oauth/relying_party.go @@ -52,8 +52,7 @@ type relyingParty struct { pkiValidator pki.Validator } -// NewRelyingParty returns an implementation of RelyingParty. -// Strict-mode URL validation is centralised in http/client.StrictHTTPClient; this constructor no longer takes a strictMode flag. +// NewRelyingParty returns an implementation of RelyingParty func NewRelyingParty( didResolver resolver.DIDResolver, serviceResolver didman.CompoundServiceResolver, privateKeyStore nutsCrypto.KeyStore, wallet holder.Wallet, httpClientTimeout time.Duration, httpClientTLS *tls.Config, pkiValidator pki.Validator) RelyingParty { @@ -108,6 +107,7 @@ func (s *relyingParty) CreateJwtGrant(ctx context.Context, request services.Crea } func (s *relyingParty) RequestRFC003AccessToken(ctx context.Context, jwtGrantToken string, authorizationServerEndpoint url.URL) (*oauth.TokenResponse, error) { + // strictHttp.StrictHTTPClient.Do enforces HTTPS (and the rest of the SSRF checks) in strict mode. httpClient := strictHttp.NewWithTLSConfig(s.httpClientTimeout, s.httpClientTLS) authClient, err := client.NewHTTPClient("", s.httpClientTimeout, client.WithHTTPClient(httpClient), client.WithRequestEditorFn(core.UserAgentRequestEditor)) if err != nil { diff --git a/auth/services/oauth/relying_party_test.go b/auth/services/oauth/relying_party_test.go index 057efbf0c2..421dd54894 100644 --- a/auth/services/oauth/relying_party_test.go +++ b/auth/services/oauth/relying_party_test.go @@ -75,6 +75,8 @@ func TestRelyingParty_RequestRFC003AccessToken(t *testing.T) { assert.EqualError(t, err, "remote server/nuts node returned error creating access token: server returned HTTP 502 (expected: 200)") }) + // Strict-mode endpoint validation (HTTPS-only, no IP/reserved hosts) is enforced by + // httpclient.StrictHTTPClient, not relyingParty; see http/client.TestStrictHTTPClient. } func TestService_CreateJwtBearerToken(t *testing.T) { diff --git a/core/url.go b/core/url.go index 22544b0892..7decbc7afb 100644 --- a/core/url.go +++ b/core/url.go @@ -52,11 +52,27 @@ func ParsePublicURL(input string, strictmode bool) (*url.URL, error) { return ParsePublicURLWithScheme(input, false, "https") } +// ParsePublicURLAllowIP behaves like ParsePublicURL, except an IP-literal host is never rejected. +// Use it where a separate, allowlist-aware check already polices the address being connected to +// (for example http/client's dial-time SSRF guard, which honors http.client.allowedinternalcidrs): +// rejecting IP literals here as well would make that allowlist unreachable, since a literal IP +// never gets resolved — it already is the address the guard needs to see. +func ParsePublicURLAllowIP(input string, strictmode bool) (*url.URL, error) { + if !strictmode { + return ParsePublicURLWithScheme(input, true, "http", "https") + } + return parsePublicURLWithScheme(input, true, false, "https") +} + // ParsePublicURLWithScheme parses the given input string as URL and asserts that // it has a scheme and that it is in the allowedSchemes if provided, // it is not an IP address, and // it is not (depending on allowReserved) a reserved address or TLD as described in RFC2606 or https://www.ietf.org/archive/id/draft-chapin-rfc2606bis-00.html. func ParsePublicURLWithScheme(input string, allowReserved bool, allowedSchemes ...string) (*url.URL, error) { + return parsePublicURLWithScheme(input, allowReserved, allowReserved, allowedSchemes...) +} + +func parsePublicURLWithScheme(input string, allowIP, allowReserved bool, allowedSchemes ...string) (*url.URL, error) { parsed, err := url.Parse(input) if err != nil { return nil, err @@ -67,7 +83,7 @@ func ParsePublicURLWithScheme(input string, allowReserved bool, allowedSchemes . if len(allowedSchemes) > 0 && !slices.Contains(allowedSchemes, parsed.Scheme) { return nil, fmt.Errorf("scheme must be %s", strings.Join(allowedSchemes, " or ")) } - if net.ParseIP(parsed.Hostname()) != nil && !allowReserved { + if net.ParseIP(parsed.Hostname()) != nil && !allowIP { return nil, errors.New("hostname is IP") } if !allowReserved && isReserved(parsed) { diff --git a/core/url_test.go b/core/url_test.go index 1ee1cbb3b2..f5299b7514 100644 --- a/core/url_test.go +++ b/core/url_test.go @@ -62,6 +62,29 @@ func Test_ParsePublicURL(t *testing.T) { }) } +func Test_ParsePublicURLAllowIP(t *testing.T) { + t.Run("ok - strict - IP address allowed", func(t *testing.T) { + u, err := ParsePublicURLAllowIP("https://127.0.0.1", true) + require.NoError(t, err) + assert.Equal(t, "https://127.0.0.1", u.String()) + }) + t.Run("error - strict - scheme must be https", func(t *testing.T) { + u, err := ParsePublicURLAllowIP("http://127.0.0.1", true) + assert.Nil(t, u) + assert.EqualError(t, err, "scheme must be https") + }) + t.Run("error - strict - reserved address still rejected", func(t *testing.T) { + u, err := ParsePublicURLAllowIP("https://localhost", true) + assert.Nil(t, u) + assert.EqualError(t, err, "hostname is RFC2606 reserved") + }) + t.Run("ok - non-strict", func(t *testing.T) { + u, err := ParsePublicURLAllowIP("http://localhost", false) + require.NoError(t, err) + assert.Equal(t, "http://localhost", u.String()) + }) +} + func Test_ParsePublicURLWithScheme(t *testing.T) { errIncompleteURL := errors.New("url must contain scheme and host") errIsIpAddress := errors.New("hostname is IP") diff --git a/docs/pages/deployment/configuration.rst b/docs/pages/deployment/configuration.rst index baa604fa4d..3de8676c4b 100644 --- a/docs/pages/deployment/configuration.rst +++ b/docs/pages/deployment/configuration.rst @@ -96,5 +96,5 @@ json-ld context can only be downloaded from trusted domains configured in ``json and the ``internalratelimiter`` is always on. Interacting with remote Nuts nodes requires HTTPS: it will refuse to connect to plain HTTP endpoints when in strict mode. -Strict mode additionally rejects outbound URLs whose host is an IP literal or an RFC 2606 reserved hostname/TLD (e.g. ``*.localhost``, ``*.test``, ``example.com/net/org``). +Strict mode additionally rejects outbound URLs whose host is an RFC 2606 reserved hostname/TLD (e.g. ``*.localhost``, ``*.test``, ``example.com/net/org``); non-public IP addresses are refused too, unless explicitly permitted via ``http.client.allowedinternalcidrs``. This applies to every outbound HTTP call made via the shared HTTP client (OpenID4VCI, OAuth relying-party, IAM, Discovery, did:web resolution, etc.) and to every redirect target along the way. \ No newline at end of file diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index 822e8b1a23..895457c789 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -16,6 +16,7 @@ Unreleased ## Security * Upgrade Go to 1.26.5 to address `GO-2026-5856 `_ (de-anonymization of Encrypted Client Hello (ECH) handshakes), `GO-2026-5039 `_ (net/textproto included user input in error messages, allowing injection of misleading content into error logs) and `GO-2026-5037 `_ (quadratic time complexity in crypto/x509 certificate hostname verification). * #4421: Stop reflecting fetched HTTP response bodies in API responses. The OAuth2 and OpenID4VCI callback handlers no longer place a remote endpoint's response body or error text into the returned ``error_description``, the did:web resolver no longer returns the fetched document body in its parse error, and the Discovery Service client no longer includes the remote server's error response in errors returned through the discovery APIs. Such content is now logged (truncated) for diagnostics instead. Static context such as the endpoint that failed is retained. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4421 +* #4244: Centralize outbound URL validation (HTTPS-only, no RFC 2606 reserved hosts) in the shared strict-mode HTTP client instead of duplicating it in each caller (OpenID4VCI, IAM, OAuth relying-party). The same check now also runs on every redirect target, not just the first request. IP-address validation is left to the dial-time SSRF guard (#4420), which already honors ``http.client.allowedinternalcidrs``/``deniedcidrs``. By @JorisHeadease in https://github.com/nuts-foundation/nuts-node/pull/4246 * #4420: Harden the strict-mode HTTP client against SSRF. In strict mode the client now refuses at connect time to reach non-public addresses (loopback, private/RFC1918, unique local, link-local and unspecified), checked against the resolved IP so DNS-rebinding cannot bypass it, and refuses to follow a redirect that downgrades from HTTPS to HTTP. Cloud provider metadata endpoints are always blocked, following the OWASP SSRF prevention cheat sheet. Deployments that legitimately reach a private address for an internal flow (such as an internal credential offering or OAuth user flow) can permit specific ranges with ``http.client.allowedinternalcidrs``; publicly routable ranges that are internal-only can additionally be blocked with ``http.client.deniedcidrs``, which takes precedence. Reported by @raysabee, fixed by @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4420 ***************** diff --git a/http/client/client.go b/http/client/client.go index 4044d3776b..cb4e347ce3 100644 --- a/http/client/client.go +++ b/http/client/client.go @@ -21,6 +21,7 @@ package client import ( "bytes" "crypto/tls" + "errors" "fmt" "io" "net" @@ -32,21 +33,6 @@ import ( "go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp" ) -// maxRedirects matches net/http's default redirect cap. -const maxRedirects = 10 - -// checkRedirect re-runs core.ParsePublicURL on every redirect target so a -// validated initial URL cannot be turned into an SSRF via a 3xx response. -func checkRedirect(req *http.Request, via []*http.Request) error { - if len(via) >= maxRedirects { - return fmt.Errorf("stopped after %d redirects", maxRedirects) - } - if _, err := core.ParsePublicURL(req.URL.String(), StrictMode); err != nil { - return fmt.Errorf("httpclient: invalid redirect target: %w", err) - } - return nil -} - // SafeHttpTransport is a http.Transport that can be used as a default transport for HTTP clients. // It carries the strict-mode SSRF dial guard (see denyNonPublicAddr), but only that: the HTTPS // requirement, the redirect-downgrade check and the response size limit live on StrictHTTPClient. @@ -84,17 +70,18 @@ var StrictMode bool // checkRedirect is the http.Client.CheckRedirect policy for the strict HTTP client. // Setting CheckRedirect replaces the standard library's default policy, so the -// 10-redirect cap is reimplemented here. In strict mode it also refuses to follow -// a redirect to a non-HTTPS target: the HTTPS check in Do only guards the first -// hop, and the dialer guard sees only the resolved IP and not the scheme, so -// without this a valid remote host could redirect the client onto a plaintext -// internal endpoint. +// 10-redirect cap is reimplemented here. It also re-runs core.ParsePublicURLAllowIP on +// every redirect target: the check in Do only guards the first hop, so without this a +// valid remote host could redirect the client onto a plaintext or otherwise disallowed +// endpoint. IP-literal hosts are left to the dial guard (see denyNonPublicAddr), which +// runs on every hop already and is aware of http.client.allowedinternalcidrs/deniedcidrs; +// rejecting them here too would make that allowlist unreachable. func checkRedirect(req *http.Request, via []*http.Request) error { if len(via) >= 10 { return errors.New("stopped after 10 redirects") } - if StrictMode && req.URL.Scheme != "https" { - return errors.New("strictmode is enabled, but redirect target is not over HTTPS") + if _, err := core.ParsePublicURLAllowIP(req.URL.String(), StrictMode); err != nil { + return fmt.Errorf("invalid redirect target: %w", err) } return nil } @@ -170,8 +157,10 @@ type StrictHTTPClient struct { } func (s *StrictHTTPClient) Do(req *http.Request) (*http.Response, error) { - if _, err := core.ParsePublicURL(req.URL.String(), StrictMode); err != nil { - return nil, fmt.Errorf("httpclient: invalid target URL: %w", err) + // IP-literal hosts are left to the dial guard (see denyNonPublicAddr): it is aware of + // http.client.allowedinternalcidrs/deniedcidrs, this check is not. + if _, err := core.ParsePublicURLAllowIP(req.URL.String(), StrictMode); err != nil { + return nil, fmt.Errorf("invalid target URL: %w", err) } req.Header.Set("User-Agent", core.UserAgent()) result, err := s.client.Do(req) diff --git a/http/client/client_test.go b/http/client/client_test.go index 1189fb8745..19ee707436 100644 --- a/http/client/client_test.go +++ b/http/client/client_test.go @@ -21,7 +21,6 @@ package client import ( "crypto/tls" "fmt" - "io" "net/http" "net/http/httptest" "strings" @@ -35,25 +34,11 @@ import ( "github.com/stretchr/testify/require" ) -// withClientGlobals captures the package-level globals StrictMode and -// DefaultCachingTransport at call time and restores them via t.Cleanup, -// so subtests can mutate them without leaking state into other tests. -func withClientGlobals(t *testing.T) { - t.Helper() - oldStrict := StrictMode - oldTransport := DefaultCachingTransport - t.Cleanup(func() { - StrictMode = oldStrict - DefaultCachingTransport = oldTransport - }) -} - func TestStrictHTTPClient(t *testing.T) { oldStrictMode := StrictMode t.Cleanup(func() { StrictMode = oldStrictMode }) t.Run("caching transport", func(t *testing.T) { t.Run("strict mode enabled", func(t *testing.T) { - withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true @@ -62,11 +47,10 @@ func TestStrictHTTPClient(t *testing.T) { httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) _, err := client.Do(httpRequest) - assert.ErrorContains(t, err, "httpclient: invalid target URL") + assert.ErrorContains(t, err, "invalid target URL") assert.Equal(t, 0, rt.invocations) }) t.Run("strict mode disabled", func(t *testing.T) { - withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = false @@ -81,7 +65,6 @@ func TestStrictHTTPClient(t *testing.T) { }) t.Run("TLS transport", func(t *testing.T) { t.Run("strict mode enabled", func(t *testing.T) { - withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true @@ -90,7 +73,7 @@ func TestStrictHTTPClient(t *testing.T) { httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) _, err := client.Do(httpRequest) - assert.ErrorContains(t, err, "httpclient: invalid target URL") + assert.ErrorContains(t, err, "invalid target URL") assert.Equal(t, 0, rt.invocations) }) t.Run("sets TLS config", func(t *testing.T) { @@ -105,7 +88,6 @@ func TestStrictHTTPClient(t *testing.T) { }) }) t.Run("error on HTTP call when strictmode is enabled", func(t *testing.T) { - withClientGlobals(t) rt := &stubRoundTripper{} DefaultCachingTransport = rt StrictMode = true @@ -114,34 +96,39 @@ func TestStrictHTTPClient(t *testing.T) { httpRequest, _ := http.NewRequest("GET", "http://example.com", nil) _, err := client.Do(httpRequest) - assert.ErrorContains(t, err, "httpclient: invalid target URL") + assert.ErrorContains(t, err, "invalid target URL") assert.Equal(t, 0, rt.invocations) }) - t.Run("strict mode rejects IP host", func(t *testing.T) { - withClientGlobals(t) + t.Run("strict mode allows IP host, deferring to the dial guard", func(t *testing.T) { + // The dial guard (see denyNonPublicAddr / TestSafeHttpTransport_SSRFDialGuard) is what + // polices IP addresses: it honors http.client.allowedinternalcidrs/deniedcidrs, which + // this string-level check has no visibility into. Rejecting IP literals here too would + // make that allowlist unreachable. + oldStrictMode := StrictMode + StrictMode = true + t.Cleanup(func() { StrictMode = oldStrictMode }) rt := &stubRoundTripper{} DefaultCachingTransport = rt - StrictMode = true client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "https://127.0.0.1/foo", nil) _, err := client.Do(httpRequest) - assert.ErrorContains(t, err, "httpclient: invalid target URL") - assert.ErrorContains(t, err, "hostname is IP") - assert.Equal(t, 0, rt.invocations) + assert.NoError(t, err) + assert.Equal(t, 1, rt.invocations) }) t.Run("strict mode rejects RFC2606 reserved host", func(t *testing.T) { - withClientGlobals(t) + oldStrictMode := StrictMode + StrictMode = true + t.Cleanup(func() { StrictMode = oldStrictMode }) rt := &stubRoundTripper{} DefaultCachingTransport = rt - StrictMode = true client := NewWithCache(time.Second) httpRequest, _ := http.NewRequest("GET", "https://service.localhost/foo", nil) _, err := client.Do(httpRequest) - assert.ErrorContains(t, err, "httpclient: invalid target URL") + assert.ErrorContains(t, err, "invalid target URL") assert.ErrorContains(t, err, "hostname is RFC2606 reserved") assert.Equal(t, 0, rt.invocations) }) @@ -152,139 +139,41 @@ func TestCheckRedirect(t *testing.T) { req, _ := http.NewRequest("GET", target, nil) return req } - t.Run("strict mode rejects http redirect", func(t *testing.T) { - withClientGlobals(t) - StrictMode = true - err := checkRedirect(makeReq("http://example.org"), nil) - assert.ErrorContains(t, err, "invalid redirect target") - assert.ErrorContains(t, err, "scheme must be https") - }) - t.Run("strict mode rejects redirect to IP host", func(t *testing.T) { - withClientGlobals(t) - StrictMode = true + setStrictMode := func(t *testing.T, v bool) { + old := StrictMode + StrictMode = v + t.Cleanup(func() { StrictMode = old }) + } + t.Run("strict mode allows redirect to IP host, deferring to the dial guard", func(t *testing.T) { + setStrictMode(t, true) err := checkRedirect(makeReq("https://127.0.0.1/x"), nil) - assert.ErrorContains(t, err, "invalid redirect target") - assert.ErrorContains(t, err, "hostname is IP") + assert.NoError(t, err) }) t.Run("strict mode rejects redirect to reserved host", func(t *testing.T) { - withClientGlobals(t) - StrictMode = true + setStrictMode(t, true) err := checkRedirect(makeReq("https://internal.localhost/x"), nil) assert.ErrorContains(t, err, "invalid redirect target") assert.ErrorContains(t, err, "hostname is RFC2606 reserved") }) - t.Run("non-strict mode allows http redirect", func(t *testing.T) { - withClientGlobals(t) - StrictMode = false - err := checkRedirect(makeReq("http://example.org"), nil) - assert.NoError(t, err) - }) t.Run("non-strict mode allows redirect to IP host", func(t *testing.T) { - withClientGlobals(t) - StrictMode = false + setStrictMode(t, false) err := checkRedirect(makeReq("http://127.0.0.1/x"), nil) assert.NoError(t, err) }) t.Run("redirect cap stops after 10 hops", func(t *testing.T) { - withClientGlobals(t) - StrictMode = false - via := make([]*http.Request, maxRedirects) + setStrictMode(t, false) + via := make([]*http.Request, 10) err := checkRedirect(makeReq("http://example.org"), via) assert.ErrorContains(t, err, "stopped after 10 redirects") }) t.Run("cap checked before URL validation", func(t *testing.T) { - // Even an invalid target should produce the cap error first when via is at the limit. - withClientGlobals(t) - StrictMode = true - via := make([]*http.Request, maxRedirects) + setStrictMode(t, true) + via := make([]*http.Request, 10) err := checkRedirect(makeReq("http://example.org"), via) assert.ErrorContains(t, err, "stopped after 10 redirects") }) } -// redirectOnceTransport is a stub RoundTripper that responds with a 302 to redirectTo -// on the first request, then a 200 OK on subsequent requests. It also counts hops so -// tests can verify the redirected request was (or was not) sent. -type redirectOnceTransport struct { - redirectTo string - requests []*http.Request -} - -func (t *redirectOnceTransport) RoundTrip(req *http.Request) (*http.Response, error) { - t.requests = append(t.requests, req) - if len(t.requests) == 1 { - return &http.Response{ - StatusCode: http.StatusFound, - Header: http.Header{"Location": []string{t.redirectTo}}, - Body: io.NopCloser(strings.NewReader("")), - Request: req, - }, nil - } - return &http.Response{ - StatusCode: http.StatusOK, - Header: http.Header{}, - Body: io.NopCloser(strings.NewReader("ok")), - Request: req, - }, nil -} - -// TestStrictHTTPClient_RedirectEndToEnd drives the full net/http redirect path -// through a stub transport so we prove the configured CheckRedirect actually -// fires and blocks the second HTTP request from being issued. -// -// Initial URLs use "nuts.nl" because strict mode rejects RFC 2606 reserved 2LDs -// (example.com/net/org), and the test needs the initial Do() to pass so the -// redirect can be exercised. The hostname is never resolved — the stub -// transport intercepts all requests. -func TestStrictHTTPClient_RedirectEndToEnd(t *testing.T) { - const initialURL = "https://nuts.nl/" - t.Run("strict mode blocks redirect to non-https target", func(t *testing.T) { - withClientGlobals(t) - rt := &redirectOnceTransport{redirectTo: "http://example.com/x"} - DefaultCachingTransport = rt - StrictMode = true - - c := NewWithCache(time.Second) - req, _ := http.NewRequest("GET", initialURL, nil) - _, err := c.Do(req) - - require.Error(t, err) - assert.ErrorContains(t, err, "invalid redirect target") - assert.ErrorContains(t, err, "scheme must be https") - // only the initial request reached the transport; the redirect was blocked - assert.Len(t, rt.requests, 1, "second request must not be issued") - }) - t.Run("strict mode blocks redirect to IP host", func(t *testing.T) { - withClientGlobals(t) - rt := &redirectOnceTransport{redirectTo: "https://10.0.0.1/x"} - DefaultCachingTransport = rt - StrictMode = true - - c := NewWithCache(time.Second) - req, _ := http.NewRequest("GET", initialURL, nil) - _, err := c.Do(req) - - require.Error(t, err) - assert.ErrorContains(t, err, "invalid redirect target") - assert.ErrorContains(t, err, "hostname is IP") - assert.Len(t, rt.requests, 1) - }) - t.Run("non-strict mode follows http redirect", func(t *testing.T) { - withClientGlobals(t) - rt := &redirectOnceTransport{redirectTo: "http://example.com/x"} - DefaultCachingTransport = rt - StrictMode = false - - c := NewWithCache(time.Second) - req, _ := http.NewRequest("GET", "http://example.com/", nil) - resp, err := c.Do(req) - - require.NoError(t, err) - assert.Equal(t, http.StatusOK, resp.StatusCode) - assert.Len(t, rt.requests, 2, "both initial and redirected request should be issued") - }) -} - func TestStrictHTTPClient_RedirectScheme(t *testing.T) { original := tracing.Enabled() tracing.SetEnabled(false) // ensure the constructed client uses the raw transport @@ -362,8 +251,9 @@ func TestLimitedReadAll(t *testing.T) { } func TestMaxConns(t *testing.T) { - withClientGlobals(t) + oldStrictMode := StrictMode StrictMode = false + t.Cleanup(func() { StrictMode = oldStrictMode }) // Our safe http Transport has MaxConnsPerHost = 5 // if we request 6 resources multiple times, we expect a max connection usage of 5 @@ -406,8 +296,9 @@ func TestMaxConns(t *testing.T) { } func TestCaching(t *testing.T) { - withClientGlobals(t) + oldStrictMode := StrictMode StrictMode = false + t.Cleanup(func() { StrictMode = oldStrictMode }) // counter for the number of concurrent requests var total atomic.Int32