From 2ce36054545ce85dde324ac24c4b0b2428e6ee2b Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 19:29:43 +0100 Subject: [PATCH 1/4] Refuse GET and HEAD on discovery settings and SAML logout Two routes registered without a mux method pattern changed state on any method. The demo-mode read-only guard and the CSRF check both admit GET and HEAD as reads, and the SameSite=Lax session cookie rides cross-site top-level GET navigations, so each was writable by a safe method: - /api/discovery/settings had no method check, so a GET or HEAD carrying a JSON body changed the discovery staleness threshold, including on a DEMO_MODE instance. - SP-initiated SAML logout at /api/saml/{id}/logout cleared the session and redirected to the IdP on GET. Nothing in the UI calls it, the /api/logout fallback it delegates to is already POST-only, and the IdP's LogoutResponse returns to /slo, not here. Add requireRequestMethod (405 with Allow) as the shared handler-level guard. Discovery settings keeps PUT, its documented method, and POST, which it accepted before; SAML logout takes POST only. An audit of every route in internal/api found no other caller-requested write reachable by GET or HEAD beyond the Assistant session routes guarded in #2670. GET flows the protocol requires (OIDC login and callback, SAML login initiation and SLO, magic-link and cloud handoff, checkout bridges) and reads that run lazy initialization or idempotent reconciliation are left as they are. One known exception is deferred: single-resource report generation still runs on GET and can call the configured AI narrator, which records provider usage in the cost ledger; moving it behind POST changes the report API the UI downloads from, so it is tracked separately. --- .../v6/internal/subsystems/agent-lifecycle.md | 9 +++ .../v6/internal/subsystems/api-contracts.md | 34 +++++++++ .../internal/subsystems/storage-recovery.md | 9 +++ internal/api/ai_handlers_more_test.go | 73 +++++++++++++++++++ internal/api/discovery_handlers.go | 5 ++ internal/api/discovery_handlers_test.go | 35 +++++++++ internal/api/method_guard.go | 23 ++++++ internal/api/saml_handlers.go | 7 +- internal/api/saml_handlers_more_test.go | 70 +++++++++++++++++- .../api/saml_runtime_license_gating_test.go | 2 +- 10 files changed, 261 insertions(+), 6 deletions(-) create mode 100644 internal/api/method_guard.go diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index a2ee20031e..6f04936296 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8927,6 +8927,15 @@ their own heartbeat cutoff (`fleethealth.AgentStaleThreshold`, which is separate from the monitoring reporting lease). Agent registration, enrolment, install, update, removal and report identity are unchanged. +### Safe methods refused on discovery settings and SAML logout + +`internal/api/method_guard.go`, `internal/api/discovery_handlers.go` and +`internal/api/saml_handlers.go` changed only so `/api/discovery/settings` and +`/api/saml/{id}/logout` answer `405` to every method outside their allowed set, +`GET` and `HEAD` included. The settings write still takes `PUT` or `POST` and +logout still takes `POST`. No agent registration, enrolment, install, update or +removal path changed. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index f415badc7d..361bbedaa8 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -3896,6 +3896,13 @@ counters exist to measure. onboarding telemetry. Customer diagnostics may expose runtime health, supportability, and sanitized troubleshooting state; admin analytics must stay behind admin-owned metrics routes. +25. Handlers that perform a caller-requested write (settings, sessions, + credentials, workflow or resource state) while answering `GET` or `HEAD`. + A route registered without a mux method pattern, or a sub-resource + dispatched by path, must refuse safe methods in the mutating handler + through `requireRequestMethod` in `internal/api/method_guard.go` (or an + equivalent explicit check) instead of relying on its callers, the + demo-mode read-only guard, or the CSRF check. ## Completion Obligations @@ -5136,6 +5143,33 @@ pass the bounded non-symlink regular-file boundary in `internal/api/sso_outbound.go`. Cloud handoff redirects consume the shared host-local redirect validator rather than carrying route-local prefix checks. +A handler that performs a caller-requested write behind a route whose mux +pattern carries no method must refuse `GET` and `HEAD` itself. +`requireRequestMethod` in `internal/api/method_guard.go` is the shared +handler-level guard: it admits only the listed methods and answers any other +method that reaches the handler with `405` and an `Allow` header naming them. +The demo-mode read-only guard and the CSRF check both admit `GET` and `HEAD` as +reads, and the `SameSite=Lax` session cookie rides cross-site top-level `GET` +navigations, so the method contract belongs to the mutating handler rather than +to its callers. `/api/discovery/settings` accepts `PUT`, its documented method, +and `POST`, which it took before the guard existed. SP-initiated SAML logout at +`/api/saml/{id}/logout` accepts only `POST`, matching the `/api/logout` +fallback it delegates to. The IdP's LogoutResponse keeps returning to +`/api/saml/{id}/slo`, which must accept the HTTP-Redirect binding's `GET` and +validates the signed response before it touches the session. +`TestDemoModeSafeMethodsCannotReachRouteMutations` in +`internal/api/ai_handlers_more_test.go` drives both routes through the full +`DEMO_MODE` router and proves the threshold and the session survive. +The rule does not cover `GET` flows the protocol or the browser handoff +requires (OIDC login and callback, SAML login initiation and SLO, magic-link +verify, cloud handoff, the checkout start and activation bridges, websocket +handshakes), or reads that run one-time lazy initialization or idempotent +reconciliation, such as the license and onboarding-overflow bootstrap behind +the entitlement reads and the action-expiry sweep behind the action reads. +Single-resource report generation at `GET /api/admin/reports/generate` still +runs the configured AI narrator, a paid provider call recorded in the cost +ledger; that is a known open exception, not part of this contract. + Alert delivery diagnosis is a read-only monitoring API contract. `GET /api/alerts/delivery-diagnosis?alertIdentifier=` returns the alert manager's current delivery-policy projection for one active alert, including diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index d1199c9224..d5112ce305 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6867,6 +6867,15 @@ The performance report and reporting runtime snapshot handlers apply the same test before tabulating a disk temperature. That changes only which held value a report shows, not any storage or recovery path. +### Safe methods refused on discovery settings and SAML logout + +`internal/api/method_guard.go` adds `requireRequestMethod`, and +`internal/api/discovery_handlers.go` and `internal/api/saml_handlers.go` use it +so `/api/discovery/settings` (`PUT` or `POST`) and `/api/saml/{id}/logout` +(`POST`) answer `405` to any other method, `GET` and `HEAD` included, instead +of changing the discovery staleness threshold or clearing a session. No +storage, retention, backup, migration or recovery path is added or moved. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` no longer exempts every request carrying diff --git a/internal/api/ai_handlers_more_test.go b/internal/api/ai_handlers_more_test.go index 52339f4893..d4838850f2 100644 --- a/internal/api/ai_handlers_more_test.go +++ b/internal/api/ai_handlers_more_test.go @@ -22,6 +22,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/monitoring" "github.com/rcourtman/pulse-go-rewrite/internal/servicediscovery" unifiedresources "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" + internalauth "github.com/rcourtman/pulse-go-rewrite/pkg/auth" ) type stubMetadataProvider struct{} @@ -521,6 +522,78 @@ func TestAISettingsHandler_DiscoveryStoreAccessors(t *testing.T) { } } +// In DEMO_MODE the demo middleware admits every GET and HEAD as a read, and +// the CSRF check skips them, so a mutating handler reachable by a safe method +// is a write any demo visitor can make. These routes dispatch without a mux +// method pattern, so each handler must refuse GET and HEAD itself. +func TestDemoModeSafeMethodsCannotReachRouteMutations(t *testing.T) { + dataPath := t.TempDir() + hashed, err := internalauth.HashPassword("demo-router-test-password") + if err != nil { + t.Fatalf("HashPassword: %v", err) + } + cfg := &config.Config{ + DataPath: dataPath, + ConfigPath: dataPath, + DemoMode: true, + AuthUser: "demo", + AuthPass: hashed, + } + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + cleanupTestRouter(t, router) + handler := router.Handler() + + discovery := servicediscovery.NewService(nil, nil, servicediscovery.DefaultConfig()) + router.SetDiscoveryService(discovery) + ageBefore := discovery.GetMaxDiscoveryAge() + + router.samlManager.services["okta"] = newTestSAMLService(t, "okta", ` + + + + + +`) + samlSession := generateSessionToken() + GetSessionStore().CreateSAMLSession(samlSession, time.Hour, "agent", "127.0.0.1", "demo", &SAMLTokenInfo{ + ProviderID: "okta", + NameID: "name-id", + SessionIndex: "sess-1", + }) + + cases := []struct { + path string + body string + allow string + }{ + {path: "/api/discovery/settings", body: `{"max_discovery_age_days":10}`, allow: "PUT, POST"}, + {path: "/api/saml/okta/logout", allow: http.MethodPost}, + } + for _, tc := range cases { + for _, method := range []string{http.MethodGet, http.MethodHead} { + req := httptest.NewRequest(method, tc.path, strings.NewReader(tc.body)) + req.SetBasicAuth("demo", "demo-router-test-password") + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: samlSession}) + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("%s %s status = %d, want %d: %s", method, tc.path, rec.Code, http.StatusMethodNotAllowed, rec.Body.String()) + } + if allow := rec.Header().Get("Allow"); allow != tc.allow { + t.Fatalf("%s %s Allow = %q, want %q", method, tc.path, allow, tc.allow) + } + } + } + + if got := discovery.GetMaxDiscoveryAge(); got != ageBefore { + t.Fatalf("safe methods changed the max discovery age: %s -> %s", ageBefore, got) + } + if !ValidateSession(samlSession) { + t.Fatal("safe methods cleared the SAML session") + } +} + func TestAISettingsHandler_GetConfig_NonDefaultFallsBackWhenMultiTenantUnavailable(t *testing.T) { handler := newTestAISettingsHandler(&config.Config{APIToken: "token"}, config.NewConfigPersistence(t.TempDir()), nil) ctx := context.WithValue(context.Background(), OrgIDContextKey, "acme") diff --git a/internal/api/discovery_handlers.go b/internal/api/discovery_handlers.go index 54dff9e5c7..f77923bb17 100644 --- a/internal/api/discovery_handlers.go +++ b/internal/api/discovery_handlers.go @@ -881,7 +881,12 @@ func (h *DiscoveryHandlers) HandleRunDiscovery(w http.ResponseWriter, r *http.Re // HandleUpdateSettings handles PUT /api/discovery/settings // Allows updating discovery settings like the staleness threshold. +// PUT is the documented method. POST stays accepted because the route took any +// method before; GET and HEAD are refused so a read can never write. func (h *DiscoveryHandlers) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { + if !requireRequestMethod(w, r, http.MethodPut, http.MethodPost) { + return + } if h.service == nil { writeDiscoveryError(w, http.StatusServiceUnavailable, "discovery service not configured") return diff --git a/internal/api/discovery_handlers_test.go b/internal/api/discovery_handlers_test.go index 4f099eefb2..0fa57d3fce 100644 --- a/internal/api/discovery_handlers_test.go +++ b/internal/api/discovery_handlers_test.go @@ -476,6 +476,41 @@ func TestHandleUpdateSettings(t *testing.T) { // We can't check service private field easily, but we check 200 OK. } +// GET and HEAD pass the demo-mode read-only guard and the CSRF check, so the +// settings write must refuse them even when they carry a JSON body. +func TestHandleUpdateSettingsRefusesSafeMethods(t *testing.T) { + h, service, _ := setupDiscoveryHandlers(t) + before := service.GetMaxDiscoveryAge() + reqBody := `{"max_discovery_age_days": 10}` + if before == 10*24*time.Hour { + t.Fatal("test body must change the default max discovery age") + } + + for _, method := range []string{http.MethodGet, http.MethodHead} { + req := httptest.NewRequest(method, "/api/discovery/settings", bytes.NewBufferString(reqBody)) + req.SetBasicAuth("admin", "admin") + w := httptest.NewRecorder() + + h.HandleUpdateSettings(w, req) + + assert.Equal(t, http.StatusMethodNotAllowed, w.Code, method) + assert.Equal(t, "PUT, POST", w.Header().Get("Allow"), method) + assert.Equal(t, before, service.GetMaxDiscoveryAge(), method) + } + + for _, method := range []string{http.MethodPut, http.MethodPost} { + service.SetMaxDiscoveryAge(before) + req := httptest.NewRequest(method, "/api/discovery/settings", bytes.NewBufferString(reqBody)) + req.SetBasicAuth("admin", "admin") + w := httptest.NewRecorder() + + h.HandleUpdateSettings(w, req) + + assert.Equal(t, http.StatusOK, w.Code, method) + assert.Equal(t, 10*24*time.Hour, service.GetMaxDiscoveryAge(), method) + } +} + func TestHandleListByType(t *testing.T) { h, _, store := setupDiscoveryHandlers(t) diff --git a/internal/api/method_guard.go b/internal/api/method_guard.go new file mode 100644 index 0000000000..9e73728668 --- /dev/null +++ b/internal/api/method_guard.go @@ -0,0 +1,23 @@ +package api + +import ( + "net/http" + "strings" +) + +// requireRequestMethod admits only the listed methods to a handler that +// changes state, answering anything else with 405 and an Allow header. Bare +// mux patterns and path-dispatched routes do not filter methods, and GET/HEAD +// pass both the demo-mode read-only guard and the CSRF check while +// SameSite=Lax session cookies ride cross-site top-level GET navigations, so a +// mutating handler must refuse safe methods itself. +func requireRequestMethod(w http.ResponseWriter, r *http.Request, allowed ...string) bool { + for _, method := range allowed { + if r.Method == method { + return true + } + } + w.Header().Set("Allow", strings.Join(allowed, ", ")) + http.Error(w, "Method not allowed", http.StatusMethodNotAllowed) + return false +} diff --git a/internal/api/saml_handlers.go b/internal/api/saml_handlers.go index f4706ec4a8..6ce686e585 100644 --- a/internal/api/saml_handlers.go +++ b/internal/api/saml_handlers.go @@ -377,8 +377,13 @@ func (r *Router) handleSAMLMetadata(w http.ResponseWriter, req *http.Request) { w.Write(metadata) } -// handleSAMLLogout handles SAML Single Logout +// handleSAMLLogout handles SP-initiated SAML Single Logout. It is POST-only, +// like the /api/logout fallback: the IdP's LogoutResponse returns to /slo, not +// here, so a GET to this path could only be a forced logout. func (r *Router) handleSAMLLogout(w http.ResponseWriter, req *http.Request) { + if !requireRequestMethod(w, req, http.MethodPost) { + return + } providerID := extractSAMLProviderID(req.URL.Path, "logout") if providerID == "" { // Fall back to regular logout diff --git a/internal/api/saml_handlers_more_test.go b/internal/api/saml_handlers_more_test.go index ff7ac9f9ac..10c04637bd 100644 --- a/internal/api/saml_handlers_more_test.go +++ b/internal/api/saml_handlers_more_test.go @@ -247,7 +247,9 @@ func TestHandleSAMLSLO_RejectsIDPInitiatedLogoutRequest(t *testing.T) { } func TestHandleSAMLLogout_SLOUnavailable(t *testing.T) { - InitSessionStore(t.TempDir()) + dataPath := t.TempDir() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} metadataXML := ` @@ -265,7 +267,7 @@ func TestHandleSAMLLogout_SLOUnavailable(t *testing.T) { SessionIndex: "sess-1", }) - req := httptest.NewRequest(http.MethodGet, "/api/saml/okta/logout", nil) + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/logout", nil) req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) rec := httptest.NewRecorder() @@ -280,7 +282,9 @@ func TestHandleSAMLLogout_SLOUnavailable(t *testing.T) { } func TestHandleSAMLLogout_SLOSuccess(t *testing.T) { - InitSessionStore(t.TempDir()) + dataPath := t.TempDir() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} metadataXML := ` @@ -299,7 +303,7 @@ func TestHandleSAMLLogout_SLOSuccess(t *testing.T) { SessionIndex: "sess-1", }) - req := httptest.NewRequest(http.MethodGet, "/api/saml/okta/logout", nil) + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/logout", nil) req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) rec := httptest.NewRecorder() @@ -314,6 +318,64 @@ func TestHandleSAMLLogout_SLOSuccess(t *testing.T) { } } +// A cross-site top-level GET carries the SameSite=Lax session cookie and skips +// the CSRF check, so SP-initiated logout must not clear the session on GET or +// HEAD. +func TestHandleSAMLLogout_RefusesSafeMethods(t *testing.T) { + dataPath := t.TempDir() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) + + router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} + metadataXML := ` + + + + + +` + router.samlManager.services["okta"] = newTestSAMLService(t, "okta", metadataXML) + + token := generateSessionToken() + GetSessionStore().CreateSAMLSession(token, time.Hour, "agent", "127.0.0.1", "user", &SAMLTokenInfo{ + ProviderID: "okta", + NameID: "name-id", + SessionIndex: "sess-1", + }) + + for _, method := range []string{http.MethodGet, http.MethodHead} { + req := httptest.NewRequest(method, "/api/saml/okta/logout", nil) + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) + rec := httptest.NewRecorder() + + router.handleSAMLLogout(rec, req) + + if rec.Code != http.StatusMethodNotAllowed { + t.Fatalf("%s: expected status %d, got %d", method, http.StatusMethodNotAllowed, rec.Code) + } + if allow := rec.Header().Get("Allow"); allow != http.MethodPost { + t.Fatalf("%s: expected Allow %q, got %q", method, http.MethodPost, allow) + } + if loc := rec.Header().Get("Location"); loc != "" { + t.Fatalf("%s: expected no SLO redirect, got %q", method, loc) + } + if !ValidateSession(token) { + t.Fatalf("%s: SAML logout cleared the session", method) + } + } + + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/logout", nil) + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) + rec := httptest.NewRecorder() + router.handleSAMLLogout(rec, req) + if rec.Code != http.StatusFound { + t.Fatalf("POST: expected status %d, got %d", http.StatusFound, rec.Code) + } + if ValidateSession(token) { + t.Fatal("POST: expected SAML logout to clear the session") + } +} + func TestExtractSAMLProviderID(t *testing.T) { if got := extractSAMLProviderID("/api/saml/okta/login", "login"); got != "okta" { t.Fatalf("expected okta, got %q", got) diff --git a/internal/api/saml_runtime_license_gating_test.go b/internal/api/saml_runtime_license_gating_test.go index 41e4ad6e0e..6a023b5dda 100644 --- a/internal/api/saml_runtime_license_gating_test.go +++ b/internal/api/saml_runtime_license_gating_test.go @@ -26,7 +26,7 @@ func TestSAMLRuntimeEndpointsUseCommunitySSOFeature(t *testing.T) { {http.MethodGet, "/api/saml/test-provider/login"}, {http.MethodPost, "/api/saml/test-provider/acs"}, {http.MethodGet, "/api/saml/test-provider/metadata"}, - {http.MethodGet, "/api/saml/test-provider/logout"}, + {http.MethodPost, "/api/saml/test-provider/logout"}, {http.MethodPost, "/api/saml/test-provider/slo"}, } From 2eec0549a3b5f630fa0a1aae383d844049a2e952 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:37:53 +0100 Subject: [PATCH 2/4] Accept SAML LogoutResponses only for the logout they answer /api/saml/{id}/slo cleared the cookie-selected session for any LogoutResponse the IdP had signed in the last 90 seconds. crewjam/saml v0.5.1 checks a LogoutResponse's signature, Destination, IssueInstant, Issuer and Status but never its InResponseTo, and the route must take GET for the HTTP-Redirect binding, where the SameSite=Lax session cookie rides a cross-site navigation and the CSRF check does not apply. A user who logged out through Pulse could replay the IdP's answer to their own logout through a link and log out anyone who followed it. Delivered as a cross-site HTTP-POST form instead, the response arrived without the victim's cookie, yet the handler's deletion cookies still logged their browser out. SP-initiated logout now records each LogoutRequest ID on the provider's service for up to ten minutes, bound to the session-store hash of the session it ends. The SLO handler spends the record the response's InResponseTo names and refuses the response unless that record exists and is unexpired and the browser carries either no session or the session that logout ended. The genuine return carries no session, because logout clears the cookie before redirecting to the IdP, so requiring the cookie-selected session to match would refuse every real SLO. Refusals answer 403 with a failed saml_slo_callback audit event, and a misdelivered response still spends its record, so each LogoutRequest is answered at most once. The SLO handler no longer writes session state or cookies for an accepted response: logout already invalidated the session and expired its cookies, and the callback cannot see which session a cookieless POST delivery's browser holds. InResponseTo is read only from the root element's unprefixed attribute, because encoding/xml would also fill it from a namespace declaration such as xmlns:InResponseTo, which exclusive canonicalization can leave outside the signature when nothing uses it. SP-initiated logout itself now answers 401 to a request without a session cookie. The SAML routes are public and skip CSRF, so a cookieless cross-site POST fell through to the plain logout and its deletion cookies, a forced logout that a deployment with authentication configured refuses on /api/logout as unauthenticated. --- .../v6/internal/subsystems/agent-lifecycle.md | 11 + .../v6/internal/subsystems/api-contracts.md | 40 +++ .../internal/subsystems/storage-recovery.md | 10 + internal/api/audit_handlers_test.go | 50 +++ internal/api/saml_handlers.go | 58 ++- internal/api/saml_handlers_more_test.go | 336 ++++++++++++++++++ internal/api/saml_service.go | 206 ++++++++++- internal/api/saml_service_test.go | 36 +- 8 files changed, 729 insertions(+), 18 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 6f04936296..0ed3d19b90 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8936,6 +8936,17 @@ install, update, removal and report identity are unchanged. logout still takes `POST`. No agent registration, enrolment, install, update or removal path changed. +### SAML SLO responses bound to the logout that requested them + +`internal/api/saml_service.go` and `internal/api/saml_handlers.go` changed only +so `/api/saml/{id}/slo` accepts an IdP LogoutResponse when its signed +`InResponseTo` names an unexpired LogoutRequest that `/api/saml/{id}/logout` +issued, once, in a browser carrying no session or the session that logout +ended. Other responses are refused, the SLO handler writes no session or +cookie for any response (the logout request already cleared both), and +`/api/saml/{id}/logout` refuses a request that carries no session. No agent +registration, enrolment, install, update or removal path changed. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 361bbedaa8..e95ea4078a 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -5170,6 +5170,46 @@ Single-resource report generation at `GET /api/admin/reports/generate` still runs the configured AI narrator, a paid provider call recorded in the cost ledger; that is a known open exception, not part of this contract. +The SAML SLO callback at `/api/saml/{id}/slo` accepts a LogoutResponse only as +the answer to a logout this SP started. crewjam/saml verifies the response's +signature, `Destination`, `Issuer` and `Status` and rejects one issued more +than 90 seconds ago, but never checks its `InResponseTo`, and the IdP signs its +answer to every user's logout, so a valid signature alone let one user replay +the answer to their own logout through a cross-site `GET` and clear another +user's session. `SAMLService.MakeLogoutRequest` in +`internal/api/saml_service.go` therefore records each LogoutRequest ID, on the +provider's own service, against the session-store hash of the session +`handleSAMLLogout` is ending. A record is honored for ten minutes unless the +1,024-record cap evicts it first, oldest first. `SAMLService.ValidateLogoutResponse` +reads `InResponseTo` from the same payload crewjam verified, taking only the +root element's attribute written without a prefix, the one the signature +covers (a namespace declaration such as `xmlns:InResponseTo` is ignored, +because exclusive canonicalization can leave an unused one out of the signed +form), and +spends the matching record. The response is accepted only when that record +exists and is unexpired, and the browser carries either no session cookie (the +normal case, because `handleSAMLLogout` clears the cookie before redirecting to +the IdP) or the session that request ended. Unsolicited, expired, evicted, +replayed and other-session responses are refused with `403` and a failed +`saml_slo_callback` audit event, and the handler leaves sessions and cookies +alone. A misdelivered response still spends its record, so each LogoutRequest +is answered at most once. The handler writes no session state or cookie for an +accepted response either: `handleSAMLLogout` already invalidated the session +and expired its cookies, and a cross-site HTTP-POST delivery withholds the +`SameSite=Lax` cookie, so a deletion cookie written there could log out a +browser whose session the request never presented. For the same reason +`handleSAMLLogout` answers `401` to a request that carries no session cookie, +API-token-only callers included, instead of falling back to a cookie-clearing +logout: the SAML routes are public and skip CSRF, so that fallback was a +cross-site forced logout of a kind a deployment with authentication configured +refuses on `/api/logout` as unauthenticated. The records live in memory, +so a restart or provider re-initialization during the IdP round trip leaves the +returning response unmatched and refused; the local session was already +cleared when logout began. `TestSAMLSLOAuditsLogoutResponseNotBoundToSession` +in `internal/api/audit_handlers_test.go` replays one user's signed response +into another user's browser and proves the victim's session survives and the +refusal is audited. + Alert delivery diagnosis is a read-only monitoring API contract. `GET /api/alerts/delivery-diagnosis?alertIdentifier=` returns the alert manager's current delivery-policy projection for one active alert, including diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index d5112ce305..3d353be523 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6876,6 +6876,16 @@ so `/api/discovery/settings` (`PUT` or `POST`) and `/api/saml/{id}/logout` of changing the discovery staleness threshold or clearing a session. No storage, retention, backup, migration or recovery path is added or moved. +### SAML SLO responses bound to the logout that requested them + +`internal/api/saml_service.go` keeps each outstanding SAML LogoutRequest ID in +memory on the provider's service, bound to the session-store hash of the +session being logged out, and honors it for ten minutes (at most 1,024 +records; expired ones are dropped on the next logout). `internal/api/saml_handlers.go` +refuses a LogoutResponse that does not answer one of them. The records are not +persisted, and the SLO handler no longer writes session state. No storage, +retention, backup, migration or recovery path is added or moved. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` no longer exempts every request carrying diff --git a/internal/api/audit_handlers_test.go b/internal/api/audit_handlers_test.go index 117f882d03..57e40f74cc 100644 --- a/internal/api/audit_handlers_test.go +++ b/internal/api/audit_handlers_test.go @@ -1081,3 +1081,53 @@ func TestParseAuditLimitCapsOversizedRequests(t *testing.T) { }) } } + +// A signed SAML LogoutResponse answering one user's logout, replayed into +// another user's browser through a cross-site GET to /slo, is refused like the +// other SLO refusals: the victim keeps their session and one failed +// saml_slo_callback audit event names the binding failure. The genuine +// completion of a logout still audits success. +func TestSAMLSLOAuditsLogoutResponseNotBoundToSession(t *testing.T) { + previousManager := GetTenantAuditManager() + SetTenantAuditManager(nil) + t.Cleanup(func() { + SetTenantAuditManager(previousManager) + }) + logger := &testAuditLoggerNoVerify{} + setAuditLogger(t, logger) + + router, idp := newSAMLSLOTestRouter(t) + attackerRequestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "attacker")) + victim := newSAMLTestSession(t, "victim") + + sloCallbackEvents := func() []audit.Event { + var events []audit.Event + for _, event := range logger.events { + if event.EventType == "saml_slo_callback" { + events = append(events, event) + } + } + return events + } + + rec := deliverSAMLTestLogoutResponse(router, idp.redirectQuery(t, attackerRequestID), victim) + assertSAMLSLORefused(t, rec, "another session's response") + if !ValidateSession(victim) { + t.Fatal("another user's LogoutResponse cleared the victim's session") + } + refusals := sloCallbackEvents() + if len(refusals) != 1 || refusals[0].Success || + refusals[0].Details != "LogoutResponse does not answer an outstanding logout request for this session" { + t.Fatalf("expected one failed saml_slo_callback binding refusal, got %+v", refusals) + } + + logger.events = nil + victimRequestID := startSAMLTestLogout(t, router, victim) + if rec := deliverSAMLTestLogoutResponse(router, idp.redirectQuery(t, victimRequestID), ""); rec.Code != http.StatusFound { + t.Fatalf("genuine completion: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + completions := sloCallbackEvents() + if len(completions) != 1 || !completions[0].Success || completions[0].Details != "SAML SLO complete" { + t.Fatalf("expected one successful saml_slo_callback event, got %+v", completions) + } +} diff --git a/internal/api/saml_handlers.go b/internal/api/saml_handlers.go index 6ce686e585..b34723de02 100644 --- a/internal/api/saml_handlers.go +++ b/internal/api/saml_handlers.go @@ -3,6 +3,7 @@ package api import ( "context" "encoding/json" + "errors" "fmt" "net/http" "strings" @@ -384,6 +385,17 @@ func (r *Router) handleSAMLLogout(w http.ResponseWriter, req *http.Request) { if !requireRequestMethod(w, req, http.MethodPost) { return } + // The SAML routes are public and skip CSRF so the IdP flows can reach + // them, and a cross-site POST withholds the SameSite=Lax session cookie. + // A request carrying no session has nothing to log out, yet the deletion + // cookies every logout path below writes would log out whichever session + // that browser holds. Refuse it, as a deployment with authentication + // configured refuses an unauthenticated /api/logout. SP-initiated SAML + // logout always needs the browser session it ends. + if samlSessionBindingKey(req) == "" { + writeErrorResponse(w, http.StatusUnauthorized, "unauthorized", "Authentication required", nil) + return + } providerID := extractSAMLProviderID(req.URL.Path, "logout") if providerID == "" { // Fall back to regular logout @@ -413,11 +425,15 @@ func (r *Router) handleSAMLLogout(w http.ResponseWriter, req *http.Request) { return } + // Bind the LogoutRequest to the session it logs out before clearing it, so + // /slo can tell the IdP's answer to this logout from a replayed one. + sessionKey := samlSessionBindingKey(req) + // Clear local session first r.clearSession(w, req) // Attempt SAML SLO - logoutURL, err := service.MakeLogoutRequest(session.NameID, session.SessionIndex) + logoutURL, err := service.MakeLogoutRequest(session.NameID, session.SessionIndex, sessionKey) if err != nil { log.Warn().Err(err).Str("provider_id", providerID).Msg("SAML SLO not available, local logout only") LogAuditEventForTenant(GetOrgID(req.Context()), "saml_logout", "", GetClientIP(req), req.URL.Path, true, "Local logout only (SLO not available)") @@ -437,6 +453,16 @@ func (r *Router) handleSAMLLogout(w http.ResponseWriter, req *http.Request) { // logout DoS against any user with a SAML session. Verify the IdP's // XML-DSig on the LogoutResponse before clearing anything; on validation // failure log the audit event and refuse to mutate session state. +// +// A valid signature alone is not enough: the IdP signs its answer to every +// user's logout, and this route must take the HTTP-Redirect binding's GET, +// which carries the SameSite=Lax session cookie on a cross-site navigation and +// skips the CSRF check. The response must also answer an outstanding +// LogoutRequest from handleSAMLLogout, once, in a browser carrying either no +// session (handleSAMLLogout already cleared its cookie) or the session that +// request logged out. Anything else is a replay or a forced logout and is +// refused the same way. Completion writes no session state or cookie: +// handleSAMLLogout already did that before redirecting to the IdP. func (r *Router) handleSAMLSLO(w http.ResponseWriter, req *http.Request) { providerID := extractSAMLProviderID(req.URL.Path, "slo") if providerID == "" || !validateProviderID(providerID) { @@ -482,7 +508,17 @@ func (r *Router) handleSAMLSLO(w http.ResponseWriter, req *http.Request) { return } - if err := service.ValidateLogoutResponse(req); err != nil { + if err := service.ValidateLogoutResponse(req, samlSessionBindingKey(req)); err != nil { + if errors.Is(err, errSAMLLogoutResponseUnbound) { + log.Warn(). + Err(err). + Str("provider_id", providerID). + Str("client_ip", GetClientIP(req)). + Msg("SAML LogoutResponse does not answer an outstanding logout for this session β€” refusing to clear session") + LogAuditEventForTenant(GetOrgID(req.Context()), "saml_slo_callback", "", GetClientIP(req), req.URL.Path, false, "LogoutResponse does not answer an outstanding logout request for this session") + http.Error(w, "invalid LogoutResponse", http.StatusForbidden) + return + } log.Warn(). Err(err). Str("provider_id", providerID). @@ -493,7 +529,11 @@ func (r *Router) handleSAMLSLO(w http.ResponseWriter, req *http.Request) { return } - r.clearSession(w, req) + // handleSAMLLogout already invalidated the session this response answers + // and expired its cookies. Write nothing here: a cross-site HTTP-POST + // delivery withholds the SameSite=Lax session cookie, so this request + // cannot see which session the browser holds, and deletion cookies would + // log out whichever one that is. LogAuditEventForTenant(GetOrgID(req.Context()), "saml_slo_callback", "", GetClientIP(req), req.URL.Path, true, "SAML SLO complete") http.Redirect(w, req, "/?logout=success", http.StatusFound) } @@ -575,6 +615,18 @@ func (r *Router) getSAMLSessionInfo(req *http.Request) *SAMLSessionInfo { } } +// samlSessionBindingKey identifies the session the request's cookie selects, +// as the session store's token hash, or returns "" when the request carries +// none. SLO binds each LogoutRequest to it and checks it again when the IdP's +// LogoutResponse arrives. +func samlSessionBindingKey(req *http.Request) string { + cookie, err := readSessionCookie(req) + if err != nil || cookie.Value == "" { + return "" + } + return sessionHash(cookie.Value) +} + // clearSession clears the current session - properly invalidates server-side session // and clears both pulse_session and pulse_csrf cookies func (r *Router) clearSession(w http.ResponseWriter, req *http.Request) { diff --git a/internal/api/saml_handlers_more_test.go b/internal/api/saml_handlers_more_test.go index 10c04637bd..82320f9793 100644 --- a/internal/api/saml_handlers_more_test.go +++ b/internal/api/saml_handlers_more_test.go @@ -1,13 +1,22 @@ package api import ( + "bytes" + "compress/flate" "context" + "crypto/x509" + "encoding/base64" + "encoding/pem" + "encoding/xml" + "io" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "time" + "github.com/crewjam/saml" "github.com/rcourtman/pulse-go-rewrite/internal/config" ) @@ -246,6 +255,333 @@ func TestHandleSAMLSLO_RejectsIDPInitiatedLogoutRequest(t *testing.T) { } } +const samlSLOTestDestination = "https://pulse.example.com/api/saml/okta/slo" + +// samlSLOTestIdP signs LogoutResponses with a key the SP under test trusts +// through its IdP metadata, the way a real IdP answers a LogoutRequest. +type samlSLOTestIdP struct { + signer *saml.ServiceProvider +} + +// newSAMLSLOTestRouter returns a router whose "okta" provider supports SLO and +// trusts the returned IdP's signing key, with fresh session and CSRF stores. +func newSAMLSLOTestRouter(t *testing.T) (*Router, *samlSLOTestIdP) { + t.Helper() + dataPath := t.TempDir() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) + + certPEM, _, key := generateTestCert(t) + block, _ := pem.Decode(certPEM) + if block == nil { + t.Fatal("decode IdP certificate PEM") + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("parse IdP certificate: %v", err) + } + metadataXML := ` + + + + + ` + base64.StdEncoding.EncodeToString(block.Bytes) + ` + + + + + +` + + router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} + router.samlManager.services["okta"] = newTestSAMLService(t, "okta", metadataXML) + return router, &samlSLOTestIdP{signer: &saml.ServiceProvider{ + EntityID: "idp", + Key: key, + Certificate: cert, + SignatureMethod: "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256", + }} +} + +// redirectQuery returns the HTTP-Redirect binding query carrying a signed, +// successful LogoutResponse that answers requestID. +func (idp *samlSLOTestIdP) redirectQuery(t *testing.T, requestID string) string { + t.Helper() + resp, err := idp.signer.MakeLogoutResponse(samlSLOTestDestination, requestID) + if err != nil { + t.Fatalf("sign LogoutResponse: %v", err) + } + return resp.Redirect("").RawQuery +} + +// postForm returns the HTTP-POST binding form body carrying the same signed +// LogoutResponse as redirectQuery. +func (idp *samlSLOTestIdP) postForm(t *testing.T, requestID string) string { + t.Helper() + query, err := url.ParseQuery(idp.redirectQuery(t, requestID)) + if err != nil { + t.Fatalf("parse redirect query: %v", err) + } + signedXML := inflateSAMLTestPayload(t, query.Get("SAMLResponse")) + return url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(signedXML)}}.Encode() +} + +func inflateSAMLTestPayload(t *testing.T, payload string) []byte { + t.Helper() + compressed, err := base64.StdEncoding.DecodeString(payload) + if err != nil { + t.Fatalf("decode SAML payload: %v", err) + } + raw, err := io.ReadAll(flate.NewReader(bytes.NewReader(compressed))) + if err != nil { + t.Fatalf("inflate SAML payload: %v", err) + } + return raw +} + +// newSAMLTestSession creates a live SAML session with the okta provider. +func newSAMLTestSession(t *testing.T, nameID string) string { + t.Helper() + token := generateSessionToken() + GetSessionStore().CreateSAMLSession(token, time.Hour, "agent", "127.0.0.1", nameID, &SAMLTokenInfo{ + ProviderID: "okta", + NameID: nameID, + SessionIndex: "sess-" + nameID, + }) + return token +} + +// startSAMLTestLogout runs SP-initiated logout for the session and returns the +// ID of the LogoutRequest sent to the IdP. +func startSAMLTestLogout(t *testing.T, router *Router, token string) string { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/logout", nil) + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) + rec := httptest.NewRecorder() + router.handleSAMLLogout(rec, req) + if rec.Code != http.StatusFound { + t.Fatalf("logout: expected status %d, got %d", http.StatusFound, rec.Code) + } + location, err := url.Parse(rec.Header().Get("Location")) + if err != nil || location.Host != "idp.example.com" { + t.Fatalf("logout: unexpected SLO redirect %q (%v)", rec.Header().Get("Location"), err) + } + var logoutRequest struct { + ID string `xml:"ID,attr"` + } + if err := xml.Unmarshal(inflateSAMLTestPayload(t, location.Query().Get("SAMLRequest")), &logoutRequest); err != nil { + t.Fatalf("parse LogoutRequest: %v", err) + } + if logoutRequest.ID == "" { + t.Fatal("LogoutRequest has no ID") + } + return logoutRequest.ID +} + +// deliverSAMLTestLogoutResponse sends an HTTP-Redirect binding LogoutResponse +// to /slo from a browser carrying the given session cookie ("" for none). +func deliverSAMLTestLogoutResponse(router *Router, query, token string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodGet, "/api/saml/okta/slo?"+query, nil) + if token != "" { + req.AddCookie(&http.Cookie{Name: "pulse_session", Value: token}) + } + rec := httptest.NewRecorder() + router.handleSAMLSLO(rec, req) + return rec +} + +// postSAMLTestLogoutResponse sends an HTTP-POST binding form body to /slo as a +// cross-site form POST does: without the SameSite=Lax session cookie. +func postSAMLTestLogoutResponse(router *Router, form string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/slo", strings.NewReader(form)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + router.handleSAMLSLO(rec, req) + return rec +} + +func assertSAMLSLORefused(t *testing.T, rec *httptest.ResponseRecorder, label string) { + t.Helper() + if rec.Code != http.StatusForbidden { + t.Fatalf("%s: expected status %d, got %d body=%q", label, http.StatusForbidden, rec.Code, rec.Body.String()) + } + if loc := rec.Header().Get("Location"); loc != "" { + t.Fatalf("%s: refused SLO must not redirect, got %q", label, loc) + } + if cookies := rec.Result().Cookies(); len(cookies) != 0 { + t.Fatalf("%s: refused SLO must not touch cookies, got %v", label, cookies) + } +} + +// The browser that started SP-initiated logout comes back from the IdP without +// a session cookie, because handleSAMLLogout cleared it before redirecting. +// The IdP's signed answer to that logout completes it, by either binding. +func TestHandleSAMLSLO_AcceptsAnswerToOutstandingLogout(t *testing.T) { + router, idp := newSAMLSLOTestRouter(t) + + token := newSAMLTestSession(t, "alice") + requestID := startSAMLTestLogout(t, router, token) + if ValidateSession(token) { + t.Fatal("SP-initiated logout must clear the local session before redirecting") + } + + rec := deliverSAMLTestLogoutResponse(router, idp.redirectQuery(t, requestID), "") + if rec.Code != http.StatusFound { + t.Fatalf("redirect binding: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + if loc := rec.Header().Get("Location"); loc != "/?logout=success" { + t.Fatalf("redirect binding: unexpected redirect %q", loc) + } + + token = newSAMLTestSession(t, "alice") + requestID = startSAMLTestLogout(t, router, token) + rec = postSAMLTestLogoutResponse(router, idp.postForm(t, requestID)) + if rec.Code != http.StatusFound { + t.Fatalf("POST binding: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + if loc := rec.Header().Get("Location"); loc != "/?logout=success" { + t.Fatalf("POST binding: unexpected redirect %q", loc) + } +} + +// Each LogoutRequest is answered once. Replaying the IdP's response, with or +// without a session cookie, is refused and leaves the presenting session alone. +func TestHandleSAMLSLO_RefusesReplayedLogoutResponse(t *testing.T) { + router, idp := newSAMLSLOTestRouter(t) + + requestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "alice")) + query := idp.redirectQuery(t, requestID) + if rec := deliverSAMLTestLogoutResponse(router, query, ""); rec.Code != http.StatusFound { + t.Fatalf("first delivery: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + + assertSAMLSLORefused(t, deliverSAMLTestLogoutResponse(router, query, ""), "replay without session") + + victim := newSAMLTestSession(t, "bob") + assertSAMLSLORefused(t, deliverSAMLTestLogoutResponse(router, query, victim), "replay into another session") + if !ValidateSession(victim) { + t.Fatal("replayed LogoutResponse cleared another user's session") + } +} + +// A response the IdP signed for one user's logout must not clear a different +// user's live session, even on its first delivery inside the issue window: a +// cross-site GET carries the victim's SameSite=Lax cookie and skips CSRF. The +// misdelivered response spends the request, so it cannot be retried either. +func TestHandleSAMLSLO_RefusesAnotherSessionsLogoutResponse(t *testing.T) { + router, idp := newSAMLSLOTestRouter(t) + + requestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "attacker")) + query := idp.redirectQuery(t, requestID) + + victim := newSAMLTestSession(t, "victim") + assertSAMLSLORefused(t, deliverSAMLTestLogoutResponse(router, query, victim), "another session's response") + if !ValidateSession(victim) { + t.Fatal("another user's LogoutResponse cleared the victim's session") + } + assertSAMLSLORefused(t, deliverSAMLTestLogoutResponse(router, query, ""), "retry after misdelivery") +} + +// A correctly signed LogoutResponse that answers no LogoutRequest this SP +// issued (an unsolicited one, or one answering a request the IdP was handed +// by someone else) is refused without touching the session. +func TestHandleSAMLSLO_RefusesUnsolicitedLogoutResponse(t *testing.T) { + router, idp := newSAMLSLOTestRouter(t) + victim := newSAMLTestSession(t, "victim") + + for _, requestID := range []string{"", "id-never-issued-by-this-sp"} { + label := "InResponseTo=" + requestID + assertSAMLSLORefused(t, deliverSAMLTestLogoutResponse(router, idp.redirectQuery(t, requestID), victim), label) + if !ValidateSession(victim) { + t.Fatalf("%s: unsolicited LogoutResponse cleared the session", label) + } + } +} + +// encoding/xml matches an un-namespaced attribute field to any attribute with +// that local name, a namespace declaration such as xmlns:InResponseTo included, +// and exclusive canonicalization can leave an unused declaration out of the +// signed form. Appending one to a signed response must not rebind it to another +// outstanding LogoutRequest. +func TestHandleSAMLSLO_InResponseToIgnoresNamespaceDeclarations(t *testing.T) { + router, idp := newSAMLSLOTestRouter(t) + ownRequestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "alice")) + otherRequestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "bob")) + + query, err := url.ParseQuery(idp.redirectQuery(t, ownRequestID)) + if err != nil { + t.Fatalf("parse redirect query: %v", err) + } + signedXML := string(inflateSAMLTestPayload(t, query.Get("SAMLResponse"))) + genuine := `InResponseTo="` + ownRequestID + `"` + if !strings.Contains(signedXML, genuine) { + t.Fatalf("signed response has no %s: %s", genuine, signedXML) + } + spoofed := strings.Replace(signedXML, genuine, genuine+` xmlns:InResponseTo="`+otherRequestID+`"`, 1) + form := url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString([]byte(spoofed))}}.Encode() + + // The signature still verifies, so the response completes the request it + // was signed for and nothing else. + if rec := postSAMLTestLogoutResponse(router, form); rec.Code != http.StatusFound { + t.Fatalf("spoofed delivery: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + if rec := deliverSAMLTestLogoutResponse(router, idp.redirectQuery(t, otherRequestID), ""); rec.Code != http.StatusFound { + t.Fatalf("a namespace declaration spent another LogoutRequest: genuine answer got %d body=%q", rec.Code, rec.Body.String()) + } + assertSAMLSLORefused(t, deliverSAMLTestLogoutResponse(router, idp.redirectQuery(t, ownRequestID), ""), "signed request after spoofed delivery") +} + +// A cross-site HTTP-POST delivery withholds the SameSite=Lax session cookie, +// so /slo cannot see which session the browser holds. Completing a logout +// must therefore write no cookie: a deletion cookie would log out the +// browser's live session even though the request never presented it. +func TestHandleSAMLSLO_CompletionWritesNoCookies(t *testing.T) { + router, idp := newSAMLSLOTestRouter(t) + + requestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "attacker")) + victim := newSAMLTestSession(t, "victim") + rec := postSAMLTestLogoutResponse(router, idp.postForm(t, requestID)) + if rec.Code != http.StatusFound { + t.Fatalf("cookieless POST binding: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + if cookies := rec.Result().Cookies(); len(cookies) != 0 { + t.Fatalf("cookieless POST binding: SLO completion wrote cookies %v", cookies) + } + if !ValidateSession(victim) { + t.Fatal("SLO completion invalidated an unrelated session") + } + + token := newSAMLTestSession(t, "alice") + requestID = startSAMLTestLogout(t, router, token) + rec = deliverSAMLTestLogoutResponse(router, idp.redirectQuery(t, requestID), token) + if rec.Code != http.StatusFound { + t.Fatalf("own session: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + if cookies := rec.Result().Cookies(); len(cookies) != 0 { + t.Fatalf("own session: SLO completion wrote cookies %v", cookies) + } +} + +// The SAML routes skip CSRF, and a cross-site POST withholds the +// SameSite=Lax session cookie, so SP-initiated logout must not answer a +// request that carries no session with the deletion cookies of a logout. +func TestHandleSAMLLogout_RefusesRequestsWithoutSession(t *testing.T) { + router, _ := newSAMLSLOTestRouter(t) + + for _, path := range []string{"/api/saml/okta/logout", "/api/saml/unknown/logout"} { + req := httptest.NewRequest(http.MethodPost, path, nil) + rec := httptest.NewRecorder() + router.handleSAMLLogout(rec, req) + + if rec.Code != http.StatusUnauthorized { + t.Fatalf("%s: expected status %d, got %d body=%q", path, http.StatusUnauthorized, rec.Code, rec.Body.String()) + } + if cookies := rec.Result().Cookies(); len(cookies) != 0 { + t.Fatalf("%s: cookieless logout wrote cookies %v", path, cookies) + } + } +} + func TestHandleSAMLLogout_SLOUnavailable(t *testing.T) { dataPath := t.TempDir() InitSessionStore(dataPath) diff --git a/internal/api/saml_service.go b/internal/api/saml_service.go index 01d6fbd187..c020c82f63 100644 --- a/internal/api/saml_service.go +++ b/internal/api/saml_service.go @@ -1,9 +1,12 @@ package api import ( + "bytes" + "compress/flate" "context" "crypto/rsa" "crypto/x509" + "encoding/base64" "encoding/pem" "encoding/xml" "errors" @@ -31,6 +34,93 @@ type SAMLService struct { httpClient *http.Client baseURL string lastRefresh time.Time + + // logoutRequests holds the SP-initiated LogoutRequests still waiting for + // the IdP's LogoutResponse. + logoutRequests samlLogoutRequestStore +} + +const ( + // samlLogoutRequestTTL bounds how long the IdP round trip of an + // SP-initiated logout may take, matching the OIDC login state lifetime. + samlLogoutRequestTTL = 10 * time.Minute + maxSAMLLogoutRequestEntries = 1024 + // samlLogoutResponseMaxBytes matches crewjam/saml's inflate limit for the + // HTTP-Redirect binding. + samlLogoutResponseMaxBytes = 10 << 20 +) + +// errSAMLLogoutResponseUnbound marks a LogoutResponse that verified but does +// not answer an outstanding LogoutRequest this SP issued for the presenting +// session. +var errSAMLLogoutResponseUnbound = errors.New("logout response does not answer an outstanding logout request for this session") + +// samlLogoutRequestStore records the LogoutRequests this SP issued, keyed by +// request ID, with the session each one logged out. crewjam/saml checks a +// LogoutResponse's signature, destination, issuer, status and age but never +// its InResponseTo, so without this record any recent signed response from the +// IdP, including one answering another user's logout, would be accepted. +type samlLogoutRequestStore struct { + mu sync.Mutex + entries map[string]samlLogoutRequest +} + +type samlLogoutRequest struct { + sessionKey string + expiresAt time.Time +} + +func (s *samlLogoutRequestStore) put(requestID, sessionKey string, now time.Time) { + s.mu.Lock() + defer s.mu.Unlock() + + if s.entries == nil { + s.entries = make(map[string]samlLogoutRequest) + } + for id, entry := range s.entries { + if !now.Before(entry.expiresAt) { + delete(s.entries, id) + } + } + for len(s.entries) >= maxSAMLLogoutRequestEntries { + oldestID := "" + var oldestExpiry time.Time + for id, entry := range s.entries { + if oldestID == "" || entry.expiresAt.Before(oldestExpiry) || (entry.expiresAt.Equal(oldestExpiry) && id < oldestID) { + oldestID = id + oldestExpiry = entry.expiresAt + } + } + delete(s.entries, oldestID) + } + s.entries[requestID] = samlLogoutRequest{ + sessionKey: sessionKey, + expiresAt: now.Add(samlLogoutRequestTTL), + } +} + +// consume spends the outstanding request a verified LogoutResponse answers. +// sessionKey identifies the session the presenting browser carries, or is +// empty when it carries none. The response may complete the logout only when +// it answers an unexpired request and the browser carries either no session +// or the session that request logged out. The request is spent on every +// verified presentation, so a response is honored at most once. +func (s *samlLogoutRequestStore) consume(requestID, sessionKey string, now time.Time) error { + s.mu.Lock() + defer s.mu.Unlock() + + entry, ok := s.entries[requestID] + if requestID == "" || !ok { + return fmt.Errorf("%w: no outstanding request matches InResponseTo %q", errSAMLLogoutResponseUnbound, requestID) + } + delete(s.entries, requestID) + if !now.Before(entry.expiresAt) { + return fmt.Errorf("%w: request %q expired", errSAMLLogoutResponseUnbound, requestID) + } + if sessionKey != "" && sessionKey != entry.sessionKey { + return fmt.Errorf("%w: request %q logged out a different session", errSAMLLogoutResponseUnbound, requestID) + } + return nil } func normalizeSAMLBaseURL(baseURL string) (string, error) { @@ -594,14 +684,19 @@ func (s *SAMLService) GetMetadata() ([]byte, error) { return xml.MarshalIndent(metadata, "", " ") } -// MakeLogoutRequest creates a SAML LogoutRequest for SLO -func (s *SAMLService) MakeLogoutRequest(nameID, sessionIdx string) (string, error) { +// MakeLogoutRequest creates a SAML LogoutRequest for SLO and records its ID +// as outstanding for sessionKey, the session being logged out, so that +// ValidateLogoutResponse accepts only the IdP's answer to this request. +func (s *SAMLService) MakeLogoutRequest(nameID, sessionIdx, sessionKey string) (string, error) { s.mu.RLock() defer s.mu.RUnlock() if s.sp == nil { return "", errors.New("service provider not initialized") } + if sessionKey == "" { + return "", errors.New("logout request has no session to bind") + } // Check if IdP supports SLO if len(s.idpMetadata.IDPSSODescriptors) == 0 || @@ -619,21 +714,32 @@ func (s *SAMLService) MakeLogoutRequest(nameID, sessionIdx string) (string, erro // Build redirect URL redirectURL := req.Redirect("") - return validateSAMLRedirectTarget(redirectURL.String(), s.idpMetadata.IDPSSODescriptors[0].SingleLogoutServices) + validatedURL, err := validateSAMLRedirectTarget(redirectURL.String(), s.idpMetadata.IDPSSODescriptors[0].SingleLogoutServices) + if err != nil { + return "", err + } + s.logoutRequests.put(req.ID, sessionKey, time.Now()) + return validatedURL, nil } // ValidateLogoutResponse verifies an incoming IdP-signed SAML LogoutResponse, // checking the XML-DSig signature against the IdP's published certificate as -// well as the standard LogoutResponse temporal / target invariants. Returns -// nil only when the response is genuinely from the configured IdP and bound -// to the SAML state β€” anything that fails validation (no payload, bad -// signature, unknown issuer, expired) returns an error and the caller MUST -// NOT mutate any session state on the strength of the request. +// well as the standard LogoutResponse temporal / target invariants, and then +// binds it to the request it answers: its InResponseTo must name an +// unexpired LogoutRequest from MakeLogoutRequest, which it spends, and +// sessionKey (the presenting browser's session, or "" for none) must be empty +// or the session that request logged out. Returns nil only when all of that +// holds β€” anything else (no payload, bad signature, unknown issuer, expired, +// unsolicited, replayed, or another session's logout) returns an error and +// the caller MUST NOT mutate any session state on the strength of the +// request. Binding failures wrap errSAMLLogoutResponseUnbound. // -// Without this validation the SLO callback endpoint was an unauthenticated -// force-logout DoS: a cross-origin POST with no payload, or any forged -// payload, cleared the user's session. -func (s *SAMLService) ValidateLogoutResponse(req *http.Request) error { +// Without signature validation the SLO callback endpoint was an +// unauthenticated force-logout DoS: a cross-origin POST with no payload, or +// any forged payload, cleared the user's session. Before the binding, any +// user could replay the IdP's answer to their own logout through a cross-site +// GET to clear someone else's session. +func (s *SAMLService) ValidateLogoutResponse(req *http.Request, sessionKey string) error { s.mu.RLock() defer s.mu.RUnlock() @@ -641,7 +747,81 @@ func (s *SAMLService) ValidateLogoutResponse(req *http.Request) error { return errors.New("service provider not initialized") } - return s.sp.ValidateLogoutResponseRequest(req) + inResponseTo, err := s.verifiedLogoutResponseInResponseTo(req) + if err != nil { + return err + } + return s.logoutRequests.consume(inResponseTo, sessionKey, time.Now()) +} + +// verifiedLogoutResponseInResponseTo selects the SAMLResponse payload the way +// crewjam/saml's ValidateLogoutResponseRequest does (the HTTP-Redirect query +// parameter first, then the HTTP-POST form field), has crewjam verify exactly +// that payload, and reads InResponseTo from the same bytes. +func (s *SAMLService) verifiedLogoutResponseInResponseTo(req *http.Request) (string, error) { + var raw []byte + if data := req.URL.Query().Get("SAMLResponse"); data != "" { + if err := s.sp.ValidateLogoutResponseRedirect(data); err != nil { + return "", err + } + compressed, err := base64.StdEncoding.DecodeString(data) + if err != nil { + return "", fmt.Errorf("decode logout response: %w", err) + } + raw, err = io.ReadAll(io.LimitReader(flate.NewReader(bytes.NewReader(compressed)), samlLogoutResponseMaxBytes)) + if err != nil { + return "", fmt.Errorf("inflate logout response: %w", err) + } + } else { + if err := req.ParseForm(); err != nil { + return "", fmt.Errorf("parse logout response form: %w", err) + } + data := req.PostForm.Get("SAMLResponse") + if err := s.sp.ValidateLogoutResponseForm(data); err != nil { + return "", err + } + var err error + raw, err = base64.StdEncoding.DecodeString(data) + if err != nil { + return "", fmt.Errorf("decode logout response: %w", err) + } + } + return logoutResponseInResponseTo(raw) +} + +// logoutResponseInResponseTo returns the root LogoutResponse's InResponseTo +// attribute as written without a prefix, the one the IdP's enveloped signature +// covers, or "" when it has none. The attributes are read by hand, lexically, +// because encoding/xml would also fill an un-namespaced attribute field from a +// namespace declaration such as xmlns:InResponseTo="...", which exclusive +// canonicalization can leave out of the signed form when nothing uses it. crewjam +// has already checked that the root is a protocol LogoutResponse. +func logoutResponseInResponseTo(raw []byte) (string, error) { + decoder := xml.NewDecoder(bytes.NewReader(raw)) + for { + token, err := decoder.RawToken() + if err != nil { + return "", fmt.Errorf("parse logout response: %w", err) + } + start, ok := token.(xml.StartElement) + if !ok { + continue + } + if start.Name.Local != "LogoutResponse" { + return "", fmt.Errorf("parse logout response: unexpected root element %q", start.Name.Local) + } + inResponseTo, found := "", false + for _, attr := range start.Attr { + if attr.Name.Space != "" || attr.Name.Local != "InResponseTo" { + continue + } + if found { + return "", errors.New("parse logout response: duplicate InResponseTo") + } + inResponseTo, found = attr.Value, true + } + return inResponseTo, nil + } } func validateSAMLRedirectTarget(rawURL string, allowedEndpoints []saml.Endpoint) (string, error) { diff --git a/internal/api/saml_service_test.go b/internal/api/saml_service_test.go index 7de7a75704..83743e181a 100644 --- a/internal/api/saml_service_test.go +++ b/internal/api/saml_service_test.go @@ -8,6 +8,8 @@ import ( "crypto/rsa" "crypto/x509" "encoding/pem" + "errors" + "fmt" "math/big" "net/http" "strings" @@ -184,7 +186,7 @@ func TestSAMLServiceBasicFlows(t *testing.T) { t.Fatalf("metadata error: %v", err) } - logoutURL, err := service.MakeLogoutRequest("user", "sess") + logoutURL, err := service.MakeLogoutRequest("user", "sess", sessionHash("session-token")) if err != nil || !strings.Contains(logoutURL, "SAMLRequest") { t.Fatalf("unexpected logout url: %v %s", err, logoutURL) } @@ -196,7 +198,7 @@ func TestSAMLServiceBasicFlows(t *testing.T) { if _, err := service.GetMetadata(); err == nil { t.Fatal("expected error when sp missing") } - if _, err := service.MakeLogoutRequest("user", "sess"); err == nil { + if _, err := service.MakeLogoutRequest("user", "sess", sessionHash("session-token")); err == nil { t.Fatal("expected error when sp missing") } if err := service.RefreshMetadata(context.Background()); err == nil { @@ -204,6 +206,36 @@ func TestSAMLServiceBasicFlows(t *testing.T) { } } +// Outstanding SLO requests expire after samlLogoutRequestTTL, and the store +// keeps at most maxSAMLLogoutRequestEntries by dropping the oldest. +func TestSAMLLogoutRequestStoreExpiresAndEvictsOldest(t *testing.T) { + var store samlLogoutRequestStore + now := time.Now() + key := sessionHash("session-token") + + store.put("id-expired", key, now) + if err := store.consume("id-expired", key, now.Add(samlLogoutRequestTTL)); !errors.Is(err, errSAMLLogoutResponseUnbound) { + t.Fatalf("expected an expired request to be refused, got %v", err) + } + store.put("id-live", key, now) + if err := store.consume("id-live", "", now.Add(samlLogoutRequestTTL-time.Second)); err != nil { + t.Fatalf("expected a live request to be accepted, got %v", err) + } + + for i := 0; i <= maxSAMLLogoutRequestEntries; i++ { + store.put(fmt.Sprintf("id-%04d", i), key, now.Add(time.Duration(i)*time.Millisecond)) + } + if len(store.entries) != maxSAMLLogoutRequestEntries { + t.Fatalf("expected %d outstanding requests, got %d", maxSAMLLogoutRequestEntries, len(store.entries)) + } + if err := store.consume("id-0000", key, now); !errors.Is(err, errSAMLLogoutResponseUnbound) { + t.Fatalf("expected the oldest request to be evicted, got %v", err) + } + if err := store.consume(fmt.Sprintf("id-%04d", maxSAMLLogoutRequestEntries), key, now); err != nil { + t.Fatalf("expected the newest request to be kept, got %v", err) + } +} + func TestValidateSAMLRedirectTarget(t *testing.T) { allowed := []saml.Endpoint{ {Location: "https://idp.example.com/sso"}, From 3cef2c1921d307023f92d68f8092efc774c0e049 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 20:48:08 +0100 Subject: [PATCH 3/4] Verify manual SAML IdP signatures with the configured certificate SAML providers configured with an SSO URL and a PEM signing certificate (idpCertificate or idpCertFile) instead of IdP metadata never verified an IdP signature. buildManualMetadata put the certificate into the IdP descriptor's X509Certificate field as a PEM block, but crewjam/saml base64-decodes that field as DER, so every ACS Response and LogoutResponse failed with "cannot parse certificate: illegal base64 data at input byte 0" before any key was compared. Providers configured from IdP metadata were unaffected because metadata carries base64 DER. Store base64 DER, as metadata does. The saved idpCertificate PEM and idpCertFile path are read as before; only the descriptor rebuilt in memory on each load changes, so no saved configuration needs migrating. TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures signs an ACS Response and a LogoutResponse with one test IdP key and checks that inline-certificate, certificate-file and metadata providers all accept them, and that a Response signed by another key is refused. The test allows IdP-initiated responses because ProcessResponse records no AuthnRequest IDs, so at the default every ACS Response is refused on InResponseTo. That is a separate open bug, not fixed here. --- .../v6/internal/subsystems/agent-lifecycle.md | 8 ++ .../v6/internal/subsystems/api-contracts.md | 21 ++++ .../internal/subsystems/storage-recovery.md | 9 ++ internal/api/contract_test.go | 75 +++++++++++++ internal/api/saml_handlers_more_test.go | 106 ++++++++++++++++++ internal/api/saml_service.go | 5 +- internal/api/saml_service_test.go | 10 ++ 7 files changed, 233 insertions(+), 1 deletion(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 0ed3d19b90..7a04ad6bac 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8947,6 +8947,14 @@ cookie for any response (the logout request already cleared both), and `/api/saml/{id}/logout` refuses a request that carries no session. No agent registration, enrolment, install, update or removal path changed. +### Manual SAML IdP certificate published as base64 DER + +`internal/api/saml_service.go` changed only so a SAML provider configured with +`idpSsoUrl` and a PEM `idpCertificate` or `idpCertFile` puts that certificate +into its in-memory IdP descriptor as base64 DER, which crewjam/saml can read +when it verifies the IdP's ACS Responses and LogoutResponses. No agent +registration, enrolment, install, update or removal path changed. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index e95ea4078a..25e4feaca9 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -6495,6 +6495,27 @@ That same SSO boundary also owns manual SAML endpoint validation payloads. through the same validated absolute HTTP(S) helpers instead of letting the manual logout URL drift out of the request model or bypass the governed URL normalization path. +That same manual SAML configuration must verify signatures with the IdP +certificate the administrator supplied. With no IdP metadata, +`SAMLService.buildManualMetadata` in `internal/api/saml_service.go` builds the +IdP descriptor from `idpSsoUrl`, optional `idpSloUrl` and the PEM signing +certificate in `idpCertificate` or `idpCertFile`, and must put that certificate +in the signing `KeyDescriptor` as base64 DER, the `` content +IdP metadata carries. crewjam/saml base64-decodes that field to verify every +ACS Response and LogoutResponse, so the PEM block it once held failed each +check with "illegal base64 data" before any key was compared, and no manually +configured provider could accept a login or complete SLO. The stored +`idpCertificate` stays the PEM the administrator supplied; only the descriptor +rebuilt in memory on each load changed, so no saved configuration needs +migrating. `TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures` in +`internal/api/contract_test.go` signs an ACS Response and a LogoutResponse with +one test IdP key and proves an inline-certificate provider, a certificate-file +provider and a metadata provider accept both alike, while a Response signed by +another key is refused. The proof enables IdP-initiated responses because +`SAMLService.ProcessResponse` records no AuthnRequest IDs: with +`allowIdpInitiated` off, crewjam refuses every ACS Response on `InResponseTo` +whatever its signature. That is a known open exception, not part of this +contract. That same SSO provider-detail boundary must return the non-secret nested provider configuration used by the settings edit form. `GET /api/security/sso/providers/{id}` may keep the flat list/card fields for diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 3d353be523..cc3cc4c71c 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6886,6 +6886,15 @@ refuses a LogoutResponse that does not answer one of them. The records are not persisted, and the SLO handler no longer writes session state. No storage, retention, backup, migration or recovery path is added or moved. +### Manual SAML IdP certificate published as base64 DER + +`internal/api/saml_service.go` now encodes a manually configured SAML IdP +signing certificate as base64 DER in the IdP descriptor it rebuilds in memory +on each load. The saved settings are read as before: `idpCertificate` holds +the PEM the administrator pasted and `idpCertFile` the path of a PEM file, so +nothing persisted changes or needs migrating. No storage, retention, backup, +migration or recovery path is added or moved. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` no longer exempts every request carrying diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 953ed0fba2..8a335d11e8 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -4450,6 +4450,81 @@ func TestContract_SAMLLoginRejectsUnsupportedMethods(t *testing.T) { } } +// A SAML provider configured by hand, with the IdP's SSO URL and its PEM +// signing certificate pasted inline or named by IDPCertFile, verifies the +// IdP's signatures with that certificate exactly as a provider configured from +// IdP metadata does: the ACS accepts the IdP's signed Response and reads the +// user from it, the IdP's signed answer to an SP-initiated logout completes +// that logout, and a Response signed by any other key is refused. Manual +// metadata used to hold the certificate as a PEM block where crewjam/saml +// base64-decodes DER, so every signature check for these providers failed +// before reaching the key. +func TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures(t *testing.T) { + idp := newSAMLManualCertTestIdP(t) + impostor := newSAMLManualCertTestIdP(t) + certFile := filepath.Join(t.TempDir(), "idp-signing.pem") + if err := os.WriteFile(certFile, idp.certPEM, 0o600); err != nil { + t.Fatalf("write IdP certificate file: %v", err) + } + manual := func(inline, file string) *config.SAMLProviderConfig { + return &config.SAMLProviderConfig{ + IDPSSOURL: "https://idp.example.com/sso", + IDPSLOURL: "https://idp.example.com/slo", + IDPEntityID: samlManualCertTestIdPEntityID, + IDPCertificate: inline, + IDPCertFile: file, + } + } + + for _, tc := range []struct { + name string + cfg *config.SAMLProviderConfig + }{ + {name: "IdP metadata", cfg: &config.SAMLProviderConfig{IDPMetadataXML: idp.metadataXML()}}, + {name: "manual inline certificate", cfg: manual(string(idp.certPEM), "")}, + {name: "manual certificate file", cfg: manual("", certFile)}, + } { + t.Run(tc.name, func(t *testing.T) { + // ProcessResponse records no AuthnRequest IDs, so by default + // crewjam refuses every Response on InResponseTo. Allowing + // IdP-initiated responses skips that check and leaves the + // signature, issuer, destination and timing checks to decide. + tc.cfg.AllowIDPInitiated = true + service, err := NewSAMLService(context.Background(), "okta", tc.cfg, "https://pulse.example.com") + if err != nil { + t.Fatalf("NewSAMLService: %v", err) + } + + result, _, err := service.ProcessResponse(idp.acsPost(t, service)) + if err != nil { + t.Fatalf("ACS refused the configured IdP's signed Response: %s", samlRejectionDetail(err)) + } + if result.Username != "alice" { + t.Fatalf("ACS read username %q from the Response, want alice", result.Username) + } + if _, _, err := service.ProcessResponse(impostor.acsPost(t, service)); err == nil { + t.Fatal("ACS accepted a Response signed by a key the provider was not given") + } + + dataPath := t.TempDir() + resetSessionStoreForTests() + t.Cleanup(resetSessionStoreForTests) + resetCSRFStoreForTests() + t.Cleanup(resetCSRFStoreForTests) + InitSessionStore(dataPath) + InitCSRFStore(dataPath) + router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} + router.samlManager.services["okta"] = service + requestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "alice")) + rec := deliverSAMLTestLogoutResponse(router, idp.slo.redirectQuery(t, requestID), "") + if rec.Code != http.StatusFound || rec.Header().Get("Location") != "/?logout=success" { + t.Fatalf("SLO refused the configured IdP's signed LogoutResponse: status=%d location=%q body=%q", + rec.Code, rec.Header().Get("Location"), rec.Body.String()) + } + }) + } +} + func TestContract_FindingJSONSnapshot(t *testing.T) { now := time.Date(2026, 2, 8, 13, 14, 15, 0, time.UTC) lastSeen := now.Add(5 * time.Minute) diff --git a/internal/api/saml_handlers_more_test.go b/internal/api/saml_handlers_more_test.go index 82320f9793..0105dfc39b 100644 --- a/internal/api/saml_handlers_more_test.go +++ b/internal/api/saml_handlers_more_test.go @@ -8,6 +8,8 @@ import ( "encoding/base64" "encoding/pem" "encoding/xml" + "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -726,3 +728,107 @@ func TestExtractSAMLProviderID(t *testing.T) { t.Fatalf("expected empty provider for non-saml path, got %q", got) } } + +const samlManualCertTestIdPEntityID = "https://idp.example.com/metadata" + +// samlManualCertTestIdP holds an IdP signing key and its PEM certificate, the +// form an administrator pastes or points IDPCertFile at for a provider +// configured without IdP metadata. It signs ACS Responses and LogoutResponses +// the way the real IdP would. +type samlManualCertTestIdP struct { + certPEM []byte + idp *saml.IdentityProvider + slo *samlSLOTestIdP +} + +func newSAMLManualCertTestIdP(t *testing.T) *samlManualCertTestIdP { + t.Helper() + certPEM, _, key := generateTestCert(t) + block, _ := pem.Decode(certPEM) + if block == nil { + t.Fatal("decode IdP certificate PEM") + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("parse IdP certificate: %v", err) + } + entityID, err := url.Parse(samlManualCertTestIdPEntityID) + if err != nil { + t.Fatalf("parse IdP entity ID: %v", err) + } + const signatureMethod = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256" + return &samlManualCertTestIdP{ + certPEM: certPEM, + idp: &saml.IdentityProvider{ + Key: key, + Certificate: cert, + MetadataURL: *entityID, + SignatureMethod: signatureMethod, + }, + slo: &samlSLOTestIdP{signer: &saml.ServiceProvider{ + EntityID: samlManualCertTestIdPEntityID, + Key: key, + Certificate: cert, + SignatureMethod: signatureMethod, + }}, + } +} + +// metadataXML publishes the same signing certificate as IdP metadata does, +// base64 DER inside . +func (idp *samlManualCertTestIdP) metadataXML() string { + return ` + + + + + ` + base64.StdEncoding.EncodeToString(idp.idp.Certificate.Raw) + ` + + + + + +` +} + +// acsPost returns the HTTP-POST binding request to the service's ACS carrying +// a signed, unsolicited (no InResponseTo) Response that identifies alice. +func (idp *samlManualCertTestIdP) acsPost(t *testing.T, service *SAMLService) *http.Request { + t.Helper() + spMetadata := service.sp.Metadata() + authn := &saml.IdpAuthnRequest{ + IDP: idp.idp, + HTTPRequest: httptest.NewRequest(http.MethodPost, "https://idp.example.com/sso", nil), + ServiceProviderMetadata: spMetadata, + SPSSODescriptor: &spMetadata.SPSSODescriptors[0], + ACSEndpoint: &saml.IndexedEndpoint{Binding: saml.HTTPPostBinding, Location: service.sp.AcsURL.String()}, + Now: time.Now(), + } + if err := (saml.DefaultAssertionMaker{}).MakeAssertion(authn, &saml.Session{ + ID: "idp-session-alice", + CreateTime: time.Now(), + ExpireTime: time.Now().Add(time.Hour), + Index: "idx-alice", + NameID: "alice", + }); err != nil { + t.Fatalf("make assertion: %v", err) + } + form, err := authn.PostBinding() + if err != nil { + t.Fatalf("sign ACS Response: %v", err) + } + req := httptest.NewRequest(http.MethodPost, service.sp.AcsURL.String(), + strings.NewReader(url.Values{"SAMLResponse": {form.SAMLResponse}}.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + return req +} + +// samlRejectionDetail adds crewjam/saml's private reason to a rejected +// response, which its public error ("Authentication failed") hides. +func samlRejectionDetail(err error) string { + var invalid *saml.InvalidResponseError + if errors.As(err, &invalid) && invalid.PrivateErr != nil { + return err.Error() + ": " + invalid.PrivateErr.Error() + } + return fmt.Sprint(err) +} diff --git a/internal/api/saml_service.go b/internal/api/saml_service.go index c020c82f63..962327f139 100644 --- a/internal/api/saml_service.go +++ b/internal/api/saml_service.go @@ -359,8 +359,11 @@ func (s *SAMLService) addIDPCertificate(metadata *saml.EntityDescriptor) error { Use: "signing", KeyInfo: saml.KeyInfo{ X509Data: saml.X509Data{ + // Base64 DER, as carries it in IdP + // metadata: crewjam/saml base64-decodes this field to + // verify signatures and cannot read a PEM block. X509Certificates: []saml.X509Certificate{ - {Data: string(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: cert.Raw}))}, + {Data: base64.StdEncoding.EncodeToString(cert.Raw)}, }, }, }, diff --git a/internal/api/saml_service_test.go b/internal/api/saml_service_test.go index 83743e181a..fe15ffa417 100644 --- a/internal/api/saml_service_test.go +++ b/internal/api/saml_service_test.go @@ -7,6 +7,7 @@ import ( "crypto/rand" "crypto/rsa" "crypto/x509" + "encoding/base64" "encoding/pem" "errors" "fmt" @@ -90,6 +91,7 @@ func TestBuildManualMetadataAndCertificate(t *testing.T) { cfg.IDPIssuer = "issuer" certPEM, _, _ := generateTestCert(t) cfg.IDPCertificate = string(certPEM) + certBlock, _ := pem.Decode(certPEM) metadata, err := service.buildManualMetadata() if err != nil { @@ -104,6 +106,14 @@ func TestBuildManualMetadataAndCertificate(t *testing.T) { if len(metadata.IDPSSODescriptors[0].KeyDescriptors) == 0 { t.Fatal("expected key descriptor with certificate") } + // crewjam/saml reads content: base64 DER, never PEM. + certs := metadata.IDPSSODescriptors[0].KeyDescriptors[0].KeyInfo.X509Data.X509Certificates + if len(certs) != 1 { + t.Fatalf("expected one signing certificate, got %d", len(certs)) + } + if certs[0].Data != base64.StdEncoding.EncodeToString(certBlock.Bytes) { + t.Fatalf("expected the signing certificate as base64 DER, got %.40q", certs[0].Data) + } cfg.IDPSSOURL = "https://user:pass@idp.example.com/sso" if _, err := service.buildManualMetadata(); err == nil { From b3b60f063224ba2d061e750782e806444176a7c6 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 7 Oct 2026 23:02:13 +0100 Subject: [PATCH 4/4] Bind SAML logins to the browser that started them With allowIdpInitiated off, the default, every SAML login failed: ProcessResponse passed crewjam/saml no request IDs, and crewjam refuses a Response whose InResponseTo is not among them. Only providers that allowed IdP-initiated logins, which skips the check, could sign in. MakeAuthRequest now records each AuthnRequest ID with the login's returnTo and the hash of a random token that /login sets as the HttpOnly SameSite=Lax cookie pulse_saml_login (__Host- over HTTPS), reused across tabs. The IdP posts its Response cross-site, which withholds Lax cookies, so the ACS answers a cookieless POST with a page that re-posts the same fields once from Pulse's origin. The Response must answer an unexpired request bound to the presenting browser; it spends that request and returns to the recorded path. Another browser's request is left for its owner. The request ID is read from the signature covering the assertion crewjam returns (the signed Response, else that assertion's own SubjectConfirmationData), not from crewjam's parsed fields and not from other signed assertions riding in the same unsigned Response. encoding/xml fills an un-namespaced attribute field from a namespace declaration with the same local name, and exclusive canonicalization drops an unused one from the signed form, so xmlns:InResponseTo could rebind a signed Response and xmlns:IssueInstant / xmlns:NotOnOrAfter let a stale one pass crewjam's age checks: an IdP-initiated login could be replayed indefinitely. The signature verifier now refuses signed content declaring a prefix named like an attribute crewjam reads, in both modes. RelayState is now escaped into the redirect query, so a returnTo with its own query string reaches the IdP whole. --- .../v6/internal/subsystems/agent-lifecycle.md | 11 + .../v6/internal/subsystems/api-contracts.md | 69 ++- .../internal/subsystems/storage-recovery.md | 10 + internal/api/audit_handlers_test.go | 45 ++ internal/api/contract_test.go | 33 +- internal/api/saml_handlers.go | 138 +++++- internal/api/saml_handlers_more_test.go | 427 +++++++++++++++++- internal/api/saml_service.go | 368 +++++++++++++-- internal/api/saml_service_additional_test.go | 2 +- internal/api/saml_service_test.go | 168 ++++++- 10 files changed, 1185 insertions(+), 86 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 7a04ad6bac..781bd1f3b8 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -8955,6 +8955,17 @@ into its in-memory IdP descriptor as base64 DER, which crewjam/saml can read when it verifies the IdP's ACS Responses and LogoutResponses. No agent registration, enrolment, install, update or removal path changed. +### SAML logins bound to the browser that started them + +`internal/api/saml_service.go` and `internal/api/saml_handlers.go` changed only +so `/api/saml/{id}/login` records each SAML AuthnRequest against a short-lived +login cookie and `/api/saml/{id}/acs` accepts the IdP's signed answer to it +once, in that browser, at default settings, reposting the IdP's cross-site +delivery once from Pulse's origin so the cookie arrives. Signed content +carrying a prefixed attribute or namespace declaration named like a SAML +attribute is refused. No agent +registration, enrolment, install, update or removal path changed. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 25e4feaca9..720412f6a6 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -5210,6 +5210,67 @@ in `internal/api/audit_handlers_test.go` replays one user's signed response into another user's browser and proves the victim's session survives and the refusal is audited. +The SAML ACS at `/api/saml/{id}/acs` accepts a Response, at default settings, +only as the IdP's answer to a login the presenting browser started. crewjam/saml +refuses a Response unless its `InResponseTo`, and that of each bearer +`SubjectConfirmationData`, is one of the request IDs its caller supplies, and +Pulse used to supply none, so with `allowIdpInitiated` off, the default, every +SAML login failed with "Authentication failed". `SAMLService.MakeAuthRequest` +in `internal/api/saml_service.go` therefore records each AuthnRequest ID, on +the provider's own service, with the sanitized `returnTo` and the hash of a +random token that `handleSAMLLogin` sets as the `HttpOnly`, `SameSite=Lax` +cookie `pulse_saml_login` (`__Host-pulse_saml_login` over HTTPS) for ten +minutes. A browser that already holds a token keeps it, so logins started one +after another in several tabs all stay valid. A record is honored for ten +minutes, judged when the Response spends it, unless the 1,024-record cap +evicts it first, oldest first. The IdP delivers its Response by a cross-site +HTTP-POST, which browsers send without `SameSite=Lax` cookies, so +`handleSAMLACS` answers a POST that carries a `SAMLResponse` or +`SAMLart` but no login cookie with a page that posts the same fields once more +from Pulse's own origin. The form has no `action`, so it returns to the URL +the IdP posted to, path prefix included, its script carries the request's CSP +nonce, and a marker field prevents a second repost. `SAMLService.ProcessResponse` +then has crewjam check the Response against every outstanding request ID and +reads the request it answers from the canonical form goxmldsig verified for +the signature covering the assertion crewjam returned, never from crewjam's +parsed fields, taking only an `InResponseTo` written without a prefix: the +signed `Response`'s, or that of the `Response` inside a signed +`ArtifactResponse`, when the IdP signed the message, and otherwise that of +every `SubjectConfirmationData` in the returned assertion's own `Subject`, +which must agree and of which there must be at least one, and no other signed +assertion may carry its ID. Other assertions in the Response, and assertions +in `Advice`, do not take part. That record is +spent only when it is bound to the presenting browser's token, and the login +returns to the recorded `returnTo`, not to the posted `RelayState`, which +`MakeAuthRequest` now escapes into the redirect query so a `returnTo` with its +own query string is not cut short at the IdP. A Response answering another browser's login, an unknown, +expired or already spent request, or none is refused with the +`saml_validation_failed` redirect and a failed `saml_login` audit event, and +refusing another browser's Response leaves that browser's record for it. With +`allowIdpInitiated` on, crewjam skips `InResponseTo`, so a Response answering +any request or none is accepted in any browser without the cookie or the +repost and returns to its posted `RelayState`, as before. In both modes the +signature verifier refuses signed content carrying a prefixed attribute or a +namespace declaration named like an un-namespaced attribute crewjam reads from +a Response or Assertion, such as `xmlns:IssueInstant`, `xmlns:NotOnOrAfter` or +`ext:ID`: encoding/xml fills an un-namespaced attribute field from any +attribute with that local name, and exclusive canonicalization leaves an +unused declaration out of the signed form, so appending one let a stale signed +Response pass crewjam's age checks, and an IdP-initiated login could be +replayed indefinitely. The records live in memory, so a +restart or a saved provider change (which replaces the provider's service; +a metadata refresh keeps the records) during the IdP round trip, or a login +started on a host that does not share cookies with the configured public +URL's, leaves the Response unmatched and the user starts the login again. Two logins started at +the same moment in a browser that holds no token yet each mint one and only +the last cookie set survives, so the other fails the same way, and, as with +OIDC login state, a flood of unauthenticated login starts can evict pending +records. +`TestSAMLACSAuditsResponseNotAnsweringThisBrowsersLogin` in +`internal/api/audit_handlers_test.go` delivers one browser's signed Response to +another browser with a login in progress and proves it is refused and audited +while its own browser completes the login. + Alert delivery diagnosis is a read-only monitoring API contract. `GET /api/alerts/delivery-diagnosis?alertIdentifier=` returns the alert manager's current delivery-policy projection for one active alert, including @@ -6511,11 +6572,9 @@ migrating. `TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures` in `internal/api/contract_test.go` signs an ACS Response and a LogoutResponse with one test IdP key and proves an inline-certificate provider, a certificate-file provider and a metadata provider accept both alike, while a Response signed by -another key is refused. The proof enables IdP-initiated responses because -`SAMLService.ProcessResponse` records no AuthnRequest IDs: with -`allowIdpInitiated` off, crewjam refuses every ACS Response on `InResponseTo` -whatever its signature. That is a known open exception, not part of this -contract. +another key is refused. The proof runs at default settings, with each +Response answering an AuthnRequest that `SAMLService.MakeAuthRequest` +recorded for the test browser, as the SAML ACS binding above requires. That same SSO provider-detail boundary must return the non-secret nested provider configuration used by the settings edit form. `GET /api/security/sso/providers/{id}` may keep the flat list/card fields for diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index cc3cc4c71c..1093076371 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6895,6 +6895,16 @@ the PEM the administrator pasted and `idpCertFile` the path of a PEM file, so nothing persisted changes or needs migrating. No storage, retention, backup, migration or recovery path is added or moved. +### SAML logins bound to the browser that started them + +`internal/api/saml_service.go` keeps each outstanding SAML AuthnRequest ID in +memory on the provider's service, with the login's return path and the hash of +the browser's `pulse_saml_login` cookie token, and honors it for ten minutes +(at most 1,024 records; expired ones are dropped on the next login). The cookie +expires after ten minutes. Nothing is persisted, so a restart during the IdP +round trip refuses the returning Response and the user signs in again. No +storage, retention, backup, migration or recovery path is added or moved. + ### Demo write guard ignores websocket upgrade headers `internal/api/demo_middleware.go` no longer exempts every request carrying diff --git a/internal/api/audit_handlers_test.go b/internal/api/audit_handlers_test.go index 57e40f74cc..cf49bf20d1 100644 --- a/internal/api/audit_handlers_test.go +++ b/internal/api/audit_handlers_test.go @@ -2,11 +2,13 @@ package api import ( "context" + "encoding/base64" "encoding/json" "fmt" "net" "net/http" "net/http/httptest" + "net/url" "strings" "testing" "time" @@ -1131,3 +1133,46 @@ func TestSAMLSLOAuditsLogoutResponseNotBoundToSession(t *testing.T) { t.Fatalf("expected one successful saml_slo_callback event, got %+v", completions) } } + +// At the ACS, a correctly signed Response answering another browser's login is +// refused, creates no session and is audited as a binding refusal, while the +// browser that started that login completes it and is audited as a success. +func TestSAMLACSAuditsResponseNotAnsweringThisBrowsersLogin(t *testing.T) { + previousManager := GetTenantAuditManager() + SetTenantAuditManager(nil) + t.Cleanup(func() { + SetTenantAuditManager(previousManager) + }) + logger := &testAuditLoggerNoVerify{} + setAuditLogger(t, logger) + + idp := newSAMLManualCertTestIdP(t) + router, service := newSAMLACSTestRouter(t, idp, false) + attackerRequestID, attackerToken := startSAMLTestLogin(t, router, "", "/") + _, victimToken := startSAMLTestLogin(t, router, "", "/") + form := url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, attackerRequestID))}} + + loginEvents := func() []audit.Event { + var events []audit.Event + for _, event := range logger.events { + if event.EventType == "saml_login" { + events = append(events, event) + } + } + return events + } + + assertSAMLLoginRefused(t, deliverSAMLTestResponse(t, router, form, victimToken), "another browser's Response") + refusals := loginEvents() + if len(refusals) != 1 || refusals[0].Success || + refusals[0].Details != "SAML response does not answer a login started in this browser" { + t.Fatalf("expected one failed saml_login binding refusal, got %+v", refusals) + } + + logger.events = nil + assertSAMLLoginSucceeded(t, deliverSAMLTestResponse(t, router, form, attackerToken), "/?saml=success", "the Response's own browser") + completions := loginEvents() + if len(completions) != 1 || !completions[0].Success || completions[0].Details != "SAML login success via okta" { + t.Fatalf("expected one successful saml_login event, got %+v", completions) + } +} diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 8a335d11e8..a02232fc8d 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -4453,9 +4453,10 @@ func TestContract_SAMLLoginRejectsUnsupportedMethods(t *testing.T) { // A SAML provider configured by hand, with the IdP's SSO URL and its PEM // signing certificate pasted inline or named by IDPCertFile, verifies the // IdP's signatures with that certificate exactly as a provider configured from -// IdP metadata does: the ACS accepts the IdP's signed Response and reads the -// user from it, the IdP's signed answer to an SP-initiated logout completes -// that logout, and a Response signed by any other key is refused. Manual +// IdP metadata does: at default settings the ACS accepts the IdP's signed +// answer to an SP-initiated login and reads the user from it, the IdP's signed +// answer to an SP-initiated logout completes that logout, and a Response +// signed by any other key is refused. Manual // metadata used to hold the certificate as a PEM block where crewjam/saml // base64-decodes DER, so every signature check for these providers failed // before reaching the key. @@ -4485,34 +4486,32 @@ func TestContract_SAMLManualIDPCertificateVerifiesIdPSignatures(t *testing.T) { {name: "manual certificate file", cfg: manual("", certFile)}, } { t.Run(tc.name, func(t *testing.T) { - // ProcessResponse records no AuthnRequest IDs, so by default - // crewjam refuses every Response on InResponseTo. Allowing - // IdP-initiated responses skips that check and leaves the - // signature, issuer, destination and timing checks to decide. - tc.cfg.AllowIDPInitiated = true service, err := NewSAMLService(context.Background(), "okta", tc.cfg, "https://pulse.example.com") if err != nil { t.Fatalf("NewSAMLService: %v", err) } + browserKey := sessionHash("login-token") + startLogin := func() string { + t.Helper() + redirectURL, err := service.MakeAuthRequest("/", browserKey) + if err != nil { + t.Fatalf("MakeAuthRequest: %v", err) + } + return samlTestAuthnRequestID(t, redirectURL) + } - result, _, err := service.ProcessResponse(idp.acsPost(t, service)) + result, _, err := service.ProcessResponse(idp.acsPostAnswering(t, service, startLogin()), browserKey) if err != nil { t.Fatalf("ACS refused the configured IdP's signed Response: %s", samlRejectionDetail(err)) } if result.Username != "alice" { t.Fatalf("ACS read username %q from the Response, want alice", result.Username) } - if _, _, err := service.ProcessResponse(impostor.acsPost(t, service)); err == nil { + if _, _, err := service.ProcessResponse(impostor.acsPostAnswering(t, service, startLogin()), browserKey); err == nil { t.Fatal("ACS accepted a Response signed by a key the provider was not given") } - dataPath := t.TempDir() - resetSessionStoreForTests() - t.Cleanup(resetSessionStoreForTests) - resetCSRFStoreForTests() - t.Cleanup(resetCSRFStoreForTests) - InitSessionStore(dataPath) - InitCSRFStore(dataPath) + useSAMLTestAuthStores(t) router := &Router{samlManager: NewSAMLServiceManager("https://pulse.example.com")} router.samlManager.services["okta"] = service requestID := startSAMLTestLogout(t, router, newSAMLTestSession(t, "alice")) diff --git a/internal/api/saml_handlers.go b/internal/api/saml_handlers.go index b34723de02..4688838a07 100644 --- a/internal/api/saml_handlers.go +++ b/internal/api/saml_handlers.go @@ -2,9 +2,11 @@ package api import ( "context" + "encoding/base64" "encoding/json" "errors" "fmt" + "html/template" "net/http" "strings" "sync" @@ -155,13 +157,26 @@ func (r *Router) handleSAMLLogin(w http.ResponseWriter, req *http.Request) { } } - // Create SAML AuthnRequest - redirectURL, err := service.MakeAuthRequest(returnTo) + // Create SAML AuthnRequest, bound to this browser's login cookie + loginToken, err := samlLoginToken(req) + if err != nil { + log.Error().Err(err).Str("provider_id", providerID).Msg("Failed to create SAML login binding") + writeErrorResponse(w, http.StatusInternalServerError, "saml_auth_failed", "Failed to create authentication request", nil) + return + } + redirectURL, err := service.MakeAuthRequest(returnTo, sessionHash(loginToken)) if err != nil { log.Error().Err(err).Str("provider_id", providerID).Msg("Failed to create SAML auth request") writeErrorResponse(w, http.StatusInternalServerError, "saml_auth_failed", "Failed to create authentication request", nil) return } + cookiePolicy := getBrowserCookiePolicy(req) + cookiePolicy.setHTTPOnly(w, &http.Cookie{ + Name: samlLoginCookieName(cookiePolicy.secure), + Value: loginToken, + Path: "/", + MaxAge: int(samlRequestTTL / time.Second), + }) LogAuditEventForTenant(GetOrgID(req.Context()), "saml_login_initiated", "", GetClientIP(req), req.URL.Path, true, "Provider: "+providerID) @@ -208,11 +223,29 @@ func (r *Router) handleSAMLACS(w http.ResponseWriter, req *http.Request) { return } + if err := req.ParseForm(); err != nil { + log.Error().Err(err).Str("provider_id", providerID).Msg("Failed to parse SAML response form") + LogAuditEventForTenant(GetOrgID(req.Context()), "saml_login", "", GetClientIP(req), req.URL.Path, false, "SAML response validation failed: failed to parse form: "+err.Error()) + r.redirectSAMLError(w, req, "", "saml_validation_failed") + return + } + browserKey := samlLoginBrowserKey(req) + if browserKey == "" && !service.AllowsIDPInitiated() && req.PostForm.Get(samlACSRepostField) == "" && + (req.PostForm.Get("SAMLResponse") != "" || req.PostForm.Get("SAMLart") != "") { + writeSAMLACSRepost(w, req) + return + } + // Process SAML response - result, relayState, err := service.ProcessResponse(req) + result, relayState, err := service.ProcessResponse(req, browserKey) if err != nil { - log.Error().Err(err).Str("provider_id", providerID).Msg("Failed to process SAML response") - LogAuditEventForTenant(GetOrgID(req.Context()), "saml_login", "", GetClientIP(req), req.URL.Path, false, "SAML response validation failed: "+err.Error()) + if errors.Is(err, errSAMLResponseUnbound) { + log.Warn().Err(err).Str("provider_id", providerID).Str("client_ip", GetClientIP(req)).Msg("SAML Response does not answer a login started in this browser") + LogAuditEventForTenant(GetOrgID(req.Context()), "saml_login", "", GetClientIP(req), req.URL.Path, false, "SAML response does not answer a login started in this browser") + } else { + log.Error().Err(err).Str("provider_id", providerID).Msg("Failed to process SAML response") + LogAuditEventForTenant(GetOrgID(req.Context()), "saml_login", "", GetClientIP(req), req.URL.Path, false, "SAML response validation failed: "+err.Error()) + } r.redirectSAMLError(w, req, relayState, "saml_validation_failed") return } @@ -627,6 +660,101 @@ func samlSessionBindingKey(req *http.Request) string { return sessionHash(cookie.Value) } +// The SAML login cookie binds each SP-initiated login to the browser that +// started it. handleSAMLLogin sets it to a random token and records the +// AuthnRequest against the token's hash; the ACS accepts the IdP's Response +// only from a browser presenting that token. Like the session cookie it uses +// the __Host- prefix over HTTPS, so a related subdomain cannot plant a token +// of its own. +const ( + cookieNameSAMLLogin = "pulse_saml_login" + cookieNameSAMLLoginSecure = "__Host-pulse_saml_login" + samlLoginTokenBytes = 32 + // samlACSRepostField marks the ACS form writeSAMLACSRepost re-posts. + samlACSRepostField = "PulseSAMLRepost" +) + +func samlLoginCookieName(secure bool) string { + if secure { + return cookieNameSAMLLoginSecure + } + return cookieNameSAMLLogin +} + +// readSAMLLoginCookie reads the login cookie the way readSessionCookie reads +// the session cookie. +func readSAMLLoginCookie(req *http.Request) (*http.Cookie, error) { + if isConnectionSecure(req) { + return req.Cookie(cookieNameSAMLLoginSecure) + } + if cookie, err := req.Cookie(cookieNameSAMLLogin); err == nil { + return cookie, nil + } + return req.Cookie(cookieNameSAMLLoginSecure) +} + +// samlLoginToken returns the login token the browser already holds, so that +// logins started in several tabs all stay bound to it, or a new one. +func samlLoginToken(req *http.Request) (string, error) { + if cookie, err := readSAMLLoginCookie(req); err == nil && validSAMLLoginToken(cookie.Value) { + return cookie.Value, nil + } + return generateRandomURLString(samlLoginTokenBytes) +} + +func validSAMLLoginToken(token string) bool { + if len(token) != base64.RawURLEncoding.EncodedLen(samlLoginTokenBytes) { + return false + } + _, err := base64.RawURLEncoding.DecodeString(token) + return err == nil +} + +// samlLoginBrowserKey returns the binding key of the login token the request +// carries, or "" when it carries none. +func samlLoginBrowserKey(req *http.Request) string { + cookie, err := readSAMLLoginCookie(req) + if err != nil || !validSAMLLoginToken(cookie.Value) { + return "" + } + return sessionHash(cookie.Value) +} + +var samlACSRepostPage = template.Must(template.New("saml-acs-repost").Parse(` +Signing in to Pulse +
{{range .Fields}}{{end}}

Finishing sign-in…

+ +`)) + +// writeSAMLACSRepost answers the IdP's HTTP-POST delivery of a Response that +// arrived without the login cookie. The IdP's page posts to the ACS from +// another site, and browsers withhold SameSite=Lax cookies from cross-site +// POSTs, so the cookie that binds the login to this browser is missing on +// arrival in every browser. The page posts the same fields once more from +// Pulse's own origin, which carries the cookie. The form has no action, so it +// returns to the URL the IdP posted to, path prefix included, and the marker +// field stops a second repost: a browser that still has no cookie started no +// login here, and the ACS refuses its Response. +func writeSAMLACSRepost(w http.ResponseWriter, req *http.Request) { + type field struct{ Name, Value string } + page := struct { + Fields []field + Nonce string + }{Nonce: CSPNonceFromContext(req.Context())} + for _, name := range []string{"SAMLResponse", "SAMLart", "RelayState"} { + if value := req.PostForm.Get(name); value != "" { + page.Fields = append(page.Fields, field{Name: name, Value: value}) + } + } + page.Fields = append(page.Fields, field{Name: samlACSRepostField, Value: "1"}) + + w.Header().Set("Content-Type", "text/html; charset=utf-8") + w.Header().Set("Cache-Control", "no-store") + if err := samlACSRepostPage.Execute(w, page); err != nil { + log.Error().Err(err).Msg("Failed to write SAML ACS repost page") + } +} + // clearSession clears the current session - properly invalidates server-side session // and clears both pulse_session and pulse_csrf cookies func (r *Router) clearSession(w http.ResponseWriter, req *http.Request) { diff --git a/internal/api/saml_handlers_more_test.go b/internal/api/saml_handlers_more_test.go index 0105dfc39b..ec1e80ca31 100644 --- a/internal/api/saml_handlers_more_test.go +++ b/internal/api/saml_handlers_more_test.go @@ -10,10 +10,12 @@ import ( "encoding/xml" "errors" "fmt" + "html" "io" "net/http" "net/http/httptest" "net/url" + "regexp" "strings" "testing" "time" @@ -794,35 +796,450 @@ func (idp *samlManualCertTestIdP) metadataXML() string { // acsPost returns the HTTP-POST binding request to the service's ACS carrying // a signed, unsolicited (no InResponseTo) Response that identifies alice. func (idp *samlManualCertTestIdP) acsPost(t *testing.T, service *SAMLService) *http.Request { + t.Helper() + return samlTestACSRequest(service, url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, ""))}}) +} + +// acsPostAnswering returns the same request for an SP-initiated login: the +// Response and its SubjectConfirmationData answer the AuthnRequest requestID. +func (idp *samlManualCertTestIdP) acsPostAnswering(t *testing.T, service *SAMLService, requestID string) *http.Request { + t.Helper() + return samlTestACSRequest(service, url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, requestID))}}) +} + +// responseXML returns the Response the IdP posts to the service's ACS after +// authenticating alice, answering requestID ("" for an unsolicited one). Both +// the Response and its Assertion are signed. +func (idp *samlManualCertTestIdP) responseXML(t *testing.T, service *SAMLService, requestID string) []byte { + t.Helper() + return idp.signedResponseXML(t, service, requestID, "alice", nil) +} + +// signedResponseXML is responseXML for the user nameID, with edit applied to +// the Assertion before the IdP signs it. +func (idp *samlManualCertTestIdP) signedResponseXML(t *testing.T, service *SAMLService, requestID, nameID string, edit func(*saml.Assertion)) []byte { t.Helper() spMetadata := service.sp.Metadata() authn := &saml.IdpAuthnRequest{ IDP: idp.idp, HTTPRequest: httptest.NewRequest(http.MethodPost, "https://idp.example.com/sso", nil), + Request: saml.AuthnRequest{ID: requestID}, ServiceProviderMetadata: spMetadata, SPSSODescriptor: &spMetadata.SPSSODescriptors[0], ACSEndpoint: &saml.IndexedEndpoint{Binding: saml.HTTPPostBinding, Location: service.sp.AcsURL.String()}, Now: time.Now(), } if err := (saml.DefaultAssertionMaker{}).MakeAssertion(authn, &saml.Session{ - ID: "idp-session-alice", + ID: "idp-session-" + nameID, CreateTime: time.Now(), ExpireTime: time.Now().Add(time.Hour), - Index: "idx-alice", - NameID: "alice", + Index: "idx-" + nameID, + NameID: nameID, }); err != nil { t.Fatalf("make assertion: %v", err) } + if edit != nil { + edit(authn.Assertion) + } form, err := authn.PostBinding() if err != nil { t.Fatalf("sign ACS Response: %v", err) } - req := httptest.NewRequest(http.MethodPost, service.sp.AcsURL.String(), - strings.NewReader(url.Values{"SAMLResponse": {form.SAMLResponse}}.Encode())) + raw, err := base64.StdEncoding.DecodeString(form.SAMLResponse) + if err != nil { + t.Fatalf("decode ACS Response: %v", err) + } + return raw +} + +func samlTestACSRequest(service *SAMLService, form url.Values) *http.Request { + req := httptest.NewRequest(http.MethodPost, service.sp.AcsURL.String(), strings.NewReader(form.Encode())) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") return req } +// samlTestAuthnRequestID returns the ID of the AuthnRequest carried by an +// HTTP-Redirect binding URL to the IdP. +func samlTestAuthnRequestID(t *testing.T, redirectURL string) string { + t.Helper() + location, err := url.Parse(redirectURL) + if err != nil || location.Host != "idp.example.com" { + t.Fatalf("unexpected AuthnRequest redirect %q (%v)", redirectURL, err) + } + var authnRequest struct { + ID string `xml:"ID,attr"` + } + if err := xml.Unmarshal(inflateSAMLTestPayload(t, location.Query().Get("SAMLRequest")), &authnRequest); err != nil { + t.Fatalf("parse AuthnRequest: %v", err) + } + if authnRequest.ID == "" { + t.Fatal("AuthnRequest has no ID") + } + return authnRequest.ID +} + +// samlTestXMLAttr appends attr="value" to the first start tag named tag (any +// prefix) in doc, the way an attacker edits a signed Response in transit. +func samlTestXMLAttr(t *testing.T, doc []byte, tag, attr, value string) []byte { + t.Helper() + re := regexp.MustCompile(`<([A-Za-z0-9]+:)?` + tag + `[\s>/]`) + loc := re.FindIndex(doc) + if loc == nil { + t.Fatalf("no <%s> in %s", tag, doc) + } + end := bytes.IndexByte(doc[loc[0]:], '>') + loc[0] + if doc[end-1] == '/' { + end-- + } + out := append([]byte{}, doc[:end]...) + out = append(out, []byte(` `+attr+`="`+value+`"`)...) + return append(out, doc[end:]...) +} + +// useSAMLTestAuthStores gives the test fresh session and CSRF stores and puts +// the previous ones back afterwards, so tests that run later still find the +// stores they were using. +func useSAMLTestAuthStores(t *testing.T) { + t.Helper() + dataPath := t.TempDir() + + sessionStoreMu.Lock() + previousSessionStore, previousSessionPath := sessionStore, sessionStoreDataPath + sessionStore, sessionStoreDataPath = nil, "" + sessionStoreMu.Unlock() + csrfStoreMu.Lock() + previousCSRFStore, previousCSRFPath := csrfStore, csrfStoreDataPath + csrfStore, csrfStoreDataPath = nil, "" + csrfStoreMu.Unlock() + InitSessionStore(dataPath) + InitCSRFStore(dataPath) + + t.Cleanup(func() { + resetSessionStoreForTests() + resetCSRFStoreForTests() + sessionStoreMu.Lock() + sessionStore, sessionStoreDataPath = previousSessionStore, previousSessionPath + sessionStoreMu.Unlock() + csrfStoreMu.Lock() + csrfStore, csrfStoreDataPath = previousCSRFStore, previousCSRFPath + csrfStoreMu.Unlock() + }) +} + +// samlTestUnsignedResponse removes the Response's own signature, leaving only +// its Assertion signed, as many IdPs send it. +func samlTestUnsignedResponse(t *testing.T, signed []byte) []byte { + t.Helper() + unsigned := regexp.MustCompile(`(?s)(]*>.*?)`).ReplaceAll(signed, []byte("$1")) + if bytes.Equal(unsigned, signed) { + t.Fatalf("could not strip the Response signature from %s", signed) + } + return unsigned +} + +const samlACSTestProvider = "okta" + +// newSAMLACSTestRouter returns a router whose "okta" SAML provider trusts idp +// through its metadata, at default settings unless allowIDPInitiated is set, +// with fresh session and CSRF stores. +func newSAMLACSTestRouter(t *testing.T, idp *samlManualCertTestIdP, allowIDPInitiated bool) (*Router, *SAMLService) { + t.Helper() + useSAMLTestAuthStores(t) + + cfg := &config.SAMLProviderConfig{IDPMetadataXML: idp.metadataXML(), AllowIDPInitiated: allowIDPInitiated} + service, err := NewSAMLService(context.Background(), samlACSTestProvider, cfg, "https://pulse.example.com") + if err != nil { + t.Fatalf("NewSAMLService: %v", err) + } + router := &Router{ + samlManager: NewSAMLServiceManager("https://pulse.example.com"), + ssoConfig: &config.SSOConfig{Providers: []config.SSOProvider{{ + ID: samlACSTestProvider, Name: "Test SAML", Type: config.SSOProviderTypeSAML, Enabled: true, SAML: cfg, + }}}, + } + router.samlManager.services[samlACSTestProvider] = service + return router, service +} + +// startSAMLTestLogin starts SP-initiated login in a browser holding loginToken +// ("" for a browser that has none yet) and returns the ID of the AuthnRequest +// sent to the IdP and the login token the browser holds afterwards. +func startSAMLTestLogin(t *testing.T, router *Router, loginToken, returnTo string) (requestID, heldToken string) { + t.Helper() + target := "/api/saml/okta/login" + if returnTo != "" { + target += "?returnTo=" + url.QueryEscape(returnTo) + } + req := httptest.NewRequest(http.MethodGet, target, nil) + if loginToken != "" { + req.AddCookie(&http.Cookie{Name: cookieNameSAMLLogin, Value: loginToken}) + } + rec := httptest.NewRecorder() + router.handleSAMLLogin(rec, req) + if rec.Code != http.StatusFound { + t.Fatalf("login: expected status %d, got %d body=%q", http.StatusFound, rec.Code, rec.Body.String()) + } + heldToken = loginToken + for _, cookie := range rec.Result().Cookies() { + if cookie.Name == cookieNameSAMLLogin { + if !cookie.HttpOnly || cookie.SameSite != http.SameSiteLaxMode || cookie.MaxAge <= 0 || cookie.Path != "/" { + t.Fatalf("login cookie must be an HttpOnly SameSite=Lax cookie that expires, got %+v", cookie) + } + heldToken = cookie.Value + } + } + if heldToken == "" { + t.Fatal("login set no login cookie") + } + return samlTestAuthnRequestID(t, rec.Header().Get("Location")), heldToken +} + +// postSAMLTestACS delivers an ACS form from a browser holding loginToken ("" +// for none). A cross-site HTTP-POST from the IdP carries no SameSite=Lax +// cookie, so pass "" to model the IdP's own delivery. +func postSAMLTestACS(router *Router, form url.Values, loginToken string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPost, "/api/saml/okta/acs", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if loginToken != "" { + req.AddCookie(&http.Cookie{Name: cookieNameSAMLLogin, Value: loginToken}) + } + rec := httptest.NewRecorder() + router.handleSAMLACS(rec, req) + return rec +} + +// samlTestRepostForm returns the hidden fields of the ACS repost page. +func samlTestRepostForm(t *testing.T, rec *httptest.ResponseRecorder) url.Values { + t.Helper() + if rec.Code != http.StatusOK || !strings.HasPrefix(rec.Header().Get("Content-Type"), "text/html") { + t.Fatalf("expected the ACS repost page, got status %d type %q body=%q", rec.Code, rec.Header().Get("Content-Type"), rec.Body.String()) + } + form := url.Values{} + for _, match := range regexp.MustCompile(``).FindAllStringSubmatch(rec.Body.String(), -1) { + form.Add(match[1], html.UnescapeString(match[2])) + } + return form +} + +// deliverSAMLTestResponse delivers a Response the way the browser that holds +// loginToken receives it: the IdP's cross-site POST, which carries no login +// cookie and gets the repost page, then the page's same-origin repost. +func deliverSAMLTestResponse(t *testing.T, router *Router, form url.Values, loginToken string) *httptest.ResponseRecorder { + t.Helper() + return postSAMLTestACS(router, samlTestRepostForm(t, postSAMLTestACS(router, form, "")), loginToken) +} + +func assertSAMLLoginSucceeded(t *testing.T, rec *httptest.ResponseRecorder, wantLocation, label string) { + t.Helper() + if rec.Code != http.StatusFound || rec.Header().Get("Location") != wantLocation { + t.Fatalf("%s: expected redirect to %q, got status %d location %q body=%q", label, wantLocation, rec.Code, rec.Header().Get("Location"), rec.Body.String()) + } + for _, cookie := range rec.Result().Cookies() { + if cookie.Name == cookieNameSession && cookie.Value != "" && ValidateSession(cookie.Value) { + return + } + } + t.Fatalf("%s: login established no session, cookies %v", label, rec.Result().Cookies()) +} + +func assertSAMLLoginRefused(t *testing.T, rec *httptest.ResponseRecorder, label string) { + t.Helper() + if rec.Code != http.StatusFound || !strings.Contains(rec.Header().Get("Location"), "saml_error=saml_validation_failed") { + t.Fatalf("%s: expected the validation-failed redirect, got status %d location %q body=%q", label, rec.Code, rec.Header().Get("Location"), rec.Body.String()) + } + for _, cookie := range rec.Result().Cookies() { + if cookie.Name == cookieNameSession || cookie.Name == cookieNameSessionSecure { + t.Fatalf("%s: refused login wrote session cookie %+v", label, cookie) + } + } +} + +// At default settings, with IdP-initiated logins off, SP-initiated login +// completes: the IdP's cross-site POST of its signed answer gets the repost +// page, whose same-origin repost carries the login cookie, and the login +// returns to the recorded returnTo whatever RelayState the IdP posts. +func TestHandleSAMLACS_CompletesLoginStartedInThisBrowser(t *testing.T) { + idp := newSAMLManualCertTestIdP(t) + router, service := newSAMLACSTestRouter(t, idp, false) + + requestID, token := startSAMLTestLogin(t, router, "", "/alerts?tab=history&range=7d") + form := url.Values{ + "SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, requestID))}, + "RelayState": {"/settings"}, + } + repost := postSAMLTestACS(router, form, "") + fields := samlTestRepostForm(t, repost) + if fields.Get("SAMLResponse") != form.Get("SAMLResponse") || fields.Get("RelayState") != "/settings" || fields.Get(samlACSRepostField) != "1" { + t.Fatalf("repost page must carry the posted fields and the repost marker, got %v", fields) + } + if cookies := repost.Result().Cookies(); len(cookies) != 0 { + t.Fatalf("repost page must not write cookies, got %v", cookies) + } + if body := repost.Body.String(); strings.Contains(body, " action=") || !strings.Contains(body, "document.forms[0].submit()") { + t.Fatalf("repost page must submit itself back to the URL the IdP posted to, got %q", body) + } + + assertSAMLLoginSucceeded(t, postSAMLTestACS(router, fields, token), "/alerts?range=7d&saml=success&tab=history", "repost with login cookie") +} + +// Each AuthnRequest is answered once, and only in the browser that started it. +// A Response answering another browser's request, an unknown request or none +// is refused, and refusing another browser leaves that browser's login intact. +func TestHandleSAMLACS_RefusesResponsesNotAnsweringThisBrowsersLogin(t *testing.T) { + idp := newSAMLManualCertTestIdP(t) + router, service := newSAMLACSTestRouter(t, idp, false) + + victimRequestID, victimToken := startSAMLTestLogin(t, router, "", "/") + attackerRequestID, attackerToken := startSAMLTestLogin(t, router, "", "/") + attackerForm := url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, attackerRequestID))}} + + assertSAMLLoginRefused(t, deliverSAMLTestResponse(t, router, attackerForm, victimToken), "another browser's Response") + for _, requestID := range []string{"", "id-never-issued-by-this-sp"} { + form := url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, requestID))}} + assertSAMLLoginRefused(t, deliverSAMLTestResponse(t, router, form, victimToken), "InResponseTo="+requestID) + } + assertSAMLLoginRefused(t, postSAMLTestACS(router, url.Values{ + "SAMLResponse": attackerForm["SAMLResponse"], + samlACSRepostField: {"1"}, + }, ""), "repost without a login cookie") + + assertSAMLLoginSucceeded(t, deliverSAMLTestResponse(t, router, attackerForm, attackerToken), "/?saml=success", "the Response's own browser") + assertSAMLLoginRefused(t, deliverSAMLTestResponse(t, router, attackerForm, attackerToken), "replay in the same browser") + + victimForm := url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, victimRequestID))}} + assertSAMLLoginSucceeded(t, deliverSAMLTestResponse(t, router, victimForm, victimToken), "/?saml=success", "the victim's own login after the refusals") +} + +// encoding/xml fills an un-namespaced attribute field from a namespace +// declaration with the same local name, and exclusive canonicalization leaves +// an unused declaration out of the signed form. Declaring xmlns:InResponseTo +// on a signed Response or Assertion must not rebind the attacker's answer to +// the victim's outstanding AuthnRequest, whether the Response itself is signed +// or only its Assertion is. +func TestHandleSAMLACS_InResponseToIgnoresNamespaceDeclarations(t *testing.T) { + idp := newSAMLManualCertTestIdP(t) + router, service := newSAMLACSTestRouter(t, idp, false) + + victimRequestID, victimToken := startSAMLTestLogin(t, router, "", "/") + attackerRequestID, _ := startSAMLTestLogin(t, router, "", "/") + signed := idp.responseXML(t, service, attackerRequestID) + + spoofed := samlTestXMLAttr(t, signed, "Response", "xmlns:InResponseTo", victimRequestID) + spoofed = samlTestXMLAttr(t, spoofed, "SubjectConfirmationData", "xmlns:InResponseTo", victimRequestID) + assertSAMLLoginRefused(t, deliverSAMLTestResponse(t, router, url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(spoofed)}}, victimToken), "signed Response") + + // Without its own signature the Response's attributes are the attacker's + // to write; only the signed Assertion says which request it answers. + assertionOnly := bytes.Replace(samlTestUnsignedResponse(t, signed), []byte(`InResponseTo="`+attackerRequestID+`"`), []byte(`InResponseTo="`+victimRequestID+`"`), 1) + assertionOnly = samlTestXMLAttr(t, assertionOnly, "SubjectConfirmationData", "xmlns:InResponseTo", victimRequestID) + assertSAMLLoginRefused(t, deliverSAMLTestResponse(t, router, url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(assertionOnly)}}, victimToken), "assertion-only signature") + + victimForm := url.Values{"SAMLResponse": {base64.StdEncoding.EncodeToString(idp.responseXML(t, service, victimRequestID))}} + assertSAMLLoginSucceeded(t, deliverSAMLTestResponse(t, router, victimForm, victimToken), "/?saml=success", "the victim's own login after the spoofs") +} + +// Many IdPs sign only the Assertion. Its signed SubjectConfirmationData then +// names the request, and the login completes. A signed assertion that confirms +// no request cannot ride along with one that does: an IdP-signed assertion for +// another user without a SubjectConfirmation, placed ahead of the victim's own +// in an unsigned Response, is the assertion crewjam would return, and the +// victim's request must not vouch for it. +func TestHandleSAMLACS_BindsTheAssertionItReturns(t *testing.T) { + idp := newSAMLManualCertTestIdP(t) + router, service := newSAMLACSTestRouter(t, idp, false) + + victimRequestID, victimToken := startSAMLTestLogin(t, router, "", "/") + victimResponse := samlTestUnsignedResponse(t, idp.responseXML(t, service, victimRequestID)) + + unconfirmed := idp.signedResponseXML(t, service, "", "mallory", func(assertion *saml.Assertion) { + assertion.Subject.SubjectConfirmations = nil + }) + unconfirmedAssertion := regexp.MustCompile(`(?s)`).Find(unconfirmed) + at := bytes.Index(victimResponse, []byte("`}, + "RelayState": {"/"}, + "Unrelated": {"dropped"}, + }.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req = req.WithContext(context.WithValue(req.Context(), cspNonceKey{}, "test-nonce")) + if err := req.ParseForm(); err != nil { + t.Fatalf("parse form: %v", err) + } + rec := httptest.NewRecorder() + writeSAMLACSRepost(rec, req) + + body := rec.Body.String() + if strings.Contains(body, "` { + t.Fatalf("repost must carry the exact posted value, got %v", fields) + } +} + // samlRejectionDetail adds crewjam/saml's private reason to a rejected // response, which its public error ("Authentication failed") hides. func samlRejectionDetail(err error) string { diff --git a/internal/api/saml_service.go b/internal/api/saml_service.go index 962327f139..fae6e23303 100644 --- a/internal/api/saml_service.go +++ b/internal/api/saml_service.go @@ -14,14 +14,18 @@ import ( "io" "net/http" "net/url" + "reflect" + "sort" "strings" "sync" "time" + "github.com/beevik/etree" "github.com/crewjam/saml" "github.com/rcourtman/pulse-go-rewrite/internal/config" "github.com/rcourtman/pulse-go-rewrite/internal/securityutil" "github.com/rs/zerolog/log" + dsig "github.com/russellhaering/goxmldsig" ) // SAMLService manages SAML Service Provider functionality for a single provider @@ -35,16 +39,19 @@ type SAMLService struct { baseURL string lastRefresh time.Time + // authnRequests holds the SP-initiated AuthnRequests still waiting for + // the IdP's Response, each bound to the browser that started the login. + authnRequests samlRequestStore // logoutRequests holds the SP-initiated LogoutRequests still waiting for // the IdP's LogoutResponse. - logoutRequests samlLogoutRequestStore + logoutRequests samlRequestStore } const ( - // samlLogoutRequestTTL bounds how long the IdP round trip of an - // SP-initiated logout may take, matching the OIDC login state lifetime. - samlLogoutRequestTTL = 10 * time.Minute - maxSAMLLogoutRequestEntries = 1024 + // samlRequestTTL bounds how long the IdP round trip of an SP-initiated + // login or logout may take, matching the OIDC login state lifetime. + samlRequestTTL = 10 * time.Minute + maxSAMLRequestEntries = 1024 // samlLogoutResponseMaxBytes matches crewjam/saml's inflate limit for the // HTTP-Redirect binding. samlLogoutResponseMaxBytes = 10 << 20 @@ -55,34 +62,46 @@ const ( // session. var errSAMLLogoutResponseUnbound = errors.New("logout response does not answer an outstanding logout request for this session") -// samlLogoutRequestStore records the LogoutRequests this SP issued, keyed by -// request ID, with the session each one logged out. crewjam/saml checks a -// LogoutResponse's signature, destination, issuer, status and age but never -// its InResponseTo, so without this record any recent signed response from the -// IdP, including one answering another user's logout, would be accepted. -type samlLogoutRequestStore struct { +// errSAMLResponseUnbound marks an ACS Response that does not answer an +// outstanding AuthnRequest this SP issued to the presenting browser. +var errSAMLResponseUnbound = errors.New("response does not answer an outstanding authentication request from this browser") + +// samlRequestStore records the requests this SP issued and is still waiting +// for the IdP to answer, keyed by request ID. Each record carries the key of +// what the request is bound to: the browser that started an SP-initiated +// login, or the session an SP-initiated logout ended. +// +// crewjam/saml checks a LogoutResponse's signature, destination, issuer, +// status and age but never its InResponseTo, so without these records any +// recent signed response from the IdP, including one answering another user's +// logout, would be accepted. For logins crewjam does check InResponseTo, but +// only against the request IDs its caller supplies, and without records there +// were none to supply: every SP-initiated Response was refused. +type samlRequestStore struct { mu sync.Mutex - entries map[string]samlLogoutRequest + entries map[string]samlRequestRecord } -type samlLogoutRequest struct { - sessionKey string - expiresAt time.Time +type samlRequestRecord struct { + bindingKey string + // returnTo is the local path an SP-initiated login returns to. + returnTo string + expiresAt time.Time } -func (s *samlLogoutRequestStore) put(requestID, sessionKey string, now time.Time) { +func (s *samlRequestStore) put(requestID string, record samlRequestRecord, now time.Time) { s.mu.Lock() defer s.mu.Unlock() if s.entries == nil { - s.entries = make(map[string]samlLogoutRequest) + s.entries = make(map[string]samlRequestRecord) } for id, entry := range s.entries { if !now.Before(entry.expiresAt) { delete(s.entries, id) } } - for len(s.entries) >= maxSAMLLogoutRequestEntries { + for len(s.entries) >= maxSAMLRequestEntries { oldestID := "" var oldestExpiry time.Time for id, entry := range s.entries { @@ -93,10 +112,8 @@ func (s *samlLogoutRequestStore) put(requestID, sessionKey string, now time.Time } delete(s.entries, oldestID) } - s.entries[requestID] = samlLogoutRequest{ - sessionKey: sessionKey, - expiresAt: now.Add(samlLogoutRequestTTL), - } + record.expiresAt = now.Add(samlRequestTTL) + s.entries[requestID] = record } // consume spends the outstanding request a verified LogoutResponse answers. @@ -105,7 +122,7 @@ func (s *samlLogoutRequestStore) put(requestID, sessionKey string, now time.Time // it answers an unexpired request and the browser carries either no session // or the session that request logged out. The request is spent on every // verified presentation, so a response is honored at most once. -func (s *samlLogoutRequestStore) consume(requestID, sessionKey string, now time.Time) error { +func (s *samlRequestStore) consume(requestID, sessionKey string, now time.Time) error { s.mu.Lock() defer s.mu.Unlock() @@ -117,12 +134,47 @@ func (s *samlLogoutRequestStore) consume(requestID, sessionKey string, now time. if !now.Before(entry.expiresAt) { return fmt.Errorf("%w: request %q expired", errSAMLLogoutResponseUnbound, requestID) } - if sessionKey != "" && sessionKey != entry.sessionKey { + if sessionKey != "" && sessionKey != entry.bindingKey { return fmt.Errorf("%w: request %q logged out a different session", errSAMLLogoutResponseUnbound, requestID) } return nil } +// outstanding returns every unexpired request, sorted, and whether any of +// them is bound to bindingKey. +func (s *samlRequestStore) outstanding(bindingKey string, now time.Time) (requestIDs []string, bound bool) { + s.mu.Lock() + defer s.mu.Unlock() + + for id, entry := range s.entries { + if now.Before(entry.expiresAt) { + requestIDs = append(requestIDs, id) + bound = bound || (bindingKey != "" && entry.bindingKey == bindingKey) + } + } + sort.Strings(requestIDs) + return requestIDs, bound +} + +// spend removes and returns the unexpired request a verified ACS Response +// answers, provided it is bound to bindingKey, the presenting browser. A +// request bound to another browser is left for that browser, so presenting a +// stolen Response elsewhere does not cost its owner the login. +func (s *samlRequestStore) spend(requestID, bindingKey string, now time.Time) (samlRequestRecord, error) { + s.mu.Lock() + defer s.mu.Unlock() + + entry, ok := s.entries[requestID] + if requestID == "" || bindingKey == "" || !ok || entry.bindingKey != bindingKey { + return samlRequestRecord{}, fmt.Errorf("%w: no outstanding request from this browser matches InResponseTo %q", errSAMLResponseUnbound, requestID) + } + delete(s.entries, requestID) + if !now.Before(entry.expiresAt) { + return samlRequestRecord{}, fmt.Errorf("%w: request %q expired", errSAMLResponseUnbound, requestID) + } + return entry, nil +} + func normalizeSAMLBaseURL(baseURL string) (string, error) { normalized := strings.TrimRight(strings.TrimSpace(baseURL), "/") if normalized == "" { @@ -544,25 +596,40 @@ func (s *SAMLService) loadSPCredentials() (*x509.Certificate, *rsa.PrivateKey, e return cert, key, nil } -// MakeAuthRequest creates a SAML AuthnRequest and returns the redirect URL -func (s *SAMLService) MakeAuthRequest(relayState string) (string, error) { +// MakeAuthRequest creates a SAML AuthnRequest and returns the HTTP-Redirect +// binding URL that sends it to the IdP. It records the request as outstanding +// for browserKey, the login binding key of the browser starting the login, +// together with returnTo, so that ProcessResponse accepts the IdP's answer to +// this request only once, only from that browser, and returns it to returnTo. +func (s *SAMLService) MakeAuthRequest(returnTo, browserKey string) (string, error) { s.mu.RLock() defer s.mu.RUnlock() if s.sp == nil { return "", errors.New("service provider not initialized") } + if browserKey == "" { + return "", errors.New("authentication request has no browser to bind") + } - if relayState == "" { - relayState = "/" + if returnTo == "" { + returnTo = "/" } if len(s.idpMetadata.IDPSSODescriptors) == 0 || len(s.idpMetadata.IDPSSODescriptors[0].SingleSignOnServices) == 0 { return "", errors.New("idp does not support single sign-on") } - // Use the simple redirect method - redirectURL, err := s.sp.MakeRedirectAuthenticationRequest(relayState) + // This is crewjam's MakeRedirectAuthenticationRequest, unrolled to learn + // the request's ID. + authnRequest, err := s.sp.MakeAuthenticationRequest(s.sp.GetSSOBindingLocation(saml.HTTPRedirectBinding), saml.HTTPRedirectBinding, saml.HTTPPostBinding) + if err != nil { + return "", fmt.Errorf("failed to create auth request: %w", err) + } + // Redirect appends RelayState to the query as given, so escape it: a + // returnTo with its own query string would otherwise split into stray + // parameters and come back from the IdP truncated. + redirectURL, err := authnRequest.Redirect(url.QueryEscape(returnTo), s.sp) if err != nil { return "", fmt.Errorf("failed to create auth request: %w", err) } @@ -576,11 +643,35 @@ func (s *SAMLService) MakeAuthRequest(relayState string) (string, error) { if err != nil { return "", fmt.Errorf("failed to validate auth redirect: %w", err) } + s.authnRequests.put(authnRequest.ID, samlRequestRecord{bindingKey: browserKey, returnTo: returnTo}, time.Now()) return validatedURL, nil } -// ProcessResponse processes a SAML response and extracts user information -func (s *SAMLService) ProcessResponse(r *http.Request) (*SAMLAuthResult, string, error) { +// AllowsIDPInitiated reports whether the provider accepts IdP-initiated +// (unsolicited) Responses, which answer no AuthnRequest and so cannot be bound +// to the browser that receives them. +func (s *SAMLService) AllowsIDPInitiated() bool { + s.mu.RLock() + defer s.mu.RUnlock() + return s.sp != nil && s.sp.AllowIDPInitiated +} + +// ProcessResponse verifies the IdP's Response posted to the ACS and extracts +// the user it authenticates, along with the local path the login returns to. +// +// Unless the provider allows IdP-initiated logins, the Response must answer +// an unexpired AuthnRequest from MakeAuthRequest that is bound to browserKey, +// the login binding key of the presenting browser ("" when it carries none), +// and it spends that request, so each login is completed at most once and only +// in the browser that started it. The request ID is read from what the IdP's +// signatures cover, never from crewjam/saml's parsed fields (see +// samlSignedContent). The returned path is the one recorded with the request, +// not the posted RelayState. Binding failures wrap errSAMLResponseUnbound. +// +// With IdP-initiated logins allowed, crewjam skips InResponseTo entirely, so a +// Response answering any request, or none, is accepted in any browser and +// returns to its posted RelayState, as before request binding existed. +func (s *SAMLService) ProcessResponse(r *http.Request, browserKey string) (*SAMLAuthResult, string, error) { s.mu.RLock() defer s.mu.RUnlock() @@ -595,18 +686,43 @@ func (s *SAMLService) ProcessResponse(r *http.Request) (*SAMLAuthResult, string, relayState := r.FormValue("RelayState") - // Allow IdP-initiated flow - possibleRequestIDs := []string{} - if s.sp.AllowIDPInitiated { - possibleRequestIDs = append(possibleRequestIDs, "") + // crewjam checks that the Response answers one of possibleRequestIDs, + // every outstanding AuthnRequest here; spend then decides whether it is + // this browser's. + now := time.Now() + possibleRequestIDs := []string{""} + if !s.sp.AllowIDPInitiated { + var bound bool + possibleRequestIDs, bound = s.authnRequests.outstanding(browserKey, now) + if !bound { + return nil, relayState, fmt.Errorf("%w: this browser has no login in progress", errSAMLResponseUnbound) + } } - // Parse and validate the SAML assertion - assertion, err := s.sp.ParseResponse(r, possibleRequestIDs) + // Parse and validate the SAML assertion on a copy of the service provider + // whose signature verifier keeps what each verified signature covers. + signed := &samlSignedContent{} + sp := *s.sp + sp.SignatureVerifier = signed + assertion, err := sp.ParseResponse(r, possibleRequestIDs) if err != nil { return nil, relayState, fmt.Errorf("failed to validate saml response: %w", err) } + if !s.sp.AllowIDPInitiated { + requestID, err := signed.inResponseTo(assertion.ID) + if err != nil { + return nil, relayState, fmt.Errorf("%w: %v", errSAMLResponseUnbound, err) + } + // Artifact resolution and signature checks take time, so judge + // the request's expiry now rather than when parsing began. + request, err := s.authnRequests.spend(requestID, browserKey, time.Now()) + if err != nil { + return nil, relayState, err + } + relayState = request.returnTo + } + // Extract user information from assertion result := &SAMLAuthResult{ Attributes: make(map[string][]string), @@ -721,7 +837,7 @@ func (s *SAMLService) MakeLogoutRequest(nameID, sessionIdx, sessionKey string) ( if err != nil { return "", err } - s.logoutRequests.put(req.ID, sessionKey, time.Now()) + s.logoutRequests.put(req.ID, samlRequestRecord{bindingKey: sessionKey}, time.Now()) return validatedURL, nil } @@ -827,6 +943,180 @@ func logoutResponseInResponseTo(raw []byte) (string, error) { } } +// samlSignedContent is the signature verifier ProcessResponse gives crewjam/saml +// for one ACS Response. It verifies each signature exactly as crewjam's default +// verifier does and keeps the content that signature covers: the canonical form +// goxmldsig digested, which holds only what the IdP signed. +// +// crewjam reads a Response's fields by unmarshalling the posted elements with +// encoding/xml, which fills an un-namespaced attribute field from any attribute +// with that local name, whatever its prefix, the last one winning. That +// includes a namespace declaration such as xmlns:IssueInstant, and exclusive +// canonicalization leaves a declaration nothing uses out of the signed form, +// so one can be appended to a signed element without breaking its signature +// and override what crewjam checks: InResponseTo, IssueInstant, NotOnOrAfter, +// Recipient, Destination. VerifySignature therefore refuses signed content +// carrying any prefixed attribute or declaration named like an un-namespaced +// attribute crewjam reads, so crewjam's fields match what was signed, and the +// binding decision still reads InResponseTo from the kept canonical content +// rather than from crewjam's fields. +type samlSignedContent struct { + elements []*etree.Element +} + +func (c *samlSignedContent) VerifySignature(validationContext *dsig.ValidationContext, el *etree.Element) error { + if shadow := samlShadowingAttribute(el); shadow != "" { + return fmt.Errorf("cannot validate signature on %s: %s shadows a SAML attribute", el.Tag, shadow) + } + signed, err := validationContext.Validate(el) + if err != nil { + return fmt.Errorf("cannot validate signature on %s: %v", el.Tag, err) + } + c.elements = append(c.elements, signed) + return nil +} + +// inResponseTo returns the AuthnRequest ID that the signature covering the +// assertion crewjam returned, identified by its ID, says it answers. +// +// When the Response, or an ArtifactResponse carrying it, is signed, the IdP +// signed the whole message, the returned assertion included, and the signed +// Response's InResponseTo names the request. Otherwise crewjam verified the +// returned assertion's own signature, and the request is named by that +// assertion's SubjectConfirmationData: every SubjectConfirmation of its +// Subject must carry the same one, and at least one must exist, because +// crewjam accepts a Subject without any. Other signed assertions in the same +// Response, and assertions in Advice, do not take part. +func (c *samlSignedContent) inResponseTo(assertionID string) (string, error) { + for _, el := range c.elements { + response := el + if el.Tag == "ArtifactResponse" { + response = samlChildElement(el, "Response") + } + if response == nil || response.Tag != "Response" { + continue + } + if requestID := samlUnprefixedAttr(response, "InResponseTo"); requestID != "" { + return requestID, nil + } + return "", errors.New("the signed Response answers no request") + } + var assertion *etree.Element + for _, el := range c.elements { + if el.Tag != "Assertion" || samlUnprefixedAttr(el, "ID") != assertionID { + continue + } + if assertion != nil { + return "", fmt.Errorf("several signed assertions carry ID %q", assertionID) + } + assertion = el + } + if assertion == nil { + return "", errors.New("no signature covers the returned assertion") + } + requestID := "" + if subject := samlChildElement(assertion, "Subject"); subject != nil { + for _, confirmation := range subject.ChildElements() { + if confirmation.Tag != "SubjectConfirmation" { + continue + } + // crewjam keeps the last SubjectConfirmationData of each + // confirmation, so every one must agree. + named := false + for _, data := range confirmation.ChildElements() { + if data.Tag != "SubjectConfirmationData" { + continue + } + value := samlUnprefixedAttr(data, "InResponseTo") + if value == "" || (requestID != "" && value != requestID) { + return "", errors.New("the signed assertion's subject confirmations do not name one request") + } + requestID, named = value, true + } + if !named { + return "", errors.New("a signed subject confirmation names no request") + } + } + } + if requestID == "" { + return "", errors.New("the signed assertion has no subject confirmation naming a request") + } + return requestID, nil +} + +// samlChildElement returns el's first child element with local name tag. +func samlChildElement(el *etree.Element, tag string) *etree.Element { + for _, child := range el.ChildElements() { + if child.Tag == tag { + return child + } + } + return nil +} + +// samlUnprefixedAttr returns el's attribute key written without a prefix, the +// one a signature over el covers, or "" when it has none. +func samlUnprefixedAttr(el *etree.Element, key string) string { + for _, attr := range el.Attr { + if attr.Space == "" && attr.Key == key { + return attr.Value + } + } + return "" +} + +// samlBoundAttributeNames holds the name of every un-namespaced XML attribute +// crewjam/saml unmarshals from a Response or an Assertion. Namespaced ones such +// as xsi:type only match their own namespace. +var samlBoundAttributeNames = func() map[string]bool { + names := make(map[string]bool) + seen := make(map[reflect.Type]bool) + var collect func(t reflect.Type) + collect = func(t reflect.Type) { + for t.Kind() == reflect.Pointer || t.Kind() == reflect.Slice || t.Kind() == reflect.Array { + t = t.Elem() + } + if t.Kind() != reflect.Struct || seen[t] { + return + } + seen[t] = true + for i := 0; i < t.NumField(); i++ { + field := t.Field(i) + name, options, _ := strings.Cut(field.Tag.Get("xml"), ",") + if strings.Contains(","+options+",", ",attr,") { + if name == "" { + name = field.Name + } + if !strings.Contains(name, " ") { + names[name] = true + } + } + collect(field.Type) + } + } + collect(reflect.TypeOf(saml.Response{})) + collect(reflect.TypeOf(saml.Assertion{})) + return names +}() + +// samlShadowingAttribute returns the first prefixed attribute or namespace +// declaration in el or its descendants whose local name is also an +// un-namespaced attribute crewjam/saml reads, as written, or "" when there is +// none. +func samlShadowingAttribute(el *etree.Element) string { + for _, attr := range el.Attr { + if attr.Space != "" && samlBoundAttributeNames[attr.Key] { + return attr.Space + ":" + attr.Key + } + } + for _, child := range el.ChildElements() { + if shadow := samlShadowingAttribute(child); shadow != "" { + return shadow + } + } + return "" +} + func validateSAMLRedirectTarget(rawURL string, allowedEndpoints []saml.Endpoint) (string, error) { validatedURL, err := securityutil.NormalizeAbsoluteHTTPURL(rawURL) if err != nil { diff --git a/internal/api/saml_service_additional_test.go b/internal/api/saml_service_additional_test.go index 4ce28c052d..3674b1d2ff 100644 --- a/internal/api/saml_service_additional_test.go +++ b/internal/api/saml_service_additional_test.go @@ -46,7 +46,7 @@ func TestProcessResponse_InvalidResponse(t *testing.T) { req := httptest.NewRequest(http.MethodPost, "/acs", body) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - _, relay, err := service.ProcessResponse(req) + _, relay, err := service.ProcessResponse(req, "") if err == nil { t.Fatal("expected error for invalid response") } diff --git a/internal/api/saml_service_test.go b/internal/api/saml_service_test.go index fe15ffa417..36f85490fb 100644 --- a/internal/api/saml_service_test.go +++ b/internal/api/saml_service_test.go @@ -17,6 +17,7 @@ import ( "testing" "time" + "github.com/beevik/etree" "github.com/crewjam/saml" "github.com/rcourtman/pulse-go-rewrite/internal/config" ) @@ -187,7 +188,7 @@ func TestSAMLServiceBasicFlows(t *testing.T) { t.Fatalf("new service: %v", err) } - url, err := service.MakeAuthRequest("") + url, err := service.MakeAuthRequest("", sessionHash("login-token")) if err != nil || !strings.Contains(url, "SAMLRequest") { t.Fatalf("unexpected auth url: %v %s", err, url) } @@ -202,7 +203,7 @@ func TestSAMLServiceBasicFlows(t *testing.T) { } service = &SAMLService{config: &config.SAMLProviderConfig{}} - if _, err := service.MakeAuthRequest(""); err == nil { + if _, err := service.MakeAuthRequest("", sessionHash("login-token")); err == nil { t.Fatal("expected error when sp missing") } if _, err := service.GetMetadata(); err == nil { @@ -216,36 +217,175 @@ func TestSAMLServiceBasicFlows(t *testing.T) { } } -// Outstanding SLO requests expire after samlLogoutRequestTTL, and the store -// keeps at most maxSAMLLogoutRequestEntries by dropping the oldest. +// Outstanding SLO requests expire after samlRequestTTL, and the store keeps +// at most maxSAMLRequestEntries by dropping the oldest. func TestSAMLLogoutRequestStoreExpiresAndEvictsOldest(t *testing.T) { - var store samlLogoutRequestStore + var store samlRequestStore now := time.Now() key := sessionHash("session-token") + record := samlRequestRecord{bindingKey: key} - store.put("id-expired", key, now) - if err := store.consume("id-expired", key, now.Add(samlLogoutRequestTTL)); !errors.Is(err, errSAMLLogoutResponseUnbound) { + store.put("id-expired", record, now) + if err := store.consume("id-expired", key, now.Add(samlRequestTTL)); !errors.Is(err, errSAMLLogoutResponseUnbound) { t.Fatalf("expected an expired request to be refused, got %v", err) } - store.put("id-live", key, now) - if err := store.consume("id-live", "", now.Add(samlLogoutRequestTTL-time.Second)); err != nil { + store.put("id-live", record, now) + if err := store.consume("id-live", "", now.Add(samlRequestTTL-time.Second)); err != nil { t.Fatalf("expected a live request to be accepted, got %v", err) } - for i := 0; i <= maxSAMLLogoutRequestEntries; i++ { - store.put(fmt.Sprintf("id-%04d", i), key, now.Add(time.Duration(i)*time.Millisecond)) + for i := 0; i <= maxSAMLRequestEntries; i++ { + store.put(fmt.Sprintf("id-%04d", i), record, now.Add(time.Duration(i)*time.Millisecond)) } - if len(store.entries) != maxSAMLLogoutRequestEntries { - t.Fatalf("expected %d outstanding requests, got %d", maxSAMLLogoutRequestEntries, len(store.entries)) + if len(store.entries) != maxSAMLRequestEntries { + t.Fatalf("expected %d outstanding requests, got %d", maxSAMLRequestEntries, len(store.entries)) } if err := store.consume("id-0000", key, now); !errors.Is(err, errSAMLLogoutResponseUnbound) { t.Fatalf("expected the oldest request to be evicted, got %v", err) } - if err := store.consume(fmt.Sprintf("id-%04d", maxSAMLLogoutRequestEntries), key, now); err != nil { + if err := store.consume(fmt.Sprintf("id-%04d", maxSAMLRequestEntries), key, now); err != nil { t.Fatalf("expected the newest request to be kept, got %v", err) } } +// Outstanding AuthnRequests are spent only by the browser they are bound to. A +// request presented by another browser stays outstanding for its owner, and +// each request is spent once and only before it expires. +func TestSAMLAuthnRequestStoreSpendsOnlyForItsBrowser(t *testing.T) { + var store samlRequestStore + now := time.Now() + alice, bob := sessionHash("alice-login"), sessionHash("bob-login") + + store.put("id-alice-2", samlRequestRecord{bindingKey: alice, returnTo: "/alerts"}, now) + store.put("id-alice-1", samlRequestRecord{bindingKey: alice, returnTo: "/"}, now) + store.put("id-bob", samlRequestRecord{bindingKey: bob, returnTo: "/"}, now) + + if got, bound := store.outstanding(alice, now); strings.Join(got, ",") != "id-alice-1,id-alice-2,id-bob" || !bound { + t.Fatalf("outstanding(alice) = %v, %v; want every request, bound", got, bound) + } + if _, bound := store.outstanding("", now); bound { + t.Fatal("a browser without a login token has no request bound to it") + } + if _, err := store.spend("id-bob", alice, now); !errors.Is(err, errSAMLResponseUnbound) { + t.Fatalf("expected another browser's request to be refused, got %v", err) + } + if _, err := store.spend("id-bob", "", now); !errors.Is(err, errSAMLResponseUnbound) { + t.Fatalf("expected a browser without a login token to be refused, got %v", err) + } + if record, err := store.spend("id-bob", bob, now); err != nil || record.returnTo != "/" { + t.Fatalf("refusing other browsers must leave the request for its owner, got %+v %v", record, err) + } + record, err := store.spend("id-alice-2", alice, now) + if err != nil || record.returnTo != "/alerts" { + t.Fatalf("spend(id-alice-2) = %+v %v, want its recorded returnTo", record, err) + } + if _, err := store.spend("id-alice-2", alice, now); !errors.Is(err, errSAMLResponseUnbound) { + t.Fatalf("expected a spent request to be refused, got %v", err) + } + if got, bound := store.outstanding(alice, now.Add(samlRequestTTL)); len(got) != 0 || bound { + t.Fatalf("expired requests must not be outstanding, got %v, %v", got, bound) + } + if _, err := store.spend("id-alice-1", alice, now.Add(samlRequestTTL)); !errors.Is(err, errSAMLResponseUnbound) { + t.Fatalf("expected an expired request to be refused, got %v", err) + } +} + +// The binding decision reads InResponseTo from the canonical content of the +// signature that covers the assertion crewjam returned, taking only the +// attribute written without a prefix: the signed Response's when the message +// is signed, otherwise the returned assertion's own subject confirmations. +func TestSAMLSignedContentInResponseTo(t *testing.T) { + parse := func(doc string) *etree.Element { + t.Helper() + parsed := etree.NewDocument() + if err := parsed.ReadFromString(doc); err != nil { + t.Fatalf("parse %s: %v", doc, err) + } + return parsed.Root() + } + assertion := func(id, subject, advice string) string { + return `` + subject + `` + advice + `` + } + confirmation := func(attrs string) string { + return `` + } + response := func(attrs, body string) string { + return `` + body + `` + } + own, other := `InResponseTo="id-own"`, `InResponseTo="id-other"` + advice := `` + assertion("a9", confirmation(other), "") + `` + + for _, tc := range []struct { + name string + signed []string + want string + }{ + {name: "signed Response", signed: []string{response(`xmlns:InResponseTo="id-other" `+own, assertion("a1", confirmation(other), ""))}, want: "id-own"}, + {name: "signed Response with encrypted Assertion", signed: []string{response(own, ``)}, want: "id-own"}, + {name: "signed ArtifactResponse", signed: []string{`` + response(own, "") + ``}, want: "id-own"}, + {name: "signed Assertion", signed: []string{assertion("a1", confirmation(`xmlns:InResponseTo="id-other" `+own), advice)}, want: "id-own"}, + {name: "signed Response answering no request", signed: []string{response(`xmlns:InResponseTo="id-own"`, assertion("a1", confirmation(own), ""))}}, + {name: "subject confirmations disagree", signed: []string{assertion("a1", confirmation(own)+confirmation(other), "")}}, + {name: "subject confirmation naming no request", signed: []string{assertion("a1", confirmation(own)+confirmation(`Recipient="x"`), "")}}, + {name: "subject confirmation without data", signed: []string{assertion("a1", confirmation(own)+``, "")}}, + {name: "one subject confirmation with conflicting data", signed: []string{assertion("a1", ``, "")}}, + {name: "assertion without subject confirmation", signed: []string{assertion("a1", "", advice)}}, + {name: "another signed assertion names the request", signed: []string{assertion("a1", "", ""), assertion("a2", confirmation(own), "")}}, + {name: "returned assertion not signed", signed: []string{assertion("a2", confirmation(own), "")}}, + {name: "two signed assertions share the returned ID", signed: []string{assertion("a1", confirmation(own), ""), assertion("a1", confirmation(own), "")}}, + } { + t.Run(tc.name, func(t *testing.T) { + content := &samlSignedContent{} + for _, doc := range tc.signed { + content.elements = append(content.elements, parse(doc)) + } + got, err := content.inResponseTo("a1") + if tc.want == "" { + if err == nil { + t.Fatalf("expected a refusal, got %q", got) + } + return + } + if err != nil || got != tc.want { + t.Fatalf("inResponseTo() = %q, %v; want %q", got, err, tc.want) + } + }) + } +} + +// Signed content may not carry a prefixed attribute or namespace declaration +// named like an un-namespaced attribute crewjam/saml reads; the declarations +// and namespaced attributes real IdPs write are untouched. +func TestSAMLShadowingAttribute(t *testing.T) { + for _, name := range []string{"ID", "InResponseTo", "IssueInstant", "Destination", "NotBefore", "NotOnOrAfter", "Recipient", "SessionIndex"} { + if !samlBoundAttributeNames[name] { + t.Fatalf("expected %s among the attributes crewjam/saml reads, got %v", name, samlBoundAttributeNames) + } + } + for _, prefix := range []string{"saml", "samlp", "saml2", "saml2p", "ds", "dsig", "xs", "xsd", "xsi", "xenc", "ec", "md", "type"} { + if samlBoundAttributeNames[prefix] { + t.Fatalf("%q, a namespace prefix or namespaced attribute real IdPs write, is refused", prefix) + } + } + + const ns = `xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:ext="urn:example"` + for _, tc := range []struct { + doc, want string + }{ + {doc: ``, want: "xmlns:NotOnOrAfter"}, + {doc: ``, want: "ext:ID"}, + {doc: `a@example.com`}, + } { + doc := etree.NewDocument() + if err := doc.ReadFromString(tc.doc); err != nil { + t.Fatalf("parse %s: %v", tc.doc, err) + } + if got := samlShadowingAttribute(doc.Root()); got != tc.want { + t.Fatalf("samlShadowingAttribute(%s) = %q, want %q", tc.doc, got, tc.want) + } + } +} + func TestValidateSAMLRedirectTarget(t *testing.T) { allowed := []saml.Endpoint{ {Location: "https://idp.example.com/sso"},