[PM-24051] Init crypto and update kdf with MasterPasswordUnlock

[mzieniukbw]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-24051

📔 Objective

• Initialise user crypto with MasterPasswordUnlock - added initialize_user_crypto_master_password_unlock function into InternalClient's crypto.
• Auth's login methods (password, and api_key specifically) use MasterPasswordUnlockData when present in response and uses new function.
  • KM team have decided that the double KDF derivation issue is something we are ok with for login.
  • Moved set_login_method to be the after the initialize_user_crypto function in execution order, since the latter overrides the set_login_method. Move everywhere (all login methods) for consistency.
  • Removed kdf as init arg of password.rs, consistently use prelogin now.
• Update user's KDF settings with MasterPasswordUnlock via update_user_master_password_unlock in InternalClient, triggered in sync.

Continuation of #376

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  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 confirmed
  issue 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

[quexten]

Only minor things from my side. I wish we could skip all of the unused code changes. Other than these I'm happy to approve.

[claude]

Claude finished @mzieniukbw's task —— View job

---

PR Review: Init crypto and update KDF with MasterPasswordUnlock

Review Progress:

• Read repository guidelines (CLAUDE.md) - No repo-specific guidelines found
• Analyze PR changes and scope
• Review crypto initialization changes
• Review auth login method updates
• Review KDF handling and security implications
• Review sync functionality changes
• Check code quality and best practices
• Verify test coverage considerations
• Provide comprehensive feedback

---

🔍 Review Summary

This PR implements significant changes to crypto initialization and authentication flows by introducing MasterPasswordUnlock support. The changes span 23 files with 832 additions and 328 deletions, focusing on improving how user crypto is initialized and KDF settings are managed.

📋 Key Changes Analyzed

✅ Strengths

1. Clear separation of concerns - New initialize_user_crypto_master_password_unlock method properly encapsulates the crypto initialization logic
2. Consistent error handling - Proper use of Result types and error propagation throughout
3. Security-conscious design - KDF parameters are properly validated and handled
4. Good documentation - Functions have clear docstrings explaining their purpose

⚠️ Security & Architecture Concerns

Critical Security Issue - Email/Salt Confusion

Location: crates/bitwarden-core/src/auth/login/password.rs:76 and api_key.rs:60

email: master_password_unlock.salt,  // ❌ This is problematic

Issue: The code is using the KDF salt as the email field in LoginMethod. This creates semantic confusion and potential security issues:

• The salt might not be a valid email format
• This breaks the expected contract of the email field
• Could lead to issues if other code expects a real email address

Recommendation: Keep email and salt separate, or clearly document why salt is being used as email here.

Double KDF Derivation

Location: crates/bitwarden-core/src/auth/login/password.rs:31-34 vs internal.rs:401-405
The code performs KDF derivation twice:

1. For authentication (line 34): MasterPasswordAuthenticationData::derive
2. For crypto initialization (line 401): MasterKey::derive

While the PR notes this is acceptable to the KM team, it's expensive and should be optimized in future iterations.

Potential Authentication Bypass

Location: crates/bitwarden-core/src/auth/login/auth_request.rs:115-118

let kdf = master_password_unlock
    .as_ref()
    .map(|mpu| mpu.kdf.clone())
    .unwrap_or_else(Kdf::default);  // ❌ Using default KDF

Issue: Falling back to Kdf::default() when no master password unlock is available could be problematic. This might allow weaker KDF settings than intended.

🎨 Code Quality Issues

Inconsistent Error Handling Pattern

Location: Multiple files (password.rs, api_key.rs)

let private_key: EncString = require!(&r.private_key).parse()?;

The require! macro with & reference creates confusing error messages. Consider a more explicit approach.

Code Duplication

The pattern for extracting and handling master_password_unlock is repeated across multiple login methods:

• password.rs:57-62
• api_key.rs:40-45
• auth_request.rs:109-114

Recommendation: Extract this into a helper function to reduce duplication.

Memory Efficiency

Location: Multiple locations with .clone()

master_password_unlock.clone()  // Consider if clone is necessary

Several unnecessary clones could be avoided with better lifetime management.

🔧 Implementation Concerns

Sync Functionality

Location: crates/bw/src/vault/sync.rs:50-60
The sync operation updates KDF settings silently. This should probably:

1. Log the KDF update for auditing
2. Have validation to prevent downgrade attacks
3. Consider notifying other client sessions

Test Coverage Gap

The new initialize_user_crypto_master_password_unlock and update_user_master_password_unlock functions appear to lack comprehensive unit tests. Given the security-critical nature of these functions, they need thorough testing.

Backwards Compatibility

The changes to login methods might affect existing clients. Ensure the changes are backwards compatible or properly versioned.

🎯 Specific Recommendations

High Priority (Security)

1. Fix email/salt confusion - Don't use salt as email in LoginMethod
2. Validate KDF fallback - Don't default to weak KDF settings in auth_request.rs
3. Add comprehensive tests - Especially for crypto initialization functions

Medium Priority (Quality)

1. Extract common patterns - Reduce duplication in master_password_unlock extraction
2. Optimize KDF derivation - Consider caching or reusing derived keys
3. Improve error messages - Make require! errors more user-friendly
4. Add logging - KDF updates in sync should be logged

Low Priority (Maintenance)

1. Remove unnecessary clones - Optimize memory usage
2. Document design decisions - Why double KDF derivation is acceptable
3. Consider future cleanup - Plan for removing old crypto initialization methods

📊 Overall Assessment

Code Quality: 7/10 - Well-structured but has some quality issues
Security: 6/10 - Concerning issues with email/salt confusion and KDF fallbacks
Testing: 5/10 - Appears to lack comprehensive test coverage for new functionality
Documentation: 8/10 - Good documentation on new functions

This is a substantial improvement to the crypto initialization flow, but the security concerns should be addressed before merging. The architecture is sound, but implementation details need refinement.

---

[claude]

Claude finished @mzieniukbw's task —— View job

---

PR Review: Init crypto and update KDF with MasterPasswordUnlock

Review Progress:

• Read repository guidelines (CLAUDE.md) - No repo-specific guidelines found
• Analyze PR changes and scope
• Review crypto initialization changes
• Review auth login method updates
• Review KDF handling and security implications
• Review sync functionality changes
• Check code quality and best practices
• Verify test coverage considerations
• Provide comprehensive feedback

---

🔍 Review Summary

This PR implements significant changes to crypto initialization and authentication flows by introducing MasterPasswordUnlock support. The changes span 23 files with 832 additions and 328 deletions, focusing on improving how user crypto is initialized and KDF settings are managed.

📋 Key Changes Analyzed

✅ Strengths

1. Clear separation of concerns - New initialize_user_crypto_master_password_unlock method properly encapsulates the crypto initialization logic
2. Consistent error handling - Proper use of Result types and error propagation throughout
3. Security-conscious design - KDF parameters are properly validated and handled
4. Good documentation - Functions have clear docstrings explaining their purpose

⚠️ Security & Architecture Concerns

Critical Security Issue - Email/Salt Confusion

Location: crates/bitwarden-core/src/auth/login/password.rs:76 and api_key.rs:60

email: master_password_unlock.salt,  // ❌ This is problematic

Issue: The code is using the KDF salt as the email field in LoginMethod. This creates semantic confusion and potential security issues:

• The salt might not be a valid email format
• This breaks the expected contract of the email field
• Could lead to issues if other code expects a real email address

Recommendation: Keep email and salt separate, or clearly document why salt is being used as email here.

Double KDF Derivation

Location: crates/bitwarden-core/src/auth/login/password.rs:31-34 vs internal.rs:401-405
The code performs KDF derivation twice:

1. For authentication (line 34): MasterPasswordAuthenticationData::derive
2. For crypto initialization (line 401): MasterKey::derive

While the PR notes this is acceptable to the KM team, it's expensive and should be optimized in future iterations.

Potential Authentication Bypass

Location: crates/bitwarden-core/src/auth/login/auth_request.rs:115-118

let kdf = master_password_unlock
    .as_ref()
    .map(|mpu| mpu.kdf.clone())
    .unwrap_or_else(Kdf::default);  // ❌ Using default KDF

Issue: Falling back to Kdf::default() when no master password unlock is available could be problematic. This might allow weaker KDF settings than intended.

🎨 Code Quality Issues

Inconsistent Error Handling Pattern

Location: Multiple files (password.rs, api_key.rs)

let private_key: EncString = require!(&r.private_key).parse()?;

The require! macro with & reference creates confusing error messages. Consider a more explicit approach.

Code Duplication

The pattern for extracting and handling master_password_unlock is repeated across multiple login methods:

• password.rs:57-62
• api_key.rs:40-45
• auth_request.rs:109-114

Recommendation: Extract this into a helper function to reduce duplication.

Memory Efficiency

Location: Multiple locations with .clone()

master_password_unlock.clone()  // Consider if clone is necessary

Several unnecessary clones could be avoided with better lifetime management.

🔧 Implementation Concerns

Sync Functionality

Location: crates/bw/src/vault/sync.rs:50-60
The sync operation updates KDF settings silently. This should probably:

1. Log the KDF update for auditing
2. Have validation to prevent downgrade attacks
3. Consider notifying other client sessions

Test Coverage Gap

The new initialize_user_crypto_master_password_unlock and set_user_master_password_unlock functions appear to lack comprehensive unit tests. Given the security-critical nature of these functions, they need thorough testing.

Backwards Compatibility

The changes to login methods might affect existing clients. Ensure the changes are backwards compatible or properly versioned.

🎯 Specific Recommendations

High Priority (Security)

1. Fix email/salt confusion - Don't use salt as email in LoginMethod
2. Validate KDF fallback - Don't default to weak KDF settings in auth_request.rs
3. Add comprehensive tests - Especially for crypto initialization functions

Medium Priority (Quality)

1. Extract common patterns - Reduce duplication in master_password_unlock extraction
2. Optimize KDF derivation - Consider caching or reusing derived keys
3. Improve error messages - Make require! errors more user-friendly
4. Add logging - KDF updates in sync should be logged

Low Priority (Maintenance)

1. Remove unnecessary clones - Optimize memory usage
2. Document design decisions - Why double KDF derivation is acceptable
3. Consider future cleanup - Plan for removing old crypto initialization methods

📊 Overall Assessment

Code Quality: 7/10 - Well-structured but has some quality issues
Security: 6/10 - Concerning issues with email/salt confusion and KDF fallbacks
Testing: 5/10 - Appears to lack comprehensive test coverage for new functionality
Documentation: 8/10 - Good documentation on new functions

This is a substantial improvement to the crypto initialization flow, but the security concerns should be addressed before merging. The architecture is sound, but implementation details need refinement.

---

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
5.8% Duplication on New Code

See analysis details on SonarQube Cloud

[shane-melton]

Vault changes and the removal of the outdated sync look good!