Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .env.example
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 4 additions & 1 deletion packages/dashboard/lib/clickhouse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
29 changes: 22 additions & 7 deletions packages/db/migrations/clickhouse/0012_eraser_user.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
144 changes: 144 additions & 0 deletions packages/db/src/__tests__/ch-migrations.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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();
Expand Down
21 changes: 19 additions & 2 deletions packages/db/src/__tests__/helpers/clickhouse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 [
Expand All @@ -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. */
Expand Down
110 changes: 110 additions & 0 deletions packages/db/src/clickhouse-credentials.ts
Original file line number Diff line number Diff line change
@@ -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<typeof createClickHouseClient>;

/**
* 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<Map<string, string | string[]>> {
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<string[]> {
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<void> {
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 },
);
}
}
Loading
Loading