feat: rework TlsClientHelloBytesCallback as a connection middleware#65808
feat: rework TlsClientHelloBytesCallback as a connection middleware#65808DeagleGross wants to merge 5 commits intodotnet:mainfrom
TlsClientHelloBytesCallback as a connection middleware#65808Conversation
648e73d to
76e7405
Compare
TlsClientHelloBytesCallback as a connection middlewareTlsClientHelloBytesCallback as a connection middleware
There was a problem hiding this comment.
Pull request overview
This PR obsoletes HttpsConnectionAdapterOptions.TlsClientHelloBytesCallback and introduces a new ListenOptions connection middleware API (UseTlsClientHelloListener) to observe raw TLS ClientHello bytes in a way that also works with TlsHandshakeCallbackOptions scenarios.
Changes:
- Add
ListenOptionsHttpsExtensions.UseTlsClientHelloListener(...)as a connection middleware that sniffs ClientHello bytes beforeUseHttps(). - Mark
HttpsConnectionAdapterOptions.TlsClientHelloBytesCallbackas obsolete and keep back-compat support insideHttpsConnectionMiddleware. - Add functional tests and a sample usage demonstrating the new middleware API.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Servers/Kestrel/test/InMemory.FunctionalTests/TlsListenerTests.cs | Adds tests for new UseTlsClientHelloListener behavior and preserves back-compat tests for the obsolete option. |
| src/Servers/Kestrel/samples/SampleApp/Startup.cs | Demonstrates calling UseTlsClientHelloListener before UseHttps(). |
| src/Servers/Kestrel/Core/src/PublicAPI.Unshipped.txt | Declares the new public API surface for UseTlsClientHelloListener. |
| src/Servers/Kestrel/Core/src/Middleware/HttpsConnectionMiddleware.cs | Continues to support the obsolete TlsClientHelloBytesCallback path (pragma-suppressed). |
| src/Servers/Kestrel/Core/src/ListenOptionsHttpsExtensions.cs | Introduces the new connection middleware extension and its default timeout. |
| src/Servers/Kestrel/Core/src/HttpsConnectionAdapterOptions.cs | Obsoletes TlsClientHelloBytesCallback and updates docs to point to the new API. |
| using var timeoutCts = new CancellationTokenSource(effectiveTimeout); | ||
| using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(timeoutCts.Token, context.ConnectionClosed); | ||
|
|
||
| await tlsListener.OnTlsClientHelloAsync(context, linkedCts.Token); |
There was a problem hiding this comment.
The timeout/linked CancellationTokenSource instances are disposed only after next(context) completes, which can be the entire lifetime of the connection. That keeps the timer/CTS alive long after the ClientHello sniffing is finished (and can still fire later), adding unnecessary per-connection overhead. Consider scoping the CTS to only the OnTlsClientHelloAsync call (dispose before invoking next).
| using var timeoutCts = new CancellationTokenSource(effectiveTimeout); | |
| using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(timeoutCts.Token, context.ConnectionClosed); | |
| await tlsListener.OnTlsClientHelloAsync(context, linkedCts.Token); | |
| using (var timeoutCts = new CancellationTokenSource(effectiveTimeout)) | |
| using (var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(timeoutCts.Token, context.ConnectionClosed)) | |
| { | |
| await tlsListener.OnTlsClientHelloAsync(context, linkedCts.Token); | |
| } |
| using var timeoutCts = new CancellationTokenSource(effectiveTimeout); | ||
| using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(timeoutCts.Token, context.ConnectionClosed); | ||
|
|
||
| await tlsListener.OnTlsClientHelloAsync(context, linkedCts.Token); |
There was a problem hiding this comment.
OnTlsClientHelloAsync can throw OperationCanceledException when the timeout elapses. As written, that exception will escape the connection delegate and be logged as an "Unhandled exception while processing..." error by Kestrel's connection dispatcher. Consider catching OperationCanceledException in this middleware and ending the connection gracefully (without treating it as an unhandled error), similar to how HttpsConnectionMiddleware handles handshake timeouts.
| await tlsListener.OnTlsClientHelloAsync(context, linkedCts.Token); | |
| try | |
| { | |
| await tlsListener.OnTlsClientHelloAsync(context, linkedCts.Token); | |
| } | |
| catch (OperationCanceledException) when (timeoutCts.IsCancellationRequested) | |
| { | |
| return; | |
| } |
| public static ListenOptions UseTlsClientHelloListener(this ListenOptions listenOptions, Action<ConnectionContext, ReadOnlySequence<byte>> tlsClientHelloBytesCallback, TimeSpan? timeout = null) | ||
| { | ||
| ArgumentNullException.ThrowIfNull(tlsClientHelloBytesCallback); | ||
|
|
||
| var effectiveTimeout = timeout ?? DefaultTlsClientHelloListenerTimeout; | ||
| var tlsListener = new TlsListener(tlsClientHelloBytesCallback); | ||
|
|
There was a problem hiding this comment.
timeout isn't validated before being passed into new CancellationTokenSource(effectiveTimeout). Negative/zero values (other than Timeout.InfiniteTimeSpan) will throw from CancellationTokenSource with a less actionable exception. Consider validating timeout up front and throwing ArgumentOutOfRangeException with a consistent message (similar to HttpsConnectionAdapterOptions.HandshakeTimeout).
| /// </summary> | ||
| /// <remarks> | ||
| /// If a client hello spans multiple record fragments then this callback only gets the first fragment. | ||
| /// Use <see cref="Hosting.ListenOptionsHttpsExtensions.UseTlsClientHelloListener"/> instead. |
There was a problem hiding this comment.
The <see cref="Hosting.ListenOptionsHttpsExtensions.UseTlsClientHelloListener"/> cref likely won't resolve (there isn't a Hosting namespace in this context). Use the fully-qualified Microsoft.AspNetCore.Hosting.ListenOptionsHttpsExtensions.UseTlsClientHelloListener (or another resolvable cref) to avoid doc generation warnings/broken links.
| /// Use <see cref="Hosting.ListenOptionsHttpsExtensions.UseTlsClientHelloListener"/> instead. | |
| /// Use <see cref="Microsoft.AspNetCore.Hosting.ListenOptionsHttpsExtensions.UseTlsClientHelloListener"/> instead. |
Obsoleted
TlsClientHelloBytesCallbackonHttpsConnectionAdapterOptions, and reworked that as a connection middlewareFixes #64860