diff --git a/.env.example b/.env.example index a0b4e2e..acd0192 100644 --- a/.env.example +++ b/.env.example @@ -48,6 +48,9 @@ CLICKHOUSE_DASHBOARD_PASSWORD= # yavio_dashboard user: SELECT only, ro CLICKHOUSE_ERASER_PASSWORD= # yavio_eraser user: ALTER DELETE on default.events ONLY — used # by the account/workspace/project deletion routes. Cannot read # what it erases. Applied by `pnpm migrate:clickhouse`. + # UPGRADING: add this line if your .env predates it. Left blank, + # the user stays unauthenticatable and deletion falls back to the + # CLICKHOUSE_URL superuser — which is what this user exists to stop. # ─── Database Connection URLs ──────────────────────────────────────────────── # localhost for local dev; Docker Compose overrides these with container hostnames. diff --git a/packages/dashboard/lib/clickhouse.ts b/packages/dashboard/lib/clickhouse.ts index 9164054..4d8eebb 100644 --- a/packages/dashboard/lib/clickhouse.ts +++ b/packages/dashboard/lib/clickhouse.ts @@ -88,7 +88,10 @@ export function getMutatingClickHouseClient(): ClickHouseClient { // Falls back to the CLICKHOUSE_URL user when unset, so a deployment that // has not yet run 0012 or set the password keeps erasing rather than // silently failing — which is the failure mode this whole file exists to - // prevent. + // prevent. That fallback is the superuser, so leaving the variable unset + // keeps the very credential 0012 set out to retire: yavio_eraser is created + // with an unusable credential until a password is supplied, and + // migrate-clickhouse.ts warns when it takes that branch. const eraserPassword = process.env.CLICKHOUSE_ERASER_PASSWORD; mutatingClient = createClickHouseClient( eraserPassword diff --git a/packages/db/migrations/clickhouse/0012_eraser_user.sql b/packages/db/migrations/clickhouse/0012_eraser_user.sql index cf31aca..b0046a1 100644 --- a/packages/db/migrations/clickhouse/0012_eraser_user.sql +++ b/packages/db/migrations/clickhouse/0012_eraser_user.sql @@ -14,15 +14,30 @@ -- No SELECT, no INSERT, no DDL, nothing on any other table. It cannot read the -- data it is allowed to erase. -- --- NO PASSWORD IS SET HERE, deliberately. Migration 0007 created its siblings --- with the literal 'yavio_dev', which is published in this public repository and --- is exactly the defect the 2026-08-05 work had to unwind. A user with no --- password cannot authenticate, so this fails closed: the operator sets --- CLICKHOUSE_ERASER_PASSWORD (scripts/setup-env.sh generates it) and --- migrate-clickhouse.ts applies it after migrations run, the same way +-- NO USABLE PASSWORD IS SET HERE, deliberately. Migration 0007 created its +-- siblings with the literal 'yavio_dev', which is published in this public +-- repository and is exactly the defect the 2026-08-05 work had to unwind. The +-- operator sets CLICKHOUSE_ERASER_PASSWORD (scripts/setup-env.sh generates it) +-- and migrate-clickhouse.ts applies it after migrations run, the same way -- CLICKHOUSE_INGEST_PASSWORD and CLICKHOUSE_DASHBOARD_PASSWORD are applied. +-- +-- AMENDED 2026-08-06. This statement originally read `IDENTIFIED WITH +-- no_password`, on the belief that an account with no password cannot +-- authenticate. That is Postgres behaviour. In ClickHouse `no_password` means no +-- credential is REQUIRED — the check succeeds for any password, including a +-- wrong one — so the original form failed open, not closed. sha256_hash takes a +-- digest whose preimage was never generated, which is the state the comment +-- above always intended. +-- +-- Deployments that already applied the original 0012 are not re-run by the +-- migrator, so this amendment does not reach them. repairPasswordlessUsers() in +-- src/clickhouse-credentials.ts repairs those instead, and does so ONLY when the +-- account is actually passwordless — a second migration ALTERing every +-- deployment unconditionally would reset working credentials, and a broken +-- eraser fails silently (the deletion routes log and still return 200). -CREATE USER IF NOT EXISTS yavio_eraser IDENTIFIED WITH no_password; +CREATE USER IF NOT EXISTS yavio_eraser + IDENTIFIED WITH sha256_hash BY '322464e430fa3579779f1c4b82b59b559c50126dccad25f347635cc480d07a33'; -- ALTER DELETE is the privilege ClickHouse checks for `ALTER TABLE ... DELETE` -- (a lightweight mutation). Granting it alone means this identity can remove diff --git a/packages/db/src/__tests__/ch-migrations.test.ts b/packages/db/src/__tests__/ch-migrations.test.ts index 57f93d0..c523240 100644 --- a/packages/db/src/__tests__/ch-migrations.test.ts +++ b/packages/db/src/__tests__/ch-migrations.test.ts @@ -1,6 +1,44 @@ import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { + assertNoPasswordlessUsers, + readManagedUserAuth, + repairPasswordlessUsers, +} from "../clickhouse-credentials.js"; +import { MANAGED_USER_NAMES, UNUSABLE_PASSWORD_HASH } from "../migrate-clickhouse-helpers.js"; import { disconnect, dropAll, getClient, runMigrations } from "./helpers/clickhouse.js"; +/** + * The HTTP endpoint with any userinfo stripped, so the tests below can present + * their own credentials. Authenticating over the wire is the only way to show + * what `no_password` really does — system.users reports the method, not the + * behaviour, and it was a belief about the behaviour that shipped the defect. + */ +const CLICKHOUSE_HTTP = (() => { + const url = new URL(process.env.CLICKHOUSE_URL ?? "http://localhost:8123"); + url.username = ""; + url.password = ""; + return url.toString(); +})(); + +/** + * Try to authenticate as yavio_eraser over HTTP and report what came back. + * + * Reads the BODY, not just the status. ClickHouse answers a rejected credential + * with HTTP 403 and puts `Code: 516 ... Authentication failed` in the body, so + * asserting on a 516 status would be an assertion that can never fail — which + * is the whole failure mode this test file exists to close. + */ +async function attemptAuth(password: string): Promise<{ status: number; body: string }> { + const res = await fetch(CLICKHOUSE_HTTP, { + method: "POST", + headers: { + Authorization: `Basic ${Buffer.from(`yavio_eraser:${password}`).toString("base64")}`, + }, + body: "SELECT 1 FORMAT TSV", + }); + return { status: res.status, body: await res.text() }; +} + describe("ClickHouse migrations", () => { beforeAll(async () => { await dropAll().catch(() => {}); // Ignore errors if tables don't exist yet @@ -85,6 +123,112 @@ describe("ClickHouse migrations", () => { }); }); + describe("credentials fail closed", () => { + // The test environment sets no CLICKHOUSE_*_PASSWORD, so this exercises the + // exact path that shipped the defect: migrations run, no password is + // applied, and the accounts must still be unreachable. + // + // These call the SHIPPED functions rather than re-implementing their + // predicates. A test that reproduces the logic it is checking proves the + // database state and nothing about the code that is supposed to enforce it. + it("leaves no managed user authenticating without a credential", async () => { + await expect(assertNoPasswordlessUsers(getClient(), true)).resolves.not.toThrow(); + }); + + it("sees every managed user, so the assertion above cannot pass vacuously", async () => { + const auth = await readManagedUserAuth(getClient()); + expect([...auth.keys()].sort()).toEqual([...MANAGED_USER_NAMES].sort()); + }); + + it("grants yavio_eraser row deletion on events and nothing else", async () => { + const ch = getClient(); + const grants = await ch.query({ + query: + "SELECT access_type, database, table, grant_option FROM system.grants WHERE user_name = 'yavio_eraser' ORDER BY access_type", + format: "JSONEachRow", + }); + expect(await grants.json()).toEqual([ + { access_type: "ALTER DELETE", database: "default", table: "events", grant_option: 0 }, + ]); + + // Direct grants are only half the picture: a role would carry its own + // privileges in, and "nothing else" has to mean that too. + const roles = await ch.query({ + query: "SELECT granted_role_name FROM system.role_grants WHERE user_name = 'yavio_eraser'", + format: "JSONEachRow", + }); + expect(await roles.json()).toEqual([]); + }); + }); + + describe("repair of an already-passwordless user", () => { + // The upgrade path is the whole reason repairPasswordlessUsers exists, and + // CI always starts from a fresh container where migration 0012 alone + // satisfies every assertion above. Without this, deleting the repair + // entirely would leave the suite green. + beforeAll(async () => { + const ch = getClient(); + await ch.command({ query: "DROP USER IF EXISTS yavio_eraser" }); + // Exactly what migration 0012 shipped before it was amended. + await ch.command({ query: "CREATE USER yavio_eraser IDENTIFIED WITH no_password" }); + await ch.command({ query: "GRANT ALTER DELETE ON default.events TO yavio_eraser" }); + }); + + it("reproduces the defect: the account authenticates with a WRONG password", async () => { + // Not a rhetorical step. `no_password` reads like "cannot log in", and + // the whole defect was believing that. This pins the real behaviour. + const { status, body } = await attemptAuth("definitely-not-the-password"); + expect(status).toBe(200); + expect(body).not.toMatch(/Authentication failed/); + }); + + it("detects it", async () => { + await expect(assertNoPasswordlessUsers(getClient(), true)).rejects.toThrow( + /authenticate with NO credential/, + ); + }); + + it("repairs it, and reports which user it repaired", async () => { + await expect(repairPasswordlessUsers(getClient())).resolves.toEqual(["yavio_eraser"]); + await expect(assertNoPasswordlessUsers(getClient(), true)).resolves.not.toThrow(); + }); + + it("leaves the repaired account unauthenticatable — including by its own published hash", async () => { + for (const password of ["", "definitely-not-the-password", UNUSABLE_PASSWORD_HASH]) { + const { status, body } = await attemptAuth(password); + expect(status).toBe(403); + // Presenting the published digest itself must fail too — that is what + // makes it safe to ship the constant in a public repository. + expect(body).toMatch(/Authentication failed/); + } + }); + + it("preserves the grant, so a real password still yields a working eraser", async () => { + const result = await getClient().query({ + query: + "SELECT access_type FROM system.grants WHERE user_name = 'yavio_eraser' AND table = 'events'", + format: "JSONEachRow", + }); + expect(await result.json()).toEqual([{ access_type: "ALTER DELETE" }]); + }); + + it("is a no-op on a user that already holds a real password", async () => { + const ch = getClient(); + await ch.command({ + query: "ALTER USER yavio_eraser IDENTIFIED WITH sha256_password BY 'a-working-password'", + }); + // The destructive version of this repair lived in a migration and reset + // every deployment's credential unconditionally. A broken eraser fails + // silently — the deletion routes log and still return 200 — so "does not + // touch a working account" is the property that matters most here. + await expect(repairPasswordlessUsers(ch)).resolves.toEqual([]); + + const { status, body } = await attemptAuth("a-working-password"); + expect(status).toBe(200); + expect(body).not.toMatch(/Authentication failed/); + }); + }); + describe("idempotent re-run", () => { it("running migrations a second time produces no errors", async () => { await expect(runMigrations()).resolves.not.toThrow(); diff --git a/packages/db/src/__tests__/helpers/clickhouse.ts b/packages/db/src/__tests__/helpers/clickhouse.ts index da83588..a261476 100644 --- a/packages/db/src/__tests__/helpers/clickhouse.ts +++ b/packages/db/src/__tests__/helpers/clickhouse.ts @@ -2,7 +2,11 @@ import { readFile, readdir } from "node:fs/promises"; import { dirname, join } from "node:path"; import { fileURLToPath } from "node:url"; import { type ClickHouseClient, createClient } from "@clickhouse/client"; -import { splitStatements, versionFromFilename } from "../../migrate-clickhouse-helpers.js"; +import { + MANAGED_USER_NAMES, + splitStatements, + versionFromFilename, +} from "../../migrate-clickhouse-helpers.js"; const CLICKHOUSE_URL = process.env.CLICKHOUSE_URL ?? "http://localhost:8123"; const CLICKHOUSE_PASSWORD = process.env.CLICKHOUSE_PASSWORD ?? "test"; @@ -77,7 +81,17 @@ export async function runMigrations() { } } -/** Drop all ClickHouse tables and views to reset state. */ +/** + * Drop all ClickHouse tables, views AND managed users to reset state. + * + * The users matter as much as the tables. They are server-level objects that + * outlive a table drop, and `CREATE USER IF NOT EXISTS` in the migrations is a + * no-op once they exist — so against a persistent ClickHouse (a developer + * pointing the suite at the dev container, which is what CLICKHOUSE_URL + * defaults to) the credential tests would assert against accounts created weeks + * ago with unknown authentication, and could not tell "the migrations did this" + * from "something did this once". + */ export async function dropAll() { const ch = getClient(); for (const obj of [ @@ -89,6 +103,9 @@ export async function dropAll() { ]) { await ch.command({ query: `DROP ${obj}` }); } + for (const user of MANAGED_USER_NAMES) { + await ch.command({ query: `DROP USER IF EXISTS ${user}` }); + } } /** Close the ClickHouse client. */ diff --git a/packages/db/src/clickhouse-credentials.ts b/packages/db/src/clickhouse-credentials.ts new file mode 100644 index 0000000..4589279 --- /dev/null +++ b/packages/db/src/clickhouse-credentials.ts @@ -0,0 +1,110 @@ +import { ErrorCode, YavioError } from "@yavio/shared/errors"; +import type { createClickHouseClient } from "./clickhouse-client.js"; +import { + MANAGED_USER_NAMES, + UNUSABLE_PASSWORD_HASH, + isPasswordless, +} from "./migrate-clickhouse-helpers.js"; + +type Client = ReturnType; + +/** + * Read the authentication method of every managed user that exists. + * + * Returns only the users ClickHouse actually reported, so callers can tell + * "this user is fine" apart from "I could not see this user" — which is the + * distinction that decides whether a check has run at all. + */ +export async function readManagedUserAuth(client: Client): Promise> { + const result = await client.query({ + query: "SELECT name, auth_type FROM system.users WHERE name IN {users:Array(String)}", + query_params: { users: MANAGED_USER_NAMES }, + format: "JSONEachRow", + }); + const rows = await result.json<{ name: string; auth_type: string | string[] }>(); + return new Map(rows.map((r) => [r.name, r.auth_type])); +} + +/** + * Put any managed user that authenticates without a credential back into a + * state where it authenticates nobody. + * + * CONDITIONAL ON PURPOSE. The obvious implementation is a migration that ALTERs + * the user unconditionally, and that is worse than it looks: it would reset a + * WORKING credential on every existing deployment and rely on applyUserPasswords + * in the same process to put it back. Any environment drift between the migrator + * and the dashboard would then leave the eraser unreachable — and the deletion + * routes catch ClickHouse failures, log, and still return 200, so the operator + * would see successful account deletions while the events were silently + * retained. Repairing only the broken state cannot do that: a user already on a + * real password is never touched. + * + * Returns the users it repaired, so the caller can report what happened rather + * than assert it. + */ +export async function repairPasswordlessUsers(client: Client): Promise { + const authByUser = await readManagedUserAuth(client); + const repaired: string[] = []; + + for (const [user, authType] of authByUser) { + if (!isPasswordless(authType)) continue; + await client.command({ + query: `ALTER USER ${user} IDENTIFIED WITH sha256_hash BY '${UNUSABLE_PASSWORD_HASH}'`, + }); + repaired.push(user); + } + + return repaired; +} + +/** + * Refuse to finish while any managed user can be authenticated into without a + * credential. + * + * This is the check that would have caught the 0012 defect, which created + * yavio_eraser with `IDENTIFIED WITH no_password` believing that could not + * authenticate. In ClickHouse it authenticates with anything at all. + * + * Note why the rollout check written for 0012 could not have caught it: it + * verified that yavio_eraser CAN authenticate, and a no_password account + * authenticates with whatever credential you present — including the one you + * believe you just set. A check that cannot fail proves nothing. + * + * Which is why this one refuses to pass vacuously. `SELECT ... FROM + * system.users` is access-filtered rather than error-raising: a connection + * without SHOW USERS sees only its own row and returns success, so "no rows" + * would read as "nothing wrong" when it actually means "I saw nothing". + * + * Seeing even one managed user proves system.users is visible, and a user that + * is genuinely absent is already fail-closed — no account, no access. So the + * case worth refusing is the one where the caller expected users and this + * inspected none. + */ +export async function assertNoPasswordlessUsers( + client: Client, + expectUsersToExist = false, +): Promise { + const authByUser = await readManagedUserAuth(client); + + if (expectUsersToExist && authByUser.size === 0) { + throw new YavioError( + ErrorCode.DB.CH_MIGRATION_FAILED, + `Cannot verify ClickHouse credentials: migrations created ${MANAGED_USER_NAMES.join(", ")}, but none of them are visible in system.users. The migrating user most likely lacks SHOW USERS, which would make this check pass without inspecting anything.`, + 500, + { users: MANAGED_USER_NAMES }, + ); + } + + const passwordless = [...authByUser] + .filter(([, authType]) => isPasswordless(authType)) + .map(([user]) => user); + + if (passwordless.length > 0) { + throw new YavioError( + ErrorCode.DB.CH_MIGRATION_FAILED, + `ClickHouse user(s) ${passwordless.join(", ")} authenticate with NO credential. In ClickHouse \`no_password\` means no credential is required — any password is accepted, including a wrong one. Set the matching CLICKHOUSE_*_PASSWORD (scripts/setup-env.sh generates them) and re-run this migration.`, + 500, + { users: passwordless }, + ); + } +} diff --git a/packages/db/src/migrate-clickhouse-helpers.ts b/packages/db/src/migrate-clickhouse-helpers.ts index 0229c12..6289174 100644 --- a/packages/db/src/migrate-clickhouse-helpers.ts +++ b/packages/db/src/migrate-clickhouse-helpers.ts @@ -39,3 +39,45 @@ export function splitStatements(sql: string): string[] { .map((s) => s.trim()) .filter((s) => s.length > 0); } + +/** + * The ClickHouse users whose credentials this migrator owns, paired with the + * environment variable that supplies each one. + * + * Single source of truth: applyUserPasswords iterates it, the passwordless + * repair and assertion derive their user list from it, and `warnWhenUnset` + * keeps the per-user policy next to the user rather than in an `if` somewhere + * downstream. Adding a user here is all it takes to bring it under the guard. + */ +export const MANAGED_USERS = [ + { user: "yavio_ingest", envVar: "CLICKHOUSE_INGEST_PASSWORD", warnWhenUnset: false }, + { user: "yavio_dashboard", envVar: "CLICKHOUSE_DASHBOARD_PASSWORD", warnWhenUnset: false }, + // Worth saying out loud: with no password the dashboard erases as the + // CLICKHOUSE_URL superuser, which is the arrangement migration 0012 exists to + // end. The other two are commonly left unset by deployments that share one + // password, so warning on those would be noise. + { user: "yavio_eraser", envVar: "CLICKHOUSE_ERASER_PASSWORD", warnWhenUnset: true }, +] as const; + +export const MANAGED_USER_NAMES = MANAGED_USERS.map((u) => u.user); + +/** + * A credential that authenticates nobody: a SHA-256 digest whose preimage was + * never generated. Publishing it leaks nothing — it is a hash, not a password, + * and a test asserts that presenting the digest itself is rejected. + * + * This exists because ClickHouse has no "disabled" authentication state. + * `no_password` is the trap: it means no credential is REQUIRED, so it accepts + * an empty password AND a wrong one. + */ +export const UNUSABLE_PASSWORD_HASH = + "322464e430fa3579779f1c4b82b59b559c50126dccad25f347635cc480d07a33"; + +/** + * `system.users.auth_type` is a single Enum on ClickHouse 24.3 and an Array + * once multiple authentication methods per user landed. Accept both rather + * than pinning to one server version. + */ +export function isPasswordless(authType: string | string[]): boolean { + return (Array.isArray(authType) ? authType : [authType]).includes("no_password"); +} diff --git a/packages/db/src/migrate-clickhouse.ts b/packages/db/src/migrate-clickhouse.ts index 4d873c4..b331168 100644 --- a/packages/db/src/migrate-clickhouse.ts +++ b/packages/db/src/migrate-clickhouse.ts @@ -3,7 +3,12 @@ import { join } from "node:path"; import { fileURLToPath } from "node:url"; import { ErrorCode, YavioError } from "@yavio/shared/errors"; import { createClickHouseClient } from "./clickhouse-client.js"; -import { splitStatements, versionFromFilename } from "./migrate-clickhouse-helpers.js"; +import { assertNoPasswordlessUsers, repairPasswordlessUsers } from "./clickhouse-credentials.js"; +import { + MANAGED_USERS, + splitStatements, + versionFromFilename, +} from "./migrate-clickhouse-helpers.js"; const client = createClickHouseClient(); @@ -65,15 +70,23 @@ async function recordMigration(version: string): Promise { * across users keep working untouched. */ async function applyUserPasswords(): Promise { - const users: Array<[user: string, envVar: string]> = [ - ["yavio_ingest", "CLICKHOUSE_INGEST_PASSWORD"], - ["yavio_dashboard", "CLICKHOUSE_DASHBOARD_PASSWORD"], - ["yavio_eraser", "CLICKHOUSE_ERASER_PASSWORD"], - ]; - - for (const [user, envVar] of users) { + for (const { user, envVar, warnWhenUnset } of MANAGED_USERS) { const password = process.env[envVar]; - if (!password) continue; + if (!password) { + // Say only what is known from here. The dashboard's fallback to the + // CLICKHOUSE_URL superuser keys off this same variable being unset + // (dashboard/lib/clickhouse.ts), so that consequence is certain. The + // account's own state is not: a deployment that applied a real password + // on an earlier run and later dropped the variable still has a working + // user, and claiming otherwise would be exactly the manufactured + // confidence this file keeps having to guard against. + if (warnWhenUnset) { + console.warn( + `[migrate:clickhouse] ${envVar} is not set — the dashboard will erase as the CLICKHOUSE_URL superuser rather than as ${user}. Set it to close that path.`, + ); + } + continue; + } // Refuse the placeholder outright. .env.example ships blank now, but an // operator upgrading from an older copy may still carry the literal — and @@ -167,7 +180,24 @@ async function main() { console.log(`[migrate:clickhouse] Done — ${appliedCount} migration(s) applied.`); } + // Repair BEFORE applying passwords, and conditionally: migration 0012 + // originally created yavio_eraser with `no_password`, which in ClickHouse + // accepts any credential. This puts such an account back into a state that + // accepts none, then applyUserPasswords replaces that with the operator's + // real password if one is configured. Doing the repair unconditionally in a + // migration would reset WORKING credentials on every existing deployment. + const repaired = await repairPasswordlessUsers(client); + for (const user of repaired) { + console.warn( + `[migrate:clickhouse] ${user} authenticated with NO credential (ClickHouse \`no_password\` accepts any password, including a wrong one) — reset to an unusable credential.`, + ); + } + await applyUserPasswords(); + // Migrations above create every managed user, so they must all be visible + // now; passing true makes the check fail rather than pass vacuously if + // system.users turns out to be invisible to the migrating user. + await assertNoPasswordlessUsers(client, true); } finally { await client.close(); }