From 08a90a27aac62762b778674ad7699c6efdb93a60 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 14 Aug 2026 06:39:36 -0700 Subject: [PATCH] Build SAML login (enterprise/internal/loginhandler), mirroring OIDC Adds GET /auth/saml/login + POST /auth/saml/acs alongside the existing OIDC pair, both converging on the same upsert-user/resolve-tenant/ issue-session path. loginhandler.New now takes an optional *saml.ServiceProvider, RegisterRoutes registers each protocol's routes independently so either, both, or neither can be configured. SAML's replay/unsolicited-response defense (InResponseTo, standing in for OIDC's state) is carried via a SameSite=None sentry_saml_request cookie -- None because the ACS endpoint receives a cross-site POST from the IdP's origin, which SameSite=Lax cookies are never sent on. enterprise-auth's main.go now fetches+parses SAML_IDP_METADATA_URL at startup (samlsp.FetchMetadata) and wires the result through. Verified to the same bar as OIDC: a real fake IdP (crewjam/saml/samlidp, genuine XML signing/verification) drives the full login->ACS->session-cookie round trip and negative paths (bad InResponseTo, missing request cookie, missing email/NameID, no/multiple tenant memberships), all in loginhandler/saml_test.go, no Docker needed. The login-form HTML is bypassed by pre-seeding a saml.Session directly into samlidp's session store and presenting the matching `session` cookie -- an IdP-supported shortcut (confirmed by reading GetSession), the same "skip the UI, keep the crypto real" approach oidctest gave the OIDC tests. Writing that test caught two real bugs in internal/saml.ParseResponse, both fixed here: it never called r.ParseForm() before reading the POSTed SAMLResponse field, so every real ACS POST would have silently decoded an empty response; and its email-attribute matching missed urn:oid:0.9.2342.19200300.100.1.3 (the standard LDAP "mail" OID), which is what an IdP sends by default absent an explicit AttributeConsumingService request for "email" -- exactly what samlidp's own DefaultAssertionMaker does, and plausibly what real IdPs' default SAML app templates do too. Docs (CLAUDE.md, threat-model.md, architecture.md, enterprise/README.md, phase-4-runbook.md, docker-compose.yml's enterprise-auth comment) updated in lockstep: SAML login moves from "protocol mechanics only" to "built, verified with a real fake IdP, not yet tried against a real external IdP or a running enterprise-auth container" -- the same disclosed gap OIDC already carried. --- CLAUDE.md | 32 +- docker-compose.yml | 14 +- docs/architecture.md | 2 +- docs/phase-4-runbook.md | 72 ++- docs/security/threat-model.md | 53 ++- enterprise/README.md | 33 +- enterprise/cmd/enterprise-auth/main.go | 51 ++- enterprise/go.mod | 1 + enterprise/internal/config/config.go | 12 +- .../internal/loginhandler/loginhandler.go | 211 ++++++--- .../loginhandler/loginhandler_test.go | 24 +- enterprise/internal/loginhandler/saml_test.go | 423 ++++++++++++++++++ enterprise/internal/saml/saml.go | 41 +- enterprise/internal/saml/saml_test.go | 5 +- 14 files changed, 825 insertions(+), 149 deletions(-) create mode 100644 enterprise/internal/loginhandler/saml_test.go diff --git a/CLAUDE.md b/CLAUDE.md index a0719c8..1fc6ac0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -152,14 +152,23 @@ access partway through the phase, so only the audit-logging guarantees were actually confirmed against a live database; the rest is untested beyond "compiles, and skips cleanly when no live database is configured" (see `/docs/phase-4-runbook.md`'s verification-status -section). Human OIDC login is now built too +section). Human SSO login is now built for both protocols (`enterprise/internal/loginhandler`: `GET /auth/oidc/login` + -`GET /auth/oidc/callback`, issuing a real session cookie after resolving -tenant/role from `tenant_memberships`) — genuinely verified, unlike the -ClickHouse pieces, via a real fake IdP that signs and verifies actual -RS256 tokens (`loginhandler_test.go`, all passing), though never tried -against a real external IdP or through a running `enterprise-auth` -container. Tantivy per-tenant index routing is now built too +`GET /auth/oidc/callback`, and `GET /auth/saml/login` + +`POST /auth/saml/acs` via `enterprise/internal/saml`'s `crewjam/saml` +wiring, both issuing a real session cookie after resolving tenant/role +from `tenant_memberships`) — genuinely verified, unlike the ClickHouse +pieces, via a real fake IdP for each protocol that performs actual +cryptographic signing and verification (`coreos/go-oidc`'s `oidctest` +for OIDC, `crewjam/saml/samlidp` for SAML — `loginhandler_test.go` and +`saml_test.go`, all passing, including the full login round trip and +negative paths for both), though never tried against a real external IdP +or through a running `enterprise-auth` container. Writing the SAML test +caught and fixed two real bugs in `internal/saml.ParseResponse`: a +missing `r.ParseForm()` call that would have silently broken every real +ACS POST, and email-attribute matching that missed the standard LDAP +"mail" OID IdPs send by default. Tantivy per-tenant index routing is now +built too (`search/src/registry.rs` + `enterprise/internal/searchclient`) — **genuinely verified**, like the OIDC login flow: Tantivy is an embedded library, not a networked service, so the isolation probe (three tenants, @@ -172,11 +181,10 @@ RBAC/audit/SSO, rendering to the same Service name/port either way — a Helm-deployed cluster can't accidentally run the wrong one. `docker-compose.yml` still runs plain `api` unconditionally, though (local/dev parity with the Helm chart's enforcement is real remaining -work). What still keeps this phase from being done: SAML login (protocol -wiring exists, no ACS handler calls it, following OIDC's now-built -pattern), ingest itself has no tenant concept for either storage engine -(every record lands in the one shared ClickHouse database and Tantivy -index no matter what — undesigned, not just unbuilt), and the two +work). What still keeps this phase from being done: ingest itself has no +tenant concept for either storage engine (every record lands in the one +shared ClickHouse database and Tantivy index no matter what — +undesigned, not just unbuilt), and the two provisioning mechanisms (`deploy/operator`'s `Tenant` CRD and `enterprise-api -provision-tenant`) still aren't unified — running both for the same tenant ID is two separate operator actions today. Full diff --git a/docker-compose.yml b/docker-compose.yml index 4b8ad29..9f656fb 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -241,11 +241,15 @@ services: # here so it can be built/run/curled like every other service, but # deliberately NOT wired into api's ENTERPRISE_AUTH_URL or alerting's # API_SERVICE_TOKEN below: turning that on makes every /query and - # /dashboards request require a valid session/service token, and there - # is no OIDC/SAML login flow built yet to issue a human one (see - # enterprise/cmd/enterprise-auth/main.go's doc comment) -- flipping it - # on by default would break the web UI and sentryctl with no way to - # log in. See enterprise/README.md for how to turn enforcement on for + # /dashboards request require a valid session/service token. Both + # OIDC and SAML login flows now exist (enterprise/internal/loginhandler), + # but this compose file sets neither OIDC_ISSUER_URL nor + # SAML_IDP_METADATA_URL, so both stay disabled here, and there's still + # no admin UI to create the first tenant_memberships row -- see + # /docs/phase-4-runbook.md sections 3a/3b for wiring a real IdP and + # bootstrapping that row by hand. Flipping enforcement on by default + # without that would break the web UI and sentryctl with no way to log + # in. See enterprise/README.md for how to turn enforcement on for # manual testing (mint a service token, set the two env vars, restart). enterprise-auth: build: diff --git a/docs/architecture.md b/docs/architecture.md index ec8ecf2..50438f0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -79,7 +79,7 @@ This split is not to be changed without discussion — see CLAUDE.md. | `search` (Rust, Phase 1) | Consumes the same Redpanda topic `ingest` does (own offset tracking), builds a Tantivy full-text index over `message`, serves matches over gRPC. Writes always go to one shared (default) index (`ingest` isn't tenant-aware); reads can be scoped per-tenant via `SearchRequest.tenant_id` and `src/registry.rs`'s `IndexRegistry` (Phase 4) — see "Tenant isolation" below. | | `api` (Go) | gRPC + REST gateway. `POST /query` compiles pipe-syntax or raw SQL to one IR, executed across ClickHouse/Tantivy (`/docs/query-language-design.md`). `internal/dashboards` is CRUD only — panel query execution happens client-side, reusing `/query`. `internal/authz` (Phase 4) enforces RBAC via a network call to `enterprise-auth`, never an import. | | `alerting` (Go, Phase 3) | Evaluates alert rules on an interval, calls `api`'s `POST /query` (via a `RoleService` credential once Phase 4 auth is configured — see `/docs/phase-4-isolation-design.md`'s alerting↔api gap), delivers firing/resolved notifications (webhook/Slack/PagerDuty). | -| `enterprise` (Go, commercial license, Phase 4) | OIDC login (`internal/loginhandler`'s `/auth/oidc/login`+`/auth/oidc/callback`, real IdP round trip, verified with a fake IdP but not a real external one), RBAC storage (`internal/rbacstore`), session/service-token issuance (`internal/session`), the append-only audit log (`internal/audit`), `enterprise-auth`'s HTTP surface (`/internal/authorize`, `/auth/features`), per-tenant ClickHouse provisioning (`internal/tenantprovision`) and query routing (`internal/chrunner`), and `cmd/enterprise-api` — a second binary combining core's `api/queryapi`/`api/dashboards` handlers with these tenant-aware implementations. Never imported by core — see "Licensing boundary" below. Also `internal/searchclient` (per-tenant Tantivy routing, wired the same way into `search`). Does **not** yet include SAML's login ACS handler (protocol mechanics only) — see `/docs/security/threat-model.md`. | +| `enterprise` (Go, commercial license, Phase 4) | OIDC login (`internal/loginhandler`'s `/auth/oidc/login`+`/auth/oidc/callback`) and SAML login (`/auth/saml/login`+`/auth/saml/acs`, via `internal/saml`'s `crewjam/saml` wiring) — both a real IdP round trip, each verified with a real fake IdP (`coreos/go-oidc`'s `oidctest`, `crewjam/saml`'s `samlidp`) but not a real external one, RBAC storage (`internal/rbacstore`), session/service-token issuance (`internal/session`), the append-only audit log (`internal/audit`), `enterprise-auth`'s HTTP surface (`/internal/authorize`, `/auth/features`), per-tenant ClickHouse provisioning (`internal/tenantprovision`) and query routing (`internal/chrunner`), and `cmd/enterprise-api` — a second binary combining core's `api/queryapi`/`api/dashboards` handlers with these tenant-aware implementations. Never imported by core — see "Licensing boundary" below. Also `internal/searchclient` (per-tenant Tantivy routing, wired the same way into `search`). | | `web` (SvelteKit, static build) | Query bar, dashboards, alerts, and (Phase 4) a settings page that renders SSO status via a runtime capability check (`GET /auth/features`) rather than bundling enterprise-licensed components. | | `cli` (`sentryctl`) | `ping`, `query`, `dashboards` (list/get/apply), `alerts` (list/get/apply). `$SENTRYCTL_TOKEN`, if set, is forwarded as a Bearer credential (Phase 4). | | `deploy` | A Helm chart covering every `docker-compose.yml` service, plus (Phase 4) a small Go Operator managing one CRD (`Tenant`) that provisions a per-tenant ClickHouse credential Secret. Never applied to a live cluster in the environment this was built in — see `/deploy/README.md`'s verification section before trusting it. | diff --git a/docs/phase-4-runbook.md b/docs/phase-4-runbook.md index e8525a0..3b52b6d 100644 --- a/docs/phase-4-runbook.md +++ b/docs/phase-4-runbook.md @@ -24,6 +24,20 @@ of what was already run and passed. Two genuine exceptions: all, so this one was actually run in this runbook's own session, not just an earlier one. What's still unverified is wiring it into a real running `enterprise-auth` container against a real external IdP. +- `enterprise/internal/loginhandler`'s full SAML login flow (§3b) -- + same bar as OIDC above, verified against a real fake SAML IdP + (`crewjam/saml/samlidp`: genuine XML signing and signature + verification, a real `AuthnRequest`/`Response` round trip), no Docker + needed. Writing this test caught two real bugs in + `enterprise/internal/saml`, now fixed: `ParseResponse` never called + `r.ParseForm()` before reading the POSTed `SAMLResponse` field (every + real ACS POST would have silently decoded to nothing), and the email- + attribute matching didn't recognize `urn:oid:0.9.2342.19200300.100.1.3` + (the standard LDAP "mail" OID), which is what an IdP sends by default + when the SP doesn't explicitly request an attribute literally named + "email" -- crewjam's own fake IdP hit this path. Same remaining gap as + OIDC: not yet tried against a real external IdP or a running + `enterprise-auth` container. Everything else — `internal/rbacstore`'s CRUD, the auth-enforcement walkthrough, the dashboards tenant-scoping fix, the Helm chart, the @@ -145,6 +159,44 @@ to create the first tenant membership) is exactly the kind of rough edge an admin UI would smooth over -- named as real future work, not hidden. +## 3b. `enterprise-auth`: human login via SAML (new -- same "verified +live in this session, not against a real running container or a real +external IdP" caveat as §3a) + +`enterprise/internal/loginhandler`'s SAML tests already prove the +mechanism works end to end against a real fake SAML IdP (`go test +./internal/loginhandler/... -run SAML -v` from `enterprise/`, no Docker +needed). What's still unverified is wiring it into this actual running +stack. To try that for real, point `docker-compose.yml`'s +`enterprise-auth` service at a real SAML IdP (many identity providers +offer a free developer/trial tenant with SAML app support): + +```sh +# Add to enterprise-auth's environment in docker-compose.yml (or a +# docker-compose.override.yml): +# SAML_ENTITY_ID: "http://localhost:8082/saml/metadata" +# SAML_ACS_URL: "http://localhost:8082/auth/saml/acs" +# SAML_IDP_METADATA_URL: "https://your-idp.example.com/metadata" +# Register SAML_ENTITY_ID/SAML_ACS_URL with the IdP's application config +# -- the IdP needs Sentry's ACS URL to know where to POST the assertion. + +docker compose up -d --build enterprise-auth +curl -s http://localhost:8082/auth/features +# expect: {"sso_configured":true,"oidc_enabled":false,"saml_enabled":true} +``` + +Bootstrapping the first `tenant_memberships` row is the same manual-SQL +dance as §3a (log in once, it fails with 403, insert the membership +using the `users` row that got created, log in again). Then visit +`http://localhost:8082/auth/saml/login` in a real browser, complete the +IdP's login, and confirm a `sentry_session` cookie lands after redirect +to `POST_LOGIN_REDIRECT_URL`. Note SAML's `sentry_saml_request` cookie +is `SameSite=None`, which requires `Secure` -- i.e. this only works over +HTTPS in a real deployment, unlike OIDC's redirect-based callback which +tolerates plain HTTP for local dev (see +`enterprise/internal/loginhandler/loginhandler.go`'s `handleSAMLLogin` +doc comment for why). + ## 4. Turn on RBAC enforcement and prove it actually blocks/allows Without touching the main stack's `api` container (so step 2's baseline @@ -164,11 +216,11 @@ docker stop sentry-api-enforced ``` `GET /dashboards` on the same enforced instance should return 401 -without a token — there's no way to mint a human (Viewer/Editor/etc.) -session yet (no OIDC/SAML login handler exists — see -`enterprise/cmd/enterprise-auth/main.go`'s doc comment), so this -runbook can't walk through a real human RBAC scenario end to end. That -gap is real, not an oversight in this runbook. +without a token — this section only demonstrates the service-token path +(§3/§3a/§3b cover minting a real human session via OIDC or SAML); walking +that session cookie through this same enforced instance to get a 200 is +left as the natural next verification step once real Docker/K8s access +exists, not yet done in this runbook. ## 5. Dashboards tenant scoping @@ -363,11 +415,11 @@ Full accounting: `/docs/security/threat-model.md`. Headline items: and the one shared Tantivy index no matter what. A newly-provisioned tenant's storage is real, isolated at query time, and permanently empty until this changes — undesigned, not just unbuilt. -- **Human SSO login now works for OIDC** (§3a) -- verified with a real - fake IdP, not yet a real external one or a running `enterprise-auth` - container. **SAML login still doesn't exist** -- protocol wiring only, - no ACS handler. No tenant-picker UI for a multi-membership identity - either (refused outright). +- **Human SSO login now works for both OIDC (§3a) and SAML (§3b)** -- + each verified with a real fake IdP (genuine cryptographic signing and + verification), not yet a real external IdP or a running + `enterprise-auth` container. No tenant-picker UI for a multi-membership + identity either (refused outright) for either protocol. - No admin UI to create a `tenant_memberships` row -- §3a's manual SQL bootstrap is the only way to grant a logged-in identity access today. - **No per-resource dashboard grants** (`dashboard_permissions` has a diff --git a/docs/security/threat-model.md b/docs/security/threat-model.md index 422e8be..e270968 100644 --- a/docs/security/threat-model.md +++ b/docs/security/threat-model.md @@ -130,7 +130,7 @@ to `enterprise-auth` — see "Deployment/network assumptions" below. ## Authentication -**Implemented for OIDC, still missing for SAML.** +**Implemented for both OIDC and SAML, to the same verification bar.** `enterprise/internal/loginhandler` serves `GET /auth/oidc/login` (redirects to the configured IdP, with a short-lived HttpOnly cookie carrying CSRF-protection state) and `GET /auth/oidc/callback` @@ -138,29 +138,46 @@ carrying CSRF-protection state) and `GET /auth/oidc/callback` `enterprise/internal/oidc`'s real `coreos/go-oidc` wiring, upserts a `users` row keyed by SSO subject, resolves tenant/role from `tenant_memberships`, and issues a `session.Manager`-signed session -cookie). Verified end-to-end with real cryptography, not mocked: the -tests spin up a real fake IdP (`coreos/go-oidc`'s own `oidctest` -package) that signs genuine RS256 ID tokens, and -`enterprise/internal/loginhandler`'s handler verifies them for real via -the same code path production uses — every test in -`loginhandler_test.go` passes, including the full login→callback→ -session-cookie round trip. **Not yet verified**: wiring this into a -running `enterprise-auth` container against a *real* external IdP -(Google/Okta/etc.) — that needs real IdP credentials and a reachable -callback URL neither of which this environment has; see -`/docs/phase-4-runbook.md`. +cookie), plus the SAML equivalent, `GET /auth/saml/login` (redirects to +the configured IdP via `enterprise/internal/saml`'s +`ServiceProvider.LoginURL`, persisting the AuthnRequest ID in a +short-lived cookie — SAML's replay/unsolicited-response defense, +standing in for OIDC's `state`) and `POST /auth/saml/acs` (validates the +assertion's signature and `InResponseTo` against that cookie via +`ServiceProvider.ParseResponse`, then converges on the same +upsert/resolve/issue-session path OIDC uses). Both are verified +end-to-end with real cryptography, not mocked: OIDC's tests spin up a +real fake IdP (`coreos/go-oidc`'s own `oidctest` package) that signs +genuine RS256 ID tokens; SAML's tests spin up a real fake IdP +(`crewjam/saml/samlidp`) that builds and signs genuine SAML assertions +and XML-signs the response, exercising the same `ServiceProvider. +ParseResponse` signature-verification path production uses. Every test +in `loginhandler_test.go` and `saml_test.go` passes, including the full +login→callback/ACS→session-cookie round trip for both protocols, and +negative-path tests for each (state/`InResponseTo` mismatch, missing/ +expired credential, missing required claim, no/multiple tenant +memberships). Writing the SAML test caught two real bugs in +`enterprise/internal/saml`'s `ParseResponse`, both fixed before this +verification was considered complete: it never called `r.ParseForm()` +before reading the POSTed `SAMLResponse` field (every real ACS POST +would have decoded an empty response), and its email-attribute matching +missed `urn:oid:0.9.2342.19200300.100.1.3` (the standard LDAP "mail" +OID) — what an IdP sends by default when the SP hasn't explicitly +requested an attribute literally named "email", which is exactly what +`samlidp`'s own default assertion builder does. **Not yet verified for +either protocol**: wiring this into a running `enterprise-auth` +container against a *real* external IdP (Google/Okta/etc.) — that needs +real IdP credentials and a reachable callback/ACS URL neither of which +this environment has; see `/docs/phase-4-runbook.md`. A user with zero or more than one `tenant_memberships` row is refused outright (403 / 501 respectively) rather than guessed at — a tenant-selection UI for the multi-membership case is real, undesigned -future work, not silently approximated. `enterprise/internal/saml` still -only does the protocol mechanics (AuthnRequest generation, assertion -validation) with no ACS HTTP handler calling it — SAML login remains -unimplemented, following `loginhandler`'s OIDC pattern once it is built. +future work, not silently approximated, for either protocol. `GET /auth/features` (`enterprise/internal/authhandler`) reports whether OIDC/SAML are *configured*, for `/web`'s settings page to conditionally render — independent of whether a login button actually exists yet in -the UI (it doesn't; only the two HTTP endpoints do). +the UI (it doesn't; only the HTTP endpoints do). **Implemented for the one machine caller.** `/alerting`'s evaluator is the sole service-to-service caller (`POST /query`, to evaluate rule @@ -370,7 +387,7 @@ terms: | Deployment actually routing traffic to `enterprise-api` (Helm) | **Enforced** — `api`/`enterprise-api` are mutually exclusive, same flag as RBAC/audit/SSO | | Deployment actually routing traffic to `enterprise-api` (docker-compose) | **Not implemented** — `docker-compose.yml` runs plain `api` unconditionally | | Human SSO login — OIDC | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | -| Human SSO login — SAML | **Not implemented** | +| Human SSO login — SAML | **Built, verified with a real fake IdP** (not yet tried against a real external IdP) | | Multi-tenant-membership login (tenant picker) | **Not implemented** — refused with a clear error, not guessed | | Per-resource dashboard grants (`own/granted`) | **Not implemented** | | Query audit logging (routine queries) | **Enforced**, fail-open, and now wired to a real writer via `enterprise-api` (`audit.QueryAPILogger`) | diff --git a/enterprise/README.md b/enterprise/README.md index 79481ff..e660530 100644 --- a/enterprise/README.md +++ b/enterprise/README.md @@ -86,12 +86,27 @@ section for exactly what "not yet run" means here and why. Don't read same "offline action, not a network endpoint" shape as `enterprise-auth -mint-service-token`. +OIDC and SAML login are both now fully wired: `internal/loginhandler` +serves `GET /auth/oidc/login`+`GET /auth/oidc/callback` and +`GET /auth/saml/login`+`POST /auth/saml/acs`, converging on the same +upsert-user/resolve-tenant/issue-session path. Both are verified the +same way -- a real fake IdP with genuine cryptographic signing and +verification (`coreos/go-oidc`'s `oidctest` for OIDC, +`crewjam/saml/samlidp` for SAML), no Docker needed, every test in +`loginhandler_test.go`/`saml_test.go` passing including the full login +round trip and negative paths (bad state/`InResponseTo`, expired/missing +credential, no/multiple tenant memberships). Writing the SAML test +caught two real bugs in `internal/saml.ParseResponse`, both fixed: +missing `r.ParseForm()` before reading the POSTed `SAMLResponse` field, +and email-attribute matching that missed the standard LDAP "mail" OID +(`urn:oid:0.9.2342.19200300.100.1.3`) that IdPs send by default absent +an explicit `AttributeConsumingService` request for "email" -- exactly +what `samlidp`'s own default assertion builder does. Neither protocol +has been tried against a real external IdP or a running +`enterprise-auth` container -- see `/docs/phase-4-runbook.md` §3a/§3b. + **Deliberately deferred, not half-built** -- named explicitly rather than silently left out: -- SAML's login handler (the ACS endpoint) -- `internal/saml` does the - protocol mechanics (AuthnRequest generation, assertion validation); - nothing calls it from an HTTP handler, following `internal/ - loginhandler`'s now-built OIDC pattern once someone builds it. - A tenant-picker UI/flow for an identity with more than one `tenant_memberships` row -- `loginhandler` refuses these logins outright rather than guessing (`ErrMultipleMemberships`). @@ -123,7 +138,7 @@ internal/oidc/ coreos/go-oidc wiring: discovery, login redirect, code internal/saml/ crewjam/saml wiring: SP setup, login redirect, response parsing/validation internal/session/ issues/validates signed session + RoleService tokens internal/authhandler/ POST /internal/authorize, GET /auth/features -internal/loginhandler/ GET /auth/oidc/login, GET /auth/oidc/callback -- the human login flow +internal/loginhandler/ GET /auth/oidc/{login,callback} + GET /auth/saml/login + POST /auth/saml/acs -- the human login flow internal/rbacstore/ users/tenants/tenant_memberships/data_sources CRUD (pgx against sentry_metadata) internal/tenantprovision/ real ClickHouse CREATE DATABASE/USER/GRANT internal/chrunner/ tenant-scoped api/querylang/executor.SQLRunner @@ -134,9 +149,9 @@ internal/apiconfig/ enterprise-api's own env-var config internal/config/ enterprise-auth's env-var config ``` -Future additions: SAML's login handler, `dashboard_permissions` CRUD, -ingest tenant-awareness (undesigned), and real deployment-topology -wiring for `enterprise-api` -- see "Status" above. +Future additions: `dashboard_permissions` CRUD, ingest tenant-awareness +(undesigned), and real deployment-topology wiring for `enterprise-api` +-- see "Status" above. ## Why OIDC and SAML aren't hand-rolled @@ -246,7 +261,7 @@ edit today, not a supported flag. | `OIDC_REDIRECT_URL` | (empty — must be `/auth/oidc/callback`, registered with the IdP) | | `SAML_ENTITY_ID` | (empty) | | `SAML_ACS_URL` | (empty) | -| `SAML_IDP_METADATA_URL` | (empty — presence only feeds `GET /auth/features`; not yet fetched/parsed) | +| `SAML_IDP_METADATA_URL` | (empty — SAML disabled if unset; if set, fetched and parsed at startup via `samlsp.FetchMetadata`, same trust level as `OIDC_ISSUER_URL`'s discovery fetch) | | `ENTERPRISE_SESSION_SIGNING_KEY` | **required**, min 32 bytes | | `POST_LOGIN_REDIRECT_URL` | `http://localhost:3000` — where the browser lands after `internal/loginhandler` sets a session cookie | diff --git a/enterprise/cmd/enterprise-auth/main.go b/enterprise/cmd/enterprise-auth/main.go index b9fe76a..9d04f04 100644 --- a/enterprise/cmd/enterprise-auth/main.go +++ b/enterprise/cmd/enterprise-auth/main.go @@ -4,15 +4,16 @@ // // Wires session issuance/validation (internal/session), the // POST /internal/authorize endpoint api/authz.HTTPAuthorizer calls (the -// piece that turns on RBAC enforcement in /api), and -- since -// internal/loginhandler -- the real GET /auth/oidc/login and -// GET /auth/oidc/callback handlers that issue a *human* session after -// an actual IdP round trip, resolving tenant/role via internal/rbacstore. -// Still deliberately missing: SAML's equivalent (ACS endpoint) -- same -// shape, not yet built, following internal/loginhandler's OIDC pattern -// once it is. What's fully wired: -mint-service-token (the RoleService -// credential /alerting presents) and, when OIDC_ISSUER_URL is -// configured, a real human login flow. +// piece that turns on RBAC enforcement in /api), and -- +// internal/loginhandler -- the real GET /auth/{oidc,saml}/login and +// GET /auth/oidc/callback + POST /auth/saml/acs handlers that issue a +// *human* session after an actual IdP round trip, resolving tenant/role +// via internal/rbacstore. Both protocols are now fully wired -- OIDC via +// discovery, SAML via fetching+parsing SAML_IDP_METADATA_URL at startup +// (crewjam/saml's samlsp.FetchMetadata; a trusted operator-supplied URL, +// same trust level as OIDC_ISSUER_URL's discovery fetch, not +// end-user-controlled input). Also fully wired: -mint-service-token +// (the RoleService credential /alerting presents). package main import ( @@ -21,12 +22,14 @@ import ( "fmt" "log/slog" "net/http" + "net/url" "os" "os/signal" "strings" "syscall" "time" + "github.com/crewjam/saml/samlsp" "github.com/jackc/pgx/v5/pgxpool" "github.com/sentry/sentry/enterprise/internal/authhandler" @@ -34,6 +37,7 @@ import ( "github.com/sentry/sentry/enterprise/internal/loginhandler" "github.com/sentry/sentry/enterprise/internal/oidc" "github.com/sentry/sentry/enterprise/internal/rbacstore" + samlpkg "github.com/sentry/sentry/enterprise/internal/saml" "github.com/sentry/sentry/enterprise/internal/session" ) @@ -114,6 +118,33 @@ func main() { logger.Info("OIDC not configured (OIDC_ISSUER_URL unset) -- skipping discovery, /auth/oidc/* routes disabled") } + // samlProvider stays nil (loginhandler.RegisterRoutes then registers + // nothing) unless SAML is actually configured -- same shape as OIDC + // above. + var samlProvider *samlpkg.ServiceProvider + if cfg.SAML.IDPMetadataURL != "" { + metadataURL, err := url.Parse(cfg.SAML.IDPMetadataURL) + if err != nil { + logger.Error("parsing SAML_IDP_METADATA_URL", "error", err) + os.Exit(1) + } + idpMetadata, err := samlsp.FetchMetadata(ctx, http.DefaultClient, *metadataURL) + if err != nil { + logger.Error("fetching SAML IdP metadata", "error", err) + os.Exit(1) + } + samlProvider, err = samlpkg.New(samlpkg.Config{ + EntityID: cfg.SAML.EntityID, ACSURL: cfg.SAML.ACSURL, IDPMetadata: idpMetadata, + }) + if err != nil { + logger.Error("constructing SAML service provider", "error", err) + os.Exit(1) + } + logger.Info("SAML provider configured", "idp_metadata_url", cfg.SAML.IDPMetadataURL) + } else { + logger.Info("SAML not configured (SAML_IDP_METADATA_URL unset) -- /auth/saml/* routes disabled") + } + mux := http.NewServeMux() mux.HandleFunc("GET /healthz", func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) @@ -123,7 +154,7 @@ func main() { SAMLEnabled: cfg.SAML.IDPMetadataURL != "", } authhandler.New(logger, sessionManager, features).RegisterRoutes(mux) - loginhandler.New(logger, oidcProvider, sessionManager, rbac, cfg.PostLoginRedirectURL).RegisterRoutes(mux) + loginhandler.New(logger, oidcProvider, samlProvider, sessionManager, rbac, cfg.PostLoginRedirectURL).RegisterRoutes(mux) srv := &http.Server{Addr: cfg.HTTPListenAddr, Handler: mux} diff --git a/enterprise/go.mod b/enterprise/go.mod index bd9a35f..6ae14d8 100644 --- a/enterprise/go.mod +++ b/enterprise/go.mod @@ -37,6 +37,7 @@ require ( github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/go-faster/city v1.0.1 // indirect github.com/go-faster/errors v0.7.1 // indirect + github.com/golang-jwt/jwt/v4 v4.5.2 // indirect github.com/jackc/pgpassfile v1.0.0 // indirect github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/jackc/puddle/v2 v2.2.2 // indirect diff --git a/enterprise/internal/config/config.go b/enterprise/internal/config/config.go index 8cb98e3..0e1cd5c 100644 --- a/enterprise/internal/config/config.go +++ b/enterprise/internal/config/config.go @@ -37,13 +37,11 @@ type OIDCConfig struct { RedirectURL string } -// SAMLConfig is likewise optional. Note this only records *presence* -- -// enough for /auth/features (internal/authhandler) to report -// saml_enabled -- it does not itself fetch/parse IDPMetadataURL into the -// *saml.EntityDescriptor internal/saml.New requires; that fetch (and the -// login/ACS HTTP handlers that would use it) is deferred, same as OIDC's -// login/callback handlers -- see cmd/enterprise-auth/main.go's doc -// comment. +// SAMLConfig is likewise optional. cmd/enterprise-auth/main.go fetches +// and parses IDPMetadataURL into the *saml.EntityDescriptor +// internal/saml.New requires at startup (crewjam/saml's +// samlsp.FetchMetadata) -- this struct just carries the raw config +// values this package's job (env-var loading) is scoped to. type SAMLConfig struct { EntityID string ACSURL string diff --git a/enterprise/internal/loginhandler/loginhandler.go b/enterprise/internal/loginhandler/loginhandler.go index 96aab97..3b4da5e 100644 --- a/enterprise/internal/loginhandler/loginhandler.go +++ b/enterprise/internal/loginhandler/loginhandler.go @@ -2,19 +2,19 @@ // the actual HTTP login/callback flow that issues a *human* session, // not just /alerting's RoleService credential (-mint-service-token) or // the RBAC-enforcement plumbing that assumes a session already exists. -// enterprise/internal/oidc does the OAuth2/OIDC protocol mechanics -// (discovery, the auth-code redirect, code exchange, ID token -// verification); this package is the two HTTP handlers that drive it -// and decide what happens with a verified identity: look up or create a -// users row, resolve which tenant/role that user belongs to, and issue -// a session.Manager-signed session cookie. +// enterprise/internal/oidc and enterprise/internal/saml do the protocol +// mechanics (discovery/AuthnRequest generation, code exchange/assertion +// parsing, signature verification); this package is the HTTP handlers +// that drive them and decide what happens with a verified identity: look +// up or create a users row, resolve which tenant/role that user belongs +// to, and issue a session.Manager-signed session cookie. Both protocols +// share that decision (resolveIdentity below) -- only how the identity +// gets verified differs. // -// Deliberately out of scope here: SAML's equivalent (ACS endpoint) -- -// same shape, not yet built, following this package's pattern once it -// is. Multi-tenant users (one identity with memberships in more than -// one tenant) are refused with a clear error rather than guessing which -// tenant to log them into -- a tenant-selection step is real, -// undesigned future work, not silently approximated. +// Multi-tenant users (one identity with memberships in more than one +// tenant) are refused with a clear error rather than guessing which +// tenant to log them into -- a tenant-selection step is real, undesigned +// future work, not silently approximated. package loginhandler import ( @@ -28,20 +28,28 @@ import ( "github.com/sentry/sentry/enterprise/internal/authhandler" "github.com/sentry/sentry/enterprise/internal/oidc" "github.com/sentry/sentry/enterprise/internal/rbacstore" + "github.com/sentry/sentry/enterprise/internal/saml" "github.com/sentry/sentry/enterprise/internal/session" ) -// stateCookieName carries the CSRF-protection state value between the -// login redirect and the callback -- a short-lived, scoped-to-the- +// oidcStateCookieName carries OIDC's CSRF-protection state value between +// the login redirect and the callback -- a short-lived, scoped-to-the- // callback-path cookie (the "double-submit cookie" pattern) rather than // server-side state, since this service otherwise has no per-browser // session store to put it in before a session exists. -const stateCookieName = "sentry_oidc_state" +const oidcStateCookieName = "sentry_oidc_state" -// stateCookieTTL bounds how long a user has to complete the IdP round +// samlRequestCookieName is SAML's analog -- carries the AuthnRequest ID +// LoginURL generated, so the ACS handler can pass it back to +// ParseResponse's possibleRequestIDs (SAML's actual replay/unsolicited- +// response defense -- see saml.ServiceProvider.LoginURL's doc comment). +const samlRequestCookieName = "sentry_saml_request" + +// loginCookieTTL bounds how long a user has to complete the IdP round // trip -- generous enough for a real login form, short enough that a -// stale state cookie isn't a long-lived CSRF token sitting in a browser. -const stateCookieTTL = 10 * time.Minute +// stale cookie isn't a long-lived CSRF token sitting in a browser. +// Shared by both protocols' cookies. +const loginCookieTTL = 10 * time.Minute // userStore is the narrow interface Handler depends on -- *rbacstore.Store // is the production implementation; tests use a fake, same pattern used @@ -60,9 +68,16 @@ type oidcProvider interface { Exchange(ctx context.Context, code string) (*oidc.Claims, error) } +// samlProvider mirrors oidcProvider's reasoning for SAML. +type samlProvider interface { + LoginURL(relayState string) (redirectURL, requestID string, err error) + ParseResponse(r *http.Request, possibleRequestIDs []string) (*saml.Claims, error) +} + type Handler struct { logger *slog.Logger oidc oidcProvider // nil if OIDC isn't configured -- RegisterRoutes registers nothing in that case + saml samlProvider // nil if SAML isn't configured -- same session *session.Manager users userStore // postLoginRedirectURL is where the browser lands after a session @@ -70,36 +85,46 @@ type Handler struct { postLoginRedirectURL string } -// New takes a concrete *oidc.Provider (nilable), not the oidcProvider -// interface directly -- a nil *oidc.Provider assigned straight into an -// interface-typed field would produce a non-nil interface wrapping a -// nil pointer (Go's classic typed-nil trap), which would silently break -// RegisterRoutes'/handleLogin's `h.oidc == nil` checks the moment a -// caller (enterprise-auth's main.go) passes a `var p *oidc.Provider` -// that's legitimately still nil because OIDC isn't configured. Checking -// the concrete pointer here, before it ever becomes the interface -// field, is what keeps that check meaningful. -func New(logger *slog.Logger, provider *oidc.Provider, sessionManager *session.Manager, users userStore, postLoginRedirectURL string) *Handler { +// New takes concrete *oidc.Provider/*saml.ServiceProvider (both +// nilable), not the narrower interfaces directly -- assigning a nil +// pointer straight into an interface-typed field would produce a +// non-nil interface wrapping a nil pointer (Go's classic typed-nil +// trap), which would silently break RegisterRoutes'/the handlers' +// `h.oidc == nil`/`h.saml == nil` checks the moment a caller +// (enterprise-auth's main.go) passes a `var p *oidc.Provider` that's +// legitimately still nil because that protocol isn't configured. +// Checking the concrete pointers here, before they ever become the +// interface fields, is what keeps those checks meaningful -- see +// loginhandler_test.go's TestRegisterRoutesNoOpWithTypedNilProviderVariable +// for the regression test that caught this the first time (OIDC; SAML +// follows the same fix from day one). +func New(logger *slog.Logger, oidcProvider *oidc.Provider, samlProvider *saml.ServiceProvider, sessionManager *session.Manager, users userStore, postLoginRedirectURL string) *Handler { h := &Handler{logger: logger, session: sessionManager, users: users, postLoginRedirectURL: postLoginRedirectURL} - if provider != nil { - h.oidc = provider + if oidcProvider != nil { + h.oidc = oidcProvider + } + if samlProvider != nil { + h.saml = samlProvider } return h } -// RegisterRoutes registers OIDC's two routes only if OIDC is actually -// configured (h.oidc != nil) -- matches the "absent, not broken" default +// RegisterRoutes registers each protocol's routes only if that protocol +// is actually configured -- matches the "absent, not broken" default // every other optional-config path in this codebase follows (e.g. // api/authz.RequireRole's nil-authorizer no-op). func (h *Handler) RegisterRoutes(mux *http.ServeMux) { - if h.oidc == nil { - return + if h.oidc != nil { + mux.HandleFunc("GET /auth/oidc/login", h.handleOIDCLogin) + mux.HandleFunc("GET /auth/oidc/callback", h.handleOIDCCallback) + } + if h.saml != nil { + mux.HandleFunc("GET /auth/saml/login", h.handleSAMLLogin) + mux.HandleFunc("POST /auth/saml/acs", h.handleSAMLACS) } - mux.HandleFunc("GET /auth/oidc/login", h.handleLogin) - mux.HandleFunc("GET /auth/oidc/callback", h.handleCallback) } -func (h *Handler) handleLogin(w http.ResponseWriter, r *http.Request) { +func (h *Handler) handleOIDCLogin(w http.ResponseWriter, r *http.Request) { state, err := oidc.NewState() if err != nil { h.logger.Error("generating oidc state", "error", err) @@ -107,29 +132,29 @@ func (h *Handler) handleLogin(w http.ResponseWriter, r *http.Request) { return } http.SetCookie(w, &http.Cookie{ - Name: stateCookieName, Value: state, Path: "/auth/oidc/callback", + Name: oidcStateCookieName, Value: state, Path: "/auth/oidc/callback", HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, - MaxAge: int(stateCookieTTL.Seconds()), + MaxAge: int(loginCookieTTL.Seconds()), }) http.Redirect(w, r, h.oidc.AuthCodeURL(state), http.StatusFound) } -// clearStateCookie is called on every path out of handleCallback -- -// the state cookie is single-use regardless of whether the login -// ultimately succeeds, same reasoning a CSRF token gets discarded after -// one use rather than left around for reuse. -func clearStateCookie(w http.ResponseWriter, r *http.Request) { +// clearCookie is called on every path out of the two callback handlers +// below -- the state/request cookie is single-use regardless of whether +// the login ultimately succeeds, same reasoning a CSRF token gets +// discarded after one use rather than left around for reuse. +func clearCookie(w http.ResponseWriter, r *http.Request, name, path string) { http.SetCookie(w, &http.Cookie{ - Name: stateCookieName, Value: "", Path: "/auth/oidc/callback", + Name: name, Value: "", Path: path, HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteLaxMode, MaxAge: -1, }) } -func (h *Handler) handleCallback(w http.ResponseWriter, r *http.Request) { - defer clearStateCookie(w, r) +func (h *Handler) handleOIDCCallback(w http.ResponseWriter, r *http.Request) { + defer clearCookie(w, r, oidcStateCookieName, "/auth/oidc/callback") - stateCookie, err := r.Cookie(stateCookieName) + stateCookie, err := r.Cookie(oidcStateCookieName) if err != nil || stateCookie.Value == "" { http.Error(w, "missing or expired login state -- start over at /auth/oidc/login", http.StatusBadRequest) return @@ -156,9 +181,79 @@ func (h *Handler) handleCallback(w http.ResponseWriter, r *http.Request) { return } - identity, status, err := h.resolveIdentity(r.Context(), claims) + h.finishLogin(w, r, claims.Subject, claims.Email) +} + +func (h *Handler) handleSAMLLogin(w http.ResponseWriter, r *http.Request) { + // relayState isn't used to carry anything here (postLoginRedirectURL + // is a fixed server-side config, not per-request) -- still generated + // fresh per login and round-tripped, since crewjam/saml's API expects + // one and an empty/constant value would be a needless deviation from + // how a real SP-initiated flow looks. + relayState, err := oidc.NewState() // same random-value generator, protocol-agnostic despite the package name if err != nil { - h.logger.Error("resolving identity after oidc login", "error", err, "email", claims.Email) + h.logger.Error("generating saml relay state", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + redirectURL, requestID, err := h.saml.LoginURL(relayState) + if err != nil { + h.logger.Error("building saml login url", "error", err) + http.Error(w, "internal error", http.StatusInternalServerError) + return + } + // SameSiteNoneMode, not Lax like OIDC's state cookie: SAML's + // HTTP-POST binding means the browser POSTs to /auth/saml/acs + // *from the IdP's origin* -- a cross-site POST, which SameSite=Lax + // cookies are never sent on (Lax only exempts top-level GET + // navigations, which is what OIDC's redirect-based callback is, but + // SAML's response delivery isn't). SameSite=None requires Secure + // per the cookie spec, so this genuinely needs the deployment to be + // on HTTPS -- realistic for any real SAML IdP integration (they + // require it too), but worth stating plainly: unlike OIDC, SAML + // login will not work correctly over plain HTTP. + http.SetCookie(w, &http.Cookie{ + Name: samlRequestCookieName, Value: requestID, Path: "/auth/saml/acs", + HttpOnly: true, Secure: r.TLS != nil, SameSite: http.SameSiteNoneMode, + MaxAge: int(loginCookieTTL.Seconds()), + }) + http.Redirect(w, r, redirectURL, http.StatusFound) +} + +func (h *Handler) handleSAMLACS(w http.ResponseWriter, r *http.Request) { + defer clearCookie(w, r, samlRequestCookieName, "/auth/saml/acs") + + requestCookie, err := r.Cookie(samlRequestCookieName) + if err != nil || requestCookie.Value == "" { + http.Error(w, "missing or expired login state -- start over at /auth/saml/login", http.StatusBadRequest) + return + } + + claims, err := h.saml.ParseResponse(r, []string{requestCookie.Value}) + if err != nil { + h.logger.Error("parsing saml response", "error", err) + http.Error(w, "login failed", http.StatusUnauthorized) + return + } + if claims.Email == "" { + http.Error(w, "identity provider did not return an email attribute", http.StatusUnauthorized) + return + } + if claims.NameID == "" { + http.Error(w, "identity provider did not return a NameID", http.StatusUnauthorized) + return + } + + h.finishLogin(w, r, claims.NameID, claims.Email) +} + +// finishLogin is the point both protocols converge on: a verified +// (subject, email) pair, still needing tenant/role resolution and a +// session cookie -- everything from here down is protocol-agnostic. +func (h *Handler) finishLogin(w http.ResponseWriter, r *http.Request, subject, email string) { + identity, status, err := h.resolveIdentity(r.Context(), subject, email) + if err != nil { + h.logger.Error("resolving identity after login", "error", err, "email", email) http.Error(w, err.Error(), status) return } @@ -181,7 +276,7 @@ var ( // ErrNoMembership and ErrMultipleMemberships are exported so tests // (and any future caller that wants to distinguish these outcomes, // e.g. to render a real tenant-picker UI instead of a flat error - // page) don't have to string-match handleCallback's HTTP error body. + // page) don't have to string-match the HTTP error body. ErrNoMembership = errors.New("loginhandler: this identity has no tenant membership -- contact your administrator") ErrMultipleMemberships = errors.New("loginhandler: this identity belongs to multiple tenants -- tenant selection is not supported yet") ) @@ -193,12 +288,14 @@ type resolvedIdentity struct { } // resolveIdentity is the policy decision this whole package exists to -// make: given a verified external identity, which tenant/role does it -// map to. Deliberately conservative -- exactly one tenant_memberships -// row is the only case handled; zero or multiple both refuse rather -// than guess (see this package's doc comment). -func (h *Handler) resolveIdentity(ctx context.Context, claims *oidc.Claims) (resolvedIdentity, int, error) { - user, err := h.users.UpsertUserBySSO(ctx, claims.Subject, claims.Email, claims.Email) +// make: given a verified external identity (subject, email -- OIDC's +// "sub"/"email" claims or SAML's NameID/email attribute, already +// protocol-normalized by the caller), which tenant/role does it map to. +// Deliberately conservative -- exactly one tenant_memberships row is the +// only case handled; zero or multiple both refuse rather than guess +// (see this package's doc comment). +func (h *Handler) resolveIdentity(ctx context.Context, subject, email string) (resolvedIdentity, int, error) { + user, err := h.users.UpsertUserBySSO(ctx, subject, email, email) if err != nil { return resolvedIdentity{}, http.StatusInternalServerError, fmt.Errorf("loginhandler: upserting user: %w", err) } diff --git a/enterprise/internal/loginhandler/loginhandler_test.go b/enterprise/internal/loginhandler/loginhandler_test.go index 28ab588..0d63cfb 100644 --- a/enterprise/internal/loginhandler/loginhandler_test.go +++ b/enterprise/internal/loginhandler/loginhandler_test.go @@ -128,7 +128,7 @@ func newTestSessionManager(t *testing.T) *session.Manager { func TestHandleLoginRedirectsAndSetsStateCookie(t *testing.T) { idp := newTestIdP(t) - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -144,7 +144,7 @@ func TestHandleLoginRedirectsAndSetsStateCookie(t *testing.T) { cookies := rec.Result().Cookies() var stateCookie *http.Cookie for _, c := range cookies { - if c.Name == stateCookieName { + if c.Name == oidcStateCookieName { stateCookie = c } } @@ -168,7 +168,7 @@ func fullLoginFlow(t *testing.T, h *Handler, idp *testIdP) *httptest.ResponseRec mux.ServeHTTP(loginRec, httptest.NewRequest(http.MethodGet, "/auth/oidc/login", nil)) var stateCookie *http.Cookie for _, c := range loginRec.Result().Cookies() { - if c.Name == stateCookieName { + if c.Name == oidcStateCookieName { stateCookie = c } } @@ -188,7 +188,7 @@ func TestFullLoginFlowIssuesSessionForSingleMembership(t *testing.T) { store := newFakeUserStore() store.memberships["user-user-1"] = []rbacstore.Membership{{TenantID: "acme", UserID: "user-user-1", Role: rbacstore.RoleEditor}} sessionManager := newTestSessionManager(t) - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), sessionManager, store, "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, sessionManager, store, "http://web/") idp.setNextIDToken(t, "user-1", "person@acme.example", true, time.Now().Add(time.Hour)) rec := fullLoginFlow(t, h, idp) @@ -220,7 +220,7 @@ func TestFullLoginFlowIssuesSessionForSingleMembership(t *testing.T) { func TestFullLoginFlowRefusesNoMembership(t *testing.T) { idp := newTestIdP(t) - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") idp.setNextIDToken(t, "user-2", "nobody@acme.example", true, time.Now().Add(time.Hour)) rec := fullLoginFlow(t, h, idp) @@ -237,7 +237,7 @@ func TestFullLoginFlowRefusesMultipleMemberships(t *testing.T) { {TenantID: "acme", UserID: "user-user-3", Role: rbacstore.RoleViewer}, {TenantID: "globex", UserID: "user-user-3", Role: rbacstore.RoleAdmin}, } - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), store, "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), store, "http://web/") idp.setNextIDToken(t, "user-3", "multi@example.com", true, time.Now().Add(time.Hour)) rec := fullLoginFlow(t, h, idp) @@ -249,12 +249,12 @@ func TestFullLoginFlowRefusesMultipleMemberships(t *testing.T) { func TestCallbackRejectsStateMismatch(t *testing.T) { idp := newTestIdP(t) - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") mux := http.NewServeMux() h.RegisterRoutes(mux) req := httptest.NewRequest(http.MethodGet, "/auth/oidc/callback?state=wrong&code=test-code", nil) - req.AddCookie(&http.Cookie{Name: stateCookieName, Value: "correct"}) + req.AddCookie(&http.Cookie{Name: oidcStateCookieName, Value: "correct"}) rec := httptest.NewRecorder() mux.ServeHTTP(rec, req) @@ -265,7 +265,7 @@ func TestCallbackRejectsStateMismatch(t *testing.T) { func TestCallbackRejectsMissingStateCookie(t *testing.T) { idp := newTestIdP(t) - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -279,7 +279,7 @@ func TestCallbackRejectsMissingStateCookie(t *testing.T) { func TestCallbackRejectsExpiredIDToken(t *testing.T) { idp := newTestIdP(t) - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), newTestOIDCProvider(t, idp), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") idp.setNextIDToken(t, "user-4", "person@example.com", true, time.Now().Add(-time.Hour)) // already expired rec := fullLoginFlow(t, h, idp) @@ -290,7 +290,7 @@ func TestCallbackRejectsExpiredIDToken(t *testing.T) { } func TestRegisterRoutesNoOpWhenOIDCNotConfigured(t *testing.T) { - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), nil, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") mux := http.NewServeMux() h.RegisterRoutes(mux) @@ -314,7 +314,7 @@ func TestRegisterRoutesNoOpWhenOIDCNotConfigured(t *testing.T) { // the trap, only passing a nil-valued typed variable does. func TestRegisterRoutesNoOpWithTypedNilProviderVariable(t *testing.T) { var provider *oidc.Provider // stays nil -- exactly main.go's shape when OIDC_ISSUER_URL is unset - h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), provider, newTestSessionManager(t), newFakeUserStore(), "http://web/") + h := New(slog.New(slog.NewTextHandler(io.Discard, nil)), provider, nil, newTestSessionManager(t), newFakeUserStore(), "http://web/") mux := http.NewServeMux() h.RegisterRoutes(mux) diff --git a/enterprise/internal/loginhandler/saml_test.go b/enterprise/internal/loginhandler/saml_test.go new file mode 100644 index 0000000..563808b --- /dev/null +++ b/enterprise/internal/loginhandler/saml_test.go @@ -0,0 +1,423 @@ +// Mirrors loginhandler_test.go's OIDC approach: exercise the full SAML +// login flow against a real fake IdP rather than mocking anything. +// crewjam/saml ships samlidp, a genuine SAML identity provider (real XML +// signing, real assertion construction) meant for exactly this kind of +// testing. To avoid driving its HTML login form, a valid saml.Session is +// seeded directly into the IdP's session store and presented via the +// `session` cookie GetSession already accepts -- confirmed by reading +// samlidp's own GetSession implementation, the same "skip the UI, keep +// the crypto real" shortcut oidctest gives the OIDC tests above. +package loginhandler + +import ( + "crypto/rand" + "crypto/rsa" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "encoding/xml" + "fmt" + "html" + "io" + "log/slog" + "math/big" + "net/http" + "net/http/httptest" + "net/url" + "regexp" + "strings" + "testing" + "time" + + "github.com/crewjam/saml" + "github.com/crewjam/saml/samlidp" + + "github.com/sentry/sentry/enterprise/internal/rbacstore" + samlpkg "github.com/sentry/sentry/enterprise/internal/saml" +) + +const ( + testSAMLEntityID = "https://sentry-test.example.com/saml/metadata" + testSAMLACSURL = "https://sentry-test.example.com/auth/saml/acs" +) + +// testSAMLIdP bundles a real samlidp.Server with the SP key/cert it was +// registered against, enough to drive a full SP-initiated login. +type testSAMLIdP struct { + server *samlidp.Server + store *samlidp.MemoryStore + spKey *rsa.PrivateKey + spCert *x509.Certificate +} + +func genSelfSignedCert(t *testing.T, commonName string) (*rsa.PrivateKey, *x509.Certificate) { + t.Helper() + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generating RSA key: %v", err) + } + serial, err := rand.Int(rand.Reader, new(big.Int).Lsh(big.NewInt(1), 128)) + if err != nil { + t.Fatalf("generating serial number: %v", err) + } + template := x509.Certificate{ + SerialNumber: serial, + Subject: pkix.Name{CommonName: commonName}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, &template, &template, &key.PublicKey, key) + if err != nil { + t.Fatalf("creating certificate: %v", err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("parsing certificate: %v", err) + } + return key, cert +} + +// newTestSAMLIdP starts a real samlidp.Server and registers Sentry's SP +// metadata with it directly via the IdP's own PUT /services/{id} +// endpoint -- the same mechanism a real IdP admin uses, not a shortcut +// that reaches into samlidp's unexported state. +func newTestSAMLIdP(t *testing.T) *testSAMLIdP { + t.Helper() + idpKey, idpCert := genSelfSignedCert(t, "sentry-test-idp") + spKey, spCert := genSelfSignedCert(t, "sentry-test-sp") + + store := &samlidp.MemoryStore{} + idpServer, err := samlidp.New(samlidp.Options{ + Key: idpKey, + Certificate: idpCert, + Store: store, + URL: url.URL{Scheme: "http", Host: "idp.example.com"}, + }) + if err != nil { + t.Fatalf("samlidp.New: %v", err) + } + + entityIDURL, err := url.Parse(testSAMLEntityID) + if err != nil { + t.Fatalf("parsing test entity id: %v", err) + } + acsURL, err := url.Parse(testSAMLACSURL) + if err != nil { + t.Fatalf("parsing test acs url: %v", err) + } + // A throwaway saml.ServiceProvider built from the same + // EntityID/ACSURL/cert samlpkg.New below uses -- Metadata() is a + // pure function of those exported fields, so this stays consistent + // with the real samlpkg.ServiceProvider without needing access to + // its unexported inner sp field. + spForRegistration := saml.ServiceProvider{ + Key: spKey, + Certificate: spCert, + MetadataURL: *entityIDURL, + AcsURL: *acsURL, + } + spMetadataXML, err := xml.Marshal(spForRegistration.Metadata()) + if err != nil { + t.Fatalf("marshaling sp metadata: %v", err) + } + putReq := httptest.NewRequest(http.MethodPut, "/services/sentry-test-sp", strings.NewReader(string(spMetadataXML))) + putRec := httptest.NewRecorder() + idpServer.ServeHTTP(putRec, putReq) + if putRec.Code != http.StatusNoContent { + t.Fatalf("registering sp metadata with fake idp: status = %d, body = %s", putRec.Code, putRec.Body.String()) + } + + return &testSAMLIdP{server: idpServer, store: store, spKey: spKey, spCert: spCert} +} + +// serviceProvider builds the samlpkg.ServiceProvider loginhandler uses, +// trusting idp's metadata. +func (idp *testSAMLIdP) serviceProvider(t *testing.T) *samlpkg.ServiceProvider { + t.Helper() + sp, err := samlpkg.New(samlpkg.Config{ + EntityID: testSAMLEntityID, + ACSURL: testSAMLACSURL, + IDPMetadata: idp.server.IDP.Metadata(), + Certificate: &tls.Certificate{ + Certificate: [][]byte{idp.spCert.Raw}, + PrivateKey: idp.spKey, + }, + }) + if err != nil { + t.Fatalf("samlpkg.New: %v", err) + } + return sp +} + +// seedSession pre-authenticates a user directly in the fake IdP's store, +// bypassing its login-form HTML entirely. Confirmed viable by reading +// samlidp's GetSession: a valid, non-expired saml.Session at +// /sessions/ plus a matching `session` cookie is exactly what a real +// login-form POST would have produced -- this is the IdP's own supported +// shortcut, not an abuse of internals. +func (idp *testSAMLIdP) seedSession(t *testing.T, nameID, email string) *http.Cookie { + t.Helper() + sessionID := fmt.Sprintf("test-session-%d", time.Now().UnixNano()) + session := &saml.Session{ + ID: sessionID, + NameID: nameID, + CreateTime: saml.TimeNow(), + ExpireTime: saml.TimeNow().Add(time.Hour), + Index: sessionID, + UserEmail: email, + } + if err := idp.store.Put(fmt.Sprintf("/sessions/%s", sessionID), session); err != nil { + t.Fatalf("seeding idp session: %v", err) + } + return &http.Cookie{Name: "session", Value: sessionID} +} + +var samlResponseFieldRe = regexp.MustCompile(`name="(SAMLResponse|RelayState)" value="([^"]*)"`) + +// extractSAMLResponseForm pulls the hidden form fields out of the IdP's +// auto-submitting HTML response -- what a real browser's inline