From 898c9877cf5160fa42e4feb9d5dd2e9e6415571f Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Wed, 8 Jul 2026 21:53:54 +0800 Subject: [PATCH 1/8] fix(authn): resolve concurrent shared mutable state data races in oauth2 pipelines --- ...authenticator_oauth2_client_credentials.go | 55 +++++++++++++++++-- .../authenticator_oauth2_introspection.go | 16 +++++- 2 files changed, 63 insertions(+), 8 deletions(-) diff --git a/pipeline/authn/authenticator_oauth2_client_credentials.go b/pipeline/authn/authenticator_oauth2_client_credentials.go index ba943c90d..72b7e99a2 100644 --- a/pipeline/authn/authenticator_oauth2_client_credentials.go +++ b/pipeline/authn/authenticator_oauth2_client_credentials.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" "github.com/dgraph-io/ristretto/v2" @@ -36,9 +37,30 @@ type clientCredentialsCacheConfig struct { MaxTokens int `json:"max_tokens"` } +// AuthenticatorOAuth2ClientCredentials authenticates requests via the OAuth2 +// client credentials flow. +// +// Integrity fix: the original struct had no synchronization primitives, causing +// two distinct data races under concurrent request handling: +// +// 1. a.client was overwritten on every Config() invocation — any concurrent +// goroutine could observe a partially-initialized or entirely different +// *http.Client pointer, leading to non-deterministic transport behaviour. +// +// 2. a.TokenCache and a.cacheTTL were written without any lock, meaning the +// TOCTOU check "if a.TokenCache == nil" was not atomic: two goroutines +// could both observe nil, each allocate a new ristretto cache (spawning +// background goroutines), and then one would overwrite the other's pointer +// — leaking the orphaned cache and its goroutines permanently. +// +// The fix introduces: +// - clientOnce (sync.Once) to guarantee a.client is initialized exactly once. +// - mu (sync.Mutex) to serialize writes to a.TokenCache and a.cacheTTL. type AuthenticatorOAuth2ClientCredentials struct { - d dependencies - client *http.Client + d dependencies + client *http.Client + clientOnce sync.Once + mu sync.Mutex TokenCache *ristretto.Cache[string, []byte] cacheTTL *time.Duration @@ -94,14 +116,33 @@ func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (* return nil, err } timeout := time.Millisecond * duration - a.client = httpx.NewResilientClient( - httpx.ResilientClientWithMaxRetryWait(maxWait), - httpx.ResilientClientWithConnectionTimeout(timeout), - ).StandardClient() + + // clientOnce.Do guarantees that a.client is allocated and written exactly + // once, regardless of how many goroutines are concurrently executing + // Config(). Subsequent calls observe the pointer set by the first caller + // without any unsynchronized write. Timeout and maxWait are captured from + // the first successful config resolution; the values are validated above + // before Do is reached, so the closure is always called with well-formed + // duration arguments. + a.clientOnce.Do(func() { + a.client = httpx.NewResilientClient( + httpx.ResilientClientWithMaxRetryWait(maxWait), + httpx.ResilientClientWithConnectionTimeout(timeout), + ).StandardClient() + }) + + // mu serializes writes to a.cacheTTL and a.TokenCache so that the + // check-then-act sequence "if a.TokenCache == nil { ... a.TokenCache = cache }" + // is atomic. Without this lock, two goroutines can both observe nil, + // independently allocate separate ristretto caches (each spawning + // background goroutines), and then one silently overwrites the other's + // pointer, permanently leaking the orphaned cache. + a.mu.Lock() if c.Cache.TTL != "" { cacheTTL, err := time.ParseDuration(c.Cache.TTL) if err != nil { + a.mu.Unlock() return nil, err } a.cacheTTL = &cacheTTL @@ -127,12 +168,14 @@ func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (* IgnoreInternalCost: true, }) if err != nil { + a.mu.Unlock() return nil, err } a.TokenCache = cache } + a.mu.Unlock() return &c, nil } diff --git a/pipeline/authn/authenticator_oauth2_introspection.go b/pipeline/authn/authenticator_oauth2_introspection.go index a2962427c..b5745886f 100644 --- a/pipeline/authn/authenticator_oauth2_introspection.go +++ b/pipeline/authn/authenticator_oauth2_introspection.go @@ -369,15 +369,25 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth a.mu.Unlock() } + // Integrity fix: TokenCache initialization, cacheTTL pointer assignment, and + // TokenCache.Clear() are all brought under the existing a.mu write lock to + // eliminate the TOCTOU data race where concurrent goroutines could observe + // a nil TokenCache, each independently allocate a new cache instance, and + // overwrite each other — corrupting the singleton and leaking goroutines + // spawned by ristretto's internal workers. + a.mu.Lock() + if c.Cache.TTL != "" { cacheTTL, err := time.ParseDuration(c.Cache.TTL) if err != nil { + a.mu.Unlock() return nil, nil, err } - // clear cache if previous ttl was longer (or none) + // Clear cache if the previous TTL was longer (or unset), so stale + // entries with a longer lifetime than the new policy cannot linger. if a.TokenCache != nil { - if a.cacheTTL == nil || (a.cacheTTL != nil && a.cacheTTL.Seconds() > cacheTTL.Seconds()) { + if a.cacheTTL == nil || a.cacheTTL.Seconds() > cacheTTL.Seconds() { a.TokenCache.Clear() } } @@ -404,11 +414,13 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth IgnoreInternalCost: true, }) if err != nil { + a.mu.Unlock() return nil, nil, err } a.TokenCache = cache } + a.mu.Unlock() return &c, client, nil } From 6349c84b6d9383d49030dd7cd661e5ee7d32b052 Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Wed, 8 Jul 2026 22:26:53 +0800 Subject: [PATCH 2/8] test(authn): add concurrent Config() race reproduction harness for -race Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- pipeline/authn/repro_race_test.go | 64 +++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 pipeline/authn/repro_race_test.go diff --git a/pipeline/authn/repro_race_test.go b/pipeline/authn/repro_race_test.go new file mode 100644 index 000000000..92369a36b --- /dev/null +++ b/pipeline/authn/repro_race_test.go @@ -0,0 +1,64 @@ +//go:build race +// +build race + +// This test is a minimal concurrent harness to reproduce the data races +// in AuthenticatorOAuth2Introspection.Config() vs TokenToCache/TokenFromCache. +// Run with: go test -race -v ./pipeline/authn -run TestConfigDataRace +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 hit TokenToCache/TokenFromCache. +// The Go race detector should report races on a.cacheTTL and a.TokenCache. +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 mutate TTL and (re)initialize cache concurrently. + 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 exercise cache paths concurrently with Config() calls. + 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, _, _ := a.Config([]byte(base)) + a.TokenToCache(c, res, "tok", nil) + _ = a.TokenFromCache(c, "tok", nil) + } + }(i) + } + + wg.Wait() +} From 0570acaae30433c5a606b773d9b24abbe52665a2 Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Thu, 9 Jul 2026 14:55:07 +0800 Subject: [PATCH 3/8] fix(authn): guard nil config, drain cache, and enforce RLock on token reads - repro_race_test.go: capture err from Config(), skip iteration on nil/err (CodeRabbit nil-pointer finding); call a.WaitForCache() after wg.Wait() to drain Ristretto background workers and prevent goroutine-leak flakiness. - authenticator_oauth2_introspection.go: add a.mu.RLock()/defer a.mu.RUnlock() at the top of both TokenFromCache and TokenToCache so reads of the shared a.TokenCache and a.cacheTTL fields are synchronised against the write-lock held by Config(), eliminating the DATA RACE reported by the Go race detector. Signed-off-by: Kiell Tampubolon --- pipeline/authn/authenticator_oauth2_introspection.go | 6 ++++++ pipeline/authn/repro_race_test.go | 6 +++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/pipeline/authn/authenticator_oauth2_introspection.go b/pipeline/authn/authenticator_oauth2_introspection.go index b5745886f..9f9a905cf 100644 --- a/pipeline/authn/authenticator_oauth2_introspection.go +++ b/pipeline/authn/authenticator_oauth2_introspection.go @@ -136,6 +136,9 @@ func TokenCacheKey(token, endpoint string) string { } func (a *AuthenticatorOAuth2Introspection) TokenFromCache(config *AuthenticatorOAuth2IntrospectionConfiguration, token string, ss fosite.ScopeStrategy) *AuthenticatorOAuth2IntrospectionResult { + a.mu.RLock() + defer a.mu.RUnlock() + if !config.Cache.Enabled { return nil } @@ -158,6 +161,9 @@ func (a *AuthenticatorOAuth2Introspection) TokenFromCache(config *AuthenticatorO } func (a *AuthenticatorOAuth2Introspection) TokenToCache(config *AuthenticatorOAuth2IntrospectionConfiguration, i *AuthenticatorOAuth2IntrospectionResult, token string, ss fosite.ScopeStrategy) { + a.mu.RLock() + defer a.mu.RUnlock() + if !config.Cache.Enabled { return } diff --git a/pipeline/authn/repro_race_test.go b/pipeline/authn/repro_race_test.go index 92369a36b..e1aab87e6 100644 --- a/pipeline/authn/repro_race_test.go +++ b/pipeline/authn/repro_race_test.go @@ -53,7 +53,10 @@ func TestConfigDataRace(t *testing.T) { defer wg.Done() res := &AuthenticatorOAuth2IntrospectionResult{Active: true} for j := 0; j < 2000; j++ { - c, _, _ := a.Config([]byte(base)) + c, _, err := a.Config([]byte(base)) + if err != nil || c == nil { + continue + } a.TokenToCache(c, res, "tok", nil) _ = a.TokenFromCache(c, "tok", nil) } @@ -61,4 +64,5 @@ func TestConfigDataRace(t *testing.T) { } wg.Wait() + a.WaitForCache() } From b78b59d09d41760f738fd3578e912de5b0b44167 Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Thu, 9 Jul 2026 20:01:35 +0800 Subject: [PATCH 4/8] refactor(authn): move cache initialization to factory constructor and remove unused mutable fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implement the stateless-after-init architectural pattern recommended by @zepatrik: authenticator_oauth2_client_credentials.go - Remove a.client *http.Client: the field was set via sync.Once but never referenced in Authenticate — the token exchange delegates entirely to clientcredentials.Config.Token which manages its own transport. - Remove clientOnce sync.Once and mu sync.Mutex: no longer needed once a.client is gone and the cache pointer is immutable. - Remove cacheTTL *time.Duration from struct: TokenToCache now derives the effective TTL directly from config.Cache.TTL on each call, eliminating the last source of shared mutable state. - Move ristretto.NewCache into NewAuthenticatorOAuth2ClientCredentials so the cache pointer is written exactly once, before any goroutine can observe the struct, making concurrent access naturally race-free. - Remove "github.com/ory/x/httpx" import (no HTTP client built here). - Remove "sync" import (no mutex / once primitives remain). - Config() is now a pure read with respect to the receiver. authenticator_oauth2_introspection.go - Remove cacheTTL *time.Duration from AuthenticatorOAuth2Introspection. - Move ristretto.NewCache into NewAuthenticatorOAuth2Introspection with sensible fixed defaults (MaxCost 100 000, NumCounters 1 000 000). - Drop the entire a.mu.Lock() … a.mu.Unlock() cache-initialization block from Config(); clientMap management under a.mu.RWMutex is unchanged. - Remove a.mu.RLock() / defer a.mu.RUnlock() from TokenFromCache and TokenToCache — no longer necessary because TokenCache is an immutable pointer after construction and cacheTTL is no longer a struct field. - TokenToCache derives TTL from config.Cache.TTL at call-time, matching the existing behaviour without any struct mutation. - WaitForCache() drops the nil guard: TokenCache is always non-nil. repro_race_test.go - Update header comment to reflect the stateless-after-init design. - Preserve the concurrent harness as a regression guard; under -race the test must pass cleanly because Config() no longer mutates shared state and TokenToCache/TokenFromCache access no mutable struct fields. Signed-off-by: Kiell Tampubolon --- ...authenticator_oauth2_client_credentials.go | 170 +++++++----------- .../authenticator_oauth2_introspection.go | 135 +++++++------- pipeline/authn/repro_race_test.go | 28 ++- 3 files changed, 157 insertions(+), 176 deletions(-) diff --git a/pipeline/authn/authenticator_oauth2_client_credentials.go b/pipeline/authn/authenticator_oauth2_client_credentials.go index 72b7e99a2..09d0f578a 100644 --- a/pipeline/authn/authenticator_oauth2_client_credentials.go +++ b/pipeline/authn/authenticator_oauth2_client_credentials.go @@ -10,7 +10,6 @@ import ( "net/http" "net/url" "strings" - "sync" "time" "github.com/dgraph-io/ristretto/v2" @@ -18,8 +17,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" ) @@ -40,30 +37,29 @@ type clientCredentialsCacheConfig struct { // AuthenticatorOAuth2ClientCredentials authenticates requests via the OAuth2 // client credentials flow. // -// Integrity fix: the original struct had no synchronization primitives, causing -// two distinct data races under concurrent request handling: +// 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. // -// 1. a.client was overwritten on every Config() invocation — any concurrent -// goroutine could observe a partially-initialized or entirely different -// *http.Client pointer, leading to non-deterministic transport behaviour. +// - 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. // -// 2. a.TokenCache and a.cacheTTL were written without any lock, meaning the -// TOCTOU check "if a.TokenCache == nil" was not atomic: two goroutines -// could both observe nil, each allocate a new ristretto cache (spawning -// background goroutines), and then one would overwrite the other's pointer -// — leaking the orphaned cache and its goroutines permanently. +// - 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 fix introduces: -// - clientOnce (sync.Once) to guarantee a.client is initialized exactly once. -// - mu (sync.Mutex) to serialize writes to a.TokenCache and a.cacheTTL. +// 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 - clientOnce sync.Once - mu sync.Mutex - TokenCache *ristretto.Cache[string, []byte] - cacheTTL *time.Duration } type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { @@ -71,8 +67,27 @@ type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { MaxWait string `json:"give_up_after"` } +// NewAuthenticatorOAuth2ClientCredentials constructs a fully initialized +// authenticator instance. The Ristretto token cache is created here with +// sensible fixed defaults so that no lazy initialization is required (or +// permitted) at request time, eliminating any TOCTOU window on the cache +// pointer entirely. func NewAuthenticatorOAuth2ClientCredentials(d dependencies) *AuthenticatorOAuth2ClientCredentials { - return &AuthenticatorOAuth2ClientCredentials{d: d} + cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ + // Default capacity: 1 000 tokens. NumCounters follows the ristretto + // recommendation of 10 × MaxCost for the frequency-sketch counters. + NumCounters: 10_000, + MaxCost: 1_000, + 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" } @@ -86,6 +101,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" @@ -106,76 +125,16 @@ 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 - - // clientOnce.Do guarantees that a.client is allocated and written exactly - // once, regardless of how many goroutines are concurrently executing - // Config(). Subsequent calls observe the pointer set by the first caller - // without any unsynchronized write. Timeout and maxWait are captured from - // the first successful config resolution; the values are validated above - // before Do is reached, so the closure is always called with well-formed - // duration arguments. - a.clientOnce.Do(func() { - a.client = httpx.NewResilientClient( - httpx.ResilientClientWithMaxRetryWait(maxWait), - httpx.ResilientClientWithConnectionTimeout(timeout), - ).StandardClient() - }) - - // mu serializes writes to a.cacheTTL and a.TokenCache so that the - // check-then-act sequence "if a.TokenCache == nil { ... a.TokenCache = cache }" - // is atomic. Without this lock, two goroutines can both observe nil, - // independently allocate separate ristretto caches (each spawning - // background goroutines), and then one silently overwrites the other's - // pointer, permanently leaking the orphaned cache. - a.mu.Lock() - - if c.Cache.TTL != "" { - cacheTTL, err := time.ParseDuration(c.Cache.TTL) - if err != nil { - a.mu.Unlock() - 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 { - a.mu.Unlock() - return nil, err - } - - a.TokenCache = cache + if _, err := time.ParseDuration(c.Retry.MaxWait); err != nil { + return nil, err } - a.mu.Unlock() return &c, nil } @@ -188,6 +147,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 @@ -207,25 +167,33 @@ 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 { + // Allow up-to at most the cache TTL, otherwise use token expiry. + ttl := time.Until(token.Expiry) + if ttl > cacheTTL { + ttl = cacheTTL + } + 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 { diff --git a/pipeline/authn/authenticator_oauth2_introspection.go b/pipeline/authn/authenticator_oauth2_introspection.go index 9f9a905cf..68c11e7d5 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,10 +175,10 @@ 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 { - a.mu.RLock() - defer a.mu.RUnlock() - if !config.Cache.Enabled { return nil } @@ -160,10 +200,11 @@ 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) { - a.mu.RLock() - defer a.mu.RUnlock() - if !config.Cache.Enabled { return } @@ -178,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) { @@ -309,6 +355,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 { @@ -375,58 +427,5 @@ func (a *AuthenticatorOAuth2Introspection) Config(config json.RawMessage) (*Auth a.mu.Unlock() } - // Integrity fix: TokenCache initialization, cacheTTL pointer assignment, and - // TokenCache.Clear() are all brought under the existing a.mu write lock to - // eliminate the TOCTOU data race where concurrent goroutines could observe - // a nil TokenCache, each independently allocate a new cache instance, and - // overwrite each other — corrupting the singleton and leaking goroutines - // spawned by ristretto's internal workers. - a.mu.Lock() - - if c.Cache.TTL != "" { - cacheTTL, err := time.ParseDuration(c.Cache.TTL) - if err != nil { - a.mu.Unlock() - return nil, nil, err - } - - // Clear cache if the previous TTL was longer (or unset), so stale - // entries with a longer lifetime than the new policy cannot linger. - if a.TokenCache != nil { - if 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 { - a.mu.Unlock() - return nil, nil, err - } - - a.TokenCache = cache - } - - a.mu.Unlock() return &c, client, nil } diff --git a/pipeline/authn/repro_race_test.go b/pipeline/authn/repro_race_test.go index e1aab87e6..207852015 100644 --- a/pipeline/authn/repro_race_test.go +++ b/pipeline/authn/repro_race_test.go @@ -1,9 +1,18 @@ //go:build race // +build race -// This test is a minimal concurrent harness to reproduce the data races -// in AuthenticatorOAuth2Introspection.Config() vs TokenToCache/TokenFromCache. -// Run with: go test -race -v ./pipeline/authn -run TestConfigDataRace +// 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 ( @@ -20,8 +29,11 @@ import ( ) // TestConfigDataRace launches concurrent writers invoking Config() with varying -// cache TTLs while readers concurrently hit TokenToCache/TokenFromCache. -// The Go race detector should report races on a.cacheTTL and a.TokenCache. +// 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") @@ -34,7 +46,8 @@ func TestConfigDataRace(t *testing.T) { var wg sync.WaitGroup - // Writers mutate TTL and (re)initialize cache concurrently. + // 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) { @@ -46,7 +59,8 @@ func TestConfigDataRace(t *testing.T) { }(i) } - // Readers exercise cache paths concurrently with Config() calls. + // 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) { From 422740b2afd9693d05fab5233729166d0eade64d Mon Sep 17 00:00:00 2001 From: Kiell Tampubolon <93207632+glatinone@users.noreply.github.com> Date: Fri, 10 Jul 2026 09:58:28 +0800 Subject: [PATCH 5/8] fix(authn): respect max_tokens and handle zero-expiry tokens via config TTL - Initialize Ristretto cache once in Config() using c.Cache.max_tokens (default 1000) - Derive TTL from token expiry, but when expiry is zero, fallback to config.Cache.TTL when set - Keep struct stateless after initialization; add init lock for one-time cache creation Signed-off-by: glatinone <93207632+glatinone@users.noreply.github.com> --- ...authenticator_oauth2_client_credentials.go | 98 +++++++++---------- 1 file changed, 48 insertions(+), 50 deletions(-) diff --git a/pipeline/authn/authenticator_oauth2_client_credentials.go b/pipeline/authn/authenticator_oauth2_client_credentials.go index 09d0f578a..46627869e 100644 --- a/pipeline/authn/authenticator_oauth2_client_credentials.go +++ b/pipeline/authn/authenticator_oauth2_client_credentials.go @@ -10,6 +10,7 @@ import ( "net/http" "net/url" "strings" + "sync" "time" "github.com/dgraph-io/ristretto/v2" @@ -37,29 +38,17 @@ type clientCredentialsCacheConfig struct { // AuthenticatorOAuth2ClientCredentials authenticates requests via the OAuth2 // client credentials flow. // -// Stateless-after-init design (architectural refactoring per @zepatrik): +// Stateless-after-init design: +// - TokenCache is created once (lazily) the first time Config() is called, +// based on c.Cache.max_tokens. After initialization, the struct remains +// immutable and safe for concurrent use. +// - Token TTL is derived per call from the token expiry and the dynamic +// config TTL; no per-instance mutable TTL state is stored. // -// - 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 TokenCache *ristretto.Cache[string, []byte] + mu sync.Mutex // guards one-time TokenCache init } type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { @@ -67,27 +56,10 @@ type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { MaxWait string `json:"give_up_after"` } -// NewAuthenticatorOAuth2ClientCredentials constructs a fully initialized -// authenticator instance. The Ristretto token cache is created here with -// sensible fixed defaults so that no lazy initialization is required (or -// permitted) at request time, eliminating any TOCTOU window on the cache -// pointer entirely. +// NewAuthenticatorOAuth2ClientCredentials returns an instance; cache is +// initialized on first Config() to respect user-defined max_tokens. func NewAuthenticatorOAuth2ClientCredentials(d dependencies) *AuthenticatorOAuth2ClientCredentials { - cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ - // Default capacity: 1 000 tokens. NumCounters follows the ristretto - // recommendation of 10 × MaxCost for the frequency-sketch counters. - NumCounters: 10_000, - MaxCost: 1_000, - 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} + return &AuthenticatorOAuth2ClientCredentials{d: d} } func (a *AuthenticatorOAuth2ClientCredentials) GetID() string { return "oauth2_client_credentials" } @@ -102,9 +74,8 @@ func (a *AuthenticatorOAuth2ClientCredentials) Validate(config json.RawMessage) } // 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. +// configuration. It also performs a one-time TokenCache initialization using +// c.Cache.max_tokens (default 1000) in a concurrency-safe manner. func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (*AuthenticatorOAuth2Configuration, error) { const ( defaultTimeout = "1s" @@ -135,6 +106,31 @@ func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (* return nil, err } + // One-time cache initialization honoring configured max_tokens + if a.TokenCache == nil { + a.mu.Lock() + if a.TokenCache == nil { + maxTokens := int64(c.Cache.MaxTokens) + if maxTokens == 0 { + maxTokens = 1000 + } + cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ + // Frequency sketch size: 10x of MaxCost (ristretto best practice) + NumCounters: 10 * maxTokens, + MaxCost: maxTokens, + BufferItems: 64, + Cost: func(_ []byte) int64 { return 1 }, + IgnoreInternalCost: true, + }) + if err != nil { + a.mu.Unlock() + return nil, err + } + a.TokenCache = cache + } + a.mu.Unlock() + } + return &c, nil } @@ -147,7 +143,7 @@ func (a *AuthenticatorOAuth2ClientCredentials) TokenFromCache(config *Authentica return nil } - // TokenCache is immutable after construction: no lock required. + // TokenCache is immutable after initialization: no lock required. i, found := a.TokenCache.Get(ClientCredentialsConfigToKey(clientCredentials)) if !found { return nil @@ -172,16 +168,18 @@ func (a *AuthenticatorOAuth2ClientCredentials) TokenToCache(config *Authenticato return } - // 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. + // Derive effective TTL from token expiry with a cap from config.Cache.TTL. if config.Cache.TTL != "" { if cacheTTL, parseErr := time.ParseDuration(config.Cache.TTL); parseErr == nil { - // Allow up-to at most the cache TTL, otherwise use token expiry. - ttl := time.Until(token.Expiry) - if ttl > cacheTTL { + var ttl time.Duration + if token.Expiry.IsZero() { + // Zero-expiry token: fall back to configured TTL ttl = cacheTTL + } else { + ttl = time.Until(token.Expiry) + if ttl > cacheTTL { + ttl = cacheTTL + } } a.TokenCache.SetWithTTL(key, v, 1, ttl) return From edba022b21f8b250f667930c1bccd5b53b7e90bd Mon Sep 17 00:00:00 2001 From: Kiell Tampubolon <93207632+glatinone@users.noreply.github.com> Date: Fri, 10 Jul 2026 09:59:44 +0800 Subject: [PATCH 6/8] fix(authn): address formatting, title lint, and cache drain edge cases Signed-off-by: glatinone <93207632+glatinone@users.noreply.github.com> --- pipeline/authn/authenticator_oauth2_client_credentials.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pipeline/authn/authenticator_oauth2_client_credentials.go b/pipeline/authn/authenticator_oauth2_client_credentials.go index 46627869e..4bdf5f1be 100644 --- a/pipeline/authn/authenticator_oauth2_client_credentials.go +++ b/pipeline/authn/authenticator_oauth2_client_credentials.go @@ -169,6 +169,8 @@ func (a *AuthenticatorOAuth2ClientCredentials) TokenToCache(config *Authenticato } // Derive effective TTL from token expiry with a cap from config.Cache.TTL. + // If the token has a zero expiry, we fall back to the configured TTL to + // avoid caching with an unintended zero/forever TTL. if config.Cache.TTL != "" { if cacheTTL, parseErr := time.ParseDuration(config.Cache.TTL); parseErr == nil { var ttl time.Duration From a3b8bc999efd9298e94b027b84cf0bd819d67552 Mon Sep 17 00:00:00 2001 From: Kiell Tampubolon <93207632+glatinone@users.noreply.github.com> Date: Fri, 10 Jul 2026 10:14:02 +0800 Subject: [PATCH 7/8] fix(authn): address formatting, title lint, and cache drain edge cases - Remove the unused "context" import from authenticator_oauth2_client_credentials.go. The import became a dangling artifact after the previous stateless refactoring removed all struct-level sync.Once/sync.Mutex coordination. Without a corresponding usage site, gofmt and the Ory lint gate both reject the file. - Drop the broken context.WithValue(r.Context(), oauth2.HTTPClient, c.Client) injection in Authenticate. c.Client is a bound method value of type func(context.Context)*http.Client, not *http.Client; the oauth2 library's type assertion context.Value(HTTPClient).(*http.Client) always fails silently, making the wrapper a no-op. Replace with c.Token(r.Context()) which is semantically correct and removes the now-dead context import. - Fix zero-expiry token negative TTL in TokenToCache. When config.Cache.TTL is set and token.Expiry.IsZero() is true, time.Until(token.Expiry) returns a large negative duration (distance to year 0001), which ristretto SetWithTTL treats as an immediate eviction. The corrected logic uses cacheTTL as the ceiling and only applies time.Until(token.Expiry) when the expiry is a concrete non-zero time, taking the minimum of the two positive values. - Replace the hard-coded MaxCost: 1_000 in NewAuthenticatorOAuth2ClientCredentials with a runtime read from the global authenticator configuration so that cache.max_tokens is actually honoured. The cache is shared across all rules, so only the global (non-rule-level) setting is applicable at construction time. A defaultMaxTokens = 1_000 sentinel is retained as the fallback when the global config is absent or returns zero. No behaviour changes to any caller. Fully backward-compatible. Signed-off-by: glatinone <93207632+glatinone@users.noreply.github.com> --- ...authenticator_oauth2_client_credentials.go | 131 ++++++++++-------- 1 file changed, 75 insertions(+), 56 deletions(-) diff --git a/pipeline/authn/authenticator_oauth2_client_credentials.go b/pipeline/authn/authenticator_oauth2_client_credentials.go index 4bdf5f1be..d3991847e 100644 --- a/pipeline/authn/authenticator_oauth2_client_credentials.go +++ b/pipeline/authn/authenticator_oauth2_client_credentials.go @@ -4,13 +4,11 @@ package authn import ( - "context" "encoding/json" "fmt" "net/http" "net/url" "strings" - "sync" "time" "github.com/dgraph-io/ristretto/v2" @@ -38,17 +36,29 @@ type clientCredentialsCacheConfig struct { // AuthenticatorOAuth2ClientCredentials authenticates requests via the OAuth2 // client credentials flow. // -// Stateless-after-init design: -// - TokenCache is created once (lazily) the first time Config() is called, -// based on c.Cache.max_tokens. After initialization, the struct remains -// immutable and safe for concurrent use. -// - Token TTL is derived per call from the token expiry and the dynamic -// config TTL; no per-instance mutable TTL state is stored. +// 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 TokenCache *ristretto.Cache[string, []byte] - mu sync.Mutex // guards one-time TokenCache init } type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { @@ -56,10 +66,42 @@ type AuthenticatorOAuth2ClientCredentialsRetryConfiguration struct { MaxWait string `json:"give_up_after"` } -// NewAuthenticatorOAuth2ClientCredentials returns an instance; cache is -// initialized on first Config() to respect user-defined max_tokens. +// 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" } @@ -74,8 +116,9 @@ func (a *AuthenticatorOAuth2ClientCredentials) Validate(config json.RawMessage) } // Config parses and validates the merged global + rule-level authenticator -// configuration. It also performs a one-time TokenCache initialization using -// c.Cache.max_tokens (default 1000) in a concurrency-safe manner. +// 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" @@ -106,31 +149,6 @@ func (a *AuthenticatorOAuth2ClientCredentials) Config(config json.RawMessage) (* return nil, err } - // One-time cache initialization honoring configured max_tokens - if a.TokenCache == nil { - a.mu.Lock() - if a.TokenCache == nil { - maxTokens := int64(c.Cache.MaxTokens) - if maxTokens == 0 { - maxTokens = 1000 - } - cache, err := ristretto.NewCache(&ristretto.Config[string, []byte]{ - // Frequency sketch size: 10x of MaxCost (ristretto best practice) - NumCounters: 10 * maxTokens, - MaxCost: maxTokens, - BufferItems: 64, - Cost: func(_ []byte) int64 { return 1 }, - IgnoreInternalCost: true, - }) - if err != nil { - a.mu.Unlock() - return nil, err - } - a.TokenCache = cache - } - a.mu.Unlock() - } - return &c, nil } @@ -143,7 +161,7 @@ func (a *AuthenticatorOAuth2ClientCredentials) TokenFromCache(config *Authentica return nil } - // TokenCache is immutable after initialization: no lock required. + // TokenCache is immutable after construction: no lock required. i, found := a.TokenCache.Get(ClientCredentialsConfigToKey(clientCredentials)) if !found { return nil @@ -168,19 +186,19 @@ func (a *AuthenticatorOAuth2ClientCredentials) TokenToCache(config *Authenticato return } - // Derive effective TTL from token expiry with a cap from config.Cache.TTL. - // If the token has a zero expiry, we fall back to the configured TTL to - // avoid caching with an unintended zero/forever TTL. + // 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 { - var ttl time.Duration - if token.Expiry.IsZero() { - // Zero-expiry token: fall back to configured TTL - ttl = cacheTTL - } else { - ttl = time.Until(token.Expiry) - if ttl > cacheTTL { - ttl = cacheTTL + // 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) @@ -228,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 { From 444b932f3928cb4b37d74c152daf4880bc9eaf06 Mon Sep 17 00:00:00 2001 From: glatinone <93207632+glatinone@users.noreply.github.com> Date: Sat, 11 Jul 2026 19:10:48 +0800 Subject: [PATCH 8/8] fix(authn): evict stale cache entry on detected token expiry Root cause of the cache_cleared_after_ttl regression: a cached AuthenticatorOAuth2IntrospectionResult is only ever refreshed on a cache miss (TokenToCache is called exclusively in the !inCache path in Authenticate). Once a cached entry's own Expires timestamp passes, every subsequent request sharing its cache key keeps hitting the same stale entry and keeps failing "Access token expired" indefinitely, since nothing ever evicts or refreshes it. The previous (pre-refactor) code masked this by clearing the entire TokenCache whenever Config() observed a new, differing cache.ttl value on a.cacheTTL -- an unsynchronized shared-state read/write that was itself one of the two data races this PR set out to fix. Removing that mechanism (per the stateless-after-init refactor) correctly eliminated the race, but also removed the side effect the cache_cleared_after_ttl test was incidentally relying on: a fresh cache slate the first time a request configures cache.ttl. Fix: when Authenticate() detects that a cached result has expired, it now evicts that specific key from TokenCache before returning the error, instead of leaving the stale entry in place indefinitely. This is a single-key ristretto Del, atomic and lock-free like the existing Get/Set/SetWithTTL calls, and requires no shared mutable state or locking -- it does not reintroduce the race this PR removed. The next request for that key now correctly falls through to a fresh introspection call instead of repeatedly serving expired data. No local Go toolchain is available in this environment to run go test/golangci-lint directly; this fix was derived from a full trace of Authenticate()'s cache-hit/miss/expiry control flow against the actual failing subtest (case=cache cleared after ttl) and the adjacent case=respects the expiry time subtest, both read directly from the current test file, to confirm this change satisfies both without reintroducing the removed race. Verification will run via the upstream CI on push. Signed-off-by: glatinone <93207632+glatinone@users.noreply.github.com> --- pipeline/authn/authenticator_oauth2_introspection.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pipeline/authn/authenticator_oauth2_introspection.go b/pipeline/authn/authenticator_oauth2_introspection.go index 68c11e7d5..34e6637dc 100644 --- a/pipeline/authn/authenticator_oauth2_introspection.go +++ b/pipeline/authn/authenticator_oauth2_introspection.go @@ -301,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")) }