diff --git a/pkg/auth/dcr/request.go b/pkg/auth/dcr/request.go new file mode 100644 index 0000000000..da6471e935 --- /dev/null +++ b/pkg/auth/dcr/request.go @@ -0,0 +1,125 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package dcr + +// Request is the profile-neutral input to ResolveCredentials. Each consumer +// (embedded authserver, CLI OAuth flow) translates its domain types into a +// Request at the call site so the resolver does not import any consumer's +// shapes. +// +// The struct collects exactly the fields the resolver reads: +// +// - Identity: Issuer (the caller's logical scope — see the Issuer field +// doc for what each consumer puts there), Scopes, and RedirectURI. +// - Endpoint discovery: DiscoveryURL or RegistrationEndpoint, with optional +// explicit AuthorizationEndpoint / TokenEndpoint overrides. +// - Registration metadata: InitialAccessToken, ClientName. The caller is +// responsible for resolving any file-or-env reference (e.g. the embedded +// authserver's InitialAccessTokenFile / InitialAccessTokenEnvVar) into +// InitialAccessToken before constructing a Request. +// +// Mutually exclusive constraints are enforced at validation time: +// +// - Exactly one of DiscoveryURL / RegistrationEndpoint must be non-empty. +// - Issuer must be non-empty. +// +// Constructing a Request is the caller's responsibility; the resolver does +// not clone or mutate it. +type Request struct { + // Issuer is the caller's logical scope for cache keying and (when + // RedirectURI is empty) the basis for the default redirect URI + // ({Issuer}/oauth/callback). Required. + // + // What each consumer puts here: + // + // - Embedded authserver: its own issuer identifier. The authserver + // owns a distinct logical identity from the upstream IdP, and + // defaulting RedirectURI to {Issuer}/oauth/callback lands the + // callback on the authserver's origin (correct). + // - CLI OAuth flow: the upstream OAuth provider's issuer URL, + // because the CLI has no separate logical issuer of its own. The + // CLI always supplies an explicit loopback RedirectURI per + // RFC 8252 §7.3, so the redirect-URI defaulting is never + // exercised on this path. + // + // The resolver's cache key is (Issuer, RedirectURI, ScopesHash); the + // two consumers do not collide on the key because the embedded + // authserver's RedirectURI lives on the authserver's origin and the + // CLI's lives on a loopback (RFC 8252 §7.3) — even when Issuer + // happens to match between the two profiles, the RedirectURI + // component keeps the cache key distinct. See the cross-consumer + // caveat on dcrFlight in resolver.go for the wider invariant. + // + // Issuer is *not* used for RFC 8414 §3.3 metadata verification — + // that uses an issuer recovered from DiscoveryURL inside the + // resolver. + Issuer string + + // RedirectURI is the redirect URI to register with the upstream. When + // empty, the resolver derives {Issuer}/oauth/callback. HTTPS is required + // except for loopback hosts (RFC 8252 §7.3 — the CLI flow uses + // http://localhost:{port}/callback). + RedirectURI string + + // Scopes are the OAuth scopes to request in the registration body. + // May be empty; the resolver will fall back to discovered scopes if the + // upstream advertises any. + Scopes []string + + // DiscoveryURL points at the upstream's RFC 8414 / OIDC Discovery + // document. The resolver fetches this URL exactly once and reads + // registration_endpoint, authorization_endpoint, token_endpoint, + // token_endpoint_auth_methods_supported, scopes_supported, and + // code_challenge_methods_supported from it. + // + // Mutually exclusive with RegistrationEndpoint. + DiscoveryURL string + + // RegistrationEndpoint is used directly when set, bypassing discovery. + // On this branch the caller is expected to also supply AuthorizationEndpoint + // and TokenEndpoint explicitly; the resolver's auth-method selection + // defaults to client_secret_basic because no server-capability fields + // are available. + // + // Mutually exclusive with DiscoveryURL. + RegistrationEndpoint string + + // AuthorizationEndpoint, when non-empty, overrides any value discovered + // via DiscoveryURL. Explicit caller configuration always wins. + AuthorizationEndpoint string + + // TokenEndpoint, when non-empty, overrides any value discovered via + // DiscoveryURL. Explicit caller configuration always wins. + TokenEndpoint string + + // InitialAccessToken is the RFC 7591 initial access token presented to + // the registration endpoint as a Bearer header. The caller resolves any + // file-or-env reference before populating this field. + InitialAccessToken string + + // ClientName is sent as the RFC 7591 "client_name" registration + // metadata. When empty, the resolver uses + // oauthproto.ToolHiveMCPClientName. + ClientName string + + // PublicClient, when true, instructs the resolver to register as a + // public PKCE client (token_endpoint_auth_method=none) regardless of + // what other methods the upstream advertises. This matches the CLI + // flow's intent (RFC 8252 §8.4 native public clients) and the + // resolver still enforces the RFC 7636 / OAuth 2.1 S256 PKCE gate: + // when the upstream's discovery metadata does not advertise S256 in + // code_challenge_methods_supported, the registration is refused with + // a clear error rather than silently downgrading. + // + // When false (the embedded-authserver default), the resolver picks + // the strongest auth method the upstream advertises (private_key_jwt + // > client_secret_basic > client_secret_post > none, with the same + // S256 gate on "none"). + // + // Has no effect on the RegistrationEndpoint-direct branch when the + // caller has not also supplied a DiscoveryURL: without + // code_challenge_methods_supported the S256 gate cannot be evaluated, + // so the resolver refuses to register as a public client. + PublicClient bool +} diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index 59e57c5532..d74742df42 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -11,6 +11,26 @@ // the registration body. Stateless RFC 7591 wire-shape primitives live in // pkg/oauthproto. // +// # API surface +// +// ResolveCredentials takes a profile-neutral Request and a CredentialStore +// and returns a Resolution. The Request carries only the fields the resolver +// actually reads (issuer, redirect URI, scopes, discovery URL or registration +// endpoint, optional explicit endpoint overrides, optional initial access +// token, optional registration metadata); each consumer translates its +// domain types into a Request at the call site so the resolver does not +// import any consumer's shapes. +// +// Two consumers exist today: +// +// - pkg/authserver/runner constructs a Request from +// *authserver.OAuth2UpstreamRunConfig and uses its own adapter helpers +// (needsDCR / consumeResolution / applyResolutionToOAuth2Config in +// runner/dcr_adapter.go) to fold the resolution back into its +// run-config and built upstream.OAuth2Config. +// - pkg/auth/discovery constructs a Request from *OAuthFlowConfig for the +// CLI OAuth flow and copies the returned Resolution onto OAuthFlowResult. +// // # Concurrency // // The package maintains a process-global singleflight keyed on the tuple @@ -21,38 +41,17 @@ // callers cannot assume per-call-site flight isolation. See the dcrFlight // doc comment below for the rationale. // -// # Current API coupling — sub-issue 4a only -// -// As of issue #5145 sub-issue 4a (the slice that lifted this code out of -// pkg/authserver/runner), the public functions on this package take -// embedded-authserver types — *authserver.OAuth2UpstreamRunConfig and -// *upstream.OAuth2Config (with *authserver.DCRUpstreamConfig reached -// transitively via OAuth2UpstreamRunConfig.DCRConfig) — directly on -// their signatures. This matches the embedded authserver's existing -// internal shapes verbatim and was the cheapest move-only change. -// -// The CLI flow migration in sub-issue 4b will introduce the second -// consumer (pkg/auth/discovery::PerformOAuthFlow) and is the right -// trigger for replacing those parameters with a profile-neutral input -// type — designing the neutral shape now, with only one consumer in -// hand, would be speculative. Until 4b lands, callers outside the -// embedded authserver MUST adapt their inputs to the authserver types -// at the call site, and the "profile-agnostic" framing in this package's -// charter is a target state, not the current state of the API. -// // See issue #5145 for the design discussion that motivated lifting this out // of pkg/authserver/runner. package dcr import ( - "bytes" "context" "errors" "fmt" "log/slog" "net/http" "net/url" - "os" "regexp" "runtime/debug" "slices" @@ -61,9 +60,7 @@ import ( "golang.org/x/sync/singleflight" - "github.com/stacklok/toolhive/pkg/authserver" "github.com/stacklok/toolhive/pkg/authserver/storage" - "github.com/stacklok/toolhive/pkg/authserver/upstream" "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" ) @@ -89,21 +86,17 @@ import ( // reuse. A Redis-backed store still wants this in-process gate so a // single replica does not double-register against itself. // -// Cross-consumer caveat (matters once issue #5145 sub-issue 4b (#5219) -// lands the CLI flow as the second consumer): because dcrFlight is -// package-global, two consumers that happen to construct identical Keys -// (same issuer, same redirect URI, same scopes hash) will share a single -// in-flight registration even if they semantically want different -// client profiles. The current call sites do not collide — the embedded -// authserver's redirect URI lives on the AS origin, the CLI flow's lives -// on a loopback — but a future consumer that defaults its redirect URI -// into either of those spaces would silently coalesce. -// -// TODO(#5219): the flight key must gain a consumer-identifier component -// when 4b wires the second consumer. Today the colliding-Key risk is -// theoretical; once two profiles share this group it becomes a -// correctness hazard the resolver itself must defend against. Track -// resolution against the 4b PR's design discussion. +// Cross-consumer caveat: because dcrFlight is package-global, two +// consumers that happen to construct identical Keys (same issuer, same +// redirect URI, same scopes hash) will share a single in-flight +// registration even if they semantically want different client profiles. +// The two current call sites do not collide by construction — the embedded +// authserver's redirect URI lives on the AS origin, and the CLI flow's +// redirect URI lives on a loopback (http://localhost:{port}/callback per +// RFC 8252 §7.3). A future consumer that defaults its redirect URI into +// either of those spaces would silently coalesce; if a third profile is +// added the flight key MUST gain a consumer-identifier component so a +// collision is impossible rather than improbable. var dcrFlight singleflight.Group // flightKeyOf canonicalises a Key into the singleflight string used by @@ -143,7 +136,10 @@ var authMethodPreference = []string{ // upstream advertises so the caller need not re-discover them. // // The struct is the unit of storage in CredentialStore and the unit of -// application via ConsumeResolution. +// application that consumers project back into their own domain types +// (e.g. consumeResolution / applyResolutionToOAuth2Config in +// pkg/authserver/runner/dcr_adapter.go, or direct OAuthFlowResult field +// writes in pkg/auth/discovery). // // MUST update both converters (resolutionToCredentials and // credentialsToResolution in store.go) when adding, renaming, or @@ -185,12 +181,12 @@ type Resolution struct { TokenEndpointAuthMethod string // RedirectURI is the redirect URI presented to the authorization server - // during registration. When the caller's run-config did not specify one, + // during registration. When the caller's Request did not specify one, // this holds the defaulted value derived from the issuer + /oauth/callback // (via resolveUpstreamRedirectURI). Persisting it on the resolution lets - // ConsumeResolution write it back onto the run-config COPY so that - // downstream consumers (buildPureOAuth2Config, upstream.OAuth2Config - // validation) see a non-empty RedirectURI. + // consumers (pkg/authserver/runner.consumeResolution) write it back onto + // the run-config COPY so that downstream code (buildPureOAuth2Config, + // upstream.OAuth2Config validation) sees a non-empty RedirectURI. RedirectURI string // ClientIDIssuedAt is the RFC 7591 §3.2.1 "client_id_issued_at" value @@ -218,96 +214,6 @@ type Resolution struct { CreatedAt time.Time } -// NeedsDCR reports whether rc requires runtime Dynamic Client Registration. -// A run-config needs DCR exactly when ClientID is empty and DCRConfig is -// non-nil (the mutually-exclusive constraint is enforced by -// OAuth2UpstreamRunConfig.Validate; this helper is a convenience check). -func NeedsDCR(rc *authserver.OAuth2UpstreamRunConfig) bool { - if rc == nil { - return false - } - return rc.ClientID == "" && rc.DCRConfig != nil -} - -// ConsumeResolution copies resolved credentials and endpoints from res into -// rc and consumes rc.DCRConfig (sets it to nil), transitioning the run- -// config copy from "DCR-pending" (ClientID == "" && DCRConfig != nil) to -// "DCR-resolved" (ClientID populated && DCRConfig == nil). The "consume" -// name is deliberate: a second call after the first is a no-op only -// because the first cleared DCRConfig — this is a one-shot state -// transition, not an idempotent default-fill. -// -// Callers must pass a COPY of the upstream run-config so the caller's -// original is unaffected; ConsumeResolution does not clone rc internally. -// -// Why DCRConfig is cleared: OAuth2UpstreamRunConfig.Validate enforces -// ClientID xor DCRConfig — a resolved copy that left DCRConfig set would -// fail the validator that runs downstream in buildPureOAuth2Config. The -// caller's *original* OAuth2Config is unaffected because -// buildUpstreamConfigs deep-copies before resolution; only the post- -// resolution copy is mutated here. -// -// ClientID, the endpoints, and RedirectURI are written only when rc leaves -// them empty — explicit caller configuration always wins. The conditional -// ClientID write is defence-in-depth against future call sites that bypass -// the resolver's validateResolveInputs precondition (which enforces -// ClientID == "" up front); an unconditional overwrite would silently -// clobber a pre-provisioned ClientID with no error. -// -// The defaulted RedirectURI write closes the loop on resolver-side defaulting: -// when the caller's run-config left RedirectURI empty, resolveUpstreamRedirectURI -// derived issuer + /oauth/callback and persisted it on the resolution; copying -// it back here means the downstream upstream.OAuth2Config has a non-empty -// RedirectURI, which authserver.Config validation requires. -// -// Note on ClientSecret: ConsumeResolution does NOT write the resolved -// secret to rc because OAuth2UpstreamRunConfig models secrets as file-or- -// env references only. To propagate the DCR-resolved secret into the -// final upstream.OAuth2Config, callers must pair this call with -// ApplyResolutionToOAuth2Config once the config has been built. Keeping -// the two helpers side-by-side localises the DCR-specific application -// logic. -func ConsumeResolution(rc *authserver.OAuth2UpstreamRunConfig, res *Resolution) { - if rc == nil || res == nil { - return - } - if rc.ClientID == "" { - rc.ClientID = res.ClientID - } - rc.DCRConfig = nil - if rc.AuthorizationEndpoint == "" { - rc.AuthorizationEndpoint = res.AuthorizationEndpoint - } - if rc.TokenEndpoint == "" { - rc.TokenEndpoint = res.TokenEndpoint - } - if rc.RedirectURI == "" { - rc.RedirectURI = res.RedirectURI - } -} - -// ApplyResolutionToOAuth2Config overlays the DCR-resolved ClientSecret onto -// a built *upstream.OAuth2Config. This is the companion to -// ConsumeResolution: where that function writes fields representable in -// the file-or-env run-config model, this one writes the inline-only -// ClientSecret directly on the runtime config. -// -// The split exists because buildPureOAuth2Config intentionally retains a -// narrow file-or-env contract (no DCR awareness) and because OAuth2's -// ClientSecret on the run-config is modelled as a reference rather than -// an inline string. Any future output path from OAuth2UpstreamRunConfig -// to upstream.OAuth2Config must call BOTH ConsumeResolution (run-config -// side) AND ApplyResolutionToOAuth2Config (built-config side) to get a -// fully-resolved DCR client. Forgetting the second call leaves -// ClientSecret empty and produces silent auth failures at request time — -// the type system does not enforce the pair, so the invariant lives here. -func ApplyResolutionToOAuth2Config(cfg *upstream.OAuth2Config, res *Resolution) { - if cfg == nil || res == nil { - return - } - cfg.ClientSecret = res.ClientSecret -} - // Step identifiers for structured error logs emitted by the caller of // ResolveCredentials. These values flow through the "step" attribute so // operators can narrow failures to a specific phase without parsing error @@ -364,25 +270,20 @@ func newDCRStepError(step, issuer, redirectURI string, err error) *dcrStepError } } -// ResolveCredentials performs Dynamic Client Registration for rc against -// the upstream authorization server identified by rc.DCRConfig, caching the -// resulting credentials in cache. On cache hit the resolver returns -// immediately without any network I/O. -// -// rc must have ClientID == "" and DCRConfig != nil — the caller is expected -// to have validated this via OAuth2UpstreamRunConfig.Validate. +// ResolveCredentials performs Dynamic Client Registration for req against +// the upstream authorization server identified by req.DiscoveryURL or +// req.RegistrationEndpoint, caching the resulting credentials in cache. +// On cache hit the resolver returns immediately without any network I/O. // -// localIssuer is *this* auth server's issuer identifier, NOT the upstream's. +// req.Issuer is *this caller's* logical issuer identifier, NOT the upstream's. // It is used to key the cache and to default the redirect URI to -// {localIssuer}/oauth/callback when rc.RedirectURI is empty. The upstream's -// issuer is recovered separately from rc.DCRConfig.DiscoveryURL inside the -// resolver and is used solely for RFC 8414 §3.3 metadata verification. -// Passing the upstream's issuer here would produce a wrong-origin default -// redirect and a cache key that does not identify the auth-server context. +// {req.Issuer}/oauth/callback when req.RedirectURI is empty. The upstream's +// issuer is recovered separately from req.DiscoveryURL inside the resolver +// and is used solely for RFC 8414 §3.3 metadata verification. Passing the +// upstream's issuer in req.Issuer would produce a wrong-origin default +// redirect and a cache key that does not identify the caller context. // -// The caller is responsible for applying the returned resolution onto a COPY -// of rc via ConsumeResolution (per the copy-before-mutate rule). This function -// neither mutates rc nor the cache on failure. +// The resolver does not mutate req or the cache on failure. // // Observability: this function never calls slog.Error directly — all // failures are annotated with a *dcrStepError and returned to the caller, @@ -395,30 +296,33 @@ func newDCRStepError(step, issuer, redirectURI string, err error) *dcrStepError // public metadata such as client_id and redirect_uri. func ResolveCredentials( ctx context.Context, - rc *authserver.OAuth2UpstreamRunConfig, - localIssuer string, + req *Request, cache CredentialStore, ) (*Resolution, error) { - if err := validateResolveInputs(rc, localIssuer, cache); err != nil { - return nil, newDCRStepError(dcrStepValidate, localIssuer, "", err) + if err := validateResolveInputs(req, cache); err != nil { + issuer := "" + if req != nil { + issuer = req.Issuer + } + return nil, newDCRStepError(dcrStepValidate, issuer, "", err) } - redirectURI, err := resolveUpstreamRedirectURI(rc.RedirectURI, localIssuer) + redirectURI, err := resolveUpstreamRedirectURI(req.RedirectURI, req.Issuer) if err != nil { - return nil, newDCRStepError(dcrStepResolveRedirect, localIssuer, "", + return nil, newDCRStepError(dcrStepResolveRedirect, req.Issuer, "", fmt.Errorf("resolve redirect uri: %w", err)) } - scopes := slices.Clone(rc.Scopes) + scopes := slices.Clone(req.Scopes) key := Key{ - Issuer: localIssuer, + Issuer: req.Issuer, RedirectURI: redirectURI, ScopesHash: storage.ScopesHash(scopes), } // Cache lookup short-circuits before any network I/O. - if cached, hit, err := lookupCachedResolution(ctx, cache, key, localIssuer, redirectURI); err != nil { - return nil, newDCRStepError(dcrStepCacheRead, localIssuer, redirectURI, err) + if cached, hit, err := lookupCachedResolution(ctx, cache, key, req.Issuer, redirectURI); err != nil { + return nil, newDCRStepError(dcrStepCacheRead, req.Issuer, redirectURI, err) } else if hit { return cached, nil } @@ -441,14 +345,14 @@ func ResolveCredentials( resolutionAny, err, _ := dcrFlight.Do(flightKey, func() (res any, err error) { defer func() { if r := recover(); r != nil { - stepErr := newDCRStepError(dcrStepRegister, localIssuer, redirectURI, + stepErr := newDCRStepError(dcrStepRegister, req.Issuer, redirectURI, fmt.Errorf("registration panicked: %v", r)) stepErr.Stack = string(debug.Stack()) err = stepErr res = nil } }() - return registerAndCache(ctx, rc, localIssuer, redirectURI, scopes, key, cache) + return registerAndCache(ctx, req, redirectURI, scopes, key, cache) }) if err != nil { return nil, err @@ -463,49 +367,53 @@ func ResolveCredentials( // the durable Put live here. func registerAndCache( ctx context.Context, - rc *authserver.OAuth2UpstreamRunConfig, - localIssuer, redirectURI string, + req *Request, + redirectURI string, scopes []string, key Key, cache CredentialStore, ) (*Resolution, error) { // Recheck cache: another flight that just finished may have populated // it between our initial lookup and our singleflight entry. - if cached, hit, err := lookupCachedResolution(ctx, cache, key, localIssuer, redirectURI); err != nil { - return nil, newDCRStepError(dcrStepCacheRead, localIssuer, redirectURI, err) + if cached, hit, err := lookupCachedResolution(ctx, cache, key, req.Issuer, redirectURI); err != nil { + return nil, newDCRStepError(dcrStepCacheRead, req.Issuer, redirectURI, err) } else if hit { return cached, nil } // Endpoint resolution: discover metadata when configured, otherwise use // the caller-supplied RegistrationEndpoint directly. The upstream's - // expected issuer is recovered from cfg.DiscoveryURL inside the helper. - // localIssuer here is *this* auth server's issuer — correct for cache - // keying and redirect URI defaulting, but it must not be used for - // RFC 8414 §3.3 metadata verification (which is the upstream's concern). - endpoints, err := resolveDCREndpoints(ctx, rc.DCRConfig) + // expected issuer is recovered from req.DiscoveryURL inside the helper. + // req.Issuer here is *this caller's* issuer — correct for cache keying + // and redirect URI defaulting, but it must not be used for RFC 8414 + // §3.3 metadata verification (which is the upstream's concern). + endpoints, err := resolveDCREndpoints(ctx, req) if err != nil { - return nil, newDCRStepError(dcrStepMetadata, localIssuer, redirectURI, err) + return nil, newDCRStepError(dcrStepMetadata, req.Issuer, redirectURI, err) } - applyExplicitEndpointOverrides(endpoints, rc) + applyExplicitEndpointOverrides(endpoints, req) // Token-endpoint auth method: intersect server support with our // preference order; default to client_secret_basic if the server does - // not advertise the field at all. + // not advertise the field at all. When the caller declared the + // registration is for a public PKCE client (CLI flow), force + // token_endpoint_auth_method=none while still enforcing the S256 + // PKCE gate. authMethod, err := selectTokenEndpointAuthMethod( endpoints.tokenEndpointAuthMethodsSupported, endpoints.codeChallengeMethodsSupported, + req.PublicClient, ) if err != nil { - return nil, newDCRStepError(dcrStepSelectAuthMethod, localIssuer, redirectURI, err) + return nil, newDCRStepError(dcrStepSelectAuthMethod, req.Issuer, redirectURI, err) } - registrationScopes := chooseRegistrationScopes(scopes, endpoints.scopesSupported, localIssuer) + registrationScopes := chooseRegistrationScopes(scopes, endpoints.scopesSupported, req.Issuer) - response, err := performRegistration(ctx, rc.DCRConfig, endpoints.registrationEndpoint, + response, err := performRegistration(ctx, req, endpoints.registrationEndpoint, redirectURI, authMethod, registrationScopes) if err != nil { - return nil, newDCRStepError(dcrStepRegister, localIssuer, redirectURI, err) + return nil, newDCRStepError(dcrStepRegister, req.Issuer, redirectURI, err) } resolution := buildResolution(response, endpoints, authMethod, redirectURI) @@ -515,13 +423,13 @@ func registerAndCache( // next call simply re-resolves rather than reading a value the cache // never saw. if err := cache.Put(ctx, key, resolution); err != nil { - return nil, newDCRStepError(dcrStepCacheWrite, localIssuer, redirectURI, + return nil, newDCRStepError(dcrStepCacheWrite, req.Issuer, redirectURI, fmt.Errorf("cache put: %w", err)) } //nolint:gosec // G706: client_id is public metadata per RFC 7591. slog.Debug("dcr: registered new client", - "local_issuer", localIssuer, + "local_issuer", req.Issuer, "redirect_uri", redirectURI, "client_id", resolution.ClientID, ) @@ -669,25 +577,25 @@ var queryStrippingPattern = regexp.MustCompile(`(?i)https?://[^\s"']+`) // ----------------------------------------------------------------------------- // validateResolveInputs performs the defensive re-check of resolver -// preconditions. Validate() enforces most of these at config-load time, but -// ResolveCredentials is an entry point that programmatic callers can -// reach with partially-constructed run-configs. +// preconditions. Each consumer is expected to validate its own domain +// types upstream of the resolver; this is the final-line check for an +// entry point that programmatic callers can reach with partially- +// constructed Requests. func validateResolveInputs( - rc *authserver.OAuth2UpstreamRunConfig, - localIssuer string, + req *Request, cache CredentialStore, ) error { - if rc == nil { - return fmt.Errorf("oauth2 upstream run-config is required") + if req == nil { + return fmt.Errorf("dcr: request is required") } - if rc.ClientID != "" { - return fmt.Errorf("dcr: oauth2 upstream has a pre-provisioned client_id") + if req.Issuer == "" { + return fmt.Errorf("dcr: issuer is required") } - if rc.DCRConfig == nil { - return fmt.Errorf("dcr: oauth2 upstream has no dcr_config") + if req.DiscoveryURL == "" && req.RegistrationEndpoint == "" { + return fmt.Errorf("dcr: request must set either discovery_url or registration_endpoint") } - if localIssuer == "" { - return fmt.Errorf("dcr: issuer is required") + if req.DiscoveryURL != "" && req.RegistrationEndpoint != "" { + return fmt.Errorf("dcr: discovery_url and registration_endpoint are mutually exclusive") } if cache == nil { return fmt.Errorf("dcr: credential store is required") @@ -771,14 +679,14 @@ func lookupCachedResolution( // applyExplicitEndpointOverrides overwrites the discovered // authorizationEndpoint / tokenEndpoint in endpoints with explicit values -// from rc when rc specifies them. Explicit caller configuration always wins -// over discovery. -func applyExplicitEndpointOverrides(endpoints *dcrEndpoints, rc *authserver.OAuth2UpstreamRunConfig) { - if rc.AuthorizationEndpoint != "" { - endpoints.authorizationEndpoint = rc.AuthorizationEndpoint +// from req when req specifies them. Explicit caller configuration always +// wins over discovery. +func applyExplicitEndpointOverrides(endpoints *dcrEndpoints, req *Request) { + if req.AuthorizationEndpoint != "" { + endpoints.authorizationEndpoint = req.AuthorizationEndpoint } - if rc.TokenEndpoint != "" { - endpoints.tokenEndpoint = rc.TokenEndpoint + if req.TokenEndpoint != "" { + endpoints.tokenEndpoint = req.TokenEndpoint } } @@ -801,24 +709,27 @@ func chooseRegistrationScopes(explicit, discovered []string, localIssuer string) // performRegistration executes the HTTP registration request exactly once. // The initial access token (if configured) is injected as a // bearer-token Authorization header via a wrapping http.Client. +// +// The caller is responsible for resolving any file-or-env initial access +// token reference into req.InitialAccessToken before calling +// ResolveCredentials; the resolver does not touch the filesystem or +// environment. func performRegistration( ctx context.Context, - dcrCfg *authserver.DCRUpstreamConfig, + req *Request, registrationEndpoint, redirectURI, authMethod string, scopes []string, ) (*oauthproto.DynamicClientRegistrationResponse, error) { - // Initial access token is optional; resolveSecret returns ("", nil) - // when neither file nor env var is configured. - initialAccessToken, err := resolveSecret(dcrCfg.InitialAccessTokenFile, dcrCfg.InitialAccessTokenEnvVar) - if err != nil { - return nil, fmt.Errorf("dcr: resolve initial access token: %w", err) - } + httpClient := newDCRHTTPClient(req.InitialAccessToken) - httpClient := newDCRHTTPClient(initialAccessToken) + clientName := req.ClientName + if clientName == "" { + clientName = oauthproto.ToolHiveMCPClientName + } - request := &oauthproto.DynamicClientRegistrationRequest{ + registrationRequest := &oauthproto.DynamicClientRegistrationRequest{ RedirectURIs: []string{redirectURI}, - ClientName: oauthproto.ToolHiveMCPClientName, + ClientName: clientName, TokenEndpointAuthMethod: authMethod, GrantTypes: []string{oauthproto.GrantTypeAuthorizationCode, oauthproto.GrantTypeRefreshToken}, ResponseTypes: []string{oauthproto.ResponseTypeCode}, @@ -827,7 +738,7 @@ func performRegistration( // Call exactly once — no retry loop. Step 2g will add retry/backoff at a // higher layer if needed. - response, err := oauthproto.RegisterClientDynamically(ctx, registrationEndpoint, request, httpClient) + response, err := oauthproto.RegisterClientDynamically(ctx, registrationEndpoint, registrationRequest, httpClient) if err != nil { return nil, fmt.Errorf("dcr: register client: %w", err) } @@ -840,7 +751,8 @@ func performRegistration( // recorded so downstream consumers see a definite value. redirectURI is the // value passed to the registration endpoint (caller-supplied or defaulted // via resolveUpstreamRedirectURI); it is persisted on the resolution so -// ConsumeResolution can propagate a defaulted value back to the run-config. +// consumers (pkg/authserver/runner.consumeResolution) can propagate a +// defaulted value back to the run-config. // // RFC 7591 §3.2.1 client_id_issued_at and client_secret_expires_at are // converted from int64 epoch seconds to time.Time. The wire value 0 means @@ -899,60 +811,54 @@ type dcrEndpoints struct { codeChallengeMethodsSupported []string } -// resolveDCREndpoints produces the endpoint bundle from the DCRUpstreamConfig. +// resolveDCREndpoints produces the endpoint bundle from req. // -// Three branches, in priority order: +// Two branches, in priority order: // -// 1. cfg.RegistrationEndpoint set — use it directly and skip discovery +// 1. req.RegistrationEndpoint set — use it directly and skip discovery // entirely. Server-capability fields (token_endpoint_auth_methods_supported, // scopes_supported) are unavailable on this branch; the caller is // expected to also supply AuthorizationEndpoint, TokenEndpoint, and an // explicit Scopes list. Auth method falls back to the // selectTokenEndpointAuthMethod default. -// 2. cfg.DiscoveryURL set — fetch the exact document the operator +// 2. req.DiscoveryURL set — fetch the exact document the operator // configured (bypassing the well-known path fallback). RFC 8414 §3.3 // requires the metadata's "issuer" field to match the authorization // server's issuer identifier; that identifier is the upstream's, not -// this auth server's, so it is recovered from the discovery URL via -// deriveExpectedIssuerFromDiscoveryURL rather than reusing the -// caller-supplied issuer (which names this auth server and is used -// elsewhere in ResolveCredentials for redirect URI defaulting and -// cache keying). -// 3. Neither set — defensive; Validate() rejects this configuration, but -// as a programmatic entry point the resolver returns an error rather -// than falling back to an unexpected strategy. +// this caller's, so it is recovered from the discovery URL via +// deriveExpectedIssuerFromDiscoveryURL rather than reusing +// req.Issuer (which names this caller and is used elsewhere in +// ResolveCredentials for redirect URI defaulting and cache keying). +// +// validateResolveInputs enforces that exactly one of the two is set, so +// this function does not re-check the both-empty case. // // When metadata is returned but omits registration_endpoint, the resolver // synthesises {origin}/register — a convention used by nanobot and Hydra // for providers that ship DCR without advertising it in discovery. Origin -// is taken from the upstream issuer, not this auth server's issuer, so the +// is taken from the upstream issuer, not this caller's issuer, so the // synthesised endpoint lands at the upstream. func resolveDCREndpoints( ctx context.Context, - cfg *authserver.DCRUpstreamConfig, + req *Request, ) (*dcrEndpoints, error) { - if cfg.RegistrationEndpoint != "" { + if req.RegistrationEndpoint != "" { // Validate locally so a non-HTTPS or malformed URL fails before // performRegistration constructs a bearer-token transport for it. - if err := validateUpstreamEndpointURL(cfg.RegistrationEndpoint, "registration_endpoint"); err != nil { + if err := validateUpstreamEndpointURL(req.RegistrationEndpoint, "registration_endpoint"); err != nil { return nil, fmt.Errorf("dcr: %w", err) } return &dcrEndpoints{ - registrationEndpoint: cfg.RegistrationEndpoint, + registrationEndpoint: req.RegistrationEndpoint, }, nil } - if cfg.DiscoveryURL == "" { - return nil, fmt.Errorf( - "dcr: dcr_config must set either discovery_url or registration_endpoint") - } - - upstreamIssuer, err := deriveExpectedIssuerFromDiscoveryURL(cfg.DiscoveryURL) + upstreamIssuer, err := deriveExpectedIssuerFromDiscoveryURL(req.DiscoveryURL) if err != nil { return nil, err } - metadata, err := oauthproto.FetchAuthorizationServerMetadataFromURL(ctx, cfg.DiscoveryURL, upstreamIssuer, nil) + metadata, err := oauthproto.FetchAuthorizationServerMetadataFromURL(ctx, req.DiscoveryURL, upstreamIssuer, nil) return endpointsFromMetadata(metadata, err, upstreamIssuer) } @@ -1183,7 +1089,11 @@ func validateUpstreamEndpointURL(rawURL, label string) error { // selectTokenEndpointAuthMethod returns the preferred token endpoint auth // method given the server's advertised set, intersected with our preference // order. When the server does not advertise any methods the caller's default -// of client_secret_basic is used (RFC 6749 §2.3.1 baseline). +// of client_secret_basic is used (RFC 6749 §2.3.1 baseline) — unless +// publicClient is true, in which case the absence of advertised methods is +// not enough to safely register as a public client and the function returns +// an error (the S256 PKCE gate cannot be evaluated without +// code_challenge_methods_supported). // // PKCE coupling for "none": the public-client method "none" is selected only // when the upstream also advertises S256 in code_challenge_methods_supported. @@ -1194,7 +1104,36 @@ func validateUpstreamEndpointURL(rawURL, label string) error { // and if no other method is mutually supported the function returns an error // so the operator sees a clear failure at boot rather than a silent // downgrade at runtime. -func selectTokenEndpointAuthMethod(serverSupported, codeChallengeMethodsSupported []string) (string, error) { +// +// publicClient: when true (CLI flow), the function returns "none" if and +// only if S256 PKCE is advertised. Any other outcome is an error — the +// caller has declared intent and the resolver must not silently switch to +// a confidential-client method. +func selectTokenEndpointAuthMethod(serverSupported, codeChallengeMethodsSupported []string, publicClient bool) (string, error) { + pkceS256Advertised := slices.Contains(codeChallengeMethodsSupported, oauthproto.PKCEMethodS256) + + if publicClient { + if !pkceS256Advertised { + return "", fmt.Errorf( + "public client requested but upstream does not advertise S256 in "+ + "code_challenge_methods_supported (got %v); refusing to register a "+ + "public client without S256 PKCE per RFC 7636 / OAuth 2.1", + codeChallengeMethodsSupported) + } + // When serverSupported is non-empty, require that it includes "none" + // so we don't claim a method the AS would reject. + if len(serverSupported) > 0 { + if !slices.Contains(serverSupported, "none") { + return "", fmt.Errorf( + "public client requested but upstream does not advertise "+ + "token_endpoint_auth_method=none (got %v); the CLI flow is a "+ + "public PKCE client and cannot register with a confidential method", + serverSupported) + } + } + return "none", nil + } + if len(serverSupported) == 0 { return "client_secret_basic", nil } @@ -1204,8 +1143,6 @@ func selectTokenEndpointAuthMethod(serverSupported, codeChallengeMethodsSupporte supported[m] = struct{}{} } - pkceS256Advertised := slices.Contains(codeChallengeMethodsSupported, oauthproto.PKCEMethodS256) - for _, m := range authMethodPreference { if _, ok := supported[m]; !ok { continue @@ -1293,39 +1230,3 @@ func newDCRHTTPClient(initialAccessToken string) *http.Client { } return client } - -// resolveSecret reads a secret from file or environment variable. File takes -// precedence over env var. Returns an error if file is specified but -// unreadable, or if envVar is specified but not set. Returns empty string with -// no error if neither file nor envVar is specified. -// -// This duplicates the logic in pkg/authserver/runner/embeddedauthserver.go -// because the DCR package is profile-agnostic and must not reach back into -// the runner — but the secret-resolution shape (file-or-env) is the same -// one every consumer needs. There is no production caller in this PR; the -// helper is staged here so sub-issue 4b (#5219) can wire it as part of the -// CLI flow migration, at which point the runner-side twin and this copy -// SHOULD be promoted to a shared helper (e.g. pkg/auth/secretref) with -// both consumers migrated in the same PR — at that point this duplication -// has served its purpose and is the right thing to remove. The parallel -// TestResolveSecret suite in secret_test.go guards against drift between -// the two copies in the meantime. -func resolveSecret(file, envVar string) (string, error) { - if file != "" { - // #nosec G304 - file path is from configuration, not user input - data, err := os.ReadFile(file) - if err != nil { - return "", fmt.Errorf("failed to read secret file %q: %w", file, err) - } - return string(bytes.TrimSpace(data)), nil - } - if envVar != "" { - value := os.Getenv(envVar) - if value == "" { - return "", fmt.Errorf("environment variable %q is not set", envVar) - } - return value, nil - } - slog.Debug("no client secret configured (neither file nor env var specified)") - return "", nil -} diff --git a/pkg/auth/dcr/resolver_test.go b/pkg/auth/dcr/resolver_test.go index d99695b574..27dcebe77f 100644 --- a/pkg/auth/dcr/resolver_test.go +++ b/pkg/auth/dcr/resolver_test.go @@ -21,7 +21,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/stacklok/toolhive/pkg/authserver" "github.com/stacklok/toolhive/pkg/authserver/storage" "github.com/stacklok/toolhive/pkg/oauthproto" ) @@ -159,14 +158,13 @@ func TestResolveDCRCredentials_CacheHitShortCircuits(t *testing.T) { } require.NoError(t, cache.Put(context.Background(), key, preloaded)) - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid", "profile"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/openid-configuration", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid", "profile"}, + DiscoveryURL: issuer + "/.well-known/openid-configuration", } - got, err := ResolveCredentials(context.Background(), rc, issuer, cache) + got, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "preloaded-id", got.ClientID) assert.Equal(t, "preloaded-secret", got.ClientSecret) @@ -190,14 +188,13 @@ func TestResolveDCRCredentials_RegistersOnCacheMiss(t *testing.T) { cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid", "profile"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid", "profile"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "test-client-id", res.ClientID) assert.Equal(t, "test-client-secret", res.ClientSecret) @@ -211,10 +208,10 @@ func TestResolveDCRCredentials_RegistersOnCacheMiss(t *testing.T) { assert.Empty(t, gotAuthHeader) // Verify the request body carried the expected fields. - var req oauthproto.DynamicClientRegistrationRequest - require.NoError(t, json.Unmarshal(gotBody, &req)) - assert.Equal(t, []string{issuer + "/oauth/callback"}, req.RedirectURIs) - assert.ElementsMatch(t, []string{"openid", "profile"}, []string(req.Scopes)) + var dcrReq oauthproto.DynamicClientRegistrationRequest + require.NoError(t, json.Unmarshal(gotBody, &dcrReq)) + assert.Equal(t, []string{issuer + "/oauth/callback"}, dcrReq.RedirectURIs) + assert.ElementsMatch(t, []string{"openid", "profile"}, []string(dcrReq.Scopes)) // Cache was populated. cached, ok, err := cache.Get(context.Background(), @@ -231,16 +228,15 @@ func TestResolveDCRCredentials_ExplicitEndpointsOverride(t *testing.T) { cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ + req := &Request{ + Issuer: issuer, AuthorizationEndpoint: "https://explicit.example.com/authorize", TokenEndpoint: "https://explicit.example.com/token", Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "https://explicit.example.com/authorize", res.AuthorizationEndpoint) assert.Equal(t, "https://explicit.example.com/token", res.TokenEndpoint) @@ -265,15 +261,14 @@ func TestResolveDCRCredentials_InitialAccessTokenAsBearer(t *testing.T) { cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - InitialAccessTokenFile: tokenPath, - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", + InitialAccessToken: readTokenFile(t, tokenPath), } - _, err := ResolveCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "Bearer iat-secret-value", gotAuthHeader) } @@ -329,15 +324,14 @@ func TestResolveDCRCredentials_DoesNotForwardBearerOnRedirect(t *testing.T) { cache := newMemoryDCRStore(t) issuer := upstream.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - InitialAccessTokenFile: tokenPath, - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", + InitialAccessToken: readTokenFile(t, tokenPath), } - _, err := ResolveCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), req, cache) require.Error(t, err, "registration must fail when the upstream returns a redirect") assert.ErrorIs(t, err, errDCRRedirectRefused, "the resolver must refuse to follow registration-endpoint redirects") @@ -398,14 +392,13 @@ func TestResolveDCRCredentials_AuthMethodPreference(t *testing.T) { }) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, tc.expected, res.TokenEndpointAuthMethod) }) @@ -437,14 +430,13 @@ func TestResolveDCRCredentials_RefusesNoneWithoutS256(t *testing.T) { }) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - _, err := ResolveCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), req, cache) require.Error(t, err) assert.Contains(t, err.Error(), "S256", "error must mention the missing S256 advertisement so operators can correlate") @@ -463,13 +455,12 @@ func TestResolveDCRCredentials_EmptyAuthMethodIntersectionErrors(t *testing.T) { }) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - _, err := ResolveCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), req, cache) require.Error(t, err) assert.Contains(t, err.Error(), "no supported token_endpoint_auth_method") } @@ -489,14 +480,13 @@ func TestResolveDCRCredentials_SynthesisedRegistrationEndpoint(t *testing.T) { }) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "test-client-id", res.ClientID) assert.Equal(t, "/register", gotPath) @@ -522,16 +512,15 @@ func TestResolveDCRCredentials_RegistrationEndpointDirectBypassesDiscovery(t *te cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ + req := &Request{ + Issuer: issuer, AuthorizationEndpoint: issuer + "/authorize", TokenEndpoint: issuer + "/token", Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - RegistrationEndpoint: issuer + "/custom/register", - }, + RegistrationEndpoint: issuer + "/custom/register", } - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "direct-id", res.ClientID) assert.Equal(t, int32(0), atomic.LoadInt32(&discoveryHits), @@ -540,55 +529,56 @@ func TestResolveDCRCredentials_RegistrationEndpointDirectBypassesDiscovery(t *te } // TestResolveDCRCredentials_RejectsInvalidInputs covers every branch of -// validateResolveInputs in one place: nil run-config, pre-provisioned -// ClientID, missing DCRConfig, empty issuer, and nil credential store. The -// previous split into two single-branch tests left three branches uncovered. +// validateResolveInputs in one place: nil request, empty issuer, neither +// discovery_url nor registration_endpoint set, both set, and nil +// credential store. func TestResolveDCRCredentials_RejectsInvalidInputs(t *testing.T) { t.Parallel() - validCfg := &authserver.DCRUpstreamConfig{ - RegistrationEndpoint: "https://example.com/register", - } - tests := []struct { name string - rc *authserver.OAuth2UpstreamRunConfig - issuer string + req *Request cache CredentialStore wantErrSub string }{ { - name: "nil run-config", - rc: nil, - issuer: "https://example.com", + name: "nil request", + req: nil, cache: newMemoryDCRStore(t), - wantErrSub: "oauth2 upstream run-config is required", + wantErrSub: "request is required", }, { - name: "pre-provisioned client_id", - rc: &authserver.OAuth2UpstreamRunConfig{ClientID: "preprovisioned", DCRConfig: validCfg}, - issuer: "https://example.com", + name: "empty issuer", + req: &Request{ + RegistrationEndpoint: "https://example.com/register", + }, cache: newMemoryDCRStore(t), - wantErrSub: "pre-provisioned", + wantErrSub: "issuer is required", }, { - name: "missing dcr_config", - rc: &authserver.OAuth2UpstreamRunConfig{}, - issuer: "https://example.com", + name: "neither discovery_url nor registration_endpoint set", + req: &Request{ + Issuer: "https://example.com", + }, cache: newMemoryDCRStore(t), - wantErrSub: "no dcr_config", + wantErrSub: "must set either discovery_url or registration_endpoint", }, { - name: "empty issuer", - rc: &authserver.OAuth2UpstreamRunConfig{DCRConfig: validCfg}, - issuer: "", + name: "both discovery_url and registration_endpoint set", + req: &Request{ + Issuer: "https://example.com", + DiscoveryURL: "https://example.com/.well-known/oauth-authorization-server", + RegistrationEndpoint: "https://example.com/register", + }, cache: newMemoryDCRStore(t), - wantErrSub: "issuer is required", + wantErrSub: "mutually exclusive", }, { - name: "nil cache", - rc: &authserver.OAuth2UpstreamRunConfig{DCRConfig: validCfg}, - issuer: "https://example.com", + name: "nil cache", + req: &Request{ + Issuer: "https://example.com", + RegistrationEndpoint: "https://example.com/register", + }, cache: nil, wantErrSub: "credential store is required", }, @@ -596,99 +586,13 @@ func TestResolveDCRCredentials_RejectsInvalidInputs(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - _, err := ResolveCredentials(context.Background(), tc.rc, tc.issuer, tc.cache) + _, err := ResolveCredentials(context.Background(), tc.req, tc.cache) require.Error(t, err) assert.Contains(t, err.Error(), tc.wantErrSub) }) } } -func TestNeedsDCR(t *testing.T) { - t.Parallel() - - tests := []struct { - name string - rc *authserver.OAuth2UpstreamRunConfig - expected bool - }{ - {name: "nil", rc: nil, expected: false}, - {name: "empty client_id and dcr_config", rc: &authserver.OAuth2UpstreamRunConfig{ - DCRConfig: &authserver.DCRUpstreamConfig{}, - }, expected: true}, - {name: "client_id without dcr", rc: &authserver.OAuth2UpstreamRunConfig{ - ClientID: "x", - }, expected: false}, - {name: "client_id wins over dcr_config (defensive AND semantic)", rc: &authserver.OAuth2UpstreamRunConfig{ - ClientID: "x", - DCRConfig: &authserver.DCRUpstreamConfig{}, - }, expected: false}, - {name: "both empty", rc: &authserver.OAuth2UpstreamRunConfig{}, expected: false}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - assert.Equal(t, tc.expected, NeedsDCR(tc.rc)) - }) - } -} - -func TestConsumeResolution_RespectsExplicitEndpoints(t *testing.T) { - t.Parallel() - - rc := &authserver.OAuth2UpstreamRunConfig{ - AuthorizationEndpoint: "https://explicit/authorize", - TokenEndpoint: "https://explicit/token", - } - res := &Resolution{ - ClientID: "got-client", - AuthorizationEndpoint: "https://discovered/authorize", - TokenEndpoint: "https://discovered/token", - } - ConsumeResolution(rc, res) - assert.Equal(t, "got-client", rc.ClientID) - assert.Equal(t, "https://explicit/authorize", rc.AuthorizationEndpoint) - assert.Equal(t, "https://explicit/token", rc.TokenEndpoint) -} - -func TestConsumeResolution_FillsMissingEndpoints(t *testing.T) { - t.Parallel() - - rc := &authserver.OAuth2UpstreamRunConfig{} - res := &Resolution{ - ClientID: "got-client", - AuthorizationEndpoint: "https://discovered/authorize", - TokenEndpoint: "https://discovered/token", - } - ConsumeResolution(rc, res) - assert.Equal(t, "got-client", rc.ClientID) - assert.Equal(t, "https://discovered/authorize", rc.AuthorizationEndpoint) - assert.Equal(t, "https://discovered/token", rc.TokenEndpoint) -} - -// TestConsumeResolution_ClearsDCRConfig pins the contract that -// ConsumeResolution clears DCRConfig on the run-config copy after writing -// the resolved ClientID. Without this, OAuth2UpstreamRunConfig.Validate -// (run by buildPureOAuth2Config downstream) trips its ClientID-xor- -// DCRConfig rule on the resolved copy and rejects the upstream at boot. -func TestConsumeResolution_ClearsDCRConfig(t *testing.T) { - t.Parallel() - - rc := &authserver.OAuth2UpstreamRunConfig{ - DCRConfig: &authserver.DCRUpstreamConfig{ - RegistrationEndpoint: "https://idp.example.com/register", - }, - } - res := &Resolution{ - ClientID: "dcr-issued-client", - } - - ConsumeResolution(rc, res) - - assert.Equal(t, "dcr-issued-client", rc.ClientID) - assert.Nil(t, rc.DCRConfig, - "ConsumeResolution must clear DCRConfig so the resolved copy satisfies the ClientID-xor-DCRConfig invariant") -} - func TestResolveUpstreamRedirectURI(t *testing.T) { t.Parallel() @@ -784,14 +688,13 @@ func TestResolveDCRCredentials_DiscoveryURLHonoured(t *testing.T) { cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/tenants/acme/metadata", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/tenants/acme/metadata", } - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "tenant-client", res.ClientID) assert.Equal(t, int32(1), atomic.LoadInt32(&discoveryHits), @@ -825,14 +728,13 @@ func TestResolveDCRCredentials_DiscoveryURLIssuerMismatchRejected(t *testing.T) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/metadata", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/metadata", } - _, err := ResolveCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), req, cache) require.Error(t, err) assert.Contains(t, err.Error(), "issuer mismatch") } @@ -852,20 +754,19 @@ func TestResolveDCRCredentials_DiscoveredScopesFallback(t *testing.T) { }) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ + req := &Request{ // Scopes intentionally left empty so the resolver falls back to // the discovered scopes_supported. - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + Issuer: issuer, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - _, err := ResolveCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) - var req oauthproto.DynamicClientRegistrationRequest - require.NoError(t, json.Unmarshal(gotBody, &req)) - assert.ElementsMatch(t, []string{"openid", "profile", "email"}, []string(req.Scopes), + var dcrReq oauthproto.DynamicClientRegistrationRequest + require.NoError(t, json.Unmarshal(gotBody, &dcrReq)) + assert.ElementsMatch(t, []string{"openid", "profile", "email"}, []string(dcrReq.Scopes), "registration request must carry the discovered scopes_supported") } @@ -884,13 +785,12 @@ func TestResolveDCRCredentials_EmptyScopesOmitted(t *testing.T) { }) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.Equal(t, "test-client-id", res.ClientID) @@ -926,18 +826,17 @@ func TestResolveDCRCredentials_UpstreamIssuerDerivedFromDiscoveryURL(t *testing. // embeddedauthserver.go: buildUpstreamConfigs(... cfg.Issuer ...)). ourIssuer := "https://our-auth.example.com" - rc := &authserver.OAuth2UpstreamRunConfig{ + req := &Request{ + Issuer: ourIssuer, // Explicit redirect URI so the resolver does not try to default // it from ourIssuer (which would still work, but isolating the // concern under test keeps the failure mode crisp). - RedirectURI: "https://our-auth.example.com/oauth/callback", - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server", - }, + RedirectURI: "https://our-auth.example.com/oauth/callback", + Scopes: []string{"openid"}, + DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server", } - res, err := ResolveCredentials(context.Background(), rc, ourIssuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err, "resolver must derive expectedIssuer from DiscoveryURL, not from the caller's issuer") assert.Equal(t, "test-client-id", res.ClientID) @@ -1063,11 +962,10 @@ func TestResolveDCRCredentials_SingleflightCoalescesConcurrentCallers(t *testing cache := &countingStore{inner: newMemoryDCRStore(t)} issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid", "profile"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid", "profile"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } const N = 8 @@ -1078,7 +976,7 @@ func TestResolveDCRCredentials_SingleflightCoalescesConcurrentCallers(t *testing for i := 0; i < N; i++ { go func(idx int) { defer wg.Done() - res, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), req, cache) results[idx] = res errs[idx] = err }(i) @@ -1198,24 +1096,6 @@ func TestResolveUpstreamRedirectURI_PreservesIssuerPath(t *testing.T) { } } -// TestConsumeResolution_DoesNotOverwritePreProvisionedClientID verifies -// the defence-in-depth in ConsumeResolution: a caller that bypasses -// validateResolveInputs and invokes ConsumeResolution directly with a -// pre-provisioned ClientID does not have it silently clobbered. -func TestConsumeResolution_DoesNotOverwritePreProvisionedClientID(t *testing.T) { - t.Parallel() - - rc := &authserver.OAuth2UpstreamRunConfig{ - ClientID: "pre-provisioned", - } - res := &Resolution{ - ClientID: "would-be-overwrite", - } - ConsumeResolution(rc, res) - assert.Equal(t, "pre-provisioned", rc.ClientID, - "ConsumeResolution must not overwrite a non-empty ClientID") -} - // TestResolveDCREndpoints_DirectRegistrationEndpointValidated covers // PR #5042 review comment #10: the cfg.RegistrationEndpoint short-circuit // branch validates the URL locally before performRegistration constructs a @@ -1247,8 +1127,8 @@ func TestResolveDCREndpoints_DirectRegistrationEndpointValidated(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - cfg := &authserver.DCRUpstreamConfig{RegistrationEndpoint: tc.registrationEndpoint} - _, err := resolveDCREndpoints(context.Background(), cfg) + req := &Request{RegistrationEndpoint: tc.registrationEndpoint} + _, err := resolveDCREndpoints(context.Background(), req) if tc.wantErrSub == "" { require.NoError(t, err) return @@ -1342,13 +1222,12 @@ func TestResolveDCRCredentials_CacheGetFailureWrapped(t *testing.T) { storeErr := errors.New("simulated backend failure") store := failingDCRStore{getErr: storeErr} - rc := &authserver.OAuth2UpstreamRunConfig{ - DCRConfig: &authserver.DCRUpstreamConfig{ - RegistrationEndpoint: "https://idp.example.com/register", - }, + req := &Request{ + Issuer: "https://idp.example.com", + RegistrationEndpoint: "https://idp.example.com/register", } - _, err := ResolveCredentials(context.Background(), rc, "https://idp.example.com", store) + _, err := ResolveCredentials(context.Background(), req, store) require.Error(t, err) assert.ErrorIs(t, err, storeErr, "cache.Get error must be wrapped with %%w so callers can inspect the cause") @@ -1370,14 +1249,13 @@ func TestResolveDCRCredentials_CachePutFailureWrapped(t *testing.T) { storeErr := errors.New("simulated put backend failure") store := failingDCRStore{putErr: storeErr} - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: server.URL, + Scopes: []string{"openid"}, + DiscoveryURL: server.URL + "/.well-known/oauth-authorization-server", } - _, err := ResolveCredentials(context.Background(), rc, server.URL, store) + _, err := ResolveCredentials(context.Background(), req, store) require.Error(t, err) assert.ErrorIs(t, err, storeErr, "cache.Put error must be wrapped with %%w so callers can inspect the cause") @@ -1474,15 +1352,14 @@ func TestResolveDCRCredentials_RefetchesOnExpiredCachedSecret(t *testing.T) { cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } // First call: registers, populates cache with already-expired entry. - res1, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res1, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) require.NotNil(t, res1) require.False(t, res1.ClientSecretExpiresAt.IsZero(), @@ -1492,7 +1369,7 @@ func TestResolveDCRCredentials_RefetchesOnExpiredCachedSecret(t *testing.T) { require.EqualValues(t, 1, atomic.LoadInt32(®istrationCalls)) // Second call: the cached entry is expired, so the resolver must refetch. - res2, err := ResolveCredentials(context.Background(), rc, issuer, cache) + res2, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) require.NotNil(t, res2) assert.EqualValues(t, 2, atomic.LoadInt32(®istrationCalls), @@ -1529,16 +1406,15 @@ func TestResolveDCRCredentials_HonoursFutureExpiryAndZero(t *testing.T) { }) cache := newMemoryDCRStore(t) issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } - _, err := ResolveCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) - _, err = ResolveCredentials(context.Background(), rc, issuer, cache) + _, err = ResolveCredentials(context.Background(), req, cache) require.NoError(t, err) assert.EqualValues(t, 1, atomic.LoadInt32(®istrationCalls), @@ -1579,11 +1455,10 @@ func TestResolveDCRCredentials_RecoversPanicInsideSingleflight(t *testing.T) { store := panickingPutDCRStore{panicValue: "boom"} issuer := server.URL - rc := &authserver.OAuth2UpstreamRunConfig{ - Scopes: []string{"openid"}, - DCRConfig: &authserver.DCRUpstreamConfig{ - DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", - }, + req := &Request{ + Issuer: issuer, + Scopes: []string{"openid"}, + DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", } const N = 6 @@ -1604,7 +1479,7 @@ func TestResolveDCRCredentials_RecoversPanicInsideSingleflight(t *testing.T) { panicked[idx] = true } }() - _, errs[idx] = ResolveCredentials(context.Background(), rc, issuer, store) + _, errs[idx] = ResolveCredentials(context.Background(), req, store) }(i) } @@ -1677,8 +1552,7 @@ func TestDcrStepError(t *testing.T) { t.Parallel() // Precondition failure → dcrStepValidate. - _, err := ResolveCredentials(context.Background(), nil, "https://as", - newMemoryDCRStore(t)) + _, err := ResolveCredentials(context.Background(), nil, newMemoryDCRStore(t)) require.Error(t, err) var stepErr *dcrStepError require.True(t, errors.As(err, &stepErr)) diff --git a/pkg/auth/dcr/secret_test.go b/pkg/auth/dcr/secret_test.go deleted file mode 100644 index cae54714d3..0000000000 --- a/pkg/auth/dcr/secret_test.go +++ /dev/null @@ -1,118 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package dcr - -import ( - "os" - "path/filepath" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// TestResolveSecret pins the dcr-package copy of resolveSecret to the same -// observable contract as the parallel runner-package copy -// (pkg/authserver/runner/embeddedauthserver_test.go::TestResolveSecret*). -// Two physically-distinct copies of this helper exist by design (the dcr -// package must not reach back into pkg/authserver/runner per its -// profile-agnostic charter); this test guards against silent drift between -// them. If a future bug fix lands on one copy without being mirrored to the -// other, this test or its runner-package twin will fail. -// -// Cases that take t.Setenv() are kept out of the parallel sub-suite because -// t.Setenv requires a non-parallel test scope. -func TestResolveSecret(t *testing.T) { - t.Parallel() - - tmpDir := t.TempDir() - secretFile := filepath.Join(tmpDir, "secret") - require.NoError(t, os.WriteFile(secretFile, []byte(" secret-value \n"), 0o600)) - - cases := []struct { - name string - file string - envVar string - want string - wantErr bool - wantErrSubs []string - }{ - { - name: "neither file nor env var set returns empty string and no error", - file: "", envVar: "", - want: "", - }, - { - name: "file content is read and surrounding whitespace trimmed", - file: secretFile, envVar: "", - want: "secret-value", - }, - { - name: "missing file returns wrapped read error", - file: "/nonexistent/file", envVar: "", - wantErr: true, wantErrSubs: []string{"failed to read secret file"}, - }, - { - name: "env var name is set but env var is empty returns explanatory error", - // Use a unique env var name that won't be set in the environment. - file: "", envVar: "TEST_SECRET_NOT_SET_DCR_PKG_12345", - wantErr: true, wantErrSubs: []string{"environment variable", "is not set"}, - }, - } - - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - got, err := resolveSecret(tc.file, tc.envVar) - if tc.wantErr { - require.Error(t, err) - for _, sub := range tc.wantErrSubs { - assert.Contains(t, err.Error(), sub) - } - assert.Empty(t, got) - return - } - require.NoError(t, err) - assert.Equal(t, tc.want, got) - }) - } -} - -// TestResolveSecretWithEnvVar covers the env-var paths separately because -// t.Setenv requires a non-parallel test scope. Mirrors the runner-package -// twin (TestResolveSecretWithEnvVar in embeddedauthserver_test.go). -func TestResolveSecretWithEnvVar(t *testing.T) { - tmpDir := t.TempDir() - secretFile := filepath.Join(tmpDir, "secret") - require.NoError(t, os.WriteFile(secretFile, []byte("secret-from-file"), 0o600)) - - t.Run("file takes precedence over env var when both are set", func(t *testing.T) { - envVar := "TEST_SECRET_FILE_PRECEDENCE_DCR_PKG" - t.Setenv(envVar, "secret-from-env") - - got, err := resolveSecret(secretFile, envVar) - require.NoError(t, err) - assert.Equal(t, "secret-from-file", got) - }) - - t.Run("env var is read when file is empty", func(t *testing.T) { - envVar := "TEST_SECRET_ENV_ONLY_DCR_PKG" - t.Setenv(envVar, "secret-from-env") - - got, err := resolveSecret("", envVar) - require.NoError(t, err) - assert.Equal(t, "secret-from-env", got) - }) - - t.Run("missing file does not silently fall back to env var", func(t *testing.T) { - envVar := "TEST_SECRET_NO_FALLBACK_DCR_PKG" - t.Setenv(envVar, "secret-from-env") - - got, err := resolveSecret("/nonexistent/file", envVar) - require.Error(t, err) - assert.Contains(t, err.Error(), "failed to read secret file") - assert.Empty(t, got) - }) -} diff --git a/pkg/auth/dcr/store.go b/pkg/auth/dcr/store.go index d7fccc2521..8694e6bbd6 100644 --- a/pkg/auth/dcr/store.go +++ b/pkg/auth/dcr/store.go @@ -7,6 +7,8 @@ import ( "context" "errors" "fmt" + "io" + "sync" "time" "github.com/stacklok/toolhive/pkg/authserver/storage" @@ -71,6 +73,111 @@ func NewStorageBackedStore(backend storage.DCRCredentialStore) CredentialStore { return &storageBackedStore{backend: backend} } +// CloseableCredentialStore is a CredentialStore that also releases an +// underlying resource (typically a background goroutine) when closed. +// NewInMemoryStore returns this interface so callers can call Close() +// directly without a runtime type assertion — Close() being part of the +// returned type at compile time prevents the silent-no-op failure mode +// that .claude/rules/go-style.md warns about ("Never define a local +// anonymous interface inside an option and type-assert against it to +// check capability — a silent no-op results if the target doesn't +// implement it."). +// +// NewStorageBackedStore returns the base CredentialStore because its +// backends (storage.MemoryStorage shared across the authserver, +// storage.RedisStorage) are owned by the authserver runner and closed +// through that lifecycle, not by the dcr package. +type CloseableCredentialStore interface { + CredentialStore + io.Closer +} + +// NewInMemoryStore returns a CloseableCredentialStore whose entries live +// only for the lifetime of the returned value. This is the constructor +// consumers reach for when they have no shared durable backend — most +// notably the CLI OAuth flow, which manages cross-invocation credential +// persistence outside the resolver (in pkg/auth/remote/handler.go's +// CachedClientID / CachedClientSecretRef fields) and only needs the +// resolver's intra-call singleflight + S256 PKCE / expiry-refetch +// behaviour for one PerformOAuthFlow call. +// +// The implementation delegates to storage.NewMemoryStorage to share the +// same Get/Put/scope-hash semantics as the durable backends, including +// the background cleanup goroutine. Callers that need to release that +// goroutine before process exit MUST call Close on the returned value +// when finished — the return type is CloseableCredentialStore precisely +// so the call site can `defer store.Close()` without a runtime +// type-assertion. (CLI flows that complete in a single invocation can +// also rely on process exit.) +func NewInMemoryStore() CloseableCredentialStore { + return &inMemoryStore{mem: storage.NewMemoryStorage()} +} + +// inMemoryStore is the CloseableCredentialStore returned by +// NewInMemoryStore. It holds exactly one *storage.MemoryStorage handle, +// which serves Get / Put / Close. No embedding of storageBackedStore — +// embedding would introduce a second handle to the same backend (typed +// as the broad storage.DCRCredentialStore interface) and require a +// manual "keep these two in sync" invariant the compiler could not +// enforce. The single-field shape gives Get / Put / Close one +// authoritative source. +// +// Get / Put logic is intentionally duplicated with storageBackedStore +// rather than delegated through it: the two methods are three lines +// each, and avoiding the embedding is a strictly stronger structural +// guarantee than DRY here. credentialsToResolution / +// resolutionToCredentials remain shared. +// +// closeOnce makes Close() idempotent at the dcr-package boundary even +// though storage.MemoryStorage.Close() is not — calling +// MemoryStorage.Close() twice panics on `close of closed channel`. The +// CLI's `defer store.Close()` is single-call today, but guarding here +// means future call sites that close-on-shortcircuit-then-defer (or +// test helpers that close-then-reuse) cannot panic the process. +type inMemoryStore struct { + mem *storage.MemoryStorage + closeOnce sync.Once + closeErr error +} + +// Get implements CredentialStore by delegating to the embedded +// *storage.MemoryStorage. The ErrNotFound translation matches +// storageBackedStore.Get; see that method for the rationale. +func (s *inMemoryStore) Get(ctx context.Context, key Key) (*Resolution, bool, error) { + creds, err := s.mem.GetDCRCredentials(ctx, key) + if err != nil { + if errors.Is(err, storage.ErrNotFound) { + return nil, false, nil + } + return nil, false, err + } + return credentialsToResolution(creds), true, nil +} + +// Put implements CredentialStore by delegating to the embedded +// *storage.MemoryStorage. The nil-resolution rejection matches +// storageBackedStore.Put; see that method for the rationale. +func (s *inMemoryStore) Put(ctx context.Context, key Key, resolution *Resolution) error { + if resolution == nil { + return fmt.Errorf("dcr: resolution must not be nil") + } + creds := resolutionToCredentials(key, resolution) + return s.mem.StoreDCRCredentials(ctx, creds) +} + +// Close releases the embedded MemoryStorage cleanup goroutine. Safe to +// call multiple times: the underlying storage.MemoryStorage.Close +// closes a channel and is NOT itself idempotent (a second call panics +// on `close of closed channel`); the closeOnce here makes the +// dcr-package boundary contract idempotent regardless. Subsequent +// calls return the same error (if any) the first call produced. +func (s *inMemoryStore) Close() error { + s.closeOnce.Do(func() { + s.closeErr = s.mem.Close() + }) + return s.closeErr +} + // storageBackedStore is the dcr-package CredentialStore wrapping a // storage.DCRCredentialStore. Its methods are the only place that converts // between the resolver's *Resolution and the persisted @@ -144,8 +251,9 @@ func resolutionToCredentials(key Key, res *Resolution) *storage.DCRCredentials { // credentialsToResolution is the inverse of resolutionToCredentials. The // RedirectURI is recovered from the persisted Key so consumers that read it -// off the resolution (e.g. ConsumeResolution, which writes it back onto a -// run-config copy when the caller left it empty) see the canonical value. +// off the resolution (e.g. pkg/authserver/runner.consumeResolution, which +// writes it back onto a run-config copy when the caller left it empty) +// see the canonical value. // // ClientIDIssuedAt is left zero because it is not persisted. Callers that // care about it (none today) must read it directly from the live RFC 7591 diff --git a/pkg/auth/dcr/store_test.go b/pkg/auth/dcr/store_test.go index 46ca9e16e1..bde087d9a7 100644 --- a/pkg/auth/dcr/store_test.go +++ b/pkg/auth/dcr/store_test.go @@ -399,3 +399,130 @@ func TestResolutionCredentialsRoundTrip(t *testing.T) { assert.Nil(t, credentialsToResolution(nil)) }) } + +// TestInMemoryStore_CloseIsIdempotent pins the contract that calling +// Close on the returned CloseableCredentialStore more than once is safe. +// The underlying storage.MemoryStorage.Close is NOT idempotent — it +// closes a channel that a second call would re-close, panicking with +// "close of closed channel" — so the dcr-package wrapper MUST guard +// against the second call. Without the sync.Once on inMemoryStore, the +// CLI's `defer store.Close()` plus any future close-on-shortcircuit +// would crash the process; this test makes the guard load-bearing. +func TestInMemoryStore_CloseIsIdempotent(t *testing.T) { + t.Parallel() + + store := NewInMemoryStore() + + // First Close must succeed and release the cleanup goroutine. + require.NoError(t, store.Close()) + + // Second Close must NOT panic and must return the same error + // (nil here, since the first call succeeded). + require.NoError(t, store.Close(), + "Close() must be idempotent at the dcr-package boundary; "+ + "the underlying MemoryStorage.Close is not idempotent and a "+ + "second call without the guard would panic") +} + +// TestInMemoryStore_CloseIsIdempotentUnderRace verifies the sync.Once +// guard holds under concurrent callers. A regression that replaced +// sync.Once with a non-atomic guard (e.g. a plain bool) would surface +// here as a panic from a racing channel close. +func TestInMemoryStore_CloseIsIdempotentUnderRace(t *testing.T) { + t.Parallel() + + store := NewInMemoryStore() + + const N = 8 + var wg sync.WaitGroup + wg.Add(N) + errs := make([]error, N) + for i := range N { + go func(idx int) { + defer wg.Done() + errs[idx] = store.Close() + }(i) + } + + done := make(chan struct{}) + go func() { wg.Wait(); close(done) }() + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("timeout waiting for concurrent Close() callers") + } + + for i := range N { + require.NoError(t, errs[i], "goroutine %d's Close() must not error", i) + } +} + +// TestInMemoryStore_PutGetCloseShareBackend pins the contract that Put, +// Get, and Close all operate against the same underlying +// *storage.MemoryStorage instance. The previous shape held two handles +// (one in an embedded storageBackedStore.backend, one in a concrete +// mem field); a constructor regression assigning distinct handles to +// the two fields would have surfaced as a Close that leaks the +// cleanup goroutine while Get still serves entries. Today only one +// handle exists, so this test asserts the observable consequence: +// after Close, the goroutine is gone (verified by t.Cleanup running +// without leaking via go test -race goroutine leak checks). +func TestInMemoryStore_PutGetCloseShareBackend(t *testing.T) { + t.Parallel() + + store := NewInMemoryStore() + t.Cleanup(func() { _ = store.Close() }) + + ctx := context.Background() + key := Key{ + Issuer: "https://idp.example.com", + RedirectURI: "https://toolhive.example.com/oauth/callback", + ScopesHash: storage.ScopesHash([]string{"openid"}), + } + resolution := &Resolution{ + ClientID: "client-abc", + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + } + + require.NoError(t, store.Put(ctx, key, resolution)) + + got, ok, err := store.Get(ctx, key) + require.NoError(t, err) + require.True(t, ok, "Get must see the value Put just wrote — confirms Put and Get share a backend") + assert.Equal(t, "client-abc", got.ClientID) +} + +// TestInMemoryStore_PutRejectsNilResolution mirrors the contract pinned +// for storageBackedStore.Put: a nil resolution is rejected at the +// adapter boundary rather than silently no-oped, so the next Get's miss +// surfaces with a debug trail. inMemoryStore implements Put directly +// (not via embedding) — this test guards against a delegation +// regression that omitted the nil check. +func TestInMemoryStore_PutRejectsNilResolution(t *testing.T) { + t.Parallel() + + store := NewInMemoryStore() + t.Cleanup(func() { _ = store.Close() }) + + err := store.Put(context.Background(), Key{Issuer: "https://idp.example.com"}, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "resolution must not be nil") +} + +// TestInMemoryStore_GetMissingKeyReturnsMissTuple pins the ErrNotFound +// translation contract: a missing key surfaces as (nil, false, nil) +// rather than as a wrapped error. inMemoryStore implements Get directly +// (not via embedding) — this test guards against a delegation +// regression that surfaced ErrNotFound as a hard error. +func TestInMemoryStore_GetMissingKeyReturnsMissTuple(t *testing.T) { + t.Parallel() + + store := NewInMemoryStore() + t.Cleanup(func() { _ = store.Close() }) + + got, ok, err := store.Get(context.Background(), Key{Issuer: "https://unknown.example.com"}) + require.NoError(t, err) + assert.False(t, ok) + assert.Nil(t, got) +} diff --git a/pkg/auth/dcr/testhelpers_test.go b/pkg/auth/dcr/testhelpers_test.go index 7dc1c4cb11..21608260a2 100644 --- a/pkg/auth/dcr/testhelpers_test.go +++ b/pkg/auth/dcr/testhelpers_test.go @@ -4,8 +4,12 @@ package dcr import ( + "io" + "os" "testing" + "github.com/stretchr/testify/require" + "github.com/stacklok/toolhive/pkg/authserver/storage" ) @@ -26,3 +30,21 @@ func newMemoryDCRStore(t *testing.T) CredentialStore { t.Cleanup(func() { _ = stor.Close() }) return NewStorageBackedStore(stor) } + +// readTokenFile reads a secret from disk and returns its trimmed contents, +// mirroring the production embeddedauthserver.resolveSecret semantics so +// resolver tests can populate Request.InitialAccessToken from a file path. +func readTokenFile(t *testing.T, path string) string { + t.Helper() + f, err := os.Open(path) //nolint:gosec // G304: test-only helper, paths are constructed under t.TempDir() + require.NoError(t, err) + defer f.Close() + data, err := io.ReadAll(f) + require.NoError(t, err) + // Trim trailing newline conventionally appended by Kubernetes mounts / + // shell heredocs — same trim production code performs. + for len(data) > 0 && (data[len(data)-1] == '\n' || data[len(data)-1] == ' ' || data[len(data)-1] == '\t' || data[len(data)-1] == '\r') { + data = data[:len(data)-1] + } + return string(data) +} diff --git a/pkg/auth/discovery/dcr_request.go b/pkg/auth/discovery/dcr_request.go deleted file mode 100644 index 8225aef0f2..0000000000 --- a/pkg/auth/discovery/dcr_request.go +++ /dev/null @@ -1,29 +0,0 @@ -// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. -// SPDX-License-Identifier: Apache-2.0 - -package discovery - -import ( - "fmt" - - "github.com/stacklok/toolhive/pkg/oauthproto" -) - -// NewDynamicClientRegistrationRequest constructs a DCR request for the CLI OAuth flow. -// -// The redirect URI is always http://localhost:/callback, following -// RFC 8252 Section 7.3 which specifies loopback interface redirects for native -// public clients. This loopback assumption is specific to the CLI flow and must -// not be moved into the protocol package. -func NewDynamicClientRegistrationRequest(scopes []string, callbackPort int) *oauthproto.DynamicClientRegistrationRequest { - redirectURIs := []string{fmt.Sprintf("http://localhost:%d/callback", callbackPort)} - - return &oauthproto.DynamicClientRegistrationRequest{ - ClientName: oauthproto.ToolHiveMCPClientName, - RedirectURIs: redirectURIs, - TokenEndpointAuthMethod: oauthproto.TokenEndpointAuthMethodNone, // For PKCE flow - GrantTypes: []string{oauthproto.GrantTypeAuthorizationCode, oauthproto.GrantTypeRefreshToken}, - ResponseTypes: []string{oauthproto.ResponseTypeCode}, - Scopes: scopes, - } -} diff --git a/pkg/auth/discovery/dcr_resolver_test.go b/pkg/auth/discovery/dcr_resolver_test.go new file mode 100644 index 0000000000..e5da69ad03 --- /dev/null +++ b/pkg/auth/discovery/dcr_resolver_test.go @@ -0,0 +1,311 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package discovery + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/oauthproto" +) + +// Tests in this file pin the behavioural properties the CLI flow inherits +// from migrating its DCR call through pkg/auth/dcr.ResolveCredentials +// (issue #5219 sub-issue 4b). Each test mounts an httptest server that +// simulates a specific upstream behaviour and asserts that +// handleDynamicRegistration surfaces the resolver's reaction unchanged. +// +// The properties under test: +// +// - S256 PKCE gating: an upstream advertising only "plain" PKCE causes +// the CLI to surface a resolver error rather than register a public +// client without S256 (RFC 7636 / OAuth 2.1 compliance). +// - Bearer-token transport redirect refusal: the registration POST does +// not follow 30x redirects, so a token-leak class of attack is +// impossible. +// - Singleflight deduplication: concurrent handleDynamicRegistration +// calls for the same (issuer, scopes, redirectURI) coalesce into +// exactly one upstream /register request. +// +// Expiry-driven refetch and panic recovery are pinned by the resolver's +// own test suite in pkg/auth/dcr; this file does not duplicate them +// because under the option (b) persistence model the CLI flow does not +// exercise either path in a CLI-specific way. Specifically, the +// resolver's RFC 7591 §3.2.1 expiry refetch is in the loop only within +// a single PerformOAuthFlow call (which the CLI never makes repeat +// queries inside). Cross-invocation expiry is handled by the remote +// handler at pkg/auth/remote/handler.go via its HasCachedClientCredentials +// gate, which runs BEFORE this code path and short-circuits to a +// refresh-token flow when a usable cached client exists. The gap between +// "the resolver supports expiry refetch" and "the CLI's cross-invocation +// persistence loop uses it" is the option (b) trade-off documented on +// handleDynamicRegistration; option (a) would close that gap and is the +// natural follow-up. + +// dcrTestServerConfig controls the mock upstream's behaviour for the CLI +// inherited-property tests below. +type dcrTestServerConfig struct { + // codeChallengeMethodsSupported is the upstream's advertised + // code_challenge_methods_supported value. Set to []string{"S256"} for + // happy-path tests; set to []string{"plain"} or nil to exercise the + // S256 gate. + codeChallengeMethodsSupported []string + + // registrationStatusCode overrides the registration endpoint's + // response status. When zero, the endpoint returns 201 Created with a + // well-formed RFC 7591 response. + registrationStatusCode int + + // registrationRedirectsTo, when non-empty, causes the registration + // endpoint to issue a 302 to this URL — used to verify the bearer + // transport's redirect refusal. + registrationRedirectsTo string + + // onRegistration is called once per registration attempt. Safe for + // concurrent use. + onRegistration func(r *http.Request, body []byte) + + // registrationDelay introduces a delay inside the registration + // handler so concurrent goroutines can pile up at the singleflight + // before any can finish. + registrationDelay time.Duration +} + +// newDCRDiscoveryServer mounts /.well-known/openid-configuration and a +// /register endpoint on a single httptest server. The metadata advertises +// the server's URL as the issuer (so RFC 8414 §3.3 issuer match passes +// against any DiscoveryURL derived from the issuer). +func newDCRDiscoveryServer(t *testing.T, cfg dcrTestServerConfig) *httptest.Server { + t.Helper() + var server *httptest.Server + mux := http.NewServeMux() + + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { + md := oauthproto.OIDCDiscoveryDocument{ + AuthorizationServerMetadata: oauthproto.AuthorizationServerMetadata{ + Issuer: server.URL, + AuthorizationEndpoint: server.URL + "/authorize", + TokenEndpoint: server.URL + "/token", + JWKSURI: server.URL + "/jwks", + RegistrationEndpoint: server.URL + "/register", + CodeChallengeMethodsSupported: cfg.codeChallengeMethodsSupported, + TokenEndpointAuthMethodsSupported: []string{"none"}, + ResponseTypesSupported: []string{"code"}, + }, + SubjectTypesSupported: []string{"public"}, + IDTokenSigningAlgValuesSupported: []string{"RS256"}, + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(md) + }) + + mux.HandleFunc("/register", func(w http.ResponseWriter, r *http.Request) { + body, _ := io.ReadAll(r.Body) + _ = r.Body.Close() + if cfg.onRegistration != nil { + cfg.onRegistration(r, body) + } + if cfg.registrationDelay > 0 { + time.Sleep(cfg.registrationDelay) + } + if cfg.registrationRedirectsTo != "" { + http.Redirect(w, r, cfg.registrationRedirectsTo, http.StatusFound) + return + } + if cfg.registrationStatusCode != 0 { + w.WriteHeader(cfg.registrationStatusCode) + return + } + resp := oauthproto.DynamicClientRegistrationResponse{ + ClientID: "cli-registered-client", + TokenEndpointAuthMethod: "none", + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusCreated) + _ = json.NewEncoder(w).Encode(resp) + }) + + server = httptest.NewServer(mux) + t.Cleanup(server.Close) + return server +} + +// TestHandleDynamicRegistration_InheritsS256Gating verifies the CLI flow +// surfaces the resolver's S256 PKCE error when the upstream advertises +// only "plain" (or omits code_challenge_methods_supported entirely). The +// pre-migration CLI would have registered as a public client regardless, +// silently violating RFC 7636 / OAuth 2.1; the migrated CLI MUST surface +// a clear error. +func TestHandleDynamicRegistration_InheritsS256Gating(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + codeChallengeMethodsSupported []string + }{ + {name: "upstream omits code_challenge_methods_supported", codeChallengeMethodsSupported: nil}, + {name: "upstream advertises only plain", codeChallengeMethodsSupported: []string{"plain"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var registrationHits int32 + server := newDCRDiscoveryServer(t, dcrTestServerConfig{ + codeChallengeMethodsSupported: tc.codeChallengeMethodsSupported, + onRegistration: func(_ *http.Request, _ []byte) { + atomic.AddInt32(®istrationHits, 1) + }, + }) + + config := &OAuthFlowConfig{ + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + } + + err := handleDynamicRegistration(context.Background(), server.URL, config) + require.Error(t, err, "S256 gating must fail registration") + assert.Contains(t, err.Error(), "S256", + "resolver error must mention the missing S256 advertisement") + assert.EqualValues(t, 0, atomic.LoadInt32(®istrationHits), + "the upstream /register endpoint must NOT be contacted when the S256 gate fails") + }) + } +} + +// TestHandleDynamicRegistration_InheritsRedirectRefusal verifies that the +// registration POST does not follow a redirect. A non-defended client +// would re-issue the registration request (with any attached bearer) +// against the redirect target; the resolver's bearerTokenTransport refuses, +// and the CLI surfaces the resulting *url.Error. +func TestHandleDynamicRegistration_InheritsRedirectRefusal(t *testing.T) { + t.Parallel() + + // Foreign origin: a separate server that records every request. + var foreignHits int32 + foreign := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&foreignHits, 1) + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(foreign.Close) + + server := newDCRDiscoveryServer(t, dcrTestServerConfig{ + codeChallengeMethodsSupported: []string{"S256"}, + registrationRedirectsTo: foreign.URL + "/stolen", + }) + + config := &OAuthFlowConfig{ + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + } + + err := handleDynamicRegistration(context.Background(), server.URL, config) + require.Error(t, err, "registration must fail when the upstream returns a redirect") + assert.Contains(t, err.Error(), "redirect", + "resolver error must mention the refused redirect so operators can correlate") + assert.EqualValues(t, 0, atomic.LoadInt32(&foreignHits), + "foreign origin must receive zero requests; the redirect refusal prevents the leak") +} + +// TestHandleDynamicRegistration_InheritsSingleflightDedup verifies that +// N concurrent handleDynamicRegistration calls for the same (issuer, +// scopes, redirectURI) tuple coalesce into exactly one upstream /register +// request. The pre-migration CLI would have made N parallel registrations +// and produced N orphaned client_ids at the upstream — a real production +// hazard for rate-limited DCR endpoints and a cleanup task for operators. +// +// "Exactly one /register" is the property under test. The CLI's +// per-invocation in-memory store means cross-invocation reuse is NOT +// observable here (that lives in pkg/auth/remote/handler.go); this test +// strictly pins the intra-call singleflight. +func TestHandleDynamicRegistration_InheritsSingleflightDedup(t *testing.T) { + t.Parallel() + + // Each invocation of handleDynamicRegistration constructs a fresh + // dcr.InMemoryStore, so cross-invocation cache hits are NOT + // possible. The singleflight at the resolver layer is package-global, + // which is what coalesces concurrent goroutines hitting the same + // (issuer, redirectURI, scopesHash) key. + var registrationHits int32 + server := newDCRDiscoveryServer(t, dcrTestServerConfig{ + codeChallengeMethodsSupported: []string{"S256"}, + registrationDelay: 250 * time.Millisecond, + onRegistration: func(_ *http.Request, _ []byte) { + atomic.AddInt32(®istrationHits, 1) + }, + }) + + const N = 6 + var wg sync.WaitGroup + wg.Add(N) + results := make([]*OAuthFlowConfig, N) + errs := make([]error, N) + + for i := 0; i < N; i++ { + // Each goroutine gets its own OAuthFlowConfig so we can assert + // the resolution was applied to every caller's config. + cfg := &OAuthFlowConfig{ + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + } + results[i] = cfg + go func(idx int) { + defer wg.Done() + errs[idx] = handleDynamicRegistration(context.Background(), server.URL, cfg) + }(i) + } + + done := make(chan struct{}) + go func() { wg.Wait(); close(done) }() + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("timeout waiting for concurrent handleDynamicRegistration goroutines") + } + + for i := 0; i < N; i++ { + require.NoError(t, errs[i], "goroutine %d errored", i) + assert.Equal(t, "cli-registered-client", results[i].ClientID, + "every concurrent caller must observe the same resolved client_id") + } + assert.EqualValues(t, 1, atomic.LoadInt32(®istrationHits), + "expected exactly one /register call despite %d concurrent goroutines; got %d", + N, atomic.LoadInt32(®istrationHits)) +} + +// TestHandleDynamicRegistration_PopulatesEndpoints verifies the contract +// the rest of the CLI flow depends on: handleDynamicRegistration writes +// the resolved AuthorizeURL / TokenURL onto OAuthFlowConfig so +// createOAuthConfig can construct the OAuth2 flow without re-discovery. +func TestHandleDynamicRegistration_PopulatesEndpoints(t *testing.T) { + t.Parallel() + + server := newDCRDiscoveryServer(t, dcrTestServerConfig{ + codeChallengeMethodsSupported: []string{"S256"}, + }) + + config := &OAuthFlowConfig{ + Scopes: []string{"openid", "profile"}, + CallbackPort: 8765, + } + + err := handleDynamicRegistration(context.Background(), server.URL, config) + require.NoError(t, err) + assert.Equal(t, "cli-registered-client", config.ClientID, + "handleDynamicRegistration must populate OAuthFlowConfig.ClientID") + assert.Equal(t, server.URL+"/authorize", config.AuthorizeURL, + "handleDynamicRegistration must populate OAuthFlowConfig.AuthorizeURL") + assert.Equal(t, server.URL+"/token", config.TokenURL, + "handleDynamicRegistration must populate OAuthFlowConfig.TokenURL") +} diff --git a/pkg/auth/discovery/discovery.go b/pkg/auth/discovery/discovery.go index 78677a7749..69ba7b1be4 100644 --- a/pkg/auth/discovery/discovery.go +++ b/pkg/auth/discovery/discovery.go @@ -27,6 +27,7 @@ import ( "golang.org/x/oauth2" "github.com/stacklok/toolhive/pkg/auth" + "github.com/stacklok/toolhive/pkg/auth/dcr" "github.com/stacklok/toolhive/pkg/auth/oauth" "github.com/stacklok/toolhive/pkg/networking" "github.com/stacklok/toolhive/pkg/oauthproto" @@ -585,31 +586,183 @@ func PerformOAuthFlow(ctx context.Context, issuer string, config *OAuthFlowConfi return newOAuthFlow(ctx, oauthConfig, config) } -// handleDynamicRegistration handles the dynamic client registration process +// handleDynamicRegistration handles the dynamic client registration process. +// +// Persistence model (option (b) from #5219): the resolver runs against an +// in-memory dcr.CredentialStore for the duration of a single CLI +// invocation, so within one PerformOAuthFlow call concurrent goroutines +// share the singleflight (proving the inherited "singleflight +// deduplication" property). Cross-invocation persistence is handled +// outside the resolver by pkg/auth/remote/handler.go, which reads +// CachedClientID / CachedClientSecretRef before this code path runs and +// short-circuits to a refresh-token flow when a usable cached client +// exists. +// +// One consequence of option (b) is that the resolver's RFC 7591 §3.2.1 +// expiry-driven refetch does NOT participate in the CLI's cross- +// invocation persistence loop: each PerformOAuthFlow call builds a fresh +// in-memory store, so a "cached but expired" entry from the previous +// invocation never reaches the resolver. Cross-invocation expiry is the +// remote handler's responsibility (its HasCachedClientCredentials gate +// runs ahead of this code path). Within a single invocation, the +// resolver's expiry check is still in the loop and would fire if the +// same call site somehow registered, persisted, and re-queried the +// in-memory store — but the CLI never does this today. +// +// Wrapping the remote handler's secretProvider into a dcr.CredentialStore +// adapter (option (a)) would close that loop and is the natural follow-up; +// it was rejected here as out-of-scope churn for sub-issue 4b. func handleDynamicRegistration(ctx context.Context, issuer string, config *OAuthFlowConfig) error { discoveredDoc, err := getDiscoveryDocument(ctx, issuer, config) if err != nil { return fmt.Errorf("failed to discover registration endpoint: %w", err) } - registrationResponse, err := registerDynamicClient(ctx, config, discoveredDoc) + // Check if the provider supports Dynamic Client Registration before + // invoking the resolver. The CLI-flag hint below is intentional: this + // function is CLI-facing (pkg/auth/discovery is not a protocol-level + // package) and the flags named here are the correct fallback for + // operators who need to supply credentials manually. The + // protocol-neutral version of this message lives in + // pkg/oauthproto.handleHTTPResponse for the HTTP 404/405/501 paths. + if discoveredDoc.RegistrationEndpoint == "" { + return fmt.Errorf("this provider does not support Dynamic Client Registration (DCR). " + + "Please configure OAuth client credentials using --remote-auth-client-id and --remote-auth-client-secret flags, " + + "or register a client manually with the provider") + } + + resolution, err := resolveDCRCredentials(ctx, issuer, config, discoveredDoc) if err != nil { return err } - // Update config with registered client credentials - config.ClientID = registrationResponse.ClientID - config.ClientSecret = registrationResponse.ClientSecret + // Update config with registered client credentials. The remote handler + // at pkg/auth/remote/handler.go reads ClientID / ClientSecret off + // OAuthFlowResult and persists them into CachedClientID / + // CachedClientSecretRef for the next invocation. + config.ClientID = resolution.ClientID + config.ClientSecret = resolution.ClientSecret - if discoveredDoc.RegistrationEndpoint != "" { - config.AuthorizeURL = discoveredDoc.AuthorizationEndpoint - config.TokenURL = discoveredDoc.TokenEndpoint + // Surface the resolved authorization / token endpoints to the OAuth + // flow. The resolver returns the endpoints it used for registration + // (caller-supplied if specified, discovered otherwise), so + // downstream OAuth-config construction can rely on these fields + // being populated. + if resolution.AuthorizationEndpoint != "" { + config.AuthorizeURL = resolution.AuthorizationEndpoint + } + if resolution.TokenEndpoint != "" { + config.TokenURL = resolution.TokenEndpoint } return nil } -// getDiscoveryDocument retrieves the OIDC discovery document +// resolveDCRCredentials routes the CLI-flow DCR registration through the +// shared pkg/auth/dcr resolver, inheriting its singleflight deduplication, +// S256 PKCE gating, RFC 7591 §3.2.1 expiry-driven refetch, bearer-token +// transport with redirect refusal, and panic recovery. +// +// The redirect URI is a loopback per RFC 8252 §7.3 — this is the CLI's +// existing public-client model, and PublicClient=true tells the resolver +// to register with token_endpoint_auth_method=none and to refuse when +// S256 PKCE is not advertised (rather than silently downgrading). +// +// Redundant discovery fetch — acknowledged trade-off. The CLI's +// pre-discovery layer (pkg/auth/remote/handler.go via +// ValidateAndDiscoverAuthServer) populates OAuthFlowConfig.AuthorizeURL / +// TokenURL / RegistrationEndpoint but does NOT carry the upstream's +// code_challenge_methods_supported through to here — that field is the +// S256 PKCE gate's only input, and AuthServerInfo does not model it. So +// when the CLI already has endpoints in hand, the resolver MUST still +// re-fetch the upstream's discovery document to evaluate the gate; the +// alternative ("skip the gate on the pre-discovered path") would either +// regress the inherited-S256 acceptance criterion or silently downgrade +// to a non-public-PKCE registration. Threading +// code_challenge_methods_supported through AuthServerInfo (and updating +// every caller of ValidateAndDiscoverAuthServer) would close this loop +// and is the natural follow-up; it is out of scope for sub-issue 4b. +// +// Threat-model correctness in the multi-tenant case: a few upstreams +// (multi-tenant IdPs) serve their OIDC discovery document at a path +// the issuer-derived well-known URL would not reach. For those +// providers the remote handler's pre-discovery already accepted the +// non-well-known URL via DiscoverActualIssuer; the resolver's second +// fetch against {issuer}/.well-known/openid-configuration may then 404. +// The CLI surfaces a clean resolver error in that case rather than +// silently registering. This is a known limitation of the option (b) +// migration and is documented for follow-up. +func resolveDCRCredentials( + ctx context.Context, + issuer string, + config *OAuthFlowConfig, + discoveredDoc *oauthproto.OIDCDiscoveryDocument, +) (*dcr.Resolution, error) { + redirectURI := fmt.Sprintf("http://localhost:%d/callback", config.CallbackPort) + + // dcr.Request.Issuer carries the caller's logical scope for cache + // keying. The CLI flow has no separate logical issuer of its own, so + // it deliberately reuses the upstream's issuer URL here. This is + // safe because the resolver's cache key is (Issuer, RedirectURI, + // ScopesHash) and the CLI always supplies an explicit loopback + // RedirectURI per RFC 8252 §7.3 — even if a future embedded-authserver + // upstream happened to share Issuer with a CLI invocation, the + // distinct RedirectURI keeps the cache keys apart. See the Issuer + // field doc on dcr.Request for the wider semantics. + req := &dcr.Request{ + Issuer: issuer, + RedirectURI: redirectURI, + Scopes: config.Scopes, + AuthorizationEndpoint: discoveredDoc.AuthorizationEndpoint, + TokenEndpoint: discoveredDoc.TokenEndpoint, + PublicClient: true, + } + + // Route the resolver through discovery (not direct registration + // endpoint) so the upstream's code_challenge_methods_supported is + // available to the S256 PKCE gate. See the function-level doc for + // why a second fetch is required even when endpoints are already in + // hand. The discovered metadata.RegistrationEndpoint will round-trip + // through the fetch — explicit AuthorizationEndpoint / TokenEndpoint + // overrides still win for endpoint selection. + // + // discoveredDoc.Issuer is non-empty for every reachable caller (both + // branches of getDiscoveryDocument set it), so the conditional is + // defence-in-depth against a future refactor that produces an empty- + // issuer doc; on that branch DiscoveryURL stays empty and the + // resolver would have nothing to fetch, surfacing a clean validation + // error rather than dereferencing an empty URL. + if discoveredDoc.Issuer != "" { + req.DiscoveryURL = strings.TrimRight(discoveredDoc.Issuer, "/") + "/.well-known/openid-configuration" + } else { + req.RegistrationEndpoint = discoveredDoc.RegistrationEndpoint + } + + store := dcr.NewInMemoryStore() + defer func() { _ = store.Close() }() + + resolution, err := dcr.ResolveCredentials(ctx, req, store) + if err != nil { + // Surface the structured error to the boundary log so operators + // see one slog.Error record with the step / issuer / redirect_uri + // attributes, then wrap with the CLI-facing prefix. + dcr.LogStepError(issuer, err) + return nil, fmt.Errorf("dynamic client registration failed: %w", err) + } + return resolution, nil +} + +// getDiscoveryDocument retrieves the OIDC discovery document. +// +// The "pre-discovered" short-circuit synthesises a document from +// OAuthFlowConfig fields populated by earlier discovery in the remote +// handler (pkg/auth/remote/handler.go via ValidateAndDiscoverAuthServer). +// On that path the synthesised document carries only the endpoint URLs, +// NOT the server-capability fields (code_challenge_methods_supported, +// token_endpoint_auth_methods_supported, scopes_supported) — those +// remain at their zero values. resolveDCRCredentials handles this by +// re-issuing a discovery fetch through the resolver (see its doc for +// the rationale and trade-off). func getDiscoveryDocument( ctx context.Context, issuer string, @@ -720,39 +873,6 @@ func newOAuthFlow(ctx context.Context, oauthConfig *oauth.Config, config *OAuthF }, nil } -func registerDynamicClient( - ctx context.Context, - config *OAuthFlowConfig, - discoveredDoc *oauthproto.OIDCDiscoveryDocument, -) (*oauthproto.DynamicClientRegistrationResponse, error) { - - // Check if the provider supports Dynamic Client Registration. - // The CLI-flag hint below is intentional: this function is CLI-facing - // (pkg/auth/discovery is not a protocol-level package) and the flags - // named here are the correct fallback for operators who need to supply - // credentials manually. The protocol-neutral version of this message lives - // in pkg/oauthproto.handleHTTPResponse for the HTTP 404/405/501 paths. - // TODO(#4978): when authserver wiring is added, consider surfacing a - // more structured error type here so non-CLI consumers can inspect the cause. - if discoveredDoc.RegistrationEndpoint == "" { - return nil, fmt.Errorf("this provider does not support Dynamic Client Registration (DCR). " + - "Please configure OAuth client credentials using --remote-auth-client-id and --remote-auth-client-secret flags, " + - "or register a client manually with the provider") - } - - // Build the CLI-specific DCR request (loopback redirect URI per RFC 8252 Section 7.3) - registrationRequest := NewDynamicClientRegistrationRequest(config.Scopes, config.CallbackPort) - - // Perform dynamic client registration; nil client uses the default HTTP client. - registrationResponse, err := oauthproto.RegisterClientDynamically( - ctx, discoveredDoc.RegistrationEndpoint, registrationRequest, nil) - if err != nil { - return nil, fmt.Errorf("dynamic client registration failed: %w", err) - } - - return registrationResponse, nil -} - // FetchResourceMetadata as specified in RFC 9728 func FetchResourceMetadata(ctx context.Context, metadataURL string) (*auth.RFC9728AuthInfo, error) { if metadataURL == "" { diff --git a/pkg/auth/discovery/discovery_test.go b/pkg/auth/discovery/discovery_test.go index 0ea97a7982..70f9c03305 100644 --- a/pkg/auth/discovery/discovery_test.go +++ b/pkg/auth/discovery/discovery_test.go @@ -18,7 +18,6 @@ import ( "github.com/stretchr/testify/require" "github.com/stacklok/toolhive/pkg/networking" - "github.com/stacklok/toolhive/pkg/oauthproto" ) const wellKnownOAuthPath = "/.well-known/oauth-protected-resource" @@ -1286,43 +1285,57 @@ func TestTryWellKnownDiscovery_ErrorPaths(t *testing.T) { }) } -// TestRegisterDynamicClient_MissingRegistrationEndpoint tests that registerDynamicClient -// returns a clear error message when the OIDC discovery document doesn't include -// a registration_endpoint (provider doesn't support DCR). -func TestRegisterDynamicClient_MissingRegistrationEndpoint(t *testing.T) { +// TestHandleDynamicRegistration_MissingRegistrationEndpoint pins the CLI's +// behaviour when the OIDC discovery document does not advertise a +// registration_endpoint (provider does not support DCR). The CLI surfaces +// a clear error directing the operator to the manual --remote-auth-client-* +// fallback flags. +// +// The check runs before the resolver is invoked: with no +// registration_endpoint there is no upstream URL to register against, so +// the CLI cannot proceed regardless of the resolver's S256 PKCE / synthesis +// behaviour. This keeps the error message stable across both code paths +// (this branch and any future resolver-side synthesis disable). +func TestHandleDynamicRegistration_MissingRegistrationEndpoint(t *testing.T) { t.Parallel() - ctx := context.Background() - - // Create a discovery document without registration_endpoint - discoveredDoc := &oauthproto.OIDCDiscoveryDocument{ - AuthorizationServerMetadata: oauthproto.AuthorizationServerMetadata{ - Issuer: "https://auth.example.com", - AuthorizationEndpoint: "https://auth.example.com/oauth/authorize", - TokenEndpoint: "https://auth.example.com/oauth/token", - JWKSURI: "https://auth.example.com/oauth/jwks", - // Note: RegistrationEndpoint is intentionally omitted (empty string) - RegistrationEndpoint: "", - }, - } + // Mount a metadata document that intentionally omits registration_endpoint. + // The CLI flow's getDiscoveryDocument short-circuit reads AuthorizeURL / + // TokenURL off OAuthFlowConfig and skips network discovery — we set + // AuthorizeURL and TokenURL but leave RegistrationEndpoint empty, which + // falls through to oauth.DiscoverOIDCEndpoints against the httptest + // server below. + var server *httptest.Server + mux := http.NewServeMux() + mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + // registration_endpoint intentionally omitted. + _, _ = w.Write([]byte(`{ + "issuer": "` + server.URL + `", + "authorization_endpoint": "` + server.URL + `/authorize", + "token_endpoint": "` + server.URL + `/token", + "jwks_uri": "` + server.URL + `/jwks", + "response_types_supported": ["code"], + "subject_types_supported": ["public"], + "id_token_signing_alg_values_supported": ["RS256"] + }`)) + }) + server = httptest.NewServer(mux) + t.Cleanup(server.Close) config := &OAuthFlowConfig{ Scopes: []string{"openid", "profile"}, CallbackPort: 8765, } - // Call registerDynamicClient with a discovery document missing registration_endpoint - result, err := registerDynamicClient(ctx, config, discoveredDoc) - - // Should return an error + err := handleDynamicRegistration(context.Background(), server.URL, config) require.Error(t, err) - assert.Nil(t, result) - // Error message should clearly indicate DCR is not supported + // The CLI-flag fallback hint MUST be preserved verbatim — it is the + // load-bearing operator guidance for upstreams that do not advertise + // registration_endpoint. assert.Contains(t, err.Error(), "does not support Dynamic Client Registration") assert.Contains(t, err.Error(), "DCR") - - // Error message should provide actionable guidance assert.Contains(t, err.Error(), "--remote-auth-client-id") assert.Contains(t, err.Error(), "--remote-auth-client-secret") } diff --git a/pkg/authserver/runner/dcr_adapter.go b/pkg/authserver/runner/dcr_adapter.go new file mode 100644 index 0000000000..1f3bc5ddc6 --- /dev/null +++ b/pkg/authserver/runner/dcr_adapter.go @@ -0,0 +1,158 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package runner + +import ( + "fmt" + + "github.com/stacklok/toolhive/pkg/auth/dcr" + "github.com/stacklok/toolhive/pkg/authserver" + "github.com/stacklok/toolhive/pkg/authserver/upstream" +) + +// This file contains the embedded-authserver-specific adapter layer between +// pkg/auth/dcr's profile-neutral API and the authserver's domain types +// (authserver.OAuth2UpstreamRunConfig, upstream.OAuth2Config). Each helper +// here: +// +// - needsDCR reports whether an OAuth2UpstreamRunConfig requires DCR. +// - newDCRRequest converts an OAuth2UpstreamRunConfig into the +// profile-neutral dcr.Request the resolver expects, including resolving +// the file-or-env InitialAccessToken into an inline string. +// - consumeResolution / applyResolutionToOAuth2Config fold the returned +// dcr.Resolution back into the run-config copy and built OAuth2Config. +// +// Keeping these helpers in the runner package (not pkg/auth/dcr) is the +// architectural split the dcr package's profile-agnostic charter requires: +// dcr cannot import authserver, but the embedded authserver freely depends +// on dcr. + +// needsDCR reports whether rc requires runtime Dynamic Client Registration. +// A run-config needs DCR exactly when ClientID is empty and DCRConfig is +// non-nil (the mutually-exclusive constraint is enforced by +// OAuth2UpstreamRunConfig.Validate; this helper is a convenience check). +func needsDCR(rc *authserver.OAuth2UpstreamRunConfig) bool { + if rc == nil { + return false + } + return rc.ClientID == "" && rc.DCRConfig != nil +} + +// newDCRRequest builds the profile-neutral dcr.Request from an +// OAuth2UpstreamRunConfig and this auth server's local issuer. The caller +// has already validated the run-config (Validate() enforces +// ClientID xor DCRConfig and the DCRConfig-internal one-of constraint), so +// this function does not re-check those invariants — it only resolves the +// file-or-env InitialAccessToken reference into an inline string. +// +// localIssuer is *this* auth server's issuer identifier, used by the +// resolver for cache keying and to default the redirect URI to +// {localIssuer}/oauth/callback when rc.RedirectURI is empty. The upstream +// issuer is recovered separately from rc.DCRConfig.DiscoveryURL inside the +// resolver and is used solely for RFC 8414 §3.3 metadata verification. +func newDCRRequest(rc *authserver.OAuth2UpstreamRunConfig, localIssuer string) (*dcr.Request, error) { + if rc == nil { + return nil, fmt.Errorf("oauth2 upstream run-config is required") + } + if rc.DCRConfig == nil { + return nil, fmt.Errorf("dcr: oauth2 upstream has no dcr_config") + } + + initialAccessToken, err := resolveSecret( + rc.DCRConfig.InitialAccessTokenFile, + rc.DCRConfig.InitialAccessTokenEnvVar, + ) + if err != nil { + return nil, fmt.Errorf("dcr: resolve initial access token: %w", err) + } + + return &dcr.Request{ + Issuer: localIssuer, + RedirectURI: rc.RedirectURI, + Scopes: rc.Scopes, + DiscoveryURL: rc.DCRConfig.DiscoveryURL, + RegistrationEndpoint: rc.DCRConfig.RegistrationEndpoint, + AuthorizationEndpoint: rc.AuthorizationEndpoint, + TokenEndpoint: rc.TokenEndpoint, + InitialAccessToken: initialAccessToken, + }, nil +} + +// consumeResolution copies resolved credentials and endpoints from res into +// rc and consumes rc.DCRConfig (sets it to nil), transitioning the run- +// config copy from "DCR-pending" (ClientID == "" && DCRConfig != nil) to +// "DCR-resolved" (ClientID populated && DCRConfig == nil). The "consume" +// name is deliberate: a second call after the first is a no-op only +// because the first cleared DCRConfig — this is a one-shot state +// transition, not an idempotent default-fill. +// +// Callers must pass a COPY of the upstream run-config so the caller's +// original is unaffected; consumeResolution does not clone rc internally. +// +// Why DCRConfig is cleared: OAuth2UpstreamRunConfig.Validate enforces +// ClientID xor DCRConfig — a resolved copy that left DCRConfig set would +// fail the validator that runs downstream in buildPureOAuth2Config. The +// caller's *original* OAuth2Config is unaffected because +// buildUpstreamConfigs deep-copies before resolution; only the post- +// resolution copy is mutated here. +// +// ClientID, the endpoints, and RedirectURI are written only when rc leaves +// them empty — explicit caller configuration always wins. The conditional +// ClientID write is defence-in-depth against future call sites that bypass +// the needsDCR precondition; an unconditional overwrite would silently +// clobber a pre-provisioned ClientID with no error. +// +// The defaulted RedirectURI write closes the loop on resolver-side defaulting: +// when the caller's run-config left RedirectURI empty, the resolver derived +// issuer + /oauth/callback and persisted it on the resolution; copying it +// back here means the downstream upstream.OAuth2Config has a non-empty +// RedirectURI, which authserver.Config validation requires. +// +// Note on ClientSecret: consumeResolution does NOT write the resolved +// secret to rc because OAuth2UpstreamRunConfig models secrets as file-or- +// env references only. To propagate the DCR-resolved secret into the +// final upstream.OAuth2Config, callers must pair this call with +// applyResolutionToOAuth2Config once the config has been built. Keeping +// the two helpers side-by-side localises the DCR-specific application +// logic. +func consumeResolution(rc *authserver.OAuth2UpstreamRunConfig, res *dcr.Resolution) { + if rc == nil || res == nil { + return + } + if rc.ClientID == "" { + rc.ClientID = res.ClientID + } + rc.DCRConfig = nil + if rc.AuthorizationEndpoint == "" { + rc.AuthorizationEndpoint = res.AuthorizationEndpoint + } + if rc.TokenEndpoint == "" { + rc.TokenEndpoint = res.TokenEndpoint + } + if rc.RedirectURI == "" { + rc.RedirectURI = res.RedirectURI + } +} + +// applyResolutionToOAuth2Config overlays the DCR-resolved ClientSecret onto +// a built *upstream.OAuth2Config. This is the companion to +// consumeResolution: where that function writes fields representable in +// the file-or-env run-config model, this one writes the inline-only +// ClientSecret directly on the runtime config. +// +// The split exists because buildPureOAuth2Config intentionally retains a +// narrow file-or-env contract (no DCR awareness) and because OAuth2's +// ClientSecret on the run-config is modelled as a reference rather than +// an inline string. Any future output path from OAuth2UpstreamRunConfig +// to upstream.OAuth2Config must call BOTH consumeResolution (run-config +// side) AND applyResolutionToOAuth2Config (built-config side) to get a +// fully-resolved DCR client. Forgetting the second call leaves +// ClientSecret empty and produces silent auth failures at request time — +// the type system does not enforce the pair, so the invariant lives here. +func applyResolutionToOAuth2Config(cfg *upstream.OAuth2Config, res *dcr.Resolution) { + if cfg == nil || res == nil { + return + } + cfg.ClientSecret = res.ClientSecret +} diff --git a/pkg/authserver/runner/dcr_adapter_test.go b/pkg/authserver/runner/dcr_adapter_test.go new file mode 100644 index 0000000000..3d9c65b27d --- /dev/null +++ b/pkg/authserver/runner/dcr_adapter_test.go @@ -0,0 +1,225 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package runner + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/auth/dcr" + "github.com/stacklok/toolhive/pkg/authserver" + "github.com/stacklok/toolhive/pkg/authserver/upstream" +) + +// Tests in this file pin the embedded-authserver-side adapter that bridges +// pkg/auth/dcr's profile-neutral API to authserver-specific domain types. +// They were lifted from pkg/auth/dcr's resolver_test.go when sub-issue 4b +// neutralised the resolver inputs — the helpers moved to runner/dcr_adapter.go +// so the dcr package no longer imports authserver types, and the tests +// follow. + +func TestNeedsDCR(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + rc *authserver.OAuth2UpstreamRunConfig + expected bool + }{ + {name: "nil", rc: nil, expected: false}, + {name: "empty client_id and dcr_config", rc: &authserver.OAuth2UpstreamRunConfig{ + DCRConfig: &authserver.DCRUpstreamConfig{}, + }, expected: true}, + {name: "client_id without dcr", rc: &authserver.OAuth2UpstreamRunConfig{ + ClientID: "x", + }, expected: false}, + {name: "client_id wins over dcr_config (defensive AND semantic)", rc: &authserver.OAuth2UpstreamRunConfig{ + ClientID: "x", + DCRConfig: &authserver.DCRUpstreamConfig{}, + }, expected: false}, + {name: "both empty", rc: &authserver.OAuth2UpstreamRunConfig{}, expected: false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tc.expected, needsDCR(tc.rc)) + }) + } +} + +func TestConsumeResolution_RespectsExplicitEndpoints(t *testing.T) { + t.Parallel() + + rc := &authserver.OAuth2UpstreamRunConfig{ + AuthorizationEndpoint: "https://explicit/authorize", + TokenEndpoint: "https://explicit/token", + } + res := &dcr.Resolution{ + ClientID: "got-client", + AuthorizationEndpoint: "https://discovered/authorize", + TokenEndpoint: "https://discovered/token", + } + consumeResolution(rc, res) + assert.Equal(t, "got-client", rc.ClientID) + assert.Equal(t, "https://explicit/authorize", rc.AuthorizationEndpoint) + assert.Equal(t, "https://explicit/token", rc.TokenEndpoint) +} + +func TestConsumeResolution_FillsMissingEndpoints(t *testing.T) { + t.Parallel() + + rc := &authserver.OAuth2UpstreamRunConfig{} + res := &dcr.Resolution{ + ClientID: "got-client", + AuthorizationEndpoint: "https://discovered/authorize", + TokenEndpoint: "https://discovered/token", + } + consumeResolution(rc, res) + assert.Equal(t, "got-client", rc.ClientID) + assert.Equal(t, "https://discovered/authorize", rc.AuthorizationEndpoint) + assert.Equal(t, "https://discovered/token", rc.TokenEndpoint) +} + +// TestConsumeResolution_ClearsDCRConfig pins the contract that +// consumeResolution clears DCRConfig on the run-config copy after writing +// the resolved ClientID. Without this, OAuth2UpstreamRunConfig.Validate +// (run by buildPureOAuth2Config downstream) trips its ClientID-xor- +// DCRConfig rule on the resolved copy and rejects the upstream at boot. +func TestConsumeResolution_ClearsDCRConfig(t *testing.T) { + t.Parallel() + + rc := &authserver.OAuth2UpstreamRunConfig{ + DCRConfig: &authserver.DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + }, + } + res := &dcr.Resolution{ + ClientID: "dcr-issued-client", + } + + consumeResolution(rc, res) + + assert.Equal(t, "dcr-issued-client", rc.ClientID) + assert.Nil(t, rc.DCRConfig, + "consumeResolution must clear DCRConfig so the resolved copy satisfies the ClientID-xor-DCRConfig invariant") +} + +// TestConsumeResolution_DoesNotOverwritePreProvisionedClientID verifies +// the defence-in-depth in consumeResolution: a caller that bypasses +// needsDCR and invokes consumeResolution directly with a pre-provisioned +// ClientID does not have it silently clobbered. +func TestConsumeResolution_DoesNotOverwritePreProvisionedClientID(t *testing.T) { + t.Parallel() + + rc := &authserver.OAuth2UpstreamRunConfig{ + ClientID: "pre-provisioned", + } + res := &dcr.Resolution{ + ClientID: "would-be-overwrite", + } + consumeResolution(rc, res) + assert.Equal(t, "pre-provisioned", rc.ClientID, + "consumeResolution must not overwrite a non-empty ClientID") +} + +// TestApplyResolutionToOAuth2Config covers the ClientSecret overlay onto +// the built upstream.OAuth2Config. The split between consumeResolution +// (run-config fields) and applyResolutionToOAuth2Config (inline-only +// ClientSecret) is documented in dcr_adapter.go; both must be paired to +// produce a fully-resolved DCR client. +func TestApplyResolutionToOAuth2Config(t *testing.T) { + t.Parallel() + + cfg := &upstream.OAuth2Config{} + res := &dcr.Resolution{ + ClientSecret: "dcr-issued-secret", + } + applyResolutionToOAuth2Config(cfg, res) + assert.Equal(t, "dcr-issued-secret", cfg.ClientSecret) + + // nil-safe: nil cfg or nil res is a no-op rather than a crash. + applyResolutionToOAuth2Config(nil, res) + applyResolutionToOAuth2Config(cfg, nil) +} + +// TestNewDCRRequest covers the OAuth2UpstreamRunConfig → dcr.Request +// translation, including the file-based InitialAccessToken resolution that +// previously lived inside the resolver. +func TestNewDCRRequest(t *testing.T) { + t.Parallel() + + tokenPath := filepath.Join(t.TempDir(), "iat") + require.NoError(t, os.WriteFile(tokenPath, []byte("iat-value\n"), 0o600)) + + tests := []struct { + name string + rc *authserver.OAuth2UpstreamRunConfig + localIssuer string + wantErrSub string + wantIssuer string + wantInitialAccessToken string + wantDiscoveryURL string + wantRegistration string + }{ + { + name: "discovery_url branch resolves file-based initial access token", + rc: &authserver.OAuth2UpstreamRunConfig{ + Scopes: []string{"openid"}, + DCRConfig: &authserver.DCRUpstreamConfig{ + DiscoveryURL: "https://idp.example.com/.well-known/oauth-authorization-server", + InitialAccessTokenFile: tokenPath, + }, + }, + localIssuer: "https://thv.example.com", + wantIssuer: "https://thv.example.com", + wantInitialAccessToken: "iat-value", + wantDiscoveryURL: "https://idp.example.com/.well-known/oauth-authorization-server", + }, + { + name: "registration_endpoint branch propagates without secret", + rc: &authserver.OAuth2UpstreamRunConfig{ + Scopes: []string{"openid"}, + DCRConfig: &authserver.DCRUpstreamConfig{ + RegistrationEndpoint: "https://idp.example.com/register", + }, + }, + localIssuer: "https://thv.example.com", + wantIssuer: "https://thv.example.com", + wantRegistration: "https://idp.example.com/register", + }, + { + name: "nil run-config rejected", + rc: nil, + localIssuer: "https://thv.example.com", + wantErrSub: "run-config is required", + }, + { + name: "missing dcr_config rejected", + rc: &authserver.OAuth2UpstreamRunConfig{}, + localIssuer: "https://thv.example.com", + wantErrSub: "no dcr_config", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + req, err := newDCRRequest(tc.rc, tc.localIssuer) + if tc.wantErrSub != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErrSub) + return + } + require.NoError(t, err) + require.NotNil(t, req) + assert.Equal(t, tc.wantIssuer, req.Issuer) + assert.Equal(t, tc.wantInitialAccessToken, req.InitialAccessToken) + assert.Equal(t, tc.wantDiscoveryURL, req.DiscoveryURL) + assert.Equal(t, tc.wantRegistration, req.RegistrationEndpoint) + }) + } +} diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index c036d0cb1f..c02d68c6eb 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -356,10 +356,10 @@ func parseTokenLifespans(cfg *authserver.TokenLifespanRunConfig) (access, refres // RFC 7591 Dynamic Client Registration against the upstream authorization // server (hitting the network on first call, using dcrStore on subsequent // calls) and overlays the resulting ClientID / ClientSecret onto the output -// config via dcr.ConsumeResolution + dcr.ApplyResolutionToOAuth2Config. The -// caller's runConfigs slice is not mutated: in-place mutation of -// caller-provided values surprises callers and can cause data races, so -// each element is cloned before applying DCR resolution. +// config via consumeResolution + applyResolutionToOAuth2Config (see +// dcr_adapter.go). The caller's runConfigs slice is not mutated: in-place +// mutation of caller-provided values surprises callers and can cause data +// races, so each element is cloned before applying DCR resolution. // // Error logging: this function is the boundary for DCR errors — on any // failure from dcr.ResolveCredentials it emits exactly one structured @@ -380,17 +380,21 @@ func buildUpstreamConfigs( rcCopy := rc var dcrResolution *dcr.Resolution - // dcr.NeedsDCR returns false for nil input, so the explicit Type == + // needsDCR returns false for nil input, so the explicit Type == // OAuth2 guard is redundant. Keeping a single source of truth for // "does this upstream require DCR" avoids drift if the condition // ever needs to be extended (e.g., to support OIDC DCR). - if dcr.NeedsDCR(rcCopy.OAuth2Config) { - // Deep-copy the OAuth2 sub-config so dcr.ConsumeResolution writes to the + if needsDCR(rcCopy.OAuth2Config) { + // Deep-copy the OAuth2 sub-config so consumeResolution writes to the // copy, not the caller's OAuth2UpstreamRunConfig pointer. o2Copy := *rcCopy.OAuth2Config rcCopy.OAuth2Config = &o2Copy - resolution, err := dcr.ResolveCredentials(ctx, &o2Copy, issuer, dcrStore) + req, err := newDCRRequest(&o2Copy, issuer) + if err != nil { + return nil, fmt.Errorf("upstream %q: %w", rc.Name, err) + } + resolution, err := dcr.ResolveCredentials(ctx, req, dcrStore) if err != nil { // Emit the single boundary Error record with enough context to // correlate the failure back to this upstream; then return the @@ -398,7 +402,7 @@ func buildUpstreamConfigs( dcr.LogStepError(rc.Name, err) return nil, fmt.Errorf("upstream %q: %w", rc.Name, err) } - dcr.ConsumeResolution(&o2Copy, resolution) + consumeResolution(&o2Copy, resolution) dcrResolution = resolution } @@ -408,12 +412,12 @@ func buildUpstreamConfigs( } // Apply the DCR-resolved ClientSecret to the built OAuth2Config. - // The split between dcr.ConsumeResolution (run-config fields) and - // dcr.ApplyResolutionToOAuth2Config (inline-only ClientSecret) is - // documented in pkg/auth/dcr/resolver.go — both calls must be - // paired to produce a fully-resolved DCR client. + // The split between consumeResolution (run-config fields) and + // applyResolutionToOAuth2Config (inline-only ClientSecret) is + // documented in dcr_adapter.go — both calls must be paired to + // produce a fully-resolved DCR client. if dcrResolution != nil && cfg.OAuth2Config != nil { - dcr.ApplyResolutionToOAuth2Config(cfg.OAuth2Config, dcrResolution) + applyResolutionToOAuth2Config(cfg.OAuth2Config, dcrResolution) } configs = append(configs, *cfg) diff --git a/pkg/authserver/runner/embeddedauthserver_test.go b/pkg/authserver/runner/embeddedauthserver_test.go index 10f49722a3..14259b955e 100644 --- a/pkg/authserver/runner/embeddedauthserver_test.go +++ b/pkg/authserver/runner/embeddedauthserver_test.go @@ -286,17 +286,15 @@ func TestParseTokenLifespans(t *testing.T) { }) } -// TestResolveSecret pins the runner-package resolveSecret against the -// observable contract its parallel twin in pkg/auth/dcr/secret_test.go -// (TestResolveSecret + TestResolveSecretWithEnvVar) covers. The two -// helpers are byte-identical by deliberate duplication (see the doc -// comment on each func resolveSecret); a behaviour change here without -// the same change in pkg/auth/dcr/resolver.go::resolveSecret will be -// caught by the dcr-side suite, and vice versa. Keep the two suites -// behaviourally in lockstep — when consolidating both copies into a -// shared helper (planned for sub-issue 4b, #5219), this drift-guard -// pair becomes unnecessary and SHOULD be replaced with a single -// authoritative suite. +// TestResolveSecret pins the observable contract of the runner-package +// resolveSecret helper: file-precedence, whitespace-trimming, and the +// explicit error modes for missing-file / unset-env. resolveSecret is +// the single authoritative implementation in the codebase; the +// pkg/auth/dcr package no longer carries a parallel copy (removed in +// #5219 sub-issue 4b, when the resolver's input was neutralised and the +// embedded-authserver adapter took responsibility for resolving the +// file-or-env reference into Request.InitialAccessToken at the call +// site). func TestResolveSecret(t *testing.T) { t.Parallel() @@ -346,9 +344,6 @@ func TestResolveSecret(t *testing.T) { // TestResolveSecretWithEnvVar tests resolveSecret with environment variables. // These tests cannot use t.Parallel() because they use t.Setenv(). -// -// Drift-guard twin: see the doc comment on TestResolveSecret above. -// The dcr-side parallel suite lives at pkg/auth/dcr/secret_test.go. func TestResolveSecretWithEnvVar(t *testing.T) { t.Run("file takes precedence over env var", func(t *testing.T) { tmpDir := t.TempDir()