From eb43f28c954ca6f5b039a2f7cd6e6dd433cdd42a Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Fri, 7 Aug 2026 21:06:51 +0900 Subject: [PATCH 1/2] feat: wire-in key-id setting --- .../Auth/Controllers/AccountsController.cs | 10 +- .../AccountsKeyManagementController.cs | 21 ++++- .../SetKeyConnectorKeyRequestModel.cs | 11 ++- .../Requests/SetUserKeyIdRequestModel.cs | 17 ++++ .../Api/Request/Accounts/KeysRequestModel.cs | 7 ++ .../Accounts/RegisterFinishRequestModel.cs | 3 + ...ishSsoJitProvisionMasterPasswordCommand.cs | 14 ++- src/Core/Entities/User.cs | 21 ++--- .../Interfaces/ISetUserKeyIdCommand.cs | 22 +++++ .../Commands/SetKeyConnectorKeyCommand.cs | 13 ++- .../Commands/SetUserKeyIdCommand.cs | 28 ++++++ ...eyManagementServiceCollectionExtensions.cs | 1 + .../Models/Data/KeyConnectorKeysData.cs | 4 + .../Models/Data/RegisterFinishData.cs | 8 +- .../RotateUserAccountKeysCommand.cs | 1 + .../Services/Implementations/UserService.cs | 12 ++- .../AccountsKeyManagementControllerTests.cs | 47 ++++++++++ .../Controllers/AccountsControllerTests.cs | 56 +++++++++++- .../AccountsKeyManagementControllerTests.cs | 27 ++++++ test/Common/AutoFixture/KeyIdFixtures.cs | 2 +- .../RegisterFinishRequestModelTests.cs | 67 ++++++++++++++ ...oJitProvisionMasterPasswordCommandTests.cs | 53 ++++++++++- test/Core.Test/Entities/UserTests.cs | 34 +++++++ .../SetKeyConnectorKeyCommandTests.cs | 60 +++++++++++- .../Commands/SetUserKeyIdCommandTests.cs | 60 ++++++++++++ .../RotateUserAccountKeysCommandTests.cs | 91 +++++++++++++++++++ 26 files changed, 660 insertions(+), 30 deletions(-) create mode 100644 src/Api/KeyManagement/Models/Requests/SetUserKeyIdRequestModel.cs create mode 100644 src/Core/KeyManagement/Commands/Interfaces/ISetUserKeyIdCommand.cs create mode 100644 src/Core/KeyManagement/Commands/SetUserKeyIdCommand.cs create mode 100644 test/Core.Test/KeyManagement/Commands/SetUserKeyIdCommandTests.cs diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index a736cb671ed8..010dcd644ebe 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -526,7 +526,15 @@ public async Task PostKeys([FromBody] KeysRequestModel model) { throw new BadRequestException("AccountKeys are only supported for V2 encryption."); } - await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, accountKeysData); + // A client that predates the key id field sends none. The account then picks one up from + // the backfill endpoint on a later sync rather than here. + var userKeyId = KeyId.FromHexEncodedString(model.UserKeyId); + var updateUserDataTasks = userKeyId == null + ? null + : new UpdateUserData[] { _userRepository.SetUserKeyId(user.Id, userKeyId) }; + + await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, accountKeysData, + updateUserDataTasks); return new KeysResponseModel(accountKeysData, user.Key); } else diff --git a/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs b/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs index 542d3aafe4e1..5ad748e36cbe 100644 --- a/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs +++ b/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs @@ -51,6 +51,7 @@ private readonly IRotationValidator, IEnumerable> deviceValidator, ISetKeyConnectorKeyCommand setKeyConnectorKeyCommand, - IConvertUserToKeyConnectorCommand convertUserToKeyConnectorCommand) + IConvertUserToKeyConnectorCommand convertUserToKeyConnectorCommand, + ISetUserKeyIdCommand setUserKeyIdCommand) { _userService = userService; _regenerateUserAsymmetricKeysCommand = regenerateUserAsymmetricKeysCommand; @@ -88,6 +90,21 @@ public AccountsKeyManagementController(IUserService userService, _keyRotationDataQuery = keyRotationDataQuery; _setKeyConnectorKeyCommand = setKeyConnectorKeyCommand; _convertUserToKeyConnectorCommand = convertUserToKeyConnectorCommand; + _setUserKeyIdCommand = setUserKeyIdCommand; + } + + /// + /// Reports the key id of the caller's current user key to the server. + /// + /// + /// This is meant for backfilling the user-key id for existing users for whom + /// the key id is not yet recorded. + /// + [HttpPost("key-management/user-key-id")] + public async Task PostUserKeyIdAsync([FromBody] SetUserKeyIdRequestModel request) + { + var user = await _userService.GetUserByPrincipalAsync(User) ?? throw new UnauthorizedAccessException(); + await _setUserKeyIdCommand.SetUserKeyIdAsync(user, request.ToKeyId()); } [HttpPost("key-management/regenerate-keys")] @@ -281,7 +298,7 @@ await _organizationUserValidator.ValidateAsync(user, Ciphers = await _cipherValidator.ValidateAsync(user, request.AccountData.Ciphers), Folders = await _folderValidator.ValidateAsync(user, request.AccountData.Folders), Sends = await _sendValidator.ValidateAsync(user, request.AccountData.Sends), - NewUserKeyId = request.NewUserKeyId != null ? KeyId.FromHexEncodedString(request.NewUserKeyId) : null + NewUserKeyId = KeyId.FromHexEncodedString(request.NewUserKeyId) }; } } diff --git a/src/Api/KeyManagement/Models/Requests/SetKeyConnectorKeyRequestModel.cs b/src/Api/KeyManagement/Models/Requests/SetKeyConnectorKeyRequestModel.cs index 6cd13fdf835f..3108707f7a95 100644 --- a/src/Api/KeyManagement/Models/Requests/SetKeyConnectorKeyRequestModel.cs +++ b/src/Api/KeyManagement/Models/Requests/SetKeyConnectorKeyRequestModel.cs @@ -30,6 +30,14 @@ public class SetKeyConnectorKeyRequestModel : IValidatableObject public string? KeyConnectorKeyWrappedUserKey { get; set; } public AccountKeysRequestModel? AccountKeys { get; set; } + /// + /// Key id of the user key wrapped by , when the client + /// supplied it. Absent for clients that predate the field, so it is deliberately not part of + /// . + /// + [KeyId] + public string? ContainedKeyId { get; init; } + [Required] public required string OrgIdentifier { get; init; } @@ -106,7 +114,8 @@ public KeyConnectorKeysData ToKeyConnectorKeysData() { KeyConnectorKeyWrappedUserKey = KeyConnectorKeyWrappedUserKey, AccountKeys = AccountKeys, - OrgIdentifier = OrgIdentifier + OrgIdentifier = OrgIdentifier, + ContainedKeyId = KeyId.FromHexEncodedString(ContainedKeyId) }; } } diff --git a/src/Api/KeyManagement/Models/Requests/SetUserKeyIdRequestModel.cs b/src/Api/KeyManagement/Models/Requests/SetUserKeyIdRequestModel.cs new file mode 100644 index 000000000000..2279d2dbf4c7 --- /dev/null +++ b/src/Api/KeyManagement/Models/Requests/SetUserKeyIdRequestModel.cs @@ -0,0 +1,17 @@ +using System.ComponentModel.DataAnnotations; +using Bit.Core.KeyManagement.Models.Data; +using Bit.Core.Utilities; + +namespace Bit.Api.KeyManagement.Models.Requests; + +public class SetUserKeyIdRequestModel +{ + /// + /// Hex-encoded key id of the user's current user key. + /// + [Required(AllowEmptyStrings = false)] + [KeyId] + public required string UserKeyId { get; init; } + + public KeyId ToKeyId() => KeyId.FromHexEncodedString(UserKeyId)!; +} diff --git a/src/Core/Auth/Models/Api/Request/Accounts/KeysRequestModel.cs b/src/Core/Auth/Models/Api/Request/Accounts/KeysRequestModel.cs index 85ddef44ce45..eedea4614215 100644 --- a/src/Core/Auth/Models/Api/Request/Accounts/KeysRequestModel.cs +++ b/src/Core/Auth/Models/Api/Request/Accounts/KeysRequestModel.cs @@ -18,6 +18,13 @@ public class KeysRequestModel public string EncryptedPrivateKey { get; set; } public AccountKeysRequestModel AccountKeys { get; set; } + /// + /// Key id of the user key these account keys belong to, when the client supplied it. Absent for + /// clients that predate the field. Only honored on the V2 path. + /// + [KeyId] + public string UserKeyId { get; set; } + [Obsolete("Use SetAccountKeysForUserCommand instead")] public User ToUser(User existingUser) { diff --git a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs index 56ac6a19868c..4d89b893fb49 100644 --- a/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs +++ b/src/Core/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModel.cs @@ -89,6 +89,8 @@ public User ToUser(bool isV2Encryption) KdfParallelism = MasterPasswordUnlock?.Kdf.Parallelism ?? KdfParallelism, MasterPasswordSalt = MasterPasswordUnlock?.Salt, Key = MasterPasswordUnlock?.MasterKeyWrappedUserKey ?? UserSymmetricKey + // Note: V1 register flows do not set the UserKeyId; those accounts report it later + // through the backfill endpoint. }; user = UserAsymmetricKeys?.ToUser(user)!; @@ -132,6 +134,7 @@ public RegisterFinishData ToData() MasterKeyWrappedUserKey = unlockData?.MasterKeyWrappedUserKey ?? UserSymmetricKey ?? throw new BadRequestException("MasterKeyWrappedUserKey couldn't be found on either the MasterPasswordUnlockData or the UserSymmetricKey property passed in."), MasterPasswordAuthenticationHash = authenticationData?.MasterPasswordAuthenticationHash ?? MasterPasswordHash ?? throw new BadRequestException("MasterPasswordHash couldn't be found on either the MasterPasswordAuthenticationData or the MasterPasswordHash property passed in."), Salt = unlockData?.Salt, + UserKeyId = unlockData?.ContainedKeyId, }; } diff --git a/src/Core/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommand.cs b/src/Core/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommand.cs index 90998ae1d0d0..b790d134c52c 100644 --- a/src/Core/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommand.cs +++ b/src/Core/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommand.cs @@ -62,13 +62,21 @@ public async Task FinishProvisionAsync(User user, throw new BadRequestException("User not found within organization."); } - var updateUserData = + var updateUserDataTasks = new List + { _masterPasswordService.BuildUpdateUserDelegateSetInitialMasterPassword( user, - masterPasswordDataModel.ToSetInitialPasswordData()); + masterPasswordDataModel.ToSetInitialPasswordData()) + }; + + var containedKeyId = masterPasswordDataModel.MasterPasswordUnlock.ContainedKeyId; + if (containedKeyId is not null) + { + updateUserDataTasks.Add(_userRepository.SetUserKeyId(user.Id, containedKeyId)); + } await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, masterPasswordDataModel.AccountKeys, - [updateUserData]); + updateUserDataTasks); await _eventService.LogUserEventAsync(user.Id, EventType.User_ChangedPassword); diff --git a/src/Core/Entities/User.cs b/src/Core/Entities/User.cs index b50fe852f27f..3c05d36f90e8 100644 --- a/src/Core/Entities/User.cs +++ b/src/Core/Entities/User.cs @@ -114,18 +114,6 @@ public class User : ITableObject, IStorableSubscriber, IRevisable, ITwoFac public string? V2UpgradeToken { get; set; } [MaxLength(256)] public string? MasterPasswordSalt { get; set; } - - public KeyId? GetUserKeyId() - { - // Todo: Database Implementation in follow-up PR - return null; - } - - public void SetUserKeyId(KeyId keyId) - { - return; // Todo: Database Implementation in follow-up PR - } - public DateTime? LastApiKeyRotationDate { get; set; } /// /// A hex-endcoded key-id of the user's current user-key. @@ -137,8 +125,17 @@ public void SetUserKeyId(KeyId keyId) /// A key rotation will set a new key id. Account registrations will carry a key id. /// [MaxLength(32)] + [KeyId] public string? UserKeyId { get; set; } + public void SetUserKeyId(KeyId? userKeyId) + { + UserKeyId = userKeyId?.ToString(); + } + + public KeyId? GetUserKeyId() => + KeyId.FromHexEncodedString(string.IsNullOrEmpty(UserKeyId) ? null : UserKeyId); + public string GetMasterPasswordSalt() { return MasterPasswordSalt ?? Email.ToLowerInvariant().Trim(); diff --git a/src/Core/KeyManagement/Commands/Interfaces/ISetUserKeyIdCommand.cs b/src/Core/KeyManagement/Commands/Interfaces/ISetUserKeyIdCommand.cs new file mode 100644 index 000000000000..7a9c7e3eb3b3 --- /dev/null +++ b/src/Core/KeyManagement/Commands/Interfaces/ISetUserKeyIdCommand.cs @@ -0,0 +1,22 @@ +using Bit.Core.Entities; +using Bit.Core.KeyManagement.Models.Data; + +namespace Bit.Core.KeyManagement.Commands.Interfaces; + +public interface ISetUserKeyIdCommand +{ + /// + /// Stores the key id of a user's current user key. + /// + /// + /// This is a backfill primitive for accounts that pre-date the key id being reported alongside + /// key material. It therefore only accepts a value when the account does not already have one — + /// changing an existing key id must happen through a key rotation. + /// + /// The user whose key id is being recorded. + /// Key id of the user's current user key. + /// + /// Thrown when the account already has a key id. + /// + Task SetUserKeyIdAsync(User user, KeyId userKeyId); +} diff --git a/src/Core/KeyManagement/Commands/SetKeyConnectorKeyCommand.cs b/src/Core/KeyManagement/Commands/SetKeyConnectorKeyCommand.cs index a96042de30f6..c94b853a83b1 100644 --- a/src/Core/KeyManagement/Commands/SetKeyConnectorKeyCommand.cs +++ b/src/Core/KeyManagement/Commands/SetKeyConnectorKeyCommand.cs @@ -46,11 +46,18 @@ public async Task SetKeyConnectorKeyForUserAsync(User user, KeyConnectorKeysData throw new BadRequestException("Cannot use Key Connector"); } - var setKeyConnectorUserKeyTask = - _userRepository.SetKeyConnectorUserKey(user.Id, keyConnectorKeysData.KeyConnectorKeyWrappedUserKey); + var updateUserDataTasks = new List + { + _userRepository.SetKeyConnectorUserKey(user.Id, keyConnectorKeysData.KeyConnectorKeyWrappedUserKey) + }; + + if (keyConnectorKeysData.ContainedKeyId is not null) + { + updateUserDataTasks.Add(_userRepository.SetUserKeyId(user.Id, keyConnectorKeysData.ContainedKeyId)); + } await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, - keyConnectorKeysData.AccountKeys.ToAccountKeysData(), [setKeyConnectorUserKeyTask]); + keyConnectorKeysData.AccountKeys.ToAccountKeysData(), updateUserDataTasks); await _eventService.LogUserEventAsync(user.Id, EventType.User_MigratedKeyToKeyConnector); diff --git a/src/Core/KeyManagement/Commands/SetUserKeyIdCommand.cs b/src/Core/KeyManagement/Commands/SetUserKeyIdCommand.cs new file mode 100644 index 000000000000..29282b9f76c1 --- /dev/null +++ b/src/Core/KeyManagement/Commands/SetUserKeyIdCommand.cs @@ -0,0 +1,28 @@ +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.KeyManagement.Commands.Interfaces; +using Bit.Core.KeyManagement.Models.Data; +using Bit.Core.Repositories; + +namespace Bit.Core.KeyManagement.Commands; + +public class SetUserKeyIdCommand : ISetUserKeyIdCommand +{ + private readonly IUserRepository _userRepository; + + public SetUserKeyIdCommand(IUserRepository userRepository) + { + _userRepository = userRepository; + } + + /// + public async Task SetUserKeyIdAsync(User user, KeyId userKeyId) + { + if (user.GetUserKeyId() is not null) + { + throw new BadRequestException("User key id is already set."); + } + + await _userRepository.UpdateUserDataAsync([_userRepository.SetUserKeyId(user.Id, userKeyId)]); + } +} diff --git a/src/Core/KeyManagement/KeyManagementServiceCollectionExtensions.cs b/src/Core/KeyManagement/KeyManagementServiceCollectionExtensions.cs index 6105ea7a4344..cec0bea1a846 100644 --- a/src/Core/KeyManagement/KeyManagementServiceCollectionExtensions.cs +++ b/src/Core/KeyManagement/KeyManagementServiceCollectionExtensions.cs @@ -33,6 +33,7 @@ private static void AddKeyManagementCommands(this IServiceCollection services) services.AddScoped(); services.AddScoped(); services.AddScoped(); + services.AddScoped(); } private static void AddKeyManagementQueries(this IServiceCollection services) diff --git a/src/Core/KeyManagement/Models/Data/KeyConnectorKeysData.cs b/src/Core/KeyManagement/Models/Data/KeyConnectorKeysData.cs index 75a0a81277fa..3a8655da8b88 100644 --- a/src/Core/KeyManagement/Models/Data/KeyConnectorKeysData.cs +++ b/src/Core/KeyManagement/Models/Data/KeyConnectorKeysData.cs @@ -10,5 +10,9 @@ public class KeyConnectorKeysData public required string OrgIdentifier { get; init; } + /// + /// Key id of the user key wrapped by , when the client + /// supplied it. + /// public KeyId? ContainedKeyId { get; init; } } diff --git a/src/Core/KeyManagement/Models/Data/RegisterFinishData.cs b/src/Core/KeyManagement/Models/Data/RegisterFinishData.cs index 3fbf4b6760c6..ba36bbf52462 100644 --- a/src/Core/KeyManagement/Models/Data/RegisterFinishData.cs +++ b/src/Core/KeyManagement/Models/Data/RegisterFinishData.cs @@ -9,6 +9,11 @@ public class RegisterFinishData public required string MasterPasswordAuthenticationHash { get; init; } public string? Salt { get; init; } + /// + /// Key id of the new account's user key, when the client supplied it. + /// + public KeyId? UserKeyId { get; init; } + public bool IsV2Encryption() { return UserAccountKeysData.IsV2Encryption(); @@ -26,11 +31,12 @@ public override bool Equals(object? obj) MasterKeyWrappedUserKey == other.MasterKeyWrappedUserKey && MasterPasswordAuthenticationHash == other.MasterPasswordAuthenticationHash && Salt == other.Salt && + Equals(UserKeyId, other.UserKeyId) && IsV2Encryption() == other.IsV2Encryption(); } public override int GetHashCode() { - return HashCode.Combine(UserAccountKeysData, Kdf, MasterKeyWrappedUserKey, MasterPasswordAuthenticationHash, Salt); + return HashCode.Combine(UserAccountKeysData, Kdf, MasterKeyWrappedUserKey, MasterPasswordAuthenticationHash, Salt, UserKeyId); } } diff --git a/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs b/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs index 3b3cb4351832..5baf936e97fb 100644 --- a/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs +++ b/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountKeysCommand.cs @@ -319,6 +319,7 @@ private async Task BaseRotateUserAccountKeysAsync(BaseRotateUserAccountKey var now = DateTime.UtcNow; user.RevisionDate = user.AccountRevisionDate = now; user.LastKeyRotationDate = now; + user.SetUserKeyId(baseModel.NewUserKeyId); // V2UpgradeToken is only valid for V1 users transitioning to V2. // For V2 users the token is semantically invalid — discard it and perform a full logout. diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index efd430188469..9e8e434237a9 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -326,8 +326,16 @@ public async Task CreateUserAsync(User user, RegisterFinishData var result = await CreateAsync(user, registerFinishData.MasterPasswordAuthenticationHash); if (result.Succeeded) { - var setRegisterFinishUserDataTask = _userRepository.UpdateMasterPasswordUnlockData(user.Id, registerFinishData); - await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, registerFinishData.UserAccountKeysData, [setRegisterFinishUserDataTask]); + var updateUserDataActions = new List + { + _userRepository.UpdateMasterPasswordUnlockData(user.Id, registerFinishData) + }; + if (registerFinishData.UserKeyId is not null) + { + updateUserDataActions.Add(_userRepository.SetUserKeyId(user.Id, registerFinishData.UserKeyId)); + } + + await _userRepository.SetV2AccountCryptographicStateAsync(user.Id, registerFinishData.UserAccountKeysData, updateUserDataActions); } return result; } diff --git a/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs b/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs index 764e3f9fa243..e1711ff49fee 100644 --- a/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs +++ b/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs @@ -82,6 +82,53 @@ public Task DisposeAsync() return Task.CompletedTask; } + [Fact] + public async Task PostUserKeyIdAsync_NotLoggedIn_Unauthorized() + { + var response = await _client.PostAsJsonAsync("/accounts/key-management/user-key-id", + new SetUserKeyIdRequestModel { UserKeyId = _mockKeyId }); + + Assert.Equal(HttpStatusCode.Unauthorized, response.StatusCode); + } + + [Fact] + public async Task PostUserKeyIdAsync_MalformedKeyId_BadRequest() + { + await _loginHelper.LoginAsync(_ownerEmail); + + var response = await _client.PostAsJsonAsync("/accounts/key-management/user-key-id", + new { UserKeyId = "not-a-key-id" }); + + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + } + + [Fact] + public async Task PostUserKeyIdAsync_NoKeyIdRecorded_BackfillsThenRejectsASecondAttempt() + { + await _loginHelper.LoginAsync(_ownerEmail); + var user = await _userRepository.GetByEmailAsync(_ownerEmail); + Assert.NotNull(user); + Assert.Null(user.GetUserKeyId()); + + var response = await _client.PostAsJsonAsync("/accounts/key-management/user-key-id", + new SetUserKeyIdRequestModel { UserKeyId = _mockKeyId }); + response.EnsureSuccessStatusCode(); + + var updatedUser = await _userRepository.GetByEmailAsync(_ownerEmail); + Assert.NotNull(updatedUser); + Assert.Equal(_mockKeyId, updatedUser.UserKeyId); + + // Reporting a key id must not be able to rename the key the account is now known to use. + var secondResponse = await _client.PostAsJsonAsync("/accounts/key-management/user-key-id", + new SetUserKeyIdRequestModel { UserKeyId = "fedcba9876543210fedcba9876543210" }); + + Assert.Equal(HttpStatusCode.BadRequest, secondResponse.StatusCode); + + var unchangedUser = await _userRepository.GetByEmailAsync(_ownerEmail); + Assert.NotNull(unchangedUser); + Assert.Equal(_mockKeyId, unchangedUser.UserKeyId); + } + [Theory] [BitAutoData] public async Task RegenerateKeysAsync_NotLoggedIn_Unauthorized(KeyRegenerationRequestModel request) diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index 875158c09575..095c605731bf 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -981,18 +981,72 @@ public async Task PostKeys_WithAccountKeys_CallsSetV2AccountCryptographicState( _userService.GetUserByPrincipalAsync(Arg.Any()).Returns(user); + var setUserKeyId = Substitute.For(); + _userRepository.SetUserKeyId(user.Id, Arg.Any()).Returns(setUserKeyId); + // Act var result = await _sut.PostKeys(model); // Assert await _userRepository.Received(1).SetV2AccountCryptographicStateAsync( user.Id, - Arg.Any()); + Arg.Any(), + Arg.Is>(actions => + actions != null && actions.Count() == 1 && actions.First() == setUserKeyId)); + _userRepository.Received(1).SetUserKeyId( + user.Id, + Arg.Is(keyId => keyId.ToString() == model.UserKeyId)); await _userService.DidNotReceiveWithAnyArgs().SaveUserAsync(Arg.Any()); Assert.NotNull(result); Assert.Equal("keys", result.Object); } + [Theory, BitAutoData] + public async Task PostKeys_WithAccountKeysAndNoUserKeyId_DoesNotSetUserKeyId( + User user, + KeysRequestModel model) + { + // Arrange + user.PublicKey = null; + user.PrivateKey = null; + model.AccountKeys = new AccountKeysRequestModel + { + UserKeyEncryptedAccountPrivateKey = "wrapped-private-key", + AccountPublicKey = "public-key", + PublicKeyEncryptionKeyPair = new PublicKeyEncryptionKeyPairRequestModel + { + PublicKey = "public-key", + WrappedPrivateKey = "wrapped-private-key", + SignedPublicKey = "signed-public-key" + }, + SignatureKeyPair = new SignatureKeyPairRequestModel + { + VerifyingKey = "verifying-key", + SignatureAlgorithm = "ed25519", + WrappedSigningKey = "wrapped-signing-key" + }, + SecurityState = new SecurityStateModel + { + SecurityState = "security-state", + SecurityVersion = 2 + } + }; + // A client that predates the key id field sends none. + model.UserKeyId = null; + + _userService.GetUserByPrincipalAsync(Arg.Any()).Returns(user); + + // Act + await _sut.PostKeys(model); + + // Assert + _userRepository.DidNotReceive().SetUserKeyId(Arg.Any(), Arg.Any()); + await _userRepository.Received(1).SetV2AccountCryptographicStateAsync( + user.Id, + Arg.Any(), + null); + } + [Theory, BitAutoData] public async Task PostKeys_WithoutAccountKeys_CallsSaveUser( User user, diff --git a/test/Api.Test/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs b/test/Api.Test/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs index 2eba9c7fe608..7e205deec200 100644 --- a/test/Api.Test/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs +++ b/test/Api.Test/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs @@ -46,6 +46,33 @@ public class AccountsKeyManagementControllerTests private const string _mockKeyId = "0123456789abcdef0123456789abcdef"; + [Theory] + [BitAutoData] + public async Task PostUserKeyIdAsync_UserNull_Throws(SutProvider sutProvider, + SetUserKeyIdRequestModel data) + { + sutProvider.GetDependency().GetUserByPrincipalAsync(Arg.Any()).ReturnsNull(); + + await Assert.ThrowsAsync(() => sutProvider.Sut.PostUserKeyIdAsync(data)); + + await sutProvider.GetDependency().ReceivedWithAnyArgs(0) + .SetUserKeyIdAsync(Arg.Any(), Arg.Any()); + } + + [Theory] + [BitAutoData] + public async Task PostUserKeyIdAsync_Success_CallsCommandWithParsedKeyId( + SutProvider sutProvider, User user) + { + var data = new SetUserKeyIdRequestModel { UserKeyId = _mockKeyId }; + sutProvider.GetDependency().GetUserByPrincipalAsync(Arg.Any()).Returns(user); + + await sutProvider.Sut.PostUserKeyIdAsync(data); + + await sutProvider.GetDependency().Received(1) + .SetUserKeyIdAsync(user, KeyId.FromHexEncodedString(_mockKeyId)!); + } + [Theory] [BitAutoData] public async Task RegenerateKeysAsync_UserNull_Throws(SutProvider sutProvider, diff --git a/test/Common/AutoFixture/KeyIdFixtures.cs b/test/Common/AutoFixture/KeyIdFixtures.cs index 0f332cc24182..c9e86e9b2740 100644 --- a/test/Common/AutoFixture/KeyIdFixtures.cs +++ b/test/Common/AutoFixture/KeyIdFixtures.cs @@ -42,6 +42,6 @@ public class KeyIdCustomization : ICustomization { public void Customize(IFixture fixture) { - fixture.Customizations.Add(new KeyIdBuilder()); + fixture.Customizations.Insert(0, new KeyIdBuilder()); } } diff --git a/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs b/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs index 96b451fbafe2..2cc12af03573 100644 --- a/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs +++ b/test/Core.Test/Auth/Models/Api/Request/Accounts/RegisterFinishRequestModelTests.cs @@ -2,6 +2,7 @@ using Bit.Core.Enums; using Bit.Core.KeyManagement.Kdf; using Bit.Core.KeyManagement.Models.Api.Request; +using Bit.Core.KeyManagement.Models.Data; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Xunit; @@ -214,6 +215,72 @@ public void ToData_Returns_ToData(string email, string masterPasswordHint, KdfRe Assert.Equal(newData.UserAccountKeysData, accountKeysRequest.ToAccountKeysData()); } + [Theory] + [BitAutoData] + [SignatureKeyPairRequestModelCustomize] + public void ToData_CarriesTheUserKeyIdFromTheUnlockData(string email, KdfRequestModel kdfRequest, + string masterPasswordAuthenticationHash, AccountKeysRequestModel accountKeysRequest, string userSymmetricKey) + { + // Arrange + const string keyId = "0123456789abcdef0123456789abcdef"; + var model = new RegisterFinishRequestModel + { + Email = email, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = kdfRequest, + MasterPasswordAuthenticationHash = masterPasswordAuthenticationHash, + Salt = email.ToLowerInvariant().Trim() + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = kdfRequest, + MasterKeyWrappedUserKey = userSymmetricKey, + Salt = email.ToLowerInvariant().Trim(), + ContainedKeyId = keyId + }, + AccountKeys = accountKeysRequest + }; + + // Act + var data = model.ToData(); + + // Assert + Assert.Equal(KeyId.FromHexEncodedString(keyId), data.UserKeyId); + } + + [Theory] + [BitAutoData] + [SignatureKeyPairRequestModelCustomize] + public void ToData_NoUserKeyIdSupplied_LeavesItUnset(string email, KdfRequestModel kdfRequest, + string masterPasswordAuthenticationHash, AccountKeysRequestModel accountKeysRequest, string userSymmetricKey) + { + // Arrange - a client that predates the key id field sends none + var model = new RegisterFinishRequestModel + { + Email = email, + MasterPasswordAuthentication = new MasterPasswordAuthenticationDataRequestModel + { + Kdf = kdfRequest, + MasterPasswordAuthenticationHash = masterPasswordAuthenticationHash, + Salt = email.ToLowerInvariant().Trim() + }, + MasterPasswordUnlock = new MasterPasswordUnlockDataRequestModel + { + Kdf = kdfRequest, + MasterKeyWrappedUserKey = userSymmetricKey, + Salt = email.ToLowerInvariant().Trim() + }, + AccountKeys = accountKeysRequest + }; + + // Act + var data = model.ToData(); + + // Assert + Assert.Null(data.UserKeyId); + } + [Theory] [BitAutoData] [SignatureKeyPairRequestModelCustomize] diff --git a/test/Core.Test/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommandTests.cs b/test/Core.Test/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommandTests.cs index dea30a82010b..55ea98f4633d 100644 --- a/test/Core.Test/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommandTests.cs +++ b/test/Core.Test/Auth/UserFeatures/UserMasterPassword/FinishSsoJitProvisionMasterPasswordCommandTests.cs @@ -142,9 +142,57 @@ public async Task FinishProvisionAsync_UserNotFoundInOrganization_ThrowsBadReque Assert.Equal("User not found within organization.", exception.Message); } + [Theory] + [BitAutoData] + public async Task FinishProvisionAsync_KeyIdSupplied_RecordsTheUserKeyId( + SutProvider sutProvider, + User user, UserAccountKeysData accountKeys, KdfSettings kdfSettings, + Organization org, OrganizationUser orgUser, string masterPasswordHint) + { + // Arrange - this flow provisions the user key, so the supplied key id is authoritative + user.Key = null; + var keyId = KeyId.FromHexEncodedString("0123456789abcdef0123456789abcdef")!; + var model = CreateValidModel(user, accountKeys, kdfSettings, org.Identifier, masterPasswordHint, keyId); + + sutProvider.GetDependency() + .GetByIdentifierAsync(org.Identifier) + .Returns(org); + + sutProvider.GetDependency() + .GetByOrganizationAsync(org.Id, user.Id) + .Returns(orgUser); + + UpdateUserData mockUpdateUserData = (connection, transaction) => Task.CompletedTask; + sutProvider.GetDependency() + .BuildUpdateUserDelegateSetInitialMasterPassword(user, Arg.Any()) + .Returns(mockUpdateUserData); + + UpdateUserData mockSetUserKeyId = (connection, transaction) => Task.CompletedTask; + sutProvider.GetDependency() + .SetUserKeyId(user.Id, keyId) + .Returns(mockSetUserKeyId); + + // Act + await sutProvider.Sut.FinishProvisionAsync(user, model); + + // Assert + sutProvider.GetDependency().Received(1).SetUserKeyId(user.Id, keyId); + await sutProvider.GetDependency().Received(1) + .SetV2AccountCryptographicStateAsync( + user.Id, + model.AccountKeys, + Arg.Do>(actions => + { + var actionsList = actions.ToList(); + Assert.Equal(2, actionsList.Count); + Assert.Same(mockUpdateUserData, actionsList[0]); + Assert.Same(mockSetUserKeyId, actionsList[1]); + })); + } + private static SetInitialMasterPasswordDataModel CreateValidModel( User user, UserAccountKeysData? accountKeys, KdfSettings kdfSettings, - string orgSsoIdentifier, string? masterPasswordHint) + string orgSsoIdentifier, string? masterPasswordHint, KeyId? containedKeyId = null) { var salt = user.GetMasterPasswordSalt(); return new SetInitialMasterPasswordDataModel @@ -159,7 +207,8 @@ private static SetInitialMasterPasswordDataModel CreateValidModel( { Salt = salt, MasterKeyWrappedUserKey = "wrapped-key", - Kdf = kdfSettings + Kdf = kdfSettings, + ContainedKeyId = containedKeyId }, AccountKeys = accountKeys, OrgSsoIdentifier = orgSsoIdentifier, diff --git a/test/Core.Test/Entities/UserTests.cs b/test/Core.Test/Entities/UserTests.cs index 3a15ffc60fa3..e42d5bda8cb3 100644 --- a/test/Core.Test/Entities/UserTests.cs +++ b/test/Core.Test/Entities/UserTests.cs @@ -2,6 +2,7 @@ using Bit.Core.Auth.Enums; using Bit.Core.Auth.Models; using Bit.Core.Entities; +using Bit.Core.KeyManagement.Models.Data; using Bit.Test.Common.Helpers; using Xunit; @@ -141,4 +142,37 @@ public void GetTwoFactorProviders_SavedWithName_Success() var emailMetaDataEmail = Assert.Contains("Email", (IDictionary)email.MetaData); Assert.Equal("test@email.com", emailMetaDataEmail); } + + [Fact] + public void SetUserKeyId_RoundTripsThroughTheColumn() + { + var keyId = KeyId.FromHexEncodedString("0123456789abcdef0123456789abcdef"); + var user = new User(); + + user.SetUserKeyId(keyId); + + Assert.Equal("0123456789abcdef0123456789abcdef", user.UserKeyId); + Assert.Equal(keyId, user.GetUserKeyId()); + } + + [Fact] + public void SetUserKeyId_Null_ClearsTheColumn() + { + var user = new User { UserKeyId = "0123456789abcdef0123456789abcdef" }; + + user.SetUserKeyId(null); + + Assert.Null(user.UserKeyId); + Assert.Null(user.GetUserKeyId()); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void GetUserKeyId_UnsetColumn_ReturnsNull(string? storedValue) + { + var user = new User { UserKeyId = storedValue }; + + Assert.Null(user.GetUserKeyId()); + } } diff --git a/test/Core.Test/KeyManagement/Commands/SetKeyConnectorKeyCommandTests.cs b/test/Core.Test/KeyManagement/Commands/SetKeyConnectorKeyCommandTests.cs index 74f76f368b5b..87f15160aace 100644 --- a/test/Core.Test/KeyManagement/Commands/SetKeyConnectorKeyCommandTests.cs +++ b/test/Core.Test/KeyManagement/Commands/SetKeyConnectorKeyCommandTests.cs @@ -50,6 +50,9 @@ public async Task SetKeyConnectorKeyForUserAsync_Success_SetsAccountKeys( var mockUpdateUserData = Substitute.For(); userRepository.SetKeyConnectorUserKey(user.Id, data.KeyConnectorKeyWrappedUserKey!) .Returns(mockUpdateUserData); + var mockSetUserKeyId = Substitute.For(); + userRepository.SetUserKeyId(user.Id, data.ContainedKeyId!) + .Returns(mockSetUserKeyId); // Act await sutProvider.Sut.SetKeyConnectorKeyForUserAsync(user, data); @@ -74,7 +77,9 @@ await userRepository data.SecurityStateData!.SecurityState == expectedAccountKeysData.SecurityStateData!.SecurityState && data.SecurityStateData.SecurityVersion == expectedAccountKeysData.SecurityStateData.SecurityVersion), Arg.Is>(actions => - actions.Count() == 1 && actions.First() == mockUpdateUserData)); + actions.Count() == 2 && + actions.First() == mockUpdateUserData && + actions.Last() == mockSetUserKeyId)); await sutProvider.GetDependency() .Received(1) @@ -85,6 +90,59 @@ await sutProvider.GetDependency() .AcceptOrgUserByOrgSsoIdAsync(data.OrgIdentifier, user, sutProvider.GetDependency()); } + [Theory, BitAutoData] + public async Task SetKeyConnectorKeyForUserAsync_NoKeyIdSupplied_DoesNotSetUserKeyId( + User user, + KeyConnectorKeysData data, + SutProvider sutProvider) + { + // Set up valid V2 encryption data + if (data.AccountKeys!.SignatureKeyPair != null) + { + data.AccountKeys.SignatureKeyPair.SignatureAlgorithm = "ed25519"; + } + + // Arrange - a client that predates the key id field sends none + data = new KeyConnectorKeysData + { + KeyConnectorKeyWrappedUserKey = data.KeyConnectorKeyWrappedUserKey, + AccountKeys = data.AccountKeys, + OrgIdentifier = data.OrgIdentifier, + ContainedKeyId = null + }; + + user.UsesKeyConnector = false; + var currentContext = sutProvider.GetDependency(); + var httpContext = Substitute.For(); + httpContext.User.Returns(new ClaimsPrincipal()); + currentContext.HttpContext.Returns(httpContext); + + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), user, Arg.Any>()) + .Returns(AuthorizationResult.Success()); + + var userRepository = sutProvider.GetDependency(); + var mockUpdateUserData = Substitute.For(); + userRepository.SetKeyConnectorUserKey(user.Id, data.KeyConnectorKeyWrappedUserKey!) + .Returns(mockUpdateUserData); + + // Act + await sutProvider.Sut.SetKeyConnectorKeyForUserAsync(user, data); + + // Assert + userRepository + .DidNotReceive() + .SetUserKeyId(Arg.Any(), Arg.Any()); + + await userRepository + .Received(1) + .SetV2AccountCryptographicStateAsync( + user.Id, + Arg.Any(), + Arg.Is>(actions => + actions.Count() == 1 && actions.First() == mockUpdateUserData)); + } + [Theory, BitAutoData] public async Task SetKeyConnectorKeyForUserAsync_UserCantUseKeyConnector_ThrowsException( User user, diff --git a/test/Core.Test/KeyManagement/Commands/SetUserKeyIdCommandTests.cs b/test/Core.Test/KeyManagement/Commands/SetUserKeyIdCommandTests.cs new file mode 100644 index 000000000000..2ef033a373a7 --- /dev/null +++ b/test/Core.Test/KeyManagement/Commands/SetUserKeyIdCommandTests.cs @@ -0,0 +1,60 @@ +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.KeyManagement.Commands; +using Bit.Core.KeyManagement.Models.Data; +using Bit.Core.Repositories; +using Bit.Test.Common.AutoFixture; +using Bit.Test.Common.AutoFixture.Attributes; +using NSubstitute; +using Xunit; + +namespace Bit.Core.Test.KeyManagement.Commands; + +[SutProviderCustomize] +public class SetUserKeyIdCommandTests +{ + [Theory, BitAutoData] + public async Task SetUserKeyIdAsync_NoKeyIdRecorded_StoresTheKeyId( + User user, + KeyId userKeyId, + SutProvider sutProvider) + { + // Arrange - an account that pre-dates the key id + user.UserKeyId = null; + + var userRepository = sutProvider.GetDependency(); + var mockUpdateUserData = Substitute.For(); + userRepository.SetUserKeyId(user.Id, userKeyId).Returns(mockUpdateUserData); + + // Act + await sutProvider.Sut.SetUserKeyIdAsync(user, userKeyId); + + // Assert + userRepository.Received(1).SetUserKeyId(user.Id, userKeyId); + await userRepository + .Received(1) + .UpdateUserDataAsync(Arg.Is>(actions => + actions.Count() == 1 && actions.First() == mockUpdateUserData)); + } + + [Theory, BitAutoData] + public async Task SetUserKeyIdAsync_KeyIdAlreadyRecorded_ThrowsAndWritesNothing( + User user, + KeyId userKeyId, + SutProvider sutProvider) + { + // Arrange - reporting a key id must not rename a key the account is already known to use + user.UserKeyId = "fedcba9876543210fedcba9876543210"; + + // Act + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.SetUserKeyIdAsync(user, userKeyId)); + + // Assert + Assert.Equal("User key id is already set.", exception.Message); + + var userRepository = sutProvider.GetDependency(); + userRepository.DidNotReceive().SetUserKeyId(Arg.Any(), Arg.Any()); + await userRepository.DidNotReceive().UpdateUserDataAsync(Arg.Any>()); + } +} diff --git a/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs b/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs index cfc674345077..2465043f9973 100644 --- a/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs +++ b/test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs @@ -1066,6 +1066,97 @@ await sutProvider.GetDependency().Received(1) .PushLogOutAsync(user.Id); } + [Theory] + [BitAutoData] + public async Task MasterPasswordRotateUserAccountKeysAsync_RecordsTheNewUserKeyId( + SutProvider sutProvider, User user, MasterPasswordRotateUserAccountKeysData model) + { + model = SetupTestData(model); + SetupUserKdf(user, model); + var signatureRepository = sutProvider.GetDependency(); + SetV2ExistingUser(user, signatureRepository); + SetV2ModelUser(model.BaseData); + user.SetUserKeyId(KeyId.FromHexEncodedString("fedcba9876543210fedcba9876543210")); + + await sutProvider.Sut.MasterPasswordRotateUserAccountKeysAsync(user, model); + + Assert.Equal(model.BaseData.NewUserKeyId, user.GetUserKeyId()); + } + + [Theory] + [BitAutoData] + public async Task TdeRotateUserAccountKeysAsync_RecordsTheNewUserKeyId( + SutProvider sutProvider, User user, TdeRotateUserAccountKeysData model) + { + SetupTdeUser(user); + var signatureRepository = sutProvider.GetDependency(); + SetV2ExistingUser(user, signatureRepository); + SetV2ModelUser(model.BaseData); + user.SetUserKeyId(KeyId.FromHexEncodedString("fedcba9876543210fedcba9876543210")); + + await sutProvider.Sut.TdeRotateUserAccountKeysAsync(user, model); + + Assert.Equal(model.BaseData.NewUserKeyId, user.GetUserKeyId()); + } + + [Theory] + [BitAutoData] + public async Task KeyConnectorRotateUserAccountKeysAsync_RecordsTheNewUserKeyId( + SutProvider sutProvider, User user, KeyConnectorRotateUserAccountKeysData model) + { + SetupKeyConnectorUser(user); + var signatureRepository = sutProvider.GetDependency(); + SetV2ExistingUser(user, signatureRepository); + SetV2ModelUser(model.BaseData); + user.SetUserKeyId(KeyId.FromHexEncodedString("fedcba9876543210fedcba9876543210")); + + await sutProvider.Sut.KeyConnectorRotateUserAccountKeysAsync(user, model); + + Assert.Equal(model.BaseData.NewUserKeyId, user.GetUserKeyId()); + } + + [Theory] + [BitAutoData] + public async Task PasswordChangeAndRotateUserAccountKeysAsync_RecordsTheNewUserKeyId( + SutProvider sutProvider, User user, + PasswordChangeAndRotateUserAccountKeysData model) + { + SetTestKdfAndSaltForUserAndModel(user, model); + var signatureRepository = sutProvider.GetDependency(); + SetV2ExistingUser(user, signatureRepository); + SetV2ModelUser(model.BaseData); + user.SetUserKeyId(KeyId.FromHexEncodedString("fedcba9876543210fedcba9876543210")); + sutProvider.GetDependency().CheckPasswordAsync(user, model.OldMasterKeyAuthenticationHash) + .Returns(true); + sutProvider.GetDependency() + .PrepareUpdateExistingMasterPasswordAsync(user, Arg.Any()) + .Returns(OneOf.FromT0(user)); + + await sutProvider.Sut.PasswordChangeAndRotateUserAccountKeysAsync(user, model); + + Assert.Equal(model.BaseData.NewUserKeyId, user.GetUserKeyId()); + } + + [Theory] + [BitAutoData] + public async Task TdeRotateUserAccountKeysAsync_NoKeyIdSupplied_ClearsTheStoredKeyId( + SutProvider sutProvider, User user, TdeRotateUserAccountKeysData model) + { + SetupTdeUser(user); + var signatureRepository = sutProvider.GetDependency(); + SetV2ExistingUser(user, signatureRepository); + SetV2ModelUser(model.BaseData); + // A client that predates the key id field sends none. The rotation still replaces the user + // key, so the stored key id names a key that no longer exists and must not survive. + model.BaseData.NewUserKeyId = null; + user.SetUserKeyId(KeyId.FromHexEncodedString("fedcba9876543210fedcba9876543210")); + + await sutProvider.Sut.TdeRotateUserAccountKeysAsync(user, model); + + Assert.Null(user.UserKeyId); + Assert.Null(user.GetUserKeyId()); + } + // Helper functions to set valid test parameters that match each other to the model and user. private static void SetTestKdfAndSaltForUserAndModel(User user, PasswordChangeAndRotateUserAccountKeysData model) { From f65bc64464e811fcbec565200d2a433fcc4adc27 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 11 Aug 2026 13:35:19 +0900 Subject: [PATCH 2/2] feat(vault): add key id validation --- .../Vault/Controllers/CiphersController.cs | 122 +++++----- .../Models/Request/CipherRequestModel.cs | 16 ++ .../Controllers/CiphersControllerTests.cs | 220 +++++++++++++++++- 3 files changed, 289 insertions(+), 69 deletions(-) diff --git a/src/Api/Vault/Controllers/CiphersController.cs b/src/Api/Vault/Controllers/CiphersController.cs index 44e3a30632a3..848a9b8cc9e5 100644 --- a/src/Api/Vault/Controllers/CiphersController.cs +++ b/src/Api/Vault/Controllers/CiphersController.cs @@ -165,15 +165,8 @@ public async Task Post([FromBody] CipherRequestModel model) { var user = await _userService.GetUserByPrincipalAsync(User); - // Validate the model was encrypted for the posting user - if (model.EncryptedFor != null) - { - if (model.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", user.Id, model.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model, user); var cipher = model.ToCipherDetails(user.Id); if (cipher.OrganizationId.HasValue && !await _currentContext.OrganizationUser(cipher.OrganizationId.Value)) @@ -191,15 +184,8 @@ public async Task PostCreate([FromBody] CipherCreateRequest { var user = await _userService.GetUserByPrincipalAsync(User); - // Validate the model was encrypted for the posting user - if (model.Cipher.EncryptedFor != null) - { - if (model.Cipher.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", user.Id, model.Cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model.Cipher, user); var cipher = model.Cipher.ToCipherDetails(user.Id); if (cipher.OrganizationId.HasValue && !await _currentContext.OrganizationUser(cipher.OrganizationId.Value)) @@ -225,14 +211,7 @@ public async Task PostAdmin([FromBody] CipherCreateRequ var userId = _userService.GetProperUserId(User).Value; // Validate the model was encrypted for the posting user - if (model.Cipher.EncryptedFor != null) - { - if (model.Cipher.EncryptedFor != userId) - { - _logger.LogError("Cipher was not encrypted for the current user. CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", userId, model.Cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + ValidateCipherEncryptedForUser(model.Cipher, userId); await _cipherService.SaveAsync(cipher, userId, model.Cipher.LastKnownRevisionDate, model.CollectionIds, true, false); @@ -250,15 +229,8 @@ public async Task Put(Guid id, [FromBody] CipherRequestMode throw new NotFoundException(); } - // Validate the model was encrypted for the posting user - if (model.EncryptedFor != null) - { - if (model.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", id, user.Id, model.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model, user, id); ValidateClientVersionForFido2CredentialSupport(cipher); @@ -291,14 +263,7 @@ public async Task PutAdmin(Guid id, [FromBody] CipherRe var cipher = await _cipherRepository.GetOrganizationDetailsByIdAsync(id); // Validate the model was encrypted for the posting user - if (model.EncryptedFor != null) - { - if (model.EncryptedFor != userId) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", id, userId, model.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + ValidateCipherEncryptedForUser(model, userId, id); ValidateClientVersionForFido2CredentialSupport(cipher); @@ -732,15 +697,8 @@ public async Task PutShare(Guid id, [FromBody] CipherShareR throw new NotFoundException(); } - // Validate the model was encrypted for the posting user - if (model.Cipher.EncryptedFor != null) - { - if (model.Cipher.EncryptedFor != user.Id) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId} CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", id, user.Id, model.Cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } - } + // Validate the model was encrypted by the posting user + ValidateCipherEncryptedByUser(model.Cipher, user, id); ValidateClientVersionForFido2CredentialSupport(cipher); @@ -1237,14 +1195,10 @@ public async Task> PutShareMany([From var ciphers = await _cipherRepository.GetManyByUserIdAsync(userId, withOrganizations: false); var ciphersDict = ciphers.ToDictionary(c => c.Id); - // Validate the model was encrypted for the posting user + // Validate the models were encrypted for the posting user foreach (var cipher in model.Ciphers) { - if (cipher.EncryptedFor.HasValue && cipher.EncryptedFor.Value != userId) - { - _logger.LogError("Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", cipher.Id, userId, cipher.EncryptedFor); - throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); - } + ValidateCipherEncryptedForUser(cipher, userId, cipher.Id); } var shareCiphers = new List<(CipherDetails, DateTime?)>(); @@ -1661,6 +1615,58 @@ private void ValidateClientVersionForFido2CredentialSupport(Cipher cipher) } } + /// + /// Validates that the cipher in was encrypted by the acting user. + /// + /// Deprecated in favor of , which identifies the key + /// rather than the user. Only checked when the client sends the field. + /// + /// + private void ValidateCipherEncryptedForUser(CipherRequestModel model, Guid userId, Guid? cipherId = null) + { +#pragma warning disable CS0618 // EncryptedFor is deprecated, but is still honored for clients that send it. + var encryptedFor = model.EncryptedFor; +#pragma warning restore CS0618 + + if (encryptedFor != null && encryptedFor != userId) + { + _logger.LogError( + "Cipher was not encrypted for the current user. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedFor: {EncryptedFor}", + cipherId, userId, encryptedFor); + throw new BadRequestException("Cipher was not encrypted for the current user. Please try again."); + } + } + + /// + /// Validates that the cipher in was encrypted by the acting user, with that + /// user's current user key. + /// + /// The key id is only compared when both sides are present: the client may predate the field, and the + /// user may not have a key id recorded yet. This mirrors + /// . + /// + /// + private void ValidateCipherEncryptedByUser(CipherRequestModel model, User user, Guid? cipherId = null) + { + ValidateCipherEncryptedForUser(model, user.Id, cipherId); + + var currentUserKeyId = user.GetUserKeyId(); + var encryptedByKeyId = model.GetEncryptedByKeyId(); + if (currentUserKeyId is null || encryptedByKeyId is null) + { + // Either the user has no key id recorded yet, or the client predates the field; nothing to compare. + return; + } + + if (!currentUserKeyId.Equals(encryptedByKeyId)) + { + _logger.LogError( + "Cipher was not encrypted with the current user key. CipherId: {CipherId}, CurrentUser: {CurrentUserId}, EncryptedByKeyId: {EncryptedByKeyId}", + cipherId, user.Id, encryptedByKeyId); + throw new BadRequestException("Cipher was not encrypted with the current user key. Please try again."); + } + } + private async Task GetByIdAsyncAdmin(Guid cipherId) { return await _cipherRepository.GetOrganizationDetailsByIdAsync(cipherId); diff --git a/src/Api/Vault/Models/Request/CipherRequestModel.cs b/src/Api/Vault/Models/Request/CipherRequestModel.cs index 9c419de4cb38..49b0b2767694 100644 --- a/src/Api/Vault/Models/Request/CipherRequestModel.cs +++ b/src/Api/Vault/Models/Request/CipherRequestModel.cs @@ -3,6 +3,7 @@ using System.ComponentModel.DataAnnotations; using System.Text.Json; +using Bit.Core.KeyManagement.Models.Data; using Bit.Core.Utilities; using Bit.Core.Vault.Entities; using Bit.Core.Vault.Enums; @@ -15,7 +16,16 @@ public class CipherRequestModel /// /// The Id of the user that encrypted the cipher. It should always represent a UserId. /// + [Obsolete("Use EncryptedByKeyId instead, which identifies the key the cipher was encrypted with.")] public Guid? EncryptedFor { get; set; } + + /// + /// Hex-encoded key id of the user key the client held when it encrypted this cipher. Absent for + /// clients that predate the field. When present, it must match the acting user's current user key id. + /// + [KeyId] + public string EncryptedByKeyId { get; set; } + public CipherType Type { get; set; } [StringLength(36)] @@ -67,6 +77,12 @@ public class CipherRequestModel public DateTime? LastKnownRevisionDate { get; set; } = null; public DateTime? ArchivedDate { get; set; } + /// + /// The key the client encrypted this cipher with, or null when it did not supply one. + /// + public KeyId GetEncryptedByKeyId() => + KeyId.FromHexEncodedString(string.IsNullOrEmpty(EncryptedByKeyId) ? null : EncryptedByKeyId); + public CipherDetails ToCipherDetails(Guid userId, bool allowOrgIdSet = true) { var hasOrgId = !string.IsNullOrWhiteSpace(OrganizationId); diff --git a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs index 926180400dd7..0c1d13a87bf8 100644 --- a/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs +++ b/test/Api.Test/Vault/Controllers/CiphersControllerTests.cs @@ -1809,13 +1809,17 @@ public async Task PutShareMany_OrganizationUserFalse_ThrowsNotFound( await Assert.ThrowsAsync(() => sut.Sut.PutShareMany(model)); } [Theory, BitAutoData] - public async Task PutShareMany_CipherNotOwned_ThrowsNotFoundException( + public async Task PutShareMany_CipherNotOwned_ThrowsBadRequestException( Guid organizationId, Guid userId, CipherWithIdRequestModel request, SutProvider sutProvider) { - request.EncryptedFor = userId; + // The controller reads the organization off the first cipher, so it has to match the stub below. + request.OrganizationId = organizationId.ToString(); +#pragma warning disable CS0618 + request.EncryptedFor = null; +#pragma warning restore CS0618 var model = new CipherBulkShareRequestModel { Ciphers = new[] { request }, @@ -1832,19 +1836,23 @@ public async Task PutShareMany_CipherNotOwned_ThrowsNotFoundException( .GetManyByUserIdAsync(userId, withOrganizations: false) .Returns(Task.FromResult((ICollection)new List())); - await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.PutShareMany(model) ); } [Theory, BitAutoData] - public async Task PutShareMany_EncryptedForWrongUser_ThrowsNotFoundException( + public async Task PutShareMany_EncryptedForWrongUser_ThrowsBadRequestException( Guid organizationId, Guid userId, CipherWithIdRequestModel request, SutProvider sutProvider) { + // The controller reads the organization off the first cipher, so it has to match the stub below. + request.OrganizationId = organizationId.ToString(); +#pragma warning disable CS0618 // Deliberately exercising the deprecated field. request.EncryptedFor = Guid.NewGuid(); // not equal to userId +#pragma warning restore CS0618 var model = new CipherBulkShareRequestModel { Ciphers = new[] { request }, @@ -1863,7 +1871,7 @@ public async Task PutShareMany_EncryptedForWrongUser_ThrowsNotFoundException( .GetManyByUserIdAsync(userId, withOrganizations: false) .Returns(Task.FromResult((ICollection)(new[] { existing }))); - await Assert.ThrowsAsync( + await Assert.ThrowsAsync( () => sutProvider.Sut.PutShareMany(model) ); } @@ -2019,8 +2027,7 @@ public async Task PutShare_WithNullFolderAndFalseFavorite_UpdatesFieldsCorrectly Name = "SharedCipher", Data = JsonSerializer.Serialize(new { Username = "test", Password = "test" }), FolderId = null, - Favorite = false, - EncryptedFor = userId + Favorite = false }, CollectionIds = [Guid.NewGuid().ToString()] }; @@ -2092,8 +2099,7 @@ public async Task PutShare_WithFolderAndFavoriteSet_AddsUserSpecificFields( Name = "SharedCipher", Data = JsonSerializer.Serialize(new { Username = "test", Password = "test" }), FolderId = folderId.ToString(), - Favorite = true, - EncryptedFor = userId + Favorite = true }, CollectionIds = [Guid.NewGuid().ToString()] }; @@ -2168,8 +2174,7 @@ public async Task PutShare_UpdateExistingFolderAndFavorite_UpdatesUserSpecificFi Name = "SharedCipher", Data = JsonSerializer.Serialize(new { Username = "test", Password = "test" }), FolderId = newFolderId.ToString(), // Update to new folder - Favorite = true, // Add favorite - EncryptedFor = userId + Favorite = true // Add favorite }, CollectionIds = [Guid.NewGuid().ToString()] }; @@ -2539,4 +2544,197 @@ await sutProvider.GetDependency().Received(1) ApiHelpers.EventGridKey = previousEventGridKey; } } + + /// + /// A well-formed key id that is never the one hands out, so it always + /// mismatches the user key id on an AutoFixture-generated . + /// + private const string MismatchedKeyId = "ffffffffffffffffffffffffffffffff"; + + private static CipherRequestModel SecureNoteRequestModel(string encryptedByKeyId) => new() + { + Type = CipherType.SecureNote, + Name = "test", + Data = "{}", + EncryptedByKeyId = encryptedByKeyId + }; + + [Theory, BitAutoData] + public async Task Post_EncryptedByKeyIdMatchesUserKeyId_SavesCipher( + User user, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + await sutProvider.Sut.Post(SecureNoteRequestModel(KeyIdBuilder.HexEncodedKeyId)); + + await sutProvider.GetDependency().Received(1) + .SaveDetailsAsync(Arg.Any(), user.Id, Arg.Any(), Arg.Any>(), Arg.Any()); + } + + [Theory, BitAutoData] + public async Task Post_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.Post(SecureNoteRequestModel(MismatchedKeyId))); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .SaveDetailsAsync(default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task Post_EncryptedByKeyIdNotSent_SavesCipher( + User user, + SutProvider sutProvider) + { + // A client that predates the field sends nothing, and must keep working. + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + await sutProvider.Sut.Post(SecureNoteRequestModel(null)); + + await sutProvider.GetDependency().Received(1) + .SaveDetailsAsync(Arg.Any(), user.Id, Arg.Any(), Arg.Any>(), Arg.Any()); + } + + [Theory, BitAutoData] + public async Task Post_UserHasNoKeyId_DoesNotValidateEncryptedByKeyId( + User user, + SutProvider sutProvider) + { + // Nothing to compare against until the user's key id has been backfilled. + user.UserKeyId = null; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + await sutProvider.Sut.Post(SecureNoteRequestModel(MismatchedKeyId)); + + await sutProvider.GetDependency().Received(1) + .SaveDetailsAsync(Arg.Any(), user.Id, Arg.Any(), Arg.Any>(), Arg.Any()); + } + + [Theory, BitAutoData] + public async Task PostCreate_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + + var model = new CipherCreateRequestModel + { + Cipher = SecureNoteRequestModel(MismatchedKeyId), + CollectionIds = [] + }; + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.PostCreate(model)); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .SaveDetailsAsync(default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task Put_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + Guid cipherId, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + // The cipher-not-found check runs before validation, so the cipher has to exist. + sutProvider.GetDependency() + .GetByIdAsync(cipherId, user.Id) + .Returns(new CipherDetails + { + Id = cipherId, + UserId = user.Id, + Type = CipherType.SecureNote, + Data = "{}" + }); + + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.Put(cipherId, SecureNoteRequestModel(MismatchedKeyId))); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .SaveDetailsAsync(default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task PutShare_EncryptedByKeyIdDoesNotMatchUserKeyId_ThrowsBadRequestException( + User user, + Guid cipherId, + Guid organizationId, + SutProvider sutProvider) + { + user.UserKeyId = KeyIdBuilder.HexEncodedKeyId; + sutProvider.GetDependency() + .GetUserByPrincipalAsync(Arg.Any()) + .Returns(user); + // Ownership and organization membership are checked before validation. + sutProvider.GetDependency() + .GetByIdAsync(cipherId) + .Returns(new Cipher + { + Id = cipherId, + UserId = user.Id, + Type = CipherType.Login, + Data = "{}" + }); + sutProvider.GetDependency() + .OrganizationUser(organizationId) + .Returns(true); + + var cipherModel = SecureNoteRequestModel(MismatchedKeyId); + cipherModel.OrganizationId = organizationId.ToString(); + var model = new CipherShareRequestModel + { + Cipher = cipherModel, + CollectionIds = [Guid.NewGuid().ToString()] + }; + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.PutShare(cipherId, model)); + Assert.Contains("current user key", exception.Message); + + await sutProvider.GetDependency().DidNotReceiveWithAnyArgs() + .ShareAsync(default, default, default, default, default, default); + } + + [Theory, BitAutoData] + public async Task PutAdmin_EncryptedForWrongUser_ThrowsBadRequestExceptionBeforeAuthorizationCheck( + Guid userId, + Guid cipherId, + SutProvider sutProvider) + { + sutProvider.GetDependency() + .GetProperUserId(default) + .ReturnsForAnyArgs(userId); + + var model = SecureNoteRequestModel(null); +#pragma warning disable CS0618 + model.EncryptedFor = Guid.NewGuid(); // not equal to userId +#pragma warning restore CS0618 + + var exception = await Assert.ThrowsAsync(() => sutProvider.Sut.PutAdmin(cipherId, model)); + Assert.Contains("encrypted for the current user", exception.Message); + } }