Conversation
Resolved unresolved merge conflict markers in ops.rs and commands.rs that prevented the project from compiling. These conflicts were left over from previous merges and included duplicate code sections and syntax errors. Changes: - Remove duplicate PermissionsExt import in ops.rs - Add missing MIN_VAULT_VERSION import - Add Zeroize trait import for explicit .zeroize() calls - Remove duplicate key commitment function definition - Fix type dereferencing in derive_wrapping_keys (*mlkem_key, *x25519_key) - Fix X25519 key type conversion with explicit TryInto - Make compute_key_commitment pub(crate) for migration module access - Fix syntax error in commands.rs match statement (removed extra ')') - Remove merge conflict markers (<<<<<<< HEAD, ======, etc.) The binary now compiles successfully and all tests pass.
There was a problem hiding this comment.
Pull request overview
This PR cuts over the vault format to v6 for the 1.0.0 release, switching the write path to “real” FIPS 203 ML‑KEM‑768 + X25519, and extending the migration engine to support upgrading v1–v5 → v6 on unlock.
Changes:
- Introduces v6 vault semantics (suite + explicit X25519 algorithm metadata) and a new v6 header commitment construction.
- Adds a full migration chain through v5→v6, including legacy Kyber compatibility for older vaults.
- Updates CLI/TUI/info output and docs to reflect v6 + 1.0.0 release details.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vault/ops.rs | v6 create/unlock/validation paths, versioned wrapping keys + commitments, and expanded tests |
| src/vault/migration.rs | Reworks migration engine to terminate at v6 and adds v5→v6 re-keying using legacy Kyber compatibility |
| src/vault/legacy.rs | Adds legacy shapes up through v5 for deserialization/migration support |
| src/vault/format.rs | Defines v5/v6 constants and adds v6 fields (suite, x25519.algorithm) with serde defaults |
| src/crypto/mlkem.rs | Switches to ml-kem crate for “real” ML‑KEM‑768 implementation used by v6 |
| src/crypto/legacy_kyber.rs | Adds Kyber768 compatibility layer for v2–v5 migration |
| src/crypto/hybrid.rs | Splits v6 vs legacy hybrid encapsulation/decapsulation with distinct HKDF labels |
| src/crypto/mod.rs | Exposes legacy Kyber module |
| src/cli/mod.rs | Updates CLI metadata text for v6 |
| src/cli/commands.rs | Updates info output for v6 fields/commitment and adds commitment description helper |
| src/tui/mod.rs | Updates info output for v6 fields/commitment and adds commitment description helper |
| src/main.rs | Updates crate-level description to reflect v6/migration behavior |
| README.md | Updates docs to describe v6 format, authenticated header, and automatic migration behavior |
| Cargo.toml | Bumps to 1.0.0 and adds ml-kem + hmac deps |
| Cargo.lock | Locks new dependencies for v6 crypto stack |
| AGENTS.md | Updates repo docs to reflect v6 as current format and the new header authentication semantics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let master_key = derive_key(passphrase, &kdf_config)?; | ||
|
|
||
| vault.key_commitment = Some(compute_key_commitment( | ||
| &master_key, | ||
| &vault.kdf, | ||
| &vault.kem.public_key, | ||
| &vault.x25519.public_key, | ||
| )); | ||
| vault.version = VAULT_VERSION; | ||
| vault.key_commitment = Some(compute_key_commitment(&master_key, &vault)?); | ||
| save_vault_file(vault_path, &vault)?; |
There was a problem hiding this comment.
migrate_vault currently bumps vault.version to the current VAULT_VERSION (v6) and recomputes key_commitment without re-wrapping/re-keying private keys, updating suite/x25519.algorithm, or re-encrypting secrets. For v4/v5 inputs this would write an on-disk file that claims to be v6 but does not satisfy v6 validation and may become unreadable. Consider either removing this function, restricting it to v6-only maintenance, or delegating to migration::upvault (full vN→v6 chain) before saving.
| use std::collections::HashMap; | ||
| use std::os::unix::fs as unix_fs; | ||
| use tempfile::{NamedTempFile, tempdir}; |
There was a problem hiding this comment.
The test module imports std::os::unix::fs unconditionally and uses unix_fs::symlink, which will fail to compile on non-Unix targets. Consider gating the import and the symlink-related tests with #[cfg(unix)] (or providing a Windows implementation via std::os::windows::fs).
| fn describe_key_commitment(vault: &crate::vault::format::Vault) -> &'static str { | ||
| match (vault.version, vault.key_commitment.is_some()) { | ||
| (6.., true) => "HMAC-SHA256 (present)", | ||
| (6.., false) => "HMAC-SHA256 (absent)", | ||
| (5, true) => "legacy HKDF-based v5 commitment (present)", |
There was a problem hiding this comment.
describe_key_commitment duplicates the same logic that also exists in src/cli/commands.rs. Consider moving this to a shared helper (e.g., vault::format or vault::ops) so the CLI and TUI stay consistent if commitment semantics change again.
| fn describe_key_commitment(vault: &crate::vault::format::Vault) -> &'static str { | ||
| match (vault.version, vault.key_commitment.is_some()) { | ||
| (6.., true) => "HMAC-SHA256 (present)", | ||
| (6.., false) => "HMAC-SHA256 (absent)", | ||
| (5, true) => "legacy HKDF-based v5 commitment (present)", | ||
| (5, false) => "legacy HKDF-based v5 commitment (absent)", |
There was a problem hiding this comment.
describe_key_commitment duplicates the same logic that also exists in src/tui/mod.rs. Consider moving this to a shared helper (e.g., vault::format or vault::ops) so the CLI and TUI stay consistent if commitment semantics change again.
|
@codex security review |
![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: c40fffed30
ℹ️ 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".
| 5 => { | ||
| let v5: Vault = | ||
| serde_json::from_str(original_json).context("Failed to parse v5 vault")?; | ||
| upvault_v5_to_v6(v5, probe.version, &migration_path, &master_key)? |
There was a problem hiding this comment.
The newly added version == 5 migration path routes all v5 unlocks through upvault_v5_to_v6, but upvault derives the Argon2 key from parsed JSON before any bounds validation (memory_cost, time_cost, etc.). Because parse_kdf_params forwards raw values, a tampered v5 vault can force very expensive Argon2 work (CPU/RAM DoS) before commitment verification, whereas the prior direct-v5 unlock path validated KDF limits first.
Useful? React with 👍 / 👎.
Summary
v6vaults backed by real FIPS 203 ML-KEM-768 + X25519v5 -> v6, including fresh key re-wrap/re-keying and strictv6validationv6semantics and the1.0.0releaseVerification
Notes
c40fffeand does not include the later interactive shell UX follow-up commit (5eb5556)