Add rate limiting to password attempts in unlock flow#13
Conversation
WalkthroughAdds a persistent per-path on-disk rate limiter, integrates rate checks into HiddenStorage and Storage unlock flows, introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Storage
participant RateLimiter as "RateLimiter\n(on-disk)"
participant Crypto
Client->>Storage: unlock(path, password)
Storage->>RateLimiter: check_rate_limit(path)
alt rate limited
RateLimiter-->>Storage: Err(remaining)
Storage-->>Client: Err(KeepError::RateLimited(remaining_secs))
else allowed
RateLimiter-->>Storage: Ok(())
Storage->>Crypto: decrypt_master_key(password)
alt decrypt fails (auth)
Crypto-->>Storage: Err(decrypt_err)
Storage->>RateLimiter: record_failure(path)
Storage-->>Client: Err(decrypt_err)
else decrypt succeeds
Crypto-->>Storage: Ok(master_key)
Storage->>Crypto: decrypt_data_key(master_key)
Crypto-->>Storage: Ok(data)
Storage->>RateLimiter: record_success(path)
Storage-->>Client: Ok(data)
end
end
sequenceDiagram
participant Client
participant HiddenStorage
participant RateLimiter as "RateLimiter\n(on-disk)"
participant Crypto
Client->>HiddenStorage: unlock(password)
HiddenStorage->>RateLimiter: check_rate_limit(outer_path)
alt outer rate limited
RateLimiter-->>HiddenStorage: Err(remaining)
HiddenStorage-->>Client: Err(KeepError::RateLimited(remaining_secs))
else outer allowed
RateLimiter-->>HiddenStorage: Ok(())
HiddenStorage->>HiddenStorage: try_unlock_outer(password)
HiddenStorage->>Crypto: decrypt_outer_volume(...)
alt outer auth fails
Crypto-->>HiddenStorage: Err(err_outer)
else outer succeeds
Crypto-->>HiddenStorage: Ok(outer)
end
HiddenStorage->>RateLimiter: check_rate_limit(hidden_path)
HiddenStorage->>HiddenStorage: try_unlock_hidden(password)
HiddenStorage->>Crypto: decrypt_hidden_volume(...)
alt hidden auth fails
Crypto-->>HiddenStorage: Err(err_hidden)
else hidden succeeds
Crypto-->>HiddenStorage: Ok(hidden)
end
HiddenStorage->>RateLimiter: record_success(chosen_path) or record_failure(path) based on auth errors
HiddenStorage-->>Client: Ok(VolumeType) or Err(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (1)keep-core/src/hidden/volume.rs (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (10)
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: 0
🧹 Nitpick comments (2)
keep-core/src/rate_limit.rs (1)
94-101: Consider logging mutex lock failures.If the mutex lock fails (lines 96-98), failures are silently ignored, which could theoretically allow unlimited attempts if an attacker could poison the mutex. While mutex poisoning is rare and indicates a serious bug elsewhere, consider logging this condition for observability.
Optional: Add logging on lock failure
pub fn record_failure(path: &Path) { let normalized = normalize_path(path); let Ok(mut limiter) = RATE_LIMITER.lock() else { + tracing::error!("Rate limiter mutex poisoned, cannot record failure"); return; }; let entry = limiter.get_or_create(&normalized); entry.record_failure(); }keep-core/Cargo.toml (1)
28-28: Update once_cell to the latest stable version.The current version 1.19 is outdated. Version 1.21.3 is the latest stable release and should be used instead.
📜 Review details
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 (6)
keep-core/Cargo.tomlkeep-core/src/error.rskeep-core/src/hidden/volume.rskeep-core/src/lib.rskeep-core/src/rate_limit.rskeep-core/src/storage.rs
🧰 Additional context used
🧬 Code graph analysis (4)
keep-core/src/error.rs (2)
keep-cli/src/server.rs (1)
error(427-433)keep-cli/src/output.rs (1)
error(24-28)
keep-core/src/rate_limit.rs (2)
keep-core/src/hidden/volume.rs (1)
path(618-620)keep-core/src/storage.rs (1)
path(331-333)
keep-core/src/storage.rs (2)
keep-core/src/rate_limit.rs (4)
check_rate_limit(80-92)record_failure(51-54)record_failure(94-101)record_success(103-111)keep-core/src/crypto.rs (2)
decrypt(96-100)decrypt(203-213)
keep-core/src/hidden/volume.rs (3)
keep-core/src/rate_limit.rs (4)
check_rate_limit(80-92)record_success(103-111)record_failure(51-54)record_failure(94-101)keep-core/src/lib.rs (3)
unlock(58-63)create(38-45)open(48-55)keep-core/src/storage.rs (4)
unlock(191-234)path(331-333)create(122-165)open(167-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (21)
keep-core/src/lib.rs (1)
15-15: LGTM! Module exposure is correct.The rate_limit module is properly exposed as a public module, making it accessible to the rest of the crate and external consumers.
keep-core/src/error.rs (1)
10-12: LGTM! Error variant is well-designed.The
RateLimited(u64)error variant properly encodes the wait time in seconds and provides a clear user-facing message.keep-core/src/storage.rs (5)
13-13: LGTM! Import is necessary and properly placed.
196-198: LGTM! Rate limit check is correctly placed.The rate limit check is properly positioned after the already-unlocked check and correctly returns the remaining delay with a minimum of 1 second.
215-221: LGTM! Failure recording on decryption error is correct.The code properly records rate limit failures when password decryption fails, ensuring the rate limiter tracks failed unlock attempts.
230-230: LGTM! Success recording resets rate limiting correctly.The success recording is properly placed after a complete successful unlock, ensuring the rate limit counter is reset.
483-525: LGTM! Tests comprehensively validate rate limiting behavior.The tests properly verify:
- Rate limiting triggers after MAX_ATTEMPTS (5) failed attempts
- Successful unlock resets the rate limit counter
- Proper cleanup with
record_successto avoid test interferencekeep-core/src/rate_limit.rs (7)
10-14: Rate limiting constants are well-balanced.The configuration (5 attempts before rate limiting, 1-second base delay, 5-minute cap) strikes a good balance between security and usability. The in-memory rate limiter will reset on process restart, which is acceptable for a desktop application but means an attacker could bypass limits by restarting the process.
16-18: Path normalization approach is pragmatic.The fallback to
to_path_buf()when canonicalization fails is reasonable. While different representations of the same path could theoretically be rate-limited separately, this is unlikely in practice for this use case.
20-60: LGTM! RateLimitEntry implementation is robust.The exponential backoff logic is correctly implemented with proper overflow/underflow protection using
saturating_addandsaturating_sub. The delay progression (1s, 2s, 4s, 8s, ..., capped at 300s) provides strong protection against brute force while allowing legitimate users to retry after waiting.
62-78: LGTM! RateLimiter struct is clean and idiomatic.The HashMap-based storage with
entry().or_insert_with()pattern is the standard approach for this use case.
80-92: LGTM! Rate limit check handles edge cases defensively.The function returns the maximum delay if the mutex lock fails (poisoned), which is appropriate since mutex poisoning indicates a serious bug. The API design using
Result<(), Duration>clearly separates the success and rate-limited cases.
103-111: LGTM! Success recording correctly uses get_mut.Using
get_mutinstead ofget_or_createis correct here - if no entry exists, there's nothing to reset. Same logging consideration asrecord_failureapplies if the mutex is poisoned.
113-224: Excellent test coverage!The tests comprehensively validate all aspects of the rate limiter:
- No delay before MAX_ATTEMPTS threshold
- Exponential backoff progression
- Maximum delay cap
- Success resets
- Delay expiration
- Path independence
The use of non-existent paths in
unique_pathis fine sincenormalize_pathhandles this with its fallback logic.keep-core/src/hidden/volume.rs (7)
43-43: LGTM! Import is necessary and correctly placed.
239-268: LGTM! Core unlock logic properly extracted.Extracting the core unlock logic into
try_unlock_outeras a private method is good separation of concerns, allowing the public method to handle rate limiting consistently.
270-289: LGTM! Rate limiting wrapper is correctly implemented.The wrapper properly:
- Checks if already unlocked before rate limiting (efficiency)
- Enforces rate limits before attempting unlock
- Records success/failure based on outcome
- Propagates errors appropriately
291-349: LGTM! Hidden volume unlock logic properly extracted.Consistent with the outer volume approach, the core hidden unlock logic is cleanly separated from rate limiting concerns.
351-370: LGTM! Consistent rate limiting pattern.The
unlock_hiddenwrapper follows the same pattern asunlock_outer, maintaining consistency across the codebase.
372-394: LGTM! Smart unlock correctly implements rate limiting and timing attack mitigation.The implementation correctly:
- Checks rate limit once upfront (not per volume type)
- Attempts both unlock methods to prevent timing side-channels
- Records success if either succeeds, failure only if both fail
- Returns appropriate volume type on success
This approach prevents an attacker from determining whether a password was tried against the outer or hidden volume based on timing or rate limit behavior.
890-949: LGTM! Comprehensive rate limiting test coverage.The tests validate rate limiting across all three unlock methods:
unlock_outer- outer volume specificunlock_hidden- hidden volume specificunlock- smart unlock attempting bothEach test properly verifies that the first 5 attempts receive password errors while the 6th triggers rate limiting, with proper cleanup to avoid test interference.
wksantiago
force-pushed
the
rate-limiting
branch
from
ad68880 to
e4fc695
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
keep-core/src/hidden/volume.rs (1)
890-949: Tests verify basic rate limiting but have manual cleanup.The three tests correctly verify that rate limiting triggers after 5 failed attempts. However, the manual
rate_limit::record_success(&path)calls at lines 908, 928, and 948 create potential test interdependencies if tests fail before cleanup executes.Consider these test improvements:
- Add a test helper or fixture to manage rate limiter state reset
- Add test coverage for successful unlock resetting the failure counter
- Verify that the rate limit delay increases with repeated failures (exponential backoff per PR description)
📜 Review details
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 (6)
keep-core/Cargo.tomlkeep-core/src/error.rskeep-core/src/hidden/volume.rskeep-core/src/lib.rskeep-core/src/rate_limit.rskeep-core/src/storage.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- keep-core/src/lib.rs
- keep-core/src/rate_limit.rs
- keep-core/src/storage.rs
- keep-core/Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (2)
keep-core/src/error.rs (2)
keep-cli/src/server.rs (1)
error(427-433)keep-cli/src/output.rs (1)
error(24-28)
keep-core/src/hidden/volume.rs (2)
keep-core/src/rate_limit.rs (4)
check_rate_limit(80-92)record_success(103-111)record_failure(51-54)record_failure(94-101)keep-core/src/storage.rs (4)
unlock(191-234)path(331-333)create(122-165)open(167-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
keep-core/src/error.rs (1)
10-11: LGTM! Clean error variant addition.The new
RateLimited(u64)error variant is well-implemented with a clear message format. The u64 parameter appropriately holds the retry delay in seconds.keep-core/src/hidden/volume.rs (3)
43-43: LGTM! Rate limit module imported.The import is correctly added to enable rate limiting functionality.
239-268: Good refactoring to separate concerns.The core unlock logic is now isolated in this private method, making it easier to add rate limiting in the public wrapper. The implementation correctly handles key derivation, decryption, and database opening.
291-349: Well-structured core unlock logic for hidden volume.The private method correctly isolates the hidden volume unlock implementation, including proper error handling that returns
InvalidPasswordfor authentication failures (lines 322, 329). The separation from rate limiting logic is clean.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
keep-core/src/hidden/volume.rs (1)
911-930: Consider more specific error assertions.The test correctly verifies rate limiting behavior, but line 922's assertion
!matches!(result, Err(KeepError::RateLimited(_)))only checks that the error is NOT rate-limited, rather than explicitly verifying it IS an authentication failure.🔎 More specific assertion
for _ in 0..5 { let mut storage = HiddenStorage::open(&path).unwrap(); let result = storage.unlock_outer("wrong"); - assert!(result.is_err()); - assert!(!matches!(result, Err(KeepError::RateLimited(_)))); + assert!(matches!( + result, + Err(KeepError::InvalidPassword | KeepError::DecryptionFailed) + )); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keep-core/src/hidden/volume.rs
🧰 Additional context used
🧬 Code graph analysis (1)
keep-core/src/hidden/volume.rs (3)
keep-core/src/rate_limit.rs (4)
check_rate_limit(80-92)record_success(103-111)record_failure(51-54)record_failure(94-101)keep-core/src/lib.rs (3)
unlock(58-63)create(38-45)open(48-55)keep-core/src/storage.rs (4)
unlock(191-234)path(331-333)create(122-165)open(167-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
keep-core/src/hidden/volume.rs (8)
43-43: LGTM: Rate limit module imported.The import is necessary for the rate limiting integration throughout the unlock flows.
239-268: LGTM: Clean extraction of core unlock logic.The separation of
try_unlock_outer(core logic) fromunlock_outer(rate-limiting wrapper) is well-structured and maintains the original unlock functionality.
270-292: Excellent: Past review feedback addressed correctly.The implementation now only records rate-limit failures for authentication-related errors (lines 286-287), preventing users from being penalized for system-level issues like IO failures or database corruption. This directly addresses the concern raised in previous reviews.
294-352: LGTM: Hidden volume unlock logic properly extracted.The private helper correctly returns
InvalidPasswordfor authentication failures (lines 325, 332), which will be properly caught by the rate-limiting logic in the public wrapper.
354-376: Excellent: Consistent implementation with past feedback addressed.Like
unlock_outer, this method correctly limits rate-limit failure recording to authentication errors only (lines 370-371), resolving the concern from previous reviews.
378-415: Excellent: Sophisticated error handling that fully addresses past feedback.The combined unlock implementation correctly:
- Checks rate limits before expensive KDF operations (line 379)
- Preserves constant-time execution for timing attack prevention (lines 383-384)
- Returns non-authentication errors immediately without rate-limiting (lines 403-408)
- Only records rate-limit failures when both unlock attempts fail with authentication errors (lines 410-412)
This comprehensively addresses the concern from previous reviews about incorrectly penalizing users for system-level failures.
932-950: LGTM: Rate limiting test with explicit error check.This test correctly verifies rate limiting for the hidden volume unlock flow. Line 942's explicit check for
InvalidPasswordis good practice.
952-970: LGTM: Comprehensive test for combined unlock rate limiting.The test correctly verifies that rate limiting applies to the combined unlock flow, with proper explicit error type assertions and cleanup.
kwsantiago
force-pushed
the
rate-limiting
branch
from
ccb2798 to
04531fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
keep-core/src/lib.rs (1)
15-15: Module visibility is inconsistent with its contents.The module is declared as
pubbut all functions inside (check_rate_limit,record_failure,record_success) arepub(crate). External crate users can seekeep_core::rate_limitexists but cannot access anything within it. This is likely unintentional.🔎 Suggested fix: Make module crate-private
-pub mod rate_limit; +pub(crate) mod rate_limit;
📜 Review details
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 (6)
keep-core/Cargo.tomlkeep-core/src/error.rskeep-core/src/hidden/volume.rskeep-core/src/lib.rskeep-core/src/rate_limit.rskeep-core/src/storage.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- keep-core/src/storage.rs
🧰 Additional context used
🧬 Code graph analysis (2)
keep-core/src/error.rs (2)
keep-cli/src/server.rs (1)
error(427-433)keep-cli/src/output.rs (1)
error(24-28)
keep-core/src/hidden/volume.rs (3)
keep-core/src/rate_limit.rs (4)
check_rate_limit(80-92)record_success(103-111)record_failure(51-54)record_failure(94-101)keep-core/src/lib.rs (3)
unlock(58-63)create(38-45)open(48-55)keep-core/src/storage.rs (4)
unlock(191-234)path(331-333)create(122-165)open(167-189)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
keep-core/Cargo.toml (1)
28-28: **Dependency addition looks good.**The latest version of once_cell is 1.21.3, while this PR uses 1.19. Consider updating to a more recent version for any bug fixes. Note that most of this crate's functionality is available instdstarting with Rust 1.70 (std::sync::OnceLock), andstd::sync::LazyLockis stable as of Rust 1.80. Since the crate's MSRV is 1.81, you could potentially usestd::sync::LazyLockdirectly without the external dependency. However, usingonce_cellis also perfectly acceptable.keep-core/src/rate_limit.rs (7)
1-14: Well-structured module setup with appropriate safety constraints.The
#![forbid(unsafe_code)]directive is excellent for security-critical rate limiting code. The constants provide sensible defaults: 5 failed attempts before lockout, 1-second base delay with exponential backoff, capped at 5 minutes.
16-18: Path normalization approach is sound.The fallback to the original path when canonicalization fails handles edge cases gracefully (e.g., paths that don't exist yet). Since
normalize_pathis called consistently within all public API functions, path representation remains consistent across calls.
20-60: Solid rate limit entry implementation with proper overflow protection.The exponential backoff calculation is correct: after 5 failed attempts, delays grow as 1s, 2s, 4s, 8s... capped at 300s. The
saturating_addon line 52 prevents integer overflow, and the shift clamping (excess.min(8)) prevents shift overflow.
62-78: Clean rate limiter structure.The implementation is straightforward. One consideration: entries are never evicted, so memory will grow with each unique path. For a key management application where the number of vaults is typically small, this is acceptable. If this were a high-throughput service with many paths, you'd want TTL-based eviction.
80-92: Fail-closed behavior on mutex poisoning is the right security choice.If the mutex is poisoned (another thread panicked while holding the lock), returning
MAX_DELAY_SECSensures the system fails safely rather than allowing unlimited attempts.
94-111: Recording functions handle edge cases correctly.Good design choice:
record_success(line 108) only resets existing entries rather than creating new ones, avoiding unnecessary memory allocation for paths that were never rate-limited.
113-224: Comprehensive test coverage.The tests effectively validate all rate limiting behaviors including the exponential backoff progression, delay caps, success resets, time expiration, and path isolation. The
unique_pathhelper ensures test isolation when tests run in parallel.keep-core/src/error.rs (1)
10-11: Well-designed error variant for rate limiting.The
RateLimited(u64)variant correctly carries the wait duration in seconds, and the error message is clear and actionable for users. The placement afterInvalidPasswordis logical since both relate to authentication flow control.keep-core/src/hidden/volume.rs (6)
43-43: Import addition looks good.
239-268: Clean refactor separating core unlock logic from rate limiting concerns.The private
try_unlock_outermethod encapsulates the actual decryption logic, allowing the publicunlock_outerto handle rate limiting as a cross-cutting concern. This separation improves maintainability.
270-292: Correctly implements selective rate limiting for authentication errors only.The implementation now properly distinguishes between authentication failures (lines 286-288) and other errors (IO, database). Only
InvalidPasswordandDecryptionFailedincrement the rate limit counter, addressing the previous review feedback.
294-376: Consistent rate limiting pattern applied to hidden volume unlock.The
try_unlock_hidden/unlock_hiddensplit mirrors the outer volume pattern, maintaining consistency. Authentication error filtering (lines 370-372) correctly prevents rate limiting on system errors.
378-415: Comprehensive error handling for combined unlock preserves plausible deniability.The implementation correctly handles all error combinations:
- Either path succeeding → record success
- Non-auth error on either path → return that error without rate-limit penalty
- Both paths failing with auth errors → record single failure
The
is_auth_errorhelper (lines 397-399) cleanly encapsulates the authentication error check. This addresses the previous review feedback about incorrectly penalizing system-level failures.
910-970: Good test coverage for rate limiting across all unlock paths.The tests effectively validate:
- First 5 failed attempts return authentication errors (not
RateLimited)- 6th attempt is blocked with
RateLimitederror- All three unlock methods (outer, hidden, combined) are covered
The
rate_limit::record_success(&path)calls at the end of each test ensure proper cleanup since the rate limiter is a global static.
kwsantiago
force-pushed
the
rate-limiting
branch
from
04531fc to
851d34d
Compare
2 participants
Implements rate limiting for password attempts with exponential backoff after 5 failed attempts.
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.