From 2dc4e9a420120aec0b0edbe2ef571304b0383068 Mon Sep 17 00:00:00 2001 From: Dave <3836813+enmande@users.noreply.github.com> Date: Mon, 29 Dec 2025 11:55:05 -0500 Subject: [PATCH] feat(2fa-webauthn) [PM-20109]: Increase 2FA WebAuthn Security Key Limit (#6751) * feat(global-settings) [PM-20109]: Add WebAuthN global settings. * feat(webauthn) [PM-20109]: Update maximum allowed WebAuthN credentials to use new settings. * test(webauthn) [PM-20109]: Update command tests to use global configs. * feat(global-settings) [PM-20109]: Set defaults for maximum allowed credentials. * feat(two-factor-request-model) [PM-20109]: Remove hard-coded 5 limit on ID validation. * Revert "test(webauthn) [PM-20109]: Update command tests to use global configs." This reverts commit ba9f0d5fb6cfc8ad1bb8812d150172df6a617a3f. * Revert "feat(webauthn) [PM-20109]: Update maximum allowed WebAuthN credentials to use new settings." This reverts commit d2faef0c1366b420d5ef04038c4fd05f391f73e2. * feat(global-settings) [PM-20109]: Add WebAuthNSettings to interface for User Service consumption. * feat(user-service) [PM-20109]: Add boundary and persistence-time validation for maximum allowed WebAuthN 2FA credentials. * test(user-service) [PM-20109]: Update tests for WebAuthN limit scenarios. * refactor(user-service) [PM-20109]: Typo in variable name. * refactor(user-service) [PM-20109]: Remove unnecessary pending check. * refactor(user-service) [PM-20109]: Pending check is necessary. * refactor(webauthn) [PM-20109]: Re-spell WebAuthN => WebAuthn. * refactor(user-service) [PM-20109]: Re-format pending checks for consistency. * refactor(user-service) [PM-20109]: Fix type spelling in comments. * test(user-service) [PM-20109]: Combine premium and non-premium test cases with AutoData. * refactor(user-service) [PM-20109]: Swap HasPremiumAccessQuery in for CanAccessPremium. * refactor(user-service) [PM-20109]: Convert limit check to positive, edit comments. --- .../Models/Request/TwoFactorRequestModels.cs | 2 +- .../Services/Implementations/UserService.cs | 28 +++ src/Core/Settings/GlobalSettings.cs | 7 + src/Core/Settings/IGlobalSettings.cs | 1 + test/Core.Test/Services/UserServiceTests.cs | 207 ++++++++++++++++++ 5 files changed, 244 insertions(+), 1 deletion(-) diff --git a/src/Api/Auth/Models/Request/TwoFactorRequestModels.cs b/src/Api/Auth/Models/Request/TwoFactorRequestModels.cs index 79df29c928..6173de81d9 100644 --- a/src/Api/Auth/Models/Request/TwoFactorRequestModels.cs +++ b/src/Api/Auth/Models/Request/TwoFactorRequestModels.cs @@ -273,7 +273,7 @@ public class TwoFactorWebAuthnDeleteRequestModel : SecretVerificationRequestMode yield return validationResult; } - if (!Id.HasValue || Id < 0 || Id > 5) + if (!Id.HasValue) { yield return new ValidationResult("Invalid Key Id", new string[] { nameof(Id) }); } diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs index 4e65e88767..498721238b 100644 --- a/src/Core/Services/Implementations/UserService.cs +++ b/src/Core/Services/Implementations/UserService.cs @@ -344,6 +344,12 @@ public class UserService : UserManager, IUserService await _mailService.SendMasterPasswordHintEmailAsync(email, user.MasterPasswordHint); } + /// + /// Initiates WebAuthn 2FA credential registration and generates a challenge for adding a new security key. + /// + /// The current user. + /// + /// Maximum allowed number of credentials already registered. public async Task StartWebAuthnRegistrationAsync(User user) { var providers = user.GetTwoFactorProviders(); @@ -364,6 +370,17 @@ public class UserService : UserManager, IUserService provider.MetaData = new Dictionary(); } + // Boundary validation to provide a better UX. There is also second-level enforcement at persistence time. + var maximumAllowedCredentialCount = await _hasPremiumAccessQuery.HasPremiumAccessAsync(user.Id) + ? _globalSettings.WebAuthn.PremiumMaximumAllowedCredentials + : _globalSettings.WebAuthn.NonPremiumMaximumAllowedCredentials; + // Count only saved credentials ("Key{id}") toward the limit. + if (provider.MetaData.Count(k => k.Key.StartsWith("Key")) >= + maximumAllowedCredentialCount) + { + throw new BadRequestException("Maximum allowed WebAuthn credential count exceeded."); + } + var fidoUser = new Fido2User { DisplayName = user.Name, @@ -402,6 +419,17 @@ public class UserService : UserManager, IUserService return false; } + // Persistence-time validation for comprehensive enforcement. There is also boundary validation for best-possible UX. + var maximumAllowedCredentialCount = await _hasPremiumAccessQuery.HasPremiumAccessAsync(user.Id) + ? _globalSettings.WebAuthn.PremiumMaximumAllowedCredentials + : _globalSettings.WebAuthn.NonPremiumMaximumAllowedCredentials; + // Count only saved credentials ("Key{id}") toward the limit. + if (provider.MetaData.Count(k => k.Key.StartsWith("Key")) >= + maximumAllowedCredentialCount) + { + throw new BadRequestException("Maximum allowed WebAuthn credential count exceeded."); + } + var options = CredentialCreateOptions.FromJson((string)pendingValue); // Callback to ensure credential ID is unique. Always return true since we don't care if another diff --git a/src/Core/Settings/GlobalSettings.cs b/src/Core/Settings/GlobalSettings.cs index f030c73809..60a1fda19f 100644 --- a/src/Core/Settings/GlobalSettings.cs +++ b/src/Core/Settings/GlobalSettings.cs @@ -66,6 +66,7 @@ public class GlobalSettings : IGlobalSettings public virtual NotificationHubPoolSettings NotificationHubPool { get; set; } = new(); public virtual YubicoSettings Yubico { get; set; } = new YubicoSettings(); public virtual DuoSettings Duo { get; set; } = new DuoSettings(); + public virtual WebAuthnSettings WebAuthn { get; set; } = new WebAuthnSettings(); public virtual BraintreeSettings Braintree { get; set; } = new BraintreeSettings(); public virtual ImportCiphersLimitationSettings ImportCiphersLimitation { get; set; } = new ImportCiphersLimitationSettings(); public virtual BitPaySettings BitPay { get; set; } = new BitPaySettings(); @@ -613,6 +614,12 @@ public class GlobalSettings : IGlobalSettings public string AKey { get; set; } } + public class WebAuthnSettings + { + public int PremiumMaximumAllowedCredentials { get; set; } = 10; + public int NonPremiumMaximumAllowedCredentials { get; set; } = 5; + } + public class BraintreeSettings { public bool Production { get; set; } diff --git a/src/Core/Settings/IGlobalSettings.cs b/src/Core/Settings/IGlobalSettings.cs index 06dece3394..c316836d09 100644 --- a/src/Core/Settings/IGlobalSettings.cs +++ b/src/Core/Settings/IGlobalSettings.cs @@ -28,4 +28,5 @@ public interface IGlobalSettings string DevelopmentDirectory { get; set; } IWebPushSettings WebPush { get; set; } GlobalSettings.EventLoggingSettings EventLogging { get; set; } + GlobalSettings.WebAuthnSettings WebAuthn { get; set; } } diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs index 9d83674f44..073379820e 100644 --- a/test/Core.Test/Services/UserServiceTests.cs +++ b/test/Core.Test/Services/UserServiceTests.cs @@ -25,11 +25,15 @@ using Bit.Core.Utilities; using Bit.Test.Common.AutoFixture; using Bit.Test.Common.AutoFixture.Attributes; using Bit.Test.Common.Helpers; +using Fido2NetLib; +using Fido2NetLib.Objects; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Caching.Distributed; using Microsoft.Extensions.Options; using NSubstitute; using Xunit; +using static Fido2NetLib.Fido2; +using GlobalSettings = Bit.Core.Settings.GlobalSettings; namespace Bit.Core.Test.Services; @@ -594,6 +598,209 @@ public class UserServiceTests user.MasterPassword = null; } } + + [Theory] + [BitAutoData(true)] + [BitAutoData(false)] + public async Task StartWebAuthnRegistrationAsync_BelowLimit_Succeeds( + bool hasPremium, SutProvider sutProvider, User user) + { + // Arrange - Non-premium user with 4 credentials (below limit of 5) + SetupWebAuthnProvider(user, credentialCount: 4); + + sutProvider.GetDependency().WebAuthn = new GlobalSettings.WebAuthnSettings + { + PremiumMaximumAllowedCredentials = 10, + NonPremiumMaximumAllowedCredentials = 5 + }; + + user.Premium = hasPremium; + user.Id = Guid.NewGuid(); + user.Email = "test@example.com"; + + sutProvider.GetDependency() + .GetManyByUserAsync(user.Id) + .Returns(new List()); + + var mockFido2 = sutProvider.GetDependency(); + mockFido2.RequestNewCredential( + Arg.Any(), + Arg.Any>(), + Arg.Any(), + Arg.Any()) + .Returns(new CredentialCreateOptions + { + Challenge = new byte[] { 1, 2, 3 }, + Rp = new PublicKeyCredentialRpEntity("example.com", "example.com", ""), + User = new Fido2User + { + Id = user.Id.ToByteArray(), + Name = user.Email, + DisplayName = user.Name + }, + PubKeyCredParams = new List() + }); + + // Act + var result = await sutProvider.Sut.StartWebAuthnRegistrationAsync(user); + + // Assert + Assert.NotNull(result); + await sutProvider.GetDependency().Received(1).ReplaceAsync(user); + } + + [Theory] + [BitAutoData(true)] + [BitAutoData(false)] + public async Task CompleteWebAuthRegistrationAsync_ExceedsLimit_ThrowsBadRequestException(bool hasPremium, + SutProvider sutProvider, User user, AuthenticatorAttestationRawResponse deviceResponse) + { + // Arrange - time-of-check/time-of-use scenario: user now has 10 credentials (at limit) + SetupWebAuthnProviderWithPending(user, credentialCount: 10); + + sutProvider.GetDependency().WebAuthn = new GlobalSettings.WebAuthnSettings + { + PremiumMaximumAllowedCredentials = 10, + NonPremiumMaximumAllowedCredentials = 5 + }; + + user.Premium = hasPremium; + sutProvider.GetDependency() + .GetManyByUserAsync(user.Id) + .Returns(new List()); + + // Act & Assert + var exception = await Assert.ThrowsAsync( + () => sutProvider.Sut.CompleteWebAuthRegistrationAsync(user, 11, "NewKey", deviceResponse)); + + Assert.Equal("Maximum allowed WebAuthn credential count exceeded.", exception.Message); + } + + [Theory] + [BitAutoData(true)] + [BitAutoData(false)] + public async Task CompleteWebAuthRegistrationAsync_BelowLimit_Succeeds(bool hasPremium, + SutProvider sutProvider, User user, AuthenticatorAttestationRawResponse deviceResponse) + { + // Arrange - User has 4 credentials (below limit of 5) + SetupWebAuthnProviderWithPending(user, credentialCount: 4); + + sutProvider.GetDependency().WebAuthn = new GlobalSettings.WebAuthnSettings + { + PremiumMaximumAllowedCredentials = 10, + NonPremiumMaximumAllowedCredentials = 5 + }; + + user.Premium = hasPremium; + user.Id = Guid.NewGuid(); + + sutProvider.GetDependency() + .GetManyByUserAsync(user.Id) + .Returns(new List()); + + var mockFido2 = sutProvider.GetDependency(); + mockFido2.MakeNewCredentialAsync( + Arg.Any(), + Arg.Any(), + Arg.Any()) + .Returns(new CredentialMakeResult("ok", "", new AttestationVerificationSuccess + { + Aaguid = Guid.NewGuid(), + Counter = 0, + CredentialId = new byte[] { 1, 2, 3 }, + CredType = "public-key", + PublicKey = new byte[] { 4, 5, 6 }, + Status = "ok", + User = new Fido2User + { + Id = user.Id.ToByteArray(), + Name = user.Email ?? "test@example.com", + DisplayName = user.Name ?? "Test User" + } + })); + + // Act + var result = await sutProvider.Sut.CompleteWebAuthRegistrationAsync(user, 5, "NewKey", deviceResponse); + + // Assert + Assert.True(result); + await sutProvider.GetDependency().Received(1).ReplaceAsync(user); + } + + private static void SetupWebAuthnProvider(User user, int credentialCount) + { + var providers = new Dictionary(); + var metadata = new Dictionary(); + + // Add credentials as Key1, Key2, Key3, etc. + for (int i = 1; i <= credentialCount; i++) + { + metadata[$"Key{i}"] = new TwoFactorProvider.WebAuthnData + { + Name = $"Key {i}", + Descriptor = new PublicKeyCredentialDescriptor(new byte[] { (byte)i }), + PublicKey = new byte[] { (byte)i }, + UserHandle = new byte[] { (byte)i }, + SignatureCounter = 0, + CredType = "public-key", + RegDate = DateTime.UtcNow, + AaGuid = Guid.NewGuid() + }; + } + + providers[TwoFactorProviderType.WebAuthn] = new TwoFactorProvider + { + Enabled = true, + MetaData = metadata + }; + + user.SetTwoFactorProviders(providers); + } + + private static void SetupWebAuthnProviderWithPending(User user, int credentialCount) + { + var providers = new Dictionary(); + var metadata = new Dictionary(); + + // Add existing credentials + for (int i = 1; i <= credentialCount; i++) + { + metadata[$"Key{i}"] = new TwoFactorProvider.WebAuthnData + { + Name = $"Key {i}", + Descriptor = new PublicKeyCredentialDescriptor(new byte[] { (byte)i }), + PublicKey = new byte[] { (byte)i }, + UserHandle = new byte[] { (byte)i }, + SignatureCounter = 0, + CredType = "public-key", + RegDate = DateTime.UtcNow, + AaGuid = Guid.NewGuid() + }; + } + + // Add pending registration + var pendingOptions = new CredentialCreateOptions + { + Challenge = new byte[] { 1, 2, 3 }, + Rp = new PublicKeyCredentialRpEntity("example.com", "example.com", ""), + User = new Fido2User + { + Id = user.Id.ToByteArray(), + Name = user.Email ?? "test@example.com", + DisplayName = user.Name ?? "Test User" + }, + PubKeyCredParams = new List() + }; + metadata["pending"] = pendingOptions.ToJson(); + + providers[TwoFactorProviderType.WebAuthn] = new TwoFactorProvider + { + Enabled = true, + MetaData = metadata + }; + + user.SetTwoFactorProviders(providers); + } } public static class UserServiceSutProviderExtensions