From d5fddebeaa7415cd90794f875252476362f3c6ac Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 17:06:23 +0100 Subject: [PATCH] R68: cache expiry TOCTOU, logout open-redirect, session max-age --- helpers.go | 28 +++++----- internal/cache/backends/memory.go | 6 ++- internal/cache/backends/memory_shard.go | 18 +++++++ internal/cache/backends/memory_test.go | 55 ++++++++++++++++++++ main_servehttp_test.go | 69 +++++++++++++++++++++++++ main_test.go | 2 +- review_r68_created_at_test.go | 47 +++++++++++++++++ session.go | 17 ++++-- 8 files changed, 223 insertions(+), 19 deletions(-) create mode 100644 review_r68_created_at_test.go diff --git a/helpers.go b/helpers.go index b5c045a..c19b235 100644 --- a/helpers.go +++ b/helpers.go @@ -13,8 +13,6 @@ import ( "net/url" "strings" "time" - - "github.com/lukaszraczylo/traefikoidc/internal/utils" ) // newUUIDv4 returns an RFC 4122 v4 UUID string (e.g. @@ -399,24 +397,30 @@ func (t *TraefikOidc) handleLogout(rw http.ResponseWriter, req *http.Request) { } } - host := utils.DetermineHost(req) - scheme := utils.DetermineScheme(req, t.forceHTTPS) - baseURL := fmt.Sprintf("%s://%s", scheme, host) - postLogoutRedirectURI := t.postLogoutRedirectURI // localRedirect is used when there is no provider end-session endpoint and // the plugin redirects the browser itself. It must never be an absolute URL // derived from the request host (X-Forwarded-Host is client-controllable and // would be an open redirect); use a host-relative path, or the operator's // own configured absolute URL, instead. + // + // The post_logout_redirect_uri we hand to the IdP has the same constraint: + // deriving it from baseURL (scheme://client-controlled host) lets an + // attacker's X-Forwarded-Host steer the browser to an arbitrary origin + // after logout (open redirect through the IdP). So we only emit it when + // the operator configured an explicit absolute URL (a trusted origin); + // otherwise BuildLogoutURL omits the parameter entirely. localRedirect := "/" - if postLogoutRedirectURI == "" { - postLogoutRedirectURI = fmt.Sprintf("%s/", baseURL) - } else if !strings.HasPrefix(postLogoutRedirectURI, "http") { - localRedirect = normalizeLogoutPath(postLogoutRedirectURI) - postLogoutRedirectURI = fmt.Sprintf("%s%s", baseURL, postLogoutRedirectURI) - } else { + if strings.HasPrefix(postLogoutRedirectURI, "http") { localRedirect = postLogoutRedirectURI + } else if postLogoutRedirectURI == "" { + // Not configured: no trusted origin available — do not send a + // host-derived post_logout_redirect_uri to the IdP. + } else { + // Relative operator-configured path is trusted for the plugin's own + // redirect but still has no trusted origin; omit the IdP param. + localRedirect = normalizeLogoutPath(postLogoutRedirectURI) + postLogoutRedirectURI = "" } // Read endSessionURL with RLock diff --git a/internal/cache/backends/memory.go b/internal/cache/backends/memory.go index e91a398..61c0547 100644 --- a/internal/cache/backends/memory.go +++ b/internal/cache/backends/memory.go @@ -179,8 +179,10 @@ func (m *MemoryCacheBackend) Get(ctx context.Context, key string) (interface{}, value, exists, expired := shard.get(key) if expired { - // Clean up expired item - shard.delete(key) + // Clean up expired item. Use deleteIfExpired so a concurrent Set + // that refreshed this key between get() and here is not deleted + // (which would drop a freshly-written value = lost update). + shard.deleteIfExpired(key) m.misses.Add(1) return nil, ErrCacheMiss } diff --git a/internal/cache/backends/memory_shard.go b/internal/cache/backends/memory_shard.go index 183cb04..8ee9301 100644 --- a/internal/cache/backends/memory_shard.go +++ b/internal/cache/backends/memory_shard.go @@ -117,6 +117,24 @@ func (s *cacheShard) delete(key string) bool { return true } +// deleteIfExpired removes a key only if the entry still present is expired. +// It is safe to call after observing an expired item via get(): a +// concurrent set() that refreshed the key between the read and this call +// leaves the fresh value intact. Unlike delete, it avoids deleting a +// just-written value (a lost update in Get's expiry cleanup). +func (s *cacheShard) deleteIfExpired(key string) bool { + s.mu.Lock() + defer s.mu.Unlock() + + item, exists := s.items[key] + if !exists || !item.isExpired() { + return false + } + + s.deleteItemLocked(item) + return true +} + // exists checks if a key exists (and is not expired) func (s *cacheShard) exists(key string) bool { s.mu.RLock() diff --git a/internal/cache/backends/memory_test.go b/internal/cache/backends/memory_test.go index 96abe20..f34ad35 100644 --- a/internal/cache/backends/memory_test.go +++ b/internal/cache/backends/memory_test.go @@ -781,3 +781,58 @@ func TestMatchPattern(t *testing.T) { }) } } + +// TestMemoryShard_DeleteIfExpiredPreservesConcurrentRefresh regresses a +// TOCTOU in MemoryCacheBackend.Get's expired-item cleanup. Get reads an +// item, sees it expired (shard.get), then was deleting it with an +// unconditional shard.delete in a separate critical section. If a +// concurrent Set refreshed the key between the read and the delete, the +// delete removed the freshly-written value (a lost update — the key +// vanished right after it was refreshed). Get now uses deleteIfExpired, +// which deletes only if the entry is still expired, leaving a concurrent +// refresh intact. This test models that interleaving deterministically. +func TestMemoryShard_DeleteIfExpiredPreservesConcurrentRefresh(t *testing.T) { + s := newCacheShard(0, 0) + const key = "k" + + // Get's first phase: observe an already-expired entry. + s.set(key, "old", time.Now().Add(-time.Second), 1) + if _, _, expired := s.get(key); !expired { + t.Fatal("precondition: seeded item must be expired") + } + + // A concurrent refresh lands a fresh value inside the get->delete window. + s.set(key, "fresh", time.Now().Add(time.Hour), 1) + + // Get's cleanup must not remove the fresh value. + if s.deleteIfExpired(key) { + t.Fatal("deleteIfExpired removed a value a concurrent Set just wrote") + } + if v, _, _ := s.get(key); v != "fresh" { + t.Fatalf("fresh value was lost by expiry cleanup, got %v", v) + } +} + +// TestMemoryShard_DeleteClobbersConcurrentRefresh documents the failure +// mode the fix addresses: the old unconditional delete removes the value +// a concurrent Set just wrote under the same interleaving. If +// deleteIfExpired ever regresses to unconditional deletion, the sibling +// test above fails. +func TestMemoryShard_DeleteClobbersConcurrentRefresh(t *testing.T) { + s := newCacheShard(0, 0) + const key = "k" + + s.set(key, "old", time.Now().Add(-time.Second), 1) + if _, _, expired := s.get(key); !expired { + t.Fatal("precondition: seeded item must be expired") + } + s.set(key, "fresh", time.Now().Add(time.Hour), 1) + + // Old cleanup (delete) removes the just-written value: lost update. + if !s.delete(key) { + t.Log("old delete found no item") + } + if v, _, _ := s.get(key); v == "fresh" { + t.Log("note: value happened to survive, but delete is unconditional") + } +} diff --git a/main_servehttp_test.go b/main_servehttp_test.go index 52cc677..8cb6907 100644 --- a/main_servehttp_test.go +++ b/main_servehttp_test.go @@ -5,6 +5,8 @@ import ( "crypto/rsa" "net/http" "net/http/httptest" + "net/url" + "strings" "testing" "time" ) @@ -1317,3 +1319,70 @@ func TestServeHTTP_EventStream_AppliesRolesGate(t *testing.T) { } }) } + +// TestLogout_NoOpenRedirectViaIdP regresses a post-logout open redirect: +// handleLogout built the IdP's post_logout_redirect_uri from the +// client-controllable request host (X-Forwarded-Host), so an attacker +// could steer the browser to an arbitrary origin after logout. With no +// configured postLogoutRedirectURI the parameter must be omitted, never +// host-derived. +func TestLogout_NoOpenRedirectViaIdP(t *testing.T) { + sessionManager := createTestSessionManager(t) + + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generate key: %v", err) + } + idToken, err := createTestJWT(key, "RS256", "test-key-id", map[string]interface{}{"sub": "user@example.com"}) + if err != nil { + t.Fatalf("craft id token: %v", err) + } + + oidc := &TraefikOidc{ + logger: NewLogger("error"), + sessionManager: sessionManager, + endSessionURL: "https://idp.example.com/session/end", + tokenBlacklist: NewCache(), + tokenCache: NewTokenCache(), + } + + req := httptest.NewRequest("GET", "/logout", nil) + req.Header.Set("X-Forwarded-Host", "attacker.example") + + session, err := sessionManager.GetSession(req) + if err != nil { + t.Fatalf("create session: %v", err) + } + session.SetUserIdentifier("user@example.com") + if err := session.SetAuthenticated(true); err != nil { + t.Fatalf("authenticate: %v", err) + } + session.SetIDToken(idToken) + rec := httptest.NewRecorder() + if err := session.Save(req, rec); err != nil { + t.Fatalf("save session: %v", err) + } + for _, c := range rec.Result().Cookies() { + req.AddCookie(c) + } + + rw := httptest.NewRecorder() + oidc.handleLogout(rw, req) + + loc := rw.Header().Get("Location") + if loc == "" { + t.Fatalf("expected logout redirect to the IdP end-session URL") + } + u, err := url.Parse(loc) + if err != nil { + t.Fatalf("parse redirect %q: %v", loc, err) + } + plr := u.Query().Get("post_logout_redirect_uri") + if plr == "" { + // Correct: no configured target, param omitted. + return + } + if strings.Contains(plr, "attacker.example") { + t.Fatalf("open redirect via IdP: post_logout_redirect_uri=%q came from client host", plr) + } +} diff --git a/main_test.go b/main_test.go index aeb2b5f..dab5dec 100644 --- a/main_test.go +++ b/main_test.go @@ -1862,7 +1862,7 @@ func TestHandleLogout(t *testing.T) { }, endSessionURL: "https://provider/end-session", expectedStatus: http.StatusFound, - expectedURL: "https://provider/end-session?id_token_hint=" + url.QueryEscape(ValidIDToken) + "&post_logout_redirect_uri=http%3A%2F%2Fexample.com%2F", + expectedURL: "https://provider/end-session?id_token_hint=" + url.QueryEscape(ValidIDToken), host: "test-host", }, { diff --git a/review_r68_created_at_test.go b/review_r68_created_at_test.go new file mode 100644 index 0000000..2c97617 --- /dev/null +++ b/review_r68_created_at_test.go @@ -0,0 +1,47 @@ +package traefikoidc + +import ( + "net/http/httptest" + "testing" + "time" +) + +// TestSession_RefreshDoesNotResetCreatedAt regresses the absolute +// session-max-age being made sliding: SetAuthenticated(true) was called on +// every token refresh and unconditionally reset created_at to now, so an +// active (or captured-but-refreshable) session never reached the +// configured maximum age. The creation anchor must be set only on the +// unauthenticated->authenticated transition, not on refresh of an +// already-authenticated session. +func TestSession_RefreshDoesNotResetCreatedAt(t *testing.T) { + sessionManager := createTestSessionManager(t) + req := httptest.NewRequest("GET", "/", nil) + + session, err := sessionManager.GetSession(req) + if err != nil { + t.Fatalf("create session: %v", err) + } + session.SetUserIdentifier("user@example.com") + if err := session.SetAuthenticated(true); err != nil { + t.Fatalf("initial login: %v", err) + } + + // Simulate a session that is still within its maximum age but was + // created a while ago (e.g. 2h ago, well under the 24h default + // maximum). getAuthenticatedUnsafe still considers it valid. + oldCreated := time.Now().Add(-2 * time.Hour).Unix() + session.mainSession.Values["created_at"] = oldCreated + + // This is what the refresh path does on success. + if err := session.SetAuthenticated(true); err != nil { + t.Fatalf("refresh SetAuthenticated: %v", err) + } + + got, ok := session.mainSession.Values["created_at"].(int64) + if !ok { + t.Fatalf("created_at missing or not int64") + } + if got != oldCreated { + t.Fatalf("refresh advanced created_at: got %d, want %d (old anchor must be preserved)", got, oldCreated) + } +} diff --git a/session.go b/session.go index b23fcfd..7a9b36a 100644 --- a/session.go +++ b/session.go @@ -1736,11 +1736,20 @@ func (sd *SessionData) SetAuthenticated(value bool) error { changed = true } sd.mainSession.ID = id - newCreationTime := time.Now().Unix() - if oldTime, ok := sd.mainSession.Values["created_at"].(int64); !ok || oldTime != newCreationTime { - changed = true + // Anchor the creation timestamp only when first authenticating + // (transition from unauthenticated). Re-asserting an already- + // authenticated session — e.g. on every token refresh — must not + // advance the absolute session-max-age clock, otherwise an active + // client's session (and a captured cookie that keeps refreshing) + // never reaches the configured maximum (sliding window defeats the + // absolute timeout). + if currentAuth != value { + newCreationTime := time.Now().Unix() + if oldTime, ok := sd.mainSession.Values["created_at"].(int64); !ok || oldTime != newCreationTime { + changed = true + } + sd.mainSession.Values["created_at"] = newCreationTime } - sd.mainSession.Values["created_at"] = newCreationTime if oldAuth, ok := sd.mainSession.Values["authenticated"].(bool); !ok || oldAuth != value { changed = true }