-
Notifications
You must be signed in to change notification settings - Fork 604
feat(oidc): cache OIDC provider instances to prevent redundant calls #2308
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.
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
Open
cemalkilic
wants to merge
2
commits into
master
Choose a base branch
from
cemal/feat-add-oidc-provider-cache
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| package provider | ||
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/coreos/go-oidc/v3/oidc" | ||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| const ( | ||
| // DefaultOIDCProviderCacheTTL is the default time-to-live for cached OIDC providers | ||
| // 1 hour is a reasonable default as OIDC discovery documents rarely change | ||
| DefaultOIDCProviderCacheTTL = 1 * time.Hour | ||
| ) | ||
|
|
||
| // OIDCProviderCache provides thread-safe caching of OIDC provider instances | ||
| // to avoid redundant HTTP calls to OIDC discovery endpoints (.well-known/openid-configuration) | ||
| type OIDCProviderCache struct { | ||
| mu sync.RWMutex | ||
| entries map[string]*oidcCacheEntry | ||
|
|
||
| // Default TTL for cache entries | ||
| defaultTTL time.Duration | ||
| } | ||
|
|
||
| type oidcCacheEntry struct { | ||
| provider *oidc.Provider | ||
| createdAt time.Time | ||
| expiresAt time.Time | ||
| } | ||
|
|
||
| // Global default OIDC provider cache instance | ||
| var defaultOIDCProviderCache = NewOIDCProviderCache(DefaultOIDCProviderCacheTTL) | ||
|
|
||
| // NewOIDCProviderCache creates a new OIDC provider cache instance with specified TTL | ||
| func NewOIDCProviderCache(ttl time.Duration) *OIDCProviderCache { | ||
| if ttl <= 0 { | ||
| ttl = DefaultOIDCProviderCacheTTL | ||
| } | ||
| return &OIDCProviderCache{ | ||
| entries: make(map[string]*oidcCacheEntry), | ||
| defaultTTL: ttl, | ||
| } | ||
| } | ||
|
|
||
| // SetDefaultOIDCProviderCacheTTL updates the default OIDC provider cache TTL | ||
| // This should be called during application initialization with the configured value | ||
| func SetDefaultOIDCProviderCacheTTL(ttl time.Duration) { | ||
| if ttl <= 0 { | ||
| ttl = DefaultOIDCProviderCacheTTL | ||
| } | ||
| defaultOIDCProviderCache.mu.Lock() | ||
| defer defaultOIDCProviderCache.mu.Unlock() | ||
| defaultOIDCProviderCache.defaultTTL = ttl | ||
| } | ||
|
|
||
| // Get returns a cached OIDC provider or creates a new one if not cached or expired. | ||
| // Uses the cache's default TTL (1 hour). | ||
| // This method is thread-safe and supports concurrent access. | ||
| // | ||
| // Parameters: | ||
| // - ctx: Context for the OIDC provider creation (only used if cache miss) | ||
| // - issuer: OIDC issuer URL (e.g., "https://accounts.google.com") | ||
| // | ||
| // Returns the cached or newly created provider instance | ||
| func (c *OIDCProviderCache) Get(ctx context.Context, issuer string) (*oidc.Provider, error) { | ||
| now := time.Now() | ||
|
|
||
| // Fast path: check if cached and not expired (read lock only) | ||
| c.mu.RLock() | ||
| entry, exists := c.entries[issuer] | ||
| c.mu.RUnlock() | ||
|
|
||
| if exists && now.Before(entry.expiresAt) { | ||
| logrus.WithFields(logrus.Fields{ | ||
| "issuer": issuer, | ||
| "created_at": entry.createdAt, | ||
| "expires_at": entry.expiresAt, | ||
| }).Debug("OIDC provider cache hit") | ||
| return entry.provider, nil | ||
| } | ||
|
|
||
| // Slow path: need to create new provider (write lock required) | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| // Double-check after acquiring write lock (another goroutine might have created it) | ||
| if entry, exists := c.entries[issuer]; exists && now.Before(entry.expiresAt) { | ||
| logrus.WithField("issuer", issuer).Debug("OIDC provider cache hit after lock acquisition") | ||
| return entry.provider, nil | ||
| } | ||
|
|
||
| // 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() | ||
| if deadline, ok := ctx.Deadline(); ok { | ||
| var cancel context.CancelFunc | ||
| bgCtx, cancel = context.WithDeadline(bgCtx, deadline) | ||
| defer cancel() | ||
| } | ||
|
|
||
| provider, err := oidc.NewProvider(bgCtx, issuer) | ||
| if err != nil { | ||
| logrus.WithError(err).WithField("issuer", issuer).Error("Failed to create OIDC provider") | ||
| return nil, err | ||
| } | ||
|
|
||
| expiresAt := now.Add(c.defaultTTL) | ||
| c.entries[issuer] = &oidcCacheEntry{ | ||
| provider: provider, | ||
| createdAt: now, | ||
| expiresAt: expiresAt, | ||
| } | ||
|
|
||
| return provider, nil | ||
| } | ||
|
|
||
| // Invalidate removes a specific OIDC provider from the cache | ||
| // Useful for manual cache invalidation or testing | ||
| func (c *OIDCProviderCache) Invalidate(issuer string) { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| delete(c.entries, issuer) | ||
| } | ||
|
|
||
| // Clear removes all entries from the cache | ||
| // Primarily used for testing | ||
| func (c *OIDCProviderCache) Clear() { | ||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
|
|
||
| c.entries = make(map[string]*oidcCacheEntry) | ||
| } | ||
|
|
||
| // Size returns the current number of cached OIDC providers | ||
| func (c *OIDCProviderCache) Size() int { | ||
| c.mu.RLock() | ||
| defer c.mu.RUnlock() | ||
| return len(c.entries) | ||
| } | ||
|
|
||
| // GetDefaultOIDCProviderCache returns the global default OIDC provider cache instance | ||
| func GetDefaultOIDCProviderCache() *OIDCProviderCache { | ||
| return defaultOIDCProviderCache | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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 likeInsecureIssuerURLContextused for Apple OIDC. When Apple requests useoidc.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.