From df84256f2f9a44c20f90b74cdafbf9c2527be8b0 Mon Sep 17 00:00:00 2001 From: Trey Date: Tue, 5 May 2026 11:24:15 -0700 Subject: [PATCH 1/2] Extract DCR resolver into pkg/auth/dcr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sub-issue 4a of #5145. Creates the shared pkg/auth/dcr package and migrates the embedded authserver to consume it. The CLI flow migration (pkg/auth/discovery::PerformOAuthFlow) is left to a follow-up so this slice stays under the project's PR-size limit. Files moved (renamed via git mv so the diff shows as renames, not deletions + additions): pkg/authserver/runner/dcr.go -> pkg/auth/dcr/resolver.go pkg/authserver/runner/dcr_store.go -> pkg/auth/dcr/store.go pkg/authserver/runner/dcr_test.go -> pkg/auth/dcr/resolver_test.go pkg/authserver/runner/dcr_store_test.go -> pkg/auth/dcr/store_test.go Public API surface (renamed from runner-package internals because they now cross a package boundary): Resolution (was DCRResolution) Key (was DCRKey, alias to storage.DCRKey unchanged) CredentialStore (was DCRCredentialStore) ResolveCredentials (was resolveDCRCredentials) NeedsDCR (was needsDCR) ConsumeResolution (was consumeResolution) ApplyResolutionToOAuth2Config (was applyResolutionToOAuth2Config) LogStepError (was logDCRStepError) NewInMemoryStore (was NewInMemoryDCRCredentialStore) NewStorageBackedStore (was newStorageBackedStore) Names follow the pkg/auth/dcr.* form so the linter's stuttering check (revive: dcr.DCRResolution would stutter) passes and the surface reads as ordinary package API. resolveSecret was duplicated into the dcr package because pkg/auth/dcr must stay profile-agnostic and cannot reach back into pkg/authserver/ runner. The duplication is intentional and called out in a comment; future consolidation can move it into a shared helper if a third caller appears. Acceptance criteria status (#5145): AC#1 (pkg/auth/dcr exists, exports a stateful resolver, consumed by both call sites): partially met — package exists and is consumed by EmbeddedAuthServer. CLI flow migration is sub-issue 4b. AC#2 (stateless RFC 7591 helpers in pkg/oauthproto): met (already satisfied before this PR). AC#3 (no direct oauthproto.RegisterClientDynamically calls outside pkg/auth/dcr): not yet met — pkg/auth/discovery still calls it. Resolved by sub-issue 4b. AC#4 (review-property behaviours apply to CLI flow): not yet met — same dependency on 4b. Verified via task lint-fix (0 issues), go test -race ./pkg/auth/dcr/... ./pkg/authserver/... (all pass), and task license-check (clean). --- .../runner/dcr.go => auth/dcr/resolver.go} | 162 +++++++++++------ .../dcr_test.go => auth/dcr/resolver_test.go} | 168 +++++++++--------- .../runner/dcr_store.go => auth/dcr/store.go} | 79 ++++---- .../dcr/store_test.go} | 84 ++++----- pkg/authserver/runner/embeddedauthserver.go | 33 ++-- .../runner/embeddedauthserver_test.go | 13 +- 6 files changed, 295 insertions(+), 244 deletions(-) rename pkg/{authserver/runner/dcr.go => auth/dcr/resolver.go} (90%) rename pkg/{authserver/runner/dcr_test.go => auth/dcr/resolver_test.go} (92%) rename pkg/{authserver/runner/dcr_store.go => auth/dcr/store.go} (67%) rename pkg/{authserver/runner/dcr_store_test.go => auth/dcr/store_test.go} (73%) diff --git a/pkg/authserver/runner/dcr.go b/pkg/auth/dcr/resolver.go similarity index 90% rename from pkg/authserver/runner/dcr.go rename to pkg/auth/dcr/resolver.go index b097bb7d5a..a0abb16d71 100644 --- a/pkg/authserver/runner/dcr.go +++ b/pkg/auth/dcr/resolver.go @@ -1,15 +1,33 @@ // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. // SPDX-License-Identifier: Apache-2.0 -package runner +// Package dcr is the shared RFC 7591 Dynamic Client Registration client used +// by every consumer in the codebase that needs to register a downstream +// OAuth 2.x client at runtime. The package owns the stateful concerns of the +// flow — credential cache, in-process singleflight deduplication, scope-set +// canonicalisation, token-endpoint auth-method selection (with the RFC 7636 / +// OAuth 2.1 S256 PKCE gate), RFC 7591 §3.2.1 expiry-driven cache invalidation, +// the bearer-token transport with redirect refusal, and panic recovery around +// the registration body. Stateless RFC 7591 wire-shape primitives live in +// pkg/oauthproto. +// +// Profile differences (public PKCE vs confidential client, redirect URI +// policy, persistence backend) stay at the call site — the package is +// profile-agnostic and lets each consumer plug a CredentialStore in. +// +// 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" @@ -25,14 +43,14 @@ import ( "github.com/stacklok/toolhive/pkg/oauthproto" ) -// dcrFlight coalesces concurrent resolveDCRCredentials calls that share the -// same DCRKey. Two goroutines hitting the resolver for the same upstream and +// dcrFlight coalesces concurrent ResolveCredentials calls that share the +// same Key. Two goroutines hitting the resolver for the same upstream and // scope set will both miss the cache, so without coalescing they would both // call RegisterClientDynamically and the loser's registration would become // orphaned at the upstream IdP — an operator-visible cleanup task and // possibly a transient startup failure if the upstream rate-limits // concurrent registrations. Followers wait for the leader's result and -// observe the same DCRResolution. +// observe the same Resolution. // // Lifetime: process-wide. This intentionally contrasts with // EmbeddedAuthServer.dcrStore, which is per-instance. The asymmetry is @@ -67,13 +85,13 @@ var authMethodPreference = []string{ "none", } -// DCRResolution captures the full RFC 7591 + RFC 7592 response for a +// Resolution captures the full RFC 7591 + RFC 7592 response for a // successful Dynamic Client Registration, together with the endpoints the // upstream advertises so the caller need not re-discover them. // -// The struct is the unit of storage in DCRCredentialStore and the unit of -// application via consumeResolution. -type DCRResolution struct { +// The struct is the unit of storage in CredentialStore and the unit of +// application via ConsumeResolution. +type Resolution struct { // ClientID is the RFC 7591 "client_id" returned by the authorization // server. ClientID string @@ -107,7 +125,7 @@ type DCRResolution struct { // during registration. When the caller's run-config 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 + // ConsumeResolution write it back onto the run-config COPY so that // downstream consumers (buildPureOAuth2Config, upstream.OAuth2Config // validation) see a non-empty RedirectURI. RedirectURI string @@ -137,18 +155,18 @@ type DCRResolution struct { CreatedAt time.Time } -// needsDCR reports whether rc requires runtime Dynamic Client Registration. +// 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 { +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 +// 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" @@ -157,7 +175,7 @@ func needsDCR(rc *authserver.OAuth2UpstreamRunConfig) bool { // 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. +// 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 @@ -179,14 +197,14 @@ func needsDCR(rc *authserver.OAuth2UpstreamRunConfig) bool { // 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 +// 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 +// 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 *DCRResolution) { +func ConsumeResolution(rc *authserver.OAuth2UpstreamRunConfig, res *Resolution) { if rc == nil || res == nil { return } @@ -205,9 +223,9 @@ func consumeResolution(rc *authserver.OAuth2UpstreamRunConfig, res *DCRResolutio } } -// applyResolutionToOAuth2Config overlays the DCR-resolved ClientSecret onto +// ApplyResolutionToOAuth2Config overlays the DCR-resolved ClientSecret onto // a built *upstream.OAuth2Config. This is the companion to -// consumeResolution: where that function writes fields representable in +// 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. // @@ -215,27 +233,27 @@ func consumeResolution(rc *authserver.OAuth2UpstreamRunConfig, res *DCRResolutio // 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 +// 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 *DCRResolution) { +func ApplyResolutionToOAuth2Config(cfg *upstream.OAuth2Config, res *Resolution) { if cfg == nil || res == nil { return } cfg.ClientSecret = res.ClientSecret } -// scopesHash is a runner-package shorthand for storage.ScopesHash, kept so the +// scopesHash is a package-local shorthand for storage.ScopesHash, kept so the // resolver and its tests can reference the canonical hash function without an // explicit storage. qualifier on every call site. The canonical implementation -// lives in the storage package next to DCRKey so any future backend hashes -// keys identically. +// lives in the storage package next to storage.DCRKey so any future backend +// hashes keys identically. var scopesHash = storage.ScopesHash // Step identifiers for structured error logs emitted by the caller of -// resolveDCRCredentials. These values flow through the "step" attribute so +// ResolveCredentials. These values flow through the "step" attribute so // operators can narrow failures to a specific phase without parsing error // messages. They are reported only at the boundary log — see // dcrStepError — so a single failure produces a single slog.Error record. @@ -252,13 +270,13 @@ const ( // dcrStepError annotates a resolver error with the phase it was produced // in. The boundary caller (buildUpstreamConfigs) emits the single // slog.Error record for the failure; individual error branches inside -// resolveDCRCredentials do not log so that each failure surfaces exactly +// ResolveCredentials do not log so that each failure surfaces exactly // once in the combined log stream. // // RedirectURI is included when known so that operators can correlate the // failure with a specific upstream registration without parsing the // wrapped error string. Stack carries a captured stack trace for the -// dcrStepRegister panic-recovery branch so logDCRStepError can include +// dcrStepRegister panic-recovery branch so LogStepError can include // it in the single boundary record without the in-defer site emitting // its own duplicate slog.Error. A zero-value dcrStepError is invalid; // construct via newDCRStepError or the resolver's internal helpers. @@ -290,7 +308,7 @@ func newDCRStepError(step, issuer, redirectURI string, err error) *dcrStepError } } -// resolveDCRCredentials performs Dynamic Client Registration for rc against +// 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. @@ -307,7 +325,7 @@ func newDCRStepError(step, issuer, redirectURI string, err error) *dcrStepError // redirect and a cache key that does not identify the auth-server 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 +// of rc via ConsumeResolution (per the copy-before-mutate rule). This function // neither mutates rc nor the cache on failure. // // Observability: this function never calls slog.Error directly — all @@ -319,12 +337,12 @@ func newDCRStepError(step, issuer, redirectURI string, err error) *dcrStepError // outer-frame equivalent. No secret values (client_secret, // registration_access_token, initial_access_token) are ever logged — only // public metadata such as client_id and redirect_uri. -func resolveDCRCredentials( +func ResolveCredentials( ctx context.Context, rc *authserver.OAuth2UpstreamRunConfig, localIssuer string, - cache DCRCredentialStore, -) (*DCRResolution, error) { + cache CredentialStore, +) (*Resolution, error) { if err := validateResolveInputs(rc, localIssuer, cache); err != nil { return nil, newDCRStepError(dcrStepValidate, localIssuer, "", err) } @@ -336,7 +354,7 @@ func resolveDCRCredentials( } scopes := slices.Clone(rc.Scopes) - key := DCRKey{ + key := Key{ Issuer: localIssuer, RedirectURI: redirectURI, ScopesHash: scopesHash(scopes), @@ -349,19 +367,19 @@ func resolveDCRCredentials( return cached, nil } - // Coalesce concurrent registrations for the same DCRKey — see dcrFlight + // Coalesce concurrent registrations for the same Key — see dcrFlight // doc comment. The leader runs the registerOnce closure; followers - // receive the leader's *DCRResolution result. The flight key embeds the - // DCRKey fields with a separator that cannot appear in any of them + // receive the leader's *Resolution result. The flight key embeds the + // Key fields with a separator that cannot appear in any of them // (newline is not valid in OAuth scope tokens, URLs, or hex digests). // // A defer/recover inside the closure converts a panic in registerAndCache // (or anything it calls) into a normal error. Without this, singleflight // re-panics the leader's panic in every follower — N concurrent callers - // for the same DCRKey would all crash with the same value. The panic is + // for the same Key would all crash with the same value. The panic is // still surfaced: the captured stack trace is attached to the wrapped // dcrStepError and surfaces in the single boundary log emitted by - // logDCRStepError, so the failure produces exactly one Error record (no + // LogStepError, so the failure produces exactly one Error record (no // in-defer log here) and callers can react to it as a normal failure. flightKey := key.Issuer + "\n" + key.RedirectURI + "\n" + key.ScopesHash resolutionAny, err, _ := dcrFlight.Do(flightKey, func() (res any, err error) { @@ -379,10 +397,10 @@ func resolveDCRCredentials( if err != nil { return nil, err } - return resolutionAny.(*DCRResolution), nil + return resolutionAny.(*Resolution), nil } -// registerAndCache is the leader-only body of resolveDCRCredentials wrapped +// registerAndCache is the leader-only body of ResolveCredentials wrapped // by the singleflight. It rechecks the cache before any network I/O so // followers that arrive after the leader's Put returns immediately see the // fresh entry on a subsequent call. Endpoint resolution, registration, and @@ -392,9 +410,9 @@ func registerAndCache( rc *authserver.OAuth2UpstreamRunConfig, localIssuer, redirectURI string, scopes []string, - key DCRKey, - cache DCRCredentialStore, -) (*DCRResolution, error) { + 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 { @@ -454,10 +472,10 @@ func registerAndCache( return resolution, nil } -// logDCRStepError emits the single boundary slog.Error record for a DCR +// LogStepError emits the single boundary slog.Error record for a DCR // resolver failure, carrying the step / issuer / redirect_uri attributes // extracted from err. If err is not a *dcrStepError, it is logged with a -// generic "unknown" step — resolveDCRCredentials always wraps its errors, +// generic "unknown" step — ResolveCredentials always wraps its errors, // so this branch indicates a programming error in a future caller rather // than a runtime condition. err == nil is a no-op so this function is // safe to call without an outer guard. @@ -467,7 +485,7 @@ func registerAndCache( // in depth — the current DCR flow sends the initial access token as an // Authorization header, not a query parameter, but nothing in the type // system prevents a future refactor from doing otherwise). -func logDCRStepError(upstreamName string, err error) { +func LogStepError(upstreamName string, err error) { if err == nil { return } @@ -585,12 +603,12 @@ 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 -// resolveDCRCredentials is an entry point that programmatic callers can +// ResolveCredentials is an entry point that programmatic callers can // reach with partially-constructed run-configs. func validateResolveInputs( rc *authserver.OAuth2UpstreamRunConfig, localIssuer string, - cache DCRCredentialStore, + cache CredentialStore, ) error { if rc == nil { return fmt.Errorf("oauth2 upstream run-config is required") @@ -634,10 +652,10 @@ func validateResolveInputs( // trigger. func lookupCachedResolution( ctx context.Context, - cache DCRCredentialStore, - key DCRKey, + cache CredentialStore, + key Key, localIssuer, redirectURI string, -) (*DCRResolution, bool, error) { +) (*Resolution, bool, error) { cached, ok, err := cache.Get(ctx, key) if err != nil { return nil, false, fmt.Errorf("dcr: cache lookup: %w", err) @@ -749,13 +767,13 @@ func performRegistration( return response, nil } -// buildResolution assembles the DCRResolution from the RFC 7591 response and +// buildResolution assembles the Resolution from the RFC 7591 response and // the resolved endpoints. If the server did not echo a // token_endpoint_auth_method in the response, the method actually sent is // 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. +// 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 @@ -766,12 +784,12 @@ func buildResolution( endpoints *dcrEndpoints, sentAuthMethod string, redirectURI string, -) *DCRResolution { +) *Resolution { authMethod := response.TokenEndpointAuthMethod if authMethod == "" { authMethod = sentAuthMethod } - return &DCRResolution{ + return &Resolution{ ClientID: response.ClientID, ClientSecret: response.ClientSecret, AuthorizationEndpoint: endpoints.authorizationEndpoint, @@ -797,7 +815,7 @@ func epochSecondsToTime(epoch int64) time.Time { } // dcrEndpoints is the internal bundle of endpoints produced by endpoint -// resolution. The separation from DCRResolution lets the resolver reason +// resolution. The separation from Resolution lets the resolver reason // about discovered vs. overridden values before committing to a resolution. type dcrEndpoints struct { authorizationEndpoint string @@ -831,7 +849,7 @@ type dcrEndpoints struct { // 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 resolveDCRCredentials for redirect URI defaulting and +// 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 @@ -1194,3 +1212,33 @@ 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. Future consolidation can move this into a shared +// pkg/secrets-style helper if a third caller appears. +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/authserver/runner/dcr_test.go b/pkg/auth/dcr/resolver_test.go similarity index 92% rename from pkg/authserver/runner/dcr_test.go rename to pkg/auth/dcr/resolver_test.go index 7be979aa45..b7cf32bf93 100644 --- a/pkg/authserver/runner/dcr_test.go +++ b/pkg/auth/dcr/resolver_test.go @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. // SPDX-License-Identifier: Apache-2.0 -package runner +package dcr import ( "context" @@ -139,18 +139,18 @@ func TestResolveDCRCredentials_CacheHitShortCircuits(t *testing.T) { })) t.Cleanup(server.Close) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL // Pre-populate the cache with a resolution matching the key we will // look up. redirectURI := issuer + "/oauth/callback" - key := DCRKey{ + key := Key{ Issuer: issuer, RedirectURI: redirectURI, ScopesHash: scopesHash([]string{"openid", "profile"}), } - preloaded := &DCRResolution{ + preloaded := &Resolution{ ClientID: "preloaded-id", ClientSecret: "preloaded-secret", AuthorizationEndpoint: "https://preloaded/authorize", @@ -165,7 +165,7 @@ func TestResolveDCRCredentials_CacheHitShortCircuits(t *testing.T) { }, } - got, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + got, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, "preloaded-id", got.ClientID) assert.Equal(t, "preloaded-secret", got.ClientSecret) @@ -187,7 +187,7 @@ func TestResolveDCRCredentials_RegistersOnCacheMiss(t *testing.T) { }, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid", "profile"}, @@ -196,7 +196,7 @@ func TestResolveDCRCredentials_RegistersOnCacheMiss(t *testing.T) { }, } - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, "test-client-id", res.ClientID) assert.Equal(t, "test-client-secret", res.ClientSecret) @@ -217,7 +217,7 @@ func TestResolveDCRCredentials_RegistersOnCacheMiss(t *testing.T) { // Cache was populated. cached, ok, err := cache.Get(context.Background(), - DCRKey{Issuer: issuer, RedirectURI: issuer + "/oauth/callback", ScopesHash: scopesHash([]string{"openid", "profile"})}) + Key{Issuer: issuer, RedirectURI: issuer + "/oauth/callback", ScopesHash: scopesHash([]string{"openid", "profile"})}) require.NoError(t, err) require.True(t, ok) assert.Equal(t, "test-client-id", cached.ClientID) @@ -227,7 +227,7 @@ func TestResolveDCRCredentials_ExplicitEndpointsOverride(t *testing.T) { t.Parallel() server := newDCRTestServer(t, dcrTestHandlerConfig{}) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ @@ -239,7 +239,7 @@ func TestResolveDCRCredentials_ExplicitEndpointsOverride(t *testing.T) { }, } - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, 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) @@ -262,7 +262,7 @@ func TestResolveDCRCredentials_InitialAccessTokenAsBearer(t *testing.T) { tokenPath := filepath.Join(t.TempDir(), "iat") require.NoError(t, os.WriteFile(tokenPath, []byte("iat-secret-value\n"), 0o600)) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -272,7 +272,7 @@ func TestResolveDCRCredentials_InitialAccessTokenAsBearer(t *testing.T) { }, } - _, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, "Bearer iat-secret-value", gotAuthHeader) } @@ -326,7 +326,7 @@ func TestResolveDCRCredentials_DoesNotForwardBearerOnRedirect(t *testing.T) { tokenPath := filepath.Join(t.TempDir(), "iat") require.NoError(t, os.WriteFile(tokenPath, []byte("iat-secret-value\n"), 0o600)) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := upstream.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -336,7 +336,7 @@ func TestResolveDCRCredentials_DoesNotForwardBearerOnRedirect(t *testing.T) { }, } - _, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), rc, issuer, 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") @@ -395,7 +395,7 @@ func TestResolveDCRCredentials_AuthMethodPreference(t *testing.T) { tokenEndpointAuthMethodsSupported: tc.supported, codeChallengeMethodsSupported: tc.codeChallenge, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -404,7 +404,7 @@ func TestResolveDCRCredentials_AuthMethodPreference(t *testing.T) { }, } - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, tc.expected, res.TokenEndpointAuthMethod) }) @@ -434,7 +434,7 @@ func TestResolveDCRCredentials_RefusesNoneWithoutS256(t *testing.T) { tokenEndpointAuthMethodsSupported: []string{"none"}, codeChallengeMethodsSupported: tc.codeChallenge, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -443,7 +443,7 @@ func TestResolveDCRCredentials_RefusesNoneWithoutS256(t *testing.T) { }, } - _, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.Error(t, err) assert.Contains(t, err.Error(), "S256", "error must mention the missing S256 advertisement so operators can correlate") @@ -460,7 +460,7 @@ func TestResolveDCRCredentials_EmptyAuthMethodIntersectionErrors(t *testing.T) { server := newDCRTestServer(t, dcrTestHandlerConfig{ tokenEndpointAuthMethodsSupported: []string{"tls_client_auth"}, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -468,7 +468,7 @@ func TestResolveDCRCredentials_EmptyAuthMethodIntersectionErrors(t *testing.T) { DiscoveryURL: issuer + "/.well-known/oauth-authorization-server", }, } - _, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.Error(t, err) assert.Contains(t, err.Error(), "no supported token_endpoint_auth_method") } @@ -486,7 +486,7 @@ func TestResolveDCRCredentials_SynthesisedRegistrationEndpoint(t *testing.T) { gotPath = r.URL.Path }, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -495,7 +495,7 @@ func TestResolveDCRCredentials_SynthesisedRegistrationEndpoint(t *testing.T) { }, } - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, "test-client-id", res.ClientID) assert.Equal(t, "/register", gotPath) @@ -519,7 +519,7 @@ func TestResolveDCRCredentials_RegistrationEndpointDirectBypassesDiscovery(t *te server := httptest.NewServer(mux) t.Cleanup(server.Close) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ AuthorizationEndpoint: issuer + "/authorize", @@ -530,7 +530,7 @@ func TestResolveDCRCredentials_RegistrationEndpointDirectBypassesDiscovery(t *te }, } - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, "direct-id", res.ClientID) assert.Equal(t, int32(0), atomic.LoadInt32(&discoveryHits), @@ -553,35 +553,35 @@ func TestResolveDCRCredentials_RejectsInvalidInputs(t *testing.T) { name string rc *authserver.OAuth2UpstreamRunConfig issuer string - cache DCRCredentialStore + cache CredentialStore wantErrSub string }{ { name: "nil run-config", rc: nil, issuer: "https://example.com", - cache: NewInMemoryDCRCredentialStore(), + cache: NewInMemoryStore(), wantErrSub: "oauth2 upstream run-config is required", }, { name: "pre-provisioned client_id", rc: &authserver.OAuth2UpstreamRunConfig{ClientID: "preprovisioned", DCRConfig: validCfg}, issuer: "https://example.com", - cache: NewInMemoryDCRCredentialStore(), + cache: NewInMemoryStore(), wantErrSub: "pre-provisioned", }, { name: "missing dcr_config", rc: &authserver.OAuth2UpstreamRunConfig{}, issuer: "https://example.com", - cache: NewInMemoryDCRCredentialStore(), + cache: NewInMemoryStore(), wantErrSub: "no dcr_config", }, { name: "empty issuer", rc: &authserver.OAuth2UpstreamRunConfig{DCRConfig: validCfg}, issuer: "", - cache: NewInMemoryDCRCredentialStore(), + cache: NewInMemoryStore(), wantErrSub: "issuer is required", }, { @@ -595,7 +595,7 @@ func TestResolveDCRCredentials_RejectsInvalidInputs(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - _, err := resolveDCRCredentials(context.Background(), tc.rc, tc.issuer, tc.cache) + _, err := ResolveCredentials(context.Background(), tc.rc, tc.issuer, tc.cache) require.Error(t, err) assert.Contains(t, err.Error(), tc.wantErrSub) }) @@ -626,7 +626,7 @@ func TestNeedsDCR(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tc.expected, needsDCR(tc.rc)) + assert.Equal(t, tc.expected, NeedsDCR(tc.rc)) }) } } @@ -638,12 +638,12 @@ func TestConsumeResolution_RespectsExplicitEndpoints(t *testing.T) { AuthorizationEndpoint: "https://explicit/authorize", TokenEndpoint: "https://explicit/token", } - res := &DCRResolution{ + res := &Resolution{ ClientID: "got-client", AuthorizationEndpoint: "https://discovered/authorize", TokenEndpoint: "https://discovered/token", } - consumeResolution(rc, res) + 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) @@ -653,19 +653,19 @@ func TestConsumeResolution_FillsMissingEndpoints(t *testing.T) { t.Parallel() rc := &authserver.OAuth2UpstreamRunConfig{} - res := &DCRResolution{ + res := &Resolution{ ClientID: "got-client", AuthorizationEndpoint: "https://discovered/authorize", TokenEndpoint: "https://discovered/token", } - consumeResolution(rc, res) + 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 +// 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. @@ -677,15 +677,15 @@ func TestConsumeResolution_ClearsDCRConfig(t *testing.T) { RegistrationEndpoint: "https://idp.example.com/register", }, } - res := &DCRResolution{ + res := &Resolution{ ClientID: "dcr-issued-client", } - consumeResolution(rc, res) + 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") + "ConsumeResolution must clear DCRConfig so the resolved copy satisfies the ClientID-xor-DCRConfig invariant") } func TestResolveUpstreamRedirectURI(t *testing.T) { @@ -781,7 +781,7 @@ func TestResolveDCRCredentials_DiscoveryURLHonoured(t *testing.T) { server = httptest.NewServer(mux) t.Cleanup(server.Close) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -790,7 +790,7 @@ func TestResolveDCRCredentials_DiscoveryURLHonoured(t *testing.T) { }, } - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, "tenant-client", res.ClientID) assert.Equal(t, int32(1), atomic.LoadInt32(&discoveryHits), @@ -822,7 +822,7 @@ func TestResolveDCRCredentials_DiscoveryURLIssuerMismatchRejected(t *testing.T) server := httptest.NewServer(mux) t.Cleanup(server.Close) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -831,7 +831,7 @@ func TestResolveDCRCredentials_DiscoveryURLIssuerMismatchRejected(t *testing.T) }, } - _, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.Error(t, err) assert.Contains(t, err.Error(), "issuer mismatch") } @@ -849,7 +849,7 @@ func TestResolveDCRCredentials_DiscoveredScopesFallback(t *testing.T) { gotBody = body }, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ // Scopes intentionally left empty so the resolver falls back to @@ -859,7 +859,7 @@ func TestResolveDCRCredentials_DiscoveredScopesFallback(t *testing.T) { }, } - _, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) var req oauthproto.DynamicClientRegistrationRequest @@ -881,7 +881,7 @@ func TestResolveDCRCredentials_EmptyScopesOmitted(t *testing.T) { gotBody = body }, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ DCRConfig: &authserver.DCRUpstreamConfig{ @@ -889,7 +889,7 @@ func TestResolveDCRCredentials_EmptyScopesOmitted(t *testing.T) { }, } - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.Equal(t, "test-client-id", res.ClientID) @@ -918,7 +918,7 @@ func TestResolveDCRCredentials_UpstreamIssuerDerivedFromDiscoveryURL(t *testing. server := newDCRTestServer(t, dcrTestHandlerConfig{ tokenEndpointAuthMethodsSupported: []string{"client_secret_basic"}, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() // Caller-supplied issuer names this auth server, NOT the upstream. // Production wiring always passes its own issuer here (see @@ -936,7 +936,7 @@ func TestResolveDCRCredentials_UpstreamIssuerDerivedFromDiscoveryURL(t *testing. }, } - res, err := resolveDCRCredentials(context.Background(), rc, ourIssuer, cache) + res, err := ResolveCredentials(context.Background(), rc, ourIssuer, 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) @@ -1009,18 +1009,18 @@ func TestDeriveExpectedIssuerFromDiscoveryURL(t *testing.T) { } } -// countingStore is a DCRCredentialStore decorator that counts the number of +// countingStore is a CredentialStore decorator that counts the number of // Get calls that returned a hit. The singleflight coalescing test uses it // to assert that no concurrent caller observed a cache hit during the run: // a hit during the test would mean a goroutine raced past the gate, took // the cache-lookup short-circuit instead of joining the singleflight, and // silently weakened the test's coverage. type countingStore struct { - inner DCRCredentialStore + inner CredentialStore hits atomic.Int32 } -func (c *countingStore) Get(ctx context.Context, key DCRKey) (*DCRResolution, bool, error) { +func (c *countingStore) Get(ctx context.Context, key Key) (*Resolution, bool, error) { res, ok, err := c.inner.Get(ctx, key) if ok { c.hits.Add(1) @@ -1028,18 +1028,18 @@ func (c *countingStore) Get(ctx context.Context, key DCRKey) (*DCRResolution, bo return res, ok, err } -func (c *countingStore) Put(ctx context.Context, key DCRKey, res *DCRResolution) error { +func (c *countingStore) Put(ctx context.Context, key Key, res *Resolution) error { return c.inner.Put(ctx, key, res) } // TestResolveDCRCredentials_SingleflightCoalescesConcurrentCallers pins the -// behaviour that N concurrent callers for the same DCRKey result in exactly +// behaviour that N concurrent callers for the same Key result in exactly // one RegisterClientDynamically call against the upstream — preventing the // orphaned-registration class of bug raised in PR #5042 review. // // "Exactly one registration" is necessary but not sufficient to prove the // singleflight coalescing path actually fired: a late-arriving goroutine -// that reached resolveDCRCredentials after the leader's cache.Put would +// that reached ResolveCredentials after the leader's cache.Put would // short-circuit through lookupCachedResolution, take the cache hit, and // still leave registrationCalls == 1. A countingStore wrapper makes that // regression loud — we assert no caller observed a cache hit, so any timing @@ -1060,7 +1060,7 @@ func TestResolveDCRCredentials_SingleflightCoalescesConcurrentCallers(t *testing }, }) - cache := &countingStore{inner: NewInMemoryDCRCredentialStore()} + cache := &countingStore{inner: NewInMemoryStore()} issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid", "profile"}, @@ -1070,14 +1070,14 @@ func TestResolveDCRCredentials_SingleflightCoalescesConcurrentCallers(t *testing } const N = 8 - results := make([]*DCRResolution, N) + results := make([]*Resolution, N) errs := make([]error, N) var wg sync.WaitGroup wg.Add(N) for i := 0; i < N; i++ { go func(idx int) { defer wg.Done() - res, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res, err := ResolveCredentials(context.Background(), rc, issuer, cache) results[idx] = res errs[idx] = err }(i) @@ -1099,7 +1099,7 @@ func TestResolveDCRCredentials_SingleflightCoalescesConcurrentCallers(t *testing select { case <-done: case <-time.After(5 * time.Second): - t.Fatal("timeout waiting for concurrent resolveDCRCredentials goroutines") + t.Fatal("timeout waiting for concurrent ResolveCredentials goroutines") } for i := 0; i < N; i++ { @@ -1198,8 +1198,8 @@ 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 +// 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() @@ -1207,12 +1207,12 @@ func TestConsumeResolution_DoesNotOverwritePreProvisionedClientID(t *testing.T) rc := &authserver.OAuth2UpstreamRunConfig{ ClientID: "pre-provisioned", } - res := &DCRResolution{ + res := &Resolution{ ClientID: "would-be-overwrite", } - consumeResolution(rc, res) + ConsumeResolution(rc, res) assert.Equal(t, "pre-provisioned", rc.ClientID, - "consumeResolution must not overwrite a non-empty ClientID") + "ConsumeResolution must not overwrite a non-empty ClientID") } // TestResolveDCREndpoints_DirectRegistrationEndpointValidated covers @@ -1320,14 +1320,14 @@ type failingDCRStore struct { putErr error } -func (f failingDCRStore) Get(_ context.Context, _ DCRKey) (*DCRResolution, bool, error) { +func (f failingDCRStore) Get(_ context.Context, _ Key) (*Resolution, bool, error) { if f.getErr != nil { return nil, false, f.getErr } return nil, false, nil } -func (f failingDCRStore) Put(_ context.Context, _ DCRKey, _ *DCRResolution) error { +func (f failingDCRStore) Put(_ context.Context, _ Key, _ *Resolution) error { return f.putErr } @@ -1347,7 +1347,7 @@ func TestResolveDCRCredentials_CacheGetFailureWrapped(t *testing.T) { }, } - _, err := resolveDCRCredentials(context.Background(), rc, "https://idp.example.com", store) + _, err := ResolveCredentials(context.Background(), rc, "https://idp.example.com", store) require.Error(t, err) assert.ErrorIs(t, err, storeErr, "cache.Get error must be wrapped with %%w so callers can inspect the cause") @@ -1376,7 +1376,7 @@ func TestResolveDCRCredentials_CachePutFailureWrapped(t *testing.T) { }, } - _, err := resolveDCRCredentials(context.Background(), rc, server.URL, store) + _, err := ResolveCredentials(context.Background(), rc, server.URL, store) require.Error(t, err) assert.ErrorIs(t, err, storeErr, "cache.Put error must be wrapped with %%w so callers can inspect the cause") @@ -1388,7 +1388,7 @@ func TestResolveDCRCredentials_CachePutFailureWrapped(t *testing.T) { // TestBuildResolution_PopulatesRFC7591ExpiryFields covers the conversion of // the int64 epoch fields client_id_issued_at and client_secret_expires_at -// into time.Time on DCRResolution. The wire convention "0 means absent / +// into time.Time on Resolution. The wire convention "0 means absent / // does not expire" is preserved as the zero time.Time. func TestBuildResolution_PopulatesRFC7591ExpiryFields(t *testing.T) { t.Parallel() @@ -1471,7 +1471,7 @@ func TestResolveDCRCredentials_RefetchesOnExpiredCachedSecret(t *testing.T) { }, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -1481,7 +1481,7 @@ func TestResolveDCRCredentials_RefetchesOnExpiredCachedSecret(t *testing.T) { } // First call: registers, populates cache with already-expired entry. - res1, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res1, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) require.NotNil(t, res1) require.False(t, res1.ClientSecretExpiresAt.IsZero(), @@ -1491,7 +1491,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 := resolveDCRCredentials(context.Background(), rc, issuer, cache) + res2, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) require.NotNil(t, res2) assert.EqualValues(t, 2, atomic.LoadInt32(®istrationCalls), @@ -1526,7 +1526,7 @@ func TestResolveDCRCredentials_HonoursFutureExpiryAndZero(t *testing.T) { atomic.AddInt32(®istrationCalls, 1) }, }) - cache := NewInMemoryDCRCredentialStore() + cache := NewInMemoryStore() issuer := server.URL rc := &authserver.OAuth2UpstreamRunConfig{ Scopes: []string{"openid"}, @@ -1535,9 +1535,9 @@ func TestResolveDCRCredentials_HonoursFutureExpiryAndZero(t *testing.T) { }, } - _, err := resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err := ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) - _, err = resolveDCRCredentials(context.Background(), rc, issuer, cache) + _, err = ResolveCredentials(context.Background(), rc, issuer, cache) require.NoError(t, err) assert.EqualValues(t, 1, atomic.LoadInt32(®istrationCalls), @@ -1554,11 +1554,11 @@ type panickingPutDCRStore struct { panicValue any } -func (panickingPutDCRStore) Get(_ context.Context, _ DCRKey) (*DCRResolution, bool, error) { +func (panickingPutDCRStore) Get(_ context.Context, _ Key) (*Resolution, bool, error) { return nil, false, nil } -func (s panickingPutDCRStore) Put(_ context.Context, _ DCRKey, _ *DCRResolution) error { +func (s panickingPutDCRStore) Put(_ context.Context, _ Key, _ *Resolution) error { panic(s.panicValue) } @@ -1566,10 +1566,10 @@ func (s panickingPutDCRStore) Put(_ context.Context, _ DCRKey, _ *DCRResolution) // behaviour that a panic inside the singleflight closure does not propagate // up as a panic to either the leader goroutine or any of the followers. // singleflight.Group re-panics the leader's panic in every follower, so -// without the recover N concurrent callers for the same DCRKey would all +// without the recover N concurrent callers for the same Key would all // crash with the same value. The defer/recover converts the panic to a // *dcrStepError(dcrStepRegister, ..., Stack: ); the boundary -// caller's logDCRStepError emits the single Error record and every caller +// caller's LogStepError emits the single Error record and every caller // gets the same wrapped error. func TestResolveDCRCredentials_RecoversPanicInsideSingleflight(t *testing.T) { t.Parallel() @@ -1603,7 +1603,7 @@ func TestResolveDCRCredentials_RecoversPanicInsideSingleflight(t *testing.T) { panicked[idx] = true } }() - _, errs[idx] = resolveDCRCredentials(context.Background(), rc, issuer, store) + _, errs[idx] = ResolveCredentials(context.Background(), rc, issuer, store) }(i) } @@ -1627,7 +1627,7 @@ func TestResolveDCRCredentials_RecoversPanicInsideSingleflight(t *testing.T) { "goroutine %d's error must include the panic value so the cause is recoverable", i) // The captured stack and dcrStepRegister tag must travel with the - // returned error so the boundary log (logDCRStepError) emits a + // returned error so the boundary log (LogStepError) emits a // single Error record without a duplicate in-defer log. var stepErr *dcrStepError require.True(t, errors.As(errs[i], &stepErr), @@ -1672,12 +1672,12 @@ func TestDcrStepError(t *testing.T) { assert.Equal(t, "https://app/cb", got.RedirectURI) }) - t.Run("resolveDCRCredentials wraps every failure in a dcrStepError", func(t *testing.T) { + t.Run("ResolveCredentials wraps every failure in a dcrStepError", func(t *testing.T) { t.Parallel() // Precondition failure → dcrStepValidate. - _, err := resolveDCRCredentials(context.Background(), nil, "https://as", - NewInMemoryDCRCredentialStore()) + _, err := ResolveCredentials(context.Background(), nil, "https://as", + NewInMemoryStore()) require.Error(t, err) var stepErr *dcrStepError require.True(t, errors.As(err, &stepErr)) diff --git a/pkg/authserver/runner/dcr_store.go b/pkg/auth/dcr/store.go similarity index 67% rename from pkg/authserver/runner/dcr_store.go rename to pkg/auth/dcr/store.go index 84e28060da..469f356c20 100644 --- a/pkg/authserver/runner/dcr_store.go +++ b/pkg/auth/dcr/store.go @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. // SPDX-License-Identifier: Apache-2.0 -package runner +package dcr import ( "context" @@ -18,76 +18,76 @@ import ( // long-lived and are only purged by explicit RFC 7592 deregistration. const dcrStaleAgeThreshold = 90 * 24 * time.Hour -// DCRKey is a re-export of storage.DCRKey, kept as a package-local alias so -// existing runner-side callers continue to compile against runner.DCRKey -// while the canonical definition lives in pkg/authserver/storage. The -// canonical form (and its ScopesHash constructor) MUST live in a single place -// so any future Redis backend hashes keys identically to the in-memory -// backend; see storage.DCRKey for the field documentation. -type DCRKey = storage.DCRKey +// Key is a re-export of storage.DCRKey, kept as a package-local alias so +// callers in this package can reference the canonical cache key without an +// explicit storage. qualifier on every call site, while the canonical +// definition lives in pkg/authserver/storage. The canonical form (and its +// ScopesHash constructor) MUST live in a single place so any future Redis +// backend hashes keys identically to the in-memory backend; see +// storage.DCRKey for the field documentation. +type Key = storage.DCRKey -// DCRCredentialStore is the runner-facing cache shape used by the DCR +// CredentialStore is the resolver-facing cache shape used by the DCR // resolver. It is a narrow re-projection of storage.DCRCredentialStore that -// exchanges *DCRResolution values (the resolver's working type) instead of +// exchanges *Resolution values (the resolver's working type) instead of // *storage.DCRCredentials so the resolver internals stay agnostic to the // persistence layer's exact field shape. // // Implementations in this package are thin adapters around a // storage.DCRCredentialStore — the durable map / Redis hash lives over -// there, and this interface adds a per-call DCRResolution <-> DCRCredentials +// there, and this interface adds a per-call Resolution <-> DCRCredentials // translation. There is exactly one persistence implementation per backend: -// storage.MemoryStorage and storage.RedisStorage. See newStorageBackedStore +// storage.MemoryStorage and storage.RedisStorage. See NewStorageBackedStore // for the adapter. // // Implementations must be safe for concurrent use. -type DCRCredentialStore interface { +type CredentialStore interface { // Get returns the cached resolution for key, or (nil, false, nil) if the // key is not present. An error is returned only on backend failure. - Get(ctx context.Context, key DCRKey) (*DCRResolution, bool, error) + Get(ctx context.Context, key Key) (*Resolution, bool, error) // Put stores the resolution for key, overwriting any existing entry. // Implementations must reject a nil resolution with an error rather // than silently succeeding — a no-op would leave callers with no // debug trail for the subsequent Get miss. - Put(ctx context.Context, key DCRKey, resolution *DCRResolution) error + Put(ctx context.Context, key Key, resolution *Resolution) error } -// NewInMemoryDCRCredentialStore returns a thread-safe in-memory -// DCRCredentialStore intended for tests and single-replica development -// deployments. It is a thin adapter over storage.NewMemoryStorage so the -// runner-side cache and the authserver's main storage backend share a -// single in-memory implementation. +// NewInMemoryStore returns a thread-safe in-memory CredentialStore intended +// for tests and single-replica development deployments. It is a thin adapter +// over storage.NewMemoryStorage so the resolver-side cache and the +// authserver's main storage backend share a single in-memory implementation. // // Production deployments should use a Redis-backed // storage.DCRCredentialStore (instantiated via storage.NewRedisStorage and // passed through this package's storage-backed adapter), which addresses // cross-replica sharing, durability, and cross-process coordination. -func NewInMemoryDCRCredentialStore() DCRCredentialStore { - return newStorageBackedStore(storage.NewMemoryStorage()) +func NewInMemoryStore() CredentialStore { + return NewStorageBackedStore(storage.NewMemoryStorage()) } -// newStorageBackedStore returns a DCRCredentialStore that delegates to a +// NewStorageBackedStore returns a CredentialStore that delegates to a // storage.DCRCredentialStore for durable persistence and translates -// DCRResolution values into DCRCredentials at the boundary. The returned -// store is safe for concurrent use because the underlying +// Resolution values into storage.DCRCredentials at the boundary. The +// returned store is safe for concurrent use because the underlying // storage.DCRCredentialStore must be (per its interface contract). -func newStorageBackedStore(backend storage.DCRCredentialStore) DCRCredentialStore { +func NewStorageBackedStore(backend storage.DCRCredentialStore) CredentialStore { return &storageBackedStore{backend: backend} } -// storageBackedStore is the runner-side DCRCredentialStore wrapping a +// storageBackedStore is the resolver-side CredentialStore wrapping a // storage.DCRCredentialStore. Its methods are the only place that converts -// between the resolver's *DCRResolution and the persisted +// between the resolver's *Resolution and the persisted // *storage.DCRCredentials shapes. type storageBackedStore struct { backend storage.DCRCredentialStore } -// Get implements DCRCredentialStore. +// Get implements CredentialStore. // // A storage-level ErrNotFound is translated into the (nil, false, nil) // miss-tuple advertised by the interface. Other errors propagate as-is. -func (s *storageBackedStore) Get(ctx context.Context, key DCRKey) (*DCRResolution, bool, error) { +func (s *storageBackedStore) Get(ctx context.Context, key Key) (*Resolution, bool, error) { creds, err := s.backend.GetDCRCredentials(ctx, key) if err != nil { if errors.Is(err, storage.ErrNotFound) { @@ -98,13 +98,13 @@ func (s *storageBackedStore) Get(ctx context.Context, key DCRKey) (*DCRResolutio return credentialsToResolution(creds), true, nil } -// Put implements DCRCredentialStore. +// Put implements CredentialStore. // // A nil resolution is rejected rather than silently no-oped: a caller // passing nil would otherwise get a successful return, observe a miss on // the next Get, and have no error trail to debug from. Failing loudly at // the boundary makes such bugs visible at the first call. -func (s *storageBackedStore) Put(ctx context.Context, key DCRKey, resolution *DCRResolution) error { +func (s *storageBackedStore) Put(ctx context.Context, key Key, resolution *Resolution) error { if resolution == nil { return fmt.Errorf("dcr: resolution must not be nil") } @@ -112,13 +112,14 @@ func (s *storageBackedStore) Put(ctx context.Context, key DCRKey, resolution *DC return s.backend.StoreDCRCredentials(ctx, creds) } -// resolutionToCredentials converts a resolver-side *DCRResolution into the -// persisted *storage.DCRCredentials shape. The DCRKey is supplied separately +// resolutionToCredentials converts a resolver-side *Resolution into the +// persisted *storage.DCRCredentials shape. The Key is supplied separately // because storage.DCRCredentials carries the key as a struct field rather // than implicitly via a map key, so the persistence layer can round-trip it // across processes and backends. // -// Fields that exist on DCRResolution but not on DCRCredentials are dropped: +// Fields that exist on Resolution but not on storage.DCRCredentials are +// dropped: // - ClientIDIssuedAt: informational only per RFC 7591 §3.2.1; the resolver // does not consult it for cache invalidation, so it does not need to // survive a process restart. @@ -128,7 +129,7 @@ func (s *storageBackedStore) Put(ctx context.Context, key DCRKey, resolution *DC // CreatedAt and ClientSecretExpiresAt are preserved so cache observers // (e.g. lookupCachedResolution's staleness Warn) and TTL-aware backends // (Redis) keep their existing behaviour after a restart. -func resolutionToCredentials(key DCRKey, res *DCRResolution) *storage.DCRCredentials { +func resolutionToCredentials(key Key, res *Resolution) *storage.DCRCredentials { if res == nil { return nil } @@ -148,17 +149,17 @@ func resolutionToCredentials(key DCRKey, res *DCRResolution) *storage.DCRCredent // 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 +// 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. // // 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 // response, not from a cached resolution. -func credentialsToResolution(creds *storage.DCRCredentials) *DCRResolution { +func credentialsToResolution(creds *storage.DCRCredentials) *Resolution { if creds == nil { return nil } - return &DCRResolution{ + return &Resolution{ ClientID: creds.ClientID, ClientSecret: creds.ClientSecret, AuthorizationEndpoint: creds.AuthorizationEndpoint, diff --git a/pkg/authserver/runner/dcr_store_test.go b/pkg/auth/dcr/store_test.go similarity index 73% rename from pkg/authserver/runner/dcr_store_test.go rename to pkg/auth/dcr/store_test.go index 8373d1cade..6a1cead5c3 100644 --- a/pkg/authserver/runner/dcr_store_test.go +++ b/pkg/auth/dcr/store_test.go @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. // SPDX-License-Identifier: Apache-2.0 -package runner +package dcr import ( "context" @@ -15,18 +15,18 @@ import ( "github.com/stretchr/testify/require" ) -func TestInMemoryDCRCredentialStore_PutGet_RoundTrip(t *testing.T) { +func TestInMemoryStore_PutGet_RoundTrip(t *testing.T) { t.Parallel() - store := NewInMemoryDCRCredentialStore() + store := NewInMemoryStore() ctx := context.Background() - key := DCRKey{ + key := Key{ Issuer: "https://idp.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: scopesHash([]string{"openid", "profile"}), } - resolution := &DCRResolution{ + resolution := &Resolution{ ClientID: "client-abc", ClientSecret: "secret-xyz", AuthorizationEndpoint: "https://idp.example.com/authorize", @@ -51,52 +51,52 @@ func TestInMemoryDCRCredentialStore_PutGet_RoundTrip(t *testing.T) { assert.Equal(t, resolution.TokenEndpointAuthMethod, got.TokenEndpointAuthMethod) } -func TestInMemoryDCRCredentialStore_Get_MissingKey(t *testing.T) { +func TestInMemoryStore_Get_MissingKey(t *testing.T) { t.Parallel() - store := NewInMemoryDCRCredentialStore() + store := NewInMemoryStore() ctx := context.Background() - got, ok, err := store.Get(ctx, DCRKey{Issuer: "https://unknown.example.com"}) + got, ok, err := store.Get(ctx, Key{Issuer: "https://unknown.example.com"}) require.NoError(t, err) assert.False(t, ok) assert.Nil(t, got) } -func TestInMemoryDCRCredentialStore_DistinctKeysDoNotCollide(t *testing.T) { +func TestInMemoryStore_DistinctKeysDoNotCollide(t *testing.T) { t.Parallel() - store := NewInMemoryDCRCredentialStore() + store := NewInMemoryStore() ctx := context.Background() - keyA := DCRKey{ + keyA := Key{ Issuer: "https://idp-a.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: scopesHash([]string{"openid"}), } - keyB := DCRKey{ + keyB := Key{ Issuer: "https://idp-b.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: scopesHash([]string{"openid"}), } - keyC := DCRKey{ + keyC := Key{ Issuer: "https://idp-a.example.com", RedirectURI: "https://other.example.com/callback", ScopesHash: scopesHash([]string{"openid"}), } - keyD := DCRKey{ + keyD := Key{ Issuer: "https://idp-a.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: scopesHash([]string{"openid", "email"}), } - require.NoError(t, store.Put(ctx, keyA, &DCRResolution{ClientID: "a"})) - require.NoError(t, store.Put(ctx, keyB, &DCRResolution{ClientID: "b"})) - require.NoError(t, store.Put(ctx, keyC, &DCRResolution{ClientID: "c"})) - require.NoError(t, store.Put(ctx, keyD, &DCRResolution{ClientID: "d"})) + require.NoError(t, store.Put(ctx, keyA, &Resolution{ClientID: "a"})) + require.NoError(t, store.Put(ctx, keyB, &Resolution{ClientID: "b"})) + require.NoError(t, store.Put(ctx, keyC, &Resolution{ClientID: "c"})) + require.NoError(t, store.Put(ctx, keyD, &Resolution{ClientID: "d"})) for _, tc := range []struct { - key DCRKey + key Key expected string }{ {keyA, "a"}, @@ -111,15 +111,15 @@ func TestInMemoryDCRCredentialStore_DistinctKeysDoNotCollide(t *testing.T) { } } -func TestInMemoryDCRCredentialStore_Put_OverwritesExisting(t *testing.T) { +func TestInMemoryStore_Put_OverwritesExisting(t *testing.T) { t.Parallel() - store := NewInMemoryDCRCredentialStore() + store := NewInMemoryStore() ctx := context.Background() - key := DCRKey{Issuer: "https://idp.example.com", RedirectURI: "https://x.example.com/cb"} - require.NoError(t, store.Put(ctx, key, &DCRResolution{ClientID: "first"})) - require.NoError(t, store.Put(ctx, key, &DCRResolution{ClientID: "second"})) + key := Key{Issuer: "https://idp.example.com", RedirectURI: "https://x.example.com/cb"} + require.NoError(t, store.Put(ctx, key, &Resolution{ClientID: "first"})) + require.NoError(t, store.Put(ctx, key, &Resolution{ClientID: "second"})) got, ok, err := store.Get(ctx, key) require.NoError(t, err) @@ -127,16 +127,16 @@ func TestInMemoryDCRCredentialStore_Put_OverwritesExisting(t *testing.T) { assert.Equal(t, "second", got.ClientID) } -// TestInMemoryDCRCredentialStore_Put_RejectsNilResolution pins the +// TestInMemoryStore_Put_RejectsNilResolution pins the // fail-loud-on-invalid-input contract: passing nil must error rather than // silently no-op. A silent no-op would leave the caller with a successful // Put followed by a Get miss and no debug trail to explain it. -func TestInMemoryDCRCredentialStore_Put_RejectsNilResolution(t *testing.T) { +func TestInMemoryStore_Put_RejectsNilResolution(t *testing.T) { t.Parallel() - store := NewInMemoryDCRCredentialStore() + store := NewInMemoryStore() ctx := context.Background() - key := DCRKey{Issuer: "https://idp.example.com", RedirectURI: "https://x.example.com/cb"} + key := Key{Issuer: "https://idp.example.com", RedirectURI: "https://x.example.com/cb"} err := store.Put(ctx, key, nil) require.Error(t, err) @@ -148,17 +148,17 @@ func TestInMemoryDCRCredentialStore_Put_RejectsNilResolution(t *testing.T) { assert.False(t, ok, "rejected Put must not leave any entry behind") } -func TestInMemoryDCRCredentialStore_GetReturnsDefensiveCopy(t *testing.T) { +func TestInMemoryStore_GetReturnsDefensiveCopy(t *testing.T) { t.Parallel() - store := NewInMemoryDCRCredentialStore() + store := NewInMemoryStore() ctx := context.Background() - key := DCRKey{ + key := Key{ Issuer: "https://idp.example.com", RedirectURI: "https://x.example.com/cb", } - require.NoError(t, store.Put(ctx, key, &DCRResolution{ClientID: "orig"})) + require.NoError(t, store.Put(ctx, key, &Resolution{ClientID: "orig"})) got, ok, err := store.Get(ctx, key) require.NoError(t, err) @@ -173,23 +173,23 @@ func TestInMemoryDCRCredentialStore_GetReturnsDefensiveCopy(t *testing.T) { // Tests for the canonical scopes-hash form live next to the canonical // implementation in pkg/authserver/storage/memory_test.go (TestScopesHash_*). -// The runner-package binding `scopesHash = storage.ScopesHash` would only +// The dcr-package binding `scopesHash = storage.ScopesHash` would only // re-exercise the same code, so duplicating the suite here would be redundant // per .claude/rules/testing.md. -// TestInMemoryDCRCredentialStore_ConcurrentAccess fans out N goroutines +// TestInMemoryStore_ConcurrentAccess fans out N goroutines // performing alternating Put / Get against overlapping and disjoint keys, -// exercising the sync.RWMutex guard advertised in the DCRCredentialStore +// exercising the sync.RWMutex guard advertised in the CredentialStore // interface doc. With go test -race this catches any future change that // drops the lock or introduces a data race in the map access. // // The test is bounded by a fail-fast deadline so a regression that // deadlocks fails loudly with a clear message rather than hanging until // the global Go test timeout. -func TestInMemoryDCRCredentialStore_ConcurrentAccess(t *testing.T) { +func TestInMemoryStore_ConcurrentAccess(t *testing.T) { t.Parallel() - store := NewInMemoryDCRCredentialStore() + store := NewInMemoryStore() const ( workers = 16 @@ -199,15 +199,15 @@ func TestInMemoryDCRCredentialStore_ConcurrentAccess(t *testing.T) { // Two key spaces: overlapping (every worker writes the same keys, so the // lock must serialise their writes) and disjoint (each worker has its own // key space, so reads never see another worker's writes). - overlappingKey := func(i int) DCRKey { - return DCRKey{ + overlappingKey := func(i int) Key { + return Key{ Issuer: "https://idp.example.com", RedirectURI: "https://thv.example.com/oauth/callback", ScopesHash: fmt.Sprintf("overlap-%d", i%4), } } - disjointKey := func(worker, i int) DCRKey { - return DCRKey{ + disjointKey := func(worker, i int) Key { + return Key{ Issuer: fmt.Sprintf("https://idp-%d.example.com", worker), RedirectURI: "https://thv.example.com/oauth/callback", ScopesHash: fmt.Sprintf("disjoint-%d", i), @@ -222,7 +222,7 @@ func TestInMemoryDCRCredentialStore_ConcurrentAccess(t *testing.T) { defer wg.Done() ctx := context.Background() for i := 0; i < opsPerWorker; i++ { - resolution := &DCRResolution{ + resolution := &Resolution{ ClientID: fmt.Sprintf("worker-%d-op-%d", worker, i), CreatedAt: time.Now(), } diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 7d98bb3af5..0659f3e716 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -14,6 +14,7 @@ import ( "sync" "time" + "github.com/stacklok/toolhive/pkg/auth/dcr" "github.com/stacklok/toolhive/pkg/authserver" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/keys" @@ -52,7 +53,7 @@ type EmbeddedAuthServer struct { // is needed here. // // Concurrency: this field is the per-instance cache layer; the - // package-level dcrFlight singleflight in dcr.go is the in-process + // package-level singleflight in pkg/auth/dcr is the in-process // thundering-herd guard. The asymmetry is by design — the store // deduplicates registrations across boots and replicas; the flight // deduplicates concurrent /register calls within a single process. @@ -138,7 +139,7 @@ func newEmbeddedAuthServerWithStorage( // 4. Build upstream configurations. The DCR resolver caches RFC 7591 // resolutions in stor so re-entrant boot/reload paths reuse // previously-registered upstream clients instead of re-registering. - upstreams, err := buildUpstreamConfigs(ctx, cfg.Upstreams, cfg.Issuer, newStorageBackedStore(stor)) + upstreams, err := buildUpstreamConfigs(ctx, cfg.Upstreams, cfg.Issuer, dcr.NewStorageBackedStore(stor)) if err != nil { return nil, fmt.Errorf("failed to build upstream configs: %w", err) } @@ -365,7 +366,7 @@ func buildUpstreamConfigs( ctx context.Context, runConfigs []authserver.UpstreamRunConfig, issuer string, - dcrStore DCRCredentialStore, + dcrStore dcr.CredentialStore, ) ([]authserver.UpstreamConfig, error) { configs := make([]authserver.UpstreamConfig, 0, len(runConfigs)) @@ -374,26 +375,26 @@ func buildUpstreamConfigs( // mutates the caller's slice element. rcCopy := rc - var dcrResolution *DCRResolution - // needsDCR returns false for nil input, so the explicit Type == + var dcrResolution *dcr.Resolution + // dcr.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 needsDCR(rcCopy.OAuth2Config) { - // Deep-copy the OAuth2 sub-config so consumeResolution writes to the - // copy, not the caller's OAuth2UpstreamRunConfig pointer. + if dcr.NeedsDCR(rcCopy.OAuth2Config) { + // Deep-copy the OAuth2 sub-config so dcr.ConsumeResolution writes + // to the copy, not the caller's OAuth2UpstreamRunConfig pointer. o2Copy := *rcCopy.OAuth2Config rcCopy.OAuth2Config = &o2Copy - resolution, err := resolveDCRCredentials(ctx, &o2Copy, issuer, dcrStore) + resolution, err := dcr.ResolveCredentials(ctx, &o2Copy, issuer, dcrStore) if err != nil { // Emit the single boundary Error record with enough context to // correlate the failure back to this upstream; then return the // wrapped error without further logging. - logDCRStepError(rc.Name, err) + dcr.LogStepError(rc.Name, err) return nil, fmt.Errorf("upstream %q: %w", rc.Name, err) } - consumeResolution(&o2Copy, resolution) + dcr.ConsumeResolution(&o2Copy, resolution) dcrResolution = resolution } @@ -403,12 +404,12 @@ func buildUpstreamConfigs( } // Apply the DCR-resolved ClientSecret to the built OAuth2Config. - // The split between consumeResolution (run-config fields) and - // applyResolutionToOAuth2Config (inline-only ClientSecret) is - // documented in dcr.go — both calls must be paired to produce a - // fully-resolved DCR client. + // 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. if dcrResolution != nil && cfg.OAuth2Config != nil { - applyResolutionToOAuth2Config(cfg.OAuth2Config, dcrResolution) + dcr.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 98062f11d1..1d3a266d5d 100644 --- a/pkg/authserver/runner/embeddedauthserver_test.go +++ b/pkg/authserver/runner/embeddedauthserver_test.go @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/stacklok/toolhive/pkg/auth/dcr" "github.com/stacklok/toolhive/pkg/authserver" servercrypto "github.com/stacklok/toolhive/pkg/authserver/server/crypto" "github.com/stacklok/toolhive/pkg/authserver/server/keys" @@ -1462,7 +1463,7 @@ func TestBuildUpstreamConfigs_DCR(t *testing.T) { AllowedAudiences: []string{"https://mcp.example.com"}, } - store := NewInMemoryDCRCredentialStore() + store := dcr.NewInMemoryStore() got, err := buildUpstreamConfigs(context.Background(), cfg.Upstreams, cfg.Issuer, store) require.NoError(t, err) require.Len(t, got, 1) @@ -1474,10 +1475,10 @@ func TestBuildUpstreamConfigs_DCR(t *testing.T) { // Store now contains the resolution under the canonical DCRKey. redirectURI := server.URL + "/oauth/callback" - key := DCRKey{ + key := dcr.Key{ Issuer: server.URL, RedirectURI: redirectURI, - ScopesHash: scopesHash([]string{"openid", "profile"}), + ScopesHash: storage.ScopesHash([]string{"openid", "profile"}), } cached, ok, err := store.Get(context.Background(), key) require.NoError(t, err) @@ -1524,7 +1525,7 @@ func TestBuildUpstreamConfigs_DCR(t *testing.T) { AllowedAudiences: []string{"https://mcp.example.com"}, } - store := NewInMemoryDCRCredentialStore() + store := dcr.NewInMemoryStore() // First call: populates the store. _, err := buildUpstreamConfigs(context.Background(), cfg.Upstreams, cfg.Issuer, store) @@ -1611,10 +1612,10 @@ func TestNewEmbeddedAuthServer_DCRBoot(t *testing.T) { // returned by createStorage, so a successful boot persisted the // resolution there directly (no separate in-memory store was created). redirectURI := server.URL + "/oauth/callback" - key := DCRKey{ + key := dcr.Key{ Issuer: server.URL, RedirectURI: redirectURI, - ScopesHash: scopesHash([]string{"openid", "profile"}), + ScopesHash: storage.ScopesHash([]string{"openid", "profile"}), } cached, err := embed.dcrStore.GetDCRCredentials(context.Background(), key) require.NoError(t, err, "dcrStore on EmbeddedAuthServer must hold the DCR resolution") From 633ed9eddfc5e1000839d43d4e563721dd72dd87 Mon Sep 17 00:00:00 2001 From: Trey Date: Tue, 5 May 2026 11:42:01 -0700 Subject: [PATCH 2/2] Address code review feedback Fixed issues from review of df84256f (Extract DCR resolver into pkg/auth/dcr) for issue #5145 sub-issue 4a: - HIGH: Stale identifier references in buildUpstreamConfigs doc comment in pkg/authserver/runner/embeddedauthserver.go named the pre-rename symbols (consumeResolution, applyResolutionToOAuth2Config, resolveDCRCredentials, logDCRStepError). Updated the comment to use the new public names (dcr.ConsumeResolution, dcr.ApplyResolutionToOAuth2Config, dcr.ResolveCredentials, dcr.LogStepError). Re-grepped the runner package; no other survivors. - HIGH: pkg/auth/dcr's package doc claimed "profile-agnostic" while the public API takes embedded-authserver types directly. Revised the package doc comment in pkg/auth/dcr/resolver.go to call out the current coupling explicitly and labelled the profile-agnostic framing as a target state for sub-issue 4b rather than the current API shape. Designing a profile-neutral input type with only one consumer in hand would be speculative; the right design moment is when 4b lands the CLI flow as the second consumer. - MEDIUM: DCRStore() accessor doc comment in pkg/authserver/runner/embeddedauthserver.go referenced the dropped "runner-DCRCredentialStore adapter" name. Replaced with "dcr.CredentialStore adapter". - MEDIUM: resolveSecret is duplicated across pkg/authserver/runner and pkg/auth/dcr. Added a parallel TestResolveSecret / TestResolveSecretWithEnvVar suite in pkg/auth/dcr/secret_test.go that mirrors the runner-package twin's observable contract, so any future drift between the two copies will fail one of the two tests at CI time. Lifting into a shared helper is deferred until 4b lands a third call site. - MEDIUM: Process-global singleflight was undocumented at the package surface. Added a "Concurrency" section to the package doc comment in pkg/auth/dcr/resolver.go. Extended the dcrFlight var doc with a cross-consumer caveat that names the "third consumer with colliding redirect URI" failure mode. Extracted the flight-key construction into a new flightKeyOf(Key) string helper so the canonical key shape is inspectable rather than buried in a string-concatenation literal. - MEDIUM: endpointsFromMetadata dereferenced metadata without a defensive nil check. Added the guard immediately after the fetchErr check in pkg/auth/dcr/resolver.go::endpointsFromMetadata with an error message that names the cross-package contract being defended. Documented the oauthproto contract on the function's doc comment so the assumption is visible at the call site. Verified via task lint-fix (0 issues), task license-check (clean), and go test -count=1 -race ./pkg/auth/dcr/... ./pkg/authserver/... (all pass). --- pkg/auth/dcr/resolver.go | 75 +++++++++++-- pkg/auth/dcr/secret_test.go | 118 ++++++++++++++++++++ pkg/authserver/runner/embeddedauthserver.go | 10 +- 3 files changed, 190 insertions(+), 13 deletions(-) create mode 100644 pkg/auth/dcr/secret_test.go diff --git a/pkg/auth/dcr/resolver.go b/pkg/auth/dcr/resolver.go index a0abb16d71..7f677a7eeb 100644 --- a/pkg/auth/dcr/resolver.go +++ b/pkg/auth/dcr/resolver.go @@ -11,9 +11,33 @@ // the registration body. Stateless RFC 7591 wire-shape primitives live in // pkg/oauthproto. // -// Profile differences (public PKCE vs confidential client, redirect URI -// policy, persistence backend) stay at the call site — the package is -// profile-agnostic and lets each consumer plug a CredentialStore in. +// # Concurrency +// +// The package maintains a process-global singleflight keyed on the tuple +// (issuer, redirectURI, scopesHash) so concurrent ResolveCredentials calls +// across all consumers in a single process coalesce when their cache keys +// match. Consumers that share any of those three values will share a flight +// — the deduplication is a feature for the embedded authserver but means +// callers cannot assume per-call-site flight isolation. See the dcrFlight +// doc comment in resolver.go 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, +// *authserver.DCRUpstreamConfig, *upstream.OAuth2Config — 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. @@ -62,8 +86,30 @@ import ( // decides whether the resolution is fresh enough to reuse. A future // Redis-backed store would still want this in-process gate so a single // replica does not double-register against itself. +// +// Cross-consumer caveat (matters once issue #5145 sub-issue 4b 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. Keep this in mind when +// adding a third consumer. var dcrFlight singleflight.Group +// flightKeyOf canonicalises a Key into the singleflight string used by +// dcrFlight. The "\n" separator is safe because newline is not a valid byte +// in any of the three components: OAuth scope tokens (RFC 6749 §3.3), URI +// reference characters (RFC 3986 §2), or hex digests (the form +// storage.ScopesHash always emits). Exposed as a function so tests and +// future inspection helpers can compute the exact key the resolver would +// route through dcrFlight without re-implementing the concatenation. +func flightKeyOf(key Key) string { + return key.Issuer + "\n" + key.RedirectURI + "\n" + key.ScopesHash +} + // defaultUpstreamRedirectPath is the redirect path derived from the issuer // origin when the caller's run-config does not supply an explicit RedirectURI. // Matches the authserver's public callback route. @@ -369,9 +415,9 @@ func ResolveCredentials( // Coalesce concurrent registrations for the same Key — see dcrFlight // doc comment. The leader runs the registerOnce closure; followers - // receive the leader's *Resolution result. The flight key embeds the - // Key fields with a separator that cannot appear in any of them - // (newline is not valid in OAuth scope tokens, URLs, or hex digests). + // receive the leader's *Resolution result. flightKeyOf canonicalises the + // Key into a singleflight string with a separator that cannot appear in + // any of the three Key components. // // A defer/recover inside the closure converts a panic in registerAndCache // (or anything it calls) into a normal error. Without this, singleflight @@ -381,8 +427,7 @@ func ResolveCredentials( // dcrStepError and surfaces in the single boundary log emitted by // LogStepError, so the failure produces exactly one Error record (no // in-defer log here) and callers can react to it as a normal failure. - flightKey := key.Issuer + "\n" + key.RedirectURI + "\n" + key.ScopesHash - resolutionAny, err, _ := dcrFlight.Do(flightKey, func() (res any, err error) { + resolutionAny, err, _ := dcrFlight.Do(flightKeyOf(key), func() (res any, err error) { defer func() { if r := recover(); r != nil { stepErr := newDCRStepError(dcrStepRegister, localIssuer, redirectURI, @@ -948,6 +993,15 @@ func deriveExpectedIssuerFromDiscoveryURL(discoveryURL string) (string, error) { // document — possible if TLS to the metadata host is compromised, or if the // upstream is misconfigured — could otherwise plant http:// URLs that flow // through to the authorization-code and token-exchange call paths. +// +// Contract with oauthproto: FetchAuthorizationServerMetadata* guarantees a +// non-nil *AuthorizationServerMetadata whenever fetchErr is nil OR +// fetchErr is ErrRegistrationEndpointMissing (in the latter case the +// metadata is otherwise valid; only registration_endpoint is missing). +// The defensive nil guard below catches a future cross-package contract +// regression — e.g., a new oauthproto sentinel that returns nil metadata +// alongside a non-fatal error — and converts it into a clean validation +// error rather than a nil-pointer dereference at the field accesses. func endpointsFromMetadata( metadata *oauthproto.AuthorizationServerMetadata, fetchErr error, @@ -956,6 +1010,11 @@ func endpointsFromMetadata( if fetchErr != nil && !errors.Is(fetchErr, oauthproto.ErrRegistrationEndpointMissing) { return nil, fmt.Errorf("discover authorization server metadata: %w", fetchErr) } + if metadata == nil { + return nil, fmt.Errorf( + "dcr: authorization server metadata is nil (oauthproto contract " + + "violation: nil metadata returned alongside a non-fatal fetch error)") + } if err := validateUpstreamEndpointURL(metadata.AuthorizationEndpoint, "authorization_endpoint"); err != nil { return nil, fmt.Errorf("dcr: discovered %w", err) diff --git a/pkg/auth/dcr/secret_test.go b/pkg/auth/dcr/secret_test.go new file mode 100644 index 0000000000..cae54714d3 --- /dev/null +++ b/pkg/auth/dcr/secret_test.go @@ -0,0 +1,118 @@ +// 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/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 0659f3e716..27fcf41521 100644 --- a/pkg/authserver/runner/embeddedauthserver.go +++ b/pkg/authserver/runner/embeddedauthserver.go @@ -221,8 +221,8 @@ func (e *EmbeddedAuthServer) KeyProvider() keys.KeyProvider { // and is intended for admin / diagnostic code paths and integration tests // that need to verify that the DCR resolver is wired into the same backend // the authserver writes to. It does NOT expose any I/O the -// runner-DCRCredentialStore adapter does not already provide; the returned -// value is the same storage.DCRCredentialStore the constructor surfaced. +// dcr.CredentialStore adapter does not already provide; the returned value +// is the same storage.DCRCredentialStore the constructor surfaced. func (e *EmbeddedAuthServer) DCRStore() storage.DCRCredentialStore { return e.dcrStore } @@ -352,14 +352,14 @@ 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 consumeResolution + applyResolutionToOAuth2Config. The +// 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. // // Error logging: this function is the boundary for DCR errors — on any -// failure from resolveDCRCredentials it emits exactly one structured -// slog.Error via logDCRStepError and returns the wrapped error to the +// failure from dcr.ResolveCredentials it emits exactly one structured +// slog.Error via dcr.LogStepError and returns the wrapped error to the // caller without logging further. The resolver itself does not log // errors, which avoids the log-and-return double-reporting pattern. func buildUpstreamConfigs(