fix(security): yavio_eraser authenticated with no credential at all - #68
Merged
Conversation
Migration 0012 created the erasure user with `IDENTIFIED WITH no_password` and the comment "a user with no password cannot authenticate, so this fails closed". That is Postgres behaviour. In ClickHouse `no_password` is an authentication method meaning no credential is REQUIRED — checkBasicAuthentication returns success for AuthenticationType::NO_PASSWORD, carrying the upstream comment "N.B. even if the password is not empty!". It failed open. Verified against a real ClickHouse rather than argued from the docs: a no_password yavio_eraser authenticates with an empty password AND with a deliberately wrong one, and then executes `ALTER TABLE default.events DELETE`. The account holds ALTER DELETE on every tenant's events, so any process that could open a socket to ClickHouse could erase the analytics store without a credential. It cannot read, so this was integrity only, and the datastores are loopback-bound — but it defeated the control the eraser user exists to be. The real password is applied out of band by applyUserPasswords(), which skips silently when CLICKHOUSE_ERASER_PASSWORD is unset. compose defaults that to empty and .env.example ships it blank, so fresh installs via setup-env.sh were fine and any deployment that upgraded without adding the variable was not. - 0013 repairs deployments that already ran 0012, via sha256_hash with a digest whose preimage was never generated. Publishing that value leaks nothing and the test proves it: authenticating with the hash itself is rejected. - 0012 is amended so fresh installs never pass through the open state, and its comment no longer asserts the opposite of what the database does. - migrate-clickhouse.ts now refuses to finish while any managed user is on no_password. The 0012 rollout check asked "can yavio_eraser authenticate?", which a no_password account answers yes to for any credential — a check that cannot fail. Asserting auth_type is that check with a failure mode. - It also warns when CLICKHOUSE_ERASER_PASSWORD is unset, because the dashboard then erases as the CLICKHOUSE_URL superuser, which is what 0012 set out to end. Tests assert no managed user is left on no_password, that yavio_eraser exists so that assertion is not vacuous, and that its grants are exactly ALTER DELETE on default.events. Reverting the migrations fails them with `expected [ 'yavio_eraser' ] to deeply equal []`.
Coverage Report for db
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Coverage Report for ingest
File CoverageNo changed files found. |
Coverage Report for dashboard
File Coverage
|
||||||||||||||||||||||||||||||||||||||
Addresses the review of this PR. The first version shipped the repair as migration 0013: an unconditional `ALTER USER yavio_eraser IDENTIFIED WITH sha256_hash`, applied to every existing deployment. That is a destructive reset of a WORKING credential, put back only by applyUserPasswords in the same process — so any environment drift between the migrator and the dashboard would leave the eraser unreachable. And a broken eraser is silent: all three deletion routes catch the ClickHouse failure, console.error it, and still return 200 after the Postgres rows are gone. The old no_password bug was masking exactly that class of mismatch, because it accepted any credential. Fixing a fail-open auth hole by introducing an undetected data-retention failure is not a fix. 0013 is gone. repairPasswordlessUsers() in the new clickhouse-credentials.ts repairs ONLY an account that is actually passwordless, so a user already on a real password is never touched. Proven live: apply a password, re-run the migrator with no password in its environment, credential survives. Also from the review: - assertNoPasswordlessUsers could pass vacuously. `SELECT ... FROM system.users` is access-filtered, not error-raising, so a migrator without SHOW USERS sees zero rows and the guard reported success — the same "check that cannot fail" its own docstring condemns. It now refuses when it was told users exist and inspected none. - Neither the guard nor the repair had any test. helpers/clickhouse.ts runMigrations() is a reimplementation of main() that skips both, and the tests duplicated the auth_type predicate instead of calling it, so they proved the database state and nothing about the shipped code. Tests now call the real functions, and a new describe block builds the pre-fix state and exercises the repair — CI runs on fresh containers where the amended 0012 alone satisfied every assertion, so deleting the repair used to leave the suite green. - dropAll() left the three users behind. They are server-level objects that survive a table drop, so against a persistent ClickHouse the credential tests asserted on accounts created weeks ago. - The grants test saw only direct grants: a granted role would have carried SELECT in invisibly. Now asserts system.role_grants empty and grant_option 0. - MANAGED_USERS was duplicated in four places and the eraser had a hardcoded string comparison. Single tuple in migrate-clickhouse-helpers.ts with a warnWhenUnset flag, so the policy lives next to the user. - The unset-password warning asserted the account was "left unauthenticatable", which the code never checked and which is false when a password was applied on an earlier run. It now states only the certain consequence: the dashboard falls back to the CLICKHOUSE_URL superuser. One of the new tests was itself vacuous on the first run — asserting `res.status !== 516` when ClickHouse answers a rejected credential with HTTP 403 and puts `Code: 516` in the BODY, so the status could never be 516. Now reads the body. Verified with teeth: neutering repairPasswordlessUsers fails 2 tests. 93/93 packages/db tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found by the security review of the #42–#66 hardening series. This is a defect in my own PR #66.
What was wrong
Migration 0012 created the erasure user with
IDENTIFIED WITH no_password, and its comment said "A user with no password cannot authenticate, so this fails closed." That is true of Postgres, which is where the pattern came from. In ClickHouse it is the exact opposite:no_passwordis an authentication method meaning no credential is required.checkBasicAuthenticationreturns success forAuthenticationType::NO_PASSWORDwith the upstream comment "N.B. even if the password is not empty!" — so a wrong password authenticates too.CREATE USERwith noHOSTclause also defaults toHOST ANY.I verified this against a real ClickHouse rather than arguing it from the docs:
That account holds
ALTER DELETE ON default.events— every tenant's events, in one statement, with no credential.Who was affected
Not this deployment, and not fresh installs.
setup-env.shgeneratesCLICKHOUSE_ERASER_PASSWORDbefore the first migrate.applyUserPasswords()skips silently when the variable is unset, compose defaults it to empty, and.env.exampleships it blank. An operator who rangit pull && docker compose up -dwith a pre-existing.envgot a permanently passwordless, delete-capable account. Nothing surfaced it: the dashboard falls back to the superuser when the variable is unset, so erasure kept working.no_password(accepts anything) fromsha256_password.127.0.0.1. Exploitation needed local host access or a compromised container on thebackendnetwork. Integrity only — the account genuinely cannot read what it erases.The fix
0013_eraser_fail_closed.sqlrepairs deployments that already ran the old 0012, usingsha256_hashwith a digest whose preimage was never generated. The account keeps its grant and stops being reachable. Publishing that value leaks nothing, and a test proves it — authenticating with the hash itself is rejected.0012is amended so fresh installs never pass through the open state, and its comment no longer asserts the opposite of what the database does.migrate-clickhouse.tsrefuses to finish while any managed user is onno_password. Worth stating plainly: the rollout check written for feat(security): erase ClickHouse data as a dedicated user, not the superuser #66 asked "can yavio_eraser authenticate?" — a question ano_passwordaccount answers yes to for any credential, including the one you believe you just set. That check could not fail. Assertingauth_typeis the same check with a failure mode.CLICKHOUSE_ERASER_PASSWORDis unset, because the dashboard then erases as theCLICKHOUSE_URLsuperuser — the arrangement feat(security): erase ClickHouse data as a dedicated user, not the superuser #66 existed to end.Verification
All against a real ClickHouse, not mocks.
The new tests fail on the old code — reverting both migrations gives
expected [ 'yavio_eraser' ] to deeply equal [].The upgrade path, simulated by leaving the container on the old 0012 with
0012recorded, then running the real migrator:The configured path still works — with the password set: authenticates, can delete, and
SELECTis stillCode: 497. Not enough privilege.87/87
packages/dbtests pass.Deploying
No
.envchange is needed here — the VM already hasCLICKHOUSE_ERASER_PASSWORD, so 0013 resets the credential andapplyUserPasswords()restores it within the same migrate run, before the dashboard is touched.A follow-up PR covers the rest of the review:
POSTGRES_APP_PASSWORDnever reaching the containerised migrator, the two:-yavio_devfallbacks on the most privileged accounts, and two low-severity items.