Skip to content

Conversation

@cemalkilic
Copy link
Contributor

Problem

I noticed we were making redundant HTTP calls to OIDC discovery endpoints (.well-known/openid-configuration) on every single OAuth/OIDC request:

  • Every /authorize request -> HTTP call to provider's discovery endpoint
  • Every /callback request -> Another HTTP call to the same endpoint
  • Every ID token validation with /token?grant_type=idtoken -> Yet another HTTP call

Impact: 10-500ms added latency per OAuth flow, 2+ unnecessary external HTTP requests per login. At scale this is huge amount of unnecessary traffic.

Solution

Implemented thread-safe caching of oidc.Provider instances with smart defaults:

// Before: Creates new provider on EVERY request
oidcProvider, err := oidc.NewProvider(ctx, issuer)

// After: Uses cached provider (1 hour TTL)
oidcProvider, err := defaultOIDCProviderCache.Get(ctx, issuer)

Also added a new optional environment variable:

GOTRUE_EXTERNAL_OIDC_PROVIDER_CACHE_TTL=1h  # Default
GOTRUE_EXTERNAL_OIDC_PROVIDER_CACHE_TTL=30m # Custom TTL

Default: 1 hour (reasonable since OIDC discovery configs rarely change)

Notes

Safe for Cross-Account Use

IMPORTANT: The cache stores ONLY discovery metadata (endpoints, supported algorithms, etc), NOT user tokens or session data.

  • Cache key: issuer URL (e.g., https://accounts.google.com)
  • Cache value: oidc.Provider instance (stateless discovery metadata)
  • User tokens, access tokens, refresh tokens: NEVER cached

JWKS Key Rotation Still Works

  • go-oidc library internally handles JWKS key fetching and rotation
  • Cached providers automatically fetch fresh keys when needed
  • Key rotation happens independently of discovery document caching
  • No risk of stale keys causing verification failures

Azure Multi-Tenant Support

  • Each Azure tenant gets its own cache entry
  • Cache key includes tenant ID in issuer URL
  • Multi-tenant configurations work as before

@cemalkilic cemalkilic requested a review from a team as a code owner December 30, 2025 16:49

// Create provider with background context to ensure it's not tied to request lifecycle
// Use a background context with a deadline if the original context has one
bgCtx := context.Background()

Choose a reason for hiding this comment

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

🟡 Severity: MEDIUM

Critical context loss vulnerability: The cache replaces the original context with context.Background(), discarding security-critical context values like InsecureIssuerURLContext used for Apple OIDC. When Apple requests use oidc.InsecureIssuerURLContext(ctx, issuer) (token_oidc.go:139), this context setting is lost during cache miss, creating a provider with different security properties than intended. This could cause Apple authentication to fail or behave unexpectedly.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: Replace context.Background() with context.WithoutCancel(ctx) to preserve context values (like InsecureIssuerURLContext for Apple OIDC) while still detaching from the parent request's cancellation. This maintains the original design intent of decoupling the cached provider from request lifecycle while fixing the security context loss issue.

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change
bgCtx := context.Background()
bgCtx := context.WithoutCancel(ctx)

@coveralls
Copy link

Pull Request Test Coverage Report for Build 20601521323

Details

  • 82 of 87 (94.25%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.1%) to 68.887%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/provider/linkedin_oidc.go 0 1 0.0%
internal/api/provider/vercel_marketplace.go 0 1 0.0%
internal/api/provider/cache.go 75 78 96.15%
Totals Coverage Status
Change from base Build 20599680500: 0.1%
Covered Lines: 14821
Relevant Lines: 21515

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants