diff --git a/auth/auth.go b/auth/auth.go index c64235dfc..742dbef9f 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, }) @@ -194,13 +192,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 db78cb8cb..a510b37ae 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,11 +81,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 { @@ -237,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 3e09f2727..2b4b758a4 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 34522f319..55ae7625d 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 4e53d033c..52673e681 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 c936ae422..b6d17dd18 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 133bf8971..8aa7ada77 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 97be47742..d36fe6dcc 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 9e6f22351..df5fc8c8e 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/openid4vci/client.go b/auth/openid4vci/client.go index 43c51dde5..4760e848d 100644 --- a/auth/openid4vci/client.go +++ b/auth/openid4vci/client.go @@ -78,43 +78,17 @@ 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} +// shared transport policies apply (HTTPS-in-strict, SSRF checks, body size +// limit, User-Agent header). +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 != "") { @@ -122,7 +96,7 @@ func (c *client) OpenIDCredentialIssuerMetadata(ctx context.Context, issuerURL s } // Per §12.2.4 the credential_issuer in the document MUST match the requested issuer; // FetchMetadata enforces that and tries the insert/append well-known placements. - metadata, err := oauth.FetchMetadata[OpenIDCredentialIssuerMetadata](ctx, c.httpClient, issuerURL, c.strictMode) + metadata, err := oauth.FetchMetadata[OpenIDCredentialIssuerMetadata](ctx, c.httpClient, issuerURL) if err != nil { return nil, fmt.Errorf("openid4vci: %w", err) } @@ -130,9 +104,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 @@ -156,9 +127,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 := make(map[string]any, len(opts.CredentialRequestParams)+2) // Node defaults. Per §8.2, credential_identifier and // credential_configuration_id are mutually exclusive; credential_identifier diff --git a/auth/openid4vci/client_test.go b/auth/openid4vci/client_test.go index b14350197..79eca10b7 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(), "https://attacker.example") @@ -151,29 +151,23 @@ func TestClient_OpenIDCredentialIssuerMetadata(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 beyond the // "openid4vci: " error prefix, so it's enough to confirm the wiring (well-known constant, - // httpClient, strictMode) and that prefix. + // httpClient) and that prefix. SSRF checks (HTTPS-only, no IP/reserved hosts) are enforced by + // httpclient.StrictHTTPClient, not this client; see http/client.TestStrictHTTPClient. t.Run("all candidates 404 names the identifier and the tried locations", func(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.Error(w, "not found", http.StatusNotFound) })) defer srv.Close() - client := NewClient(srv.Client(), false) + client := NewClient(srv.Client()) _, err := client.OpenIDCredentialIssuerMetadata(context.Background(), srv.URL+"/oauth2/alice") require.Error(t, err) assert.Contains(t, err.Error(), "failed to retrieve metadata") assert.Contains(t, err.Error(), srv.URL+"/oauth2/alice") }) - 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") @@ -190,7 +184,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 metadata") @@ -221,7 +215,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", @@ -245,7 +239,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", @@ -266,7 +260,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", @@ -289,7 +283,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", @@ -315,7 +309,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 c3091d19c..8b2393167 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/v3/jwt" @@ -47,7 +46,6 @@ type relyingParty struct { keyResolver resolver.KeyResolver privateKeyStore nutsCrypto.KeyStore serviceResolver didman.CompoundServiceResolver - strictMode bool httpClientTimeout time.Duration httpClientTLS *tls.Config wallet holder.Wallet @@ -57,14 +55,13 @@ type relyingParty struct { // 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, 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 +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) { - 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()) - } + // 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 9a3d647ca..421dd5489 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,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)") }) - 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) - }) - }) + // 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 22544b089..7decbc7af 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 1ee1cbb3b..f5299b751 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 4385fcb20..3de8676c4 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 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 822e8b1a2..895457c78 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 46ce1151d..cb4e347ce 100644 --- a/http/client/client.go +++ b/http/client/client.go @@ -70,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 } @@ -156,8 +157,10 @@ 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") + // 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 76c5665c7..19ee70743 100644 --- a/http/client/client_test.go +++ b/http/client/client_test.go @@ -47,7 +47,7 @@ func TestStrictHTTPClient(t *testing.T) { 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, "invalid target URL") assert.Equal(t, 0, rt.invocations) }) t.Run("strict mode disabled", func(t *testing.T) { @@ -73,7 +73,7 @@ func TestStrictHTTPClient(t *testing.T) { 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, "invalid target URL") assert.Equal(t, 0, rt.invocations) }) t.Run("sets TLS config", func(t *testing.T) { @@ -96,9 +96,82 @@ func TestStrictHTTPClient(t *testing.T) { 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, "invalid target URL") assert.Equal(t, 0, rt.invocations) }) + 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 + + client := NewWithCache(time.Second) + httpRequest, _ := http.NewRequest("GET", "https://127.0.0.1/foo", nil) + _, err := client.Do(httpRequest) + + assert.NoError(t, err) + assert.Equal(t, 1, rt.invocations) + }) + t.Run("strict mode rejects RFC2606 reserved host", func(t *testing.T) { + oldStrictMode := StrictMode + StrictMode = true + t.Cleanup(func() { StrictMode = oldStrictMode }) + rt := &stubRoundTripper{} + DefaultCachingTransport = rt + + client := NewWithCache(time.Second) + httpRequest, _ := http.NewRequest("GET", "https://service.localhost/foo", nil) + _, err := client.Do(httpRequest) + + assert.ErrorContains(t, err, "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 + } + 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.NoError(t, err) + }) + t.Run("strict mode rejects redirect to reserved host", func(t *testing.T) { + 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 redirect to IP host", func(t *testing.T) { + 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) { + 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) { + setStrictMode(t, true) + via := make([]*http.Request, 10) + err := checkRedirect(makeReq("http://example.org"), via) + assert.ErrorContains(t, err, "stopped after 10 redirects") + }) } func TestStrictHTTPClient_RedirectScheme(t *testing.T) { diff --git a/vcr/vcr_test.go b/vcr/vcr_test.go index 62ba52e37..affb404c1 100644 --- a/vcr/vcr_test.go +++ b/vcr/vcr_test.go @@ -108,7 +108,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, "http request error: invalid target URL") + assert.ErrorContains(t, err, "scheme must be https") }) }