Judge the advertised OAuth issuer's address, not just its spelling

Merged
JE
2mo ago
+845-28

Closes one real SSRF exposure in the Go OAuth discovery path, at one call site.

The exposure

DiscoverFromResource hop 2 dials an origin lifted out of a parsed HTTP response body. authorization_servers[] is chosen by the remote resource, and without WithExpectedIssuer the "single non-Launchpad entry wins" heuristic makes that string the socket destination.

requireOriginRoot is a syntax gate — it has no notion of what a host resolves to. Issuer binding runs only after the response comes back, which is after the connection has already reported whether an internal host and port are live. SPEC.md §16 already calls advertised AS URLs untrusted input (RFC 9728 §7.7).

The discovery fetch itself is bounded: fixed path, GET, no credentials, near-blind. But it is a working internal host/port oracle, and it is not where the selected issuer stops being used. The Config it returns carries TokenEndpoint (required) and DeviceAuthorizationEndpoint, and those become the destinations of the grant itself — PerformDeviceLogin takes exactly this already-selected *Config and posts the client_id and device_code to them; exchange and refresh post the authorization code, client secret, or refresh token to TokenEndpoint.

Those are form-body credentials, not an Authorization: Bearer header. An earlier revision of this description claimed the selected issuer is handed to calls that attach a Bearer token. That was an overclaim and it is retracted. Tracing it: the only two Bearer sites in the Go SDK are BearerAuth.Authenticate (auth_strategy.go:31), which authenticates the generated API client's requests to the operator-configured cfg.BaseURL, and AuthorizationService.GetInfo (authorization.go:197), which targets a hardcoded Launchpad URL or a caller-supplied opts.Endpoint. Neither is reachable from a discovered issuer. Nothing in this repo constructs a Credentials{TokenEndpoint: ...} or references DiscoveryResult outside the oauth package at all — the discovery-to-credential-store wiring lives in consumers such as basecamp-cli, so a Bearer claim is not demonstrable here and is not made.

Demonstrated, not asserted. Reverting hop 2 to the unpoliced client makes the new tests fail like this:

--- FAIL: TestDiscoverFromResource_BlockedIssuerIsNeverContacted
    expected the advertised issuer to be refused, got nil error
--- FAIL: TestDiscoverFromResource_BlockedIssuerNamedByHostname
    the blocked issuer was contacted 1 time(s); the policy must refuse before any connect
--- FAIL: TestDiscoverFromResource_RefusesLegacyNumericIssuer/https
    err = ... Get "https://2130706433/.well-known/oauth-authorization-server": dial tcp 127.0.0.1:443: connect: connection refused

That third line is the whole problem in one string: a well-formed https origin root, admitted by every syntax check, dialing loopback.

The change

One hop moves onto a client whose transport judges the literal address of every connect attempt, via surfguard's IANASpecialUse policy — the one its docs name for "advertised or discovered infrastructure values".

func DefaultIssuerPolicy() surfguard.Policy   // IANASpecialUse().AllowAllPorts()
func NewDiscoverer(httpClient *http.Client, opts ...DiscovererOption) *Discoverer
// WithIssuerPolicy / WithIssuerHTTPClient / WithoutIssuerPolicy

Scope is deliberately narrow. Hop 1, Discover, and DiscoverLaunchpad all target an origin the caller named; they keep the caller's client and still reach loopback. Three other candidate sites were checked and left alone — see below.

A refusal maps onto the existing ErrInvalidIssuerOrigin rather than a sixth sentinel: requireOriginRoot judged the spelling, the policy judged what it resolves to, and the consumer's remedy is identical. It is re-coded non-retryable — the dial failure is classified as a retryable network error deep in the fetch, before anything knows why it failed, and SelectionError inherits its cause's retryability, so without this callers would be told to retry a target they must stop talking to.

Decisions worth flagging

The policy applies uniformly, WithExpectedIssuer included. Strictly that path is caller-determined and could be exempt. It isn't, on purpose: "the policy applies sometimes" is the branch a bypass grows out of, and the exemption is silently wrong for a consumer that computed its expected issuer from untrusted input. Pinned by TestDiscoverFromResource_ExpectedIssuerIsPoliciedToo.

This is a behavioral tightening, not a pure addition. A consumer whose BC5 issuer is advertised on loopback or in RFC 1918 space, and which worked before, now needs WithIssuerPolicy. That is the point of the change, but it is a break and it is recorded as one in Appendix F.

Allow(prefix) does NOT re-admit RFC 1918 under IANASpecialUse(). This one contradicts the plan this PR was written from, which said an internal deployment "fixes itself with one Allow(prefix)". It does not: surfguard's precedence puts the special-use tables above Allow, and AllowLoopback() is the only derivation that pierces them. Verified directly:

10.4.1.2   default=true  Allow(10.4.0.0/16)=true  AllowLoopback=true
127.0.0.1  default=true  Allow(10.4.0.0/16)=true  AllowLoopback=false

The correct on-premises spelling is surfguard.Policy{}.AllowAllPorts().Allow(...) — dropping IANASpecialUse, keeping the default deny tables. The docs say so, and TestWithIssuerPolicy_AllowUnderSpecialUseDoesNotReadmitPrivateSpace fails if either half of that claim stops being true.

AllowAllPorts — the origin-root profile legitimately admits an explicit port, and address classification is what closes the exposure. A {80, 443} list would refuse honest issuers without narrowing which hosts are reachable.

ErrInvalidIssuerOrigin now means two things. Mapping ErrBlocked onto the existing sentinel avoids a sixth public error, but it widens what that sentinel signals for every consumer — so its doc comment now spells out both checks, notes that an address refusal also matches errors.Is(err, surfguard.ErrBlocked) for anyone who needs to tell them apart, and warns that code logging "malformed issuer URL" on it should widen that wording.

The default issuer client is shared. It owns a transport, and a Discoverer has no Close, so building one per construction would leak a connection pool nothing can reclaim. sync.OnceValue builds it once per process. Sharing is legal only because nothing mutates it — noRedirectClient copies before setting CheckRedirect — and only the default is shared: WithIssuerPolicy gets its own transport, or one caller's Allow would silently widen every other Discoverer in the process. Three tests pin this, each mutation-verified: shared identity for defaults, a distinct client under WithIssuerPolicy, the shared client still unmutated after a real discovery, and a behavioral isolation check that a permissive Discoverer does not widen defaults built before or after it.

API break. NewDiscoverer gains a variadic parameter. Source-compatible for every direct call, but apidiff will report it as incompatible, so this needs the breaking label.

The existing security assertion stayed live

resource_discovery_test.go wraps a countingTransport to assert launchpadContacted: false. Hop 2 moving to a client built inside NewDiscoverer would have taken it off that transport's path — the assertion would have kept passing while no longer watching the hop that could break it.

Verified rather than reasoned about. Temporarily pointing hop 2 at Launchpad, with the harness fixed:

--- FAIL: .../07-issuer-binding-mismatch.json
    Launchpad was contacted 1 time(s); hard/selected case must not touch Launchpad

And the same mutation with the harness left on the default hop-2 client:

ok — 07-issuer-binding-mismatch.json PASSED

That is the vacuous pass, reproduced. The harness now builds the hop-2 client explicitly, keeping the surfguard round tripper underneath so the policy is still live on that lane with only loopback re-admitted.

Scope: three sites checked and left alone

  • download.go — hop 1 rewrites the URL to keep only path and query, pinning the host to cfg.BaseURL. Hop 2 follows a Location chosen by that same operator-configured API host. Gating it would break on-prem object stores for no gain. (One correction to the brief: it is hop 1 that does the rewriting; fetchSignedDownload takes the Location as given. The conclusion holds — the Location's author is the trusted API host — but see the filed issue.)
  • eventfeed — the cable URL is minted by the trusted, already-token-holding API host, and ws://localhost is a specified §9 carve-out. (Not present on main — it lives on the unmerged event-feed-go-connector branch.)
  • main API client — URL is operator-configured.

Coordination

SPEC.md §1658's SSRF block enumerates hardening for five SDKs; this is a Go-only divergence, so it is not merged silently.

COORDINATION.md is exclusively about SDK ↔ BC3 API parity — the wrong home. The repo's convention for cross-SDK language divergences is SPEC.md plus Appendix F: Known Cross-SDK Divergences, so it is raised there as a spec requirement:

  • §16 gains requirement 5, marked [Go-first] and written SHOULD. Deliberately not [conformance]: the assertion is "no connection was attempted", which the mock-HTTP runner cannot observe, and the other four SDKs have no equivalent enforcement layer to point at yet.
  • Appendix F records the per-SDK state, the behavioral tightening, and the Allow/IANASpecialUse precedence trap.

Verification

  • go test ./..., go vet ./..., gofmt -s -l, golangci-lint — all clean in the go module; conformance/runner/go and scripts/check-wrapper-drift still build and pass; go.mod/go.sum tidy.
  • same_origin_test.go and download_test.go pass unedited (TestBuildURL_RejectsForeignOriginAbsoluteURL and all 23 TestDownload*).
  • Every blocked-issuer test asserts the target handler ran zero times — an error alone would not prove the issuer was never contacted, and "the internal host answered, then binding rejected it" is precisely the outcome being prevented. The mock serves usable metadata, so a policy that failed to block yields a successful discovery rather than a differently-worded failure.
  • Both new guarantees mutation-tested: removing the policy, and removing the retryability re-coding, each make the intended assertion fail.

Scope of the claim, and the residual it leaves

This PR closes the advertised-issuer metadata GET. It does not close the discovery SSRF surface in general, and the distinction matters enough to state rather than leave to inference.

Still open — #806. An attacker-controlled issuer on public space passes this policy, is selected, and returns issuer-bound metadata whose token_endpoint / device_authorization_endpoint point anywhere. Those endpoints are neither origin-constrained (discovery.go:545 checks only non-empty; :558 copies verbatim) nor address-judged, and the credential-bearing POSTs go through http.DefaultClient (device.go:137, exchange.go:25). So the indirect route reaches private space at the cost of one public host — and delivers real credentials rather than a blind GET.

Pre-existing, and unchanged by this PR. Not fixed here because every SDK has device and exchange flows, so a Go-only patch would close one spelling and leave four open — the same cross-SDK shape as #805. Whether to constrain endpoints to the issuer origin, address-police the endpoint requests, or both is a specification decision.

Also open — #805. Download hop 2 follows redirects in four SDKs.

Read this PR as "the metadata hop is now policed", not "discovery is SSRF-safe".