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({ 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. 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"`