From 1b628022dea0893b2ea1b3cfff343f5f614d17f3 Mon Sep 17 00:00:00 2001 From: wfurt Date: Sun, 20 Sep 2026 19:34:05 +0000 Subject: [PATCH 1/2] Pool TLS 1.3 session tickets per host instead of keeping one On Linux the client TLS session cache kept exactly one SSL_SESSION per SNI host. For TLS 1.3 that is wrong in two compounding ways. An OpenSSL server sends two NewSessionTickets after an initial handshake, so new_session_cb fires twice. The old TryAddSession handled the second by evicting the first and calling SSL_CTX_remove_session on it, and remove_session_lock sets not_resumable = 1 unconditionally. The discarded ticket was therefore invalidated, not merely dropped, on every connection. Concurrent connections then contend for the single survivor. SSL_set_session shares the pointer rather than copying, so when the first of several concurrent handshakes completes, tls_finish_handshake removes that shared session and marks it not_resumable; every other connection holding it falls back to a full handshake. Cache TLS 1.3 tickets in a small per-host pool so concurrent handshakes can each take a distinct one. TLS 1.2 still keeps a single entry, which is both sufficient and all OpenSSL ever provides, since it skips new_session_cb on resumed TLS 1.2 handshakes. Add a CryptoNative_SslSessionUpRef shim so TrySetSession can take a reference under the managed lock, release the lock, and only then call SSL_set_session. RemoveSession frees outside that lock, so relying on the lock alone to keep the session alive across the call was unsound. Measured with the concurrent handshake benchmark from dotnet/performance#5314 over loopback sockets, TLS 1.3 with resumption: 14% faster at 64 concurrent connections and 12% at 128. TLS 1.2 and the non-resuming cases are unchanged; low concurrency costs 2-3%. --- .../Interop.OpenSsl.cs | 8 +- .../Interop.Ssl.cs | 6 + .../Interop.SslCtx.cs | 180 +++++++++++++----- .../entrypoints.c | 1 + .../opensslshim.h | 2 + .../pal_ssl.c | 5 + .../pal_ssl.h | 5 + 7 files changed, 157 insertions(+), 50 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 c783ba680e0c0d..32bc1933d41552 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 @@ -29,7 +29,7 @@ internal static partial class OpenSsl // Special value of 0 means unlimited, -1 means the implementation (OpenSSL) default, which is currently 20 * 1024. private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize"; private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE"; - private const int DefaultTlsCacheSizeClient = 500; // since we keep only one TLS Session per hostname, 500 should be enough to cover most scenarios + private const int DefaultTlsCacheSizeClient = 500; // bounds the total number of pooled client sessions across all hostnames private const int DefaultTlsCacheSizeServer = -1; // use implementation default private const SslProtocols FakeAlpnSslProtocol = (SslProtocols)1; // used to distinguish server sessions with ALPN private static readonly Lazy s_defaultSigAlgs = new(GetDefaultSignatureAlgorithms); @@ -1299,7 +1299,11 @@ private static unsafe int NewSessionCallback(IntPtr ssl, IntPtr session) if (ctxHandle != null) { - if (ctxHandle.TryAddSession(name, session)) + // TLS 1.3 tickets are single-use, TLS 1.2 sessions are not, and the two + // need opposite caching policies. + ReadOnlySpan version = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(Ssl.SslGetVersion(ssl)); + + if (ctxHandle.TryAddSession(name, session, version.SequenceEqual("TLSv1.3"u8))) { // offered session was stored in our cache. return 1; 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 4b4269bb23af8d..1e4097e3ab6e10 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 @@ -50,6 +50,9 @@ internal static partial class Ssl [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetVersion")] internal static partial IntPtr SslGetVersion(SafeSslHandle ssl); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetVersion")] + internal static unsafe partial byte* SslGetVersion(IntPtr ssl); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSetTlsExtHostName", StringMarshalling = StringMarshalling.Utf8)] [return: MarshalAs(UnmanagedType.Bool)] internal static partial bool SslSetTlsExtHostName(SafeSslHandle ssl, string host); @@ -304,6 +307,9 @@ internal static SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl) [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionFree")] internal static partial void SessionFree(IntPtr session); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionUpRef")] + internal static partial int SessionUpRef(IntPtr session); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionSetHostname")] internal static unsafe partial int SessionSetHostname(IntPtr session, byte* name); 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 1579ec9c3e22c7..041c0ddda75820 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 @@ -79,8 +79,21 @@ namespace Microsoft.Win32.SafeHandles { internal sealed class SafeSslContextHandle : SafeHandle, ISafeHandleCachable { + // OpenSSL retires a TLS 1.3 session when the handshake using it finishes + // (tls_finish_handshake calls SSL_CTX_remove_session, which sets not_resumable on + // the shared object), so offering one session to several concurrent handshakes + // silently downgrades all but the first to a full handshake. Pooling several + // tickets per host lets concurrent connections each take a distinct one. + private const int TlsResumePoolSize = 8; + + private readonly struct CachedSession(IntPtr session, bool isTls13) + { + public IntPtr Session { get; } = session; + public bool IsTls13 { get; } = isTls13; + } + // This is session cache keyed by SNI e.g. TargetHost - private Dictionary? _sslSessions; + private Dictionary>? _sslSessions; private GCHandle _gch; // SSL_CTX handles are cached, so we need to keep track of the @@ -146,9 +159,12 @@ protected override bool ReleaseHandle() lock (_sslSessions) { - foreach (IntPtr session in _sslSessions.Values) + foreach (List sessions in _sslSessions.Values) { - Interop.Ssl.SessionFree(session); + foreach (CachedSession cached in sessions) + { + Interop.Ssl.SessionFree(cached.Session); + } } _sslSessions.Clear(); @@ -168,14 +184,14 @@ internal void EnableSessionCache() { Debug.Assert(_sslSessions == null); - _sslSessions = new Dictionary(); + _sslSessions = new Dictionary>(); _gch = GCHandle.Alloc(this); Debug.Assert(_gch.IsAllocated); // This is needed so we can find the handle from session in SessionRemove callback. Interop.Ssl.SslCtxSetData(this, (IntPtr)_gch); } - internal unsafe bool TryAddSession(byte* namePtr, IntPtr session) + internal unsafe bool TryAddSession(byte* namePtr, IntPtr session, bool isTls13) { Debug.Assert(_sslSessions != null && session != IntPtr.Zero); @@ -187,70 +203,112 @@ internal unsafe bool TryAddSession(byte* namePtr, IntPtr session) string? targetName = Utf8StringMarshaller.ConvertToManaged(namePtr); Debug.Assert(targetName != null); - if (!string.IsNullOrEmpty(targetName)) + if (string.IsNullOrEmpty(targetName)) { - // We do this only for lookup in RemoveSession. - // Since this is part of cache manipulation and no function impact it is done here. - // This will use strdup() so it is safe to pass in raw pointer. - Interop.Ssl.SessionSetHostname(session, namePtr); + return false; + } - IntPtr oldSession = IntPtr.Zero; + // We do this only for lookup in RemoveSession. + // Since this is part of cache manipulation and no function impact it is done here. + // This will use strdup() so it is safe to pass in raw pointer. + Interop.Ssl.SessionSetHostname(session, namePtr); - lock (_sslSessions) + // A TLS 1.2 session stays usable after a resumption and is never replaced by a + // new one (OpenSSL skips new_session_cb on resumed TLS 1.2 handshakes), so a + // single entry is both sufficient and all we will ever be given. + int limit = isTls13 ? TlsResumePoolSize : 1; + + IntPtr[]? evicted = null; + int evictedCount = 0; + + lock (_sslSessions) + { + if (!_sslSessions.TryGetValue(targetName, out List? sessions)) { - if (!_sslSessions.TryAdd(targetName, session)) - { - // session to this target host exists, replace it - _sslSessions.Remove(targetName, out oldSession); - bool added = _sslSessions.TryAdd(targetName, session); - Debug.Assert(added); - } + sessions = new List(); + _sslSessions[targetName] = sessions; } - if (oldSession != IntPtr.Zero) + // Pooled tickets are only usable by the protocol version that produced them, + // so a change of negotiated version drops the pool rather than leaving a + // stale entry at the head masking everything behind it. + int toEvict = sessions.Count > 0 && sessions[0].IsTls13 != isTls13 + ? sessions.Count + : Math.Max(0, sessions.Count - limit + 1); + + if (toEvict > 0) { - // remove old session also from the internal OpenSSL cache - // and drop reference count. Since SSL_CTX_remove_session - // will call session_remove_cb, we need to do this outside - // of _sslSessions lock to avoid deadlock with another thread - // which could be holding SSL_CTX lock and trying to acquire - // _sslSessions lock. - Interop.Ssl.SslCtxRemoveSession(this, oldSession); - Interop.Ssl.SessionFree(oldSession); + evicted = new IntPtr[toEvict]; + for (; evictedCount < toEvict; evictedCount++) + { + evicted[evictedCount] = sessions[evictedCount].Session; + } + + sessions.RemoveRange(0, toEvict); } - return true; + sessions.Add(new CachedSession(session, isTls13)); + } + + for (int i = 0; i < evictedCount; i++) + { + // Remove the evicted session also from the internal OpenSSL cache and drop + // the reference count. Since SSL_CTX_remove_session will call + // session_remove_cb, we need to do this outside of the _sslSessions lock to + // avoid deadlock with another thread which could be holding the SSL_CTX lock + // and trying to acquire _sslSessions. + Interop.Ssl.SslCtxRemoveSession(this, evicted![i]); + Interop.Ssl.SessionFree(evicted[i]); } - return false; + return true; } internal unsafe void RemoveSession(byte* namePtr, IntPtr session) { Debug.Assert(_sslSessions != null); + if (_sslSessions == null || namePtr == null) + { + return; + } + string? targetName = Utf8StringMarshaller.ConvertToManaged(namePtr); Debug.Assert(targetName != null); - if (_sslSessions != null && targetName != null) + if (targetName == null) { - IntPtr oldSession = IntPtr.Zero; - bool removed = false; - lock (_sslSessions) + return; + } + + bool removed = false; + + lock (_sslSessions) + { + if (_sslSessions.TryGetValue(targetName, out List? sessions)) { - if (_sslSessions.TryGetValue(targetName, out IntPtr existingSession) && existingSession == session) + for (int i = 0; i < sessions.Count; i++) { - removed = _sslSessions.Remove(targetName, out oldSession); + if (sessions[i].Session == session) + { + sessions.RemoveAt(i); + removed = true; + break; + } } - } - if (removed) - { - // It seems like we may be called more than once. Since we grabbed only one refference - // when added to Dictionary, we will also drop exactly one when removed. - Interop.Ssl.SessionFree(oldSession); + if (sessions.Count == 0) + { + _sslSessions.Remove(targetName); + } } + } + if (removed) + { + // It seems like we may be called more than once. Since we grabbed only one + // reference when added to the cache, we will also drop exactly one when removed. + Interop.Ssl.SessionFree(session); } } @@ -263,18 +321,44 @@ internal bool TrySetSession(SafeSslHandle sslHandle, string name) return false; } + IntPtr owned; + lock (_sslSessions) { - if (_sslSessions.TryGetValue(name, out IntPtr session)) + if (!_sslSessions.TryGetValue(name, out List? sessions) || sessions.Count == 0) + { + return false; + } + + CachedSession cached = sessions[0]; + + // While the pool holds more than one ticket each concurrent handshake can + // take its own. The last one is still shared rather than withheld, since a + // shared ticket only costs a fallback to a full handshake, while withholding + // it guarantees one. + bool singleUse = cached.IsTls13 && sessions.Count > 1; + + if (singleUse) + { + // Taking the entry out of the cache transfers the cache's reference to us. + // The pool holds more than one entry here, so it cannot become empty. + sessions.RemoveAt(0); + } + else if (Interop.Ssl.SessionUpRef(cached.Session) != 1) { - // This will increase reference count on the session as needed. - // We need to hold lock here to prevent session being deleted before the call is done. - Interop.Ssl.SslSetSession(sslHandle, session); - return true; + return false; } + + owned = cached.Session; } - return false; + // Held outside the lock: RemoveSession frees on a callback OpenSSL raises while + // holding the SSL_CTX lock, so the reference taken above, not the lock, is what + // keeps the session alive across this call. + bool set = Interop.Ssl.SslSetSession(sslHandle, owned) == 1; + Interop.Ssl.SessionFree(owned); + + return set; } } } diff --git a/src/native/libs/System.Security.Cryptography.Native/entrypoints.c b/src/native/libs/System.Security.Cryptography.Native/entrypoints.c index f086723428540b..dab76aae4696e2 100644 --- a/src/native/libs/System.Security.Cryptography.Native/entrypoints.c +++ b/src/native/libs/System.Security.Cryptography.Native/entrypoints.c @@ -408,6 +408,7 @@ static const Entry s_cryptoNative[] = DllImportEntry(CryptoNative_SslSessionFree) DllImportEntry(CryptoNative_SslSessionGetHostname) DllImportEntry(CryptoNative_SslSessionSetHostname) + DllImportEntry(CryptoNative_SslSessionUpRef) DllImportEntry(CryptoNative_SslSessionReused) DllImportEntry(CryptoNative_SslSessionGetData) DllImportEntry(CryptoNative_SslSessionSetData) diff --git a/src/native/libs/System.Security.Cryptography.Native/opensslshim.h b/src/native/libs/System.Security.Cryptography.Native/opensslshim.h index 9be8eb55fa1431..b3823fd4f6fa55 100644 --- a/src/native/libs/System.Security.Cryptography.Native/opensslshim.h +++ b/src/native/libs/System.Security.Cryptography.Native/opensslshim.h @@ -771,6 +771,7 @@ extern bool g_libSslUses32BitTime; REQUIRED_FUNCTION(SSL_SESSION_set_ex_data) \ REQUIRED_FUNCTION(SSL_SESSION_get0_hostname) \ REQUIRED_FUNCTION(SSL_SESSION_set1_hostname) \ + REQUIRED_FUNCTION(SSL_SESSION_up_ref) \ REQUIRED_FUNCTION(SSL_session_reused) \ REQUIRED_FUNCTION(SSL_set_accept_state) \ REQUIRED_FUNCTION(SSL_set_bio) \ @@ -1379,6 +1380,7 @@ extern TYPEOF(OPENSSL_gmtime)* OPENSSL_gmtime_ptr; #define SSL_SESSION_free SSL_SESSION_free_ptr #define SSL_SESSION_get0_hostname SSL_SESSION_get0_hostname_ptr #define SSL_SESSION_set1_hostname SSL_SESSION_set1_hostname_ptr +#define SSL_SESSION_up_ref SSL_SESSION_up_ref_ptr #define SSL_session_reused SSL_session_reused_ptr #define SSL_SESSION_get_ex_data SSL_SESSION_get_ex_data_ptr #define SSL_SESSION_set_ex_data SSL_SESSION_set_ex_data_ptr diff --git a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c index b1be9276d696c0..7bed7ec8fb83fb 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c @@ -874,6 +874,11 @@ void CryptoNative_SslSessionFree(SSL_SESSION* session) SSL_SESSION_free(session); } +int32_t CryptoNative_SslSessionUpRef(SSL_SESSION* session) +{ + return SSL_SESSION_up_ref(session); +} + const char* CryptoNative_SslSessionGetHostname(SSL_SESSION* session) { return SSL_SESSION_get0_hostname(session); diff --git a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h index 024dc4d3716b6d..0d86c460c038bf 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h @@ -207,6 +207,11 @@ PALEXPORT int32_t CryptoNative_SslSetSession(SSL* ssl, SSL_SESSION* session); */ PALEXPORT void CryptoNative_SslSessionFree(SSL_SESSION* session); +/* + * Takes an additional reference on an SSL session. Returns 1 on success, 0 on failure. + */ +PALEXPORT int32_t CryptoNative_SslSessionUpRef(SSL_SESSION* session); + /* * Get name associated with given SSL_SESSION. */ From 9cbaa9e55f80be6c7529de1b455fa3d72fb2c3db Mon Sep 17 00:00:00 2001 From: wfurt Date: Mon, 28 Sep 2026 22:22:31 +0000 Subject: [PATCH 2/2] Use CollectionsMarshal for TLS session pool lookup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../System.Security.Cryptography.Native/Interop.SslCtx.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) 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 041c0ddda75820..7815b0b3d70056 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 @@ -223,11 +223,8 @@ internal unsafe bool TryAddSession(byte* namePtr, IntPtr session, bool isTls13) lock (_sslSessions) { - if (!_sslSessions.TryGetValue(targetName, out List? sessions)) - { - sessions = new List(); - _sslSessions[targetName] = sessions; - } + ref List? sessions = ref CollectionsMarshal.GetValueRefOrAddDefault(_sslSessions, targetName, out _); + sessions ??= new List(); // Pooled tickets are only usable by the protocol version that produced them, // so a change of negotiated version drops the pool rather than leaving a