Add vault key rotation support #131
Merged
Conversation
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
WalkthroughAdds end-to-end vault rotation: CLI commands to rotate vault password and data key, new Keep APIs that call Storage rotation routines, storage-level implementations for locks/backups/atomic header writes/re-encryption/verification/rollback, a rotation-specific error variant, and tests. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI
participant Keep
participant Storage
User->>CLI: rotate-password
CLI->>CLI: Prompt old/new password, validate, confirm
alt confirmed
CLI->>Keep: rotate_password(old, new)
Keep->>Storage: rotate_password(old, new)
Storage->>Storage: acquire rotation lock, backup header
Storage->>Storage: decrypt & re-encrypt DEK with new password
Storage->>Storage: atomic write new header + fsync
alt success
Storage-->>Keep: Ok
Keep->>Keep: clear & reload keyring
Keep-->>CLI: Ok
CLI-->>User: Success
else failure
Storage->>Storage: restore backups
Storage-->>Keep: RotationFailed
Keep-->>CLI: Error
CLI-->>User: Error
end
else cancelled
CLI-->>User: Cancelled
end
sequenceDiagram
actor User
participant CLI
participant Keep
participant Storage
User->>CLI: rotate-data-key
CLI->>CLI: Prompt password, confirm
alt confirmed
CLI->>Keep: rotate_data_key(password)
Keep->>Storage: rotate_data_key(password)
Storage->>Storage: acquire rotation lock, backup header+db
Storage->>Storage: decrypt keys/shares with old data key
Storage->>Storage: generate new data key & re-encrypt keys/shares
Storage->>Storage: atomic write new header + fsync
Storage->>Storage: verify integrity
alt integrity ok
Storage-->>Keep: Ok
Keep->>Keep: clear & reload keyring
Keep-->>CLI: Ok
CLI-->>User: Success
else integrity fail
Storage->>Storage: restore backups
Storage-->>Keep: RotationFailed
Keep-->>CLI: Error
CLI-->>User: Error
end
else cancelled
CLI-->>User: Cancelled
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
Closed
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
wksantiago
force-pushed
the
key-rotation
branch
from
4915999 to
9cee378
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@keep-cli/src/commands/vault.rs`:
- Around line 598-648: Add an explicit guard for hidden vaults in
cmd_rotate_password and cmd_rotate_data_key: before proceeding (ideally before
calling Keep::open or immediately after), detect whether the provided path
corresponds to a hidden vault (use the existing helper such as is_hidden_path /
Vault::is_hidden if available, or add one) and return an error or reject the
operation with a clear message if hidden vaults are passed; alternatively, if
hidden-vault rotation is supported, call the hidden-vault rotation flow instead
of the normal Keep::rotate_password / Keep::rotate_data_key paths so you don't
operate on the wrong storage type.
In `@keep-core/src/storage.rs`:
- Around line 423-433: secure_delete currently allocates the whole file via
vec![0u8; metadata.len() as usize], which OOMs for large files; change
secure_delete to allocate a fixed-size zero buffer (e.g., const CHUNK: usize =
8*1024) once and loop writing chunks until metadata.len() bytes are written:
seek to start with file.seek(SeekFrom::Start(0))?, compute remaining =
metadata.len(), then while remaining > 0 write std::cmp::min(CHUNK as u64,
remaining) bytes from the zero buffer, subtracting from remaining each
iteration, call file.sync_all() after the loop, and finally call
fs::remove_file(path); keep the function signature secure_delete and error
propagation as-is.
- Around line 522-576: The rotate_data_key implementation can leave the DB
partially re-encrypted if an error occurs mid-rotation; wrap the entire rotation
sequence (from copying backups through updating self.header/self.data_key and
verification) in a single Result-returning block (e.g., let result = (|| ->
Result<()> { ... })();), and on any Err perform a deterministic rollback:
restore header and DB from backup (fs::copy backup_path -> header_path and
db_backup_path -> db_path), reset self.header and self.data_key to
old_header/old_data_key, call self.lock() and self.unlock(password) as needed,
and ensure backups are securely deleted; finally return the original error. Use
the existing symbols rotate_data_key, acquire_rotation_lock, reencrypt_database,
list_keys, list_shares, decrypt_all_keys, decrypt_all_shares,
write_header_atomically, secure_delete to locate and wrap the core steps so
failures in reencrypt_database/list_keys/list_shares/etc. always trigger the
rollback before returning.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@keep-core/src/storage.rs`:
- Around line 441-449: fsync_dir currently swallows errors by returning Ok(())
when path.parent() is None or File::open(parent) fails, which hides fsync
failures from callers like write_header_atomically; change fsync_dir to
propagate errors instead: if parent() is None return an io::Error (e.g.,
io::ErrorKind::Other with a clear message) and do not ignore File::open errors
(use ? to propagate), then return the result of dir.sync_all(); ensure callers
like write_header_atomically observe and handle the propagated io::Result so
rotation only reports success when metadata was durably flushed.
- Around line 475-515: The rotation currently releases the rotation lock early
(the explicit drop(lock) before verification), which allows concurrent rotations
during verification/rollback; modify rotate_password so the lock acquired by
acquire_rotation_lock(&self.path) is held until after the new-password
verification and any rollback/cleanup (secure_delete of backup) complete — i.e.,
remove the early drop(lock) and only drop the lock (or let it go out of scope)
at the very end of rotate_password, ensuring every error/rollback path still
releases the lock (use a finally-style cleanup or ensure drop(lock) runs after
restoring old_header and secure_delete); refer to the rotate_password function,
the acquire_rotation_lock call, the old_header/header_path/backup_path handling,
and the unlock(new_password) verification/rollback branch when relocating the
lock release.
- Around line 531-596: The rotation lock is released early by drop(lock) before
the rollback/cleanup path, allowing a concurrent rotation to start during
rollback; keep the lock held until after rollback and backup cleanup by removing
the early drop(lock) and only dropping (or letting) the lock go after the error
handling finishes (i.e., after restoring header/db, unlocking, and secure_delete
calls) in rotate_data_key so the lock variable created by
acquire_rotation_lock(&self.path) remains in scope through both the success and
error paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-core/src/storage.rs`:
- Around line 478-594: Both rotate_password and rotate_data_key are vulnerable
to TOCTOU because they call unlock() and read header/data_key before acquiring
the rotation lock; move acquire_rotation_lock(&self.path)? to the very start of
each function (rotate_password and rotate_data_key) and only read/clone
self.header and self.data_key after the lock is held, then proceed with fs::copy
backups, creation of new header/data key, atomic writes, and restores on error;
ensure you drop the lock and perform secure_delete in all failure and success
paths as before.
- rotate_password: verify header decryption inline instead of close/reopen database cycle that fails on Windows - rotate_data_key: close database before fs::copy and reopen after, since redb holds exclusive lock on Windows
Add retry logic with backoff for file operations that may encounter transient file locking issues on Windows when redb database handles are being released.
kwsantiago
approved these changes
Jan 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-core/src/storage.rs`:
- Around line 624-629: After setting self.backend = None and before calling
copy_with_retry on header_path/backup_path and db_path/db_backup_path, ensure
that any early-returning error from copy_with_retry or the subsequent reopen
path attempts to reopen the backend first as a best-effort: on any error from
copy_with_retry or the copy sequence, call RedbBackend::open(&db_path) and
assign Some(Box::new(...)) back to self.backend (ignore reopen errors), then
return the original error; this ensures the instance is not left without a
backend when copy_with_retry or reopen fails.
🧹 Nitpick comments (1)
keep-core/src/rate_limit.rs (1)
151-160: Consider enforcing 0600 on existing rate-limit files.
mode(0o600)only applies on creation; a pre-existing file with broader permissions keeps them. If you want consistent hardening, explicitly set permissions after open on Unix.Suggested adjustment
fn open_rate_limit_file(path: &Path) -> std::io::Result<File> { let mut opts = OpenOptions::new(); opts.read(true).write(true).create(true).truncate(false); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; opts.mode(0o600); } - opts.open(path) + let file = opts.open(path)?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.set_permissions(fs::Permissions::from_mode(0o600))?; + } + Ok(file) }
- Fix silent backup restoration failures in rotation rollback - Add restricted file permissions (0600) on rate limit file - Add comprehensive integrity verification with content comparison - Add restricted directory permissions (0700) on storage creation - Clean up lock file after successful rotation - Extract helpers to reduce code duplication
kwsantiago
force-pushed
the
key-rotation
branch
from
afbb89b to
9ecf3a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@keep-core/src/storage.rs`:
- Around line 499-502: The cleanup_rotation_lock function currently deletes the
".rotation.lock" file which creates a race (between drop(lock) and remove_file)
that can allow concurrent rotations; change the behavior so the lock file is not
removed—make cleanup_rotation_lock a no-op (or remove calls to
fs::remove_file(".rotation.lock")) and ensure any other places that call
cleanup_rotation_lock or directly call fs::remove_file on ".rotation.lock"
(e.g., the other similar usages) stop deleting the file; continue relying solely
on the file lock held by the lock object rather than unlinking the lock file.
🧹 Nitpick comments (1)
keep-core/src/storage.rs (1)
702-729: Minimize plaintext lifetime during rotation.
decrypt_all_keys/decrypt_all_sharesmaterialize secrets intoVec<u8>, which won’t be zeroized. Consider keeping secrets inSecretVec(or explicitly zeroizing buffers) to reduce memory exposure during rotation.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
keep rotate-passwordcommand to re-derive DEK with new passwordkeep rotate-data-keycommand for periodic DEK rotationTest plan
Summary by CodeRabbit
New Features
Reliability / Integrity
Tests
✏️ Tip: You can customize this high-level summary in your review settings.