From 01414322a5a9ca25799fbbea251df619d417ba40 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 14 Feb 2025 17:01:49 +0100 Subject: [PATCH 1/3] Use the same cache for client and server SSL_CTX objects --- .../Interop.OpenSsl.cs | 53 ++++++------------- .../SslStreamCertificateContext.Linux.cs | 20 ++----- 2 files changed, 20 insertions(+), 53 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 9d963c4015aa5f..66d165fd902b65 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -31,15 +31,17 @@ internal static partial class OpenSsl private sealed class SafeSslContextCache : SafeHandleCache { } - private static readonly SafeSslContextCache s_clientSslContexts = new(); + private static readonly SafeSslContextCache s_sslContexts = new(); internal readonly struct SslContextCacheKey : IEquatable { + public readonly bool IsClient; public readonly byte[]? CertificateThumbprint; public readonly SslProtocols SslProtocols; - public SslContextCacheKey(SslProtocols sslProtocols, byte[]? certificateThumbprint) + public SslContextCacheKey(bool isClient, SslProtocols sslProtocols, byte[]? certificateThumbprint) { + IsClient = isClient; SslProtocols = sslProtocols; CertificateThumbprint = certificateThumbprint; } @@ -47,6 +49,7 @@ public SslContextCacheKey(SslProtocols sslProtocols, byte[]? certificateThumbpri public override bool Equals(object? obj) => obj is SslContextCacheKey key && Equals(key); public bool Equals(SslContextCacheKey other) => + IsClient == other.IsClient && SslProtocols == other.SslProtocols && (CertificateThumbprint == null && other.CertificateThumbprint == null || CertificateThumbprint != null && other.CertificateThumbprint != null && CertificateThumbprint.AsSpan().SequenceEqual(other.CertificateThumbprint)); @@ -55,6 +58,7 @@ public override int GetHashCode() { HashCode hash = default; + hash.Add(IsClient); hash.Add(SslProtocols); if (CertificateThumbprint != null) { @@ -161,41 +165,19 @@ internal static SafeSslContextHandle GetOrCreateSslContextHandle(SslAuthenticati return AllocateSslContext(sslAuthenticationOptions, protocols, allowCached); } - if (sslAuthenticationOptions.IsClient) - { - var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash(HashAlgorithmName.SHA512)); - return s_clientSslContexts.GetOrCreate(key, static (args) => - { - var (sslAuthOptions, protocols, allowCached) = args; - return AllocateSslContext(sslAuthOptions, protocols, allowCached); - }, (sslAuthenticationOptions, protocols, allowCached)); - } - - // cache in SslStreamCertificateContext is bounded and there is no eviction - // so the handle should always be valid, - bool hasAlpn = sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0; - SslProtocols serverCacheKey = protocols | (hasAlpn ? FakeAlpnSslProtocol : SslProtocols.None); - if (!sslAuthenticationOptions.CertificateContext!.SslContexts!.TryGetValue(serverCacheKey, out SafeSslContextHandle? handle)) - { - // not found in cache, create and insert - handle = AllocateSslContext(sslAuthenticationOptions, protocols, allowCached); - - SafeSslContextHandle cached = sslAuthenticationOptions.CertificateContext!.SslContexts!.GetOrAdd(serverCacheKey, handle); - - if (handle != cached) - { - // lost the race, another thread created the SSL_CTX meanwhile, prefer the cached one - handle.Dispose(); - Debug.Assert(handle.IsClosed); - handle = cached; - } - } + SslProtocols serverProtocolCacheKey = protocols | (hasAlpn ? FakeAlpnSslProtocol : SslProtocols.None); - Debug.Assert(!handle.IsClosed); - handle.TryAddRentCount(); - return handle; + var key = new SslContextCacheKey( + sslAuthenticationOptions.IsClient, + sslAuthenticationOptions.IsClient ? protocols : serverProtocolCacheKey, + sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash(HashAlgorithmName.SHA512)); + return s_sslContexts.GetOrCreate(key, static (args) => + { + var (sslAuthOptions, protocols, allowCached) = args; + return AllocateSslContext(sslAuthOptions, protocols, allowCached); + }, (sslAuthenticationOptions, protocols, allowCached)); } // This essentially wraps SSL_CTX* aka SSL_CTX_new + setting @@ -367,8 +349,7 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth { // Server should always have certificate Debug.Assert(sslAuthenticationOptions.CertificateContext != null); - if (sslAuthenticationOptions.CertificateContext == null || - sslAuthenticationOptions.CertificateContext.SslContexts == null) + if (sslAuthenticationOptions.CertificateContext == null) { cacheSslContext = false; } diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs index cba712dea97677..f7972b34b76689 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs @@ -25,25 +25,11 @@ public partial class SslStreamCertificateContext private const bool TrimRootCertificate = true; private const bool ChainBuildNeedsTrustedRoot = false; - internal ConcurrentDictionary SslContexts - { - get - { - ConcurrentDictionary? sslContexts = _sslContexts; - if (sslContexts is null) - { - Interlocked.CompareExchange(ref _sslContexts, new(), null); - sslContexts = _sslContexts; - } - - return sslContexts; - } - } - - private ConcurrentDictionary? _sslContexts; internal readonly SafeX509Handle CertificateHandle; internal readonly SafeEvpPKeyHandle KeyHandle; + internal object SyncObject => KeyHandle; + private bool _staplingForbidden; private byte[]? _ocspResponse; private DateTimeOffset _ocspExpiration; @@ -239,7 +225,7 @@ partial void AddRootCertificate(X509Certificate2? rootCertificate, ref bool tran return new ValueTask((byte[]?)null); } - lock (SslContexts) + lock (SyncObject) { pending = _pendingDownload; From fdb019a0b64a59c3468aca37e9bdb40ba5904fd4 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 17 Feb 2025 12:25:24 +0100 Subject: [PATCH 2/3] Feedback --- .../System/Net/Security/SslStreamCertificateContext.Linux.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs index f7972b34b76689..9566022bdc0885 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs @@ -28,7 +28,7 @@ public partial class SslStreamCertificateContext internal readonly SafeX509Handle CertificateHandle; internal readonly SafeEvpPKeyHandle KeyHandle; - internal object SyncObject => KeyHandle; + private object SyncObject => KeyHandle; private bool _staplingForbidden; private byte[]? _ocspResponse; From cc77e40fe677eeb21acc7158933d2cc0077fe74e Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 17 Feb 2025 16:53:30 +0100 Subject: [PATCH 3/3] Minor improvements --- .../Interop.OpenSsl.cs | 28 +++++++++++++------ .../Interop.Ssl.cs | 4 +-- .../Interop.SslCtx.cs | 4 +-- 3 files changed, 23 insertions(+), 13 deletions(-) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs index 66d165fd902b65..bd99e73adb70f9 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs @@ -376,6 +376,25 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth throw CreateSslException(SR.net_allocate_ssl_context_failed); } + if (cacheSslContext) + { + // For non-cached SSL_CTX instances, we free the `sslCtxHandle` + // after creating the SSL instance and don't use it again. We don't + // access it afterwards and OpenSSL has internal refcount which + // keeps it alive until the last SSL using it is freed. + // + // For cached SSL_CTX instances, we want to keep an outstanding + // up-ref to indicate that it is in use and does not get + // evicted from the cache. + // + // This call should always succeed because we already + // increased the rent count when getting the context from + // the cache. + bool success = sslCtxHandle.TryAddRentCount(); + Debug.Assert(success); + sslHandle.SslContextHandle = sslCtxHandle; + } + if (sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0) { if (sslAuthenticationOptions.IsServer) @@ -407,15 +426,6 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth if (cacheSslContext) { sslCtxHandle.TrySetSession(sslHandle, sslAuthenticationOptions.TargetHost); - - // Maintain additional rent count for the context so - // that it is not evicted from the cache and future - // SSL objects can reuse it. This call should always - // succeed because already have increased rent count - // when getting the context from the cache - bool success = sslCtxHandle.TryAddRentCount(); - Debug.Assert(success); - sslHandle.SslContextHandle = sslCtxHandle; } } diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs index ffd8ec5946c1f4..f3ff16df4c564d 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.Ssl.cs @@ -352,6 +352,8 @@ internal sealed class SafeSslHandle : SafeDeleteSslContext private bool _handshakeCompleted; public GCHandle AlpnHandle; + // Reference to the parent SSL_CTX handle in the SSL_CTX is being cached. Only used for + // refcount management. public SafeSslContextHandle? SslContextHandle; public bool IsServer @@ -445,8 +447,6 @@ protected override bool ReleaseHandle() Disconnect(); } - // drop reference to any SSL_CTX handle, any handle present here is being - // rented from (client) SSL_CTX cache. SslContextHandle?.Dispose(); if (AlpnHandle.IsAllocated) diff --git a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs index bad664886dac07..7c3e8d2e14bfc6 100644 --- a/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs +++ b/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.SslCtx.cs @@ -130,8 +130,8 @@ protected override bool ReleaseHandle() if (_sslSessions != null) { // The SSL_CTX is ref counted and may not immediately die when we call SslCtxDestroy() - // Since there is no relation between SafeSslContextHandle and SafeSslHandle `this` can be release - // while we still have SSL session using it. + // Since there is no relation between SafeSslContextHandle and SafeSslHandle `this` + // can be released while we still have SSL session using it. Interop.Ssl.SslCtxSetData(handle, IntPtr.Zero); lock (_sslSessions)