diff --git a/api/package.json b/api/package.json index fea50e9e..ded41299 100644 --- a/api/package.json +++ b/api/package.json @@ -53,6 +53,7 @@ "prom-client": "^15.1.3", "qrcode": "^1.5.4", "rate-limiter-flexible": "^5.0.5", + "@authenio/samlify-node-xmllint": "^2.0.0", "samlify": "^2.11.0", "seedrandom": "^3.0.5", "serialize-javascript": "^7.0.4", diff --git a/api/src/saml2/service.ts b/api/src/saml2/service.ts index 921f0a42..623ba5ab 100644 --- a/api/src/saml2/service.ts +++ b/api/src/saml2/service.ts @@ -7,16 +7,22 @@ import type { SAML2 } from '../../config/type/index.ts' import config from '#config' import _slug from 'slugify' import samlify from 'samlify' +// @ts-ignore -- no ambient types; see node_modules/@authenio/samlify-node-xmllint/build/index.d.ts +import * as xsdValidator from '@authenio/samlify-node-xmllint' import Debug from 'debug' import mongo from '#mongo' import { decipher, cipher } from '../utils/cipher.ts' -import { exec } from 'node:child_process' +import { execFile } from 'node:child_process' import { promisify } from 'node:util' +import { generateKeyPairSync } from 'node:crypto' +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' import { getSiteBaseUrl, reqSite } from '#services' import { type Site } from '#types' import eventsLog, { type EventLogContext } from '@data-fair/lib-express/events-log.js' -const execAsync = promisify(exec) +const execFileAsync = promisify(execFile) const debug = Debug('saml') const slug = _slug.default @@ -35,14 +41,11 @@ export type Saml2RelayState = { adminMode?: boolean } -// const validator = require('@authenio/samlify-xsd-schema-validator') -// samlify.setSchemaValidator(validator) -// TODO: apply an actual validator cf https://github.com/tngan/samlify#installation -samlify.setSchemaValidator({ - validate: (response) => { - return Promise.resolve('skipped') - } -}) +// Schema validation is REQUIRED — samlify relies on it to mitigate XML signature-wrapping +// attacks (e.g. CVE-2025-47949, CVE-2024-45231 / -45239). Do not replace with a pass-through +// shim under any circumstances: a `validate: () => Promise.resolve('skipped')` reintroduces +// every known samlify assertion-injection CVE. See security review C-2. +samlify.setSchemaValidator(xsdValidator) export const getSamlProviderById = async (req: Request, id: string): Promise => { const site = await reqSite(req) @@ -168,6 +171,19 @@ export const init = async () => { } } +// For test-env DELETE / cleanup: drop the cached SAML cert from mongo, reset the in-memory +// SP singleton, and re-run init so the next request mints a fresh cert via createCert. +// Without this, dev/CI cycles reuse whatever cert was minted at first server startup and +// the createCert path stays untested in the live env even after code changes. Site-level +// certs share the `saml-certificates*` _id prefix so they're cleaned up too. +export const resetForTests = async () => { + await mongo.saml2RelayStates.deleteMany({}) + await mongo.secrets.deleteMany({ _id: { $regex: /^saml-certificates/ } }) + _sp = undefined + _globalProviders.length = 0 + await init() +} + export const initServiceProvider = async (site?: Site) => { const certificates = await initCertificates(site) const url = site ? `${getSiteBaseUrl(site)}/simple-directory` : config.publicUrl @@ -176,27 +192,55 @@ export const initServiceProvider = async (site?: Site) => { Location: `${url}/api/auth/saml2-assert` }] debug('config service provider') + // config.saml2.sp is spread FIRST so it can only tune tangential options (NameIDFormat, + // signing algorithm hints, etc.) — it cannot override our cert material, entityID, + // assertionConsumerService, or flip wantAssertionsSigned off. Order matters (see C-2). return samlify.ServiceProvider({ + ...config.saml2.sp, entityID: `${url}/api/auth/saml2-metadata.xml`, assertionConsumerService, signingCert: certificates.signing.cert, privateKey: certificates.signing.privateKey, encryptCert: certificates.encrypt.cert, encPrivateKey: certificates.encrypt.privateKey, + wantAssertionsSigned: true, // @ts-ignore if we use a boolean the attribute is set as empty in the xml output, and some IDP don't like that - allowCreate: 'false', - ...config.saml2.sp + allowCreate: 'false' }) } -const createCert = async () => { - const subject = `/C=FR/CN=${new URL(config.publicUrl).hostname}` - const privateKey = (await execAsync('openssl genpkey -algorithm RSA -pkeyopt rsa_keygen_bits:2048')).stdout - const certPromise = execAsync(`echo "${privateKey}" | openssl req -key /dev/stdin -x509 -sha256 -nodes -days 1095 -subj "${subject}"`) - certPromise.child.stdin?.write(privateKey) - certPromise.child.stdin?.end() - const cert = (await certPromise).stdout - return { privateKey, cert } +// Exported for the C-2 regression test in tests/features/saml2-create-cert.unit.spec.ts — +// the live dev env almost always reads a cached cert from the `secrets` collection, so +// without a direct call this path would be entirely untested. +export const createCert = async () => { + const hostname = new URL(config.publicUrl).hostname + // Reject hostnames that would be parsed as shell metacharacters by openssl's -subj + // arg or that contain CR/LF (defense-in-depth; execFile already prevents shell parsing). + if (!/^[A-Za-z0-9.\-_:[\]]+$/.test(hostname)) { + throw new Error(`refusing to mint SAML certificate for suspicious hostname: ${hostname}`) + } + const subject = `/C=FR/CN=${hostname}` + // Generate the RSA key in-process (no shell, no string interpolation, no stdin race). + const { privateKey } = generateKeyPairSync('rsa', { + modulusLength: 2048, + privateKeyEncoding: { type: 'pkcs8', format: 'pem' }, + publicKeyEncoding: { type: 'spki', format: 'pem' } + }) + // OpenSSL 3.x's STORE subsystem refuses to read `-key /dev/stdin` from a Node-spawned + // pipe (fopen() on a non-seekable fd fails). Stage the key in a 0700 temp directory + // so it's owner-only readable for the brief openssl invocation, then unlink in finally. + const tmpDir = mkdtempSync(join(tmpdir(), 'sd-saml-')) + const keyPath = join(tmpDir, 'key.pem') + try { + writeFileSync(keyPath, privateKey, { mode: 0o600 }) + // execFile with argv array — `subject` and `keyPath` never reach a shell. + const { stdout: cert } = await execFileAsync('openssl', [ + 'req', '-key', keyPath, '-x509', '-sha256', '-nodes', '-days', '1095', '-subj', subject + ]) + return { privateKey, cert } + } finally { + rmSync(tmpDir, { recursive: true, force: true }) + } } // attributes for microsoft entra https://learn.microsoft.com/en-us/entra/identity-platform/reference-saml-tokens diff --git a/api/src/test-env.ts b/api/src/test-env.ts index fc793fc4..f67a6d95 100644 --- a/api/src/test-env.ts +++ b/api/src/test-env.ts @@ -35,6 +35,10 @@ router.delete('/', async (req, res) => { await mongo.db.collection('sd-rate-limiter-auth').deleteMany() const { getSiteByHost } = await import('./sites/service.ts') getSiteByHost.clear() + // Force a fresh SAML cert mint on the next request — exercises createCert end-to-end + // every cleanup cycle instead of letting a one-time bootstrap cert linger. + const { resetForTests: resetSaml } = await import('./saml2/service.ts') + await resetSaml() res.status(204).send() }) diff --git a/docs/architecture/email-trust-and-site-isolation.md b/docs/architecture/email-trust-and-site-isolation.md index acc8f9ff..93399566 100644 --- a/docs/architecture/email-trust-and-site-isolation.md +++ b/docs/architecture/email-trust-and-site-isolation.md @@ -61,6 +61,13 @@ looser IdP. - **LinkedIn** — primary email is considered verified upstream. - **SAML 2** — no standard verified flag; adding a SAML IdP is an explicit trust statement. Site-level IdPs remain confined by user scoping. + `api/src/saml2/service.ts` installs `@authenio/samlify-node-xmllint` as the + samlify schema validator, which is required to mitigate XML + signature-wrapping attacks. The SP also advertises + `wantAssertionsSigned: true` in its metadata, and the + `config.saml2.sp` operator-config spread cannot override SP cert material, + `entityID`, `assertionConsumerService`, or the wantAssertionsSigned flag + (the spread is applied first, the trusted fields last). ### Why OIDC is looser than the OAuth providers diff --git a/package-lock.json b/package-lock.json index 462ac520..da5ddda1 100644 --- a/package-lock.json +++ b/package-lock.json @@ -63,6 +63,7 @@ "name": "@sd/api", "license": "MIT", "dependencies": { + "@authenio/samlify-node-xmllint": "^2.0.0", "@data-fair/lib-express": "^1.19.0", "@data-fair/lib-node": "^2.12.1", "@data-fair/lib-utils": "^1.10.1", @@ -131,6 +132,18 @@ "url": "https://github.com/sponsors/philsturgeon" } }, + "node_modules/@authenio/samlify-node-xmllint": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@authenio/samlify-node-xmllint/-/samlify-node-xmllint-2.0.0.tgz", + "integrity": "sha512-V9cQ0CHqu3JwOmbSecGPUnzIES5kHxD00FEZKnWh90ksQUJG5/TscV2r9XLbKp7MlRMOSUfWxecM35xPSLFdSg==", + "license": "MIT", + "dependencies": { + "node-xmllint": "^1.0.0" + }, + "peerDependencies": { + "samlify": ">= 2.6.0" + } + }, "node_modules/@authenio/xml-encryption": { "version": "2.0.2", "license": "MIT", @@ -8848,6 +8861,12 @@ "asn1": "^0.2.4" } }, + "node_modules/node-xmllint": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/node-xmllint/-/node-xmllint-1.0.0.tgz", + "integrity": "sha512-71UV2HRUP+djvHpdyatiuv+Y1o8hI4ZI7bMfuuoACMLR1JJCErM4WXAclNeHd6BgHXkqeqnnAk3wpDkSQWmFXw==", + "license": "MIT" + }, "node_modules/nodemailer": { "version": "6.10.1", "resolved": "https://registry.npmjs.org/nodemailer/-/nodemailer-6.10.1.tgz", diff --git a/tests/features/saml2-create-cert.unit.spec.ts b/tests/features/saml2-create-cert.unit.spec.ts new file mode 100644 index 00000000..91768c90 --- /dev/null +++ b/tests/features/saml2-create-cert.unit.spec.ts @@ -0,0 +1,71 @@ +// Regression coverage for the C-2 follow-up: api/src/saml2/service.ts#createCert +// was rewritten from +// exec(`echo "${privateKey}" | openssl req -key /dev/stdin ... -subj "${subject}"`) +// — a shell pipeline with string interpolation and a stdin race — to +// crypto.generateKeyPairSync + execFile('openssl', [...]) (no shell, argv array). +// +// In production the path runs once per (deployment, site) and the result is cached +// in mongo's `secrets` collection forever, so the live dev env will normally NOT +// exercise it after the first restart. This test calls createCert directly so any +// regression (wrong PEM shape, broken openssl invocation, cert without the right +// CN) fails CI even when the dev env is happily reading cached certs. + +import { strict as assert } from 'node:assert' +import { test } from '@playwright/test' +import { createPrivateKey, X509Certificate } from 'node:crypto' + +process.env.NODE_CONFIG_DIR = process.env.NODE_CONFIG_DIR || './api/config/' +process.env.NODE_ENV = process.env.NODE_ENV || 'test' +process.env.SUPPRESS_NO_CONFIG_WARNING = '1' + +test.describe('SAML2 createCert (C-2 follow-up)', () => { + let createCert: () => Promise<{ privateKey: string, cert: string }> + let expectedHostname: string + + test.beforeAll(async () => { + const samlService = await import('../../api/src/saml2/service.ts') + createCert = samlService.createCert + const config = (await import('../../api/src/config.ts')).default + expectedHostname = new URL(config.publicUrl).hostname + }) + + test('produces a PKCS#8 RSA private key parseable by node:crypto', async () => { + const { privateKey } = await createCert() + assert.match(privateKey, /^-----BEGIN PRIVATE KEY-----/) + assert.match(privateKey, /-----END PRIVATE KEY-----\s*$/) + const key = createPrivateKey(privateKey) + assert.equal(key.asymmetricKeyType, 'rsa') + // 2048-bit modulus per the explicit modulusLength in createCert + assert.equal(key.asymmetricKeyDetails?.modulusLength, 2048) + }) + + test('produces a self-signed X.509 cert with the configured publicUrl hostname as CN', async () => { + const { cert } = await createCert() + assert.match(cert, /^-----BEGIN CERTIFICATE-----/) + assert.match(cert, /-----END CERTIFICATE-----\s*$/) + const x509 = new X509Certificate(cert) + // openssl emits the subject as 'C=FR, CN=' (or with newlines depending on version) + assert.ok( + x509.subject.includes(`CN=${expectedHostname}`), + `cert subject ${JSON.stringify(x509.subject)} does not contain CN=${expectedHostname}` + ) + // self-signed: issuer matches subject + assert.equal(x509.issuer, x509.subject) + // validity window: 3 years (1095 days) per the -days arg in createCert + const validFrom = new Date(x509.validFrom).getTime() + const validTo = new Date(x509.validTo).getTime() + const days = (validTo - validFrom) / (1000 * 60 * 60 * 24) + assert.ok(Math.abs(days - 1095) < 1, `expected ~1095-day validity window, got ${days}`) + }) + + test('cert and key form a matching keypair (cert public key verifies against private key signature shape)', async () => { + const { privateKey, cert } = await createCert() + const x509 = new X509Certificate(cert) + const keyObj = createPrivateKey(privateKey) + // Both should expose the same public modulus length + assert.equal(keyObj.asymmetricKeyDetails?.modulusLength, 2048) + assert.equal(x509.publicKey.asymmetricKeyDetails?.modulusLength, 2048) + // X509Certificate.checkPrivateKey is the canonical "do these belong together" check + assert.ok(x509.checkPrivateKey(keyObj), 'cert public key does not match the generated private key') + }) +}) diff --git a/tests/features/saml2-schema-validator.unit.spec.ts b/tests/features/saml2-schema-validator.unit.spec.ts new file mode 100644 index 00000000..729d2832 --- /dev/null +++ b/tests/features/saml2-schema-validator.unit.spec.ts @@ -0,0 +1,82 @@ +// Regression guard for C-2 (security-review-2026-04.md): +// before PR #120 (chore: harden SAML), api/src/saml2/service.ts installed +// samlify.setSchemaValidator({ validate: () => Promise.resolve('skipped') }) +// which re-enabled every samlify XML-signature-wrapping CVE because the validator +// itself is the defense layer. If anyone reverts to that no-op shim, the tests +// below must fail loudly. + +import { strict as assert } from 'node:assert' +import { test } from '@playwright/test' + +// Minimal env bootstrap so `import '#config'` loads the test config without requiring +// the full in-process server setup. +process.env.NODE_CONFIG_DIR = process.env.NODE_CONFIG_DIR || './api/config/' +process.env.NODE_ENV = process.env.NODE_ENV || 'test' +process.env.SUPPRESS_NO_CONFIG_WARNING = '1' + +test.describe('SAML2 schema validator (C-2 regression)', () => { + let samlifyCtx: { validate?: (xml: string) => Promise } + + test.beforeAll(async () => { + // Importing the module triggers `samlify.setSchemaValidator(xsdValidator)`. + await import('../../api/src/saml2/service.ts') + // samlify's top-level index does not re-export getContext; import it from the + // CJS submodule where it lives. Brittle-ish, but the path has been stable + // across samlify 2.x. + // @ts-ignore -- no declaration file for this submodule + const { getContext } = await import('samlify/build/src/api.js') as any + samlifyCtx = getContext() + }) + + test('a validator is wired in', async () => { + assert.equal(typeof samlifyCtx.validate, 'function') + }) + + test('validator is not the "skipped" no-op shim', async () => { + // The historical shim resolved every input (including `undefined`) with + // the literal string 'skipped'. A real validator either rejects on + // invalid SAML XML, or throws synchronously, or resolves with a + // different success marker — anything except 'skipped'. + let result: unknown + try { + result = await samlifyCtx.validate!('') + } catch (err) { + result = err + } + assert.notEqual(result, 'skipped', + 'C-2: SAML schema validator has been reverted to the no-op shim — see docs/local/security-review-2026-04.md') + }) + + test('validator rejects garbage (non-XML) input', async () => { + await assert.rejects(() => samlifyCtx.validate!('this is not xml at all')) + }) + + test('validator rejects well-formed XML that is not a SAML protocol envelope', async () => { + // Well-formed XML, no SAML namespace, no root — must fail + // schema validation. Before the fix, the shim silently accepted this. + await assert.rejects(() => samlifyCtx.validate!( + 'bar' + )) + }) + + test('validator rejects a Response whose Assertion carries unexpected child elements (signature-wrapping shape)', async () => { + // A classic XML-signature-wrapping shape: a root whose + // contains a sibling element that is NOT in the SAML + // assertion schema. samlify's lib would extract attributes from the + // attacker-supplied element while verifying the signature of the + // legitimate one. The schema validator is the defense that stops the + // malformed document at the gate. This XML is deliberately crafted to be + // well-formed but schema-invalid. + const wrapped = ` + + https://evil.example/idp + + https://evil.example/idp + admin@victim.com + +` + await assert.rejects(() => samlifyCtx.validate!(wrapped)) + }) +})