From 0e57c0cad392da352b39716bcd737bd434f4c861 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Thu, 13 Aug 2026 18:07:32 +0100 Subject: [PATCH] R77: bearer allowedUsers gate, session inUse CAS --- middleware.go | 12 ++++++ review_r77_test.go | 86 +++++++++++++++++++++++++++++++++++++++ session.go | 26 +++++++----- session_behaviour_test.go | 4 +- token_manager.go | 2 +- 5 files changed, 117 insertions(+), 13 deletions(-) create mode 100644 review_r77_test.go diff --git a/middleware.go b/middleware.go index 0943713..065012a 100644 --- a/middleware.go +++ b/middleware.go @@ -831,6 +831,18 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques } } + // Bearer (M2M) path: enforce allowedUsers / allowedUserDomains the same + // way every cookie-path entry point does. The bearer path reaches here + // via buildPrincipalFromBearerToken with no earlier isAllowedUser check; + // without this, a valid bearer whose subject is absent from the + // allowlist would still be forwarded, making the restriction a no-op. + if p.Source == sourceBearer && !t.isAllowedUser(p.Identifier) { + t.logger.Infof("Bearer user %s is not in allowed users or domains", p.Identifier) + errorMsg := fmt.Sprintf("Access denied: User not allowed. To log out, visit: %s", t.logoutURLPath) + t.sendErrorResponse(rw, req, errorMsg, http.StatusForbidden) + return + } + if len(t.allowedRolesAndGroups) > 0 { allowed := false for _, roleOrGroup := range append(groups, roles...) { diff --git a/review_r77_test.go b/review_r77_test.go new file mode 100644 index 0000000..f203685 --- /dev/null +++ b/review_r77_test.go @@ -0,0 +1,86 @@ +package traefikoidc + +import ( + "net/http" + "net/http/httptest" + "sync" + "sync/atomic" + "testing" +) + +// TestServeHTTP_Bearer_AllowedUsersGate guards the bearer (M2M) path against +// bypassing the allowedUsers / allowedUserDomains gate. Every cookie-path +// entry point enforces isAllowedUser; the bearer path reaches +// forwardAuthorized with no earlier check, so a valid bearer whose subject +// is absent from the allowlist must be rejected with 403 rather than be +// forwarded (which would make the restriction a no-op on the bearer path). +func TestServeHTTP_Bearer_AllowedUsersGate(t *testing.T) { + t.Parallel() + cases := []struct { + name string + sub string + want int + }{ + {name: "subject allowed", sub: "service-account-1", want: http.StatusOK}, + {name: "subject not allowed", sub: "other-sa", want: http.StatusForbidden}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + oidc := makeBearerOIDC(t, next) + oidc.allowedUsers = map[string]struct{}{"service-account-1": {}} + claims := defaultBearerClaims() + claims["sub"] = tc.sub + token := makeBearerJWT(t, defaultBearerHeader(), claims) + seedVerified(t, oidc, token, claims) + + req := httptest.NewRequest("GET", "/api/work", nil) + req.Header.Set("Authorization", "Bearer "+token) + rw := httptest.NewRecorder() + oidc.ServeHTTP(rw, req) + if rw.Code != tc.want { + t.Fatalf("status=%d, want %d", rw.Code, tc.want) + } + }) + } +} + +// TestSessionPool_ReturnToPool_ExactlyOnce is a regression for the check- +// then-put TOCTOU in returnToPoolSafely. The old code read sd.inUse, +// decided to return it, then wrote inUse=false and Put the session back — +// so two concurrent returns (request path + a background goroutine) could +// both decide to return the SAME SessionData, putting it into the pool +// twice and double-decrementing activeSessions. The fix uses a single +// atomic CompareAndSwap so exactly one caller claims and returns the +// object. The observable contract: N concurrent returns of one in-use +// session decrement activeSessions by exactly 1. +func TestSessionPool_ReturnToPool_ExactlyOnce(t *testing.T) { + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + }) + oidc := makeBearerOIDC(t, next) + sm := oidc.sessionManager + + sd, err := sm.GetSession(httptest.NewRequest("GET", "/", nil)) + if err != nil { + t.Fatalf("GetSession: %v", err) + } + before := atomic.LoadInt64(&sm.activeSessions) + + var wg sync.WaitGroup + for i := 0; i < 16; i++ { + wg.Add(1) + go func() { + defer wg.Done() + sd.returnToPoolSafely() + }() + } + wg.Wait() + + got := before - atomic.LoadInt64(&sm.activeSessions) + if got != 1 { + t.Fatalf("returnToPoolSafely returned session %d times, want exactly 1 (double-return would corrupt the pool)", got) + } +} diff --git a/session.go b/session.go index 0c645f7..bd7fce8 100644 --- a/session.go +++ b/session.go @@ -509,7 +509,7 @@ func NewSessionManager(encryptionKey string, forceHTTPS bool, cookieDomain strin refreshMutex: sync.Mutex{}, sessionMutex: sync.RWMutex{}, dirty: false, - inUse: false, + inUse: atomic.Bool{}, } sd.Reset() return sd @@ -654,7 +654,7 @@ func (sm *SessionManager) cleanupSessionPool() { if poolSession := sm.sessionPool.Get(); poolSession != nil { sessionData, ok := poolSession.(*SessionData) - if ok && sessionData != nil && !sessionData.inUse { + if ok && sessionData != nil && !sessionData.inUse.Load() { sessionData.Reset() cleaned++ } @@ -724,7 +724,7 @@ func (sm *SessionManager) PeriodicChunkCleanup() { for i := 0; i < 10; i++ { if poolSession := sm.sessionPool.Get(); poolSession != nil { sessionData, ok := poolSession.(*SessionData) - if ok && sessionData != nil && !sessionData.inUse { + if ok && sessionData != nil && !sessionData.inUse.Load() { sessionData.Reset() poolCleaned++ } @@ -1065,13 +1065,13 @@ func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) { atomic.AddInt64(&sm.poolHits, 1) atomic.AddInt64(&sm.activeSessions, 1) - sessionData.inUse = true + sessionData.inUse.Store(true) sessionData.request = r sessionData.dirty = false handleError := func(err error, message string) (*SessionData, error) { if sessionData != nil { - sessionData.inUse = false + sessionData.inUse.Store(false) sessionData.Reset() sm.sessionPool.Put(sessionData) atomic.AddInt64(&sm.activeSessions, -1) @@ -1305,7 +1305,7 @@ type SessionData struct { dirty bool - inUse bool + inUse atomic.Bool // cachedClaimsToken is the ID token string whose claims were last parsed and // cached. A lazy, per-request cache to avoid re-parsing the JWT on every @@ -1681,8 +1681,14 @@ func (sd *SessionData) Clear(r *http.Request, w http.ResponseWriter) error { // It ensures the session is marked as not in use and properly reset before pooling. func (sd *SessionData) returnToPoolSafely() { if sd != nil && sd.manager != nil { - if sd.inUse { - sd.inUse = false + // Exactly-once return: only the goroutine that flips inUse with a + // compare-and-swap claims the object. Without the CAS, two + // concurrent calls (e.g. request path + a goroutine cleanup) can + // both observe inUse and both Put the same SessionData back into + // the pool, leaving it present twice — which later yields two + // concurrent holders of one session (double side-effects) plus an + // extra activeSessions decrement. + if sd.inUse.CompareAndSwap(true, false) { sd.Reset() sd.manager.sessionPool.Put(sd) atomic.AddInt64(&sd.manager.activeSessions, -1) @@ -1844,7 +1850,7 @@ func (sd *SessionData) Reset() { } sd.dirty = false - sd.inUse = false + sd.inUse.Store(false) sd.request = nil sd.useCombinedStorage = true // Reset to use combined storage by default @@ -1864,7 +1870,7 @@ func (sd *SessionData) Reset() { // It only returns the session if it's not currently in use. func (sd *SessionData) ReturnToPool() { if sd != nil && sd.manager != nil { - if !sd.inUse { + if !sd.inUse.Load() { sd.Reset() sd.manager.sessionPool.Put(sd) atomic.AddInt64(&sd.manager.activeSessions, -1) diff --git a/session_behaviour_test.go b/session_behaviour_test.go index cb146d1..cf09dfa 100644 --- a/session_behaviour_test.go +++ b/session_behaviour_test.go @@ -610,13 +610,13 @@ func (s *SessionBehaviourSuite) TestSessionData_ReturnToPool() { s.Require().NoError(err) // Initially in use - s.True(session.inUse) + s.True(session.inUse.Load()) // Return to pool safely session.returnToPoolSafely() // Should no longer be in use - s.False(session.inUse) + s.False(session.inUse.Load()) } // TestTokenCompression tests token compression functionality diff --git a/token_manager.go b/token_manager.go index ce16e0b..afc0ff4 100644 --- a/token_manager.go +++ b/token_manager.go @@ -444,7 +444,7 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se t.logger.Debug("Attempting to refresh token (mutex acquired)") - if !session.inUse { + if !session.inUse.Load() { t.logger.Debug("refreshToken aborted: Session no longer in use") return false }