From b60d77aa01f5ccfbb9a855b0150d07622d34719c Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 10 Sep 2026 02:13:21 -0400 Subject: [PATCH 1/3] docs(specs): require AuthnStatement and accept Redirect-binding SSO The SAML assertion spec described Subject, Conditions and the attribute set but never an AuthnStatement, and the implementation followed it. The Web Browser SSO profile requires at least one, and Slack's SP config sends a RequestedAuthnContext, so an assertion without one is the most likely rejection when we test against the real workspace. Spec the statement: AuthnInstant = assertion issue time, a fresh SessionIndex, and a fixed AuthnContextClassRef of ...:ac:classes:Password. That class matches what the legacy Emergence SAML2 connector emitted (setAuthnContext(AC_PASSWORD)) against this same workspace, which is worth more than the marginally more precise PasswordProtectedTransport. Slack's live "Test configuration" also failed with a 404: it sends the AuthnRequest over HTTP-Redirect (GET with a DEFLATEd SAMLRequest in the query), and although our metadata advertises that binding at the /sso Location, the spec only defined POST. Add GET to the endpoints table and describe both bindings as one flow that differs only in transport. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR --- specs/api/saml.md | 46 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/specs/api/saml.md b/specs/api/saml.md index f99b2f0..05d53e1 100644 --- a/specs/api/saml.md +++ b/specs/api/saml.md @@ -10,7 +10,8 @@ GitHub OAuth is how a member proves identity to the new site. SAML is how the si | ------ | ---- | ---- | ------- | | `GET` | `/api/saml/slack/metadata` | public | IdP metadata XML for Slack to consume | | `GET` | `/api/saml/slack/launch` | user | IdP-initiated SSO — site → Slack | -| `POST` | `/api/saml/slack/sso` | user | SP-initiated SSO callback — handles AuthnRequest from Slack | +| `GET` | `/api/saml/slack/sso` | user | SP-initiated SSO callback — AuthnRequest from Slack via the HTTP-Redirect binding | +| `POST` | `/api/saml/slack/sso` | user | SP-initiated SSO callback — AuthnRequest from Slack via the HTTP-POST binding | For the existing `/chat` redirect that Slack-launches members into channels, see [screens/chat.md](../screens/chat.md). The SAML endpoints live under `/api/saml/slack/*` because the v1 design leaves room for additional SAML SP integrations later. @@ -43,6 +44,25 @@ The attribute values come from: - `first_name` → `Person.firstName` - `last_name` → `Person.lastName` +### Authentication statement + +Every assertion carries exactly one `` — the Web Browser SSO profile (saml-profiles §4.1.4.2) requires at least one, and an assertion without it is a valid rejection reason for any SP. It sits between `` and `` (schema order: Subject, Conditions, then statements). The legacy connector emitted one via simplesamlphp's `Assertion` (`setSessionIndex(generateId())` + `setAuthnContext(SAML2_Constants::AC_PASSWORD)`); v1 preserves that shape: + +```text +AuthnStatement: + AuthnInstant the moment this assertion was issued + SessionIndex one per assertion; not the assertion ID + AuthnContext/ + AuthnContextClassRef urn:oasis:names:tc:SAML:2.0:ac:classes:Password fixed — see below +``` + +Rules: + +- **`AuthnInstant` is the assertion's issue time**, not the time the member's underlying session was established. We don't track the original sign-in instant in the JWT, and re-asserting "now" is what the legacy connector did. +- **`SessionIndex` is a fresh opaque identifier per assertion.** Slack never sends us a LogoutRequest, so nothing correlates on it; it exists to satisfy the profile. Use a fresh id rather than reusing the assertion ID so the two values stay independently meaningful. +- **The `AuthnContextClassRef` is the fixed value above.** It is *not* echoed back from the AuthnRequest's `RequestedAuthnContext` — a member proves identity to us via GitHub OAuth or the legacy password path, and we describe that once, the same way for every SP-initiated and IdP-initiated response. Slack's default `RequestedAuthnContext` is `PasswordProtectedTransport`; the legacy connector asserted `Password` against that same workspace for years without rejection, and matching the value existing Slack accounts were established under is worth more than the marginally more precise class. +- **`SessionNotOnOrAfter` is omitted.** The assertion's `Conditions/@NotOnOrAfter` already bounds the assertion; we make no claim about IdP-session lifetime. + ## GET /api/saml/slack/metadata Returns the IdP's SAML metadata XML, signed with the IdP cert. Slack consumes this once during admin setup; we generally don't re-fetch. @@ -111,30 +131,42 @@ The destination URL inside the Response includes the `redir` so Slack's POST end - `400 validation_failed` — bad `channel` format - `500 internal_error` with `error.code = "saml_signing_failed"` — IdP cert/key misconfiguration -## POST /api/saml/slack/sso +## GET | POST /api/saml/slack/sso **SP-initiated sign-in** — Slack received a request from a member who wants to sign in, sent us a SAML AuthnRequest. We complete authentication and return a SAML Response. -### Request body +The metadata advertises this one Location under both `SingleSignOnService` bindings, so the endpoint accepts the AuthnRequest either way. Slack uses **HTTP-Redirect** (`GET`) when a member starts sign-in from Slack and when an admin runs "Test configuration"; `POST` is accepted for the HTTP-POST binding. The two differ only in how `SAMLRequest` is transported; everything after decoding is one flow. + +### Request — HTTP-Redirect binding (`GET`) + +Query string (saml-bindings §3.4.4.1, `DEFLATE` encoding): + +| Param | Required | Notes | +| ----- | -------- | ----- | +| `SAMLRequest` | yes | raw-DEFLATEd, then base64-encoded, then URL-encoded SAML AuthnRequest XML | +| `RelayState` | no | opaque value Slack wants us to echo back | +| `SigAlg`, `Signature` | no | detached signature — ignored unless request signing is enabled (it isn't for Slack) | + +### Request — HTTP-POST binding (`POST`) `application/x-www-form-urlencoded`: | Field | Required | Notes | | ----- | -------- | ----- | -| `SAMLRequest` | yes | base64-encoded SAML AuthnRequest XML | +| `SAMLRequest` | yes | base64-encoded SAML AuthnRequest XML (no DEFLATE) | | `RelayState` | no | opaque value Slack wants us to echo back | ### Behavior -1. Decode + parse the AuthnRequest. Validate signature if Slack signs requests (configurable; usually no for Slack). +1. Decode + parse the AuthnRequest (inflate first for the Redirect binding). Validate signature if Slack signs requests (configurable; usually no for Slack). 2. Require a signed-in session. If not → store the AuthnRequest in a short-lived signed cookie, redirect to `/login?return=/api/saml/slack/sso?resume=1`. After login the user comes back here and the AuthnRequest replays from the cookie. 3. Resolve the AuthnRequest's `AssertionConsumerServiceURL` against Slack's documented ACS endpoint(s) — only Slack's ACS is accepted. 4. Build + sign a SAML Response as in `/launch`. -5. POST back to Slack's ACS via the auto-submitting form, including `RelayState`. +5. POST back to Slack's ACS via the auto-submitting form, including `RelayState`. The Response always goes back over HTTP-POST regardless of which binding carried the request — Slack's ACS only accepts POST. ### Errors -- `400 validation_failed` with code `saml_request_invalid` — malformed AuthnRequest or unrecognized ACS URL +- `400 validation_failed` with code `saml_request_invalid` — malformed AuthnRequest (including a Redirect-binding payload that fails to inflate) or unrecognized ACS URL - `401 unauthenticated` — no session (with resume-cookie flow as above) - `403 forbidden` with `error.code = "saml_not_permitted"` From 3d6bca2a452f3f92756063b196e920764275a8ba Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 10 Sep 2026 02:28:03 -0400 Subject: [PATCH 2/3] fix(api): emit AuthnStatement and accept Redirect-binding AuthnRequests Two gaps found while preparing to test the IdP against the live Slack workspace, both in the SP-initiated path. The assertion carried Subject, Conditions and AttributeStatement but no AuthnStatement, which the Web Browser SSO profile requires and which Slack's RequestedAuthnContext gives it a reason to check. Add one to the login response template between Conditions and AttributeStatement, with AuthnInstant = the assertion issue time, a fresh SessionIndex from the same id source as the Response/Assertion IDs, and the fixed class urn:oasis:names:tc:SAML:2.0:ac:classes:Password. That class is what the legacy Emergence SAML2 connector asserted against this workspace (setAuthnContext(AC_PASSWORD)), so existing Slack accounts see the same context they were established under. The placeholders ride the existing customTagReplacement path; samlify signs after that callback returns, so the statement lands inside the signed subtree. Slack's "Test configuration" then 404ed: Slack sends the AuthnRequest over HTTP-Redirect (GET with a DEFLATEd SAMLRequest in the query), and while the metadata advertised that binding at /sso, only POST was registered. Add GET /api/saml/slack/sso. Rather than call samlify's 'redirect' parser and teach the resume cookie about bindings, inflate at the edge back to the plain-base64 form the POST binding carries and run both through one handleSpInitiatedSso; samlify's redirect flow is that same inflate followed by the same parser, so nothing is lost and the cookie's samlRequest claim keeps a single shape. Fold spaces back to '+' in the query value, since a sender that leaves base64 '+' unescaped has it URL-decoded to a space. Tests cover the AuthnStatement (position, ClassRef, AuthnInstant <= IssueInstant, SessionIndex distinct from the assertion ID), verify the signature cryptographically against the metadata endpoint's cert and show that tampering with the ClassRef breaks it, and exercise the Redirect binding for signed-in, anonymous (through /sso/resume), bad ACS and non-DEFLATEd payloads. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR --- apps/api/src/routes/saml.ts | 299 +++++++++++++++++++++++------------- apps/api/src/saml/config.ts | 34 +++- apps/api/tests/saml.test.ts | 201 ++++++++++++++++++++++++ 3 files changed, 426 insertions(+), 108 deletions(-) diff --git a/apps/api/src/routes/saml.ts b/apps/api/src/routes/saml.ts index e565398..8331cb1 100644 --- a/apps/api/src/routes/saml.ts +++ b/apps/api/src/routes/saml.ts @@ -4,15 +4,17 @@ * Implements specs/api/saml.md: * GET /api/saml/slack/metadata * GET /api/saml/slack/launch - * POST /api/saml/slack/sso + * GET /api/saml/slack/sso (SP-initiated, HTTP-Redirect binding) + * POST /api/saml/slack/sso (SP-initiated, HTTP-POST binding) * GET /api/saml/slack/sso/resume (sign-in continuation) * * Cert + key load lazily — endpoints return 500 saml_signing_failed if the * environment is missing them. Routes are mounted regardless so the * metadata URL is always discoverable. */ -import type { FastifyInstance, FastifyRequest } from 'fastify'; +import type { FastifyInstance, FastifyReply, FastifyRequest } from 'fastify'; import { randomBytes } from 'node:crypto'; +import { inflateRawSync } from 'node:zlib'; import { ForbiddenError, UnauthenticatedError, ApiValidationError } from '../lib/errors.js'; import { errorResponse } from '../lib/response.js'; import { @@ -186,6 +188,11 @@ function buildAssertionUser(opts: { * - fill in NameQualifier / SPNameQualifier (samlify's default skips both) * - substitute the per-attribute placeholder tags built by samlify's * `attributeStatementBuilder` (e.g. `{attrEmail}`, `{attrUsername}`). + * - fill the AuthnStatement (AuthnInstant / SessionIndex / ClassRef) our + * template carries — samlify's default path blanks its own slot. + * + * samlify signs the assertion *after* this callback returns, so everything + * substituted here is covered by the signature. */ function buildCustomTagReplacement(opts: { readonly user: SlackAssertionUser; @@ -197,11 +204,15 @@ function buildCustomTagReplacement(opts: { return (template) => { const id = opts.generateID(); const assertionId = opts.generateID(); + // Fresh per assertion and distinct from the assertion ID — mirrors the + // legacy connector's `setSessionIndex(generateId())`. + const sessionIndex = opts.generateID(); const subs = buildResponseSubstitutions({ user: opts.user, slackTeamHost: opts.slackTeamHost, issuerEntityId: opts.issuerEntityId, inResponseTo: opts.inResponseTo, + sessionIndex, }); const fullSubs: Record = { ID: id, @@ -249,6 +260,149 @@ async function loadPersonAndProfile( return { person, profile }; } +/** + * Lift an HTTP-Redirect-binding `SAMLRequest` (saml-bindings §3.4.4.1: XML → + * raw DEFLATE → base64 → URL-encode) back to the plain-base64 form the + * HTTP-POST binding carries, so both bindings share one parse + resume path + * and the resume cookie's `samlRequest` claim keeps a single shape. + * + * samlify's own redirect flow is exactly this inflate followed by the same + * parser; doing the inflate here rather than calling the 'redirect' parser + * is what lets the cookie stay binding-agnostic. + * + * Fastify has already URL-decoded the query. A sender that leaves base64 `+` + * unescaped would have it decoded to a space, so fold spaces back — base64 + * never legitimately contains one. + */ +function inflateRedirectBindingRequest(deflatedB64: string): string { + const normalised = deflatedB64.replace(/ /g, '+'); + if (!normalised) { + throw new ApiValidationError('SAMLRequest is required', { SAMLRequest: 'required' }); + } + let xml: string; + try { + xml = inflateRawSync(Buffer.from(normalised, 'base64')).toString('utf8'); + } catch { + throw new ApiValidationError('Malformed SAMLRequest', { + SAMLRequest: 'inflate failed', + }); + } + return Buffer.from(xml, 'utf8').toString('base64'); +} + +/** + * SP-initiated SSO, binding-agnostic. Takes the AuthnRequest as plain base64 + * XML (the HTTP-POST wire form — the Redirect handler inflates into it first) + * and either issues the signed Response (signed-in) or parks the request in + * the resume cookie and bounces through /login (anonymous). + */ +async function handleSpInitiatedSso( + fastify: FastifyInstance, + request: FastifyRequest, + reply: FastifyReply, + input: { readonly samlRequestB64: string; readonly relayState: string }, +): Promise { + const cfg = fastify.config; + if (!cfg.SAML_PRIVATE_KEY || !cfg.SAML_CERTIFICATE) { + return reply.code(500).send( + errorResponse( + 'saml_signing_failed', + 'SAML IdP is not configured', + (request as FastifyRequest & { traceId?: string }).traceId, + ), + ); + } + + const { samlRequestB64, relayState } = input; + if (!samlRequestB64) { + throw new ApiValidationError('SAMLRequest is required', { + SAMLRequest: 'required', + }); + } + + const { entities } = getSamlContext(fastify); + + // Parse the AuthnRequest to extract its ID + AssertionConsumerServiceURL. + let parsed: Awaited>; + try { + parsed = await entities.idp.parseLoginRequest(entities.sp, 'post', { + body: { SAMLRequest: samlRequestB64 }, + }); + } catch (err) { + fastify.log.warn({ err }, 'SAML AuthnRequest parse failed'); + throw new ApiValidationError('Malformed SAMLRequest', { + SAMLRequest: 'parse failed', + }); + } + + const extract = parsed.extract as { + request?: { id?: string; assertionConsumerServiceUrl?: string }; + }; + const acsUrl = extract.request?.assertionConsumerServiceUrl ?? entities.acsUrl; + const requestId = extract.request?.id ?? ''; + + assertAcsAllowed(acsUrl, cfg.SLACK_TEAM_HOST); + + // Anonymous → stash the AuthnRequest in the resume cookie, redirect to /login. + if (!request.session.personId) { + const resumeToken = await signSamlResume( + { + samlRequest: samlRequestB64, + relayState, + acsUrl, + requestId, + }, + cfg.CFP_JWT_SIGNING_KEY, + ); + reply.setCookie(RESUME_COOKIE, resumeToken, { + httpOnly: true, + sameSite: 'lax', + secure: isSecure(cfg.NODE_ENV), + path: '/api/saml', + maxAge: RESUME_COOKIE_TTL_SECONDS, + }); + const resumeReturn = `${originBase(request)}/api/saml/slack/sso/resume`; + return reply.redirect(`/login?return=${encodeURIComponent(resumeReturn)}`); + } + + // Signed in — build the assertion immediately. + const { person, profile } = await loadPersonAndProfile(fastify, request.session.personId); + const user = buildAssertionUser({ person, profile }); + + const customTagReplacement = buildCustomTagReplacement({ + user, + slackTeamHost: cfg.SLACK_TEAM_HOST, + issuerEntityId: entities.entityId, + inResponseTo: requestId, + generateID: () => `_${cryptoRandomId()}`, + }); + + // The Response always returns over HTTP-POST regardless of which binding + // carried the request — Slack's ACS only accepts POST. + const bindingCtx = await entities.idp.createLoginResponse( + entities.sp, + { extract: parsed.extract }, + 'post', + {}, + { relayState, customTagReplacement }, + ); + + const samlResponse = bindingCtx.context; + const actionUrl = + 'entityEndpoint' in bindingCtx && typeof bindingCtx.entityEndpoint === 'string' + ? bindingCtx.entityEndpoint + : acsUrl; + const replyRelayState = 'relayState' in bindingCtx ? bindingCtx.relayState : relayState; + + return reply.header('Content-Type', 'text/html; charset=utf-8').send( + renderPostForm({ + actionUrl, + samlResponse, + relayState: replyRelayState ?? undefined, + }), + ); +} + // --------------------------------------------------------------------------- // Routes // --------------------------------------------------------------------------- @@ -368,15 +522,48 @@ export async function samlRoutes(fastify: FastifyInstance): Promise { ); // ------------------------------------------------------------------------- - // POST /api/saml/slack/sso — SP-initiated SSO + // GET | POST /api/saml/slack/sso — SP-initiated SSO + // + // One Location, two bindings (the metadata advertises both). The handlers + // below only differ in how they lift `SAMLRequest` off the wire; everything + // from parsing onward is `handleSpInitiatedSso`. Slack itself uses the + // Redirect binding (GET) — both for member-started sign-in and the admin + // "Test configuration" button. // ------------------------------------------------------------------------- + fastify.get( + '/api/saml/slack/sso', + { + schema: { + tags: ['saml'], + summary: 'SP-initiated Slack sign-in (AuthnRequest, HTTP-Redirect binding)', + querystring: { + type: 'object', + properties: { + SAMLRequest: { type: 'string' }, + RelayState: { type: 'string' }, + SigAlg: { type: 'string' }, + Signature: { type: 'string' }, + }, + required: ['SAMLRequest'], + }, + }, + }, + async (request, reply) => { + const query = request.query as { SAMLRequest?: string; RelayState?: string }; + return handleSpInitiatedSso(fastify, request, reply, { + samlRequestB64: inflateRedirectBindingRequest(query.SAMLRequest ?? ''), + relayState: query.RelayState ?? '', + }); + }, + ); + fastify.post( '/api/saml/slack/sso', { schema: { tags: ['saml'], - summary: 'SP-initiated Slack sign-in (AuthnRequest)', + summary: 'SP-initiated Slack sign-in (AuthnRequest, HTTP-POST binding)', body: { type: 'object', properties: { @@ -388,109 +575,11 @@ export async function samlRoutes(fastify: FastifyInstance): Promise { }, }, async (request, reply) => { - const cfg = fastify.config; - if (!cfg.SAML_PRIVATE_KEY || !cfg.SAML_CERTIFICATE) { - return reply.code(500).send( - errorResponse( - 'saml_signing_failed', - 'SAML IdP is not configured', - (request as FastifyRequest & { traceId?: string }).traceId, - ), - ); - } - const body = request.body as { SAMLRequest?: string; RelayState?: string }; - const samlRequestB64 = body.SAMLRequest ?? ''; - const relayState = body.RelayState ?? ''; - if (!samlRequestB64) { - throw new ApiValidationError('SAMLRequest is required', { - SAMLRequest: 'required', - }); - } - - const { entities } = getSamlContext(fastify); - - // Parse the AuthnRequest to extract its ID + AssertionConsumerServiceURL. - let parsed: Awaited>; - try { - parsed = await entities.idp.parseLoginRequest(entities.sp, 'post', { - body: { SAMLRequest: samlRequestB64 }, - }); - } catch (err) { - fastify.log.warn({ err }, 'SAML AuthnRequest parse failed'); - throw new ApiValidationError('Malformed SAMLRequest', { - SAMLRequest: 'parse failed', - }); - } - - const extract = parsed.extract as { - request?: { id?: string; assertionConsumerServiceUrl?: string }; - }; - const acsUrl = - extract.request?.assertionConsumerServiceUrl ?? entities.acsUrl; - const requestId = extract.request?.id ?? ''; - - assertAcsAllowed(acsUrl, cfg.SLACK_TEAM_HOST); - - // Anonymous → stash the AuthnRequest in the resume cookie, redirect to /login. - if (!request.session.personId) { - const resumeToken = await signSamlResume( - { - samlRequest: samlRequestB64, - relayState, - acsUrl, - requestId, - }, - cfg.CFP_JWT_SIGNING_KEY, - ); - reply.setCookie(RESUME_COOKIE, resumeToken, { - httpOnly: true, - sameSite: 'lax', - secure: isSecure(cfg.NODE_ENV), - path: '/api/saml', - maxAge: RESUME_COOKIE_TTL_SECONDS, - }); - const resumeReturn = `${originBase(request)}/api/saml/slack/sso/resume`; - return reply.redirect(`/login?return=${encodeURIComponent(resumeReturn)}`); - } - - // Signed in — build the assertion immediately. - const { person, profile } = await loadPersonAndProfile(fastify, request.session.personId); - const user = buildAssertionUser({ person, profile }); - - const customTagReplacement = buildCustomTagReplacement({ - user, - slackTeamHost: cfg.SLACK_TEAM_HOST, - issuerEntityId: entities.entityId, - inResponseTo: requestId, - generateID: () => `_${cryptoRandomId()}`, + return handleSpInitiatedSso(fastify, request, reply, { + samlRequestB64: body.SAMLRequest ?? '', + relayState: body.RelayState ?? '', }); - - const bindingCtx = await entities.idp.createLoginResponse( - entities.sp, - { extract: parsed.extract }, - 'post', - {}, - { relayState, customTagReplacement }, - ); - - const samlResponse = bindingCtx.context; - const actionUrl = - 'entityEndpoint' in bindingCtx && typeof bindingCtx.entityEndpoint === 'string' - ? bindingCtx.entityEndpoint - : acsUrl; - const replyRelayState = - 'relayState' in bindingCtx ? bindingCtx.relayState : relayState; - - return reply - .header('Content-Type', 'text/html; charset=utf-8') - .send( - renderPostForm({ - actionUrl, - samlResponse, - relayState: replyRelayState ?? undefined, - }), - ); }, ); diff --git a/apps/api/src/saml/config.ts b/apps/api/src/saml/config.ts index 7038188..84aee48 100644 --- a/apps/api/src/saml/config.ts +++ b/apps/api/src/saml/config.ts @@ -22,6 +22,15 @@ const { IdentityProvider, ServiceProvider, Constants, SamlLib, setSchemaValidato // `@authenio/samlify-xsd-schema-validator` java dependency. const NAMEID_FORMAT_PERSISTENT = 'urn:oasis:names:tc:SAML:2.0:nameid-format:persistent'; +/** + * The one `AuthnContextClassRef` every assertion carries, per + * specs/api/saml.md#authentication-statement. Fixed — never echoed back from + * the AuthnRequest's `RequestedAuthnContext`. Matches what the legacy + * Emergence SAML2 connector asserted (`SAML2\Constants::AC_PASSWORD`) against + * this same Slack workspace. + */ +export const SLACK_AUTHN_CONTEXT_CLASS_REF = 'urn:oasis:names:tc:SAML:2.0:ac:classes:Password'; + let schemaValidatorConfigured = false; function ensureSchemaValidator(): void { if (schemaValidatorConfigured) return; @@ -79,6 +88,12 @@ export interface BuildResponseSubstitutionsOptions { readonly slackTeamHost: string; readonly issuerEntityId: string; readonly inResponseTo: string; + /** + * Opaque per-assertion `AuthnStatement/@SessionIndex`. Generated by the + * caller alongside the Response/Assertion IDs so all three share one id + * source; it must differ from the assertion ID (see spec). + */ + readonly sessionIndex: string; /** RFC3339 timestamp (current time) — passed in so tests can pin it. */ readonly nowIso?: string; } @@ -113,6 +128,14 @@ export function buildResponseSubstitutions( SPNameQualifier: 'https://slack.com', NameID: opts.user.nameId, InResponseTo: opts.inResponseTo, + // AuthnStatement (specs/api/saml.md#authentication-statement). These are + // our own placeholders — samlify's built-in template has a bare + // `{AuthnStatement}` slot that the default (non-custom) path blanks out, + // so we carry the statement literally in our template and fill its + // attributes here. AuthnInstant is the assertion issue time by design. + AuthnInstant: now, + SessionIndex: opts.sessionIndex, + AuthnContextClassRef: SLACK_AUTHN_CONTEXT_CLASS_REF, // samlify's `attributeStatementBuilder` derives placeholder names from // each attribute's `valueTag` via `'attr' + camelCase + first-upper`, so // `valueTag: 'email'` → `{attrEmail}`, `valueTag: 'firstName'` → @@ -199,10 +222,15 @@ export function buildSlackSamlEntities(settings: SamlIdpSettings): SlackSamlEnti ], loginResponseTemplate: { // samlify substitutes {AttributeStatement} from the configured attribute - // list; the rest of the template comes from the library's built-in - // response template. + // list at IdP construction; every other placeholder is filled by our + // customTagReplacement callback (see buildResponseSubstitutions). + // + // Element order inside follows the schema (saml-core + // §2.3.3): Issuer, [Signature — inserted after Issuer by samlify], + // Subject, Conditions, then statements. AuthnStatement precedes + // AttributeStatement, mirroring the legacy connector's output. context: - '{Issuer}{Issuer}{NameID}{Audience}{AttributeStatement}', + '{Issuer}{Issuer}{NameID}{Audience}{AuthnContextClassRef}{AttributeStatement}', attributes: [ { name: 'User.Email', diff --git a/apps/api/tests/saml.test.ts b/apps/api/tests/saml.test.ts index c2ce141..1fdc524 100644 --- a/apps/api/tests/saml.test.ts +++ b/apps/api/tests/saml.test.ts @@ -8,7 +8,12 @@ * SAMLResponse carrying the expected NameID + attribute set * - GET /api/saml/slack/launch?channel=phlask → relayState carries channel * - GET /api/saml/slack/launch?channel= → 422 + * - Assertion carries an AuthnStatement (fixed ClassRef, AuthnInstant <= + * IssueInstant, fresh SessionIndex) — specs/api/saml.md#authentication-statement * - POST /api/saml/slack/sso (anonymous) → resume cookie + 302 to /login + * - GET /api/saml/slack/sso (HTTP-Redirect binding, DEFLATEd SAMLRequest) + * behaves exactly as POST for signed-in / signed-out / bad payload, and + * the anonymous path resumes through /sso/resume * - GET /api/saml/slack/sso/resume (signed-in, valid cookie) → POST form * - Metadata endpoint without SAML_PRIVATE_KEY → 500 saml_signing_failed */ @@ -16,7 +21,9 @@ import { afterAll, beforeAll, describe, expect, it } from 'vitest'; import { type FastifyInstance } from 'fastify'; import { writeFile } from 'node:fs/promises'; import { join } from 'node:path'; +import { deflateRawSync } from 'node:zlib'; import { DOMParser } from '@xmldom/xmldom'; +import * as samlify from 'samlify'; import { buildApp } from '../src/app.js'; import { mintSessionFor } from '../src/auth/issue.js'; @@ -57,6 +64,34 @@ function decodeSamlResponse(html: string): string { return Buffer.from(match![1]!, 'base64').toString('utf8'); } +/** Fixed AuthnContextClassRef per specs/api/saml.md#authentication-statement. */ +const AUTHN_CONTEXT_CLASS_REF = 'urn:oasis:names:tc:SAML:2.0:ac:classes:Password'; + +/** A minimal Slack-shaped AuthnRequest targeting the configured ACS. */ +function slackAuthnRequestXml(id: string, acsHost: string = SLACK_TEAM_HOST): string { + return ` +https://slack.com`; +} + +/** + * HTTP-Redirect binding encoding (saml-bindings §3.4.4.1): raw DEFLATE → + * base64 → URL-encode. Returns the ready-to-append query string. + */ +function redirectBindingQuery(xml: string, relayState?: string): string { + const deflated = deflateRawSync(Buffer.from(xml, 'utf8')).toString('base64'); + const params = new URLSearchParams({ SAMLRequest: deflated }); + if (relayState !== undefined) params.set('RelayState', relayState); + return params.toString(); +} + +function resumeCookieValue(res: { headers: Record }): string { + const cookies = res.headers['set-cookie']; + const list = Array.isArray(cookies) ? cookies : [String(cookies ?? '')]; + const hit = list.find((c) => c.startsWith('cfp_saml_resume=')); + expect(hit).toBeDefined(); + return hit!.split(';')[0]!.slice('cfp_saml_resume='.length); +} + async function seedPerson( repoDir: string, opts: { @@ -272,6 +307,172 @@ describe('SAML IdP — Slack', () => { expect((sigs?.length ?? 0)).toBeGreaterThan(0); }); + it('assertion carries an AuthnStatement with the fixed ClassRef and a sane AuthnInstant', async () => { + const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); + const res = await app.inject({ + method: 'GET', + url: '/api/saml/slack/launch', + cookies: { cfp_session: accessToken }, + }); + expect(res.statusCode).toBe(200); + + const xml = decodeSamlResponse(res.body); + const doc = new DOMParser().parseFromString(xml, 'application/xml'); + const root = doc.documentElement!; + const assertion = root.getElementsByTagNameNS(ASSERTION_NS, 'Assertion')[0]!; + + // Exactly one AuthnStatement, placed after Conditions and before + // AttributeStatement (schema order, saml-core §2.3.3). + const authnStatements = assertion.getElementsByTagNameNS(ASSERTION_NS, 'AuthnStatement'); + expect(authnStatements.length).toBe(1); + const authn = authnStatements[0]!; + const childNames = Array.from(assertion.childNodes) + .filter((n) => n.nodeType === 1) + .map((n) => (n as Element).localName); + expect(childNames.indexOf('AuthnStatement')).toBeGreaterThan(childNames.indexOf('Conditions')); + expect(childNames.indexOf('AuthnStatement')).toBeLessThan( + childNames.indexOf('AttributeStatement'), + ); + + // Fixed ClassRef — not echoed from any RequestedAuthnContext. + const classRef = authn.getElementsByTagNameNS(ASSERTION_NS, 'AuthnContextClassRef')[0]; + expect(classRef?.textContent).toBe(AUTHN_CONTEXT_CLASS_REF); + + // AuthnInstant parses as an ISO date and is <= the assertion's IssueInstant. + const authnInstant = authn.getAttribute('AuthnInstant'); + const issueInstant = assertion.getAttribute('IssueInstant'); + expect(authnInstant).toMatch(/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(\.\d+)?Z$/); + expect(Number.isNaN(Date.parse(authnInstant!))).toBe(false); + expect(Date.parse(authnInstant!)).toBeLessThanOrEqual(Date.parse(issueInstant!)); + + // SessionIndex is present, opaque, and not the assertion ID. + const sessionIndex = authn.getAttribute('SessionIndex'); + expect(sessionIndex).toMatch(/^_[0-9a-f]+$/); + expect(sessionIndex).not.toBe(assertion.getAttribute('ID')); + + // SessionNotOnOrAfter is omitted by design. + expect(authn.hasAttribute('SessionNotOnOrAfter')).toBe(false); + + // The statement sits inside the signed subtree: the enveloped Signature + // is a child of the Assertion and its Reference points at the Assertion ID. + const sig = assertion.getElementsByTagNameNS('http://www.w3.org/2000/09/xmldsig#', 'Signature')[0]; + expect(sig?.parentNode).toBe(assertion); + const ref = sig?.getElementsByTagNameNS('http://www.w3.org/2000/09/xmldsig#', 'Reference')[0]; + expect(ref?.getAttribute('URI')).toBe(`#${assertion.getAttribute('ID')}`); + }); + + it('assertion signature verifies against the metadata cert and covers the AuthnStatement', async () => { + const meta = await app.inject({ method: 'GET', url: '/api/saml/slack/metadata' }); + expect(meta.statusCode).toBe(200); + const idpMetadata = samlify.IdPMetadata(meta.body); + + const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); + const res = await app.inject({ + method: 'GET', + url: '/api/saml/slack/launch', + cookies: { cfp_session: accessToken }, + }); + expect(res.statusCode).toBe(200); + const xml = decodeSamlResponse(res.body); + + // Signing happens after templating, so the substituted AuthnStatement is + // inside the signed subtree: the untouched Response verifies... + const [verified, signedAssertion] = samlify.SamlLib.verifySignature(xml, { + metadata: idpMetadata, + }); + expect(verified).toBe(true); + expect(signedAssertion).toContain(' { + const res = await app.inject({ + method: 'GET', + url: `/api/saml/slack/sso?${redirectBindingQuery(slackAuthnRequestXml('id-redirect-1'), 'opaque-redirect-state')}`, + }); + expect(res.statusCode).toBe(302); + expect(res.headers.location).toMatch(/^\/login\?return=/); + expect(resumeCookieValue(res)).not.toBe(''); + }); + + it('GET /api/saml/slack/sso (redirect binding, anonymous) → /sso/resume completes with InResponseTo + RelayState', async () => { + const start = await app.inject({ + method: 'GET', + url: `/api/saml/slack/sso?${redirectBindingQuery(slackAuthnRequestXml('id-redirect-2'), 'opaque-redirect-state')}`, + }); + expect(start.statusCode).toBe(302); + const resumeCookie = resumeCookieValue(start); + + const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); + const res = await app.inject({ + method: 'GET', + url: '/api/saml/slack/sso/resume', + cookies: { cfp_session: accessToken, cfp_saml_resume: resumeCookie }, + }); + expect(res.statusCode).toBe(200); + expect(res.body).toContain(`action="https://${SLACK_TEAM_HOST}/sso/saml"`); + expect(res.body).toContain('name="RelayState" value="opaque-redirect-state"'); + + const xml = decodeSamlResponse(res.body); + const doc = new DOMParser().parseFromString(xml, 'application/xml'); + expect(doc.documentElement?.getAttribute('InResponseTo')).toBe('id-redirect-2'); + expect( + doc.documentElement?.getElementsByTagNameNS(ASSERTION_NS, 'AuthnStatement').length, + ).toBe(1); + }); + + it('GET /api/saml/slack/sso (redirect binding, signed-in) returns auto-submit form back to Slack ACS', async () => { + const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); + const res = await app.inject({ + method: 'GET', + url: `/api/saml/slack/sso?${redirectBindingQuery(slackAuthnRequestXml('id-redirect-3'), 'opaque-redirect-state')}`, + cookies: { cfp_session: accessToken }, + }); + expect(res.statusCode).toBe(200); + expect(res.headers['content-type']).toMatch(/text\/html/); + expect(res.body).toContain(`action="https://${SLACK_TEAM_HOST}/sso/saml"`); + expect(res.body).toContain('name="RelayState" value="opaque-redirect-state"'); + + const xml = decodeSamlResponse(res.body); + const doc = new DOMParser().parseFromString(xml, 'application/xml'); + const root = doc.documentElement!; + expect(root.localName).toBe('Response'); + expect(root.getAttribute('InResponseTo')).toBe('id-redirect-3'); + expect(root.getElementsByTagNameNS(ASSERTION_NS, 'NameID')[0]?.textContent).toBe(slug); + }); + + it('GET /api/saml/slack/sso (redirect binding) with bad ACS URL → 422', async () => { + const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); + const res = await app.inject({ + method: 'GET', + url: `/api/saml/slack/sso?${redirectBindingQuery(slackAuthnRequestXml('id-redirect-4', 'evil.example.com'))}`, + cookies: { cfp_session: accessToken }, + }); + expect(res.statusCode).toBe(422); + expect(res.json<{ error: { code: string } }>().error.code).toBe('validation_failed'); + }); + + it('GET /api/saml/slack/sso with a non-DEFLATEd (POST-style) SAMLRequest → 422', async () => { + // Plain base64 (no DEFLATE) is the POST binding's wire form; on the + // Redirect binding it must fail to inflate rather than be accepted. + const plainB64 = Buffer.from(slackAuthnRequestXml('id-redirect-5'), 'utf8').toString('base64'); + const res = await app.inject({ + method: 'GET', + url: `/api/saml/slack/sso?${new URLSearchParams({ SAMLRequest: plainB64 }).toString()}`, + }); + expect(res.statusCode).toBe(422); + expect(res.json<{ error: { code: string } }>().error.code).toBe('validation_failed'); + }); + it('GET /api/saml/slack/launch?channel=phlask carries channel as RelayState', async () => { const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY); const res = await app.inject({ From 78ab9be303e74fc6085c79bc19a9303e05625960 Mon Sep 17 00:00:00 2001 From: Chris Alfano Date: Thu, 10 Sep 2026 02:36:02 -0400 Subject: [PATCH 3/3] chore(plans): mark saml-authn-statement done (PR #174) Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015jmpurQpBHXD9yLFEGUriR --- plans/saml-authn-statement.md | 152 ++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) create mode 100644 plans/saml-authn-statement.md diff --git a/plans/saml-authn-statement.md b/plans/saml-authn-statement.md new file mode 100644 index 0000000..92d40eb --- /dev/null +++ b/plans/saml-authn-statement.md @@ -0,0 +1,152 @@ +--- +status: done +depends: [saml-idp, saml-self-host] +specs: + - specs/api/saml.md +issues: [] +pr: 174 +--- + +# Plan: SAML AuthnStatement + Redirect-binding SSO + +## Scope + +Close the two conformance gaps that surfaced while preparing to test the IdP +against the real Slack workspace: + +1. The assertion emits no ``. The Web Browser SSO profile + requires at least one and Slack sends a `RequestedAuthnContext`, so this is + the most likely rejection on first contact. +2. Slack's "Test configuration" 404s: Slack delivers the AuthnRequest over the + HTTP-Redirect binding (`GET /api/saml/slack/sso?SAMLRequest=`), + which our metadata advertises but the API only registered as `POST`. + +In: spec both behaviours, implement, test. Out: any change to NameID / +attribute shape (frozen by [`saml-idp`](saml-idp.md)); echoing the SP's +`RequestedAuthnContext` (deliberately not — see spec); signed AuthnRequests +(Slack doesn't sign; `wantAuthnRequestsSigned` stays false); the still-open +end-to-end verification against the live workspace (#51). + +## Implements + +- [api/saml.md](../specs/api/saml.md) — `## Identity assertion` → + `### Authentication statement` (AuthnInstant, SessionIndex, fixed + `AuthnContextClassRef`, `SessionNotOnOrAfter` omitted); the endpoints table + and `## GET | POST /api/saml/slack/sso` (both bindings, one flow). + +## Approach + +- **Legacy check first.** Read the Emergence SAML2 connector laddr used + (`JarvusInnovations/emergence-saml2` → `Emergence\SAML2\Connector`) to see + what it emitted: `setSessionIndex(generateId())` and + `setAuthnContext(SAML2_Constants::AC_PASSWORD)` on a simplesamlphp + `Assertion`, whose `AuthnInstant` defaults to construction time and whose + `SessionNotOnOrAfter` is left null. `AC_PASSWORD` is + `urn:oasis:names:tc:SAML:2.0:ac:classes:Password` (confirmed in + simplesamlphp/saml2 `Constants.php`). Match that class rather than the + brief's initial `PasswordProtectedTransport` — continuity with what the + workspace already accepted beats the more precise label. +- **Template.** Add the AuthnStatement literally to `loginResponseTemplate.context` + in `apps/api/src/saml/config.ts`, after `` and before + `{AttributeStatement}`. samlify's built-in template has a bare + `{AuthnStatement}` slot that its default path blanks; we're on the + `customTagReplacement` path anyway, so we own the markup and fill three + placeholders of our own: `{AuthnInstant}`, `{SessionIndex}`, + `{AuthnContextClassRef}`. +- **Substitutions.** `buildResponseSubstitutions` gains a `sessionIndex` input + and emits the three tags; `AuthnInstant` reuses the same `now` as + `IssueInstant`. The route's `buildCustomTagReplacement` mints the + SessionIndex from the same `generateID` as the Response/Assertion IDs so it + is fresh and distinct. Signing happens after the callback returns, so the + statement is inside the signed subtree with no further work. +- **Redirect binding.** Register `GET /api/saml/slack/sso`. Instead of samlify's + `parseLoginRequest(sp, 'redirect', { query })` plus a binding-aware resume + cookie, inflate at the edge (`inflateRawSync` → re-base64) into the plain + form the POST binding carries, then run both methods through a shared + `handleSpInitiatedSso`. samlify's redirect flow is exactly that inflate + followed by the same parser (checked in `flow.js`), and the resume cookie's + `samlRequest` claim keeps one shape. Fold `' '` back to `'+'` in the query + value before decoding — a sender that leaves base64 `+` unescaped has it + URL-decoded to a space. +- **Tests** in `apps/api/tests/saml.test.ts`: AuthnStatement position, fixed + ClassRef, `AuthnInstant` ISO and `<= IssueInstant`, SessionIndex distinct + from the assertion ID, no `SessionNotOnOrAfter`; cryptographic signature + verification via `samlify.SamlLib.verifySignature` against the metadata + endpoint's cert, plus a tamper case; Redirect binding for signed-in, + anonymous → `/sso/resume`, bad ACS, and non-DEFLATEd payload. + +## Validation + +- [x] Every issued assertion contains exactly one `AuthnStatement`, after + `Conditions` and before `AttributeStatement` +- [x] `AuthnContextClassRef` is `urn:oasis:names:tc:SAML:2.0:ac:classes:Password` + on both IdP-initiated and SP-initiated responses +- [x] `AuthnInstant` parses as ISO-8601 and is `<=` the assertion `IssueInstant` +- [x] `SessionIndex` present, opaque, and not equal to the assertion `ID`; + `SessionNotOnOrAfter` absent +- [x] Assertion signature verifies against the metadata cert with the + AuthnStatement present; altering the ClassRef invalidates it +- [x] `GET /api/saml/slack/sso` with a DEFLATEd `SAMLRequest`: signed-in → 200 + auto-submit form with `InResponseTo` + `RelayState`; anonymous → resume + cookie + 302 `/login`, and `/sso/resume` completes the flow +- [x] `GET /api/saml/slack/sso` with a bad ACS → 422; with a non-DEFLATEd + payload → 422 +- [x] Existing POST-binding tests unchanged and green +- [x] `type-check` + `lint` clean; api suite green (saml.test.ts 18/18; full + api suite 440/440, no fixture-clone flake this run) +- [ ] Slack "Test configuration" passes against the live workspace + +## Risks / unknowns + +- **Slack rejecting `Password` vs its requested `PasswordProtectedTransport`.** + Slack's `RequestedAuthnContext` uses `Comparison="exact"` by default, but the + legacy connector asserted `Password` against this workspace for years, so the + check is evidently lenient. If the live test rejects on context, the fix is a + one-line constant change plus a spec edit — not a redesign. +- **Redirect-binding query decoding.** Fastify's query parser URL-decodes once; + a compliant sender percent-encodes `+`, `/`, `=` and we get the base64 back + intact. The space-fold covers the one common non-compliance. A sender that + double-encodes would still fail to inflate → 422 with a log line. +- **Fixture-clone flake (#171)** on the full api suite is unrelated; re-run the + file alone if it trips. + +## Notes + +- **Live-workspace criterion left unchecked.** Requires a Slack admin to run + "Test configuration" against a deployed build; the code fix for the 404 it + hit is here, but the pass itself can only be observed in sandbox/prod. Closes + out under #51 with the rest of the e2e verification. +- **`Password`, not `PasswordProtectedTransport`.** The brief opened with + `PasswordProtectedTransport` (Slack's default request). Reading the legacy + connector changed the call: laddr asserted `AC_PASSWORD` against this exact + workspace with no rejections, so matching it is the lower-risk choice and is + now the spec'd value. The class is a single exported constant + (`SLACK_AUTHN_CONTEXT_CLASS_REF`) if the live test disagrees. +- **Legacy `NotBefore` skew.** The legacy connector set `Conditions/@NotBefore` + to `time() - 30`; ours is `now`. Not changed here — samlify's own default is + `now`, there's no evidence Slack has trouble with it, and it's out of this + plan's scope. Worth remembering if the live test reports a `NotBefore` + clock-skew failure. +- **samlify template ownership.** samlify's built-in login-response template + has a `{AuthnStatement}` slot, but the default (non-custom) substitution + blanks it and we're on the `customTagReplacement` path regardless, so the + statement lives literally in our template with our own placeholder names. + Do not expect samlify to inject one for you. +- **Redirect-binding normalisation.** `inflateRedirectBindingRequest` folds + the GET wire form into the POST wire form at the edge; downstream code and + the resume cookie never learn which binding carried the request. samlify's + `'redirect'` parser was checked (`flow.js` → `inflateRawSync` then the same + parse) and deliberately not used so the cookie stays binding-agnostic. +- **Cryptographic signature check in tests.** `samlify.SamlLib.verifySignature` + with `IdPMetadata()` verifies against the exact cert + the metadata advertises and returns the signed subtree, which is a cheap way + to assert "X is inside the signature" — reusable for future assertion-shape + changes. + +## Follow-ups + +- Tracked as: live Slack "Test configuration" + member sign-in against the + deployed build — issue [#51](https://github.com/CodeForPhilly/codeforphilly-ng/issues/51) + (e2e verification against a real workspace) already covers it; this plan + adds the AuthnStatement/ClassRef and Redirect-binding outcomes to what that + run should confirm.