ci: nightly pg_dump artifacts and restore runbook - #122
Conversation
|
@namdamdoi68-oss Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Signed-off-by: namdamdoi68-oss <namdamdoi68@gmail.com>
77f5b01 to
18b5969
Compare
Miracle656
left a comment
There was a problem hiding this comment.
The design here is right and the docs are genuinely good — the direct-vs-pooler distinction for Neon, skipping a network whose secret is absent, --format=custom for selective restore, and scheduling at 03:00 to stay clear of the 04:00 load test are all considered choices. Thank you for the workflow test too; that is rarer than it should be.
There is one problem, and unfortunately it is a blocking one.
This repository is public, so the artifacts are public
actions/upload-artifact inherits the repository's visibility. On a public repo the Actions tab is world-readable and artifacts can be downloaded by anyone with a GitHub account. This workflow would therefore publish a complete nightly dump of the production database, retained 14 days.
You were careful with the secrets going in — "Do not put these URIs in the repo, .env.example, or workflow YAML" is exactly right. The gap is the data coming out.
What is in the dump
Checking prisma/schema.prisma, it is not only market data:
model Webhook {
url String
secret String // plaintext HMAC signing key
}
model ApiKey {
hash String @unique
label String
ratePerMin Int
monthlyQuotaCents Int?
}webhooks.secretis plaintext. It is the signing key subscribers use to verify our deliveries. Anyone who downloads a dump can forge webhook payloads that pass signature verification, aimed at the endpoints inwebhooks.url— which are in the same dump.api_keysexposes hashes (credit for hashing them), plus customer labels and commercial quota terms.
Price history is re-derivable from chain and harmless. These two tables are not.
Options, roughly in order of preference
- Check whether this is needed at all. Neon has branching and point-in-time restore built in. If the retention window there already covers the recovery objective, the strongest version of this PR may be
docs/backup-restore.mddocumenting that, with no workflow. - Send dumps somewhere private — S3/R2/Backblaze with credentials in secrets. Keeps the schedule and the runbook; changes only the destination.
- Encrypt before upload. Smallest change that preserves the current shape: pipe through
ageorgpgwith the recipient key in a secret, so the artifact is public but inert. Then the runbook grows a decrypt step and a note on where the private key lives. - Excluding the two tables would technically stop the leak, but it also stops this being a restore path, so I would not go that way.
Whichever you pick, worth adding to docs/backup-restore.md: a dump of this database contains live credentials and must never land anywhere world-readable. That sentence is the thing that stops it regressing later.
Happy to merge once the destination changes — the rest of this is ready.
Signed-off-by: namdamdoi68-oss <namdamdoi68@gmail.com>
|
Public artifacts were the gap — dumps now go through age before upload, so the Actions tab only has ciphertext. AGE_RECIPIENT is the public key secret; the identity stays off GitHub. Decrypt + the webhook-secret warning are in docs/backup-restore.md. |
Two gaps in an otherwise correct workflow, both of the same kind — a backup that did not happen reporting success: - A missing DATABASE_URL exited 0 silently. It now emits a ::warning:: annotation and marks the step as having produced nothing, so a skipped network is visible in the run summary instead of looking identical to one that ran. - if-no-files-found: ignore meant that if age ever produced no output, the upload would find nothing and pass. It is now 'error', gated on the dump step actually having produced a file, plus an explicit check that no cleartext survived encryption. The encryption design was already right: age with a public-key recipient, so the runner never holds the key that decrypts the artifact, and a hard refusal when AGE_RECIPIENT is unset.
Miracle656
left a comment
There was a problem hiding this comment.
Approved and merging — I pushed two hardening commits (1b341cc) rather than block, since the wave has closed.
I owe you a correction on my earlier review. I flagged this as publishing an unencrypted dump to a public repo's artifacts. That was true of the version I read; it is not true of this one, and the header comment says so in the first three lines. My apologies for the stale objection.
What is here now is the better of the two approaches I suggested. age with a public-key recipient means the runner never holds the key that decrypts the artifact — a symmetric passphrase in Actions secrets is decryptable by anyone who can read that secret or exfiltrate it from a workflow, whereas here the private key never enters CI at all. Refusing outright when AGE_RECIPIENT is unset — refusing an unencrypted dump, exit 1 — is the correct failure direction: a missing backup is recoverable, a published one is not. That matters specifically because Webhook.secret (schema.prisma:252) is a plaintext HMAC signing key sitting next to its delivery URL. ApiKey storing only a hash is the reason this is one exposure and not two.
Nice details: the 03:00 UTC slot chosen to sit an hour clear of the 04:00 load test so dump I/O and a 5k-RPS flood do not overlap; permissions: contents: read; concurrency with cancel-in-progress: false, because cancelling a dump halfway is worse than letting two queue.
And testing the workflow YAML itself is a genuinely good idea — asserting it never echoes $DATABASE_URL, never sets set -x, and contains no inline connection string turns 'we were careful' into something that stays true after the next edit.
My two changes, both the same class of bug — a backup that did not happen reporting green:
- A missing
DATABASE_URLexited 0 silently. It now emits a::warning::and records that it produced nothing, so a skipped network is visible in the run summary instead of indistinguishable from one that ran. if-no-files-found: ignoremeant that ifageever produced no output, the upload found nothing and passed. Nowerror, gated on the dump step having actually produced a file, plus an explicit check that no cleartext survived.set -euo pipefailalready covers the common case; this covers the one whereageexits 0 having written nothing useful.
Verified: workflow YAML parses, its 7 tests pass, full suite 373 passed / 1 skipped.
Worth noting that wraith #173 hit the identical trap independently. Two contributors landing on the same mistake is a gap in how I wrote the issue — it said 'upload as an artifact' and never said 'this repo is public.' You got there without being told, which is the part I should have led with.
Nightly pg_dump of mainnet/testnet to Actions artifacts (14 days). Restore into a fresh DB, then swap DATABASE_URL — docs/backup-restore.md.
Closes #119