Skip to content

Discovery: cap presentation size on registration and raise the client response cap #4596

Description

@reinkrul

Context

Found during a security review of the discovery module. Two independent limits interact badly: the discovery client reads server responses through the generic outbound HTTP client, which refuses bodies over 1 MiB (http/client/client.go:92-101, in place since #3508), while the discovery server accepts individual registrations up to the generic 1 MB request-body limit (http/engine.go:51) and applies no size check of its own.

Problem

GET /discovery/{serviceID} returns every entry after the given timestamp in one response (sqlStore.get, discovery/store.go:243); there is no page size. On the client, DefaultHTTPClient.Get (discovery/api/server/client/http.go:74) buffers the whole body, json.Unmarshals it into a map[string]vc.VerifiablePresentation, and clientUpdater.updateService (discovery/client.go:416) stores nothing on error. A response over 1 MiB therefore fails the sync and it never advances.

Measured entry sizes:

Entry Size
Registration VP from the test helpers (JWT VC + registration VC) 1.8 KB
Production did:web VP with an embedded JSON-LD credential ~3.5 KB
VP with an X509Credential from go-didx509-toolkit (3-level G4 chain from #4589, RSA-4096) plus the registration VC 16.2 KB
Same with production PKIoverheid certificates (more extensions), estimate ~20 KB

The X509Credential VP is large because the chain is base64-encoded twice: once as x5c in the VC header (9.0 KB of the 10.7 KB credential), again when the VC JWT sits inside the VP payload.

Two ways to hit the 1 MiB cap:

Scenario Effect
A service list grows past roughly 300 did:web entries, or roughly 65 X509Credential entries A newly onboarded node, or one restored after a long outage, can never complete its initial sync. Nodes that synced earlier keep working, so this goes unnoticed until onboarding.
A participant registers two VPs of close to 1 MB (padding in the self-attested registration credential or extra JWT claims; verifyRegistration at discovery/module.go:230 does not look at size) Every client behind that timestamp stops syncing until the VPs expire (presentation_max_validity, 10 hours in the documented example). Re-registering keeps it going. Requires credentials that satisfy the service's presentation definition, so this is an insider or compromised-participant scenario.

Nodes that forward Get to a configured server use the same client and fail the same way.

Impact is availability only: a stalled client cannot discover parties it has not seen before. No trust or integrity impact.

Memory constraint

Raising the client cap alone does not scale, because the node is expected to run in 256 MB. Parsing a response into the presentation map costs several times the body size (measured with copies of the 16.2 KB X509Credential VP):

Response body Entries Heap growth from parse Transient total
10 MiB 648 40 MiB ~50 MiB
16 MiB 1,037 91 MiB ~107 MiB
32 MiB 2,074 143 MiB ~175 MiB

So with the current parse, 10 MiB is the largest defensible cap. Anything above needs the client to stop materialising the whole list.

Proposed change

1. Server: reject presentations over 64 KiB (hard cap, no configuration)

Add a len(presentation.Raw()) check at the top of Module.verifyRegistration, returned as ErrInvalidPresentation so the server answers 400. 64 KiB is 4x the measured X509Credential VP and about 3x the production estimate, leaving room for a fourth CA level or a second credential in the presentation definition. 32 KiB was considered and rejected as too tight for that case. verifyRegistration is also the verifier the client runs on downloaded entries, so clients ignore oversized entries from a misbehaving server for free.

2. Client: per-client response cap

Make the read limit a per-client setting on StrictHTTPClient (default stays 1 MiB) and pass a larger value from Module.Configure (discovery/module.go:109). The endpoint is operator-configured, so the SSRF, TLS and redirect checks stay as they are; only the size budget changes. The value depends on option A or B below.

3. Remove the parse multiplier

Option A, preferred: stream the response on the client, single pass.
The client decodes the response body directly from the network with a json.Decoder and processes each entry as it arrives, so peak memory is one presentation regardless of list size. Three parts:

  • Server: emit seed and timestamp before entries by reordering the fields of PresentationsResponse (discovery/api/server/client/types.go). Same wire format, old clients are order-agnostic.
  • Client: DefaultHTTPClient.Get walks the top-level object token by token. On seed it reports the seed so wipeOnSeedChange runs before any store; on each entries member it hands the entry to a callback; on timestamp it reports the server timestamp. Because timestamp now arrives after all entries, the service timestamp is updated once at the end, after everything is stored, which also closes the interrupted-sync gap noted under Considerations. Shape:
// HTTPClient
Get(ctx context.Context, serviceEndpointURL string, timestamp int, handler EntryHandler) error

type EntryHandler interface {
    Seed(seed string) error
    Entry(timestamp string, presentation vc.VerifiablePresentation) error
    Timestamp(serverTimestamp int) error
}
  • Compatibility: against an old server entries arrives before seed. The client detects that and falls back to collecting entries into the map under the current 10 MiB budget, then replays them through the handler once seed is known. Legacy path, removable in the next major.

StrictHTTPClient gets an option to return a size-limited streaming body instead of buffering it (io.LimitReader with the same over-limit error), used only by the discovery client. With streaming the cap bounds bytes and time rather than memory. Set it to 16 MiB: about 4,700 did:web entries, about 1,000 X509Credential entries, about 250 worst-case 64 KiB entries.

clientUpdater.updateService becomes the EntryHandler (exists check, store.add, verify, updateValidated per entry). The forwarding path in Module.Get (discovery/module.go:345) collects entries into the map it returns today; a forwarding node proxies the whole response, which is inherent to forwarding and bounded by the same cap.

Option B: paging on GET /discovery/{serviceID}.
The server returns at most N rows and reports the timestamp of the last row included instead of its latest timestamp; the client loops until the returned timestamp stops advancing. Backward compatible: an old client against a paged server fetches one page per refresh, which is slow but correct. A new client against an old server sees one full page. Lifts the ceiling entirely but is a protocol change on both sides and does not by itself reduce the per-response parse cost, so it still wants a bounded page size.

Both options are compatible. A alone is enough for the foreseeable sizes; B can follow if a service ever approaches the 16 MiB delta.

If neither option is done in the same PR, the client cap must stay at 10 MiB.

Scope

  • http/client/client.go: per-client max response size and a streaming-body option on the constructors
  • discovery/module.go: two constants, size check in verifyRegistration, larger cap in Configure
  • Option A: discovery/api/server/client/types.go (field order), http.go and interface.go (Get with EntryHandler, legacy fallback), discovery/client.go (updateService as handler), discovery/module.go (Get forwarding collects into a map), mock regeneration
  • Tests for all of the above; docs/pages/deployment/discovery.rst note on the 64 KiB limit; release notes

Considerations

  • sqlStore.add currently sets the service's last timestamp to the server's latest timestamp on the first stored entry, so an interrupted sync already records itself as complete. Streaming does not make this worse, but per-entry storing makes it easy to record the entry's own timestamp instead and update the service timestamp once at the end. Worth deciding in the PR.
  • Go marshals the entries map in string-sorted key order ("10" before "2"), so the client must not assume numeric order when streaming.

Out of scope

  • A per-definition presentation_max_size. Decided against: a node-wide constant is enough.

Backport

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Fields

    Priority

    None yet

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions