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.

src/Core/Services/Fido2AuthenticatorService.cs

@@ -26,7 +26,7 @@ namespace Bit.Core.Services
 
         public async Task<Fido2AuthenticatorMakeCredentialResult> 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<Fido2AuthenticatorGetAssertionResult> GetAssertionAsync(Fido2AuthenticatorGetAssertionParams assertionParams, IFido2GetAssertionUserInterface userInterface)
         {
             List<CipherView> 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<Fido2AuthenticatorDiscoverableCredentialMetadata[]> 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
         /// </summary>
         private async Task<string[]> FindExcludedCredentialsAsync(
             PublicKeyCredentialDescriptor[] credentials
-        ) {
+        )
-            if (credentials == null || credentials.Length == 0) {
+        {
+            if (credentials == null || credentials.Length == 0)
+            {
                 return Array.Empty<string>();
             }
 
             var ids = new List<string>();
 
-            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<string>();
             }
 
@@ -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<byte> authData = new List<byte>();
@@ -328,7 +355,7 @@ namespace Bit.Core.Services
                 var attestedCredentialData = new List<byte>();
 
                 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);

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<Fido2GetAssertionUserInterfaceCredential[]>()).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<Fido2GetAssertionUserInterfaceCredential[]>()).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<Fido2GetAssertionUserInterfaceCredential[]>()).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<Fido2GetAssertionUserInterfaceCredential[]>()).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<ICipherService>().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
+        // 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) {
+        public async Task GetAssertionAsync_DoesNotIncrementsCounter_CounterIsZero(Cipher encryptedCipher)
+        {
             // Arrange
             _selectedCipher.Login.MainFido2Credential.CounterValue = 0;
             _sutProvider.GetDependency<ICipherService>().EncryptAsync(_selectedCipher).Returns(encryptedCipher);
-            
+
             // Act
             await _sutProvider.Sut.GetAssertionAsync(_params, _userInterface);
 
             // Assert
-            await _sutProvider.GetDependency<ICipherService>().Received().SaveWithServerAsync(encryptedCipher);
+            await _sutProvider.GetDependency<ICipherService>().DidNotReceive().SaveWithServerAsync(Arg.Any<Cipher>());
-            await _sutProvider.GetDependency<ICipherService>().Received().EncryptAsync(Arg.Is<CipherView>(
-                (cipher) => cipher.Login.MainFido2Credential.CounterValue == 0
-            ));
         }
 
         [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<ICryptoFunctionService>().HashAsync(_params.RpId, CryptoHashAlgorithm.Sha256).Returns(rpIdHashMock);
             _userInterface.PickCredentialAsync(Arg.Any<Fido2GetAssertionUserInterfaceCredential[]>()).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<ICipherService>().SaveWithServerAsync(Arg.Any<Cipher>()).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<Fido2CredentialView> {
                         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,