Migrate CLI OAuth flow to pkg/auth/dcr resolver

[tgrunnagle]

Summary

Sub-issue 4b of #5145. The CLI OAuth flow in pkg/auth/discovery::PerformOAuthFlow used to call oauthproto.RegisterClientDynamically directly, so it bypassed the review-property behaviours added during #5042 (S256 PKCE gating, RFC 7591 §3.2.1 expiry-driven refetch, bearer-token transport with redirect refusal, panic recovery, singleflight deduplication). This PR routes that call site through the shared pkg/auth/dcr resolver introduced in sub-issue 4a (PR #5198) so both consumers — the embedded authserver and the CLI — share the same hardened DCR path.

To make the resolver genuinely profile-agnostic, pkg/auth/dcr now exposes a Request struct carrying exactly the fields the resolver reads. The embedded-authserver adapter helpers move into pkg/authserver/runner/dcr_adapter.go where they belong by ownership, and ResolveCredentials no longer imports authserver or upstream domain types.

Closes #5219.

Large PR Justification

The diff is 1903 insertions / 845 deletions across 18 files (872 / 414 excluding tests). Production-code changes that cannot be split:

• Neutralising ResolveCredentials's API is a single atomic step. Replacing *authserver.OAuth2UpstreamRunConfig with the profile-neutral dcr.Request envelope and lifting NeedsDCR / ConsumeResolution / ApplyResolutionToOAuth2Config into pkg/authserver/runner/dcr_adapter.go must land together — splitting them would leave the resolver importing authserver types between PRs, which is the architectural drift this sub-issue exists to close.
• The CLI migration is the second consumer that makes the neutralised API load-bearing. Migrating pkg/auth/discovery::PerformOAuthFlow onto the shared resolver in the same PR is what proves the neutralisation is correct; a follow-up CLI migration would leave the API change unjustified and unexercised at merge time.

Test churn accounts for the remainder. pkg/auth/dcr/resolver_test.go was rewritten to the new Request API (the old test file constructed OAuth2UpstreamRunConfig fixtures throughout); the consumer-side tests for the lifted helpers moved to pkg/authserver/runner/dcr_adapter_test.go to follow the production code. New pkg/auth/discovery/dcr_resolver_test.go pins the CLI's inherited review properties (S256 PKCE gating, redirect refusal, singleflight dedup, fallback error verbatim) — these are the load-bearing acceptance criteria for sub-issue 4b and cannot be deferred to a follow-up.

Type of change

• Refactoring (no behavior change)

Test plan

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

New tests in pkg/auth/discovery/dcr_resolver_test.go cover the CLI's inherited properties:

• S256 PKCE gating — CLI surfaces a clear resolver error when upstream advertises only plain.
• Bearer-token transport redirect refusal — registration POSTs do not follow redirects.
• Singleflight deduplication — concurrent invocations with the same (issuer, scopes, redirectURI) produce one /register call.
• Fallback error preservation — TestHandleDynamicRegistration_MissingRegistrationEndpoint pins the verbatim "configure with --remote-auth-client-id / --remote-auth-client-secret" message.

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 operator API surface is touched. The pkg/auth/dcr package was added in 4a (#5198) and has no external consumers; the ResolveCredentials signature change is internal.

Changes

File	Change
pkg/auth/dcr/request.go	New profile-neutral Request input type for ResolveCredentials.
pkg/auth/dcr/resolver.go	ResolveCredentials now takes *dcr.Request; no longer imports authserver / upstream.
pkg/auth/dcr/store.go	New CloseableCredentialStore interface; NewInMemoryStore returns it so defer Close() is compile-time safe.
pkg/auth/discovery/discovery.go	handleDynamicRegistration builds a dcr.Request and calls dcr.ResolveCredentials instead of oauthproto.RegisterClientDynamically. Loopback redirect URI (RFC 8252 §7.3) and CLI-flag fallback error preserved.
pkg/auth/discovery/dcr_request.go	Deleted — superseded by pkg/auth/dcr.Request.
pkg/auth/discovery/dcr_resolver_test.go	New tests pinning the CLI's inherited review properties.
pkg/authserver/runner/dcr_adapter.go	Embedded-authserver-specific adapter helpers (needsDCR, newDCRRequest, consumeResolution, applyResolutionToOAuth2Config) moved here from pkg/auth/dcr.
pkg/authserver/runner/embeddedauthserver.go	Call sites updated to use the adapter and pass *dcr.Request into dcr.ResolveCredentials.
pkg/authserver/runner/dcr_store.go	Deleted — folded into the shared pkg/auth/dcr/store.go (4a follow-through).

Does this introduce a user-facing change?

No. The CLI OAuth flow's observable behaviour is unchanged on the happy path. The new error paths (S256 gating, redirect refusal) are inherited security improvements that surface as clearer error messages when an upstream provider misbehaves.

Special notes for reviewers

Persistence model — option (b) from the issue. The resolver runs against an in-memory dcr.CredentialStore scoped to one PerformOAuthFlow invocation. Cross-invocation persistence is handled outside the resolver by pkg/auth/remote/handler.go's existing CachedClientID / CachedClientSecretRef fields, which already preserved cross-invocation reuse and continue to do so unchanged. Option (a) — wrapping the secret provider as a CredentialStore adapter — was rejected as out-of-scope churn; the trade-off is documented at the wiring site in handleDynamicRegistration. A consequence worth flagging: cross-invocation expiry refetch lives in the remote handler, not in the resolver, on the CLI path. Closing that loop is a natural follow-up once option (a) is wired.

PublicClient flag. A new bool on dcr.Request tells the resolver to register as a public PKCE client (token_endpoint_auth_method=none). The S256 gate still fires — the CLI surfaces a clear resolver error rather than silently downgrading when the upstream advertises only plain PKCE.

Resolver invariant — code review only. The issue's acceptance criterion for a CI grep guard against direct oauthproto.RegisterClientDynamically calls outside pkg/auth/dcr is intentionally out of scope for this PR. A task check-dcr-isolation Taskfile target and matching workflow step were prototyped and then removed (commit fd66a1f) per reviewer preference. The resolver boundary is enforced by code review alone going forward.

[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.

[tgrunnagle]

Multi-Agent Consensus Review

Agents consulted: oauth-security, go-correctness, architecture, concurrency, test-coverage, general-code-quality (codex cross-review skipped — CLI not installed)

Consensus Summary

#	Finding	Consensus	Severity	Action
1	PublicClient absent from singleflight + cache keys	9/10	MEDIUM	Discuss
2	applyExplicitEndpointOverrides bypasses URL validation	7/10	MEDIUM	Fix
3	Two-call consumeResolution/applyResolutionToOAuth2Config invariant not type-enforced	8/10	MEDIUM	Discuss
4	SanitizeErrorForLog is http(s)-only — redis-go error chain	7/10	MEDIUM	Discuss
5	DCRKey.Issuer doc contradicts new CLI usage	8/10	MEDIUM	Fix
6	_ = store.Close() swallows error without comment/log	7/10	LOW	Fix
7	TestNewDCRRequest missing env-var IAT branch	7/10	LOW	Fix
8	TestConsumeResolution does not pin nil-input no-op	6/10	LOW	Optional
9	LogStepError "upstream" attribute shape varies per consumer	7/10	LOW	Discuss
10	Direct-registration + PublicClient=true error msg is misleading	6/10	LOW	Optional
11	Wall-clock ClientSecretExpiresAt comparison lacks skew tolerance	6/10	LOW	Optional
12	PR body still reads "DRAFT - not ready for review"	7/10	LOW	Fix
13	Option-(b) rationale duplicated across two function docs	6/10	LOW	Optional

Overall

This is a well-executed migration of the CLI OAuth flow onto the shared pkg/auth/dcr resolver. The package boundary is structurally clean (resolver no longer imports any authserver/discovery types — verified), the new dcr.Request envelope is genuinely profile-neutral, and the new inherited-property tests (S256 gate, redirect refusal, singleflight dedup) pin exactly the security-relevant behaviours that motivated the migration. The PR description, doc comments, and orientation prose are unusually good — a cold reviewer can pick up the architectural picture in a single pass.

The consensus findings cluster around one theme: correctness invariants encoded in comments rather than types. The singleflight + cache keys do not include PublicClient, the two-call resolution invariant relies on review, the sanitiser is scheme-specific despite the generic name, and DCRKey.Issuer's field doc still claims "local issuer ... NOT the upstream's" even though the CLI now puts the upstream issuer there. Each one is a small refactor away from being structural. Current call sites do not exhibit the failure modes by construction; the risk is forward-looking, not active.

None of the findings rise to a hard blocker. Recommended priorities are #1 (PublicClient in keys), #3 (two-call consolidation), and #5 (DCRKey doc fix) — all cheap, all close defense-in-depth gaps the author already calls out in inline comments.

For consistency, please note: the authenticated reviewer and PR author are the same user, so this review uses COMMENT rather than REQUEST_CHANGES — this is not a draft-blocker assessment, it's an artefact of GitHub's policy.

Additional findings (file not in this PR's diff, or line outside diff hunks)

• [MEDIUM] DCRKey.Issuer doc drift — pkg/authserver/storage/types.go:122-130 documents Issuer as "the local issuer of the embedded authorization server that performed the registration, NOT the upstream's". The new CLI call site at pkg/auth/discovery/discovery.go:712-713 puts the upstream's issuer URL there. The new dcr.Request.Issuer doc accommodates both consumers, but the underlying storage type's field doc still claims the old single-consumer semantic. Update DCRKey.Issuer to reflect the dual semantic, e.g. "For the embedded authserver this is its own (local) issuer; for the CLI flow this is the upstream's issuer because the CLI has no separate logical issuer of its own. The cache key (Issuer, RedirectURI, ScopesHash) keeps the two consumers' entries apart via the RedirectURI component."
• [MEDIUM] SanitizeErrorForLog is http(s)-only — pkg/auth/dcr/resolver.go:481-538 (outside this PR's diff hunks). The function name reads generic, but the implementation only strips userinfo/query/fragment from http(s):// URLs. On the embedded-authserver path, cache.Get/Put errors from storage/redis.go flow into LogStepError. If a future operator (or redis-go release) puts credentials in a sentinel/cluster URL, those credentials hit the slog.Error attribute. Either rename to sanitizeHTTPURLs so the scope is unambiguous, or extend the regex to also match redis(s)?://. Add a small test asserting a redis-style error string with credentials is sanitised.
• [LOW] LogStepError "upstream" attribute is inconsistent per consumer — pkg/auth/dcr/resolver.go:452-479 (outside this PR's diff hunks). Embedded-authserver path passes rc.Name (operator-defined short name); CLI path passes issuer (upstream issuer URL). Operators reading mixed logs see two different shapes under the same key. Either rename the parameter to upstreamID and document it as opaque, or accept slog.Attr pairs so callers can label ("upstream_name" vs "upstream_issuer").
• [LOW] ClientSecretExpiresAt wall-clock comparison lacks skew tolerance — pkg/auth/dcr/resolver.go:641 (outside this PR's diff hunks). time.Now().After(cached.ClientSecretExpiresAt) is exact. NTP step on resume or AS-clock-ahead skew leads to either serving an expired secret (already broken at the AS) or refetching too early. Consider time.Now().Add(30 * time.Second).After(cached.ClientSecretExpiresAt) as defense-in-depth.
• [LOW] PR body still reads "DRAFT - not ready for review" — The PR is in draft state and the body header says "DRAFT - not ready for review". Since you are explicitly requesting a review now, either flip out of draft or remove the header so other reviewers landing on this PR aren't confused about its status.

Dropped findings appendix (consensus < 7)

The following observations were raised by individual agents but did not meet the consensus threshold. Listed for auditability only — no action requested:

• Regex (?i)https?://[^\s"']+ can match across a comma (security LOW, score 5). Failure mode is "did not sanitise", not corrupted output.
• selectTokenEndpointAuthMethod with empty serverSupported + publicClient=true trusts upstream to accept "none" (security LOW, score 5).
• CloseableCredentialStore loses Close capability when widened to base interface (architecture LOW, score 6). No occurrence in this PR.
• Hardcoded CallbackPort: 8765 in CLI tests (test INFO, score 5). Tests don't bind the port — acceptable today.
• dcr.Key is generically named (quality INFO, score 5).
• flightKeyOf(key) is a free function where a method would read more idiomatically (quality LOW, score 5).
• Long doc comments on Request/Resolution partially restate code (quality LOW, score 5).
• handleDynamicRegistration doc preamble is 26 lines for a 45-line body (quality LOW, score 5).
• ResolveCredentials takes *Request by pointer; could take by value (architecture LOW, score 5).
• PublicClient bool tri-state suggestion to fail-loud on unset (security INFO, score 5).
• CLI cross-invocation expiry refetch not in resolver's loop (architecture INFO, score 6) — explicitly documented as option-(b) trade-off.

---

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 87.16578% with 24 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.20%. Comparing base (3fa65b0) to head (fc4fa7d).

Files with missing lines	Patch %	Lines
pkg/auth/dcr/resolver.go	81.25%	10 Missing and 5 partials ⚠️
pkg/auth/discovery/discovery.go	87.87%	2 Missing and 2 partials ⚠️
pkg/authserver/runner/dcr_adapter.go	95.34%	1 Missing and 1 partial ⚠️
pkg/authserver/runner/embeddedauthserver.go	71.42%	1 Missing and 1 partial ⚠️
pkg/auth/dcr/store.go	94.73%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5250      +/-   ##
==========================================
+ Coverage   68.17%   68.20%   +0.02%     
==========================================
  Files         618      618              
  Lines       63146    63212      +66     
==========================================
+ Hits        43049    43112      +63     
- Misses      16884    16886       +2     
- Partials     3213     3214       +1     

☔ 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.

[tgrunnagle]

Responding to the review-body findings (no inline anchor):

• MEDIUM — `DCRKey.Issuer` doc drift (pkg/authserver/storage/types.go:122-130): addressed in c805aa3. The field doc now describes both consumer profiles (embedded authserver's own issuer vs CLI's upstream issuer) and the cache-key non-collision invariant via the RedirectURI component.

• MEDIUM — `SanitizeErrorForLog` http-only (pkg/auth/dcr/resolver.go:481-538): addressed in c805aa3. queryStrippingPattern now matches (?i)(?:https?|rediss?):// so redis-go error chains on the embedded-authserver Get/Put critical path get their userinfo / query / fragment stripped. Added TestSanitizeErrorForLog cases pinning redis:// and rediss:// URL userinfo redaction.

• LOW — `LogStepError` "upstream" attribute shape varies: deferring. Two consumers populate the field differently today (rc.Name for embedded authserver, upstream issuer URL for the CLI). Renaming to upstreamID is correct but cosmetic — operators reading mixed logs already have to disambiguate by step and the other structured attributes. A typed-slog.Attr API change would be invasive enough to warrant its own PR; the current shape is documented and the observability cost is low.

• LOW — `ClientSecretExpiresAt` lacks skew tolerance (pkg/auth/dcr/resolver.go:641): deferring. The 30-second early-refetch buffer is a real ergonomics improvement but defense-in-depth on top of an already-defensive refetch path (RFC 7591 §3.2.1 expiry handling). Best paired with the broader review of clock-sensitive operations across the auth path rather than landed in isolation.

• LOW — PR body "DRAFT" header: removed via gh pr edit.

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.