diff --git a/pkg/authserver/runner/dcr.go b/pkg/auth/dcr/resolver.go similarity index 84% rename from pkg/authserver/runner/dcr.go rename to pkg/auth/dcr/resolver.go index 6dde6158e0..318e5ad044 100644 --- a/pkg/authserver/runner/dcr.go +++ b/pkg/auth/dcr/resolver.go @@ -1,15 +1,57 @@ // 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. +// +// # 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 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, +// *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. +package dcr import ( + "bytes" "context" "errors" "fmt" "log/slog" "net/http" "net/url" + "os" "regexp" "runtime/debug" "slices" @@ -25,14 +67,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 @@ -44,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. @@ -67,23 +131,23 @@ 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 dcrResolutionCache and the unit of -// application via consumeResolution. +// The struct is the unit of storage in CredentialStore and the unit of +// application via ConsumeResolution. // // MUST update both converters (resolutionToCredentials and -// credentialsToResolution in dcr_store.go) when adding, renaming, or +// credentialsToResolution in store.go) when adding, renaming, or // removing a field here. The two converters are the seam between this -// runner-side type and the persisted *storage.DCRCredentials shape; a +// dcr-package type and the persisted *storage.DCRCredentials shape; a // field added here without a paired converter update will silently fail // to round-trip across an authserver restart, the exact "parallel types // drift" failure mode .claude/rules/go-style.md warns about. The // round-trip behaviour is pinned by TestResolutionCredentialsRoundTrip -// in dcr_store_test.go. -type DCRResolution struct { +// in store_test.go. +type Resolution struct { // ClientID is the RFC 7591 "client_id" returned by the authorization // server. ClientID string @@ -117,7 +181,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 @@ -147,18 +211,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" @@ -167,7 +231,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 @@ -189,14 +253,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 } @@ -215,9 +279,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. // @@ -225,12 +289,12 @@ 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 } @@ -238,7 +302,7 @@ func applyResolutionToOAuth2Config(cfg *upstream.OAuth2Config, res *DCRResolutio } // 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. @@ -255,13 +319,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. @@ -293,7 +357,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. @@ -310,7 +374,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 @@ -322,12 +386,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 dcrResolutionCache, -) (*DCRResolution, error) { + cache CredentialStore, +) (*Resolution, error) { if err := validateResolveInputs(rc, localIssuer, cache); err != nil { return nil, newDCRStepError(dcrStepValidate, localIssuer, "", err) } @@ -339,7 +403,7 @@ func resolveDCRCredentials( } scopes := slices.Clone(rc.Scopes) - key := DCRKey{ + key := Key{ Issuer: localIssuer, RedirectURI: redirectURI, ScopesHash: storage.ScopesHash(scopes), @@ -352,21 +416,21 @@ 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 + flightKey := flightKeyOf(key) resolutionAny, err, _ := dcrFlight.Do(flightKey, func() (res any, err error) { defer func() { if r := recover(); r != nil { @@ -382,10 +446,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 @@ -395,9 +459,9 @@ func registerAndCache( rc *authserver.OAuth2UpstreamRunConfig, localIssuer, redirectURI string, scopes []string, - key DCRKey, - cache dcrResolutionCache, -) (*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 { @@ -457,20 +521,20 @@ 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. // -// Every wrapped error is passed through sanitizeErrorForLog to strip URL +// Every wrapped error is passed through SanitizeErrorForLog to strip URL // query parameters that could plausibly contain sensitive tokens (defense // 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 } @@ -479,7 +543,7 @@ func logDCRStepError(upstreamName string, err error) { slog.Error("dcr: resolve failed", "upstream", upstreamName, "step", "unknown", - "error", sanitizeErrorForLog(err), + "error", SanitizeErrorForLog(err), ) return } @@ -488,7 +552,7 @@ func logDCRStepError(upstreamName string, err error) { "upstream", upstreamName, "step", stepErr.Step, "issuer", stepErr.Issuer, - "error", sanitizeErrorForLog(stepErr.Err), + "error", SanitizeErrorForLog(stepErr.Err), } if stepErr.RedirectURI != "" { attrs = append(attrs, "redirect_uri", stepErr.RedirectURI) @@ -499,7 +563,7 @@ func logDCRStepError(upstreamName string, err error) { slog.Error("dcr: resolve failed", attrs...) } -// sanitizeErrorForLog strips secret-bearing components from any URLs +// SanitizeErrorForLog strips secret-bearing components from any URLs // embedded in err's message. The Go HTTP client, url.Error, and other // net/* wrappers embed the full request URL — including userinfo, // query, and fragment — in their error strings. Any of those can carry @@ -519,7 +583,7 @@ func logDCRStepError(upstreamName string, err error) { // host:port) are not sanitised; the current DCR flow never embeds // those in errors, and broadening the match risks false positives on // unrelated text. -func sanitizeErrorForLog(err error) string { +func SanitizeErrorForLog(err error) string { if err == nil { return "" } @@ -572,7 +636,7 @@ func trimURLTrailingPunctuation(s string) (core, tail string) { } // queryStrippingPattern matches URL-shaped substrings inside an error -// message — sufficient to reach the url.Parse path in sanitizeErrorForLog +// message — sufficient to reach the url.Parse path in SanitizeErrorForLog // and let it decide whether a secret-bearing component exists to strip. // The regexp is intentionally narrow (http/https schemes only) to avoid // false positives, but matches schemes case-insensitively per RFC 3986 @@ -588,12 +652,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 dcrResolutionCache, + cache CredentialStore, ) error { if rc == nil { return fmt.Errorf("oauth2 upstream run-config is required") @@ -637,10 +701,10 @@ func validateResolveInputs( // trigger. func lookupCachedResolution( ctx context.Context, - cache dcrResolutionCache, - 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) @@ -752,13 +816,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 @@ -769,12 +833,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, @@ -800,7 +864,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 @@ -834,7 +898,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 @@ -933,6 +997,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, @@ -941,6 +1014,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) @@ -1197,3 +1275,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 93% rename from pkg/authserver/runner/dcr_test.go rename to pkg/auth/dcr/resolver_test.go index ea5f1ebbdc..d99695b574 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" @@ -146,12 +146,12 @@ func TestResolveDCRCredentials_CacheHitShortCircuits(t *testing.T) { // 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: storage.ScopesHash([]string{"openid", "profile"}), } - preloaded := &DCRResolution{ + preloaded := &Resolution{ ClientID: "preloaded-id", ClientSecret: "preloaded-secret", AuthorizationEndpoint: "https://preloaded/authorize", @@ -166,7 +166,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) @@ -197,7 +197,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) @@ -218,7 +218,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: storage.ScopesHash([]string{"openid", "profile"})}) + Key{Issuer: issuer, RedirectURI: issuer + "/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid", "profile"})}) require.NoError(t, err) require.True(t, ok) assert.Equal(t, "test-client-id", cached.ClientID) @@ -240,7 +240,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) @@ -273,7 +273,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) } @@ -337,7 +337,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") @@ -405,7 +405,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) }) @@ -444,7 +444,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") @@ -469,7 +469,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") } @@ -496,7 +496,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) @@ -531,7 +531,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), @@ -554,7 +554,7 @@ func TestResolveDCRCredentials_RejectsInvalidInputs(t *testing.T) { name string rc *authserver.OAuth2UpstreamRunConfig issuer string - cache dcrResolutionCache + cache CredentialStore wantErrSub string }{ { @@ -596,7 +596,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) }) @@ -627,7 +627,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)) }) } } @@ -639,12 +639,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) @@ -654,19 +654,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. @@ -678,15 +678,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) { @@ -791,7 +791,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), @@ -832,7 +832,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") } @@ -860,7 +860,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 @@ -890,7 +890,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) @@ -937,7 +937,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) @@ -1010,18 +1010,18 @@ func TestDeriveExpectedIssuerFromDiscoveryURL(t *testing.T) { } } -// countingStore is a dcrResolutionCache 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 dcrResolutionCache + 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) @@ -1029,18 +1029,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 @@ -1071,14 +1071,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) @@ -1100,7 +1100,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++ { @@ -1199,8 +1199,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() @@ -1208,12 +1208,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 @@ -1321,14 +1321,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 } @@ -1348,7 +1348,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") @@ -1377,7 +1377,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") @@ -1389,7 +1389,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() @@ -1482,7 +1482,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(), @@ -1492,7 +1492,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), @@ -1536,9 +1536,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), @@ -1555,11 +1555,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) } @@ -1567,10 +1567,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() @@ -1604,7 +1604,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) } @@ -1628,7 +1628,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), @@ -1673,11 +1673,11 @@ 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", + _, err := ResolveCredentials(context.Background(), nil, "https://as", newMemoryDCRStore(t)) require.Error(t, err) var stepErr *dcrStepError @@ -1781,7 +1781,7 @@ func TestSanitizeErrorForLog(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tc.expected, sanitizeErrorForLog(tc.in)) + assert.Equal(t, tc.expected, SanitizeErrorForLog(tc.in)) }) } } 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/dcr_store.go b/pkg/auth/dcr/store.go similarity index 65% rename from pkg/authserver/runner/dcr_store.go rename to pkg/auth/dcr/store.go index 7b03334c13..b32bedf98e 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,64 +18,78 @@ 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 -// dcrResolutionCache caches RFC 7591 Dynamic Client Registration resolutions +// CredentialStore caches RFC 7591 Dynamic Client Registration resolutions // keyed by the (Issuer, RedirectURI, ScopesHash) tuple. Implementations must // be safe for concurrent use. // // This is the runner-facing interface 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 +// *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. -type dcrResolutionCache 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 } -// newStorageBackedStore returns a dcrResolutionCache that delegates to a +// 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 NewInMemoryStore() CredentialStore { + return NewStorageBackedStore(storage.NewMemoryStorage()) +} + +// NewStorageBackedStore returns a CredentialStore that delegates to a // storage.DCRCredentialStore for durable persistence and translates -// DCRResolution values into DCRCredentials at the boundary. The returned +// Resolution values into 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) dcrResolutionCache { +func NewStorageBackedStore(backend storage.DCRCredentialStore) CredentialStore { return &storageBackedStore{backend: backend} } -// storageBackedStore is the runner-side dcrResolutionCache wrapping a +// storageBackedStore is the dcr-package 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 dcrResolutionCache. +// 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) { @@ -86,13 +100,13 @@ func (s *storageBackedStore) Get(ctx context.Context, key DCRKey) (*DCRResolutio return credentialsToResolution(creds), true, nil } -// Put implements dcrResolutionCache. +// 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") } @@ -100,13 +114,13 @@ 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 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. @@ -116,7 +130,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 } @@ -136,17 +150,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 91% rename from pkg/authserver/runner/dcr_store_test.go rename to pkg/auth/dcr/store_test.go index 4f61fa79fa..46ca9e16e1 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" @@ -23,12 +23,12 @@ func TestStorageBackedStore_PutGet_RoundTrip(t *testing.T) { store := newMemoryDCRStore(t) ctx := context.Background() - key := DCRKey{ + key := Key{ Issuer: "https://idp.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid", "profile"}), } - resolution := &DCRResolution{ + resolution := &Resolution{ ClientID: "client-abc", ClientSecret: "secret-xyz", AuthorizationEndpoint: "https://idp.example.com/authorize", @@ -59,7 +59,7 @@ func TestStorageBackedStore_Get_MissingKey(t *testing.T) { store := newMemoryDCRStore(t) 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) @@ -71,22 +71,22 @@ func TestStorageBackedStore_DistinctKeysDoNotCollide(t *testing.T) { store := newMemoryDCRStore(t) ctx := context.Background() - keyA := DCRKey{ + keyA := Key{ Issuer: "https://idp-a.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid"}), } - keyB := DCRKey{ + keyB := Key{ Issuer: "https://idp-b.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid"}), } - keyC := DCRKey{ + keyC := Key{ Issuer: "https://idp-a.example.com", RedirectURI: "https://other.example.com/callback", ScopesHash: storage.ScopesHash([]string{"openid"}), } - keyD := DCRKey{ + keyD := Key{ Issuer: "https://idp-a.example.com", RedirectURI: "https://toolhive.example.com/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid", "email"}), @@ -96,8 +96,8 @@ func TestStorageBackedStore_DistinctKeysDoNotCollide(t *testing.T) { // AuthorizationEndpoint / TokenEndpoint per validateDCRCredentialsForStore; // supply a minimal valid resolution so the storage-backed adapter accepts // the Put, since the test asserts key-distinctness and not field shape. - resolution := func(clientID string) *DCRResolution { - return &DCRResolution{ + resolution := func(clientID string) *Resolution { + return &Resolution{ ClientID: clientID, AuthorizationEndpoint: "https://idp.example.com/authorize", TokenEndpoint: "https://idp.example.com/token", @@ -110,7 +110,7 @@ func TestStorageBackedStore_DistinctKeysDoNotCollide(t *testing.T) { require.NoError(t, store.Put(ctx, keyD, resolution("d"))) for _, tc := range []struct { - key DCRKey + key Key expected string }{ {keyA, "a"}, @@ -131,7 +131,7 @@ func TestStorageBackedStore_Put_OverwritesExisting(t *testing.T) { store := newMemoryDCRStore(t) ctx := context.Background() - key := DCRKey{ + key := Key{ Issuer: "https://idp.example.com", RedirectURI: "https://x.example.com/cb", ScopesHash: storage.ScopesHash([]string{"openid"}), @@ -143,12 +143,12 @@ func TestStorageBackedStore_Put_OverwritesExisting(t *testing.T) { Authorization: "https://idp.example.com/authorize", Token: "https://idp.example.com/token", } - require.NoError(t, store.Put(ctx, key, &DCRResolution{ + require.NoError(t, store.Put(ctx, key, &Resolution{ ClientID: "first", AuthorizationEndpoint: endpoints.Authorization, TokenEndpoint: endpoints.Token, })) - require.NoError(t, store.Put(ctx, key, &DCRResolution{ + require.NoError(t, store.Put(ctx, key, &Resolution{ ClientID: "second", AuthorizationEndpoint: endpoints.Authorization, TokenEndpoint: endpoints.Token, @@ -169,7 +169,7 @@ func TestStorageBackedStore_Put_RejectsNilResolution(t *testing.T) { store := newMemoryDCRStore(t) 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) @@ -187,12 +187,12 @@ func TestStorageBackedStore_GetReturnsDefensiveCopy(t *testing.T) { store := newMemoryDCRStore(t) ctx := context.Background() - key := DCRKey{ + key := Key{ Issuer: "https://idp.example.com", RedirectURI: "https://x.example.com/cb", ScopesHash: storage.ScopesHash([]string{"openid"}), } - require.NoError(t, store.Put(ctx, key, &DCRResolution{ + require.NoError(t, store.Put(ctx, key, &Resolution{ ClientID: "orig", AuthorizationEndpoint: "https://idp.example.com/authorize", TokenEndpoint: "https://idp.example.com/token", @@ -217,7 +217,7 @@ func TestStorageBackedStore_GetReturnsDefensiveCopy(t *testing.T) { // TestStorageBackedStore_ConcurrentAccess fans out N goroutines // performing alternating Put / Get against overlapping and disjoint keys, // exercising the safe-for-concurrent-use contract advertised on the -// dcrResolutionCache interface. The contract is satisfied by the underlying +// CredentialStore interface. The contract is satisfied by the underlying // storage.DCRCredentialStore (which holds the lock); this test runs under // `go test -race` so any future regression that drops the storage backend's // guarantee, or an adapter change that races on its own state, fails loudly. @@ -238,15 +238,15 @@ func TestStorageBackedStore_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), @@ -261,7 +261,7 @@ func TestStorageBackedStore_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), AuthorizationEndpoint: "https://idp.example.com/authorize", TokenEndpoint: "https://idp.example.com/token", @@ -302,9 +302,9 @@ func TestStorageBackedStore_ConcurrentAccess(t *testing.T) { // TestResolutionCredentialsRoundTrip pins the field-by-field contract // between resolutionToCredentials and credentialsToResolution: which // fields survive a round-trip, which are intentionally dropped, and -// which are recovered from the persisted DCRKey. The test exists +// which are recovered from the persisted Key. The test exists // because the two converters are the seam where a field added to either -// DCRResolution or storage.DCRCredentials must be paired with an update +// Resolution or storage.DCRCredentials must be paired with an update // here; without coverage, a future field addition would silently fail // to persist across an authserver restart. // @@ -315,7 +315,7 @@ func TestStorageBackedStore_ConcurrentAccess(t *testing.T) { // dropped from DCRCredentials and recovered via Key.RedirectURI on // read. // -// ProviderName is the one DCRCredentials field with no DCRResolution +// ProviderName is the one DCRCredentials field with no Resolution // counterpart. It is documented in storage.DCRCredentials as "debug / // audit only — never used as a primary key" and no current consumer // reads it. The decision to leave it unpopulated by the runner is @@ -327,13 +327,13 @@ func TestResolutionCredentialsRoundTrip(t *testing.T) { now := time.Now().UTC().Round(time.Second) expiry := now.Add(30 * 24 * time.Hour) - key := DCRKey{ + key := Key{ Issuer: "https://idp.example.com", RedirectURI: "https://thv.example.com/oauth/callback", ScopesHash: storage.ScopesHash([]string{"openid", "profile"}), } - original := &DCRResolution{ + original := &Resolution{ ClientID: "round-trip-client-id", ClientSecret: "round-trip-secret", AuthorizationEndpoint: "https://idp.example.com/authorize", @@ -358,7 +358,7 @@ func TestResolutionCredentialsRoundTrip(t *testing.T) { assert.Equal(t, key, creds.Key, "Key must round-trip via the explicit parameter") assert.Empty(t, creds.ProviderName, - "ProviderName has no DCRResolution counterpart and is intentionally not populated; "+ + "ProviderName has no Resolution counterpart and is intentionally not populated; "+ "a future contributor threading it through must update this assertion and the converters together") roundTripped := credentialsToResolution(creds) diff --git a/pkg/auth/dcr/testhelpers_test.go b/pkg/auth/dcr/testhelpers_test.go new file mode 100644 index 0000000000..7dc1c4cb11 --- /dev/null +++ b/pkg/auth/dcr/testhelpers_test.go @@ -0,0 +1,28 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package dcr + +import ( + "testing" + + "github.com/stacklok/toolhive/pkg/authserver/storage" +) + +// newMemoryDCRStore is a test-only convenience constructor wrapping +// storage.NewMemoryStorage in the runner-side adapter. Production deployments +// do NOT reach this constructor — NewEmbeddedAuthServer type-asserts the +// shared authserver storage to storage.DCRCredentialStore and passes it to +// NewStorageBackedStore directly. +// +// The caller's *testing.T is required because storage.NewMemoryStorage +// launches a background cleanup goroutine on construction; the helper +// registers t.Cleanup(stor.Close) so each test releases the goroutine when +// it finishes. Without this every test that built a fresh store would leak a +// cleanupLoop goroutine for the duration of the test process. +func newMemoryDCRStore(t *testing.T) CredentialStore { + t.Helper() + stor := storage.NewMemoryStorage() + t.Cleanup(func() { _ = stor.Close() }) + return NewStorageBackedStore(stor) +} diff --git a/pkg/authserver/runner/dcr_testhelpers_test.go b/pkg/authserver/runner/dcr_testhelpers_test.go index b0abdfd8ad..495dae7414 100644 --- a/pkg/authserver/runner/dcr_testhelpers_test.go +++ b/pkg/authserver/runner/dcr_testhelpers_test.go @@ -6,23 +6,29 @@ package runner import ( "testing" + "github.com/stacklok/toolhive/pkg/auth/dcr" "github.com/stacklok/toolhive/pkg/authserver/storage" ) // newMemoryDCRStore is a test-only convenience constructor wrapping -// storage.NewMemoryStorage in the runner-side adapter. Production deployments +// storage.NewMemoryStorage in the dcr-package adapter. Production deployments // do NOT reach this constructor — NewEmbeddedAuthServer type-asserts the // shared authserver storage to storage.DCRCredentialStore and passes it to -// newStorageBackedStore directly. +// dcr.NewStorageBackedStore directly. // // The caller's *testing.T is required because storage.NewMemoryStorage // launches a background cleanup goroutine on construction; the helper // registers t.Cleanup(stor.Close) so each test releases the goroutine when // it finishes. Without this every test that built a fresh store would leak a // cleanupLoop goroutine for the duration of the test process. -func newMemoryDCRStore(t *testing.T) dcrResolutionCache { +// +// A near-identical helper lives in pkg/auth/dcr/testhelpers_test.go for +// tests inside that package; the duplication is intentional because Go test +// helpers cannot be shared across packages without exporting them, and the +// cleanup-aware shape is too narrow to justify a public API surface. +func newMemoryDCRStore(t *testing.T) dcr.CredentialStore { t.Helper() stor := storage.NewMemoryStorage() t.Cleanup(func() { _ = stor.Close() }) - return newStorageBackedStore(stor) + return dcr.NewStorageBackedStore(stor) } diff --git a/pkg/authserver/runner/embeddedauthserver.go b/pkg/authserver/runner/embeddedauthserver.go index 9d67d06a43..c036d0cb1f 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" @@ -94,13 +95,13 @@ func newEmbeddedAuthServerWithStorage( ) (retEAS *EmbeddedAuthServer, retErr error) { // From here on, any error must close stor before returning. // - // Both errors are passed through sanitizeErrorForLog before being + // Both errors are passed through dcr.SanitizeErrorForLog before being // recorded: closeErr for symmetry with retErr, retErr because the // most common cause of reaching this gate is a wrapped DCR failure // whose error chain may inline several KiB of the upstream's raw // /register response body — that body is attacker-influenced and may // contain URL components that carry credentials (userinfo, query, - // fragment). The existing logDCRStepError boundary log routes + // fragment). The existing dcr.LogStepError boundary log routes // through the same sanitiser; keep the two log paths consistent so // the cleanup log cannot regress to a less-defended state. The // "cause" key matches the package-wide vocabulary for the @@ -109,8 +110,8 @@ func newEmbeddedAuthServerWithStorage( if retErr != nil { if closeErr := stor.Close(); closeErr != nil { slog.Warn("failed to close storage on NewEmbeddedAuthServer error path", - "error", sanitizeErrorForLog(closeErr), - "cause", sanitizeErrorForLog(retErr), + "error", dcr.SanitizeErrorForLog(closeErr), + "cause", dcr.SanitizeErrorForLog(retErr), ) } } @@ -147,7 +148,7 @@ func newEmbeddedAuthServerWithStorage( // 5. Build upstream configurations. The DCR resolver caches RFC 7591 // resolutions in dcrStore so re-entrant boot/reload paths reuse // previously-registered upstream clients instead of re-registering. - upstreams, err := buildUpstreamConfigs(ctx, cfg.Upstreams, cfg.Issuer, newStorageBackedStore(dcrStore)) + upstreams, err := buildUpstreamConfigs(ctx, cfg.Upstreams, cfg.Issuer, dcr.NewStorageBackedStore(dcrStore)) if err != nil { return nil, fmt.Errorf("failed to build upstream configs: %w", err) } @@ -355,21 +356,21 @@ 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( ctx context.Context, runConfigs []authserver.UpstreamRunConfig, issuer string, - dcrStore dcrResolutionCache, + dcrStore dcr.CredentialStore, ) ([]authserver.UpstreamConfig, error) { configs := make([]authserver.UpstreamConfig, 0, len(runConfigs)) @@ -378,26 +379,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 + 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 } @@ -407,12 +408,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 925200c8f0..9e43cfce59 100644 --- a/pkg/authserver/runner/embeddedauthserver_test.go +++ b/pkg/authserver/runner/embeddedauthserver_test.go @@ -1582,7 +1582,7 @@ func TestBuildUpstreamConfigs_DCR(t *testing.T) { // Store now contains the resolution under the canonical DCRKey. redirectURI := server.URL + "/oauth/callback" - key := DCRKey{ + key := storage.DCRKey{ Issuer: server.URL, RedirectURI: redirectURI, ScopesHash: storage.ScopesHash([]string{"openid", "profile"}), @@ -1721,7 +1721,7 @@ func TestNewEmbeddedAuthServer_DCRBoot(t *testing.T) { // boot persisted the resolution there directly (no separate in-memory // store was created). redirectURI := server.URL + "/oauth/callback" - key := DCRKey{ + key := storage.DCRKey{ Issuer: server.URL, RedirectURI: redirectURI, ScopesHash: storage.ScopesHash([]string{"openid", "profile"}), @@ -1901,7 +1901,7 @@ func TestEmbeddedAuthServer_DCRStorePersistsAcrossClose(t *testing.T) { // replica and cross-restart reuse paths depend on: that the // resolution lives in storage, not in process-local cache state. redirectURI := server.URL + "/oauth/callback" - key := DCRKey{ + key := storage.DCRKey{ Issuer: server.URL, RedirectURI: redirectURI, ScopesHash: storage.ScopesHash([]string{"openid", "profile"}), @@ -1926,7 +1926,7 @@ func TestEmbeddedAuthServer_DCRStorePersistsAcrossClose(t *testing.T) { // URL-bearing error from Close. It exists so // TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog can verify that the // deferred-cleanup gate routes both closeErr and retErr through -// sanitizeErrorForLog, scrubbing any query / userinfo / fragment that might +// dcr.SanitizeErrorForLog, scrubbing any query / userinfo / fragment that might // carry credentials in a future regression. type urlErrorOnCloseStorage struct { storage.Storage @@ -1944,7 +1944,7 @@ func (s *urlErrorOnCloseStorage) Close() error { // TestNewEmbeddedAuthServer_DeferredCleanupSanitizesLog pins the post-#5196 // invariant that the deferred-cleanup slog.Warn at the top of // newEmbeddedAuthServerWithStorage routes both closeErr and retErr through -// sanitizeErrorForLog, so a future regression that drops the call (or that +// dcr.SanitizeErrorForLog, so a future regression that drops the call (or that // changes the error chain to inline an upstream response body containing a // userinfo/query/fragment) cannot silently leak secrets to operator logs. //