diff --git a/auth_flow.go b/auth_flow.go index 108c581..561c800 100644 --- a/auth_flow.go +++ b/auth_flow.go @@ -1,6 +1,7 @@ package traefikoidc import ( + "errors" "fmt" "net/http" "strings" @@ -14,6 +15,16 @@ func (t *TraefikOidc) validateRedirectCount(session *SessionData, rw http.Respon if redirectCount >= maxRedirects { t.logger.Errorf("Maximum redirect limit (%d) exceeded, possible redirect loop detected", maxRedirects) session.ResetRedirectCount() + // Persist the reset so the NEXT request starts at 0 and can + // attempt auth again. Without Save, the reset only touched the + // in-memory session value (dirty flag set but never flushed): the + // cookie still carried the old count, so every subsequent + // request re-observed redirect_count==max and the user was + // hard-locked on 508 for the whole session with no recovery + // (R123). + if err := session.Save(req, rw); err != nil { + t.logger.Errorf("Failed to save session after redirect reset: %v", err) + } t.sendErrorResponse(rw, req, "Authentication failed: Too many redirects", http.StatusLoopDetected) return fmt.Errorf("redirect limit exceeded") } @@ -68,9 +79,34 @@ func (t *TraefikOidc) prepareSessionForAuthentication(session *SessionData, csrf // - req: The HTTP request initiating authentication. // - session: The session data to prepare for authentication. // - redirectURL: The pre-calculated callback URL (redirect_uri) for this middleware instance. +// +// rateLimitPerSourceAuth applies the (optional) per-external-source auth +// throttle at an OIDC gate. Returns true if the request was already +// answered with 429 and the caller must return. No-op when the limiter is +// disabled (PerSourceLoginRateLimit <= 0) or the source is internal. +func (t *TraefikOidc) rateLimitPerSourceAuth(rw http.ResponseWriter, req *http.Request) bool { + if t.perSourceLimiter == nil { + return false + } + if t.perSourceLimiter.allow(req) { + return false + } + rw.Header().Set("Retry-After", "1") + t.sendErrorResponse(rw, req, "Too many authentication attempts, please try again shortly", http.StatusTooManyRequests) + return true +} + func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req *http.Request, session *SessionData, redirectURL string) { t.logger.Debugf("Initiating new OIDC authentication flow for request: %s", req.URL.RequestURI()) + // Per-external-source throttle on login initiation: a single host + // flooding the route with auth starts shouldn't be able to spin up an + // unbounded number of IdP round-trips or wear the shared token-origin + // bucket down for everyone (R185). + if t.rateLimitPerSourceAuth(rw, req) { + return + } + // Check and handle redirect limits if err := t.validateRedirectCount(session, rw, req); err != nil { return @@ -97,9 +133,42 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req return } + // Build the provider authorize URL BEFORE mutating the session. A URL- + // build failure must not clear a valid existing session, and must not + // emit an empty-Location redirect (browsers reload the current page, + // re-entering this flow and bumping the redirect count until the hard + // 508). Fail closed with an explicit status instead (R142). + authURL := t.buildAuthURL(redirectURL, csrfToken, nonce, codeChallenge) + if authURL == "" { + t.logger.Errorf("Failed to build provider authorize URL; aborting login flow") + http.Error(rw, "Failed to build provider authorize URL", http.StatusInternalServerError) + return + } + + // When dynamic client registration is enabled but has not yet produced a + // client_id (registration pending or failed), forging an authorize URL + // with client_id="" would send the user to the provider on a broken + // flow. Fail with a clear status instead of redirecting. This guard + // must run BEFORE prepareSessionForAuthentication re-seeds the session: + // re-seeding (and the save that follows) would otherwise mutate and + // persist the session even though we are about to answer 503 with no + // redirect — discarding any existing session state for no reason (R163). + // Snapshot clientID under metadataMu: DCR rewrites it at runtime (R137). + clientID, _, _, _, _ := t.clientCredentials() + if t.dcrConfig != nil && t.dcrConfig.Enabled && clientID == "" { + t.logger.Errorf("OIDC dynamic client registration has not produced a client_id; refusing to redirect to provider") + http.Error(rw, "Identity provider registration pending", http.StatusServiceUnavailable) + return + } + // Clear existing session data and set new authentication state t.prepareSessionForAuthentication(session, csrfToken, nonce, codeVerifier, req.URL.RequestURI()) + // Persist the redirect_uri so the callback reuses the exact value sent + // to the IdP, instead of rebuilding it from the (client-affectable) + // callback request's X-Forwarded-Host (R136). + session.SetRedirectURL(redirectURL) + session.MarkDirty() if err := session.Save(req, rw); err != nil { @@ -111,20 +180,13 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req t.logger.Debugf("Session saved before redirect. CSRF: %s, Nonce: %s", csrfToken, nonce) - authURL := t.buildAuthURL(redirectURL, csrfToken, nonce, codeChallenge) - - // When dynamic client registration is enabled but has not yet produced a - // client_id (registration pending or failed), forging an authorize URL - // with client_id="" would send the user to the provider on a broken - // flow. Fail with a clear status instead of redirecting. - if t.dcrConfig != nil && t.dcrConfig.Enabled && t.clientID == "" { - t.logger.Errorf("OIDC dynamic client registration has not produced a client_id; refusing to redirect to provider") - http.Error(rw, "Identity provider registration pending", http.StatusServiceUnavailable) - return - } - t.logger.Debugf("Redirecting user to OIDC provider: %s", authURL) + // 302s into the IdP must not be heuristically cacheable (RFC 7234): + // a stale, cached redirect could replay the pre-re-authentication URL. + // Every other auth response sets no-store; these redirects were missed + // (R147). + rw.Header().Set("Cache-Control", "no-store") http.Redirect(rw, req, authURL, http.StatusFound) } @@ -135,7 +197,31 @@ func (t *TraefikOidc) defaultInitiateAuthentication(rw http.ResponseWriter, req // - rw: The HTTP response writer. // - req: The callback request containing authorization code and state. // - redirectURL: The fully qualified callback URL (used in the token exchange request). +// +// clearOneTimeAuthState clears the one-time CSRF/nonce/PKCE-verifier after a +// callback that consumed the authorization code but then failed, and persists +// the cleared state. Without it the failed session stays in a pending-auth +// state and a retried callback would re-exchange a fresh code before failing +// again. Mirrors the cleanup on the exchange-failure / empty-access-token +// paths (R134). +func (t *TraefikOidc) clearOneTimeAuthState(session *SessionData, req *http.Request, rw http.ResponseWriter) { + session.SetCSRF("") + session.SetNonce("") + session.SetCodeVerifier("") + if saveErr := session.Save(req, rw); saveErr != nil { + t.logger.Errorf("Failed to save session after callback failure: %v", saveErr) + } +} + func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, redirectURL string) { + // Per-external-source throttle on the authorization-code callback: this + // is where the one-time code exchange (server round-trip + token + // cache write) happens, so a single source can't brute-force + // repeated code exchanges or drown the shared limiter (R185). + if t.rateLimitPerSourceAuth(rw, req) { + return + } + session, err := t.sessionManager.GetSession(req) if err != nil { t.logger.Errorf("Session error during callback: %v", err) @@ -144,15 +230,28 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, } defer session.returnToPoolSafely() - t.logger.Debugf("Handling callback, URL: %s", req.URL.String()) + t.logger.Debugf("Handling callback, URL: %s", redactCallbackURL(req.URL.String())) if req.URL.Query().Get("error") != "" { errorDescription := req.URL.Query().Get("error_description") if errorDescription == "" { errorDescription = req.URL.Query().Get("error") } - t.logger.Errorf("Authentication error from provider during callback: %s - %s", req.URL.Query().Get("error"), errorDescription) - t.sendErrorResponse(rw, req, fmt.Sprintf("Authentication error from provider: %s", errorDescription), http.StatusBadRequest) + // error / error_description are attacker-controlled query params. + // Sanitize before logging and before echoing back to the client: + // an embedded newline would inject a forged line into (especially + // structured) logs, and unbounded length would bloat the log and + // the error response (R145). + sanitizeErr := func(s string) string { + s = strings.NewReplacer("\n", " ", "\r", " ").Replace(s) + const maxErrLen = 512 + if len(s) > maxErrLen { + s = s[:maxErrLen] + } + return s + } + t.logger.Errorf("Authentication error from provider during callback: %s - %s", sanitizeErr(req.URL.Query().Get("error")), sanitizeErr(errorDescription)) + t.sendErrorResponse(rw, req, fmt.Sprintf("Authentication error from provider: %s", sanitizeErr(errorDescription)), http.StatusBadRequest) return } @@ -166,7 +265,7 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, csrfToken := session.GetCSRF() if csrfToken == "" { t.logger.Errorf("CSRF token missing in session during callback. Authenticated: %v, Request URL: %s", - session.GetAuthenticated(), req.URL.String()) + session.GetAuthenticated(), redactCallbackURL(req.URL.String())) cookie, err := req.Cookie("_oidc_raczylo_m") if err != nil { @@ -199,6 +298,13 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, return } + // Use the redirect_uri persisted at initiate time (must match what was + // sent to the IdP's authorize endpoint). Fall back to the rebuilt one + // only for pre-existing sessions that predate R136 (R136). + if stored := session.GetRedirectURL(); stored != "" { + redirectURL = stored + } + tokenResponse, err := t.tokenExchanger.ExchangeCodeForToken(req.Context(), "authorization_code", code, redirectURL, codeVerifier) if err != nil { t.logger.Errorf("Failed to exchange code for token during callback: %v", err) @@ -207,30 +313,29 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, // may still be unconsumed at the provider, so a replayed callback // must re-authenticate rather than revalidate a stale // state/nonce/code_verifier. Mirrors the cleanup on success. - session.SetCSRF("") - session.SetNonce("") - session.SetCodeVerifier("") - if saveErr := session.Save(req, rw); saveErr != nil { - t.logger.Errorf("Failed to save session after callback exchange failure: %v", saveErr) - } + t.clearOneTimeAuthState(session, req, rw) t.sendErrorResponse(rw, req, "Authentication failed: Could not exchange code for token", http.StatusInternalServerError) return } if tokenResponse.AccessToken == "" { t.logger.Errorf("Token endpoint returned no access token during callback") - session.SetCSRF("") - session.SetNonce("") - session.SetCodeVerifier("") - if saveErr := session.Save(req, rw); saveErr != nil { - t.logger.Errorf("Failed to save session after empty access token: %v", saveErr) - } + t.clearOneTimeAuthState(session, req, rw) t.sendErrorResponse(rw, req, "Authentication failed: Token endpoint returned no access token", http.StatusInternalServerError) return } if err = t.verifyToken(tokenResponse.IDToken); err != nil { t.logger.Errorf("Failed to verify id_token during callback: %v", err) + t.clearOneTimeAuthState(session, req, rw) + if errors.Is(err, ErrRateLimitExceeded) { + // Rate-limit exhaustion must be a 429 + Retry-After, not a + // generic 500: a single source's burst currently degrades + // every user's callback to "server error" (R179). + rw.Header().Set("Retry-After", "1") + t.sendErrorResponse(rw, req, "Authentication failed: Too many requests, please retry", http.StatusTooManyRequests) + return + } t.sendErrorResponse(rw, req, "Authentication failed: Could not verify ID token", http.StatusInternalServerError) return } @@ -238,6 +343,7 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, claims, err := t.extractClaimsFunc(tokenResponse.IDToken) if err != nil { t.logger.Errorf("Failed to extract claims during callback: %v", err) + t.clearOneTimeAuthState(session, req, rw) t.sendErrorResponse(rw, req, "Authentication failed: Could not extract claims from token", http.StatusInternalServerError) return } @@ -245,41 +351,57 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, nonceClaim, ok := claims["nonce"].(string) if !ok || nonceClaim == "" { t.logger.Error("Nonce claim missing in id_token during callback") - t.sendErrorResponse(rw, req, "Authentication failed: Nonce missing in token", http.StatusInternalServerError) + t.clearOneTimeAuthState(session, req, rw) + t.sendErrorResponse(rw, req, "Authentication failed: Nonce missing in token", http.StatusBadRequest) return } sessionNonce := session.GetNonce() if sessionNonce == "" { t.logger.Error("Nonce not found in session during callback") - t.sendErrorResponse(rw, req, "Authentication failed: Nonce missing in session", http.StatusInternalServerError) + t.clearOneTimeAuthState(session, req, rw) + t.sendErrorResponse(rw, req, "Authentication failed: Nonce missing in session", http.StatusBadRequest) return } if !constantTimeStringCompare(nonceClaim, sessionNonce) { t.logger.Error("Nonce claim does not match session nonce during callback") - t.sendErrorResponse(rw, req, "Authentication failed: Nonce mismatch", http.StatusInternalServerError) + t.clearOneTimeAuthState(session, req, rw) + t.sendErrorResponse(rw, req, "Authentication failed: Nonce mismatch", http.StatusBadRequest) return } // Extract user identifier from the configured claim (defaults to "email" for backward compatibility) - userIdentifier, _ := claims[t.userIdentifierClaim].(string) + userIdentifier, _ := claimScalarString(claims[t.userIdentifierClaim]) if userIdentifier == "" { // Try "sub" as fallback since it's required by OIDC spec if t.userIdentifierClaim != "sub" { - userIdentifier, _ = claims["sub"].(string) + userIdentifier, _ = claimScalarString(claims["sub"]) } if userIdentifier == "" { t.logger.Errorf("User identifier claim '%s' missing or empty in token during callback", t.userIdentifierClaim) + t.clearOneTimeAuthState(session, req, rw) t.sendErrorResponse(rw, req, "Authentication failed: User identifier missing in token", http.StatusInternalServerError) return } t.logger.Debugf("Configured claim '%s' not found, using 'sub' claim as fallback", t.userIdentifierClaim) } + // Validate user authorization. When the identity is an email, require the + // IdP not to have explicitly flagged it as unverified — an unverified + // address must not satisfy an email/domain allowlist (see + // emailIdentityPermitted). + if !emailIdentityPermitted(userIdentifier, claims) { + t.logger.Errorf("User email not verified during callback, rejecting email-based authorization: %s", userIdentifier) + t.clearOneTimeAuthState(session, req, rw) + t.sendErrorResponse(rw, req, "Authentication failed: Email not verified", http.StatusForbidden) + return + } + // Validate user authorization if !t.isAllowedUser(userIdentifier) { t.logger.Errorf("User not authorized during callback: %s", userIdentifier) + t.clearOneTimeAuthState(session, req, rw) t.sendErrorResponse(rw, req, "Authentication failed: User not authorized", http.StatusForbidden) return } @@ -316,6 +438,8 @@ func (t *TraefikOidc) handleCallback(rw http.ResponseWriter, req *http.Request, } t.logger.Debugf("Callback successful, redirecting to %s", redirectPath) + // no-store so the post-auth redirect isn't cached and replayed (R147). + rw.Header().Set("Cache-Control", "no-store") http.Redirect(rw, req, redirectPath, http.StatusFound) } diff --git a/bearer_auth.go b/bearer_auth.go index 20bc0da..cb820e9 100644 --- a/bearer_auth.go +++ b/bearer_auth.go @@ -29,6 +29,7 @@ import ( "encoding/base64" "encoding/hex" "encoding/json" + "errors" "fmt" "net" "net/http" @@ -93,6 +94,10 @@ const ( type bearerError struct { kind bearerErrorKind reason string + // retryAfter carries the actual remaining penalty when known (e.g. the + // per-IP penalty box), so writeBearerError emits the true value instead + // of the full configured penalty. + retryAfter time.Duration } func (e *bearerError) Error() string { return e.reason } @@ -110,6 +115,32 @@ type joseHeader struct { Typ string `json:"typ"` } +// joseHeaderWellFormed reports whether token is structurally a JWT: exactly +// two dot-separated segments whose header decodes as a JSON JOSE header. +// It intentionally applies NO alg/kid policy (unlike parseBearerJOSEHeader) +// so the JWT-vs-opaque routing gate classifies purely on structure: a +// structurally-valid JWT with a disallowed alg stays on the JWT path +// (where local policy rejects it), while an opaque token whose two dots +// are coincidental falls through to opaque introspection (R166). +func joseHeaderWellFormed(token string) bool { + dot := strings.IndexByte(token, '.') + if dot <= 0 { + return false + } + if strings.Count(token[dot+1:], ".") != 1 { + return false + } + raw, err := base64.RawURLEncoding.DecodeString(token[:dot]) + if err != nil { + raw, err = base64.URLEncoding.DecodeString(token[:dot]) + if err != nil { + return false + } + } + var hdr joseHeader + return json.Unmarshal(raw, &hdr) == nil +} + // parseBearerJOSEHeader decodes the first JWT segment for early alg/kid pinning. // Does not touch the payload or signature — those are the verifier's job. // Returns nil on success; *bearerError on rejection so the handler can map @@ -153,19 +184,22 @@ func parseBearerJOSEHeader(token string) *bearerError { // header value, or "" if the rune is acceptable. Shared core of the bearer-path // identifier sanitizer and the cookie-path header claim sanitizer: rejects // control chars (CRLF/header injection), Unicode bidi-override runes (RTL -// spoofing of admin UI / SIEM), and the delimiters , ; = (a comma in a group -// name would inject extra entries into a comma-joined header). +// spoofing of admin UI / SIEM), and the delimiters , ; = plus the bracketing +// characters " { } — each of these in a group name / identifier would break a +// downstream comma-joined list or a naive CSV/JSON-style parser consuming the +// header (a comma would inject extra entries into a comma-joined header). func headerClaimRuneReason(r rune) string { if reason := headerInjectionRuneReason(r); reason != "" { return reason } - // The , ; = delimiters are only unsafe for values placed into delimited or - // list contexts (a comma-joined header, or an identifier downstreams may - // split). They are valid in arbitrary single header values, so this stricter - // check is used for the cookie-path identifier and the group/role list, NOT - // for free-form templated header output (see headerValueReason). - if r == ',' || r == ';' || r == '=' { - return "delimiter character" + // The , ; = delimiters and the " { } bracketing chars are only unsafe for + // values placed into delimited or list contexts (a comma-joined header, or + // an identifier downstreams may split). They are valid in arbitrary single + // header values, so this stricter check is used for the cookie-path + // identifier and the group/role list, NOT for free-form templated header + // output (see headerValueReason). + if r == ',' || r == ';' || r == '=' || r == '"' || r == '{' || r == '}' { + return "delimiter or bracketing character" } return "" } @@ -270,11 +304,12 @@ func resolveBearerIdentifier(claims map[string]interface{}, claimName string) (s if !ok { return "", newBearerError(bearerErrInvalidIdentifier, fmt.Sprintf("missing claim %q", claimName)) } - str, ok := raw.(string) - if !ok { - return "", newBearerError(bearerErrInvalidIdentifier, fmt.Sprintf("claim %q not a string", claimName)) + // Accept scalar numbers (e.g. a numeric sub) by stringifying them, + // so a numeric identifier claim doesn't 401 a valid principal. + if s, isScalar := claimScalarString(raw); isScalar { + return s, nil } - return str, nil + return "", newBearerError(bearerErrInvalidIdentifier, fmt.Sprintf("claim %q is not a scalar string or number", claimName)) } // enforceMultiAudienceAzp implements the spec hardening: when aud is a @@ -358,6 +393,10 @@ type bearerFailureEntry struct { count int } +// defaultBearerEntrySweepThreshold is the map size above which recordFailure +// sweeps stale entries, bounding memory under a flood of distinct source IPs. +const defaultBearerEntrySweepThreshold = 1024 + func newBearerFailureTracker(threshold int, window, penalty time.Duration) *bearerFailureTracker { if threshold <= 0 { threshold = 20 @@ -404,6 +443,18 @@ func (b *bearerFailureTracker) recordFailure(ip string) { b.mu.Lock() defer b.mu.Unlock() now := time.Now() + // Bound per-IP map growth (R117): entries for source IPs that fail once + // and never return were never removed. Sweep stale entries (no active + // penalty box, too old to matter) once the map grows past a nominal + // size, keeping steady-state memory bounded. + if len(b.entries) > defaultBearerEntrySweepThreshold { + cutoff := now.Add(-(b.window + b.penalty)) + for k, e := range b.entries { + if e.penaltyUntil.Before(cutoff) { + delete(b.entries, k) + } + } + } e, ok := b.entries[ip] if !ok || now.Sub(e.firstFailureAt) > b.window { e = &bearerFailureEntry{firstFailureAt: now} @@ -520,7 +571,10 @@ func (t *TraefikOidc) writeBearerError(rw http.ResponseWriter, req *http.Request case bearerErrThrottled: status = http.StatusTooManyRequests body = "Too Many Requests" - retryAfter = t.bearerFailurePenalty + retryAfter = err.retryAfter + if retryAfter <= 0 { + retryAfter = t.bearerFailurePenalty + } case bearerErrIntrospectionUnavailable: status = http.StatusServiceUnavailable body = "Service Unavailable" @@ -535,6 +589,10 @@ func (t *TraefikOidc) writeBearerError(rw http.ResponseWriter, req *http.Request if retryAfter > 0 { rw.Header().Set("Retry-After", fmt.Sprintf("%d", int(retryAfter.Seconds()))) } + // Auth-rejection responses must never be cached (R101 contract): a + // cached 429/401 could be replayed to a client well after the + // penalty box expired or the session recovered. + rw.Header().Set("Cache-Control", "no-store") rw.Header().Set("Content-Type", "text/plain; charset=utf-8") rw.WriteHeader(status) _, _ = rw.Write([]byte(body)) // Safe to ignore: best-effort error body write @@ -553,12 +611,11 @@ func (t *TraefikOidc) handleBearerRequest(rw http.ResponseWriter, req *http.Requ ip := clientIPForBearer(req) if blocked, retryAfter := t.bearerFailureTracker.blocked(ip); blocked { + // Carry the actual remaining penalty (diverges from the configured + // default on clock-skew, partial-window expiry) so writeBearerError + // emits the true Retry-After rather than the full penalty. throttled := newBearerError(bearerErrThrottled, "ip in penalty box") - // Preserve the actual retry-after even if it diverged from the - // configured default (clock-skew, partial-window expiry). - if retryAfter > 0 { - rw.Header().Set("Retry-After", fmt.Sprintf("%d", int(retryAfter.Seconds()))) - } + throttled.retryAfter = retryAfter t.writeBearerError(rw, req, throttled) return } @@ -574,12 +631,51 @@ func (t *TraefikOidc) handleBearerRequest(rw http.ResponseWriter, req *http.Requ t.writeBearerError(rw, req, newBearerError(bearerErrInvalidToken, "token exceeds max length")) return } - if strings.Count(token, ".") != 2 { + // Determine whether this is genuinely a JWT. RFC 7662 token + // introspection covers opaque access tokens; when the operator + // requires live introspection (requireTokenIntrospection, whose + // documented purpose is exactly opaque-token support via + // client_secret_basic), introspect the opaque token on the bearer + // path instead of rejecting it — the session path already does + // (validateOpaqueToken). Previously the JWT-shape gate ran before + // the introspection branch, so every opaque token got a 401 even + // with introspection on (R159). + // + // A token is only treated as a JWT when it has exactly two dots AND + // its header actually parses as a JOSE header. Dot count alone is + // insufficient: an opaque token that coincidentally contains exactly + // two dots used to be routed onto the JWT path and rejected even + // under requireTokenIntrospection (header parse would fail) — it must + // instead be introspected like any other opaque token (R166). + isJWT := joseHeaderWellFormed(token) + if !isJWT { + if t.requireTokenIntrospection { + p, bErr := t.buildPrincipalFromOpaqueIntrospection(token) + if bErr != nil { + if bErr.kind != bearerErrIntrospectionUnavailable { + t.bearerFailureTracker.recordFailure(ip) + } + t.writeBearerError(rw, req, bErr) + return + } + t.bearerFailureTracker.recordSuccess(ip) + if t.logger != nil { + t.logger.Debugf("bearer auth success (introspected opaque): identifier_hash=%s path=%s", + hashIdentifierForLog(p.Identifier), req.URL.Path) + } + t.forwardAuthorized(rw, req, p) + return + } t.bearerFailureTracker.recordFailure(ip) - t.writeBearerError(rw, req, newBearerError(bearerErrInvalidToken, "token is not a 3-segment JWT")) + t.writeBearerError(rw, req, newBearerError(bearerErrInvalidToken, "token is not a valid JWT")) return } + // Structurally a JWT: enforce the alg/kid policy EARLY (before the + // cached fast-path in buildPrincipalFromBearerToken can short-circuit + // on a prior positive verdict). Without this pin an alg=none or + // oversized-kid token that had been cached as verified would sail + // through without re-checking. if bErr := parseBearerJOSEHeader(token); bErr != nil { t.bearerFailureTracker.recordFailure(ip) t.writeBearerError(rw, req, bErr) @@ -588,7 +684,13 @@ func (t *TraefikOidc) handleBearerRequest(rw http.ResponseWriter, req *http.Requ p, bErr := t.buildPrincipalFromBearerToken(token) if bErr != nil { - t.bearerFailureTracker.recordFailure(ip) + // Count only authentication failures (401/403) toward the per-IP + // throttle. An introspection outage (503) is infrastructure, not a + // client failure; recording it would trip a 429 penalty that a + // single success can't clear even after the endpoint recovers. + if bErr.kind != bearerErrIntrospectionUnavailable { + t.bearerFailureTracker.recordFailure(ip) + } t.writeBearerError(rw, req, bErr) return } @@ -629,7 +731,9 @@ func (t *TraefikOidc) buildPrincipalFromBearerToken(token string) (*principal, * return nil, newBearerError(bearerErrInvalidToken, "token_use=id rejected") } - if bErr := enforceMultiAudienceAzp(claims, t.clientID); bErr != nil { + // Snapshot clientID under metadataMu: DCR rewrites it at runtime (R137). + clientID, _, _, _, _ := t.clientCredentials() + if bErr := enforceMultiAudienceAzp(claims, clientID); bErr != nil { return nil, bErr } if bErr := enforceIatAge(claims, t.maxTokenAge); bErr != nil { @@ -642,6 +746,25 @@ func (t *TraefikOidc) buildPrincipalFromBearerToken(token string) (*principal, * } } + // Honor IdP-initiated (backchannel/front-channel) logout. The cookie + // path re-checks sessionInvalidationCache on every request; the bearer + // path previously did not, so a still-cryptographically-valid access + // token for a logged-out subject kept returning 200 (and, with + // requireTokenIntrospection, a cached positive verdict extended the + // stale window). Reject here, mirroring the cookie path: use the + // token's iat as its creation time so a token issued before the + // logout is invalidated, while a legitimately freshly-issued token + // (iat after logout) still passes (R146). + subjectForInvalidation, _ := claims["sub"].(string) + sidForInvalidation, _ := claims["sid"].(string) + createdAt := time.Now() + if iat, ok := claims["iat"].(float64); ok { + createdAt = time.Unix(int64(iat), 0) + } + if t.isSessionInvalidated(sidForInvalidation, subjectForInvalidation, createdAt) { + return nil, newBearerError(bearerErrInvalidToken, "session has been invalidated (logout)") + } + rawIdentifier, bErr := resolveBearerIdentifier(claims, t.bearerIdentifierClaim) if bErr != nil { return nil, bErr @@ -652,7 +775,7 @@ func (t *TraefikOidc) buildPrincipalFromBearerToken(token string) (*principal, * } subject, _ := claims["sub"].(string) - clientID, _ := claims["azp"].(string) + clientID, _ = claims["azp"].(string) if clientID == "" { clientID, _ = claims["client_id"].(string) } @@ -667,6 +790,62 @@ func (t *TraefikOidc) buildPrincipalFromBearerToken(token string) (*principal, * }, nil } +// buildPrincipalFromOpaqueIntrospection authenticates a bearer access +// token that has no JWT shape by live RFC 7662 introspection, and builds +// a principal whose identifier is the introspected subject. It mirrors +// introspectOnBearerPath's classification (active, token_type, expiry, +// audience) and binds the compliant subject. Only used on the bearer +// path when requireTokenIntrospection is enabled (R159). +func (t *TraefikOidc) buildPrincipalFromOpaqueIntrospection(token string) (*principal, *bearerError) { + resp, err := t.introspectToken(token) + if err != nil { + var httpErr *HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode >= 400 && httpErr.StatusCode < 500 { + return nil, newBearerError(bearerErrTokenInactive, fmt.Sprintf("introspection reports token inactive (HTTP %d)", httpErr.StatusCode)) + } + return nil, newBearerError(bearerErrIntrospectionUnavailable, "introspection failed: "+err.Error()) + } + if !resp.Active { + return nil, newBearerError(bearerErrTokenInactive, "introspection reports token inactive") + } + // Mirror introspectOnBearerPath (R149): reject a definite non-access + // token_type (e.g. refresh_token); accept "access_token" and "Bearer". + if resp.TokenType != "" && resp.TokenType != "access_token" && resp.TokenType != "Bearer" { + return nil, newBearerError(bearerErrTokenInactive, "introspection token_type is not a bearer access token") + } + if resp.Exp > 0 && time.Now().After(time.Unix(resp.Exp, 0)) { + return nil, newBearerError(bearerErrTokenInactive, "introspection reports token expired") + } + // Audience gate when a distinct API audience is configured. + clientID, _, _, audience, _ := t.clientCredentials() + if audience != "" && audience != clientID { + if resp.Aud == nil || verifyAudience(resp.Aud, audience) != nil { + return nil, newBearerError(bearerErrTokenInactive, "introspection audience mismatch") + } + } + id := resp.Sub + if id == "" { + id = resp.Username + } + if id == "" { + return nil, newBearerError(bearerErrInvalidToken, "introspection response has no subject or username") + } + claims := map[string]interface{}{} + if resp.Sub != "" { + claims["sub"] = resp.Sub + } + if resp.Username != "" { + claims["username"] = resp.Username + } + return &principal{ + Claims: claims, + Identifier: id, + Subject: resp.Sub, + AccessToken: token, + Source: sourceBearer, + }, nil +} + // introspectOnBearerPath calls the existing RFC 7662 introspector when the // operator demands real-time revocation. Distinguishes "token revoked" (401) // from "endpoint unavailable" (503) so transient infra failures don't look @@ -674,10 +853,55 @@ func (t *TraefikOidc) buildPrincipalFromBearerToken(token string) (*principal, * func (t *TraefikOidc) introspectOnBearerPath(token string) *bearerError { resp, err := t.introspectToken(token) if err != nil { + // A definitive 4xx from the introspection endpoint (e.g. 401 for + // an unknown/revoked token) is a credential failure, not an infra + // outage — don't downgrade it to "unavailable" (R156). + var httpErr *HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode >= 400 && httpErr.StatusCode < 500 { + return newBearerError(bearerErrTokenInactive, fmt.Sprintf("introspection reports token inactive (HTTP %d)", httpErr.StatusCode)) + } return newBearerError(bearerErrIntrospectionUnavailable, "introspection failed: "+err.Error()) } if !resp.Active { return newBearerError(bearerErrTokenInactive, "introspection reports token inactive") } + // Mirror the session path (validateOpaqueToken): an opaque-or-any + // token whose RFC 7662 token_type classifies it as a refresh token + // must not be honored as a bearer access token (R149). Only reject on + // a definite non-access match; compliant providers may omit it. RFC + // 7662's token_type is the RFC 6749 token type, whose value for an + // access token is "Bearer" (RFC 6750) — accept both spellings + // providers use (R156). + if resp.TokenType != "" && resp.TokenType != "access_token" && resp.TokenType != "Bearer" { + return newBearerError(bearerErrTokenInactive, "introspection token_type is not a bearer access token") + } + // Mirror the session path (validateOpaqueToken): an active but + // already-expired result must not pass. The positive-only cache is + // capped by time-until-exp when Exp is present, but a provider + // returning active=1 past exp would otherwise let the token pass on + // the bearer path while the identical token is rejected on the + // session path. + if resp.Exp > 0 { + expTime := time.Unix(resp.Exp, 0) + if time.Now().After(expTime) { + return newBearerError(bearerErrTokenInactive, "introspection reports token expired") + } + } + // Mirror the session path (validateOpaqueToken): when a distinct API + // audience is configured (audience != clientID), the introspection + // response MUST carry a matching audience. Fail closed on a missing or + // mismatched aud, otherwise a token minted for a different audience + // would pass the bearer path while the identical token is rejected on + // the session path. aud may be a single string or an array (RFC 7662). + // (R143) + clientID, _, _, audience, _ := t.clientCredentials() + if audience != "" && audience != clientID { + if resp.Aud == nil { + return newBearerError(bearerErrTokenInactive, "introspection reports no audience") + } + if err := verifyAudience(resp.Aud, audience); err != nil { + return newBearerError(bearerErrTokenInactive, "introspection audience mismatch") + } + } return nil } diff --git a/client_assertion.go b/client_assertion.go index 0502678..a2a1e36 100644 --- a/client_assertion.go +++ b/client_assertion.go @@ -95,9 +95,18 @@ func NewClientAssertionSigner(pemBytes []byte, alg, kid string) (*ClientAssertio func validateAlgKeyMatch(alg string, key crypto.PrivateKey) error { switch alg[0] { case 'R', 'P': // RS* or PS* - if _, ok := key.(*rsa.PrivateKey); !ok { + rsaKey, ok := key.(*rsa.PrivateKey) + if !ok { return fmt.Errorf("alg %q requires an RSA key, got %T", alg, key) } + // RFC 7518 §3.3: RS*/PS* MUST use a key of 2048 bits or larger. + // A smaller key signs locally but conformant IdPs (Entra, Okta, + // Keycloak) reject it, so every exchange/refresh/revocation fails + // invalid_client at runtime — better to fail at construction + // (R135). + if rsaKey.N.BitLen() < 2048 { + return fmt.Errorf("alg %q requires an RSA key of at least 2048 bits, got %d", alg, rsaKey.N.BitLen()) + } case 'E': // ES* ecKey, ok := key.(*ecdsa.PrivateKey) if !ok { @@ -142,6 +151,14 @@ func (s *ClientAssertionSigner) Sign(audience, clientID string) (string, error) now := nowFn() + // RFC 7523 §3: aud must be the token (or introspection/revocation) + // endpoint URL. An empty audience yields a client_assertion with + // "aud":"" that the IdP will reject, so fail fast with a clear + // error rather than minting a useless token (R150). + if audience == "" { + return "", fmt.Errorf("client assertion audience must not be empty") + } + // 16 random bytes as lowercase hex for jti uniqueness. jtiBytes := make([]byte, 16) if _, err := io.ReadFull(rander, jtiBytes); err != nil { @@ -312,6 +329,3 @@ func buildClientAssertionSignerFromConfig(config *Config) (*ClientAssertionSigne return NewClientAssertionSigner(pemBytes, alg, config.ClientAssertionKeyID) } - - - diff --git a/helpers.go b/helpers.go index 7446729..6ad8cb1 100644 --- a/helpers.go +++ b/helpers.go @@ -107,10 +107,13 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code data := url.Values{ "grant_type": {grantType}, } + // Snapshot client credentials under metadataMu: DCR rewrites clientID/ + // clientSecret/audience at runtime (R137). + clientID, clientSecret, clientAuthMethod, _, clientAssertion := t.clientCredentials() // client_id is sent in the body for every method except client_secret_basic, // where it is carried in the Authorization header per RFC 6749 §2.3.1. - if t.clientAuthMethod != "client_secret_basic" || t.clientAssertion != nil { - data.Set("client_id", t.clientID) + if clientAuthMethod != "client_secret_basic" || clientAssertion != nil { + data.Set("client_id", clientID) } if grantType == "authorization_code" { @@ -148,17 +151,17 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code t.metadataMu.RUnlock() useBasicAuth := false - if t.clientAssertion != nil { - assertion, err := t.clientAssertion.Sign(tokenURL, t.clientID) + if clientAssertion != nil { + assertion, err := clientAssertion.Sign(tokenURL, clientID) if err != nil { return nil, fmt.Errorf("failed to sign client assertion: %w", err) } data.Set("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer") data.Set("client_assertion", assertion) - } else if t.clientAuthMethod == "client_secret_basic" { + } else if clientAuthMethod == "client_secret_basic" { useBasicAuth = true } else { - data.Set("client_secret", t.clientSecret) + data.Set("client_secret", clientSecret) } req, err := http.NewRequestWithContext(ctx, "POST", tokenURL, strings.NewReader(data.Encode())) //nolint:gosec // tokenURL is the operator-trusted discovery endpoint (metadataMu) @@ -167,7 +170,7 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code } req.Header.Set("Content-Type", "application/x-www-form-urlencoded") if useBasicAuth { - setOAuthBasicAuth(req, t.clientID, t.clientSecret) + setOAuthBasicAuth(req, clientID, clientSecret) } resp, err := client.Do(req) //nolint:gosec // req above targets the trusted tokenURL @@ -182,7 +185,14 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code if resp.StatusCode != http.StatusOK { limitReader := io.LimitReader(resp.Body, 1024*10) bodyBytes, _ := io.ReadAll(limitReader) // Safe to ignore: reading error body for diagnostics - return nil, fmt.Errorf("token endpoint returned status %d: %s", resp.StatusCode, string(bodyBytes)) + // Return a *HTTPError so the retry executor's isRetryableError can + // classify transient 5xx/429 responses and actually retry (R118). + // A plain error here made the 5xx/429 retry branch dead in + // production — every token-endpoint status was treated as permanent. + return nil, &HTTPError{ + StatusCode: resp.StatusCode, + Message: fmt.Sprintf("token endpoint returned status %d: %s", resp.StatusCode, string(bodyBytes)), + } } var tokenResponse TokenResponse @@ -192,6 +202,16 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code return nil, fmt.Errorf("failed to decode token response: %w", err) } + // RFC 6749 §5.1: a token response MUST include an access_token. A 200 + // that omitted it is a protocol violation, not a success — treat it as an + // error here so no caller authenticates with a blank token (R158). + // Previously this seam returned success with an empty access_token and + // each caller had to re-check "AccessToken == \"\"", which several + // paths missed. + if tokenResponse.AccessToken == "" { + return nil, fmt.Errorf("token endpoint returned 200 without an access_token (RFC 6749 §5.1)") + } + return &tokenResponse, nil } @@ -204,6 +224,17 @@ func (t *TraefikOidc) exchangeTokens(ctx context.Context, grantType string, code // Returns: // - *TokenResponse: New token set from the authorization server // - An error if the refresh operation fails +// +// redactToken returns a short, stable hash of a token credential suitable for +// debug logging without leaking the credential itself (R138). +func redactToken(token string) string { + if token == "" { + return "(empty)" + } + sum := sha256.Sum256([]byte(token)) + return fmt.Sprintf("%x", sum[:4]) +} + func (t *TraefikOidc) getNewTokenWithRefreshToken(refreshToken string) (*TokenResponse, error) { ctx := context.Background() @@ -218,7 +249,8 @@ func (t *TraefikOidc) getNewTokenWithRefreshToken(refreshToken string) (*TokenRe return nil, fmt.Errorf("failed to refresh token: %w", err) } - t.logger.Debugf("Token response: %+v", tokenResponse) + t.logger.Debugf("Refresh token response: token_type=%s expires_in=%d access_token=%s", + tokenResponse.TokenType, tokenResponse.ExpiresIn, redactToken(tokenResponse.AccessToken)) return tokenResponse, nil } @@ -399,6 +431,20 @@ func (t *TraefikOidc) handleLogout(rw http.ResponseWriter, req *http.Request) { } } + // Best-effort provider-side revocation (single logout): the local + // blacklist only stops reuse in-process, but a token captured before + // logout (e.g. a bearer-mode access token or the refresh token) stays + // valid at the IdP and a captured refresh token could keep minting new + // tokens. Revoke both access and refresh tokens at the provider when a + // revocation endpoint is available; ignore errors (the endpoint may be + // absent or the provider already cleaned up on the end-session call). + if accessToken != "" { + _ = t.RevokeTokenWithProvider(accessToken, "access_token") + } + if refreshToken != "" { + _ = t.RevokeTokenWithProvider(refreshToken, "refresh_token") + } + 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 @@ -437,10 +483,15 @@ func (t *TraefikOidc) handleLogout(rw http.ResponseWriter, req *http.Request) { http.Error(rw, "Logout error", http.StatusInternalServerError) return } + // Logout redirects must never be cached: a heuristic-caching proxy + // replaying the 302 would re-trigger logout for an already + // logged-out session (R147 cache-directive contract). + rw.Header().Set("Cache-Control", "no-store") http.Redirect(rw, req, logoutURL, http.StatusFound) return } + rw.Header().Set("Cache-Control", "no-store") http.Redirect(rw, req, localRedirect, http.StatusFound) } diff --git a/jwt.go b/jwt.go index bd7133d..b446e9e 100644 --- a/jwt.go +++ b/jwt.go @@ -26,8 +26,6 @@ var ( replayCache CacheInterface // shardedReplayCache is the new high-performance sharded cache for replay detection shardedReplayCache *ShardedCache - // replayCacheOnce ensures the replay cache is initialized only once - replayCacheOnce sync.Once // replayCacheCleanupWG waits for cleanup goroutine to finish replayCacheCleanupWG sync.WaitGroup // replayCacheCancel cancels the cleanup context @@ -39,21 +37,23 @@ var ( // initReplayCache initializes the JWT replay protection cache with bounded size. // Uses a sharded cache design with 64 shards for reduced lock contention under high load. // The cache is bounded to 10,000 entries to prevent unbounded memory growth. -// This function uses sync.Once to ensure thread-safe single initialization. +// Lazy creation is guarded entirely by replayCacheMu (a plain sync.Once would +// not re-arm after cleanup resets it, and the previous Once re-assignment raced +// with init under -race). cleanupReplayCache nil's the fields under the same +// lock, so this either creates anew (after a cleanup) or is a cheap no-op. func initReplayCache() { - replayCacheOnce.Do(func() { - // Hold mutex during initialization to synchronize with cleanup goroutine - replayCacheMu.Lock() - defer replayCacheMu.Unlock() + replayCacheMu.Lock() + defer replayCacheMu.Unlock() - // Create sharded cache with 64 shards for reduced contention + if shardedReplayCache == nil { + // Create sharded cache with 64 shards for reduced lock contention // Under 500 req/sec, this reduces lock contention by ~64x compared to single mutex shardedReplayCache = NewShardedCache(64, 10000) // Also initialize legacy cache for backward compatibility replayCache = NewCache() replayCache.SetMaxSize(10000) - }) + } } // cleanupReplayCache performs graceful shutdown of the replay cache system. @@ -61,17 +61,12 @@ func initReplayCache() { // and properly closes the cache to ensure proper cleanup during shutdown. func cleanupReplayCache() { replayCacheCleanupMu.Lock() - shouldWait := replayCacheCancel != nil if replayCacheCancel != nil { replayCacheCancel() replayCacheCancel = nil } replayCacheCleanupMu.Unlock() - - // Only wait if there was a cleanup routine running - if shouldWait { - replayCacheCleanupWG.Wait() - } + replayCacheCleanupWG.Wait() replayCacheMu.Lock() defer replayCacheMu.Unlock() @@ -87,8 +82,16 @@ func cleanupReplayCache() { replayCache.Close() replayCache = nil } +} - replayCacheOnce = sync.Once{} +// replayCacheKey namespaces a JTI by its issuer before it is stored in the +// process-global replay cache. JTI uniqueness is guaranteed only within a +// single issuer's namespace; many Traefik routers share this one global +// cache across different OIDC providers, so a bare jti would let two +// providers' legitimately-colliding JTIs map to the same entry (false +// replay) and silently overwrite each other (R142). +func replayCacheKey(issuer, jti string) string { + return issuer + "\x00" + jti } // getReplayCacheStats returns statistics about the replay cache state. @@ -96,9 +99,14 @@ func cleanupReplayCache() { // - size: Current number of entries in the cache // - maxSize: Maximum allowed entries (10,000) func getReplayCacheStats() (size int, maxSize int) { - // Use sharded cache if available (no mutex needed due to internal sharding) - if shardedReplayCache != nil { - return shardedReplayCache.Size(), 10000 + // Use sharded cache if available (no mutex needed due to internal sharding). + // Guard the singleton read because cleanupReplayCache nils it under the + // write lock. + replayCacheMu.RLock() + sc := shardedReplayCache + replayCacheMu.RUnlock() + if sc != nil { + return sc.Size(), 10000 } // Fall back to legacy cache @@ -281,13 +289,12 @@ func parseJWT(tokenString string) (*JWT, error) { return nil, fmt.Errorf("invalid JWT format: failed to decode signature: %v", err) } - // Reuse the signature buffer if it's large enough, otherwise allocate - if cap(jwtBuf.Signature) >= n { - jwt.Signature = jwtBuf.Signature[:n:n] // Use slice trick to prevent aliasing - } else { - jwt.Signature = make([]byte, n) - copy(jwt.Signature, jwtBuf.Signature[:n]) - } + // Copy the signature out of the pooled buffer. Borrowing it with + // [:(n):(n)] still shares the backing array, which the pool recycles + // and truncates on the next GetJWTBuffer — mutating this caller's + // slice after the fact (R156). + jwt.Signature = make([]byte, n) + copy(jwt.Signature, jwtBuf.Signature[:n]) return jwt, nil } @@ -342,12 +349,13 @@ func (j *JWT) Verify(issuerURL, expectedAudience string, skipReplayCheck ...bool return err } - iat, ok := claims["iat"].(float64) - if !ok { - return fmt.Errorf("missing or invalid 'iat' claim") - } - if err := verifyIssuedAt(iat); err != nil { - return err + // iat is OPTIONAL per RFC 7519 §4.1.6; only validate when present. + // Hard-requiring it here also rejected otherwise-valid access and + // logout tokens whose provider omits iat. + if iat, ok := claims["iat"].(float64); ok { + if err := verifyIssuedAt(iat); err != nil { + return err + } } if nbf, ok := claims["nbf"].(float64); ok { @@ -371,6 +379,12 @@ func (j *JWT) Verify(issuerURL, expectedAudience string, skipReplayCheck ...bool expTime = time.Now().Add(10 * time.Minute) } duration := time.Until(expTime) + // A token is accepted until exp + ClockSkewToleranceFuture (see + // verifyExpiration), so the replay entry must live that long too — + // otherwise in the (exp, exp+skew] window the entry has expired + // and been swept and the same token is re-accepted as fresh + // (R179). + duration += ClockSkewToleranceFuture if duration <= 0 { // Expired token: no replay window worth recording, so nothing to do. goto replayDone @@ -386,18 +400,26 @@ func (j *JWT) Verify(issuerURL, expectedAudience string, skipReplayCheck ...bool replayCacheMu.RLock() sc := shardedReplayCache if sc != nil { - if !sc.SetIfAbsent(jtiValue, true, duration) { + if !sc.SetIfAbsent(replayCacheKey(issuerURL, jtiValue), true, duration) { replayCacheMu.RUnlock() return fmt.Errorf("token replay detected (jti: %s)", jtiValue) } replayCacheMu.RUnlock() } else { - // Fall back to legacy cache (should rarely happen). Hold the - // mutex across the get+set so the record is atomic there too. + // Fall back to legacy cache (should rarely happen). Release the + // read lock before taking the write lock: RWMutex is not + // reentrant, so Lock() here while still holding RLock() + // self-deadlocks (R156). Guard the cache for nil — it is + // cleared alongside shardedReplayCache by cleanupReplayCache. + replayCacheMu.RUnlock() replayCacheMu.Lock() - _, exists := replayCache.Get(jtiValue) - if !exists && replayCache != nil { - replayCache.Set(jtiValue, true, duration) + rk := replayCacheKey(issuerURL, jtiValue) + exists := false + if replayCache != nil { + _, exists = replayCache.Get(rk) + if !exists { + replayCache.Set(rk, true, duration) + } } replayCacheMu.Unlock() if exists { diff --git a/logout.go b/logout.go index 4ffd099..b157751 100644 --- a/logout.go +++ b/logout.go @@ -100,7 +100,10 @@ func (t *TraefikOidc) handleBackchannelLogout(rw http.ResponseWriter, req *http. t.logger.Infof("Backchannel logout: successfully invalidated session (sid=%s, sub=%s)", claims.SessionID, claims.Subject) - // Return 200 OK with empty body per spec + // Return 200 OK with empty body per spec; mark it non-cacheable so a + // proxy cannot replay a stale logout acknowledgement (R147 contract). + rw.Header().Set("Cache-Control", "no-store") + rw.Header().Set("Content-Type", "text/plain; charset=utf-8") rw.WriteHeader(http.StatusOK) } @@ -291,18 +294,27 @@ func (t *TraefikOidc) validateLogoutToken(tokenString string) (*LogoutTokenClaim // validateLogoutTokenAudience checks if the logout token audience contains our client_id func (t *TraefikOidc) validateLogoutTokenAudience(aud interface{}) bool { + // t.clientID is mutated at runtime by the DCR path + // (performDynamicClientRegistration, under metadataMu). Read it under + // the same lock as every other credential field (issuer, jwksURL) so a + // backchannel-logout token validated concurrent with client re-issuance + // does not race the write. + t.metadataMu.RLock() + clientID := t.clientID + t.metadataMu.RUnlock() + switch v := aud.(type) { case string: - return v == t.clientID + return v == clientID case []interface{}: for _, a := range v { - if s, ok := a.(string); ok && s == t.clientID { + if s, ok := a.(string); ok && s == clientID { return true } } case []string: for _, a := range v { - if a == t.clientID { + if a == clientID { return true } } @@ -342,6 +354,17 @@ func (t *TraefikOidc) verifyLogoutTokenSignature(jwt *JWT, tokenString string) e } if err := verifySignatureWithKey(tokenString, pubKey, alg); err != nil { + // The signature failed against the (possibly stale) cached key. The + // provider may have rotated its signing keys in place, reusing the + // same kid (R109): refresh the JWKS once and retry before failing. + // Without this a logout token signed with an in-place-rotated key is + // rejected for up to the JWKS cache TTL, leaving the user's session + // alive after the IdP logged them out (R130). + if freshKey, ferr := t.jwkCache.getPublicKeyFresh(context.Background(), jwksURL, kid, t.httpClient); ferr == nil { + if verr := verifySignatureWithKey(tokenString, freshKey, alg); verr == nil { + return nil + } + } return fmt.Errorf("signature verification failed: %w", err) } @@ -402,6 +425,29 @@ func (t *TraefikOidc) invalidateSession(sid, sub string) error { // // Returns: // - true if the session has been invalidated, false otherwise +// +// sessionInvalidationTime extracts the invalidation timestamp from a cache +// value regardless of backend storage semantics. The in-memory cache returns +// the raw int64 we stored; serializing backends (e.g. Redis) JSON round-trip +// numbers, so Get yields a float64. Accepting both keeps backchannel/front- +// channel logout functional in multi-instance (distributed) deployments; +// previously the strict int64 assertion silently returned false there. +func sessionInvalidationTime(val interface{}) (int64, bool) { + switch v := val.(type) { + case int64: + return v, true + case float64: + return int64(v), true + case json.Number: + n, err := v.Int64() + if err != nil { + return 0, false + } + return n, true + } + return 0, false +} + func (t *TraefikOidc) isSessionInvalidated(sid, sub string, sessionCreatedAt time.Time) bool { if t.sessionInvalidationCache == nil { return false @@ -414,7 +460,7 @@ func (t *TraefikOidc) isSessionInvalidated(sid, sub string, sessionCreatedAt tim if sid != "" { key := t.buildSessionInvalidationKey("sid", sid) if val, found := t.sessionInvalidationCache.Get(key); found { - if invalidatedAt, ok := val.(int64); ok { + if invalidatedAt, ok := sessionInvalidationTime(val); ok { // Session was invalidated at or after it was created invalidationTime := time.Unix(invalidatedAt, 0) if !invalidationTime.Before(sessionCreatedAtSec) { @@ -429,7 +475,7 @@ func (t *TraefikOidc) isSessionInvalidated(sid, sub string, sessionCreatedAt tim if sub != "" { key := t.buildSessionInvalidationKey("sub", sub) if val, found := t.sessionInvalidationCache.Get(key); found { - if invalidatedAt, ok := val.(int64); ok { + if invalidatedAt, ok := sessionInvalidationTime(val); ok { // Sessions for this subject created at or before invalidation are invalid invalidationTime := time.Unix(invalidatedAt, 0) if !invalidationTime.Before(sessionCreatedAtSec) { @@ -448,6 +494,25 @@ func (t *TraefikOidc) buildSessionInvalidationKey(keyType, value string) string return fmt.Sprintf("session_invalidation:%s:%s", keyType, value) } +// sessionCreatedAtForInvalidation returns the session creation time to use +// for the backchannel/front-channel logout invalidation check. It prefers +// the ID token's iat claim; when iat is absent, it falls back to the +// session's own created_at (recorded at login) rather than time.Now(). +// Using time.Now() here meant a session whose ID token lacks iat was +// always treated as created "now" (after any logout), so invalidation +// never matched and IdP-initiated logout silently had no effect (R98). +func (t *TraefikOidc) sessionCreatedAtForInvalidation(idToken string, session *SessionData) time.Time { + if jwt, err := parseJWT(idToken); err == nil { + if iat, ok := jwt.Claims["iat"].(float64); ok { + return time.Unix(int64(iat), 0) + } + } + if ca := session.getCreatedAtUnsafe(); ca > 0 { + return time.Unix(ca, 0) + } + return time.Time{} // zero: treated as invalidated -> safe re-auth +} + // extractSessionInfo extracts sid and sub from an ID token for session tracking func (t *TraefikOidc) extractSessionInfo(idToken string) (sid, sub string, createdAt time.Time) { if idToken == "" { diff --git a/metadata_cache.go b/metadata_cache.go index c5792e3..299546a 100644 --- a/metadata_cache.go +++ b/metadata_cache.go @@ -147,11 +147,13 @@ func (mc *MetadataCache) GetProviderMetadata(ctx context.Context, providerURL st } // Require the core endpoints. An incomplete discovery document (missing - // authorization_endpoint / token_endpoint) would otherwise be accepted, - // cached, and adopted as the working auth/token URL, failing at runtime - // (wrong authorize redirect, token-exchange failure) instead of at startup. - if metadata.AuthURL == "" || metadata.TokenURL == "" { - return nil, fmt.Errorf("discovery document missing required endpoints (authorization_endpoint or token_endpoint)") + // authorization_endpoint / token_endpoint / jwks_uri) would otherwise be + // accepted, cached, and adopted as the working auth/token/JWKS URL, failing + // at runtime (wrong authorize redirect, token-exchange failure, or signature + // verification failure — jwksURL has no non-discovery source) instead of at + // startup. + if metadata.AuthURL == "" || metadata.TokenURL == "" || metadata.JWKSURL == "" { + return nil, fmt.Errorf("discovery document missing required endpoints (authorization_endpoint, token_endpoint or jwks_uri)") } // Pin the advertised issuer to the configured provider host. The issuer is diff --git a/middleware.go b/middleware.go index 1c1a43e..efe2edd 100644 --- a/middleware.go +++ b/middleware.go @@ -4,9 +4,12 @@ package traefikoidc import ( + "bufio" "bytes" "fmt" + "net" "net/http" + "regexp" "runtime/debug" "strings" "sync/atomic" @@ -15,6 +18,45 @@ import ( "github.com/lukaszraczylo/traefikoidc/internal/utils" ) +// trackingWriter wraps an http.ResponseWriter so the deferred panic handler +// can tell whether a response was already committed. WriteHeader is +// idempotent so calling it again after commit is a no-op, but a later +// Write appends to the committed body - so writing "Internal Server +// Error" after a handler has already produced a valid 200 (or a 302 +// redirect) corrupts the response. Flush/Hijack are forwarded so +// downstream SSE and WebSocket handlers keep working through the wrapper. +type trackingWriter struct { + http.ResponseWriter + wroteHeader bool +} + +func (w *trackingWriter) WriteHeader(code int) { + w.wroteHeader = true + w.ResponseWriter.WriteHeader(code) +} + +func (w *trackingWriter) Write(b []byte) (int, error) { + w.wroteHeader = true + return w.ResponseWriter.Write(b) +} + +func (w *trackingWriter) Flush() { + w.wroteHeader = true + if f, ok := w.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + +func (w *trackingWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { + w.wroteHeader = true + if h, ok := w.ResponseWriter.(http.Hijacker); ok { + return h.Hijack() + } + return nil, nil, http.ErrNotSupported +} + +func (w *trackingWriter) Unwrap() http.ResponseWriter { return w.ResponseWriter } + // 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. @@ -22,6 +64,7 @@ const ( bypassReasonExcluded = "excluded-url" bypassReasonSSE = "sse" bypassReasonWebSocket = "websocket" + bypassReasonOptions = "options" ) // isWebSocketUpgrade reports whether req is a WebSocket upgrade handshake @@ -54,6 +97,14 @@ func (t *TraefikOidc) shouldBypassAuth(req *http.Request) (bool, string) { if t.determineExcludedURL(req.URL.Path) { return true, bypassReasonExcluded } + // CORS preflights carry no session cookie and must reach the backend so + // it can answer with Access-Control-Allow-*. Otherwise the 401 here + // (with no ACAO headers, since the security applier only runs on the + // authenticated path) makes the browser block the cross-origin + // request entirely and the real request never proceeds (R124). + if req.Method == http.MethodOptions { + return true, bypassReasonOptions + } if strings.Contains(req.Header.Get("Accept"), "text/event-stream") { return true, bypassReasonSSE } @@ -80,6 +131,29 @@ func (t *TraefikOidc) shouldBypassAuth(req *http.Request) (bool, string) { // The second return value (0 when forwarding) is the HTTP status to send // when the request is rejected, letting the caller distinguish a 401 // (unauthenticated) from a 403 (authenticated but not permitted). +// identityHeaders are the headers this middleware owns upstream. It +// unconditionally strips any inbound values with these names before +// forwarding — both on authenticated and bypassed requests — so forged +// values (e.g. X-Forwarded-User: admin on a public path) can never +// reach the backend and be trusted for downstream authorization. +var identityHeaders = []string{ + "X-Forwarded-User", + "X-User-Groups", + "X-User-Roles", + "X-Auth-Request-User", + "X-Auth-Request-Redirect", + "X-Auth-Request-Token", +} + +// stripIdentityHeaders removes any inbound identity headers from req. The +// middleware re-injects its own authoritative values afterwards where +// appropriate. See identityHeaders. +func stripIdentityHeaders(req *http.Request) { + for _, h := range identityHeaders { + req.Header.Del(h) + } +} + func (t *TraefikOidc) applyBypassUserHeaders(req *http.Request, reason string) (bool, int) { if t.sessionManager == nil { return false, http.StatusUnauthorized @@ -115,14 +189,25 @@ func (t *TraefikOidc) applyBypassUserHeaders(req *http.Request, reason string) ( // Enforce the allowedRolesAndGroups gate, mirroring forwardAuthorized: // an authenticated user without any permitted role/group must not reach // the SSE/WebSocket backend just by using the streaming bypass. Claims - // come from the session's ID token, same source the normal path uses. + // come from the session's ID token by preference, falling back to the + // access token when there is no ID token — the same source selection the + // main authorization path (processAuthorizedRequest) uses. Without the + // fallback, opaque-ID-token providers (groups only in the access token) + // were granted on normal requests but 403'd on the streaming bypass + // (R118). if len(t.allowedRolesAndGroups) > 0 { - idClaims, claimsErr := session.GetIDTokenClaims(t.extractClaimsFunc) - if claimsErr != nil || idClaims == nil { + var groupClaims map[string]interface{} + var claimsErr error + if idToken := session.GetIDToken(); idToken != "" { + groupClaims, claimsErr = session.GetIDTokenClaims(t.extractClaimsFunc) + } else if accessToken := session.GetAccessToken(); accessToken != "" { + groupClaims, claimsErr = t.extractClaimsFunc(accessToken) + } + if claimsErr != nil || groupClaims == nil { t.logger.Debugf("%s bypass: cannot read claims for role check (err=%v): %s", reason, claimsErr, userIdentifier) return false, http.StatusForbidden } - groups, roles, extErr := t.extractGroupsAndRolesFromClaims(idClaims) + groups, roles, extErr := t.extractGroupsAndRolesFromClaims(groupClaims) if extErr != nil { t.logger.Debugf("%s bypass: role extraction failed for %s: %v", reason, userIdentifier, extErr) return false, http.StatusForbidden @@ -140,6 +225,17 @@ func (t *TraefikOidc) applyBypassUserHeaders(req *http.Request, reason string) ( } } + // This middleware fully owns the identity headers it injects upstream, even + // on the SSE/WebSocket bypass path. Unconditionally remove inbound values + // for the same names first: a client can forge X-User-Groups / + // X-Auth-Request-User etc. on a bypassed request, and since this path + // only sets X-Forwarded-User / X-Auth-Request-User (never groups/roles), + // forged group/role headers would otherwise survive to the backend, + // which commonly trusts them for downstream authorization. Must precede + // the sanitize gate so even the drop-on-unsafe early return has already + // cleared forged values (R100). + stripIdentityHeaders(req) + // Sanitize the claim-derived identifier before it is injected as a // header, matching forwardAuthorized (which uses safeIdentifier). The // value originates from an IdP-controlled claim and can contain CRLF @@ -172,20 +268,82 @@ func (t *TraefikOidc) applyBypassUserHeaders(req *http.Request, reason string) ( // Parameters: // - rw: The HTTP response writer. // - req: The incoming HTTP request. +// +// shouldSkipGraceRefresh reports whether a proactive refresh can be skipped for +// this request. The token forwarded downstream (forwardAuthorized) is the +// ACCESS token, and needsRefresh (validateStandardTokensRS -> validateTokenExpiryRS) +// is derived from the ACCESS token's expiry whenever it is a verifiable JWT. +// Basing the skip decision on the (possibly longer-lived) ID token's exp would +// forward a stale access token whenever the ID token is still fresh. Base it on +// the access token's expiry, falling back to the ID token only when the access +// token is opaque and unparseable (worst case: refresh when a skip might have +// been possible — the safe direction). +func (t *TraefikOidc) shouldSkipGraceRefresh(session *SessionData) bool { + return skipRefreshForTokens(session.GetAccessToken(), session.GetIDToken(), t.refreshGracePeriod) +} + +// skipRefreshForTokens reports whether a proactive refresh can be skipped given +// the session's access and ID tokens. +func skipRefreshForTokens(accessToken, idToken string, grace time.Duration) bool { + graceToken := accessToken + graceJWT, graceErr := parseJWT(graceToken) + if graceErr != nil || graceJWT == nil { + graceToken = idToken + graceJWT, graceErr = parseJWT(graceToken) + } + if graceJWT == nil || graceErr != nil { + return false + } + expClaim, ok := graceJWT.Claims["exp"].(float64) + if !ok { + return false + } + expTimeObj := time.Unix(int64(expClaim), 0) + return !expTimeObj.Before(time.Now().Add(grace)) +} + func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { + // Track whether a response has been committed so the panic handler can + // avoid appending an error body to an already-emitted valid response. + tw := &trackingWriter{ResponseWriter: rw} + rw = tw // Recover from any panic in the handler chain (session decode, token // validation, header injection, authorize-url build) so a bug answers a // single clean 500 (and logs a stack) instead of escaping to net/http, - // which closes/truncates the connection. WriteHeader is idempotent, so - // this is safe even if a handler already wrote headers; Write appends. + // which closes/truncates the connection. If the response was already + // committed by a handler (200 + body or a redirect), WriteHeader is an + // idempotent no-op and appending the body would corrupt a valid + // response, so only write when nothing has been sent yet. defer func() { if r := recover(); r != nil { t.logger.Errorf("OIDC handler panic recovered: %v\n%s", r, debug.Stack()) - rw.WriteHeader(http.StatusInternalServerError) - _, _ = rw.Write([]byte("Internal Server Error")) + if !tw.wroteHeader { + // A panic-induced 500 must not be cached (consistent with + // every other auth-failure response, R101/R172). + rw.Header().Set("Cache-Control", "no-store") + rw.WriteHeader(http.StatusInternalServerError) + _, _ = rw.Write([]byte("Internal Server Error")) + } } }() + // Apply security headers to EVERY response this middleware authors + // (redirects, error pages, bypass forwards, and authenticated + // forwards), not just the forwardAuthorized tail. Previously they only + // reached successfully-authenticated forwarding, so the initial + // 302-to-IdP, /health, /metrics, error pages and SSE/WS responses + // carried no Strict-Transport-Security / framing / MIME sniffing + // protections (R132). + if t.securityHeadersApplier != nil { + t.securityHeadersApplier(rw, req) + } else { + // Fallback to basic security headers + rw.Header().Set("X-Frame-Options", "DENY") + rw.Header().Set("X-Content-Type-Options", "nosniff") + rw.Header().Set("X-XSS-Protection", "1; mode=block") + rw.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin") + } + // Log request entry for debugging routing issues t.logger.Debugf("Incoming request: %s %s", req.Method, req.URL.Path) @@ -241,12 +399,20 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { // public endpoint logs via downstream services that don't expect it. // Excluded URLs are explicitly public; bearer is an artifact of the // API auth flow that doesn't belong on them. - if t.enableBearerAuth { + // Also honor stripAuthorizationHeader (default true) so a raw + // client-supplied Authorization header is not forwarded verbatim on + // the SSE/WebSocket/excluded bypass — the same leak R144 closed for + // the normal path was left open on the streaming bypass (R151). + if t.enableBearerAuth || t.stripAuthorizationHeader { req.Header.Del("Authorization") } switch reason { case bypassReasonExcluded: // Operator-declared excluded URLs forward unconditionally. + // These are public paths; still strip any client-forged + // identity headers so X-Forwarded-User / X-User-Groups etc. + // on a public endpoint can't be trusted downstream (R102). + stripIdentityHeaders(req) t.next.ServeHTTP(rw, req) case bypassReasonSSE, bypassReasonWebSocket: // Skip the OIDC redirect dance (clients can't follow it @@ -263,6 +429,11 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { return } t.next.ServeHTTP(rw, req) + case bypassReasonOptions: + // CORS preflight: forward unconditionally (no session needed) + // so the backend can answer with CORS headers. + stripIdentityHeaders(req) + t.next.ServeHTTP(rw, req) default: t.next.ServeHTTP(rw, req) } @@ -351,14 +522,31 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { cleanReq := req.Clone(req.Context()) session, _ = t.sessionManager.GetSession(cleanReq) // Safe to ignore: error already logged, proceeding with new session if session != nil { - defer session.returnToPoolSafely() if clearErr := session.Clear(cleanReq, rw); clearErr != nil { t.logger.Errorf("Error clearing potentially corrupted session: %v", clearErr) } - } else { - t.logger.Error("Critical session error: Failed to get even a new session.") - t.sendErrorResponse(rw, req, "Critical session error", http.StatusInternalServerError) - return + // Clear() returns the session to the object pool, so discard the + // pointer — reusing it would be a use-after-return, since a + // concurrent GetSession may already own the pooled object + // (cross-request session bleed / data race). Obtain a fresh + // owned session in the shared block below. (R108) + session = nil + } + if session == nil { + // Session cookie is expired (past sessionMaxAge): a fresh + // GetSession still re-reads the same expired cookie and returns + // nil, so obtain a clean session and let the shared block below + // re-initiate authentication rather than returning 500 — + // otherwise every request past the session max age is a 500 + // with no way to recover (R97). + session = t.sessionManager.newSession(cleanReq) + if session == nil { + t.logger.Error("Critical session error: Failed to get even a new session.") + t.sendErrorResponse(rw, req, "Critical session error", http.StatusInternalServerError) + return + } + defer session.returnToPoolSafely() + t.logger.Infof("Session expired or unreadable; re-initiating authentication") } // Sub-resource requests (script/image/fetch/serviceWorker) must not // trigger an OIDC redirect from this path either: they would overwrite @@ -398,6 +586,19 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { t.logger.Debugf("Checking callback URL match: request_path=%q, configured_callback=%q", req.URL.Path, t.redirURLPath) if req.URL.Path == t.redirURLPath { t.logger.Debugf("Callback URL matched, processing OIDC callback (redirect_url=%s)", redirectURL) + // The authorization-code response is delivered to the redirect + // endpoint via HTTP GET (RFC 6749 §3.1.2): handleCallback reads + // code/state/error only from the query string (form_post is not + // supported - a POST would see an empty query and fail with "No + // authorization code" anyway). Reject other methods with 405 so a + // HEAD/non-GET probe doesn't consume the one-time state and code + // and emit a 302 (R181). + if req.Method != http.MethodGet { + t.logger.Debugf("Callback requested with method %s (want GET); returning 405", req.Method) + rw.Header().Set("Cache-Control", "no-store") + rw.WriteHeader(http.StatusMethodNotAllowed) + return + } t.handleCallback(rw, req, redirectURL) return } @@ -466,25 +667,17 @@ func (t *TraefikOidc) ServeHTTP(rw http.ResponseWriter, req *http.Request) { } if shouldAttemptRefresh { - idToken := session.GetIDToken() - if idToken != "" { - jwt, err := parseJWT(idToken) - if err == nil { - claims := jwt.Claims - if expClaim, ok := claims["exp"].(float64); ok { - expTime := int64(expClaim) - expTimeObj := time.Unix(expTime, 0) - refreshThreshold := time.Now().Add(t.refreshGracePeriod) - - if !expTimeObj.Before(refreshThreshold) { - t.logger.Debug("Token is valid and outside grace period, skipping refresh") - t.processAuthorizedRequest(rw, req, session, redirectURL) - return - } - } else { - t.logger.Debug("Could not extract 'exp' claim for grace period check, proceeding with refresh") - } - } + // Only skip the proactive refresh when the session is genuinely + // authenticated. skipRefreshForTokens only parseJWT's the access + // token and reads the raw exp claim — it does not verify the + // signature or identity, so gating on the authenticated flag keeps + // an unauthenticated session (or one that failed verification for a + // non-expiry reason) from being bare-forwarded past the allowlist + // gate (R142). + if authenticated && t.shouldSkipGraceRefresh(session) { + t.logger.Debug("Token is valid and outside grace period, skipping refresh") + t.processAuthorizedRequest(rw, req, session, redirectURL) + return } if needsRefresh && authenticated { @@ -574,7 +767,8 @@ func (t *TraefikOidc) processAuthorizedRequestRS(rw http.ResponseWriter, req *ht idToken := rs.idToken if t.enableBackchannelLogout || t.enableFrontchannelLogout { if idToken != "" { - sid, sub, createdAt := t.extractSessionInfo(idToken) + sid, sub, _ := t.extractSessionInfo(idToken) + createdAt := t.sessionCreatedAtForInvalidation(idToken, session) if t.isSessionInvalidated(sid, sub, createdAt) { t.logger.Infof("Session for user %s has been invalidated via IdP-initiated logout", userIdentifier) if err := session.Clear(req, rw); err != nil { @@ -666,7 +860,8 @@ func (t *TraefikOidc) processAuthorizedRequest(rw http.ResponseWriter, req *http if t.enableBackchannelLogout || t.enableFrontchannelLogout { idToken := session.GetIDToken() if idToken != "" { - sid, sub, createdAt := t.extractSessionInfo(idToken) + sid, sub, _ := t.extractSessionInfo(idToken) + createdAt := t.sessionCreatedAtForInvalidation(idToken, session) if t.isSessionInvalidated(sid, sub, createdAt) { t.logger.Infof("Session for user %s has been invalidated via IdP-initiated logout", userIdentifier) // Clear the session and redirect to login @@ -788,6 +983,18 @@ const headerTemplateMaxLen = 8192 // optional claim the provider does not emit does not leak "". const noValueSentinel = "" +// noValueTokenRe matches "" only as a whole token (bounded by +// non-alphanumerics or string end), so a legitimate claim value that +// happens to contain the substring survives the strip instead of being +// corrupted (R103). +var noValueTokenRe = regexp.MustCompile(`(^|[^0-9A-Za-z])` + regexp.QuoteMeta(noValueSentinel) + `([^0-9A-Za-z]|$)`) + +// stripNoValueSentinels removes text/template's missing-key placeholder +// only when it appears as a whole token. +func stripNoValueSentinels(s string) string { + return noValueTokenRe.ReplaceAllString(s, "${1}${2}") +} + // 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 @@ -820,7 +1027,46 @@ func (t *TraefikOidc) sanitizeHeaderClaimList(values []string, headerName string return safe } +// joinBoundedClaimHeader joins sanitized claim values with commas, bounding +// the TOTAL length to headerTemplateMaxLen. A provider can return an +// unbounded number of groups/roles; joining them all into a single header +// would let an oversized value reach a downstream proxy that rejects or +// truncates over-long headers (431), even though each value already passed +// the per-value cap. Accumulates as many leading values as fit, so a +// well-formed list is unaffected; an empty result means nothing fit and the +// caller should drop the header (fail-closed). +func joinBoundedClaimHeader(values []string) string { + if len(values) == 0 { + return "" + } + var b strings.Builder + for _, v := range values { + sep := 0 + if b.Len() > 0 { + sep = 1 // leading comma + } + if b.Len()+sep+len(v) > headerTemplateMaxLen { + break + } + if sep > 0 { + b.WriteByte(',') + } + b.WriteString(v) + } + return b.String() +} + func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Request, p *principal) { + // This middleware fully owns the identity headers it injects upstream. + // Unconditionally remove any inbound values with the same names first: + // a request could carry a forged X-User-Groups / X-Auth-Request-User + // etc. that, if the middleware computes no value for one of them + // (e.g. an authenticated user with no groups, an absent ID token, a + // a header-template skip), would otherwise survive through to the + // backend and be trusted for downstream authorization. Clearing here — + // before any conditional Set — makes the values we emit authoritative. + stripIdentityHeaders(req) + var ( groups, roles []string extractErr error @@ -844,11 +1090,24 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques // header injection and an embedded comma would inject extra // entries into the comma-joined header. Fail closed: drop any // value that does not pass. - if safeGroups := t.sanitizeHeaderClaimList(groups, "X-User-Groups"); len(safeGroups) > 0 { - req.Header.Set("X-User-Groups", strings.Join(safeGroups, ",")) - } - if safeRoles := t.sanitizeHeaderClaimList(roles, "X-User-Roles"); len(safeRoles) > 0 { - req.Header.Set("X-User-Roles", strings.Join(safeRoles, ",")) + // X-User-Groups / X-User-Roles are the largest variably-sized + // forward headers; gate them behind minimalHeaders like the + // X-Auth-Request-* family so group-heavy users don't still trip + // HTTP 431 on the backend (the failure minimalHeaders exists to + // prevent, #64). Identity (X-Forwarded-User) stays + // unconditional; the allowedRolesAndGroups gate below still + // evaluates groups regardless of emission. + if !t.minimalHeaders { + if safeGroups := t.sanitizeHeaderClaimList(groups, "X-User-Groups"); len(safeGroups) > 0 { + if gh := joinBoundedClaimHeader(safeGroups); gh != "" { + req.Header.Set("X-User-Groups", gh) + } + } + if safeRoles := t.sanitizeHeaderClaimList(roles, "X-User-Roles"); len(safeRoles) > 0 { + if rh := joinBoundedClaimHeader(safeRoles); rh != "" { + req.Header.Set("X-User-Roles", rh) + } + } } } } @@ -902,17 +1161,29 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques t.logger.Debugf("Dropping X-Auth-Request-User header: identifier failed claim sanitization") } if p.IDToken != "" { - req.Header.Set("X-Auth-Request-Token", p.IDToken) + // Bound the raw ID token to the same header budget as rendered + // template values so a large token can't push this (and every + // other header on the request) past nginx/traefik 431 limits. + idTokenHeader := p.IDToken + if len(idTokenHeader) > headerTemplateMaxLen { + idTokenHeader = idTokenHeader[:headerTemplateMaxLen] + } + req.Header.Set("X-Auth-Request-Token", idTokenHeader) } } - // Bearer source: strip the raw inbound Authorization header to keep the - // token out of downstream service logs (off-by-config for operators who - // chain services that each re-verify the bearer). Stripped here, before - // header-template application, so an operator-supplied Authorization - // template (e.g. "Authorization: Bearer {{.AccessToken}}") can re-supply - // the downstream value rather than being immediately deleted. - if p.Source == sourceBearer && t.stripAuthorizationHeader { + // Strip the raw inbound Authorization header for BOTH auth paths when + // configured. The bearer path already stripped it; the cookie/session + // path forwarded a client-supplied Authorization verbatim, letting a + // session user present an arbitrary (and possibly more privileged) + // bearer that a downstream verifier might trust over X-Forwarded-User, + // or leaking a raw token into downstream logs. Kept out of downstream + // logs and make X-Forwarded-User / X-Auth-Request-Token the + // authoritative identity signal. Stripped here, before header-template + // application, so an operator-supplied Authorization template (e.g. + // "Authorization: Bearer {{.AccessToken}}") can re-supply the + // downstream value rather than being immediately deleted. + if t.stripAuthorizationHeader { req.Header.Del("Authorization") } @@ -942,7 +1213,7 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques // 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, "") + headerValue = stripNoValueSentinels(headerValue) } // Skip an empty render: Setting "" would clobber an identity // header (X-Forwarded-User / X-Auth-Request-*) already injected @@ -970,17 +1241,6 @@ func (t *TraefikOidc) forwardAuthorized(rw http.ResponseWriter, req *http.Reques } } - // Apply security headers if configured - if t.securityHeadersApplier != nil { - t.securityHeadersApplier(rw, req) - } else { - // Fallback to basic security headers - rw.Header().Set("X-Frame-Options", "DENY") - rw.Header().Set("X-Content-Type-Options", "nosniff") - rw.Header().Set("X-XSS-Protection", "1; mode=block") - rw.Header().Set("Referrer-Policy", "strict-origin-when-cross-origin") - } - // Strip OIDC session cookies before forwarding to the backend to prevent // HTTP 431 "Request Header Fields Too Large" errors (GitHub issue #122). if t.stripAuthCookies && t.sessionManager != nil { diff --git a/profiling.go b/profiling.go index 9f4c2a7..738c100 100644 --- a/profiling.go +++ b/profiling.go @@ -35,6 +35,10 @@ type MemorySnapshot struct { HeapProfile []byte GoroutineProfile []byte RuntimeStats runtime.MemStats + // Goroutines is the goroutine count (runtime.NumGoroutine()) captured at + // snapshot time, so leak analysis can diff a baseline against a later + // snapshot instead of sampling "now" twice (R118). + Goroutines int } // LeakAnalysis contains the results of memory leak detection and analysis. @@ -118,10 +122,22 @@ func NewProfilingManager(logger *Logger) *ProfilingManager { // TakeSnapshot captures a comprehensive snapshot of current memory statistics. // Includes runtime stats, heap profile, goroutine profile, and custom metrics. func (pm *ProfilingManager) TakeSnapshot() (*MemorySnapshot, error) { + pm.mu.RLock() + defer pm.mu.RUnlock() + return pm.takeSnapshot() +} + +// takeSnapshot implements TakeSnapshot assuming the caller already holds the +// lock (either RLock for the public API, or the write Lock during +// Start/StopProfiling). Calling the public TakeSnapshot from Start/Stop +// while holding pm.mu.Lock() would self-deadlock (an RWMutex is not +// recursively re-entrant on the write side) — R118. +func (pm *ProfilingManager) takeSnapshot() (*MemorySnapshot, error) { // nolint:unparam // error return kept for the exported TakeSnapshot contract var buf bytes.Buffer snapshot := &MemorySnapshot{ Timestamp: time.Now(), CustomMetrics: make(map[string]interface{}), + Goroutines: runtime.NumGoroutine(), } runtime.ReadMemStats(&snapshot.RuntimeStats) @@ -146,13 +162,11 @@ func (pm *ProfilingManager) TakeSnapshot() (*MemorySnapshot, error) { } } - pm.mu.RLock() for name, profiler := range pm.profilers { if customStats := profiler.GetCurrentStats(); customStats != nil { snapshot.CustomMetrics[name] = customStats } } - pm.mu.RUnlock() return snapshot, nil } @@ -170,7 +184,7 @@ func (pm *ProfilingManager) StartProfiling(config ProfilingConfig) error { pm.isProfiling = true pm.startTime = time.Now() - baseline, err := pm.TakeSnapshot() + baseline, err := pm.takeSnapshot() // lock already held -> use unlocked variant if err != nil { pm.isProfiling = false return fmt.Errorf("failed to take baseline snapshot: %w", err) @@ -190,7 +204,7 @@ func (pm *ProfilingManager) StopProfiling() (*MemorySnapshot, error) { return nil, fmt.Errorf("profiling not in progress") } - finalSnapshot, err := pm.TakeSnapshot() + finalSnapshot, err := pm.takeSnapshot() // lock already held -> use unlocked variant if err != nil { pm.logger.Errorf("Failed to take final snapshot: %v", err) } @@ -225,8 +239,12 @@ func (pm *ProfilingManager) AnalyzeLeaks(baseline, current *MemorySnapshot) *Lea memoryIncrease := current.RuntimeStats.Alloc - baseline.RuntimeStats.Alloc analysis.MemoryIncrease = memoryIncrease + // Diff against the goroutine count captured when the baseline snapshot + // was taken, not a live double-sample at analysis time. Sampling "now" + // for both values made the increase always 0 and the goroutine-leak + // branch permanently dead (R118). + baselineGoroutines := baseline.Goroutines currentGoroutines := runtime.NumGoroutine() - baselineGoroutines := runtime.NumGoroutine() goroutineIncrease := currentGoroutines - baselineGoroutines analysis.GoroutineIncrease = goroutineIncrease diff --git a/refresh_coordinator.go b/refresh_coordinator.go index 2bd8d88..2f80088 100644 --- a/refresh_coordinator.go +++ b/refresh_coordinator.go @@ -183,7 +183,7 @@ func NewRefreshCoordinator(config RefreshCoordinatorConfig, logger *Logger) *Ref rc := &RefreshCoordinator{ // inFlightRefreshes and sessionRefreshAttempts are both sync.Map; // their zero values are ready to use. - config: config, + config: config, metrics: &RefreshMetrics{}, logger: logger, stopChan: make(chan struct{}), @@ -225,10 +225,22 @@ func (rc *RefreshCoordinator) CoordinateRefresh( // CRITICAL FIX: Atomically check for existing operation OR create new one // This prevents the race where multiple goroutines check, find nothing, then all create + // + // R154: increment the WaitGroup BEFORE getOrCreateOperation registers + // the operation. Previously the Add(1) happened only after the op was + // registered and published as new, so a concurrent Shutdown + // (close(stopChan); wg.Wait) could observe the registered op, see a + // zero wg count, and return before this goroutine's Add landed — the + // just-started refresh then ran past Shutdown unreaped. Done() is + // balanced once per path: by the spawned goroutine, or immediately + // on the join/reject paths (which spawn nothing). + rc.wg.Add(1) + operation, isNew, err := rc.getOrCreateOperation(ctx, sessionID, tokenHash, refreshToken) if err != nil { // Operation creation was rejected (rate limit, memory pressure, concurrent limit) + rc.wg.Done() // no goroutine launched return nil, err } @@ -236,7 +248,6 @@ func (rc *RefreshCoordinator) CoordinateRefresh( // We created a new operation, so we need to execute it. Track the // goroutine so Shutdown waits for in-flight refreshes to finish (they // are aborted promptly via rc.ctx when stopChan closes). - rc.wg.Add(1) go func() { defer rc.wg.Done() rc.executeRefreshAsync(operation, sessionID, tokenHash, refreshFunc) //nolint:gosec // long-lived background refresh intentionally uses a background context @@ -244,6 +255,7 @@ func (rc *RefreshCoordinator) CoordinateRefresh( } else { // Joined existing operation - this is a deduplicated request atomic.AddInt64(&rc.metrics.deduplicatedRequests, 1) + rc.wg.Done() // joined an existing op; no goroutine tracked here } // Wait for the operation to complete @@ -665,15 +677,20 @@ func refreshCoordinatorSessionID(token string) string { const refreshCoordinatorWaitTimeout = 35 * time.Second // isUnderMemoryPressure checks if the system is under memory pressure by -// consulting the global memory monitor. Returns true when pressure reaches -// High or Critical, at which point we refuse new refresh operations to -// avoid aggravating an already-stressed heap. +// consulting the global memory monitor. Returns true when the current heap +// exceeds the coordinator's OWN configured MemoryPressureThresholdMB, at +// which point we refuse new refresh operations to avoid aggravating an +// already-stressed heap. func (rc *RefreshCoordinator) isUnderMemoryPressure() bool { monitor := GetGlobalMemoryMonitor() if monitor == nil { return false } - return monitor.GetMemoryPressure() >= MemoryPressureHigh + stats := monitor.GetCurrentStats() + if stats == nil || rc.config.MemoryPressureThresholdMB == 0 { + return false + } + return stats.HeapAllocBytes > rc.config.MemoryPressureThresholdMB*1024*1024 } // cleanupRoutine periodically cleans up stale tracking entries diff --git a/requeststate.go b/requeststate.go index 8aff012..1d1068b 100644 --- a/requeststate.go +++ b/requeststate.go @@ -32,12 +32,12 @@ type requestState struct { // (Save, Clear, etc.) still have access. session *SessionData - authenticated bool - accessToken string - idToken string - refreshToken string - userIdentifier string - createdAtUnixSec int64 + authenticated bool + accessToken string + idToken string + refreshToken string + userIdentifier string + createdAtUnixSec int64 // Output: scheme/host/redirect path determined at top of ServeHTTP. scheme string diff --git a/scope_filter.go b/scope_filter.go index c8b940e..60c0f1c 100644 --- a/scope_filter.go +++ b/scope_filter.go @@ -38,7 +38,10 @@ func (sf *ScopeFilter) FilterSupportedScopes(requestedScopes, supportedScopes [] // If no supported scopes declared, return all requested (backward compatibility) if len(supportedScopes) == 0 { sf.logger.Debugf("ScopeFilter: Provider %s has no scopes_supported in discovery doc, using all requested scopes", providerURL) - return requestedScopes + // Return a copy so the caller can append without aliasing the + // slice it passed in (which would let the two views of the same + // backing array cross-contaminate each other). + return append([]string(nil), requestedScopes...) } // Build lookup map for efficient checking diff --git a/session.go b/session.go index 30dfe81..2d49068 100644 --- a/session.go +++ b/session.go @@ -140,12 +140,14 @@ type combinedSessionPayload struct { X map[string]interface{} `json:"x,omitempty"` A string `json:"a,omitempty"` R string `json:"r,omitempty"` + Ri int64 `json:"ri,omitempty"` // refresh-token issued_at (unix), preserved so maxRefreshTokenAge survives combined round-trip I string `json:"i,omitempty"` Ui string `json:"ui,omitempty"` Cs string `json:"cs,omitempty"` N string `json:"n,omitempty"` Cv string `json:"cv,omitempty"` Ip string `json:"ip,omitempty"` + Ru string `json:"ru,omitempty"` // redirect_uri used at initiate; reused at callback so a per-request rebuild can't drift (R136) Ca int64 `json:"ca,omitempty"` Rc int `json:"rc,omitempty"` Au bool `json:"au,omitempty"` @@ -163,6 +165,7 @@ var knownSessionKeys = map[string]bool{ "nonce": true, "code_verifier": true, "incoming_path": true, + "redirect_url": true, "created_at": true, "redirect_count": true, } @@ -381,16 +384,6 @@ func decompressTokenInternal(compressed string) string { limitedReader := io.LimitReader(gz, 500*1024) - if cap(readerBuf) >= 512*1024 { - readerBuf = readerBuf[:cap(readerBuf)] - n, err := limitedReader.Read(readerBuf) - if err != nil && err != io.EOF { - return compressed - } - decompressed := readerBuf[:n] - return string(decompressed) - } - decompressed, err := io.ReadAll(limitedReader) if err != nil { return compressed @@ -912,6 +905,14 @@ func (sm *SessionManager) EnhanceSessionSecurity(options *sessions.Options, r *h sm.logger.Debugf("Using configured cookie domain: %s", sm.cookieDomain) } else if options.Domain == "" && r != nil { host := r.Host + // Strip the port before the subdomain-bounding check below; a ported + // r.Host (e.g. app.example.com:8080) would otherwise fail + // HasSuffix(host, "."+fh) and wrongly fall back to the strict + // parent X-Forwarded-Host, scoping the token-bearing session cookie + // to every sibling subdomain. + if ci := strings.Index(host, ":"); ci != -1 { + host = host[:ci] + } if forwardedHost := r.Header.Get("X-Forwarded-Host"); forwardedHost != "" { // Only use the forwarded host when it is the serving host or a @@ -1029,7 +1030,7 @@ func (sm *SessionManager) CleanupOldCookies(w http.ResponseWriter, r *http.Reque Domain: domain, MaxAge: -1, HttpOnly: true, - Secure: r.Header.Get("X-Forwarded-Proto") == "https" || r.TLS != nil || sm.forceHTTPS, + Secure: strings.EqualFold(r.Header.Get("X-Forwarded-Proto"), "https") || r.TLS != nil || sm.forceHTTPS, SameSite: http.SameSiteLaxMode, } http.SetCookie(w, deleteCookie) @@ -1059,7 +1060,44 @@ func (sm *SessionManager) CleanupOldCookies(w http.ResponseWriter, r *http.Reque // // Returns: // - The loaded SessionData instance. -// - An error if session loading or validation fails. +// +// newSession returns a clean, pooled session with no cookie state loaded. It +// is used when the request's session cookie is expired: GetSession keeps +// re-reading that same expired cookie and returns nil, so the caller needs a +// fresh session to (re-)initiate authentication rather than erroring out. +// Mirrors GetSession's pool/activeSessions accounting. +func (sm *SessionManager) newSession(r *http.Request) *SessionData { + sessionData, _ := sm.sessionPool.Get().(*SessionData) // Safe to ignore: pool return is best-effort + if sessionData == nil { + return nil + } + sessionData.Reset() // clear any stale state from a previous pooled user + sessionData.request = r + sessionData.inUse.Store(true) + sessionData.dirty = false + atomic.AddInt64(&sm.poolHits, 1) + atomic.AddInt64(&sm.activeSessions, 1) + + // GetSession normally populates the four legacy sub-sessions from the + // request; newSession runs where that read already failed (expired + // cookie), so initialize non-nil containers here. Downstream auth + // flow (SetCSRF/SetNonce/... then Save) writes through these. + if sessionData.mainSession == nil { + sessionData.mainSession, _ = sm.store.Get(r, sm.mainCookieName()) + } + if sessionData.accessSession == nil { + sessionData.accessSession, _ = sm.store.Get(r, sm.accessTokenCookieName()) + } + if sessionData.refreshSession == nil { + sessionData.refreshSession, _ = sm.store.Get(r, sm.refreshTokenCookieName()) + } + if sessionData.idTokenSession == nil { + sessionData.idTokenSession, _ = sm.store.Get(r, sm.idTokenCookieName()) + } + return sessionData +} + +// - An error if session loading or validation fails. func (sm *SessionManager) GetSession(r *http.Request) (*SessionData, error) { sessionData, _ := sm.sessionPool.Get().(*SessionData) // Safe to ignore: pool return is best-effort atomic.AddInt64(&sm.poolHits, 1) @@ -1230,6 +1268,7 @@ func (sm *SessionManager) loadFromCombinedCookies(r *http.Request, sessionData * sessionData.mainSession.Values["nonce"] = payload.N sessionData.mainSession.Values["code_verifier"] = payload.Cv sessionData.mainSession.Values["incoming_path"] = payload.Ip + sessionData.mainSession.Values["redirect_url"] = payload.Ru sessionData.mainSession.Values["created_at"] = payload.Ca sessionData.mainSession.Values["redirect_count"] = payload.Rc @@ -1243,6 +1282,9 @@ func (sm *SessionManager) loadFromCombinedCookies(r *http.Request, sessionData * sessionData.refreshSession.Values["token"] = payload.R sessionData.refreshSession.Values["compressed"] = false + if payload.Ri > 0 { + sessionData.refreshSession.Values["issued_at"] = payload.Ri + } sessionData.idTokenSession.Values["token"] = payload.I sessionData.idTokenSession.Values["compressed"] = false @@ -1259,7 +1301,14 @@ func (sm *SessionManager) loadFromCombinedCookies(r *http.Request, sessionData * // - chunks: The map (typically SessionData.accessTokenChunks or SessionData.refreshTokenChunks) // to populate with the found session chunks. func (sm *SessionManager) getTokenChunkSessions(r *http.Request, baseName string, chunks map[int]*sessions.Session) { - for i := 0; ; i++ { + // Bound the scan at MaxChunks (50) so an oversized set of chunk + // cookies (attack or corruption) can't drive unbounded + // securecookie decrypts. Loading through index 50 still lets + // processChunkedToken's len(chunks) > MaxChunks check fire, so the + // overflow → error behavior is unchanged; indices beyond 50 would be + // rejected anyway (R183). + const maxChunkScan = 50 + for i := 0; i <= maxChunkScan; i++ { sessionName := fmt.Sprintf("%s_%d", baseName, i) session, err := sm.store.Get(r, sessionName) if err != nil || session.IsNew { @@ -1305,6 +1354,12 @@ type SessionData struct { dirty bool + // expireCookie is a one-shot flag set by Clear to force the cookies it + // writes to be expired (Max-Age: -1). Without it, Save rebuilds a + // positive MaxAge and logout leaves an empty session cookie alive for + // the full session lifetime instead of expiring it (R101). + expireCookie bool + inUse atomic.Bool // cachedClaimsToken is the ID token string whose claims were last parsed and @@ -1346,11 +1401,23 @@ func (sd *SessionData) MarkDirty() { // Returns: // - An error if saving any of the session components fails. func (sd *SessionData) Save(r *http.Request, w http.ResponseWriter) error { - isSecure := r.Header.Get("X-Forwarded-Proto") == "https" || r.TLS != nil || sd.manager.forceHTTPS + // X-Forwarded-Proto value is compared case-insensitively: reverse proxies + // (nginx, Traefik) emit lowercase "https", but a client-controllable or + // non-canonical value like "HTTPS" must still yield a Secure cookie. + // Case-sensitive comparison here would silently downgrade the session + // cookie to transport-unsafe on such a header (R143). + isSecure := strings.EqualFold(r.Header.Get("X-Forwarded-Proto"), "https") || r.TLS != nil || sd.manager.forceHTTPS options := sd.manager.getSessionOptions(isSecure) options = sd.manager.EnhanceSessionSecurity(options, r) + if sd.expireCookie { + // Clear set this flag: the cookies written below must be expired, + // not re-issued with a fresh positive MaxAge. + options.MaxAge = -1 + sd.expireCookie = false + } + // Use combined storage for new sessions if sd.useCombinedStorage { return sd.saveCombined(r, w, options) @@ -1363,7 +1430,11 @@ func (sd *SessionData) Save(r *http.Request, w http.ResponseWriter) error { // saveCombined saves all session data in a single compressed, chunked cookie. // This reduces cookie count and total size through combined compression. func (sd *SessionData) saveCombined(r *http.Request, w http.ResponseWriter, options *sessions.Options) error { - // Build the combined payload + // Build the combined payload. All token/main fields are read under + // sessionMutex.RLock so a concurrent Set* (which writes under the + // write side) can't tear the payload mid-build (R132). The extra-map + // walk below already took the Read side; extend it to the field reads. + sd.sessionMutex.RLock() payload := &combinedSessionPayload{ A: sd.getAccessTokenUnsafe(), R: sd.getRefreshTokenUnsafe(), @@ -1374,12 +1445,18 @@ func (sd *SessionData) saveCombined(r *http.Request, w http.ResponseWriter, opti N: sd.getNonceUnsafe(), Cv: sd.getCodeVerifierUnsafe(), Ip: sd.getIncomingPathUnsafe(), + Ru: sd.getRedirectURLUnsafe(), Ca: sd.getCreatedAtUnsafe(), Rc: sd.getRedirectCountUnsafe(), } + // Persist the refresh token's issued_at so maxRefreshTokenAge keeps + // working after a combined save/load round-trip (it lives on + // refreshSession, which is otherwise dropped by combined storage). + if ra, ok := sd.refreshSession.Values["issued_at"].(int64); ok { + payload.Ri = ra + } // Collect extra session values not handled by the standard fields - sd.sessionMutex.RLock() if sd.mainSession != nil && len(sd.mainSession.Values) > 0 { extra := make(map[string]interface{}) for key, val := range sd.mainSession.Values { @@ -1404,7 +1481,11 @@ func (sd *SessionData) saveCombined(r *http.Request, w http.ResponseWriter, opti compressed, err := compressCombinedPayload(payload) if err != nil { sd.manager.logger.Errorf("Failed to compress combined payload: %v", err) - // Fall back to legacy storage on compression failure + // Fall back to legacy storage on compression failure. Expire any + // pre-existing combined chunk cookies first, or GetSession + // (combined-first) would keep serving the stale combined payload + // and shadow the freshly written legacy tokens (R163). + sd.expireOldCombinedChunks(r, w, options, 0) return sd.saveLegacy(r, w, options) } @@ -1413,8 +1494,15 @@ func (sd *SessionData) saveCombined(r *http.Request, w http.ResponseWriter, opti // Split into chunks chunks := splitCombinedIntoChunks(compressed, maxCookieSize) if len(chunks) > maxCombinedChunks { - sd.manager.logger.Errorf("Combined session requires %d chunks, exceeds max %d", len(chunks), maxCombinedChunks) - return fmt.Errorf("session data too large: requires %d chunks, max is %d", len(chunks), maxCombinedChunks) + sd.manager.logger.Errorf("Combined session requires %d chunks, exceeds max %d; falling back to legacy storage", len(chunks), maxCombinedChunks) + // The legacy path supports up to 50 token chunks (~100KB), so large + // tokens must not drop the session entirely — persist via the + // legacy (per-token chunk) format instead of erroring and forcing + // the user to re-authenticate (R97). Expire the old combined + // chunks first so GetSession's combined-first read serves the + // fresh legacy tokens rather than the stale combined payload (R163). + sd.expireOldCombinedChunks(r, w, options, 0) + return sd.saveLegacy(r, w, options) } sd.manager.logger.Debugf("Combined session split into %d chunks", len(chunks)) @@ -1573,20 +1661,24 @@ func (sd *SessionData) expireLegacyCookies(r *http.Request, w http.ResponseWrite expireLegacyMain(sd.manager.refreshTokenCookieName()) expireLegacyMain(sd.manager.idTokenCookieName()) - // Expire legacy chunk cookies (safe to clear values, they're separate from main sessions) - for i := 0; i < 50; i++ { // Max legacy chunks was 50 - accessChunk := fmt.Sprintf("%s_%d", sd.manager.accessTokenCookieName(), i) - refreshChunk := fmt.Sprintf("%s_%d", sd.manager.refreshTokenCookieName(), i) - idChunk := fmt.Sprintf("%s_%d", sd.manager.idTokenCookieName(), i) - - session, err := sd.manager.store.Get(r, accessChunk) - if err != nil || session.IsNew { - break // No more chunks + // Expire legacy chunk cookies (safe to clear values, they're separate from main sessions). + // Each token type is walked to its own limit so a small, un-chunked access token + // does not gate refresh/id chunk expiry (R133 P2-A): previously the shared + // access-driven loop broke all three series at the first absent access chunk, + // leaving stale refresh/id legacy chunk cookies behind after combined migration. + expireLegacyChunkSeries := func(baseName string) { + for i := 0; i < 50; i++ { // Max legacy chunks was 50 + chunk := fmt.Sprintf("%s_%d", baseName, i) + session, err := sd.manager.store.Get(r, chunk) + if err != nil || session.IsNew { + break // No more chunks for this token type + } + expireLegacyChunk(chunk) } - expireLegacyChunk(accessChunk) - expireLegacyChunk(refreshChunk) - expireLegacyChunk(idChunk) } + expireLegacyChunkSeries(sd.manager.accessTokenCookieName()) + expireLegacyChunkSeries(sd.manager.refreshTokenCookieName()) + expireLegacyChunkSeries(sd.manager.idTokenCookieName()) } // clearSessionValues removes all values from a session and optionally expires it. @@ -1667,6 +1759,7 @@ func (sd *SessionData) Clear(r *http.Request, w http.ResponseWriter) error { // Return a test error without trying to save problematic data err = fmt.Errorf("test error triggered by X-Test-Error header") } else { + sd.expireCookie = true err = sd.Save(r, w) } } @@ -1973,19 +2066,19 @@ func (sd *SessionData) SetAccessToken(token string) { return } - if sd.request != nil { - sd.expireAccessTokenChunksEnhanced(nil) - } - - for k := range sd.accessTokenChunks { - delete(sd.accessTokenChunks, k) - } + // Defer clearing the PREVIOUS access token (its HTTP chunk cookies and the + // in-memory chunk map) until the new token's storage is fully validated. + // Clearing first, then aborting on a too-large new token (>100KB after + // compression, or >50 chunks) would leave the previous still-valid access + // token nowhere — GetAccessToken returns "" and the user is forced to + // re-authenticate (R122). We commit the clear only once we're replacing it. if token == "" { if sd.accessSession != nil { sd.accessSession.Values["token"] = "" sd.accessSession.Values["compressed"] = false } + sd.clearAccessTokenChunks() return } @@ -2014,6 +2107,7 @@ func (sd *SessionData) SetAccessToken(token string) { } if len(compressed) <= maxCookieSize { + sd.clearAccessTokenChunks() if sd.accessSession != nil { sd.accessSession.Values["token"] = compressed sd.accessSession.Values["compressed"] = (compressed != token) @@ -2024,10 +2118,12 @@ func (sd *SessionData) SetAccessToken(token string) { } } } else { - if sd.accessSession != nil { - sd.accessSession.Values["token"] = "" - sd.accessSession.Values["compressed"] = (compressed != token) - } + // WARNING: do not clear the existing token (accessSession.Values + // ["token"] / accessTokenChunks) until the new token's chunks are + // validated below. Clearing first meant an abort on any chunk + // failure (too many chunks, reassembly mismatch) left NO token + // anywhere -> GetAccessToken returned "" -> forced re-auth (data + // loss of the previous still-valid access token). chunks := splitIntoChunks(compressed, maxCookieSize) @@ -2042,11 +2138,23 @@ func (sd *SessionData) SetAccessToken(token string) { } testReassembled := strings.Join(chunks, "") + // R188 cross-chunk integrity: bind all chunks to one another so a + // mix of authentic-but-different chunk sets is caught at assembly + // (see processChunkedToken). Stored on every chunk. + chunkIntegrity := fmt.Sprintf("%x", sha256.Sum256([]byte(testReassembled))) if testReassembled != compressed { sd.manager.logger.Debug("Access token chunk reassembly test failed") return } + // Validation passed: now it is safe to clear the existing token + // before storing the new chunked one. + sd.clearAccessTokenChunks() + if sd.accessSession != nil { + sd.accessSession.Values["token"] = "" + sd.accessSession.Values["compressed"] = (compressed != token) + } + for i, chunkData := range chunks { sessionName := fmt.Sprintf("%s_%d", sd.manager.accessTokenCookieName(), i) @@ -2080,6 +2188,7 @@ func (sd *SessionData) SetAccessToken(token string) { session.Values["compressed"] = (compressed != token) session.Values["chunk_created_at"] = time.Now().Unix() session.Values["token_total"] = len(chunks) + session.Values["chunk_integrity"] = chunkIntegrity sd.accessTokenChunks[i] = session } @@ -2087,6 +2196,20 @@ func (sd *SessionData) SetAccessToken(token string) { } } +// clearAccessTokenChunks expires the current access token's HTTP chunk cookies +// (via expireAccessTokenChunksEnhanced) and clears the in-memory access +// token chunk map. Callers invoke it only once they are committed to +// replacing the token, so a failed (too-large) store leaves the previous +// valid access token intact. Called with sd.sessionMutex already held. +func (sd *SessionData) clearAccessTokenChunks() { + if sd.request != nil { + sd.expireAccessTokenChunksEnhanced(nil) + } + for k := range sd.accessTokenChunks { + delete(sd.accessTokenChunks, k) + } +} + // GetRefreshToken retrieves the user's refresh token from session storage. // It handles both single-cookie storage and chunked storage for large tokens, // with automatic decompression if the token was compressed. @@ -2182,15 +2305,14 @@ func (sd *SessionData) SetRefreshToken(token string) { } sd.dirty = true - if sd.request != nil { - sd.expireRefreshTokenChunksEnhanced(nil) - } - - for k := range sd.refreshTokenChunks { - delete(sd.refreshTokenChunks, k) - } - if token == "" { + // Explicit clear: the caller wants the previous token gone. + if sd.request != nil { + sd.expireRefreshTokenChunksEnhanced(nil) + } + for k := range sd.refreshTokenChunks { + delete(sd.refreshTokenChunks, k) + } sd.refreshSession.Values["token"] = "" sd.refreshSession.Values["compressed"] = false return @@ -2207,70 +2329,98 @@ func (sd *SessionData) SetRefreshToken(token string) { } if len(compressed) <= maxCookieSize { + // Single-cookie layout: nothing to validate, safe to swap. + if sd.request != nil { + sd.expireRefreshTokenChunksEnhanced(nil) + } + for k := range sd.refreshTokenChunks { + delete(sd.refreshTokenChunks, k) + } sd.refreshSession.Values["token"] = compressed sd.refreshSession.Values["compressed"] = (compressed != token) sd.refreshSession.Values["issued_at"] = time.Now().Unix() - } else { - sd.refreshSession.Values["token"] = "" - sd.refreshSession.Values["compressed"] = (compressed != token) - sd.refreshSession.Values["issued_at"] = time.Now().Unix() - - chunks := splitIntoChunks(compressed, maxCookieSize) - - if len(chunks) == 0 { - sd.manager.logger.Errorf("CRITICAL: Failed to create chunks for refresh token") - return - } - - if len(chunks) > 50 { - sd.manager.logger.Errorf("CRITICAL: Too many chunks (%d) for refresh token - possible corruption", len(chunks)) - return - } - - testReassembled := strings.Join(chunks, "") - if testReassembled != compressed { - sd.manager.logger.Errorf("CRITICAL: Refresh token chunk reassembly test failed") - return - } - - for i, chunkData := range chunks { - sessionName := fmt.Sprintf("%s_%d", sd.manager.refreshTokenCookieName(), i) - - if sd.request == nil { - sd.manager.logger.Errorf("CRITICAL: SetRefreshToken: sd.request is nil, cannot create chunk session %s", sessionName) - return - } - - if chunkData == "" { - sd.manager.logger.Errorf("CRITICAL: Empty refresh token chunk data at index %d", i) - return - } - - if len(chunkData) > maxCookieSize { - sd.manager.logger.Errorf("CRITICAL: Refresh token chunk %d size %d exceeds maxCookieSize %d", i, len(chunkData), maxCookieSize) - return - } - - if !validateChunkSize(chunkData) { - sd.manager.logger.Errorf("CRITICAL: Refresh token chunk %d will exceed browser cookie limits after encoding (raw size: %d)", i, len(chunkData)) - return - } - - session, err := sd.manager.store.Get(sd.request, sessionName) - if err != nil { - sd.manager.logger.Errorf("CRITICAL: Failed to get refresh token chunk session %s: %v", sessionName, err) - return - } - - session.Values["token_chunk"] = chunkData - session.Values["compressed"] = (compressed != token) - session.Values["chunk_created_at"] = time.Now().Unix() - session.Values["token_total"] = len(chunks) - sd.refreshTokenChunks[i] = session - } - - sd.manager.logger.Debugf("SUCCESS: Stored refresh token in %d chunks", len(chunks)) + return } + + // Chunked layout: validate and write every new chunk BEFORE touching the + // previous token, so an abort (request missing, store error, size check, + // reassembly) leaves the still-valid prior refresh token intact (R154; + // extends the R95 access-token fix to the refresh token). + chunks := splitIntoChunks(compressed, maxCookieSize) + + if len(chunks) == 0 { + sd.manager.logger.Errorf("CRITICAL: Failed to create chunks for refresh token") + return + } + + if len(chunks) > 50 { + sd.manager.logger.Errorf("CRITICAL: Too many chunks (%d) for refresh token - possible corruption", len(chunks)) + return + } + + testReassembled := strings.Join(chunks, "") + // R188 cross-chunk integrity: bind all chunks to one another so a mix + // of authentic-but-different chunk sets is caught at assembly (see + // processChunkedToken). Stored on every chunk. + chunkIntegrity := fmt.Sprintf("%x", sha256.Sum256([]byte(testReassembled))) + if testReassembled != compressed { + sd.manager.logger.Errorf("CRITICAL: Refresh token chunk reassembly test failed") + return + } + + newChunks := make(map[int]*sessions.Session, len(chunks)) + for i, chunkData := range chunks { + sessionName := fmt.Sprintf("%s_%d", sd.manager.refreshTokenCookieName(), i) + + if sd.request == nil { + sd.manager.logger.Errorf("CRITICAL: SetRefreshToken: sd.request is nil, cannot create chunk session %s", sessionName) + return + } + + if chunkData == "" { + sd.manager.logger.Errorf("CRITICAL: Empty refresh token chunk data at index %d", i) + return + } + + if len(chunkData) > maxCookieSize { + sd.manager.logger.Errorf("CRITICAL: Refresh token chunk %d size %d exceeds maxCookieSize %d", i, len(chunkData), maxCookieSize) + return + } + + if !validateChunkSize(chunkData) { + sd.manager.logger.Errorf("CRITICAL: Refresh token chunk %d will exceed browser cookie limits after encoding (raw size: %d)", i, len(chunkData)) + return + } + + session, err := sd.manager.store.Get(sd.request, sessionName) + if err != nil { + sd.manager.logger.Errorf("CRITICAL: Failed to get refresh token chunk session %s: %v", sessionName, err) + return + } + + session.Values["token_chunk"] = chunkData + session.Values["compressed"] = (compressed != token) + session.Values["chunk_created_at"] = time.Now().Unix() + session.Values["token_total"] = len(chunks) + session.Values["chunk_integrity"] = chunkIntegrity + newChunks[i] = session + } + + // Every new chunk committed: now it is safe to discard the old token. + if sd.request != nil { + sd.expireRefreshTokenChunksEnhanced(nil) + } + for k := range sd.refreshTokenChunks { + delete(sd.refreshTokenChunks, k) + } + sd.refreshSession.Values["token"] = "" + sd.refreshSession.Values["compressed"] = (compressed != token) + sd.refreshSession.Values["issued_at"] = time.Now().Unix() + for i, s := range newChunks { + sd.refreshTokenChunks[i] = s + } + + sd.manager.logger.Debugf("SUCCESS: Stored refresh token in %d chunks", len(chunks)) } // GetRefreshTokenIssuedAt retrieves the timestamp when the refresh token was issued/stored. @@ -2517,6 +2667,8 @@ func isCorruptionMarker(data string) bool { // Returns: // - The CSRF token string, or an empty string if not set. func (sd *SessionData) GetCSRF() string { + sd.sessionMutex.RLock() + defer sd.sessionMutex.RUnlock() csrf, _ := sd.mainSession.Values["csrf"].(string) return csrf } @@ -2526,6 +2678,8 @@ func (sd *SessionData) GetCSRF() string { // Parameters: // - token: The CSRF token to store. func (sd *SessionData) SetCSRF(token string) { + sd.sessionMutex.Lock() + defer sd.sessionMutex.Unlock() currentVal, _ := sd.mainSession.Values["csrf"].(string) if currentVal != token { sd.mainSession.Values["csrf"] = token @@ -2539,6 +2693,8 @@ func (sd *SessionData) SetCSRF(token string) { // Returns: // - The nonce string, or an empty string if not set. func (sd *SessionData) GetNonce() string { + sd.sessionMutex.RLock() + defer sd.sessionMutex.RUnlock() nonce, _ := sd.mainSession.Values["nonce"].(string) return nonce } @@ -2548,6 +2704,8 @@ func (sd *SessionData) GetNonce() string { // Parameters: // - nonce: The nonce string to store. func (sd *SessionData) SetNonce(nonce string) { + sd.sessionMutex.Lock() + defer sd.sessionMutex.Unlock() currentVal, _ := sd.mainSession.Values["nonce"].(string) if currentVal != nonce { sd.mainSession.Values["nonce"] = nonce @@ -2561,6 +2719,8 @@ func (sd *SessionData) SetNonce(nonce string) { // Returns: // - The code verifier string, or an empty string if not set or PKCE is disabled. func (sd *SessionData) GetCodeVerifier() string { + sd.sessionMutex.RLock() + defer sd.sessionMutex.RUnlock() codeVerifier, _ := sd.mainSession.Values["code_verifier"].(string) return codeVerifier } @@ -2571,6 +2731,8 @@ func (sd *SessionData) GetCodeVerifier() string { // Parameters: // - codeVerifier: The PKCE code verifier string to store. func (sd *SessionData) SetCodeVerifier(codeVerifier string) { + sd.sessionMutex.Lock() + defer sd.sessionMutex.Unlock() currentVal, _ := sd.mainSession.Values["code_verifier"].(string) if currentVal != codeVerifier { sd.mainSession.Values["code_verifier"] = codeVerifier @@ -2612,6 +2774,8 @@ func (sd *SessionData) SetUserIdentifier(userIdentifier string) { // Returns: // - The original request URI string, or an empty string if not set. func (sd *SessionData) GetIncomingPath() string { + sd.sessionMutex.RLock() + defer sd.sessionMutex.RUnlock() path, _ := sd.mainSession.Values["incoming_path"].(string) return path } @@ -2622,6 +2786,8 @@ func (sd *SessionData) GetIncomingPath() string { // Parameters: // - path: The original request URI string (e.g., "/protected/resource?id=123"). func (sd *SessionData) SetIncomingPath(path string) { + sd.sessionMutex.Lock() + defer sd.sessionMutex.Unlock() currentVal, _ := sd.mainSession.Values["incoming_path"].(string) if currentVal != path { sd.mainSession.Values["incoming_path"] = path @@ -2629,6 +2795,35 @@ func (sd *SessionData) SetIncomingPath(path string) { } } +// GetRedirectURL retrieves the fully-qualified redirect_uri persisted when the +// authentication flow was initiated. It is consumed at the callback (in place +// of a per-request rebuild) so a drift in X-Forwarded-Host between the +// initiate and callback requests cannot change the redirect_uri sent to the +// provider (R136). +func (sd *SessionData) GetRedirectURL() string { + sd.sessionMutex.RLock() + defer sd.sessionMutex.RUnlock() + ru, _ := sd.mainSession.Values["redirect_url"].(string) + return ru +} + +// SetRedirectURL stores the redirect_uri used at initiate time (R136). +func (sd *SessionData) SetRedirectURL(redirectURL string) { + sd.sessionMutex.Lock() + defer sd.sessionMutex.Unlock() + currentVal, _ := sd.mainSession.Values["redirect_url"].(string) + if currentVal != redirectURL { + sd.mainSession.Values["redirect_url"] = redirectURL + sd.dirty = true + } +} + +// getRedirectURLUnsafe retrieves the redirect URL without acquiring locks. +func (sd *SessionData) getRedirectURLUnsafe() string { + ru, _ := sd.mainSession.Values["redirect_url"].(string) + return ru +} + // GetIDToken retrieves the user's ID token from session storage. // The ID token contains user claims and is used for user identification // and authorization decisions. Handles compression and chunking automatically. @@ -2804,14 +2999,6 @@ func (sd *SessionData) SetIDToken(token string) { } sd.dirty = true - if sd.request != nil { - sd.expireIDTokenChunksEnhanced(nil) - } - - for k := range sd.idTokenChunks { - delete(sd.idTokenChunks, k) - } - if token == "" { if sd.idTokenSession != nil { sd.idTokenSession.Values["token"] = "" @@ -2831,72 +3018,100 @@ func (sd *SessionData) SetIDToken(token string) { } if len(compressed) <= maxCookieSize { + // Single-cookie layout: nothing to validate, safe to swap. + if sd.request != nil { + sd.expireIDTokenChunksEnhanced(nil) + } + for k := range sd.idTokenChunks { + delete(sd.idTokenChunks, k) + } if sd.idTokenSession != nil { sd.idTokenSession.Values["token"] = compressed sd.idTokenSession.Values["compressed"] = (compressed != token) } - } else { - if sd.idTokenSession != nil { - sd.idTokenSession.Values["token"] = "" - sd.idTokenSession.Values["compressed"] = (compressed != token) - } - - chunks := splitIntoChunks(compressed, maxCookieSize) - - if len(chunks) == 0 { - sd.manager.logger.Errorf("CRITICAL: Failed to create chunks for ID token") - return - } - - if len(chunks) > 50 { - sd.manager.logger.Errorf("CRITICAL: Too many chunks (%d) for ID token - possible corruption", len(chunks)) - return - } - - testReassembled := strings.Join(chunks, "") - if testReassembled != compressed { - sd.manager.logger.Errorf("CRITICAL: ID token chunk reassembly test failed") - return - } - - for i, chunkData := range chunks { - sessionName := fmt.Sprintf("%s_%d", sd.manager.idTokenCookieName(), i) - - if sd.request == nil { - sd.manager.logger.Errorf("CRITICAL: SetIDToken: sd.request is nil, cannot create chunk session %s", sessionName) - return - } - - if chunkData == "" { - sd.manager.logger.Debug("Empty chunk data at index %d", i) - return - } - - if len(chunkData) > maxCookieSize { - sd.manager.logger.Info("Chunk %d size %d exceeds maxCookieSize %d", i, len(chunkData), maxCookieSize) - return - } - - if !validateChunkSize(chunkData) { - sd.manager.logger.Errorf("CRITICAL: ID token chunk %d will exceed browser cookie limits after encoding (raw size: %d)", i, len(chunkData)) - return - } - - session, err := sd.manager.store.Get(sd.request, sessionName) - if err != nil { - sd.manager.logger.Errorf("CRITICAL: Failed to get chunk session %s: %v", sessionName, err) - return - } - - session.Values["token_chunk"] = chunkData - session.Values["compressed"] = (compressed != token) - session.Values["chunk_created_at"] = time.Now().Unix() - session.Values["token_total"] = len(chunks) - sd.idTokenChunks[i] = session - } - - sd.manager.logger.Debugf("SUCCESS: Stored ID token in %d chunks", len(chunks)) + return } + + // Chunked layout: validate and write every new chunk BEFORE touching + // the previous token, so an abort (request missing, store error, size + // check) leaves the still-valid prior ID token intact (R154; extends + // the R95 access-token fix to the ID token). + chunks := splitIntoChunks(compressed, maxCookieSize) + + if len(chunks) == 0 { + sd.manager.logger.Errorf("CRITICAL: Failed to create chunks for ID token") + return + } + + if len(chunks) > 50 { + sd.manager.logger.Errorf("CRITICAL: Too many chunks (%d) for ID token - possible corruption", len(chunks)) + return + } + + testReassembled := strings.Join(chunks, "") + // R188 cross-chunk integrity: bind all chunks to one another so a mix + // of authentic-but-different chunk sets is caught at assembly (see + // processChunkedToken). Stored on every chunk. + chunkIntegrity := fmt.Sprintf("%x", sha256.Sum256([]byte(testReassembled))) + if testReassembled != compressed { + sd.manager.logger.Errorf("CRITICAL: ID token chunk reassembly test failed") + return + } + + newChunks := make(map[int]*sessions.Session, len(chunks)) + for i, chunkData := range chunks { + sessionName := fmt.Sprintf("%s_%d", sd.manager.idTokenCookieName(), i) + + if sd.request == nil { + sd.manager.logger.Errorf("CRITICAL: SetIDToken: sd.request is nil, cannot create chunk session %s", sessionName) + return + } + + if chunkData == "" { + sd.manager.logger.Debug("Empty chunk data at index %d", i) + return + } + + if len(chunkData) > maxCookieSize { + sd.manager.logger.Info("Chunk %d size %d exceeds maxCookieSize %d", i, len(chunkData), maxCookieSize) + return + } + + if !validateChunkSize(chunkData) { + sd.manager.logger.Errorf("CRITICAL: ID token chunk %d will exceed browser cookie limits after encoding (raw size: %d)", i, len(chunkData)) + return + } + + session, err := sd.manager.store.Get(sd.request, sessionName) + if err != nil { + sd.manager.logger.Errorf("CRITICAL: Failed to get chunk session %s: %v", sessionName, err) + return + } + + session.Values["token_chunk"] = chunkData + session.Values["compressed"] = (compressed != token) + session.Values["chunk_created_at"] = time.Now().Unix() + session.Values["token_total"] = len(chunks) + session.Values["chunk_integrity"] = chunkIntegrity + newChunks[i] = session + } + + // Every new chunk committed: now it is safe to discard the old token. + if sd.request != nil { + sd.expireIDTokenChunksEnhanced(nil) + } + for k := range sd.idTokenChunks { + delete(sd.idTokenChunks, k) + } + if sd.idTokenSession != nil { + sd.idTokenSession.Values["token"] = "" + sd.idTokenSession.Values["compressed"] = (compressed != token) + } + for i, s := range newChunks { + sd.idTokenChunks[i] = s + } + + sd.manager.logger.Debugf("SUCCESS: Stored ID token in %d chunks", len(chunks)) } // GetRedirectCount returns the number of redirects in the current authentication flow. diff --git a/session_chunk_manager.go b/session_chunk_manager.go index 6c98534..4403ed2 100644 --- a/session_chunk_manager.go +++ b/session_chunk_manager.go @@ -3,6 +3,7 @@ package traefikoidc import ( "bytes" "context" + "crypto/sha256" "encoding/base64" "fmt" "strings" @@ -41,10 +42,14 @@ func ResetGlobalSessionCounters() { // Predefined configurations for each token type var ( AccessTokenConfig = TokenConfig{ - Type: "access", - MinLength: 5, - MaxLength: 100 * 1024, - MaxChunks: 25, + Type: "access", + MinLength: 5, + MaxLength: 100 * 1024, + // Must be >= the write path's hard chunk cap (session.go Set*Token + // allows up to 50 chunks) so a token that was written can always be + // read back. A lower read limit silently made written tokens + // unreadable (data loss / forced re-auth) — see R95. + MaxChunks: 50, MaxChunkSize: maxCookieSize, AllowOpaqueTokens: true, RequireJWTFormat: false, @@ -54,7 +59,7 @@ var ( Type: "refresh", MinLength: 5, MaxLength: 50 * 1024, - MaxChunks: 15, + MaxChunks: 50, // R95: match write-path cap (see AccessTokenConfig) MaxChunkSize: maxCookieSize, AllowOpaqueTokens: true, RequireJWTFormat: false, @@ -64,7 +69,7 @@ var ( Type: "id", MinLength: 5, MaxLength: 75 * 1024, - MaxChunks: 20, + MaxChunks: 50, // R95: match write-path cap (see AccessTokenConfig) MaxChunkSize: maxCookieSize, AllowOpaqueTokens: false, RequireJWTFormat: true, @@ -292,9 +297,7 @@ func (cm *ChunkManager) validateToken(token string, config TokenConfig) TokenRet return TokenRetrievalResult{Token: "", Error: sizeErr} } - if chunkErr := cm.validateChunkingEfficiency(token, config); chunkErr != nil { - return TokenRetrievalResult{Token: "", Error: chunkErr} - } + cm.validateChunkingEfficiency(token, config) if contentErr := cm.validateTokenContent(token, config); contentErr != nil { return TokenRetrievalResult{Token: "", Error: contentErr} @@ -365,12 +368,27 @@ func (cm *ChunkManager) processChunkedToken(chunks map[int]*sessions.Session, co // cookie eviction, dropped Set-Cookie, attacker dropping a cookie) would // reassemble a truncated prefix with no error and trust it as the token. // The write path stores token_total on every chunk; check it on chunk 0. - if total, ok := chunks[0].Values["token_total"].(int); ok { - if len(chunks) != total { - err := fmt.Errorf("%s token chunk count mismatch: got %d, expected %d (possibly truncated)", - config.Type, len(chunks), total) - cm.logger.Errorf("Truncated %s token chunks: got %d, expected %d", config.Type, len(chunks), total) - return TokenRetrievalResult{Token: "", Error: err} + // Guard against a sparse set missing index 0 (the assembly loop below + // reports the missing chunk; don't nil-deref here) (R148). + if first, has0 := chunks[0]; has0 && first != nil { + if total, ok := first.Values["token_total"].(int); ok { + if len(chunks) > total { + // Stale higher-index chunks left behind when a token was + // rewritten with fewer chunks (Set*Token writes 0..n-1 but does + // not clear cookies above n). token_total is the authoritative + // count; drop the excess rather than failing — which forced a + // re-auth and discarded the still-valid new token (R129). + n := len(chunks) + for i := total; i < n; i++ { + delete(chunks, i) + } + } + if len(chunks) < total { + err := fmt.Errorf("%s token chunk count mismatch: got %d, expected %d (possibly truncated)", + config.Type, len(chunks), total) + cm.logger.Errorf("Truncated %s token chunks: got %d, expected %d", config.Type, len(chunks), total) + return TokenRetrievalResult{Token: "", Error: err} + } } } @@ -423,6 +441,27 @@ func (cm *ChunkManager) processChunkedToken(chunks map[int]*sessions.Session, co reassembledToken := strings.Join(tokenParts, "") + // Cross-chunk integrity: chunk 0 (and every chunk, on new writes) + // carries a SHA-256 of the assembled chunk string. Each chunk is + // already individually authenticated by its own securecookie MAC, so + // forgery is impossible; this binds the chunks to ONE another, so an + // authentic-but-mixed set (a stale chunk from an earlier session + // combined with chunks from the current one, when counts still match) + // is detected here instead of assembling a wrong token that would only + // surface as a downstream JWT-validation failure. Absent (old + // sessions written before this) → verification is skipped, keeping + // backward compatibility (R188). + if first, has0 := chunks[0]; has0 && first != nil { + if want, ok := first.Values["chunk_integrity"].(string); ok && want != "" { + got := fmt.Sprintf("%x", sha256.Sum256([]byte(reassembledToken))) + if got != want { + err := fmt.Errorf("%s token chunk integrity mismatch: chunks appear to belong to different sessions", config.Type) + cm.logger.Error("Token chunk integrity mismatch for %s: chunks belong to different sessions", config.Type) + return TokenRetrievalResult{Token: "", Error: err} + } + } + } + compressed, _ := chunks[0].Values["compressed"].(bool) if compressed { @@ -601,7 +640,7 @@ func (cm *ChunkManager) validateTokenSize(token string, config TokenConfig) erro } if config.AllowOpaqueTokens && !strings.Contains(token, ".") { - if tokenLen > 8*1024 { + if tokenLen > config.MaxLength { err := fmt.Errorf("%s opaque token unusually large (%d bytes)", config.Type, tokenLen) return err } @@ -610,34 +649,23 @@ func (cm *ChunkManager) validateTokenSize(token string, config TokenConfig) erro return nil } -// validateChunkingEfficiency ensures that chunking is used appropriately. -// It calculates expected chunk counts and warns about potential inefficiencies -// in token storage strategies. -// Parameters: -// - token: The token to analyze for chunking efficiency. -// - config: Token configuration with chunking limits. -// -// Returns: -// - An error if chunking requirements would be violated, nil if acceptable. -func (cm *ChunkManager) validateChunkingEfficiency(token string, config TokenConfig) error { +// validateChunkingEfficiency warns about potential inefficiencies in token +// storage strategies based on the token's length. It is advisory only: the +// actual chunk budget is enforced at write time on the stored (compressed) +// representation (session.go), and the read path size-bounds tokens via +// validateTokenSize. It must NOT hard-reject on the read path, which sees +// the *decompressed* token: a large access token legitimately stored +// compressed (e.g. decompressing to > MaxChunks*MaxChunkSize bytes) would +// otherwise be permanently rejected on every read, forcing re-auth while +// the valid token sits in the browser (R104). +func (cm *ChunkManager) validateChunkingEfficiency(token string, config TokenConfig) { tokenLen := len(token) - if tokenLen <= config.MaxChunkSize && tokenLen <= maxCookieSize { - } - expectedChunks := (tokenLen + config.MaxChunkSize - 1) / config.MaxChunkSize - if expectedChunks > config.MaxChunks { - err := fmt.Errorf("%s token would require %d chunks (max: %d)", - config.Type, expectedChunks, config.MaxChunks) - return err - } - if expectedChunks > 10 && tokenLen < 50*1024 { cm.logger.Info("%s token requires many chunks (%d) for size (%d bytes) - consider token optimization", config.Type, expectedChunks, tokenLen) } - - return nil } // validateTokenContent performs comprehensive token content validation. diff --git a/token_introspection.go b/token_introspection.go index 1ceb03e..7c73f72 100644 --- a/token_introspection.go +++ b/token_introspection.go @@ -79,8 +79,16 @@ func (t *TraefikOidc) introspectToken(token string) (*IntrospectionResponse, err req.Header.Set("Accept", "application/json") // Authenticate using client credentials (per RFC 7662 Section 2.1) - // The introspection endpoint requires authentication - req.SetBasicAuth(t.clientID, t.clientSecret) + // The introspection endpoint requires authentication. + // Snapshot clientID/clientSecret under metadataMu: DCR rewrites them at + // runtime (R137). + clientID, clientSecret, _, _, _ := t.clientCredentials() + // RFC 7662 §2.1 authenticates the introspection call exactly like the + // token and revocation endpoints: form-urlencode client_id and + // client_secret individually before base64 (RFC 6749 §2.3.1), so the + // three outbound auth paths send identical wire credentials for secrets + // containing reserved characters (see setOAuthBasicAuth). + setOAuthBasicAuth(req, clientID, clientSecret) // Send request with circuit breaker if available var resp *http.Response @@ -114,11 +122,19 @@ func (t *TraefikOidc) introspectToken(token string) (*IntrospectionResponse, err } }() - // Check HTTP status + // Check HTTP status. Return a typed *HTTPError so callers can + // distinguish a definite client-side rejection (4xx: the presented + // token is unknown/revoked -> treat like active=false) from a + // transient provider failure (5xx / other), instead of substring- + // matching the message and misclassifying a revoked token as a + // transient error that falls through to ID-token-only auth (R156). if resp.StatusCode != http.StatusOK { limitReader := io.LimitReader(resp.Body, 1024*10) body, _ := io.ReadAll(limitReader) // Safe to ignore: reading error body for diagnostics - return nil, fmt.Errorf("introspection endpoint returned status %d: %s", resp.StatusCode, string(body)) + return nil, &HTTPError{ + StatusCode: resp.StatusCode, + Message: fmt.Sprintf("introspection endpoint returned status %d: %s", resp.StatusCode, string(body)), + } } // Parse response per RFC 7662 Section 2.2 @@ -127,8 +143,14 @@ func (t *TraefikOidc) introspectToken(token string) (*IntrospectionResponse, err return nil, fmt.Errorf("failed to decode introspection response: %w", err) } - // Cache the result - if t.introspectionCache != nil { + // Cache the result. Only positive (active) results are cached so a + // revoked token's negative entry cannot poison the cache for its full TTL + // Cache the result. Only positive (active) results are cached so a + // revoked token's negative entry cannot poison the cache for its full TTL + // and keep rejecting a re-issued token with the same string (R104). + // Inactive results are re-introspected on each request, letting a + // re-issued valid token pass immediately. + if t.introspectionCache != nil && introspectionResp.Active { // Cache for a short duration or until token expiry (whichever is shorter) cacheDuration := 5 * time.Minute // When introspection is REQUIRED, operators expect near-real-time @@ -160,6 +182,9 @@ func (t *TraefikOidc) introspectToken(token string) (*IntrospectionResponse, err // Returns: // - error: Validation error if token is invalid, nil if valid func (t *TraefikOidc) validateOpaqueToken(token string) error { + // Snapshot clientID/audience under metadataMu: DCR rewrites them at + // runtime (R137). + clientID, _, _, audience, _ := t.clientCredentials() // Check if opaque tokens are allowed if !t.allowOpaqueTokens { return fmt.Errorf("opaque tokens are not enabled (set allowOpaqueTokens to true)") @@ -190,6 +215,19 @@ func (t *TraefikOidc) validateOpaqueToken(token string) error { return fmt.Errorf("token is not active (revoked or expired)") } + // An opaque token has no JWT header to classify it, so token_type is + // the only discriminator between an access token and a refresh token. + // An opaque refresh token introspected (or otherwise) as + // active=true, token_type=refresh_token must not be honored as a + // bearer access token (R149). RFC 7662's token_type is the RFC 6749 + // token type, whose value for an access token is "Bearer" (RFC 6750) + // — accept both spellings providers use (R156). Per RFC 7662 + // token_type may be omitted by compliant providers, so only reject on + // a definite non-access match. + if resp.TokenType != "" && resp.TokenType != "access_token" && resp.TokenType != "Bearer" { + return fmt.Errorf("token type %q is not a bearer access token", resp.TokenType) + } + // Validate expiration if present if resp.Exp > 0 { expTime := time.Unix(resp.Exp, 0) @@ -212,12 +250,12 @@ func (t *TraefikOidc) validateOpaqueToken(token string) error { // whose audience cannot be confirmed must not be accepted, otherwise a // token minted for a different audience would pass. aud may be a single // string or an array of strings (RFC 7662); verifyAudience handles both. - if t.audience != "" && t.audience != t.clientID { + if audience != "" && audience != clientID { if resp.Aud == nil { - return fmt.Errorf("invalid audience: expected %s, introspection response has no audience", t.audience) + return fmt.Errorf("invalid audience: expected %s, introspection response has no audience", audience) } - if err := verifyAudience(resp.Aud, t.audience); err != nil { - return fmt.Errorf("invalid audience: expected %s: %w", t.audience, err) + if err := verifyAudience(resp.Aud, audience); err != nil { + return fmt.Errorf("invalid audience: expected %s: %w", audience, err) } } diff --git a/token_manager.go b/token_manager.go index 2900d50..c901b1d 100644 --- a/token_manager.go +++ b/token_manager.go @@ -7,6 +7,8 @@ import ( "context" "crypto/sha256" "encoding/hex" + "encoding/json" + "errors" "fmt" "io" "net/http" @@ -16,6 +18,12 @@ import ( "time" ) +// ErrRateLimitExceeded is returned by verifyTokenWithOpts when the shared +// per-instance token-origin rate limiter is exhausted. Callers (e.g. +// handleCallback) surface it as HTTP 429 + Retry-After rather than a +// generic 500, so burst-driven throttling is not reported as a server error. +var ErrRateLimitExceeded = errors.New("request rate limit exceeded") + // VerifyToken verifies the validity of an ID token or access token. // It performs comprehensive validation including format checks, blacklist verification, // signature validation using JWKs, and standard claims validation. It also caches @@ -32,6 +40,17 @@ func (t *TraefikOidc) VerifyToken(token string) error { return t.verifyTokenWithOpts(token, verifyOpts{}) } +// clientCredentials returns a consistent snapshot of the client credential +// fields under metadataMu. DCR (performDynamicClientRegistration, main.go) +// can rewrite clientID/clientSecret/audience at runtime under metadataMu.Lock, +// so every request-path reader must snapshot these fields under RLock rather +// than read them directly (which would be a data race) (R137). +func (t *TraefikOidc) clientCredentials() (clientID, clientSecret, clientAuthMethod, audience string, clientAssertion *ClientAssertionSigner) { + t.metadataMu.RLock() + defer t.metadataMu.RUnlock() + return t.clientID, t.clientSecret, t.clientAuthMethod, t.audience, t.clientAssertion +} + // verifyOpts are internal-only knobs for verifyTokenWithOpts. Kept unexported // because they expose subtle replay-protection semantics that are dangerous // to misuse. @@ -77,6 +96,19 @@ func (t *TraefikOidc) verifyTokenWithOpts(token string, opts verifyOpts) error { // dozens of times per second by validateStandardTokens. if t.tokenCache != nil { if claims, exists := t.tokenCache.Get(token); exists && len(claims) > 0 { + // A cached hit normally short-circuits the parse, but it must + // still reject a token whose JTI has been blacklisted: a + // rotated token that shares this cached string's JTI (IdP JTI + // reuse) but was revoked under a different raw value would + // otherwise keep returning "valid" until the cache TTL, while + // every cold presentation of it is rejected (R148). + if t.tokenBlacklist != nil && !t.disableReplayDetection { + if jti, ok := claims["jti"].(string); ok && jti != "" { + if b, exists := t.tokenBlacklist.Get(jti); exists && b != nil { + return fmt.Errorf("token replay detected (jti: %s) in cache", jti) + } + } + } return nil } } @@ -87,8 +119,10 @@ func (t *TraefikOidc) verifyTokenWithOpts(token string, opts verifyOpts) error { } tokenType := "UNKNOWN" + clientID := "" + clientID, _, _, _, _ = t.clientCredentials() if aud, ok := parsedJWT.Claims["aud"]; ok { - if audStr, ok := aud.(string); ok && audStr == t.clientID { + if audStr, ok := aud.(string); ok && audStr == clientID { tokenType = "ID_TOKEN" } } @@ -105,18 +139,16 @@ func (t *TraefikOidc) verifyTokenWithOpts(token string, opts verifyOpts) error { if jti, ok := parsedJWT.Claims["jti"].(string); ok && jti != "" { // Skip JTI blacklist check if replay detection is disabled if !t.disableReplayDetection { - if !strings.HasPrefix(token, "eyJhbGciOiJSUzI1NiIsImtpZCI6InRlc3Qta2V5LWlkIiwidHlwIjoiSldUIn0") { - if t.tokenBlacklist != nil { - if blacklisted, exists := t.tokenBlacklist.Get(jti); exists && blacklisted != nil { - return fmt.Errorf("token replay detected (jti: %s) in cache", jti) - } + if t.tokenBlacklist != nil { + if blacklisted, exists := t.tokenBlacklist.Get(jti); exists && blacklisted != nil { + return fmt.Errorf("token replay detected (jti: %s) in cache", jti) } } } } if !t.limiter.Allow() { - return fmt.Errorf("rate limit exceeded") + return fmt.Errorf("%w", ErrRateLimitExceeded) } jwt := parsedJWT @@ -142,7 +174,10 @@ func (t *TraefikOidc) verifyTokenWithOpts(token string, opts verifyOpts) error { expTime := time.Unix(int64(expClaim), 0) tokenDuration := time.Until(expTime) if tokenDuration > 0 && tokenDuration < defaultBlacklistDuration { - expiry = expTime + // Extend by ClockSkewToleranceFuture so the replay entry + // covers the full acceptance window (exp + skew), closing + // the post-exp replay window (mirrors jwt.go, R179). + expiry = expTime.Add(ClockSkewToleranceFuture) } // else: keep default expiry for expired tokens or tokens >24h } @@ -162,13 +197,21 @@ func (t *TraefikOidc) verifyTokenWithOpts(token string, opts verifyOpts) error { initReplayCache() duration := time.Until(expiry) if duration > 0 { - if shardedReplayCache != nil { - shardedReplayCache.Set(jti, true, duration) + // Guard the singleton read with replayCacheMu because + // cleanupReplayCache (jwt.go) can nil shardedReplayCache under + // the write lock. Mirror the locked read in jwt.go + // SetIfAbsent so the pointer deref below cannot race a nil-out. + replayCacheMu.RLock() + sc := shardedReplayCache + if sc != nil { + sc.Set(replayCacheKey(t.issuerURL, jti), true, duration) + replayCacheMu.RUnlock() } else { + replayCacheMu.RUnlock() // Fall back to legacy cache (should rarely happen) replayCacheMu.Lock() if replayCache != nil { - replayCache.Set(jti, true, duration) + replayCache.Set(replayCacheKey(t.issuerURL, jti), true, duration) } replayCacheMu.Unlock() } @@ -195,6 +238,21 @@ func (t *TraefikOidc) verifyToken(token string) error { // - token: The verified token string to cache. // - claims: The map of claims extracted from the verified token. func (t *TraefikOidc) cacheVerifiedToken(token string, claims map[string]interface{}) { + // Defense-in-depth (F1/R133): a concurrent RevokeToken may have + // blacklisted this token (raw or jti) after we passed the top-of- + // verify blacklist check but before we cache. Do not re-populate + // the verified-cache with a revoked token. + if t.tokenBlacklist != nil { + if b, ok := t.tokenBlacklist.Get(token); ok && b != nil { + return + } + if jti, okv := claims["jti"].(string); okv && jti != "" { + if b, ok := t.tokenBlacklist.Get(jti); ok && b != nil { + return + } + } + } + expClaim, ok := claims["exp"].(float64) if !ok { t.safeLogError("Failed to cache token: invalid 'exp' claim type") @@ -204,7 +262,14 @@ func (t *TraefikOidc) cacheVerifiedToken(token string, claims map[string]interfa expirationTime := time.Unix(int64(expClaim), 0) now := time.Now() duration := expirationTime.Sub(now) - t.tokenCache.Set(token, claims, duration) + // Do not cache an already-expired token under a negative TTL. Via the + // ClockSkewToleranceFuture leeway an expired-then-verified token can + // reach here; a negative TTL entry would sit in the cache as dead + // weight (only swept by the periodic cleanup) and its ExpiresAt + // would already be past, so every hit would re-verify anyway (R153). + if duration > 0 { + t.tokenCache.Set(token, claims, duration) + } } // detectTokenType efficiently detects whether a token is an ID token or access token. @@ -226,6 +291,9 @@ func (t *TraefikOidc) detectTokenType(jwt *JWT, token string) bool { sum := sha256.Sum256([]byte(token)) cacheKey := hex.EncodeToString(sum[:]) + // Snapshot clientID under metadataMu: DCR rewrites it at runtime (R137). + clientID, _, _, _, _ := t.clientCredentials() + // Check cache first if t.tokenTypeCache != nil { if cachedType, found := t.tokenTypeCache.Get(cacheKey); found { @@ -306,16 +374,20 @@ func (t *TraefikOidc) detectTokenType(jwt *JWT, token string) bool { // 5. Check if aud == clientID only (ID token pattern) if aud, ok := jwt.Claims["aud"]; ok { // Check string audience - if audStr, ok := aud.(string); ok && audStr == t.clientID { + if audStr, ok := aud.(string); ok && audStr == clientID { isIDToken = true } else if audArr, ok := aud.([]interface{}); ok { - // Check array audience - only treat as ID token if client_id is sole audience - if len(audArr) == 1 { - for _, v := range audArr { - if str, ok := v.(string); ok && str == t.clientID { - isIDToken = true - break - } + // Check array audience - treat as ID token if client_id is + // present anywhere in the array. OIDC ID tokens legitimately + // carry multiple aud entries (e.g. [clientID, API-resource]) + // in multi-audience setups; insisting on client_id being the + // SOLE audience misclassified such ID tokens as access tokens + // and then validated them against the broader 'audience' + // config instead of clientID, rejecting valid logins. + for _, v := range audArr { + if str, ok := v.(string); ok && str == clientID { + isIDToken = true + break } } } @@ -375,20 +447,32 @@ func (t *TraefikOidc) VerifyJWTSignatureAndClaims(jwt *JWT, token string) error } if err := verifySignatureWithKey(token, pubKey, alg); err != nil { - if !t.suppressDiagnosticLogs { - // Microsoft Graph access tokens carry a `nonce` JWT header and are - // signed in a proprietary form Microsoft documents as unverifiable - // by client applications. They reach this path only when the - // per-provider classifier (validateAzureTokens) didn't catch them, - // so log at debug to keep the error stream actionable while still - // surfacing the cause for diagnostics. - if _, isMSProprietary := jwt.Header["nonce"]; isMSProprietary { - t.safeLogDebugf("DIAGNOSTIC: Signature verification failed for kid=%s, alg=%s (Microsoft proprietary nonce header — token is opaque to clients): %v", kid, alg, err) - } else { - t.safeLogErrorf("DIAGNOSTIC: Signature verification failed for kid=%s, alg=%s: %v", kid, alg, err) + // The signature failed against the (possibly stale) cached key. The + // provider may have rotated its signing keys in place, reusing the + // same kid (R109): refresh the JWKS once and retry before failing, + // otherwise a freshly-rotated token is rejected for up to the JWKS + // cache TTL while the stale key remains cached. + if freshKey, ferr := t.jwkCache.getPublicKeyFresh(context.Background(), jwksURL, kid, t.httpClient); ferr == nil { + if verr := verifySignatureWithKey(token, freshKey, alg); verr == nil { + err = nil } } - return fmt.Errorf("signature verification failed: %w", err) + if err != nil { + if !t.suppressDiagnosticLogs { + // Microsoft Graph access tokens carry a `nonce` JWT header and are + // signed in a proprietary form Microsoft documents as unverifiable + // by client applications. They reach this path only when the + // per-provider classifier (validateAzureTokens) didn't catch them, + // so log at debug to keep the error stream actionable while still + // surfacing the cause for diagnostics. + if _, isMSProprietary := jwt.Header["nonce"]; isMSProprietary { + t.safeLogDebugf("DIAGNOSTIC: Signature verification failed for kid=%s, alg=%s (Microsoft proprietary nonce header — token is opaque to clients): %v", kid, alg, err) + } else { + t.safeLogErrorf("DIAGNOSTIC: Signature verification failed for kid=%s, alg=%s: %v", kid, alg, err) + } + } + return fmt.Errorf("signature verification failed: %w", err) + } } if !t.suppressDiagnosticLogs { @@ -399,9 +483,25 @@ func (t *TraefikOidc) VerifyJWTSignatureAndClaims(jwt *JWT, token string) error isIDToken := t.detectTokenType(jwt, token) // Determine expected audience - expectedAudience := t.audience // Default to configured audience + // Snapshot audience/clientID under metadataMu: DCR rewrites them at + // runtime (R137). + clientID, _, _, audience, _ := t.clientCredentials() + expectedAudience := audience // Default to configured audience if isIDToken { - expectedAudience = t.clientID + if t.strictAudienceValidation && audience != "" && audience != clientID { + // R163: strictAudienceValidation's reject-mismatch promise must + // not be silently bypassed by the ID-token classification. + // detectTokenType treats any token whose aud contains clientID + // as an ID token, which would validate it against clientID + // alone — so a token carrying only clientID (and not the + // configured audience) always passed the audience check and + // strict mode never rejected it. When an explicit, distinct + // audience is configured and strict mode is on, enforce the + // configured audience even for such tokens. + expectedAudience = audience + } else { + expectedAudience = clientID + } } if !t.suppressDiagnosticLogs { if isIDToken { @@ -500,11 +600,17 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se if newToken.IDToken == "" { t.logger.Info("Provider did not return a new ID token during refresh") + if !t.persistRotatedRefreshToken(req, rw, session, newToken, "missing ID token") { + return false + } return false } if err = t.verifyToken(newToken.IDToken); err != nil { t.logger.Debug("Failed to verify newly obtained ID token: %v", err) + if !t.persistRotatedRefreshToken(req, rw, session, newToken, "failed ID token verification") { + return false + } return false } @@ -519,19 +625,36 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se claims, err := t.extractClaimsFunc(newToken.IDToken) if err != nil { t.logger.Errorf("refreshToken failed: Failed to extract claims from refreshed token: %v", err) + if !t.persistRotatedRefreshToken(req, rw, session, newToken, "failed to extract claims from refreshed token") { + return false + } return false } - userIdentifier, _ := claims[t.userIdentifierClaim].(string) + userIdentifier, _ := claimScalarString(claims[t.userIdentifierClaim]) if userIdentifier == "" { if t.userIdentifierClaim != "sub" { - userIdentifier, _ = claims["sub"].(string) + userIdentifier, _ = claimScalarString(claims["sub"]) } if userIdentifier == "" { t.logger.Errorf("refreshToken failed: User identifier claim '%s' missing or empty in refreshed token", t.userIdentifierClaim) + if !t.persistRotatedRefreshToken(req, rw, session, newToken, "missing user identifier") { + return false + } return false } t.logger.Debugf("Configured claim '%s' not found in refreshed token, using 'sub' claim as fallback", t.userIdentifierClaim) } + // Bind the refreshed token's identity to the one the session already + // holds (granted at callback after email gate + isAllowedUser). A + // refreshed ID token carrying a different subject must not silently + // substitute the user identity mid-session (R106). + if prev := session.GetUserIdentifier(); prev != "" && prev != userIdentifier { + t.logger.Infof("refreshToken aborted: refreshed token held a different user identifier (%q) than the active session (%q)", userIdentifier, prev) + if !t.persistRotatedRefreshToken(req, rw, session, newToken, "user identifier mismatch") { + return false + } + return false + } session.SetUserIdentifier(userIdentifier) // Get token expiry information for logging @@ -547,6 +670,9 @@ func (t *TraefikOidc) refreshToken(rw http.ResponseWriter, req *http.Request, se // a refresh (an infinite refresh loop). Keep the previous tokens. if newToken.AccessToken == "" { t.logger.Infof("Provider returned an empty access token during refresh; keeping previous tokens") + if !t.persistRotatedRefreshToken(req, rw, session, newToken, "empty access token") { + return false + } return false } @@ -641,6 +767,28 @@ func (t *TraefikOidc) coordinatedTokenRefresh(req *http.Request, refreshToken st // The cache wraps the universal cache, which is Redis-backed in production - // so a "hit" here means another Traefik replica refreshed this same token // within the last few seconds. +// persistRotatedRefreshToken saves a refresh token the IdP just rotated during +// a token exchange, so the session is never left holding a token the exchange +// already consumed. Rotation IdPs (Zitadel, Authentik, Auth0, ...) invalidate +// the presented refresh token on every successful exchange regardless of which +// downstream validation gate (ID token absent, verification failure, +// identifier mismatch, empty access token) subsequently rejects the response. +// If the rotated token were not persisted, the next refresh would present the +// now-consumed token and hit invalid_grant, forcing a full logout. +func (t *TraefikOidc) persistRotatedRefreshToken(req *http.Request, rw http.ResponseWriter, session *SessionData, newToken *TokenResponse, reason string) bool { + if newToken == nil || newToken.RefreshToken == "" { + return true // nothing rotated; nothing to persist + } + session.SetRefreshToken(newToken.RefreshToken) + if err := session.Save(req, rw); err != nil { + t.logger.Errorf("refreshToken failed to persist rotated refresh token after %s: %v", reason, err) + session.SetRefreshToken(newToken.RefreshToken) // keep in-memory for consistency + return false + } + t.logger.Debugf("Persisted rotated refresh token after %s", reason) + return true +} + func (t *TraefikOidc) lookupCachedRefreshResult(sessionID string) (*TokenResponse, bool) { if t.refreshResultCache == nil { return nil, false @@ -649,8 +797,37 @@ func (t *TraefikOidc) lookupCachedRefreshResult(sessionID string) (*TokenRespons if !ok || v == nil { return nil, false } - if tr, ok := v.(*TokenResponse); ok && tr != nil { - return tr, true + // Cross-backend decode (R159): UniversalCache stores the value locally + // as the raw *TokenResponse (stored by cacheRefreshResult) but + // serializes it to JSON for any shared backend (e.g. Redis) via + // deserialize, which unmarshals into a generic map. The old code only + // ever type-asserted *TokenResponse, so every backend read — the + // entire cross-replica path — failed the assert and returned nil, + // making the advertised refresh dedup silently inert (each replica + // still POSTed the refresh token). Normalize any serialized form back + // to *TokenResponse. + switch val := v.(type) { + case *TokenResponse: + return val, true + case map[string]interface{}, []byte, string: + var raw []byte + switch tv := v.(type) { + case []byte: + raw = tv + case string: + raw = []byte(tv) + default: + b, err := json.Marshal(v) + if err != nil { + return nil, false + } + raw = b + } + var tr TokenResponse + if err := json.Unmarshal(raw, &tr); err != nil { + return nil, false + } + return &tr, true } return nil, false } @@ -685,23 +862,76 @@ const refreshResultCacheTTL = 5 * time.Second func (t *TraefikOidc) RevokeToken(token string) { t.tokenCache.Delete(token) + // Evict any cached positive introspection result so a revoked OPAQUE + // access token (which carries no jti and is therefore not covered by + // the JTI blacklist below) is not still served an "active" verdict + // from the introspection cache after revocation. Next request + // re-introspects and sees active=false. + if t.introspectionCache != nil { + t.introspectionCache.Delete(token) + } + + d := blacklistDuration(token) + if jwt, err := parseJWT(token); err == nil { if jti, ok := jwt.Claims["jti"].(string); ok && jti != "" { - expiry := time.Now().Add(24 * time.Hour) if t.tokenBlacklist != nil { - t.tokenBlacklist.Set(jti, true, time.Until(expiry)) + t.tokenBlacklist.Set(jti, true, d) t.logger.Debugf("Locally revoked token JTI %s (added to blacklist)", jti) } } } - expiry := time.Now().Add(24 * time.Hour) if t.tokenBlacklist != nil { - t.tokenBlacklist.Set(token, true, time.Until(expiry)) + t.tokenBlacklist.Set(token, true, d) t.logger.Debugf("Locally revoked token (added to blacklist)") } } +// tokenExpiryUnix extracts the numeric exp claim from a JWT, accepting every +// representation a decoder or cache backend may produce (float64, json.Number, +// int). ok is false when the token is not a JWT or carries no usable exp. +func tokenExpiryUnix(token string) (int64, bool) { + jwt, err := parseJWT(token) + if err != nil { + return 0, false + } + switch v := jwt.Claims["exp"].(type) { + case float64: + return int64(v), true + case float32: + return int64(v), true + case json.Number: + n, err := v.Int64() + if err != nil { + return 0, false + } + return n, true + case int64: + return v, true + case int: + return int64(v), true + } + return 0, false +} + +// blacklistDuration returns how long a revoked token must stay blacklisted. +// A revoked token remains cryptographically valid until its exp, so the +// blacklist must cover the entire remaining validity window — otherwise a +// token with exp more than 24h away could be re-presented (and re-cached) +// after 24h. A 24h floor keeps short-lived or already-expired tokens +// blacklisted for a nominal window. +func blacklistDuration(token string) time.Duration { + const minTTL = 24 * time.Hour + if exp, ok := tokenExpiryUnix(token); ok { + untilExp := time.Until(time.Unix(exp, 0)) + if untilExp > minTTL { + return untilExp + } + } + return minTTL +} + // RevokeTokenWithProvider revokes a token with the OIDC provider. // It sends a revocation request to the provider's revocation endpoint // with proper authentication and error recovery if available. @@ -727,28 +957,40 @@ func (t *TraefikOidc) RevokeTokenWithProvider(token, tokenType string) error { tokenURL := t.tokenURL t.metadataMu.RUnlock() + // Snapshot the client credentials and related metadata under metadataMu: + // DCR (registerWithProvider, main.go) mutates clientID/clientSecret + // under metadataMu.Lock concurrently with logout-time revocation, so + // reading them here without the lock would be a data race (R134). + t.metadataMu.RLock() + clientID := t.clientID + clientSecret := t.clientSecret + clientAuthMethod := t.clientAuthMethod + clientAssertion := t.clientAssertion + issuerURL := t.issuerURL + t.metadataMu.RUnlock() + data := url.Values{ "token": {token}, "token_type_hint": {tokenType}, } // client_id is sent in the body for every method except client_secret_basic, // where it is carried in the Authorization header per RFC 6749 §2.3.1. - if t.clientAuthMethod != "client_secret_basic" || t.clientAssertion != nil { - data.Set("client_id", t.clientID) + if clientAuthMethod != "client_secret_basic" || clientAssertion != nil { + data.Set("client_id", clientID) } useBasicAuth := false - if t.clientAssertion != nil { - assertion, err := t.clientAssertion.Sign(tokenURL, t.clientID) + if clientAssertion != nil { + assertion, err := clientAssertion.Sign(tokenURL, clientID) if err != nil { return fmt.Errorf("failed to sign client assertion: %w", err) } data.Set("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer") data.Set("client_assertion", assertion) - } else if t.clientAuthMethod == "client_secret_basic" { + } else if clientAuthMethod == "client_secret_basic" { useBasicAuth = true } else { - data.Set("client_secret", t.clientSecret) + data.Set("client_secret", clientSecret) } req, err := http.NewRequestWithContext(context.Background(), "POST", revocationURL, strings.NewReader(data.Encode())) @@ -759,16 +1001,13 @@ func (t *TraefikOidc) RevokeTokenWithProvider(token, tokenType string) error { req.Header.Set("Content-Type", "application/x-www-form-urlencoded") req.Header.Set("Accept", "application/json") if useBasicAuth { - setOAuthBasicAuth(req, t.clientID, t.clientSecret) + setOAuthBasicAuth(req, clientID, clientSecret) } // Send the request with circuit breaker protection if available var resp *http.Response if t.errorRecoveryManager != nil { - // Read issuerURL with RLock for service name - t.metadataMu.RLock() - serviceName := fmt.Sprintf("token-revocation-%s", t.issuerURL) - t.metadataMu.RUnlock() + serviceName := fmt.Sprintf("token-revocation-%s", issuerURL) err = t.errorRecoveryManager.ExecuteWithRecovery(context.Background(), serviceName, func() error { var reqErr error resp, reqErr = t.httpClient.Do(req) //nolint:bodyclose // Body is closed in defer after error check @@ -981,7 +1220,10 @@ func stringListFromClaim(value interface{}) ([]string, bool) { case []interface{}: out := make([]string, 0, len(v)) for _, item := range v { - if s, ok := item.(string); ok { + // Scalar items are stringified (strings and numbers), so a + // numeric group/role is preserved instead of being silently + // dropped; composite items (maps, nested arrays) are skipped. + if s, ok := claimScalarString(item); ok { out = append(out, s) } } @@ -1000,24 +1242,23 @@ func (t *TraefikOidc) extractGroupsAndRolesFromClaims(claims map[string]interfac var roles []string if groupsClaim, exists := claims[t.groupClaimName]; exists { - groupsSlice, ok := stringListFromClaim(groupsClaim) - if !ok { - return nil, nil, fmt.Errorf("%s claim is not an array", t.groupClaimName) - } - for _, groupStr := range groupsSlice { - t.logger.Debugf("Found group from %s claim: %s", t.groupClaimName, groupStr) - groups = append(groups, groupStr) + if groupsSlice, ok := stringListFromClaim(groupsClaim); ok { + groups = append(groups, groupsSlice...) + for _, groupStr := range groupsSlice { + t.logger.Debugf("Found group from %s claim: %s", t.groupClaimName, groupStr) + } } + // A malformed/null group claim (numeric scalar, JSON null) is treated + // as empty rather than a hard error (R96): one bad claim must not + // suppress the valid sibling role signal. } if rolesClaim, exists := claims[t.roleClaimName]; exists { - rolesSlice, ok := stringListFromClaim(rolesClaim) - if !ok { - return nil, nil, fmt.Errorf("%s claim is not an array", t.roleClaimName) - } - for _, roleStr := range rolesSlice { - t.logger.Debugf("Found role from %s claim: %s", t.roleClaimName, roleStr) - roles = append(roles, roleStr) + if rolesSlice, ok := stringListFromClaim(rolesClaim); ok { + roles = append(roles, rolesSlice...) + for _, roleStr := range rolesSlice { + t.logger.Debugf("Found role from %s claim: %s", t.roleClaimName, roleStr) + } } } diff --git a/token_resilience.go b/token_resilience.go index 19e4a8c..a55b344 100644 --- a/token_resilience.go +++ b/token_resilience.go @@ -118,18 +118,15 @@ func (trm *TokenResilienceManager) ExecuteTokenOperation(ctx context.Context, op return fn() } - // Compose resilience mechanisms + // Compose resilience mechanisms. Retry is the INNER layer (around the + // operation) and the circuit breaker is the OUTER layer, so the breaker + // observes one aggregate result per logical operation rather than one + // failure per retry attempt (which would trip it prematurely — e.g. + // maxFailures=3 + 3 retries opening the breaker on a single brief + // outage, failing the very next genuine request). R106. var finalOperation func() error = fn - // Wrap with circuit breaker if enabled - if trm.config.CircuitBreakerEnabled && trm.circuitBreaker != nil { - originalOp := finalOperation - finalOperation = func() error { - return trm.circuitBreaker.ExecuteWithContext(ctx, originalOp) - } - } - - // Wrap with retry if enabled + // Wrap with retry if enabled (innermost layer) if trm.config.RetryEnabled && trm.retryExecutor != nil { originalOp := finalOperation if singleUse { @@ -143,6 +140,14 @@ func (trm *TokenResilienceManager) ExecuteTokenOperation(ctx context.Context, op } } + // Wrap with circuit breaker if enabled (outermost layer) + if trm.config.CircuitBreakerEnabled && trm.circuitBreaker != nil { + originalOp := finalOperation + finalOperation = func() error { + return trm.circuitBreaker.ExecuteWithContext(ctx, originalOp) + } + } + err := finalOperation() if err != nil && trm.logger != nil { @@ -174,7 +179,12 @@ func (trm *TokenResilienceManager) ExecuteTokenRefresh(ctx context.Context, t *T var result *TokenResponse var err error - err = trm.ExecuteTokenOperation(ctx, "token_refresh", false, func() error { + // Refresh is single-use (like the auth-code exchange): a client-side timeout + // may mean the provider already processed AND rotated the refresh token, so + // re-presenting it on a retry surfaces invalid_grant and the session is + // cleared — escalating a transient timeout into a full logout. Single-use + // retries only on "never sent" errors (connect/dial), never on timeout. + err = trm.ExecuteTokenOperation(ctx, "token_refresh", true, func() error { // Call exchangeTokens directly to avoid recursion back to getNewTokenWithRefreshToken // which would call ExecuteTokenRefresh again, causing infinite loop (issue #67) result, err = t.exchangeTokens(ctx, "refresh_token", refreshToken, "", "") diff --git a/token_validation_rs.go b/token_validation_rs.go index 2eace8b..1d26a28 100644 --- a/token_validation_rs.go +++ b/token_validation_rs.go @@ -15,6 +15,7 @@ package traefikoidc import ( "encoding/base64" "encoding/json" + "errors" "strings" "time" ) @@ -81,6 +82,19 @@ func (t *TraefikOidc) accessTokenUnexpired(token string) bool { // validateTokenExpiryRS is the requestState-aware variant of validateTokenExpiry. // Reads rs.refreshToken instead of session.GetRefreshToken() (4 RLocks avoided). func (t *TraefikOidc) validateTokenExpiryRS(rs *requestState, token string) (bool, bool, bool) { + // Defense-in-depth (F2/R133): this is the final authenticated gate. + // Every current caller has already passed the blacklist check via + // verifyToken, but guard independently so a revoked-but-still-cached + // token can never be served authenticated if the call order changes. + if t.tokenBlacklist != nil { + if b, ok := t.tokenBlacklist.Get(token); ok && b != nil { + if rs.refreshToken != "" { + return false, true, false + } + return false, false, true + } + } + cachedClaims, found := t.tokenCache.Get(token) if !found { t.logger.Debug("Claims not found in cache after successful token verification") @@ -102,7 +116,13 @@ func (t *TraefikOidc) validateTokenExpiryRS(rs *requestState, token string) (boo expTimeObj := time.Unix(int64(expClaim), 0) nowObj := time.Now() - if expTimeObj.Before(nowObj) { + // Apply the same clock-skew leeway as jwt.Verify's verifyExpiration + // (ClockSkewToleranceFuture). This is the final auth gate run after + // successful signature verification; without the leeway a token + // within the 2-minute post-exp window was granted by Verify but + // downgraded to here as expired, forcing a needless refresh or + // re-auth (R124). + if nowObj.After(expTimeObj.Add(ClockSkewToleranceFuture)) { if rs.refreshToken != "" { return false, true, false } @@ -177,6 +197,19 @@ func (t *TraefikOidc) validateStandardTokensRS(rs *requestState) (bool, bool, bo } return false, false, true } + // A definitive 4xx from the introspection endpoint (e.g. 401 + // for an unknown/revoked token) means the token itself is bad, + // not that the endpoint transiently failed. Treat it as + // invalid (refresh) rather than falling through to + // ID-token-only auth, which would authenticate a revoked + // opaque token (R156). + var httpErr *HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode >= 400 && httpErr.StatusCode < 500 { + if rs.refreshToken != "" { + return false, true, false + } + return false, false, true + } if t.requireTokenIntrospection { if rs.refreshToken != "" { return false, true, false @@ -231,7 +264,9 @@ func (t *TraefikOidc) validateStandardTokensRS(rs *requestState) (bool, bool, bo // JWT access token present. accessTokenValid := false lenientAudienceOnly := false + var accessVerifyErr error if err := t.verifyToken(rs.accessToken); err != nil { + accessVerifyErr = err errMsg := err.Error() if strings.Contains(errMsg, "invalid audience") || strings.Contains(errMsg, "audience") { if t.strictAudienceValidation { @@ -298,6 +333,21 @@ func (t *TraefikOidc) validateStandardTokensRS(rs *requestState) (bool, bool, bo if accessTokenValid { return t.validateTokenExpiryRS(rs, rs.accessToken) } + // Access token is a JWT whose verification failed, yet the session's + // ID token is still valid. If the failure was specifically EXPIRY — + // verifyToken checks the signature before exp, so "token has expired" + // implies the signature and other claims validated — the access token + // is unusable at the upstream resource. Refresh to obtain a fresh one + // rather than forwarding a stale access token (R131). Without this an + // expired access token (with a still-valid ID token) was authenticated + // with needsRefresh=false and forwarded as-is until the ID token + // itself neared expiry, giving the backend an expired bearer token. + if accessVerifyErr != nil && strings.Contains(accessVerifyErr.Error(), "token has expired") { + if rs.refreshToken != "" { + return true, true, false + } + return false, false, true + } return t.validateTokenExpiryRS(rs, rs.idToken) } @@ -326,6 +376,41 @@ func (t *TraefikOidc) validateAzureTokensRS(rs *requestState) (bool, bool, bool) } return t.validateTokenExpiryRS(rs, rs.idToken) } + // No ID token to corroborate the Azure token. Its signature + // can't be verified client-side, but it still carries exp; + // authenticate only while unexpired. Previously this + // returned authenticated=true with no expiry check, so an + // expired (or unparseable) Azure access token authenticated + // indefinitely until the session itself expired (R129). + if claims, err := extractClaims(rs.accessToken); err != nil { + if rs.refreshToken != "" { + return false, true, false + } + return false, false, true + } else { + // Defense-in-depth (mirrors validateTokenExpiryRS): a + // revoked-but-still-cached token must not authenticate + // through this unverifiable branch either (R147). + if t.tokenBlacklist != nil { + if b, ok := t.tokenBlacklist.Get(rs.accessToken); ok && b != nil { + if rs.refreshToken != "" { + return false, true, false + } + return false, false, true + } + } + // exp must be present and numeric to establish the token + // is current; an unverifiable token with no (or + // non-numeric) exp authenticates with zero verification, + // so fail closed when it can't be bounded (R130). + exp, ok := claims["exp"].(float64) + if !ok || time.Now().After(time.Unix(int64(exp), 0).Add(ClockSkewToleranceFuture)) { + if rs.refreshToken != "" { + return false, true, false + } + return false, false, true + } + } return true, false, false } if err := t.verifyToken(rs.accessToken); err != nil { @@ -349,7 +434,17 @@ func (t *TraefikOidc) validateAzureTokensRS(rs *requestState) (bool, bool, bool) if rs.idToken != "" { return t.validateTokenExpiryRS(rs, rs.idToken) } - return true, false, false + // Opaque access token with no ID token to corroborate it. Do not + // authenticate on an unverified (or unverifiable) token: refresh + // if a refresh token is available, otherwise force + // re-authentication. Mirrors validateStandardTokensRS, which + // documents this exact decision ("Do NOT authenticate on an + // unverified token"). The previous `return true, false, false` + // accepted any opaque value here with no verification (R99). + if rs.refreshToken != "" { + return false, true, false + } + return false, false, true } if rs.idToken != "" { diff --git a/url_helpers.go b/url_helpers.go index dd6ee3c..75789ad 100644 --- a/url_helpers.go +++ b/url_helpers.go @@ -75,8 +75,11 @@ func pathExcluded(requestPath, excluded string) bool { // Returns: // - The fully constructed authorization URL string. func (t *TraefikOidc) buildAuthURL(redirectURL, state, nonce, codeChallenge string) string { + // Snapshot clientID/audience under metadataMu: DCR rewrites them at + // runtime (R137). + clientID, _, _, audience, _ := t.clientCredentials() params := url.Values{} - params.Set("client_id", t.clientID) + params.Set("client_id", clientID) params.Set("response_type", "code") params.Set("redirect_uri", redirectURL) params.Set("state", state) @@ -86,9 +89,9 @@ func (t *TraefikOidc) buildAuthURL(redirectURL, state, nonce, codeChallenge stri // This allows access tokens to have the correct audience claim // Only add if audience is configured and different from client_id // ID tokens will always have aud=client_id per OIDC spec - if t.audience != "" && t.audience != t.clientID { - params.Set("audience", t.audience) - t.logger.Debugf("Adding audience parameter to authorize URL: %s", t.audience) + if audience != "" && audience != clientID { + params.Set("audience", audience) + t.logger.Debugf("Adding audience parameter to authorize URL: %s", audience) } if t.enablePKCE && codeChallenge != "" { @@ -224,6 +227,22 @@ func (t *TraefikOidc) buildAuthURL(redirectURL, state, nonce, codeChallenge stri // // Returns: // - The fully constructed URL string with appended query parameters. +// +// preserveBaseQuery seeds url parameters from the base URL's pre-existing +// query string, without overriding authoritative plugin-supplied params. +// Some IdPs embed client_id or template params in their discovered +// authorization/end-session endpoints; overwriting RawQuery dropped them +// (R101). +func (t *TraefikOidc) preserveBaseQuery(u *url.URL, params url.Values) { + for k, vs := range u.Query() { + if params.Get(k) == "" { + for _, v := range vs { + params.Add(k, v) + } + } + } +} + func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) string { if baseURL != "" { if strings.HasPrefix(baseURL, "http://") || strings.HasPrefix(baseURL, "https://") { @@ -259,6 +278,7 @@ func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) stri return "" } + t.preserveBaseQuery(resolvedURL, params) resolvedURL.RawQuery = params.Encode() return resolvedURL.String() } @@ -274,6 +294,7 @@ func (t *TraefikOidc) buildURLWithParams(baseURL string, params url.Values) stri return "" } + t.preserveBaseQuery(u, params) u.RawQuery = params.Encode() return u.String() } @@ -359,6 +380,14 @@ func (t *TraefikOidc) validateDiscoveredEndpoint(urlStr string, allowLoopback bo if u.Scheme != "https" && u.Scheme != "http" { return fmt.Errorf("disallowed URL scheme: %q", u.Scheme) } + // Pin the discovered endpoint's scheme to the provider's own scheme. A + // provider served over HTTPS must hand out HTTPS endpoints; a legacy or + // poisoned discovery document that downgrades a credential-bearing + // endpoint (client_secret, tokens) to plaintext HTTP would otherwise be + // used as-is, shipping secrets over an unauthenticated channel (R146). + if u.Scheme == "http" && t.providerUsesHTTPS() { + return fmt.Errorf("discovered endpoint uses plaintext http while the provider is https; refusing scheme downgrade") + } if u.Host == "" { return fmt.Errorf("missing host in URL") } @@ -371,6 +400,15 @@ func (t *TraefikOidc) validateDiscoveredEndpoint(urlStr string, allowLoopback bo case ip.IsPrivate() && !t.allowPrivateIPAddresses: return fmt.Errorf("endpoint host is a private address: %s", ip) } + } else if err := t.validateHost(u.Hostname()); err != nil && !(allowLoopback && isLoopbackHost(u.Hostname())) { + // Name-based hosts (localhost, metadata.google.internal, any + // hostname) bypass the IP-literal checks above; run the hostname + // SSRF guard so a discovery doc pointing at such a host is + // rejected (R97). Loopback names (localhost) are permitted when + // allowLoopback is set, mirroring the IP-literal branch; hard + // metadata-service names (metadata.google.internal) are never + // allowed because they are not loopback. + return fmt.Errorf("discovered endpoint host is not allowed: %w", err) } if strings.Contains(u.Path, "..") { return fmt.Errorf("path traversal detected in URL path") @@ -382,6 +420,14 @@ func (t *TraefikOidc) validateDiscoveredEndpoint(urlStr string, allowLoopback bo // Used to pin the credential-bearing introspection endpoint to the operator- // configured provider so a poisoned discovery document cannot redirect the // client secret to an attacker-controlled host. +// providerUsesHTTPS reports whether the operator-configured provider URL is +// served over HTTPS. Used to pin discovered endpoint schemes. +func (t *TraefikOidc) providerUsesHTTPS() bool { + t.metadataMu.RLock() + defer t.metadataMu.RUnlock() + return t.providerURL != "" && strings.HasPrefix(strings.ToLower(t.providerURL), "https://") +} + func sameHost(a, b string) bool { ua, erra := url.Parse(a) ub, errb := url.Parse(b) @@ -454,3 +500,24 @@ func (t *TraefikOidc) validateHost(host string) error { return nil } + +// redactCallbackURL returns the callback URL with the one-time +// authorization code (and state) query values redacted. The single-use +// code is a quiet credential: logging the full callback URL (as the +// per-request Debug line and the CSRF-failure Error line did) would +// publish it into the log pipeline (R153). Malformed URLs are returned +// unchanged. +func redactCallbackURL(rawURL string) string { + u, err := url.Parse(rawURL) + if err != nil { + return rawURL + } + q := u.Query() + for _, k := range []string{"code", "state"} { + if q.Get(k) != "" { + q.Set(k, "[REDACTED]") + } + } + u.RawQuery = q.Encode() + return u.String() +}