Add vault migration engine for automatic version upgrades by johnzfitch · Pull Request #7 · johnzfitch/dota

johnzfitch · 2026-03-05T22:24:53Z

Summary

This PR introduces a comprehensive vault migration system ("upvault") that automatically upgrades vaults from any older version (v1-v4) to the current version (v5). The migration engine chains stepwise transformations through intermediate versions, ensuring all cryptographic material is properly re-encrypted at each step while maintaining security invariants.

Key Changes

New migration engine (src/vault/migration.rs):
- Implements stepwise migration functions (upvault_v1 through upvault_v4) that convert each vault version to the next
- Handles cryptographic transformations: v1→v2 adds ML-KEM-768 hybrid KEM, v3→v4 re-wraps keys with HKDF-derived wrapping keys, v4→v5 adds migration metadata
- Deferred backup strategy: backups are created only after successful in-memory migration, preventing disk writes on decryption failures
- Comprehensive test suite covering all migration paths, error cases, and backup management
Legacy format definitions (src/vault/legacy.rs):
- Deserialization-only structs for vault formats v1-v3
- Minimal VaultVersionProbe for version detection without full parsing
- Enables reading old vault formats without maintaining serialization support
Updated vault format (src/vault/format.rs):
- Bumped VAULT_VERSION from 4 to 5
- Added MigrationInfo struct to track migration history (original version, timestamp, migration path)
- Added migrated_from field to Vault struct
Integrated migration into unlock flow (src/vault/ops.rs):
- unlock_vault() now automatically detects vault version and triggers migration if needed
- Wrapped decrypted key material in Zeroizing<T> for RAII cleanup of sensitive data
- Maintains backward compatibility: current-version vaults skip migration entirely
Enhanced info command (src/cli/commands.rs):
- Displays migration metadata when available (original version, migration timestamp, version path)

Security Invariants

Deferred backup: Backup created only after full in-memory migration succeeds; wrong passphrase or corrupted data causes error before any disk writes
Key material protection: All decrypted secrets wrapped in Zeroizing<T> for automatic cleanup
Version validation: Rejects future versions and already-current versions with clear error messages
Backup limits: Enforces MAX_BACKUPS (5) to prevent accumulation; oldest backups automatically pruned
Symlink protection: Refuses to write backups through symlinks

Migration Path Examples

v1 → v5: X25519-only → hybrid KEM → nested layout → HKDF key separation → metadata
v3 → v5: Nested layout (direct master key) → HKDF key separation → metadata
v4 → v5: Just adds migration metadata

All intermediate steps preserve secret data integrity through re-encryption with appropriate keys at each stage.

https://claude.ai/code/session_01CyG4gMbVH7AxwFkzHeVyfH

Implement the "upvault" migration system that automatically upgrades vaults from any older version (v1-v4) to the current v5 format when unlocking. Key security properties: - Deferred backup: backups created ONLY after in-memory migration succeeds, preventing orphaned backup accumulation from wrong-passphrase attempts that could lock out users - RAII zeroization: all decrypted key material wrapped in Zeroizing<T> for automatic cleanup on all exit paths including ? early returns - Symlink protection on backup writes - Stepwise migration chain: v1→v2→v3→v4→v5 with crypto transitions at each step (X25519-only → hybrid KEM → nested structs → HKDF key separation → migration metadata) Also fixes existing zeroization gaps in unlock_vault, derive_wrapping_keys, rotate_keys, and get_secret where plaintext key/secret material was not scrubbed on deallocation. https://claude.ai/code/session_01CyG4gMbVH7AxwFkzHeVyfH

johnzfitch · 2026-03-05T22:25:05Z

@claude code review

johnzfitch · 2026-03-05T22:25:12Z

@codex code review security review

Copilot

Pull request overview

Adds an “upvault” migration engine to transparently upgrade on-disk vault JSON from legacy versions (v1–v4) to the current format (v5) during unlock, while recording migration metadata.

Changes:

Introduces src/vault/migration.rs stepwise migration pipeline (v1→v5) with backup management and tests.
Adds legacy, deserialization-only structs for v1–v3 and version probing to support migration.
Bumps vault format to v5 and integrates automatic migration into the unlock flow; exposes migration metadata via the info CLI command.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/vault/ops.rs`	Auto-detects vault version and triggers migration; expands zeroization usage; exposes helpers for migration persistence.
`src/vault/mod.rs`	Exposes new `legacy` and `migration` modules.
`src/vault/migration.rs`	Implements migration engine, backup strategy, and test suite.
`src/vault/legacy.rs`	Adds legacy structs for deserializing v1–v3 vaults and a version probe.
`src/vault/format.rs`	Bumps `VAULT_VERSION` to 5 and adds `MigrationInfo`/`migrated_from` metadata.
`src/cli/commands.rs`	Displays migration metadata in `info` output when present.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T22:28:12Z

src/vault/ops.rs

+    // Decrypt the secret value (zeroize raw bytes after conversion)
    let nonce: [u8; 12] = encrypted.nonce.as_slice().try_into()?;
-    let plaintext = aes_decrypt(&aes_key, &encrypted.ciphertext, &nonce)?;
-
-    String::from_utf8(plaintext).context("Secret contains invalid UTF-8")
+    let mut plaintext = aes_decrypt(&aes_key, &encrypted.ciphertext, &nonce)?;
+    let result = String::from_utf8(plaintext.clone()).context("Secret contains invalid UTF-8");
+    plaintext.zeroize();
+    result


get_secret clones the decrypted plaintext bytes before UTF-8 conversion. If the bytes are not valid UTF-8, the cloned Vec<u8> is retained inside FromUtf8Error and returned (via context(...)) without being zeroized, undermining the intent to wipe decrypted secret material. Restructure this to avoid cloning and explicitly zeroize the bytes on the error path (e.g., match on String::from_utf8(plaintext) and, in the Err, call into_bytes() and zeroize() before returning an error).

Copilot · 2026-03-05T22:28:13Z

src/vault/ops.rs

+        )
+        .context("Failed to decrypt ML-KEM private key (wrong passphrase?)")?,
+    );
+    let mlkem_private = MlKemPrivateKey::from_bytes(mlkem_sk_bytes.to_vec())?;


mlkem_sk_bytes is wrapped in Zeroizing<Vec<u8>> but then converted with to_vec(), creating an extra copy of the decrypted ML‑KEM private key bytes. Since MlKemPrivateKey already zeroizes on drop, consider constructing it without duplicating the key material (e.g., avoid to_vec() and instead move the decrypted Vec<u8> into MlKemPrivateKey::from_bytes, while ensuring the temporary is zeroized if from_bytes fails).

Suggested change

let mlkem_private = MlKemPrivateKey::from_bytes(mlkem_sk_bytes.to_vec())?;

let mlkem_private = MlKemPrivateKey::from_bytes(mlkem_sk_bytes.as_slice())?;

Copilot · 2026-03-05T22:28:13Z

src/vault/migration.rs

+        let encap = hybrid_encapsulate(
+            &MlKemPublicKey::from_bytes(mlkem_public.as_bytes().to_vec())?,
+            &x25519_public,
+        )?;


In upvault_v1, the ML‑KEM public key is re-parsed from bytes (MlKemPublicKey::from_bytes(mlkem_public.as_bytes().to_vec())) inside the secrets loop. This is unnecessary (you already have mlkem_public: MlKemPublicKey) and adds repeated allocations/work for every secret. Parse once outside the loop (or just pass &mlkem_public) to keep migration linear and simpler.

Suggested change

let encap = hybrid_encapsulate(

&MlKemPublicKey::from_bytes(mlkem_public.as_bytes().to_vec())?,

&x25519_public,

)?;

let encap = hybrid_encapsulate(&mlkem_public, &x25519_public)?;

Copilot · 2026-03-05T22:28:13Z

src/vault/migration.rs

+    // Symlink protection: refuse to write through a symlink
+    if let Ok(meta) = fs::symlink_metadata(&backup_path) {
+        if meta.file_type().is_symlink() {
+            bail!(
+                "Refusing to write backup through symlink: {}",
+                backup_path.display()
+            );
+        }
+    }
+
+    fs::copy(vault_path, &backup_path)
+        .with_context(|| format!("Failed to create vault backup at {}", backup_path.display()))?;
+


create_backup performs a symlink_metadata check and then calls fs::copy. This check is vulnerable to a TOCTOU race: an attacker (or concurrent process) could replace the backup path with a symlink between the check and the copy, defeating the stated symlink protection. Consider creating the backup file with OpenOptions using platform no-follow semantics (e.g., O_NOFOLLOW on Unix via std::os::unix::fs::OpenOptionsExt) and then copying bytes into the opened file, or write to a securely-created temp file in the same dir and atomically persist it to the final backup name.

Suggested change

// Symlink protection: refuse to write through a symlink

if let Ok(meta) = fs::symlink_metadata(&backup_path) {

if meta.file_type().is_symlink() {

bail!(

"Refusing to write backup through symlink: {}",

backup_path.display()

);

}

}

fs::copy(vault_path, &backup_path)

.with_context(|| format!("Failed to create vault backup at {}", backup_path.display()))?;

// Write backup to a temporary file in the same directory, then atomically

// rename it into place. This avoids writing through a potentially malicious

// symlink at `backup_path`.

let tmp_backup_name = format!("{}.backup.{}.{}.tmp", stem, timestamp, ext);

let tmp_backup_path = parent.join(&tmp_backup_name);

// Open source vault file for reading

let mut src = std::fs::File::open(path).with_context(|| {

format!("Failed to open source vault for backup at {}", path.display())

})?;

// Create a new temporary backup file; `create_new(true)` ensures we do not

// follow or overwrite any existing file/symlink at this path.

let mut dst = std::fs::OpenOptions::new()

.write(true)

.create_new(true)

.open(&tmp_backup_path)

.with_context(|| {

format!(

"Failed to create temporary vault backup at {}",

tmp_backup_path.display()

)

})?;

// Stream-copy the contents from the source vault into the temporary backup.

std::io::copy(&mut src, &mut dst).with_context(|| {

format!(

"Failed to write vault backup data to temporary file at {}",

tmp_backup_path.display()

)

})?;

// Atomically move the completed temporary backup into its final location.

fs::rename(&tmp_backup_path, &backup_path).with_context(|| {

format!(

"Failed to finalize vault backup from {} to {}",

tmp_backup_path.display(),

backup_path.display()

)

})?;

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8508993c4

ℹ️ 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".

chatgpt-codex-connector · 2026-03-05T22:29:47Z

src/vault/migration.rs

+        4 => {
+            let v4: Vault =
+                serde_json::from_str(original_json).context("Failed to parse v4 vault")?;
+            upvault_v4(v4, probe.version, &migration_path)?


Validate passphrase before migrating v4 vaults

In the version == 4 branch, migration only deserializes JSON and immediately proceeds to create_backup/save_vault_file later in upvault, so a wrong passphrase is never detected before disk writes. This means an unlock attempt with an incorrect passphrase can still rewrite the vault (and create/prune backups) before failing during the subsequent unlock path, violating the stated security invariant that failed authentication must not mutate disk state.

Useful? React with 👍 / 👎.

…lback Consolidates security improvements from three independent PR branches: - security.rs: SecretString/SecretVec zeroizing wrappers, OS hardening (disable core dumps, ptrace, mlockall), signal handlers for graceful shutdown ensuring ZeroizeOnDrop fires - Key commitment: HMAC-SHA256 over KDF params + public keys, verified at unlock to detect tampering before decryption - Anti-rollback: min_version field prevents opening vault with older tools - SecretString wrapping for all passphrases in CLI handlers - Upgrade command for explicit vault version migration - Migration engine (upvault) now sets key_commitment and min_version All 60 tests pass. https://claude.ai/code/session_01CyG4gMbVH7AxwFkzHeVyfH

johnzfitch · 2026-03-07T21:25:10Z

@copilot fix Ci tests

Copilot · 2026-03-07T21:25:16Z

@johnzfitch I've opened a new pull request, #8, to work on those changes. Once the pull request is ready, I'll request review from you.

Copilot AI review requested due to automatic review settings March 5, 2026 22:24

Copilot started reviewing on behalf of johnzfitch March 5, 2026 22:25 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

chatgpt-codex-connector bot reviewed Mar 5, 2026

View reviewed changes

Copilot AI mentioned this pull request Mar 7, 2026

Fix CI: clippy needless_borrows and rustfmt formatting #8

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vault migration engine for automatic version upgrades#7

Add vault migration engine for automatic version upgrades#7
johnzfitch wants to merge 2 commits intomainfrom
claude/vault-migration-security-VShCS

johnzfitch commented Mar 5, 2026

Uh oh!

johnzfitch commented Mar 5, 2026

Uh oh!

johnzfitch commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

chatgpt-codex-connector bot Mar 5, 2026

Uh oh!

johnzfitch commented Mar 7, 2026

Uh oh!

Copilot AI commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	let mlkem_private = MlKemPrivateKey::from_bytes(mlkem_sk_bytes.to_vec())?;
	let mlkem_private = MlKemPrivateKey::from_bytes(mlkem_sk_bytes.as_slice())?;

-    // Symlink protection: refuse to write through a symlink
-    if let Ok(meta) = fs::symlink_metadata(&backup_path) {
-        if meta.file_type().is_symlink() {
-            bail!(
-                "Refusing to write backup through symlink: {}",
-                backup_path.display()
-            );
-        }
-    }
-    fs::copy(vault_path, &backup_path)
-        .with_context(|| format!("Failed to create vault backup at {}", backup_path.display()))?;
+    // Write backup to a temporary file in the same directory, then atomically
+    // rename it into place. This avoids writing through a potentially malicious
+    // symlink at `backup_path`.
+    let tmp_backup_name = format!("{}.backup.{}.{}.tmp", stem, timestamp, ext);
+    let tmp_backup_path = parent.join(&tmp_backup_name);
+    // Open source vault file for reading
+    let mut src = std::fs::File::open(path).with_context(|| {
+        format!("Failed to open source vault for backup at {}", path.display())
+    })?;
+    // Create a new temporary backup file; `create_new(true)` ensures we do not
+    // follow or overwrite any existing file/symlink at this path.
+    let mut dst = std::fs::OpenOptions::new()
+        .write(true)
+        .create_new(true)
+        .open(&tmp_backup_path)
+        .with_context(|| {
+            format!(
+                "Failed to create temporary vault backup at {}",
+                tmp_backup_path.display()
+            )
+        })?;
+    // Stream-copy the contents from the source vault into the temporary backup.
+    std::io::copy(&mut src, &mut dst).with_context(|| {
+        format!(
+            "Failed to write vault backup data to temporary file at {}",
+            tmp_backup_path.display()
+        )
+    })?;
+    // Atomically move the completed temporary backup into its final location.
+    fs::rename(&tmp_backup_path, &backup_path).with_context(|| {
+        format!(
+            "Failed to finalize vault backup from {} to {}",
+            tmp_backup_path.display(),
+            backup_path.display()
+        )
+    })?;

Conversation

johnzfitch commented Mar 5, 2026

Summary

Key Changes

Security Invariants

Migration Path Examples

Uh oh!

johnzfitch commented Mar 5, 2026

Uh oh!

johnzfitch commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector bot Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

johnzfitch commented Mar 7, 2026

Uh oh!

Copilot AI commented Mar 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants