Repository navigation
Conversation
A remote JSON-LD context fetch had no timeout. In non-strict mode, a context server that never answered blocked network synchronization at the first credential referring to it. Fetches now go through the node's strict HTTP client with a 5 second timeout, and a failed URL is not fetched again for 5 minutes. Fixes #4615 Assisted by AI
Assisted by AI
0 new issues
|
| // json-gold needs a *http.Client, so the node's strict HTTP client (timeout, SSRF guard, URL checks, response size limit) | ||
| // is wrapped as its transport. | ||
| func newRemoteContextHTTPClient() *http.Client { | ||
| return &http.Client{Transport: strictClientTransport{client: client.New(remoteContextTimeout)}} |
There was a problem hiding this comment.
This nests two http.Clients: the outer one json-gold needs, and the one inside
StrictHTTPClient. All the real behaviour lives in the inner one. It applies the
5s timeout, follows redirects with checkRedirect, and buffers the body before
RoundTrip returns, so the outer client only ever sees a final non-redirect
response. Its Timeout and CheckRedirect are therefore unused on purpose.
Worth stating that in the comment, because the next reader will see
&http.Client{Transport: ...} with no Timeout and add one, and then there are
two timeouts that disagree. Suggested wording:
// The outer client's Timeout and CheckRedirect are intentionally unset:
// the strict client inside the transport owns both, follows redirects
// itself, and has read the body by the time RoundTrip returns.
Also note that this RoundTrip bends the http.RoundTripper contract (it follows
redirects and returns errors for oversized bodies), so it should stay private
to this package rather than become a general adapter. If we want a real
*http.Client with the strict protections, that belongs in http/client as a
per-hop transport, and pki/validator.go (CRL fetching on http.DefaultTransport,
no SSRF guard, no body limit) would be the second consumer.
| type failureCachingLoader struct { | ||
| ttl time.Duration | ||
| nextLoader ld.DocumentLoader | ||
| mutex sync.Mutex |
There was a problem hiding this comment.
This type copies much of the already used patrickmn/go-cache which also has a TTL option. I propose to used that one here as well instead of rolling our own logic for that.
This branch has not been deployed
2 participants
Fixes #4615
Problem
Remote JSON-LD contexts were fetched with
http.DefaultClient, which has no response timeout. Credentials are verified while network transactions are processed, so in non-strict mode a context server that never answers stopped network synchronization at the first credential referring to it. Nothing was logged. Failed fetches were also not remembered, so a timeout alone would still cost one timeout per credential.Changes
jsonld/ldutils.goclient.New(5s), the node's strict HTTP client, wrapped as the transport of the*http.Clientjson-gold needs. This gives the timeout, the SSRF dial guard, the strict-mode URL checks on every redirect, the 1 MiB response limit, and the node's own User-Agent.failureCachingLoaderdirectly in front of json-gold's remote loader. A URL that failed is not fetched again for 5 minutes; loads return the original*ld.JsonLdErrorimmediately. The VCR ambassador therefore still treats it as recoverable and retries the event later. One warning per failure, debug for skipped fetches. Expired entries are pruned when a new failure is recorded.NewContextLoader's signature is unchanged.Behavior changes beyond the fix
http://URL or a reserved hostname is now rejected by the strict client's URL checks. The defaults are embedded, so this only affects custom allow-list entries.Go-http-client/1.1tonuts-node-refimpl/<version>.Testing
LoadingDocumentFailedafter the timeout.failureCachingLoaderunit tests: expiry, successes not remembered, per-URL, pruning.go test -race -count=3 ./jsonld/andgo test ./vcr/... ./auth/... ./discovery/... ./vdr/...pass.Backport
V6.2 and V5.4, as listed in #4615. To be prepared once this is reviewed.
Assisted by AI