Increase Argon2id memory cost from 64 MB to 256 MB#28
Conversation
- Change Params::new(65536, ...) to Params::new(262144, ...) in derive_key() and derive_key_v3() - Update comment in V3 header documentation - Update README.md Technical Specifications, Memory Profile, Security Stack, and Threat Model sections - Update Key Derivation Details diagram This matches the SECURITY_DEFER.md recommendation for stronger memory-hard KDF.
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIncreases the Argon2id memory cost from 64 MiB to 256 MiB for both V2 and V3 key derivation functions and updates all related documentation, comments, and diagrams to reflect the new memory profile and security posture. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughArgon2id memory cost is raised from 65,536 KiB (64 MiB) to 262,144 KiB (256 MiB) in both ChangesArgon2id memory cost bump: code and docs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The Argon2 parameters are now duplicated in both
derive_keyandderive_key_v3; consider centralizing them in a single constant/helper (e.g.,ARGON2_PARAMS) so future tuning only needs to be done in one place. - With the memory cost increased to 256 MiB, Argon2 may fail more often on low-memory systems; it might be helpful to map
CryptoError::Argon2Failedspecifically for memory-allocation errors into a clearer, user-facing error message (e.g., "not enough RAM for key derivation").
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Argon2 parameters are now duplicated in both `derive_key` and `derive_key_v3`; consider centralizing them in a single constant/helper (e.g., `ARGON2_PARAMS`) so future tuning only needs to be done in one place.
- With the memory cost increased to 256 MiB, Argon2 may fail more often on low-memory systems; it might be helpful to map `CryptoError::Argon2Failed` specifically for memory-allocation errors into a clearer, user-facing error message (e.g., "not enough RAM for key derivation").Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| let mut intermediate = Zeroizing::new(vec![0u8; 64]); | ||
| let params = Params::new(65_536, 3, 4, Some(64)) | ||
| let params = Params::new(262_144, 3, 4, Some(64)) |
There was a problem hiding this comment.
🔥 The Roast: You've cranked the Argon2id memory from 64 MB to 256 MB — a 4× jump with zero warning to existing users. Every file encrypted with the old version is now permanently unreadable because the key derivation output will differ. This isn't an upgrade; it's a silent data hostage situation. 🎤
🩹 The Fix: Either bump the file format version (e.g. V4) so new files use 256 MB while V2/V3 files still decrypt with 64 MB, or at minimum add a colossal prerelease warning that all previously encrypted data will be orphaned. Hardcoding different Argon2id parameters without version detection is a critical breaking change for an encryption tool.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| let mut final_key = Zeroizing::new(vec![0u8; 32]); | ||
| let mut intermediate = Zeroizing::new(vec![0u8; 64]); | ||
| let params = Params::new(65_536, 3, 4, Some(64)) | ||
| let params = Params::new(262_144, 3, 4, Some(64)) |
There was a problem hiding this comment.
🔥 The Roast: Same 4× memory jump to 256 MB copy-pasted into derive_key_v3. Nothing says "we've thought this through" like repeating the same breaking change in a second function. V3 files encrypted with the previous release are equally unreadable now. Two times the breaking change, zero times the migration path. 🎤
🩹 The Fix: Same as above — version the file format so old V3 files still derive keys with the original 64 MB parameters, or provide a data migration tool. Don't silently break decryption for existing users.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review Roast 🔥Verdict: 2 Critical Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)
🏆 Best part: The docs were actually updated consistently with the code. In a world where many PRs lie about memory usage in the README, that's almost impressive enough to make me lower my flamethrower. Almost. 💀 Worst part: Hardcoding different Argon2id parameters without file-format version detection is the encryption equivalent of changing your door lock and throwing away the old keys. Every .vx2 file encrypted before this merge becomes unreadable ornament with zero warning to users. 📊 Overall: Like upgrading to a new security system and forgetting to keep the old key on your keychain — technically stronger, but you've just locked yourself out of everything you owned. Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash-20260528 · Input: 153.7K · Output: 11.2K · Cached: 3.8K |
![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: ee2795054f
ℹ️ 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 mut final_key = Zeroizing::new(vec![0u8; 32]); | ||
| let mut intermediate = Zeroizing::new(vec![0u8; 64]); | ||
| let params = Params::new(65_536, 3, 4, Some(64)) | ||
| let params = Params::new(262_144, 3, 4, Some(64)) |
There was a problem hiding this comment.
When a user decrypts any VAULTX03 file produced by previous builds, the unchanged header only tells decrypt_file to dispatch to the V3 path; it does not store Argon2 parameters or use a new magic/version. Because those files were sealed with m=65_536, deriving with m=262_144 here produces a different key and all existing V3 ciphertexts fail authentication, making users' encrypted files unrecoverable unless a fallback or versioned format is added.
Useful? React with 👍 / 👎.
|
|
||
| let mut intermediate = Zeroizing::new(vec![0u8; 64]); | ||
| let params = Params::new(65_536, 3, 4, Some(64)) | ||
| let params = Params::new(262_144, 3, 4, Some(64)) |
There was a problem hiding this comment.
This function is also used by decrypt_file_legacy/decrypt_stream_v2, but VAULTX02 headers only contain magic, salt, and nonce and all historical V2 files were encrypted with m=65_536. Switching the legacy derivation to m=262_144 means the documented backwards-compatible V2 decryption path now derives the wrong key and rejects every existing legacy file as DecryptionFailed.
Useful? React with 👍 / 👎.
1 participant
This matches the SECURITY_DEFER.md recommendation for stronger memory-hard KDF.
Summary by Sourcery
Increase the Argon2id memory cost used for key derivation to strengthen resistance against offline brute-force attacks and update documentation to reflect the new memory profile.
Enhancements:
Documentation:
Summary by CodeRabbit