Add authserver DCR credential store and resolver

[tgrunnagle]

Summary

Upstream Identity Providers that support RFC 7591 Dynamic Client Registration can now be configured without a pre-provisioned client_id. At startup the authserver discovers the registration endpoint, registers itself once per (issuer, redirect_uri, scopes) tuple, and caches the resulting credentials for the process lifetime. This unblocks the remaining wiring + observability work in sub-issue C and removes the last manual provisioning step for DCR-capable upstreams (e.g., nanobot, Hydra).

• WHY: Operators running the embedded authserver against a DCR-capable upstream were forced to pre-register a client manually and hand-roll the client_id/client_secret into the run config. Phase 2 of the DCR story (Authserver DCR integration (Phase 2, Steps 2a-2g) #4978) lets the authserver do this at startup, keyed canonically so a future Redis backend (Phase 3) can share the cache across replicas without redefining the key shape.
• WHAT:
  • DCRCredentialStore interface + in-memory implementation keyed by (Issuer, RedirectURI, ScopesHash). ScopesHash is SHA-256 of the deduplicated + sorted scope list, so permuted or duplicated scope inputs share a digest. No TTL, no eviction — RFC 7591 registrations are long-lived and are only revoked by explicit RFC 7592 deregistration.
  • resolveDCRCredentials performs (1) cache lookup (short-circuits before any network I/O on hit), (2) authorization-server metadata discovery via FetchAuthorizationServerMetadata when DiscoveryURL is set, or the operator-supplied RegistrationEndpoint directly, (3) explicit rc.AuthorizationEndpoint / rc.TokenEndpoint override of discovered values, (4) redirect-URI derivation from the issuer origin + /oauth/callback with HTTPS enforcement outside loopback, (5) auth-method intersection against the preference order private_key_jwt > client_secret_basic > client_secret_post > none, (6) synthesis of {origin}/register when metadata omits registration_endpoint (nanobot/Hydra convention), (7) a single call to oauthproto.RegisterClientDynamically with the initial access token injected as Authorization: Bearer via a wrapping RoundTripper, and (8) DCRCredentialStore.Put before returning — per the write-durable-first rule in .claude/rules/go-style.md.
  • Full RFC 7591 + RFC 7592 response is captured on the DCRResolution (RegistrationAccessToken, RegistrationClientURI, TokenEndpointAuthMethod, CreatedAt) so Phase 3 has everything it needs for management operations.
  • DCRUpstreamConfig.DiscoveryURL is honoured exactly (no well-known-path fallback) via the new oauthproto.FetchAuthorizationServerMetadataFromURL, with the RFC 8414 §3.3 issuer-equality check preserved.

Closes #5038

Type of change

• New feature

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

New test coverage (pkg/authserver/runner/dcr_store_test.go, pkg/authserver/runner/dcr_test.go):

• Put/Get round-trip; distinct DCRKey values don't collide; ScopesHash is stable across permuted and duplicated scope order.
• Resolver end-to-end flow against an httptest.NewServer mounting AS metadata + DCR endpoint.
• Cache-hit short-circuit: store pre-populated, then assert the counting server receives zero requests. The server is wired as the issuer so a regression that moved discovery before the cache lookup would fail.
• Explicit AuthorizationEndpoint overrides discovered value.
• Initial access token forwarded as Authorization: Bearer ….
• client_secret_basic chosen over none when both are advertised.
• Metadata missing registration_endpoint triggers the synthesized /register path.
• Discovered-scopes fallback and empty-scope omitted-field branches.
• grep -nE '(client_secret|registration_access_token|initial_access_token|refresh_token)' pkg/authserver/runner/dcr.go returns only struct-field / JSON-tag hits — no slog.* leakage.

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

No CRD changes. The new DCRConfig field on OAuth2UpstreamRunConfig is additive and gated by an exactly-one-of validator against ClientID.

Changes

File	Change
pkg/authserver/runner/dcr_store.go	New DCRCredentialStore interface, DCRKey, in-memory implementation, dcrStaleAgeThreshold constant.
pkg/authserver/runner/dcr_store_test.go	Round-trip, collision, and scope-hash canonicalisation tests.
pkg/authserver/runner/dcr.go	DCRResolution, needsDCR, applyResolution, scopesHash, auth-method intersection, resolveDCRCredentials.
pkg/authserver/runner/dcr_test.go	Resolver flow, cache short-circuit, endpoint overrides, bearer token, auth-method selection, synthesized /register.
pkg/authserver/config.go	DCRUpstreamConfig + DCRConfig field on OAuth2UpstreamRunConfig; Validate enforces exactly-one-of for ClientID vs DCRConfig and DiscoveryURL vs RegistrationEndpoint.
pkg/authserver/config_test.go	Validation table tests.
pkg/oauthproto/discovery.go	FetchAuthorizationServerMetadata (three-path RFC 8414 / OIDC Discovery fallback, bounded per-call timeout, issuer-equality check) and FetchAuthorizationServerMetadataFromURL (exact fetch, no fallback). ErrRegistrationEndpointMissing sentinel.
pkg/oauthproto/discovery_test.go	Three-path fallback, issuer-equality, loopback HTTP carve-out, timeout-overrides-caller-context, invalid issuer rejection, body-cap verification.
pkg/oauthproto/dcr.go	Moved from pkg/auth/oauth/dynamic_registration.go; RegisterClientDynamically now takes *http.Client directly (nil builds default); NewDefaultDCRClient exposed so callers inherit the canonical timeout policy.
pkg/oauthproto/{constants,errors,redirect,locality}.go	Moved from pkg/oauth; IsLoopbackHost relocated here with pkg/networking.IsLocalhost kept as a thin wrapper.
pkg/oauth/*	Deleted — orphaned after rename.
17 import sites	Updated to pkg/oauthproto; 8 redundant aliases dropped.

Does this introduce a user-facing change?

Yes. Operators can now omit client_id on an OAuth2UpstreamRunConfig and instead set dcr_config with either a discovery_url or a direct registration_endpoint. The authserver will register itself with the upstream at startup and cache the result in memory for the process lifetime. The existing pre-provisioned-client_id path is unchanged.

Implementation plan

Approved implementation plan

Phase 2 of the DCR story (#4978), delivered across four commits on this branch:

1. Phase 1 (3cb8de58) — Rename pkg/oauth → pkg/oauthproto and consolidate DCR primitives into pkg/oauthproto/dcr.go. Relocate IsLocalhost logic into pkg/oauthproto/IsLoopbackHost, keep pkg/networking.IsLocalhost as a thin wrapper. RegisterClientDynamically takes *http.Client directly. Host NewDynamicClientRegistrationRequest in pkg/auth/discovery/dcr_request.go (sole caller; loopback redirect-URI assumption stays out of the protocol package).

2. Step 2a/2b/2e (d578dfee) — Add FetchAuthorizationServerMetadata to pkg/oauthproto (three-path RFC 8414 / OIDC Discovery fallback, bounded per-call timeout, RFC 8414 §3.3 issuer equality check, ErrRegistrationEndpointMissing sentinel). Add DCRUpstreamConfig struct and DCRConfig field on OAuth2UpstreamRunConfig. Exactly-one-of validation for ClientID vs DCRConfig and DiscoveryURL vs RegistrationEndpoint. Purely additive: no behaviour change, no CRD changes.

3. Step 2f/2c (e965b86d) — Add DCRCredentialStore interface + in-memory implementation keyed by (Issuer, RedirectURI, ScopesHash). Add resolveDCRCredentials: cache lookup, metadata discovery or direct registration endpoint, explicit endpoint override, auth-method intersection, synthesis of {origin}/register when metadata omits it, single call to oauthproto.RegisterClientDynamically with initial access token injected via a wrapping RoundTripper, Put before returning.

4. Review iterations (abbc15ab + amendments in earlier commits) — Address HIGH / MEDIUM findings:

   • Honour DCRUpstreamConfig.DiscoveryURL exactly via FetchAuthorizationServerMetadataFromURL (no well-known-path fallback on an operator-supplied URL).
   • Rewrite cache-hit test to wire the counting server as the issuer and assert zero requests.
   • Expose NewDefaultDCRClient so newDCRHTTPClient inherits the canonical timeout.
   • scopesHash deduplicates via slices.Compact.
   • applyResolution doc comment clarifies it overwrites ClientID.
   • Direct-RegistrationEndpoint path documented as disabling server-capability negotiation.
   • io.ReadAll replaces hand-rolled body readers; response bodies drained before close in error paths.

Final review state: 0 CRITICAL / 0 HIGH / 0 MEDIUM.

Large PR Justification

• The resolver core (~700 lines) is irreducible: RFC 7591's validate → cache → singleflight → discover → register → cache-write sequence is one logical change with no shippable subunit, so a minimal viable DCR resolver lands well over the 400-line guidance no matter how it's structured.
• The original issue scoped this poorly: it framed "DCR credential store + resolver" as a single deliverable, where in hindsight three independent units (pkg/oauthproto FetchFromURL ~94 lines, DCRCredentialStore ~150 lines, run-config validator additions ~50 lines) had clean dependency seams and could have stacked ahead of the resolver core.
• Review-driven additions inflated the bundle by ~250 lines (panic recovery, RFC 7591 expiry refetch, S256 PKCE gate, redirect refusal, URL validators, path-preservation fixes, endpoint-required-on-bypass) that each could have been small follow-up PRs against a merged resolver, but folded in here because they surfaced during this PR's review — splitting them off now would re-pay merge cost across the stack and lose the review trail, so the bundle stays as-is.

[github-actions]

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

---

This review will be automatically dismissed once you add the justification section.

[codecov]

Codecov Report

❌ Patch coverage is 89.57746% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.51%. Comparing base (ffefa61) to head (5c1e606).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/authserver/runner/dcr.go	88.44%	18 Missing and 17 partials ⚠️
pkg/oauthproto/discovery.go	90.90%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5042      +/-   ##
==========================================
+ Coverage   67.29%   67.51%   +0.21%     
==========================================
  Files         598      601       +3     
  Lines       60348    60969     +621     
==========================================
+ Hits        40612    41163     +551     
- Misses      16659    16693      +34     
- Partials     3077     3113      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jhrozek]

While reviewing I noticed we're about to ship two DCR-client implementations: this resolver and the CLI flow in pkg/auth/discovery/discovery.go::PerformOAuthFlow (which already calls oauthproto.RegisterClientDynamically and persists creds via Config.CachedClientID/CachedClientSecretRef/CachedRegTokenRef). Same RFC 7591 role, different client profile (public-PKCE vs confidential) and concurrency model.

A shared pkg/auth/dcr package — stateful resolver, cache, singleflight, scope canonicalisation — consumed by both, with stateless RFC primitives lifted into pkg/oauthproto, would prevent the duplication calcifying. Worth filing a follow-up issue to track? Easier to migrate now than after another consumer lands.

[jhrozek]

Inline review of the DCR resolver scaffolding — seven questions / suggestions across dcr.go, dcr_test.go, and dcr_store.go. Top-level architectural note posted separately.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: security, concurrency, test-coverage, error-handling/api-design, architecture, oauth-protocol, code-quality (general). Codex cross-review: skipped (codex CLI not installed).

Consensus Summary

#	Finding	Consensus	Severity	Action
F1	resolveDCRCredentials doc misidentifies the issuer parameter as the upstream's	9/10	HIGH	Fix doc / consider rename
F2	Direct RegistrationEndpoint path: nothing forces caller to set AuthorizationEndpoint/TokenEndpoint	8/10	MEDIUM	Tighten OAuth2UpstreamRunConfig.Validate
F3	endpointsFromMetadata derefs metadata without nil guard	7/10	LOW	Add nil guard
F4	singleflight returns shared *DCRResolution to followers	7/10	LOW	Document or copy
F5	Synthesised registration endpoint not re-validated for HTTPS-or-loopback	7/10	LOW	Add validateUpstreamEndpointURL call
F6	chooseRegistrationScopes warning logs the wrong issuer (auth-server vs. upstream)	7/10	LOW	Pass upstream issuer (related to F1)
F7	validateResolveInputs nil-rc / empty-issuer / nil-cache branches uncovered	7/10	LOW	Add table rows
F8	synthesiseRegistrationEndpoint error paths uncovered	7/10	LOW	Add error rows
F9	deriveExpectedIssuerFromDiscoveryURL missing-host branch uncovered	7/10	LOW	Add 1 row
F10	buildResolution fallback to sent auth method uncovered	7/10	LOW	Add 1 test
F11	dcrStaleAgeThreshold is dead code in this PR (nolint:unused)	9/10	INFO	Acknowledge / track for #5039

Overall

The PR delivers Phase 2 of the DCR story with strong defensive engineering: HTTPS-or-loopback enforced at every URL boundary, RFC 8414 §3.3 issuer-equality, a registration redirect-block that prevents bearer-token leakage to foreign origins (pinned by _DoesNotForwardBearerOnRedirect with a foreign-origin tripwire), defensive copies on the cache, and singleflight-coalesced registration to prevent orphaned clients at the upstream. Test coverage is dense (~30 functions, 1216 lines on the resolver alone) with explicit security-property tests and a table-driven concurrent-store stress test bounded by a fail-fast deadline. Module boundaries are clean — protocol primitives stay in pkg/oauthproto, runtime resolution and the bearer-token transport stay in pkg/authserver/runner. The PR description and doc-comment density are exemplary.

The blocking concern is F1: the public doc on resolveDCRCredentials says issuer is the upstream's identifier used both for discovery and for keying the cache, but the implementation never uses issuer for discovery — deriveExpectedIssuerFromDiscoveryURL(cfg.DiscoveryURL) recovers the upstream issuer separately. The inline comment in registerAndCache and the test _UpstreamIssuerDerivedFromDiscoveryURL both confirm that the parameter actually names this auth server (used for redirect-URI defaulting and cache keying). The next caller (#5039) reading the public doc will pass the wrong identifier, producing a wrong-origin default redirect URI under the upstream and a wrong cache key. Either fix the doc or rename the parameter (localIssuer / authserverIssuer); the same ambiguity is on DCRKey.Issuer in dcr_store.go.

F2 is the next concern: when an operator chooses dcr_config.registration_endpoint (bypass discovery), the doc on RegistrationEndpoint warns them to also supply AuthorizationEndpoint/TokenEndpoint/Scopes, but OAuth2UpstreamRunConfig.Validate does not enforce this. A misconfigured run-config registers a client successfully and silently fails at first authorize/token-exchange call. Per .claude/rules/go-style.md Constructor Validation: Fail Loudly on Invalid Input, this should fail at startup.

The remaining findings (F3–F11) are individually small but mostly mechanical: a defensive nil-guard, a singleflight pointer-aliasing note, a missed re-validation of one synthesised URL, a logging-issuer mix-up that follows naturally from F1, and four test-coverage gaps each a row or two of additions. F11 is a tracked forward-prep nolint:unused constant that is fine as-is provided #5039 lands on schedule.

No security-property regression and no breakage of the pre-provisioned-client_id path. Recommendation: COMMENT — F1/F2 are worth addressing before merge but neither blocks correctness today (the wiring caller does not yet exist).

Documentation

• F1's recommendation should propagate to the doc on DCRKey.Issuer in pkg/authserver/runner/dcr_store.go, which currently reads the authorization server's issuer identifier — same auth-server-vs-upstream ambiguity.

---

Generated with Claude Code

Large PR justification has been provided. Thank you!

[github-actions]

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

[jhrozek]

Seven inline comments addressed across dedicated commits — each fix matches or exceeds the suggestion. Two extra commits close adjacent footguns (local/upstream issuer disambiguation, explicit-endpoints validation when discovery is bypassed). LGTM.