[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 53 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 0 63.23% +63.23%
=========================================
Files 0 2381 +2381
Lines 0 103788 +103788
Branches 0 9402 +9402
=========================================
+ Hits 0 65635 +65635
- Misses 0 35924 +35924
- Partials 0 2229 +2229 ☔ 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
rr-bw
added
t:feature
ike-kottlowski
left a comment
ike-kottlowski
left a comment
There was a problem hiding this comment.
Just some more questions. Otherwise looks good.
| /// request because they already have one. Checks both AccountKeys (new) and Keys (legacy) so | ||
| /// the predicate is correct for the transitional period where clients may send either key shape. | ||
| /// </summary> | ||
| public bool IsTdeSetPasswordRequest() |
There was a problem hiding this comment.
❓ : If a client could send either AccountKeys or Keys shouldn't the logic be || (OR) not && (AND).
If AccountKeys == null then it's V1 TDESetPassword, if Keys == null then it's V2 TDESetPassword?
But both have to be null in this case? Maybe the logic works either way?
There was a problem hiding this comment.
IsTdeSetPasswordRequest() is not concerned with distinguishing V1 vs. V2 encryption users. It’s simply asking “Is this a TDE user?”, which is determined by the complete absence (hence the use of &&) of a key pair of either sort on the request.
The method doesn’t need to distinguish V1 vs. V2 encryption because in a TDE set-password scenario, the user’s cryptographic state was already previously set up (whether V1 or V2), and all we are doing through the "set-password" endpoint is just setting the password via _tdeSetPasswordCommand.SetMasterPasswordAsync() (i.e. that command does not set cryptographic state; it just sets the password).
// TDE set-password (for TDE users who obtain the "manage account recovery" permission).
// _tdeSetPasswordCommand handles both V1 and V2 TDE users (it sets the master password
// without touching cryptographic state).
//
// Note: Why not check the V2RegistrationTDEJIT flag?
// The V2RegistrationTDEJIT flag governs SSO+TDE account creation, not set-password, so
// don't reference it here (no feature-flag gate), because it isn't relevant. A TDE user
// reaching this endpoint already has keys that were set up at registration time, regardless
// of the flag.
if (model.IsTdeSetPasswordRequest())
{
await _tdeSetPasswordCommand.SetMasterPasswordAsync(user, model.ToData());
return;
}This is distinct from our handling of MP JIT users, where we do need to distinguish between V1 vs. V2 encryption. The _finishSsoJitProvisionMasterPasswordCommand.FinishProvisionAsync() method does set cryptographic state - specifically it sets V2 encryption state via _userRepository.SetV2AccountCryptographicStateAsync() - which is why that branch must be gated by an AccountKeys presence check (not legacy Keys) as well as a feature flag check.
// V2 encryption - MP JIT.
// We require AccountKeys (the new key shape) here, not legacy Keys — otherwise
// a modern V1 MP JIT request (MPAD + MPUD + legacy Keys) would be incorrectly routed here
// when the flag is on, and `model.ToData().AccountKeys` would be null, breaking the V2 MP
// JIT command (which requires AccountKeys per FinishSsoJitProvisionMasterPasswordCommand).
if (model.AccountKeys != null &&
_featureService.IsEnabled(FeatureFlagKeys.EnableAccountEncryptionV2JitPasswordRegistration))
{
await _finishSsoJitProvisionMasterPasswordCommand.FinishProvisionAsync(user, model.ToData());
return;
}But if the request instead contains Keys, then we are dealing with a V1 encryption and so we fall back to SetInitialPasswordV1Async().
There was a problem hiding this comment.
@mzieniukbw Do you believe that the above is an accurate description? Especially regarding the idea that the _tdeSetPasswordCommand.SetMasterPasswordAsync() covers (sets a password for) both V1 and V2 encryption TDE users, and therefore there is no need to distinguish between V1 vs. V2 encryption for TDE users?
There was a problem hiding this comment.
I am fine with this one. We should not expect either. That being said, see my other comment, where i have concerns about gaps in validation. We should really expect one or another, never a mix of MPAD/MPUD and legacy.
| { | ||
| // Arrange — modern MP JIT client: MPAD + MPUD + legacy Keys (no AccountKeys) | ||
| // Salt must match the user's email-derived salt (Stage 1 PM-27044 invariant). | ||
| var emailSalt = user.GetMasterPasswordSalt(); |
There was a problem hiding this comment.
👍🏻: In this test since we call GetMasterPasswordSalt the salt may or may not be an email thus adding flexibility.
Ownership transferred. Changes have been addressed.
|
|
||
| // If both modern and legacy fields are present for the same value, they must agree — | ||
| // reject ambiguous input rather than silently letting the modern field win via ?? fallback. | ||
| if (MasterPasswordAuthentication != null && MasterPasswordHash != null |
There was a problem hiding this comment.
That impossible scenario. Furthermore, you should MasterPasswordAuthentication or MasterPasswordHash, not both at the same time. (Checked existing code and linked clients PR).
If you want to check for correctness, then expect there is at least one - preferably expect that either MasterPasswordAuthentication and MasterPasswordUnlock are provided (with Keys or AccountKeys to distinguish modern V1 vs V2) at the same time OR the other legacy. If you have both or none, then that's should be validation error.
There was a problem hiding this comment.
Update here: c0e2ba7 (and related tests ed33516)
if (hasModern && hasLegacy)- ensures that we don't have both shapes- After that, the
if (HasAuthAndUnlockData())check, followed by theif (string.IsNullOrEmpty(MasterPasswordHash))andif (string.IsNullOrEmpty(Key))checks, will validate that we have one or the other (i.e. if we receive neither, we yield"MasterPasswordHash must be supplied."
| [nameof(MasterPasswordAuthentication), nameof(MasterPasswordHash)]); | ||
| } | ||
|
|
||
| if (MasterPasswordUnlock != null && Key != null |
There was a problem hiding this comment.
See my other comment, this scenario does not exist in clients.
There was a problem hiding this comment.
Update here: c0e2ba7 (and related tests ed33516)
if (hasModern && hasLegacy)- ensures that we don't have both shapes- After that, the
if (HasAuthAndUnlockData())check, followed by theif (string.IsNullOrEmpty(MasterPasswordHash))andif (string.IsNullOrEmpty(Key))checks, will validate that we have one or the other (i.e. if we receive neither, we yield"MasterPasswordHash must be supplied."
| public IEnumerable<ValidationResult> Validate(ValidationContext validationContext) | ||
| { | ||
| if (IsV2Request()) | ||
| if (AccountKeys != null && Keys != null) |
There was a problem hiding this comment.
See my other comment, this scenario does not exist in clients.
There was a problem hiding this comment.
I would argue keep this. Even though clients don't send both, the endpoint/model technically does accept both at the same time, so this is just a defensive measure.
| /// request because they already have one. Checks both AccountKeys (new) and Keys (legacy) so | ||
| /// the predicate is correct for the transitional period where clients may send either key shape. | ||
| /// </summary> | ||
| public bool IsTdeSetPasswordRequest() |
There was a problem hiding this comment.
I am fine with this one. We should not expect either. That being said, see my other comment, where i have concerns about gaps in validation. We should really expect one or another, never a mix of MPAD/MPUD and legacy.
4 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