fix(authn): resolve concurrent shared mutable state data races in oauth2 pipelines#1282
fix(authn): resolve concurrent shared mutable state data races in oauth2 pipelines#1282glatinone wants to merge 2 commits into
Conversation
|
|
📝 WalkthroughWalkthroughOAuth2 client-credentials and introspection authenticators now synchronize client and token cache initialization with ChangesOAuth2 authenticator concurrency fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
In case of data races, please include the full data race detector report 🙏 |
|
Hi @gaultier — Since the current isolated environment constraints prevent a live concurrent
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Hi @gaultier — I’ve added a concrete, executable race-repro harness to our PR branch to validate the concurrent writes this change fixes. How to run locally:
What it does:
For convenience, the full test source is embedded below: //go:build race
// +build race
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"
)
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
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)
}
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()
}This supersedes my earlier dry-run note; please feel free to hide it. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pipeline/authn/repro_race_test.go (1)
63-63: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueCall
a.WaitForCache()afterwg.Wait()to drain ristretto background goroutines.Without this, ristretto's internal goroutines may still be running when the test exits, potentially causing goroutine leak warnings or flaky behavior in test suites with
-race.♻️ Proposed fix
wg.Wait() + a.WaitForCache() }🤖 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/repro_race_test.go` at line 63, The test in repro_race_test.go finishes after wg.Wait() but does not drain Ristretto’s background work, which can leave goroutines running and make -race tests flaky. Update the repro race test to call a.WaitForCache() immediately after wg.Wait(), using the existing a cache instance in that test, so the cache is fully settled before the test exits.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@pipeline/authn/repro_race_test.go`:
- Around line 56-58: The race test ignores the error from Config and then passes
a potentially nil config into TokenToCache/TokenFromCache, which can panic when
c is nil. Update repro_race_test.go to check the result of a.Config before using
c, and either skip that iteration or guard the TokenToCache/TokenFromCache calls
when config creation fails. Use the Config, TokenToCache, and TokenFromCache
calls as the locations to adjust.
- Around line 50-61: `TokenToCache` and `TokenFromCache` still access shared
state unsafely, so they can race with `Config()` despite the new write lock.
Update both methods in `AuthenticatorOAuth2Introspection` to take
`a.mu.RLock()`/`a.mu.RUnlock()` around reads of `a.cacheTTL` and `a.TokenCache`,
matching the existing `a.mu.Lock()` protection in `Config()`. This will keep
cache initialization and TTL reads synchronized and prevent the
`repro_race_test.go` concurrency test from failing under `-race`.
---
Nitpick comments:
In `@pipeline/authn/repro_race_test.go`:
- Line 63: The test in repro_race_test.go finishes after wg.Wait() but does not
drain Ristretto’s background work, which can leave goroutines running and make
-race tests flaky. Update the repro race test to call a.WaitForCache()
immediately after wg.Wait(), using the existing a cache instance in that test,
so the cache is fully settled before the test exits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9ecb6b86-f8b3-4383-826f-5da3ab180a9c
📒 Files selected for processing (1)
pipeline/authn/repro_race_test.go
| 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) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Test would fail under -race — TokenToCache/TokenFromCache still race with Config().
The fix in Config() adds a.mu.Lock() around a.cacheTTL writes and a.TokenCache initialization, but TokenToCache and TokenFromCache read a.cacheTTL and a.TokenCache without any lock (see authenticator_oauth2_introspection.go:155-181). This test's reader goroutines call TokenToCache (which reads a.cacheTTL at line if a.cacheTTL != nil { ... *a.cacheTTL }) concurrently with writer goroutines whose Config() calls write a.cacheTTL under a.mu.Lock(). The race detector will flag this as a data race, causing the test to fail.
The fix is incomplete: TokenToCache and TokenFromCache need to acquire a.mu.RLock() / a.mu.RUnlock() to be properly synchronized with Config()'s write lock. Since mu is already an RWMutex, this is straightforward.
🔒 Proposed fix: add RLock to TokenToCache and TokenFromCache
In authenticator_oauth2_introspection.go:
func (a *AuthenticatorOAuth2Introspection) TokenFromCache(config *AuthenticatorOAuth2IntrospectionConfiguration, token string, ss fosite.ScopeStrategy) *AuthenticatorOAuth2IntrospectionResult {
if !config.Cache.Enabled {
return nil
}
if ss == nil && len(config.Scopes) > 0 {
return nil
}
+ a.mu.RLock()
+ defer a.mu.RUnlock()
+
key := TokenCacheKey(token, config.IntrospectionURL)
i, found := a.TokenCache.Get(key) func (a *AuthenticatorOAuth2Introspection) TokenToCache(config *AuthenticatorOAuth2IntrospectionConfiguration, i *AuthenticatorOAuth2IntrospectionResult, token string, ss fosite.ScopeStrategy) {
if !config.Cache.Enabled {
return
}
if ss == nil && len(config.Scopes) > 0 {
return
}
+ a.mu.RLock()
+ defer a.mu.RUnlock()
+
key := TokenCacheKey(token, config.IntrospectionURL)
v, err := json.Marshal(i)
if err != nil {
return
}
if a.cacheTTL != nil {
a.TokenCache.SetWithTTL(key, v, 1, *a.cacheTTL)
} else {
a.TokenCache.Set(key, v, 1)
}
}🤖 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/repro_race_test.go` around lines 50 - 61, `TokenToCache` and
`TokenFromCache` still access shared state unsafely, so they can race with
`Config()` despite the new write lock. Update both methods in
`AuthenticatorOAuth2Introspection` to take `a.mu.RLock()`/`a.mu.RUnlock()`
around reads of `a.cacheTTL` and `a.TokenCache`, matching the existing
`a.mu.Lock()` protection in `Config()`. This will keep cache initialization and
TTL reads synchronized and prevent the `repro_race_test.go` concurrency test
from failing under `-race`.
| c, _, _ := a.Config([]byte(base)) | ||
| a.TokenToCache(c, res, "tok", nil) | ||
| _ = a.TokenFromCache(c, "tok", nil) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Nil-pointer panic risk: Config() error ignored before TokenToCache/TokenFromCache.
c, _, _ := a.Config([]byte(base)) discards the error. If Config() fails (e.g., invalid config, registry misconfiguration), it returns nil for c. The subsequent a.TokenToCache(c, ...) dereferences c.Cache.Enabled, which panics on nil. Add a nil guard or skip the iteration on error.
🛡️ Proposed fix
- c, _, _ := a.Config([]byte(base))
- a.TokenToCache(c, res, "tok", nil)
- _ = a.TokenFromCache(c, "tok", nil)
+ c, _, err := a.Config([]byte(base))
+ if err != nil || c == nil {
+ continue
+ }
+ a.TokenToCache(c, res, "tok", nil)
+ _ = a.TokenFromCache(c, "tok", nil)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| c, _, _ := a.Config([]byte(base)) | |
| a.TokenToCache(c, res, "tok", nil) | |
| _ = a.TokenFromCache(c, "tok", nil) | |
| c, _, err := a.Config([]byte(base)) | |
| if err != nil || c == nil { | |
| continue | |
| } | |
| a.TokenToCache(c, res, "tok", nil) | |
| _ = a.TokenFromCache(c, "tok", nil) |
🤖 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/repro_race_test.go` around lines 56 - 58, The race test
ignores the error from Config and then passes a potentially nil config into
TokenToCache/TokenFromCache, which can panic when c is nil. Update
repro_race_test.go to check the result of a.Config before using c, and either
skip that iteration or guard the TokenToCache/TokenFromCache calls when config
creation fails. Use the Config, TokenToCache, and TokenFromCache calls as the
locations to adjust.
3 participants
P0/P1 Integrity Fix — Concurrent Data Races in OAuth2 Authentication Pipelines
Executive Summary
Two confirmed concurrent data races exist inside the OAuth2 authentication layer. Both races involve unsynchronized writes to shared mutable singleton state (
TokenCache,cacheTTL,client) across goroutines serving simultaneous requests. Under load, these produce non-deterministic token validation, silent cache corruption, and goroutine/FD leaks from orphaned ristretto background workers.Race 1 — P0:
authenticator_oauth2_introspection.go— TOCTOU onTokenCacheandcacheTTLDefect Location
Config()method — the final block managingTokenCacheinitialization andcacheTTLassignment.Root Cause
The struct carries a
sync.RWMutex(a.mu) that correctly protects theclientMap. However, the subsequentTokenCache/cacheTTLblock runs entirely outside any lock boundary:Failure Scenarios
a.TokenCache == niland independently construct a newristretto.Cachea.cacheTTL = &cacheTTL; Goroutine B readsa.cacheTTLinTokenToCache*a.cacheTTLdereferences stale or partial memorya.TokenCache.Clear()is called while another goroutine is mid-write viaTokenToCacheFix
Bring the entire TTL + cache-initialization block under
a.mu.Lock(), which already exists on the struct. Explicit unlock on each error return path; no deferred unlock to avoid lock-state confusion on the multiple return paths preceding this block.Additionally, the redundant double-nil guard
(a.cacheTTL != nil && a.cacheTTL.Seconds() > cacheTTL.Seconds())is simplified — the outera.cacheTTL == nilbranch already handles the nil case.Race 2 — P1:
authenticator_oauth2_client_credentials.go— Unsynchronized struct overwritesDefect Location
Config()method —a.clientassignment and theTokenCache/cacheTTLblock.Root Cause
The
AuthenticatorOAuth2ClientCredentialsstruct carried zero synchronization primitives.Config()is called on every authentication request, and every call unconditionally overwrote the shareda.clientpointer:Additionally,
TokenCacheinitialization exhibited the same TOCTOU pattern as Race 1:Failure Scenarios
Config()*http.Clientinstances are created and raced intoa.client; 99 are immediately abandoned with open connection poolsa.client(Transport.RoundTrip) when Goroutine B overwritesa.clienta.TokenCache == nilFix
Two primitives added to the struct:
sync.Oncefora.client: The duration values are validated beforeDois reached, so the closure always executes with well-formed arguments. Every concurrent call toConfig()that loses theDorace observes the pointer written by the winner — no allocation, no race, no abandoned transport pools.sync.MutexforTokenCache/cacheTTL: Identical lock pattern to the introspection fix — explicit unlock on each error path, serializing the nil check and the pointer write atomically.Impact Matrix
ory/oathkeeperpipeline/authn/authenticator_oauth2_introspection.go: Config()TokenCacheinit +cacheTTLwrite unguarded outside existinga.mulock boundary (TOCTOU)a.mu.Lock()before the cache block; explicit unlock on all error pathscacheTTLpointer dereference under concurrent loadory/oathkeeperpipeline/authn/authenticator_oauth2_client_credentials.go: Config()a.clientunconditionally overwritten on every request call with no synchronization;TokenCacheTOCTOUsync.Oncefor one-timea.clientinit;sync.MutexforTokenCache+cacheTTLwrites*http.Clientpointer race and dual-cache allocation under concurrent OAuth2 client-credentials requestsFiles Changed
pipeline/authn/authenticator_oauth2_introspection.gopipeline/authn/authenticator_oauth2_client_credentials.gosync.Once+sync.Mutex+ struct annotationNo behaviour changes to any caller. No new dependencies. Fully backward-compatible.
Summary by CodeRabbit