[PM-27279] Implement TDE Registration with V2 Keys #6671
Conversation
|
New Issues (3)Checkmarx found the following issues in this Pull Request
|
quexten
changed the title
quexten
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6671 +/- ##
===========================================
+ Coverage 13.40% 53.86% +40.45%
===========================================
Files 1146 1917 +771
Lines 49687 85128 +35441
Branches 3896 7615 +3719
===========================================
+ Hits 6661 45850 +39189
+ Misses 42904 37512 -5392
- Partials 122 1766 +1644 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
quexten
force-pushed
the
km/account-keys-command
branch
from
08db72c to
e4a8a41
Compare
quexten
force-pushed
the
km/tde-registration
branch
from
115f158 to
6210b74
Compare
|
quexten
force-pushed
the
km/tde-registration
branch
from
056ce5d to
dd73f95
Compare
| await _userService.SaveUserAsync(model.ToUser(user)); | ||
| return new KeysResponseModel(new UserAccountKeysData | ||
| { | ||
| PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData( | ||
| user.PrivateKey, | ||
| user.PublicKey | ||
| ) | ||
| }, user.Key); |
There was a problem hiding this comment.
SaveUserAsync, the user object in memory may not reflect what was actually persisted to the database.
The SaveUserAsync method calls model.ToUser(user) which modifies the user object, but there's no guarantee that the returned user.PrivateKey and user.PublicKey match what was saved. This could happen if:
- Database triggers modify the values
- The save operation fails partially
- There are concurrent updates
Recommendation: Reload the user from the database after saving:
var updatedUser = model.ToUser(user);
await _userService.SaveUserAsync(updatedUser);
// Reload to ensure we return what was actually saved
user = await _userService.GetUserByIdAsync(updatedUser.Id);
return new KeysResponseModel(new UserAccountKeysData
{
PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData(
user.PrivateKey,
user.PublicKey
)
}, user.Key);Alternatively, use the updatedUser reference:
var updatedUser = model.ToUser(user);
await _userService.SaveUserAsync(updatedUser);
return new KeysResponseModel(new UserAccountKeysData
{
PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData(
updatedUser.PrivateKey,
updatedUser.PublicKey
)
}, updatedUser.Key);
| else | ||
| { | ||
| // Todo: Drop this after a transition period | ||
| await _userService.SaveUserAsync(model.ToUser(user)); | ||
| return new KeysResponseModel(new UserAccountKeysData | ||
| { | ||
| PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData( | ||
| user.PrivateKey, | ||
| user.PublicKey | ||
| ) | ||
| }, user.Key); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| // Todo: Drop this after a transition period | ||
| await _userService.SaveUserAsync(model.ToUser(user)); | ||
| return new KeysResponseModel(new UserAccountKeysData | ||
| { | ||
| PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData( | ||
| user.PrivateKey, | ||
| user.PublicKey | ||
| ) | ||
| }, user.Key); | ||
| } |
There was a problem hiding this comment.
♻️ CODE DUPLICATION: The v1 fallback logic (lines 458-466 and 471-479) is duplicated. This makes the code harder to maintain and increases the risk of bugs.
Recommendation: Extract the common v1 handling logic:
if (model.AccountKeys != null)
{
var accountKeysData = model.AccountKeys.ToAccountKeysData();
if (accountKeysData.IsV2Encryption())
{
await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, accountKeysData);
return new KeysResponseModel(accountKeysData, user.Key);
}
}
// V1 fallback path (handles both null AccountKeys and non-V2 AccountKeys)
var updatedUser = model.ToUser(user);
await _userService.SaveUserAsync(updatedUser);
return new KeysResponseModel(new UserAccountKeysData
{
PublicKeyEncryptionKeyPairData = new PublicKeyEncryptionKeyPairData(
updatedUser.PrivateKey,
updatedUser.PublicKey
)
}, updatedUser.Key);This simplifies the logic and makes it clear that there are two paths: V2 and V1 (fallback).
| [Obsolete("Use AccountKeys.UserKeyEncryptedAccountPrivateKey instead")] | ||
| [Required] | ||
| public string EncryptedPrivateKey { get; set; } | ||
| public AccountKeysRequestModel AccountKeys { get; set; } |
There was a problem hiding this comment.
AccountKeys property has no validation attributes, while the obsolete properties have [Required].
This creates an ambiguous state where:
- Old clients send only
PublicKeyandEncryptedPrivateKey(both marked[Required]) - New clients send only
AccountKeys(no validation) - But a request could potentially provide neither, or both
The current #nullable disable at the top of the file masks this issue.
Recommendation: Add validation to ensure at least one set of keys is provided:
public class KeysRequestModel : IValidatableObject
{
[Obsolete("Use AccountKeys.AccountPublicKey instead")]
public string? PublicKey { get; set; }
[Obsolete("Use AccountKeys.UserKeyEncryptedAccountPrivateKey instead")]
public string? EncryptedPrivateKey { get; set; }
public AccountKeysRequestModel? AccountKeys { get; set; }
public IEnumerable<ValidationResult> Validate(ValidationContext validationContext)
{
// Either new AccountKeys or both legacy fields must be provided
if (AccountKeys == null &&
(string.IsNullOrWhiteSpace(PublicKey) || string.IsNullOrWhiteSpace(EncryptedPrivateKey)))
{
yield return new ValidationResult(
"Either AccountKeys or both PublicKey and EncryptedPrivateKey must be provided.",
new[] { nameof(AccountKeys), nameof(PublicKey), nameof(EncryptedPrivateKey) });
}
}
}This would also allow removing the #nullable disable directive and the now-incorrect [Required] attributes.
| Assert.NotNull(result); | ||
| Assert.Equal("keys", result.Object); | ||
| } |
There was a problem hiding this comment.
📝 TEST COVERAGE GAP: This test only validates that the result is not null and has the correct object type. It doesn't verify:
- The response contains the correct key data
- The
AccountKeysproperty is properly populated - The
Keyproperty matchesuser.Key
Recommendation: Add comprehensive response validation:
// Assert
await _userRepository.Received(1).SetV2AccountCryptographicStateAsync(
user.Id,
Arg.Any<UserAccountKeysData>());
await _userService.DidNotReceiveWithAnyArgs().SaveUserAsync(Arg.Any<User>());
Assert.NotNull(result);
Assert.Equal("keys", result.Object);
Assert.Equal(user.Key, result.Key);
Assert.NotNull(result.AccountKeys);
Assert.NotNull(result.PublicKey);
Assert.NotNull(result.PrivateKey);Similar validation should be added to the PostKeys_WithoutAccountKeys_CallsSaveUser test as well.
| Assert.NotNull(result); | ||
| Assert.Equal("keys", result.Object); | ||
| } |
There was a problem hiding this comment.
📝 TEST COVERAGE GAP: Similar to the V2 test, this test needs better response validation to catch potential bugs in response construction.
Recommendation: Add assertions to verify the response data:
// Assert
await _userService.Received(1).SaveUserAsync(Arg.Is<User>(u =>
u.PublicKey == model.PublicKey &&
u.PrivateKey == model.EncryptedPrivateKey));
await _userRepository.DidNotReceiveWithAnyArgs()
.SetV2AccountCryptographicStateAsync(Arg.Any<Guid>(), Arg.Any<UserAccountKeysData>());
Assert.NotNull(result);
Assert.Equal("keys", result.Object);
Assert.Equal(user.Key, result.Key);
Assert.NotNull(result.AccountKeys);
Assert.Equal(model.PublicKey, result.PublicKey);
Assert.Equal(model.EncryptedPrivateKey, result.PrivateKey);Note: This assumes the controller is fixed to use the updated user reference (see my comment on AccountsController.cs).
| if (accountKeysData.IsV2Encryption()) | ||
| { | ||
| await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, accountKeysData); | ||
| return new KeysResponseModel(accountKeysData, user.Key); |
There was a problem hiding this comment.
✅ GOOD: Properly validating V2 encryption state with IsV2Encryption() before calling SetV2AccountCryptographicStateAsync. This prevents invalid cryptographic states from being persisted.
The repository method also performs this validation, providing defense-in-depth.
| public KeysResponseModel(UserAccountKeysData accountKeys, string? masterKeyWrappedUserKey) | ||
| : base("keys") | ||
| { | ||
| if (user == null) | ||
| if (masterKeyWrappedUserKey != null) | ||
| { | ||
| throw new ArgumentNullException(nameof(user)); | ||
| Key = masterKeyWrappedUserKey; | ||
| } | ||
|
|
||
| Key = user.Key; | ||
| PublicKey = user.PublicKey; | ||
| PrivateKey = user.PrivateKey; | ||
| PublicKey = accountKeys.PublicKeyEncryptionKeyPairData.PublicKey; | ||
| PrivateKey = accountKeys.PublicKeyEncryptionKeyPairData.WrappedPrivateKey; | ||
| AccountKeys = new PrivateKeysResponseModel(accountKeys); |
There was a problem hiding this comment.
👍 GOOD REFACTORING: The new constructor properly:
- Accepts
UserAccountKeysDatainstead of the fullUserentity, following separation of concerns - Makes
masterKeyWrappedUserKeynullable and optional - Populates both legacy properties (for backward compatibility) and new
AccountKeysproperty - Removes the
#nullable disabledirective
This is a clean migration path that maintains backward compatibility while supporting V2 encryption.
enmande
left a comment
enmande
left a comment
There was a problem hiding this comment.
Nice work, thank you! Appreciate the test coverage and the Obsolete decorations which extend clarity to the v1/v2 encryption transition. I'd like to request two very minor changes that Claude has suggested. I've 👍 both of these for reference.
- The comment marked " ♻️ CODE DUPLICATION ": Arranging this logic to reduce nesting and focus the comment(s) in one place improves readability and makes it clearer what is being left behind when we come back to codify v2. This feels in line with how we would structure the branching logic for, e.g., a feature flag, to support targeted unwinding.
- The comment marked "
⚠️ DATA INCONSISTENCY RISK ": Mutation can occur on the in-memory object's key members. Unless it is intentional to not send these back, in which case I think we're relying on a sync if there is change, we should be saving and returning the result of the.ToUserextension cast (Claude's "alternative" recommendation) for consistency.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27279
📔 Objective
Adds registration with V2 keys for TDE flows. TDE flows use the postKeys method to set the keys. In this PR, this is extended to set the keys via a newly introduced command instead. The command supports both v1 and v2 under the hood.
📸 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