diff --git a/discovery/api/server/client/http.go b/discovery/api/server/client/http.go index fd2733f89..5e67f64bc 100644 --- a/discovery/api/server/client/http.go +++ b/discovery/api/server/client/http.go @@ -33,10 +33,10 @@ import ( "time" ) -// New creates a new DefaultHTTPClient. -func New(timeout time.Duration) *DefaultHTTPClient { +// New creates a new DefaultHTTPClient. Options are passed to the underlying strict HTTP client. +func New(timeout time.Duration, options ...client.Option) *DefaultHTTPClient { return &DefaultHTTPClient{ - client: client.New(timeout), + client: client.New(timeout, options...), } } diff --git a/discovery/module.go b/discovery/module.go index 9251b22b4..1c145cbdf 100644 --- a/discovery/module.go +++ b/discovery/module.go @@ -28,6 +28,7 @@ import ( "github.com/nuts-foundation/nuts-node/v6/core" "github.com/nuts-foundation/nuts-node/v6/discovery/api/server/client" "github.com/nuts-foundation/nuts-node/v6/discovery/log" + httpclient "github.com/nuts-foundation/nuts-node/v6/http/client" "github.com/nuts-foundation/nuts-node/v6/storage" "github.com/nuts-foundation/nuts-node/v6/vcr" "github.com/nuts-foundation/nuts-node/v6/vcr/credential" @@ -57,6 +58,20 @@ var ( errRetractionContainsCredentials = errors.New("retraction presentation must not contain credentials") errInvalidRetractionJTIClaim = errors.New("invalid/missing 'retract_jti' claim for retraction presentation") errCyclicForwardingDetected = errors.New("cyclic forwarding detected") + errPresentationTooLarge = fmt.Errorf("presentation exceeds maximum size of %d bytes", maxPresentationSize) +) + +const ( + // maxPresentationSize is the maximum size (in bytes) of a Verifiable Presentation that can be registered on a Discovery Service. + // Legitimate registrations are a few KB; a VP carrying an X509Credential with a full PKIoverheid certificate chain + // measures about 16 KB. The cap bounds what a single participant can add to the service, so that a few + // registrations cannot push the service's responses over what clients are willing to read (maxResponseSize). + maxPresentationSize = 64 * 1024 + // maxResponseSize is the maximum size (in bytes) of a response from a remote Discovery Service that the client reads. + // It is larger than the default of the strict HTTP client, since the endpoint is operator-configured (through the + // service definition) and a service's full list of presentations can exceed 1 MB. It is bounded because the + // response is buffered and parsed in memory. + maxResponseSize = 10 * 1024 * 1024 ) var _ core.Injectable = &Module{} @@ -106,7 +121,7 @@ func (m *Module) Configure(serverConfig core.ServerConfig) error { return err } - m.httpClient = client.New(serverConfig.HTTPClient.Timeout) + m.httpClient = client.New(serverConfig.HTTPClient.Timeout, httpclient.WithMaxResponseSize(maxResponseSize)) return m.loadDefinitions() @@ -229,6 +244,9 @@ func (m *Module) Register(context context.Context, serviceID string, presentatio func (m *Module) verifyRegistration(definition ServiceDefinition, presentation vc.VerifiablePresentation) error { // First, simple sanity checks + if len(presentation.Raw()) > maxPresentationSize { + return errors.Join(ErrInvalidPresentation, errPresentationTooLarge) + } if presentation.Format() != vc.JWTPresentationProofFormat { return errors.Join(ErrInvalidPresentation, errUnsupportedPresentationFormat) } diff --git a/discovery/module_test.go b/discovery/module_test.go index dadff3b27..82377eeca 100644 --- a/discovery/module_test.go +++ b/discovery/module_test.go @@ -40,6 +40,7 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" "gorm.io/gorm" + "strings" "sync" "sync/atomic" "testing" @@ -118,6 +119,17 @@ func Test_Module_Register(t *testing.T) { assert.EqualError(t, err, "presentation is invalid for registration\npresentation is valid for too long (max 1s)") }) + t.Run("too large", func(t *testing.T) { + m, _ := setupModule(t, storageEngine) + // pad the VP with a claim to push it over the maximum size, a real one is a few KB + largeVP := createPresentationCustom(aliceDID, func(claims map[string]interface{}, _ *vc.VerifiablePresentation) { + claims["padding"] = strings.Repeat("a", maxPresentationSize) + }, vcAlice) + + err := m.Register(ctx, testServiceID, largeVP) + + assert.EqualError(t, err, "presentation is invalid for registration\npresentation exceeds maximum size of 65536 bytes") + }) t.Run("no expiration", func(t *testing.T) { m, _ := setupModule(t, storageEngine) err := m.Register(ctx, testServiceID, createPresentationCustom(aliceDID, func(claims map[string]interface{}, _ *vc.VerifiablePresentation) { diff --git a/docs/pages/deployment/discovery.rst b/docs/pages/deployment/discovery.rst index aa10f7ad2..879a051c4 100644 --- a/docs/pages/deployment/discovery.rst +++ b/docs/pages/deployment/discovery.rst @@ -184,4 +184,7 @@ A service definition consists of: - ``presentation_max_validity``: the maximum validity of the Verifiable Presentation in seconds - ``presentation_definition``: the presentation definition that specifies the required Verifiable Credentials (see `Presentation Definitions `_) +Registered Verifiable Presentations may be at most 64 KiB. This is a fixed limit of the Nuts node, not configurable per service definition. +Legitimate presentations are a few KB (about 16 KB when they carry an ``X509Credential`` with a full certificate chain), the limit keeps a single participant from inflating the service's responses. + For details see `Nuts RFC022 `_. \ No newline at end of file diff --git a/docs/pages/release_notes.rst b/docs/pages/release_notes.rst index 8022441e5..e737a127c 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -20,6 +20,7 @@ Unreleased * Network: connections on which no message was received for ``network.idletimeout`` (default ``2m``) are now closed and re-established. Peers send gossip and diagnostics messages every few seconds, so a silent connection is a dead one: typically a half-open TCP connection or a reverse proxy that kept the stream open after the other side went away. Previously such connections lingered until the proxy or node was restarted, and the peer holding the stale connection rejected new connections with ``already connected``. Set ``network.idletimeout`` to ``0`` to disable. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4562 ## Security +* #4596: Discovery Service: registered Verifiable Presentations are now limited to 64 KiB, and the Discovery Service client reads responses of up to 10 MiB from the (operator-configured) Discovery Server instead of the 1 MiB applied to other outbound HTTP calls. Previously a client could no longer synchronize a service whose response exceeded 1 MiB, which a few hundred registrations or a couple of deliberately padded ones could cause. By @reinkrul in https://github.com/nuts-foundation/nuts-node/pull/4597 * Docker image: base image upgraded to alpine 3.24.2. Upgrade github.com/go-chi/chi/v5 to v5.3.0 (`GO-2026-5777 `_, `GO-2026-5775 `_ and `GO-2026-5774 `_: IP spoofing through the ``X-Forwarded-For`` header in the ``RealIP`` middleware) as reported by image scanners. This code path is not reachable from the node according to govulncheck. * #4441: Inbound HTTP request bodies are now limited to 1MB on both the public and internal interfaces; larger requests are rejected with HTTP 413 (Request Entity Too Large). Previously no limit was enforced, contrary to what the deployment documentation stated. The heaviest legitimate requests (OAuth POSTs carrying Verifiable Presentations) stay well below this limit, and it matches the ``client_max_body_size 1M`` reverse proxy configuration the documentation recommends. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4441 * #4439: Helm chart (version 0.0.9): default ``verbosity`` changed from ``debug`` to ``info``, matching the node's own default. Debug verbosity produces far more log output than production needs and increases the impact of any log-hygiene issue. Set ``nuts.config.verbosity: debug`` in your own values to restore the old behavior. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4439 diff --git a/http/client/client.go b/http/client/client.go index a5c01ea1c..acfa254b7 100644 --- a/http/client/client.go +++ b/http/client/client.go @@ -91,26 +91,47 @@ func checkRedirect(req *http.Request, via []*http.Request) error { // This of course heavily depends on the use case, but 1MB is a reasonable default. const DefaultMaxHttpResponseSize = 1024 * 1024 -// limitedReadAll reads the given reader until the DefaultMaxHttpResponseSize is reached. -// It returns an error if more data is available than DefaultMaxHttpResponseSize. -func limitedReadAll(reader io.Reader) ([]byte, error) { - result, err := io.ReadAll(io.LimitReader(reader, DefaultMaxHttpResponseSize+1)) - if len(result) > DefaultMaxHttpResponseSize { - return nil, fmt.Errorf("data to read exceeds max. safety limit of %d bytes", DefaultMaxHttpResponseSize) +// limitedReadAll reads the given reader until the given limit is reached. +// It returns an error if more data is available than the limit. +func limitedReadAll(reader io.Reader, limit int64) ([]byte, error) { + result, err := io.ReadAll(io.LimitReader(reader, limit+1)) + if int64(len(result)) > limit { + return nil, fmt.Errorf("data to read exceeds max. safety limit of %d bytes", limit) } return result, err } +// Option configures a StrictHTTPClient. +type Option func(*StrictHTTPClient) + +// WithMaxResponseSize overrides the maximum HTTP response body size (in bytes) the client reads. +// The default is DefaultMaxHttpResponseSize. Only raise it for endpoints that are operator-configured +// and known to return large responses, since the response is buffered in memory. +func WithMaxResponseSize(size int64) Option { + return func(client *StrictHTTPClient) { + client.maxResponseSize = size + } +} + // New creates a new HTTP client with the given timeout. -func New(timeout time.Duration) *StrictHTTPClient { +func New(timeout time.Duration, options ...Option) *StrictHTTPClient { transport := getTransport(SafeHttpTransport) - return &StrictHTTPClient{ - client: &http.Client{ - Transport: transport, - Timeout: timeout, - CheckRedirect: checkRedirect, - }, + return newStrictHTTPClient(&http.Client{ + Transport: transport, + Timeout: timeout, + CheckRedirect: checkRedirect, + }, options) +} + +func newStrictHTTPClient(httpClient *http.Client, options []Option) *StrictHTTPClient { + result := &StrictHTTPClient{ + client: httpClient, + maxResponseSize: DefaultMaxHttpResponseSize, + } + for _, option := range options { + option(result) } + return result } // getTransport wraps the given transport with OpenTelemetry instrumentation if tracing is enabled. @@ -126,34 +147,31 @@ func getTransport(base http.RoundTripper) http.RoundTripper { // NewWithCache creates a new HTTP client with the given timeout. // It uses the DefaultCachingTransport as the underlying transport. -func NewWithCache(timeout time.Duration) *StrictHTTPClient { +func NewWithCache(timeout time.Duration, options ...Option) *StrictHTTPClient { transport := getTransport(DefaultCachingTransport) - return &StrictHTTPClient{ - client: &http.Client{ - Transport: transport, - Timeout: timeout, - CheckRedirect: checkRedirect, - }, - } + return newStrictHTTPClient(&http.Client{ + Transport: transport, + Timeout: timeout, + CheckRedirect: checkRedirect, + }, options) } // NewWithTLSConfig creates a new HTTP client with the given timeout and TLS configuration. // It copies the http.DefaultTransport and sets the TLSClientConfig to the given tls.Config. // As such, it can't be used in conjunction with the CachingRoundTripper. -func NewWithTLSConfig(timeout time.Duration, tlsConfig *tls.Config) *StrictHTTPClient { +func NewWithTLSConfig(timeout time.Duration, tlsConfig *tls.Config, options ...Option) *StrictHTTPClient { transport := SafeHttpTransport.Clone() transport.TLSClientConfig = tlsConfig - return &StrictHTTPClient{ - client: &http.Client{ - Transport: getTransport(transport), - Timeout: timeout, - CheckRedirect: checkRedirect, - }, - } + return newStrictHTTPClient(&http.Client{ + Transport: getTransport(transport), + Timeout: timeout, + CheckRedirect: checkRedirect, + }, options) } type StrictHTTPClient struct { - client *http.Client + client *http.Client + maxResponseSize int64 } func (s *StrictHTTPClient) Do(req *http.Request) (*http.Response, error) { @@ -168,7 +186,7 @@ func (s *StrictHTTPClient) Do(req *http.Request) (*http.Response, error) { return nil, err } if result.Body != nil { - body, err := limitedReadAll(result.Body) + body, err := limitedReadAll(result.Body, s.maxResponseSize) if err != nil { return nil, err } diff --git a/http/client/client_test.go b/http/client/client_test.go index f0b3b0411..337e63252 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" @@ -236,20 +237,52 @@ func TestStrictHTTPClient_RedirectScheme(t *testing.T) { func TestLimitedReadAll(t *testing.T) { t.Run("less than limit", func(t *testing.T) { data := strings.Repeat("a", 10) - result, err := limitedReadAll(strings.NewReader(data)) + result, err := limitedReadAll(strings.NewReader(data), DefaultMaxHttpResponseSize) assert.NoError(t, err) assert.Equal(t, []byte(data), result) }) t.Run("more than limit", func(t *testing.T) { data := strings.Repeat("a", DefaultMaxHttpResponseSize+1) - result, err := limitedReadAll(strings.NewReader(data)) + result, err := limitedReadAll(strings.NewReader(data), DefaultMaxHttpResponseSize) assert.EqualError(t, err, "data to read exceeds max. safety limit of 1048576 bytes") assert.Nil(t, result) }) } +func TestWithMaxResponseSize(t *testing.T) { + body := strings.Repeat("a", DefaultMaxHttpResponseSize+1) + server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, _ *http.Request) { + _, _ = writer.Write([]byte(body)) + })) + t.Cleanup(server.Close) + + t.Run("default limit rejects response", func(t *testing.T) { + request, _ := http.NewRequest(http.MethodGet, server.URL, nil) + + _, err := New(time.Second).Do(request) + + assert.EqualError(t, err, "data to read exceeds max. safety limit of 1048576 bytes") + }) + t.Run("raised limit accepts response", func(t *testing.T) { + request, _ := http.NewRequest(http.MethodGet, server.URL, nil) + + response, err := New(time.Second, WithMaxResponseSize(2*DefaultMaxHttpResponseSize)).Do(request) + + require.NoError(t, err) + data, _ := io.ReadAll(response.Body) + assert.Len(t, data, len(body)) + }) + t.Run("applies to all constructors", func(t *testing.T) { + option := WithMaxResponseSize(123) + assert.Equal(t, int64(123), New(time.Second, option).maxResponseSize) + assert.Equal(t, int64(123), NewWithCache(time.Second, option).maxResponseSize) + assert.Equal(t, int64(123), NewWithTLSConfig(time.Second, &tls.Config{}, option).maxResponseSize) + assert.Equal(t, int64(DefaultMaxHttpResponseSize), New(time.Second).maxResponseSize) + }) +} + func TestMaxConns(t *testing.T) { oldStrictMode := StrictMode StrictMode = false