fix(security): close confirmed findings from the 2026-10-15 hunt (#166)

Seven findings are fixed. Each fix has a regression test.

- Compare X-Forwarded-Host and the serving host case-insensitively, so a
  case variant cannot widen the session cookie Domain.
- Reject a future iat on the opaque introspection path.
- Serve the cached parsed JWKS inside the signature-refresh cooldown.
- Release the half-open circuit slot when a probe panics.
- Cap requests that wait for provider initialisation at 512. Extra
  requests receive 503 with Retry-After.
- Cap cookie deletion headers at 32 cookie names per response.
- Cap front-channel sid markers at 1000, so unauthenticated requests
  cannot evict backchannel or jti replay markers.
This commit is contained in:
2026-10-16 00:47:35 +01:00
committed by GitHub
parent f0af6282dc
commit 9ab7220828
9 changed files with 371 additions and 2 deletions
+5
View File
@@ -371,6 +371,11 @@ func enforceIatAge(claims map[string]interface{}, maxAge time.Duration) *bearerE
if time.Since(iat) > maxAge { if time.Since(iat) > maxAge {
return newBearerError(bearerErrInvalidToken, "token iat outside age bound") 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 return nil
} }
+238
View File
@@ -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)
}
}
}
+9 -1
View File
@@ -93,8 +93,16 @@ func (cb *CircuitBreaker) ExecuteWithContext(ctx context.Context, fn func() erro
return fmt.Errorf("circuit breaker is open") 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() err := fn()
panicked = false
if err != nil { if err != nil {
cb.recordFailure() cb.recordFailure()
@@ -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())
}
}
+7
View File
@@ -389,6 +389,13 @@ func (c *JWKCache) getPublicKeyFresh(ctx context.Context, jwksURL, kid string, h
if withinCooldown { if withinCooldown {
// Serve the cached keyset; do not fetch upstream again. // 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 cached, found := c.cache.GetLocal(jwksURL); found {
if jwks, ok := cached.(*JWKSet); ok { if jwks, ok := cached.(*JWKSet); ok {
if k, ok := buildParsedJWKS(jwks).keys[kid]; ok { if k, ok := buildParsedJWKS(jwks).keys[kid]; ok {
+39
View File
@@ -51,8 +51,43 @@ const (
// sessionInvalidationTTL is how long to remember invalidated sessions // sessionInvalidationTTL is how long to remember invalidated sessions
// Should be at least as long as your session max age // Should be at least as long as your session max age
sessionInvalidationTTL = 25 * time.Hour 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 // LogoutTokenClaims represents the claims in an OIDC logout token
// as defined in OpenID Connect Back-Channel Logout 1.0 // as defined in OpenID Connect Back-Channel Logout 1.0
type LogoutTokenClaims struct { type LogoutTokenClaims struct {
@@ -191,6 +226,10 @@ func (t *TraefikOidc) handleFrontchannelLogout(rw http.ResponseWriter, req *http
return 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) t.logger.Infof("Front-channel logout: successfully invalidated session (sid=%s)", sid)
// Return a minimal HTML response that's suitable for iframe loading // Return a minimal HTML response that's suitable for iframe loading
+19
View File
@@ -158,6 +158,15 @@ func (w *optionsPreflightWriter) Write(b []byte) (int, error) {
return len(b), nil 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. // bypassReason describes why a request is being forwarded without OIDC auth.
// It is only used for logging and to decide whether extra side-effects // It is only used for logging and to decide whether extra side-effects
// (propagating the user header from an existing session) should run. // (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 // Log waiting for initialization to help diagnose hanging requests
t.logger.Debug("Waiting for OIDC provider initialization...") 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 // time.NewTimer + Stop avoids leaking a goroutine+channel for 30s on every
// request when initComplete fires quickly (would happen with time.After). // request when initComplete fires quickly (would happen with time.After).
initTimer := time.NewTimer(30 * time.Second) initTimer := time.NewTimer(30 * time.Second)
@@ -708,6 +724,7 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
select { select {
case <-t.initComplete: case <-t.initComplete:
atomic.AddInt32(&t.initWaiters, -1)
// Read issuerURL via atomic snapshot when available — replaces the // Read issuerURL via atomic snapshot when available — replaces the
// metadataMu.RLock that previously fired on every non-bypass request. // metadataMu.RLock that previously fired on every non-bypass request.
// Under Yaegi each RLock acquisition costs 1-5ms of interpreter // 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 return
} }
case <-req.Context().Done(): case <-req.Context().Done():
atomic.AddInt32(&t.initWaiters, -1)
t.logger.Debug("Request canceled while waiting for OIDC initialization") t.logger.Debug("Request canceled while waiting for OIDC initialization")
t.sendErrorResponse(rw, req, "Request canceled", http.StatusRequestTimeout) t.sendErrorResponse(rw, req, "Request canceled", http.StatusRequestTimeout)
return return
case <-initTimer.C: case <-initTimer.C:
atomic.AddInt32(&t.initWaiters, -1)
t.logger.Error("Timeout waiting for OIDC initialization") t.logger.Error("Timeout waiting for OIDC initialization")
t.sendErrorResponse(rw, req, "Timeout waiting for OIDC provider initialization - please try again later", http.StatusServiceUnavailable) t.sendErrorResponse(rw, req, "Timeout waiting for OIDC provider initialization - please try again later", http.StatusServiceUnavailable)
return return
+10 -1
View File
@@ -116,6 +116,10 @@ const (
defaultCookiePrefix = "_oidc_raczylo_" 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 ( const (
// maxCookieSize is the maximum raw data size per chunk before securecookie encoding. // 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 // service). Keep it unless it is a strict parent of the serving
// host, which would scope the token-bearing cookie to sibling // host, which would scope the token-bearing cookie to sibling
// subdomains — then fall back to the serving host. // 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) { if !strings.HasSuffix(host, "."+fh) {
host = fh host = fh
} }
@@ -1024,6 +1031,7 @@ func (sm *SessionManager) CleanupOldCookies(w http.ResponseWriter, r *http.Reque
} }
processedCookies := make(map[string]bool) processedCookies := make(map[string]bool)
deletedCookies := 0
for _, cookie := range cookies { for _, cookie := range cookies {
// Check if cookie belongs to this middleware instance using the configured prefix // 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 shouldCleanup := currentDomain != "" && !sm.cleanupDone
sm.cleanupMutex.RUnlock() sm.cleanupMutex.RUnlock()
if shouldCleanup { if shouldCleanup && deletedCookies < maxCleanupCookies {
deletedCookies++
for _, domain := range domainsToClean { for _, domain := range domainsToClean {
if domain == currentDomain || domain == "."+currentDomain || "."+domain == currentDomain { if domain == currentDomain || domain == "."+currentDomain || "."+domain == currentDomain {
continue continue
+4
View File
@@ -111,6 +111,9 @@ type TraefikOidc struct {
// recovery attempt. Stored atomically so the hot ServeHTTP path can // recovery attempt. Stored atomically so the hot ServeHTTP path can
// throttle retries without acquiring metadataRetryMutex on every request. // throttle retries without acquiring metadataRetryMutex on every request.
lastMetadataRetryNano int64 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 // firstRequestStarted is 0 until the very first non-health request fires
// the background-task bootstrap; then it flips to 1 via CAS. Replaces the // the background-task bootstrap; then it flips to 1 via CAS. Replaces the
// firstRequestMutex + firstRequestReceived combo which previously took // firstRequestMutex + firstRequestReceived combo which previously took
@@ -224,6 +227,7 @@ type TraefikOidc struct {
dcrMu sync.Mutex dcrMu sync.Mutex
shutdownOnce sync.Once shutdownOnce sync.Once
sessionInvalidationCache CacheInterface sessionInvalidationCache CacheInterface
frontchannelMarkers markerQuota
refreshResultCache CacheInterface refreshResultCache CacheInterface
minimalHeaders bool minimalHeaders bool
stripAuthCookies bool stripAuthCookies bool