[PM-35599] Update set-password endpoint to handle V1 encryption when given Auth and Unlock data - #7723
[PM-35599] Update set-password endpoint to handle V1 encryption when given Auth and Unlock data#7723rr-bw wants to merge 42 commits into
Conversation
dbd5a89 to
0f724cf
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
0f724cf to
9fb3fd8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7723 +/- ##
==========================================
+ Coverage 62.87% 62.88% +0.01%
==========================================
Files 2301 2301
Lines 100212 100243 +31
Branches 9019 9034 +15
==========================================
+ Hits 63008 63038 +30
- Misses 35020 35021 +1
Partials 2184 2184 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
17985b4 to
98d982a
Compare
…() falls back from MPAD/MPUD to legacy fields
…tInitialPasswordRequestModel
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE This PR retightens Code Review DetailsNo findings at or above the reporting threshold. Considered and not raised:
|
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
There was a problem hiding this comment.
Partial review but leaving this here:
| { | ||
| await _finishSsoJitProvisionMasterPasswordCommand.FinishProvisionAsync(user, model.ToData()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Per discussion with @Thomas-Avery , if we get a V2 payload when the flag is off, we cannot process the request. That is an error state that shouldn't happen unless a BW client is out of sync w/ the FF being turned off, but we should still handle it.
There was a problem hiding this comment.
If what you're getting at here is...
- If the request contains
MPAD+MPUD+AccountKeys, but theEnableAccountEncryptionV2JitPasswordRegistrationflag is OFF on the server, then throw.
...then that error state / thrown exception is handled at the top of SetInitialPasswordV1Async():
private async Task SetInitialPasswordV1Async(User user, SetInitialPasswordRequestModel model)
{
// Defensive: V1 cannot consume AccountKeys (the new key shape). If a request carries
// AccountKeys we'd silently drop the keypair, so fail loudly. This can only happen if
// the V2 MP JIT flag is off (otherwise the V2 branch above would have handled it) — i.e.,
// a client/server flag-state mismatch or a non-Angular caller.
if (model.AccountKeys != null)
{
throw new BadRequestException(
"Request includes V2 AccountKeys but V2 encryption is not enabled.");
}
// ...| if (model.IsTdeSetPasswordRequest()) | ||
| // V2 encryption - TDE user with "manage account recovery" permission | ||
| if (model.IsTdeSetPasswordRequest() && | ||
| _featureService.IsEnabled(FeatureFlagKeys.V2RegistrationTDEJIT)) |
There was a problem hiding this comment.
V2RegistrationTDEJIT flag where it is turned off, TDE users without MPs (who can have v1 or v2 encryption for their user key) can still be given the manage account recovery permission and they must be able to set a password.
V2RegistrationTDEJIT just governs SSO + TDE account creation - this process of setting an initial password happens later.
There was a problem hiding this comment.
17dfee5 - includes comments with your note about V2RegistrationTDEJIT governing SSO + TDE account creation. Might help avoid confusion with the asymmetry (i.e. not checking a flag for TDE, but checking a flag for MP JIT).
| public KeysRequestModel? Keys { get; set; } | ||
|
|
||
| [Obsolete("Use MasterPasswordAuthentication instead")] | ||
| [Obsolete("Use MasterPasswordUnlock instead")] |
There was a problem hiding this comment.
❓ : why the change?
There was a problem hiding this comment.
No major reason. "Unlock" data is just more consistent with what the ToUser() method uses after the changes on this PR.
| catch (Exception e) | ||
| { | ||
| ModelState.AddModelError(string.Empty, e.Message); | ||
| throw new BadRequestException(ModelState); | ||
| } |
|
… of hard-coding the fallback
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // MasterPasswordSalt column must never be null/empty after a successful password-set | ||
| // operation. Modern clients send an explicit salt via MPUD; older clients don't send one, | ||
| // so we fall back to the email-derived V1 salt via GetMasterPasswordSalt(). | ||
| existingUser.MasterPasswordSalt = MasterPasswordUnlock?.Salt ?? existingUser.GetMasterPasswordSalt(); |
There was a problem hiding this comment.
Details and fix
Every other initial-set / KDF-change / rotate path funnels through SetInitialPasswordData.ValidateDataForUser (or ChangeKdfCommand), which calls MasterPasswordUnlockData.ValidateSaltUnchangedForUser(user) — the Stage 1 email-salt invariant (salt == GetMasterPasswordSalt()), documented in src/Core/Auth/UserFeatures/UserMasterPassword/Data/SetInitialPasswordData.cs:70-77 as necessary because "the server cannot yet handle divergent salts... a mismatch here would make the user un-decryptable on next login."
The new modern-V1 route has no equivalent check. POST /accounts/set-password with masterPasswordUnlock.salt set to anything other than the email-derived salt, plus legacy keys, with EnableAccountEncryptionV2JitPasswordRegistration off, reaches SetInitialPasswordV1Async → ToUser() persists that salt → SetInitialMasterPasswordCommandV1 calls ReplaceAsync. Since PasswordPreloginResponseModel.Salt is still annotated "Not used yet", clients re-derive from email on the next login and the stored authentication hash can never match, leaving the account permanently un-loginable.
This is also a behavior change rather than a pre-existing gap: before this PR, the MPAD + MPUD + legacy Keys shape was routed to _tdeSetPasswordCommand, which did enforce the invariant.
Suggested fix in SetInitialPasswordV1Async, before model.ToUser(user):
model.MasterPasswordUnlock?.ToData().ValidateSaltUnchangedForUser(user);
model.MasterPasswordAuthentication?.ToData().ValidateSaltUnchangedForUser(user);Both throw BadRequestException, matching the surrounding error handling.
…idation in SetInitialPasswordData.ValidateDataForUser()
rr-bw
added
t:feature
2 participants
🎟️ Tracking
PM-35599
Client-side changes:
📔 Objective
Update the
accounts/set-passwordendpoint to be able to handle aSetInitialPasswordRequestthat containsMasterPasswordAuthenticationData+MasterPasswordAuthentication+ legacyKeysRequest. In other words, update the endpoint to allow a client to passMPAD+MPUDbut still do V1 encryption. This update is necessary for the corresponding client-side changes in bitwarden/clients#20643Routing matrix
_tdeSetPasswordCommand.SetMasterPasswordAsync(no cryptographic-state change)ValidateSaltUnchangedForUserthen_setInitialMasterPasswordCommandV1;Keys.ToUser()sets keypairAccountKeys != null, falls through_finishSsoJitProvisionMasterPasswordCommand.FinishProvisionAsyncSetInitialPasswordV1Async_setInitialMasterPasswordCommandV1;ToUser()reads legacy KDF fields; keypair set_setInitialMasterPasswordCommandV1; legacy fields; no keypair mutationHasAuthAndUnlockData()is false; legacy validation demands legacy fields tooKdfSettingsValidator.ValidateAuthenticationAndUnlockDatarejectsAccountKeysandKeyssetValidate()MasterPasswordHashValidate()KeyValidate()ValidateSaltUnchangedForUserrejects (Stage 1 PM-27044 invariant)📸 Screenshots
Regression testing existing paths (tested with
clientspointing atmain)(Note: the new path (
MPAD+MPUD+ legacyKeys) will be tested as part of client-side changes in bitwarden/clients#20643)MP JIT with legacy properties
A MP decryption org invites a user. The user JIT provisions. Upon setting initial password, the request sends legacy properties:
masterPasswordHash+key+keys.mp-jit-legacy-properties.mov
MP JIT with new properties and feature flag on
In this video, the
"enable-account-encryption-v2-jit-password-registration"feature flag on (proven by a breakpoint in the video).A MP decryption org invites a user. The user JIT provisions. Upon setting initial password, the request sends
MPAD+MPUD+accountKeys(via SDK).mp-jit-new-properties.mov
TDE JIT with Manage Account Recovery Permission
A TDE org invites a user with the "manage account recovery" permission. The user JIT provisions. Upon setting initial password, the request sends legacy properties:
masterPasswordHash+key+ NO keypair.tde-jit-legacy-properties.mov