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 18a78b0f0..f3ef4ac28 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 0cb178ef8..23f05c47a 100644 --- a/docs/pages/release_notes.rst +++ b/docs/pages/release_notes.rst @@ -6,6 +6,9 @@ Release notes Unreleased **************** +## 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 + ## Minor fixes/changes - #4620: HTTP client: the response body is now closed when a response exceeds the maximum size, so an oversized response no longer holds a connection until the client timeout. By @stevenvegt in https://github.com/nuts-foundation/nuts-node/pull/4621 diff --git a/http/client/client.go b/http/client/client.go index 96428f853..040a9aa57 100644 --- a/http/client/client.go +++ b/http/client/client.go @@ -90,26 +90,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. @@ -125,34 +146,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) { @@ -167,7 +185,7 @@ func (s *StrictHTTPClient) Do(req *http.Request) (*http.Response, error) { if result.Body != nil { originalBody := result.Body defer originalBody.Close() - body, err := limitedReadAll(originalBody) + body, err := limitedReadAll(originalBody, s.maxResponseSize) if err != nil { return nil, err } diff --git a/http/client/client_test.go b/http/client/client_test.go index a4c5d1b36..96e1ff5bc 100644 --- a/http/client/client_test.go +++ b/http/client/client_test.go @@ -165,24 +165,57 @@ 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 TestStrictHTTPClient_ClosesResponseBody(t *testing.T) { oldStrictMode := StrictMode StrictMode = false t.Cleanup(func() { StrictMode = oldStrictMode }) + const limit = 10 server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { size, _ := strconv.Atoi(request.URL.Query().Get("size")) _, _ = writer.Write([]byte(strings.Repeat("a", size))) @@ -191,24 +224,24 @@ func TestStrictHTTPClient_ClosesResponseBody(t *testing.T) { t.Run("response exceeds limit", func(t *testing.T) { transport := &closeRecordingTransport{base: SafeHttpTransport} - client := &StrictHTTPClient{client: &http.Client{Transport: transport}} - request, _ := http.NewRequest(http.MethodGet, server.URL+"?size="+strconv.Itoa(DefaultMaxHttpResponseSize+1), nil) + client := newStrictHTTPClient(&http.Client{Transport: transport}, []Option{WithMaxResponseSize(limit)}) + request, _ := http.NewRequest(http.MethodGet, server.URL+"?size="+strconv.Itoa(limit+1), nil) _, err := client.Do(request) - assert.EqualError(t, err, "data to read exceeds max. safety limit of 1048576 bytes") + assert.EqualError(t, err, "data to read exceeds max. safety limit of 10 bytes") assert.Equal(t, int32(1), transport.closed.Load()) }) t.Run("response within limit", func(t *testing.T) { transport := &closeRecordingTransport{base: SafeHttpTransport} - client := &StrictHTTPClient{client: &http.Client{Transport: transport}} - request, _ := http.NewRequest(http.MethodGet, server.URL+"?size=10", nil) + client := newStrictHTTPClient(&http.Client{Transport: transport}, []Option{WithMaxResponseSize(limit)}) + request, _ := http.NewRequest(http.MethodGet, server.URL+"?size="+strconv.Itoa(limit), nil) response, err := client.Do(request) require.NoError(t, err) data, _ := io.ReadAll(response.Body) - assert.Len(t, data, 10) + assert.Len(t, data, limit) assert.Equal(t, int32(1), transport.closed.Load()) }) }