diff --git a/logout.go b/logout.go index b7aa6fb..7a29b51 100644 --- a/logout.go +++ b/logout.go @@ -256,6 +256,20 @@ func (t *TraefikOidc) validateLogoutToken(tokenString string) (*LogoutTokenClaim return nil, fmt.Errorf("logout token must contain either sid or sub claim") } + // OIDC Back-Channel Logout 1.0 §2.5: the RP MUST record the logout + // token's jti and reject any replayed token with the same jti. + // Without this a captured token could be re-applied to a session the + // user re-established after the genuine logout. Only enforced when a + // cache is available; the jti is stored under its own namespace so it + // never collides with sid/sub invalidation entries. + if claims.JTI != "" && t.sessionInvalidationCache != nil { + key := t.buildSessionInvalidationKey("jti", claims.JTI) + if _, found := t.sessionInvalidationCache.Get(key); found { + return nil, fmt.Errorf("logout token replay: jti %s already processed", claims.JTI) + } + t.sessionInvalidationCache.Set(key, claims.IssuedAt, sessionInvalidationTTL) + } + return claims, nil } diff --git a/middleware.go b/middleware.go index dbc6b4e..0ec8851 100644 --- a/middleware.go +++ b/middleware.go @@ -395,6 +395,15 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { authenticated, needsRefresh, expired := t.isUserAuthenticatedRS(rs) if expired { + // AJAX / sub-resource requests must not follow a 302 into the IdP: + // parallel loads would each overwrite the shared session's CSRF/nonce + // mid-navigation (issue #129), the same reason the sibling branches + // below return 401 instead of redirecting. + if t.isAjaxRequest(req) || t.isNonNavigationRequest(req) { + t.logger.Debug("Session token expired on AJAX/sub-resource request, sending 401") + t.sendErrorResponse(rw, req, "Authentication required", http.StatusUnauthorized) + return + } t.logger.Debug("Session token is definitively expired or invalid, initiating re-auth") t.handleExpiredToken(rw, req, session, redirectURL) return @@ -738,6 +747,11 @@ func (t *TraefikOidc) processAuthorizedRequest(rw http.ResponseWriter, req *http // abusive output. Matches the input-validation default header cap (8KB). const headerTemplateMaxLen = 8192 +// noValueSentinel is Go's text/template placeholder for a missing key in an +// interface-valued map. It is stripped from rendered header values so an +// optional claim the provider does not emit does not leak "". +const noValueSentinel = "" + // headerClaimMaxLen returns the maximum accepted length for a claim-derived // header value (principal identifier, group, role). Reuses the operator- // configured identifier cap (default 256) so a single setting governs both @@ -874,6 +888,14 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques continue } headerValue := buf.String() + // Go's text/template renders a missing key in an interface-valued + // map as the literal "" even with missingkey=zero + // (zero only applies to typed keys). Replace that sentinel so an + // optional claim the provider did not emit (e.g. email) does not + // leak "" into the downstream header. + if strings.Contains(headerValue, noValueSentinel) { + headerValue = strings.ReplaceAll(headerValue, noValueSentinel, "") + } // Sanitize the rendered output: template inputs are claim-derived // and attacker-influenceable, so reject control chars (header // injection), bidi-override runes, the , ; = delimiters, and an diff --git a/review_r73_test.go b/review_r73_test.go new file mode 100644 index 0000000..b7535e4 --- /dev/null +++ b/review_r73_test.go @@ -0,0 +1,196 @@ +package traefikoidc + +import ( + "context" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "text/template" + "time" +) + +// ---- jti replay protection (OIDC Back-Channel Logout 1.0 §2.5) ---- + +type replayJWKCache struct{ jwks *JWKSet } + +func (r *replayJWKCache) GetJWKS(ctx context.Context, u string, c *http.Client) (*JWKSet, error) { + return r.jwks, nil +} +func (r *replayJWKCache) GetPublicKey(ctx context.Context, u, kid string, c *http.Client) (crypto.PublicKey, error) { + for i := range r.jwks.Keys { + if r.jwks.Keys[i].Kid == kid { + switch r.jwks.Keys[i].Kty { + case "EC": + return r.jwks.Keys[i].ToECDSAPublicKey() + case "RSA": + return r.jwks.Keys[i].ToRSAPublicKey() + } + } + } + return nil, fmt.Errorf("missing key %s", kid) +} +func (r *replayJWKCache) Clear() {} +func (r *replayJWKCache) Cleanup() {} +func (r *replayJWKCache) Close() {} + +// TestBackchannelLogout_RejectsReplayedJTI regresses the missing jti +// replay protection in validateLogoutToken: per OIDC BCL §2.5 the same +// logout token must be rejected on a second submission. Before the fix a +// captured token could be re-applied within its iat window and force-logout +// a session a user re-established after the genuine logout. +func TestBackchannelLogout_RejectsReplayedJTI(t *testing.T) { + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("generate key: %v", err) + } + x := base64.RawURLEncoding.EncodeToString(priv.PublicKey.X.Bytes()) + y := base64.RawURLEncoding.EncodeToString(priv.PublicKey.Y.Bytes()) + + oidc := &TraefikOidc{ + logger: NewLogger("error"), + enableBackchannelLogout: true, + backchannelLogoutPath: "/backchannel-logout", + sessionInvalidationCache: &mockCacheInterface{data: map[string]interface{}{}}, + clientID: "test-client", + issuerURL: "https://provider.example.com", + jwkCache: &replayJWKCache{jwks: &JWKSet{Keys: []JWK{{ + Kty: "EC", Crv: "P-256", X: x, Y: y, Kid: "test-key-1", Use: "sig", Alg: "ES256", + }}}}, + } + + token := func() string { + h, _ := json.Marshal(map[string]interface{}{"alg": "ES256", "typ": "logout+jwt", "kid": "test-key-1"}) + hb := base64.RawURLEncoding.EncodeToString(h) + c, _ := json.Marshal(map[string]interface{}{ + "iss": "https://provider.example.com", + "aud": "test-client", + "iat": time.Now().Unix(), + "jti": "unique-jti-123", + "events": map[string]interface{}{ + "http://schemas.openid.net/event/backchannel-logout": map[string]interface{}{}, + }, + "sid": "session-to-logout", + }) + cb := base64.RawURLEncoding.EncodeToString(c) + hash := sha256.Sum256([]byte(hb + "." + cb)) + r, s, _ := ecdsa.Sign(rand.Reader, priv, hash[:]) + sig := make([]byte, 64) + rb, sb := r.Bytes(), s.Bytes() + copy(sig[32-len(rb):32], rb) + copy(sig[64-len(sb):], sb) + return hb + "." + cb + "." + base64.RawURLEncoding.EncodeToString(sig) + } + + post := func() int { + req := httptest.NewRequest(http.MethodPost, "/backchannel-logout", + strings.NewReader("logout_token="+url.QueryEscape(token()))) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rw := httptest.NewRecorder() + oidc.handleBackchannelLogout(rw, req) + return rw.Code + } + + if code := post(); code != http.StatusOK { + t.Fatalf("first logout token accepted? got %d, expected 200", code) + } + if code := post(); code != http.StatusBadRequest { + t.Fatalf("replayed logout token should be rejected (400), got %d", code) + } +} + +// ---- expired-token AJAX guard ---- + +// TestServeHTTP_ExpiredAjaxReturns401 regresses the expired-token branch +// sending an AJAX/sub-resource request down a 302 to the IdP. It must +// answer 401 like the sibling branches so sub-resource loads to not +// overwrite the in-flight session's CSRF/nonce. +func TestServeHTTP_ExpiredAjaxReturns401(t *testing.T) { + sessionManager := createTestSessionManager(t) + oidc := &TraefikOidc{ + next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK) }), + logger: NewLogger("error"), + initComplete: make(chan struct{}), + sessionManager: sessionManager, + firstRequestStarted: 1, + metadataRefreshStartedAtomic: 1, + issuerURL: "https://provider.example.com", + redirURLPath: "/callback", + logoutURLPath: "/logout", + clientID: "test-client", + audience: "test-client", + // Access-token verification reports "token has expired" and the + // session has no refresh or ID token -> isUserAuthenticatedRS sets + // expired=true, reaching the branch under test. + tokenVerifier: &EnhancedMockTokenVerifier{VerifyFunc: func(token string) error { return errors.New("token has expired") }}, + } + close(oidc.initComplete) + + req := httptest.NewRequest(http.MethodGet, "/api/data", nil) + req.Header.Set("X-Requested-With", "XMLHttpRequest") + session, err := sessionManager.GetSession(req) + if err != nil { + t.Fatalf("get session: %v", err) + } + _ = session.SetAuthenticated(true) + session.SetAccessToken("aaa.bbb.ccc") + 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.ServeHTTP(rw, req) + + if rw.Code != http.StatusUnauthorized { + t.Fatalf("expired-token AJAX request: expected 401, got %d (Location=%q)", rw.Code, rw.Header().Get("Location")) + } +} + +// ---- missing claim renders no '' ---- + +func mustTemplate(src string) *template.Template { + tmpl, err := template.New("h").Parse(src) + if err != nil { + panic(err) + } + return tmpl +} + +// TestHeaderTemplate_MissingClaimNoNoValue regresses optional claims +// rendering literal "" into a downstream header when the +// provider does not emit them (e.g. email, given_name). +func TestHeaderTemplate_MissingClaimNoNoValue(t *testing.T) { + oidc := &TraefikOidc{ + next: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}), + logger: NewLogger("error"), + groupClaimName: "groups", + roleClaimName: "roles", + minimalHeaders: true, + headerTemplates: map[string]*template.Template{ + "X-Email": mustTemplate("{{.Claims.email}}"), + }, + } + + req := httptest.NewRequest(http.MethodGet, "/", nil) + rw := httptest.NewRecorder() + p := &principal{Identifier: "user", Claims: map[string]interface{}{"sub": "subject"}} + oidc.forwardAuthorized(rw, req, p) + + if got := req.Header.Get("X-Email"); got != "" { + t.Fatalf("expected empty header for a missing claim, got %q", got) + } +}