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
No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions