Fix P1: Resource leaks in Recovery Tokens, Rate Limiter, and OIDC Service

Fixed three P1 goroutine/memory leaks that prevent proper resource cleanup:

1. Recovery Tokens goroutine leak
   - Cleanup routine runs forever without stop mechanism
   - Added stopCleanup channel and Stop() method
   - Cleanup loop now uses select with stopCleanup case

2. Rate Limiter goroutine leak
   - Cleanup routine runs forever without stop mechanism
   - Added stopCleanup channel and Stop() method
   - Changed from 'for range ticker.C' to select with stopCleanup case

3. OIDC Service memory leak (DoS vector)
   - Abandoned OIDC flows never cleaned up
   - State entries accumulate unboundedly
   - Added cleanup routine with 5-minute ticker
   - Periodically removes expired state entries (10min TTL)
   - Added Stop() method for proper shutdown

All three follow consistent pattern:
- Add stopCleanup chan struct{} field
- Initialize in constructor
- Use select with ticker and stopCleanup cases
- Close channel in Stop() method to signal goroutine exit

Impact:
- Prevents goroutine leaks during service restarts/reloads
- Prevents memory exhaustion from abandoned OIDC login attempts
- Enables proper cleanup in tests and graceful shutdown
This commit is contained in:
rcourtman 2025-11-07 10:18:44 +00:00
parent 1bf9cfea88
commit e30757720a
3 changed files with 83 additions and 19 deletions

View file

@ -173,8 +173,9 @@ func (s *OIDCService) exchangeCode(ctx context.Context, code string, entry *oidc
// oidcStateStore keeps short-lived authorization state tokens. // oidcStateStore keeps short-lived authorization state tokens.
type oidcStateStore struct { type oidcStateStore struct {
mu sync.RWMutex mu sync.RWMutex
entries map[string]*oidcStateEntry entries map[string]*oidcStateEntry
stopCleanup chan struct{}
} }
type oidcStateEntry struct { type oidcStateEntry struct {
@ -186,7 +187,45 @@ type oidcStateEntry struct {
} }
func newOIDCStateStore() *oidcStateStore { func newOIDCStateStore() *oidcStateStore {
return &oidcStateStore{entries: make(map[string]*oidcStateEntry)} s := &oidcStateStore{
entries: make(map[string]*oidcStateEntry),
stopCleanup: make(chan struct{}),
}
// Start cleanup routine to prevent memory leak from abandoned OIDC flows
go func() {
ticker := time.NewTicker(5 * time.Minute)
defer ticker.Stop()
for {
select {
case <-ticker.C:
s.cleanup()
case <-s.stopCleanup:
return
}
}
}()
return s
}
// cleanup removes expired state entries
func (s *oidcStateStore) cleanup() {
s.mu.Lock()
defer s.mu.Unlock()
now := time.Now()
for state, entry := range s.entries {
if now.After(entry.ExpiresAt) {
delete(s.entries, state)
}
}
}
// Stop stops the cleanup routine
func (s *oidcStateStore) Stop() {
close(s.stopCleanup)
} }
func (s *oidcStateStore) Put(state string, entry *oidcStateEntry) { func (s *oidcStateStore) Put(state string, entry *oidcStateEntry) {

View file

@ -7,17 +7,19 @@ import (
) )
type RateLimiter struct { type RateLimiter struct {
attempts map[string][]time.Time attempts map[string][]time.Time
mu sync.RWMutex mu sync.RWMutex
limit int limit int
window time.Duration window time.Duration
stopCleanup chan struct{}
} }
func NewRateLimiter(limit int, window time.Duration) *RateLimiter { func NewRateLimiter(limit int, window time.Duration) *RateLimiter {
rl := &RateLimiter{ rl := &RateLimiter{
attempts: make(map[string][]time.Time), attempts: make(map[string][]time.Time),
limit: limit, limit: limit,
window: window, window: window,
stopCleanup: make(chan struct{}),
} }
// Clean up old entries periodically // Clean up old entries periodically
@ -25,14 +27,24 @@ func NewRateLimiter(limit int, window time.Duration) *RateLimiter {
ticker := time.NewTicker(1 * time.Minute) ticker := time.NewTicker(1 * time.Minute)
defer ticker.Stop() defer ticker.Stop()
for range ticker.C { for {
rl.cleanup() select {
case <-ticker.C:
rl.cleanup()
case <-rl.stopCleanup:
return
}
} }
}() }()
return rl return rl
} }
// Stop stops the cleanup routine
func (rl *RateLimiter) Stop() {
close(rl.stopCleanup)
}
func (rl *RateLimiter) Allow(ip string) bool { func (rl *RateLimiter) Allow(ip string) bool {
rl.mu.Lock() rl.mu.Lock()
defer rl.mu.Unlock() defer rl.mu.Unlock()

View file

@ -25,9 +25,10 @@ type RecoveryToken struct {
// RecoveryTokenStore manages recovery tokens // RecoveryTokenStore manages recovery tokens
type RecoveryTokenStore struct { type RecoveryTokenStore struct {
tokens map[string]*RecoveryToken tokens map[string]*RecoveryToken
mu sync.RWMutex mu sync.RWMutex
dataPath string dataPath string
stopCleanup chan struct{}
} }
var ( var (
@ -39,8 +40,9 @@ var (
func InitRecoveryTokenStore(dataPath string) { func InitRecoveryTokenStore(dataPath string) {
recoveryStoreOnce.Do(func() { recoveryStoreOnce.Do(func() {
recoveryStore = &RecoveryTokenStore{ recoveryStore = &RecoveryTokenStore{
tokens: make(map[string]*RecoveryToken), tokens: make(map[string]*RecoveryToken),
dataPath: dataPath, dataPath: dataPath,
stopCleanup: make(chan struct{}),
} }
recoveryStore.load() recoveryStore.load()
@ -182,11 +184,22 @@ func (r *RecoveryTokenStore) cleanupRoutine() {
ticker := time.NewTicker(1 * time.Hour) ticker := time.NewTicker(1 * time.Hour)
defer ticker.Stop() defer ticker.Stop()
for range ticker.C { for {
r.cleanup() select {
case <-ticker.C:
r.cleanup()
case <-r.stopCleanup:
log.Debug().Msg("Recovery token cleanup routine stopped")
return
}
} }
} }
// Stop stops the cleanup routine
func (r *RecoveryTokenStore) Stop() {
close(r.stopCleanup)
}
// cleanup removes expired and used tokens // cleanup removes expired and used tokens
func (r *RecoveryTokenStore) cleanup() { func (r *RecoveryTokenStore) cleanup() {
r.mu.Lock() r.mu.Lock()