fix(security): close the credential gaps the hardening series left behind - #69
Merged
Conversation
…hind Three residual findings from reviewing #42–#66, all the same shape: a credential that stays published or absent when the operator takes a path the scripts do not cover. yavio_app kept the published 'yavio_dev'. 0001 creates the role with that literal under an IF NOT EXISTS guard; migrate.ts repairs it from POSTGRES_APP_PASSWORD, and setup-env.sh generates one — but the compose `migrate` service never passed the variable, and compose-injected environment is not inherited from .env. So a deployment migrating through compose kept a role with a password anyone can read on GitHub and DML on all 14 tables, even for an operator who ran setup-env.sh and followed every documented step. RLS is no barrier: its policies key on current_setting('app.current_user_id'), which the connecting session sets for itself, and verification_tokens, login_attempts and stripe_webhook_events have no RLS at all. The two MOST privileged accounts still fell back to 'yavio_dev'. #57 made POSTGRES_API_PASSWORD fail closed with `:?` but left `:-yavio_dev` on POSTGRES_SERVICE_PASSWORD — the Postgres SUPERUSER, which can COPY ... FROM PROGRAM — and on CLICKHOUSE_PASSWORD, which is unrestricted and carries CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT=1. Both now use the same `:?` form, and .env.example ships them blank rather than pre-filled. Compose refuses to start instead of substituting; a startup error costs a minute, a published default is a credential in a public repo. setup-env.sh put every secret on a command line. `sed -i "s|^${key}=.*|..."` makes the value part of argv, which is world-readable via ps and /proc/<pid>/cmdline while /proc/<pid>/environ is owner-only — so all seven datastore passwords and four app secrets were exposed to any local user, and the chmod 600 two lines above bought nothing (CWE-214). Now the value goes through the environment into awk. That also drops the BSD/GNU `sed -i` split and the escaping question: awk prints the value literally, so a `|`, `&` or backslash from a future generator cannot break the substitution or inject a line. The temp file is created under umask 077 because mv keeps its mode. Also: docker-compose.test.yml published a password-`test` Postgres and ClickHouse on 0.0.0.0 from every developer's machine — the dev compose file has bound to loopback since #42. And the self-hosting docs still advertised 'yavio_dev' as the default for five variables, omitted POSTGRES_API_PASSWORD and CLICKHOUSE_ERASER_PASSWORD entirely, and told readers to change defaults that no longer exist. Verified rather than assumed. The first attempt at proving the yavio_app rotation connected via 127.0.0.1 from inside the container, where pg_hba says `host all all 127.0.0.1/32 trust` — both the old and new passwords "authenticated" because no password was being checked at all. Re-run over a scram-checked connection from the host: 'yavio_dev' REJECTED (28P01), rotated password AUTHENTICATED. Compose fails closed with no .env and validates with the variables set; setup-env.sh produces a 0600 .env, leaves no temp file, and writes no placeholder into any value.
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
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.
The rest of the security review of #42–#66. Independent of #68 — different files, no shared logic (both touch
.env.example, in different sections).Three findings, all the same shape: a credential that stays published or absent when the operator takes a path the scripts don't cover.
1.
yavio_appkept the publishedyavio_dev— even if you did everything right0001_row_level_security.sqlcreates the role with that literal under anIF NOT EXISTSguard.migrate.tsrepairs it fromPOSTGRES_APP_PASSWORD, andsetup-env.shgenerates one — but the composemigrateservice never passed the variable, and compose-injected environment is not inherited from.env.So the host path (
pnpm migrate, which reads.envdirectly) repaired the role and the compose path did not. This is the one credential in the system that survived the documented remediation.What it grants: DML on all 14 tables. RLS is not a barrier — the policies key on
current_setting('app.current_user_id'), a GUC the connecting session sets for itself, andverification_tokens,login_attemptsandstripe_webhook_eventshave no RLS at all. The role is dormant (nothing connects as it), which is why nobody would ever have noticed.2. The two most privileged accounts still fell back to
yavio_dev#57 made
POSTGRES_API_PASSWORDfail closed with:?, but left:-yavio_devon:POSTGRES_SERVICE_PASSWORD— this isPOSTGRES_USERon thepostgres:16image, i.e. the bootstrap superuser, not just a table owner. Superuser meansCOPY ... FROM PROGRAM.CLICKHOUSE_PASSWORD— unrestricted, and carryingCLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT: 1, so it can also create users and grants.Both now use the same
:?form, and.env.exampleships them blank instead of pre-filled. Compose refuses to start rather than substituting a value..envhas bothPOSTGRES_SERVICE_PASSWORDandCLICKHOUSE_PASSWORDset — if either is missing,docker compose upwill now refuse to start instead of silently using the published default. I couldn't check the VM myself; the gcloud session expired mid-review. Happy to verify once you've re-authed.3.
setup-env.shput every secret on a command linesed -i "s|^${key}=.*|${key}=${value}|"makes the value part ofargv./proc/<pid>/cmdlineis world-readable (mode 444);/proc/<pid>/environis owner-only (400). So all seven datastore passwords and four app secrets were exposed to any local user, and thechmod 600two lines above bought nothing (CWE-214).The value now travels through the environment into
awk. That also drops the BSD/GNUsed -isplit and removes the escaping question entirely —awkprints the value literally, so a|,&or backslash from some future generator can't break the substitution or inject a line. Today's base64/hex values can't contain those; this is about not depending on that. The temp file is created underumask 077becausemvkeeps the temp file's mode.Also
docker-compose.test.ymlpublished a password-testPostgres and ClickHouse on0.0.0.0from every developer's machine. The dev compose file has bound to loopback since fix(security): bind datastores to loopback and stop shipping a default DB password #42; this one never did.yavio_devas the default for five variables, omittedPOSTGRES_API_PASSWORDandCLICKHOUSE_ERASER_PASSWORDentirely, and told readers to "change all default passwords" that no longer exist.Verification
Worth recording how the first attempt went wrong, because it's the same trap as
verification-must-prove-it-ran. I proved theyavio_approtation by connecting via127.0.0.1from inside the container — wherepg_hba.confsayshost all all 127.0.0.1/32 trust. Both the old and the new password "authenticated", because no password was being checked at all. The test was measuring nothing.Re-run over a scram-checked connection from the host:
Also verified:
.env:required variable POSTGRES_API_PASSWORD is missing a value, exit 1. Validates cleanly (exit 0) with the variables set.setup-env.shproduces a 0600.env, leaves no temp file behind, generates all seven passwords, rewrites both connection URLs to match, and writes no placeholder oryavio_devinto any value — the only two remaining mentions in the generated file are explanatory comments.