Add vault migration framework with v3→v5 upgrade path#6
Add vault migration framework with v3→v5 upgrade path#6johnzfitch wants to merge 1 commit intomainfrom
Conversation
Implement a secure upgrade path with no downgrade capability: - Add migrate.rs: version validation, migration chain, per-version HKDF wrapping key derivation, anti-rollback via min_version field - Bump VAULT_VERSION to 5, add MIN_SUPPORTED_VERSION = 3 - Add min_version field to Vault struct (serde default for compat) - Auto-migrate on unlock with atomic backup (O_CREAT|O_EXCL, 0600) - Add `dota upgrade` CLI command for controlled fleet upgrades - Zeroize all intermediate key material (WrappingKeys, HKDF arrays, decrypted private keys during rewrap) - Symlink protection on vault and backup paths - 20+ unit tests covering migration, validation, and edge cases Security hardening (two-pass review): - H-1: Zeroize decrypted private keys after rewrap - H-2: Replace fs::copy with atomic O_CREAT|O_EXCL backup creation - M-1: Derive ZeroizeOnDrop on WrappingKeys struct - M-2: Zeroize HKDF intermediate key arrays - M-3: Validate version before early-exit in handle_upgrade - L-2: Use explicit version match arms (no open-ended ranges) https://claude.ai/code/session_01119wsw4Dqok7UZsk8ZQbjH
|
@codex — requesting code review on this PR. Scope: Forward-only vault migration framework for dota (post-quantum secrets manager). Key areas to review:
|
|
@claude — requesting code review on this PR. Context: This PR adds a forward-only vault migration framework to dota, a post-quantum secrets manager using hybrid ML-KEM-768 + X25519 encryption. The vault format is being upgraded from v4 to v5 with a migration chain supporting v3→v4→v5. What changed (8 files, +983/-58 lines):
Review focus areas:
Please review for correctness, security, and any design concerns. |
There was a problem hiding this comment.
Pull request overview
Adds a forward-only vault migration framework and wires it into vault unlock + a new explicit CLI upgrade command, enabling automatic upgrades from older vault format versions to the current format while introducing anti-rollback protection.
Changes:
- Introduces
src/vault/migrate.rswith version validation, v3→v4→v5 migration chain, and version-scoped wrapping-key derivation. - Updates vault format to v5 and adds
min_versionfor anti-rollback enforcement. - Updates unlock flow to auto-backup + auto-migrate, and adds a CLI
upgradecommand plus UI/CLI info display formin_version.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/vault/ops.rs |
Auto-migrate on unlock; create pre-migration backups; switch wrapping-key derivation to migration module. |
src/vault/mod.rs |
Exposes new migrate module. |
src/vault/migrate.rs |
Implements migration framework, wrapping-key derivation, and tests. |
src/vault/format.rs |
Bumps vault version to 5; adds min_version and documents version history. |
src/tui/mod.rs |
Displays min_version in vault info. |
src/main.rs |
Wires Upgrade command dispatch. |
src/cli/mod.rs |
Adds Upgrade CLI subcommand variant. |
src/cli/commands.rs |
Implements handle_upgrade command and prints min_version in info output. |
Comments suppressed due to low confidence (1)
src/vault/ops.rs:103
- The
unlock_vaultdoc comment says the backup is written to<vault_path>.v<old>.bak, butcreate_backupmay fall back to<vault_path>.v<old>.bak.<N>if that name already exists. Consider updating the comment to reflect the suffix behavior so operators know where to look.
/// Unlock a vault with a passphrase.
///
/// If the vault is at an older (but supported) version, it is automatically
/// migrated to `VAULT_VERSION`. A backup of the pre-migration vault is
/// written to `<vault_path>.v<old>.bak` before any changes are persisted.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let data = fs::read(vault_path).context("Failed to read vault for backup")?; | ||
| f.write_all(&data) | ||
| .context("Failed to write backup data")?; | ||
| f.sync_all().context("Failed to sync backup file")?; | ||
| return Ok(candidate); |
There was a problem hiding this comment.
If fs::read(vault_path), write_all, or sync_all fails after the backup file has been created, the function returns an error but leaves a (possibly empty/partial) backup file behind. Consider cleaning up the just-created candidate on failure (best-effort remove_file) to avoid accumulating misleading backups.
| let data = fs::read(vault_path).context("Failed to read vault for backup")?; | |
| f.write_all(&data) | |
| .context("Failed to write backup data")?; | |
| f.sync_all().context("Failed to sync backup file")?; | |
| return Ok(candidate); | |
| let result: Result<()> = (|| { | |
| let data = fs::read(vault_path).context("Failed to read vault for backup")?; | |
| f.write_all(&data) | |
| .context("Failed to write backup data")?; | |
| f.sync_all().context("Failed to sync backup file")?; | |
| Ok(()) | |
| })(); | |
| match result { | |
| Ok(()) => return Ok(candidate), | |
| Err(e) => { | |
| // Best-effort cleanup of the just-created backup file on failure | |
| let _ = fs::remove_file(p); | |
| return Err(e); | |
| } | |
| } |
| /// Anti-rollback floor: the minimum binary version required to open this | ||
| /// vault. Set to VAULT_VERSION on creation and on every migration. Older | ||
| /// binaries that do not understand this field will already fail on the | ||
| /// `version` check, but this provides an additional explicit safeguard | ||
| /// against targeted downgrade attacks. |
There was a problem hiding this comment.
The min_version docstring says it is the “minimum binary version required”, but the code compares it against VAULT_VERSION (vault format version) and sets it to VAULT_VERSION during creation/migration. To avoid confusion for operators/users, please adjust the documentation/error wording to refer to the vault format version (or rename the field) rather than implying it maps to the application’s semver.
| /// Anti-rollback floor: the minimum binary version required to open this | |
| /// vault. Set to VAULT_VERSION on creation and on every migration. Older | |
| /// binaries that do not understand this field will already fail on the | |
| /// `version` check, but this provides an additional explicit safeguard | |
| /// against targeted downgrade attacks. | |
| /// Anti-rollback floor: the minimum vault format version required to open | |
| /// this vault. Set to VAULT_VERSION (the current vault format version) on | |
| /// creation and on every migration. Older binaries that do not understand | |
| /// this field will already fail on the `version` (format) check, but this | |
| /// provides an additional explicit safeguard against targeted downgrade | |
| /// attacks. |
| "This vault requires dota v{} or later (anti-rollback). \ | ||
| Please upgrade dota.", | ||
| vault.min_version, |
There was a problem hiding this comment.
validate_version reports “This vault requires dota v{} or later” using min_version, but min_version is a vault format floor (compared to VAULT_VERSION), not the application semver. Consider rephrasing the message to reference the vault format version (or the minimum supporting release) to avoid telling users to install a non-existent “dota v5”.
| "This vault requires dota v{} or later (anti-rollback). \ | |
| Please upgrade dota.", | |
| vault.min_version, | |
| "This vault requires vault format v{} or later (anti-rollback), \ | |
| but this binary only supports up to v{}. Please upgrade dota.", | |
| vault.min_version, | |
| VAULT_VERSION, |
| fn create_backup(vault_path: &str, old_version: u32) -> Result<String> { | ||
| use std::os::unix::fs::OpenOptionsExt; | ||
|
|
||
| let candidates = (0u32..).map(|i| { | ||
| if i == 0 { | ||
| format!("{}.v{}.bak", vault_path, old_version) | ||
| } else { | ||
| format!("{}.v{}.bak.{}", vault_path, old_version, i) | ||
| } | ||
| }); | ||
|
|
||
| for candidate in candidates.take(1000) { | ||
| let p = Path::new(&candidate); | ||
|
|
||
| // Refuse symlinks at the backup destination | ||
| if let Ok(meta) = fs::symlink_metadata(p) { | ||
| if meta.file_type().is_symlink() { | ||
| continue; // skip symlinks, try next suffix | ||
| } | ||
| } | ||
|
|
||
| // Atomically create-or-fail (O_CREAT | O_EXCL equivalent) | ||
| match fs::OpenOptions::new() | ||
| .write(true) | ||
| .create_new(true) | ||
| .mode(0o600) | ||
| .open(p) |
There was a problem hiding this comment.
create_backup uses std::os::unix::fs::OpenOptionsExt and .mode(0o600) in non-test code, which will not compile on non-Unix targets. If this project is intended to build on Windows, consider moving the Unix-specific permission setting behind #[cfg(unix)] and providing a portable fallback (e.g., create the file with create_new(true) and set permissions conditionally).
| // Refuse symlinks at the backup destination | ||
| if let Ok(meta) = fs::symlink_metadata(p) { | ||
| if meta.file_type().is_symlink() { | ||
| continue; // skip symlinks, try next suffix | ||
| } | ||
| } | ||
|
|
||
| // Atomically create-or-fail (O_CREAT | O_EXCL equivalent) | ||
| match fs::OpenOptions::new() | ||
| .write(true) | ||
| .create_new(true) | ||
| .mode(0o600) | ||
| .open(p) |
There was a problem hiding this comment.
The symlink check in create_backup (symlink_metadata then open) is still vulnerable to a TOCTOU race: a path can be swapped to a symlink between the check and open, and OpenOptions::open will follow it. If the goal is to refuse symlinks reliably, use an atomic no-follow open (e.g., O_NOFOLLOW via OpenOptionsExt::custom_flags) or open then verify the opened file is not a symlink via fstat/metadata on the file descriptor.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de43ae842c
ℹ️ 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".
| let backup_path = create_backup(vault_path, old_version)?; | ||
|
|
||
| migrate::migrate_vault(&mut vault, &master_key) | ||
| .context("Vault migration failed (backup preserved)")?; |
There was a problem hiding this comment.
When opening a v3/v4 vault, unlock_vault writes a backup before it knows the passphrase can decrypt the private keys. If the passphrase is wrong (or key ciphertext is corrupted), migration fails but the backup remains, so repeated failed attempts keep consuming new *.bak suffixes until create_backup hits its 1000-file limit and then blocks unlock entirely even for the correct passphrase. This is a user-facing lockout path introduced by the new migration flow; defer backup creation until after in-memory migration succeeds, or clean up the backup on migration failure.
Useful? React with 👍 / 👎.
| let (enc_x25519, new_x25519_nonce) = aes_encrypt(&new.x25519, &x25519_sk)?; | ||
|
|
||
| // Zeroize decrypted private key material before dropping | ||
| mlkem_sk.zeroize(); | ||
| x25519_sk.zeroize(); |
There was a problem hiding this comment.
In rewrap_private_keys, decrypted private key buffers are scrubbed only after both re-encryptions succeed. Any error from the second aes_encrypt(...)? returns early and skips zeroization, leaving plaintext key material in heap memory until normal deallocation. Since this module is explicitly security-sensitive and claims zeroization guarantees, these buffers should be wrapped/guarded so they are cleared on every exit path (including ? errors).
Useful? React with 👍 / 👎.
- Fix UTF-8 zeroization flaw: explicitly zeroize bytes on FromUtf8Error in get_secret() to prevent plaintext lingering in error (PR #5 P1) - Fix non-canonical commitment input: add length prefixes to variable- length fields in compute_key_commitment() (PR #5 P2) - Fix unsafe signal handler: replace signal()/raise() with sigaction and _exit() for async-signal-safe operation (PR #5 P2) - Fix TOCTOU symlink race in backup: use atomic temp file + rename instead of symlink check + fs::copy (PR #6/7 P2) - Fix clippy needless_borrows_for_generic_args in migration.rs - Fix cargo fmt formatting across all changed files https://claude.ai/code/session_01QD9ukQ4n5q7aDmVXVWELNw
Summary
Implement a forward-only vault migration framework that enables automatic and explicit upgrades from vault v3 to v5. The system includes version validation, anti-rollback protection, automatic backup creation, and sequential migration steps with domain-separated HKDF key derivation.
Key Changes
New migration module (
src/vault/migrate.rs):Vault format updates (
src/vault/format.rs):VAULT_VERSIONfrom 4 to 5min_versionfield for anti-rollback enforcementAutomatic migration in unlock (
src/vault/ops.rs):unlock_vault()now automatically migrates older vaults to current versionNew CLI command (
src/cli/commands.rs,src/cli/mod.rs,src/main.rs):upgradecommand for explicit, controlled vault migrationUI updates (
src/tui/mod.rs):min_versionfield in vault infoImplementation Details
dota-v4-wrap-*vsdota-v5-wrap-*) to prevent key reuse across versionsmin_versionis set toVAULT_VERSION, preventing older binaries from opening the vaultO_CREAT | O_EXCLsemantics with symlink rejection and sequential suffix fallbackZeroizeandZeroizeOnDroptraitshttps://claude.ai/code/session_01119wsw4Dqok7UZsk8ZQbjH