Police the endpoints a discovered issuer names, not only the issuer

Merged
JE
2mo ago
+788-45

Closes #806.

The exposure

#804 judges the advertised issuer's address before the metadata GET. That closes the direct route and leaves the indirect one open: an attacker-controlled issuer on public space passes the policy, is selected, and returns correctly issuer-bound metadata whose token_endpoint and device_authorization_endpoint point wherever it likes. parseAndBindASMetadata checks only that token_endpoint is non-empty and copies it verbatim; RequireSecureEndpoint admits any https host, private space included. The credential POSTs then went out on http.DefaultClient — client_id and device_code from the device flow, the authorization code, client secret, and refresh token from the Exchanger. One public host buys private space, and what arrives is credentials rather than a blind GET.

The decision

Dial-time address enforcement on every device and token endpoint request, on the same shared DefaultIssuerPolicy() client as the issuer hop. Not same-origin, and the reasons are written into SPEC §16 so they are not re-litigated: RFC 8414 §2 does not require endpoints on the issuer origin, RFC 8705 §5 publishes off-origin endpoints by design, and an origin comparison judges a hostname string that is resolved twice — publicly at discovery, privately at the POST, under a rebind. Scheme alone is satisfied by any private host.

The device and exchange functions cannot tell a discovered endpoint from a hand-configured one (PerformDeviceLogin takes a *Config; Exchange/Refresh take a string), so the policy applies to every request on the default client. That is the same uniformity call as #804 made for WithExpectedIssuer, for the same reason; a provenance flag on Config is a marker that a consumer round-tripping the config through storage silently drops.

The change

func WithDevicePolicy(p surfguard.Policy) DeviceOption          // new
func NewExchanger(httpClient *http.Client, opts ...ExchangerOption) *Exchanger  // variadic added
func WithExchangerPolicy(p surfguard.Policy) ExchangerOption    // new

Precedence is one rule across all three surfaces of the package: a client you hand in is yours, enforcement included; otherwise your policy; otherwise the shared default. http.DefaultClient is the spelling of "no policy", so no Without* options were added for these two surfaces — there is nothing they would express that passing a plain client does not.

A refusal is a non-retryable *basecamp.Error (api_error) that also matches errors.Is(err, surfguard.ErrBlocked). In the poll loop it is classified ahead of the timeout and transport cases, so the loop neither backs off and re-dials a target it must stop talking to nor reports it retryable.

The shared-client invariant from #804 extends to the new surfaces: WithDevicePolicy builds its transport once at option construction (PerformDeviceLogin hands the same opts to the auth request and the poll, so a transport per newDeviceConfig would be two pools per login), WithExchangerPolicy builds one per Exchanger, and suppressRedirects keeps copying so the shared default is never mutated.

Demonstrated, not asserted

Seven mutations, each run against the new test file, each caught:

MutationTests that fail
device default back to http.DefaultClientPerformDeviceLogin_BlockedEndpointIsNeverContacted, PollDeviceToken_BlockedTokenEndpointTerminates, RefusesLegacyNumericSpelling, DefaultPolicyClientIsSharedAcrossSurfaces, EndpointPolicyIsolation
exchanger default back to http.DefaultClientExchanger_BlockedTokenEndpointIsNeverContacted, RefusesLegacyNumericSpelling, DefaultPolicyClientIsSharedAcrossSurfaces
poll loop stops classifying ErrBlocked (falls to timeout/transport)PollDeviceToken_BlockedTokenEndpointTerminates
device-auth stops classifying ErrBlocked (falls to transport)PerformDeviceLogin_Blocked…, RefusesLegacyNumericSpelling, EndpointPolicyIsolation
exchange ErrBlocked branch made deadExchanger_Blocked…, RefusesLegacyNumericSpelling
WithDevicePolicy builds a transport per callDefaultPolicyClientIsSharedAcrossSurfaces
refusal marked Retryable: trueall five blocked-endpoint tests

Every blocked case asserts the endpoint's handler ran zero times, and every mock serves a usable response, so a policy that failed to block yields a successful login or exchange rather than a differently-worded failure. The poll test's injected clock advances by each sleep, so a loop that wrongly kept polling runs out to expiry with a recognizably wrong reason instead of hanging.

go test ./..., go vet, gofmt -s, golangci-lint clean; the conformance runner still builds; make doc-constants-check and make go-check-drift pass. The 74 existing device tests and 10 exchange tests pass unedited — every one that reaches the wire already injected srv.Client(), and the five that do not all fail before the request.

What this does and does not close

basecamp-cli is not protected by this change. It passes its general-purpose m.httpClient to NewExchanger, WithDeviceHTTPClient, and NewDiscoverer alike, and a caller-supplied client is the caller's — the enforcement seam is the transport's dialer, so there is nothing to layer on top of an arbitrary RoundTripper. That is the #804 WithIssuerHTTPClient contract applied consistently; the alternative (clone an *http.Transport and overwrite its DialContext/Proxy) is "the policy applies sometimes" in another form. The consumer-side fix is one line, &http.Client{Transport: oauth.DefaultIssuerPolicy().RoundTripper()} for these three call sites, and it is recorded in MIGRATING and Appendix F rather than hidden. The SDK default is now safe; the CLI needs its own PR.

Still open, recorded in Appendix F:

  • The other four SDKs. Only Go and Ruby have surfguard, and the Ruby gem is classification-only by design (the caller pins the resolved address), so Ruby's enforcement would be a Net::HTTP#ipaddr= pin under the default Fetcher transport — not reachable from the injected-Faraday lane. TS, Python, and Kotlin have no classification tables at all; the seams (undici connector lookup, a resolving httpcore backend, OkHttp Dns) are named so the follow-ups are specified rather than rediscovered. Same cross-SDK shape as #805.
  • Exchange redirect-following is not uniform: Go's Exchanger, Kotlin's default client, and TS's exchange (no redirect: option) follow redirects, where the device flow suppresses them in all four. Under the policy client each redirect hop's dial is judged, so the address policy holds; a 307 re-POST of the secret to a public Location does not need the policy to be exploitable. Same shape as #805, so noted rather than patched here.
  • AuthManager.refreshLocked posts a refresh token to Credentials.TokenEndpoint on the main API client. That endpoint is consumer-stored; if the consumer stored a discovered one, the same reasoning applies at the consumer's boundary.

API break

NewExchanger gains a variadic parameter. Source-compatible for every direct call; a function value of the old type stops compiling, and apidiff reports it incompatible — hence breaking, as #804 was for NewDiscoverer.

Summary by cubic

Polices the addresses of device-authorization and token endpoints, not just the issuer metadata GET, closing the indirect SSRF path where a public issuer could steer credentials into private space.

  • Device flow (PerformDeviceLogin, RequestDeviceAuthorization, PollDeviceToken) and Exchanger POST via a shared client that enforces oauth.DefaultIssuerPolicy() (same as discovery). Device-flow redirects remain suppressed.
  • Precedence: caller-provided HTTP client is used as-is; else a provided policy; else the shared default. Passing http.DefaultClient restores prior behavior.
  • New options: WithDevicePolicy, WithDeviceHTTPClient, WithExchangerPolicy. NewExchanger now accepts variadic options.
  • Refusals surface as non-retryable api_error matching surfguard.ErrBlocked; the poll loop terminates on the first attempt.
  • Scope: this default enforcement applies to the device-flow and Exchanger POSTs only. AuthManager.refreshLocked posts to a stored TokenEndpoint on the caller’s client and remains outside this default policy. Caller-supplied clients are never wrapped; compose the policy into their transport if desired.

Migration

  • If you used defaults with loopback/private AS endpoints, add WithDevicePolicy(oauth.DefaultIssuerPolicy().AllowLoopback()) or WithExchangerPolicy(...), or supply your own client. Otherwise these requests are now refused.
  • Callers already passing their own client are unchanged and not protected; to enable enforcement, build it with &http.Client{Transport: oauth.DefaultIssuerPolicy().RoundTripper()}.
  • If you used NewExchanger as a function value, update to its new type with ...oauth.ExchangerOption. Direct calls remain source-compatible.