You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
We are about to ship two RFC 7591 Dynamic Client Registration client implementations side by side:
pkg/authserver/runner/dcr.go (this PR, sub-issue B of Authserver DCR integration (Phase 2, Steps 2a-2g) #4978) — the embedded authserver's resolver: stateful, in-process cache (DCRCredentialStore), singleflight.Group deduplication, scope canonicalisation, RFC 7591 §3.2.1 expiry-driven refetch, S256 PKCE gating, panic recovery, etc. Public-PKCE / confidential-client capable depending on what the upstream advertises.
pkg/auth/discovery/discovery.go::PerformOAuthFlow — the CLI flow. Already calls oauthproto.RegisterClientDynamically and persists credentials as OAuthFlowResult.ClientID / ClientSecret, which the remote handler later reads via Config.CachedClientID / CachedClientSecretRef / CachedRegTokenRef (see pkg/auth/remote/handler.go).
Same RFC role, different client profile, different concurrency model, different cache shape. Without consolidation each side will diverge: one will gain features (S256 gate, expiry refetch, singleflight) the other won't, and a third consumer landing later will inherit whichever one the author happened to import.
Proposal
Lift the shared logic into a new pkg/auth/dcr package consumed by both call sites:
Anything else that's pure "RFC 7591 wire shape, no state".
Profile differences stay at the call sites — the resolver caller chooses its own client profile (public PKCE vs confidential), redirect URI policy, and credential persistence strategy. The shared package is profile-agnostic.
Extract pkg/auth/dcr from pkg/authserver/runner/dcr.go + dcr_store.go. The embedded authserver becomes the first consumer; its existing tests carry over with minimal surface change (mostly import paths).
Migrate pkg/auth/discovery/discovery.go::PerformOAuthFlow to consume pkg/auth/dcr instead of calling oauthproto.RegisterClientDynamically directly. The CLI will then inherit S256 gating, expiry refetch, redirect refusal, and so on automatically.
Audit pkg/auth/remote/handler.go::CachedClientID / CachedClientSecretRef / CachedRegTokenRef usage. Either replace with the shared cache interface, or treat those fields as the CLI profile's persistence backend implementing the shared interface.
Why now
Easier to migrate now than after another consumer lands.
The third consumer almost certainly arrives in the next 1–2 quarters (every new OAuth2 upstream that supports DCR is a candidate). Migrating two sites is a straightforward refactor; migrating three or four with subtly diverged behaviour is a multi-week archaeology project.
Acceptance
pkg/auth/dcr exists and exports a stateful resolver consumed by both the embedded authserver and the CLI flow.
Stateless RFC 7591 wire-shape helpers live in pkg/oauthproto.
No direct calls to oauthproto.RegisterClientDynamically outside pkg/auth/dcr (search-grep enforces this).
Behaviour properties added during Add authserver DCR credential store and resolver #5042 review (S256 gate, expiry-driven refetch, redirect refusal, panic recovery, singleflight) apply to the CLI flow without per-call-site re-implementation.
Context
Surfaced during review of #5042 (comment).
We are about to ship two RFC 7591 Dynamic Client Registration client implementations side by side:
pkg/authserver/runner/dcr.go(this PR, sub-issue B of Authserver DCR integration (Phase 2, Steps 2a-2g) #4978) — the embedded authserver's resolver: stateful, in-process cache (DCRCredentialStore),singleflight.Groupdeduplication, scope canonicalisation, RFC 7591 §3.2.1 expiry-driven refetch, S256 PKCE gating, panic recovery, etc. Public-PKCE / confidential-client capable depending on what the upstream advertises.pkg/auth/discovery/discovery.go::PerformOAuthFlow— the CLI flow. Already callsoauthproto.RegisterClientDynamicallyand persists credentials asOAuthFlowResult.ClientID/ClientSecret, which the remote handler later reads viaConfig.CachedClientID/CachedClientSecretRef/CachedRegTokenRef(seepkg/auth/remote/handler.go).Same RFC role, different client profile, different concurrency model, different cache shape. Without consolidation each side will diverge: one will gain features (S256 gate, expiry refetch, singleflight) the other won't, and a third consumer landing later will inherit whichever one the author happened to import.
Proposal
Lift the shared logic into a new
pkg/auth/dcrpackage consumed by both call sites:Stateful concerns belong in
pkg/auth/dcr:DCRCredentialStoreinterface + in-memory backend, eventually a Redis-backed one — see issue Harden DCR resolver against SSRF when operator trust boundary changes #5135 for the SSRF hardening that lives downstream of this).singleflight.Groupkeyed on the canonical key tuple.scopesHashin the resolver).Stateless RFC 7591 primitives belong in
pkg/oauthproto:DynamicClientRegistrationRequest/DynamicClientRegistrationResponse(already there).RegisterClientDynamically(already there).FetchAuthorizationServerMetadata*(already there).Profile differences stay at the call sites — the resolver caller chooses its own client profile (public PKCE vs confidential), redirect URI policy, and credential persistence strategy. The shared package is profile-agnostic.
Migration sketch
pkg/auth/dcrfrompkg/authserver/runner/dcr.go+dcr_store.go. The embedded authserver becomes the first consumer; its existing tests carry over with minimal surface change (mostly import paths).pkg/auth/discovery/discovery.go::PerformOAuthFlowto consumepkg/auth/dcrinstead of callingoauthproto.RegisterClientDynamicallydirectly. The CLI will then inherit S256 gating, expiry refetch, redirect refusal, and so on automatically.pkg/auth/remote/handler.go::CachedClientID/CachedClientSecretRef/CachedRegTokenRefusage. Either replace with the shared cache interface, or treat those fields as the CLI profile's persistence backend implementing the shared interface.Why now
The third consumer almost certainly arrives in the next 1–2 quarters (every new OAuth2 upstream that supports DCR is a candidate). Migrating two sites is a straightforward refactor; migrating three or four with subtly diverged behaviour is a multi-week archaeology project.
Acceptance
pkg/auth/dcrexists and exports a stateful resolver consumed by both the embedded authserver and the CLI flow.pkg/oauthproto.oauthproto.RegisterClientDynamicallyoutsidepkg/auth/dcr(search-grep enforces this).References