fix: require explicit credentials in the production overlays (#595) - #604
Merged
Conversation
Every deployment path shipped the same well-known credentials — tracepcap_pass,
minioadmin, P@ssw0rd — hard-coded as literals rather than as ${VAR:-default}, so
there was no way to override them from .env without editing the compose file.
Splits the two audiences instead of picking one:
base (docker-compose.yml, docker-compose.offline.yml)
credentials become ${VAR:-<dev default>}, so `docker compose up -d` still
works with no .env. CI runs compose without one (.env is gitignored), and
e2e/system-tests would break on a required variable.
prod overlays (docker-compose.prod.yml, docker-compose.offline-prod.yml)
re-declare the same variables as ${VAR:?message}. Compose aborts before
starting anything if any is unset, so a production deployment cannot
silently come up on the shipped defaults.
Credentials are threaded through every consumer that has to agree on them: the
postgres healthcheck's pg_isready, the minio-init `mc alias set`, DATABASE_URL's
database name, and the backend's S3 access/secret key. Missing one would leave a
service unable to authenticate against a datastore whose password had moved.
Documents all seven variables in .env.example, including the caveat that
POSTGRES_USER/POSTGRES_DB only apply on first volume init.
Verified: base config resolves unchanged with no .env (CI case); both prod
overlays fail with an actionable message; prod with .env propagates custom values
everywhere and leaves no shipped default behind. Full stack rebuilt — Flyway
migrated, bucket created, API returns 200.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…review) Code review on #604 found that the previous commit made credentials configurable but spliced them textually into two `sh -c` command strings, so a password containing a space or shell metacharacter broke — or executed. The bug defeated the PR's own purpose: it only triggers once an operator sets a real password, which is exactly what that PR invites them to do for the first time. Verified before the fix: MINIO_ROOT_PASSWORD='p@ss w0rd!' -> mc receives `p@ss` as the secret key; bucket never created, backend then fails every S3 call against a missing bucket MINIO_ROOT_PASSWORD='a;touch /tmp/pwned' -> `;` terminates the command and the rest runs as a second shell command POSTGRES_USER='u;id' -> pg_isready never passes, so postgres stays unhealthy forever and every dependent service blocks on it Credentials now reach both containers as `environment:` entries and are read back as `"$$VAR"`, so the container's shell expands them and Compose never splices the value into the command. Runtime-verified: an injection payload arrives as one literal argument and does not execute. Also from the review: - Require POSTGRES_DB in the prod overlays alongside its siblings, and pin DATABASE_URL to it. Compose merges environment maps key-by-key, so it was silently falling through to the base default while every other credential was required — a trap given that POSTGRES_DB only applies on first volume init. - Note MinIO's 8-character password minimum in .env.example. - Stop advertising `user` / `P@ssw0rd` as working defaults in the Keycloak docs and the offline-prod header. The prod overlays abort instead of defaulting. Verified: stack boots clean with a password containing both a space and a semicolon (bucket created, postgres healthy); prod aborts on any missing credential; base config unchanged with no .env; 39 migrations applied on a fresh volume; API returns 200; docs build clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NotYuSheng
added a commit
that referenced
this pull request
Jul 28, 2026
Addresses code review on #611. Two findings were genuine blockers, and one of them reintroduced on the host exactly the bug #604 fixed for mc. .env was SOURCED, not parsed (backup.sh, restore.sh) `set -a; source ./.env` executes the file as shell. Verified: a password containing $(...) RUNS as a command, and one containing a space aborts the script with `spaces: command not found` before any backup happens. .env is Compose's format, not bash. Now parsed line by line — no execution path. Sourcing also inverted precedence: .env clobbered the real environment, so the systemd unit's BACKUP_DIR was silently discarded and every nightly archive would land on local disk instead of the off-host storage the docs call the whole point. The caller's environment now wins. Failed config restore printed NOTHING and still reported success (restore.sh) `docker cp … && log "restored"` with no || branch: under set -e a failed command in that position neither prints nor exits, so signatures.yml could go missing while the operator was told "Restore complete." Also fixed: - Restore now inspects pg_restore stderr, separating real errors from the "does not exist" notices --clean always emits. Previously the exit code was ignored entirely and a half-restored database still passed the table check. - Bucket restore uses --remove, making it the replacement the prompt and docs promise rather than a merge that orphans objects the database can't see. - Restore compares the bucket against the manifest's object count, so a partial transfer fails instead of silently restoring some PCAPs. - Backup cross-checks captured objects against the bucket, so a wrong bucket name can't produce a "successful" backup containing no PCAPs. - Staging moved from /tmp into BACKUP_DIR: it holds an uncompressed copy of the whole capture set, and /tmp is tmpfs on many server images. - Pruning reads the timestamp in the filename, not mtime, which resets when archives are copied to a NAS and would let BACKUP_DIR grow unbounded. - flock guards concurrent runs (timer + manual) from truncating one archive. - Service: Restart=on-failure for boot-time catch-up runs that fire before the containers are ready, and the [Install] section removed so enabling the service alongside the timer can't add a backup on every boot. - .gitignore: anchor /backups/ to the repo root. Verified: hostile .env ($(...) and spaces) completes with no execution; env overrides .env; full destroy/restore cycle still returns exact row counts and a byte-identical PCAP; tampered manifest and wrong bucket both rejected; pruning removes an old-named archive with a fresh mtime; both systemd units pass systemd-analyze verify. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
NotYuSheng
added a commit
that referenced
this pull request
Jul 28, 2026
* feat: automated backups with a tested restore (#379) Backup was documented but not implemented: backup-restore.rst listed pg_dump and mc mirror commands for a human to run by hand, with no schedule, no verification, and no rehearsed restore. The leadership deck ranked losing imported data as the top risk, and this was the last item behind that framing. scripts/backup.sh One timestamped archive holding all three things that cannot be regenerated: the PostgreSQL dump (custom format, so pg_restore --clean works), the MinIO objects, and signatures.yml from the config volume. Plus a manifest recording what was captured. scripts/restore.sh Restores an archive, and verifies rather than assuming: it counts the tables actually present afterwards and fails if the database came back empty. It rejects corrupt archives, tarballs that aren't TracePcap backups, and warns when the manifest's database name doesn't match the target. systemd service + timer Nightly at 02:30, Persistent=true so a powered-off night is caught up rather than skipped, RandomizedDelaySec so several hosts don't hit the same NAS at once. Cron equivalent documented. Two properties that matter more than the happy path: - Old backups are pruned only AFTER the new archive passes its size check and a tar -tzf read-back, so a failing run can never destroy the last good one. - Both scripts exit non-zero on failure, so cron and systemd surface a broken backup instead of passing over it silently. Credentials come from .env and are passed to `mc` inside the container rather than interpolated into a shell command, following the fix from #604. Validated end to end, not just executed: uploaded and analysed a PCAP, took a backup, dropped the public schema and wiped the bucket entirely, then restored. Row counts returned exactly (files=1 packets=15 conversations=11), 31 tables came back, the API served the file list, and the recovered PCAP was byte-identical to the original by MD5. Corrupt/foreign/missing archives were all rejected, and retention pruning kept the current backups while dropping a 30-day-old one. PITR is deliberately out of scope — WAL archiving is a poor trade for a single-server deployment whose primary data is re-importable. RPO (24h) and RTO, and what is NOT covered (PITR, Keycloak users, redundancy), are documented. Refs #379 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix/backup-env-parsing-and-restore-verification Addresses code review on #611. Two findings were genuine blockers, and one of them reintroduced on the host exactly the bug #604 fixed for mc. .env was SOURCED, not parsed (backup.sh, restore.sh) `set -a; source ./.env` executes the file as shell. Verified: a password containing $(...) RUNS as a command, and one containing a space aborts the script with `spaces: command not found` before any backup happens. .env is Compose's format, not bash. Now parsed line by line — no execution path. Sourcing also inverted precedence: .env clobbered the real environment, so the systemd unit's BACKUP_DIR was silently discarded and every nightly archive would land on local disk instead of the off-host storage the docs call the whole point. The caller's environment now wins. Failed config restore printed NOTHING and still reported success (restore.sh) `docker cp … && log "restored"` with no || branch: under set -e a failed command in that position neither prints nor exits, so signatures.yml could go missing while the operator was told "Restore complete." Also fixed: - Restore now inspects pg_restore stderr, separating real errors from the "does not exist" notices --clean always emits. Previously the exit code was ignored entirely and a half-restored database still passed the table check. - Bucket restore uses --remove, making it the replacement the prompt and docs promise rather than a merge that orphans objects the database can't see. - Restore compares the bucket against the manifest's object count, so a partial transfer fails instead of silently restoring some PCAPs. - Backup cross-checks captured objects against the bucket, so a wrong bucket name can't produce a "successful" backup containing no PCAPs. - Staging moved from /tmp into BACKUP_DIR: it holds an uncompressed copy of the whole capture set, and /tmp is tmpfs on many server images. - Pruning reads the timestamp in the filename, not mtime, which resets when archives are copied to a NAS and would let BACKUP_DIR grow unbounded. - flock guards concurrent runs (timer + manual) from truncating one archive. - Service: Restart=on-failure for boot-time catch-up runs that fire before the containers are ready, and the [Install] section removed so enabling the service alongside the timer can't add a backup on every boot. - .gitignore: anchor /backups/ to the repo root. Verified: hostile .env ($(...) and spaces) completes with no execution; env overrides .env; full destroy/restore cycle still returns exact row counts and a byte-identical PCAP; tampered manifest and wrong bucket both rejected; pruning removes an old-named archive with a fresh mtime; both systemd units pass systemd-analyze verify. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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.
Problem
Every deployment path shipped the same well-known credentials —
tracepcap_pass,minioadmin,P@ssw0rd— hard-coded as literals rather than${VAR:-default}. There was no way to override them from.envwithout editing the compose file itself.Why not just make them required
CI runs
docker compose up -d --buildwith no.env(it's gitignored and absent in CI). A blanket${VAR:?}would have brokene2e.ymlandsystem-tests.ymlimmediately. So this splits by audience instead of picking one:Base (
docker-compose.yml,docker-compose.offline.yml) →${VAR:-<dev default>}Local dev and CI work exactly as before, now overridable from
.env.Prod overlays (
docker-compose.prod.yml,docker-compose.offline-prod.yml) →${VAR:?message}Compose aborts before starting anything if a credential is unset, so a production deployment cannot silently come up on shipped defaults.
The subtle part
Credentials are threaded through every consumer that has to agree on them:
pg_isready -Umc alias setDATABASE_URLMissing any one would leave a service unable to authenticate against a datastore whose password had moved — a failure that only surfaces at runtime.
Verification
.env(the CI case).env.envPOSTGRES_USER.envdocker compose up -d --buildHTTP 200Caveat worth knowing before the box goes live
POSTGRES_USER/POSTGRES_DBonly take effect on first volume init — Postgres will not rename an existing role or database. Setting them on an already-initialised deployment silently leaves the old role in place while the backend tries the new one.Documented in
.env.example, but it means rotating credentials on the running deploy box needs a volume recreate or a manualALTER ROLE.Closes #595
🤖 Generated with Claude Code