From fabb9a3ac0ce395233546148a494098ca2d95dbd Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 23 May 2024 16:41:41 +0200 Subject: [PATCH 01/16] Change SSL_CTX caching --- .../Interop.OpenSsl.cs | 67 +++++++++++++++---- .../System/Net/Security/SslStreamPal.Unix.cs | 22 ++++-- 2 files changed, 71 insertions(+), 18 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 171980d0a7654..89a84347ab760 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 @@ -24,7 +24,39 @@ internal static partial class OpenSsl private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize"; private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE"; private const SslProtocols FakeAlpnSslProtocol = (SslProtocols)1; // used to distinguish server sessions with ALPN - private static readonly ConcurrentDictionary s_clientSslContexts = new ConcurrentDictionary(); + private static readonly ConcurrentDictionary s_clientSslContexts = new ConcurrentDictionary(); + + internal readonly struct SslContextCacheKey : IEquatable + { + public readonly byte[]? CertificateThumbprint; + public readonly SslProtocols SslProtocols; + + public SslContextCacheKey(SslProtocols sslProtocols, byte[]? certificateThumbprint) + { + SslProtocols = sslProtocols; + CertificateThumbprint = certificateThumbprint; + } + + public override bool Equals(object? obj) => obj is SslContextCacheKey key && Equals(key); + + public bool Equals(SslContextCacheKey other) => + SslProtocols == other.SslProtocols && + (CertificateThumbprint == null && other.CertificateThumbprint == null || + CertificateThumbprint != null && other.CertificateThumbprint != null && CertificateThumbprint.AsSpan().SequenceEqual(other.CertificateThumbprint)); + + public override int GetHashCode() + { + HashCode hash = default; + + hash.Add(SslProtocols); + if (CertificateThumbprint != null) + { + hash.AddBytes(CertificateThumbprint); + } + + return hash.ToHashCode(); + } + } #region internal methods internal static SafeChannelBindingHandle? QueryChannelBinding(SafeSslHandle context, ChannelBindingKind bindingType) @@ -188,7 +220,7 @@ internal static unsafe SafeSslContextHandle AllocateSslContext(SslAuthentication Interop.Ssl.SslCtxSetAlpnSelectCb(sslCtx, &AlpnServerSelectCallback, IntPtr.Zero); } - if (sslAuthenticationOptions.CertificateContext != null) + if (sslAuthenticationOptions.CertificateContext != null && sslAuthenticationOptions.IsServer) { SetSslCertificate(sslCtx, sslAuthenticationOptions.CertificateContext.CertificateHandle, sslAuthenticationOptions.CertificateContext.KeyHandle); @@ -269,13 +301,12 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth { // We don't support client resume on old OpenSSL versions. // We don't want to try on empty TargetName since that is our key. - // And we don't want to mess up with client authentication. It may be possible - // but it seems safe to get full new session. + // If we already have CertificateContext, then we know which cert the user wants to use and we can cache. + // The only client auth scenario where we can't cache is when user provides a cert callback and we don't know + // beforehand which cert will be used. and wan't to avoid resuming session created with different certificate. if (!Interop.Ssl.Capabilities.Tls13Supported || string.IsNullOrEmpty(sslAuthenticationOptions.TargetHost) || - sslAuthenticationOptions.CertificateContext != null || - sslAuthenticationOptions.ClientCertificates?.Count > 0 || - sslAuthenticationOptions.CertSelectionDelegate != null) + (sslAuthenticationOptions.CertificateContext == null && sslAuthenticationOptions.CertSelectionDelegate != null)) { cacheSslContext = false; } @@ -300,8 +331,8 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth } else { - - s_clientSslContexts.TryGetValue(protocols, out sslCtxHandle); + var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash()); + s_clientSslContexts.TryGetValue(key, out sslCtxHandle); } } @@ -312,9 +343,18 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth if (cacheSslContext) { - bool added = sslAuthenticationOptions.IsServer ? - sslAuthenticationOptions.CertificateContext!.SslContexts!.TryAdd(protocols | (SslProtocols)(hasAlpn ? 1 : 0), newCtxHandle) : - s_clientSslContexts.TryAdd(protocols, newCtxHandle); + bool added; + + if (sslAuthenticationOptions.IsServer) + { + added = sslAuthenticationOptions.CertificateContext!.SslContexts!.TryAdd(protocols | (SslProtocols)(hasAlpn ? 1 : 0), newCtxHandle); + } + else + { + var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash()); + added = s_clientSslContexts.TryAdd(key, newCtxHandle); + } + if (added) { newCtxHandle = null; @@ -373,7 +413,8 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth // relevant to TLS 1.3 only: if user supplied a client cert or cert callback, // advertise that we are willing to send the certificate post-handshake. - if (sslAuthenticationOptions.ClientCertificates?.Count > 0 || + if (sslAuthenticationOptions.CertificateContext != null || + sslAuthenticationOptions.ClientCertificates?.Count > 0 || sslAuthenticationOptions.CertSelectionDelegate != null) { Ssl.SslSetPostHandshakeAuth(sslHandle, 1); diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs index 1ebe6467d0f63..c5bc8c2b846e0 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs @@ -17,7 +17,7 @@ public static Exception GetException(SecurityStatusPal status) return status.Exception ?? new Interop.OpenSsl.SslException((int)status.ErrorCode); } - internal const bool StartMutualAuthAsAnonymous = true; + internal const bool StartMutualAuthAsAnonymous = false; internal const bool CanEncryptEmptyMessage = false; public static void VerifyPackageInfo() @@ -168,8 +168,8 @@ public static bool TryUpdateClintCertificate( return true; } - private static ProtocolToken HandshakeInternal(ref SafeDeleteSslContext? context, - ReadOnlySpan inputBuffer, SslAuthenticationOptions sslAuthenticationOptions) + private static ProtocolToken HandshakeInternal(ref SafeDeleteSslContext? context, + ReadOnlySpan inputBuffer, SslAuthenticationOptions sslAuthenticationOptions) { ProtocolToken token = default; token.RentBuffer = true; @@ -186,8 +186,20 @@ private static ProtocolToken HandshakeInternal(ref SafeDeleteSslContext? context { // this should happen only for clients Debug.Assert(sslAuthenticationOptions.IsClient); - token.Status = new SecurityStatusPal(errorCode); - return token; + + // if we don't have a clien certificate ready, bubble up so + // that the certificate selection routine runs again. This + // happens if the first call to LocalCertificateSelectionCallback + // returns null. + if (sslAuthenticationOptions.CertificateContext == null) + { + token.Status = new SecurityStatusPal(SecurityStatusPalErrorCode.CredentialsNeeded); + return token; + } + + // set the cert and continue + TryUpdateClintCertificate(null, context, sslAuthenticationOptions); + errorCode = Interop.OpenSsl.DoSslHandshake((SafeSslHandle)context, ReadOnlySpan.Empty, ref token); } // sometimes during renegotiation processing message does not yield new output. From 5f30d11666d8ec08bd4b05cbb964c25ceadcc426 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 23 May 2024 16:45:41 +0200 Subject: [PATCH 02/16] Add failing test --- .../SslStreamMutualAuthenticationTest.cs | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index 7a858a644bab3..e475471abd287 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.IO; +using System.Collections.Generic; using System.Threading.Tasks; using System.Net.Test.Common; using System.Security.Authentication; @@ -115,11 +116,19 @@ public async Task SslStream_RequireClientCert_IsMutuallyAuthenticated_ReturnsTru } } } + public static IEnumerable SslProtocolsAndBoolData() + { + foreach (object[] protocol in new SslProtocolSupport.SupportedSslProtocolsTestData()) + { + yield return new object[] { protocol[0], true }; + yield return new object[] { protocol[0], false }; + } + } [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] - [ClassData(typeof(SslProtocolSupport.SupportedSslProtocolsTestData))] + [MemberData(nameof(SslProtocolsAndBoolData))] public async Task SslStream_CachedCredentials_IsMutuallyAuthenticatedCorrect( - SslProtocols protocol) + SslProtocols protocol, bool startWithMtls) { var clientOptions = new SslClientAuthenticationOptions { @@ -129,18 +138,20 @@ public async Task SslStream_CachedCredentials_IsMutuallyAuthenticatedCorrect( TargetHost = Guid.NewGuid().ToString("N") }; + SslStreamCertificateContext context = SslStreamCertificateContext.Create(_serverCertificate, null); + for (int i = 0; i < 5; i++) { (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(); using (client) using (server) { - bool expectMutualAuthentication = (i % 2) == 0; + bool expectMutualAuthentication = (i % 2) == (startWithMtls ? 0 : 1); var serverOptions = new SslServerAuthenticationOptions { ClientCertificateRequired = expectMutualAuthentication, - ServerCertificate = expectMutualAuthentication ? _serverCertificate : _selfSignedCertificate, + ServerCertificateContext = context, RemoteCertificateValidationCallback = delegate { return true; }, EnabledSslProtocols = protocol }; @@ -193,7 +204,6 @@ public async Task SslStream_NegotiateClientCertificate_IsMutuallyAuthenticatedCo ServerCertificateContext = context, ClientCertificateRequired = false, EnabledSslProtocols = SslProtocols.Tls12, - }); await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2); @@ -266,7 +276,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -320,7 +330,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -357,7 +367,7 @@ public async Task SslStream_ResumedSessionsCallbackMaybeSet_IsMutuallyAuthentica if (expectMutualAuthentication) { - clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; + clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; } else { @@ -378,7 +388,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } From c401a27661729b01ad8814d76b3257d53f661948 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 23 May 2024 18:50:04 +0200 Subject: [PATCH 03/16] Revert "Add failing test" This reverts commit 5f30d11666d8ec08bd4b05cbb964c25ceadcc426. --- .../SslStreamMutualAuthenticationTest.cs | 28 ++++++------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index e475471abd287..7a858a644bab3 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.IO; -using System.Collections.Generic; using System.Threading.Tasks; using System.Net.Test.Common; using System.Security.Authentication; @@ -116,19 +115,11 @@ public async Task SslStream_RequireClientCert_IsMutuallyAuthenticated_ReturnsTru } } } - public static IEnumerable SslProtocolsAndBoolData() - { - foreach (object[] protocol in new SslProtocolSupport.SupportedSslProtocolsTestData()) - { - yield return new object[] { protocol[0], true }; - yield return new object[] { protocol[0], false }; - } - } [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotWindows7))] - [MemberData(nameof(SslProtocolsAndBoolData))] + [ClassData(typeof(SslProtocolSupport.SupportedSslProtocolsTestData))] public async Task SslStream_CachedCredentials_IsMutuallyAuthenticatedCorrect( - SslProtocols protocol, bool startWithMtls) + SslProtocols protocol) { var clientOptions = new SslClientAuthenticationOptions { @@ -138,20 +129,18 @@ public async Task SslStream_CachedCredentials_IsMutuallyAuthenticatedCorrect( TargetHost = Guid.NewGuid().ToString("N") }; - SslStreamCertificateContext context = SslStreamCertificateContext.Create(_serverCertificate, null); - for (int i = 0; i < 5; i++) { (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(); using (client) using (server) { - bool expectMutualAuthentication = (i % 2) == (startWithMtls ? 0 : 1); + bool expectMutualAuthentication = (i % 2) == 0; var serverOptions = new SslServerAuthenticationOptions { ClientCertificateRequired = expectMutualAuthentication, - ServerCertificateContext = context, + ServerCertificate = expectMutualAuthentication ? _serverCertificate : _selfSignedCertificate, RemoteCertificateValidationCallback = delegate { return true; }, EnabledSslProtocols = protocol }; @@ -204,6 +193,7 @@ public async Task SslStream_NegotiateClientCertificate_IsMutuallyAuthenticatedCo ServerCertificateContext = context, ClientCertificateRequired = false, EnabledSslProtocols = SslProtocols.Tls12, + }); await TestConfiguration.WhenAllOrAnyFailedWithTimeout(t1, t2); @@ -276,7 +266,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -330,7 +320,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -367,7 +357,7 @@ public async Task SslStream_ResumedSessionsCallbackMaybeSet_IsMutuallyAuthentica if (expectMutualAuthentication) { - clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; + clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; } else { @@ -388,7 +378,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } From af6a6d3f78b5fe61cab1381807421f05aa926479 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 24 May 2024 14:00:42 +0200 Subject: [PATCH 04/16] WIP, tests pass. --- .../Interop.OpenSsl.cs | 6 ++ .../Interop.Ssl.cs | 15 +++++ .../Net/CertificateValidationPal.Unix.cs | 40 +++++++++++- .../SslStreamAllowTlsResumeTests.cs | 2 +- .../SslStreamMutualAuthenticationTest.cs | 8 +-- .../entrypoints.c | 4 ++ .../openssl.c | 61 +++++++++++++++++-- .../openssl.h | 4 ++ .../opensslshim.h | 8 +++ .../pal_ssl.c | 20 ++++++ .../pal_ssl.h | 22 +++++++ 11 files changed, 178 insertions(+), 12 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 89a84347ab760..6093e684d65fe 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 @@ -749,6 +749,12 @@ private static unsafe int NewSessionCallback(IntPtr ssl, IntPtr session) Debug.Assert(ssl != IntPtr.Zero); Debug.Assert(session != IntPtr.Zero); + // remember if the session used a certificate, this information is used after + // session resumption, the pointer is not being dereferenced and the refcount + // is not going to be manipulated. + IntPtr cert = Interop.Ssl.SslGetCertificate(ssl); + Interop.Ssl.SslSessionSetData(session, cert); + IntPtr ptr = Ssl.SslGetData(ssl); if (ptr != IntPtr.Zero) { 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 e1f2dfdc1f23e..be3176f4a752a 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 @@ -116,6 +116,12 @@ internal static unsafe ReadOnlySpan SslGetAlpnSelected(SafeSslHandle ssl) [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerCertificate")] internal static partial IntPtr SslGetPeerCertificate(SafeSslHandle ssl); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetCertificate")] + internal static partial IntPtr SslGetCertificate(SafeSslHandle ssl); + + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetCertificate")] + internal static partial IntPtr SslGetCertificate(IntPtr ssl); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetPeerCertChain")] internal static partial SafeSharedX509StackHandle SslGetPeerCertChain(SafeSslHandle ssl); @@ -129,6 +135,9 @@ internal static unsafe ReadOnlySpan SslGetAlpnSelected(SafeSslHandle ssl) [return: MarshalAs(UnmanagedType.Bool)] internal static partial bool SslSessionReused(SafeSslHandle ssl); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetSession")] + internal static partial IntPtr SslGetSession(SafeSslHandle ssl); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslGetClientCAList")] private static partial SafeSharedX509NameStackHandle SslGetClientCAList_private(SafeSslHandle ssl); @@ -182,6 +191,12 @@ internal static unsafe ReadOnlySpan SslGetAlpnSelected(SafeSslHandle ssl) [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionSetHostname")] internal static partial int SessionSetHostname(IntPtr session, IntPtr name); + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionGetData")] + internal static partial IntPtr SslSessionGetData(IntPtr session); + + [LibraryImport(Libraries.CryptoNative, EntryPoint = "CryptoNative_SslSessionSetData")] + internal static partial void SslSessionSetData(IntPtr session, IntPtr val); + internal static class Capabilities { // needs separate type (separate static cctor) to be sure OpenSSL is initialized. diff --git a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs index 5a2daad3d2019..6fda4835d4ae2 100644 --- a/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/CertificateValidationPal.Unix.cs @@ -95,9 +95,43 @@ internal static SslPolicyErrors VerifyCertificateProperties( return result; } - // This is only called when we selected local client certificate. - // Currently this is only when OpenSSL needs it because peer asked. - internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _1, SafeDeleteContext? _2) => true; + internal static bool IsLocalCertificateUsed(SafeFreeCredentials? _1, SafeDeleteContext? ctx) + { + if (ctx is not SafeSslHandle ssl) + { + return false; + } + + if (!Interop.Ssl.SslSessionReused(ssl)) + { + // Fresh session, we set the certificate on the SSL object only + // if the peer explicitly requested it. + return Interop.Ssl.SslGetCertificate(ssl) != IntPtr.Zero; + } + + // resumed session, we keep the information about cert being used in the SSL_SESSION + // object's ex_data + bool addref = false; + try + { + // make sure the ssl is not freed while we accessing its SSL_SESSION + // this makes sure the `session` pointer is valid during this call + // despite not being a SafeHandle. + ssl.DangerousAddRef(ref addref); + + // the information about certificate usage is stored in the session ex data + IntPtr session = Interop.Ssl.SslGetSession(ssl); + Debug.Assert(session != IntPtr.Zero); + return Interop.Ssl.SslSessionGetData(session) != IntPtr.Zero; + } + finally + { + if (addref) + { + ssl.DangerousRelease(); + } + } + } // // Used only by client SSL code, never returns null. diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs index 494250e92e3b6..453454cfe60d6 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs @@ -13,6 +13,7 @@ namespace System.Net.Security.Tests { using Configuration = System.Net.Test.Common.Configuration; + [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.Linux)] public class SslStreamTlsResumeTests { private static FieldInfo connectionInfo = typeof(SslStream).GetField( @@ -29,7 +30,6 @@ private bool CheckResumeFlag(SslStream ssl) [ConditionalTheory] [InlineData(true)] [InlineData(false)] - [PlatformSpecific(TestPlatforms.Windows | TestPlatforms.Linux)] public async Task SslStream_ClientDisableTlsResume_Succeeds(bool testClient) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs index 7a858a644bab3..e8ddb07f82df7 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamMutualAuthenticationTest.cs @@ -266,7 +266,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -320,7 +320,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } @@ -357,7 +357,7 @@ public async Task SslStream_ResumedSessionsCallbackMaybeSet_IsMutuallyAuthentica if (expectMutualAuthentication) { - clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; + clientOptions.LocalCertificateSelectionCallback = (s, t, l, r, a) => _clientCertificate; } else { @@ -378,7 +378,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } else { - Assert.Null(server.RemoteCertificate); + Assert.Null(server.RemoteCertificate); } }; } diff --git a/src/native/libs/System.Security.Cryptography.Native/entrypoints.c b/src/native/libs/System.Security.Cryptography.Native/entrypoints.c index cb4b621e27fa3..87d78e2136ff5 100644 --- a/src/native/libs/System.Security.Cryptography.Native/entrypoints.c +++ b/src/native/libs/System.Security.Cryptography.Native/entrypoints.c @@ -340,14 +340,18 @@ static const Entry s_cryptoNative[] = DllImportEntry(CryptoNative_SslGetFinished) DllImportEntry(CryptoNative_SslGetPeerCertChain) DllImportEntry(CryptoNative_SslGetPeerCertificate) + DllImportEntry(CryptoNative_SslGetCertificate) DllImportEntry(CryptoNative_SslGetPeerFinished) DllImportEntry(CryptoNative_SslGetServerName) + DllImportEntry(CryptoNative_SslGetSession) DllImportEntry(CryptoNative_SslGetVersion) DllImportEntry(CryptoNative_SslRead) DllImportEntry(CryptoNative_SslSessionFree) DllImportEntry(CryptoNative_SslSessionGetHostname) DllImportEntry(CryptoNative_SslSessionSetHostname) DllImportEntry(CryptoNative_SslSessionReused) + DllImportEntry(CryptoNative_SslSessionGetData) + DllImportEntry(CryptoNative_SslSessionSetData) DllImportEntry(CryptoNative_SslSetAcceptState) DllImportEntry(CryptoNative_SslSetAlpnProtos) DllImportEntry(CryptoNative_SslSetBio) diff --git a/src/native/libs/System.Security.Cryptography.Native/openssl.c b/src/native/libs/System.Security.Cryptography.Native/openssl.c index 50a3292d05418..3b54097d240b0 100644 --- a/src/native/libs/System.Security.Cryptography.Native/openssl.c +++ b/src/native/libs/System.Security.Cryptography.Native/openssl.c @@ -1196,7 +1196,7 @@ int64_t CryptoNative_OpenSslVersionNumber(void) return (int64_t)OpenSSL_version_num(); } -static void ExDataFree( +static void ExDataFreeOcspResponse( void* parent, void* ptr, CRYPTO_EX_DATA* ad, @@ -1221,7 +1221,7 @@ static void ExDataFree( // In the OpenSSL 1.0.2 headers, the `from` argument is not const (became const in 1.1.0) // In the OpenSSL 3 headers, `from_d` changed from (void*) to (void**). -static int ExDataDup( +static int ExDataDupOcspResponse( CRYPTO_EX_DATA* to, #if OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_1_1_0_RTM const CRYPTO_EX_DATA* from, @@ -1260,6 +1260,53 @@ static int ExDataDup( return 1; } +static void ExDataFreeNoOp( + void* parent, + void* ptr, + CRYPTO_EX_DATA* ad, + int idx, + long argl, + void* argp) +{ + (void)parent; + (void)ptr; + (void)ad; + (void)idx; + (void)argl; + (void)argp; + + // do nothing. +} + + +static int ExDataDupNoOp( + CRYPTO_EX_DATA* to, +#if OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_1_1_0_RTM + const CRYPTO_EX_DATA* from, +#else + CRYPTO_EX_DATA* from, +#endif +#if OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_3_0_RTM + void** from_d, +#else + void* from_d, +#endif + int idx, + long argl, + void* argp) +{ + (void)to; + (void)from; + (void)from_d; + (void)idx; + (void)argl; + (void)argp; + + // do nothing, this should lead to copy of the pointer being stored in the + // destination, we treat the ptr as an opaque blob. + return 1; +} + void CryptoNative_RegisterLegacyAlgorithms(void) { #ifdef NEED_OPENSSL_3_0 @@ -1393,7 +1440,9 @@ static int32_t EnsureOpenSsl10Initialized(void) ERR_load_crypto_strings(); // In OpenSSL 1.0.2-, CRYPTO_EX_INDEX_X509 is 10. - g_x509_ocsp_index = CRYPTO_get_ex_new_index(10, 0, NULL, NULL, ExDataDup, ExDataFree); + g_x509_ocsp_index = CRYPTO_get_ex_new_index(10, 0, NULL, NULL, ExDataDupOcspResponse, ExDataFreeOcspResponse); + // In OpenSSL 1.0.2-, CRYPTO_EX_INDEX_SSL_SESSION is 3. + g_ssl_sess_cert_index = CRYPTO_get_ex_new_index(3, 0, NULL, NULL, ExDataDupNoOp, ExDataFreeNoOp); done: if (ret != 0) @@ -1461,7 +1510,9 @@ static int32_t EnsureOpenSsl11Initialized(void) atexit(HandleShutdown); // In OpenSSL 1.1.0+, CRYPTO_EX_INDEX_X509 is 3. - g_x509_ocsp_index = CRYPTO_get_ex_new_index(3, 0, NULL, NULL, ExDataDup, ExDataFree); + g_x509_ocsp_index = CRYPTO_get_ex_new_index(3, 0, NULL, NULL, ExDataDupOcspResponse, ExDataFreeOcspResponse); + // In OpenSSL 1.1.0+, CRYPTO_EX_INDEX_SSL_SESSION is 2. + g_ssl_sess_cert_index = CRYPTO_get_ex_new_index(2, 0, NULL, NULL, ExDataDupNoOp, ExDataFreeNoOp); return 0; } @@ -1480,6 +1531,7 @@ int32_t CryptoNative_OpenSslAvailable(void) static int32_t g_initStatus = 1; int g_x509_ocsp_index = -1; +int g_ssl_sess_cert_index = -1; static int32_t EnsureOpenSslInitializedCore(void) { @@ -1510,6 +1562,7 @@ static int32_t EnsureOpenSslInitializedCore(void) // On OpenSSL 1.0.2 our expected index is 0. // On OpenSSL 1.1.0+ 0 is a reserved value and we expect 1. assert(g_x509_ocsp_index != -1); + assert(g_ssl_sess_cert_index != -1); } return ret; diff --git a/src/native/libs/System.Security.Cryptography.Native/openssl.h b/src/native/libs/System.Security.Cryptography.Native/openssl.h index 10ed8480e4776..e031df1e81a12 100644 --- a/src/native/libs/System.Security.Cryptography.Native/openssl.h +++ b/src/native/libs/System.Security.Cryptography.Native/openssl.h @@ -7,6 +7,10 @@ #include "pal_compiler.h" #include "opensslshim.h" +// index for storing an opaque pointer of used (client) certificate in SSL_SESSION. +// we need dedicated index in order to tell OpenSSL how to copy the pointer during SSL_SESSION_dup. +extern int g_ssl_sess_cert_index; + PALEXPORT int32_t CryptoNative_GetX509Thumbprint(X509* x509, uint8_t* pBuf, int32_t cBuf); PALEXPORT const ASN1_TIME* CryptoNative_GetX509NotBefore(X509* x509); diff --git a/src/native/libs/System.Security.Cryptography.Native/opensslshim.h b/src/native/libs/System.Security.Cryptography.Native/opensslshim.h index c4aa47d18cfae..cf24f810bb6e8 100644 --- a/src/native/libs/System.Security.Cryptography.Native/opensslshim.h +++ b/src/native/libs/System.Security.Cryptography.Native/opensslshim.h @@ -589,6 +589,7 @@ extern bool g_libSslUses32BitTime; REQUIRED_FUNCTION(SSL_get_version) \ LIGHTUP_FUNCTION(SSL_get0_alpn_selected) \ RENAMED_FUNCTION(SSL_get1_peer_certificate, SSL_get_peer_certificate) \ + REQUIRED_FUNCTION(SSL_get_certificate) \ LEGACY_FUNCTION(SSL_library_init) \ LEGACY_FUNCTION(SSL_load_error_strings) \ REQUIRED_FUNCTION(SSL_new) \ @@ -597,6 +598,8 @@ extern bool g_libSslUses32BitTime; REQUIRED_FUNCTION(SSL_renegotiate) \ REQUIRED_FUNCTION(SSL_renegotiate_pending) \ REQUIRED_FUNCTION(SSL_SESSION_free) \ + REQUIRED_FUNCTION(SSL_SESSION_get_ex_data) \ + REQUIRED_FUNCTION(SSL_SESSION_set_ex_data) \ LIGHTUP_FUNCTION(SSL_SESSION_get0_hostname) \ LIGHTUP_FUNCTION(SSL_SESSION_set1_hostname) \ FALLBACK_FUNCTION(SSL_session_reused) \ @@ -609,6 +612,7 @@ extern bool g_libSslUses32BitTime; REQUIRED_FUNCTION(SSL_set_ex_data) \ FALLBACK_FUNCTION(SSL_set_options) \ REQUIRED_FUNCTION(SSL_set_session) \ + REQUIRED_FUNCTION(SSL_get_session) \ REQUIRED_FUNCTION(SSL_set_verify) \ REQUIRED_FUNCTION(SSL_shutdown) \ LEGACY_FUNCTION(SSL_state) \ @@ -1109,6 +1113,7 @@ extern TYPEOF(OPENSSL_gmtime)* OPENSSL_gmtime_ptr; #define SSL_free SSL_free_ptr #define SSL_get_ciphers SSL_get_ciphers_ptr #define SSL_get_client_CA_list SSL_get_client_CA_list_ptr +#define SSL_get_certificate SSL_get_certificate_ptr #define SSL_get_current_cipher SSL_get_current_cipher_ptr #define SSL_get_error SSL_get_error_ptr #define SSL_get_ex_data SSL_get_ex_data_ptr @@ -1133,6 +1138,8 @@ extern TYPEOF(OPENSSL_gmtime)* OPENSSL_gmtime_ptr; #define SSL_SESSION_get0_hostname SSL_SESSION_get0_hostname_ptr #define SSL_SESSION_set1_hostname SSL_SESSION_set1_hostname_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 #define SSL_set_accept_state SSL_set_accept_state_ptr #define SSL_set_bio SSL_set_bio_ptr #define SSL_set_cert_cb SSL_set_cert_cb_ptr @@ -1142,6 +1149,7 @@ extern TYPEOF(OPENSSL_gmtime)* OPENSSL_gmtime_ptr; #define SSL_set_ex_data SSL_set_ex_data_ptr #define SSL_set_options SSL_set_options_ptr #define SSL_set_session SSL_set_session_ptr +#define SSL_get_session SSL_get_session_ptr #define SSL_set_verify SSL_set_verify_ptr #define SSL_shutdown SSL_shutdown_ptr #define SSL_state SSL_state_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 e320d1c73d776..dc0478ef2a230 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c @@ -595,6 +595,11 @@ X509* CryptoNative_SslGetPeerCertificate(SSL* ssl) return cert; } +X509* CryptoNative_SslGetCertificate(SSL* ssl) +{ + return SSL_get_certificate(ssl); +} + X509Stack* CryptoNative_SslGetPeerCertChain(SSL* ssl) { // No error queue impact. @@ -711,6 +716,11 @@ const char* CryptoNative_SslGetServerName(SSL* ssl) return SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name); } +SSL_SESSION* CryptoNative_SslGetSession(SSL* ssl) +{ + return SSL_get_session(ssl); +} + int32_t CryptoNative_SslSetSession(SSL* ssl, SSL_SESSION* session) { return SSL_set_session(ssl, session); @@ -748,6 +758,16 @@ int CryptoNative_SslSessionSetHostname(SSL_SESSION* session, const char* hostnam return 0; } +void CryptoNative_SslSessionSetData(SSL_SESSION* session, void* val) +{ + SSL_SESSION_set_ex_data(session, g_ssl_sess_cert_index, val); +} + +void* CryptoNative_SslSessionGetData(SSL_SESSION* session) +{ + return SSL_SESSION_get_ex_data(session, g_ssl_sess_cert_index); +} + int32_t CryptoNative_SslCtxSetEncryptionPolicy(SSL_CTX* ctx, EncryptionPolicy policy) { // No error queue impact. 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 9c9d7026119c5..28d4adcf02f5a 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h @@ -183,6 +183,11 @@ OpenSSL holds reference to it and it must not be freed. */ PALEXPORT const char* CryptoNative_SslGetServerName(SSL* ssl); +/* +Returns session associated with given ssl. +*/ +PALEXPORT SSL_SESSION* CryptoNative_SslGetSession(SSL* ssl); + /* This function will attach existing ssl session for possible TLS resume. */ @@ -326,6 +331,13 @@ Returns the certificate presented by the peer. */ PALEXPORT X509* CryptoNative_SslGetPeerCertificate(SSL* ssl); +/* +Shims the SSL_get_certificate method. + +Returns the certificate representing local peer. +*/ +PALEXPORT X509* CryptoNative_SslGetCertificate(SSL* ssl); + /* Shims the SSL_get_peer_cert_chain method. @@ -450,6 +462,16 @@ Shims the SSL_session_reused macro. */ PALEXPORT int32_t CryptoNative_SslSessionReused(SSL* ssl); +/* +Sets app data pointer to given session instance. +*/ +PALEXPORT void CryptoNative_SslSessionSetData(SSL_SESSION* session, void* val); + +/* +Returns the stored application data pointer. +*/ +PALEXPORT void* CryptoNative_SslSessionGetData(SSL_SESSION* session); + /* Adds the given certificate to the extra chain certificates associated with ctx. From a495093f5c92d5be3466c6ec64232f427e1e6891 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Fri, 24 May 2024 14:16:42 +0200 Subject: [PATCH 05/16] Update src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs --- .../src/System/Net/Security/SslStreamPal.Unix.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs index c5bc8c2b846e0..0aa6238c84751 100644 --- a/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs +++ b/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamPal.Unix.cs @@ -187,7 +187,7 @@ private static ProtocolToken HandshakeInternal(ref SafeDeleteSslContext? context // this should happen only for clients Debug.Assert(sslAuthenticationOptions.IsClient); - // if we don't have a clien certificate ready, bubble up so + // if we don't have a client certificate ready, bubble up so // that the certificate selection routine runs again. This // happens if the first call to LocalCertificateSelectionCallback // returns null. From ca364a7ee603fed4d49837a70e1a0d3e7cf5b6fb Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 27 May 2024 11:51:25 +0200 Subject: [PATCH 06/16] Apply suggestions from code review --- .../Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs | 2 +- src/native/libs/System.Security.Cryptography.Native/openssl.c | 1 - src/native/libs/System.Security.Cryptography.Native/pal_ssl.h | 2 +- 3 files changed, 2 insertions(+), 3 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 6093e684d65fe..fc76786d20085 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 @@ -331,7 +331,7 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth } else { - var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash()); + var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash(HashAlgorithmName.SHA256)); s_clientSslContexts.TryGetValue(key, out sslCtxHandle); } } diff --git a/src/native/libs/System.Security.Cryptography.Native/openssl.c b/src/native/libs/System.Security.Cryptography.Native/openssl.c index 3b54097d240b0..14026c6cc7785 100644 --- a/src/native/libs/System.Security.Cryptography.Native/openssl.c +++ b/src/native/libs/System.Security.Cryptography.Native/openssl.c @@ -1278,7 +1278,6 @@ static void ExDataFreeNoOp( // do nothing. } - static int ExDataDupNoOp( CRYPTO_EX_DATA* to, #if OPENSSL_VERSION_NUMBER >= OPENSSL_VERSION_1_1_0_RTM 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 28d4adcf02f5a..d1ebc0327532f 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h @@ -463,7 +463,7 @@ Shims the SSL_session_reused macro. PALEXPORT int32_t CryptoNative_SslSessionReused(SSL* ssl); /* -Sets app data pointer to given session instance. +Sets the app data pointer for the given session instance. */ PALEXPORT void CryptoNative_SslSessionSetData(SSL_SESSION* session, void* val); From ea1e9febc561a84b5abac29c474cbd78e56118fc Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 27 May 2024 11:59:57 +0200 Subject: [PATCH 07/16] Move definition --- src/native/libs/System.Security.Cryptography.Native/openssl.c | 1 + src/native/libs/System.Security.Cryptography.Native/openssl.h | 4 ---- src/native/libs/System.Security.Cryptography.Native/pal_ssl.h | 4 ++++ 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/native/libs/System.Security.Cryptography.Native/openssl.c b/src/native/libs/System.Security.Cryptography.Native/openssl.c index 14026c6cc7785..32ad26dd8bbe7 100644 --- a/src/native/libs/System.Security.Cryptography.Native/openssl.c +++ b/src/native/libs/System.Security.Cryptography.Native/openssl.c @@ -6,6 +6,7 @@ #include "pal_utilities.h" #include "pal_safecrt.h" #include "pal_x509.h" +#include "pal_ssl.h" #include "openssl.h" #ifdef FEATURE_DISTRO_AGNOSTIC_SSL diff --git a/src/native/libs/System.Security.Cryptography.Native/openssl.h b/src/native/libs/System.Security.Cryptography.Native/openssl.h index e031df1e81a12..10ed8480e4776 100644 --- a/src/native/libs/System.Security.Cryptography.Native/openssl.h +++ b/src/native/libs/System.Security.Cryptography.Native/openssl.h @@ -7,10 +7,6 @@ #include "pal_compiler.h" #include "opensslshim.h" -// index for storing an opaque pointer of used (client) certificate in SSL_SESSION. -// we need dedicated index in order to tell OpenSSL how to copy the pointer during SSL_SESSION_dup. -extern int g_ssl_sess_cert_index; - PALEXPORT int32_t CryptoNative_GetX509Thumbprint(X509* x509, uint8_t* pBuf, int32_t cBuf); PALEXPORT const ASN1_TIME* CryptoNative_GetX509NotBefore(X509* x509); 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 d1ebc0327532f..8566c7b8ff9b3 100644 --- a/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h +++ b/src/native/libs/System.Security.Cryptography.Native/pal_ssl.h @@ -5,6 +5,10 @@ #include "pal_compiler.h" #include "opensslshim.h" +// index for storing an opaque pointer of used (client) certificate in SSL_SESSION. +// we need dedicated index in order to tell OpenSSL how to copy the pointer during SSL_SESSION_dup. +extern int g_ssl_sess_cert_index; + /* These values should be kept in sync with System.Security.Authentication.SslProtocols. */ From 13e6f9265d95e8a2df1db19972125ea5d740a1d2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 27 May 2024 13:35:23 +0200 Subject: [PATCH 08/16] Minor improvements --- .../Interop.OpenSsl.cs | 24 +++++++++++++------ 1 file changed, 17 insertions(+), 7 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 fc76786d20085..6485d78e854ec 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 @@ -290,7 +290,7 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth { SafeSslHandle? sslHandle = null; SafeSslContextHandle? sslCtxHandle = null; - SafeSslContextHandle? newCtxHandle = null; + SafeSslContextHandle? toDisposeHandle = null; SslProtocols protocols = CalculateEffectiveProtocols(sslAuthenticationOptions); bool hasAlpn = sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0; bool cacheSslContext = sslAuthenticationOptions.AllowTlsResume && !SslStream.DisableTlsResume && sslAuthenticationOptions.EncryptionPolicy == EncryptionPolicy.RequireEncryption && sslAuthenticationOptions.CipherSuitesPolicy == null; @@ -338,8 +338,13 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth if (sslCtxHandle == null) { - // We did not get SslContext from cache - sslCtxHandle = newCtxHandle = AllocateSslContext(sslAuthenticationOptions, protocols, cacheSslContext); + // We did not get SslContext from cache, allocate a one. If we + // won't end up caching the context, we will dispose the new + // SafeSslContextHandle at end of this method since we won't + // reuse it. SSL object created later keeps a reference to it + // and will free it when we close the SafeSslHandle. + + sslCtxHandle = toDisposeHandle = AllocateSslContext(sslAuthenticationOptions, protocols, cacheSslContext); if (cacheSslContext) { @@ -347,17 +352,22 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth if (sslAuthenticationOptions.IsServer) { - added = sslAuthenticationOptions.CertificateContext!.SslContexts!.TryAdd(protocols | (SslProtocols)(hasAlpn ? 1 : 0), newCtxHandle); + added = sslAuthenticationOptions.CertificateContext!.SslContexts!.TryAdd(protocols | (SslProtocols)(hasAlpn ? 1 : 0), sslCtxHandle); } else { var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash()); - added = s_clientSslContexts.TryAdd(key, newCtxHandle); + + // Check for concurrent inserts, if not added, use the existing one, + // the new one will be disposed. + sslCtxHandle = s_clientSslContexts.GetOrAdd(key, sslCtxHandle); + added = sslCtxHandle == toDisposeHandle; } if (added) { - newCtxHandle = null; + // handle will be cached, don't dispose it + toDisposeHandle = null; } } } @@ -477,7 +487,7 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth } finally { - newCtxHandle?.Dispose(); + toDisposeHandle?.Dispose(); } return sslHandle; From 75ee95f28c976f2f628a709e5301669612085322 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 30 May 2024 16:50:40 +0200 Subject: [PATCH 09/16] Add more tests --- .../SslStreamAllowTlsResumeTests.cs | 150 ++++++++++++++++++ 1 file changed, 150 insertions(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs index 453454cfe60d6..ff23835bc24fc 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs @@ -128,6 +128,156 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( client.Dispose(); server.Dispose(); } + + [ConditionalTheory] + [InlineData(SslProtocols.Tls12)] + [InlineData(SslProtocols.Tls13)] + public Task SslStream_NoClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol) + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + EnabledSslProtocols = sslProtocol, + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false) + }; + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + EnabledSslProtocols = sslProtocol, + CertificateRevocationCheckMode = X509RevocationMode.NoCheck, + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + }; + + return ResumeSucceedsInternal(serverOptions, clientOptions); + } + + public enum ClientCertSource + { + ClientCertificate, + SelectionCallback, + CertificateContext + } + + public static TheoryData ClientCertTestData() + { + var data = new TheoryData(); + + foreach (SslProtocols protocol in new [] { SslProtocols.Tls12, SslProtocols.Tls13 }) + foreach (bool certRequired in new[] { true, false }) + foreach (ClientCertSource source in Enum.GetValues(typeof(ClientCertSource))) + { + data.Add(protocol, certRequired, source); + } + + return data; + } + + [ConditionalTheory] + [MemberData(nameof(ClientCertTestData))] + public Task SslStream_ClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol, bool certificateRequired, ClientCertSource certSource) + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + EnabledSslProtocols = sslProtocol, + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false), + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + ClientCertificateRequired = certificateRequired, + }; + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + EnabledSslProtocols = sslProtocol, + CertificateRevocationCheckMode = X509RevocationMode.NoCheck, + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + }; + + X509Certificate2 clientCertificate = Configuration.Certificates.GetClientCertificate(); + + switch (certSource) + { + case ClientCertSource.ClientCertificate: + clientOptions.ClientCertificates = new X509CertificateCollection() { clientCertificate }; + break; + case ClientCertSource.SelectionCallback: + clientOptions.LocalCertificateSelectionCallback = delegate { return clientCertificate; }; + break; + case ClientCertSource.CertificateContext: + clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(clientCertificate, new()); + break; + } + + return ResumeSucceedsInternal(serverOptions, clientOptions); + } + + private async Task ResumeSucceedsInternal(SslServerAuthenticationOptions serverOptions, SslClientAuthenticationOptions clientOptions) + { + // no resume on the first run + await RunConnectionAsync(serverOptions, clientOptions, false); + + for (int i = 0; i < 3; i++) + { + // create new TLS to the same server. This should resume TLS. + await RunConnectionAsync(serverOptions, clientOptions, true); + } + } + + private async Task RunConnectionAsync(SslServerAuthenticationOptions serverOptions, SslClientAuthenticationOptions clientOptions, bool? expectResume) + { + (SslStream client, SslStream server) = TestHelper.GetConnectedSslStreams(); + using (client) + using (server) + { + await TestConfiguration.WhenAllOrAnyFailedWithTimeout( + client.AuthenticateAsClientAsync(clientOptions), + server.AuthenticateAsServerAsync(serverOptions)); + + if (expectResume.HasValue) + { + Assert.Equal(expectResume.Value, CheckResumeFlag(client)); + Assert.Equal(expectResume.Value, CheckResumeFlag(server)); + } + + await TestHelper.PingPong(client, server); + + await client.ShutdownAsync(); + await server.ShutdownAsync(); + } + } + + [ConditionalTheory] + [InlineData(SslProtocols.Tls12)] + [InlineData(SslProtocols.Tls13)] + public async Task SslStream_ClientChangeCert_NoResume(SslProtocols sslProtocol) + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + EnabledSslProtocols = sslProtocol, + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false) + }; + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + EnabledSslProtocols = sslProtocol, + CertificateRevocationCheckMode = X509RevocationMode.NoCheck, + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + ClientCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetClientCertificate(), null, false) + }; + + await RunConnectionAsync(serverOptions, clientOptions, false); + await RunConnectionAsync(serverOptions, clientOptions, true); + + // change to a different client cert, it should not resume + clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetSelfSignedClientCertificate(), null, false); + await RunConnectionAsync(serverOptions, clientOptions, false); + await RunConnectionAsync(serverOptions, clientOptions, true); + + // remove the client cert and try to resume + clientOptions.ClientCertificateContext = null; + await RunConnectionAsync(serverOptions, clientOptions, false); + await RunConnectionAsync(serverOptions, clientOptions, true); + } } } #endif From 305ad23850e68436189027f5e123c43317b6d7bf Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Fri, 31 May 2024 13:58:13 +0200 Subject: [PATCH 10/16] Add more no-resume tests --- .../SslStreamAllowTlsResumeTests.cs | 142 ++++++++++++++++-- 1 file changed, 127 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs index ff23835bc24fc..a31f63435913c 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs @@ -30,7 +30,7 @@ private bool CheckResumeFlag(SslStream ssl) [ConditionalTheory] [InlineData(true)] [InlineData(false)] - public async Task SslStream_ClientDisableTlsResume_Succeeds(bool testClient) + public async Task ClientDisableTlsResume_Succeeds(bool testClient) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions { @@ -129,10 +129,10 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( server.Dispose(); } - [ConditionalTheory] + [Theory] [InlineData(SslProtocols.Tls12)] [InlineData(SslProtocols.Tls13)] - public Task SslStream_NoClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol) + public Task NoClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions { @@ -172,9 +172,9 @@ public static TheoryData ClientCertTestDat return data; } - [ConditionalTheory] + [Theory] [MemberData(nameof(ClientCertTestData))] - public Task SslStream_ClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol, bool certificateRequired, ClientCertSource certSource) + public Task ClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol, bool certificateRequired, ClientCertSource certSource) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions { @@ -245,10 +245,37 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } } - [ConditionalTheory] + [Theory] [InlineData(SslProtocols.Tls12)] [InlineData(SslProtocols.Tls13)] - public async Task SslStream_ClientChangeCert_NoResume(SslProtocols sslProtocol) + public Task ClientChangeCert_NoResume(SslProtocols sslProtocol) + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + EnabledSslProtocols = sslProtocol, + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false), + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + ClientCertificateRequired = true, + }; + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + EnabledSslProtocols = sslProtocol, + CertificateRevocationCheckMode = X509RevocationMode.NoCheck, + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + ClientCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetClientCertificate(), null, false) + }; + + return TestNoResumeAfterChange(serverOptions, clientOptions, + (clientOps, _) => clientOps.ClientCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetSelfSignedClientCertificate(), null, false), + (clientOps, _) => clientOps.ClientCertificateContext = null); + } + + [Theory] + [InlineData(SslProtocols.Tls12)] + [InlineData(SslProtocols.Tls13)] + public Task DifferentHost_NoResume(SslProtocols sslProtocol) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions { @@ -264,19 +291,104 @@ public async Task SslStream_ClientChangeCert_NoResume(SslProtocols sslProtocol) RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, ClientCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetClientCertificate(), null, false) }; + + return TestNoResumeAfterChange(serverOptions, clientOptions, + (clientOps, _) => clientOps.TargetHost = Guid.NewGuid().ToString("N")); + } - await RunConnectionAsync(serverOptions, clientOptions, false); - await RunConnectionAsync(serverOptions, clientOptions, true); + [Fact] + public Task DifferentProtocol_NoResume() + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false) + }; - // change to a different client cert, it should not resume - clientOptions.ClientCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetSelfSignedClientCertificate(), null, false); - await RunConnectionAsync(serverOptions, clientOptions, false); - await RunConnectionAsync(serverOptions, clientOptions, true); + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + EnabledSslProtocols = SslProtocols.Tls12, + CertificateRevocationCheckMode = X509RevocationMode.NoCheck, + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + }; + + return TestNoResumeAfterChange(serverOptions, clientOptions, + (clientOps, _) => clientOps.EnabledSslProtocols = SslProtocols.None); + } - // remove the client cert and try to resume - clientOptions.ClientCertificateContext = null; + [Fact] + [PlatformSpecific(TestPlatforms.Windows)] + public Task DifferentRevocationCheckMode_NoResume() + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false) + }; + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + }; + + return TestNoResumeAfterChange(serverOptions, clientOptions, + (clientOps, _) => clientOps.CertificateRevocationCheckMode = X509RevocationMode.NoCheck); + } + + [Fact] + public Task DifferentEncryptionPolicy_NoResume() + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + EnabledSslProtocols = SslProtocols.Tls12, + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false) + }; + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + CertificateRevocationCheckMode = X509RevocationMode.NoCheck, + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + }; +#pragma warning disable SYSLIB0040 // 'AllowNoEncryption' is obsolete + return TestNoResumeAfterChange(serverOptions, clientOptions, + (clientOps, _) => clientOps.EncryptionPolicy = EncryptionPolicy.AllowNoEncryption); +#pragma warning restore SYSLIB0040 // 'AllowNoEncryption' is obsolete + } + + [Fact] + [PlatformSpecific(TestPlatforms.Linux)] // CipherSuitesPolicy is suppoted only on Linux + public Task DifferentCipherSuitesPolicy_NoResume() + { + SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions + { + ServerCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetServerCertificate(), null, false) + }; + + SslClientAuthenticationOptions clientOptions = new SslClientAuthenticationOptions + { + TargetHost = Guid.NewGuid().ToString("N"), + CertificateRevocationCheckMode = X509RevocationMode.NoCheck, + RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + }; + + return TestNoResumeAfterChange(serverOptions, clientOptions, + (clientOps, _) => clientOps.CipherSuitesPolicy = new CipherSuitesPolicy(new[] { TlsCipherSuite.TLS_AES_128_GCM_SHA256 })); + } + + private async Task TestNoResumeAfterChange(SslServerAuthenticationOptions serverOptions, SslClientAuthenticationOptions clientOptions, params Action[] updateOptions) + { + // confirm sessions are resumable and prime for resumption await RunConnectionAsync(serverOptions, clientOptions, false); await RunConnectionAsync(serverOptions, clientOptions, true); + + foreach (Action update in updateOptions) + { + update(clientOptions, serverOptions); + + // after changing options, the session should not be resumed + await RunConnectionAsync(serverOptions, clientOptions, false); + } } } } From 70be46e54e1b01cac40f80277ffaaaefdb38ae9e Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 3 Jun 2024 16:02:19 +0200 Subject: [PATCH 11/16] Move MsQuicConfiguration cache logic to common code --- .../Common/src/System/SafeHandleCache.cs | 149 ++++++++++++++++++ .../src/System.Net.Quic.csproj | 1 + .../Internal/MsQuicConfiguration.Cache.cs | 114 ++------------ .../Net/Quic/Internal/MsQuicSafeHandle.cs | 2 +- 4 files changed, 161 insertions(+), 105 deletions(-) create mode 100644 src/libraries/Common/src/System/SafeHandleCache.cs diff --git a/src/libraries/Common/src/System/SafeHandleCache.cs b/src/libraries/Common/src/System/SafeHandleCache.cs new file mode 100644 index 0000000000000..2691f8d55afb8 --- /dev/null +++ b/src/libraries/Common/src/System/SafeHandleCache.cs @@ -0,0 +1,149 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Diagnostics; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Globalization; +using System.Runtime.InteropServices; + +namespace System.Net +{ + internal interface ISafeHandleCachable + { + bool TryAddRentCount(); + bool TryMarkForDispose(); + } + + /// + /// Helper class for implementing a cache for types deriving from . The purpose of the cache is to allow reuse of + /// resources which may enable additional features (such as TLS resumption). + /// The cache handles insertion and eviction in a thread-safe manner and + /// implements simple mechanism for preventing unbounded growth and memory + /// leaks. + /// + internal class SafeHandleCache where TKey : IEquatable where THandle : SafeHandle, ISafeHandleCachable + { + private const int CheckExpiredModulo = 32; + + private readonly ConcurrentDictionary _cache = new(); + + /// + /// Gets the handle from the cache if it exists, otherwise creates a new one using the + /// provided factory function and context. + /// + /// In case of two racing inserts with the same key, the handle returned by the factory may + /// end up being discarded in favor of the one that was inserted first. In such case, the + /// factory handle is disposed and the cached handle is returned. + /// + /// The handle returned from this function should be disposed exactly once when it is no + /// longer needed. + /// + internal THandle GetOrCreate(TKey key, Func factory, TContext factoryContext) + { + if (_cache.TryGetValue(key, out THandle? handle) && handle.TryAddRentCount()) + { + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Found cached {handle}."); + } + return handle; + } + + // if we get here, the handle is either not in the cache, or we lost + // the race between TryAddRentCount on this thread and + // MarkForDispose on another thread doing cache cleanup. In either + // case, we need to create a new handle. + + handle = factory(factoryContext); + handle.TryAddRentCount(); // The caler is the first renter + + THandle cached; + do + { + cached = _cache.GetOrAdd(key, handle); + } + // If we get the same handle back, we successfully added it to the cache and we are done. + // If we get a different handle back, we need to increase the rent count. + // If we fail to add the rent count, then the existing/cached handle is in process of + // being removed from the cache and we can try again, eventually either succeeding to + // add our new handle or getting a fresh handle inserted by another thread meanwhile. + while (cached != handle && !cached.TryAddRentCount()); + + if (cached != handle) + { + // we lost a race with another thread to insert new handle into the cache + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Discarding {handle} (preferring cached {cached})."); + } + + // First dispose decrements the rent count we added before attempting the cache insertion + // and second closes the handle + handle.Dispose(); + handle.Dispose(); + Debug.Assert(handle.IsClosed); + + return cached; + } + + CheckForCleanup(); + + return handle; + } + + private void CheckForCleanup() + { + // We check the cache size after every couple of insertions, and + // discard all handles which are not being actively rented. This + // should still be flexible enough to allow "stable set" of + // arbitrary size, while still preventing unbounded growth. + + var count = _cache.Count; + if (count % CheckExpiredModulo == 0) + { + // let only one thread perform cleanup at a time + lock (_cache) + { + // check again, if another thread just cleaned up (and cached count went down) we are unlikely + // to clean anything + if (_cache.Count >= count) + { + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Current size: {_cache.Count}."); + } + + foreach ((TKey key, THandle handle) in _cache) + { + if (!handle.TryMarkForDispose()) + { + // handle in use + continue; + } + + // the handle is not in use and has been marked such that no new rents can be added. + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"Evicting cached {handle}."); + } + + bool removed = _cache.TryRemove(key, out _); + Debug.Assert(removed); + handle.Dispose(); + + // Since the handle is not used anywhere, this should close the handle + Debug.Assert(handle.IsClosed); + } + + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Info(this, $"New size: {_cache.Count}."); + } + } + } + } + } + } +} diff --git a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj index 56f1d2837ac67..4f77f90e7cfa6 100644 --- a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj +++ b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj @@ -29,6 +29,7 @@ + diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 38a02cad2328b..f7d2e751611d1 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -15,8 +15,6 @@ namespace System.Net.Quic; internal static partial class MsQuicConfiguration { - private const int CheckExpiredModulo = 32; - private const string DisableCacheEnvironmentVariable = "DOTNET_SYSTEM_NET_QUIC_DISABLE_CONFIGURATION_CACHE"; private const string DisableCacheCtxSwitch = "System.Net.Quic.DisableConfigurationCache"; @@ -38,7 +36,12 @@ private static bool GetConfigurationCacheEnabled() // enabled by default return true; } - private static readonly ConcurrentDictionary s_configurationCache = new(); + + private static readonly MsQuicConfigurationCache s_configurationCache = new MsQuicConfigurationCache(); + + private sealed class MsQuicConfigurationCache : SafeHandleCache + { + } private readonly struct CacheKey : IEquatable { @@ -130,107 +133,10 @@ private static MsQuicConfigurationSafeHandle GetCachedCredentialOrCreate(QUIC_SE { CacheKey key = new CacheKey(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); - MsQuicConfigurationSafeHandle? handle; - - if (s_configurationCache.TryGetValue(key, out handle) && handle.TryAddRentCount()) - { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(null, $"Found cached MsQuicConfiguration: {handle}."); - } - return handle; - } - - // if we get here, the handle is either not in the cache, or we lost the race between - // TryAddRentCount on this thread and MarkForDispose on another thread doing cache cleanup. - // In either case, we need to create a new handle. - - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(null, $"MsQuicConfiguration not found in cache, creating new."); - } - - handle = CreateInternal(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); - handle.TryAddRentCount(); // we are the first renter - - MsQuicConfigurationSafeHandle cached; - do - { - cached = s_configurationCache.GetOrAdd(key, handle); - } - // If we get the same handle back, we successfully added it to the cache and we are done. - // If we get a different handle back, we need to increase the rent count. - // If we fail to add the rent count, then the existing/cached handle is in process of - // being removed from the cache and we can try again, eventually either succeeding to add our - // new handle or getting a fresh handle inserted by another thread meanwhile. - while (cached != handle && !cached.TryAddRentCount()); - - if (cached != handle) - { - // we lost a race with another thread to insert new handle into the cache - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(null, $"Discarding MsQuicConfiguration {handle} (preferring cached {cached})."); - } - - // First dispose decrements the rent count we added before attempting the cache insertion - // and second closes the handle - handle.Dispose(); - handle.Dispose(); - Debug.Assert(handle.IsClosed); - - return cached; - } - - // we added a new handle, check if we need to cleanup - var count = s_configurationCache.Count; - if (count % CheckExpiredModulo == 0) - { - // let only one thread perform cleanup at a time - lock (s_configurationCache) - { - // check again, if another thread just cleaned up (and cached count went down) we are unlikely - // to clean anything - if (s_configurationCache.Count >= count) - { - CleanupCache(); - } - } - } - - return handle; - } - - private static void CleanupCache() - { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, current size: {s_configurationCache.Count}."); - } - - foreach ((CacheKey key, MsQuicConfigurationSafeHandle handle) in s_configurationCache) - { - if (!handle.TryMarkForDispose()) - { - // handle in use - continue; - } - - // the handle is not in use and has been marked such that no new rents can be added. - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Info(null, $"Removing cached MsQuicConfiguration {handle}."); - } - - bool removed = s_configurationCache.TryRemove(key, out _); - Debug.Assert(removed); - handle.Dispose(); - Debug.Assert(handle.IsClosed); - } - - if (NetEventSource.Log.IsEnabled()) + return s_configurationCache.GetOrCreate(key, (args) => { - NetEventSource.Info(null, $"Cleaning up MsQuicConfiguration cache, new size: {s_configurationCache.Count}."); - } + (settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites) = args; + return CreateInternal(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); + }, (settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites)); } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs index 8e70ec2057245..3015eff1767be 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicSafeHandle.cs @@ -145,7 +145,7 @@ protected override unsafe bool ReleaseHandle() } } -internal sealed class MsQuicConfigurationSafeHandle : MsQuicSafeHandle +internal sealed class MsQuicConfigurationSafeHandle : MsQuicSafeHandle, ISafeHandleCachable { // MsQuicConfiguration handles are cached, so we need to keep track of the // number of times a handle is rented. Once we decide to dispose the handle, From d15357d0e991c2a35dcedd3aa3df5a80738e39d4 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 4 Jun 2024 12:23:37 +0200 Subject: [PATCH 12/16] Use shared cache code for client SSL_CTX cache --- .../Interop.OpenSsl.cs | 128 ++++++++++-------- .../Interop.Ssl.cs | 4 +- .../Interop.SslCtx.cs | 41 +++++- .../src/System/{ => Net}/SafeHandleCache.cs | 8 ++ .../src/System.Net.Quic.csproj | 2 +- .../Internal/MsQuicConfiguration.Cache.cs | 2 +- .../src/System.Net.Security.csproj | 4 +- .../System.Net.Security.Unit.Tests.csproj | 4 +- 8 files changed, 127 insertions(+), 66 deletions(-) rename src/libraries/Common/src/System/{ => Net}/SafeHandleCache.cs (93%) 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 6485d78e854ec..d2cfca8e320f2 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 @@ -24,7 +24,10 @@ internal static partial class OpenSsl private const string TlsCacheSizeCtxName = "System.Net.Security.TlsCacheSize"; private const string TlsCacheSizeEnvironmentVariable = "DOTNET_SYSTEM_NET_SECURITY_TLSCACHESIZE"; private const SslProtocols FakeAlpnSslProtocol = (SslProtocols)1; // used to distinguish server sessions with ALPN - private static readonly ConcurrentDictionary s_clientSslContexts = new ConcurrentDictionary(); + + private sealed class SafeSslContextCache : SafeHandleCache { } + + private static readonly SafeSslContextCache s_clientSslContexts = new(); internal readonly struct SslContextCacheKey : IEquatable { @@ -145,6 +148,54 @@ private static SslProtocols CalculateEffectiveProtocols(SslAuthenticationOptions return protocols; } + internal static SafeSslContextHandle GetOrCreateSslContextHandle(SslAuthenticationOptions sslAuthenticationOptions, bool allowCached) + { + SslProtocols protocols = CalculateEffectiveProtocols(sslAuthenticationOptions); + + if (!allowCached) + { + return AllocateSslContext(sslAuthenticationOptions, protocols, allowCached); + } + + if (!sslAuthenticationOptions.IsServer) + { + var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash(HashAlgorithmName.SHA256)); + + 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; + + SafeSslContextHandle? handle = AllocateSslContext(sslAuthenticationOptions, protocols, allowCached); + + if (!sslAuthenticationOptions.CertificateContext!.SslContexts!.TryGetValue(protocols | (hasAlpn ? FakeAlpnSslProtocol : SslProtocols.None), out handle)) + { + // not found in cache, create and insert + handle = AllocateSslContext(sslAuthenticationOptions, protocols, allowCached); + + SafeSslContextHandle cached = sslAuthenticationOptions.CertificateContext!.SslContexts!.GetOrAdd(protocols | (hasAlpn ? FakeAlpnSslProtocol : SslProtocols.None), 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; + } + } + + Debug.Assert(!handle.IsClosed); + handle.TryAddRentCount(); + return handle; + } + // This essentially wraps SSL_CTX* aka SSL_CTX_new + setting internal static unsafe SafeSslContextHandle AllocateSslContext(SslAuthenticationOptions sslAuthenticationOptions, SslProtocols protocols, bool enableResume) { @@ -289,10 +340,6 @@ internal static void UpdateClientCertificate(SafeSslHandle ssl, SslAuthenticatio internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuthenticationOptions) { SafeSslHandle? sslHandle = null; - SafeSslContextHandle? sslCtxHandle = null; - SafeSslContextHandle? toDisposeHandle = null; - SslProtocols protocols = CalculateEffectiveProtocols(sslAuthenticationOptions); - bool hasAlpn = sslAuthenticationOptions.ApplicationProtocols != null && sslAuthenticationOptions.ApplicationProtocols.Count != 0; bool cacheSslContext = sslAuthenticationOptions.AllowTlsResume && !SslStream.DisableTlsResume && sslAuthenticationOptions.EncryptionPolicy == EncryptionPolicy.RequireEncryption && sslAuthenticationOptions.CipherSuitesPolicy == null; if (cacheSslContext) @@ -323,54 +370,14 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth } } - if (cacheSslContext) - { - if (sslAuthenticationOptions.IsServer) - { - sslAuthenticationOptions.CertificateContext!.SslContexts!.TryGetValue(protocols | (hasAlpn ? FakeAlpnSslProtocol : SslProtocols.None), out sslCtxHandle); - } - else - { - var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash(HashAlgorithmName.SHA256)); - s_clientSslContexts.TryGetValue(key, out sslCtxHandle); - } - } - - if (sslCtxHandle == null) - { - // We did not get SslContext from cache, allocate a one. If we - // won't end up caching the context, we will dispose the new - // SafeSslContextHandle at end of this method since we won't - // reuse it. SSL object created later keeps a reference to it - // and will free it when we close the SafeSslHandle. - - sslCtxHandle = toDisposeHandle = AllocateSslContext(sslAuthenticationOptions, protocols, cacheSslContext); - - if (cacheSslContext) - { - bool added; - - if (sslAuthenticationOptions.IsServer) - { - added = sslAuthenticationOptions.CertificateContext!.SslContexts!.TryAdd(protocols | (SslProtocols)(hasAlpn ? 1 : 0), sslCtxHandle); - } - else - { - var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash()); - - // Check for concurrent inserts, if not added, use the existing one, - // the new one will be disposed. - sslCtxHandle = s_clientSslContexts.GetOrAdd(key, sslCtxHandle); - added = sslCtxHandle == toDisposeHandle; - } - - if (added) - { - // handle will be cached, don't dispose it - toDisposeHandle = null; - } - } - } + // We do not touch the SSL_CTX after we create and configure SSL + // objects, and SSL object created later in this function will keep an + // outstanding up-ref on SSL_CTX. + // + // For uncached SafeSslContextHandles, the handle will be disposed and closed. + // Cached SafeSslContextHandles are returned with increaset rent count so that + // Dispose() here will not close the handle. + using SafeSslContextHandle sslCtxHandle = GetOrCreateSslContextHandle(sslAuthenticationOptions, cacheSslContext); GCHandle alpnHandle = default; try @@ -411,12 +418,17 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth Crypto.ErrClearError(); } - if (cacheSslContext) { sslCtxHandle.TrySetSession(sslHandle, sslAuthenticationOptions.TargetHost); - bool ignored = false; - sslCtxHandle.DangerousAddRef(ref ignored); + + // 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; } } @@ -485,10 +497,6 @@ internal static SafeSslHandle AllocateSslHandle(SslAuthenticationOptions sslAuth throw; } - finally - { - toDisposeHandle?.Dispose(); - } return sslHandle; } 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 be3176f4a752a..9f3d05e43e96a 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 @@ -445,7 +445,9 @@ protected override bool ReleaseHandle() Disconnect(); } - SslContextHandle?.DangerousRelease(); + // 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 d92e15e940e65..bad664886dac0 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 @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System; +using System.Net; using System.Collections.Generic; using System.Collections.ObjectModel; using System.Diagnostics; @@ -9,6 +10,7 @@ using System.Runtime.InteropServices; using System.Security.Cryptography.X509Certificates; using System.Text; +using System.Threading; using Microsoft.Win32.SafeHandles; internal static partial class Interop @@ -65,12 +67,17 @@ internal static bool AddExtraChainCertificates(SafeSslContextHandle ctx, ReadOnl namespace Microsoft.Win32.SafeHandles { - internal sealed class SafeSslContextHandle : SafeHandle + internal sealed class SafeSslContextHandle : SafeHandle, ISafeHandleCachable { // This is session cache keyed by SNI e.g. TargetHost private Dictionary? _sslSessions; private GCHandle _gch; + // SSL_CTX handles are cached, so we need to keep track of the + // number of times a handle is being used. Once we decide to dispose the handle, + // we set the _rentCount to -1. + private volatile int _rentCount; + public SafeSslContextHandle() : base(IntPtr.Zero, true) { @@ -86,6 +93,38 @@ public override bool IsInvalid get { return handle == IntPtr.Zero; } } + public bool TryAddRentCount() + { + int oldCount; + + do + { + oldCount = _rentCount; + if (oldCount < 0) + { + // The handle is already disposed. + return false; + } + } while (Interlocked.CompareExchange(ref _rentCount, oldCount + 1, oldCount) != oldCount); + + return true; + } + + public bool TryMarkForDispose() + { + return Interlocked.CompareExchange(ref _rentCount, -1, 0) == 0; + } + + protected override void Dispose(bool disposing) + { + if (Interlocked.Decrement(ref _rentCount) < 0) + { + // _rentCount is 0 if the handle was never rented (e.g. failure during creation), + // and is -1 when evicted from cache. + base.Dispose(disposing); + } + } + protected override bool ReleaseHandle() { if (_sslSessions != null) diff --git a/src/libraries/Common/src/System/SafeHandleCache.cs b/src/libraries/Common/src/System/Net/SafeHandleCache.cs similarity index 93% rename from src/libraries/Common/src/System/SafeHandleCache.cs rename to src/libraries/Common/src/System/Net/SafeHandleCache.cs index 2691f8d55afb8..5fd9b6855a410 100644 --- a/src/libraries/Common/src/System/SafeHandleCache.cs +++ b/src/libraries/Common/src/System/Net/SafeHandleCache.cs @@ -11,7 +11,15 @@ namespace System.Net { internal interface ISafeHandleCachable { + // Attempts to resever the handle for use. If the handle is already + // disposed (or scheduled to be disposed), this will return false. + // + // each successful call to TryAddRentCount() must be paired with a Dispose() call. bool TryAddRentCount(); + + // Marks the handle as scheduled for disposal if it is not being used. + // Returns false if the handle is currently being used. + // once marked, no new renters are allowed. bool TryMarkForDispose(); } diff --git a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj index 4f77f90e7cfa6..3dc678ce8442c 100644 --- a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj +++ b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj @@ -29,7 +29,7 @@ - + diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index f7d2e751611d1..5c9fe5666ab57 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -133,7 +133,7 @@ private static MsQuicConfigurationSafeHandle GetCachedCredentialOrCreate(QUIC_SE { CacheKey key = new CacheKey(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); - return s_configurationCache.GetOrCreate(key, (args) => + return s_configurationCache.GetOrCreate(key, static (args) => { (settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites) = args; return CreateInternal(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); diff --git a/src/libraries/System.Net.Security/src/System.Net.Security.csproj b/src/libraries/System.Net.Security/src/System.Net.Security.csproj index e6650f80670e1..2e263173ad81d 100644 --- a/src/libraries/System.Net.Security/src/System.Net.Security.csproj +++ b/src/libraries/System.Net.Security/src/System.Net.Security.csproj @@ -353,8 +353,10 @@ Link="Common\Interop\Unix\System.Security.Cryptography.Native\Interop.OCSP.cs" /> + + Link="Common\Interop\Unix\System.Security.Cryptography.Native\Interop.OpenSslVersion.cs" /> + + Link="Common\Interop\Unix\System.Security.Cryptography.Native\Interop.OpenSslVersion.cs" /> Date: Tue, 4 Jun 2024 17:26:15 +0200 Subject: [PATCH 13/16] Fix build --- .../src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs index 5c9fe5666ab57..4db0f88bf71a4 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicConfiguration.Cache.cs @@ -135,7 +135,7 @@ private static MsQuicConfigurationSafeHandle GetCachedCredentialOrCreate(QUIC_SE return s_configurationCache.GetOrCreate(key, static (args) => { - (settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites) = args; + var (settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites) = args; return CreateInternal(settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites); }, (settings, flags, certificate, intermediates, alpnProtocols, allowedCipherSuites)); } From 87aa08609186112a9a2ee4e568006bffe866c51a Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Thu, 6 Jun 2024 13:36:26 +0200 Subject: [PATCH 14/16] Update src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs --- .../Unix/System.Security.Cryptography.Native/Interop.OpenSsl.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 d2cfca8e320f2..a255394728683 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 @@ -157,7 +157,7 @@ internal static SafeSslContextHandle GetOrCreateSslContextHandle(SslAuthenticati return AllocateSslContext(sslAuthenticationOptions, protocols, allowCached); } - if (!sslAuthenticationOptions.IsServer) + if (sslAuthenticationOptions.IsClient) { var key = new SslContextCacheKey(protocols, sslAuthenticationOptions.CertificateContext?.TargetCertificate.GetCertHash(HashAlgorithmName.SHA256)); From 856eeaa9a949e43944995b13fbb4251cee6e07a8 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 6 Jun 2024 19:48:20 +0200 Subject: [PATCH 15/16] Fix tests --- .../SslStreamAllowTlsResumeTests.cs | 46 +++++++++++++------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs index a31f63435913c..ee1da9c92d474 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs @@ -1,13 +1,17 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Collections.Generic; using System.Reflection; using System.Security.Authentication; using System.Security.Cryptography.X509Certificates; using System.Threading.Tasks; +using System.Linq; using Xunit; using Microsoft.DotNet.XUnitExtensions; +using System.Net.Test.Common; + #if DEBUG namespace System.Net.Security.Tests { @@ -130,8 +134,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } [Theory] - [InlineData(SslProtocols.Tls12)] - [InlineData(SslProtocols.Tls13)] + [MemberData(nameof(SslProtocolsData))] public Task NoClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions @@ -151,6 +154,25 @@ public Task NoClientCert_DefaultValue_ResumeSucceeds(SslProtocols sslProtocol) return ResumeSucceedsInternal(serverOptions, clientOptions); } + public static TheoryData SslProtocolsData() + { + var data = new TheoryData(); + + data.Add(SslProtocols.None); + + if (PlatformDetection.SupportsTls12) + { + data.Add(SslProtocols.Tls12); + } + + if (PlatformDetection.SupportsTls13) + { + data.Add(SslProtocols.Tls13); + } + + return data; + } + public enum ClientCertSource { ClientCertificate, @@ -162,7 +184,7 @@ public static TheoryData ClientCertTestDat { var data = new TheoryData(); - foreach (SslProtocols protocol in new [] { SslProtocols.Tls12, SslProtocols.Tls13 }) + foreach (SslProtocols protocol in SslProtocolsData().Select(x => x[0])) foreach (bool certRequired in new[] { true, false }) foreach (ClientCertSource source in Enum.GetValues(typeof(ClientCertSource))) { @@ -246,8 +268,7 @@ await TestConfiguration.WhenAllOrAnyFailedWithTimeout( } [Theory] - [InlineData(SslProtocols.Tls12)] - [InlineData(SslProtocols.Tls13)] + [MemberData(nameof(SslProtocolsData))] public Task ClientChangeCert_NoResume(SslProtocols sslProtocol) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions @@ -273,8 +294,7 @@ public Task ClientChangeCert_NoResume(SslProtocols sslProtocol) } [Theory] - [InlineData(SslProtocols.Tls12)] - [InlineData(SslProtocols.Tls13)] + [MemberData(nameof(SslProtocolsData))] public Task DifferentHost_NoResume(SslProtocols sslProtocol) { SslServerAuthenticationOptions serverOptions = new SslServerAuthenticationOptions @@ -291,7 +311,7 @@ public Task DifferentHost_NoResume(SslProtocols sslProtocol) RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, ClientCertificateContext = SslStreamCertificateContext.Create(Configuration.Certificates.GetClientCertificate(), null, false) }; - + return TestNoResumeAfterChange(serverOptions, clientOptions, (clientOps, _) => clientOps.TargetHost = Guid.NewGuid().ToString("N")); } @@ -311,7 +331,7 @@ public Task DifferentProtocol_NoResume() CertificateRevocationCheckMode = X509RevocationMode.NoCheck, RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, }; - + return TestNoResumeAfterChange(serverOptions, clientOptions, (clientOps, _) => clientOps.EnabledSslProtocols = SslProtocols.None); } @@ -330,7 +350,7 @@ public Task DifferentRevocationCheckMode_NoResume() TargetHost = Guid.NewGuid().ToString("N"), RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, }; - + return TestNoResumeAfterChange(serverOptions, clientOptions, (clientOps, _) => clientOps.CertificateRevocationCheckMode = X509RevocationMode.NoCheck); } @@ -350,10 +370,10 @@ public Task DifferentEncryptionPolicy_NoResume() CertificateRevocationCheckMode = X509RevocationMode.NoCheck, RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, }; -#pragma warning disable SYSLIB0040 // 'AllowNoEncryption' is obsolete +#pragma warning disable SYSLIB0040 // 'AllowNoEncryption' is obsolete return TestNoResumeAfterChange(serverOptions, clientOptions, (clientOps, _) => clientOps.EncryptionPolicy = EncryptionPolicy.AllowNoEncryption); -#pragma warning restore SYSLIB0040 // 'AllowNoEncryption' is obsolete +#pragma warning restore SYSLIB0040 // 'AllowNoEncryption' is obsolete } [Fact] @@ -371,7 +391,7 @@ public Task DifferentCipherSuitesPolicy_NoResume() CertificateRevocationCheckMode = X509RevocationMode.NoCheck, RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, }; - + return TestNoResumeAfterChange(serverOptions, clientOptions, (clientOps, _) => clientOps.CipherSuitesPolicy = new CipherSuitesPolicy(new[] { TlsCipherSuite.TLS_AES_128_GCM_SHA256 })); } From 0d65ed38ef6a4e6e486a9f299b1fb948402e855b Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 11 Jun 2024 14:20:37 +0200 Subject: [PATCH 16/16] Fix test --- .../tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs index ee1da9c92d474..bc0df20697b38 100644 --- a/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs +++ b/src/libraries/System.Net.Security/tests/FunctionalTests/SslStreamAllowTlsResumeTests.cs @@ -349,6 +349,7 @@ public Task DifferentRevocationCheckMode_NoResume() { TargetHost = Guid.NewGuid().ToString("N"), RemoteCertificateValidationCallback = (sender, cert, chain, errors) => true, + CertificateRevocationCheckMode = X509RevocationMode.Offline, }; return TestNoResumeAfterChange(serverOptions, clientOptions,