From 74575d4eb1e86f1a34d0d0fa61143320c636136c Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 12 Feb 2024 13:37:53 +0100 Subject: [PATCH 01/12] Allow switching execution profiles using env vars --- .../System.Net.Quic/src/System.Net.Quic.csproj | 1 + .../src/System/Net/Quic/Internal/MsQuicApi.cs | 15 ++++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) 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 cd56ff8e72bd0..fc16388b4bfe3 100644 --- a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj +++ b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj @@ -127,6 +127,7 @@ + diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs index e89119844c744..4a34f3cc0d1f8 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs @@ -38,10 +38,23 @@ private MsQuicApi(QUIC_API_TABLE* apiTable) fixed (byte* pAppName = "System.Net.Quic"u8) { + + QUIC_EXECUTION_PROFILE profile = QUIC_EXECUTION_PROFILE.LOW_LATENCY; + + if (Environment.GetEnvironmentVariable("DOTNET_QUIC_EXECUTION_PROFILE") is string p) + { + if (Enum.TryParse(p, out QUIC_EXECUTION_PROFILE newProfile)) + { + profile = newProfile; + } + } + + System.Console.WriteLine($"Execution profile: {profile}"); + var cfg = new QUIC_REGISTRATION_CONFIG { AppName = (sbyte*)pAppName, - ExecutionProfile = QUIC_EXECUTION_PROFILE.LOW_LATENCY + ExecutionProfile = profile }; QUIC_HANDLE* handle; From aada566d76654985755bbd3fe73b424e1436cbb7 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 12 Feb 2024 15:43:16 +0100 Subject: [PATCH 02/12] Quick and dirty version to enable benchmarking --- .../Quic/Internal/MsQuicApi.NativeMethods.cs | 17 +++++ .../QuicConnection.SslConnectionOptions.cs | 70 ++++++------------- .../src/System/Net/Quic/QuicConnection.cs | 65 +++++++++++++++-- 3 files changed, 97 insertions(+), 55 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs index 206eac76ac787..92dc51388c52d 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs @@ -375,4 +375,21 @@ public int StreamReceiveSetEnabled(MsQuicSafeHandle stream, byte enabled) } } } + + public int ConnectionCertificateValidationComplete(MsQuicSafeHandle connection, bool result, QUIC_TLS_ALERT_CODES alert) + { + bool success = false; + try + { + connection.DangerousAddRef(ref success); + return ApiTable->ConnectionCertificateValidationComplete(connection.QuicHandle, (byte)(result ? 1 : 0), alert); + } + finally + { + if (success) + { + connection.DangerousRelease(); + } + } + } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index dad23bfc342c8..e5e2c91d90189 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -63,65 +63,38 @@ public SslConnectionOptions(QuicConnection connection, bool isClient, _certificateChainPolicy = certificateChainPolicy; } - public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER* chainPtr, out X509Certificate2? certificate) + public unsafe int ValidateCertificate(X509Certificate2? certificate, X509Chain? chain) { SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None; - IntPtr certificateBuffer = 0; - int certificateLength = 0; bool wrapException = false; - X509Chain? chain = null; - X509Certificate2? result = null; try { - if (certificatePtr is not null) - { - chain = new X509Chain(); - if (_certificateChainPolicy != null) - { - chain.ChainPolicy = _certificateChainPolicy; - } - else - { - chain.ChainPolicy.RevocationMode = _revocationMode; - chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot; - - // TODO: configure chain.ChainPolicy.CustomTrustStore to mirror behavior of SslStream.VerifyRemoteCertificate (https://github.com/dotnet/runtime/issues/73053) - } + chain ??= new X509Chain(); - // set ApplicationPolicy unless already provided. - if (chain.ChainPolicy.ApplicationPolicy.Count == 0) - { - // Authenticate the remote party: (e.g. when operating in server mode, authenticate the client). - chain.ChainPolicy.ApplicationPolicy.Add(_isClient ? s_serverAuthOid : s_clientAuthOid); - } + if (_certificateChainPolicy != null) + { + chain.ChainPolicy = _certificateChainPolicy; + } + else + { + chain.ChainPolicy.RevocationMode = _revocationMode; + chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot; - if (MsQuicApi.UsesSChannelBackend) - { - result = new X509Certificate2((IntPtr)certificatePtr); - } - else - { - if (certificatePtr->Length > 0) - { - certificateBuffer = (IntPtr)certificatePtr->Buffer; - certificateLength = (int)certificatePtr->Length; - result = new X509Certificate2(certificatePtr->Span); - } + // TODO: configure chain.ChainPolicy.CustomTrustStore to mirror behavior of SslStream.VerifyRemoteCertificate (https://github.com/dotnet/runtime/issues/73053) + } - if (chainPtr->Length > 0) - { - X509Certificate2Collection additionalCertificates = new X509Certificate2Collection(); - additionalCertificates.Import(chainPtr->Span); - chain.ChainPolicy.ExtraStore.AddRange(additionalCertificates); - } - } + // set ApplicationPolicy unless already provided. + if (chain.ChainPolicy.ApplicationPolicy.Count == 0) + { + // Authenticate the remote party: (e.g. when operating in server mode, authenticate the client). + chain.ChainPolicy.ApplicationPolicy.Add(_isClient ? s_serverAuthOid : s_clientAuthOid); } - if (result is not null) + if (certificate is not null) { bool checkCertName = !chain!.ChainPolicy!.VerificationFlags.HasFlag(X509VerificationFlags.IgnoreInvalidName); - sslPolicyErrors |= CertificateValidation.BuildChainAndVerifyProperties(chain!, result, checkCertName, !_isClient, TargetHostNameHelper.NormalizeHostName(_targetHost), certificateBuffer, certificateLength); + sslPolicyErrors |= CertificateValidation.BuildChainAndVerifyProperties(chain!, certificate, checkCertName, !_isClient, TargetHostNameHelper.NormalizeHostName(_targetHost), IntPtr.Zero, 0); } else if (_certificateRequired) { @@ -132,7 +105,7 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER* if (_validationCallback is not null) { wrapException = true; - if (!_validationCallback(_connection, result, chain, sslPolicyErrors)) + if (!_validationCallback(_connection, certificate, chain, sslPolicyErrors)) { wrapException = false; if (_isClient) @@ -153,12 +126,11 @@ public unsafe int ValidateCertificate(QUIC_BUFFER* certificatePtr, QUIC_BUFFER* status = QUIC_STATUS_HANDSHAKE_FAILURE; } - certificate = result; return status; } catch (Exception ex) { - result?.Dispose(); + certificate?.Dispose(); if (wrapException) { throw new QuicException(QuicError.CallbackError, null, SR.net_quic_callback_error, ex); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index db3adf776d542..b0fd5a7c9ce9b 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Diagnostics.Metrics; using System.Net.Security; using System.Net.Sockets; using System.Runtime.CompilerServices; @@ -571,15 +572,56 @@ private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA dat } private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEIVED_DATA data) { - try + X509Certificate2? certificate = null; + X509Chain? chain = null; + + QUIC_BUFFER* certificatePtr = (QUIC_BUFFER*)data.Certificate; + QUIC_BUFFER* chainPtr = (QUIC_BUFFER*)data.Chain; + + if (certificatePtr is not null) { - return _sslConnectionOptions.ValidateCertificate((QUIC_BUFFER*)data.Certificate, (QUIC_BUFFER*)data.Chain, out _remoteCertificate); + chain = new X509Chain(); + if (MsQuicApi.UsesSChannelBackend) + { + certificate = new X509Certificate2((IntPtr)certificatePtr); + } + else + { + if (certificatePtr->Length > 0) + { + certificate = new X509Certificate2(certificatePtr->Span); + } + + if (chainPtr->Length > 0) + { + X509Certificate2Collection additionalCertificates = new X509Certificate2Collection(); + additionalCertificates.Import(chainPtr->Span); + chain.ChainPolicy.ExtraStore.AddRange(additionalCertificates); + } + } } - catch (Exception ex) + + _ = Task.Run(() => { - _connectedTcs.TrySetException(ex); - return QUIC_STATUS_HANDSHAKE_FAILURE; - } + int result; + try + { + result = _sslConnectionOptions.ValidateCertificate(certificate, chain); + _remoteCertificate = certificate; + } + catch (Exception ex) + { + _connectedTcs.TrySetException(ex); + result = QUIC_STATUS_HANDSHAKE_FAILURE; + } + + bool success = result == QUIC_STATUS_SUCCESS; + + // return result; + MsQuicApi.Api.ConnectionCertificateValidationComplete(_handle, success, success ? QUIC_TLS_ALERT_CODES.SUCCESS : QUIC_TLS_ALERT_CODES.BAD_CERTIFICATE); + }); + + return QUIC_STATUS_PENDING; } private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEvent) @@ -596,6 +638,9 @@ private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEve _ => QUIC_STATUS_SUCCESS, }; + private static readonly Meter s_meter = new Meter("System.Net.Quic.QuicConnection.Events"); + private static readonly Histogram s_eventProcessing = s_meter.CreateHistogram("eventloop.process", "ms", ""); + #pragma warning disable CS3016 [UnmanagedCallersOnly(CallConvs = new Type[] { typeof(CallConvCdecl) })] #pragma warning restore CS3016 @@ -613,6 +658,7 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context, return QUIC_STATUS_INVALID_STATE; } + long timestamp = Stopwatch.GetTimestamp(); try { // Process the event. @@ -630,6 +676,13 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context, } return QUIC_STATUS_INTERNAL_ERROR; } + finally + { + var elapsed = Stopwatch.GetElapsedTime(timestamp); + TagList tags = default; + tags.Add("Type", connectionEvent->Type.ToString()); + s_eventProcessing.Record(elapsed.TotalMilliseconds, tags); + } } /// From 5b3d46b24969fa1858ad5eff8003c4068e4fb9d2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 13 Feb 2024 16:51:00 +0100 Subject: [PATCH 03/12] Don't call callbacks from MsQuic threads --- .../src/System.Net.Quic.csproj | 1 - .../src/System/Net/Quic/Internal/MsQuicApi.cs | 15 +---------- .../src/System/Net/Quic/QuicConnection.cs | 27 +++++++++---------- .../src/System/Net/Quic/QuicListener.cs | 6 +++-- .../tests/FunctionalTests/MsQuicTests.cs | 10 +++---- 5 files changed, 23 insertions(+), 36 deletions(-) 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 fc16388b4bfe3..cd56ff8e72bd0 100644 --- a/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj +++ b/src/libraries/System.Net.Quic/src/System.Net.Quic.csproj @@ -127,7 +127,6 @@ - diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs index 4a34f3cc0d1f8..e89119844c744 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs @@ -38,23 +38,10 @@ private MsQuicApi(QUIC_API_TABLE* apiTable) fixed (byte* pAppName = "System.Net.Quic"u8) { - - QUIC_EXECUTION_PROFILE profile = QUIC_EXECUTION_PROFILE.LOW_LATENCY; - - if (Environment.GetEnvironmentVariable("DOTNET_QUIC_EXECUTION_PROFILE") is string p) - { - if (Enum.TryParse(p, out QUIC_EXECUTION_PROFILE newProfile)) - { - profile = newProfile; - } - } - - System.Console.WriteLine($"Execution profile: {profile}"); - var cfg = new QUIC_REGISTRATION_CONFIG { AppName = (sbyte*)pAppName, - ExecutionProfile = profile + ExecutionProfile = QUIC_EXECUTION_PROFILE.LOW_LATENCY }; QUIC_HANDLE* handle; diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index b0fd5a7c9ce9b..41303253430e3 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; -using System.Diagnostics.Metrics; using System.Net.Security; using System.Net.Sockets; using System.Runtime.CompilerServices; @@ -601,6 +600,13 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI } } + // + // the certificate validation is an expensive operation and we don't want to delay MsQuic + // worker thread. So we offload the validation to the .NET threadpool. Incidentally, this + // also prevents potential user RemoteCertificateValidationCallback from blocking MsQuic + // worker threads. + // + _ = Task.Run(() => { int result; @@ -615,10 +621,13 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI result = QUIC_STATUS_HANDSHAKE_FAILURE; } - bool success = result == QUIC_STATUS_SUCCESS; + int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( + _handle, + MsQuic.StatusSucceeded(result), + MsQuic.StatusSucceeded(result) ? QUIC_TLS_ALERT_CODES.SUCCESS : QUIC_TLS_ALERT_CODES.USER_CANCELED); - // return result; - MsQuicApi.Api.ConnectionCertificateValidationComplete(_handle, success, success ? QUIC_TLS_ALERT_CODES.SUCCESS : QUIC_TLS_ALERT_CODES.BAD_CERTIFICATE); + _tlsSecret?.WriteSecret(); + Debug.Assert(MsQuic.StatusSucceeded(status), $"ConnectionCertificateValidationComplete failed with 0x{status:X}"); }); return QUIC_STATUS_PENDING; @@ -638,8 +647,6 @@ private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEve _ => QUIC_STATUS_SUCCESS, }; - private static readonly Meter s_meter = new Meter("System.Net.Quic.QuicConnection.Events"); - private static readonly Histogram s_eventProcessing = s_meter.CreateHistogram("eventloop.process", "ms", ""); #pragma warning disable CS3016 [UnmanagedCallersOnly(CallConvs = new Type[] { typeof(CallConvCdecl) })] @@ -658,7 +665,6 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context, return QUIC_STATUS_INVALID_STATE; } - long timestamp = Stopwatch.GetTimestamp(); try { // Process the event. @@ -676,13 +682,6 @@ private static unsafe int NativeCallback(QUIC_HANDLE* connection, void* context, } return QUIC_STATUS_INTERNAL_ERROR; } - finally - { - var elapsed = Stopwatch.GetElapsedTime(timestamp); - TagList tags = default; - tags.Add("Type", connectionEvent->Type.ToString()); - s_eventProcessing.Record(elapsed.TotalMilliseconds, tags); - } } /// diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs index 6f0a0d8bb5b75..c98a71d562cbc 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs @@ -207,7 +207,7 @@ public async ValueTask AcceptConnectionAsync(CancellationToken c /// /// The new connection. /// The TLS ClientHello data. - private async void StartConnectionHandshake(QuicConnection connection, SslClientHelloInfo clientHello) + private async Task StartConnectionHandshake(QuicConnection connection, SslClientHelloInfo clientHello) { bool wrapException = false; CancellationToken cancellationToken = default; @@ -333,7 +333,9 @@ private unsafe int HandleEventNewConnection(ref NEW_CONNECTION_DATA data) SslClientHelloInfo clientHello = new SslClientHelloInfo(data.Info->ServerNameLength > 0 ? Marshal.PtrToStringUTF8((IntPtr)data.Info->ServerName, data.Info->ServerNameLength) : "", SslProtocols.Tls13); // Kicks off the rest of the handshake in the background, the process itself will enqueue the result in the accept queue. - StartConnectionHandshake(connection, clientHello); + // this also makes sure the connection options callback provided by the user is not invoked + // from the MsQuic thread and cannot delay acks or other operations on other connections. + _ = Task.Run(() => StartConnectionHandshake(connection, clientHello)); return QUIC_STATUS_SUCCESS; diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index 31177d6c15e04..078c2f07ac81f 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -389,11 +389,11 @@ public async Task CertificateCallbackThrowPropagates() await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); // Make sure the listener is still usable and there is no lingering bad connection - validationResult = true; - (QuicConnection clientConnection, QuicConnection serverConnection) = await CreateConnectedQuicConnection(listener); - await PingPong(clientConnection, serverConnection); - await clientConnection.DisposeAsync(); - await serverConnection.DisposeAsync(); + // validationResult = true; + // (QuicConnection clientConnection, QuicConnection serverConnection) = await CreateConnectedQuicConnection(listener); + // await PingPong(clientConnection, serverConnection); + // await clientConnection.DisposeAsync(); + // await serverConnection.DisposeAsync(); } [Fact] From 08cf80740a9fd75ef88ca7bc3477fd7ac8bb9037 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 13 Feb 2024 19:03:52 +0100 Subject: [PATCH 04/12] Remove unintentional changes --- .../src/System/Net/Quic/QuicConnection.cs | 1 - .../tests/FunctionalTests/MsQuicTests.cs | 10 +++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 41303253430e3..7b22d9e24b2a4 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -626,7 +626,6 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI MsQuic.StatusSucceeded(result), MsQuic.StatusSucceeded(result) ? QUIC_TLS_ALERT_CODES.SUCCESS : QUIC_TLS_ALERT_CODES.USER_CANCELED); - _tlsSecret?.WriteSecret(); Debug.Assert(MsQuic.StatusSucceeded(status), $"ConnectionCertificateValidationComplete failed with 0x{status:X}"); }); diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index 078c2f07ac81f..31177d6c15e04 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -389,11 +389,11 @@ public async Task CertificateCallbackThrowPropagates() await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); // Make sure the listener is still usable and there is no lingering bad connection - // validationResult = true; - // (QuicConnection clientConnection, QuicConnection serverConnection) = await CreateConnectedQuicConnection(listener); - // await PingPong(clientConnection, serverConnection); - // await clientConnection.DisposeAsync(); - // await serverConnection.DisposeAsync(); + validationResult = true; + (QuicConnection clientConnection, QuicConnection serverConnection) = await CreateConnectedQuicConnection(listener); + await PingPong(clientConnection, serverConnection); + await clientConnection.DisposeAsync(); + await serverConnection.DisposeAsync(); } [Fact] From 702ea9423320429e335e68a3d8a326a500f995c2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 14 Feb 2024 10:59:00 +0100 Subject: [PATCH 05/12] Offload parsing to threadpool as well --- .../Net/Security/CertificateValidation.OSX.cs | 27 +++++--- .../Security/CertificateValidation.Unix.cs | 2 +- .../Security/CertificateValidation.Windows.cs | 2 +- .../QuicConnection.SslConnectionOptions.cs | 50 ++++++++------ .../src/System/Net/Quic/QuicConnection.cs | 67 ++++++++++++++----- 5 files changed, 97 insertions(+), 51 deletions(-) diff --git a/src/libraries/Common/src/System/Net/Security/CertificateValidation.OSX.cs b/src/libraries/Common/src/System/Net/Security/CertificateValidation.OSX.cs index aee4b77b50834..b269a0fb70fa5 100644 --- a/src/libraries/Common/src/System/Net/Security/CertificateValidation.OSX.cs +++ b/src/libraries/Common/src/System/Net/Security/CertificateValidation.OSX.cs @@ -14,7 +14,7 @@ internal static class CertificateValidation private static readonly IdnMapping s_idnMapping = new IdnMapping(); // WARNING: This function will do the verification using OpenSSL. If the intention is to use OS function, caller should use CertificatePal interface. - internal static SslPolicyErrors BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, bool checkCertName, bool _ /*isServer*/, string? hostName, IntPtr certificateBuffer, int bufferLength = 0) + internal static SslPolicyErrors BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, bool checkCertName, bool _ /*isServer*/, string? hostName, Span certificateBuffer) { SslPolicyErrors errors = chain.Build(remoteCertificate) ? SslPolicyErrors.None : @@ -31,15 +31,24 @@ internal static SslPolicyErrors BuildChainAndVerifyProperties(X509Chain chain, X } SafeX509Handle certHandle; - if (certificateBuffer != IntPtr.Zero && bufferLength > 0) + unsafe { - certHandle = Interop.Crypto.DecodeX509(certificateBuffer, bufferLength); - } - else - { - // We dont't have DER encoded buffer. - byte[] der = remoteCertificate.Export(X509ContentType.Cert); - certHandle = Interop.Crypto.DecodeX509(Marshal.UnsafeAddrOfPinnedArrayElement(der, 0), der.Length); + if (certificateBuffer.Length > 0) + { + fixed (byte* pCert = certificateBuffer) + { + certHandle = Interop.Crypto.DecodeX509((IntPtr)pCert, certificateBuffer.Length); + } + } + else + { + // We dont't have DER encoded buffer. + byte[] der = remoteCertificate.Export(X509ContentType.Cert); + fixed (byte* pDer = der) + { + certHandle = Interop.Crypto.DecodeX509((IntPtr)pDer, der.Length); + } + } } int hostNameMatch; diff --git a/src/libraries/Common/src/System/Net/Security/CertificateValidation.Unix.cs b/src/libraries/Common/src/System/Net/Security/CertificateValidation.Unix.cs index 65a1adb492fa2..da3cb38a86822 100644 --- a/src/libraries/Common/src/System/Net/Security/CertificateValidation.Unix.cs +++ b/src/libraries/Common/src/System/Net/Security/CertificateValidation.Unix.cs @@ -13,7 +13,7 @@ internal static class CertificateValidation private static readonly IdnMapping s_idnMapping = new IdnMapping(); #pragma warning disable IDE0060 - internal static SslPolicyErrors BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, bool checkCertName, bool isServer, string? hostName, IntPtr certificateBuffer, int bufferLength) + internal static SslPolicyErrors BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, bool checkCertName, bool isServer, string? hostName, Span certificateBuffer) => BuildChainAndVerifyProperties(chain, remoteCertificate, checkCertName, isServer, hostName); #pragma warning restore IDE0060 diff --git a/src/libraries/Common/src/System/Net/Security/CertificateValidation.Windows.cs b/src/libraries/Common/src/System/Net/Security/CertificateValidation.Windows.cs index d068015e534c4..90be80c734cc7 100644 --- a/src/libraries/Common/src/System/Net/Security/CertificateValidation.Windows.cs +++ b/src/libraries/Common/src/System/Net/Security/CertificateValidation.Windows.cs @@ -14,7 +14,7 @@ namespace System.Net internal static partial class CertificateValidation { #pragma warning disable IDE0060 - internal static SslPolicyErrors BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, bool checkCertName, bool isServer, string? hostName, IntPtr certificateBuffer, int bufferLength) + internal static SslPolicyErrors BuildChainAndVerifyProperties(X509Chain chain, X509Certificate2 remoteCertificate, bool checkCertName, bool isServer, string? hostName, Span certificateBuffer) => BuildChainAndVerifyProperties(chain, remoteCertificate, checkCertName, isServer, hostName); #pragma warning restore IDE0060 diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index e5e2c91d90189..e037fa7a88542 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -63,38 +63,45 @@ public SslConnectionOptions(QuicConnection connection, bool isClient, _certificateChainPolicy = certificateChainPolicy; } - public unsafe int ValidateCertificate(X509Certificate2? certificate, X509Chain? chain) + public unsafe int ValidateCertificate(X509Certificate2? certificate, Span certData, Span chainData) { SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None; bool wrapException = false; + X509Chain? chain = null; try { - chain ??= new X509Chain(); - - if (_certificateChainPolicy != null) - { - chain.ChainPolicy = _certificateChainPolicy; - } - else + if (certificate is not null) { - chain.ChainPolicy.RevocationMode = _revocationMode; - chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot; + chain = new X509Chain(); + if (_certificateChainPolicy != null) + { + chain.ChainPolicy = _certificateChainPolicy; + } + else + { + chain.ChainPolicy.RevocationMode = _revocationMode; + chain.ChainPolicy.RevocationFlag = X509RevocationFlag.ExcludeRoot; - // TODO: configure chain.ChainPolicy.CustomTrustStore to mirror behavior of SslStream.VerifyRemoteCertificate (https://github.com/dotnet/runtime/issues/73053) - } + // TODO: configure chain.ChainPolicy.CustomTrustStore to mirror behavior of SslStream.VerifyRemoteCertificate (https://github.com/dotnet/runtime/issues/73053) + } - // set ApplicationPolicy unless already provided. - if (chain.ChainPolicy.ApplicationPolicy.Count == 0) - { - // Authenticate the remote party: (e.g. when operating in server mode, authenticate the client). - chain.ChainPolicy.ApplicationPolicy.Add(_isClient ? s_serverAuthOid : s_clientAuthOid); - } + // set ApplicationPolicy unless already provided. + if (chain.ChainPolicy.ApplicationPolicy.Count == 0) + { + // Authenticate the remote party: (e.g. when operating in server mode, authenticate the client). + chain.ChainPolicy.ApplicationPolicy.Add(_isClient ? s_serverAuthOid : s_clientAuthOid); + } + + if (chainData.Length > 0) + { + X509Certificate2Collection additionalCertificates = new X509Certificate2Collection(); + additionalCertificates.Import(chainData); + chain.ChainPolicy.ExtraStore.AddRange(additionalCertificates); + } - if (certificate is not null) - { bool checkCertName = !chain!.ChainPolicy!.VerificationFlags.HasFlag(X509VerificationFlags.IgnoreInvalidName); - sslPolicyErrors |= CertificateValidation.BuildChainAndVerifyProperties(chain!, certificate, checkCertName, !_isClient, TargetHostNameHelper.NormalizeHostName(_targetHost), IntPtr.Zero, 0); + sslPolicyErrors |= CertificateValidation.BuildChainAndVerifyProperties(chain!, certificate, checkCertName, !_isClient, TargetHostNameHelper.NormalizeHostName(_targetHost), certData); } else if (_certificateRequired) { @@ -130,7 +137,6 @@ public unsafe int ValidateCertificate(X509Certificate2? certificate, X509Chain? } catch (Exception ex) { - certificate?.Dispose(); if (wrapException) { throw new QuicException(QuicError.CallbackError, null, SR.net_quic_callback_error, ex); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 7b22d9e24b2a4..2bb36ec86468a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Buffers; using System.Diagnostics; using System.Net.Security; using System.Net.Sockets; @@ -571,55 +572,85 @@ private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA dat } private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEIVED_DATA data) { + // + // the certificate validation is an expensive operation and we don't want to delay MsQuic + // worker thread. So we offload the validation to the .NET threadpool. Incidentally, this + // also prevents potential user RemoteCertificateValidationCallback from blocking MsQuic + // worker threads. + // + // the provided data pointers are valid only while still inside the callback so they need to be + // copied before handing them to threadpool. + // + X509Certificate2? certificate = null; - X509Chain? chain = null; - QUIC_BUFFER* certificatePtr = (QUIC_BUFFER*)data.Certificate; - QUIC_BUFFER* chainPtr = (QUIC_BUFFER*)data.Chain; + byte[]? certDataRented = null; + Memory certData = default; + byte[]? chainDataRented = null; + Memory chainData = default; - if (certificatePtr is not null) + if (data.Certificate != null) { - chain = new X509Chain(); if (MsQuicApi.UsesSChannelBackend) { - certificate = new X509Certificate2((IntPtr)certificatePtr); + certificate = new X509Certificate2((IntPtr)data.Certificate); + // TODO: what about chainPtr? } else { + // on non-SChannel backends we specify USE_PORTABLE_CERTIFICATES and the content is buffers + // with DER encoded cert and chain + QUIC_BUFFER* certificatePtr = (QUIC_BUFFER*)data.Certificate; + QUIC_BUFFER* chainPtr = (QUIC_BUFFER*)data.Chain; + if (certificatePtr->Length > 0) { - certificate = new X509Certificate2(certificatePtr->Span); + certDataRented = ArrayPool.Shared.Rent((int)certificatePtr->Length); + certData = certDataRented.AsMemory(0, (int)certificatePtr->Length); + certificatePtr->Span.CopyTo(certData.Span); } if (chainPtr->Length > 0) { - X509Certificate2Collection additionalCertificates = new X509Certificate2Collection(); - additionalCertificates.Import(chainPtr->Span); - chain.ChainPolicy.ExtraStore.AddRange(additionalCertificates); + chainDataRented = ArrayPool.Shared.Rent((int)chainPtr->Length); + chainData = chainDataRented.AsMemory(0, (int)chainPtr->Length); + chainPtr->Span.CopyTo(chainData.Span); } } } - // - // the certificate validation is an expensive operation and we don't want to delay MsQuic - // worker thread. So we offload the validation to the .NET threadpool. Incidentally, this - // also prevents potential user RemoteCertificateValidationCallback from blocking MsQuic - // worker threads. - // - _ = Task.Run(() => { int result; try { - result = _sslConnectionOptions.ValidateCertificate(certificate, chain); + if (certData.Length > 0) + { + Debug.Assert(certificate == null); + certificate = new X509Certificate2(certData.Span); + } + + result = _sslConnectionOptions.ValidateCertificate(certificate, certData.Span, chainData.Span); _remoteCertificate = certificate; } catch (Exception ex) { + certificate?.Dispose(); _connectedTcs.TrySetException(ex); result = QUIC_STATUS_HANDSHAKE_FAILURE; } + finally + { + if (certDataRented != null) + { + ArrayPool.Shared.Return(certDataRented); + } + + if (chainDataRented != null) + { + ArrayPool.Shared.Return(chainDataRented); + } + } int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( _handle, From 17de07f5e6ca2b35de83e4ed66dd2a535d940309 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 14 Feb 2024 15:48:56 +0100 Subject: [PATCH 06/12] Customize TLS ALERT code --- .../Net/Quic/QuicConnection.SslConnectionOptions.cs | 10 +++++----- .../src/System/Net/Quic/QuicConnection.cs | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index e037fa7a88542..6e333265c0b74 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -63,7 +63,7 @@ public SslConnectionOptions(QuicConnection connection, bool isClient, _certificateChainPolicy = certificateChainPolicy; } - public unsafe int ValidateCertificate(X509Certificate2? certificate, Span certData, Span chainData) + public unsafe QUIC_TLS_ALERT_CODES ValidateCertificate(X509Certificate2? certificate, Span certData, Span chainData) { SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None; bool wrapException = false; @@ -108,7 +108,7 @@ public unsafe int ValidateCertificate(X509Certificate2? certificate, Span sslPolicyErrors |= SslPolicyErrors.RemoteCertificateNotAvailable; } - int status = QUIC_STATUS_SUCCESS; + QUIC_TLS_ALERT_CODES result = QUIC_TLS_ALERT_CODES.SUCCESS; if (_validationCallback is not null) { wrapException = true; @@ -120,7 +120,7 @@ public unsafe int ValidateCertificate(X509Certificate2? certificate, Span throw new AuthenticationException(SR.net_quic_cert_custom_validation); } - status = QUIC_STATUS_USER_CANCELED; + result = QUIC_TLS_ALERT_CODES.BAD_CERTIFICATE; } } else if (sslPolicyErrors != SslPolicyErrors.None) @@ -130,10 +130,10 @@ public unsafe int ValidateCertificate(X509Certificate2? certificate, Span throw new AuthenticationException(SR.Format(SR.net_quic_cert_chain_validation, sslPolicyErrors)); } - status = QUIC_STATUS_HANDSHAKE_FAILURE; + result = QUIC_TLS_ALERT_CODES.BAD_CERTIFICATE; } - return status; + return result; } catch (Exception ex) { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 2bb36ec86468a..a0b7e8a441689 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -621,7 +621,7 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI _ = Task.Run(() => { - int result; + QUIC_TLS_ALERT_CODES result; try { if (certData.Length > 0) @@ -637,7 +637,7 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI { certificate?.Dispose(); _connectedTcs.TrySetException(ex); - result = QUIC_STATUS_HANDSHAKE_FAILURE; + result = QUIC_TLS_ALERT_CODES.USER_CANCELED; } finally { @@ -654,8 +654,8 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( _handle, - MsQuic.StatusSucceeded(result), - MsQuic.StatusSucceeded(result) ? QUIC_TLS_ALERT_CODES.SUCCESS : QUIC_TLS_ALERT_CODES.USER_CANCELED); + result == QUIC_TLS_ALERT_CODES.SUCCESS, + result); Debug.Assert(MsQuic.StatusSucceeded(status), $"ConnectionCertificateValidationComplete failed with 0x{status:X}"); }); From 3b23b49dcf85275aba09598d59ebaf87b9e487d4 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 15 Feb 2024 12:38:08 +0100 Subject: [PATCH 07/12] Code review feedback --- .../Quic/Internal/MsQuicApi.NativeMethods.cs | 38 ++++++- .../QuicConnection.SslConnectionOptions.cs | 100 +++++++++++++++++- .../src/System/Net/Quic/QuicConnection.cs | 84 +-------------- 3 files changed, 136 insertions(+), 86 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs index 92dc51388c52d..6906392f79ebe 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.NativeMethods.cs @@ -376,13 +376,47 @@ public int StreamReceiveSetEnabled(MsQuicSafeHandle stream, byte enabled) } } - public int ConnectionCertificateValidationComplete(MsQuicSafeHandle connection, bool result, QUIC_TLS_ALERT_CODES alert) + public int DatagramSend(MsQuicSafeHandle connection, QUIC_BUFFER* buffers, uint buffersCount, QUIC_SEND_FLAGS flags, void* context) { bool success = false; try { connection.DangerousAddRef(ref success); - return ApiTable->ConnectionCertificateValidationComplete(connection.QuicHandle, (byte)(result ? 1 : 0), alert); + return ApiTable->DatagramSend(connection.QuicHandle, buffers, buffersCount, flags, context); + } + finally + { + if (success) + { + connection.DangerousRelease(); + } + } + } + + public int ConnectionResumptionTicketValidationComplete(MsQuicSafeHandle connection, byte result) + { + bool success = false; + try + { + connection.DangerousAddRef(ref success); + return ApiTable->ConnectionResumptionTicketValidationComplete(connection.QuicHandle, result); + } + finally + { + if (success) + { + connection.DangerousRelease(); + } + } + } + + public int ConnectionCertificateValidationComplete(MsQuicSafeHandle connection, byte result, QUIC_TLS_ALERT_CODES alert) + { + bool success = false; + try + { + connection.DangerousAddRef(ref success); + return ApiTable->ConnectionCertificateValidationComplete(connection.QuicHandle, result, alert); } finally { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index 6e333265c0b74..cb9c9855c1423 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -1,10 +1,13 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Buffers; +using System.Diagnostics; using System.Net.Security; using System.Security.Authentication; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; +using System.Threading.Tasks; using Microsoft.Quic; using static Microsoft.Quic.MsQuic; @@ -63,7 +66,102 @@ public SslConnectionOptions(QuicConnection connection, bool isClient, _certificateChainPolicy = certificateChainPolicy; } - public unsafe QUIC_TLS_ALERT_CODES ValidateCertificate(X509Certificate2? certificate, Span certData, Span chainData) + internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* chainPtr) + { + // + // the provided data pointers are valid only while still inside this function, so they need to be + // copied to separate buffers which are then handed off to threadpool. + // + + X509Certificate2? certificate = null; + + byte[]? certDataRented = null; + Memory certData = default; + byte[]? chainDataRented = null; + Memory chainData = default; + + if (certificatePtr != null) + { + if (MsQuicApi.UsesSChannelBackend) + { + certificate = new X509Certificate2((IntPtr)certificatePtr); + // TODO: what about chainPtr? + } + else + { + // on non-SChannel backends we specify USE_PORTABLE_CERTIFICATES and the content is buffers + // with DER encoded cert and chain + QUIC_BUFFER* certificateBuffer = (QUIC_BUFFER*)certificatePtr; + QUIC_BUFFER* chainBuffer = (QUIC_BUFFER*)chainPtr; + + if (certificateBuffer->Length > 0) + { + certDataRented = ArrayPool.Shared.Rent((int)certificateBuffer->Length); + certData = certDataRented.AsMemory(0, (int)certificateBuffer->Length); + certificateBuffer->Span.CopyTo(certData.Span); + } + + if (chainBuffer->Length > 0) + { + chainDataRented = ArrayPool.Shared.Rent((int)chainBuffer->Length); + chainData = chainDataRented.AsMemory(0, (int)chainBuffer->Length); + chainBuffer->Span.CopyTo(chainData.Span); + } + } + } + + // hand-off rest of the work to the threadpool, certificatePtr and chainPtr are invalid inside the lambda + QuicConnection thisConnection = _connection; // cannot use "this" inside lambda since SslConnectionOptions is struct + _ = Task.Run(() => + { + + QUIC_TLS_ALERT_CODES result; + try + { + if (certData.Length > 0) + { + Debug.Assert(certificate == null); + certificate = new X509Certificate2(certData.Span); + } + + result = thisConnection._sslConnectionOptions.ValidateCertificate(certificate, certData.Span, chainData.Span); + thisConnection._remoteCertificate = certificate; + } + catch (Exception ex) + { + certificate?.Dispose(); + thisConnection._connectedTcs.TrySetException(ex); + result = QUIC_TLS_ALERT_CODES.USER_CANCELED; + } + finally + { + if (certDataRented != null) + { + ArrayPool.Shared.Return(certDataRented); + } + + if (chainDataRented != null) + { + ArrayPool.Shared.Return(chainDataRented); + } + } + + int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( + thisConnection._handle, + result == QUIC_TLS_ALERT_CODES.SUCCESS ? (byte)1 : (byte)0, + result); + + if (MsQuic.StatusFailed(status)) + { + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Error(thisConnection, $"{thisConnection} ConnectionCertificateValidationComplete failed with {ThrowHelper.GetErrorMessageForStatus(status)}"); + } + } + }); + } + + private QUIC_TLS_ALERT_CODES ValidateCertificate(X509Certificate2? certificate, Span certData, Span chainData) { SslPolicyErrors sslPolicyErrors = SslPolicyErrors.None; bool wrapException = false; diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index a0b7e8a441689..18c08d5e65b0f 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Buffers; using System.Diagnostics; using System.Net.Security; using System.Net.Sockets; @@ -578,88 +577,8 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI // also prevents potential user RemoteCertificateValidationCallback from blocking MsQuic // worker threads. // - // the provided data pointers are valid only while still inside the callback so they need to be - // copied before handing them to threadpool. - // - - X509Certificate2? certificate = null; - - byte[]? certDataRented = null; - Memory certData = default; - byte[]? chainDataRented = null; - Memory chainData = default; - - if (data.Certificate != null) - { - if (MsQuicApi.UsesSChannelBackend) - { - certificate = new X509Certificate2((IntPtr)data.Certificate); - // TODO: what about chainPtr? - } - else - { - // on non-SChannel backends we specify USE_PORTABLE_CERTIFICATES and the content is buffers - // with DER encoded cert and chain - QUIC_BUFFER* certificatePtr = (QUIC_BUFFER*)data.Certificate; - QUIC_BUFFER* chainPtr = (QUIC_BUFFER*)data.Chain; - - if (certificatePtr->Length > 0) - { - certDataRented = ArrayPool.Shared.Rent((int)certificatePtr->Length); - certData = certDataRented.AsMemory(0, (int)certificatePtr->Length); - certificatePtr->Span.CopyTo(certData.Span); - } - - if (chainPtr->Length > 0) - { - chainDataRented = ArrayPool.Shared.Rent((int)chainPtr->Length); - chainData = chainDataRented.AsMemory(0, (int)chainPtr->Length); - chainPtr->Span.CopyTo(chainData.Span); - } - } - } - - _ = Task.Run(() => - { - QUIC_TLS_ALERT_CODES result; - try - { - if (certData.Length > 0) - { - Debug.Assert(certificate == null); - certificate = new X509Certificate2(certData.Span); - } - - result = _sslConnectionOptions.ValidateCertificate(certificate, certData.Span, chainData.Span); - _remoteCertificate = certificate; - } - catch (Exception ex) - { - certificate?.Dispose(); - _connectedTcs.TrySetException(ex); - result = QUIC_TLS_ALERT_CODES.USER_CANCELED; - } - finally - { - if (certDataRented != null) - { - ArrayPool.Shared.Return(certDataRented); - } - - if (chainDataRented != null) - { - ArrayPool.Shared.Return(chainDataRented); - } - } - - int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( - _handle, - result == QUIC_TLS_ALERT_CODES.SUCCESS, - result); - - Debug.Assert(MsQuic.StatusSucceeded(status), $"ConnectionCertificateValidationComplete failed with 0x{status:X}"); - }); + _sslConnectionOptions.StartAsyncCertificateValidation(data.Certificate, data.Chain); return QUIC_STATUS_PENDING; } @@ -677,7 +596,6 @@ private unsafe int HandleConnectionEvent(ref QUIC_CONNECTION_EVENT connectionEve _ => QUIC_STATUS_SUCCESS, }; - #pragma warning disable CS3016 [UnmanagedCallersOnly(CallConvs = new Type[] { typeof(CallConvCdecl) })] #pragma warning restore CS3016 From fce941c124b0ffa32f530bffc104dece303c9cd0 Mon Sep 17 00:00:00 2001 From: Radek Zikmund <32671551+rzikm@users.noreply.github.com> Date: Tue, 20 Feb 2024 17:55:04 +0100 Subject: [PATCH 08/12] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Marie Píchová <11718369+ManickaP@users.noreply.github.com> --- .../Net/Quic/QuicConnection.SslConnectionOptions.cs | 8 ++++---- .../System.Net.Quic/src/System/Net/Quic/QuicConnection.cs | 2 +- .../System.Net.Quic/src/System/Net/Quic/QuicListener.cs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index cb9c9855c1423..00baa96daa4a3 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -69,7 +69,7 @@ public SslConnectionOptions(QuicConnection connection, bool isClient, internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* chainPtr) { // - // the provided data pointers are valid only while still inside this function, so they need to be + // The provided data pointers are valid only while still inside this function, so they need to be // copied to separate buffers which are then handed off to threadpool. // @@ -89,8 +89,8 @@ internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* } else { - // on non-SChannel backends we specify USE_PORTABLE_CERTIFICATES and the content is buffers - // with DER encoded cert and chain + // On non-SChannel backends we specify USE_PORTABLE_CERTIFICATES and the content is buffers + // with DER encoded cert and chain. QUIC_BUFFER* certificateBuffer = (QUIC_BUFFER*)certificatePtr; QUIC_BUFFER* chainBuffer = (QUIC_BUFFER*)chainPtr; @@ -110,7 +110,7 @@ internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* } } - // hand-off rest of the work to the threadpool, certificatePtr and chainPtr are invalid inside the lambda + // Hand-off rest of the work to the threadpool, certificatePtr and chainPtr are invalid inside the lambda. QuicConnection thisConnection = _connection; // cannot use "this" inside lambda since SslConnectionOptions is struct _ = Task.Run(() => { diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 18c08d5e65b0f..b839a5356021a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -572,7 +572,7 @@ private unsafe int HandleEventPeerStreamStarted(ref PEER_STREAM_STARTED_DATA dat private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEIVED_DATA data) { // - // the certificate validation is an expensive operation and we don't want to delay MsQuic + // The certificate validation is an expensive operation and we don't want to delay MsQuic // worker thread. So we offload the validation to the .NET threadpool. Incidentally, this // also prevents potential user RemoteCertificateValidationCallback from blocking MsQuic // worker threads. diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs index c98a71d562cbc..a2a0bd9194ee1 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs @@ -333,7 +333,7 @@ private unsafe int HandleEventNewConnection(ref NEW_CONNECTION_DATA data) SslClientHelloInfo clientHello = new SslClientHelloInfo(data.Info->ServerNameLength > 0 ? Marshal.PtrToStringUTF8((IntPtr)data.Info->ServerName, data.Info->ServerNameLength) : "", SslProtocols.Tls13); // Kicks off the rest of the handshake in the background, the process itself will enqueue the result in the accept queue. - // this also makes sure the connection options callback provided by the user is not invoked + // This also makes sure the connection options callback provided by the user is not invoked // from the MsQuic thread and cannot delay acks or other operations on other connections. _ = Task.Run(() => StartConnectionHandshake(connection, clientHello)); From c595b0517f08a7109ea8d25d02e2191078fcb775 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 20 Feb 2024 19:22:27 +0100 Subject: [PATCH 09/12] use ConfigureAwaitOptions.ForceYielding --- .../src/System/Net/Quic/QuicListener.cs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs index a2a0bd9194ee1..88ea309054a7d 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.cs @@ -207,8 +207,13 @@ public async ValueTask AcceptConnectionAsync(CancellationToken c /// /// The new connection. /// The TLS ClientHello data. - private async Task StartConnectionHandshake(QuicConnection connection, SslClientHelloInfo clientHello) + private async void StartConnectionHandshake(QuicConnection connection, SslClientHelloInfo clientHello) { + // Yield to the threadpool immediately. This makes sure the connection options callback + // provided by the user is not invoked from the MsQuic thread and cannot delay acks + // or other operations on other connections. + await Task.CompletedTask.ConfigureAwait(ConfigureAwaitOptions.ForceYielding); + bool wrapException = false; CancellationToken cancellationToken = default; @@ -333,9 +338,7 @@ private unsafe int HandleEventNewConnection(ref NEW_CONNECTION_DATA data) SslClientHelloInfo clientHello = new SslClientHelloInfo(data.Info->ServerNameLength > 0 ? Marshal.PtrToStringUTF8((IntPtr)data.Info->ServerName, data.Info->ServerNameLength) : "", SslProtocols.Tls13); // Kicks off the rest of the handshake in the background, the process itself will enqueue the result in the accept queue. - // This also makes sure the connection options callback provided by the user is not invoked - // from the MsQuic thread and cannot delay acks or other operations on other connections. - _ = Task.Run(() => StartConnectionHandshake(connection, clientHello)); + StartConnectionHandshake(connection, clientHello); return QUIC_STATUS_SUCCESS; From 6b9142f7c6f4cb29993523326e982f322f70b4e2 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 26 Feb 2024 15:10:05 +0100 Subject: [PATCH 10/12] Version check to work around https://github.com/microsoft/msquic/issues/4132 --- .../src/System/Net/Quic/Internal/MsQuicApi.cs | 14 +++++++++---- .../QuicConnection.SslConnectionOptions.cs | 21 +++++++++++++++---- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs index e89119844c744..4b284284f5262 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicApi.cs @@ -54,11 +54,16 @@ private MsQuicApi(QUIC_API_TABLE* apiTable) private static readonly Lazy _api = new Lazy(AllocateMsQuicApi); internal static MsQuicApi Api => _api.Value; + internal static Version? Version { get; private set; } + internal static bool IsQuicSupported { get; } internal static string MsQuicLibraryVersion { get; } = "unknown"; internal static string? NotSupportedReason { get; } + // workaround for https://github.com/microsoft/msquic/issues/4132 + internal static bool SupportsAsyncCertValidation => Version >= new Version(2, 4, 0); + internal static bool UsesSChannelBackend { get; } internal static bool Tls13ServerMayBeDisabled { get; } @@ -69,6 +74,7 @@ static MsQuicApi() { bool loaded = false; IntPtr msQuicHandle; + Version = default; // MsQuic is using DualMode sockets and that will fail even for IPv4 if AF_INET6 is not available. if (!Socket.OSSupportsIPv6) @@ -135,7 +141,7 @@ static MsQuicApi() } return; } - Version version = new Version((int)libVersion[0], (int)libVersion[1], (int)libVersion[2], (int)libVersion[3]); + Version = new Version((int)libVersion[0], (int)libVersion[1], (int)libVersion[2], (int)libVersion[3]); paramSize = 64 * sizeof(sbyte); sbyte* libGitHash = stackalloc sbyte[64]; @@ -150,11 +156,11 @@ static MsQuicApi() } string? gitHash = Marshal.PtrToStringUTF8((IntPtr)libGitHash); - MsQuicLibraryVersion = $"{Interop.Libraries.MsQuic} {version} ({gitHash})"; + MsQuicLibraryVersion = $"{Interop.Libraries.MsQuic} {Version} ({gitHash})"; - if (version < s_minMsQuicVersion) + if (Version < s_minMsQuicVersion) { - NotSupportedReason = $"Incompatible MsQuic library version '{version}', expecting higher than '{s_minMsQuicVersion}'."; + NotSupportedReason = $"Incompatible MsQuic library version '{Version}', expecting higher than '{s_minMsQuicVersion}'."; if (NetEventSource.Log.IsEnabled()) { NetEventSource.Info(null, NotSupportedReason); diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index 00baa96daa4a3..063b8bb9f2ebf 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -110,11 +110,24 @@ internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* } } - // Hand-off rest of the work to the threadpool, certificatePtr and chainPtr are invalid inside the lambda. - QuicConnection thisConnection = _connection; // cannot use "this" inside lambda since SslConnectionOptions is struct - _ = Task.Run(() => + QuicConnection connection = _connection; + if (MsQuicApi.SupportsAsyncCertValidation) { + // hand-off rest of the work to the thread pool, certificatePtr and chainPtr are invalid beyond this point + _ = Task.Run(() => + { + StartAsyncCertificateValidationCore(connection, certificate, certData, chainData, certDataRented, chainDataRented); + }); + } + else + { + // due to a bug in MsQuic, we need to call the callback synchronously to close the connection properly when + // we reject the certificate + StartAsyncCertificateValidationCore(connection, certificate, certData, chainData, certDataRented, chainDataRented); + } + static void StartAsyncCertificateValidationCore(QuicConnection thisConnection, X509Certificate2? certificate, Memory certData, Memory chainData, byte[]? certDataRented, byte[]? chainDataRented) + { QUIC_TLS_ALERT_CODES result; try { @@ -158,7 +171,7 @@ internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* NetEventSource.Error(thisConnection, $"{thisConnection} ConnectionCertificateValidationComplete failed with {ThrowHelper.GetErrorMessageForStatus(status)}"); } } - }); + } } private QUIC_TLS_ALERT_CODES ValidateCertificate(X509Certificate2? certificate, Span certData, Span chainData) From 9fdb79040a837ccee8d46eea5c51f34c4addde62 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Mon, 26 Feb 2024 15:34:16 +0100 Subject: [PATCH 11/12] Use configure await to yield to threadpool --- .../QuicConnection.SslConnectionOptions.cs | 122 +++++++++--------- .../src/System/Net/Quic/QuicConnection.cs | 2 +- 2 files changed, 59 insertions(+), 65 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index 063b8bb9f2ebf..8770ae157ec8a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -66,7 +66,7 @@ public SslConnectionOptions(QuicConnection connection, bool isClient, _certificateChainPolicy = certificateChainPolicy; } - internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* chainPtr) + internal async void StartAsyncCertificateValidation(IntPtr certificatePtr, IntPtr chainPtr) { // // The provided data pointers are valid only while still inside this function, so they need to be @@ -80,96 +80,90 @@ internal unsafe void StartAsyncCertificateValidation(void* certificatePtr, void* byte[]? chainDataRented = null; Memory chainData = default; - if (certificatePtr != null) + if (certificatePtr != IntPtr.Zero) { if (MsQuicApi.UsesSChannelBackend) { - certificate = new X509Certificate2((IntPtr)certificatePtr); + // provided data is a pointer to a CERT_CONTEXT + certificate = new X509Certificate2(certificatePtr); // TODO: what about chainPtr? } else { - // On non-SChannel backends we specify USE_PORTABLE_CERTIFICATES and the content is buffers - // with DER encoded cert and chain. - QUIC_BUFFER* certificateBuffer = (QUIC_BUFFER*)certificatePtr; - QUIC_BUFFER* chainBuffer = (QUIC_BUFFER*)chainPtr; - - if (certificateBuffer->Length > 0) + unsafe { - certDataRented = ArrayPool.Shared.Rent((int)certificateBuffer->Length); - certData = certDataRented.AsMemory(0, (int)certificateBuffer->Length); - certificateBuffer->Span.CopyTo(certData.Span); - } + // On non-SChannel backends we specify USE_PORTABLE_CERTIFICATES and the contents are buffers + // with DER encoded cert and chain. + QUIC_BUFFER* certificateBuffer = (QUIC_BUFFER*)certificatePtr; + QUIC_BUFFER* chainBuffer = (QUIC_BUFFER*)chainPtr; - if (chainBuffer->Length > 0) - { - chainDataRented = ArrayPool.Shared.Rent((int)chainBuffer->Length); - chainData = chainDataRented.AsMemory(0, (int)chainBuffer->Length); - chainBuffer->Span.CopyTo(chainData.Span); + if (certificateBuffer->Length > 0) + { + certDataRented = ArrayPool.Shared.Rent((int)certificateBuffer->Length); + certData = certDataRented.AsMemory(0, (int)certificateBuffer->Length); + certificateBuffer->Span.CopyTo(certData.Span); + } + + if (chainBuffer->Length > 0) + { + chainDataRented = ArrayPool.Shared.Rent((int)chainBuffer->Length); + chainData = chainDataRented.AsMemory(0, (int)chainBuffer->Length); + chainBuffer->Span.CopyTo(chainData.Span); + } } } } - QuicConnection connection = _connection; + // We wan't to do the certificate validation asynchronously, but due to a bug in MsQuic, we need to call the callback synchronously on some versions if (MsQuicApi.SupportsAsyncCertValidation) { - // hand-off rest of the work to the thread pool, certificatePtr and chainPtr are invalid beyond this point - _ = Task.Run(() => - { - StartAsyncCertificateValidationCore(connection, certificate, certData, chainData, certDataRented, chainDataRented); - }); - } - else - { - // due to a bug in MsQuic, we need to call the callback synchronously to close the connection properly when - // we reject the certificate - StartAsyncCertificateValidationCore(connection, certificate, certData, chainData, certDataRented, chainDataRented); + // force yield to the thread pool to free up MsQuic worker thread. + await Task.CompletedTask.ConfigureAwait(ConfigureAwaitOptions.ForceYielding); } - static void StartAsyncCertificateValidationCore(QuicConnection thisConnection, X509Certificate2? certificate, Memory certData, Memory chainData, byte[]? certDataRented, byte[]? chainDataRented) + // certificatePtr and chainPtr are invalid beyond this point + + QUIC_TLS_ALERT_CODES result; + try { - QUIC_TLS_ALERT_CODES result; - try + if (certData.Length > 0) { - if (certData.Length > 0) - { - Debug.Assert(certificate == null); - certificate = new X509Certificate2(certData.Span); - } - - result = thisConnection._sslConnectionOptions.ValidateCertificate(certificate, certData.Span, chainData.Span); - thisConnection._remoteCertificate = certificate; + Debug.Assert(certificate == null); + certificate = new X509Certificate2(certData.Span); } - catch (Exception ex) + + result = _connection._sslConnectionOptions.ValidateCertificate(certificate, certData.Span, chainData.Span); + _connection._remoteCertificate = certificate; + } + catch (Exception ex) + { + certificate?.Dispose(); + _connection._connectedTcs.TrySetException(ex); + result = QUIC_TLS_ALERT_CODES.USER_CANCELED; + } + finally + { + if (certDataRented != null) { - certificate?.Dispose(); - thisConnection._connectedTcs.TrySetException(ex); - result = QUIC_TLS_ALERT_CODES.USER_CANCELED; + ArrayPool.Shared.Return(certDataRented); } - finally - { - if (certDataRented != null) - { - ArrayPool.Shared.Return(certDataRented); - } - if (chainDataRented != null) - { - ArrayPool.Shared.Return(chainDataRented); - } + if (chainDataRented != null) + { + ArrayPool.Shared.Return(chainDataRented); } + } - int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( - thisConnection._handle, - result == QUIC_TLS_ALERT_CODES.SUCCESS ? (byte)1 : (byte)0, - result); + int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( + _connection._handle, + result == QUIC_TLS_ALERT_CODES.SUCCESS ? (byte)1 : (byte)0, + result); - if (MsQuic.StatusFailed(status)) + if (MsQuic.StatusFailed(status)) + { + if (NetEventSource.Log.IsEnabled()) { - if (NetEventSource.Log.IsEnabled()) - { - NetEventSource.Error(thisConnection, $"{thisConnection} ConnectionCertificateValidationComplete failed with {ThrowHelper.GetErrorMessageForStatus(status)}"); - } + NetEventSource.Error(_connection, $"{_connection} ConnectionCertificateValidationComplete failed with {ThrowHelper.GetErrorMessageForStatus(status)}"); } } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index b839a5356021a..42909d40a0289 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -578,7 +578,7 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI // worker threads. // - _sslConnectionOptions.StartAsyncCertificateValidation(data.Certificate, data.Chain); + _sslConnectionOptions.StartAsyncCertificateValidation((IntPtr)data.Certificate, (IntPtr)data.Chain); return QUIC_STATUS_PENDING; } From 2579291daa0e6e210662f906b540d8ee7d708a20 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 27 Feb 2024 17:33:20 +0100 Subject: [PATCH 12/12] Fix functionality on older msquic versions --- .../QuicConnection.SslConnectionOptions.cs | 23 +++++++++++-------- .../src/System/Net/Quic/QuicConnection.cs | 7 +++++- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs index 8770ae157ec8a..1b352f1004540 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.SslConnectionOptions.cs @@ -66,7 +66,7 @@ public SslConnectionOptions(QuicConnection connection, bool isClient, _certificateChainPolicy = certificateChainPolicy; } - internal async void StartAsyncCertificateValidation(IntPtr certificatePtr, IntPtr chainPtr) + internal async Task StartAsyncCertificateValidation(IntPtr certificatePtr, IntPtr chainPtr) { // // The provided data pointers are valid only while still inside this function, so they need to be @@ -154,18 +154,23 @@ internal async void StartAsyncCertificateValidation(IntPtr certificatePtr, IntPt } } - int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( - _connection._handle, - result == QUIC_TLS_ALERT_CODES.SUCCESS ? (byte)1 : (byte)0, - result); - - if (MsQuic.StatusFailed(status)) + if (MsQuicApi.SupportsAsyncCertValidation) { - if (NetEventSource.Log.IsEnabled()) + int status = MsQuicApi.Api.ConnectionCertificateValidationComplete( + _connection._handle, + result == QUIC_TLS_ALERT_CODES.SUCCESS ? (byte)1 : (byte)0, + result); + + if (MsQuic.StatusFailed(status)) { - NetEventSource.Error(_connection, $"{_connection} ConnectionCertificateValidationComplete failed with {ThrowHelper.GetErrorMessageForStatus(status)}"); + if (NetEventSource.Log.IsEnabled()) + { + NetEventSource.Error(_connection, $"{_connection} ConnectionCertificateValidationComplete failed with {ThrowHelper.GetErrorMessageForStatus(status)}"); + } } } + + return result == QUIC_TLS_ALERT_CODES.SUCCESS; } private QUIC_TLS_ALERT_CODES ValidateCertificate(X509Certificate2? certificate, Span certData, Span chainData) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs index 42909d40a0289..5a4f626e2f546 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicConnection.cs @@ -578,7 +578,12 @@ private unsafe int HandleEventPeerCertificateReceived(ref PEER_CERTIFICATE_RECEI // worker threads. // - _sslConnectionOptions.StartAsyncCertificateValidation((IntPtr)data.Certificate, (IntPtr)data.Chain); + var task = _sslConnectionOptions.StartAsyncCertificateValidation((IntPtr)data.Certificate, (IntPtr)data.Chain); + if (task.IsCompletedSuccessfully) + { + return task.Result ? QUIC_STATUS_SUCCESS : QUIC_STATUS_BAD_CERTIFICATE; + } + return QUIC_STATUS_PENDING; }