From 86b988cf924a16f6b32416dae20b3610e57a2cf6 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Tue, 8 Sep 2026 19:50:11 -0400 Subject: [PATCH 1/3] docs(saml): spec entityID as SAML_ENTITY_ID, endpoints on CFP_SITE_HOST The live IdP metadata advertises entityID and SingleSignOnService Locations on the Slack team host because the route derived both from SLACK_TEAM_HOST. Spec now separates the three values by source: the entity ID (metadata entityID + assertion Issuer) is a stable logical identifier from a new optional SAML_ENTITY_ID env var, defaulting to https://codeforphilly.org/api/saml/slack/metadata and deliberately independent of CFP_SITE_HOST so the pre-/post-cutover host flip doesn't invalidate the issuer Slack stored at setup; endpoint Locations follow CFP_SITE_HOST; SLACK_TEAM_HOST keeps only its Slack-side roles. Env tables in architecture.md, deploy.md, secrets.md and .env.example gain the new var. Adds plans/saml-self-host.md (in-progress) to carry the code change, closing the follow-up left open by plans/saml-idp.md. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr --- .env.example | 10 ++++ docs/operations/deploy.md | 4 +- docs/operations/secrets.md | 5 ++ plans/saml-self-host.md | 110 +++++++++++++++++++++++++++++++++++++ specs/api/saml.md | 24 +++++++- specs/architecture.md | 7 ++- 6 files changed, 153 insertions(+), 7 deletions(-) create mode 100644 plans/saml-self-host.md diff --git a/.env.example b/.env.example index 240dcb5..39b3952 100644 --- a/.env.example +++ b/.env.example @@ -71,6 +71,16 @@ CFP_JWT_SIGNING_KEY=change-me-to-a-random-string-at-least-32-chars # PEM-encoded certificate matching SAML_PRIVATE_KEY. # SAML_CERTIFICATE=-----BEGIN CERTIFICATE-----\n...\n-----END CERTIFICATE----- +# Stable IdP entity ID — also the on every assertion. Slack stores +# this at setup, so it must NOT change when CFP_SITE_HOST flips at cutover. +# Leave unset unless registering a separate IdP with a different workspace. +# See specs/api/saml.md#idp-identity-and-hosts. +# SAML_ENTITY_ID=https://codeforphilly.org/api/saml/slack/metadata + +# Slack workspace host. Drives the ACS URL, NameID NameQualifier, and the +# /chat + /launch redirects. Never used for our own entity ID or endpoints. +# SLACK_TEAM_HOST=codeforphilly.slack.com + # --------------------------------------------------------------------------- # Static SPA serving (production only) # --------------------------------------------------------------------------- diff --git a/docs/operations/deploy.md b/docs/operations/deploy.md index 9989b5a..4945c72 100644 --- a/docs/operations/deploy.md +++ b/docs/operations/deploy.md @@ -225,7 +225,8 @@ comments. Production pod gets these mounted: | `CFP_DATA_BRANCH` | ConfigMap | e.g. `fixture` / `main` | | `CFP_DATA_RELOAD_SECRET` | **Secret** | Shared bearer-token for the hot-reload webhook; when unset the `/api/_internal/reload-data` endpoint returns 503. See [runbook.md](runbook.md#hot-reload-webhook). | | `CFP_WEB_DIST_PATH` | ConfigMap | `/app/apps/web/dist` | -| `CFP_SITE_HOST` | ConfigMap | Public-facing host (`codeforphilly.org` base, `next-v2.codeforphilly.org` sandbox). Drives the markdown renderer's external-link transform — anchors with a different host get `target="_blank" rel="noopener nofollow"`. | +| `CFP_SITE_HOST` | ConfigMap | Public-facing host (`codeforphilly.org` base, `next-v2.codeforphilly.org` sandbox). Drives the markdown renderer's external-link transform — anchors with a different host get `target="_blank" rel="noopener nofollow"` — and the SAML IdP metadata's `SingleSignOnService` endpoint URLs. | +| `SAML_ENTITY_ID` | ConfigMap | Optional. Stable SAML IdP entity ID / assertion `Issuer` (default `https://codeforphilly.org/api/saml/slack/metadata`). Leave unset everywhere Slack should keep trusting the production IdP identity — it deliberately does **not** follow `CFP_SITE_HOST`, so flipping the host at cutover doesn't require editing Slack's SAML config. Only set it when standing up a separate IdP registration (e.g. a sandbox pointed at a test workspace). See [specs/api/saml.md](../../specs/api/saml.md#idp-identity-and-hosts). | | `POSTMARK_SERVER_TOKEN` | **Secret** | Postmark server API token for outbound notifications. When unset, the email notifier falls back to a no-op LoggingNotifier — convenient for dev + tests but means no real emails go out. | | `POSTMARK_MESSAGE_STREAM` | ConfigMap | Postmark message stream for outbound mail (default `outbound`). Must exist on the server the token belongs to. | | `CFP_NOTIFICATION_FROM` | ConfigMap | RFC 5322 sender address for outbound notifications (default `"Code for Philly "`). Sender domain must be a verified Postmark sender signature (already true for `codeforphilly.org` via the legacy site) before flipping `POSTMARK_SERVER_TOKEN` on. | @@ -237,6 +238,7 @@ comments. Production pod gets these mounted: | `GITHUB_OAUTH_CLIENT_SECRET` | **Secret** | OAuth app client secret | | `CFP_JWT_SIGNING_KEY` | **Secret** | HS256 key (`openssl rand -base64 64`) | | `SAML_PRIVATE_KEY` / `SAML_CERTIFICATE` | **Secret** | Slack IdP cert chain | +| `SLACK_TEAM_HOST` | ConfigMap | Slack workspace host (default `codeforphilly.slack.com`). ACS URL, NameID `NameQualifier`, `/chat` + `/launch` redirect target. Never our own IdP identity. | | `GIT_SSH_COMMAND` | ConfigMap | Wires `ssh` to the mounted deploy key | ## Rollback diff --git a/docs/operations/secrets.md b/docs/operations/secrets.md index 2e86f0e..e236000 100644 --- a/docs/operations/secrets.md +++ b/docs/operations/secrets.md @@ -108,6 +108,11 @@ integration ([specs/api/saml.md](../../specs/api/saml.md)). - **Rotation impact:** Slack stops trusting assertions until its IdP config is updated with the new cert. **Do not rotate without coordinating with the Slack workspace admin.** +- **Not a secret, but paired:** `SAML_ENTITY_ID` (ConfigMap, optional) is + the IdP identity Slack stores alongside this cert. It defaults to + `https://codeforphilly.org/api/saml/slack/metadata` and must stay stable + across host changes — see [deploy.md](deploy.md#environment-variables-reference) + and [specs/api/saml.md](../../specs/api/saml.md#idp-identity-and-hosts). - **Rotation procedure:** 1. Generate new key + cert. 2. Upload the *new cert* to Slack as a secondary signing cert. diff --git a/plans/saml-self-host.md b/plans/saml-self-host.md new file mode 100644 index 0000000..822dbbb --- /dev/null +++ b/plans/saml-self-host.md @@ -0,0 +1,110 @@ +--- +status: in-progress +depends: [saml-idp] +specs: + - specs/api/saml.md + - specs/architecture.md +issues: [] +--- + +# Plan: SAML IdP identity and endpoint hosts + +## Scope + +Fix the SAML IdP so that the metadata `entityID`, the assertion `Issuer`, and +the `SingleSignOnService` endpoint URLs are built from our own settings rather +than from `SLACK_TEAM_HOST`. Today the live metadata at +`https://next.codeforphilly.org/api/saml/slack/metadata` advertises +`entityID="https://codeforphilly.slack.com/api/saml/slack/metadata"` and +`Location="https://codeforphilly.slack.com/api/saml/slack/sso"` — both on +Slack's host. This closes the "entityID host source" follow-up left open by +[`saml-idp`](saml-idp.md). + +In scope: + +- A new optional `SAML_ENTITY_ID` env var (default + `https://codeforphilly.org/api/saml/slack/metadata`) that is the single + source for both the metadata `entityID` and every assertion ``. +- SSO endpoint `Location`s built from `CFP_SITE_HOST`. +- `SLACK_TEAM_HOST` retains exactly its Slack-side roles: ACS URL, NameID + `NameQualifier`, `/launch` redirect target. +- Spec + operator docs + `.env.example` describing the three-way split. +- Tests covering the default entity ID, the `CFP_SITE_HOST`-driven endpoint + URL, and the entity ID staying put when `CFP_SITE_HOST` changes. + +Out of scope: re-registering the IdP with Slack (operator action, not code); +any change to NameID or the attribute set. + +## Implements + +- [api/saml.md](../specs/api/saml.md) — the new "IdP identity and hosts" + section: `entityID` = `SAML_ENTITY_ID`; `SingleSignOnService/@Location` = + `https:///api/saml/slack/sso`; `Issuer` = `entityID`. +- [architecture.md](../specs/architecture.md) — env table rows for + `SAML_ENTITY_ID`, `SLACK_TEAM_HOST`, `CFP_SITE_HOST`. + +## Approach + +1. **Spec first.** Add the "IdP identity and hosts" section to + `specs/api/saml.md` stating the three sources and the stability rule for + the entity ID; add `SAML_ENTITY_ID` to the env tables in the spec, + `specs/architecture.md`, `docs/operations/deploy.md`, + `docs/operations/secrets.md`, and `.env.example`. +2. **Env.** Add `SAML_ENTITY_ID` to `EnvSchema` (zod) and the mirrored JSON + schema in `apps/api/src/env.ts`, defaulting to + `https://codeforphilly.org/api/saml/slack/metadata`. +3. **Route.** In `getSamlContext` (`apps/api/src/routes/saml.ts`) build the + `SamlIdpSettings` as: + - `entityId: cfg.SAML_ENTITY_ID` + - `ssoLoginPostUrl` / `ssoLoginRedirectUrl`: + `https://${cfg.CFP_SITE_HOST}/api/saml/slack/sso` + - `slackTeamHost: cfg.SLACK_TEAM_HOST` (unchanged) + Drop the `issuerHost` derivation and its misleading comment. +4. **Config.** `apps/api/src/saml/config.ts` already threads + `settings.entityId` into both the samlify `IdentityProvider({ entityID })` + (→ metadata) and `SlackSamlEntities.entityId`, which the route passes as + `issuerEntityId` into `buildResponseSubstitutions` (→ `{Issuer}` on both + the Response and the Assertion). No change needed there beyond doc + comments; verify rather than assume. +5. **Tests.** Update `apps/api/tests/saml.test.ts`: metadata `entityID` + equals the default `SAML_ENTITY_ID`; SSO `Location`s use `CFP_SITE_HOST`; + assertion `Issuer` (both Response and Assertion) equals the entity ID; a + second app booted with `CFP_SITE_HOST=next.example.org` gets `Location` + on that host while `entityID` stays the default; an explicit + `SAML_ENTITY_ID` override flows through to both metadata and `Issuer`. + +## Validation + +- [ ] `GET /api/saml/slack/metadata` `entityID` is + `https://codeforphilly.org/api/saml/slack/metadata` with no + `SAML_ENTITY_ID` set, regardless of `SLACK_TEAM_HOST` / `CFP_SITE_HOST`. +- [ ] Both `SingleSignOnService/@Location` values are + `https:///api/saml/slack/sso`; with + `CFP_SITE_HOST=next.example.org` they use that host. +- [ ] `` on the Response and the Assertion equal the metadata + `entityID`, including when `SAML_ENTITY_ID` is overridden. +- [ ] ACS URL, form action, `NameQualifier`, and `/launch` redirect still use + `SLACK_TEAM_HOST` (existing tests keep passing). +- [ ] `SLACK_TEAM_HOST` no longer appears in any IdP-side URL (grep the route). +- [ ] `npm run type-check && npm run lint && npm test` clean. + +## Risks / unknowns + +- **Slack already has the wrong entity ID on file.** If the `next.` + deployment's metadata was uploaded to Slack, Slack stored + `https://codeforphilly.slack.com/api/saml/slack/metadata` as the IdP + issuer. After this ships, assertions carry the correct issuer and Slack + will reject them until its SAML config is re-synced from the metadata URL. + Operator coordination, noted in Follow-ups. +- **Cutover host flip.** `Location` changes from `next.codeforphilly.org` to + `codeforphilly.org` at cutover; Slack's IdP config needs a metadata refresh + then too (endpoint URLs only — the entity ID is untouched, so the trust + relationship survives). + +## Notes + +(Populated at closeout.) + +## Follow-ups + +(Populated at closeout.) diff --git a/specs/api/saml.md b/specs/api/saml.md index d586994..f99b2f0 100644 --- a/specs/api/saml.md +++ b/specs/api/saml.md @@ -59,11 +59,28 @@ Content-Type: application/samlmetadata+xml; charset=utf-8 The metadata declares: -- `entityID` — our IdP entity ID, e.g., `https://codeforphilly.org/api/saml/slack/metadata` -- `SingleSignOnService` binding(s) — `urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST` (for SP-initiated) and `urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect` +- `entityID` — our IdP entity ID: the value of `SAML_ENTITY_ID` (default `https://codeforphilly.org/api/saml/slack/metadata`). See [IdP identity and hosts](#idp-identity-and-hosts) — this is a stable logical identifier, not a reflection of whatever host is serving the request. +- `SingleSignOnService` binding(s) — `urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST` (for SP-initiated) and `urn:oasis:names:tc:SAML:2.0:bindings:HTTP-Redirect`, both with `Location="https:///api/saml/slack/sso"` - `X509Certificate` — the IdP cert from `SAML_CERTIFICATE` - NameID formats supported: `urn:oasis:names:tc:SAML:2.0:nameid-format:persistent` +## IdP identity and hosts + +Three distinct hosts/identifiers appear in the IdP's output, and each comes from its own setting. They must never be conflated: + +| Value | Source | Where it appears | +| ----- | ------ | ---------------- | +| IdP entity ID | `SAML_ENTITY_ID` (default `https://codeforphilly.org/api/saml/slack/metadata`) | Metadata `entityID`; `` on every Response **and** every Assertion | +| Our SSO endpoint URLs | `https:///api/saml/slack/...` | Metadata `SingleSignOnService/@Location` (both bindings) | +| Slack workspace host | `SLACK_TEAM_HOST` (default `codeforphilly.slack.com`) | ACS URL (`https:///sso/saml`) — `Destination`, `Recipient`, and the auto-submit form action; NameID `NameQualifier`; the `/launch` and `/chat` redirect target | + +Rules: + +- **The entity ID is a stable logical identifier.** Slack stores it at setup time and matches every assertion's `Issuer` against it, so it must not change when the site moves hosts. In particular it does **not** follow `CFP_SITE_HOST`: production runs at `next.codeforphilly.org` before cutover and `codeforphilly.org` after, and the entity ID is `https://codeforphilly.org/api/saml/slack/metadata` throughout. It is a URI by convention only — nothing fetches it. Change `SAML_ENTITY_ID` only when deliberately re-registering the IdP with Slack. +- **The metadata `entityID` and the assertion `Issuer` are the same value**, read from the same setting. A metadata document that advertises one entity ID while assertions carry another is a bug. +- **Endpoint URLs follow the host actually serving the API.** `SingleSignOnService/@Location` is built from `CFP_SITE_HOST`, so the metadata always points Slack at a URL that resolves to this deployment. When `CFP_SITE_HOST` flips at cutover the metadata's endpoint URLs change and Slack's IdP config should be refreshed from the metadata URL — but the entity ID (and therefore the trust relationship) is untouched. +- **Nothing IdP-side is ever built from `SLACK_TEAM_HOST` other than the Slack-side values in the table.** `SLACK_TEAM_HOST` is Slack's host; it never appears in our entity ID or our endpoint URLs. + ## GET /api/saml/slack/launch **IdP-initiated sign-in** — the member is on our site and wants to sign into Slack. @@ -132,12 +149,13 @@ There's no v1 plan to vary this — keeping the hook just preserves the legacy e ## Cert + key rotation -The cert + private key are env-injected: +The cert + private key are env-injected, alongside the IdP identity: | Env var | Purpose | | ------- | ------- | | `SAML_PRIVATE_KEY` | PEM-encoded RSA private key for signing assertions | | `SAML_CERTIFICATE` | PEM-encoded X.509 cert (the public half) | +| `SAML_ENTITY_ID` | Optional. The IdP entity ID / assertion `Issuer` (default `https://codeforphilly.org/api/saml/slack/metadata`). Not a secret. See [IdP identity and hosts](#idp-identity-and-hosts). | Slack's admin panel holds the matching public cert. Rotation is a coordinated procedure (per the legacy `docs/operations/update-saml2-certificate.md`): diff --git a/specs/architecture.md b/specs/architecture.md index 010a0c8..1dbff64 100644 --- a/specs/architecture.md +++ b/specs/architecture.md @@ -177,11 +177,12 @@ Runtime configuration (sealed-secrets in our cluster): | `GITHUB_OAUTH_CLIENT_ID` / `GITHUB_OAUTH_CLIENT_SECRET` | GitHub OAuth app credentials — see [api/auth.md](api/auth.md) | | `CFP_JWT_SIGNING_KEY` | HS256 key for session JWTs | | `SAML_PRIVATE_KEY` / `SAML_CERTIFICATE` | Slack SAML IdP cert chain — see [api/saml.md](api/saml.md) | -| `SLACK_TEAM_HOST` | Slack workspace host (default `codeforphilly.slack.com`). Used by the `/chat` redirect ([api/chat](screens/chat.md)) and the SAML SP entity binding. | +| `SAML_ENTITY_ID` | Optional. Stable SAML IdP entity ID / assertion `Issuer` (default `https://codeforphilly.org/api/saml/slack/metadata`). Deliberately independent of `CFP_SITE_HOST` so cutover doesn't change the identifier Slack has on file — see [api/saml.md#idp-identity-and-hosts](api/saml.md#idp-identity-and-hosts). | +| `SLACK_TEAM_HOST` | Slack workspace host (default `codeforphilly.slack.com`). Used by the `/chat` redirect ([api/chat](screens/chat.md)) and the SAML SP entity binding (ACS URL, NameQualifier). Never used for our own IdP entity ID or endpoint URLs. | | `POSTMARK_SERVER_TOKEN` | Optional. When set, mutates the notifier from the no-op `LoggingNotifier` to the live `EmailNotifier` (Postmark transport). | | `POSTMARK_MESSAGE_STREAM` | Optional. Postmark message stream for outbound mail (default `outbound`). | | `CFP_NOTIFICATION_FROM` | Required when `POSTMARK_SERVER_TOKEN` is set; the `From:` address on outbound mail. | -| `CFP_SITE_HOST` | Public site host (e.g., `codeforphilly.org`) — used by notifiers to build canonical URLs in email bodies. | +| `CFP_SITE_HOST` | Public site host (e.g., `codeforphilly.org`) — used by notifiers to build canonical URLs in email bodies, by the markdown renderer's external-link transform, and by the SAML IdP metadata's `SingleSignOnService` endpoint URLs ([api/saml.md](api/saml.md#idp-identity-and-hosts)). | | `CFP_DATA_RELOAD_SECRET` | Bearer token gating `POST /api/_internal/reload-data` — the hot-reload webhook. Optional in dev; required in prod. | On pod start the entrypoint: @@ -213,7 +214,7 @@ The importer pulls only public fields. Private data (emails, password hashes, ne Three levels, matching laddr's `Person.AccountLevel`: | Level | Who | Can | -|-------|-----|-----| +| ------- | ----- | ----- | | **Anonymous** | Not signed in | Browse public content; view profiles, projects, updates, buzz | | **User** | Signed-in member | Update own profile; post project updates on projects they're a member of; post buzz to any project; create new projects (auto-becomes maintainer) | | **Staff** | Trusted contributor | Edit any project; manage project members; promote help-wanted to highlighted; moderate content | From 4536eaa64785f8f5c92e4a62f7c75c962e7d1676 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Tue, 8 Sep 2026 20:02:52 -0400 Subject: [PATCH 2/3] fix(api): build SAML entityID and SSO endpoints from our own settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getSamlContext derived the IdP entity ID and both SingleSignOnService Locations from SLACK_TEAM_HOST, so the live metadata advertised https://codeforphilly.slack.com/api/saml/slack/... — Slack's host, not ours. Per specs/api/saml.md#idp-identity-and-hosts: - entityId (metadata entityID + assertion Issuer, one value flowing through SlackSamlEntities.entityId → issuerEntityId) now comes from the new SAML_ENTITY_ID env var, defaulting to https://codeforphilly.org/api/saml/slack/metadata. It is a stable logical identifier and intentionally does not follow CFP_SITE_HOST, so the next.codeforphilly.org → codeforphilly.org flip at cutover leaves the issuer Slack stored at setup untouched. - ssoLoginPostUrl / ssoLoginRedirectUrl are built on CFP_SITE_HOST so the metadata points Slack at the host actually serving the API. - SLACK_TEAM_HOST keeps only its Slack-side roles (ACS URL, NameID NameQualifier, launch redirect). Tests assert the default entityID, the CFP_SITE_HOST-driven Locations, Issuer == entityID on both Response and Assertion, that CFP_SITE_HOST=next.example.org moves the Locations without moving the entityID, and that an explicit SAML_ENTITY_ID flows to both. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr --- apps/api/src/env.ts | 24 +++++++- apps/api/src/routes/saml.ts | 19 +++--- apps/api/src/saml/config.ts | 16 +++-- apps/api/tests/saml.test.ts | 115 ++++++++++++++++++++++++++++++++++-- 4 files changed, 155 insertions(+), 19 deletions(-) diff --git a/apps/api/src/env.ts b/apps/api/src/env.ts index c460c84..6c2908f 100644 --- a/apps/api/src/env.ts +++ b/apps/api/src/env.ts @@ -6,6 +6,12 @@ */ import { z } from 'zod'; +/** + * Default SAML IdP entity ID. Stable across hosts — see the SAML_ENTITY_ID + * field below and specs/api/saml.md#idp-identity-and-hosts. + */ +export const SAML_ENTITY_ID_DEFAULT = 'https://codeforphilly.org/api/saml/slack/metadata'; + export const EnvSchema = z.object({ /** TCP port the Fastify server listens on. */ PORT: z.coerce.number().default(3001), @@ -49,8 +55,17 @@ export const EnvSchema = z.object({ /** SAML IdP certificate (PEM) for the Slack SAML integration. */ SAML_CERTIFICATE: z.string().optional(), /** - * Slack workspace host. Used as the SAML `NameQualifier` per - * specs/api/saml.md and shared with the `/chat` redirect handler. + * SAML IdP entity ID — the metadata `entityID` and the `` on every + * assertion. A stable logical identifier Slack stores at setup time, so it + * deliberately does NOT follow CFP_SITE_HOST: the pre-cutover + * `next.codeforphilly.org` deploy and the post-cutover `codeforphilly.org` + * deploy present the same issuer. Per specs/api/saml.md#idp-identity-and-hosts. + */ + SAML_ENTITY_ID: z.url().default(SAML_ENTITY_ID_DEFAULT), + /** + * Slack workspace host. Used for the SAML ACS URL and `NameQualifier` per + * specs/api/saml.md and shared with the `/chat` redirect handler. Never + * used for our own IdP entity ID or endpoint URLs. */ SLACK_TEAM_HOST: z.string().default('codeforphilly.slack.com'), /** @@ -64,7 +79,9 @@ export const EnvSchema = z.object({ * `next-v2.codeforphilly.org` in sandbox). Used by the server-side * markdown renderer to distinguish internal from external links — anchors * with a host different from this one get `target="_blank" rel="noopener - * nofollow"`. Per specs/behaviors/markdown-rendering.md. + * nofollow"`. Per specs/behaviors/markdown-rendering.md. Also the host the + * SAML IdP metadata advertises for its SSO endpoint Locations (per + * specs/api/saml.md#idp-identity-and-hosts). */ CFP_SITE_HOST: z.string().default('codeforphilly.org'), /** @@ -121,6 +138,7 @@ export const envJsonSchema = { CFP_JWT_SIGNING_KEY: { type: 'string', minLength: 1 }, SAML_PRIVATE_KEY: { type: 'string' }, SAML_CERTIFICATE: { type: 'string' }, + SAML_ENTITY_ID: { type: 'string', default: SAML_ENTITY_ID_DEFAULT }, SLACK_TEAM_HOST: { type: 'string', default: 'codeforphilly.slack.com' }, CFP_WEB_DIST_PATH: { type: 'string' }, CFP_SITE_HOST: { type: 'string', default: 'codeforphilly.org' }, diff --git a/apps/api/src/routes/saml.ts b/apps/api/src/routes/saml.ts index d07c7d0..e565398 100644 --- a/apps/api/src/routes/saml.ts +++ b/apps/api/src/routes/saml.ts @@ -128,18 +128,21 @@ function getSamlContext(fastify: FastifyInstance): SamlContext { throw new ApiValidationError('SAML IdP is not configured'); } - const base = `https://${cfg.SLACK_TEAM_HOST}`.replace('https://', ''); - const issuerHost = base; - // Fallback to the team host for the metadata entity ID if we can't see - // the inbound request origin. Per spec the entityID is our own URL — - // we'll prefer the request origin when building responses. + // Three distinct sources, per specs/api/saml.md#idp-identity-and-hosts: + // - entityId (metadata entityID + assertion Issuer) is the stable + // SAML_ENTITY_ID — it must NOT track the serving host, because Slack + // stored it at setup and the host flips at cutover; + // - the SSO endpoint Locations follow CFP_SITE_HOST so the metadata + // points Slack at whatever host this deployment answers on; + // - SLACK_TEAM_HOST is Slack's side only (ACS URL, NameQualifier). + const ssoUrl = `https://${cfg.CFP_SITE_HOST}/api/saml/slack/sso`; const ctx: SamlContext = { entities: buildSlackSamlEntities({ privateKey: cfg.SAML_PRIVATE_KEY, certificate: cfg.SAML_CERTIFICATE, - entityId: `https://${issuerHost}/api/saml/slack/metadata`, - ssoLoginPostUrl: `https://${issuerHost}/api/saml/slack/sso`, - ssoLoginRedirectUrl: `https://${issuerHost}/api/saml/slack/sso`, + entityId: cfg.SAML_ENTITY_ID, + ssoLoginPostUrl: ssoUrl, + ssoLoginRedirectUrl: ssoUrl, slackTeamHost: cfg.SLACK_TEAM_HOST, }), }; diff --git a/apps/api/src/saml/config.ts b/apps/api/src/saml/config.ts index caa67a7..7038188 100644 --- a/apps/api/src/saml/config.ts +++ b/apps/api/src/saml/config.ts @@ -36,13 +36,21 @@ export interface SamlIdpSettings { readonly privateKey: string; /** PEM-encoded X.509 certificate (the public half). */ readonly certificate: string; - /** The IdP entity ID — also the metadata URL. */ + /** + * The IdP entity ID — becomes the metadata `entityID` AND the `` + * on every Response/Assertion (via `SlackSamlEntities.entityId` → + * `issuerEntityId`). A stable logical identifier (`SAML_ENTITY_ID`), not + * necessarily a URL that resolves on the serving host. + */ readonly entityId: string; - /** The IdP SSO POST binding location (the /launch endpoint). */ + /** The IdP SSO POST binding location — `https:///api/saml/slack/sso`. */ readonly ssoLoginPostUrl: string; - /** The IdP SSO Redirect binding location. */ + /** The IdP SSO Redirect binding location — same URL as the POST binding. */ readonly ssoLoginRedirectUrl: string; - /** Slack team host (e.g. `codeforphilly.slack.com`). */ + /** + * Slack team host (e.g. `codeforphilly.slack.com`). Slack-side only: the + * ACS URL and the NameID `NameQualifier`. Never part of our own identity. + */ readonly slackTeamHost: string; } diff --git a/apps/api/tests/saml.test.ts b/apps/api/tests/saml.test.ts index 1352ca2..c2ce141 100644 --- a/apps/api/tests/saml.test.ts +++ b/apps/api/tests/saml.test.ts @@ -26,6 +26,36 @@ import { getSamlTestKeyPair, type SamlTestKeyPair } from './helpers/saml-cert.js const JWT_KEY = 'test-jwt-signing-key-at-least-32-chars!!'; const SLACK_TEAM_HOST = 'codeforphilly.slack.com'; +/** Default SAML_ENTITY_ID per specs/api/saml.md#idp-identity-and-hosts. */ +const DEFAULT_ENTITY_ID = 'https://codeforphilly.org/api/saml/slack/metadata'; +/** Default CFP_SITE_HOST — the SSO endpoint Locations are built on it. */ +const DEFAULT_SITE_HOST = 'codeforphilly.org'; + +const MD_NS = 'urn:oasis:names:tc:SAML:2.0:metadata'; +const ASSERTION_NS = 'urn:oasis:names:tc:SAML:2.0:assertion'; + +function ssoLocations(metadataXml: string): { entityId: string | null; locations: string[] } { + const doc = new DOMParser().parseFromString(metadataXml, 'application/xml'); + const root = doc.documentElement; + const locations = Array.from(root?.getElementsByTagNameNS(MD_NS, 'SingleSignOnService') ?? []) + .map((el) => el.getAttribute('Location')) + .filter((v): v is string => typeof v === 'string'); + return { entityId: root?.getAttribute('entityID') ?? null, locations }; +} + +/** Every `` text in a decoded SAMLResponse (Response + Assertion). */ +function issuers(responseXml: string): string[] { + const doc = new DOMParser().parseFromString(responseXml, 'application/xml'); + return Array.from(doc.documentElement?.getElementsByTagNameNS(ASSERTION_NS, 'Issuer') ?? []).map( + (el) => el.textContent ?? '', + ); +} + +function decodeSamlResponse(html: string): string { + const match = /name="SAMLResponse" value="([^"]+)"/.exec(html); + expect(match).not.toBeNull(); + return Buffer.from(match![1]!, 'base64').toString('utf8'); +} async function seedPerson( repoDir: string, @@ -129,10 +159,17 @@ describe('SAML IdP — Slack', () => { const root = doc.documentElement; expect(root?.localName).toBe('EntityDescriptor'); - // entityID present - expect(root?.getAttribute('entityID')).toBe( - `https://${SLACK_TEAM_HOST}/api/saml/slack/metadata`, - ); + // entityID is the stable SAML_ENTITY_ID default — NOT built on + // SLACK_TEAM_HOST (Slack's host) and NOT on the serving host. + expect(root?.getAttribute('entityID')).toBe(DEFAULT_ENTITY_ID); + + // Both SSO bindings point at our own site host. + const { locations } = ssoLocations(res.body); + expect(locations).toHaveLength(2); + for (const loc of locations) { + expect(loc).toBe(`https://${DEFAULT_SITE_HOST}/api/saml/slack/sso`); + } + expect(res.body).not.toContain(`https://${SLACK_TEAM_HOST}/api/saml`); // IDPSSODescriptor + at least one SingleSignOnService and an X509Certificate. const idpDescriptors = root?.getElementsByTagNameNS( @@ -190,6 +227,10 @@ describe('SAML IdP — Slack', () => { const root = doc.documentElement; expect(root?.localName).toBe('Response'); + // Issuer on both the Response and the Assertion is the entity ID — the + // same value the metadata advertises as entityID. + expect(issuers(xml)).toEqual([DEFAULT_ENTITY_ID, DEFAULT_ENTITY_ID]); + // NameID is the slackSamlNameId, format persistent const nameIdEl = root?.getElementsByTagNameNS( 'urn:oasis:names:tc:SAML:2.0:assertion', @@ -319,6 +360,72 @@ describe('SAML IdP — Slack', () => { }); }); +describe('SAML IdP — entity ID vs. site host', () => { + let dataRepo: { path: string; cleanup: () => Promise }; + let privateStore: { path: string; cleanup: () => Promise }; + let keyPair: SamlTestKeyPair; + const personId = '01951a3c-0000-7000-8000-000000000002'; + const slug = 'sam'; + + beforeAll(async () => { + keyPair = await getSamlTestKeyPair(); + dataRepo = await createFullDataRepo(); + privateStore = await createPrivateStorageDir(); + await seedPerson(dataRepo.path, { id: personId, slug, slackSamlNameId: slug }); + await seedPrivateProfile(privateStore.path, { personId, email: 'sam@example.com' }); + }); + + afterAll(async () => { + await dataRepo.cleanup(); + await privateStore.cleanup(); + }); + + it('CFP_SITE_HOST moves the SSO Locations but leaves entityID alone', async () => { + const app = await buildTestApp(dataRepo.path, privateStore.path, keyPair, { + CFP_SITE_HOST: 'next.example.org', + }); + try { + const res = await app.inject({ method: 'GET', url: '/api/saml/slack/metadata' }); + expect(res.statusCode).toBe(200); + const { entityId, locations } = ssoLocations(res.body); + expect(locations).toHaveLength(2); + for (const loc of locations) { + expect(loc).toBe('https://next.example.org/api/saml/slack/sso'); + } + // The pre-cutover host does not leak into the identifier Slack stores. + expect(entityId).toBe(DEFAULT_ENTITY_ID); + } finally { + await app.close(); + } + }); + + it('SAML_ENTITY_ID overrides both the metadata entityID and the assertion Issuer', async () => { + const entityId = 'https://idp.example.org/saml/slack'; + const app = await buildTestApp(dataRepo.path, privateStore.path, keyPair, { + SAML_ENTITY_ID: entityId, + CFP_SITE_HOST: 'next.example.org', + }); + try { + const meta = await app.inject({ method: 'GET', url: '/api/saml/slack/metadata' }); + expect(meta.statusCode).toBe(200); + expect(ssoLocations(meta.body).entityId).toBe(entityId); + + const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); + const launch = await app.inject({ + method: 'GET', + url: '/api/saml/slack/launch', + cookies: { cfp_session: accessToken }, + }); + expect(launch.statusCode).toBe(200); + expect(issuers(decodeSamlResponse(launch.body))).toEqual([entityId, entityId]); + // Slack-side values still come from SLACK_TEAM_HOST. + expect(launch.body).toContain(`action="https://${SLACK_TEAM_HOST}/sso/saml"`); + } finally { + await app.close(); + } + }); +}); + describe('SAML IdP — without configured cert/key', () => { let dataRepo: { path: string; cleanup: () => Promise }; let privateStore: { path: string; cleanup: () => Promise }; From b8a3fe1e444041d9317b2761016a10b8fd3f674d Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Tue, 8 Sep 2026 20:03:23 -0400 Subject: [PATCH 3/3] chore(plans): mark saml-self-host done (PR #161) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01RdRwHvDupRLV8GuJpYKzEr --- plans/saml-self-host.md | 41 ++++++++++++++++++++++++++++++++--------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/plans/saml-self-host.md b/plans/saml-self-host.md index 822dbbb..bfb804b 100644 --- a/plans/saml-self-host.md +++ b/plans/saml-self-host.md @@ -1,10 +1,11 @@ --- -status: in-progress +status: done depends: [saml-idp] specs: - specs/api/saml.md - specs/architecture.md issues: [] +pr: 161 --- # Plan: SAML IdP identity and endpoint hosts @@ -75,18 +76,18 @@ any change to NameID or the attribute set. ## Validation -- [ ] `GET /api/saml/slack/metadata` `entityID` is +- [x] `GET /api/saml/slack/metadata` `entityID` is `https://codeforphilly.org/api/saml/slack/metadata` with no `SAML_ENTITY_ID` set, regardless of `SLACK_TEAM_HOST` / `CFP_SITE_HOST`. -- [ ] Both `SingleSignOnService/@Location` values are +- [x] Both `SingleSignOnService/@Location` values are `https:///api/saml/slack/sso`; with `CFP_SITE_HOST=next.example.org` they use that host. -- [ ] `` on the Response and the Assertion equal the metadata +- [x] `` on the Response and the Assertion equal the metadata `entityID`, including when `SAML_ENTITY_ID` is overridden. -- [ ] ACS URL, form action, `NameQualifier`, and `/launch` redirect still use +- [x] ACS URL, form action, `NameQualifier`, and `/launch` redirect still use `SLACK_TEAM_HOST` (existing tests keep passing). -- [ ] `SLACK_TEAM_HOST` no longer appears in any IdP-side URL (grep the route). -- [ ] `npm run type-check && npm run lint && npm test` clean. +- [x] `SLACK_TEAM_HOST` no longer appears in any IdP-side URL (grep the route). +- [x] `npm run type-check && npm run lint && npm test` clean. ## Risks / unknowns @@ -103,8 +104,30 @@ any change to NameID or the attribute set. ## Notes -(Populated at closeout.) +- **`config.ts` needed no logic change.** `SamlIdpSettings.entityId` already + fed both samlify's `IdentityProvider({ entityID })` (→ metadata) and + `SlackSamlEntities.entityId`, which the route passes as `issuerEntityId` + into `buildResponseSubstitutions` (→ `{Issuer}` on Response + Assertion). + The bug was entirely in `getSamlContext`'s choice of host. Only doc + comments changed there. +- **Entity ID is a URI by convention, not a fetchable URL.** Nothing (Slack + included) dereferences it, which is why it can stay on `codeforphilly.org` + while the deployment answers on `next.codeforphilly.org`. `SAML_ENTITY_ID` + is validated as a URL (`z.url()`) only to catch typos. +- **`SLACK_TEAM_HOST` was never in the deploy env table** — it rode on its + default. Added a row so operators see it next to `SAML_ENTITY_ID` and + don't confuse the two hosts again. +- **Web-suite timeout flake.** `apps/web/tests/ProjectEdit.test.tsx` hit its + 5s limit (5252ms) once under full-suite load and passed in isolation. + Unrelated to this change; noted in case it recurs. ## Follow-ups -(Populated at closeout.) +- Tracked as: operator action before deploying to `next.codeforphilly.org` — + if Slack's SAML config was seeded from the old metadata it holds the wrong + issuer (`https://codeforphilly.slack.com/...`) and will reject assertions + until re-synced from the metadata URL. Same metadata refresh (endpoint + URLs only) is needed again at cutover when `CFP_SITE_HOST` flips; see + `docs/operations/cutover.md` owners. +- Tracked as: boot-time `slackSamlNameId` invariant scan — carried forward + unchanged from [`saml-idp`](saml-idp.md); not touched here.