Add portable vault backup and restore#297
Merged
kwsantiago merged 5 commits intomainfrom Feb 28, 2026
Merged
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughAdds vault backup and restore features: new CLI subcommands, CLI dispatch, vault command handlers, a core backup module (create/verify/restore with Argon2 encryption), desktop and mobile UI/bridge changes, and Keep methods to restore pre-encrypted records/shares. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI Handler
participant Vault as Vault Command
participant Backup as Core Backup
participant FS as File System
User->>CLI: backup --output /path/to/backup
CLI->>Vault: cmd_backup(output_path)
Vault->>Vault: Prompt for vault password & unlock
Vault->>Vault: Prompt for backup passphrase (confirm & validate)
Vault->>Backup: create_backup(keep, passphrase)
Backup->>Backup: Serialize state, derive key (Argon2), encrypt, compute hash
Backup->>FS: Return encrypted bytes to Vault
Vault->>FS: Write to chosen output path
FS-->>Vault: Persisted
Vault-->>User: Display backup metadata / success
sequenceDiagram
actor User
participant CLI as CLI Handler
participant Vault as Vault Command
participant Backup as Core Backup
participant Keep as Keep Core
participant Storage as Storage Layer
User->>CLI: restore --file /path/to/backup --target /new/vault
CLI->>Vault: cmd_restore(file, target)
Vault->>Vault: Prompt for backup passphrase
Vault->>Backup: verify_backup(data, passphrase)
Backup->>Backup: Parse header, decrypt, verify hash, return BackupInfo
Backup-->>Vault: BackupInfo
Vault->>Vault: Prompt for new vault password
Vault->>Backup: restore_backup(data, passphrase, target, vault_password)
Backup->>Keep: instantiate new Keep at target
Keep->>Storage: restore keys, shares, descriptors, configs, statuses
Storage-->>Keep: persisted
Backup-->>Vault: Success
Vault-->>User: Display restore metadata / success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-cli/src/commands/vault.rs`:
- Line 840: The code naively calls std::fs::read(file) into backup_data which
can OOM on very large backup files; first check the file size via
std::fs::metadata(file) and enforce a MAX_BACKUP_SIZE constant limit, returning
an error if the file is larger than allowed, then open the file
(std::fs::File::open) and read it with a bounded reader (e.g.,
std::io::Read::take or a BufReader and read_to_end into a preallocated buffer)
to ensure you never allocate more than the allowed size; reference symbols:
backup_data, std::fs::read, file, and use std::fs::metadata/File::open to
implement the check and bounded read.
In `@keep-core/src/backup.rs`:
- Around line 218-234: The parsed Argon2 parameters (memory_kib, iterations,
parallelism) are read from untrusted header bytes and must be validated before
any key derivation; update the header parsing logic that constructs ParsedHeader
/ Argon2Params to enforce safe bounds (e.g., min/max memory_kib, iterations, and
parallelism, and non-zero values) and return an error if any value is out of
range instead of accepting them; ensure the same validation is applied for the
additional fields referenced around the other parsed block (the ones noted at
lines ~240-243) so any downstream call that uses Argon2Params for key derivation
or verification only receives vetted parameters.
- Around line 287-368: The restore currently writes directly into the target
vault (using Keep::create and methods like keep.restore_key_record,
keep.restore_stored_share, keep.store_wallet_descriptor,
keep.store_relay_config, keep.store_health_status, keep.set_kill_switch,
keep.set_proxy_config) so a mid-run failure leaves a partially-populated vault
and future attempts hit AlreadyExists; change the flow to restore into a
temporary/isolated vault target (e.g. create a temp target path or temp Keep via
Keep::create(temp_target, vault_password)), perform all restore operations
against that temp Keep, and on success atomically replace the original vault
with the temp (or rename temp -> target); on any error, ensure you clean up the
temp target (remove temp files) and return the error so the original vault
remains untouched.
- Around line 291-294: Decoded bk.secret is used without validating its length,
allowing corrupted or wrong-size keys to be encrypted and stored; before calling
crypto::encrypt on secret_bytes, check that secret_bytes.len() matches the
expected key length (e.g., 32 bytes or the correct constant for your key type)
and return a Clear KeepError::Other on mismatch (include the actual length in
the error message) so invalid secrets are rejected rather than persisted; place
this check right after hex::decode(&bk.secret) and before crypto::encrypt to
stop bad data early.
In `@keep-core/src/lib.rs`:
- Around line 957-965: The restore_key_record and restore_stored_share methods
bypass the instance lock check and directly call self.storage.store_key /
self.storage.store_share; update both to enforce the Keep unlock-state like
other mutating APIs by checking the instance lock before writing (e.g., if the
Keep is locked return an appropriate Err(Error::Locked) or call the existing
unlock-guard helper used elsewhere such as ensure_unlocked()/check_unlocked()),
and only proceed to call self.storage.store_key or self.storage.store_share
after the lock check passes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
keep-cli/src/cli.rskeep-cli/src/commands/vault.rskeep-cli/src/main.rskeep-core/src/backup.rskeep-core/src/lib.rs
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
keep-cli/src/commands/vault.rs (1)
840-850:⚠️ Potential issue | 🟠 MajorUse a bounded file read (single-open path) to fully close the OOM gap.
The metadata size guard is helpful, but
std::fs::read(file)is still an unbounded allocation if the file is swapped between checks.🔧 Suggested hardening
- const MAX_BACKUP_SIZE: u64 = 64 * 1024 * 1024; - let meta = std::fs::metadata(file)?; + use std::io::Read; + const MAX_BACKUP_SIZE: u64 = 64 * 1024 * 1024; + let f = std::fs::File::open(file)?; + let meta = f.metadata()?; + if !meta.is_file() { + return Err(KeepError::InvalidInput("backup path is not a file".into())); + } if meta.len() > MAX_BACKUP_SIZE { return Err(KeepError::InvalidInput(format!( "backup file too large ({} bytes, max {})", meta.len(), MAX_BACKUP_SIZE ))); } - let backup_data = std::fs::read(file)?; + let mut reader = std::io::BufReader::new(f); + let mut backup_data = Vec::with_capacity(meta.len() as usize); + reader + .by_ref() + .take(MAX_BACKUP_SIZE + 1) + .read_to_end(&mut backup_data)?; + if backup_data.len() as u64 > MAX_BACKUP_SIZE { + return Err(KeepError::InvalidInput(format!( + "backup file too large (max {})", + MAX_BACKUP_SIZE + ))); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-cli/src/commands/vault.rs` around lines 840 - 850, The metadata size check followed by std::fs::read(file) leaves a TOCTOU OOM risk; open the file once, call metadata() or seek to get length from that opened File, enforce MAX_BACKUP_SIZE against that length, then read from the same File using a bounded read (e.g., set Vec capacity from the checked size and use read_to_end or use File.take(len) to limit) so that the allocation is based on the single-open file size; update uses of MAX_BACKUP_SIZE, std::fs::metadata, std::fs::read and the variable backup_data to use the opened File flow and reject files exceeding the limit.
🧹 Nitpick comments (2)
keep-desktop/src/screen/settings.rs (1)
742-751: Disable Export until passphrases match.Current enablement allows clicking Export when confirm is non-empty but mismatched, then fails in submit logic. Tighten the precondition in
can_exportfor smoother UX.🎯 Proposed fix
let can_export = !self.backup_loading && self.backup_passphrase.len() >= MIN_BACKUP_PASSPHRASE - && !self.backup_confirm.is_empty(); + && *self.backup_passphrase == *self.backup_confirm;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/screen/settings.rs` around lines 742 - 751, The Export button enablement currently uses can_export which only checks backup_confirm is non-empty, allowing mismatched passphrases; update the condition used in can_export to also require that self.backup_passphrase == self.backup_confirm (in addition to existing checks of !self.backup_loading and length >= MIN_BACKUP_PASSPHRASE) so the export_btn (the button built into export_btn with .on_press(Message::BackupExport) when can_export) is only enabled when passphrases match.keep-desktop/src/app.rs (1)
3099-3111: Decouple restore destination and new vault password from backup passphrase.Line 3099 hardcodes a single restore target and Line 3110 reuses the backup passphrase as the restored vault password. This makes repeated restores brittle and couples two different secrets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@keep-desktop/src/app.rs` around lines 3099 - 3111, The code currently hardcodes restore_dir via keep_path.with_file_name("keep-restored") and passes the backup passphrase variable (passphrase) as the restored vault password into keep_core::backup::restore_backup, which couples the two secrets and breaks repeated restores; change this by deriving a unique restore destination (e.g., keep_path.with_file_name(format!("keep-restored-{}", timestamp_or_random))) and ensure you check for existence in a loop or fail with a clear message, and obtain a separate new_vault_password (prompt or parameter) distinct from passphrase, then call keep_core::backup::restore_backup(&data, &passphrase, &restore_dir, &new_vault_password) so the backup passphrase and restored vault password are decoupled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@keep-core/src/backup.rs`:
- Around line 199-203: Reject overly large backup blobs early by adding a hard
size cap: define a MAX_BACKUP_SIZE constant and in parse_header(data: &[u8])
check if data.len() > MAX_BACKUP_SIZE and return
Err(KeepError::InvalidInput("backup file too large".into())); do the same
upper-size check at the other parse stage referenced (the block around lines
259-266 / the subsequent parse function used before decrypt_backup) so
decrypt_backup never receives unbounded input; reference parse_header and the
parse-stage function invoked prior to decrypt_backup when adding these checks.
- Around line 208-214: The parser currently checks VERSION but ignores the
header flags; read the header's flags field from the same input (the bytes
immediately before version), validate that flags == 0 and return
Err(KeepError::InvalidInput(...)) if non-zero so unsupported header semantics
are rejected; update the logic near the existing VERSION check (the code that
uses data, VERSION, and KeepError::InvalidInput) to parse the flags value and
error out on any non-zero flags.
In `@keep-desktop/src/app.rs`:
- Around line 3069-3072: The code reads the entire restore file into memory
without checking size; add a pre-read size guard: query the file size from the
provided handle (e.g., via handle.metadata().await or handle.len()/size API on
the handle) and compare it against a new constant (e.g., MAX_RESTORE_BYTES or
MAX_RESTORE_FILE_SIZE) before calling handle.read().await; if the size exceeds
the limit return an Err variant (or map to the existing error type) instead of
reading, otherwise proceed to read and return Ok((name, data)). Ensure the size
constant and error path are used in the same scope as the Some(handle) => branch
so the check happens before allocating the data buffer.
- Around line 3026-3028: The backup write currently uses tokio::fs::write which
creates files with default, potentially world-readable permissions; replace it
with a secure write that creates a temp file in the destination directory,
writes the bytes, sets POSIX permissions to 0o600 on Unix, syncs, then
atomically persists/renames to the target path (suggested helper name:
write_private_bytes); because the helper is blocking, call it from async code
via tokio::task::spawn_blocking or provide an async wrapper, and preserve the
existing error handling by mapping errors into the same "Failed to write backup:
{e}" message where the call site previously used tokio::fs::write.
---
Duplicate comments:
In `@keep-cli/src/commands/vault.rs`:
- Around line 840-850: The metadata size check followed by std::fs::read(file)
leaves a TOCTOU OOM risk; open the file once, call metadata() or seek to get
length from that opened File, enforce MAX_BACKUP_SIZE against that length, then
read from the same File using a bounded read (e.g., set Vec capacity from the
checked size and use read_to_end or use File.take(len) to limit) so that the
allocation is based on the single-open file size; update uses of
MAX_BACKUP_SIZE, std::fs::metadata, std::fs::read and the variable backup_data
to use the opened File flow and reject files exceeding the limit.
---
Nitpick comments:
In `@keep-desktop/src/app.rs`:
- Around line 3099-3111: The code currently hardcodes restore_dir via
keep_path.with_file_name("keep-restored") and passes the backup passphrase
variable (passphrase) as the restored vault password into
keep_core::backup::restore_backup, which couples the two secrets and breaks
repeated restores; change this by deriving a unique restore destination (e.g.,
keep_path.with_file_name(format!("keep-restored-{}", timestamp_or_random))) and
ensure you check for existence in a loop or fail with a clear message, and
obtain a separate new_vault_password (prompt or parameter) distinct from
passphrase, then call keep_core::backup::restore_backup(&data, &passphrase,
&restore_dir, &new_vault_password) so the backup passphrase and restored vault
password are decoupled.
In `@keep-desktop/src/screen/settings.rs`:
- Around line 742-751: The Export button enablement currently uses can_export
which only checks backup_confirm is non-empty, allowing mismatched passphrases;
update the condition used in can_export to also require that
self.backup_passphrase == self.backup_confirm (in addition to existing checks of
!self.backup_loading and length >= MIN_BACKUP_PASSPHRASE) so the export_btn (the
button built into export_btn with .on_press(Message::BackupExport) when
can_export) is only enabled when passphrases match.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
keep-cli/src/commands/vault.rskeep-core/src/backup.rskeep-core/src/lib.rskeep-desktop/Cargo.tomlkeep-desktop/src/app.rskeep-desktop/src/message.rskeep-desktop/src/screen/settings.rskeep-mobile/src/keep_mobile.udlkeep-mobile/src/lib.rs
💤 Files with no reviewable changes (1)
- keep-mobile/src/keep_mobile.udl
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit