From 34580f0472b3a17f5587a46c408e8d19c33b4484 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 24 Jun 2025 16:32:38 +0200 Subject: [PATCH] Remove key rotation v1 (#5939) --- .../Auth/Controllers/AccountsController.cs | 85 +----------- .../UserServiceCollectionExtensions.cs | 1 - .../Models/Data/RotateUserKeyData.cs | 20 --- .../UserKey/IRotateUserAccountKeysCommand.cs | 10 ++ .../UserKey/IRotateUserKeyCommand.cs | 29 ----- .../Implementations/RotateUserKeyCommand.cs | 121 ------------------ .../Controllers/AccountsControllerTests.cs | 45 +------ .../UserKey/RotateUserKeyCommandTests.cs | 86 ------------- 8 files changed, 13 insertions(+), 384 deletions(-) delete mode 100644 src/Core/KeyManagement/Models/Data/RotateUserKeyData.cs delete mode 100644 src/Core/KeyManagement/UserKey/IRotateUserKeyCommand.cs delete mode 100644 src/Core/KeyManagement/UserKey/Implementations/RotateUserKeyCommand.cs delete mode 100644 test/Core.Test/KeyManagement/UserKey/RotateUserKeyCommandTests.cs diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index 2499b269f5..ec542daec7 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -1,34 +1,21 @@ -using Bit.Api.AdminConsole.Models.Request.Organizations; -using Bit.Api.AdminConsole.Models.Response; -using Bit.Api.Auth.Models.Request; +using Bit.Api.AdminConsole.Models.Response; using Bit.Api.Auth.Models.Request.Accounts; -using Bit.Api.Auth.Models.Request.WebAuthn; -using Bit.Api.KeyManagement.Validators; using Bit.Api.Models.Request.Accounts; using Bit.Api.Models.Response; -using Bit.Api.Tools.Models.Request; -using Bit.Api.Vault.Models.Request; using Bit.Core; using Bit.Core.AdminConsole.Enums.Provider; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; -using Bit.Core.Auth.Entities; using Bit.Core.Auth.Models.Api.Request.Accounts; -using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.UserFeatures.TdeOffboardingPassword.Interfaces; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; -using Bit.Core.Entities; using Bit.Core.Enums; using Bit.Core.Exceptions; -using Bit.Core.KeyManagement.Models.Data; -using Bit.Core.KeyManagement.UserKey; using Bit.Core.Models.Api.Response; using Bit.Core.Repositories; using Bit.Core.Services; -using Bit.Core.Tools.Entities; using Bit.Core.Utilities; -using Bit.Core.Vault.Entities; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; @@ -45,22 +32,9 @@ public class AccountsController : Controller private readonly IPolicyService _policyService; private readonly ISetInitialMasterPasswordCommand _setInitialMasterPasswordCommand; private readonly ITdeOffboardingPasswordCommand _tdeOffboardingPasswordCommand; - private readonly IRotateUserKeyCommand _rotateUserKeyCommand; private readonly ITwoFactorIsEnabledQuery _twoFactorIsEnabledQuery; private readonly IFeatureService _featureService; - private readonly IRotationValidator, IEnumerable> _cipherValidator; - private readonly IRotationValidator, IEnumerable> _folderValidator; - private readonly IRotationValidator, IReadOnlyList> _sendValidator; - private readonly IRotationValidator, IEnumerable> - _emergencyAccessValidator; - private readonly IRotationValidator, - IReadOnlyList> - _organizationUserValidator; - private readonly IRotationValidator, IEnumerable> - _webauthnKeyValidator; - - public AccountsController( IOrganizationService organizationService, IOrganizationUserRepository organizationUserRepository, @@ -69,17 +43,8 @@ public class AccountsController : Controller IPolicyService policyService, ISetInitialMasterPasswordCommand setInitialMasterPasswordCommand, ITdeOffboardingPasswordCommand tdeOffboardingPasswordCommand, - IRotateUserKeyCommand rotateUserKeyCommand, ITwoFactorIsEnabledQuery twoFactorIsEnabledQuery, - IFeatureService featureService, - IRotationValidator, IEnumerable> cipherValidator, - IRotationValidator, IEnumerable> folderValidator, - IRotationValidator, IReadOnlyList> sendValidator, - IRotationValidator, IEnumerable> - emergencyAccessValidator, - IRotationValidator, IReadOnlyList> - organizationUserValidator, - IRotationValidator, IEnumerable> webAuthnKeyValidator + IFeatureService featureService ) { _organizationService = organizationService; @@ -89,15 +54,8 @@ public class AccountsController : Controller _policyService = policyService; _setInitialMasterPasswordCommand = setInitialMasterPasswordCommand; _tdeOffboardingPasswordCommand = tdeOffboardingPasswordCommand; - _rotateUserKeyCommand = rotateUserKeyCommand; _twoFactorIsEnabledQuery = twoFactorIsEnabledQuery; _featureService = featureService; - _cipherValidator = cipherValidator; - _folderValidator = folderValidator; - _sendValidator = sendValidator; - _emergencyAccessValidator = emergencyAccessValidator; - _organizationUserValidator = organizationUserValidator; - _webauthnKeyValidator = webAuthnKeyValidator; } @@ -313,45 +271,6 @@ public class AccountsController : Controller throw new BadRequestException(ModelState); } - [Obsolete("Replaced by the safer rotate-user-account-keys endpoint.")] - [HttpPost("key")] - public async Task PostKey([FromBody] UpdateKeyRequestModel model) - { - var user = await _userService.GetUserByPrincipalAsync(User); - if (user == null) - { - throw new UnauthorizedAccessException(); - } - - var dataModel = new RotateUserKeyData - { - MasterPasswordHash = model.MasterPasswordHash, - Key = model.Key, - PrivateKey = model.PrivateKey, - Ciphers = await _cipherValidator.ValidateAsync(user, model.Ciphers), - Folders = await _folderValidator.ValidateAsync(user, model.Folders), - Sends = await _sendValidator.ValidateAsync(user, model.Sends), - EmergencyAccesses = await _emergencyAccessValidator.ValidateAsync(user, model.EmergencyAccessKeys), - OrganizationUsers = await _organizationUserValidator.ValidateAsync(user, model.ResetPasswordKeys), - WebAuthnKeys = await _webauthnKeyValidator.ValidateAsync(user, model.WebAuthnKeys) - }; - - var result = await _rotateUserKeyCommand.RotateUserKeyAsync(user, dataModel); - - if (result.Succeeded) - { - return; - } - - foreach (var error in result.Errors) - { - ModelState.AddModelError(string.Empty, error.Description); - } - - await Task.Delay(2000); - throw new BadRequestException(ModelState); - } - [HttpPost("security-stamp")] public async Task PostSecurityStamp([FromBody] SecretVerificationRequestModel model) { diff --git a/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs b/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs index 7731e04af2..53bd8bdba2 100644 --- a/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs +++ b/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs @@ -38,7 +38,6 @@ public static class UserServiceCollectionExtensions public static void AddUserKeyCommands(this IServiceCollection services, IGlobalSettings globalSettings) { - services.AddScoped(); services.AddScoped(); } diff --git a/src/Core/KeyManagement/Models/Data/RotateUserKeyData.cs b/src/Core/KeyManagement/Models/Data/RotateUserKeyData.cs deleted file mode 100644 index 9813f760f3..0000000000 --- a/src/Core/KeyManagement/Models/Data/RotateUserKeyData.cs +++ /dev/null @@ -1,20 +0,0 @@ -using Bit.Core.Auth.Entities; -using Bit.Core.Auth.Models.Data; -using Bit.Core.Entities; -using Bit.Core.Tools.Entities; -using Bit.Core.Vault.Entities; - -namespace Bit.Core.KeyManagement.Models.Data; - -public class RotateUserKeyData -{ - public string MasterPasswordHash { get; set; } - public string Key { get; set; } - public string PrivateKey { get; set; } - public IEnumerable Ciphers { get; set; } - public IEnumerable Folders { get; set; } - public IReadOnlyList Sends { get; set; } - public IEnumerable EmergencyAccesses { get; set; } - public IReadOnlyList OrganizationUsers { get; set; } - public IEnumerable WebAuthnKeys { get; set; } -} diff --git a/src/Core/KeyManagement/UserKey/IRotateUserAccountKeysCommand.cs b/src/Core/KeyManagement/UserKey/IRotateUserAccountKeysCommand.cs index ec40e7031d..1550f3c186 100644 --- a/src/Core/KeyManagement/UserKey/IRotateUserAccountKeysCommand.cs +++ b/src/Core/KeyManagement/UserKey/IRotateUserAccountKeysCommand.cs @@ -1,6 +1,7 @@ using Bit.Core.Entities; using Bit.Core.KeyManagement.Models.Data; using Microsoft.AspNetCore.Identity; +using Microsoft.Data.SqlClient; namespace Bit.Core.KeyManagement.UserKey; @@ -18,3 +19,12 @@ public interface IRotateUserAccountKeysCommand /// User KDF settings and email must match the model provided settings. Task RotateUserAccountKeysAsync(User user, RotateUserAccountKeysData model); } + +/// +/// A type used to implement updates to the database for key rotations. Each domain that requires an update of encrypted +/// data during a key rotation should use this to implement its own database call. The user repository loops through +/// these during a key rotation. +/// Note: connection and transaction are only used for Dapper. They won't be available in EF +/// +public delegate Task UpdateEncryptedDataForKeyRotation(SqlConnection connection = null, + SqlTransaction transaction = null); diff --git a/src/Core/KeyManagement/UserKey/IRotateUserKeyCommand.cs b/src/Core/KeyManagement/UserKey/IRotateUserKeyCommand.cs deleted file mode 100644 index 90dc90541f..0000000000 --- a/src/Core/KeyManagement/UserKey/IRotateUserKeyCommand.cs +++ /dev/null @@ -1,29 +0,0 @@ -using Bit.Core.Entities; -using Bit.Core.KeyManagement.Models.Data; -using Microsoft.AspNetCore.Identity; -using Microsoft.Data.SqlClient; - -namespace Bit.Core.KeyManagement.UserKey; - -/// -/// Responsible for rotation of a user key and updating database with re-encrypted data -/// -public interface IRotateUserKeyCommand -{ - /// - /// Sets a new user key and updates all encrypted data. - /// - /// All necessary information for rotation. Warning: Any encrypted data not included will be lost. - /// An IdentityResult for verification of the master password hash - /// User must be provided. - Task RotateUserKeyAsync(User user, RotateUserKeyData model); -} - -/// -/// A type used to implement updates to the database for key rotations. Each domain that requires an update of encrypted -/// data during a key rotation should use this to implement its own database call. The user repository loops through -/// these during a key rotation. -/// Note: connection and transaction are only used for Dapper. They won't be available in EF -/// -public delegate Task UpdateEncryptedDataForKeyRotation(SqlConnection connection = null, - SqlTransaction transaction = null); diff --git a/src/Core/KeyManagement/UserKey/Implementations/RotateUserKeyCommand.cs b/src/Core/KeyManagement/UserKey/Implementations/RotateUserKeyCommand.cs deleted file mode 100644 index 8cece5f762..0000000000 --- a/src/Core/KeyManagement/UserKey/Implementations/RotateUserKeyCommand.cs +++ /dev/null @@ -1,121 +0,0 @@ -using Bit.Core.Auth.Repositories; -using Bit.Core.Entities; -using Bit.Core.KeyManagement.Models.Data; -using Bit.Core.Platform.Push; -using Bit.Core.Repositories; -using Bit.Core.Services; -using Bit.Core.Tools.Repositories; -using Bit.Core.Vault.Repositories; -using Microsoft.AspNetCore.Identity; - -namespace Bit.Core.KeyManagement.UserKey.Implementations; - -/// -public class RotateUserKeyCommand : IRotateUserKeyCommand -{ - private readonly IUserService _userService; - private readonly IUserRepository _userRepository; - private readonly ICipherRepository _cipherRepository; - private readonly IFolderRepository _folderRepository; - private readonly ISendRepository _sendRepository; - private readonly IEmergencyAccessRepository _emergencyAccessRepository; - private readonly IOrganizationUserRepository _organizationUserRepository; - private readonly IPushNotificationService _pushService; - private readonly IdentityErrorDescriber _identityErrorDescriber; - private readonly IWebAuthnCredentialRepository _credentialRepository; - - /// - /// Instantiates a new - /// - /// Master password hash validation - /// Updates user keys and re-encrypted data if needed - /// Provides a method to update re-encrypted cipher data - /// Provides a method to update re-encrypted folder data - /// Provides a method to update re-encrypted send data - /// Provides a method to update re-encrypted emergency access data - /// Logs out user from other devices after successful rotation - /// Provides a password mismatch error if master password hash validation fails - public RotateUserKeyCommand(IUserService userService, IUserRepository userRepository, - ICipherRepository cipherRepository, IFolderRepository folderRepository, ISendRepository sendRepository, - IEmergencyAccessRepository emergencyAccessRepository, IOrganizationUserRepository organizationUserRepository, - IPushNotificationService pushService, IdentityErrorDescriber errors, IWebAuthnCredentialRepository credentialRepository) - { - _userService = userService; - _userRepository = userRepository; - _cipherRepository = cipherRepository; - _folderRepository = folderRepository; - _sendRepository = sendRepository; - _emergencyAccessRepository = emergencyAccessRepository; - _organizationUserRepository = organizationUserRepository; - _pushService = pushService; - _identityErrorDescriber = errors; - _credentialRepository = credentialRepository; - } - - /// - public async Task RotateUserKeyAsync(User user, RotateUserKeyData model) - { - if (user == null) - { - throw new ArgumentNullException(nameof(user)); - } - - if (!await _userService.CheckPasswordAsync(user, model.MasterPasswordHash)) - { - return IdentityResult.Failed(_identityErrorDescriber.PasswordMismatch()); - } - - var now = DateTime.UtcNow; - user.RevisionDate = user.AccountRevisionDate = now; - user.LastKeyRotationDate = now; - user.SecurityStamp = Guid.NewGuid().ToString(); - user.Key = model.Key; - user.PrivateKey = model.PrivateKey; - if (model.Ciphers.Any() || model.Folders.Any() || model.Sends.Any() || model.EmergencyAccesses.Any() || - model.OrganizationUsers.Any() || model.WebAuthnKeys.Any()) - { - List saveEncryptedDataActions = new(); - - if (model.Ciphers.Any()) - { - saveEncryptedDataActions.Add(_cipherRepository.UpdateForKeyRotation(user.Id, model.Ciphers)); - } - - if (model.Folders.Any()) - { - saveEncryptedDataActions.Add(_folderRepository.UpdateForKeyRotation(user.Id, model.Folders)); - } - - if (model.Sends.Any()) - { - saveEncryptedDataActions.Add(_sendRepository.UpdateForKeyRotation(user.Id, model.Sends)); - } - - if (model.EmergencyAccesses.Any()) - { - saveEncryptedDataActions.Add( - _emergencyAccessRepository.UpdateForKeyRotation(user.Id, model.EmergencyAccesses)); - } - - if (model.OrganizationUsers.Any()) - { - saveEncryptedDataActions.Add( - _organizationUserRepository.UpdateForKeyRotation(user.Id, model.OrganizationUsers)); - } - - if (model.WebAuthnKeys.Any()) - { - saveEncryptedDataActions.Add(_credentialRepository.UpdateKeysForRotationAsync(user.Id, model.WebAuthnKeys)); - } - - await _userRepository.UpdateUserKeyAndEncryptedDataAsync(user, saveEncryptedDataActions); - } - else - { - await _userRepository.ReplaceAsync(user); - } - - await _pushService.PushLogOutAsync(user.Id, excludeCurrentContextFromPush: true); - return IdentityResult.Success; - } -} diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index 581a7e8f04..64261ede82 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -1,27 +1,16 @@ using System.Security.Claims; -using Bit.Api.AdminConsole.Models.Request.Organizations; using Bit.Api.Auth.Controllers; -using Bit.Api.Auth.Models.Request; using Bit.Api.Auth.Models.Request.Accounts; -using Bit.Api.Auth.Models.Request.WebAuthn; -using Bit.Api.KeyManagement.Validators; -using Bit.Api.Tools.Models.Request; -using Bit.Api.Vault.Models.Request; using Bit.Core.AdminConsole.Repositories; using Bit.Core.AdminConsole.Services; -using Bit.Core.Auth.Entities; using Bit.Core.Auth.Models.Api.Request.Accounts; -using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.UserFeatures.TdeOffboardingPassword.Interfaces; using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; using Bit.Core.Entities; using Bit.Core.Exceptions; -using Bit.Core.KeyManagement.UserKey; using Bit.Core.Repositories; using Bit.Core.Services; -using Bit.Core.Tools.Entities; -using Bit.Core.Vault.Entities; using Bit.Test.Common.AutoFixture.Attributes; using Microsoft.AspNetCore.Identity; using NSubstitute; @@ -39,23 +28,10 @@ public class AccountsControllerTests : IDisposable private readonly IProviderUserRepository _providerUserRepository; private readonly IPolicyService _policyService; private readonly ISetInitialMasterPasswordCommand _setInitialMasterPasswordCommand; - private readonly IRotateUserKeyCommand _rotateUserKeyCommand; private readonly ITwoFactorIsEnabledQuery _twoFactorIsEnabledQuery; private readonly ITdeOffboardingPasswordCommand _tdeOffboardingPasswordCommand; private readonly IFeatureService _featureService; - private readonly IRotationValidator, IEnumerable> _cipherValidator; - private readonly IRotationValidator, IEnumerable> _folderValidator; - private readonly IRotationValidator, IReadOnlyList> _sendValidator; - private readonly IRotationValidator, IEnumerable> - _emergencyAccessValidator; - private readonly IRotationValidator, - IReadOnlyList> - _resetPasswordValidator; - private readonly IRotationValidator, IEnumerable> - _webauthnKeyRotationValidator; - - public AccountsControllerTests() { _userService = Substitute.For(); @@ -64,21 +40,9 @@ public class AccountsControllerTests : IDisposable _providerUserRepository = Substitute.For(); _policyService = Substitute.For(); _setInitialMasterPasswordCommand = Substitute.For(); - _rotateUserKeyCommand = Substitute.For(); _twoFactorIsEnabledQuery = Substitute.For(); _tdeOffboardingPasswordCommand = Substitute.For(); _featureService = Substitute.For(); - _cipherValidator = - Substitute.For, IEnumerable>>(); - _folderValidator = - Substitute.For, IEnumerable>>(); - _sendValidator = Substitute.For, IReadOnlyList>>(); - _emergencyAccessValidator = Substitute.For, - IEnumerable>>(); - _webauthnKeyRotationValidator = Substitute.For, IEnumerable>>(); - _resetPasswordValidator = Substitute - .For, - IReadOnlyList>>(); _sut = new AccountsController( _organizationService, @@ -88,15 +52,8 @@ public class AccountsControllerTests : IDisposable _policyService, _setInitialMasterPasswordCommand, _tdeOffboardingPasswordCommand, - _rotateUserKeyCommand, _twoFactorIsEnabledQuery, - _featureService, - _cipherValidator, - _folderValidator, - _sendValidator, - _emergencyAccessValidator, - _resetPasswordValidator, - _webauthnKeyRotationValidator + _featureService ); } diff --git a/test/Core.Test/KeyManagement/UserKey/RotateUserKeyCommandTests.cs b/test/Core.Test/KeyManagement/UserKey/RotateUserKeyCommandTests.cs deleted file mode 100644 index 000fa7e90c..0000000000 --- a/test/Core.Test/KeyManagement/UserKey/RotateUserKeyCommandTests.cs +++ /dev/null @@ -1,86 +0,0 @@ -using Bit.Core.Auth.Entities; -using Bit.Core.Auth.Repositories; -using Bit.Core.Entities; -using Bit.Core.KeyManagement.Models.Data; -using Bit.Core.KeyManagement.UserKey.Implementations; -using Bit.Core.Platform.Push; -using Bit.Core.Services; -using Bit.Test.Common.AutoFixture; -using Bit.Test.Common.AutoFixture.Attributes; -using Microsoft.AspNetCore.Identity; -using NSubstitute; -using Xunit; - -namespace Bit.Core.Test.KeyManagement.UserKey; - -[SutProviderCustomize] -public class RotateUserKeyCommandTests -{ - [Theory, BitAutoData] - public async Task RotateUserKeyAsync_Success(SutProvider sutProvider, User user, - RotateUserKeyData model) - { - sutProvider.GetDependency().CheckPasswordAsync(user, model.MasterPasswordHash) - .Returns(true); - foreach (var webauthnCred in model.WebAuthnKeys) - { - var dbWebauthnCred = new WebAuthnCredential - { - EncryptedPublicKey = "encryptedPublicKey", - EncryptedUserKey = "encryptedUserKey" - }; - sutProvider.GetDependency().GetByIdAsync(webauthnCred.Id, user.Id) - .Returns(dbWebauthnCred); - } - - var result = await sutProvider.Sut.RotateUserKeyAsync(user, model); - - Assert.Equal(IdentityResult.Success, result); - } - - [Theory, BitAutoData] - public async Task RotateUserKeyAsync_InvalidMasterPasswordHash_ReturnsFailedIdentityResult( - SutProvider sutProvider, User user, RotateUserKeyData model) - { - sutProvider.GetDependency().CheckPasswordAsync(user, model.MasterPasswordHash) - .Returns(false); - foreach (var webauthnCred in model.WebAuthnKeys) - { - var dbWebauthnCred = new WebAuthnCredential - { - EncryptedPublicKey = "encryptedPublicKey", - EncryptedUserKey = "encryptedUserKey" - }; - sutProvider.GetDependency().GetByIdAsync(webauthnCred.Id, user.Id) - .Returns(dbWebauthnCred); - } - - var result = await sutProvider.Sut.RotateUserKeyAsync(user, model); - - Assert.False(result.Succeeded); - } - - [Theory, BitAutoData] - public async Task RotateUserKeyAsync_LogsOutUser( - SutProvider sutProvider, User user, RotateUserKeyData model) - { - sutProvider.GetDependency().CheckPasswordAsync(user, model.MasterPasswordHash) - .Returns(true); - foreach (var webauthnCred in model.WebAuthnKeys) - { - var dbWebauthnCred = new WebAuthnCredential - { - EncryptedPublicKey = "encryptedPublicKey", - EncryptedUserKey = "encryptedUserKey" - }; - sutProvider.GetDependency().GetByIdAsync(webauthnCred.Id, user.Id) - .Returns(dbWebauthnCred); - } - - await sutProvider.Sut.RotateUserKeyAsync(user, model); - - await sutProvider.GetDependency().ReceivedWithAnyArgs() - .PushLogOutAsync(default, default); - } - -}