Skip to content

Commit 2d4fabd

Browse files
themightychristhemightychris
authored
Merge pull request #161 from CodeForPhilly/fix/saml-self-host
fix(api): build SAML entityID and SSO endpoints from our own host, not Slack's
2 parents 8357170 + b8a3fe1 commit 2d4fabd

10 files changed

Lines changed: 331 additions & 26 deletions

File tree

‎.env.example‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,16 @@ CFP_JWT_SIGNING_KEY=change-me-to-a-random-string-at-least-32-chars
7171
# PEM-encoded certificate matching SAML_PRIVATE_KEY.
7272
# SAML_CERTIFICATE=-----BEGIN CERTIFICATE-----\n...\n-----END CERTIFICATE-----
7373

74+
# Stable IdP entity ID — also the <Issuer> on every assertion. Slack stores
75+
# this at setup, so it must NOT change when CFP_SITE_HOST flips at cutover.
76+
# Leave unset unless registering a separate IdP with a different workspace.
77+
# See specs/api/saml.md#idp-identity-and-hosts.
78+
# SAML_ENTITY_ID=https://codeforphilly.org/api/saml/slack/metadata
79+
80+
# Slack workspace host. Drives the ACS URL, NameID NameQualifier, and the
81+
# /chat + /launch redirects. Never used for our own entity ID or endpoints.
82+
# SLACK_TEAM_HOST=codeforphilly.slack.com
83+
7484
# ---------------------------------------------------------------------------
7585
# Static SPA serving (production only)
7686
# ---------------------------------------------------------------------------

‎apps/api/src/env.ts‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,12 @@
66
*/
77
import { z } from 'zod';
88

9+
/**
10+
* Default SAML IdP entity ID. Stable across hosts — see the SAML_ENTITY_ID
11+
* field below and specs/api/saml.md#idp-identity-and-hosts.
12+
*/
13+
export const SAML_ENTITY_ID_DEFAULT = 'https://codeforphilly.org/api/saml/slack/metadata';
14+
915
export const EnvSchema = z.object({
1016
/** TCP port the Fastify server listens on. */
1117
PORT: z.coerce.number().default(3001),
@@ -49,8 +55,17 @@ export const EnvSchema = z.object({
4955
/** SAML IdP certificate (PEM) for the Slack SAML integration. */
5056
SAML_CERTIFICATE: z.string().optional(),
5157
/**
52-
* Slack workspace host. Used as the SAML `NameQualifier` per
53-
* specs/api/saml.md and shared with the `/chat` redirect handler.
58+
* SAML IdP entity ID — the metadata `entityID` and the `<Issuer>` on every
59+
* assertion. A stable logical identifier Slack stores at setup time, so it
60+
* deliberately does NOT follow CFP_SITE_HOST: the pre-cutover
61+
* `next.codeforphilly.org` deploy and the post-cutover `codeforphilly.org`
62+
* deploy present the same issuer. Per specs/api/saml.md#idp-identity-and-hosts.
63+
*/
64+
SAML_ENTITY_ID: z.url().default(SAML_ENTITY_ID_DEFAULT),
65+
/**
66+
* Slack workspace host. Used for the SAML ACS URL and `NameQualifier` per
67+
* specs/api/saml.md and shared with the `/chat` redirect handler. Never
68+
* used for our own IdP entity ID or endpoint URLs.
5469
*/
5570
SLACK_TEAM_HOST: z.string().default('codeforphilly.slack.com'),
5671
/**
@@ -64,7 +79,9 @@ export const EnvSchema = z.object({
6479
* `next-v2.codeforphilly.org` in sandbox). Used by the server-side
6580
* markdown renderer to distinguish internal from external links — anchors
6681
* with a host different from this one get `target="_blank" rel="noopener
67-
* nofollow"`. Per specs/behaviors/markdown-rendering.md.
82+
* nofollow"`. Per specs/behaviors/markdown-rendering.md. Also the host the
83+
* SAML IdP metadata advertises for its SSO endpoint Locations (per
84+
* specs/api/saml.md#idp-identity-and-hosts).
6885
*/
6986
CFP_SITE_HOST: z.string().default('codeforphilly.org'),
7087
/**
@@ -121,6 +138,7 @@ export const envJsonSchema = {
121138
CFP_JWT_SIGNING_KEY: { type: 'string', minLength: 1 },
122139
SAML_PRIVATE_KEY: { type: 'string' },
123140
SAML_CERTIFICATE: { type: 'string' },
141+
SAML_ENTITY_ID: { type: 'string', default: SAML_ENTITY_ID_DEFAULT },
124142
SLACK_TEAM_HOST: { type: 'string', default: 'codeforphilly.slack.com' },
125143
CFP_WEB_DIST_PATH: { type: 'string' },
126144
CFP_SITE_HOST: { type: 'string', default: 'codeforphilly.org' },

‎apps/api/src/routes/saml.ts‎

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -128,18 +128,21 @@ function getSamlContext(fastify: FastifyInstance): SamlContext {
128128
throw new ApiValidationError('SAML IdP is not configured');
129129
}
130130

131-
const base = `https://${cfg.SLACK_TEAM_HOST}`.replace('https://', '');
132-
const issuerHost = base;
133-
// Fallback to the team host for the metadata entity ID if we can't see
134-
// the inbound request origin. Per spec the entityID is our own URL —
135-
// we'll prefer the request origin when building responses.
131+
// Three distinct sources, per specs/api/saml.md#idp-identity-and-hosts:
132+
// - entityId (metadata entityID + assertion Issuer) is the stable
133+
// SAML_ENTITY_ID — it must NOT track the serving host, because Slack
134+
// stored it at setup and the host flips at cutover;
135+
// - the SSO endpoint Locations follow CFP_SITE_HOST so the metadata
136+
// points Slack at whatever host this deployment answers on;
137+
// - SLACK_TEAM_HOST is Slack's side only (ACS URL, NameQualifier).
138+
const ssoUrl = `https://${cfg.CFP_SITE_HOST}/api/saml/slack/sso`;
136139
const ctx: SamlContext = {
137140
entities: buildSlackSamlEntities({
138141
privateKey: cfg.SAML_PRIVATE_KEY,
139142
certificate: cfg.SAML_CERTIFICATE,
140-
entityId: `https://${issuerHost}/api/saml/slack/metadata`,
141-
ssoLoginPostUrl: `https://${issuerHost}/api/saml/slack/sso`,
142-
ssoLoginRedirectUrl: `https://${issuerHost}/api/saml/slack/sso`,
143+
entityId: cfg.SAML_ENTITY_ID,
144+
ssoLoginPostUrl: ssoUrl,
145+
ssoLoginRedirectUrl: ssoUrl,
143146
slackTeamHost: cfg.SLACK_TEAM_HOST,
144147
}),
145148
};

‎apps/api/src/saml/config.ts‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,21 @@ export interface SamlIdpSettings {
3636
readonly privateKey: string;
3737
/** PEM-encoded X.509 certificate (the public half). */
3838
readonly certificate: string;
39-
/** The IdP entity ID — also the metadata URL. */
39+
/**
40+
* The IdP entity ID — becomes the metadata `entityID` AND the `<Issuer>`
41+
* on every Response/Assertion (via `SlackSamlEntities.entityId` →
42+
* `issuerEntityId`). A stable logical identifier (`SAML_ENTITY_ID`), not
43+
* necessarily a URL that resolves on the serving host.
44+
*/
4045
readonly entityId: string;
41-
/** The IdP SSO POST binding location (the /launch endpoint). */
46+
/** The IdP SSO POST binding location — `https://<CFP_SITE_HOST>/api/saml/slack/sso`. */
4247
readonly ssoLoginPostUrl: string;
43-
/** The IdP SSO Redirect binding location. */
48+
/** The IdP SSO Redirect binding location — same URL as the POST binding. */
4449
readonly ssoLoginRedirectUrl: string;
45-
/** Slack team host (e.g. `codeforphilly.slack.com`). */
50+
/**
51+
* Slack team host (e.g. `codeforphilly.slack.com`). Slack-side only: the
52+
* ACS URL and the NameID `NameQualifier`. Never part of our own identity.
53+
*/
4654
readonly slackTeamHost: string;
4755
}
4856

‎apps/api/tests/saml.test.ts‎

Lines changed: 111 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,36 @@ import { getSamlTestKeyPair, type SamlTestKeyPair } from './helpers/saml-cert.js
2626

2727
const JWT_KEY = 'test-jwt-signing-key-at-least-32-chars!!';
2828
const SLACK_TEAM_HOST = 'codeforphilly.slack.com';
29+
/** Default SAML_ENTITY_ID per specs/api/saml.md#idp-identity-and-hosts. */
30+
const DEFAULT_ENTITY_ID = 'https://codeforphilly.org/api/saml/slack/metadata';
31+
/** Default CFP_SITE_HOST — the SSO endpoint Locations are built on it. */
32+
const DEFAULT_SITE_HOST = 'codeforphilly.org';
33+
34+
const MD_NS = 'urn:oasis:names:tc:SAML:2.0:metadata';
35+
const ASSERTION_NS = 'urn:oasis:names:tc:SAML:2.0:assertion';
36+
37+
function ssoLocations(metadataXml: string): { entityId: string | null; locations: string[] } {
38+
const doc = new DOMParser().parseFromString(metadataXml, 'application/xml');
39+
const root = doc.documentElement;
40+
const locations = Array.from(root?.getElementsByTagNameNS(MD_NS, 'SingleSignOnService') ?? [])
41+
.map((el) => el.getAttribute('Location'))
42+
.filter((v): v is string => typeof v === 'string');
43+
return { entityId: root?.getAttribute('entityID') ?? null, locations };
44+
}
45+
46+
/** Every `<saml:Issuer>` text in a decoded SAMLResponse (Response + Assertion). */
47+
function issuers(responseXml: string): string[] {
48+
const doc = new DOMParser().parseFromString(responseXml, 'application/xml');
49+
return Array.from(doc.documentElement?.getElementsByTagNameNS(ASSERTION_NS, 'Issuer') ?? []).map(
50+
(el) => el.textContent ?? '',
51+
);
52+
}
53+
54+
function decodeSamlResponse(html: string): string {
55+
const match = /name="SAMLResponse" value="([^"]+)"/.exec(html);
56+
expect(match).not.toBeNull();
57+
return Buffer.from(match![1]!, 'base64').toString('utf8');
58+
}
2959

3060
async function seedPerson(
3161
repoDir: string,
@@ -129,10 +159,17 @@ describe('SAML IdP — Slack', () => {
129159
const root = doc.documentElement;
130160
expect(root?.localName).toBe('EntityDescriptor');
131161

132-
// entityID present
133-
expect(root?.getAttribute('entityID')).toBe(
134-
`https://${SLACK_TEAM_HOST}/api/saml/slack/metadata`,
135-
);
162+
// entityID is the stable SAML_ENTITY_ID default — NOT built on
163+
// SLACK_TEAM_HOST (Slack's host) and NOT on the serving host.
164+
expect(root?.getAttribute('entityID')).toBe(DEFAULT_ENTITY_ID);
165+
166+
// Both SSO bindings point at our own site host.
167+
const { locations } = ssoLocations(res.body);
168+
expect(locations).toHaveLength(2);
169+
for (const loc of locations) {
170+
expect(loc).toBe(`https://${DEFAULT_SITE_HOST}/api/saml/slack/sso`);
171+
}
172+
expect(res.body).not.toContain(`https://${SLACK_TEAM_HOST}/api/saml`);
136173

137174
// IDPSSODescriptor + at least one SingleSignOnService and an X509Certificate.
138175
const idpDescriptors = root?.getElementsByTagNameNS(
@@ -190,6 +227,10 @@ describe('SAML IdP — Slack', () => {
190227
const root = doc.documentElement;
191228
expect(root?.localName).toBe('Response');
192229

230+
// Issuer on both the Response and the Assertion is the entity ID — the
231+
// same value the metadata advertises as entityID.
232+
expect(issuers(xml)).toEqual([DEFAULT_ENTITY_ID, DEFAULT_ENTITY_ID]);
233+
193234
// NameID is the slackSamlNameId, format persistent
194235
const nameIdEl = root?.getElementsByTagNameNS(
195236
'urn:oasis:names:tc:SAML:2.0:assertion',
@@ -319,6 +360,72 @@ describe('SAML IdP — Slack', () => {
319360
});
320361
});
321362

363+
describe('SAML IdP — entity ID vs. site host', () => {
364+
let dataRepo: { path: string; cleanup: () => Promise<void> };
365+
let privateStore: { path: string; cleanup: () => Promise<void> };
366+
let keyPair: SamlTestKeyPair;
367+
const personId = '01951a3c-0000-7000-8000-000000000002';
368+
const slug = 'sam';
369+
370+
beforeAll(async () => {
371+
keyPair = await getSamlTestKeyPair();
372+
dataRepo = await createFullDataRepo();
373+
privateStore = await createPrivateStorageDir();
374+
await seedPerson(dataRepo.path, { id: personId, slug, slackSamlNameId: slug });
375+
await seedPrivateProfile(privateStore.path, { personId, email: 'sam@example.com' });
376+
});
377+
378+
afterAll(async () => {
379+
await dataRepo.cleanup();
380+
await privateStore.cleanup();
381+
});
382+
383+
it('CFP_SITE_HOST moves the SSO Locations but leaves entityID alone', async () => {
384+
const app = await buildTestApp(dataRepo.path, privateStore.path, keyPair, {
385+
CFP_SITE_HOST: 'next.example.org',
386+
});
387+
try {
388+
const res = await app.inject({ method: 'GET', url: '/api/saml/slack/metadata' });
389+
expect(res.statusCode).toBe(200);
390+
const { entityId, locations } = ssoLocations(res.body);
391+
expect(locations).toHaveLength(2);
392+
for (const loc of locations) {
393+
expect(loc).toBe('https://next.example.org/api/saml/slack/sso');
394+
}
395+
// The pre-cutover host does not leak into the identifier Slack stores.
396+
expect(entityId).toBe(DEFAULT_ENTITY_ID);
397+
} finally {
398+
await app.close();
399+
}
400+
});
401+
402+
it('SAML_ENTITY_ID overrides both the metadata entityID and the assertion Issuer', async () => {
403+
const entityId = 'https://idp.example.org/saml/slack';
404+
const app = await buildTestApp(dataRepo.path, privateStore.path, keyPair, {
405+
SAML_ENTITY_ID: entityId,
406+
CFP_SITE_HOST: 'next.example.org',
407+
});
408+
try {
409+
const meta = await app.inject({ method: 'GET', url: '/api/saml/slack/metadata' });
410+
expect(meta.statusCode).toBe(200);
411+
expect(ssoLocations(meta.body).entityId).toBe(entityId);
412+
413+
const { accessToken } = await mintSessionFor(personId, 'user', JWT_KEY);
414+
const launch = await app.inject({
415+
method: 'GET',
416+
url: '/api/saml/slack/launch',
417+
cookies: { cfp_session: accessToken },
418+
});
419+
expect(launch.statusCode).toBe(200);
420+
expect(issuers(decodeSamlResponse(launch.body))).toEqual([entityId, entityId]);
421+
// Slack-side values still come from SLACK_TEAM_HOST.
422+
expect(launch.body).toContain(`action="https://${SLACK_TEAM_HOST}/sso/saml"`);
423+
} finally {
424+
await app.close();
425+
}
426+
});
427+
});
428+
322429
describe('SAML IdP — without configured cert/key', () => {
323430
let dataRepo: { path: string; cleanup: () => Promise<void> };
324431
let privateStore: { path: string; cleanup: () => Promise<void> };

‎docs/operations/deploy.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,7 +225,8 @@ comments. Production pod gets these mounted:
225225
| `CFP_DATA_BRANCH` | ConfigMap | e.g. `fixture` / `main` |
226226
| `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). |
227227
| `CFP_WEB_DIST_PATH` | ConfigMap | `/app/apps/web/dist` |
228-
| `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"`. |
228+
| `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. |
229+
| `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). |
229230
| `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. |
230231
| `POSTMARK_MESSAGE_STREAM` | ConfigMap | Postmark message stream for outbound mail (default `outbound`). Must exist on the server the token belongs to. |
231232
| `CFP_NOTIFICATION_FROM` | ConfigMap | RFC 5322 sender address for outbound notifications (default `"Code for Philly <notifications@codeforphilly.org>"`). 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:
237238
| `GITHUB_OAUTH_CLIENT_SECRET` | **Secret** | OAuth app client secret |
238239
| `CFP_JWT_SIGNING_KEY` | **Secret** | HS256 key (`openssl rand -base64 64`) |
239240
| `SAML_PRIVATE_KEY` / `SAML_CERTIFICATE` | **Secret** | Slack IdP cert chain |
241+
| `SLACK_TEAM_HOST` | ConfigMap | Slack workspace host (default `codeforphilly.slack.com`). ACS URL, NameID `NameQualifier`, `/chat` + `/launch` redirect target. Never our own IdP identity. |
240242
| `GIT_SSH_COMMAND` | ConfigMap | Wires `ssh` to the mounted deploy key |
241243

242244
## Rollback

‎docs/operations/secrets.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,11 @@ integration ([specs/api/saml.md](../../specs/api/saml.md)).
108108
- **Rotation impact:** Slack stops trusting assertions until its IdP config
109109
is updated with the new cert. **Do not rotate without coordinating with
110110
the Slack workspace admin.**
111+
- **Not a secret, but paired:** `SAML_ENTITY_ID` (ConfigMap, optional) is
112+
the IdP identity Slack stores alongside this cert. It defaults to
113+
`https://codeforphilly.org/api/saml/slack/metadata` and must stay stable
114+
across host changes — see [deploy.md](deploy.md#environment-variables-reference)
115+
and [specs/api/saml.md](../../specs/api/saml.md#idp-identity-and-hosts).
111116
- **Rotation procedure:**
112117
1. Generate new key + cert.
113118
2. Upload the *new cert* to Slack as a secondary signing cert.

0 commit comments

Comments
 (0)