diff --git a/bearer_auth.go b/bearer_auth.go index a494b74..591799c 100644 --- a/bearer_auth.go +++ b/bearer_auth.go @@ -371,6 +371,11 @@ func enforceIatAge(claims map[string]interface{}, maxAge time.Duration) *bearerE if time.Since(iat) > maxAge { return newBearerError(bearerErrInvalidToken, "token iat outside age bound") } + // The opaque introspection path never runs jwt.Verify, so a future iat + // would otherwise defeat the age bound for the token's whole lifetime. + if time.Until(iat) > ClockSkewToleranceFuture { + return newBearerError(bearerErrInvalidToken, "token iat in the future") + } return nil } diff --git a/hunt_20261015_regression_test.go b/hunt_20261015_regression_test.go new file mode 100644 index 0000000..894394d --- /dev/null +++ b/hunt_20261015_regression_test.go @@ -0,0 +1,238 @@ +package traefikoidc + +import ( + "context" + "crypto" + "crypto/rand" + "crypto/rsa" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" +) + +func TestEnhanceSessionSecurity_CaseVariantForwardedHostStaysHostScoped(t *testing.T) { + cases := []struct { + name string + xfh string + }{ + {name: "exact-case parent", xfh: "corp.example.com"}, + {name: "upper-case parent", xfh: "CORP.example.com"}, + {name: "mixed-case parent", xfh: "Corp.Example.COM"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + sm := createTestSessionManager(t) + t.Cleanup(func() { _ = sm.Shutdown() }) + req := httptest.NewRequest(http.MethodGet, "https://app.corp.example.com/", nil) + req.Host = "app.corp.example.com" + req.Header.Set("X-Forwarded-Host", tc.xfh) + opts := sm.EnhanceSessionSecurity(nil, req) + if opts.Domain != "app.corp.example.com" { + t.Fatalf("Domain = %q, want host-scoped app.corp.example.com", opts.Domain) + } + }) + } + + t.Run("case-variant forwarded host equal to serving host is kept", func(t *testing.T) { + sm := createTestSessionManager(t) + t.Cleanup(func() { _ = sm.Shutdown() }) + req := httptest.NewRequest(http.MethodGet, "https://internal.svc/", nil) + req.Host = "internal.svc" + req.Header.Set("X-Forwarded-Host", "App.Example.com") + opts := sm.EnhanceSessionSecurity(nil, req) + if opts.Domain != "app.example.com" { + t.Fatalf("Domain = %q, want app.example.com", opts.Domain) + } + }) +} + +func TestEnforceIatAge_RejectsFutureIat(t *testing.T) { + const maxAge = 24 * time.Hour + now := time.Now() + cases := []struct { + name string + iat time.Time + wantErr bool + }{ + {name: "now", iat: now}, + {name: "within future skew", iat: now.Add(ClockSkewToleranceFuture / 2)}, + {name: "one hour ahead", iat: now.Add(time.Hour), wantErr: true}, + {name: "just beyond skew", iat: now.Add(ClockSkewToleranceFuture + 10*time.Second), wantErr: true}, + {name: "older than max age", iat: now.Add(-48 * time.Hour), wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + err := enforceIatAge(map[string]interface{}{"iat": float64(tc.iat.Unix())}, maxAge) + if (err != nil) != tc.wantErr { + t.Fatalf("enforceIatAge err = %v, wantErr %v", err, tc.wantErr) + } + }) + } +} + +func TestJWKCache_CooldownServesParsedEntryWithoutReparse(t *testing.T) { + priv, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("keygen: %v", err) + } + c := NewJWKCache() + url := fmt.Sprintf("https://jwks.invalid/%d", time.Now().UnixNano()) + // The raw set is empty, so only the pre-parsed entry can satisfy the lookup. + _ = c.cache.SetLocal(url, &JWKSet{}, time.Hour) + _ = c.cache.SetLocal(url+parsedKeysSuffix, &parsedJWKS{keys: map[string]crypto.PublicKey{"k1": &priv.PublicKey}}, time.Hour) + c.lastSignatureRefresh[url] = time.Now() + + got, err := c.getPublicKeyFresh(context.Background(), url, "k1", http.DefaultClient) + if err != nil { + t.Fatalf("getPublicKeyFresh: %v", err) + } + if got != crypto.PublicKey(&priv.PublicKey) { + t.Fatal("cooldown branch did not serve the cached parsed key") + } +} + +func TestFrontchannelLogout_FloodCannotEvictSignedMarkers(t *testing.T) { + cache := &CacheInterfaceWrapper{cache: NewUniversalCache(UniversalCacheConfig{ + Type: CacheTypeSession, + MaxSize: 5000, + DefaultTTL: sessionInvalidationTTL, + Logger: NewLogger("error"), + SkipAutoCleanup: true, + MonotonicMarkers: true, + })} + oidc := &TraefikOidc{ + logger: NewLogger("error"), + enableFrontchannelLogout: true, + sessionInvalidationCache: cache, + issuerURL: "https://idp.example.com", + } + t.Cleanup(func() { _ = cache.cache.Close() }) + + if err := oidc.invalidateSession("", "real-subject"); err != nil { + t.Fatalf("invalidateSession: %v", err) + } + if err := oidc.checkAndMarkLogoutJTIProcessed("real-jti", time.Now().Unix()); err != nil { + t.Fatalf("mark jti: %v", err) + } + + for i := 0; i < 6000; i++ { + req := httptest.NewRequest(http.MethodGet, fmt.Sprintf("/fc?iss=https://idp.example.com&sid=flood-%d", i), nil) + rw := httptest.NewRecorder() + oidc.handleFrontchannelLogout(rw, req) + if rw.Code != http.StatusOK { + t.Fatalf("flood request %d: status %d", i, rw.Code) + } + } + + if _, ok := cache.Get(oidc.buildSessionInvalidationKey("sub", "real-subject")); !ok { + t.Error("backchannel sub marker evicted by front-channel flood") + } + if err := oidc.checkAndMarkLogoutJTIProcessed("real-jti", time.Now().Unix()); err == nil { + t.Error("jti replay marker evicted by front-channel flood: replay accepted") + } + held := 0 + for i := 0; i < 6000; i++ { + if _, ok := cache.Get(oidc.buildSessionInvalidationKey("sid", fmt.Sprintf("flood-%d", i))); ok { + held++ + } + } + if held > maxFrontchannelMarkers { + t.Errorf("front-channel markers held = %d, want <= %d", held, maxFrontchannelMarkers) + } + if _, ok := cache.Get(oidc.buildSessionInvalidationKey("sid", "flood-5999")); !ok { + t.Error("most recent front-channel marker must remain") + } +} + +func TestMarkerQuota_Admit(t *testing.T) { + var q markerQuota + if got := q.admit("a", 2); got != "" { + t.Errorf("admit a = %q, want none", got) + } + if got := q.admit("a", 2); got != "" { + t.Errorf("re-admit a = %q, want none", got) + } + if got := q.admit("b", 2); got != "" { + t.Errorf("admit b = %q, want none", got) + } + if got := q.admit("c", 2); got != "a" { + t.Errorf("admit c evicted %q, want a", got) + } +} + +func TestServeHTTP_InitWaitQueueFullFailsFast(t *testing.T) { + oidc := &TraefikOidc{ + logger: NewLogger("error"), + initComplete: make(chan struct{}), + sessionManager: createTestSessionManager(t), + firstRequestStarted: 1, + metadataRefreshStartedAtomic: 1, + } + oidc.initWaiters = maxInitWaiters + + rw := httptest.NewRecorder() + start := time.Now() + oidc.ServeHTTP(rw, httptest.NewRequest(http.MethodGet, "/protected", nil)) + + if rw.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want 503", rw.Code) + } + if rw.Header().Get("Retry-After") == "" { + t.Error("missing Retry-After header") + } + if time.Since(start) > 5*time.Second { + t.Errorf("rejected after %v, want immediate", time.Since(start)) + } + if oidc.initWaiters != maxInitWaiters { + t.Errorf("initWaiters = %d, want unchanged %d", oidc.initWaiters, maxInitWaiters) + } +} + +func TestServeHTTP_InitWaiterReleasedOnCancel(t *testing.T) { + oidc := &TraefikOidc{ + logger: NewLogger("error"), + initComplete: make(chan struct{}), + sessionManager: createTestSessionManager(t), + firstRequestStarted: 1, + metadataRefreshStartedAtomic: 1, + } + ctx, cancel := context.WithCancel(context.Background()) + cancel() + req := httptest.NewRequest(http.MethodGet, "/protected", nil).WithContext(ctx) + oidc.ServeHTTP(httptest.NewRecorder(), req) + if oidc.initWaiters != 0 { + t.Fatalf("initWaiters = %d after canceled wait, want 0", oidc.initWaiters) + } +} + +func TestCleanupOldCookies_CapsDeletionHeaders(t *testing.T) { + sm, err := NewSessionManager("test-encryption-key-32-bytes-long!!", false, "cookie.example.com", "testpref", 0, newNoOpLogger()) + if err != nil { + t.Fatalf("session manager: %v", err) + } + t.Cleanup(func() { _ = sm.Shutdown() }) + + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = "a.b.example.com" + for i := 0; i < 180; i++ { + req.AddCookie(&http.Cookie{Name: fmt.Sprintf("testpref_%d", i), Value: "x"}) + } + rw := httptest.NewRecorder() + sm.CleanupOldCookies(rw, req) + + got := len(rw.Result().Cookies()) + if got == 0 { + t.Fatal("expected deletion cookies for the first wave") + } + if limit := maxCleanupCookies * 4; got > limit { + t.Fatalf("deletion Set-Cookie headers = %d, want <= %d", got, limit) + } + for _, c := range rw.Result().Cookies() { + if !strings.HasPrefix(c.Name, "testpref_") { + t.Errorf("unexpected deletion cookie %q", c.Name) + } + } +} diff --git a/internal/recovery/circuit_breaker.go b/internal/recovery/circuit_breaker.go index ed963a0..547f222 100644 --- a/internal/recovery/circuit_breaker.go +++ b/internal/recovery/circuit_breaker.go @@ -93,8 +93,16 @@ func (cb *CircuitBreaker) ExecuteWithContext(ctx context.Context, fn func() erro return fmt.Errorf("circuit breaker is open") } - // Execute the function + // A panicking fn must still settle the breaker, otherwise a half-open + // probe slot leaks until the next open->half-open transition. + panicked := true + defer func() { + if panicked { + cb.recordFailure() + } + }() err := fn() + panicked = false if err != nil { cb.recordFailure() diff --git a/internal/recovery/hunt_20261015_regression_test.go b/internal/recovery/hunt_20261015_regression_test.go new file mode 100644 index 0000000..2f75135 --- /dev/null +++ b/internal/recovery/hunt_20261015_regression_test.go @@ -0,0 +1,40 @@ +package recovery + +import ( + "context" + "errors" + "testing" + "time" +) + +func TestCircuitBreaker_PanickingProbeReleasesHalfOpenSlot(t *testing.T) { + cb := NewCircuitBreaker(CircuitBreakerConfig{ + FailureThreshold: 1, + SuccessThreshold: 1, + Timeout: 20 * time.Millisecond, + MaxRequests: 1, + }, &mockLogger{}) + + _ = cb.ExecuteWithContext(context.Background(), func() error { return errors.New("fail") }) + time.Sleep(40 * time.Millisecond) + + func() { + defer func() { + if recover() == nil { + t.Error("panic must still propagate to the caller") + } + }() + _ = cb.ExecuteWithContext(context.Background(), func() error { panic("boom") }) + }() + + if cb.GetState() != CircuitBreakerOpen { + t.Fatalf("state after panicking probe = %s, want open", cb.GetState()) + } + time.Sleep(40 * time.Millisecond) + if err := cb.ExecuteWithContext(context.Background(), func() error { return nil }); err != nil { + t.Fatalf("probe after next timeout rejected: %v", err) + } + if cb.GetState() != CircuitBreakerClosed { + t.Fatalf("state = %s, want closed", cb.GetState()) + } +} diff --git a/jwk.go b/jwk.go index 1b81618..8713c7d 100644 --- a/jwk.go +++ b/jwk.go @@ -389,6 +389,13 @@ func (c *JWKCache) getPublicKeyFresh(ctx context.Context, jwksURL, kid string, h if withinCooldown { // Serve the cached keyset; do not fetch upstream again. + if v, found := c.cache.GetLocal(jwksURL + parsedKeysSuffix); found { + if pj, ok := v.(*parsedJWKS); ok { + if k, ok := pj.keys[kid]; ok { + return k, nil + } + } + } if cached, found := c.cache.GetLocal(jwksURL); found { if jwks, ok := cached.(*JWKSet); ok { if k, ok := buildParsedJWKS(jwks).keys[kid]; ok { diff --git a/logout.go b/logout.go index f21bd29..fe94d17 100644 --- a/logout.go +++ b/logout.go @@ -51,8 +51,43 @@ const ( // sessionInvalidationTTL is how long to remember invalidated sessions // Should be at least as long as your session max age sessionInvalidationTTL = 25 * time.Hour + + // maxFrontchannelMarkers caps the sid markers the unauthenticated + // front-channel endpoint may keep in the shared invalidation cache, so + // a flood cannot LRU-evict signed backchannel and jti replay markers. + maxFrontchannelMarkers = 1000 ) +// markerQuota is a FIFO of cache keys written on behalf of one untrusted +// source. The zero value is ready to use. +type markerQuota struct { + set map[string]struct{} + keys []string + mu sync.Mutex +} + +// admit records key and returns the oldest key to drop once more than limit +// keys are held, or "" when none must go. Re-admitting a held key is a no-op. +func (q *markerQuota) admit(key string, limit int) string { + q.mu.Lock() + defer q.mu.Unlock() + if q.set == nil { + q.set = make(map[string]struct{}) + } + if _, ok := q.set[key]; ok { + return "" + } + q.set[key] = struct{}{} + q.keys = append(q.keys, key) + if len(q.keys) <= limit { + return "" + } + oldest := q.keys[0] + q.keys = q.keys[1:] + delete(q.set, oldest) + return oldest +} + // LogoutTokenClaims represents the claims in an OIDC logout token // as defined in OpenID Connect Back-Channel Logout 1.0 type LogoutTokenClaims struct { @@ -191,6 +226,10 @@ func (t *TraefikOidc) handleFrontchannelLogout(rw http.ResponseWriter, req *http return } + if evicted := t.frontchannelMarkers.admit(t.buildSessionInvalidationKey("sid", sid), maxFrontchannelMarkers); evicted != "" { + t.sessionInvalidationCache.Delete(evicted) + } + t.logger.Infof("Front-channel logout: successfully invalidated session (sid=%s)", sid) // Return a minimal HTML response that's suitable for iframe loading diff --git a/middleware.go b/middleware.go index cd1ee80..8fa931d 100644 --- a/middleware.go +++ b/middleware.go @@ -158,6 +158,15 @@ func (w *optionsPreflightWriter) Write(b []byte) (int, error) { return len(b), nil } +const ( + // maxInitWaiters bounds requests parked on initComplete so a boot-time + // flood with the IdP down cannot hold unbounded connections for 30s each. + maxInitWaiters = 512 + // initWaitRetryAfter is the Retry-After value (seconds) sent when the + // init-wait queue is full. + initWaitRetryAfter = "5" +) + // bypassReason describes why a request is being forwarded without OIDC auth. // It is only used for logging and to decide whether extra side-effects // (propagating the user header from an existing session) should run. @@ -701,6 +710,13 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { // Log waiting for initialization to help diagnose hanging requests t.logger.Debug("Waiting for OIDC provider initialization...") + if atomic.AddInt32(&t.initWaiters, 1) > maxInitWaiters { + atomic.AddInt32(&t.initWaiters, -1) + rw.Header().Set("Retry-After", initWaitRetryAfter) + t.sendErrorResponse(rw, req, "OIDC provider initialization in progress - please try again later", http.StatusServiceUnavailable) + return + } + // time.NewTimer + Stop avoids leaking a goroutine+channel for 30s on every // request when initComplete fires quickly (would happen with time.After). initTimer := time.NewTimer(30 * time.Second) @@ -708,6 +724,7 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { select { case <-t.initComplete: + atomic.AddInt32(&t.initWaiters, -1) // Read issuerURL via atomic snapshot when available — replaces the // metadataMu.RLock that previously fired on every non-bypass request. // Under Yaegi each RLock acquisition costs 1-5ms of interpreter @@ -744,10 +761,12 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { return } case <-req.Context().Done(): + atomic.AddInt32(&t.initWaiters, -1) t.logger.Debug("Request canceled while waiting for OIDC initialization") t.sendErrorResponse(rw, req, "Request canceled", http.StatusRequestTimeout) return case <-initTimer.C: + atomic.AddInt32(&t.initWaiters, -1) t.logger.Error("Timeout waiting for OIDC initialization") t.sendErrorResponse(rw, req, "Timeout waiting for OIDC provider initialization - please try again later", http.StatusServiceUnavailable) return diff --git a/session.go b/session.go index 3e6f149..0fa3066 100644 --- a/session.go +++ b/session.go @@ -116,6 +116,10 @@ const ( defaultCookiePrefix = "_oidc_raczylo_" ) +// maxCleanupCookies bounds the cookie names CleanupOldCookies emits deletion +// headers for per response (up to 4 Set-Cookie headers each). +const maxCleanupCookies = 32 + const ( // maxCookieSize is the maximum raw data size per chunk before securecookie encoding. // @@ -937,6 +941,9 @@ func (sm *SessionManager) EnhanceSessionSecurity(options *sessions.Options, r *h // service). Keep it unless it is a strict parent of the serving // host, which would scope the token-bearing cookie to sibling // subdomains — then fall back to the serving host. + // DNS names and the cookie Domain attribute are case-insensitive. + host = strings.ToLower(host) + fh = strings.ToLower(fh) if !strings.HasSuffix(host, "."+fh) { host = fh } @@ -1024,6 +1031,7 @@ func (sm *SessionManager) CleanupOldCookies(w http.ResponseWriter, r *http.Reque } processedCookies := make(map[string]bool) + deletedCookies := 0 for _, cookie := range cookies { // Check if cookie belongs to this middleware instance using the configured prefix @@ -1037,7 +1045,8 @@ func (sm *SessionManager) CleanupOldCookies(w http.ResponseWriter, r *http.Reque shouldCleanup := currentDomain != "" && !sm.cleanupDone sm.cleanupMutex.RUnlock() - if shouldCleanup { + if shouldCleanup && deletedCookies < maxCleanupCookies { + deletedCookies++ for _, domain := range domainsToClean { if domain == currentDomain || domain == "."+currentDomain || "."+domain == currentDomain { continue diff --git a/types.go b/types.go index ae33b5d..6be0651 100644 --- a/types.go +++ b/types.go @@ -111,6 +111,9 @@ type TraefikOidc struct { // recovery attempt. Stored atomically so the hot ServeHTTP path can // throttle retries without acquiring metadataRetryMutex on every request. lastMetadataRetryNano int64 + // initWaiters counts requests parked on initComplete; bounded by + // maxInitWaiters so a boot-time flood cannot hold unbounded connections. + initWaiters int32 // firstRequestStarted is 0 until the very first non-health request fires // the background-task bootstrap; then it flips to 1 via CAS. Replaces the // firstRequestMutex + firstRequestReceived combo which previously took @@ -224,6 +227,7 @@ type TraefikOidc struct { dcrMu sync.Mutex shutdownOnce sync.Once sessionInvalidationCache CacheInterface + frontchannelMarkers markerQuota refreshResultCache CacheInterface minimalHeaders bool stripAuthCookies bool