-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-27280] Support v2 encryption on key-connector signups #6712
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 10 commits
bfb856b
b58e3ec
9fa43f0
e424f19
daee25d
51b533f
99e94a6
9325ac8
0c207fb
caa0e2a
2fc9629
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ | |
| using Bit.Core.Entities; | ||
| using Bit.Core.Exceptions; | ||
| using Bit.Core.KeyManagement.Commands.Interfaces; | ||
| using Bit.Core.KeyManagement.Models.Api.Request; | ||
| using Bit.Core.KeyManagement.Models.Data; | ||
| using Bit.Core.KeyManagement.Queries.Interfaces; | ||
| using Bit.Core.KeyManagement.UserKey; | ||
|
|
@@ -47,6 +48,7 @@ private readonly IRotationValidator<IEnumerable<WebAuthnLoginRotateKeyRequestMod | |
| _webauthnKeyValidator; | ||
| private readonly IRotationValidator<IEnumerable<OtherDeviceKeysUpdateRequestModel>, IEnumerable<Device>> _deviceValidator; | ||
| private readonly IKeyConnectorConfirmationDetailsQuery _keyConnectorConfirmationDetailsQuery; | ||
| private readonly ISetKeyConnectorKeyCommand _setKeyConnectorKeyCommand; | ||
|
|
||
| public AccountsKeyManagementController(IUserService userService, | ||
| IFeatureService featureService, | ||
|
|
@@ -62,8 +64,10 @@ public AccountsKeyManagementController(IUserService userService, | |
| emergencyAccessValidator, | ||
| IRotationValidator<IEnumerable<ResetPasswordWithOrgIdRequestModel>, IReadOnlyList<OrganizationUser>> | ||
| organizationUserValidator, | ||
| IRotationValidator<IEnumerable<WebAuthnLoginRotateKeyRequestModel>, IEnumerable<WebAuthnLoginRotateKeyData>> webAuthnKeyValidator, | ||
| IRotationValidator<IEnumerable<OtherDeviceKeysUpdateRequestModel>, IEnumerable<Device>> deviceValidator) | ||
| IRotationValidator<IEnumerable<WebAuthnLoginRotateKeyRequestModel>, IEnumerable<WebAuthnLoginRotateKeyData>> | ||
| webAuthnKeyValidator, | ||
| IRotationValidator<IEnumerable<OtherDeviceKeysUpdateRequestModel>, IEnumerable<Device>> deviceValidator, | ||
| ISetKeyConnectorKeyCommand setKeyConnectorKeyCommand) | ||
| { | ||
| _userService = userService; | ||
| _featureService = featureService; | ||
|
|
@@ -79,6 +83,7 @@ public AccountsKeyManagementController(IUserService userService, | |
| _webauthnKeyValidator = webAuthnKeyValidator; | ||
| _deviceValidator = deviceValidator; | ||
| _keyConnectorConfirmationDetailsQuery = keyConnectorConfirmationDetailsQuery; | ||
| _setKeyConnectorKeyCommand = setKeyConnectorKeyCommand; | ||
| } | ||
|
|
||
| [HttpPost("key-management/regenerate-keys")] | ||
|
|
@@ -146,18 +151,28 @@ public async Task PostSetKeyConnectorKeyAsync([FromBody] SetKeyConnectorKeyReque | |
| throw new UnauthorizedAccessException(); | ||
| } | ||
|
|
||
| var result = await _userService.SetKeyConnectorKeyAsync(model.ToUser(user), model.Key, model.OrgIdentifier); | ||
| if (result.Succeeded) | ||
| if (model.IsV2Request()) | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 If 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. |
||
| { | ||
| return; | ||
| // V2 account registration | ||
| await _setKeyConnectorKeyCommand.SetKeyConnectorKeyForUserAsync(user, model); | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| foreach (var error in result.Errors) | ||
| else | ||
| { | ||
| ModelState.AddModelError(string.Empty, error.Description); | ||
| // V1 account registration | ||
| // TODO removed with https://bitwarden.atlassian.net/browse/PM-27328 | ||
| var result = await _userService.SetKeyConnectorKeyAsync(model.ToUser(user), model.Key, model.OrgIdentifier); | ||
| if (result.Succeeded) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| foreach (var error in result.Errors) | ||
| { | ||
| ModelState.AddModelError(string.Empty, error.Description); | ||
| } | ||
|
|
||
| throw new BadRequestException(ModelState); | ||
| } | ||
|
|
||
| throw new BadRequestException(ModelState); | ||
| } | ||
|
|
||
| [HttpPost("convert-to-key-connector")] | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Typically we don't use API request models as input to commands which is why I think we moved this? We do this to enforce better separation of concerns for request models, data models, and entities. Sometimes we have to put API request models that need to be shared between multiple web projects like API and Identity in For good separation of concerns in these kinds of cases we would make an data domain model in In our controller we would have something like Looking at the command I would expect something like this for a data model. This allows us to not expose things like deprecated JSON properties and request validation to our Core and commands. It's very DTO pattern like, but we use different names. |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
| using Bit.Core.KeyManagement.Models.Api.Request; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Commands.Interfaces; | ||
|
|
||
| /// <summary> | ||
| /// Creates the user key and account cryptographic state for a new user registering | ||
| /// with Key Connector SSO configuration. | ||
| /// </summary> | ||
| public interface ISetKeyConnectorKeyCommand | ||
| { | ||
| Task SetKeyConnectorKeyForUserAsync(User user, SetKeyConnectorKeyRequestModel requestModel); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,55 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Exceptions; | ||
| using Bit.Core.KeyManagement.Commands.Interfaces; | ||
| using Bit.Core.KeyManagement.Models.Api.Request; | ||
| using Bit.Core.KeyManagement.Queries.Interfaces; | ||
| using Bit.Core.OrganizationFeatures.OrganizationUsers.Interfaces; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Services; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Commands; | ||
|
|
||
| public class SetKeyConnectorKeyCommand : ISetKeyConnectorKeyCommand | ||
| { | ||
| private readonly ICanUseKeyConnectorQuery _canUseKeyConnectorQuery; | ||
| private readonly IEventService _eventService; | ||
| private readonly IAcceptOrgUserCommand _acceptOrgUserCommand; | ||
| private readonly IUserService _userService; | ||
| private readonly IUserRepository _userRepository; | ||
|
|
||
| public SetKeyConnectorKeyCommand( | ||
| ICanUseKeyConnectorQuery canUseKeyConnectorQuery, | ||
| IEventService eventService, | ||
| IAcceptOrgUserCommand acceptOrgUserCommand, | ||
| IUserService userService, | ||
| IUserRepository userRepository) | ||
| { | ||
| _canUseKeyConnectorQuery = canUseKeyConnectorQuery; | ||
| _eventService = eventService; | ||
| _acceptOrgUserCommand = acceptOrgUserCommand; | ||
| _userService = userService; | ||
| _userRepository = userRepository; | ||
| } | ||
|
|
||
| public async Task SetKeyConnectorKeyForUserAsync(User user, SetKeyConnectorKeyRequestModel requestModel) | ||
| { | ||
| // TODO remove validation with https://bitwarden.atlassian.net/browse/PM-27280 | ||
mzieniukbw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
mzieniukbw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
mzieniukbw marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if (string.IsNullOrEmpty(requestModel.KeyConnectorKeyWrappedUserKey) || requestModel.AccountKeys == null) | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| throw new BadRequestException("KeyConnectorKeyWrappedUserKey and AccountKeys must be provided"); | ||
| } | ||
|
|
||
| _canUseKeyConnectorQuery.VerifyCanUseKeyConnector(user); | ||
|
|
||
| var setKeyConnectorUserKeyTask = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 While
Recommendation: Consider adding optimistic concurrency control or checking |
||
| _userRepository.SetKeyConnectorUserKey(user.Id, requestModel.KeyConnectorKeyWrappedUserKey); | ||
|
|
||
| await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, requestModel.AccountKeys.ToAccountKeysData(), | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Concern: Null Reference Exception Risk The code calls More importantly, the
Without this validation, if a client sends only partial V2 data, the call to 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");
} |
||
| [setKeyConnectorUserKeyTask]); | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| await _eventService.LogUserEventAsync(user.Id, EventType.User_MigratedKeyToKeyConnector); | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Event Logging Timing: Log Before Full Success The event 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); |
||
|
|
||
| await _acceptOrgUserCommand.AcceptOrgUserByOrgSsoIdAsync(requestModel.OrgIdentifier, user, _userService); | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| ๏ปฟusing System.ComponentModel.DataAnnotations; | ||
| using Bit.Core.Auth.Models.Api.Request.Accounts; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Utilities; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Models.Api.Request; | ||
|
|
||
| public class SetKeyConnectorKeyRequestModel : IValidatableObject | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would recommend moving this back to API as discussed above. |
||
| { | ||
| // TODO will be removed with https://bitwarden.atlassian.net/browse/PM-27328 | ||
| [Obsolete("Use KeyConnectorKeyWrappedUserKey instead")] | ||
| public string? Key { get; set; } | ||
|
|
||
| [Obsolete("Use AccountKeys instead")] | ||
| public KeysRequestModel? Keys { get; set; } | ||
| [Obsolete("Not used anymore")] | ||
| public KdfType? Kdf { get; set; } | ||
| [Obsolete("Not used anymore")] | ||
| public int? KdfIterations { get; set; } | ||
| [Obsolete("Not used anymore")] | ||
| public int? KdfMemory { get; set; } | ||
| [Obsolete("Not used anymore")] | ||
| public int? KdfParallelism { get; set; } | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| [EncryptedString] | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| public string? KeyConnectorKeyWrappedUserKey { get; set; } | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| public AccountKeysRequestModel? AccountKeys { get; set; } | ||
|
|
||
| [Required] | ||
| public required string OrgIdentifier { get; init; } | ||
|
|
||
| public IEnumerable<ValidationResult> Validate(ValidationContext validationContext) | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing Validation for V2 Requests The
However, this doesn't validate that Security Risk: A client could send a V2 request with:
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;
} |
||
| { | ||
| if (IsV2Request()) | ||
| { | ||
| // V2 registration | ||
| yield break; | ||
| } | ||
|
|
||
| // V1 registration | ||
| // TODO removed with https://bitwarden.atlassian.net/browse/PM-27328 | ||
| if (string.IsNullOrEmpty(Key)) | ||
| { | ||
| yield return new ValidationResult("Key must be supplied."); | ||
| } | ||
|
|
||
| if (Keys == null) | ||
| { | ||
| yield return new ValidationResult("Keys must be supplied."); | ||
| } | ||
|
|
||
| if (Kdf == null) | ||
| { | ||
| yield return new ValidationResult("Kdf must be supplied."); | ||
| } | ||
|
|
||
| if (KdfIterations == null) | ||
| { | ||
| yield return new ValidationResult("KdfIterations must be supplied."); | ||
| } | ||
|
|
||
| if (Kdf == KdfType.Argon2id) | ||
| { | ||
| if (KdfMemory == null) | ||
| { | ||
| yield return new ValidationResult("KdfMemory must be supplied when Kdf is Argon2id."); | ||
| } | ||
|
|
||
| if (KdfParallelism == null) | ||
| { | ||
| yield return new ValidationResult("KdfParallelism must be supplied when Kdf is Argon2id."); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public bool IsV2Request() | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| return !string.IsNullOrEmpty(KeyConnectorKeyWrappedUserKey) && AccountKeys != null; | ||
| } | ||
|
|
||
| // TODO removed with https://bitwarden.atlassian.net/browse/PM-27328 | ||
| public User ToUser(User existingUser) | ||
| { | ||
| existingUser.Kdf = Kdf!.Value; | ||
| existingUser.KdfIterations = KdfIterations!.Value; | ||
| existingUser.KdfMemory = KdfMemory; | ||
| existingUser.KdfParallelism = KdfParallelism; | ||
| existingUser.Key = Key; | ||
| Keys!.ToUser(existingUser); | ||
| return existingUser; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| ๏ปฟusing Bit.Core.Context; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Exceptions; | ||
| using Bit.Core.KeyManagement.Queries.Interfaces; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Queries; | ||
|
|
||
| public class CanUseKeyConnectorQuery : ICanUseKeyConnectorQuery | ||
| { | ||
| private readonly ICurrentContext _currentContext; | ||
|
|
||
| public CanUseKeyConnectorQuery(ICurrentContext currentContext) | ||
| { | ||
| _currentContext = currentContext; | ||
| } | ||
|
|
||
| public void VerifyCanUseKeyConnector(User user) | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ญ Perhaps this would be better as an AuthorizationHandler on the controller. It is odd for a query not to return any data. Happy to help if you would like to see what that would look like. |
||
| { | ||
| if (user.UsesKeyConnector) | ||
| { | ||
| throw new BadRequestException("Already uses Key Connector."); | ||
| } | ||
|
|
||
| if (_currentContext.Organizations.Any(u => | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
This means a user could potentially call this endpoint with any Recommendation: Add validation that:
This validation might belong in the command itself rather than this query, but it should exist somewhere before the key is set. |
||
| u.Type is OrganizationUserType.Owner or OrganizationUserType.Admin)) | ||
| { | ||
| throw new BadRequestException("Cannot use Key Connector when admin or owner of an organization."); | ||
mzieniukbw marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| ๏ปฟusing Bit.Core.Entities; | ||
|
|
||
| namespace Bit.Core.KeyManagement.Queries.Interfaces; | ||
|
|
||
| /// <summary> | ||
| /// Query to verify if the user can use the key connector | ||
| /// </summary> | ||
| public interface ICanUseKeyConnectorQuery | ||
| { | ||
| /// <summary> | ||
| /// Throws an exception if the user cannot use the key connector | ||
| /// </summary> | ||
| /// <param name="user">User to validate</param> | ||
| void VerifyCanUseKeyConnector(User user); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.