From c31444dc8bb47ddba72fdf7983d4063712d28aa2 Mon Sep 17 00:00:00 2001 From: Andreas Coroiu Date: Thu, 22 Feb 2024 14:34:10 +0100 Subject: [PATCH] feat: optimize assertion network calls (#3021) The server only needs to be updated if we have changed the counter. New passkeys that leave their counters at zero can therefore skip this step. --- .../Services/Fido2AuthenticatorService.cs | 126 +++++++++++------- .../Fido2AuthenticatorGetAssertionTests.cs | 56 ++++---- 2 files changed, 113 insertions(+), 69 deletions(-) diff --git a/src/Core/Services/Fido2AuthenticatorService.cs b/src/Core/Services/Fido2AuthenticatorService.cs index bd7e0cc06..426b8a2e6 100644 --- a/src/Core/Services/Fido2AuthenticatorService.cs +++ b/src/Core/Services/Fido2AuthenticatorService.cs @@ -26,7 +26,7 @@ namespace Bit.Core.Services public async Task MakeCredentialAsync(Fido2AuthenticatorMakeCredentialParams makeCredentialParams, IFido2MakeCredentialUserInterface userInterface) { - if (makeCredentialParams.CredTypesAndPubKeyAlgs.All((p) => p.Alg != (int) Fido2AlgorithmIdentifier.ES256)) + if (makeCredentialParams.CredTypesAndPubKeyAlgs.All((p) => p.Alg != (int)Fido2AlgorithmIdentifier.ES256)) { throw new NotSupportedError(); } @@ -37,12 +37,14 @@ namespace Bit.Core.Services var existingCipherIds = await FindExcludedCredentialsAsync( makeCredentialParams.ExcludeCredentialDescriptorList ); - if (existingCipherIds.Length > 0) { + if (existingCipherIds.Length > 0) + { await userInterface.InformExcludedCredentialAsync(existingCipherIds); throw new NotAllowedError(); } - var response = await userInterface.ConfirmNewCredentialAsync(new Fido2ConfirmNewCredentialParams { + var response = await userInterface.ConfirmNewCredentialAsync(new Fido2ConfirmNewCredentialParams + { CredentialName = makeCredentialParams.RpEntity.Name, UserName = makeCredentialParams.UserEntity.Name, UserVerification = makeCredentialParams.RequireUserVerification @@ -51,18 +53,21 @@ namespace Bit.Core.Services var cipherId = response.CipherId; var userVerified = response.UserVerified; string credentialId; - if (cipherId == null) { + if (cipherId == null) + { throw new NotAllowedError(); } - - try { + + try + { var keyPair = GenerateKeyPair(); var fido2Credential = CreateCredentialView(makeCredentialParams, keyPair.privateKey); var encrypted = await _cipherService.GetAsync(cipherId); var cipher = await encrypted.DecryptAsync(); - if (!userVerified && (makeCredentialParams.RequireUserVerification || cipher.Reprompt != CipherRepromptType.None)) { + if (!userVerified && (makeCredentialParams.RequireUserVerification || cipher.Reprompt != CipherRepromptType.None)) + { throw new NotAllowedError(); } @@ -86,15 +91,19 @@ namespace Bit.Core.Services AttestationObject = EncodeAttestationObject(authData), AuthData = authData, PublicKey = keyPair.publicKey.ExportDer(), - PublicKeyAlgorithm = (int) Fido2AlgorithmIdentifier.ES256, + PublicKeyAlgorithm = (int)Fido2AlgorithmIdentifier.ES256, }; - } catch (NotAllowedError) { + } + catch (NotAllowedError) + { throw; - } catch (Exception) { + } + catch (Exception) + { throw new UnknownError(); } } - + public async Task GetAssertionAsync(Fido2AuthenticatorGetAssertionParams assertionParams, IFido2GetAssertionUserInterface userInterface) { List cipherOptions; @@ -102,21 +111,26 @@ namespace Bit.Core.Services await userInterface.EnsureUnlockedVaultAsync(); await _syncService.FullSyncAsync(false); - if (assertionParams.AllowCredentialDescriptorList?.Length > 0) { + if (assertionParams.AllowCredentialDescriptorList?.Length > 0) + { cipherOptions = await FindCredentialsByIdAsync( assertionParams.AllowCredentialDescriptorList, assertionParams.RpId ); - } else { + } + else + { cipherOptions = await FindCredentialsByRpAsync(assertionParams.RpId); } - if (cipherOptions.Count == 0) { + if (cipherOptions.Count == 0) + { throw new NotAllowedError(); } var response = await userInterface.PickCredentialAsync( - cipherOptions.Select((cipher) => new Fido2GetAssertionUserInterfaceCredential { + cipherOptions.Select((cipher) => new Fido2GetAssertionUserInterfaceCredential + { CipherId = cipher.Id, RequireUserVerification = assertionParams.RequireUserVerification || cipher.Reprompt != CipherRepromptType.None }).ToArray() @@ -125,25 +139,29 @@ namespace Bit.Core.Services var userVerified = response.UserVerified; var selectedCipher = cipherOptions.FirstOrDefault((c) => c.Id == selectedCipherId); - if (selectedCipher == null) { + if (selectedCipher == null) + { throw new NotAllowedError(); } - if (!userVerified && (assertionParams.RequireUserVerification || selectedCipher.Reprompt != CipherRepromptType.None)) { + if (!userVerified && (assertionParams.RequireUserVerification || selectedCipher.Reprompt != CipherRepromptType.None)) + { throw new NotAllowedError(); } - - try { + + try + { var selectedFido2Credential = selectedCipher.Login.MainFido2Credential; var selectedCredentialId = selectedFido2Credential.CredentialId; - if (selectedFido2Credential.CounterValue != 0) { - ++selectedFido2Credential.CounterValue; - } - await _cipherService.UpdateLastUsedDateAsync(selectedCipher.Id); - var encrypted = await _cipherService.EncryptAsync(selectedCipher); - await _cipherService.SaveWithServerAsync(encrypted); + + if (selectedFido2Credential.CounterValue != 0) + { + ++selectedFido2Credential.CounterValue; + var encrypted = await _cipherService.EncryptAsync(selectedCipher); + await _cipherService.SaveWithServerAsync(encrypted); + } var authenticatorData = await GenerateAuthDataAsync( rpId: selectedFido2Credential.RpId, @@ -168,14 +186,17 @@ namespace Bit.Core.Services AuthenticatorData = authenticatorData, Signature = signature }; - } catch (Exception) { + } + catch (Exception) + { throw new UnknownError(); } } public async Task SilentCredentialDiscoveryAsync(string rpId) { - var credentials = (await FindCredentialsByRpAsync(rpId)).Select(cipher => new Fido2AuthenticatorDiscoverableCredentialMetadata { + var credentials = (await FindCredentialsByRpAsync(rpId)).Select(cipher => new Fido2AuthenticatorDiscoverableCredentialMetadata + { Type = Constants.DefaultFido2CredentialType, Id = cipher.Login.MainFido2Credential.CredentialId.GuidToRawFormat(), RpId = cipher.Login.MainFido2Credential.RpId, @@ -191,22 +212,26 @@ namespace Bit.Core.Services /// private async Task FindExcludedCredentialsAsync( PublicKeyCredentialDescriptor[] credentials - ) { - if (credentials == null || credentials.Length == 0) { + ) + { + if (credentials == null || credentials.Length == 0) + { return Array.Empty(); } var ids = new List(); - foreach (var credential in credentials) + foreach (var credential in credentials) { try { ids.Add(credential.Id.GuidToStandardFormat()); - } catch {} + } + catch { } } - if (ids.Count == 0) { + if (ids.Count == 0) + { return Array.Empty(); } @@ -234,7 +259,7 @@ namespace Bit.Core.Services { ids.Add(credential.Id.GuidToStandardFormat()); } - catch {} + catch { } } if (ids.Count == 0) @@ -276,7 +301,8 @@ namespace Bit.Core.Services private Fido2CredentialView CreateCredentialView(Fido2AuthenticatorMakeCredentialParams makeCredentialsParams, byte[] privateKey) { - return new Fido2CredentialView { + return new Fido2CredentialView + { CredentialId = Guid.NewGuid().ToString(), KeyType = Constants.DefaultFido2CredentialType, KeyAlgorithm = Constants.DefaultFido2CredentialAlgorithm, @@ -300,7 +326,8 @@ namespace Bit.Core.Services int counter, byte[] credentialId = null, PublicKey publicKey = null - ) { + ) + { var isAttestation = credentialId != null && publicKey != null; List authData = new List(); @@ -328,7 +355,7 @@ namespace Bit.Core.Services var attestedCredentialData = new List(); attestedCredentialData.AddRange(AAGUID); - + // credentialIdLength (2 bytes) and credential Id var credentialIdLength = new byte[] { (byte)((credentialId.Length - (credentialId.Length & 0xff)) / 256), @@ -344,14 +371,17 @@ namespace Bit.Core.Services return authData.ToArray(); } - private byte AuthDataFlags(bool extensionData, bool attestationData, bool userVerification, bool userPresence, bool backupEligibility = true, bool backupState = true) { + private byte AuthDataFlags(bool extensionData, bool attestationData, bool userVerification, bool userPresence, bool backupEligibility = true, bool backupState = true) + { byte flags = 0; - if (extensionData) { + if (extensionData) + { flags |= 0b1000000; } - if (attestationData) { + if (attestationData) + { flags |= 0b01000000; } @@ -365,18 +395,21 @@ namespace Bit.Core.Services flags |= 0b00001000; } - if (userVerification) { + if (userVerification) + { flags |= 0b00000100; } - if (userPresence) { + if (userPresence) + { flags |= 0b00000001; } return flags; } - private byte[] EncodeAttestationObject(byte[] authData) { + private byte[] EncodeAttestationObject(byte[] authData) + { var attestationObject = new CborWriter(CborConformanceMode.Ctap2Canonical); attestationObject.WriteStartMap(3); attestationObject.WriteTextString("fmt"); @@ -398,7 +431,7 @@ namespace Bit.Core.Services var dsa = ECDsa.Create(); dsa.ImportPkcs8PrivateKey(privateKey, out var bytesRead); - if (bytesRead == 0) + if (bytesRead == 0) { throw new Exception("Failed to import private key"); } @@ -410,7 +443,8 @@ namespace Bit.Core.Services { private readonly ECDsa _dsa; - public PublicKey(ECDsa dsa) { + public PublicKey(ECDsa dsa) + { _dsa = dsa; } @@ -426,14 +460,14 @@ namespace Bit.Core.Services { var result = new CborWriter(CborConformanceMode.Ctap2Canonical); result.WriteStartMap(5); - + // kty = EC2 result.WriteInt32(1); result.WriteInt32(2); // alg = ES256 result.WriteInt32(3); - result.WriteInt32((int) Fido2AlgorithmIdentifier.ES256); + result.WriteInt32((int)Fido2AlgorithmIdentifier.ES256); // crv = P-256 result.WriteInt32(-1); diff --git a/test/Core.Test/Services/Fido2AuthenticatorGetAssertionTests.cs b/test/Core.Test/Services/Fido2AuthenticatorGetAssertionTests.cs index 96964ba20..c21e8fa83 100644 --- a/test/Core.Test/Services/Fido2AuthenticatorGetAssertionTests.cs +++ b/test/Core.Test/Services/Fido2AuthenticatorGetAssertionTests.cs @@ -57,7 +57,7 @@ namespace Bit.Core.Test.Services _selectedCipherCredentialId = _credentialIds[0]; _selectedCipherRawCredentialId = _rawCredentialIds[0]; _params = CreateParams( - rpId: _rpId, + rpId: _rpId, allowCredentialDescriptorList: [ new PublicKeyCredentialDescriptor { Id = _rawCredentialIds[0], @@ -74,7 +74,7 @@ namespace Bit.Core.Test.Services _userInterface.PickCredentialAsync(Arg.Any()).Returns((_ciphers[0].Id, false)); } - public void Dispose() + public void Dispose() { } @@ -149,7 +149,8 @@ namespace Bit.Core.Test.Services [Fact] // Spec: Prompt the user to select a public key credential source `selectedCredential` from `credentialOptions`. // If requireUserVerification is true, the authorization gesture MUST include user verification. - public async Task GetAssertionAsync_RequestsUserVerification_ParamsRequireUserVerification() { + public async Task GetAssertionAsync_RequestsUserVerification_ParamsRequireUserVerification() + { // Arrange _params.RequireUserVerification = true; _userInterface.PickCredentialAsync(Arg.Any()).Returns((_ciphers[0].Id, true)); @@ -168,7 +169,8 @@ namespace Bit.Core.Test.Services // If `requireUserPresence` is true, the authorization gesture MUST include a test of user presence. // Comment: User presence is implied by the UI returning a credential. // Extension: UserVerification is required if the cipher requires reprompting. - public async Task GetAssertionAsync_DoesNotRequestUserVerification_ParamsDoNotRequireUserVerification() { + public async Task GetAssertionAsync_DoesNotRequestUserVerification_ParamsDoNotRequireUserVerification() + { // Arrange _params.RequireUserVerification = false; @@ -183,7 +185,8 @@ namespace Bit.Core.Test.Services [Fact] // Spec: If the user does not consent, return an error code equivalent to "NotAllowedError" and terminate the operation. - public async Task GetAssertionAsync_ThrowsNotAllowed_UserDoesNotConsent() { + public async Task GetAssertionAsync_ThrowsNotAllowed_UserDoesNotConsent() + { // Arrange _userInterface.PickCredentialAsync(Arg.Any()).Returns((null, false)); @@ -193,7 +196,8 @@ namespace Bit.Core.Test.Services [Fact] // Spec: If the user does not consent, return an error code equivalent to "NotAllowedError" and terminate the operation. - public async Task GetAssertionAsync_ThrowsNotAllowed_NoUserVerificationWhenRequired() { + public async Task GetAssertionAsync_ThrowsNotAllowed_NoUserVerificationWhenRequired() + { // Arrange _params.RequireUserVerification = true; _userInterface.PickCredentialAsync(Arg.Any()).Returns((_selectedCipher.Id, false)); @@ -204,7 +208,8 @@ namespace Bit.Core.Test.Services [Fact] // Spec: If the user does not consent, return an error code equivalent to "NotAllowedError" and terminate the operation. - public async Task GetAssertionAsync_ThrowsNotAllowed_NoUserVerificationForCipherWithReprompt() { + public async Task GetAssertionAsync_ThrowsNotAllowed_NoUserVerificationForCipherWithReprompt() + { // Arrange _selectedCipher.Reprompt = CipherRepromptType.Password; _params.RequireUserVerification = false; @@ -221,11 +226,12 @@ namespace Bit.Core.Test.Services [Theory] [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) })] // Spec: Increment the credential associated signature counter - public async Task GetAssertionAsync_IncrementsCounter_CounterIsLargerThanZero(Cipher encryptedCipher) { + public async Task GetAssertionAsync_IncrementsCounter_CounterIsLargerThanZero(Cipher encryptedCipher) + { // Arrange _selectedCipher.Login.MainFido2Credential.CounterValue = 9000; _sutProvider.GetDependency().EncryptAsync(_selectedCipher).Returns(encryptedCipher); - + // Act await _sutProvider.Sut.GetAssertionAsync(_params, _userInterface); @@ -238,24 +244,23 @@ namespace Bit.Core.Test.Services [Theory] [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) })] - // Spec: Increment the credential associated signature counter - public async Task GetAssertionAsync_DoesNotIncrementsCounter_CounterIsZero(Cipher encryptedCipher) { + // Spec: Authenticators that do not implement a signature counter leave the signCount in the authenticator data constant at zero. + public async Task GetAssertionAsync_DoesNotIncrementsCounter_CounterIsZero(Cipher encryptedCipher) + { // Arrange _selectedCipher.Login.MainFido2Credential.CounterValue = 0; _sutProvider.GetDependency().EncryptAsync(_selectedCipher).Returns(encryptedCipher); - + // Act await _sutProvider.Sut.GetAssertionAsync(_params, _userInterface); // Assert - await _sutProvider.GetDependency().Received().SaveWithServerAsync(encryptedCipher); - await _sutProvider.GetDependency().Received().EncryptAsync(Arg.Is( - (cipher) => cipher.Login.MainFido2Credential.CounterValue == 0 - )); + await _sutProvider.GetDependency().DidNotReceive().SaveWithServerAsync(Arg.Any()); } [Fact] - public async Task GetAssertionAsync_ReturnsAssertion() { + public async Task GetAssertionAsync_ReturnsAssertion() + { // Arrange var keyPair = GenerateKeyPair(); var rpIdHashMock = RandomBytes(32); @@ -265,7 +270,7 @@ namespace Bit.Core.Test.Services _selectedCipher.Login.MainFido2Credential.KeyValue = CoreHelpers.Base64UrlEncode(keyPair.ExportPkcs8PrivateKey()); _sutProvider.GetDependency().HashAsync(_params.RpId, CryptoHashAlgorithm.Sha256).Returns(rpIdHashMock); _userInterface.PickCredentialAsync(Arg.Any()).Returns((_selectedCipher.Id, true)); - + // Act var result = await _sutProvider.Sut.GetAssertionAsync(_params, _userInterface); @@ -284,8 +289,10 @@ namespace Bit.Core.Test.Services } [Fact] - public async Task GetAssertionAsync_ThrowsUnknownError_SaveFails() { + public async Task GetAssertionAsync_ThrowsUnknownError_SaveFails() + { // Arrange + _selectedCipher.Login.MainFido2Credential.CounterValue = 1; _sutProvider.GetDependency().SaveWithServerAsync(Arg.Any()).Throws(new Exception()); // Act & Assert @@ -309,14 +316,16 @@ namespace Bit.Core.Test.Services return dsa; } - #nullable enable +#nullable enable private CipherView CreateCipherView(string credentialId, string? rpId, bool? discoverable, bool reprompt = false) { - return new CipherView { + return new CipherView + { Type = CipherType.Login, Id = Guid.NewGuid().ToString(), Reprompt = reprompt ? CipherRepromptType.Password : CipherRepromptType.None, - Login = new LoginView { + Login = new LoginView + { Fido2Credentials = new List { new Fido2CredentialView { CredentialId = credentialId, @@ -332,7 +341,8 @@ namespace Bit.Core.Test.Services private Fido2AuthenticatorGetAssertionParams CreateParams(string? rpId = null, byte[]? hash = null, PublicKeyCredentialDescriptor[]? allowCredentialDescriptorList = null, bool? requireUserPresence = null, bool? requireUserVerification = null) { - return new Fido2AuthenticatorGetAssertionParams { + return new Fido2AuthenticatorGetAssertionParams + { RpId = rpId ?? "bitwarden.com", Hash = hash ?? RandomBytes(32), AllowCredentialDescriptorList = allowCredentialDescriptorList ?? null,