API Suggestion: Add HttpSys feature interface for fetching TLS ClientHello

[markalward]

Background and Motivation

API Proposal: Expose TLS client hello message adds a feature to expose the TLS ClientHello message to users on HttpSys and Kestrel. The HttpSys API in that proposal is a callback that is invoked once for every incoming TLS connection. It would be useful to also expose a "get" API, accessible through the HttpContext, that allows users to query the ClientHello on-demand. This API would have the following benefits over the current callback approach:

Users have better control over when/if the ClientHello should be fetched.

1. The user can decide whether to query for the ClientHello based on runtime settings such as feature flags.
2. The underlying Windows API only allows querying for the ClientHello on SSL configuration records (per-hostname or per-IP TLS settings configured using HttpSetServiceConfiguration) that have the HTTP_SERVICE_CONFIG_SSL_FLAG_ENABLE_CACHE_CLIENT_HELLO flag set. The end-user knows how they have configured these settings, and can choose to only fetch the ClientHello for requests to hostnames/IPs that have been configured with this flag.

It is easier for the user to correlate the ClientHello with the requests flowing over the connection.

The current callback approach leads users down a difficult path:

1. There is no HttpSys equivalent to the ConnectionContext that is available to Kestrel connection middleware. The user is forced to build an adhoc connection cache to store per-connection state.
2. ASP.NET Core HttpSys doesn't offer a reliable way to track connection lifetime. While HttpRequest.RequestAborted can be used to detect connection closure on HTTP/1.1, it does not work on HTTP/2 - the cancellation token fires whenever the current HTTP/2 stream is completed, not when the TCP connection closes. This means the connection cache needs to use a heuristic approach to tracking connections, and is likely to contain far more connections than are actually active on the web server.

Users can choose a CPU/memory tradeoff when using the API.

The API can be called once per request, resulting in higher CPU usage but lower memory usage. Or the API can be called once per connection (by building a connection cache), resulting in lower CPU but higher memory usage.

Proposed API

namespace Microsoft.AspNetCore.Server.HttpSys;

+ delegate void ClientHelloBytesCallback<TState>(TState state, ReadOnlySpan<byte> clientHelloData);

+ public interface IHttpSysRequestPropertiesFeature
+ {
+ 	void GetClientHello<TState>(TState state, ClientHelloBytesCallback<TState> clientHelloCallback);
+ }

GetClientHello() invokes the user-provided callback, passing the Client Hello bytes as a ReadOnlySpan<byte>. The user can also pass in a state argument, which will be passed directly to the callback. The API shape is modeled after string.Create() and allows the implementation to be allocation-free, since the span can be backed by a pooled array. GetClientHello() will throw an exception on error.

The IHttpSysRequestPropertiesFeature naming is based on the name of the underlying Windows http.sys HttpQueryRequestProperty() API. The idea is that the same feature interface could be used to expose other parts of this Windows API in the future if needed.

Usage Examples

app.Use(async (httpContext, next) =>
{
    var feature = httpContext.Features.Get<IHttpSysRequestPropertiesFeature>();

    feature.GetClientHello(httpContext, static (context, clientHello) =>
    {
        // Extract some data from the ClientHello and store it in the HttpContext for use in later middleware.
        context.Items["ClientHelloData"] = Convert.ToHexString(clientHello);
    });

    await next();
});

Alternative Designs

The API could take a user-provided buffer instead. The user has to guess a buffer size with this approach. If the buffer size is too small, the method returns false and indicates the required size in bytesRequired.

public interface IHttpSysRequestPropertiesFeature
{
    bool GetClientHello(ReadOnlySpan<byte> clientHello, out int bytesWritten, out int bytesRequired);
}

For any error other than "buffer too small", the API would throw an exception.

Risks

The HttpQueryRequestProperty() Windows API takes an LPOVERLAPPED as an argument, allowing async completion. It may be necessary to expose an async API if this routine ever blocks.

[DeagleGross]

Thanks for proposal Mark.
I personally like alternative design more (we will discuss it on the API review): it is a straightforward invocation. Originally proposed API seems too complicated - the usage is to call a method and expect an immediate callback.

However, I would change alternative design a bit:

1. pattern of methods using Span<byte> has naming like Try...
2. it should have Span not ReadOnlySpan parameter, since implementation will be writing the data into it
3. rename to **Tls**ClientHello as in initial design

public interface IHttpSysRequestPropertiesFeature
{
+    bool TryGetTlsClientHello(Span<byte> tlsClientHelloBytes, out int bytesWritten, out int bytesRequired);
}

[dotnet-policy-service]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[DeagleGross]

Another great suggestion from @BrennanConroy is to return a win-error int value (which is what OS returns to us from interop call):

public interface IHttpSysRequestPropertiesFeature
{
+    int TryGetTlsClientHello(Span<byte> tlsClientHelloBytes, out int bytesWritten, out int bytesRequired);
}

this should help differentiate between different unsuccessful attempts (also showing insufficient buffer error explicitly)