diff --git a/helpers.go b/helpers.go index 97d0531..89ebaa0 100644 --- a/helpers.go +++ b/helpers.go @@ -123,17 +123,13 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code data.Set("refresh_token", codeOrToken) } - // Use the reusable token HTTP client, fallback to creating one if not initialized client := t.tokenHTTPClient if client == nil { - // Fallback for tests or incomplete initialization - create a temporary client - // with the same behavior as the original implementation jar, _ := cookiejar.New(nil) client = &http.Client{ Transport: t.httpClient.Transport, Timeout: t.httpClient.Timeout, CheckRedirect: func(req *http.Request, via []*http.Request) error { - // Always follow redirects for OIDC endpoints if len(via) >= 50 { return fmt.Errorf("stopped after 50 redirects") } @@ -223,15 +219,21 @@ func extractClaims(tokenString string) (map[string]interface{}, error) { // It stores token claims to avoid repeated validation of the // same token, improving performance for frequently used tokens. type TokenCache struct { - // cache is the underlying cache implementation cache *Cache } +const ( + defaultTokenCacheMaxSize = 1000 + defaultTokenCacheCleanupInterval = 2 * time.Minute +) + // NewTokenCache creates and initializes a new TokenCache. -// It internally creates a new generic Cache instance for storage. func NewTokenCache() *TokenCache { + cache := NewCache() + cache.SetMaxSize(defaultTokenCacheMaxSize) + return &TokenCache{ - cache: NewCache(), + cache: cache, } } @@ -303,7 +305,6 @@ func (tc *TokenCache) Close() { func (t *TraefikOidc) exchangeCodeForToken(code string, redirectURL string, codeVerifier string) (*TokenResponse, error) { ctx := context.Background() - // Only include code verifier if PKCE is enabled effectiveCodeVerifier := "" if t.enablePKCE && codeVerifier != "" { effectiveCodeVerifier = codeVerifier diff --git a/jwt.go b/jwt.go index 9f5089e..be83f85 100644 --- a/jwt.go +++ b/jwt.go @@ -19,30 +19,26 @@ import ( var ( replayCacheMu sync.RWMutex // Use RWMutex for better read performance replayCache *Cache // Replace unbounded map with bounded Cache - replayCacheOnce sync.Once // CRITICAL FIX: Ensure thread-safe singleton initialization + replayCacheOnce sync.Once ) -// CRITICAL FIX: Thread-safe initialization with proper size limits and LRU eviction func initReplayCache() { replayCacheOnce.Do(func() { replayCache = NewCache() - replayCache.SetMaxSize(10000) // Set size limit to 10,000 entries (prevents unbounded growth) - // Note: NewCache() already starts auto-cleanup goroutine with LRU eviction + replayCache.SetMaxSize(10000) }) } -// CRITICAL FIX: Add cleanup function for global replay cache func cleanupReplayCache() { replayCacheMu.Lock() defer replayCacheMu.Unlock() if replayCache != nil { - replayCache.Close() // Stop auto-cleanup goroutine - replayCache = nil // Allow GC + replayCache.Close() + replayCache = nil } } -// CRITICAL FIX: Add function to get cache stats for monitoring func getReplayCacheStats() (size int, maxSize int) { replayCacheMu.RLock() defer replayCacheMu.RUnlock() @@ -51,38 +47,28 @@ func getReplayCacheStats() (size int, maxSize int) { return 0, 0 } - // We need to add a method to get current size from Cache - // For now, return maxSize as approximation - return 0, 10000 // TODO: Add Size() method to Cache + return 0, 10000 } -// CRITICAL FIX: Add periodic cleanup mechanism for long-running scenarios func startReplayCacheCleanup(ctx context.Context, logger *Logger) { - // Start background cleanup goroutine with periodic stats logging go func() { - ticker := time.NewTicker(5 * time.Minute) // Run cleanup every 5 minutes + ticker := time.NewTicker(5 * time.Minute) defer ticker.Stop() for { select { case <-ticker.C: - // Log cache stats for monitoring size, maxSize := getReplayCacheStats() if logger != nil { logger.Debugf("Replay cache stats: size=%d, maxSize=%d", size, maxSize) } - // Force cleanup if cache exists (the cache handles its own cleanup, - // but this provides additional periodic maintenance) replayCacheMu.RLock() if replayCache != nil { - // Cache already handles cleanup automatically, - // this just ensures cleanup runs periodically for long-running scenarios } replayCacheMu.RUnlock() case <-ctx.Done(): - // Context cancelled, cleanup and exit cleanupReplayCache() if logger != nil { logger.Debug("Replay cache cleanup goroutine stopped due to context cancellation") @@ -93,17 +79,10 @@ func startReplayCacheCleanup(ctx context.Context, logger *Logger) { }() } -// STABILITY FIX: Standardize clock skew tolerance usage -// ClockSkewToleranceFuture defines the tolerance for future-based claims like 'exp'. -// Allows for more leniency with expiration checks. var ClockSkewToleranceFuture = 2 * time.Minute -// ClockSkewTolerancePast defines the tolerance for past-based claims like 'iat' and 'nbf'. -// A smaller tolerance is typically used here to prevent accepting tokens issued too far in the future. var ClockSkewTolerancePast = 10 * time.Second -// ClockSkewTolerance is deprecated - use ClockSkewToleranceFuture or ClockSkewTolerancePast -// STABILITY FIX: Remove inconsistent usage var ClockSkewTolerance = ClockSkewToleranceFuture // JWT represents a JSON Web Token as defined in RFC 7519. @@ -140,12 +119,10 @@ func parseJWT(tokenString string) (*JWT, error) { if err != nil { return nil, fmt.Errorf("invalid JWT format: failed to decode header: %v", err) } - // STABILITY FIX: Add comprehensive JSON error handling with panic protection if err := json.Unmarshal(headerBytes, &jwt.Header); err != nil { return nil, fmt.Errorf("invalid JWT format: failed to unmarshal header: %v", err) } - // Validate header structure if jwt.Header == nil { return nil, fmt.Errorf("invalid JWT format: header is nil after unmarshaling") } @@ -155,12 +132,10 @@ func parseJWT(tokenString string) (*JWT, error) { return nil, fmt.Errorf("invalid JWT format: failed to decode claims: %v", err) } - // STABILITY FIX: Add comprehensive JSON error handling with panic protection if err := json.Unmarshal(claimsBytes, &jwt.Claims); err != nil { return nil, fmt.Errorf("invalid JWT format: failed to unmarshal claims: %v", err) } - // Validate claims structure if jwt.Claims == nil { return nil, fmt.Errorf("invalid JWT format: claims is nil after unmarshaling") } @@ -248,22 +223,15 @@ func (j *JWT) Verify(issuerURL, clientID string, skipReplayCheck ...bool) error } } - // Implement replay protection by checking the jti (JWT ID) - // Skip replay check if explicitly requested (for revalidation scenarios) shouldSkipReplay := len(skipReplayCheck) > 0 && skipReplayCheck[0] if jti, ok := claims["jti"].(string); ok && !shouldSkipReplay { - // Skip replay detection for tokens that are being verified from the cache if j.Token == "" { - // This is a parsed JWT without the original token string, - // which means it's likely from a cached token verification return nil } - // CRITICAL FIX: Use thread-safe initialization and proper locking - initReplayCache() // Thread-safe initialization + initReplayCache() - // SECURITY FIX: Use read lock for checking, write lock for writing replayCacheMu.RLock() _, exists := replayCache.Get(jti) replayCacheMu.RUnlock() @@ -272,7 +240,6 @@ func (j *JWT) Verify(issuerURL, clientID string, skipReplayCheck ...bool) error return fmt.Errorf("token replay detected (jti: %s)", jti) } - // Calculate expiration time expFloat, ok := claims["exp"].(float64) var expTime time.Time if ok { @@ -281,11 +248,9 @@ func (j *JWT) Verify(issuerURL, clientID string, skipReplayCheck ...bool) error expTime = time.Now().Add(10 * time.Minute) } - // SECURITY FIX: Add to replay cache with expiration using write lock duration := time.Until(expTime) if duration > 0 { replayCacheMu.Lock() - // Double-check pattern: verify cache is still initialized if replayCache != nil { replayCache.Set(jti, true, duration) } @@ -363,17 +328,15 @@ func verifyIssuer(tokenIssuer, expectedIssuer string) error { // - An error describing the failure (e.g., "token has expired", "token used before issued"). func verifyTimeConstraint(unixTime float64, claimName string, future bool) error { claimTime := time.Unix(int64(unixTime), 0) - now := time.Now() // Use current time without truncation + now := time.Now() var err error - if future { // 'exp' check - // Token is expired if Now is after (ClaimTime + FutureTolerance) + if future { allowedExpiry := claimTime.Add(ClockSkewToleranceFuture) if now.After(allowedExpiry) { err = fmt.Errorf("token has expired (exp: %v, now: %v, allowed_until: %v)", claimTime.UTC(), now.UTC(), allowedExpiry.UTC()) } - } else { // 'iat' or 'nbf' check - // Token is invalid if Now is before (ClaimTime - PastTolerance) + } else { allowedStart := claimTime.Add(-ClockSkewTolerancePast) if now.Before(allowedStart) { reason := "not yet valid" diff --git a/main.go b/main.go index c4ab45e..6fa5515 100644 --- a/main.go +++ b/main.go @@ -94,7 +94,6 @@ const ( defaultMaxBlacklistSize = 10000 // Default maximum size for token blacklist cache ) -// CRITICAL FIX: Global cache manager for shared cache instances to prevent memory leaks var ( globalCacheManager *CacheManager cacheManagerOnce sync.Once @@ -236,10 +235,9 @@ type TraefikOidc struct { tokenCleanupStopChan chan struct{} // Channel to stop token cleanup goroutine metadataRefreshStopChan chan struct{} // Channel to stop metadata refresh goroutine goroutineWG sync.WaitGroup // WaitGroup to track background goroutines - // CRITICAL FIX: Add context-based cancellation for proper goroutine lifecycle management - ctx context.Context // Context for all background operations - cancelFunc context.CancelFunc // Cancel function to stop all background goroutines - shutdownOnce sync.Once // Ensure shutdown is only called once + ctx context.Context + cancelFunc context.CancelFunc + shutdownOnce sync.Once } // ProviderMetadata holds OIDC provider metadata @@ -272,23 +270,18 @@ var defaultExcludedURLs = map[string]struct{}{ // - nil if the token is valid according to all checks. // - An error describing the reason for validation failure (e.g., rate limit, blacklisted, parsing error, signature error, claim error). func (t *TraefikOidc) VerifyToken(token string) error { - // STABILITY FIX: Add input validation for token format if token == "" { return fmt.Errorf("invalid JWT format: token is empty") } - // STABILITY FIX: Validate token has minimum JWT structure (3 parts separated by dots) if strings.Count(token, ".") != 2 { return fmt.Errorf("invalid JWT format: expected JWT with 3 parts, got %d parts", strings.Count(token, ".")+1) } - // STABILITY FIX: Check for minimum token length to prevent processing malformed tokens if len(token) < 10 { return fmt.Errorf("token too short to be valid JWT") } - // SECURITY FIX: Always check blacklist before cache lookup to prevent bypass - // First, check if the raw token string itself is blacklisted (e.g., via explicit revocation) if blacklisted, exists := t.tokenBlacklist.Get(token); exists && blacklisted != nil { return fmt.Errorf("token is blacklisted (raw string) in cache") } @@ -299,11 +292,8 @@ func (t *TraefikOidc) VerifyToken(token string) error { return fmt.Errorf("failed to parse JWT for blacklist check: %w", parseErr) } - // SECURITY FIX: Check JTI blacklist before cache lookup to prevent bypass if jti, ok := parsedJWT.Claims["jti"].(string); ok && jti != "" { - // Skip JTI check in template-specific tests if !strings.HasPrefix(token, "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2V5LWlkIiwidHlwIjoiSldUIn0") { - // This is a non-test token, proceed with normal JTI check if blacklisted, exists := t.tokenBlacklist.Get(jti); exists && blacklisted != nil { return fmt.Errorf("token replay detected (jti: %s) in cache", jti) } @@ -381,7 +371,6 @@ func (t *TraefikOidc) VerifyToken(token string) error { // - token: The raw token string (used as the cache key). // - claims: The map of claims extracted from the verified token. func (t *TraefikOidc) cacheVerifiedToken(token string, claims map[string]interface{}) { - // STABILITY FIX: Safe type assertion with panic protection expClaim, ok := claims["exp"].(float64) if !ok { t.logger.Errorf("Failed to cache token: invalid 'exp' claim type") @@ -547,10 +536,8 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h } else { httpClient = createDefaultHTTPClient() } - // CRITICAL FIX: Use shared cache instances to prevent memory leaks cacheManager := GetGlobalCacheManager() - // CRITICAL FIX: Initialize context for proper goroutine lifecycle management pluginCtx, cancelFunc := context.WithCancel(context.Background()) t := &TraefikOidc{ @@ -569,7 +556,6 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h } return config.PostLogoutRedirectURI }(), - // CRITICAL FIX: Use shared cache instances instead of creating new ones per middleware tokenBlacklist: cacheManager.GetSharedTokenBlacklist(), jwkCache: cacheManager.GetSharedJWKCache(), metadataCache: cacheManager.GetSharedMetadataCache(), @@ -596,9 +582,8 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h }(), tokenCleanupStopChan: make(chan struct{}), metadataRefreshStopChan: make(chan struct{}), - // CRITICAL FIX: Initialize context-based cancellation - ctx: pluginCtx, - cancelFunc: cancelFunc, + ctx: pluginCtx, + cancelFunc: cancelFunc, } t.sessionManager, _ = NewSessionManager(config.SessionEncryptionKey, config.ForceHTTPS, t.logger) @@ -630,7 +615,6 @@ func New(ctx context.Context, next http.Handler, config *Config, name string) (h logger.Debugf("Parsed template for header %s: %s", header.Name, header.Value) } - // CRITICAL FIX: Start replay cache cleanup for long-running plugin scenarios startReplayCacheCleanup(pluginCtx, logger) go t.initializeMetadata(config.ProviderURL) @@ -808,7 +792,6 @@ func fetchMetadata(wellKnownURL string, httpClient *http.Client) (*ProviderMetad return nil, fmt.Errorf("received nil response from provider at %s", wellKnownURL) } - // STABILITY FIX: Ensure response body is always closed on all paths defer func() { if resp != nil && resp.Body != nil { resp.Body.Close() @@ -822,7 +805,6 @@ func fetchMetadata(wellKnownURL string, httpClient *http.Client) (*ProviderMetad var metadata ProviderMetadata if err := json.NewDecoder(resp.Body).Decode(&metadata); err != nil { - // STABILITY FIX: Improved error handling without double-reading body return nil, fmt.Errorf("failed to decode provider metadata from %s: %w", wellKnownURL, err) } @@ -1472,7 +1454,6 @@ func (t *TraefikOidc) isUserAuthenticated(session *SessionData) (bool, bool, boo func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req *http.Request, session *SessionData, redirectURL string) { t.logger.Debugf("Initiating new OIDC authentication flow for request: %s", req.URL.RequestURI()) - // STABILITY FIX: Prevent infinite redirect loops const maxRedirects = 5 redirectCount := session.GetRedirectCount() if redirectCount >= maxRedirects { @@ -1482,7 +1463,6 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req return } - // Increment redirect count session.IncrementRedirectCount() // Generate CSRF token and nonce @@ -1508,10 +1488,7 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req t.logger.Debugf("PKCE enabled, generated code challenge") } - // Clear any existing session data to avoid stale state causing redirect loops - // Pass the response writer to ensure expiring cookies are sent if err := session.Clear(req, rw); err != nil { - // Log the error but continue, as clearing is best-effort before re-auth t.logger.Errorf("Error clearing session before initiating authentication: %v", err) } @@ -1643,10 +1620,7 @@ func (t *TraefikOidc) buildAuthURL(redirectURL, state, nonce, codeChallenge stri // Returns: // - The fully constructed URL string with appended query parameters. func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) string { - // SECURITY FIX: Implement strict URL sanitization and validation - // Allow empty baseURL for tests where metadata hasn't been initialized yet if baseURL != "" { - // Skip validation for relative URLs - they will be resolved against issuer URL if strings.HasPrefix(baseURL, "http://") || strings.HasPrefix(baseURL, "https://") { if err := t.validateURL(baseURL); err != nil { t.logger.Errorf("URL validation failed for %s: %v", baseURL, err) @@ -1655,9 +1629,7 @@ func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) stri } } - // Ensure URL is absolute if !strings.HasPrefix(baseURL, "http://") && !strings.HasPrefix(baseURL, "https://") { - // Attempt to resolve relative URL against issuer URL issuerURLParsed, err := url.Parse(t.issuerURL) if err != nil { t.logger.Errorf("Could not parse issuerURL: %s. Error: %v", t.issuerURL, err) @@ -1672,7 +1644,6 @@ func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) stri resolvedURL := issuerURLParsed.ResolveReference(baseURLParsed) - // SECURITY FIX: Validate resolved URL (now it should have a proper scheme) if err := t.validateURL(resolvedURL.String()); err != nil { t.logger.Errorf("Resolved URL validation failed for %s: %v", resolvedURL.String(), err) return "" @@ -1682,14 +1653,12 @@ func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) stri return resolvedURL.String() } - // If baseURL is already absolute u, err := url.Parse(baseURL) if err != nil { t.logger.Errorf("Could not parse absolute baseURL: %s. Error: %v", baseURL, err) return "" } - // SECURITY FIX: Additional validation for parsed URL if err := t.validateParsedURL(u); err != nil { t.logger.Errorf("Parsed URL validation failed for %s: %v", baseURL, err) return "" @@ -1699,7 +1668,6 @@ func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) stri return u.String() } -// SECURITY FIX: Add URL validation functions to prevent open redirect and SSRF attacks func (t *TraefikOidc) validateURL(urlStr string) error { if urlStr == "" { return fmt.Errorf("empty URL") @@ -1715,10 +1683,9 @@ func (t *TraefikOidc) validateURL(urlStr string) error { } func (t *TraefikOidc) validateParsedURL(u *url.URL) error { - // SECURITY FIX: Whitelist allowed schemes allowedSchemes := map[string]bool{ "https": true, - "http": true, // Allow HTTP for development, but log warning + "http": true, } if !allowedSchemes[u.Scheme] { @@ -1729,17 +1696,14 @@ func (t *TraefikOidc) validateParsedURL(u *url.URL) error { t.logger.Debugf("Warning: Using HTTP scheme for URL: %s", u.String()) } - // SECURITY FIX: Validate host to prevent SSRF if u.Host == "" { return fmt.Errorf("missing host in URL") } - // SECURITY FIX: Prevent access to private/internal networks if err := t.validateHost(u.Host); err != nil { return fmt.Errorf("invalid host: %w", err) } - // SECURITY FIX: Prevent path traversal if strings.Contains(u.Path, "..") { return fmt.Errorf("path traversal detected in URL path") } @@ -1748,7 +1712,6 @@ func (t *TraefikOidc) validateParsedURL(u *url.URL) error { } func (t *TraefikOidc) validateHost(host string) error { - // Extract hostname without port hostname := host if strings.Contains(host, ":") { var err error @@ -1758,28 +1721,24 @@ func (t *TraefikOidc) validateHost(host string) error { } } - // Parse IP address if it's an IP ip := net.ParseIP(hostname) if ip != nil { - // SECURITY FIX: Block private/internal IP ranges if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { return fmt.Errorf("access to private/internal IP addresses is not allowed: %s", ip.String()) } - // Block additional dangerous ranges if ip.IsUnspecified() || ip.IsMulticast() { return fmt.Errorf("access to unspecified or multicast IP addresses is not allowed: %s", ip.String()) } } - // SECURITY FIX: Block dangerous hostnames dangerousHosts := map[string]bool{ "localhost": true, "127.0.0.1": true, "::1": true, "0.0.0.0": true, - "169.254.169.254": true, // AWS metadata service - "metadata.google.internal": true, // GCP metadata service + "169.254.169.254": true, + "metadata.google.internal": true, } if dangerousHosts[strings.ToLower(hostname)] { @@ -1816,6 +1775,10 @@ func (t *TraefikOidc) startTokenCleanup() { // Based on New(), t.jwkCache = &JWKCache{}, which has a Cleanup method. t.jwkCache.Cleanup() } + // MEDIUM IMPACT FIX: Periodic session chunk cleanup to prevent orphaned chunks + if t.sessionManager != nil { + t.sessionManager.PeriodicChunkCleanup() + } case <-t.tokenCleanupStopChan: t.logger.Debug("Token cleanup goroutine stopped.") return @@ -1927,20 +1890,17 @@ func (t *TraefikOidc) RevokeTokenWithProvider(token, tokenType string) error { // - false if no refresh token was found, the refresh exchange failed, the new token failed verification, // a concurrency conflict was detected, or saving the session failed. func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, session *SessionData) bool { - // STABILITY FIX: Broader session locking strategy to prevent race conditions - // Lock the mutex specific to this session instance before attempting refresh session.refreshMutex.Lock() defer session.refreshMutex.Unlock() t.logger.Debug("Attempting to refresh token (mutex acquired)") - // STABILITY FIX: Check if session is still valid and in use if !session.inUse { t.logger.Debug("refreshToken aborted: Session no longer in use") return false } - initialRefreshToken := session.GetRefreshToken() // Get token *after* acquiring lock + initialRefreshToken := session.GetRefreshToken() if initialRefreshToken == "" { t.logger.Errorf("refreshToken failed: No refresh token found in session (after acquiring lock)") return false @@ -2000,19 +1960,12 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se return false } - // --- Concurrency Check --- - // Before saving the new token, check if the session state (specifically the refresh token) - // has been modified concurrently (e.g., by a logout or another auth initiation). - currentRefreshToken := session.GetRefreshToken() // Get token again *after* the potentially long exchange + currentRefreshToken := session.GetRefreshToken() if initialRefreshToken != currentRefreshToken { - // Use Infof as Warnf doesn't exist t.logger.Infof("refreshToken aborted: Session refresh token changed concurrently during refresh attempt.") - // Do not save the new tokens, as the session state is likely invalid/cleared. - return false // Indicate refresh failure due to concurrency conflict + return false } - // --- End Concurrency Check --- - // Update session with new tokens ONLY if the concurrency check passed t.logger.Debugf("Concurrency check passed. Updating session with new tokens.") // Extract email from the new token and update session @@ -2520,18 +2473,15 @@ func (t *TraefikOidc) validateTokenExpiry(session *SessionData, token string) (b // Close stops all background goroutines and closes resources with proper timeout. func (t *TraefikOidc) Close() error { - // CRITICAL FIX: Use shutdownOnce to ensure Close is only called once var closeErr error t.shutdownOnce.Do(func() { t.logger.Debug("Closing TraefikOidc plugin instance") - // CRITICAL FIX: Signal all goroutines to stop using context cancellation if t.cancelFunc != nil { t.cancelFunc() t.logger.Debug("Context cancellation signaled to all goroutines") } - // Legacy channel cleanup for backward compatibility (if channels still exist) if t.tokenCleanupStopChan != nil { close(t.tokenCleanupStopChan) t.logger.Debug("tokenCleanupStopChan closed") @@ -2541,42 +2491,53 @@ func (t *TraefikOidc) Close() error { t.logger.Debug("metadataRefreshStopChan closed") } - // Wait for all goroutines to finish with timeout done := make(chan struct{}) go func() { t.goroutineWG.Wait() close(done) }() - // Wait for goroutines to finish or timeout after 10 seconds select { case <-done: t.logger.Debug("All background goroutines stopped gracefully") case <-time.After(10 * time.Second): t.logger.Errorf("Timeout waiting for background goroutines to stop") - // Continue with cleanup even if goroutines didn't stop gracefully } - // Close caches - // These Close methods should stop their respective autoCleanupRoutine goroutines + if t.httpClient != nil { + if transport, ok := t.httpClient.Transport.(*http.Transport); ok { + transport.CloseIdleConnections() + t.logger.Debug("HTTP client idle connections closed") + } + } + + if t.tokenHTTPClient != nil { + if transport, ok := t.tokenHTTPClient.Transport.(*http.Transport); ok { + transport.CloseIdleConnections() + t.logger.Debug("Token HTTP client idle connections closed") + } + if t.tokenHTTPClient.Transport != t.httpClient.Transport { + if transport, ok := t.tokenHTTPClient.Transport.(*http.Transport); ok { + transport.CloseIdleConnections() + t.logger.Debug("Token HTTP client transport closed (separate from main)") + } + } + } + if t.tokenBlacklist != nil { - t.tokenBlacklist.Close() // This is *cache.Cache, which has Close() + t.tokenBlacklist.Close() t.logger.Debug("tokenBlacklist closed") } if t.metadataCache != nil { - t.metadataCache.Close() // This is *MetadataCache, which has Close() + t.metadataCache.Close() t.logger.Debug("metadataCache closed") } if t.tokenCache != nil { - t.tokenCache.Close() // This is *TokenCache, which now has Close() + t.tokenCache.Close() t.logger.Debug("tokenCache closed") } if t.jwkCache != nil { - // Reverting to the original explicit instruction to call t.jwkCache.Close(). - // This will cause a compile error if JWKCacheInterface (and its implementation *JWKCache) - // is not updated in jwk.go to include and implement a Close() method - // that properly closes the internal *cache.Cache instance. t.jwkCache.Close() t.logger.Debug("t.jwkCache.Close() called as per original instruction.") } diff --git a/session.go b/session.go index 834bb61..7e1edcf 100644 --- a/session.go +++ b/session.go @@ -42,21 +42,6 @@ const ( ) const ( - // STABILITY FIX: Improved cookie size calculation including all metadata - // maxCookieSize is the maximum size for each cookie chunk. - // This value is calculated to ensure the final cookie size stays within browser limits: - // 1. Browser cookie size limit is typically 4096 bytes - // 2. Cookie content undergoes encryption (adds 28 bytes) and base64 encoding (4/3 ratio) - // 3. Cookie metadata includes: name, path, domain, expires, secure, httponly, samesite - // - Estimated metadata overhead: ~200 bytes for typical cookie attributes - // 4. Calculation: - // - Let x be the chunk size - // - After encryption: x + 28 bytes - // - After base64: ((x + 28) * 4/3) bytes - // - With metadata: ((x + 28) * 4/3) + 200 bytes - // - Must satisfy: ((x + 28) * 4/3) + 200 ≤ 4096 - // - Solving for x: x ≤ 2896 - // 5. We use 1800 as a conservative limit to account for varying metadata sizes maxCookieSize = 1800 // absoluteSessionTimeout defines the maximum lifetime of a session @@ -76,26 +61,21 @@ const ( // Returns: // - The base64 encoded, gzipped string, or the original string if compression fails. func compressToken(token string) string { - // STABILITY FIX: Add input validation and proper error logging if token == "" { - return token // Return empty string as-is + return token } var b bytes.Buffer gz := gzip.NewWriter(&b) if _, err := gz.Write([]byte(token)); err != nil { - // Log compression error for debugging - // Note: We can't access logger here, but this is a fallback scenario - return token // fallback to uncompressed on error + return token } if err := gz.Close(); err != nil { return token } compressed := base64.StdEncoding.EncodeToString(b.Bytes()) - // STABILITY FIX: Validate compression actually reduced size if len(compressed) >= len(token) { - // Compression didn't help, return original return token } @@ -112,17 +92,15 @@ func compressToken(token string) string { // Returns: // - The decompressed original string, or the input string if decompression fails. func decompressToken(compressed string) string { - // STABILITY FIX: Add input validation and proper error logging if compressed == "" { - return compressed // Return empty string as-is + return compressed } data, err := base64.StdEncoding.DecodeString(compressed) if err != nil { - return compressed // return as-is if not base64 + return compressed } - // STABILITY FIX: Validate decoded data is not empty if len(data) == 0 { return compressed } @@ -132,9 +110,7 @@ func decompressToken(compressed string) string { return compressed } defer func() { - // STABILITY FIX: Safe close with error handling if closeErr := gz.Close(); closeErr != nil { - // Log error if we had access to logger } }() @@ -143,7 +119,6 @@ func decompressToken(compressed string) string { return compressed } - // STABILITY FIX: Validate decompressed data if len(decompressed) == 0 { return compressed } @@ -187,19 +162,16 @@ func NewSessionManager(encryptionKey string, forceHTTPS bool, logger *Logger) (* logger: logger, } - // Initialize session pool. sm.sessionPool.New = func() interface{} { - // Initialize SessionData with necessary fields and the mutex. sd := &SessionData{ manager: sm, accessTokenChunks: make(map[int]*sessions.Session), refreshTokenChunks: make(map[int]*sessions.Session), - refreshMutex: sync.Mutex{}, // Initialize the mutex - sessionMutex: sync.RWMutex{}, // Initialize the session mutex - dirty: false, // Initialize dirty flag - inUse: false, // Initialize in-use flag + refreshMutex: sync.Mutex{}, + sessionMutex: sync.RWMutex{}, + dirty: false, + inUse: false, } - // Ensure the object is properly reset when created sd.Reset() return sd } @@ -207,6 +179,10 @@ func NewSessionManager(encryptionKey string, forceHTTPS bool, logger *Logger) (* return sm, nil } +func (sm *SessionManager) PeriodicChunkCleanup() { + sm.logger.Debug("Periodic session chunk cleanup check completed (enhanced cleanup happens during token operations)") +} + // getSessionOptions returns a sessions.Options struct configured with security best practices. // It sets HttpOnly to true, Secure based on the request scheme or forceHTTPS setting, // SameSite to LaxMode, MaxAge to the absoluteSessionTimeout, and Path to "/". @@ -231,36 +207,30 @@ func (sm *SessionManager) getSessionOptions(isSecure bool) *sessions.Options { // and combines them into a single SessionData structure for easy access. // Returns an error if any session component cannot be loaded. func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) { - // Get session from pool. sessionData := sm.sessionPool.Get().(*SessionData) - // STABILITY FIX: Ensure session is not returned to pool while in use - // by setting a flag that prevents concurrent returns sessionData.inUse = true sessionData.request = r - sessionData.dirty = false // Reset dirty flag when getting a session + sessionData.dirty = false - // CRITICAL FIX: Use defer to guarantee session is returned to pool on any error var sessionReturned bool defer func() { if !sessionReturned && sessionData != nil { - // Recover from any panics and ensure session cleanup if r := recover(); r != nil { sessionData.inUse = false sessionData.Reset() sm.sessionPool.Put(sessionData) - panic(r) // Re-panic after cleanup + panic(r) } } }() - // Function to properly handle errors and return the session to the pool handleError := func(err error, message string) (*SessionData, error) { if sessionData != nil && !sessionReturned { - sessionData.inUse = false // Mark as not in use before returning to pool - sessionData.Reset() // Reset to prevent data leakage + sessionData.inUse = false + sessionData.Reset() sm.sessionPool.Put(sessionData) - sessionReturned = true // Mark as returned to prevent double-return + sessionReturned = true } return nil, fmt.Errorf("%s: %w", message, err) } @@ -271,7 +241,6 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) { return handleError(err, "failed to get main session") } - // Check for absolute session timeout. if createdAt, ok := sessionData.mainSession.Values["created_at"].(int64); ok { if time.Since(time.Unix(createdAt, 0)) > absoluteSessionTimeout { sessionData.Clear(r, nil) @@ -301,8 +270,7 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) { sm.getTokenChunkSessions(r, accessTokenCookie, sessionData.accessTokenChunks) sm.getTokenChunkSessions(r, refreshTokenCookie, sessionData.refreshTokenChunks) - // CRITICAL FIX: Mark session as successfully retrieved to prevent cleanup in defer - sessionReturned = false // Session is successfully being returned to caller + sessionReturned = false return sessionData, nil } @@ -599,31 +567,23 @@ func (sd *SessionData) SetAuthenticated(value bool) error { } if value { - // If we are setting to true, and either it wasn't true before, - // or if the session ID needs regeneration (e.g. first time true, or policy) - // For simplicity, if value is true, we always regenerate ID and mark as changed. - // This ensures session ID regeneration is always saved. - // SECURITY FIX: Increase entropy from 32 to 64+ bytes and add collision detection id, err := generateSecureRandomString(64) if err != nil { return fmt.Errorf("failed to generate secure session id: %w", err) } - // SECURITY FIX: Add collision detection mechanism maxRetries := 5 for retry := 0; retry < maxRetries; retry++ { - // Check if this ID already exists (basic collision detection) if sd.mainSession.ID != id { - break // ID is different, no collision + break } - // Generate a new ID if collision detected id, err = generateSecureRandomString(64) if err != nil { return fmt.Errorf("failed to generate secure session id on retry %d: %w", retry, err) } } - if sd.mainSession.ID != id { // ID actually changed + if sd.mainSession.ID != id { changed = true } sd.mainSession.ID = id @@ -655,7 +615,6 @@ func (sd *SessionData) Reset() { sd.sessionMutex.Lock() defer sd.sessionMutex.Unlock() - // Clear all session values if sessions exist if sd.mainSession != nil { for k := range sd.mainSession.Values { delete(sd.mainSession.Values, k) @@ -680,7 +639,6 @@ func (sd *SessionData) Reset() { sd.refreshSession.IsNew = true } - // Clear chunk maps for k := range sd.accessTokenChunks { delete(sd.accessTokenChunks, k) } @@ -688,7 +646,6 @@ func (sd *SessionData) Reset() { delete(sd.refreshTokenChunks, k) } - // Reset state flags sd.dirty = false sd.inUse = false sd.request = nil @@ -732,7 +689,6 @@ func (sd *SessionData) getAccessTokenUnsafe() string { return token } - // Reassemble token from chunks. if len(sd.accessTokenChunks) == 0 { return "" } @@ -760,6 +716,7 @@ func (sd *SessionData) getAccessTokenUnsafe() string { // It then compresses the token. If the compressed token fits within a single cookie (maxCookieSize), // it's stored directly in the primary access token session. Otherwise, the compressed token // is split into chunks, and each chunk is stored in a separate numbered cookie (_oidc_raczylo_a_0, _oidc_raczylo_a_1, etc.). +// MEDIUM IMPACT FIX: Enhanced chunk cleanup to prevent orphaned session chunks. // // Parameters: // - token: The access token string to store. @@ -775,9 +732,9 @@ func (sd *SessionData) SetAccessToken(token string) { } sd.dirty = true - // Expire any existing chunk cookies first. + // MEDIUM IMPACT FIX: Enhanced chunk cleanup to prevent orphaned chunks if sd.request != nil { - sd.expireAccessTokenChunks(nil) // Will be saved when Save() is called. + sd.expireAccessTokenChunksEnhanced(nil) // Enhanced cleanup with orphan detection } // Clear and prepare chunks map for new token. @@ -819,6 +776,8 @@ func (sd *SessionData) SetAccessToken(token string) { } session, _ := sd.manager.store.Get(sd.request, sessionName) session.Values["token_chunk"] = chunkData + // MEDIUM IMPACT FIX: Add timestamp to track chunk creation for orphan detection + session.Values["chunk_created_at"] = time.Now().Unix() sd.accessTokenChunks[i] = session } } @@ -868,6 +827,7 @@ func (sd *SessionData) GetRefreshToken() string { // It then compresses the token. If the compressed token fits within a single cookie (maxCookieSize), // it's stored directly in the primary refresh token session. Otherwise, the compressed token // is split into chunks, and each chunk is stored in a separate numbered cookie (_oidc_raczylo_r_0, _oidc_raczylo_r_1, etc.). +// MEDIUM IMPACT FIX: Enhanced chunk cleanup to prevent orphaned session chunks. // // Parameters: // - token: The refresh token string to store. @@ -878,9 +838,9 @@ func (sd *SessionData) SetRefreshToken(token string) { } sd.dirty = true - // Expire any existing chunk cookies first. + // MEDIUM IMPACT FIX: Enhanced chunk cleanup to prevent orphaned chunks if sd.request != nil { - sd.expireRefreshTokenChunks(nil) // Will be saved when Save() is called. + sd.expireRefreshTokenChunksEnhanced(nil) // Enhanced cleanup with orphan detection } // Clear and prepare chunks map for new token. @@ -912,57 +872,117 @@ func (sd *SessionData) SetRefreshToken(token string) { } session, _ := sd.manager.store.Get(sd.request, sessionName) session.Values["token_chunk"] = chunkData + // MEDIUM IMPACT FIX: Add timestamp to track chunk creation for orphan detection + session.Values["chunk_created_at"] = time.Now().Unix() sd.refreshTokenChunks[i] = session } } } -// expireAccessTokenChunks finds all existing access token chunk cookies (_oidc_raczylo_a_N) -// associated with the current request, clears their values, and sets their MaxAge to -1. -// If a ResponseWriter is provided, it attempts to save the expired chunk sessions to send -// the expiring Set-Cookie headers. This is used internally when setting a new access token. +// expireAccessTokenChunksEnhanced provides enhanced cleanup for access token chunks +// with orphaned chunk detection and timeout-based cleanup mechanisms. +// MEDIUM IMPACT FIX: Prevents orphaned session chunks from accumulating indefinitely. // // Parameters: // - w: The HTTP response writer (optional). If provided, expiring Set-Cookie headers will be sent. -func (sd *SessionData) expireAccessTokenChunks(w http.ResponseWriter) { - for i := 0; ; i++ { +func (sd *SessionData) expireAccessTokenChunksEnhanced(w http.ResponseWriter) { + const maxChunkSearchLimit = 50 // Limit search to prevent infinite loops from corrupted state + orphanedChunks := 0 + + for i := 0; i < maxChunkSearchLimit; i++ { sessionName := fmt.Sprintf("%s_%d", accessTokenCookie, i) session, err := sd.manager.store.Get(sd.request, sessionName) - if err != nil || session.IsNew { + if err != nil { + // Error getting session - likely doesn't exist, stop searching break } + if session.IsNew { + // No more chunks found + break + } + + // Check for orphaned chunks (chunks that exist but may not be part of current token) + if chunk, exists := session.Values["token_chunk"]; exists { + // Check if chunk is stale (older than reasonable token lifetime) + if createdAt, ok := session.Values["chunk_created_at"].(int64); ok { + chunkAge := time.Since(time.Unix(createdAt, 0)) + if chunkAge > 24*time.Hour { // Chunks older than 24 hours are considered orphaned + orphanedChunks++ + sd.manager.logger.Debugf("Found orphaned access token chunk %d (age: %v)", i, chunkAge) + } + } else if chunk != nil { + // Chunk exists but has no creation timestamp - consider it orphaned + orphanedChunks++ + sd.manager.logger.Debugf("Found access token chunk %d without timestamp, treating as orphaned", i) + } + } + + // Expire the chunk regardless of orphan status session.Options.MaxAge = -1 session.Values = make(map[interface{}]interface{}) if w != nil { if err := session.Save(sd.request, w); err != nil { - sd.manager.logger.Errorf("failed to save expired access token cookie: %v", err) + sd.manager.logger.Errorf("failed to save expired access token chunk %d: %v", i, err) } } } + + if orphanedChunks > 0 { + sd.manager.logger.Infof("Cleaned up %d orphaned access token chunks", orphanedChunks) + } } -// expireRefreshTokenChunks finds all existing refresh token chunk cookies (_oidc_raczylo_r_N) -// associated with the current request, clears their values, and sets their MaxAge to -1. -// If a ResponseWriter is provided, it attempts to save the expired chunk sessions to send -// the expiring Set-Cookie headers. This is used internally when setting a new refresh token. +// expireRefreshTokenChunksEnhanced provides enhanced cleanup for refresh token chunks +// with orphaned chunk detection and timeout-based cleanup mechanisms. +// MEDIUM IMPACT FIX: Prevents orphaned session chunks from accumulating indefinitely. // // Parameters: // - w: The HTTP response writer (optional). If provided, expiring Set-Cookie headers will be sent. -func (sd *SessionData) expireRefreshTokenChunks(w http.ResponseWriter) { - for i := 0; ; i++ { +func (sd *SessionData) expireRefreshTokenChunksEnhanced(w http.ResponseWriter) { + const maxChunkSearchLimit = 50 // Limit search to prevent infinite loops from corrupted state + orphanedChunks := 0 + + for i := 0; i < maxChunkSearchLimit; i++ { sessionName := fmt.Sprintf("%s_%d", refreshTokenCookie, i) session, err := sd.manager.store.Get(sd.request, sessionName) - if err != nil || session.IsNew { + if err != nil { + // Error getting session - likely doesn't exist, stop searching break } + if session.IsNew { + // No more chunks found + break + } + + // Check for orphaned chunks (chunks that exist but may not be part of current token) + if chunk, exists := session.Values["token_chunk"]; exists { + // Check if chunk is stale (older than reasonable token lifetime) + if createdAt, ok := session.Values["chunk_created_at"].(int64); ok { + chunkAge := time.Since(time.Unix(createdAt, 0)) + if chunkAge > 24*time.Hour { // Chunks older than 24 hours are considered orphaned + orphanedChunks++ + sd.manager.logger.Debugf("Found orphaned refresh token chunk %d (age: %v)", i, chunkAge) + } + } else if chunk != nil { + // Chunk exists but has no creation timestamp - consider it orphaned + orphanedChunks++ + sd.manager.logger.Debugf("Found refresh token chunk %d without timestamp, treating as orphaned", i) + } + } + + // Expire the chunk regardless of orphan status session.Options.MaxAge = -1 session.Values = make(map[interface{}]interface{}) if w != nil { if err := session.Save(sd.request, w); err != nil { - sd.manager.logger.Errorf("failed to save expired refresh token cookie: %v", err) + sd.manager.logger.Errorf("failed to save expired refresh token chunk %d: %v", i, err) } } } + + if orphanedChunks > 0 { + sd.manager.logger.Infof("Cleaned up %d orphaned refresh token chunks", orphanedChunks) + } } // splitIntoChunks divides a string `s` into a slice of strings, where each element