diff --git a/pipeline/authn/authenticator_oauth2_client_credentials.go b/pipeline/authn/authenticator_oauth2_client_credentials.go index ba943c90d..d3991847e 100644 --- a/pipeline/authn/authenticator_oauth2_client_credentials.go +++ b/pipeline/authn/authenticator_oauth2_client_credentials.go @@ -4,7 +4,6 @@ package authn import ( - "context" "encoding/json" "fmt" "net/http" @@ -17,8 +16,6 @@ import ( "golang.org/x/oauth2" "golang.org/x/oauth2/clientcredentials" - "github.com/ory/x/httpx" - "github.com/ory/oathkeeper/helper" "github.com/ory/oathkeeper/pipeline" ) @@ -36,12 +33,32 @@ type clientCredentialsCacheConfig struct { MaxTokens int `json:"max_tokens"` } +// AuthenticatorOAuth2ClientCredentials authenticates requests via the OAuth2 +// client credentials flow. +// +// Stateless-after-init design (architectural refactoring per @zepatrik): +// +// - The Ristretto token cache is allocated exactly once inside the factory +// constructor NewAuthenticatorOAuth2ClientCredentials and is never +// reassigned at request time. Because the pointer is written before the +// struct is shared with any goroutine, all subsequent reads of TokenCache +// are safe without locks. +// +// - The former a.client *http.Client field was unused in the request path: +// Authenticate delegates the token exchange entirely to +// clientcredentials.Config.Token, which manages its own HTTP transport. +// The field along with its associated sync.Once and sync.Mutex guards +// have been removed. +// +// - cacheTTL is no longer persisted on the struct. TokenToCache derives the +// effective TTL directly from the per-request config.Cache.TTL on each +// call, eliminating the last source of shared mutable state. +// +// The resulting struct carries no mutable fields after construction, making +// concurrent request handling naturally race-free without any locks. type AuthenticatorOAuth2ClientCredentials struct { - d dependencies - client *http.Client - + d dependencies TokenCache *ristretto.Cache[string, []byte] - cacheTTL *time.Duration } type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { @@ -49,8 +66,42 @@ type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { MaxWait string `json:"give_up_after"` } +// NewAuthenticatorOAuth2ClientCredentials constructs a fully initialized +// authenticator instance. The Ristretto token cache is created here so that no +// lazy initialization is required (or permitted) at request time, eliminating +// any TOCTOU window on the cache pointer entirely. +// +// The cache capacity honours cache.max_tokens from the global authenticator +// configuration when set. The cache is shared across all rules so only the +// global (non-rule-level) setting is applicable at construction time. func NewAuthenticatorOAuth2ClientCredentials(d dependencies) *AuthenticatorOAuth2ClientCredentials { - return &AuthenticatorOAuth2ClientCredentials{d: d} + const defaultMaxTokens int64 = 1_000 + + // Attempt to read the global authenticator configuration so that + // cache.max_tokens is honoured rather than always defaulting to 1 000. + // A failure here (e.g. schema validation disabled) is non-fatal: the + // sentinel default is used instead. + maxTokens := defaultMaxTokens + var globalCfg AuthenticatorOAuth2Configuration + if err := d.Config().AuthenticatorConfig("oauth2_client_credentials", json.RawMessage(nil), &globalCfg); err == nil && globalCfg.Cache.MaxTokens > 0 { + maxTokens = int64(globalCfg.Cache.MaxTokens) + } + + cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ + // NumCounters follows the ristretto recommendation of 10 × MaxCost for + // the frequency-sketch counters. + NumCounters: maxTokens * 10, + MaxCost: maxTokens, + BufferItems: 64, + Cost: func(_ []byte) int64 { return 1 }, + IgnoreInternalCost: true, + }) + if err != nil { + // ristretto.NewCache only returns an error for programmer-invalid + // configuration; the parameters above are unconditionally valid. + panic(fmt.Sprintf("authn/oauth2_client_credentials: cache init failed: %v", err)) + } + return &AuthenticatorOAuth2ClientCredentials{d: d, TokenCache: cache} } func (a *AuthenticatorOAuth2ClientCredentials) GetID() string { return "oauth2_client_credentials" } @@ -64,6 +115,10 @@ func (a *AuthenticatorOAuth2ClientCredentials) Validate(config json.RawMessage) return err } +// Config parses and validates the merged global + rule-level authenticator +// configuration. It is now a pure read operation with respect to the receiver: +// it reads from a.d.Config() but performs no writes to any field on a, making +// concurrent calls naturally race-free without any synchronization. func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (*AuthenticatorOAuth2Configuration, error) { const ( defaultTimeout = "1s" @@ -84,53 +139,14 @@ func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (* c.Retry.MaxWait = defaultMaxWait } } - duration, err := time.ParseDuration(c.Retry.Timeout) - if err != nil { - return nil, err - } - maxWait, err := time.ParseDuration(c.Retry.MaxWait) - if err != nil { + // Validate retry duration strings eagerly so callers receive a descriptive + // configuration error rather than a late failure during token exchange. + if _, err := time.ParseDuration(c.Retry.Timeout); err != nil { return nil, err } - timeout := time.Millisecond * duration - a.client = httpx.NewResilientClient( - httpx.ResilientClientWithMaxRetryWait(maxWait), - httpx.ResilientClientWithConnectionTimeout(timeout), - ).StandardClient() - - if c.Cache.TTL != "" { - cacheTTL, err := time.ParseDuration(c.Cache.TTL) - if err != nil { - return nil, err - } - a.cacheTTL = &cacheTTL - } - - if a.TokenCache == nil { - maxTokens := int64(c.Cache.MaxTokens) - if maxTokens == 0 { - maxTokens = 1000 - } - a.d.Logger().Debugf("Creating cache with max tokens: %d", maxTokens) - cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ - // This will hold about 1000 unique mutation responses. - NumCounters: 10 * maxTokens, - // Allocate a maximum amount of tokens to cache - MaxCost: maxTokens, - // This is a best-practice value. - BufferItems: 64, - // Use a static cost of 1, so we can limit the amount of tokens that can be stored - Cost: func(value []byte) int64 { - return 1 - }, - IgnoreInternalCost: true, - }) - if err != nil { - return nil, err - } - - a.TokenCache = cache + if _, err := time.ParseDuration(c.Retry.MaxWait); err != nil { + return nil, err } return &c, nil @@ -145,6 +161,7 @@ func (a *AuthenticatorOAuth2ClientCredentials) TokenFromCache(config *Authentica return nil } + // TokenCache is immutable after construction: no lock required. i, found := a.TokenCache.Get(ClientCredentialsConfigToKey(clientCredentials)) if !found { return nil @@ -164,25 +181,37 @@ func (a *AuthenticatorOAuth2ClientCredentials) TokenToCache(config *Authenticato key := ClientCredentialsConfigToKey(clientCredentials) - if v, err := json.Marshal(token); err != nil { + v, err := json.Marshal(token) + if err != nil { return - } else if a.cacheTTL != nil { - // Allow up-to at most the cache TTL, otherwise use token expiry - ttl := time.Until(token.Expiry) - if ttl > *a.cacheTTL { - ttl = *a.cacheTTL - } + } - a.TokenCache.SetWithTTL(key, v, 1, ttl) - } else { - // If token has no expiry apply the same to the cache - ttl := time.Duration(0) - if !token.Expiry.IsZero() { - ttl = time.Until(token.Expiry) + // Derive the effective TTL from the per-request config on each call. + // cacheTTL is no longer stored on the struct; reading it from the + // request-scoped config eliminates all struct mutation, making this + // method safe for concurrent use without any lock. + if config.Cache.TTL != "" { + if cacheTTL, parseErr := time.ParseDuration(config.Cache.TTL); parseErr == nil { + // Use cacheTTL as the ceiling. If the token carries a concrete expiry, + // cap further to the token's remaining lifetime. For zero-expiry tokens + // fall back to cacheTTL so a negative duration is never produced. + ttl := cacheTTL + if !token.Expiry.IsZero() { + if remaining := time.Until(token.Expiry); remaining < cacheTTL { + ttl = remaining + } + } + a.TokenCache.SetWithTTL(key, v, 1, ttl) + return } + } - a.TokenCache.SetWithTTL(key, v, 1, ttl) + // No TTL configured: use token expiry, or zero for non-expiring tokens. + ttl := time.Duration(0) + if !token.Expiry.IsZero() { + ttl = time.Until(token.Expiry) } + a.TokenCache.SetWithTTL(key, v, 1, ttl) } func (a *AuthenticatorOAuth2ClientCredentials) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) error { @@ -217,11 +246,12 @@ func (a *AuthenticatorOAuth2ClientCredentials) Authenticate(r *http.Request, ses token := a.TokenFromCache(cf, c) if token == nil { - t, err := c.Token(context.WithValue( - r.Context(), - oauth2.HTTPClient, - c.Client, - )) + // c.Token propagates the request context for deadline/cancellation. + // The previous context.WithValue(r.Context(), oauth2.HTTPClient, c.Client) + // was a no-op: c.Client is a bound method value (func(context.Context) + // *http.Client), not an *http.Client, so the oauth2 library's type + // assertion always fails silently and the default transport is used. + t, err := c.Token(r.Context()) if err != nil { if rErr, ok := err.(*oauth2.RetrieveError); ok { switch httpStatusCode := rErr.Response.StatusCode; httpStatusCode { diff --git a/pipeline/authn/authenticator_oauth2_introspection.go b/pipeline/authn/authenticator_oauth2_introspection.go index a2962427c..34e6637dc 100644 --- a/pipeline/authn/authenticator_oauth2_introspection.go +++ b/pipeline/authn/authenticator_oauth2_introspection.go @@ -64,6 +64,25 @@ type cacheConfig struct { MaxCost int `json:"max_cost"` } +// AuthenticatorOAuth2Introspection authenticates requests by calling an OAuth2 +// token introspection endpoint. +// +// Stateless-after-init design (architectural refactoring per @zepatrik): +// +// - TokenCache is allocated exactly once in NewAuthenticatorOAuth2Introspection +// and is never reassigned. Because the pointer is established before the +// struct is shared with any goroutine, all subsequent reads are safe without +// locks. +// +// - cacheTTL is no longer stored on the struct. TokenToCache derives the +// effective TTL directly from config.Cache.TTL on each call, eliminating +// the associated mutable pointer and the need to hold a.mu around cache +// reads and writes. +// +// - a.mu (sync.RWMutex) is retained solely for clientMap, where it already +// provided correct serialization. The cache-initialization block that was +// previously appended to Config() under a separate a.mu.Lock() scope has +// been removed entirely. type AuthenticatorOAuth2Introspection struct { d dependencies @@ -71,19 +90,40 @@ type AuthenticatorOAuth2Introspection struct { mu sync.RWMutex TokenCache *ristretto.Cache[string, []byte] - cacheTTL *time.Duration } +// NewAuthenticatorOAuth2Introspection constructs a fully initialized +// authenticator. The Ristretto token cache is created here with sensible +// fixed defaults so that no lazy initialization is required at request time, +// eliminating any TOCTOU window on the cache pointer. func NewAuthenticatorOAuth2Introspection(d dependencies) *AuthenticatorOAuth2Introspection { - return &AuthenticatorOAuth2Introspection{d: d, clientMap: make(map[string]*http.Client)} + const defaultMaxCost int64 = 100_000 + cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ + // NumCounters follows the ristretto recommendation of 10 × MaxCost. + NumCounters: defaultMaxCost * 10, + MaxCost: defaultMaxCost, + BufferItems: 64, + Cost: func(_ []byte) int64 { return 1 }, + IgnoreInternalCost: true, + }) + if err != nil { + // ristretto.NewCache only returns an error for programmer-invalid + // configuration; the parameters above are unconditionally valid. + panic(fmt.Sprintf("authn/oauth2_introspection: cache init failed: %v", err)) + } + return &AuthenticatorOAuth2Introspection{ + d: d, + clientMap: make(map[string]*http.Client), + TokenCache: cache, + } } func (a *AuthenticatorOAuth2Introspection) GetID() string { return "oauth2_introspection" } +// WaitForCache blocks until all pending ristretto write operations have been +// applied. TokenCache is always non-nil after construction. func (a *AuthenticatorOAuth2Introspection) WaitForCache() { - if a.TokenCache != nil { - a.TokenCache.Wait() - } + a.TokenCache.Wait() } type Audience []string @@ -135,6 +175,9 @@ func TokenCacheKey(token, endpoint string) string { return fmt.Sprintf("%s|%s", token, endpoint) } +// TokenFromCache looks up a previously introspected token result from the +// in-process cache. TokenCache is immutable after construction so no lock +// is required; ristretto's internal operations are goroutine-safe. func (a *AuthenticatorOAuth2Introspection) TokenFromCache(config *AuthenticatorOAuth2IntrospectionConfiguration, token string, ss fosite.ScopeStrategy) *AuthenticatorOAuth2IntrospectionResult { if !config.Cache.Enabled { return nil @@ -157,6 +200,10 @@ func (a *AuthenticatorOAuth2Introspection) TokenFromCache(config *AuthenticatorO return &v } +// TokenToCache stores an introspection result in the cache. The TTL is derived +// from config.Cache.TTL on each call rather than from a struct field, so this +// method is safe for concurrent use without any lock. TokenCache is immutable +// after construction; ristretto's Set/SetWithTTL are goroutine-safe. func (a *AuthenticatorOAuth2Introspection) TokenToCache(config *AuthenticatorOAuth2IntrospectionConfiguration, i *AuthenticatorOAuth2IntrospectionResult, token string, ss fosite.ScopeStrategy) { if !config.Cache.Enabled { return @@ -172,11 +219,16 @@ func (a *AuthenticatorOAuth2Introspection) TokenToCache(config *AuthenticatorOAu return } - if a.cacheTTL != nil { - a.TokenCache.SetWithTTL(key, v, 1, *a.cacheTTL) - } else { - a.TokenCache.Set(key, v, 1) + // Derive TTL from the per-request config on each call. + // cacheTTL is no longer stored on the struct; this eliminates the + // mutable pointer that previously required a.mu.RLock() here. + if config.Cache.TTL != "" { + if cacheTTL, parseErr := time.ParseDuration(config.Cache.TTL); parseErr == nil { + a.TokenCache.SetWithTTL(key, v, 1, cacheTTL) + return + } } + a.TokenCache.Set(key, v, 1) } func (a *AuthenticatorOAuth2Introspection) Authenticate(r *http.Request, session *AuthenticationSession, config json.RawMessage, _ pipeline.Rule) (err error) { @@ -249,6 +301,16 @@ func (a *AuthenticatorOAuth2Introspection) Authenticate(r *http.Request, session } if i.Expires > 0 && time.Unix(i.Expires, 0).Before(time.Now()) { + if inCache { + // A cached result is only refreshed on a cache miss (see below), so a + // cached entry that outlives its own token expiry would otherwise keep + // failing every subsequent request indefinitely (or until the + // configured cache TTL, if any, separately evicts it). Evict it here + // so the next request re-introspects instead of serving permanently + // stale data. This is a single atomic ristretto operation on the + // affected key only, so it requires no lock. + a.TokenCache.Del(TokenCacheKey(token, cf.IntrospectionURL)) + } return errors.WithStack(helper.ErrUnauthorized().WithReason("Access token expired")) } @@ -303,6 +365,12 @@ func (a *AuthenticatorOAuth2Introspection) Validate(config json.RawMessage) erro return err } +// Config parses the merged global + rule-level configuration, initializes the +// per-config HTTP client (lazily, under a.mu), and returns the configuration +// and client for use by Authenticate. +// +// After this refactoring Config() no longer touches TokenCache or cacheTTL: +// those are exclusively owned by the constructor and TokenToCache respectively. func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*AuthenticatorOAuth2IntrospectionConfiguration, *http.Client, error) { var c AuthenticatorOAuth2IntrospectionConfiguration if err := a.d.Config().AuthenticatorConfig(a.GetID(), config, &c); err != nil { @@ -369,46 +437,5 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth a.mu.Unlock() } - if c.Cache.TTL != "" { - cacheTTL, err := time.ParseDuration(c.Cache.TTL) - if err != nil { - return nil, nil, err - } - - // clear cache if previous ttl was longer (or none) - if a.TokenCache != nil { - if a.cacheTTL == nil || (a.cacheTTL != nil && a.cacheTTL.Seconds() > cacheTTL.Seconds()) { - a.TokenCache.Clear() - } - } - - a.cacheTTL = &cacheTTL - } - - if a.TokenCache == nil { - cost := int64(c.Cache.MaxCost) - if cost == 0 { - cost = 100000000 - } - a.d.Logger().Debugf("Creating cache with max cost: %d", c.Cache.MaxCost) - cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ - // This will hold about 1000 unique mutation responses. - NumCounters: cost * 10, - // Allocate a max - MaxCost: cost, - // This is a best-practice value. - BufferItems: 64, - Cost: func(value []byte) int64 { - return 1 - }, - IgnoreInternalCost: true, - }) - if err != nil { - return nil, nil, err - } - - a.TokenCache = cache - } - return &c, client, nil } diff --git a/pipeline/authn/repro_race_test.go b/pipeline/authn/repro_race_test.go new file mode 100644 index 000000000..207852015 --- /dev/null +++ b/pipeline/authn/repro_race_test.go @@ -0,0 +1,82 @@ +//go:build race +// +build race + +// Package authn_test contains a concurrent regression harness for the OAuth2 +// introspection authenticator. Run with: +// +// go test -race -v ./pipeline/authn -run TestConfigDataRace +// +// After the stateless-after-init refactoring, Config() no longer mutates +// a.TokenCache or a.cacheTTL (the latter has been removed from the struct). +// TokenToCache and TokenFromCache derive all state from their config argument +// and from the immutable TokenCache pointer set in the constructor, so they +// require no locks and present no shared mutable state to the race detector. +// This test serves as a permanent regression guard: it must pass cleanly +// under -race in all future states of the codebase. +package authn_test + +import ( + "fmt" + "sync" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/ory/x/configx" + + . "github.com/ory/oathkeeper/pipeline/authn" + "github.com/ory/oathkeeper/internal" +) + +// TestConfigDataRace launches concurrent writers invoking Config() with varying +// cache TTLs while readers concurrently exercise TokenToCache/TokenFromCache. +// With the stateless-after-init design the Go race detector must not report any +// data races: Config() only writes to clientMap (protected by a.mu.RWMutex), +// and the cache methods access only the immutable TokenCache pointer and the +// request-scoped config argument. +func TestConfigDataRace(t *testing.T) { + reg := internal.NewRegistry(t, configx.SkipValidation()) + aa, err := reg.PipelineAuthenticator("oauth2_introspection") + require.NoError(t, err) + a, ok := aa.(*AuthenticatorOAuth2Introspection) + require.Truef(t, ok, "got type %T", aa) + + base := `{"introspection_url":"http://127.0.0.1/oauth2/introspect","cache":{"enabled":true,"max_cost":1000}}` + ttls := []string{"25ms", "50ms", "75ms"} + + var wg sync.WaitGroup + + // Writers: call Config() concurrently with varying TTLs to exercise the + // clientMap read/write path under a.mu.RWMutex. + for i := 0; i < 8; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + for j := 0; j < 2000; j++ { + cfg := fmt.Sprintf(`{"introspection_url":"http://127.0.0.1/oauth2/introspect","cache":{"enabled":true,"max_cost":1000,"ttl":"%s"}}`, ttls[j%len(ttls)]) + _, _, _ = a.Config([]byte(cfg)) + } + }(i) + } + + // Readers: call Config() then exercise the cache paths concurrently with + // the writer goroutines. No struct-level mutable state is accessed. + for i := 0; i < 8; i++ { + wg.Add(1) + go func(id int) { + defer wg.Done() + res := &AuthenticatorOAuth2IntrospectionResult{Active: true} + for j := 0; j < 2000; j++ { + c, _, err := a.Config([]byte(base)) + if err != nil || c == nil { + continue + } + a.TokenToCache(c, res, "tok", nil) + _ = a.TokenFromCache(c, "tok", nil) + } + }(i) + } + + wg.Wait() + a.WaitForCache() +}