[PM-27280] Support v2 encryption on key-connector signups #6712
Conversation
|
New Issues (3)Checkmarx found the following issues in this Pull Request
|
mzieniukbw
force-pushed
the
km/pm-27280-v2-encryption-on-key-connector-signups
branch
from
5e1c1cc to
e424f19
Compare
mzieniukbw
requested review from
Patrick-Pimentel-Bitwarden and
Thomas-Avery
|
Claude finished @mzieniukbw's task —— View job Code Review Complete ✓Tasks
Review SummaryI've completed a comprehensive security-focused code review of this PR. The bitwarden-code-reviewer agent has posted detailed inline comments on specific issues found in the code. Key findings: Critical Issues:
High Priority:
Positive Aspects:
The implementation follows Bitwarden's security principles overall, but the validation gaps and transaction safety issues should be addressed before merging to ensure data integrity and security. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/Infrastructure.EntityFramework/Repositories/UserRepository.cs
Outdated
Show resolved
Hide resolved
src/Core/KeyManagement/Models/Api/Request/SetKeyConnectorKeyRequestModel.cs
Show resolved
Hide resolved
util/Migrator/DbScripts/2025-12-11_00_User_UpdateKeyConnectorUserKey.sql
Show resolved
Hide resolved
src/Core/KeyManagement/Models/Api/Request/SetKeyConnectorKeyRequestModel.cs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6712 +/- ##
==========================================
+ Coverage 53.86% 57.86% +4.00%
==========================================
Files 1917 1924 +7
Lines 85126 85350 +224
Branches 7620 7635 +15
==========================================
+ Hits 45853 49389 +3536
+ Misses 37508 34118 -3390
- Partials 1765 1843 +78 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/Core/KeyManagement/Models/Api/Request/SetKeyConnectorKeyRequestModel.cs
Show resolved
Hide resolved
util/Migrator/DbScripts/2025-12-11_00_User_UpdateKeyConnectorUserKey.sql
Show resolved
Hide resolved
src/Core/KeyManagement/Models/Api/Request/SetKeyConnectorKeyRequestModel.cs
Show resolved
Hide resolved
src/Sql/dbo/KeyManagement/Stored Procedures/User_UpdateKeyConnectorUserKey.sql
Show resolved
Hide resolved
| @@ -0,0 +1,35 @@ | |||
| IF OBJECT_ID('[dbo].[User_UpdateKeyConnectorUserKey]') IS NOT NULL | |||
There was a problem hiding this comment.
✅ Migration Safety: Good DROP IF EXISTS pattern
The migration script properly checks for existence before dropping, which is good practice.
Minor suggestions:
- Consider adding a comment explaining what this stored procedure does:
-- Creates/updates the User_UpdateKeyConnectorUserKey stored procedure
-- Used during Key Connector setup to update user's cryptographic key state
-- and KDF parameters when enrolling in Key Connector- Consider adding a rollback script in comments for easy reference:
-- To rollback: DROP PROCEDURE [dbo].[User_UpdateKeyConnectorUserKey]This helps with understanding and potential rollback scenarios.
| } | ||
|
|
||
| [Theory, DatabaseData] | ||
| public async Task SetKeyConnectorUserKey_UpdatesUserKey(IUserRepository userRepository, Database database) |
There was a problem hiding this comment.
✅ Good Test Coverage: Integration test for SetKeyConnectorUserKey
This test validates that the method updates the user key correctly. However, consider expanding test coverage:
Missing test scenarios:
- Non-existent user: What happens if the userId doesn't exist?
- KDF parameters: Test verifies Key is updated but doesn't check if KDF parameters are set correctly
- UsesKeyConnector flag: Verify the flag is set to true
- Revision dates: Confirm RevisionDate and AccountRevisionDate are updated
- Concurrent updates: Test for race conditions
Suggested additions:
[Theory, DatabaseData]
public async Task SetKeyConnectorUserKey_SetsKdfParametersCorrectly(IUserRepository userRepository, Database database)
{
var user = await userRepository.CreateTestUserAsync();
var keyConnectorWrappedKey = "key-connector-wrapped-user-key";
var setKeyConnectorUserKeyDelegate = userRepository.SetKeyConnectorUserKey(user.Id, keyConnectorWrappedKey);
await RunUpdateUserDataAsync(setKeyConnectorUserKeyDelegate, database);
var updatedUser = await userRepository.GetByIdAsync(user.Id);
Assert.Equal(KdfType.Argon2id, updatedUser.Kdf);
Assert.Equal(AuthConstants.ARGON2_ITERATIONS.Default, updatedUser.KdfIterations);
Assert.True(updatedUser.UsesKeyConnector);
Assert.InRange(updatedUser.RevisionDate, DateTime.UtcNow.AddMinutes(-1), DateTime.UtcNow);
}
src/Core/KeyManagement/Models/Api/Request/SetKeyConnectorKeyRequestModel.cs
Show resolved
Hide resolved
| var setKeyConnectorUserKeyTask = | ||
| _userRepository.SetKeyConnectorUserKey(user.Id, requestModel.KeyConnectorKeyWrappedUserKey); | ||
|
|
||
| await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, requestModel.AccountKeys.ToAccountKeysData(), |
There was a problem hiding this comment.
Security Concern: Null Reference Exception Risk
The code calls requestModel.AccountKeys.ToAccountKeysData() without null checking, even though validation only checks if AccountKeys is not null at line 38. However, there's a potential race condition or programming error risk here.
More importantly, the ToAccountKeysData() method can return different types of data structures depending on whether certain optional fields are present (see AccountKeysRequestModel.cs lines 19-48). For V2 encryption with Key Connector, we should be explicitly validating that ALL required V2 fields are present:
- PublicKeyEncryptionKeyPair
- SignatureKeyPair
- SecurityState
Without this validation, if a client sends only partial V2 data, the call to SetV2AccountCryptographicStateAsync at line 297 of UserRepository.cs will throw an ArgumentException saying "Provided account keys data is not valid V2 encryption data", but this happens after we've already called SetKeyConnectorUserKey.
Recommendation: Add explicit validation that all V2 encryption fields are present before making any repository calls:
if (requestModel.AccountKeys.PublicKeyEncryptionKeyPair == null ||
requestModel.AccountKeys.SignatureKeyPair == null ||
requestModel.AccountKeys.SecurityState == null)
{
throw new BadRequestException("Complete V2 encryption data must be provided for Key Connector registration");
}
| [Required] | ||
| public required string OrgIdentifier { get; init; } | ||
|
|
||
| public IEnumerable<ValidationResult> Validate(ValidationContext validationContext) |
There was a problem hiding this comment.
Missing Validation for V2 Requests
The Validate method returns early for V2 requests (line 35-38) without performing any validation. This means V2 requests bypass all validation logic, relying solely on:
- The
[EncryptedString]attribute onKeyConnectorKeyWrappedUserKey(line 26) - The
[Required]attribute onOrgIdentifier(line 30)
However, this doesn't validate that AccountKeys is not null or that it contains all required V2 encryption fields (PublicKeyEncryptionKeyPair, SignatureKeyPair, SecurityState).
Security Risk: A client could send a V2 request with:
- Valid
KeyConnectorKeyWrappedUserKey - Valid
OrgIdentifier AccountKeys = new AccountKeysRequestModel()with only minimal fields
This would pass validation here but fail later in the repository layer, potentially after partial database updates.
Recommendation: Add validation for V2 requests:
if (IsV2Request())
{
// V2 registration - validate all required fields are present
if (AccountKeys!.PublicKeyEncryptionKeyPair == null)
{
yield return new ValidationResult("PublicKeyEncryptionKeyPair is required for V2 encryption.");
}
if (AccountKeys!.SignatureKeyPair == null)
{
yield return new ValidationResult("SignatureKeyPair is required for V2 encryption.");
}
if (AccountKeys!.SecurityState == null)
{
yield return new ValidationResult("SecurityState is required for V2 encryption.");
}
yield break;
}
| return result.SingleOrDefault(); | ||
| } | ||
|
|
||
| public UpdateUserData SetKeyConnectorUserKey(Guid userId, string keyConnectorWrappedUserKey) |
There was a problem hiding this comment.
Security Concern: Transaction Safety Issue
The SetKeyConnectorUserKey method returns an UpdateUserData delegate that will be executed within a transaction by SetV2AccountCryptographicStateAsync. However, there's no validation that the user exists before returning the delegate.
Problem: If the user doesn't exist, the stored procedure User_UpdateKeyConnectorUserKey will execute successfully (with 0 rows affected) but won't raise an error. This means:
- The Key Connector key update silently fails
- But
SetV2AccountCryptographicStateAsynccontinues and updates account keys - The user ends up in an inconsistent state with V2 account keys but without the Key Connector key
Recommendation: The stored procedure should validate that exactly 1 row was affected:
DECLARE @RowsAffected INT
UPDATE [dbo].[User]
SET ...
WHERE [Id] = @Id
SET @RowsAffected = @@ROWCOUNT
IF @RowsAffected = 0
THROW 50000, 'User not found', 1Or alternatively, add a check in the delegate:
var result = await connection!.ExecuteAsync(...);
if (result == 0)
{
throw new ArgumentException("User not found", nameof(userId));
}
| using var scope = ServiceScopeFactory.CreateScope(); | ||
| var dbContext = GetDatabaseContext(scope); | ||
|
|
||
| var userEntity = await dbContext.Users.FindAsync(userId); |
There was a problem hiding this comment.
Inconsistency: User Not Found Handling
The Entity Framework implementation throws an ArgumentException if the user is not found (line 463), but the Dapper implementation does not have this check and would silently succeed with 0 rows updated.
This inconsistency means:
- EF Implementation: Will fail early with a clear error
- Dapper Implementation: Will silently fail, leaving the user in an inconsistent state
Recommendation: Ensure both implementations handle the "user not found" case consistently by adding validation to the Dapper version as well (see comment on Dapper implementation).
| BEGIN | ||
| SET NOCOUNT ON | ||
|
|
||
| UPDATE |
There was a problem hiding this comment.
Security: Missing Row Count Validation
The stored procedure updates the user record but doesn't verify that a row was actually updated. If the user doesn't exist (wrong @Id), the UPDATE succeeds with 0 rows affected, and the procedure returns successfully.
This can lead to silent failures where the Key Connector key is not set, but the calling code assumes it was successful and proceeds with other operations, leaving the user in an inconsistent state.
Recommendation: Add validation that exactly one row was affected:
UPDATE [dbo].[User]
SET ...
WHERE [Id] = @Id
IF @@ROWCOUNT = 0
THROW 50000, 'User not found for Key Connector key update', 1;
|
|
||
| _canUseKeyConnectorQuery.VerifyCanUseKeyConnector(user); | ||
|
|
||
| var setKeyConnectorUserKeyTask = |
There was a problem hiding this comment.
Potential Race Condition: Multiple Simultaneous Calls
If a client makes multiple simultaneous calls to this endpoint (which could happen due to network issues or client bugs), there's no protection against race conditions. The VerifyCanUseKeyConnector check at line 43 happens outside the transaction, so two concurrent requests could both pass the check.
While SetV2AccountCryptographicStateAsync uses a transaction, the validation user.UsesKeyConnector in CanUseKeyConnectorQuery (line 20) happens before the transaction starts. This means:
- Request A checks
user.UsesKeyConnector→ false ✓ - Request B checks
user.UsesKeyConnector→ false ✓ - Both proceed and one overwrites the other
Recommendation: Consider adding optimistic concurrency control or checking UsesKeyConnector within the transaction. Alternatively, document that this is expected behavior (last write wins) if that's acceptable.
|
|
||
| var result = await _userService.SetKeyConnectorKeyAsync(model.ToUser(user), model.Key, model.OrgIdentifier); | ||
| if (result.Succeeded) | ||
| if (model.IsV2Request()) |
There was a problem hiding this comment.
Missing Error Handling for V2 Path
The V2 request path (lines 154-158) doesn't have any error handling or result checking, unlike the V1 path which checks result.Succeeded and adds model errors.
If SetKeyConnectorKeyForUserAsync completes without throwing an exception, the method returns successfully. However, if the command doesn't throw but also doesn't fully succeed (which shouldn't happen with current implementation, but could with future changes), there's no feedback to the client.
Recommendation for Consistency: While the current implementation is correct (the command throws exceptions on failure), consider adding explicit success tracking for consistency with other endpoints and to make the contract clearer:
if (model.IsV2Request())
{
// V2 account registration
await _setKeyConnectorKeyCommand.SetKeyConnectorKeyForUserAsync(user, model);
return; // Explicitly return on success
}The current code is functionally correct, but the explicit return makes it clearer that this is the success path.
| await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, requestModel.AccountKeys.ToAccountKeysData(), | ||
| [setKeyConnectorUserKeyTask]); | ||
|
|
||
| await _eventService.LogUserEventAsync(user.Id, EventType.User_MigratedKeyToKeyConnector); |
There was a problem hiding this comment.
Event Logging Timing: Log Before Full Success
The event User_MigratedKeyToKeyConnector is logged at line 51, but AcceptOrgUserByOrgSsoIdAsync is called at line 53. If the organization user acceptance fails (line 53), the event will have already been logged, creating an inconsistent audit trail.
Risk: The audit log will show the user migrated to Key Connector, but they won't actually be accepted into the organization. This could cause confusion during security audits.
Recommendation: Move the event logging to after all operations complete successfully:
await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, requestModel.AccountKeys.ToAccountKeysData(),
[setKeyConnectorUserKeyTask]);
await _acceptOrgUserCommand.AcceptOrgUserByOrgSsoIdAsync(requestModel.OrgIdentifier, user, _userService);
// Log event only after all operations succeed
await _eventService.LogUserEventAsync(user.Id, EventType.User_MigratedKeyToKeyConnector);
| throw new BadRequestException("Already uses Key Connector."); | ||
| } | ||
|
|
||
| if (_currentContext.Organizations.Any(u => |
There was a problem hiding this comment.
Security: Missing Verification of Organization SSO Configuration
The validation checks if the user is an admin/owner of ANY organization, but it doesn't verify:
- That the organization identified by
OrgIdentifier(passed in the request) exists - That the organization has Key Connector enabled/configured
- That the user is actually invited to or a member of that specific organization
This means a user could potentially call this endpoint with any OrgIdentifier, and as long as they're not an admin/owner of other orgs and don't already use Key Connector, the validation passes.
Recommendation: Add validation that:
- The organization exists and has Key Connector configured
- The user has a valid invitation or membership to that specific organization
This validation might belong in the command itself rather than this query, but it should exist somewhere before the key is set.
| BEGIN | ||
| SET NOCOUNT ON | ||
|
|
||
| UPDATE |
There was a problem hiding this comment.
Security: Missing Row Count Validation (Migration Script)
This migration script has the same issue as the stored procedure definition. The UPDATE statement doesn't verify that a row was actually updated. If the user doesn't exist, the procedure completes successfully with 0 rows affected.
Recommendation: Add row count validation:
UPDATE [dbo].[User]
SET ...
WHERE [Id] = @Id
IF @@ROWCOUNT = 0
THROW 50000, 'User not found for Key Connector key update', 1;This should match the fix applied to the stored procedure in /src/Sql/dbo/KeyManagement/Stored Procedures/User_UpdateKeyConnectorUserKey.sql.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27280
📔 Objective
Support v2 encryption on key-connector signups.
Changes in
POST /accounts/key-management/set-key-connector-keycontroller:keyConnectorKeyWrappedUserKeyandAccountKeysrequest fields.enable-account-encryption-v2-key-connector-registration📸 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