Remove locks in SignatureProviders

src/Microsoft.IdentityModel.Tokens/AsymmetricAdapter.cs

@@ -67,19 +67,13 @@ internal class AsymmetricAdapter : IDisposable
 
 #if NET461 || NETSTANDARD2_0
         private RSAEncryptionPadding _rsaEncryptionPadding;
-        private object _signRsaLock = new object();
 #endif
 
         private bool _disposeCryptoOperators = false;
         private bool _disposed = false;
         private SignDelegate SignatureFunction;
         private VerifyDelegate VerifyFunction;
 
-
-        private object _signEcdsaLock = new object();
-        private object _verifyRsaLock = new object();
-        private object _verifyEcdsaLock = new object();
-
 #if NET461 || NETSTANDARD2_0
         // HasAlgorithmName was introduced into Net46
         internal AsymmetricAdapter(SecurityKey key, string algorithm, HashAlgorithm hashAlgorithm, HashAlgorithmName hashAlgorithmName, bool requirePrivateKey)
@@ -344,19 +338,13 @@ internal byte[] Sign(byte[] bytes)
 
         private byte[] SignWithECDsa(byte[] bytes)
         {
-            lock (_signEcdsaLock)
-            {
-                return ECDsaSecurityKey.ECDsa.SignHash(HashAlgorithm.ComputeHash(bytes));
-            }
+            return ECDsaSecurityKey.ECDsa.SignHash(HashAlgorithm.ComputeHash(bytes));
         }
 
 #if NET461 || NETSTANDARD2_0
         private byte[] SignWithRsa(byte[] bytes)
         {
-            lock (_signRsaLock)
-            {
-                return RSA.SignHash(HashAlgorithm.ComputeHash(bytes), HashAlgorithmName, RSASignaturePadding);
-            }
+            return RSA.SignHash(HashAlgorithm.ComputeHash(bytes), HashAlgorithmName, RSASignaturePadding);
         }
 #endif
 
@@ -378,29 +366,20 @@ internal bool Verify(byte[] bytes, byte[] signature)
 
         private bool VerifyWithECDsa(byte[] bytes, byte[] signature)
         {
-            lock (_verifyEcdsaLock)
-            {
-                return ECDsaSecurityKey.ECDsa.VerifyHash(HashAlgorithm.ComputeHash(bytes), signature);
-            }
+            return ECDsaSecurityKey.ECDsa.VerifyHash(HashAlgorithm.ComputeHash(bytes), signature);
         }
 
 #if NET461 || NETSTANDARD2_0
         private bool VerifyWithRsa(byte[] bytes, byte[] signature)
         {
-            lock (_verifyRsaLock)
-            {
-                return RSA.VerifyHash(HashAlgorithm.ComputeHash(bytes), signature, HashAlgorithmName, RSASignaturePadding);
-            }
+            return RSA.VerifyHash(HashAlgorithm.ComputeHash(bytes), signature, HashAlgorithmName, RSASignaturePadding);
         }
 #endif
 
 #if DESKTOP
         private bool VerifyWithRsaCryptoServiceProviderProxy(byte[] bytes, byte[] signature)
         {
-            lock (_verifyRsaLock)
-            {
-                return RsaCryptoServiceProviderProxy.VerifyData(bytes, HashAlgorithm, signature);
-            }
+            return RsaCryptoServiceProviderProxy.VerifyData(bytes, HashAlgorithm, signature);
         }
 #endif
 

src/Microsoft.IdentityModel.Tokens/AsymmetricSignatureProvider.cs

@@ -37,9 +37,9 @@ namespace Microsoft.IdentityModel.Tokens
     /// </summary>
     public class AsymmetricSignatureProvider : SignatureProvider
     {
-        private bool _disposed;
-        private AsymmetricAdapter _asymmetricAdapter;
+        private DisposableObjectPool<AsymmetricAdapter> _asymmetricAdapterObjectPool;
         private CryptoProviderFactory _cryptoProviderFactory;
+        private bool _disposed;
         private IReadOnlyDictionary<string, int> _minimumAsymmetricKeySizeInBitsForSigningMap;
         private IReadOnlyDictionary<string, int> _minimumAsymmetricKeySizeInBitsForVerifyingMap;
 
@@ -153,10 +153,10 @@ public AsymmetricSignatureProvider(SecurityKey key, string algorithm, bool willC
             if (!_cryptoProviderFactory.IsSupportedAlgorithm(algorithm, key))
                 throw LogHelper.LogExceptionMessage(new NotSupportedException(LogHelper.FormatInvariant(LogMessages.IDX10634, (algorithm ?? "null"), key)));
 
-            ValidateAsymmetricSecurityKeySize(key, algorithm, willCreateSignatures);
-            _asymmetricAdapter = ResolveAsymmetricAdapter(jsonWebKey?.ConvertedSecurityKey ?? key, algorithm, willCreateSignatures);
             WillCreateSignatures = willCreateSignatures;
-        }
+            ValidateAsymmetricSecurityKeySize(key, algorithm, WillCreateSignatures);
+            _asymmetricAdapterObjectPool = new DisposableObjectPool<AsymmetricAdapter>(CreateAsymmetricAdapter);
+    }
 
         /// <summary>
         /// Gets the mapping from algorithm to the minimum <see cref="AsymmetricSecurityKey"/>.KeySize for creating signatures.
@@ -203,10 +203,10 @@ protected virtual HashAlgorithmName GetHashAlgorithmName(string algorithm)
             return SupportedAlgorithms.GetHashAlgorithmName(algorithm);
         }
 
-        private AsymmetricAdapter ResolveAsymmetricAdapter(SecurityKey key, string algorithm, bool requirePrivateKey)
+        private AsymmetricAdapter CreateAsymmetricAdapter()
         {
-            var hashAlgoritmName = GetHashAlgorithmName(algorithm);
-            return new AsymmetricAdapter(key, algorithm, _cryptoProviderFactory.CreateHashAlgorithm(hashAlgoritmName), hashAlgoritmName, requirePrivateKey);
+            var hashAlgoritmName = GetHashAlgorithmName(Algorithm);
+            return new AsymmetricAdapter(Key, Algorithm, _cryptoProviderFactory.CreateHashAlgorithm(hashAlgoritmName), hashAlgoritmName, WillCreateSignatures);
         }
 #endif
 
@@ -227,16 +227,9 @@ protected virtual string GetHashAlgorithmString(string algorithm)
             return SupportedAlgorithms.GetDigestFromSignatureAlgorithm(algorithm);
         }
 
-        /// <summary>
-        /// This method is here, just to keep the #if out of the constructor.
-        /// </summary>
-        /// <param name="key"></param>
-        /// <param name="algorithm"></param>
-        /// <param name="requirePrivateKey"></param>
-        /// <returns></returns>
-        private AsymmetricAdapter ResolveAsymmetricAdapter(SecurityKey key, string algorithm, bool requirePrivateKey)
+        private AsymmetricAdapter CreateAsymmetricAdapter()
         {
-            return new AsymmetricAdapter(key, algorithm, _cryptoProviderFactory.CreateHashAlgorithm(GetHashAlgorithmString(algorithm)), requirePrivateKey);
+            return new AsymmetricAdapter(Key, Algorithm, _cryptoProviderFactory.CreateHashAlgorithm(GetHashAlgorithmString(Algorithm)), WillCreateSignatures);
         }
 #endif
 
@@ -260,15 +253,23 @@ public override byte[] Sign(byte[] input)
                 throw LogHelper.LogExceptionMessage(new ObjectDisposedException(GetType().ToString()));
             }
 
+            AsymmetricAdapter asym = null;
             try
             {
-                return _asymmetricAdapter.Sign(input);
+                asym = _asymmetricAdapterObjectPool.Allocate();
+                return asym.Sign(input);
             }
             catch
             {
                 CryptoProviderCache?.TryRemove(this);
+                Dispose(true);
                 throw;
             }
+            finally
+            {
+                if (!_disposed)
+                    _asymmetricAdapterObjectPool.Free(asym);
+            }
         }
 
         /// <summary>
@@ -350,15 +351,23 @@ public override bool Verify(byte[] input, byte[] signature)
                 throw LogHelper.LogExceptionMessage(new ObjectDisposedException(GetType().ToString()));
             }
 
+            AsymmetricAdapter asym = null;
             try
             {
-                return _asymmetricAdapter.Verify(input, signature);
+                asym = _asymmetricAdapterObjectPool.Allocate();
+                return asym.Verify(input, signature);
             }
             catch
             {
                 CryptoProviderCache?.TryRemove(this);
+                Dispose(true);
                 throw;
             }
+            finally
+            {
+                if (!_disposed)
+                    _asymmetricAdapterObjectPool.Free(asym);
+            }
         }
 
         /// <summary>
@@ -370,11 +379,12 @@ protected override void Dispose(bool disposing)
             if (!_disposed)
             {
                 _disposed = true;
-
                 if (disposing)
                 {
+                    foreach (var item in _asymmetricAdapterObjectPool.Items)
+                        item.Value?.Dispose();
+
                     CryptoProviderCache?.TryRemove(this);
-                    _asymmetricAdapter.Dispose();
                 }
             }
         }

src/Microsoft.IdentityModel.Tokens/CryptoProviderFactory.cs

@@ -220,7 +220,7 @@ private KeyWrapProvider CreateKeyWrapProvider(SecurityKey key, string algorithm,
         /// <returns>A <see cref="SignatureProvider"/> that can be used to create a signature using the <see cref="SecurityKey"/> and algorithm.</returns>
         public virtual SignatureProvider CreateForSigning(SecurityKey key, string algorithm)
         {
-            return CreateForSigning(key, algorithm, true);
+            return CreateForSigning(key, algorithm, CacheSignatureProviders);
         }
 
 #pragma warning disable 1573
@@ -251,7 +251,7 @@ public virtual SignatureProvider CreateForSigning(SecurityKey key, string algori
         /// <returns>A <see cref="SignatureProvider"/> that can be used to validate a signature using the <see cref="SecurityKey"/> and algorithm.</returns>
         public virtual SignatureProvider CreateForVerifying(SecurityKey key, string algorithm)
         {
-            return CreateForVerifying(key, algorithm, true);
+            return CreateForVerifying(key, algorithm, CacheSignatureProviders);
         }
 
 #pragma warning disable 1573
@@ -557,7 +557,7 @@ public virtual void ReleaseHashAlgorithm(HashAlgorithm hashAlgorithm)
                 throw LogHelper.LogArgumentNullException(nameof(hashAlgorithm));
             else if (CustomCryptoProvider != null && _typeToAlgorithmMap.TryGetValue(hashAlgorithm.GetType().ToString(), out var algorithm) && CustomCryptoProvider.IsSupportedAlgorithm(algorithm))
                 CustomCryptoProvider.Release(hashAlgorithm);
-            else 
+            else
                 hashAlgorithm.Dispose();
         }
 

src/Microsoft.IdentityModel.Tokens/LogMessages.cs

@@ -185,7 +185,7 @@ internal static class LogMessages
         public const string IDX10674 = "IDX10674: JsonWebKeyConverter does not support SecurityKey of type: {0}";
         public const string IDX10675 = "IDX10675: Cannot create a ECDsa object from the '{0}', the bytes from the decoded value of '{1}' must be less than the size associated with the curve: '{2}'. Size was: '{3}'.";
         // public const string IDX10676 = "IDX10676:";
-        public const string IDX10677 = "IDX10677: GetKeyedHashAlgorithm threw, key: {0}, algorithm {1}.";
+        // public const string IDX10677 = "IDX10677:";
         // public const string IDX10678 = "IDX10678:";
         public const string IDX10679 = "IDX10679: Failed to decompress using algorithm '{0}'.";
         public const string IDX10680 = "IDX10680: Failed to compress using algorithm '{0}'.";

src/Microsoft.IdentityModel.Tokens/ObjectPools.cs

@@ -0,0 +1,139 @@
+//------------------------------------------------------------------------------
+//
+// Copyright (c) Microsoft Corporation.
+// All rights reserved.
+//
+// This code is licensed under the MIT License.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files(the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and / or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions :
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+//
+//------------------------------------------------------------------------------
+
+using System;
+using System.Threading;
+
+namespace Microsoft.IdentityModel.Tokens
+{
+    /// <summary>
+    /// Generic implementation of object pooling pattern with predefined pool size limit. The main
+    /// purpose is that limited number of frequently used objects can be kept in the pool for
+    /// further recycling.
+    ///
+    /// Notes: 
+    /// 1) it is not the goal to keep all returned objects. Pool is not meant for storage. If there
+    ///    is no space in the pool, extra returned objects will be dropped.
+    ///
+    /// 2) it is implied that if object was obtained from a pool, the caller will return it back in
+    ///    a relatively short time. Keeping checked out objects for long durations is ok, but
+    ///    reduces usefulness of pooling. Just new up your own.
+    ///
+    /// Not returning objects to the pool in not detrimental to the pool's work, but is a bad practice.
+    /// Rationale:
+    ///    If there is no intent for reusing the object, do not use pool - just use "new".
+    /// </summary>
+    internal sealed class DisposableObjectPool<T> where T : class, IDisposable
+    {
+        internal struct Element
+        {
+            internal T Value;
+        }
+
+        // factory is stored for the lifetime of the pool. We will call this only when pool needs to
+        // expand. compared to "new T()", Func gives more flexibility to implementers and faster
+        // than "new T()".
+        private readonly Func<T> _factory;
+
+        internal DisposableObjectPool(Func<T> factory)
+            : this(factory, Environment.ProcessorCount * 2)
+        { }
+
+        internal DisposableObjectPool(Func<T> factory, int size)
+        {
+            _factory = factory;
+            Items = new Element[size];
+        }
+
+        // storage for the pool objects.
+        internal Element[] Items { get; }
+
+        private T CreateInstance()
+        {
+            var inst = _factory();
+            return inst;
+        }
+
+        /// <summary>
+        /// Produces an instance.
+        /// </summary>
+        /// <remarks>
+        /// Search strategy is a simple linear probing which is chosen for it cache-friendliness.
+        /// Note that Free will try to store recycled objects close to the start thus statistically
+        /// reducing how far we will typically search.
+        /// </remarks>
+        internal T Allocate()
+        {
+            var items = Items;
+            T inst;
+
+            for (int i = 0; i < items.Length; i++)
+            {
+                // Note that the read is optimistically not synchronized. That is intentional. 
+                // We will interlock only when we have a candidate. in a worst case we may miss some
+                // recently returned objects. Not a big deal.
+                inst = items[i].Value;
+                if (inst != null)
+                {
+                    if (inst == Interlocked.CompareExchange(ref items[i].Value, null, inst))
+                    {
+                        goto gotInstance;
+                    }
+                }
+            }
+
+            inst = CreateInstance();
+        gotInstance:
+
+            return inst;
+        }
+
+        /// <summary>
+        /// Returns objects to the pool.
+        /// </summary>
+        /// <remarks>
+        /// Search strategy is a simple linear probing which is chosen for it cache-friendliness.
+        /// Note that Free will try to store recycled objects close to the start thus statistically
+        /// reducing how far we will typically search in Allocate.
+        /// </remarks>
+        internal void Free(T obj)
+        {
+            var items = Items;
+            for (int i = 0; i < items.Length; i++)
+            {
+                if (items[i].Value == null)
+                {
+                    // Intentionally not using interlocked here. 
+                    // In a worst case scenario two objects may be stored into same slot.
+                    // It is very unlikely to happen and will only mean that one of the objects will get collected.
+                    items[i].Value = obj;
+                    break;
+                }
+            }
+        }
+    }
+}