[JamesNK]

Fixes #43723

Description

If an HTTP/3 connection request from a client fails because of an auth issue, e.g. client rejects an invalid server certificate, then the connection listener accept loop stops. This prevents future connections.

Customer Impact

Without this fix, a client can create a request that causes to Kestrel to stop accepting HTTP/3 connections.

Regression?

• Yes
• No

Maybe. There might have been changes in System.Net.Quic to throw this error. The XML docs on QuicListener.AcceptConnectionAsync have mixed signals about whether this exception should be thrown.

@dotnet/ncl could you confirm that this is expected behavior?

Risk

• High
• Medium
• Low

This fixes this particular error, but I worry about if there are other exceptions that are thrown out of listener accept that would stop the connection accept loop. I'd like to get some advice on @dotnet/ncl on how best to use QuicListener.AcceptConnectionAsync.

We should keep listening if AuthenticationException is thrown. Is there a list of QuicError values for which we should continue listening? Or the inverse, what QuicError values should stop the accept loop?

I think we have certainty from the NCL team on how best to use the QUIC connection listener.

Verification

• Manual (required)
• Automated

I'm unsure if it is possible to test this error in a browser manually. A browser HTTP/3 connection only works when the server offers a valid TLS cert; this failure only happens when the TLS cert is invalid.

Packaging changes reviewed?

• Yes
• No
• N/A

@Pilchie

[rzikm]

Also propagates exceptions from failed connection handshake, e.g. AuthenticationException, QuicException

I think the behavior is consistent with the documentation.

I realize it is not very user-friendly in the case of AuthenticationException, because the error is not caused by misconfiguring the QuicConnectionOptions, but swallowing this exception would make it hard for users to debug why they are not receiving any connections, Thoughts? @ManickaP?

[rzikm]

I'd like to get some advice on https://github.com/orgs/dotnet/teams/ncl on how best to use QuicListener.AcceptConnectionAsync.

We should keep listening if AuthenticationException is thrown. Is there a list of QuicError values for which we should continue listening? Or the inverse, what QuicError values should stop the accept loop?

I believe only the QuicError.OperationAborted should stop the accept loop (because the listener was stopped), The rest of the errors/exceptions would be related to a failure of a specific connection which we did not accept.

[ManickaP]

The changed behavior was reaction to dotnet/runtime#72160. Yes, the original issue was about misconfigured listener, but the decision was to expose all errors, not filter for specific ones and swallow the rest. Listener users can do that themselves, at least they have the option now.

I believe only the QuicError.OperationAborted should stop the accept loop

Yes, I believe this as well.

[JamesNK]

While I think that AcceptConnectionAsync rethrowing the exception from the connection callback is well-intentioned, right now it means any exception thrown in ConnectionOptionsCallback can be rethrown by AcceptConnectionAsync. That means AcceptConnectionAsync can throw any exception.

For example, you could even create a callback with throw new QuicException(QuicError.OperationAborted, ...) and make someone think the listener has aborted (I don't know why someone would want to do that... just a suggestion of a problem of the current pattern).

A way to avoid this could be to wrap the exception thrown in ConnectionOptionsCallback before rethrowing it, e.g. wrap it with QuicException and have a new QuicError of ConnectionCallbackError. Or perhaps all errors related to a connection attempt failing should be nested under a QUIC exception with a type of ListenerConnectionError.

[Pilchie]

Approved for .NET 7 RC2.

[JamesNK]

@dotnet/aspnet-build Please merge.

[wtgodbe]

@JamesNK Are the test failures expected?

[JamesNK]

Oh. Tests. I'll take a look.

[halter73]

An error thrown inside ConnectionOptionsCallback can also throw from AcceptConnectionAsync.

Do exceptions originating from the TlsHandshakeCallbackOptions.OnConnection
get logged at a debug level when we're using the HttpsConnectionMiddleware?

Won't most exception types bubble out through AuthenticateAsServerAsync and get logged as an error by KestrelConnectionOfT? Ideally errors thrown by user code like the OnConnection callback should be logged at a higher level.

This is a fairly low-level API though, and it's best to get the fix in quickly, so I'll approve it either way.

[JamesNK]

@JamesNK Are the test failures expected?

I improved a flaky test. Ready to merge? 🙏