[PM-24468] Introduce CipherRiskClient #499
Conversation
|
Claude finished @shane-melton's task —— View job Claude finished @shane-melton's task —— View job Code Review Complete ✅I've completed a comprehensive review of the 📊 Summary of Changes Since Last ReviewThe PR has addressed previous feedback well:
🔴 Critical Issues1. SHA-1 Algorithm Lacks Required Documentation (crates/bitwarden-vault/src/cipher_risk/hibp.rs:16)Per CLAUDE.md guidelines: "New crypto algorithms or key derivation: Detailed description, review and audit trail required. Document algorithm choice rationale and test vectors." Issue: SHA-1 is cryptographically broken for security purposes. While it's the correct choice here (HIBP API requirement + k-anonymity protection), this needs explicit documentation. Required: Add a comment explaining: /// Hash password with SHA-1 for HIBP k-anonymity lookup.
///
/// NOTE: SHA-1 is cryptographically broken for collision resistance, but is the correct
/// choice here because:
/// 1. HIBP API requires SHA-1 for backwards compatibility with their existing database
/// 2. The k-anonymity model (sending only first 5 characters) protects the password itself
/// 3. This is used purely as a hash function for lookup, not for cryptographic security
/// 4. An attacker cannot reverse the hash or learn the password from the 5-char prefix
fn hash_password_for_hibp(password: &str) -> (String, String) {2. Potential Password Leakage in Error Messages (crates/bitwarden-vault/src/cipher_risk/cipher_risk_client.rs:81)Per CLAUDE.md: "Do not log keys, passwords, or vault data in logs or error paths. Redact sensitive data." Issue: When HIBP API calls fail, we store the error message in Verification needed: Test that Recommendation: Add explicit test case: #[tokio::test]
async fn test_error_messages_dont_leak_password_data() {
// Verify that error messages don't contain hash prefixes or passwords
}3. Missing Documentation on Concurrency Limit Rationale (crates/bitwarden-vault/src/cipher_risk/cipher_risk_client.rs:29)Issue: Required: Document the rationale: /// Maximum concurrent HIBP API requests.
///
/// Limits concurrent requests to balance performance with:
/// - API courtesy (avoid overwhelming HIBP service)
/// - Client connection pool limits (typical HTTP/2 allows ~100 streams)
/// - Memory usage for concurrent futures
///
/// Value chosen based on typical HTTP client defaults and HIBP API guidelines.
const MAX_CONCURRENT_REQUESTS: usize = 100;
|
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #499 +/- ##
==========================================
+ Coverage 78.36% 78.74% +0.38%
==========================================
Files 291 295 +4
Lines 29343 29960 +617
==========================================
+ Hits 22994 23592 +598
- Misses 6349 6368 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
shane-melton
force-pushed
the
vault/pm-24468/cipher-risk-client
branch
from
915fe76 to
a10fef6
Compare
Remove performance test
…er in exposed_count
| let http_client = self.client.internal.get_http_client().clone(); | ||
| let password_map = password_map.clone(); | ||
| let base_url = options | ||
| .hibp_base_url | ||
| .clone() | ||
| .unwrap_or_else(|| HIBP_DEFAULT_BASE_URL.to_string()); | ||
|
|
||
| async move { | ||
| if details.password.is_empty() { | ||
| // Skip empty passwords, return default risk values | ||
| return CipherRisk { | ||
| id: details.id, | ||
| password_strength: 0, | ||
| exposed_result: ExposedPasswordResult::NotChecked, | ||
| reuse_count: None, | ||
| }; | ||
| } | ||
|
|
||
| let password_strength = Self::calculate_password_strength( | ||
| &details.password, | ||
| details.username.as_deref(), | ||
| ); | ||
|
|
||
| // Check exposure via HIBP API if enabled | ||
| // Capture errors per-cipher instead of propagating them | ||
| let exposed_result = if options.check_exposed { | ||
| match Self::check_password_exposed(&http_client, &details.password, &base_url) | ||
| .await | ||
| { | ||
| Ok(count) => ExposedPasswordResult::Found(count), | ||
| Err(e) => ExposedPasswordResult::Error(e.to_string()), | ||
| } | ||
| } else { | ||
| ExposedPasswordResult::NotChecked | ||
| }; | ||
|
|
||
| // Check reuse from provided map | ||
| let reuse_count = if let Some(map) = &password_map { | ||
| map.map.get(&details.password).copied() | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| CipherRisk { | ||
| id: details.id, | ||
| password_strength, | ||
| exposed_result, | ||
| reuse_count, | ||
| } | ||
| } |
There was a problem hiding this comment.
⛏️ Could this be moved to a separate function async fn to_cipher_risk(..) or similar?
|
Will this replace the existing |
|
@Hinton I originally planned on utilizing their implementation, however it is adding Bitwarden specific inputs as it was being used to specifically check the user's Bitwarden password. It also assumed there was always an email (a requirement for Bitwarden accounts). Not opposed to making this more generic so it can be reused by Auth. However, at that point I'm not sure the Vault crate is the proper spot for a generic password strength checking utility, especially if it means the Auth crate would depend on Vault. |
crates/bitwarden-vault/Cargo.toml
Outdated
| uuid = { workspace = true } | ||
| wasm-bindgen = { workspace = true, optional = true } | ||
| wasm-bindgen-futures = { workspace = true, optional = true } | ||
| zxcvbn = ">=3.0.1, <4.0" |
There was a problem hiding this comment.
issue: These should be moved to the root workspace and referenced as workspace dependencies since they are consumed by multiple crates.
There was a problem hiding this comment.
Moved to the root workspace in c039308
crates/bitwarden-vault/src/error.rs
Outdated
| /// Error type for cipher risk evaluation operations | ||
| #[allow(missing_docs)] | ||
| #[bitwarden_error(flat)] | ||
| #[derive(Debug, Error)] | ||
| pub enum CipherRiskError { | ||
| #[error(transparent)] | ||
| Reqwest(#[from] reqwest::Error), | ||
| } |
There was a problem hiding this comment.
suggestion: Having a root error.rs is a bit of an anti pattern we're slowly moving away from. Defining it where it's used will result in less file jumping.
| /// | ||
| /// Returns `Some(CipherLoginDetails)` if this is a login cipher with a password, | ||
| /// otherwise returns `None`. | ||
| pub fn to_login_details(&self) -> Option<crate::cipher::cipher_risk::CipherLoginDetails> { |
There was a problem hiding this comment.
question: Is this used anywhere?
There was a problem hiding this comment.
Nope, it was a leftover. Removed d201755
| #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] | ||
| pub struct CipherLoginDetails { | ||
| /// Cipher ID to identify which cipher in results. | ||
| pub id: Option<CipherId>, |
There was a problem hiding this comment.
question: Can CipherId be empty?
There was a problem hiding this comment.
Good point, it really doesn't make sense for it to ever be null in this context. Made required in 9b8b001
There was a problem hiding this comment.
suggestion: This feature seems completely decoupled from ciphers. Maybe move it to a separate dedicated module, and/or crate.
There was a problem hiding this comment.
Good idea, moved to a separate cipher_risk module in 5d0e1d2
There was a problem hiding this comment.
issue: We generally only use clients to expose a public interface, this file seems to currently do a lot of various things.
suggestion:
- Extract two sub-modules, HIBP and reuse detection. These can be unit tested in isolation quite nicely which will also significantly decrease the filesize.
- Consider what public interface you need, is it sufficient to just accept a single struct containing a list of ciphers to check? If so everything except this struct and response can be made internal to the module or crate.
There was a problem hiding this comment.
Refactored the HIBP and password strength into their own sub-modules to reduce the complexity of the cipher_risk_client. (the reuse detection is fairly small and didn't seem worth the same overhead).
I also cleaned up the public interface to only export the required input/output structs, the error, and the client itself.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
| /// Hash password with SHA-1 and split into prefix/suffix for k-anonymity. | ||
| /// | ||
| /// Returns a tuple of (prefix: first 5 chars, suffix: remaining chars). | ||
| pub(super) fn hash_password_for_hibp(password: &str) -> (String, String) { |
There was a problem hiding this comment.
⛏️ It looks like these are only called from this mod in check_password_exposed, so I think you can do away with the pub(super) here and on parse_hibp_response()
There was a problem hiding this comment.
Nice catch! Removed in bc1808f
| /// - For emails: extracts and tokenizes the local part (before @) | ||
| /// - For usernames: tokenizes the entire string | ||
| /// - Splits on non-alphanumeric characters and converts to lowercase | ||
| pub(super) fn extract_user_inputs(username: &str) -> Vec<String> { |
There was a problem hiding this comment.
nit: This is only used in calculate_password_strength, so you can ditch the pub(super)
4 participants
🎟️ Tracking
PM-24468
📔 Objective
Implement the cipher risk evaluation logic in the Vault SDK as a separate
CipherRiskClientso that it can be re-used in mobile and gain potential performance improvements.The
CipherRiskClientcontains logic to check if a multiple login ciphers' passwords are exposed (via HIBP), weak (via zxcvbn), or reused.⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes