From 4c5fea67090d60a197cf47aede133634c746fd7e Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 8 Jun 2022 16:31:28 +0200 Subject: [PATCH 1/4] Inline state transition helpers Fixes #55437 --- .../Implementations/MsQuic/MsQuicStream.cs | 96 ++++++++----------- 1 file changed, 40 insertions(+), 56 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs index 4ad758ef87bc27..2fa19d8a061a2a 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs @@ -292,52 +292,6 @@ private async ValueTask WriteAsync(Action stateSetup, T { ThrowIfDisposed(); - using CancellationTokenRegistration registration = SetupWriteStartState(isEmpty, cancellationToken); - - await WriteAsyncCore(stateSetup, buffer, isEmpty, endStream).ConfigureAwait(false); - - CleanupWriteCompletedState(); - } - - private unsafe ValueTask WriteAsyncCore(Action stateSetup, TBuffer buffer, bool isEmpty, bool endStream) - { - if (isEmpty) - { - if (endStream) - { - // Start graceful shutdown sequence if passed in the fin flag and there is an empty buffer. - StartShutdown(QUIC_STREAM_SHUTDOWN_FLAGS.GRACEFUL, errorCode: 0); - } - return default; - } - - stateSetup(_state, buffer); - - Debug.Assert(!Monitor.IsEntered(_state), "!Monitor.IsEntered(_state)"); - int status = MsQuicApi.Api.ApiTable->StreamSend( - _state.Handle.QuicHandle, - _state.SendBuffers.Buffers, - (uint)_state.SendBuffers.Count, - endStream ? QUIC_SEND_FLAGS.FIN : QUIC_SEND_FLAGS.NONE, - (void*)IntPtr.Zero); - - if (StatusFailed(status)) - { - CleanupWriteFailedState(); - CleanupSendState(_state); - - if (status == QUIC_STATUS_ABORTED) - { - throw ThrowHelper.GetConnectionAbortedException(_state.ConnectionState.AbortErrorCode); - } - ThrowIfFailure(status, "Could not send data to peer."); - } - - return _state.SendResettableCompletionSource.GetTypelessValueTask(); - } - - private CancellationTokenRegistration SetupWriteStartState(bool emptyBuffer, CancellationToken cancellationToken) - { if (cancellationToken.IsCancellationRequested) { lock (_state) @@ -369,7 +323,7 @@ private CancellationTokenRegistration SetupWriteStartState(bool emptyBuffer, Can } // if token was already cancelled, this would execute synchronously - CancellationTokenRegistration registration = cancellationToken.UnsafeRegister(static (s, token) => + using CancellationTokenRegistration registration = cancellationToken.UnsafeRegister(static (s, token) => { var state = (State)s!; bool shouldComplete = false; @@ -417,14 +371,11 @@ private CancellationTokenRegistration SetupWriteStartState(bool emptyBuffer, Can // Change the state in the same lock where we check for final states to prevent coming back from Aborted/ConnectionClosed. Debug.Assert(_state.SendState != SendState.Pending); - _state.SendState = emptyBuffer ? SendState.Finished : SendState.Pending; + _state.SendState = isEmpty ? SendState.Finished : SendState.Pending; } - return registration; - } + await WriteAsyncCore(stateSetup, buffer, isEmpty, endStream).ConfigureAwait(false); - private void CleanupWriteCompletedState() - { lock (_state) { if (_state.SendState == SendState.Finished) @@ -434,15 +385,48 @@ private void CleanupWriteCompletedState() } } - private void CleanupWriteFailedState() + private unsafe ValueTask WriteAsyncCore(Action stateSetup, TBuffer buffer, bool isEmpty, bool endStream) { - lock (_state) + if (isEmpty) { - if (_state.SendState == SendState.Pending) + if (endStream) { - _state.SendState = SendState.Finished; + // Start graceful shutdown sequence if passed in the fin flag and there is an empty buffer. + StartShutdown(QUIC_STREAM_SHUTDOWN_FLAGS.GRACEFUL, errorCode: 0); } + return default; } + + stateSetup(_state, buffer); + + Debug.Assert(!Monitor.IsEntered(_state), "!Monitor.IsEntered(_state)"); + int status = MsQuicApi.Api.ApiTable->StreamSend( + _state.Handle.QuicHandle, + _state.SendBuffers.Buffers, + (uint)_state.SendBuffers.Count, + endStream ? QUIC_SEND_FLAGS.FIN : QUIC_SEND_FLAGS.NONE, + (void*)IntPtr.Zero); + + if (StatusFailed(status)) + { + lock (_state) + { + if (_state.SendState == SendState.Pending) + { + _state.SendState = SendState.Finished; + } + } + + CleanupSendState(_state); + + if (status == QUIC_STATUS_ABORTED) + { + throw ThrowHelper.GetConnectionAbortedException(_state.ConnectionState.AbortErrorCode); + } + ThrowIfFailure(status, "Could not send data to peer."); + } + + return _state.SendResettableCompletionSource.GetTypelessValueTask(); } internal override async ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken = default) From 69dc051f5c8049aa5a70e0a34060e63872fb5388 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Wed, 8 Jun 2022 16:31:55 +0200 Subject: [PATCH 2/4] Add high level comments for HandleEventReceive and ReadAsync --- .../Net/Quic/Implementations/MsQuic/MsQuicStream.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs index 2fa19d8a061a2a..2b57a49f5bf4a0 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/MsQuicStream.cs @@ -431,6 +431,11 @@ private unsafe ValueTask WriteAsyncCore(Action stateSet internal override async ValueTask ReadAsync(Memory destination, CancellationToken cancellationToken = default) { + // + // If MsQuic indicated that some data were received (QUIC_STREAM_EVENT_RECEIVE), we use it to complete the request + // synchronously. Otherwise we setup the request to be completed by the HandleEventReceive handler. + // + ThrowIfDisposed(); if (_state.ReadState == ReadState.Closed) @@ -993,6 +998,13 @@ private static unsafe int NativeCallback(QUIC_HANDLE* stream, void* context, QUI private static unsafe int HandleEventReceive(State state, ref QUIC_STREAM_EVENT streamEvent) { + // + // Handle MsQuic QUIC_STREAM_EVENT_RECEIVE event + // + // If there is a pending ReadAsync call, then we complete it. Otherwise we keep a pointer to the received data + // and use it to complete the next ReadAsync operation synchronously. + // + ref var receiveEvent = ref streamEvent.RECEIVE; if (NetEventSource.Log.IsEnabled()) From 754f16901af30759cdbecd95abe2b15a355441f3 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Tue, 14 Jun 2022 12:02:50 +0200 Subject: [PATCH 3/4] [QUIC] Call `LocalCertificateSelectionCallback` to get client certificate --- .../Interop/SafeMsQuicConfigurationHandle.cs | 28 ++++++++++++++++--- .../tests/FunctionalTests/MsQuicTests.cs | 19 +++++++++---- 2 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs index 7fd84cc8ce3a8e..31b6fc3832a899 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs @@ -26,16 +26,36 @@ public static SafeMsQuicConfigurationHandle Create(QuicClientConnectionOptions o if (options.ClientAuthenticationOptions != null) { + SslClientAuthenticationOptions clientAuthenticationOptions = options.ClientAuthenticationOptions; + #pragma warning disable SYSLIB0040 // NoEncryption and AllowNoEncryption are obsolete - if (options.ClientAuthenticationOptions.EncryptionPolicy == EncryptionPolicy.NoEncryption) + if (clientAuthenticationOptions.EncryptionPolicy == EncryptionPolicy.NoEncryption) { - throw new PlatformNotSupportedException(SR.Format(SR.net_quic_ssl_option, nameof(options.ClientAuthenticationOptions.EncryptionPolicy))); + throw new PlatformNotSupportedException(SR.Format(SR.net_quic_ssl_option, nameof(clientAuthenticationOptions.EncryptionPolicy))); } #pragma warning restore SYSLIB0040 - if (options.ClientAuthenticationOptions.ClientCertificates != null) + if (clientAuthenticationOptions.LocalCertificateSelectionCallback != null) + { + X509Certificate? cert = clientAuthenticationOptions.LocalCertificateSelectionCallback( + options, + clientAuthenticationOptions.TargetHost ?? string.Empty, + clientAuthenticationOptions.ClientCertificates ?? new X509CertificateCollection(), + null, + Array.Empty()); + + try + { + if (((X509Certificate2)cert).HasPrivateKey) + { + certificate = cert; + } + } + catch { } + } + else if (clientAuthenticationOptions.ClientCertificates != null) { - foreach (var cert in options.ClientAuthenticationOptions.ClientCertificates) + foreach (var cert in clientAuthenticationOptions.ClientCertificates) { try { diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index 65e78735e1282a..0a273445bec9f3 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -190,7 +190,7 @@ public async Task CertificateCallbackThrowPropagates() } [Fact] - public async Task ConnectWithCertificateCallback() + public async Task ConnectWithServerCertificateCallback() { X509Certificate2 c1 = System.Net.Test.Common.Configuration.Certificates.GetServerCertificate(); X509Certificate2 c2 = System.Net.Test.Common.Configuration.Certificates.GetClientCertificate(); // This 'wrong' certificate but should be sufficient @@ -340,10 +340,12 @@ public async Task ConnectWithCertificateForLoopbackIP_IndicatesExpectedError(str } [ConditionalTheory] - [InlineData(true)] - [InlineData(false)] + [InlineData(true, true)] + [InlineData(false, true)] + [InlineData(true, false)] + [InlineData(false, false)] [ActiveIssue("https://github.com/dotnet/runtime/issues/64944", TestPlatforms.Windows)] - public async Task ConnectWithClientCertificate(bool sendCertificate) + public async Task ConnectWithClientCertificate(bool sendCertificate, bool useClientSelectionCallback) { if (PlatformDetection.IsWindows10Version20348OrLower) { @@ -371,7 +373,14 @@ public async Task ConnectWithClientCertificate(bool sendCertificate) using QuicListener listener = new QuicListener(QuicImplementationProviders.MsQuic, listenerOptions); QuicClientConnectionOptions clientOptions = CreateQuicClientOptions(); - if (sendCertificate) + if (useClientSelectionCallback) + { + clientOptions.ClientAuthenticationOptions.LocalCertificateSelectionCallback = delegate + { + return sendCertificate ? ClientCertificate : null; + }; + } + else if (sendCertificate) { clientOptions.ClientAuthenticationOptions.ClientCertificates = new X509CertificateCollection() { ClientCertificate }; } From db52738ef0b75b5d1a0ee35c0c3fadfd39cade10 Mon Sep 17 00:00:00 2001 From: Radek Zikmund Date: Thu, 16 Jun 2022 16:39:11 +0200 Subject: [PATCH 4/4] Code review feedback --- .../Interop/SafeMsQuicConfigurationHandle.cs | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs index 31b6fc3832a899..1ace58df055d6b 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/Implementations/MsQuic/Interop/SafeMsQuicConfigurationHandle.cs @@ -44,29 +44,22 @@ public static SafeMsQuicConfigurationHandle Create(QuicClientConnectionOptions o null, Array.Empty()); - try + if (cert is X509Certificate2 cert2 && cert2.Handle != IntPtr.Zero && cert2.HasPrivateKey) { - if (((X509Certificate2)cert).HasPrivateKey) - { - certificate = cert; - } + certificate = cert; } - catch { } } else if (clientAuthenticationOptions.ClientCertificates != null) { - foreach (var cert in clientAuthenticationOptions.ClientCertificates) + foreach (X509Certificate cert in clientAuthenticationOptions.ClientCertificates) { - try + + if (cert is X509Certificate2 cert2 && cert2.Handle != IntPtr.Zero && cert2.HasPrivateKey) { - if (((X509Certificate2)cert).HasPrivateKey) - { - // Pick first certificate with private key. - certificate = cert; - break; - } + // Pick first certificate with private key. + certificate = cert; + break; } - catch { } } } }