Commit f180fa9
authored
Migrate CLI OAuth flow to pkg/auth/dcr resolver (#5250)
* Migrate CLI OAuth flow to pkg/auth/dcr resolver
Sub-issue 4b of #5145. The CLI OAuth flow at
pkg/auth/discovery::PerformOAuthFlow used to call
oauthproto.RegisterClientDynamically directly, so it did not inherit 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 commit routes
that call site through the shared pkg/auth/dcr resolver introduced in
sub-issue 4a (PR #5198) and pins the invariant with a CI grep guard.
Profile-neutral resolver input: pkg/auth/dcr now exposes a Request struct
that carries exactly the fields the resolver reads (issuer, redirect
URI, scopes, discovery URL or registration endpoint, optional explicit
endpoint overrides, initial access token, client name, public-client
flag). ResolveCredentials takes a Request and no longer imports
authserver / upstream domain types. The embedded-authserver adapter
helpers (needsDCR, consumeResolution, applyResolutionToOAuth2Config)
move to pkg/authserver/runner/dcr_adapter.go where they belong by
ownership.
CLI 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. Wrapping the secretProvider into
a CredentialStore adapter (option (a)) was rejected as out-of-scope
churn — the existing remote-handler caching is sufficient.
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 upstream advertises only "plain".
Invariant guard: Taskfile target check-dcr-isolation (wired into task
lint) and a matching CI step in .github/workflows/lint.yml fail if
oauthproto.RegisterClientDynamically is referenced anywhere outside
pkg/auth/dcr or pkg/oauthproto.
Tests added for the CLI's inherited properties (S256 gating, redirect
refusal, singleflight deduplication) in
pkg/auth/discovery/dcr_resolver_test.go. The fallback error message for
upstreams that omit registration_endpoint is preserved verbatim and
pinned by TestHandleDynamicRegistration_MissingRegistrationEndpoint.
Closes #5145.
* Address code review feedback
Fixed issues from code review:
- MEDIUM: Rewrote TestResolveSecret / TestResolveSecretWithEnvVar doc
comments so they no longer point at the deleted dcr-package twin; the
runner-side resolveSecret is now described as the single authoritative
implementation.
- MEDIUM: Documented the redundant discovery fetch in resolveDCRCredentials
as an acknowledged trade-off (the S256 PKCE gate needs
code_challenge_methods_supported, which AuthServerInfo does not carry).
Threading that field through AuthServerInfo is the natural follow-up.
- MEDIUM: Rewrote the dcr.Request.Issuer field doc to spell out what each
consumer puts there and why the cache key cannot collide between the
embedded authserver and CLI consumers. Added a matching call-site
comment in resolveDCRCredentials.
- MEDIUM: Introduced dcr.CloseableCredentialStore (embeds CredentialStore
+ io.Closer). NewInMemoryStore now returns it, so the CLI's
defer store.Close() is compile-time safe — no more anonymous-interface
type-assertion that would silently no-op on a future refactor.
- MEDIUM: Reconciled the CLI flow's "expiry refetch inheritance" wording
with what option (b) actually delivers. The handleDynamicRegistration
doc and the dcr_resolver_test.go file-level comment now spell out that
cross-invocation expiry handling lives in the remote handler, not the
resolver, and that option (a) would close the loop as a follow-up.
- LOW: The unreachable "fall back to RegistrationEndpoint-direct path"
comment is replaced by an accurate description of when each branch
fires.
* Address iteration-2 code review feedback
Fixed issues from second-iteration review:
- MEDIUM: Made dcr.inMemoryStore.Close() idempotent via sync.Once.
storage.MemoryStorage.Close() closes a channel and is NOT itself
idempotent — the previous wrapper inherited that defect through an
incorrect doc comment ("Safe to call multiple times"). The wrapper now
delivers what the comment claims: a second Close returns the captured
error from the first call rather than panicking on
"close of closed channel". Pinned by TestInMemoryStore_CloseIsIdempotent
and TestInMemoryStore_CloseIsIdempotentUnderRace (8 concurrent callers).
- MEDIUM: Dropped the embedded storageBackedStore from inMemoryStore so
the type holds a single *storage.MemoryStorage handle instead of two
parallel handles (one via embedded interface, one concrete) that
required a manual "keep these two in sync" invariant. Get and Put are
implemented directly on inMemoryStore, delegating to s.mem — three
lines each, structurally guaranteed to share a backend with Close.
Pinned by TestInMemoryStore_PutGetCloseShareBackend,
TestInMemoryStore_PutRejectsNilResolution, and
TestInMemoryStore_GetMissingKeyReturnsMissTuple.
* Drop CI guard against direct RegisterClientDynamically calls
The Taskfile check-dcr-isolation grep guard and its workflow
counterpart added extra surface area to enforce an architectural
invariant. Removing both per reviewer preference; the resolver
boundary remains enforced by code review alone.
* Tighten DCR resolver defense-in-depth from PR review
Addresses #5250 review comments:
- MEDIUM pkg/auth/dcr/resolver.go (3221902433): include PublicClient in
storage.DCRKey, flightKeyOf, and the Redis key serialisation so public
and confidential registrations cannot share a cache entry under any
Issuer/RedirectURI/ScopesHash collision.
- MEDIUM pkg/auth/dcr/resolver.go (3221902453): apply
validateUpstreamEndpointURL to explicit authorization_endpoint /
token_endpoint overrides; propagate the error via dcrStepMetadata.
- MEDIUM pkg/authserver/runner/dcr_adapter.go (3221902473): add a
tripwire test that pins the two-call invariant — calling only
consumeResolution must leave the built upstream.OAuth2Config's
ClientSecret empty.
- MEDIUM pkg/authserver/storage/types.go (body): rewrite DCRKey.Issuer
doc to describe both consumer profiles (embedded authserver's own
issuer vs CLI's upstream issuer) and the cache-key non-collision
invariant.
- MEDIUM pkg/auth/dcr/resolver.go (body): extend
queryStrippingPattern / SanitizeErrorForLog to also match
redis(s):// URLs; covers the persistent CredentialStore error-chain
surface on the embedded-authserver path.
- LOW pkg/auth/discovery/discovery.go (3221902485): log the
store.Close() error at debug instead of dropping it; rationale in
the deferred-Close block.
- LOW pkg/authserver/runner/dcr_adapter_test.go (3221902504): add an
env-var case to TestNewDCRRequest pinning the
InitialAccessTokenEnvVar wiring end-to-end.
* Fix spell-check failure on "Get's" in store_test.go
The doc comment for TestInMemoryStore_PutRejectsNilResolution used a
possessive apostrophe where the plain noun is correct.
* Drop PublicClient from DCR cache and flight keys
The original PublicClient-in-keys fix was defense-in-depth against a
collision that today's two consumers cannot reach: the embedded
authserver registers on AS-origin redirect URIs and the CLI registers
on RFC 8252 loopback redirect URIs, and the two address spaces are
disjoint. RedirectURI alone separates the public-client and
confidential-client profiles at both the persistent-cache and
singleflight layers.
Encoding PublicClient additionally would invalidate every existing
Redis-cached entry across a deployment without buying additional
protection. Drop it from DCRKey and flightKeyOf and document the
RedirectURI-disjointness invariant alongside the migration condition
for a hypothetical future consumer that brings the two address spaces
into collision.
* Correct stale DCR doc comments from PR review
Addresses #5250 review comments:
- LOW pkg/auth/discovery/discovery.go (3233521790): handleDynamicRegistration
claimed HasCachedClientCredentials enforces cross-invocation expiry, but
the gate only checks CachedClientID != "" and does not consult
CachedSecretExpiry. Rewrite the option-(b) doc block to acknowledge that
cross-invocation expiry is unhandled today and that tightening the gate
is open follow-up work.
- LOW pkg/auth/dcr/resolver.go (3233521799): replace two stale "Step 2g"
forward-references with prose describing today's reality — staleness
observability already lives in lookupCachedResolution (no separate sub-
issue tracks it), and the "no retry loop" comment now spells out that
any future retry/backoff belongs above ResolveCredentials.1 parent 88667f7 commit f180fa9
17 files changed
Lines changed: 1873 additions & 837 deletions
File tree
- pkg
- authserver
- runner
- storage
- auth
- dcr
- discovery
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
0 commit comments