diff --git a/README.md b/README.md index 526e261..8fbbe8f 100644 --- a/README.md +++ b/README.md @@ -425,6 +425,13 @@ https://yourdomain.com/api/auth/oidc//callback The slug is generated rather than chosen so two customers cannot collide on — or guess — each other's. A domain may be claimed by only one organization; a second claim is refused. +A customer bringing **Microsoft/Entra** registers a single-tenant application in their own directory +and uses `https://login.microsoftonline.com//v2.0` as the issuer. Because Entra +does not send `email_verified`, an organization's provider is trusted to assert addresses **once it +has verified a domain** — the DNS proof is what stands in for the claim, and the provider is confined +to those domains regardless. A provider that has verified nothing assumes nothing, and an explicit +`email_verified: false` is refused whoever sends it. + ⚠️ **A provider may only authenticate emails inside the domains it has VERIFIED.** An organization supplies its own issuer and client ID, so it controls that identity provider completely and could otherwise assert any address at all — including another company's, or an administrator's. Confining diff --git a/server/lib/oidc-providers.js b/server/lib/oidc-providers.js index b41ed9d..b9b171c 100644 --- a/server/lib/oidc-providers.js +++ b/server/lib/oidc-providers.js @@ -74,11 +74,18 @@ function fromEnv(env, slug) { * already exempts it from domain confinement — and the Microsoft entry is additionally pinned to one * tenant GUID, so only that directory can issue tokens for it. * - * Two limits keep this from becoming the hole the strict check was closing: + * An organization's own provider may assume too, but only once it has DNS-verified a domain — the + * callback confines it to those domains, so it can only ever speak for names it proved it controls. + * Requiring the claim from it as well meant a customer's Entra tenant went green on domain + * verification and then failed the login anyway. + * + * Three limits keep this from becoming the hole the strict check was closing: * - an EXPLICIT `email_verified: false` is always refused. Assuming only ever covers an omitted * claim, never a provider actively saying the address is unverified; - * - org-configured providers can never set it (rowToProvider pins it false, and nothing reads it - * from the database), so the account-takeover path stays shut. + * - for an org provider it is DERIVED from proof (`verified.length > 0`) and is never a column, so + * a customer cannot switch it on for themselves; + * - domain confinement is unchanged and still runs first, so an org provider that assumes still + * cannot assert an address outside a domain it has proven. */ function emailIsVerified(claims, provider) { const asserted = (claims || {}).email_verified; @@ -211,6 +218,9 @@ function db() { } function rowToProvider(row, secretbox) { + // Resolved once: it decides both which addresses this provider may assert and, below, whether it + // has proven anything at all. + const verified = verifiedDomainsFor(row.id); return { slug: row.slug, name: row.name, @@ -226,14 +236,26 @@ function rowToProvider(row, secretbox) { : null, scopes: row.scopes || DEFAULT_SCOPES, /* - * ⚠️ NEVER settable for an organization's provider, and deliberately not read from the row. + * DERIVED from proof, never read from a column. * - * A customer chooses this provider, so it speaks for the party it vouches for. Letting an org - * assume verification would hand back exactly the takeover primitive the strict check exists to - * stop. Domain confinement narrows WHICH addresses it may assert; this keeps the assertion - * itself honest. + * Entra ID v2 omits email_verified, so demanding it refused every customer who brought their own + * Microsoft tenant — the domain went green and the login still failed. Requiring a claim + * Microsoft does not send is not a security control, it is an outage. + * + * What makes it safe to stop requiring it is the proof that already gates this provider: the + * callback confines it to DNS-verified domains, and an address is only reached here after + * passing that. Whoever controls a domain's DNS controls its mail, which is the same trust that + * makes a verification link meaningful in the first place. + * + * So the assumption is tied to having proven SOMETHING. A provider with no verified domain + * assumes nothing — belt and braces, because emailAllowedForProvider already refuses it (an + * empty allow-list matches no domain), and this way a future refactor that reorders those checks + * cannot silently widen it. + * + * ⚠️ Still never a column. An org must not be able to switch this on for itself; it is a + * consequence of DNS proof, not a setting. */ - assumeEmailVerified: false, + assumeEmailVerified: verified.length > 0, source: 'org', organizationId: row.organization_id, /* @@ -244,7 +266,7 @@ function rowToProvider(row, secretbox) { * Reading the typed column here would reduce the whole verification feature to a decoration: * a tenant could type any company's domain and immediately assert addresses in it. */ - emailDomains: verifiedDomainsFor(row.id).join(','), + emailDomains: verified.join(','), }; } diff --git a/server/test/oidc-email-verified-policy.test.js b/server/test/oidc-email-verified-policy.test.js index 7bdbeda..ca8ed46 100644 --- a/server/test/oidc-email-verified-policy.test.js +++ b/server/test/oidc-email-verified-policy.test.js @@ -9,10 +9,14 @@ * never pushed a Microsoft-shaped token through the policy, so nothing failed. * * The rule these tests pin down: - * - `email_verified: true` -> believed, always - * - claim ABSENT + operator-chosen -> believed (Microsoft, or an opted-in generic provider) - * - claim ABSENT + org-configured -> refused - * - `email_verified: false` -> refused, whoever asked + * - `email_verified: true` -> believed, always + * - claim ABSENT + operator-chosen -> believed (Microsoft, or an opted-in provider) + * - claim ABSENT + org with a VERIFIED domain -> believed (it proved DNS control) + * - claim ABSENT + org with NO verified domain -> refused (it has proven nothing) + * - `email_verified: false` -> refused, whoever asked + * + * The org case is a consequence of DNS proof, never a setting: an organization must not be able to + * turn it on for itself, so it is derived and never read from a column. */ const { test } = require('node:test'); @@ -22,10 +26,18 @@ const { emailIsVerified, list } = require('../lib/oidc-providers'); const MS_ENV = { MICROSOFT_CLIENT_ID: 'client-abc', MICROSOFT_TENANT_ID: 'ffffffff-1111-2222-3333-444444444444' }; const microsoft = () => list(MS_ENV).find((p) => p.slug === 'microsoft'); const google = () => list({ GOOGLE_CLIENT_ID: 'g-abc' }).find((p) => p.slug === 'google'); -const orgProvider = { slug: 'acme7f3', source: 'org', organizationId: 'org-1', assumeEmailVerified: false }; +// An org provider as rowToProvider builds it: `assumeEmailVerified` follows from whether any domain +// has actually been DNS-verified. +const orgProvider = (verifiedDomains = []) => ({ + slug: 'acme7f3', source: 'org', organizationId: 'org-1', + emailDomains: verifiedDomains.join(','), + assumeEmailVerified: verifiedDomains.length > 0, +}); +const orgUnproven = orgProvider(); // configured, nothing verified yet +const orgProven = orgProvider(['bytetinker.net']); // TXT published, domain green test('an explicit true is believed from any provider', () => { - for (const p of [microsoft(), google(), orgProvider]) { + for (const p of [microsoft(), google(), orgUnproven, orgProven]) { assert.equal(emailIsVerified({ email_verified: true }, p), true, `${p.slug} should accept an explicit true`); } }); @@ -42,14 +54,26 @@ test('Google stays strict — it does send the claim, so there is nothing to ass assert.equal(emailIsVerified({ email: 'someone@example.com' }, g), false); }); -test('an ORG-configured provider may never assume, even if the object claims it can', () => { - assert.equal(emailIsVerified({ email: 'a@b.c' }, orgProvider), false); - // Belt and braces: a tampered/hand-built org object must not be able to opt itself in through - // the database, which is why rowToProvider pins the field rather than reading a column. +test('an org provider that has proven a domain may assume — the customer-Entra case', () => { + // Entra sends no email_verified. Before this, the domain went green and the login still failed. + assert.equal(emailIsVerified({ email: 'dan@bytetinker.net' }, orgProven), true); +}); + +test('an org provider that has proven NOTHING assumes nothing', () => { + assert.equal(emailIsVerified({ email: 'dan@bytetinker.net' }, orgUnproven), false); +}); + +test('the org assumption is DERIVED from proof, never readable from the row', () => { const src = require('fs').readFileSync(require.resolve('../lib/oidc-providers'), 'utf8'); - assert.match(src, /assumeEmailVerified: false,\s*\n\s*source: 'org'/, - 'rowToProvider must hard-code assumeEmailVerified:false next to source:org'); + assert.match(src, /assumeEmailVerified: verified\.length > 0,\s*\n\s*source: 'org'/, + 'rowToProvider must derive it from the verified-domain list, next to source:org'); + // The whole point: an organization must not be able to switch this on for itself. assert.doesNotMatch(src, /assumeEmailVerified: *row\./, 'must never be read from the org row'); + // ...and there must be no column for it to be read FROM. Checked against the schema rather than + // this file, where `OIDC__ASSUME_EMAIL_VERIFIED` is a legitimate operator-set env var. + const schema = require('fs').readFileSync(require.resolve('../db/database'), 'utf8'); + assert.doesNotMatch(schema, /assume_email_verified/i, + 'org_sso_providers must have no assume_email_verified column'); }); test('an EXPLICIT false is refused even where absence would be assumed', () => {