feat: secure password cache #7
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new security subsystem introduces per-profile and global password caching with age-based encryption, replacing direct password prompts across backup, restore, and validate tasks. ANSI escape code stripping is added for clean output, and .gitignore is extended to exclude the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Task as Task (backup/restore/validate)
participant Cache as Password Cache
participant Encryption as age Encryption
participant Identity as Identity Manager
participant Prompt as User Prompt
User->>Task: Initiate encrypted operation
Task->>Cache: Check cached password
alt Cache hit & not expired
Cache->>Task: Return cached password
Task->>User: Proceed with operation
else Cache miss or expired
Task->>Prompt: Request password from user
Prompt->>User: Display prompt
User->>Prompt: Enter password
Prompt->>Task: Return password input
Task->>Identity: Get or create cache identity
Identity->>Encryption: Encrypt password with identity
Encryption->>Cache: Store encrypted password + TTL
Task->>User: Proceed with operation
end
alt Operation failure (password-related)
Task->>Cache: Invalidate cached password
Cache-->>Task: Cache cleared
Task->>User: Log error & warn user
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tasks/restore.rs (1)
316-342:⚠️ Potential issue | 🟠 MajorAfter detecting a bad password, the loop continues decrypting remaining files with the same stale password.
When
is_password_erroris true on Line 323, the cache is invalidated — good. But the loop keeps iterating over the remaining entries using the samepasswordvalue, which will almost certainly fail for every subsequent file with the same error, producing noisy warnings and redundant cache-invalidation calls.Consider breaking out of the loop (or at least short-circuiting the remaining encrypted entries) after the first password-related failure, since retrying with the same password is pointless.
Proposed fix
Err(e) => { let err_msg = e.to_string(); if is_password_error(&err_msg) { invalidate_password_cache(profile); + log_warning(&format!( + "Failed to restore encrypted {}: {}", + entry.name, e + )); + log_warning("Password error detected — skipping remaining encrypted restores"); + skipped_count += 1; + // Count remaining entries as skipped + // (they would fail with the same password) + break; } log_warning(&format!( "Failed to restore encrypted {}: {}", entry.name, e )); skipped_count += 1; }Note: you'll need to account for the remaining unprocessed entries in
skipped_countafter thebreak.
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Around line 5-8: The changelog entry for v2.3.0 is missing the primary feature
from this PR: the secure password caching subsystem; update CHANGELOG.md under
v2.3.0 to add an "Added" section describing the new secure password cache
(mentioning the implemented types/APIs such as PasswordCache or
SecurePasswordStore and the public methods like cachePassword and
getCachedPassword or equivalent) and a brief note about security/expiry behavior
so users know it's available and how it behaves.
In `@src/security.rs`:
- Around line 291-303: The current code writes the encrypted bytes to cache_path
with default permissions (fs::write) then tightens them, creating a TOCTOU
window; change it to write to a temporary file in the same directory (e.g.,
cache_path.with_extension("tmp") or a generated unique name), write the
encrypted bytes using the existing encryptor.wrap_output/write_all/finish flow
into that temp file, set Unix permissions
(std::fs::Permissions::from_mode(0o600)) on the temp file, flush/sync the temp
file if needed, then atomically rename (std::fs::rename) the temp file to
cache_path so the final cache_path is created with correct permissions and no
window exists between write and permission change (refer to variables/functions:
encrypted, writer, encryptor.wrap_output, cache_path, fs::set_permissions).
- Around line 174-186: The function get_password_cache_path currently only
replaces '/' in profile.name which is insufficient; create and call a sanitizer
(e.g., sanitize_profile_name) used inside get_password_cache_path to convert
profile.name to a safe basename: decode Option with unwrap_or("common"), trim
whitespace, reject or replace null bytes and path separators (both '/' and
'\\'), collapse/normalize sequences of "..", and then apply a whitelist (e.g.,
allow only ASCII letters, digits, '-', '_', and '.') falling back to a safe
default like "common" if the result is empty or too long; reference
get_password_cache_path, ActiveProfile.name, and PASSWORD_CACHE_FILE_PREFIX when
updating code.
- Around line 308-330: The private key file is created with default umask then
permissions are tightened after writing, creating a TOCTOU window; change
get_or_create_cache_identity to create the key file atomically with restrictive
permissions: on Unix use std::fs::OpenOptions +
std::os::unix::fs::OpenOptionsExt and call
.create_new(true).write(true).mode(0o600).open(&key_path) to obtain a File,
write identity.to_string() via file.write_all(...)? and call file.sync_all()
(and drop the file) instead of fs::write, falling back to the existing fs::write
+ set_permissions behavior on non-Unix; reference symbols:
get_or_create_cache_identity, PASSWORD_CACHE_KEY_FILE, key_path.
🧹 Nitpick comments (6)
src/security.rs (3)
79-83:PasswordCacheRecordstores the password as a plainStringin the serialization struct.While the record is encrypted before hitting disk, the plaintext
passwordfield inPasswordCacheRecord(and the serialized JSONVec<u8>) will remain in memory until dropped, with no zeroing guarantee. Consider usingsecrecy::Zeroizeon thedecryptedbuffer inload_cached_password(line 222) and onserializedinstore_cached_password(line 289) after use.This is a defense-in-depth concern rather than a critical vulnerability.
25-30:ProfileSecurityConfigstruct only has one optional field.Not an issue now, but the struct seems like a thin wrapper. If more per-profile security settings are planned, this is fine as-is. Otherwise, consider a note/comment about intended extensibility.
332-344: Test coverage is minimal — onlyas_seconds()is tested.The core logic (load/store/invalidate cache, identity management, TTL expiry) has no test coverage. Consider adding tests for at least
SecurityConfig::default()serialization round-trip and TTL expiry logic.Would you like me to open an issue to track adding integration tests for the security subsystem?
src/utils/system.rs (1)
26-30: Regex is recompiled on every call — consider using a staticLazyLock.
Regex::newparses and compiles the pattern each timestrip_ansi_codesis invoked. If this is called frequently (e.g., for each package manager output), that's wasteful.std::sync::LazyLockis stable as of Rust 1.80.0 and provides an efficient solution.♻️ Proposed fix using std::sync::LazyLock
+use std::sync::LazyLock; + +static ANSI_RE: LazyLock<Regex> = LazyLock::new(|| { + Regex::new(r"\x1B\[[0-?]*[ -/]*[`@-`~]").unwrap() +}); + /// Strips ANSI escape codes from a string. pub fn strip_ansi_codes(input: &str) -> String { - // Matches CSI (Control Sequence Introducer) ANSI escape codes. - let re = Regex::new(r"\x1B\[[0-?]*[ -/]*[`@-`~]").unwrap(); - re.replace_all(input, "").to_string() + ANSI_RE.replace_all(input, "").to_string() }src/tasks/backup.rs (1)
174-175: Good addition, butstrip_ansi_codesrecompiles aRegexon every call.The underlying
strip_ansi_codesinsrc/utils/system.rscallsRegex::new(…).unwrap()each invocation. In a loop over many package managers this is wasteful. Consider usinglazy_static!orstd::sync::LazyLock(stable since Rust 1.80) to compile the regex once.Since the fix lives in
src/utils/system.rs(not this file), flagging here for awareness.src/tasks/restore.rs (1)
347-350:is_password_errorheuristic is reasonable but brittle.String-matching on error messages couples this to the
agecrate's internal wording. If the crate changes its error text, this silently stops working. Consider adding a comment noting the dependency onageerror strings, and/or adding a unit test that verifies the expected error substring from the currentageversion.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new security layer for password handling (encrypted, TTL-based password caching) and integrates it into the backup/restore/validate workflows, while also cleaning package registry output by stripping ANSI codes.
Changes:
- Added
securitymodule that caches the encryption password in~/.mntn/.secrets/(encrypted withage) with configurable TTL and invalidation support. - Updated backup/restore/validate tasks to use cached password prompting and to invalidate the cache on suspected password/decrypt failures.
- Added ANSI escape stripping for package-manager command output and ensured
.secrets/is excluded from git sync via.gitignoremanagement.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
src/utils/system.rs |
Adds strip_ansi_codes helper + unit test. |
src/utils/paths.rs |
Adds .secrets + security.json paths/helpers + tests. |
src/security.rs |
New encrypted password cache + security config logic + minimal tests. |
src/tasks/backup.rs |
Switches to cached password prompting; strips ANSI from package outputs. |
src/tasks/restore.rs |
Switches to cached password prompting; invalidates cache on password-like errors. |
src/tasks/validate.rs |
Switches to cached password prompting; invalidates cache on decrypt/password-like errors. |
src/tasks/sync.rs |
Adds .secrets/ to default .gitignore and appends it if missing. |
src/main.rs |
Registers the new security module. |
CHANGELOG.md |
Adds a v2.3.0 entry (currently only documents ANSI stripping). |
Cargo.lock |
Dependency lockfile updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores