From 8fd48374dc4942fba8c8c0cd9525776a503eb8aa Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Thu, 3 Apr 2025 11:30:49 +0200 Subject: [PATCH] [PM-2199] Implement userkey rotation for all TDE devices (#5446) * Implement userkey rotation v2 * Update request models * Cleanup * Update tests * Improve test * Add tests * Fix formatting * Fix test * Remove whitespace * Fix namespace * Enable nullable on models * Fix build * Add tests and enable nullable on masterpasswordunlockdatamodel * Fix test * Remove rollback * Add tests * Make masterpassword hint optional * Update user query * Add EF test * Improve test * Cleanup * Set masterpassword hint * Remove connection close * Add tests for invalid kdf types * Update test/Core.Test/KeyManagement/UserKey/RotateUserAccountKeysCommandTests.cs Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Fix formatting * Update src/Api/KeyManagement/Models/Requests/RotateAccountKeysAndDataRequestModel.cs Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update src/Api/Auth/Models/Request/Accounts/MasterPasswordUnlockDataModel.cs Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update src/Api/Auth/Models/Request/Accounts/MasterPasswordUnlockDataModel.cs Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Update src/Api/KeyManagement/Models/Requests/AccountKeysRequestModel.cs Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> * Fix imports * Fix tests * Add poc for tde rotation * Improve rotation transaction safety * Add validator tests * Clean up validator * Add newline * Add devicekey unlock data to integration test * Fix tests * Fix tests * Remove null check * Remove null check * Fix IsTrusted returning wrong result * Add rollback * Cleanup * Address feedback * Further renames --------- Co-authored-by: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> --- src/Api/Controllers/DevicesController.cs | 10 +--- .../AccountsKeyManagementController.cs | 7 ++- .../Models/Requests/UnlockDataRequestModel.cs | 2 + .../Validators/DeviceRotationValidator.cs | 53 +++++++++++++++++++ src/Api/Startup.cs | 5 +- .../Request/DeviceKeysUpdateRequestModel.cs | 8 +++ .../DeviceAuthRequestResponseModel.cs | 3 +- .../Models/Data/RotateUserAccountKeysData.cs | 1 + .../RotateUserAccountkeysCommand.cs | 8 +++ src/Core/Repositories/IDeviceRepository.cs | 2 + .../Repositories/DeviceRepository.cs | 33 ++++++++++++ .../Repositories/DeviceRepository.cs | 27 ++++++++++ .../AccountsKeyManagementControllerTests.cs | 4 ++ .../DeviceRotationValidatorTests.cs | 49 +++++++++++++++++ 14 files changed, 199 insertions(+), 13 deletions(-) create mode 100644 src/Api/KeyManagement/Validators/DeviceRotationValidator.cs create mode 100644 test/Api.Test/KeyManagement/Validators/DeviceRotationValidatorTests.cs diff --git a/src/Api/Controllers/DevicesController.cs b/src/Api/Controllers/DevicesController.cs index 02eb2d36d5..4e21b5e9dc 100644 --- a/src/Api/Controllers/DevicesController.cs +++ b/src/Api/Controllers/DevicesController.cs @@ -1,6 +1,5 @@ using System.ComponentModel.DataAnnotations; using Bit.Api.Auth.Models.Request; -using Bit.Api.Auth.Models.Request.Accounts; using Bit.Api.Models.Request; using Bit.Api.Models.Response; using Bit.Core.Auth.Models.Api.Request; @@ -125,7 +124,7 @@ public class DevicesController : Controller } [HttpPost("{identifier}/retrieve-keys")] - public async Task GetDeviceKeys(string identifier, [FromBody] SecretVerificationRequestModel model) + public async Task GetDeviceKeys(string identifier) { var user = await _userService.GetUserByPrincipalAsync(User); @@ -134,14 +133,7 @@ public class DevicesController : Controller throw new UnauthorizedAccessException(); } - if (!await _userService.VerifySecretAsync(user, model.Secret)) - { - await Task.Delay(2000); - throw new BadRequestException(string.Empty, "User verification failed."); - } - var device = await _deviceRepository.GetByIdentifierAsync(identifier, user.Id); - if (device == null) { throw new NotFoundException(); diff --git a/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs b/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs index 85e0981f22..0764e2ee28 100644 --- a/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs +++ b/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs @@ -8,6 +8,7 @@ using Bit.Api.Tools.Models.Request; using Bit.Api.Vault.Models.Request; using Bit.Core; using Bit.Core.Auth.Entities; +using Bit.Core.Auth.Models.Api.Request; using Bit.Core.Auth.Models.Data; using Bit.Core.Entities; using Bit.Core.Exceptions; @@ -43,6 +44,7 @@ public class AccountsKeyManagementController : Controller _organizationUserValidator; private readonly IRotationValidator, IEnumerable> _webauthnKeyValidator; + private readonly IRotationValidator, IEnumerable> _deviceValidator; public AccountsKeyManagementController(IUserService userService, IFeatureService featureService, @@ -57,7 +59,8 @@ public class AccountsKeyManagementController : Controller emergencyAccessValidator, IRotationValidator, IReadOnlyList> organizationUserValidator, - IRotationValidator, IEnumerable> webAuthnKeyValidator) + IRotationValidator, IEnumerable> webAuthnKeyValidator, + IRotationValidator, IEnumerable> deviceValidator) { _userService = userService; _featureService = featureService; @@ -71,6 +74,7 @@ public class AccountsKeyManagementController : Controller _emergencyAccessValidator = emergencyAccessValidator; _organizationUserValidator = organizationUserValidator; _webauthnKeyValidator = webAuthnKeyValidator; + _deviceValidator = deviceValidator; } [HttpPost("regenerate-keys")] @@ -109,6 +113,7 @@ public class AccountsKeyManagementController : Controller EmergencyAccesses = await _emergencyAccessValidator.ValidateAsync(user, model.AccountUnlockData.EmergencyAccessUnlockData), OrganizationUsers = await _organizationUserValidator.ValidateAsync(user, model.AccountUnlockData.OrganizationAccountRecoveryUnlockData), WebAuthnKeys = await _webauthnKeyValidator.ValidateAsync(user, model.AccountUnlockData.PasskeyUnlockData), + DeviceKeys = await _deviceValidator.ValidateAsync(user, model.AccountUnlockData.DeviceKeyUnlockData), Ciphers = await _cipherValidator.ValidateAsync(user, model.AccountData.Ciphers), Folders = await _folderValidator.ValidateAsync(user, model.AccountData.Folders), diff --git a/src/Api/KeyManagement/Models/Requests/UnlockDataRequestModel.cs b/src/Api/KeyManagement/Models/Requests/UnlockDataRequestModel.cs index 5156e2a655..23c3eb95d0 100644 --- a/src/Api/KeyManagement/Models/Requests/UnlockDataRequestModel.cs +++ b/src/Api/KeyManagement/Models/Requests/UnlockDataRequestModel.cs @@ -3,6 +3,7 @@ using Bit.Api.AdminConsole.Models.Request.Organizations; using Bit.Api.Auth.Models.Request; using Bit.Api.Auth.Models.Request.Accounts; using Bit.Api.Auth.Models.Request.WebAuthn; +using Bit.Core.Auth.Models.Api.Request; namespace Bit.Api.KeyManagement.Models.Requests; @@ -13,4 +14,5 @@ public class UnlockDataRequestModel public required IEnumerable EmergencyAccessUnlockData { get; set; } public required IEnumerable OrganizationAccountRecoveryUnlockData { get; set; } public required IEnumerable PasskeyUnlockData { get; set; } + public required IEnumerable DeviceKeyUnlockData { get; set; } } diff --git a/src/Api/KeyManagement/Validators/DeviceRotationValidator.cs b/src/Api/KeyManagement/Validators/DeviceRotationValidator.cs new file mode 100644 index 0000000000..cbaf508766 --- /dev/null +++ b/src/Api/KeyManagement/Validators/DeviceRotationValidator.cs @@ -0,0 +1,53 @@ +using Bit.Core.Auth.Models.Api.Request; +using Bit.Core.Auth.Utilities; +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; + +namespace Bit.Api.KeyManagement.Validators; + +/// +/// Device implementation for +/// +public class DeviceRotationValidator : IRotationValidator, IEnumerable> +{ + private readonly IDeviceRepository _deviceRepository; + + /// + /// Instantiates a new + /// + /// Retrieves all user s + public DeviceRotationValidator(IDeviceRepository deviceRepository) + { + _deviceRepository = deviceRepository; + } + + public async Task> ValidateAsync(User user, IEnumerable devices) + { + var result = new List(); + + var existingTrustedDevices = (await _deviceRepository.GetManyByUserIdAsync(user.Id)).Where(d => d.IsTrusted()).ToList(); + if (existingTrustedDevices.Count == 0) + { + return result; + } + + foreach (var existing in existingTrustedDevices) + { + var device = devices.FirstOrDefault(c => c.DeviceId == existing.Id); + if (device == null) + { + throw new BadRequestException("All existing trusted devices must be included in the rotation."); + } + + if (device.EncryptedUserKey == null || device.EncryptedPublicKey == null) + { + throw new BadRequestException("Rotated encryption keys must be provided for all devices that are trusted."); + } + + result.Add(device.ToDevice(existing)); + } + + return result; + } +} diff --git a/src/Api/Startup.cs b/src/Api/Startup.cs index 5849bfb634..deac7bf0c9 100644 --- a/src/Api/Startup.cs +++ b/src/Api/Startup.cs @@ -31,7 +31,7 @@ using Bit.Core.Auth.Models.Data; using Bit.Core.Auth.Identity.TokenProviders; using Bit.Core.Tools.ImportFeatures; using Bit.Core.Tools.ReportFeatures; - +using Bit.Core.Auth.Models.Api.Request; #if !OSS using Bit.Commercial.Core.SecretsManager; @@ -168,6 +168,9 @@ public class Startup services .AddScoped, IEnumerable>, WebAuthnLoginKeyRotationValidator>(); + services + .AddScoped, IEnumerable>, + DeviceRotationValidator>(); // Services services.AddBaseServices(globalSettings); diff --git a/src/Core/Auth/Models/Api/Request/DeviceKeysUpdateRequestModel.cs b/src/Core/Auth/Models/Api/Request/DeviceKeysUpdateRequestModel.cs index 2b815afd16..111b03a3a3 100644 --- a/src/Core/Auth/Models/Api/Request/DeviceKeysUpdateRequestModel.cs +++ b/src/Core/Auth/Models/Api/Request/DeviceKeysUpdateRequestModel.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using Bit.Core.Entities; using Bit.Core.Utilities; namespace Bit.Core.Auth.Models.Api.Request; @@ -7,6 +8,13 @@ public class OtherDeviceKeysUpdateRequestModel : DeviceKeysUpdateRequestModel { [Required] public Guid DeviceId { get; set; } + + public Device ToDevice(Device existingDevice) + { + existingDevice.EncryptedPublicKey = EncryptedPublicKey; + existingDevice.EncryptedUserKey = EncryptedUserKey; + return existingDevice; + } } public class DeviceKeysUpdateRequestModel diff --git a/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs b/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs index 3cfea51ee3..59630a6d2c 100644 --- a/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs +++ b/src/Core/Auth/Models/Api/Response/DeviceAuthRequestResponseModel.cs @@ -1,5 +1,4 @@ using Bit.Core.Auth.Models.Data; -using Bit.Core.Auth.Utilities; using Bit.Core.Enums; using Bit.Core.Models.Api; @@ -19,7 +18,7 @@ public class DeviceAuthRequestResponseModel : ResponseModel Type = deviceAuthDetails.Type, Identifier = deviceAuthDetails.Identifier, CreationDate = deviceAuthDetails.CreationDate, - IsTrusted = deviceAuthDetails.IsTrusted() + IsTrusted = deviceAuthDetails.IsTrusted, }; if (deviceAuthDetails.AuthRequestId != null && deviceAuthDetails.AuthRequestCreatedAt != null) diff --git a/src/Core/KeyManagement/Models/Data/RotateUserAccountKeysData.cs b/src/Core/KeyManagement/Models/Data/RotateUserAccountKeysData.cs index 7cb1c273a3..f81baf6fab 100644 --- a/src/Core/KeyManagement/Models/Data/RotateUserAccountKeysData.cs +++ b/src/Core/KeyManagement/Models/Data/RotateUserAccountKeysData.cs @@ -20,6 +20,7 @@ public class RotateUserAccountKeysData public IEnumerable EmergencyAccesses { get; set; } public IReadOnlyList OrganizationUsers { get; set; } public IEnumerable WebAuthnKeys { get; set; } + public IEnumerable DeviceKeys { get; set; } // User vault data encrypted by the userkey public IEnumerable Ciphers { get; set; } diff --git a/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountkeysCommand.cs b/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountkeysCommand.cs index f4dcf31d5c..6967c9bf85 100644 --- a/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountkeysCommand.cs +++ b/src/Core/KeyManagement/UserKey/Implementations/RotateUserAccountkeysCommand.cs @@ -20,6 +20,7 @@ public class RotateUserAccountKeysCommand : IRotateUserAccountKeysCommand private readonly ISendRepository _sendRepository; private readonly IEmergencyAccessRepository _emergencyAccessRepository; private readonly IOrganizationUserRepository _organizationUserRepository; + private readonly IDeviceRepository _deviceRepository; private readonly IPushNotificationService _pushService; private readonly IdentityErrorDescriber _identityErrorDescriber; private readonly IWebAuthnCredentialRepository _credentialRepository; @@ -42,6 +43,7 @@ public class RotateUserAccountKeysCommand : IRotateUserAccountKeysCommand public RotateUserAccountKeysCommand(IUserService userService, IUserRepository userRepository, ICipherRepository cipherRepository, IFolderRepository folderRepository, ISendRepository sendRepository, IEmergencyAccessRepository emergencyAccessRepository, IOrganizationUserRepository organizationUserRepository, + IDeviceRepository deviceRepository, IPasswordHasher passwordHasher, IPushNotificationService pushService, IdentityErrorDescriber errors, IWebAuthnCredentialRepository credentialRepository) { @@ -52,6 +54,7 @@ public class RotateUserAccountKeysCommand : IRotateUserAccountKeysCommand _sendRepository = sendRepository; _emergencyAccessRepository = emergencyAccessRepository; _organizationUserRepository = organizationUserRepository; + _deviceRepository = deviceRepository; _pushService = pushService; _identityErrorDescriber = errors; _credentialRepository = credentialRepository; @@ -127,6 +130,11 @@ public class RotateUserAccountKeysCommand : IRotateUserAccountKeysCommand saveEncryptedDataActions.Add(_credentialRepository.UpdateKeysForRotationAsync(user.Id, model.WebAuthnKeys)); } + if (model.DeviceKeys.Any()) + { + saveEncryptedDataActions.Add(_deviceRepository.UpdateKeysForRotationAsync(user.Id, model.DeviceKeys)); + } + await _userRepository.UpdateUserKeyAndEncryptedDataV2Async(user, saveEncryptedDataActions); await _pushService.PushLogOutAsync(user.Id); return IdentityResult.Success; diff --git a/src/Core/Repositories/IDeviceRepository.cs b/src/Core/Repositories/IDeviceRepository.cs index c9809c1de6..fc2f1556b7 100644 --- a/src/Core/Repositories/IDeviceRepository.cs +++ b/src/Core/Repositories/IDeviceRepository.cs @@ -1,5 +1,6 @@ using Bit.Core.Auth.Models.Data; using Bit.Core.Entities; +using Bit.Core.KeyManagement.UserKey; #nullable enable @@ -16,4 +17,5 @@ public interface IDeviceRepository : IRepository // other requests. Task> GetManyByUserIdWithDeviceAuth(Guid userId); Task ClearPushTokenAsync(Guid id); + UpdateEncryptedDataForKeyRotation UpdateKeysForRotationAsync(Guid userId, IEnumerable devices); } diff --git a/src/Infrastructure.Dapper/Repositories/DeviceRepository.cs b/src/Infrastructure.Dapper/Repositories/DeviceRepository.cs index 4abf4a4649..723200ff1c 100644 --- a/src/Infrastructure.Dapper/Repositories/DeviceRepository.cs +++ b/src/Infrastructure.Dapper/Repositories/DeviceRepository.cs @@ -1,8 +1,10 @@ using System.Data; using Bit.Core.Auth.Models.Data; using Bit.Core.Entities; +using Bit.Core.KeyManagement.UserKey; using Bit.Core.Repositories; using Bit.Core.Settings; +using Bit.Core.Utilities; using Dapper; using Microsoft.Data.SqlClient; @@ -109,4 +111,35 @@ public class DeviceRepository : Repository, IDeviceRepository commandType: CommandType.StoredProcedure); } } + + public UpdateEncryptedDataForKeyRotation UpdateKeysForRotationAsync(Guid userId, IEnumerable devices) + { + return async (SqlConnection connection, SqlTransaction transaction) => + { + const string sql = @" + UPDATE D + SET + D.[EncryptedPublicKey] = UD.[encryptedPublicKey], + D.[EncryptedUserKey] = UD.[encryptedUserKey] + FROM + [dbo].[Device] D + INNER JOIN + OPENJSON(@DeviceCredentials) + WITH ( + id UNIQUEIDENTIFIER, + encryptedPublicKey NVARCHAR(MAX), + encryptedUserKey NVARCHAR(MAX) + ) UD + ON UD.[id] = D.[Id] + WHERE + D.[UserId] = @UserId"; + var deviceCredentials = CoreHelpers.ClassToJsonData(devices); + + await connection.ExecuteAsync( + sql, + new { UserId = userId, DeviceCredentials = deviceCredentials }, + transaction: transaction, + commandType: CommandType.Text); + }; + } } diff --git a/src/Infrastructure.EntityFramework/Repositories/DeviceRepository.cs b/src/Infrastructure.EntityFramework/Repositories/DeviceRepository.cs index ad31d0fb8b..19f38c6098 100644 --- a/src/Infrastructure.EntityFramework/Repositories/DeviceRepository.cs +++ b/src/Infrastructure.EntityFramework/Repositories/DeviceRepository.cs @@ -1,5 +1,6 @@ using AutoMapper; using Bit.Core.Auth.Models.Data; +using Bit.Core.KeyManagement.UserKey; using Bit.Core.Repositories; using Bit.Core.Settings; using Bit.Infrastructure.EntityFramework.Auth.Repositories.Queries; @@ -91,4 +92,30 @@ public class DeviceRepository : Repository, return await query.GetQuery(dbContext, userId, expirationMinutes).ToListAsync(); } } + + public UpdateEncryptedDataForKeyRotation UpdateKeysForRotationAsync(Guid userId, IEnumerable devices) + { + return async (_, _) => + { + var deviceUpdates = devices.ToList(); + using var scope = ServiceScopeFactory.CreateScope(); + var dbContext = GetDatabaseContext(scope); + var userDevices = await GetDbSet(dbContext) + .Where(device => device.UserId == userId) + .ToListAsync(); + var userDevicesWithUpdatesPending = userDevices + .Where(existingDevice => deviceUpdates.Any(updatedDevice => updatedDevice.Id == existingDevice.Id)) + .ToList(); + + foreach (var deviceToUpdate in userDevicesWithUpdatesPending) + { + var deviceUpdate = deviceUpdates.First(deviceUpdate => deviceUpdate.Id == deviceToUpdate.Id); + deviceToUpdate.EncryptedPublicKey = deviceUpdate.EncryptedPublicKey; + deviceToUpdate.EncryptedUserKey = deviceUpdate.EncryptedUserKey; + } + + await dbContext.SaveChangesAsync(); + }; + } + } diff --git a/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs b/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs index 7c05e1d680..1b065adbd6 100644 --- a/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs +++ b/test/Api.IntegrationTest/KeyManagement/Controllers/AccountsKeyManagementControllerTests.cs @@ -29,6 +29,7 @@ public class AccountsKeyManagementControllerTests : IClassFixture _passwordHasher; private string _ownerEmail = null!; @@ -40,6 +41,7 @@ public class AccountsKeyManagementControllerTests : IClassFixture(); + _deviceRepository = _factory.GetService(); _emergencyAccessRepository = _factory.GetService(); _organizationUserRepository = _factory.GetService(); _passwordHasher = _factory.GetService>(); @@ -238,10 +240,12 @@ public class AccountsKeyManagementControllerTests : IClassFixture sutProvider, User user, IEnumerable devices) + { + var userCiphers = devices.Select(c => new Device { Id = c.DeviceId, EncryptedPrivateKey = "EncryptedPrivateKey", EncryptedPublicKey = "EncryptedPublicKey", EncryptedUserKey = "EncryptedUserKey" }).ToList(); + sutProvider.GetDependency().GetManyByUserIdAsync(user.Id) + .Returns(userCiphers); + await Assert.ThrowsAsync(async () => await sutProvider.Sut.ValidateAsync(user, Enumerable.Empty())); + } + + [Theory, BitAutoData] + public async Task ValidateAsync_SentDevicesTrustedButDatabaseUntrusted_Throws( + SutProvider sutProvider, User user, IEnumerable devices) + { + var userCiphers = devices.Select(c => new Device { Id = c.DeviceId, EncryptedPrivateKey = "Key", EncryptedPublicKey = "Key", EncryptedUserKey = "Key" }).ToList(); + sutProvider.GetDependency().GetManyByUserIdAsync(user.Id) + .Returns(userCiphers); + await Assert.ThrowsAsync(async () => await sutProvider.Sut.ValidateAsync(user, [ + new OtherDeviceKeysUpdateRequestModel { DeviceId = userCiphers.First().Id, EncryptedPublicKey = null, EncryptedUserKey = null } + ])); + } + + [Theory, BitAutoData] + public async Task ValidateAsync_Validates( + SutProvider sutProvider, User user, IEnumerable devices) + { + var userCiphers = devices.Select(c => new Device { Id = c.DeviceId, EncryptedPrivateKey = "Key", EncryptedPublicKey = "Key", EncryptedUserKey = "Key" }).ToList().Slice(0, 1); + sutProvider.GetDependency().GetManyByUserIdAsync(user.Id) + .Returns(userCiphers); + Assert.NotEmpty(await sutProvider.Sut.ValidateAsync(user, [ + new OtherDeviceKeysUpdateRequestModel { DeviceId = userCiphers.First().Id, EncryptedPublicKey = "Key", EncryptedUserKey = "Key" } + ])); + } +}