Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove locks in SignatureProviders #1535

Merged
merged 3 commits into from
Oct 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 21 additions & 38 deletions src/Microsoft.IdentityModel.Tokens/AsymmetricAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -67,18 +67,12 @@ 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();
private SignDelegate SignatureFunction = SignatureFunctionNotFound;
private VerifyDelegate VerifyFunction = VerifyFunctionNotFound;

#if NET461 || NETSTANDARD2_0
// HasAlgorithmName was introduced into Net46
Expand Down Expand Up @@ -335,28 +329,18 @@ private void InitializeUsingRsa(RSA rsa, string algorithm)

internal byte[] Sign(byte[] bytes)
{
if (SignatureFunction != null)
return SignatureFunction(bytes);

// we should never get here, its a bug if we do.
throw LogHelper.LogExceptionMessage(new CryptographicException(LogMessages.IDX10685));
return SignatureFunction(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

Expand All @@ -367,43 +351,42 @@ internal byte[] SignWithRsaCryptoServiceProviderProxy(byte[] bytes)
}
#endif

internal bool Verify(byte[] bytes, byte[] signature)
internal static byte[] SignatureFunctionNotFound(byte[] bytes)
{
if (VerifyFunction != null)
return VerifyFunction(bytes, signature);

// we should never get here, its a bug if we do.
throw LogHelper.LogExceptionMessage(new NotSupportedException(LogMessages.IDX10686));
throw LogHelper.LogExceptionMessage(new CryptographicException(LogMessages.IDX10685));
}

internal bool Verify(byte[] bytes, byte[] signature)
{
return VerifyFunction(bytes, 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);
GeoK marked this conversation as resolved.
Show resolved Hide resolved
}
#endif

internal static bool VerifyFunctionNotFound(byte[] bytes, byte[] signature)
{
// we should never get here, its a bug if we do.
throw LogHelper.LogExceptionMessage(new NotSupportedException(LogMessages.IDX10686));
}

// Put all the 'lightup' code here.
#if NET45
private string GetLightUpHashAlgorithmName()
Expand Down
57 changes: 36 additions & 21 deletions src/Microsoft.IdentityModel.Tokens/AsymmetricSignatureProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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, _cryptoProviderFactory.SignatureProviderObjectPoolCacheSize);
}

/// <summary>
/// Gets the mapping from algorithm to the minimum <see cref="AsymmetricSecurityKey"/>.KeySize for creating signatures.
Expand Down Expand Up @@ -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);
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
return new AsymmetricAdapter(Key, Algorithm, _cryptoProviderFactory.CreateHashAlgorithm(hashAlgoritmName), hashAlgoritmName, WillCreateSignatures);
brentschmaltz marked this conversation as resolved.
Show resolved Hide resolved
}
#endif

Expand All @@ -227,19 +227,17 @@ 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

/// <summary>
/// For testing purposes
/// </summary>
internal override int ObjectPoolSize => _asymmetricAdapterObjectPool.Size;

/// <summary>
/// Produces a signature over the 'input' using the <see cref="AsymmetricSecurityKey"/> and algorithm passed to <see cref="AsymmetricSignatureProvider( SecurityKey, string, bool )"/>.
/// </summary>
Expand All @@ -260,15 +258,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>
Expand Down Expand Up @@ -350,15 +356,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>
Expand All @@ -370,11 +384,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();
}
}
}
Expand Down
29 changes: 26 additions & 3 deletions src/Microsoft.IdentityModel.Tokens/CryptoProviderFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ public class CryptoProviderFactory
private static CryptoProviderFactory _default;
private static ConcurrentDictionary<string, string> _typeToAlgorithmMap = new ConcurrentDictionary<string, string>();
private static object _cacheLock = new object();
private static int _defaultSignatureProviderObjectPoolCacheSize = Environment.ProcessorCount * 4;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curios on why we decided to change the default object pool size?

private int _signatureProviderObjectPoolCacheSize = _defaultSignatureProviderObjectPoolCacheSize;

/// <summary>
/// Returns the default <see cref="CryptoProviderFactory"/> instance.
Expand All @@ -60,6 +62,15 @@ public static CryptoProviderFactory Default
[DefaultValue(true)]
public static bool DefaultCacheSignatureProviders { get; set; } = true;

/// <summary>
/// Gets or sets the maximum size of the object pool used by the SignatureProvider that are used for crypto objects.
/// </summary>
public static int DefaultSignatureProviderObjectPoolCacheSize
{
get => _defaultSignatureProviderObjectPoolCacheSize;
set => _defaultSignatureProviderObjectPoolCacheSize = value > 0 ? value : throw LogHelper.LogExceptionMessage(new ArgumentOutOfRangeException(nameof(value), LogHelper.FormatInvariant(LogMessages.IDX10698, value)));
}

/// <summary>
/// Static constructor that initializes the default <see cref="CryptoProviderFactory"/>.
/// </summary>
Expand All @@ -85,6 +96,8 @@ public CryptoProviderFactory(CryptoProviderFactory other)
throw LogHelper.LogArgumentNullException(nameof(other));

CustomCryptoProvider = other.CustomCryptoProvider;
CacheSignatureProviders = other.CacheSignatureProviders;
SignatureProviderObjectPoolCacheSize = other.SignatureProviderObjectPoolCacheSize;
}

/// <summary>
Expand All @@ -106,6 +119,16 @@ public CryptoProviderFactory(CryptoProviderFactory other)
[DefaultValue(true)]
public bool CacheSignatureProviders { get; set; } = DefaultCacheSignatureProviders;

/// <summary>
/// Gets or sets the maximum size of the object pool used by the SignatureProvider that are used for crypto objects.
/// </summary>
public int SignatureProviderObjectPoolCacheSize
{
get => _signatureProviderObjectPoolCacheSize;

set => _signatureProviderObjectPoolCacheSize = value > 0 ? value : throw LogHelper.LogExceptionMessage(new ArgumentOutOfRangeException(nameof(value), LogHelper.FormatInvariant(LogMessages.IDX10698, value)));
}

/// <summary>
/// Creates an instance of <see cref="AuthenticatedEncryptionProvider"/> for a specific &lt;SecurityKey, Algorithm>.
/// </summary>
Expand Down Expand Up @@ -220,7 +243,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
Expand Down Expand Up @@ -251,7 +274,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
Expand Down Expand Up @@ -557,7 +580,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();
}

Expand Down
14 changes: 8 additions & 6 deletions src/Microsoft.IdentityModel.Tokens/ECDsaAdapter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ namespace Microsoft.IdentityModel.Tokens
/// </summary>
internal class ECDsaAdapter
{
internal readonly CreateECDsaDelegate CreateECDsaFunction = null;
internal readonly CreateECDsaDelegate CreateECDsaFunction = ECDsaNotSupported;
internal static ECDsaAdapter Instance = new ECDsaAdapter();

/// <summary>
Expand All @@ -67,11 +67,7 @@ internal ECDsaAdapter()
/// </summary>
internal ECDsa CreateECDsa(JsonWebKey jsonWebKey, bool usePrivateKey)
{
if (CreateECDsaFunction != null)
return CreateECDsaFunction(jsonWebKey, usePrivateKey);

// we will get here on platforms that are not supported.
throw LogHelper.LogExceptionMessage(new PlatformNotSupportedException(LogMessages.IDX10690));
return CreateECDsaFunction(jsonWebKey, usePrivateKey);
}

/// <summary>
Expand Down Expand Up @@ -170,6 +166,12 @@ private ECDsa CreateECDsaUsingCNGKey(JsonWebKey jsonWebKey, bool usePrivateKey)
}
}

internal static ECDsa ECDsaNotSupported(JsonWebKey jsonWebKey, bool usePrivateKey)
{
// we will get here on platforms that are not supported.
throw LogHelper.LogExceptionMessage(new PlatformNotSupportedException(LogMessages.IDX10690));
}

/// <summary>
/// Returns the size of key in bytes
/// </summary>
Expand Down
3 changes: 2 additions & 1 deletion src/Microsoft.IdentityModel.Tokens/LogMessages.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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}'.";
Expand All @@ -206,6 +206,7 @@ internal static class LogMessages
public const string IDX10695 = "IDX10695: Unable to create a JsonWebKey from an ECDsa object. Required ECParameters structure is not supported by .NET Framework < 4.7.";
public const string IDX10696 = "IDX10696: The algorithm '{0}' is not in the user-defined accepted list of algorithms.";
public const string IDX10697 = "IDX10697: The user defined 'Delegate' AlgorithmValidator specified on TokenValidationParameters returned false when validating Algorithm: '{0}', SecurityKey: '{1}'.";
public const string IDX10698 = "IDX10698: The SignatureProviderObjectPoolCacheSize must be greater than 0. Value: '{0}'.";

// security keys
public const string IDX10700 = "IDX10700: {0} is unable to use 'rsaParameters'. {1} is null.";
Expand Down
Loading