Repository navigation
fix(security): encrypt secrets with a random IV, full key, and a MAC - #619
Rushikeshiitb wants to merge 3 commits into
Conversation
|
@claude review the current head against dev for correctness, security, compatibility and regression coverage. Please distinguish PR defects from shared-base or infrastructure blockers. |
|
Claude finished @kshivang's task in 9m 46s —— View job Review:
|
| stored plaintext bytes | backfill result |
|---|---|
a\b (one backslash) |
ERROR: invalid input syntax for type bytea - aborts migration 20260913000000 and the deploy |
a\\b (two backslashes) |
silently rewritten to a\b - a stored secret changes value |
Repro against your local PG 16.15, after seeding a 64-hex vault key:
-- how such a row gets created today: create_secret('a\\b') stores the bytes a\b
select public.decrypt_text(public.encrypt_text('a\\b')); -- 'a\b', already lossy (pre-existing)
select public.encrypt_text('a\b'); -- ERROR: invalid input syntax for type bytea
select public.encrypt_text(public.decrypt_text(public.encrypt_text('a\\\\b')));
-- succeeds, but 2 backslashes became 1The root cause (plaintext::bytea) predates this PR, so a backslash password was already mangled at insert time. What is new is that the migration re-runs that encode over every stored plaintext, turning a latent encoding bug into a deploy-blocking exception or an in-place rewrite.
Worth noting that rekey_secret_envelope gets this right: it keeps plain as bytea from decrypt_iv (:250) straight into encrypt_iv (:257) and never round-trips through text. The two re-encryption paths in this same PR disagree about byte preservation.
Fix: pg_catalog.convert_to(plaintext, 'utf8') at :94. That makes the backfill a true no-op on plaintext, and it is the idiom the ops script already uses for fingerprinting.
2. HIGH: one unreadable row aborts the whole migration, and the error names nothing
All three backfill statements call decrypt_text directly, which raises. But the application's read path for two of those three columns deliberately tolerates an unreadable row: safe_decrypt_recovery_codes returns [] and safe_decrypt_twofa_secret returns NULL. So a database that is working today can fail to migrate, and the operator gets a bare Wrong key or corrupt data (or a UTF-8 decode error) with no table, column or row id.
This is precisely the case the ops script already guards, and its own comment explains why:
-- 0. Every mapped row must be readable through its own read path BEFORE we touch anything. The safe_decrypt_* wrappers return NULL rather than raising, so without this an already-corrupt row would only surface at step 5 as "a row was missed" - fail-closed, but pointing at the wrong thing.
The migration should borrow that preflight (count and name the unreadable rows per column, raise once with the counts), or skip them and report a RAISE NOTICE count. Degenerate values are in the same family: password_encrypted = '', a twofa_secret of exactly 'v1:', or a legacy value whose byteain output is not valid UTF-8.
For password_encrypted specifically the strictness is arguably consistent with the app (the list RPC reads it through bare decrypt_text, so such a row already breaks the whole page). For the other two it is a real behaviour change.
3. HIGH: a non-hex vault key is a hard deploy stop, with a cryptic error and no remediation path
encrypt_text now requires the key to hex-decode to exactly 32 bytes. The base tree explicitly contemplated a key that is not hex: the line this PR deletes from the ops script said
-- The old key may be the documented hex string or a deployed base64 key.
and the pre-PR ops script generated encode(gen_random_bytes(32), 'base64'). Any deployment that rotated with it holds a base64 key today.
On such a deployment:
decode(encryption_key, 'hex')at:82raises first, so the friendly message at:83-86is unreachable for that input. The operator seesERROR: invalid hexadecimal digit: "+". The nice error only fires for a valid-hex key of the wrong length.- The remediation (rotate to a hex key) now runs through
rekey_secret_envelope, which only exists after the migration that just failed. Chicken and egg.
Suggest validating before decoding, with the remediation in the message:
IF encryption_key !~ '^[0-9a-fA-F]{64}$' THEN
RAISE EXCEPTION 'master_encryption_key must be 64 hex characters (openssl rand -hex 32); ...'
USING ERRCODE = '22023', HINT = '<what to do if the deployed key is base64>';
END IF;and stating in the migration header what an operator with a base64 key has to do. The evidence that non-hex keys were in real use is in this PR: five test fixtures had to be moved off them.
4. MEDIUM: the backfill has no automated coverage, and CI cannot reach it
supabase db start applies migrations to empty tables, and no vault key exists at that point. Every backfill WHERE clause matches zero rows, so encrypt_text is never called during migration and get_encryption_key() never raises. The migration applies green in CI having exercised nothing.
The repo already has the mechanism for this: scripts/test/prepare-totp-migration-fixtures.py plus secret_totp_encryption_test.sql replay the real 20260909000000 migration over seeded pre-migration rows and assert the backfill, the round trip and replay idempotency. This PR does not extend it.
Uncovered today: legacy to v2: backfill for all three columns; the v1:v2: nesting and the trigger drop/restore; replay idempotency of this migration; the 32-byte key rejection (PR body says hand-checked, no assertion exists); and rekey_secret_envelope other than indirectly through master_key_rotation_test. Findings 1 and 2 above are both things a fixture-replay test would have caught.
5. LOW: rekey_secret_envelope is missing from audit CHECK 5
supabase/audit/identity_disclosure_audit.sql CHECK 5 is the standing list of internal-only routines that must not be client-callable, and it runs in CI through the generated db_access_audit_test. It names get_encryption_key(), encrypt_text(text), decrypt_text(text) and safe_decrypt_recovery_codes(text); the new routine is not there.
This is worth more than it looks, because the event trigger in 20260909140000 grants EXECUTE to authenticated and service_role at CREATE FUNCTION time (the default ACL includes PUBLIC, so has_function_privilege is true for both), and the only thing that closes it is the REVOKE at :266. That grant-then-revoke dance on a brand-new function is exactly what CHECK 5 exists to pin. (safe_decrypt_twofa_secret(text) is also absent from CHECK 5 - pre-existing.)
6. LOW: decrypt_text's v2 branch does not validate the key length
encrypt_text rejects a wrong-length key, decrypt_text does not (:135). A misconfigured or half-rotated key therefore fails the HMAC check and reports ciphertext failed integrity verification, misdiagnosing a key problem as tampering. Both safe_decrypt_* wrappers swallow it, so the user-visible symptom is "my recovery codes and TOTP seeds vanished" rather than an error.
7. Nits
encode(..., 'base64')wraps at 76 characters. A v2 envelope for a short secret is 64 to 80 bytes, so 88+ base64 characters, so every stored value now contains an embedded\n. Round-trip is fine (decodeskips newlines) but legacy short passwords were newline-free, so this is new for every row.translate(encode(...), E'\n', '')removes the surprise for anything that ever exact-matches or exports these columns.octet_length(raw) < 48(:139,:235) admits a 48-byte envelope with an empty ciphertext and does not check(len - 48) % 16 = 0. Unreachable with a real key (the MAC gates it, andencrypt_ivnever emits zero bytes), but if it were reached the error would come out ofdecrypt_ivrather than as22023.- MAC comparison uses
<>(:147,:241), which ismemcmp, not constant time. I would not call this exploitable here:decrypt_textis revoked from every client role, and the only indirect path is writing a forged envelope into your own row and reading it back through the list RPC, where a byte-wise timing signal over PostgREST is not a practical channel. Noting it because being explicit is cheap. - The new test file calls
vault.create_secretunconditionally. That matches the majority of the suite, butexplicit_anon_and_org_visibility_test.sqlandmaster_key_rotation_test.sql.inboth use an exists-guard and record why ("Unconditionalcreate_secret()made the whole suite fail in exactly that case").
What I verified as correct
Construction. AES-256-CBC, 16-byte IV from gen_random_bytes, encrypt-then-MAC over iv || ciphertext, 32-byte tag, verify strictly before decrypt. The MAC key is domain-separated via hmac(label, enc_key, 'sha256') so the AES key is never reused as the MAC key. The envelope is versioned and : is outside the base64 alphabet, so left(x,3) = 'v2:' cannot collide with a legacy value, which makes the idempotency guards sound and the replay a genuine no-op.
Grants. CREATE OR REPLACE preserves the restricted ACL from #423; the 20260909140000 event trigger only re-grants roles that already had effective access, so a replaced encrypt_text gains nothing; the explicit REVOKEs at :182-183 are correct belt-and-braces and tests 8 and 9 pin them.
Backfill ordering and trigger handling. Statement 2 does not name twofa_secret in its SET list, so the UPDATE OF twofa_secret trigger does not fire and it is safe to run before the drop. Statement 3's recreated trigger at :306 is byte-identical to 20260909000000's definition, so the ops script's known_triggers whitelist and its unreviewed-trigger guard still match.
No side effects on the backfilled rows. There is no updated_at or audit trigger on secrets or secret_metadata (updated_at is set explicitly inside the RPCs), so the backfill does not bump timestamps or emit audit rows for every secret.
Rotation still passes its own guards. The envelope check is twofa_secret not like 'v1:%', and the post-migration form v1:v2:... matches, so rotation is not refused. master_key_rotation_test.sql.in's negative case mutates to v2:v2:..., which still does not match v1:%, so its P0001 assertion holds unchanged even though v2: now means something. Its fixture key was already encode(gen_random_bytes(32),'hex'), so it correctly needed no edit. rekey_secret_envelope is correctly invoker-rights (it takes both keys as parameters and never reads the vault) and correctly VOLATILE; the unused %3$ positional in the rewritten format() is harmless.
Fixtures. I checked every master_encryption_key reference in supabase/tests, supabase/ops and scripts/test. None is left non-hex.
"No JDK surface" holds. The only hit across *.kt / *.ts is a @SerialName("twofa_secret") in composeApp/.../models/SecretModels.kt:56, on the plaintext field the RPC returns. Nothing reads the ciphertext columns or calls these functions.
Shared-base or infrastructure, not this PR
plaintext::bytea/encryption_key::byteabyteain interpretation predates the change. I filed it under PR defects because the migration is what re-runs the encode over stored data, but the root cause is base.get_user_secrets_with_sharedreads passwords through baredecrypt_text, so a single unreadable password row already breaks the entire list RPC. That is why finding 2 matters less forpassword_encryptedthan for the other two columns.- The legacy decrypt branch has to stay for the out-of-repo
qbo_token_state/google_token_statecolumns, which leaves an unauthenticated decrypt path in place indefinitely. For anyone who can write a column that is read back through an RPC, that branch is a format oracle under the live key. Not exploitable in a way I can construct today (an owner can only feed it their own column), but a follow-up issue for a v2-only cutover once the brokers have rotated would be worth opening. Not a change for this PR. - CHECK 5 omits
safe_decrypt_twofa_secret(text)independently of this change. - The dead-tuple / WAL / PITR caveat is correctly documented and genuinely out of scope.
- I could not run
psql,dockerorsupabasein this environment, so nothing here was executed. Findings 1 and 3 are mechanism-level and come with repros you can run against the PG 16.15 instance the PR body mentions.
Summary
The crypto is sound and the access-control reasoning is careful. I would not merge without addressing 1 and 2: as written, a stored secret containing two backslashes either aborts the deploy or is silently rewritten, and a single unreadable row blocks the migration with an error that points nowhere. 3 is a smaller population but the failure mode (cryptic error plus a circular remediation) makes it expensive when it hits. 4 is why 1 and 2 were not caught, and the repo already has the fixture-replay pattern to close it.
· branch fix/secret-encryption-random-iv
|
@Rushikeshiitb Review handoff: validation infrastructure is the remaining blocker, not an identified unresolved author defect. Your randomized authenticated encryption, legacy-read support and rotation integration solve a real storage weakness. The original work remains credited to you. I pushed review fixes for base64 deployment keys, literal plaintext preservation, actionable fail-closed backfill diagnostics and real-migration regression coverage. Validated repair head: The current-head build https://github.com/risa-labs-inc/BossConsole/actions/runs/34736946712 remains queued with no runner steps after the bounded wait. Earlier green checks were for the original head and do not validate these repairs. Required local Gradle validation was also blocked waiting for the shared build lock (zero Gradle output); I canceled only my queued commands after the wait. A final gate attempt under the required dev-merge lock also could not acquire that lock and was canceled without executing a merge. No required check was bypassed, and no live database or app instance was touched. Next steps for maintainers/author: allow or restore CI runner capacity, run the current-head platform/compatibility/database/quality checks, and run the focused desktop tests where applicable plus Per the user's final disposition policy, this unresolved validation handoff is being marked draft with all pushed fixes and contributor credit preserved. No merge or auto-merge was performed. |
c18e24a to
b39ced5
Compare
encrypt_text used the 3-arg extensions.encrypt(plaintext, key, 'aes'), which encrypted under an all-zero IV (deterministic ciphertext), keyed on the ASCII bytes of the hex key rather than hex-decoding it (128-bit, not 256), and added no integrity tag. Deterministic ciphertext alone let a plain GROUP BY over password_encrypted name every set of users sharing a password, and made any signed-in user an encryption oracle for their own rows - readable from a dump without the vault key. Replace encrypt_text/decrypt_text with a versioned v2: envelope (base64 of iv(16) || hmac(32) || ciphertext): AES-256-CBC under a per-row random IV, encrypt-then-MAC HMAC-SHA256 with a domain-separated MAC key, and a hex-decoded 32-byte master key. decrypt_text still reads legacy zero-IV rows, and the migration backfills password_encrypted, recovery_codes_encrypted and twofa_secret into the new form. Re-issue the client REVOKEs from risa-labs-inc#423 after CREATE OR REPLACE so access cannot widen, and correct the inaccurate "AES-256" claims in the three historical migrations. Add pgTAP coverage for the construction (non-determinism, legacy round-trip, integrity, retained revocation) and move the affected secret tests to valid 64-hex vault keys that the hex-decode now requires.
The v2 change broke supabase/ops/rotate_master_encryption_key.sql (risa-labs-inc#423): it re-encrypted with raw pgcrypto - decode(col,'base64') + extensions.encrypt under key::bytea - which crashes on a 'v2:' prefix ("invalid symbol ':' found while decoding base64") and, if it had not, would have rewritten every row back into the deterministic zero-IV form the v2 migration exists to remove. It also minted a base64 new key, which the hex-decoding encrypt_text would reject for every write after a rotation. Add public.rekey_secret_envelope(stored, prefix, old_key, new_key): a version-aware decrypt (v2 body HMAC-verified under the hex-decoded old key, else legacy zero-IV under the old key's ASCII bytes) that always re-encrypts as a fresh v2 envelope under the hex-decoded new key. Rotation calls it in place of the raw expression, so it upgrades legacy rows and never produces deterministic output; plaintext stays inside the call, preserving the script's no-materialization guarantee. The key material stays a parameter, not the vault, and the function is revoked from every client role. The rotation now generates a 64-hex new key so encrypt_text keeps working after the swap. Verified on PostgreSQL 16.15 + pgcrypto by emulating master_key_rotation_test: secrets, recovery codes, TOTP (v1: envelope preserved) and the broker columns all survive one and two rotations, duplicate plaintexts diverge after rotation, and the construction suite still passes 10/10. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
24da8f1 to
dc3b1d6
Compare
|
Rebased onto current dev (which advanced / was squash-rewritten since the review, moving the merge base). The three commits replay cleanly; the only conflict was an additive .gitignore hunk, resolved keeping both entries. All SQL is byte-identical to the previously CI-green head 24da8f1, and the one auto-merged file (prepare-db-access-tests.py) correctly layers this PR's suite entry on top of dev's newer client_grant_audit entry. Migration 20260913000000 sorts after dev's newest (20260912*), and #619+#621 combine with no conflict. |
|
#647 has merged into dev and fixes defect 1 of #618 (random IV, v2: envelope). As #647's own description notes, this PR's authenticated v2: envelope is wire-incompatible with #647's IV-only v2:, and it also decodes the full 32-byte key while #647's now-deployed rows use the truncated key - so it can't be rebased or merged over #647 without making those rows undecryptable, and fixing the key inside a migration is exactly what #647 correctly avoided (hex-vs-base64 can't be guessed per project). Closing this in favour of two follow-ups on top of #647: (1) a distinct v3: authenticated envelope adding the MAC (I can open this), and (2) the full-key fix as an operator-coordinated rotation tracked on #618. Happy to build the v3: PR if you want the integrity layer now. |
2 participants
Fixes #618.
Stored secrets previously used deterministic zero-IV encryption without an integrity check. This change writes a versioned
v2:envelope with a random IV, AES-256-CBC and encrypt-then-MAC HMAC-SHA256, while retaining legacy reads. It backfills passwords, recovery codes and TOTP seeds, and updates key rotation to keep using the authenticated envelope.Original implementation and contribution: @Rushikeshiitb, including the envelope, legacy decoder, backfill, rotation integration and construction tests. This remains distinct from the access-control work in merged #423 and the resilient-listing change in #621.
Review repairs by @kshivang in
24da8f1da:convert_to(..., 'UTF8'), so replaying encryption does not parse or alter literal backslashes.Validation:
git diff --checkpass.Operational limits: this is an operator-applied migration. No live database was changed during review. Existing corrupt rows intentionally stop the upgrade for recovery. Rewriting rows does not erase dead tuples, WAL or backups. Broker columns created outside this repo retain legacy-read compatibility until their rotation path upgrades them. Merge is permitted only into dev after current-head required CI and validation pass; auto-merge remains disabled.