From ce185eb3dfb759a1366b84de63b6fbdca6563481 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 23 Jul 2024 20:53:08 +0200 Subject: [PATCH] [PM-5963] Fix tde offboarding vault corruption (#4144) * Attempt to fix tde to mp flow * Move tde offboarding to dedicated flag * Add tde offboarding password request * Validate tde offboarding input * Correctly check whether tde is active when building trusted device options * Refactor Tde offboarding into a separate command * Add unit tests for tde offboarding * Update tde offboarding request model * Fix tests * Fix further tests * Fix documentation * Add validation for updatetdepasswordasync key/newmasterpassword * Add comment explaining test * Remove unrelated changes --- .../Auth/Controllers/AccountsController.cs | 27 +++++ ...pdateTdeOffboardingPasswordRequestModel.cs | 14 +++ .../Api/Response/UserDecryptionOptions.cs | 3 + .../ITdeOffboardingPasswordCommand.cs | 14 +++ .../TdeOffboardingPasswordCommand.cs | 99 +++++++++++++++++++ .../UserServiceCollectionExtensions.cs | 7 ++ src/Core/Enums/EventType.cs | 1 + .../UserDecryptionOptionsBuilder.cs | 6 +- .../Controllers/AccountsControllerTests.cs | 4 + .../TdeOffboardingPasswordCommandTests.cs | 99 +++++++++++++++++++ .../Endpoints/IdentityServerSsoTests.cs | 11 +++ 11 files changed, 283 insertions(+), 2 deletions(-) create mode 100644 src/Api/Auth/Models/Request/Accounts/UpdateTdeOffboardingPasswordRequestModel.cs create mode 100644 src/Core/Auth/UserFeatures/TdeOffboardingPassword/Interfaces/ITdeOffboardingPasswordCommand.cs create mode 100644 src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs create mode 100644 test/Core.Test/Auth/UserFeatures/TdeOffboarding/TdeOffboardingPasswordCommandTests.cs diff --git a/src/Api/Auth/Controllers/AccountsController.cs b/src/Api/Auth/Controllers/AccountsController.cs index bb7a3d0a6..142617466 100644 --- a/src/Api/Auth/Controllers/AccountsController.cs +++ b/src/Api/Auth/Controllers/AccountsController.cs @@ -17,6 +17,7 @@ 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.UserKey; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; using Bit.Core.Billing.Models; @@ -53,6 +54,7 @@ public class AccountsController : Controller private readonly IUserService _userService; private readonly IPolicyService _policyService; private readonly ISetInitialMasterPasswordCommand _setInitialMasterPasswordCommand; + private readonly ITdeOffboardingPasswordCommand _tdeOffboardingPasswordCommand; private readonly IRotateUserKeyCommand _rotateUserKeyCommand; private readonly IFeatureService _featureService; private readonly ISubscriberService _subscriberService; @@ -83,6 +85,7 @@ public class AccountsController : Controller IUserService userService, IPolicyService policyService, ISetInitialMasterPasswordCommand setInitialMasterPasswordCommand, + ITdeOffboardingPasswordCommand tdeOffboardingPasswordCommand, IRotateUserKeyCommand rotateUserKeyCommand, IFeatureService featureService, ISubscriberService subscriberService, @@ -106,6 +109,7 @@ public class AccountsController : Controller _userService = userService; _policyService = policyService; _setInitialMasterPasswordCommand = setInitialMasterPasswordCommand; + _tdeOffboardingPasswordCommand = tdeOffboardingPasswordCommand; _rotateUserKeyCommand = rotateUserKeyCommand; _featureService = featureService; _subscriberService = subscriberService; @@ -877,6 +881,29 @@ public class AccountsController : Controller throw new BadRequestException(ModelState); } + [HttpPut("update-tde-offboarding-password")] + public async Task PutUpdateTdePasswordAsync([FromBody] UpdateTdeOffboardingPasswordRequestModel model) + { + var user = await _userService.GetUserByPrincipalAsync(User); + if (user == null) + { + throw new UnauthorizedAccessException(); + } + + var result = await _tdeOffboardingPasswordCommand.UpdateTdeOffboardingPasswordAsync(user, model.NewMasterPasswordHash, model.Key, model.MasterPasswordHint); + if (result.Succeeded) + { + return; + } + + foreach (var error in result.Errors) + { + ModelState.AddModelError(string.Empty, error.Description); + } + + throw new BadRequestException(ModelState); + } + [HttpPost("request-otp")] public async Task PostRequestOTP() { diff --git a/src/Api/Auth/Models/Request/Accounts/UpdateTdeOffboardingPasswordRequestModel.cs b/src/Api/Auth/Models/Request/Accounts/UpdateTdeOffboardingPasswordRequestModel.cs new file mode 100644 index 000000000..e246a99c9 --- /dev/null +++ b/src/Api/Auth/Models/Request/Accounts/UpdateTdeOffboardingPasswordRequestModel.cs @@ -0,0 +1,14 @@ +using System.ComponentModel.DataAnnotations; + +namespace Bit.Api.Auth.Models.Request.Accounts; + +public class UpdateTdeOffboardingPasswordRequestModel +{ + [Required] + [StringLength(300)] + public string NewMasterPasswordHash { get; set; } + [Required] + public string Key { get; set; } + [StringLength(50)] + public string MasterPasswordHint { get; set; } +} diff --git a/src/Core/Auth/Models/Api/Response/UserDecryptionOptions.cs b/src/Core/Auth/Models/Api/Response/UserDecryptionOptions.cs index 06990afea..b5f2b77cf 100644 --- a/src/Core/Auth/Models/Api/Response/UserDecryptionOptions.cs +++ b/src/Core/Auth/Models/Api/Response/UserDecryptionOptions.cs @@ -54,18 +54,21 @@ public class TrustedDeviceUserDecryptionOption public bool HasAdminApproval { get; } public bool HasLoginApprovingDevice { get; } public bool HasManageResetPasswordPermission { get; } + public bool IsTdeOffboarding { get; } public string? EncryptedPrivateKey { get; } public string? EncryptedUserKey { get; } public TrustedDeviceUserDecryptionOption(bool hasAdminApproval, bool hasLoginApprovingDevice, bool hasManageResetPasswordPermission, + bool isTdeOffboarding, string? encryptedPrivateKey, string? encryptedUserKey) { HasAdminApproval = hasAdminApproval; HasLoginApprovingDevice = hasLoginApprovingDevice; HasManageResetPasswordPermission = hasManageResetPasswordPermission; + IsTdeOffboarding = isTdeOffboarding; EncryptedPrivateKey = encryptedPrivateKey; EncryptedUserKey = encryptedUserKey; } diff --git a/src/Core/Auth/UserFeatures/TdeOffboardingPassword/Interfaces/ITdeOffboardingPasswordCommand.cs b/src/Core/Auth/UserFeatures/TdeOffboardingPassword/Interfaces/ITdeOffboardingPasswordCommand.cs new file mode 100644 index 000000000..1ff64ffab --- /dev/null +++ b/src/Core/Auth/UserFeatures/TdeOffboardingPassword/Interfaces/ITdeOffboardingPasswordCommand.cs @@ -0,0 +1,14 @@ +using Bit.Core.Entities; +using Microsoft.AspNetCore.Identity; + +namespace Bit.Core.Auth.UserFeatures.TdeOffboardingPassword.Interfaces; + +/// +/// Manages the setting of the master password for JIT provisioned TDE in an organization, after the organization disabled TDE. +/// This command is invoked, when the user first logs in after the organization has switched from TDE to master password based decryption. +/// +public interface ITdeOffboardingPasswordCommand +{ + public Task UpdateTdeOffboardingPasswordAsync(User user, string masterPassword, string key, + string orgSsoIdentifier); +} diff --git a/src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs b/src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs new file mode 100644 index 000000000..d33db18e4 --- /dev/null +++ b/src/Core/Auth/UserFeatures/TdeOffboardingPassword/TdeOffboardingPasswordCommand.cs @@ -0,0 +1,99 @@ +using Bit.Core.Auth.Repositories; +using Bit.Core.Auth.UserFeatures.TdeOffboardingPassword.Interfaces; +using Bit.Core.Entities; +using Bit.Core.Enums; +using Bit.Core.Exceptions; +using Bit.Core.Repositories; +using Bit.Core.Services; +using Microsoft.AspNetCore.Identity; + +namespace Bit.Core.Auth.UserFeatures.UserMasterPassword; + +public class TdeOffboardingPasswordCommand : ITdeOffboardingPasswordCommand +{ + private readonly IUserService _userService; + private readonly IUserRepository _userRepository; + private readonly IEventService _eventService; + private readonly IOrganizationUserRepository _organizationUserRepository; + private readonly ISsoUserRepository _ssoUserRepository; + private readonly ISsoConfigRepository _ssoConfigRepository; + private readonly IPushNotificationService _pushService; + + + public TdeOffboardingPasswordCommand( + IUserService userService, + IUserRepository userRepository, + IEventService eventService, + IOrganizationUserRepository organizationUserRepository, + ISsoUserRepository ssoUserRepository, + ISsoConfigRepository ssoConfigRepository, + IPushNotificationService pushService) + { + _userService = userService; + _userRepository = userRepository; + _eventService = eventService; + _organizationUserRepository = organizationUserRepository; + _ssoUserRepository = ssoUserRepository; + _ssoConfigRepository = ssoConfigRepository; + _pushService = pushService; + } + + public async Task UpdateTdeOffboardingPasswordAsync(User user, string newMasterPassword, string key, string hint) + { + if (string.IsNullOrWhiteSpace(newMasterPassword)) + { + throw new BadRequestException("Master password is required."); + } + + if (string.IsNullOrWhiteSpace(key)) + { + throw new BadRequestException("Key is required."); + } + + if (user.HasMasterPassword()) + { + throw new BadRequestException("User already has a master password."); + } + var orgUserDetails = await _organizationUserRepository.GetManyDetailsByUserAsync(user.Id); + orgUserDetails = orgUserDetails.Where(x => x.UseSso).ToList(); + if (orgUserDetails.Count == 0) + { + throw new BadRequestException("User is not part of any organization that has SSO enabled."); + } + + var orgSSOUsers = await Task.WhenAll(orgUserDetails.Select(async x => await _ssoUserRepository.GetByUserIdOrganizationIdAsync(x.OrganizationId, user.Id))); + if (orgSSOUsers.Length != 1) + { + throw new BadRequestException("User is part of no or multiple SSO configurations."); + } + + var orgUser = orgUserDetails.First(); + var orgSSOConfig = await _ssoConfigRepository.GetByOrganizationIdAsync(orgUser.OrganizationId); + if (orgSSOConfig == null) + { + throw new BadRequestException("Organization SSO configuration not found."); + } + else if (orgSSOConfig.GetData().MemberDecryptionType != Enums.MemberDecryptionType.MasterPassword) + { + throw new BadRequestException("Organization SSO Member Decryption Type is not Master Password."); + } + + var result = await _userService.UpdatePasswordHash(user, newMasterPassword); + if (!result.Succeeded) + { + return result; + } + + user.RevisionDate = user.AccountRevisionDate = DateTime.UtcNow; + user.ForcePasswordReset = false; + user.Key = key; + user.MasterPasswordHint = hint; + + await _userRepository.ReplaceAsync(user); + await _eventService.LogUserEventAsync(user.Id, EventType.User_UpdatedTempPassword); + await _pushService.PushLogOutAsync(user.Id); + + return IdentityResult.Success; + } + +} diff --git a/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs b/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs index cbe7b0d4e..15e6f5e44 100644 --- a/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs +++ b/src/Core/Auth/UserFeatures/UserServiceCollectionExtensions.cs @@ -2,6 +2,7 @@ using Bit.Core.Auth.UserFeatures.Registration; using Bit.Core.Auth.UserFeatures.Registration.Implementations; +using Bit.Core.Auth.UserFeatures.TdeOffboardingPassword.Interfaces; using Bit.Core.Auth.UserFeatures.UserKey; using Bit.Core.Auth.UserFeatures.UserKey.Implementations; using Bit.Core.Auth.UserFeatures.UserMasterPassword; @@ -22,6 +23,7 @@ public static class UserServiceCollectionExtensions services.AddUserPasswordCommands(); services.AddUserRegistrationCommands(); services.AddWebAuthnLoginCommands(); + services.AddTdeOffboardingPasswordCommands(); } public static void AddUserKeyCommands(this IServiceCollection services, IGlobalSettings globalSettings) @@ -34,6 +36,11 @@ public static class UserServiceCollectionExtensions services.AddScoped(); } + private static void AddTdeOffboardingPasswordCommands(this IServiceCollection services) + { + services.AddScoped(); + } + private static void AddUserRegistrationCommands(this IServiceCollection services) { services.AddScoped(); diff --git a/src/Core/Enums/EventType.cs b/src/Core/Enums/EventType.cs index af3673f10..ed3fdb21d 100644 --- a/src/Core/Enums/EventType.cs +++ b/src/Core/Enums/EventType.cs @@ -14,6 +14,7 @@ public enum EventType : int User_UpdatedTempPassword = 1008, User_MigratedKeyToKeyConnector = 1009, User_RequestedDeviceApproval = 1010, + User_TdeOffboardingPasswordSet = 1011, Cipher_Created = 1100, Cipher_Updated = 1101, diff --git a/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs b/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs index b9fba5af2..77f822c49 100644 --- a/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs +++ b/src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs @@ -95,8 +95,9 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder return; } - var ssoConfigurationData = _ssoConfig.GetData(); - if (ssoConfigurationData is not { MemberDecryptionType: MemberDecryptionType.TrustedDeviceEncryption }) + var isTdeActive = _ssoConfig.GetData() is { MemberDecryptionType: MemberDecryptionType.TrustedDeviceEncryption }; + var isTdeOffboarding = _user != null && !_user.HasMasterPassword() && _device != null && _device.IsTrusted() && !isTdeActive; + if (!isTdeActive && !isTdeOffboarding) { return; } @@ -144,6 +145,7 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder hasAdminApproval, hasLoginApprovingDevice, hasManageResetPasswordPermission, + isTdeOffboarding, encryptedPrivateKey, encryptedUserKey); } diff --git a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs index d1911a0dc..a16a9cb55 100644 --- a/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs +++ b/test/Api.Test/Auth/Controllers/AccountsControllerTests.cs @@ -12,6 +12,7 @@ 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.UserKey; using Bit.Core.Auth.UserFeatures.UserMasterPassword.Interfaces; using Bit.Core.Billing.Services; @@ -44,6 +45,7 @@ public class AccountsControllerTests : IDisposable private readonly IPolicyService _policyService; private readonly ISetInitialMasterPasswordCommand _setInitialMasterPasswordCommand; private readonly IRotateUserKeyCommand _rotateUserKeyCommand; + private readonly ITdeOffboardingPasswordCommand _tdeOffboardingPasswordCommand; private readonly IFeatureService _featureService; private readonly ISubscriberService _subscriberService; private readonly IReferenceEventService _referenceEventService; @@ -72,6 +74,7 @@ public class AccountsControllerTests : IDisposable _policyService = Substitute.For(); _setInitialMasterPasswordCommand = Substitute.For(); _rotateUserKeyCommand = Substitute.For(); + _tdeOffboardingPasswordCommand = Substitute.For(); _featureService = Substitute.For(); _subscriberService = Substitute.For(); _referenceEventService = Substitute.For(); @@ -97,6 +100,7 @@ public class AccountsControllerTests : IDisposable _userService, _policyService, _setInitialMasterPasswordCommand, + _tdeOffboardingPasswordCommand, _rotateUserKeyCommand, _featureService, _subscriberService, diff --git a/test/Core.Test/Auth/UserFeatures/TdeOffboarding/TdeOffboardingPasswordCommandTests.cs b/test/Core.Test/Auth/UserFeatures/TdeOffboarding/TdeOffboardingPasswordCommandTests.cs new file mode 100644 index 000000000..49558783f --- /dev/null +++ b/test/Core.Test/Auth/UserFeatures/TdeOffboarding/TdeOffboardingPasswordCommandTests.cs @@ -0,0 +1,99 @@ +using Bit.Core.Auth.Entities; +using Bit.Core.Auth.Enums; +using Bit.Core.Auth.Repositories; +using Bit.Core.Auth.UserFeatures.UserMasterPassword; +using Bit.Core.Entities; +using Bit.Core.Exceptions; +using Bit.Core.Models.Data.Organizations.OrganizationUsers; +using Bit.Core.Repositories; +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.Auth.UserFeatures.UserMasterPassword; + +[SutProviderCustomize] +public class TdeOffboardingPasswordTests +{ + [Theory] + [BitAutoData] + public async Task TdeOffboardingPasswordCommand_Success(SutProvider sutProvider, + User user, string masterPassword, string key, string hint, OrganizationUserOrganizationDetails orgUserDetails, SsoUser ssoUser) + { + // Arrange + user.MasterPassword = null; + + sutProvider.GetDependency() + .UpdatePasswordHash(Arg.Any(), Arg.Any()) + .Returns(IdentityResult.Success); + + orgUserDetails.UseSso = true; + sutProvider.GetDependency() + .GetManyDetailsByUserAsync(user.Id) + .Returns(new List { orgUserDetails }); + + sutProvider.GetDependency() + .GetByUserIdOrganizationIdAsync(orgUserDetails.OrganizationId, user.Id) + .Returns(ssoUser); + + var ssoConfig = new SsoConfig(); + var ssoConfigData = ssoConfig.GetData(); + ssoConfigData.MemberDecryptionType = MemberDecryptionType.MasterPassword; + ssoConfig.SetData(ssoConfigData); + sutProvider.GetDependency() + .GetByOrganizationIdAsync(orgUserDetails.OrganizationId) + .Returns(ssoConfig); + + // Act + var result = await sutProvider.Sut.UpdateTdeOffboardingPasswordAsync(user, masterPassword, key, hint); + + // Assert + Assert.Equal(IdentityResult.Success, result); + } + + [Theory] + [BitAutoData] + public async Task TdeOffboardingPasswordCommand_RejectWithTdeEnabled(SutProvider sutProvider, + User user, string masterPassword, string key, string hint, OrganizationUserOrganizationDetails orgUserDetails, SsoUser ssoUser) + { + // Arrange + user.MasterPassword = null; + + sutProvider.GetDependency() + .UpdatePasswordHash(Arg.Any(), Arg.Any(), true, false) + .Returns(IdentityResult.Success); + + orgUserDetails.UseSso = true; + sutProvider.GetDependency() + .GetManyDetailsByUserAsync(user.Id) + .Returns(new List { orgUserDetails }); + + sutProvider.GetDependency() + .GetByUserIdOrganizationIdAsync(orgUserDetails.OrganizationId, user.Id) + .Returns(ssoUser); + + var ssoConfig = new SsoConfig(); + var ssoConfigData = ssoConfig.GetData(); + ssoConfigData.MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption; + ssoConfig.SetData(ssoConfigData); + sutProvider.GetDependency() + .GetByOrganizationIdAsync(orgUserDetails.OrganizationId) + .Returns(ssoConfig); + + await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateTdeOffboardingPasswordAsync(user, masterPassword, key, hint)); + } + + + [Theory] + [BitAutoData] + public async Task TdeOffboardingPasswordCommand_RejectWithMasterPassword(SutProvider sutProvider, + User user, string masterPassword, string key, string hint) + { + // the user already has a master password, so the off-boarding request should fail, since off-boarding only applies to passwordless TDE users + await Assert.ThrowsAsync(() => sutProvider.Sut.UpdateTdeOffboardingPasswordAsync(user, masterPassword, key, hint)); + } + +} diff --git a/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs b/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs index 1856ddb95..0a2514b23 100644 --- a/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs +++ b/test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs @@ -178,6 +178,11 @@ public class IdentityServerSsoTests { Assert.Equal("HasManageResetPasswordPermission", p.Name); Assert.Equal(JsonValueKind.False, p.Value.ValueKind); + }, + p => + { + Assert.Equal("IsTdeOffboarding", p.Name); + Assert.Equal(JsonValueKind.False, p.Value.ValueKind); }); } @@ -219,6 +224,7 @@ public class IdentityServerSsoTests // "HasAdminApproval": true, // "HasLoginApprovingDevice": true, // "HasManageResetPasswordPermission": false + // "IsTdeOffboarding": false // } // } @@ -242,6 +248,11 @@ public class IdentityServerSsoTests { Assert.Equal("HasManageResetPasswordPermission", p.Name); Assert.Equal(JsonValueKind.False, p.Value.ValueKind); + }, + p => + { + Assert.Equal("IsTdeOffboarding", p.Name); + Assert.Equal(JsonValueKind.False, p.Value.ValueKind); }); }