Skip to content

Centralise outbound URL validation in StrictHTTPClient #4244

@JorisHeadease

Description

@JorisHeadease

Background

PR #4057 introduced a new auth/openid4vci.Client that validates outbound URLs via core.ParsePublicURL to address a CodeQL SSRF finding (see thread). Rein noted the validation conceptually belongs on the shared HTTP transport so every outbound call benefits, not just OpenID4VCI.

Goal

Centralise URL validation in http/client.StrictHTTPClient.Do so all outbound HTTP requests are checked against core.ParsePublicURL (HTTPS-only, no IP, no RFC 2606 reserved hosts when httpclient.StrictMode is true). Remove the per-caller validation that duplicates this today.

Current state

Existing callers of StrictHTTPClient and what they validate today:

Caller Today After centralisation
auth/openid4vci.Client.validateURL Full core.ParsePublicURL on every call Remove (dedupe). Drop strictMode from NewClient.
auth/client/iam.HTTPClient metadata methods (OAuthAuthorizationServerMetadata, ClientMetadata, OpenIDConfiguration) Validate via oauth.IssuerIdToWellKnowncore.ParsePublicURL Could keep belt-and-suspenders or remove.
auth/client/iam.HTTPClient lower-level methods (RequestObjectByGet/Post, AccessToken, PresentationDefinition, redirect posts) None at this layer; rely on OpenID4VPClient callers to validate first Inherit consistent validation.
auth/services/oauth/relying_party.RequestRFC003AccessToken Scheme-only check (strictMode && Scheme != https) Inherits IP/reserved blocking.
discovery/api/server/client/http.go, discovery/module.go None Inherit.
vcr/openid4vci/identifiers.go, vcr/vcr.go (issuer/wallet/StatusList) None Inherit.
vdr/didweb.Resolver None (constructs HTTPS URLs from did:web by convention) Inherit (no functional impact expected).

Implementation sketch

// http/client/client.go
func (s *StrictHTTPClient) Do(req *http.Request) (*http.Response, error) {
    if _, err := core.ParsePublicURL(req.URL.String(), StrictMode); err != nil {
        return nil, fmt.Errorf("httpclient: invalid target URL: %w", err)
    }
    req.Header.Set("User-Agent", core.UserAgent())
    // ... rest unchanged
}

Redirects are followed by http.Client up to 10 times without re-running Do, so the initial-URL check alone leaves a redirect-based SSRF gap (302 → 169.254.169.254/...). Set Client.CheckRedirect to re-run core.ParsePublicURL on each redirect target.

Plus follow-up cleanup in auth/openid4vci.Client (drop strictMode + validateURL), the relying-party scheme check, and the oauth.IssuerIdToWellKnown callers if dedupe is preferred.

Compatibility notes

  • Non-strict mode: core.ParsePublicURL(_, false) calls ParsePublicURLWithScheme(input, true, "http", "https"), so it still allows IPs and reserved TLDs but rejects non-http(s) schemes. Grep confirms no file:///ftp:// outbound calls exist, so this is a no-op in practice.
  • Tests: most tests use httptest.NewServer (127.0.0.1) under StrictMode=false. With allowReserved=true in non-strict mode IPs are accepted, so existing tests should stay green. Verify per-package after the change.
  • Strict-mode tightening: today strict mode only requires HTTPS; after this change it also rejects IP literals and RFC 2606 reserved TLDs. Whether any production config (RFC003 relying party, Discovery, VCR node-to-node) relies on such hosts is best raised on the PR.

TODO marker in code

auth/openid4vci/client.go:validateURL has a TODO referencing this work.

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions