Skip to content

Outbound HTTP client layout: strict protections are not available as an *http.Client #4619

Description

@stevenvegt

Problem

All strict-mode protections for outbound HTTP live on http/client.StrictHTTPClient, which applies them per request in Do: the public-URL check (core.ParsePublicURLAllowIP), the User-Agent header, and the 1 MiB response body limit. The timeout and redirect policy sit on the *http.Client inside it, and the SSRF dial guard and TLS 1.2 floor sit on SafeHttpTransport below that. StrictHTTPClient is not an *http.Client and does not expose the one it wraps.

That is fine for code we write ourselves (auth, discovery, vcr, vdr/didweb all call client.New* and use Do). It is a problem for every library or generated client that insists on a *http.Client:

  • jsonld/ldutils.go: json-gold's DefaultDocumentLoader takes a *http.Client. Until JSON-LD: time out remote context fetches and remember failures #4617 it got nil, meaning http.DefaultClient: no timeout, no SSRF guard, no body limit. JSON-LD: time out remote context fetches and remember failures #4617 fixes it by wrapping the whole StrictHTTPClient as an http.RoundTripper inside a second, outer http.Client. This works, but the outer client's Timeout and CheckRedirect are dead config, and the adapter bends the RoundTripper contract (it follows redirects and buffers the body inside RoundTrip).
  • pki/validator.go:104: CRL fetching builds a raw http.Client on http.DefaultTransport. CRL distribution points come from certificates we did not issue, so this is an outbound fetch to URLs we do not control, with no SSRF guard, no body limit and no strict-mode URL check. Only the timeout is set.
  • crypto/storage/external/client.go:96: raw http.Client on http.DefaultTransport. This one targets an operator-configured internal address, so the strict client would be wrong for it, but it also means there is no single place that says "this is the client for trusted internal targets".
  • core/http_client.go:131 (CreateHTTPInternalClient): raw http.Client, used for CLI-to-node calls. Same category as the previous one.

The comment on SafeHttpTransport already warns not to build raw clients on it, but the package offers no alternative when a *http.Client is required, so people either bypass the protections or write an adapter like the one in #4617.

Proposal

Move the per-request protections from StrictHTTPClient.Do into an http.RoundTripper in http/client, so they apply per hop and a real *http.Client can be built on it:

  • A strictTransport that wraps SafeHttpTransport (or the caching or custom-TLS variant) and does, per hop: the public-URL check, set User-Agent, and limit the response body. Redirect targets are then checked by the transport on every hop as well, which makes the current checkRedirect re-check redundant but harmless.
  • client.New* keep returning *StrictHTTPClient so the existing call sites do not change. StrictHTTPClient.Do becomes a thin call to the inner client.
  • A new constructor, for example client.NewHTTPClient(timeout, options...) *http.Client, that returns the inner client directly for libraries that need one. Timeout and CheckRedirect are set on that client, so there is one place to configure them.
  • jsonld drops its private adapter and uses NewHTTPClient. pki switches CRL fetching to it (with a response size option sized for CRLs; some are several MB).
  • Document in http/client which constructor to use for which target class: public internet (strict), operator-configured internal services (no SSRF guard, but still timeout and body limit), and CLI-to-node.

Response limit semantics

The response limit is a buffer-and-error, not a streaming cap: Do reads up to limit+1 bytes into memory and fails the request on overflow. It never truncates, because a silently shortened body (DER, a JSON-LD context) can parse into something smaller than intended. The transport must keep that: RoundTrip reads the body, errors on overflow, and returns an in-memory body on success. http.Client.Do then surfaces the error exactly as StrictHTTPClient.Do does today, so callers never see a response object on overflow.

Two consequences: http.Client wraps transport errors in *url.Error, so tests comparing error strings change (errors.Is/errors.As keep working), and redirect hops are limited as well, which is stricter than today. A lazy limiting body would allow streaming but moves the error from Do to the first read, which is a behaviour change for every existing caller, so it is out of scope here.

Non-goals

  • Changing behaviour for the existing client.New* callers.
  • Touching the generated OpenAPI clients; they take an HttpRequestDoer interface and already accept StrictHTTPClient.
  • Unifying the two internal-target clients (crypto/storage/external, core.CreateHTTPInternalClient); documenting which constructor they should use is enough.

Open questions

  • Tracing: getTransport wraps in otelhttp today. The strict transport should sit inside the otel wrapper so a rejected URL still produces a span, or outside so it does not; decide which.

Related

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

    No labels
    No 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