diff --git a/docs/pages/deployment/server_options.rst b/docs/pages/deployment/server_options.rst index 3fe005a03..516ad6d12 100755 --- a/docs/pages/deployment/server_options.rst +++ b/docs/pages/deployment/server_options.rst @@ -34,7 +34,7 @@ discovery.server.ids [] IDs of the Discovery Service for which to act as server. If an ID does not map to a loaded service definition, the node will fail to start. **HTTP** http.clientipheader X-Forwarded-For Case-sensitive HTTP Header that contains the client IP used for audit logs. For the X-Forwarded-For header only link-local, loopback, and private IPs are excluded. Switch to X-Real-IP or a custom header if you see your own proxy/infra in the logs. - http.log metadata What to log about HTTP requests. Options are 'nothing', 'metadata' (log request method, URI, IP and response code), and 'metadata-and-body' (log the request and response body, in addition to the metadata). When debug vebosity is set the authorization headers are also logged when the request is fully logged. + http.log metadata What to log about HTTP requests. Options are 'nothing', 'metadata' (log request method, URI, IP and response code), and 'metadata-and-body' (log the request and response body, in addition to the metadata). http.cache.maxbytes 10485760 HTTP client maximum size of the response cache in bytes. If 0, the HTTP client does not cache responses. http.client.allowedinternalcidrs [] IP ranges (CIDR notation, e.g. 10.0.0.0/8) exempted from the strict-mode SSRF guard, which otherwise blocks outbound requests to non-public networks. Use to permit internal flows that legitimately target a private address, such as an internal credential offering or an internal OAuth user flow. Leave empty to block all non-public addresses. http.client.deniedcidrs [] IP ranges (CIDR notation) that outbound HTTP requests must never target in strict mode, in addition to the built-in blocked ranges (non-public addresses and cloud metadata endpoints). Use for publicly routable ranges that are internal-only in your infrastructure. Takes precedence over http.client.allowedinternalcidrs. diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index 563d39aa8..56d2ac85e 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -14,6 +14,7 @@ Unreleased * #4233: ``request-credential`` API gains an optional ``credential_request_params`` JSON object overlaid on top of the OpenID4VCI Credential Request body sent to the issuer. Lets the wallet talk to issuers that accept additional fields, or to override the credential request entirely. ## Security +* #4437: Remove the never-functional ``Authorization``/``DPoP`` request header capture from the HTTP request logger. The middleware was configured to copy these headers into request log entries at ``debug`` verbosity, which would have written bearer tokens to the logs. A level-check bug meant the capture never fired, so no released version has ever logged these headers and no operator action is needed. The capture is removed entirely, instead of repaired and filtered, so a future fix of the level check cannot silently start logging bearer tokens. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4437 * #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 * #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/cmd/cmd.go b/http/cmd/cmd.go index 3700e5d3b..ccdd8b707 100644 --- a/http/cmd/cmd.go +++ b/http/cmd/cmd.go @@ -34,7 +34,7 @@ func FlagSet() *pflag.FlagSet { flags.String("http.internal.auth.type", string(defs.Internal.Auth.Type), fmt.Sprintf("Whether to enable authentication for /internal endpoints, specify '%s' for bearer token mode or '%s' for legacy bearer token mode.", http.BearerTokenAuthV2, http.BearerTokenAuth)) flags.String("http.internal.auth.audience", defs.Internal.Auth.Audience, "Expected audience for JWT tokens (default: hostname)") flags.String("http.internal.auth.authorizedkeyspath", defs.Internal.Auth.AuthorizedKeysPath, "Path to an authorized_keys file for trusted JWT signers") - flags.String("http.log", string(defs.Log), fmt.Sprintf("What to log about HTTP requests. Options are '%s', '%s' (log request method, URI, IP and response code), and '%s' (log the request and response body, in addition to the metadata). When debug vebosity is set the authorization headers are also logged when the request is fully logged.", http.LogNothingLevel, http.LogMetadataLevel, http.LogMetadataAndBodyLevel)) + flags.String("http.log", string(defs.Log), fmt.Sprintf("What to log about HTTP requests. Options are '%s', '%s' (log request method, URI, IP and response code), and '%s' (log the request and response body, in addition to the metadata).", http.LogNothingLevel, http.LogMetadataLevel, http.LogMetadataAndBodyLevel)) flags.String("http.clientipheader", defs.ClientIPHeaderName, "Case-sensitive HTTP Header that contains the client IP used for audit logs. For the X-Forwarded-For header only link-local, loopback, and private IPs are excluded. Switch to X-Real-IP or a custom header if you see your own proxy/infra in the logs.") flags.Int("http.cache.maxbytes", defs.ResponseCacheSize, "HTTP client maximum size of the response cache in bytes. If 0, the HTTP client does not cache responses.") flags.StringSlice("http.client.allowedinternalcidrs", defs.Client.AllowedInternalCIDRs, "IP ranges (CIDR notation, e.g. 10.0.0.0/8) exempted from the strict-mode SSRF guard, which otherwise blocks outbound requests to non-public networks. Use to permit internal flows that legitimately target a private address, such as an internal credential offering or an internal OAuth user flow. Leave empty to block all non-public addresses.") diff --git a/http/requestlogger.go b/http/requestlogger.go index 9a30299b2..c1e2f4498 100644 --- a/http/requestlogger.go +++ b/http/requestlogger.go @@ -31,7 +31,6 @@ import ( func requestLoggerMiddleware(skipper middleware.Skipper, logger *logrus.Entry) echo.MiddlewareFunc { return middleware.RequestLoggerWithConfig(middleware.RequestLoggerConfig{ Skipper: skipper, - LogHeaders: []string{"Authorization", "DPoP"}, LogURI: true, LogStatus: true, LogMethod: true, @@ -43,16 +42,13 @@ func requestLoggerMiddleware(skipper middleware.Skipper, logger *logrus.Entry) e status = core.GetHTTPStatusCode(values.Error, c) } - fields := logrus.Fields{ + // Never log request headers: Authorization carries bearer tokens, which must not end up in logs. + logger.WithFields(logrus.Fields{ "remote_ip": values.RemoteIP, "method": values.Method, "uri": values.URI, "status": status, - } - if logger.Level >= logrus.DebugLevel { - fields["headers"] = values.Headers - } - logger.WithFields(fields).Info("HTTP request") + }).Info("HTTP request") return nil }, diff --git a/http/requestlogger_test.go b/http/requestlogger_test.go index 93cf70f3f..8526a5f55 100644 --- a/http/requestlogger_test.go +++ b/http/requestlogger_test.go @@ -60,6 +60,32 @@ func Test_requestLoggerMiddleware(t *testing.T) { assert.Equal(t, "/test", hook.LastEntry().Data["uri"]) }) + t.Run("it does not log headers, even on debug", func(t *testing.T) { + ctrl := gomock.NewController(t) + response := &echo.Response{} + echoMock := mock.NewMockContext(ctrl) + echoMock.EXPECT().NoContent(http.StatusNoContent).Do(func(status int) { response.Status = status }) + request := &http.Request{RequestURI: "/test", Header: http.Header{}} + request.Header.Set("Authorization", "Bearer secret") + request.Header.Set("DPoP", "some-proof") + echoMock.EXPECT().Request().Return(request) + echoMock.EXPECT().Response().Return(response) + echoMock.EXPECT().RealIP().Return("::1") + + logger, hook := test.NewNullLogger() + logger.SetLevel(logrus.DebugLevel) + logFunc := requestLoggerMiddleware(func(c echo.Context) bool { + return false + }, logger.WithFields(logrus.Fields{})) + err := logFunc(func(context echo.Context) error { + return context.NoContent(http.StatusNoContent) + })(echoMock) + + require.NoError(t, err) + require.Len(t, hook.Entries, 1) + assert.NotContains(t, hook.LastEntry().Data, "headers") + }) + t.Run("it handles echo.HTTPErrors", func(t *testing.T) { ctrl := gomock.NewController(t) echoMock := mock.NewMockContext(ctrl)