Skip to content

refactor(authn): make oauth2 authenticators stateless after initialization #1282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
glatinone wants to merge 9 commits into from
Closed

refactor(authn): make oauth2 authenticators stateless after initialization #1282

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
898c987
fix(authn): resolve concurrent shared mutable state data races in oau…
glatinone Jul 8, 2026
6349c84
test(authn): add concurrent Config() race reproduction harness for -race
glatinone Jul 8, 2026
0570aca
fix(authn): guard nil config, drain cache, and enforce RLock on token…
glatinone Jul 9, 2026
b78b59d
refactor(authn): move cache initialization to factory constructor and…
glatinone Jul 9, 2026
422740b
fix(authn): respect max_tokens and handle zero-expiry tokens via conf…
glatinone Jul 10, 2026
edba022
fix(authn): address formatting, title lint, and cache drain edge cases
glatinone Jul 10, 2026
a3b8bc9
fix(authn): address formatting, title lint, and cache drain edge cases
glatinone Jul 10, 2026
444b932
fix(authn): evict stale cache entry on detected token expiry
glatinone Jul 11, 2026
54ae8e4
chore: fix goimports grouping in cors.go and repro_race_test.go
glatinone Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 76 additions & 65 deletions pipeline/authn/authenticator_oauth2_client_credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,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"
)
Expand All @@ -36,21 +34,60 @@ 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 {
Timeout string `json:"max_delay"`
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}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

func (a *AuthenticatorOAuth2ClientCredentials) GetID() string { return "oauth2_client_credentials" }
Expand All @@ -64,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"
Expand All @@ -84,53 +125,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
Expand All @@ -145,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
Expand All @@ -164,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
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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 {
Expand Down
117 changes: 67 additions & 50 deletions pipeline/authn/authenticator_oauth2_introspection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,26 +64,66 @@ 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

clientMap map[string]*http.Client
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,
}
Comment on lines 99 to +118

@coderabbitai coderabbitai Bot Jul 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm max_cost was previously used to size the cache and is now unused
rg -nP -C3 'MaxCost|max_cost' pipeline/authn/authenticator_oauth2_introspection.go
git log -n 1 --format='%H' 2>/dev/null

Repository: ory/oathkeeper

Length of output: 1059

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the rest of the authenticator implementation and nearby tests/wiring.
sed -n '1,260p' pipeline/authn/authenticator_oauth2_introspection.go

printf '\n--- SEARCH max_cost / MaxCost USAGE ---\n'
rg -n --hidden --glob '!**/vendor/**' --glob '!**/node_modules/**' 'max_cost|MaxCost' .

printf '\n--- TEST FILES FOR THIS AUTHENTICATOR ---\n'
rg -n --hidden --glob '*test.go' 'OAuth2Introspection|max_cost|MaxCost' pipeline/authn

Repository: ory/oathkeeper

Length of output: 17538

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the config/init path in this file and any documentation for cache.max_cost.
ast-grep outline pipeline/authn/authenticator_oauth2_introspection.go --view expanded
printf '\n--- CONFIG/MAXCOST REFERENCES IN THIS FILE ---\n'
rg -n -C4 'func \(a \*AuthenticatorOAuth2Introspection\) Config|MaxCost|max_cost|TokenCache|Cache' pipeline/authn/authenticator_oauth2_introspection.go

printf '\n--- SCHEMA/DOC REFERENCES ---\n'
rg -n -C2 'max_cost|cache.*MaxCost|MaxCost.*cache' .schema/config.schema.json spec/config.schema.json README.md docs pipeline/authn

Repository: ory/oathkeeper

Length of output: 13415

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the Config() implementation to confirm whether cache sizing still uses c.Cache.MaxCost.
sed -n '364,460p' pipeline/authn/authenticator_oauth2_introspection.go

Repository: ory/oathkeeper

Length of output: 2180

cache.max_cost is ignored in the introspection authenticator. Config() still parses the field, but the cache is always created with a hard-coded 100_000 cost, so any per-rule value is silently dropped. Thread the configured cost into the constructor or remove/deprecate the field and update the schema/docs.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pipeline/authn/authenticator_oauth2_introspection.go` around lines 99 - 118,
`NewAuthenticatorOAuth2Introspection` ignores the configured `cache.max_cost`
and always uses a hard-coded `100_000` when building the Ristretto cache. Update
the constructor path so the parsed max cost from `Config()` is passed into
`NewAuthenticatorOAuth2Introspection` and used for `defaultMaxCost`, or else
remove/deprecate the field consistently. Make sure the
`AuthenticatorOAuth2Introspection` cache initialization and any related
schema/docs reflect the chosen behavior.

}

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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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) {
Expand Down Expand Up @@ -303,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 {
Expand Down Expand Up @@ -369,46 +427,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
}
Loading
Loading