[PM-35395] MasterPasswordService Key Management Integration#7637
[PM-35395] MasterPasswordService Key Management Integration#7637enmande wants to merge 14 commits into
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR routes the Change-KDF flow through No findings worth posting. The most material risk in the diff — semantic drift on Code Review DetailsNo findings. |
| // LastPasswordChangeDate is intentionally not set: KDF rotation re-derives the authentication | ||
| // hash from the same password using new KDF parameters — the user's password has not changed. | ||
| var now = _timeProvider.GetUtcNow().UtcDateTime; | ||
| user.LastPasswordChangeDate = now; |
There was a problem hiding this comment.
This method incorrectly indexed on a change of hash to warrant an update to the LastPasswordChangeDate. KDF- and Master Password-change operations are separate, and LastPasswordChangeDate is intended to capture the user action/intent to change the Master Password.
LastKdfChangeDate is provided to separate these concerns.
There was a problem hiding this comment.
Excellent catch.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Prevent a de-synced salt value from creating an un-decryptable unlock method | ||
| // Prevent a de-synced salt value from creating an un-decryptable unlock method. | ||
| // Also checked in the MasterPasswordService via UpdateExistingKdfConfigurationData.ValidateDataForUser. | ||
| authenticationData.ValidateSaltUnchangedForUser(user); |
There was a problem hiding this comment.
MasterPasswordService will perform salt-unchanged validation for authentication and unlock data requests consistently. That makes this check (now) an additional check of the same.
I have elected not to remove it from the command at this time, considering that up to Key Management's discretion.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7637 +/- ##
==========================================
- Coverage 64.61% 60.42% -4.20%
==========================================
Files 2137 2140 +3
Lines 94388 94612 +224
Branches 8405 8441 +36
==========================================
- Hits 60993 57168 -3825
- Misses 31312 35437 +4125
+ Partials 2083 2007 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
enmande
requested review from
JaredSnider-Bitwarden,
ike-kottlowski and
mzieniukbw
and removed request for
a team and
JaredSnider-Bitwarden
Thomas-Avery
left a comment
Thomas-Avery
left a comment
There was a problem hiding this comment.
KM changes look good. My callout here is before this is merged into main we need good QA regression testing around feature flags pm-18021-force-update-kdf-settings and pm-23995-no-logout-on-kdf-change on to make sure we don't impact the KDF migrations currently on going.
|
Merge plan: Auth integration PR (base) merges first then we will rebase this PR and re-request reviews then this can go to QA |
JaredSnider-Bitwarden
dismissed
Thomas-Avery’s stale review
The base branch was changed.
Master Password hash will be updated as a necessary side-effect of KDF configuration change, but should not be represented in the method naming to prevent confusion. This is for KDF-update flows only.
Mismatched salt should fail fast at the command level (existing). A defense-in-depth check also resides at ValidateDataForUser inside the MasterPasswordService.
JaredSnider-Bitwarden
force-pushed
the
auth/pm-35395/master-password-service-km-integration
branch
from
848f6d7 to
d6e86c6
Compare
|
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
There was a problem hiding this comment.
Excellent work
…cy fields Removes the deprecated NewMasterPasswordHash/Key fields and the MasterPasswordPayloadVariantValidator path from ChangeKdfRequestModel. AuthenticationData and UnlockData become C# required + [Required]; rejection of null/missing values is delegated to System.Text.Json and the ASP.NET ModelStateValidationFilter. Validate() adds an early return when either cross-field input is null, so it remains safe to invoke after [Required] has already populated ModelState (the framework runs IValidatableObject.Validate regardless of data-annotation outcome). Tests for the dropped variant-validator path are removed; a new Validate_EitherFieldNull_ReturnsNoErrors test pins the defensive guard.
…, align tests Removes the manual null check from PostKdf — null/missing payload fields are now rejected by [Required] + ModelStateValidationFilterAttribute before the action runs. Drops two unit tests that asserted the old custom error message (framework-layer behavior, no longer reachable at the controller layer). Updates the integration test PostKdf_AuthenticationDataOrUnlockDataNull to assert the default [Required] messages, and adds PostKdf_MissingRequiredJsonKey to lock in the `required` C# keyword contract for the wire payload.
JaredSnider-Bitwarden
dismissed stale reviews from Thomas-Avery and themself
via
f564c17
|
When reviewing the QA testing notes, I realized that when we (BW) modified So, I've updated the |
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
There was a problem hiding this comment.
Smoke tested latest changes - everything looks good.
4 participants
🎟️ Tracking
PM-35395
📔 Objective
Refactors the Change-KDF flow to use the new
MasterPasswordService, routing throughSaveUpdateKdfConfigurationAsync(source).📓 Note the name change from the base branch of invoked method to
SaveUpdateKdfConfigurationAsync.At implementation time, it was decided
SaveUpdateMasterPasswordAndKdfAsyncwas not an ideal name for this method. It is used for KDF-affecting flows (only); the fact that the hash of a master password will change as a result of key derivation is a side effect, and naming it in the method directly invites misunderstanding. Tests and comments were updated throughout to reflect this.Part of the PM-33011 story-of-stories to route all password set/change/rotate flows through MasterPasswordService. Depends on PM-35393.
📸 Screenshots
KDF Configuration change
zed is a user with a Master Password.
zed changes their KDF configuration:
PBKDF2->Argon2idwith default settings.LastKdfChangeDateis updated as expectedLastMasterPasswordChangeDateis not updated as expectedPM-39395_kdf-change.mov