From 51921cbd2dcf0e706ae6087b1144dcd2b06403de Mon Sep 17 00:00:00 2001 From: ManickaP Date: Mon, 18 Jul 2022 12:00:46 +0200 Subject: [PATCH 1/4] Propagate exception from handshake to the caller of AcceptConnectionAsync --- .../Quic/QuicListener.PendingConnection.cs | 19 +++++++++++++------ .../src/System/Net/Quic/QuicListener.cs | 14 +++++--------- .../src/System/Net/Quic/QuicStream.cs | 2 +- .../tests/FunctionalTests/MsQuicTests.cs | 11 ++++++----- 4 files changed, 25 insertions(+), 21 deletions(-) diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs index 3b139ccc61663..e69527eea783d 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicListener.PendingConnection.cs @@ -28,7 +28,7 @@ private sealed class PendingConnection : IAsyncDisposable /// /// It will contain the established in case of a successful handshake; otherwise, null. /// - private readonly TaskCompletionSource _finishHandshakeTask; + private readonly TaskCompletionSource _finishHandshakeTask; /// /// Use to impose the handshake timeout. /// @@ -36,7 +36,7 @@ private sealed class PendingConnection : IAsyncDisposable public PendingConnection() { - _finishHandshakeTask = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + _finishHandshakeTask = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); _cancellationTokenSource = new CancellationTokenSource(); } @@ -74,7 +74,7 @@ public async void StartHandshake(QuicConnection connection, SslClientHelloInfo c await connection.CloseAsync(default).ConfigureAwait(false); await connection.DisposeAsync().ConfigureAwait(false); - _finishHandshakeTask.SetResult(null); + _finishHandshakeTask.SetException(ex); } } @@ -83,7 +83,7 @@ public async void StartHandshake(QuicConnection connection, SslClientHelloInfo c /// /// A cancellation token that can be used to cancel the asynchronous operation. /// An asynchronous task that completes with the established connection if it succeeded or null if it failed. - public ValueTask FinishHandshakeAsync(CancellationToken cancellationToken = default) + public ValueTask FinishHandshakeAsync(CancellationToken cancellationToken = default) => new(_finishHandshakeTask.Task.WaitAsync(cancellationToken)); @@ -91,10 +91,17 @@ public async void StartHandshake(QuicConnection connection, SslClientHelloInfo c /// Cancels the handshake in progress and awaits for it so that the connection can be safely cleaned from the listener queue. /// /// A task that represents the asynchronous dispose operation. - public ValueTask DisposeAsync() + public async ValueTask DisposeAsync() { _cancellationTokenSource.Cancel(); - return new ValueTask(_finishHandshakeTask.Task); + try + { + await _finishHandshakeTask.Task.ConfigureAwait(false); + } + catch + { + // Just swallow the exception, we don't want it to propagate outside of dispose and it has already been logged in StartHandshake catch block. + } } } } 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 f1b9ee1e40c29..e22f9fb1e98c0 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 @@ -66,7 +66,7 @@ public static ValueTask ListenAsync(QuicListenerOptions options, C /// /// Handle to MsQuic listener object. /// - private MsQuicContextSafeHandle _handle; + private readonly MsQuicContextSafeHandle _handle; /// /// Set to non-zero once disposed. Prevents double and/or concurrent disposal. @@ -152,6 +152,9 @@ private unsafe QuicListener(QuicListenerOptions options) /// /// Note that doesn't have a mechanism to report inbound connections that fail the handshake process. /// Such connections are only logged by the listener and never surfaced on the outside. + /// + /// Propagates exceptions from , including validation errors from misconfigured , e.g. . + /// Also propagates exceptions from failed connection handshake, e.g. , . /// /// A task that will contain a fully connected which successfully finished the handshake and is ready to be used. public async ValueTask AcceptConnectionAsync(CancellationToken cancellationToken = default) @@ -165,14 +168,7 @@ public async ValueTask AcceptConnectionAsync(CancellationToken c PendingConnection pendingConnection = await _acceptQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false); await using (pendingConnection.ConfigureAwait(false)) { - QuicConnection? connection = await pendingConnection.FinishHandshakeAsync(cancellationToken).ConfigureAwait(false); - // Handshake failed, discard this connection and try to get another from the queue. - if (connection is null) - { - continue; - } - - return connection; + return await pendingConnection.FinishHandshakeAsync(cancellationToken).ConfigureAwait(false); } } } diff --git a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs index a974a54257ebb..f27dafb0360cf 100644 --- a/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs +++ b/src/libraries/System.Net.Quic/src/System/Net/Quic/QuicStream.cs @@ -27,7 +27,7 @@ public sealed partial class QuicStream /// /// Handle to MsQuic connection object. /// - private MsQuicContextSafeHandle _handle; + private readonly MsQuicContextSafeHandle _handle; /// /// Set to non-zero once disposed. Prevents double and/or concurrent disposal. diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index 51dbaf0202df2..dd756dd076c47 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -132,12 +132,11 @@ public async Task UntrustedClientCertificateFails() clientOptions.ClientAuthenticationOptions.ClientCertificates = new X509CertificateCollection() { ClientCertificate }; Task clientTask = CreateQuicConnection(clientOptions).AsTask(); - using CancellationTokenSource cts = new CancellationTokenSource(); - cts.CancelAfter(500); //Some delay to see if we would get failed connection. - Task serverTask = listener.AcceptConnectionAsync(cts.Token).AsTask(); + // This will propagate the AuthenticationException since the client certificate is not trusted. + Task serverTask = listener.AcceptConnectionAsync().AsTask(); Assert.True(clientTask.Wait(PassingTestTimeout)); - await Assert.ThrowsAsync(() => serverTask); + await Assert.ThrowsAsync(() => serverTask); // The task will likely succeed but we don't really care. // It may fail if the server aborts quickly. try @@ -179,6 +178,7 @@ public async Task CertificateCallbackThrowPropagates() clientOptions.ClientAuthenticationOptions.TargetHost = "foobar1"; await Assert.ThrowsAsync(() => CreateQuicConnection(clientOptions).AsTask()); + await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); // Make sure the listener is still usable and there is no lingering bad connection validationResult = true; @@ -245,8 +245,9 @@ public async Task ConnectWithServerCertificateCallback() clientOptions.ClientAuthenticationOptions.TargetHost = "foobar3"; Task clientTask = CreateQuicConnection(clientOptions).AsTask(); - await Assert.ThrowsAnyAsync(() => clientTask); + await Assert.ThrowsAsync(() => clientTask); Assert.Equal(clientOptions.ClientAuthenticationOptions.TargetHost, receivedHostName); + await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); // Do this last to make sure Listener is still functional. clientOptions.ClientAuthenticationOptions.TargetHost = "foobar2"; From 2c7e5af2b14c3d4fbbd85f8dd44a2364578d5161 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Mon, 18 Jul 2022 20:02:16 +0200 Subject: [PATCH 2/4] ThrowsAnyAsync -> ThrowsAsync for QuicException since there's no hierarchy anymore --- .../tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs | 2 +- .../System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs | 2 +- .../tests/FunctionalTests/QuicConnectionTests.cs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs index a16681154fd9d..df827e64feada 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicCipherSuitesPolicyTests.cs @@ -71,7 +71,7 @@ public void NoSupportedCiphers_ThrowsArgumentException(TlsCipherSuite[] ciphers) [Fact] public async Task MismatchedCipherPolicies_ConnectAsync_ThrowsQuicException() { - await Assert.ThrowsAnyAsync(() => TestConnection( + await Assert.ThrowsAsync(() => TestConnection( new CipherSuitesPolicy(new[] { TlsCipherSuite.TLS_AES_128_GCM_SHA256 }), new CipherSuitesPolicy(new[] { TlsCipherSuite.TLS_AES_256_GCM_SHA384 }) )); diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs index dd756dd076c47..ea127c52c7a06 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/MsQuicTests.cs @@ -600,7 +600,7 @@ ValueTask OpenStreamAsync(QuicConnection connection, CancellationTok // (CloseAsync may be processed before OpenStreamAsync as it is scheduled to the front of the operation queue) // To be revisited once we standartize on exceptions. // [ActiveIssue("https://github.com/dotnet/runtime/issues/55619")] - await Assert.ThrowsAnyAsync(() => waitTask.AsTask().WaitAsync(TimeSpan.FromSeconds(3))); + await Assert.ThrowsAsync(() => waitTask.AsTask().WaitAsync(TimeSpan.FromSeconds(3))); } else { diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs index d27aae22484ac..cd81d38f60900 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicConnectionTests.cs @@ -73,12 +73,12 @@ await RunClientServer( // (CloseAsync may be processed before OpenStreamAsync as it is scheduled to the front of the operation queue) // To be revisited once we standartize on exceptions. // [ActiveIssue("https://github.com/dotnet/runtime/issues/55619")] - await Assert.ThrowsAnyAsync(() => connectTask); + await Assert.ThrowsAsync(() => connectTask); // Subsequent attempts should fail // TODO: Which exception is correct? await AssertThrowsQuicExceptionAsync(QuicError.OperationAborted, async () => await serverConnection.AcceptInboundStreamAsync()); - await Assert.ThrowsAnyAsync(() => OpenAndUseStreamAsync(serverConnection)); + await Assert.ThrowsAsync(() => OpenAndUseStreamAsync(serverConnection)); }); } From 66d05048976f846c71afb5c458f6a9099a1f8a51 Mon Sep 17 00:00:00 2001 From: ManickaP Date: Mon, 18 Jul 2022 20:54:43 +0200 Subject: [PATCH 3/4] Added tests --- .../FunctionalTests/QuicListenerTests.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs index 45cf135c503bf..6fddd9627de93 100644 --- a/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs +++ b/src/libraries/System.Net.Quic/tests/FunctionalTests/QuicListenerTests.cs @@ -55,5 +55,34 @@ await Task.Run(async () => await using QuicConnection clientConnection = await clientStreamTask; }).WaitAsync(TimeSpan.FromSeconds(6)); } + + [Fact] + public async Task AcceptConnectionAsync_InvalidConnectionOptions_Throws() + { + QuicListenerOptions listenerOptions = CreateQuicListenerOptions(); + // Do not set any options, which should throw an argument exception from accept. + listenerOptions.ConnectionOptionsCallback = (_, _, _) => ValueTask.FromResult(new QuicServerConnectionOptions()); + await using QuicListener listener = await CreateQuicListener(listenerOptions); + + ValueTask connectTask = CreateQuicConnection(listener.LocalEndPoint); + await Assert.ThrowsAnyAsync(async () => await listener.AcceptConnectionAsync()); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task AcceptConnectionAsync_ThrowingOptionsCallback_Throws(bool useFromException) + { + const string expectedMessage = "Expected Message"; + + QuicListenerOptions listenerOptions = CreateQuicListenerOptions(); + // Throw an exception, which should throw the same from accept. + listenerOptions.ConnectionOptionsCallback = (_, _, _) => useFromException ? ValueTask.FromException(new Exception(expectedMessage)) : throw new Exception(expectedMessage); + await using QuicListener listener = await CreateQuicListener(listenerOptions); + + ValueTask connectTask = CreateQuicConnection(listener.LocalEndPoint); + Exception exception = await Assert.ThrowsAsync(async () => await listener.AcceptConnectionAsync()); + Assert.Equal(expectedMessage, exception.Message); + } } } From ad7e972e4c6b95365c6dbce699ae96b954e3503c Mon Sep 17 00:00:00 2001 From: ManickaP Date: Tue, 19 Jul 2022 09:58:30 +0200 Subject: [PATCH 4/4] Feedback --- .../System.Net.Quic/src/System/Net/Quic/QuicListener.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 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 e22f9fb1e98c0..059ef13f197e8 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 @@ -163,13 +163,10 @@ public async ValueTask AcceptConnectionAsync(CancellationToken c try { - while (true) + PendingConnection pendingConnection = await _acceptQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false); + await using (pendingConnection.ConfigureAwait(false)) { - PendingConnection pendingConnection = await _acceptQueue.Reader.ReadAsync(cancellationToken).ConfigureAwait(false); - await using (pendingConnection.ConfigureAwait(false)) - { - return await pendingConnection.FinishHandshakeAsync(cancellationToken).ConfigureAwait(false); - } + return await pendingConnection.FinishHandshakeAsync(cancellationToken).ConfigureAwait(false); } } catch (ChannelClosedException ex) when (ex.InnerException is not null)