Skip to content

fix(security): vault Argon2id + XChaCha20-Poly1305 + safe v0→v1 migration (H2/L3) - #14

Merged
willamhou merged 2 commits into
masterfrom
sec/pr-b-vault-kdf
Jun 24, 2026
Merged

fix(security): vault Argon2id + XChaCha20-Poly1305 + safe v0→v1 migration (H2/L3)#14
willamhou merged 2 commits into
masterfrom
sec/pr-b-vault-kdf

Conversation

@willamhou

Copy link
Copy Markdown
Owner

PR-B of the security-hardening plan. Findings H2 (weak vault KDF + dev password) and L3 (AES-GCM nonce). The migration design uses per-record versioning + explicit migrate (revised after a plan-review BLOCKER on store-wide switching).

Crypto

  • Argon2id key derivation (per-vault random salt + pinned params persisted in kdf.json, OWASP floor m=19456,t=2,p=1), via hash_password_into (raw key, not the PHC verifier).
  • XChaCha20-Poly1305 AEAD (24-byte random nonce → L3 nonce-collision concern gone), with AAD = site_id so a record can't be copied to another site and decrypted.
  • v1 records carry a 0x01 version byte.

Migration safety (the careful part)

  • Legacy v0 records (AES-256-GCM, SHA-256 key) stay readable via per-record dispatch (try v1 when the version byte + AEAD tag verify, else v0). A mixed store never locks out; a v0 record whose nonce happens to start with 0x01 still decrypts (v1 tag fails → v0 fallback).
  • relais vault migrate re-encrypts all records to v1 (idempotent, crash-safe — partial migration stays readable).
  • kdf.json loss protection: a reserved db marker records initialisation, so a missing kdf.json while the vault was initialised is refused (not silently regenerated — which would permanently lock out credentials). A corrupted kdf.json errors, never regenerates. kdf.json is written durably (temp + fsync + rename + parent-dir fsync).

Dev password

  • Remove the silent relais-dev-password default: require RELAIS_VAULT_PASSWORD; the old default only under RELAIS_ALLOW_DEV_VAULT_PASSWORD=1|true. exec/serve degrade to no-vault when unset.

Tests

v1 round-trip + kdf.json; reads legacy v0; v0-record-starting-with-0x01 fallback; migrate→reopen; missing-kdf-with-marker refuses; corrupted-kdf errors; AAD blocks cross-site copy; wrong-password fails; CLI vault migrate parse. cargo test --workspace green; clippy clean.

Codex-reviewed; both BLOCKERs (kdf regeneration lockout, parent-dir fsync) and the cross-site AAD MEDIUM are fixed. A follow-up will tighten open_vault().ok() error visibility in exec/serve (Q5).

🤖 Generated with Claude Code

…tion (H2/L3)

PR-B of the security-hardening plan.

- Derive the vault key with Argon2id (per-vault salt + pinned params in kdf.json)
  and seal records with XChaCha20-Poly1305 (24-byte nonce), AAD = site_id so a
  record can't be copied between sites. v1 records carry a 0x01 version byte.
- Legacy v0 records (AES-256-GCM, SHA-256 key) stay readable via per-record
  dispatch; `relais vault migrate` re-encrypts them. Mixed stores never lock out.
- kdf.json is created only when safe: a reserved db marker records initialisation,
  so a *lost* kdf.json with existing v1 records is refused (not silently
  regenerated, which would lock out credentials). Corrupt kdf.json errors, never
  regenerates. kdf.json is written durably (temp + fsync + rename + parent fsync).
- Remove the silent dev vault password: require RELAIS_VAULT_PASSWORD; the old
  default only under RELAIS_ALLOW_DEV_VAULT_PASSWORD=1|true.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e36122d704

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/core/src/vault.rs
Comment on lines +264 to +265
db.insert(KDF_MARKER, &[1u8])?;
db.flush()?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Defer the KDF marker until a v1 write

Opening an existing legacy v0-only vault now writes KDF_MARKER even if no credential is migrated or stored as v1. If that generated kdf.json is later omitted from a backup/restore or otherwise lost, load_kdf refuses to open the vault solely because this marker exists, even though all remaining records are still v0 and decryptable with just the password; this can lock users out after a read-only open of an old vault. Consider recording the marker when the first v1 record is actually written/migrated, not during initial v0-compatible open.

Useful? React with 👍 / 👎.

Reject NUL-prefixed site ids in store/retrieve/delete so a caller can't collide
with the internal KDF_MARKER (e.g. delete it and defeat the lost-kdf lockout
protection). Closes the Codex confirmation NO-GO.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@willamhou
willamhou merged commit 369d688 into master Jun 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant