Skip to content

Commit b1d9d93

Browse files
tgrunnagletgrunnagle
committed
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.
1 parent 3fa65b0 commit b1d9d93

15 files changed

Lines changed: 1458 additions & 786 deletions

‎.github/workflows/lint.yml‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,32 @@ jobs:
4444
with:
4545
# Enable golangci-lint's built-in caching (removes skip-cache: true)
4646
args: --timeout=5m
47+
48+
# Enforces issue #5145's invariant that
49+
# oauthproto.RegisterClientDynamically is only called from
50+
# pkg/auth/dcr (the shared resolver) or pkg/oauthproto (the
51+
# wire-shape primitive itself). Direct calls from elsewhere bypass
52+
# the resolver's S256 PKCE gating, singleflight deduplication,
53+
# expiry-driven refetch, bearer-token transport with redirect
54+
# refusal, and panic recovery. See Taskfile.yml::check-dcr-isolation
55+
# for the rationale and matching shell command.
56+
- name: Check DCR isolation invariant
57+
run: |
58+
matches=$(grep -rn 'oauthproto\.RegisterClientDynamically\|\.RegisterClientDynamically(' \
59+
--include='*.go' \
60+
--exclude-dir='vendor' \
61+
--exclude-dir='mocks' \
62+
. 2>/dev/null \
63+
| grep -v '^./pkg/auth/dcr/' \
64+
| grep -v '^./pkg/oauthproto/' \
65+
|| true)
66+
if [ -n "$matches" ]; then
67+
echo "ERROR: oauthproto.RegisterClientDynamically must only be called from pkg/auth/dcr or pkg/oauthproto."
68+
echo " Route the call through pkg/auth/dcr.ResolveCredentials so the consumer inherits"
69+
echo " S256 PKCE gating, singleflight, expiry refetch, redirect refusal, and panic recovery."
70+
echo ""
71+
echo "Offending references:"
72+
echo "$matches"
73+
exit 1
74+
fi
75+
echo "check-dcr-isolation: ok"

‎Taskfile.yml‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ tasks:
6060

6161
lint:
6262
desc: Run linting tools
63+
deps: [check-dcr-isolation]
6364
cmds:
6465
- golangci-lint run --allow-parallel-runners ./...
6566
- go vet ./...
@@ -69,6 +70,41 @@ tasks:
6970
cmds:
7071
- golangci-lint run --allow-parallel-runners --fix ./...
7172

73+
check-dcr-isolation:
74+
desc: Verify oauthproto.RegisterClientDynamically is not called outside pkg/auth/dcr or pkg/oauthproto
75+
summary: |
76+
Enforces the invariant that the RFC 7591 Dynamic Client Registration
77+
wire-shape primitive (oauthproto.RegisterClientDynamically) is only
78+
called by the shared resolver in pkg/auth/dcr. Direct calls from
79+
elsewhere bypass the resolver's S256 PKCE gating, singleflight
80+
deduplication, expiry-driven refetch, bearer-token transport (with
81+
redirect refusal), and panic recovery — every property added in PR
82+
#5042 — silently weakening every CLI/UI consumer's compliance.
83+
84+
See issue #5145 (parent) and #5219 (this guard's introducing
85+
sub-issue) for the design discussion.
86+
silent: true
87+
cmds:
88+
- |
89+
matches=$(grep -rn 'oauthproto\.RegisterClientDynamically\|\.RegisterClientDynamically(' \
90+
--include='*.go' \
91+
--exclude-dir='vendor' \
92+
--exclude-dir='mocks' \
93+
. 2>/dev/null \
94+
| grep -v '^./pkg/auth/dcr/' \
95+
| grep -v '^./pkg/oauthproto/' \
96+
|| true)
97+
if [ -n "$matches" ]; then
98+
echo "ERROR: oauthproto.RegisterClientDynamically must only be called from pkg/auth/dcr or pkg/oauthproto." >&2
99+
echo " Route the call through pkg/auth/dcr.ResolveCredentials so the consumer inherits" >&2
100+
echo " S256 PKCE gating, singleflight, expiry refetch, redirect refusal, and panic recovery." >&2
101+
echo "" >&2
102+
echo "Offending references:" >&2
103+
echo "$matches" >&2
104+
exit 1
105+
fi
106+
echo "check-dcr-isolation: ok"
107+
72108
test-unixlike:
73109
desc: Run unit tests (excluding e2e tests) on Linux and macOS with race detection
74110
platforms: [linux, darwin]

‎pkg/auth/dcr/request.go‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
package dcr
5+
6+
// Request is the profile-neutral input to ResolveCredentials. Each consumer
7+
// (embedded authserver, CLI OAuth flow) translates its domain types into a
8+
// Request at the call site so the resolver does not import any consumer's
9+
// shapes.
10+
//
11+
// The struct collects exactly the fields the resolver reads:
12+
//
13+
// - Identity: Issuer (this caller's logical issuer, used to key the cache
14+
// and to default RedirectURI when the caller leaves it empty), Scopes,
15+
// and RedirectURI.
16+
// - Endpoint discovery: DiscoveryURL or RegistrationEndpoint, with optional
17+
// explicit AuthorizationEndpoint / TokenEndpoint overrides.
18+
// - Registration metadata: InitialAccessToken, ClientName. The caller is
19+
// responsible for resolving any file-or-env reference (e.g. the embedded
20+
// authserver's InitialAccessTokenFile / InitialAccessTokenEnvVar) into
21+
// InitialAccessToken before constructing a Request.
22+
//
23+
// Mutually exclusive constraints are enforced at validation time:
24+
//
25+
// - Exactly one of DiscoveryURL / RegistrationEndpoint must be non-empty.
26+
// - Issuer must be non-empty.
27+
//
28+
// Constructing a Request is the caller's responsibility; the resolver does
29+
// not clone or mutate it.
30+
type Request struct {
31+
// Issuer is the caller's logical issuer identifier. Used to key the
32+
// cache and (when RedirectURI is empty) to derive a default redirect
33+
// URI of {Issuer}/oauth/callback. Required.
34+
//
35+
// This is *not* the upstream's issuer — that is recovered from
36+
// DiscoveryURL inside the resolver for RFC 8414 §3.3 verification.
37+
Issuer string
38+
39+
// RedirectURI is the redirect URI to register with the upstream. When
40+
// empty, the resolver derives {Issuer}/oauth/callback. HTTPS is required
41+
// except for loopback hosts (RFC 8252 §7.3 — the CLI flow uses
42+
// http://localhost:{port}/callback).
43+
RedirectURI string
44+
45+
// Scopes are the OAuth scopes to request in the registration body.
46+
// May be empty; the resolver will fall back to discovered scopes if the
47+
// upstream advertises any.
48+
Scopes []string
49+
50+
// DiscoveryURL points at the upstream's RFC 8414 / OIDC Discovery
51+
// document. The resolver fetches this URL exactly once and reads
52+
// registration_endpoint, authorization_endpoint, token_endpoint,
53+
// token_endpoint_auth_methods_supported, scopes_supported, and
54+
// code_challenge_methods_supported from it.
55+
//
56+
// Mutually exclusive with RegistrationEndpoint.
57+
DiscoveryURL string
58+
59+
// RegistrationEndpoint is used directly when set, bypassing discovery.
60+
// On this branch the caller is expected to also supply AuthorizationEndpoint
61+
// and TokenEndpoint explicitly; the resolver's auth-method selection
62+
// defaults to client_secret_basic because no server-capability fields
63+
// are available.
64+
//
65+
// Mutually exclusive with DiscoveryURL.
66+
RegistrationEndpoint string
67+
68+
// AuthorizationEndpoint, when non-empty, overrides any value discovered
69+
// via DiscoveryURL. Explicit caller configuration always wins.
70+
AuthorizationEndpoint string
71+
72+
// TokenEndpoint, when non-empty, overrides any value discovered via
73+
// DiscoveryURL. Explicit caller configuration always wins.
74+
TokenEndpoint string
75+
76+
// InitialAccessToken is the RFC 7591 initial access token presented to
77+
// the registration endpoint as a Bearer header. The caller resolves any
78+
// file-or-env reference before populating this field.
79+
InitialAccessToken string
80+
81+
// ClientName is sent as the RFC 7591 "client_name" registration
82+
// metadata. When empty, the resolver uses
83+
// oauthproto.ToolHiveMCPClientName.
84+
ClientName string
85+
86+
// PublicClient, when true, instructs the resolver to register as a
87+
// public PKCE client (token_endpoint_auth_method=none) regardless of
88+
// what other methods the upstream advertises. This matches the CLI
89+
// flow's intent (RFC 8252 §8.4 native public clients) and the
90+
// resolver still enforces the RFC 7636 / OAuth 2.1 S256 PKCE gate:
91+
// when the upstream's discovery metadata does not advertise S256 in
92+
// code_challenge_methods_supported, the registration is refused with
93+
// a clear error rather than silently downgrading.
94+
//
95+
// When false (the embedded-authserver default), the resolver picks
96+
// the strongest auth method the upstream advertises (private_key_jwt
97+
// > client_secret_basic > client_secret_post > none, with the same
98+
// S256 gate on "none").
99+
//
100+
// Has no effect on the RegistrationEndpoint-direct branch when the
101+
// caller has not also supplied a DiscoveryURL: without
102+
// code_challenge_methods_supported the S256 gate cannot be evaluated,
103+
// so the resolver refuses to register as a public client.
104+
PublicClient bool
105+
}

0 commit comments

Comments
 (0)