[PM-16603] Add userkey rotation v2 #5204
Conversation
|
New Issues (5)Checkmarx found the following issues in this Pull Request
Fixed Issues (17)Great job! The following issues were fixed in this Pull Request
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5204 +/- ##
==========================================
+ Coverage 44.32% 44.65% +0.32%
==========================================
Files 1482 1491 +9
Lines 68376 68690 +314
Branches 6172 6198 +26
==========================================
+ Hits 30307 30671 +364
+ Misses 36761 36704 -57
- Partials 1308 1315 +7 ☔ View full report in Codecov by Sentry. |
quexten
force-pushed
the
km/userkey-rotation-v2
branch
from
7b07f5e to
865137d
Compare
src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountkeysCommand.cs
Show resolved
Hide resolved
|
Forgot to add the feature flag server-side; added it now. |
| @@ -415,6 +419,54 @@ public async Task PostKey([FromBody] UpdateKeyRequestModel model) | |||
| throw new BadRequestException(ModelState); | |||
| } | |||
|
|
|||
| [HttpPost("rotate-user-account-keys")] | |||
| public async Task PostUserAccountKeys([FromBody] RotateUserAccountKeysModel model) | |||
There was a problem hiding this comment.
no unit test and integration test coverage 😢
There was a problem hiding this comment.
Added unit tests and integration tests, please confirm if I missed any obvious cases
| public int? KdfMemory { get; set; } | ||
| public int? KdfParallelism { get; set; } |
There was a problem hiding this comment.
add custom validator instead to cover this scenario for Argon2. Example
| /// <param name="model">All necessary information for rotation. Warning: Any encrypted data not included will be lost.</param> | ||
| /// <returns>An IdentityResult for verification of the master password hash</returns> | ||
| /// <exception cref="ArgumentNullException">User must be provided.</exception> | ||
| Task<IdentityResult> rotateUserAccountKeysAsync(User user, RotateUserAccountKeysData model); |
There was a problem hiding this comment.
in C#, aren't methods supposed to start uppercase ?
| Task<IdentityResult> rotateUserAccountKeysAsync(User user, RotateUserAccountKeysData model); | |
| Task<IdentityResult> RotateUserAccountKeysAsync(User user, RotateUserAccountKeysData model); |
| /// </summary> | ||
| /// <param name="model">All necessary information for rotation. Warning: Any encrypted data not included will be lost.</param> | ||
| /// <returns>An IdentityResult for verification of the master password hash</returns> | ||
| /// <exception cref="ArgumentNullException">User must be provided.</exception> |
There was a problem hiding this comment.
should also mention that InvalidOperationException is possible
|
|
||
| if (!await _userService.CheckPasswordAsync(user, model.OldMasterPasswordHash)) | ||
| { | ||
| return IdentityResult.Failed(_identityErrorDescriber.PasswordMismatch()); |
There was a problem hiding this comment.
why not just throw error instead ?
There was a problem hiding this comment.
This just copies the existing behavior for RotateUserKeyAsync. Are you saying we should remove the entire "IdentityResult" and make it either return nothing (on success) or throw on password mismatch?
| if (model.Ciphers.Any() || model.Folders.Any() || model.Sends.Any() || model.EmergencyAccesses.Any() || | ||
| model.OrganizationUsers.Any() || model.WebAuthnKeys.Any()) | ||
| { | ||
| List<UpdateEncryptedDataForKeyRotation> saveEncryptedDataActions = new(); | ||
|
|
||
| if (model.Ciphers.Any()) | ||
| { | ||
| saveEncryptedDataActions.Add(_cipherRepository.UpdateForKeyRotation(user.Id, model.Ciphers)); | ||
| } | ||
|
|
||
| if (model.Folders.Any()) | ||
| { | ||
| saveEncryptedDataActions.Add(_folderRepository.UpdateForKeyRotation(user.Id, model.Folders)); | ||
| } | ||
|
|
||
| if (model.Sends.Any()) | ||
| { | ||
| saveEncryptedDataActions.Add(_sendRepository.UpdateForKeyRotation(user.Id, model.Sends)); | ||
| } | ||
|
|
||
| if (model.EmergencyAccesses.Any()) | ||
| { | ||
| saveEncryptedDataActions.Add( | ||
| _emergencyAccessRepository.UpdateForKeyRotation(user.Id, model.EmergencyAccesses)); | ||
| } | ||
|
|
||
| if (model.OrganizationUsers.Any()) | ||
| { | ||
| saveEncryptedDataActions.Add( | ||
| _organizationUserRepository.UpdateForKeyRotation(user.Id, model.OrganizationUsers)); | ||
| } | ||
|
|
||
| if (model.WebAuthnKeys.Any()) | ||
| { | ||
| saveEncryptedDataActions.Add(_credentialRepository.UpdateKeysForRotationAsync(user.Id, model.WebAuthnKeys)); | ||
| } | ||
|
|
||
| await _userRepository.UpdateUserKeyAndEncryptedDataAsync(user, saveEncryptedDataActions); | ||
| } | ||
| else | ||
| { | ||
| await _userRepository.ReplaceAsync(user); | ||
| } |
There was a problem hiding this comment.
since we do individual checks anyway below, it would be sufficient to just guard the _userRepository.UpdateUserKeyAndEncryptedDataAsync with the if (saveEncryptedDataActions.Any()), like so:
| if (model.Ciphers.Any() || model.Folders.Any() || model.Sends.Any() || model.EmergencyAccesses.Any() || | |
| model.OrganizationUsers.Any() || model.WebAuthnKeys.Any()) | |
| { | |
| List<UpdateEncryptedDataForKeyRotation> saveEncryptedDataActions = new(); | |
| if (model.Ciphers.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_cipherRepository.UpdateForKeyRotation(user.Id, model.Ciphers)); | |
| } | |
| if (model.Folders.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_folderRepository.UpdateForKeyRotation(user.Id, model.Folders)); | |
| } | |
| if (model.Sends.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_sendRepository.UpdateForKeyRotation(user.Id, model.Sends)); | |
| } | |
| if (model.EmergencyAccesses.Any()) | |
| { | |
| saveEncryptedDataActions.Add( | |
| _emergencyAccessRepository.UpdateForKeyRotation(user.Id, model.EmergencyAccesses)); | |
| } | |
| if (model.OrganizationUsers.Any()) | |
| { | |
| saveEncryptedDataActions.Add( | |
| _organizationUserRepository.UpdateForKeyRotation(user.Id, model.OrganizationUsers)); | |
| } | |
| if (model.WebAuthnKeys.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_credentialRepository.UpdateKeysForRotationAsync(user.Id, model.WebAuthnKeys)); | |
| } | |
| await _userRepository.UpdateUserKeyAndEncryptedDataAsync(user, saveEncryptedDataActions); | |
| } | |
| else | |
| { | |
| await _userRepository.ReplaceAsync(user); | |
| } | |
| List<UpdateEncryptedDataForKeyRotation> saveEncryptedDataActions = new(); | |
| if (model.Ciphers.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_cipherRepository.UpdateForKeyRotation(user.Id, model.Ciphers)); | |
| } | |
| if (model.Folders.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_folderRepository.UpdateForKeyRotation(user.Id, model.Folders)); | |
| } | |
| if (model.Sends.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_sendRepository.UpdateForKeyRotation(user.Id, model.Sends)); | |
| } | |
| if (model.EmergencyAccesses.Any()) | |
| { | |
| saveEncryptedDataActions.Add( | |
| _emergencyAccessRepository.UpdateForKeyRotation(user.Id, model.EmergencyAccesses)); | |
| } | |
| if (model.OrganizationUsers.Any()) | |
| { | |
| saveEncryptedDataActions.Add( | |
| _organizationUserRepository.UpdateForKeyRotation(user.Id, model.OrganizationUsers)); | |
| } | |
| if (model.WebAuthnKeys.Any()) | |
| { | |
| saveEncryptedDataActions.Add(_credentialRepository.UpdateKeysForRotationAsync(user.Id, model.WebAuthnKeys)); | |
| } | |
| if (saveEncryptedDataActions.Any()) | |
| { | |
| await _userRepository.UpdateUserKeyAndEncryptedDataAsync(user, saveEncryptedDataActions); | |
| } | |
| else | |
| { | |
| await _userRepository.ReplaceAsync(user); | |
| } |
| @@ -219,31 +219,19 @@ public async Task UpdateUserKeyAndEncryptedDataAsync( | |||
| await using var transaction = connection.BeginTransaction(); | |||
| try | |||
| { | |||
| // Update user | |||
| await using (var cmd = new SqlCommand("[dbo].[User_UpdateKeys]", connection, transaction)) | |||
| if (user.AccountRevisionDate == null) | |||
There was a problem hiding this comment.
The User model suggests, this field can never be null
There was a problem hiding this comment.
:| I wonder why the SP had handling for this then:
| } | ||
|
|
||
| // Update user | ||
| await ReplaceAsync(user); |
There was a problem hiding this comment.
this method will operate under new connection, hence commit immediately. There is a reason why the previous SP existed.
This repository overrides ReplaceAsync and protects Key and Master Password in some way, might be worth to check and possibly revert to use the previous SP.
Note: Previously used SP could be refactored with connection.ExecuteAsync, where transaction can be passed in. That way we don't have to deal with SqlDbType and @ named arguments.
There was a problem hiding this comment.
This repository overrides ReplaceAsync and protects Key and Master Password in some way, might be worth to check and possibly revert to use the previous SP.
Hmm, this is actually a bug leading to keys not being adequately protected when doing a key-rotation :| So if we want to use the stored procedure, we would have to fix it to also protect the data properly.
There was a problem hiding this comment.
Replaced with:
ProtectData(user);
await connection.ExecuteAsync(
$"[{Schema}].[{Table}_Update]",
user,
transaction: transaction,
commandType: CommandType.StoredProcedure);
| @@ -0,0 +1,7 @@ | |||
| --- Remove old updatekeys procedure that is now unused --- | |||
|
|
|||
| IF OBJECT_ID('[dbo].[User_UpdateKeys]') IS NOT NULL | |||
There was a problem hiding this comment.
Do you want to delete this SP from repo ? Location:
quexten
force-pushed
the
km/userkey-rotation-v2
branch
from
f3c884d to
5dd74db
Compare
There was a problem hiding this comment.
Looking good, a few minor things to take a look at.
An overall suggestion for net new code, can we use #nullable enable at the top of the files to enable nullable reference types? This is currently a mix of using that feature in some places and not in others, which I find confusing.
| [StringLength(50)] | ||
| public string MasterPasswordHint { get; set; } | ||
|
|
||
| public IEnumerable<ValidationResult> Validate(ValidationContext validationContext) |
There was a problem hiding this comment.
Can we add unit tests for this?
See https://github.com/bitwarden/server/blob/main/test/Api.Test/NotificationCenter/Models/Request/NotificationFilterRequestModelTests.cs for examples.
| @@ -0,0 +1,11 @@ | |||
| using System.ComponentModel.DataAnnotations; | |||
There was a problem hiding this comment.
| using System.ComponentModel.DataAnnotations; | |
| #nullable enable | |
| using System.ComponentModel.DataAnnotations; |
I would recommend keeping consistent with using nullable reference types on all these request models.
At the moment, the AccountsKeyManagementController uses it.
So AccountKeysRequestModel, RotateUserAccountKeysAndDataRequestModel, MasterPasswordUnlockDataModel, UnlockDataRequestModel, and AccountDataRequestModel.
Feel free to reach out if you have questions around this.
| @@ -0,0 +1,11 @@ | |||
| using System.ComponentModel.DataAnnotations; | |||
|
|
|||
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; | |||
There was a problem hiding this comment.
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; | |
| namespace Bit.Api.KeyManagement.Models.Requests; |
⛏️ Optional.
There is a common convention in .NET projects to have the namespace match the file location.
Some IDEs will scan for it https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/style-rules/ide0130.
In the server project for the most part we follow it. There are areas we don't currently.
| @@ -0,0 +1,15 @@ | |||
| using System.ComponentModel.DataAnnotations; | |||
|
|
|||
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; | |||
There was a problem hiding this comment.
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; | |
| namespace Bit.Api.KeyManagement.Models.Requests; |
| using Bit.Api.Auth.Models.Request.Accounts; | ||
| using Bit.Api.Auth.Models.Request.WebAuthn; | ||
|
|
||
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; |
There was a problem hiding this comment.
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; | |
| namespace Bit.Api.KeyManagement.Models.Requests; |
| using Bit.Api.Tools.Models.Request; | ||
| using Bit.Api.Vault.Models.Request; | ||
|
|
||
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; |
There was a problem hiding this comment.
| namespace Bit.Api.KeyManagement.Models.Request.Accounts; | |
| namespace Bit.Api.KeyManagement.Models.Requests; |
| try | ||
| { | ||
| // Update user | ||
| var userEntity = await dbContext.Users.FindAsync(user.Id); | ||
| if (userEntity == null) | ||
| { | ||
| throw new ArgumentException("User not found", nameof(user)); | ||
| } | ||
|
|
||
| userEntity.SecurityStamp = user.SecurityStamp; | ||
| userEntity.Key = user.Key; | ||
| userEntity.MasterPassword = user.MasterPassword; | ||
| userEntity.PrivateKey = user.PrivateKey; | ||
| userEntity.LastKeyRotationDate = user.LastKeyRotationDate; | ||
| userEntity.AccountRevisionDate = user.AccountRevisionDate; | ||
| userEntity.RevisionDate = user.RevisionDate; | ||
|
|
||
| await dbContext.SaveChangesAsync(); | ||
|
|
||
| // Update re-encrypted data | ||
| foreach (var action in updateDataActions) | ||
| { | ||
| // connection and transaction aren't used in EF | ||
| await action(); | ||
| } | ||
|
|
||
| await transaction.CommitAsync(); | ||
| } | ||
| catch | ||
| { | ||
| await transaction.RollbackAsync(); | ||
| throw; | ||
| } | ||
|
|
There was a problem hiding this comment.
| try | |
| { | |
| // Update user | |
| var userEntity = await dbContext.Users.FindAsync(user.Id); | |
| if (userEntity == null) | |
| { | |
| throw new ArgumentException("User not found", nameof(user)); | |
| } | |
| userEntity.SecurityStamp = user.SecurityStamp; | |
| userEntity.Key = user.Key; | |
| userEntity.MasterPassword = user.MasterPassword; | |
| userEntity.PrivateKey = user.PrivateKey; | |
| userEntity.LastKeyRotationDate = user.LastKeyRotationDate; | |
| userEntity.AccountRevisionDate = user.AccountRevisionDate; | |
| userEntity.RevisionDate = user.RevisionDate; | |
| await dbContext.SaveChangesAsync(); | |
| // Update re-encrypted data | |
| foreach (var action in updateDataActions) | |
| { | |
| // connection and transaction aren't used in EF | |
| await action(); | |
| } | |
| await transaction.CommitAsync(); | |
| } | |
| catch | |
| { | |
| await transaction.RollbackAsync(); | |
| throw; | |
| } | |
| // Update user | |
| var userEntity = await dbContext.Users.FindAsync(user.Id); | |
| if (userEntity == null) | |
| { | |
| throw new ArgumentException("User not found", nameof(user)); | |
| } | |
| userEntity.SecurityStamp = user.SecurityStamp; | |
| userEntity.Key = user.Key; | |
| userEntity.MasterPassword = user.MasterPassword; | |
| userEntity.PrivateKey = user.PrivateKey; | |
| userEntity.LastKeyRotationDate = user.LastKeyRotationDate; | |
| userEntity.AccountRevisionDate = user.AccountRevisionDate; | |
| userEntity.RevisionDate = user.RevisionDate; | |
| await dbContext.SaveChangesAsync(); | |
| // Update re-encrypted data | |
| foreach (var action in updateDataActions) | |
| { | |
| // connection and transaction aren't used in EF | |
| await action(); | |
| } | |
| await transaction.CommitAsync(); | |
⛏️ Optional
With the default behavior, you get rollbacks without having to do the try catch https://learn.microsoft.com/en-us/ef/core/saving/transactions#default-transaction-behavior
| @@ -93,4 +105,46 @@ await sutProvider.GetDependency<IRegenerateUserAsymmetricKeysCommand>().Received | |||
| Arg.Is(orgUsers), | |||
| Arg.Is(accessDetails)); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Would expect unit tests for failure cases.
_userService.GetUserByPrincipalAsync returning null and throwing UnauthorizedAccessException.
var result = await _rotateUserAccountKeysCommand.RotateUserAccountKeysAsync(user, dataModel); returning result.Errors and throwing BadRequestException
|
|
||
| ProtectData(user); | ||
| await connection.ExecuteAsync( | ||
| $"[{Schema}].[{Table}_Update]", |
There was a problem hiding this comment.
Seems odd to be doing a complete replacement of the user entity when this is called UpdateUserKeyAndEncryptedDataV2Async. If anything changes on that user between the controller fetch and calling this repository method, we will overwrite it.
Can we do the following or is there something I'm missing?:
if (user.AccountRevisionDate == null)
{
user.AccountRevisionDate = user.RevisionDate;
}
// Update user values using data protection
ProtectData(user);
// Update user columns needed for key rotation
await connection.ExecuteAsync(
$"[{Schema}].[User_UpdateKeys]",
new
{
user.Id,
user.SecurityStamp,
user.Key,
user.PrivateKey,
user.RevisionDate,
user.AccountRevisionDate,
user.LastKeyRotationDate
},
commandType: CommandType.StoredProcedure);
// Update re-encrypted data
foreach (var action in updateDataActions)
{
await action(connection, transaction);
}
There was a problem hiding this comment.
The above is missing at least the masterpassword hash and masterpassword hint.
As it is implemented right now, we do our sanity/safety checks regarding the: public key, email, kdf settings, before calling UpdateUserKeyAndEncryptedDataV2Async, with one user object that has been validated.
If we just update the above, it could be the case that f.e the kdf setting changes after the sanity check, but before the above update, leading to a key that does not match the kdf settings being stored.
So we would either need to include:
public key, email, kdf settings, in the update too, replacing them?
There was a problem hiding this comment.
Looks like we're missing updating those fields in the EF code too then?
I'm confused here.
There was a problem hiding this comment.
Hmm, that means that the EF code is wrong. I think that means the EF code needs better test coverage in this case because this could have slipped through.
There was a problem hiding this comment.
I know we recently published some new contributing docs info on repository integration tests.
https://contributing.bitwarden.com/contributing/testing/database/
https://contributing.bitwarden.com/getting-started/server/database/ef/#testing-ef-changes
Might be something to look into here.
…into km/userkey-rotation-v2
LaunchDarkly flag references🔍 1 flag added or modified
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16603
Client PR: bitwarden/clients#12646
📔 Objective
Adds a new command for user-key rotation that includes the KDF settings too, and updates the password. Full description of why this change is made is on the client PR.
In short, adds a new endpoint
rotate-user-account-keyswhich performs userkey rotation, while updating the masterpassword, and kdf parameters, to ensure a consistent update.📸 Screenshots
⏰ Reminders before review
🦮 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