diff --git a/CHANGELOG.md b/CHANGELOG.md index 06a05fe..26468db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,32 @@ # Changelog +## v1.8.3 + +- **`supertokens:check` no longer reports a correctly-secured core as wide + open.** The API-key probe used a single guessed path, + `/recipe/users/count`, which SuperTokens core 12 does not implement — the + path is tenant-scoped (`/public/users/count`). Every probe returned 404, and + the check read "not 401" as "not secured", producing: + + > The core is running without API_KEYS: anyone who can reach it can mint a + > login session for any user id + + …against a core that was refusing anonymous callers correctly. It also + blamed `SUPERTOKENS_API_KEY` for the same 404. + + The probe now tries the tenant-scoped path first and falls back to the legacy + one, and — more importantly — **a 404 is reported as "could not determine", + never as an open core**. A security check that cries wolf is worse than no + check, because the next real warning gets ignored too. + +- The core's telemetry is disabled by default in `docker-compose.yml`. It phones + home to `api.supertokens.io`, whose certificate chains to the new + `ISRG Root YR` root that the core image's JVM truststore does not carry, + logging `SSLHandshakeException: PKIX path building failed` on every boot. The + error is non-fatal, but it is noise in the log of the component that signs + every session. + + ## v1.8.2 - **`npm run supertokens:check`** — a deployment preflight for the SuperTokens diff --git a/Dockerfile b/Dockerfile index 9143946..1e6b16e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -44,7 +44,7 @@ LABEL org.opencontainers.image.licenses="MIT" # only on a pushed vX.Y.Z tag, and docker/metadata-action derives the # published image's version label from that tag - so this literal only # affects locally-built images, not what GHCR publishes. -LABEL org.opencontainers.image.version="1.8.2" +LABEL org.opencontainers.image.version="1.8.3" VOLUME ["/app/data"] EXPOSE 3000 diff --git a/docker-compose.yml b/docker-compose.yml index 420ea23..6ff98f3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -104,6 +104,20 @@ services: # instead (see initSuperTokens), where it can fire only for operators who # have actually opted in. API_KEYS: ${SUPERTOKENS_API_KEY:-} + # The core phones home to api.supertokens.io on startup unless this is + # set. Off by default here for two reasons: + # + # 1. A self-hosted game server has no reason to report usage anywhere. + # 2. It is the fix for + # `javax.net.ssl.SSLHandshakeException: PKIX path building failed` + # in the core's log. api.supertokens.io's certificate chains to + # ISRG Root YR, a new Let's Encrypt root that the JVM truststore + # baked into the core image does not carry yet - so the call fails + # on every boot. It is non-fatal (the core starts and serves + # normally), but it is alarming noise in the log of the component + # that signs your sessions, and there is no reason to make the call + # at all. + DISABLE_TELEMETRY: ${SUPERTOKENS_DISABLE_TELEMETRY:-true} healthcheck: test: ["CMD-SHELL", "bash -c ':> /dev/tcp/127.0.0.1/3567' || exit 1"] interval: 10s diff --git a/docs/supertokens-rollout-runbook.md b/docs/supertokens-rollout-runbook.md index ffe5692..ed3fbb0 100644 --- a/docs/supertokens-rollout-runbook.md +++ b/docs/supertokens-rollout-runbook.md @@ -275,8 +275,18 @@ publish port 3567**: ``` POSTGRESQL_CONNECTION_URI=postgresql://rackstack_user:PASSWORD@192.168.x.x:5432/supertokens API_KEYS= +DISABLE_TELEMETRY=true ``` +> `DISABLE_TELEMETRY` is not required, but set it. A self-hosted game server has +> no reason to report usage anywhere, and leaving it on produces +> `javax.net.ssl.SSLHandshakeException: PKIX path building failed` in the core's +> log on every boot — `api.supertokens.io` chains to `ISRG Root YR`, the same +> new Let's Encrypt root the image-pull problem comes from, and the JVM +> truststore inside the core image does not carry it. **The error is non-fatal** +> — the core starts and serves normally — but it is alarming noise in the log +> of the component that signs your sessions. + > **Two things about that image reference.** > > **Pin the major, let the minor float — `:12`, not `:latest` and not @@ -593,4 +603,6 @@ gone wrong and will send you chasing the wrong problem. | `shadow:check` says the database predates the v1.7 split | You restored a pre-v1.7 export. Migrate it to v1.7 first, or point at the right database. | | `shadow:check` reports `ORPHAN` rows | An identity points at a user that does not exist; that player cannot log in. Investigate before cutting over — do not ignore it. | | Pulling the core fails with `x509: certificate signed by unknown authority` | Not an outage. The SuperTokens registry chains to `ISRG Root YR`, a new Let's Encrypt root your CA bundle lacks. Pull `supertokens/supertokens-postgresql:12` from Docker Hub instead, or update the host's `ca-certificates`. Confirm which by running `openssl s_client -connect registry.supertokens.io:443 -servername registry.supertokens.io /users/count`, per supertokens-node's own querier) and was not + * on older ones. The first version of this shipped a single guessed path, + * `/recipe/users/count`, which does not exist on core 12 - so every probe came + * back 404 and the check reported a correctly-locked-down core as running wide + * open. See the 404 handling below: that false alarm is the reason this is a + * list and not a constant. */ -const AUTHED_ENDPOINT = '/recipe/users/count'; +const AUTHED_ENDPOINTS = Object.freeze([ + '/public/users/count', // core with multitenancy (the default tenant) + '/recipe/users/count', // older cores +]); async function probe(url, { apiKey, fetchImpl, timeoutMs = 5000 }) { const headers = { 'api-version': '3.0' }; @@ -40,6 +51,23 @@ async function probe(url, { apiKey, fetchImpl, timeoutMs = 5000 }) { return fetchImpl(url, { method: 'GET', headers, signal: AbortSignal.timeout(timeoutMs) }); } +/** + * Probes the first endpoint this core actually implements. + * + * Returns `{ status, path }`, or `{ status: null }` when every candidate 404s - + * which means "this core does not expose any path we know how to ask", NOT + * "the core answered". The distinction is the whole point: a 404 is evidence + * about our URL, not about the core's authentication. + */ +async function probeAuthedEndpoint({ connectionURI, apiKey, fetchImpl }) { + for (const path of AUTHED_ENDPOINTS) { + // eslint-disable-next-line no-await-in-loop + const res = await probe(`${connectionURI}${path}`, { apiKey, fetchImpl }); + if (res.status !== 404) return { status: res.status, path }; + } + return { status: null, path: null }; +} + /** * Runs every deployment check and returns the results. * @@ -129,17 +157,35 @@ export async function runPreflight({ // single request touching RackStack. Nothing about this fails visibly, which // is exactly why it is checked here rather than left to be noticed. try { - const anon = await probe(`${connectionURI}${AUTHED_ENDPOINT}`, { fetchImpl }); - if (anon.status === 401) { - checks.push(result(PASS, 'core requires authentication', 'anonymous request rejected (401)')); - } else { + const anon = await probeAuthedEndpoint({ connectionURI, fetchImpl }); + if (anon.status === 401 || anon.status === 403) { + checks.push(result(PASS, 'core requires authentication', `anonymous request rejected (${anon.status})`)); + } else if (anon.status === 200) { checks.push(result( FAIL, 'core requires authentication', - `An anonymous request got HTTP ${anon.status}. The core is running without API_KEYS: ` + `An anonymous request to ${anon.path} succeeded. The core is running without API_KEYS: ` + 'anyone who can reach it can mint a login session for any user id, including every ' + 'value in SUPER_ADMIN_IDS. Set API_KEYS on the core (openssl rand -hex 32), set the ' + "same value as SUPERTOKENS_API_KEY here, and do not publish the core's port.", )); + } else if (anon.status === null) { + // Every candidate path 404'd. That says our URL is wrong for this core + // version, and nothing whatsoever about its authentication - so it must + // NOT be reported as an open core. Saying "wide open" on the strength of + // a 404 is precisely the false alarm this check shipped with, and a + // security check that cries wolf is worse than one that admits it does + // not know. + checks.push(result( + WARN, 'core requires authentication', + 'Could not determine: this core exposes none of the endpoints used to test it ' + + `(${AUTHED_ENDPOINTS.join(', ')} all returned 404). This is NOT evidence the core is ` + + "open - verify by hand that an unkeyed request is refused, and check the core's log.", + )); + } else { + checks.push(result( + WARN, 'core requires authentication', + `Could not determine: an anonymous request returned an unexpected HTTP ${anon.status}.`, + )); } } catch (e) { checks.push(result(WARN, 'core requires authentication', `could not verify (${e.message})`)); @@ -190,14 +236,28 @@ export async function runPreflight({ )); } else { try { - const authed = await probe(`${connectionURI}${AUTHED_ENDPOINT}`, { apiKey, fetchImpl }); - checks.push(authed.status === 200 - ? result(PASS, 'SUPERTOKENS_API_KEY', 'accepted by the core') - : result( + const authed = await probeAuthedEndpoint({ connectionURI, apiKey, fetchImpl }); + if (authed.status === 200) { + checks.push(result(PASS, 'SUPERTOKENS_API_KEY', 'accepted by the core')); + } else if (authed.status === 401 || authed.status === 403) { + checks.push(result( FAIL, 'SUPERTOKENS_API_KEY', `The core rejected it (HTTP ${authed.status}). It must be byte-identical to a value in ` + "the core's API_KEYS. A mismatch here fails every login once AUTH_MODE is set.", )); + } else { + // Same reasoning as above: a 404, or anything else unexpected, is + // evidence about the URL rather than about the key. Reporting "the + // core rejected it" would send an operator to re-check a key that was + // never the problem. + checks.push(result( + WARN, 'SUPERTOKENS_API_KEY', + authed.status === null + ? 'Could not determine: this core exposes none of the endpoints used to test it. ' + + 'The key may well be correct.' + : `Could not determine: unexpected HTTP ${authed.status}.`, + )); + } } catch (e) { checks.push(result(WARN, 'SUPERTOKENS_API_KEY', `could not verify (${e.message})`)); } diff --git a/tests/supertokens.preflight.test.js b/tests/supertokens.preflight.test.js index 0f16bd8..347b1e6 100644 --- a/tests/supertokens.preflight.test.js +++ b/tests/supertokens.preflight.test.js @@ -22,8 +22,21 @@ const BASE_ENV = { }; /** A fake core. `open: true` models one running with no API_KEYS. */ +/** + * A fake core. + * + * `countPath` is which tenant-scoped path this core version implements; + * anything else 404s, exactly as a real core does. That detail matters: the + * first release of this preflight probed a single guessed path that core 12 + * does not have, so every probe 404'd and a correctly-locked-down core was + * reported as running wide open. + */ function fakeCore({ - open = false, keyAccepted = true, reachable = true, cdi = ['5.3', '5.4', '5.5'], + open = false, + keyAccepted = true, + reachable = true, + cdi = ['5.3', '5.4', '5.5'], + countPath = '/public/users/count', } = {}) { return async (url, { headers } = {}) => { if (!reachable) throw new Error('ECONNREFUSED'); @@ -31,6 +44,7 @@ function fakeCore({ if (url.endsWith('/apiversion')) { return { status: 200, json: async () => ({ versions: cdi }) }; } + if (countPath === null || !url.endsWith(countPath)) return { status: 404 }; const hasKey = Boolean(headers && headers['api-key']); if (!hasKey) return { status: open ? 200 : 401 }; return { status: keyAccepted ? 200 : 401 }; @@ -76,7 +90,11 @@ describe('the preflight catches an open core', () => { }); const anonProbes = probed.filter((p) => !p.keyed && !p.url.endsWith('/hello')); expect(anonProbes.length).toBeGreaterThan(0); - expect(anonProbes.every((p) => p.url.includes('/recipe/'))).toBe(true); + // An endpoint that is actually gated by the API key - the specific path + // varies by core version, which is why this asserts the property rather + // than a literal (the literal is what broke in v1.8.2). + expect(anonProbes.every((p) => p.url.includes('users/count'))).toBe(true); + expect(anonProbes.every((p) => !p.url.endsWith('/hello'))).toBe(true); }); }); @@ -123,6 +141,83 @@ describe('the preflight catches a core too old for the SDK', () => { }); }); +describe('a 404 is never reported as an open core', () => { + // The regression that shipped in v1.8.2. The probe used a single guessed + // path, `/recipe/users/count`, which core 12 does not implement. Every probe + // came back 404, "not 401" was read as "open", and a properly locked-down + // core was reported as: "The core is running without API_KEYS: anyone who + // can reach it can mint a login session for any user id." + // + // A security check that cries wolf is worse than no check, because the next + // real warning gets ignored too. + + it('uses the tenant-scoped path a modern core actually implements', async () => { + const checks = await runPreflight({ + env: BASE_ENV, + fetchImpl: fakeCore({ countPath: '/public/users/count' }), + pgConnect: noStrayTables, + }); + expect(byName(checks, 'core requires authentication').status).toBe('PASS'); + expect(byName(checks, 'SUPERTOKENS_API_KEY').status).toBe('PASS'); + expect(preflightPassed(checks)).toBe(true); + }); + + it('still works against an older core using the legacy path', async () => { + const checks = await runPreflight({ + env: BASE_ENV, + fetchImpl: fakeCore({ countPath: '/recipe/users/count' }), + pgConnect: noStrayTables, + }); + expect(byName(checks, 'core requires authentication').status).toBe('PASS'); + expect(byName(checks, 'SUPERTOKENS_API_KEY').status).toBe('PASS'); + }); + + it('WARNs, and does NOT claim the core is open, when every path 404s', async () => { + const checks = await runPreflight({ + env: BASE_ENV, + fetchImpl: fakeCore({ countPath: null }), + pgConnect: noStrayTables, + }); + + const auth = byName(checks, 'core requires authentication'); + expect(auth.status).toBe('WARN'); + expect(auth.detail).toMatch(/NOT evidence the core is open/i); + expect(auth.detail).not.toMatch(/running without API_KEYS/); + + // And the key check must not blame a key that was never the problem. + const key = byName(checks, 'SUPERTOKENS_API_KEY'); + expect(key.status).toBe('WARN'); + expect(key.detail).not.toMatch(/rejected/); + }); + + it('a WARN does not block the cutover, but a genuine open core still does', async () => { + const unknown = await runPreflight({ + env: BASE_ENV, fetchImpl: fakeCore({ countPath: null }), pgConnect: noStrayTables, + }); + expect(preflightPassed(unknown)).toBe(true); + + const reallyOpen = await runPreflight({ + env: BASE_ENV, fetchImpl: fakeCore({ open: true }), pgConnect: noStrayTables, + }); + expect(preflightPassed(reallyOpen)).toBe(false); + expect(byName(reallyOpen, 'core requires authentication').detail).toMatch(/without API_KEYS/); + }); + + it('treats 403 like 401 - refused is refused', async () => { + const checks = await runPreflight({ + env: BASE_ENV, + fetchImpl: async (url, o) => { + if (url.endsWith('/hello')) return { status: 200 }; + if (url.endsWith('/apiversion')) return { status: 200, json: async () => ({ versions: ['5.4'] }) }; + if (!url.endsWith('/public/users/count')) return { status: 404 }; + return { status: o?.headers?.['api-key'] ? 200 : 403 }; + }, + pgConnect: noStrayTables, + }); + expect(byName(checks, 'core requires authentication').status).toBe('PASS'); + }); +}); + describe('the preflight catches a key mismatch', () => { it('FAILS when the core rejects our key', async () => { const checks = await runPreflight({