QuicListener accepts an invalid connection after it was rejected due to client cert issues

[Tratcher]

Scenario:

1. The listener sets ClientCertificateRequired and does not override RemoteCertificateValidationCallback
2. The client connects using a client certificate that's invalid (self-signed)
3. The listener accepts the connection even though validation failed
4. Calling OpenUnidirectionalStream from the server fails with INVALID_STATE

Workaround: override RemoteCertificateValidationCallback to accept the certificate.

Expected: AcceptConnectionAsync should not return connections that have already failed due to client cert validation errors.

  Message: 
    System.Net.Quic.QuicException : Failed to open stream to peer. Error Code: INVALID_STATE

  Stack Trace: 
    QuicExceptionHelpers.ThrowIfFailed(UInt32 status, String message, Exception innerException)
    MsQuicStream.ctor(State connectionState, QUIC_STREAM_OPEN_FLAGS flags)
    MsQuicConnection.OpenUnidirectionalStream()
    QuicConnection.OpenUnidirectionalStream()
    QuicConnectionContextTests.ClientCertificate_ControlStream() line 775
    --- End of stack trace from previous location ---

  Standard Output: 
    | [0.007s] TestLifetime Information: Starting test ClientCertificate_ControlStream at 2021-08-11T23:36:26
    | [0.224s] Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.Tests.QuicConnectionContextTests Error: Test threw an exception.
    | System.Net.Quic.QuicException: Failed to open stream to peer. Error Code: INVALID_STATE
    |    at System.Net.Quic.Implementations.MsQuic.Internal.QuicExceptionHelpers.ThrowIfFailed(UInt32 status, String message, Exception innerException)
    |    at System.Net.Quic.Implementations.MsQuic.MsQuicStream..ctor(State connectionState, QUIC_STREAM_OPEN_FLAGS flags)
    |    at System.Net.Quic.Implementations.MsQuic.MsQuicConnection.OpenUnidirectionalStream()
    |    at System.Net.Quic.QuicConnection.OpenUnidirectionalStream()
    |    at Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.Tests.QuicConnectionContextTests.ClientCertificate_ControlStream() in D:\github\aspnetcore\src\Servers\Kestrel\Transport.Quic\test\QuicConnectionContextTests.cs:line 775
    |    at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 264
    | --- End of stack trace from previous location ---
    |    at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:line 48
    |    at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in C:\Dev\xunit\xunit\src\xunit.core\Sdk\ExceptionAggregator.cs:line 90
    | [0.245s] TestLifetime Information: Finished test ClientCertificate_ControlStream in 0.2425817s 

            var listenerOptions = new QuicListenerOptions()
            {
                MaxBidirectionalStreams = 100,
                MaxUnidirectionalStreams = 100,
                ServerAuthenticationOptions = new SslServerAuthenticationOptions()
                {
                    ServerCertificate = TestResources.GetTestCertificate(),
                    ApplicationProtocols = new List<SslApplicationProtocol>() { new SslApplicationProtocol("h3") },
                    ClientCertificateRequired = true,
                    // RemoteCertificateValidationCallback = RemoteCertificateValidationCallback,
                },
                ListenEndPoint = new IPEndPoint(IPAddress.Loopback, 0),
            };
            var listener = new QuicListener(listenerOptions);
            var acceptConnection = listener.AcceptConnectionAsync();

            var clientOptions = new QuicClientConnectionOptions
            {
                MaxBidirectionalStreams = 200,
                MaxUnidirectionalStreams = 200,
                RemoteEndPoint = listener.ListenEndPoint,
                ClientAuthenticationOptions = new SslClientAuthenticationOptions
                {
                    ApplicationProtocols = new List<SslApplicationProtocol>
                    {
                        new SslApplicationProtocol("h3")
                    },
                    RemoteCertificateValidationCallback = RemoteCertificateValidationCallback
                }
            };
            var testCert = TestResources.GetTestCertificate();
            clientOptions.ClientAuthenticationOptions.ClientCertificates = new X509CertificateCollection { testCert };

            using var clientConnection = new QuicConnection(clientOptions);
            await clientConnection.ConnectAsync().DefaultTimeout();

            var serverConnection = await acceptConnection;

            var clientStreamAccept = clientConnection.AcceptStreamAsync();

            var serverStream = serverConnection.OpenUnidirectionalStream();

            var clientStream = await clientStreamAccept;

            static bool RemoteCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
            {
                return true;
            }

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Scenario:

1. The listener sets ClientCertificateRequired and does not override RemoteCertificateValidationCallback
2. The client connects using a client certificate that's invalid (self-signed)
3. The listener accepts the connection even though validation failed
4. Calling OpenUnidirectionalStream from the server fails with INVALID_STATE

Workaround: override RemoteCertificateValidationCallback to accept the certificate.

Expected: AcceptConnectionAsync should not return connections that have already failed due to client cert validation errors.

  Message: 
    System.Net.Quic.QuicException : Failed to open stream to peer. Error Code: INVALID_STATE

  Stack Trace: 
    QuicExceptionHelpers.ThrowIfFailed(UInt32 status, String message, Exception innerException)
    MsQuicStream.ctor(State connectionState, QUIC_STREAM_OPEN_FLAGS flags)
    MsQuicConnection.OpenUnidirectionalStream()
    QuicConnection.OpenUnidirectionalStream()
    QuicConnectionContextTests.ClientCertificate_ControlStream() line 775
    --- End of stack trace from previous location ---

  Standard Output: 
    | [0.007s] TestLifetime Information: Starting test ClientCertificate_ControlStream at 2021-08-11T23:36:26
    | [0.224s] Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.Tests.QuicConnectionContextTests Error: Test threw an exception.
    | System.Net.Quic.QuicException: Failed to open stream to peer. Error Code: INVALID_STATE
    |    at System.Net.Quic.Implementations.MsQuic.Internal.QuicExceptionHelpers.ThrowIfFailed(UInt32 status, String message, Exception innerException)
    |    at System.Net.Quic.Implementations.MsQuic.MsQuicStream..ctor(State connectionState, QUIC_STREAM_OPEN_FLAGS flags)
    |    at System.Net.Quic.Implementations.MsQuic.MsQuicConnection.OpenUnidirectionalStream()
    |    at System.Net.Quic.QuicConnection.OpenUnidirectionalStream()
    |    at Microsoft.AspNetCore.Server.Kestrel.Transport.Quic.Tests.QuicConnectionContextTests.ClientCertificate_ControlStream() in D:\github\aspnetcore\src\Servers\Kestrel\Transport.Quic\test\QuicConnectionContextTests.cs:line 775
    |    at Xunit.Sdk.TestInvoker`1.<>c__DisplayClass48_1.<<InvokeTestMethodAsync>b__1>d.MoveNext() in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs:line 264
    | --- End of stack trace from previous location ---
    |    at Xunit.Sdk.ExecutionTimer.AggregateAsync(Func`1 asyncAction) in C:\Dev\xunit\xunit\src\xunit.execution\Sdk\Frameworks\ExecutionTimer.cs:line 48
    |    at Xunit.Sdk.ExceptionAggregator.RunAsync(Func`1 code) in C:\Dev\xunit\xunit\src\xunit.core\Sdk\ExceptionAggregator.cs:line 90
    | [0.245s] TestLifetime Information: Finished test ClientCertificate_ControlStream in 0.2425817s 

            var listenerOptions = new QuicListenerOptions()
            {
                MaxBidirectionalStreams = 100,
                MaxUnidirectionalStreams = 100,
                ServerAuthenticationOptions = new SslServerAuthenticationOptions()
                {
                    ServerCertificate = TestResources.GetTestCertificate(),
                    ApplicationProtocols = new List<SslApplicationProtocol>() { new SslApplicationProtocol("h3") },
                    ClientCertificateRequired = true,
                    // RemoteCertificateValidationCallback = RemoteCertificateValidationCallback,
                },
                ListenEndPoint = new IPEndPoint(IPAddress.Loopback, 0),
            };
            var listener = new QuicListener(listenerOptions);
            var acceptConnection = listener.AcceptConnectionAsync();

            var clientOptions = new QuicClientConnectionOptions
            {
                MaxBidirectionalStreams = 200,
                MaxUnidirectionalStreams = 200,
                RemoteEndPoint = listener.ListenEndPoint,
                ClientAuthenticationOptions = new SslClientAuthenticationOptions
                {
                    ApplicationProtocols = new List<SslApplicationProtocol>
                    {
                        new SslApplicationProtocol("h3")
                    },
                    RemoteCertificateValidationCallback = RemoteCertificateValidationCallback
                }
            };
            var testCert = TestResources.GetTestCertificate();
            clientOptions.ClientAuthenticationOptions.ClientCertificates = new X509CertificateCollection { testCert };

            using var clientConnection = new QuicConnection(clientOptions);
            await clientConnection.ConnectAsync().DefaultTimeout();

            var serverConnection = await acceptConnection;

            var clientStreamAccept = clientConnection.AcceptStreamAsync();

            var serverStream = serverConnection.OpenUnidirectionalStream();

            var clientStream = await clientStreamAccept;

            static bool RemoteCertificateValidationCallback(object sender, X509Certificate certificate, X509Chain chain, SslPolicyErrors sslPolicyErrors)
            {
                return true;
            }

Author:	Tratcher
Assignees:	-
Labels:	area-System.Net.Quic
Milestone:	-

[geoffkizer]

@wfurt can you take a look?

[wfurt]

I think there is conceptual issue with the accept logic @geoffkizer.
First, we get NEW_CONNECTION event and we put it to AcceptQueue.
That wakes the AcceptConnectionAsync() and we hand out the connection before it is actually connected.
Then the validation fails and for Debug builds we hit the assert you added.
For Release build we will return error code to MsQuic and that will clean it up.
However, the caller has no clue since that happens on msquic's thread.

There are several ways how to fix it:

1. add PreAccept queue and hold to the connection internally until we get CONNECTED event.
2. document that the AcceptConnectionAsync can hand out incomplete connection. This could possibly give caller some visibility about failures.
3. .... ???

In either case, I will fix the validation handling. For 'ConnectAsync' we hand out the Exception to caller but for the current QuicListener there is nowhere to throw. (unless we want to throw to the AcceptConnectionAsync() and expect to be called again.