Skip to content

API Proposal: Expose TLS client hello message #60805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
1 task done
DeagleGross opened this issue Mar 7, 2025 · 8 comments · Fixed by #60806
Closed
1 task done

API Proposal: Expose TLS client hello message #60805

DeagleGross opened this issue Mar 7, 2025 · 8 comments · Fixed by #60806
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-httpsys
Milestone

Comments

@DeagleGross
Copy link
Member

DeagleGross commented Mar 7, 2025

Is there an existing issue for this?

  • I have searched the existing issues

Intro

We need to add implementation at least for HTTP.SYS / Kestrel servers. Based on SslStream API Proposal for Kestrel support (dotnet/runtime proposal), it seems more likely that a callback will be introduced there (something like SslServerAuthenticationOptions.ClientHelloBytesCallback).

Or we can just support it in aspnetcore directly similarly to how YARP has already done it (see TlsFilter middleware).

Proposed API

Kestrel

Exposing a separate extension on ListenOptionsHttpsExtensions or on a new type ListenOptionsTlsExtensions works for Kestrel

namespace Microsoft.AspNetCore.Hosting
{
  public static class ListenOptionsHttpsExtensions
  {
+   public static ListenOptions ConfigureTlsClientHelloCallback(this ListenOptions listenOptions, Action<ConnectionContext, ReadOnlySequence<byte>> tlsClientHelloCallback);  // has info about connection; has the tls client hello bytes
  }

+ public static class ListenOptionsTlsExtensions
+  {
+   public static ListenOptions ConfigureTlsClientHelloCallback(this ListenOptions listenOptions, Action<ConnectionContext, ReadOnlySequence<byte>> tlsClientHelloCallback);  // has info about connection; has the tls client hello bytes
+  }
}

Kestrel Usage:

options.ListenAnyIP(443, listenOptions =>
{
   listenOptions.ConfigureTlsClientHelloCallback((connectionContext, tlsClientHelloBytes) => {
       // do whatever with tlsClientHelloBytes
   });
});

Implementation of such a kind would be reading every packet, then if it appears to start with TLS client hello header, then we would try to identify the length of it, read needed data, and then invoke a callback. Later we would be able to even add an overload to provide a parsed TLS client hello message in a strictly typed way (like TlsFrameInfo)

This also helps user to associate the tls client hello with a specific connection (ConnectionContext parameter).

ReadOnlySequence<byte> should be chosen instead of ReadOnlySpan<byte> because tls info can come in different segments, and it will not be a contiguous memory then (see TlsFilter's if (!buffer.IsSingleSegment))

Http.Sys

In order to align the implementation for different servers, I am proposing to add a similar callback to HttpSysOptions

namespace Microsoft.AspNetCore.Server.HttpSys;

public class HttpSysOptions
{
+   Action<IFeatureCollection, ReadOnlySpan<byte>> ClientHelloBytesCallback { get; set; }
}

Http.sys Usage:

options.ClientHelloBytesCallback = (requestFeature, bytes)=>
{
       // do whatever with tlsClientHelloBytes
});

HTTP.SYS API would have same idea, but different implementation: a) having a callback in a different options class and b) having an IFeatureCollection instead of ConnectionContext (there is none yet for HTTP.SYS). That would also give full control to the user to lookup to any feature collection or even set their own feature for future call rejection for instance.

Other considered designs (and why they are not chosen)

Middleware

Adding a separate middleware on top of IApplicationBuilder.Run() will not fulfil the need, since such a middleware is terminal (runs after the internal ASP.NET middleware chain including SslStream one):

        app.Run(async (HttpContext httpContext) =>
        {
            var transportFeature = httpContext.Features.Get<IConnectionTransportFeature>();
            var pipeReader = transportFeature.Transport.Input;

            // imagine here we get the buffer for the TLS Client Hello message
            var bytes = await pipeReader.ReadAsync();
            var tlsCLientHelloMessage = bytes.Buffer.FirstSpan;

            tlsClientHelloCallback(httpContext, tlsCLientHelloMessage); // invoking, so users can do whatever
        });

Exposing bytes field on the ITlsHandshakeFeature (or similar)

This is considered a non-performant API for both servers due to increasing a memory footprint significantly (see dotnet/runtime#113729 (comment)):

public interface ITlsHandshakeFeature
{
  public byte[] TlsClientHelloMessageBytes { get; }
}

Is your feature request related to a problem? Please describe the problem.

The intention here is to have an API to access a TLS client hello message in a format of raw bytes (in example byte[]). In such a way users can analyze it and make whatever decision they want to.

@DeagleGross DeagleGross self-assigned this Mar 7, 2025
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Mar 7, 2025
@DeagleGross DeagleGross added feature-httpsys area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions labels Mar 7, 2025
@gfoidl gfoidl removed the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Mar 7, 2025
@DeagleGross DeagleGross added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Mar 12, 2025
@DeagleGross DeagleGross changed the title Expose TLS client hello message API Proposal: Expose TLS client hello message Mar 12, 2025
@halter73
Copy link
Member

What about adding a property with a default implementation to ITlsHandshakeFeature? It's already got a few of them. Would it be okay if this API is NETCOREAPP only like NegotiatedCipherSuite and HostName?

@DeagleGross
Copy link
Member Author

DeagleGross commented Mar 20, 2025

@halter73 I have no problem adding it to the ITlsHandshakeFeature. Why do you think we should cover it with NETCOREAPP only? I think we can even add it as #if NET_10_OR_GREATER to not introduce any change to interface for existing net10- users

I dont think it should be a property though, because we want it to be "on-demand" API (basically we do not fetch and save it if it is not requested)

@halter73
Copy link
Member

API Review Notes:

  • We like the callback based API for efficiency, but we want to see usage examples before approving such an API.

@DeagleGross
Copy link
Member Author

updated the proposal with a purely aspnetcore change (no SslStream / dotnet-runtime changes required):
dotnet/runtime#113729 (comment)

@halter73 @BrennanConroy let me know if you have comments, or we can go through the review

@halter73
Copy link
Member

halter73 commented Apr 1, 2025

?some request type here to correlate request with the invocation?

@DeagleGross Have you figured this part out yet?

@DeagleGross
Copy link
Member Author

@halter73 yep, proposed IFeatureCollection for http.sys ready for review

@BrennanConroy
Copy link
Member

  • Naming for normal ListenOptions extensions follows "Use*" pattern
    • UseTlsClientHelloBytesCallback?
  • Analyzer for ordering of connection middleware? e.g. must be called before UseHttps otherwise it won't work
    • In a perfect world, but it doesn't seem worth it at this time
  • Actually, we have HttpsConnectionAdapterOptions in Kestrel
  • Kestrel uses ReadOnlySequence because we're reading from a PipeReader which returns a ReadOnlySequence whereas in Http.Sys we pass a buffer to native and that buffer needs to be contiguous so we have a ReadOnlySpan already. Making Kestrel use a ReadOnlySpan would require extra copying and allocating which we'd like to avoid.
  • ClientHelloBytesCallback in the Http.Sys case, naming line up with Kestrels?
    • TlsClientHelloBytesCallback?
    • We like to keep Bytes in the name since we could potentially add another callback API with a strongly typed TLS frame in the future.

API Approved!

namespace Microsoft.AspNetCore.Server.Kestrel.Https;

public class HttpsConnectionAdapterOptions
{
+    Action<ConnectionContext, ReadOnlySequence<byte>> TlsClientHelloBytesCallback { get; set; }
}
namespace Microsoft.AspNetCore.Server.HttpSys;

public class HttpSysOptions
{
+    Action<IFeatureCollection, ReadOnlySpan<byte>> TlsClientHelloBytesCallback { get; set; }
}

@BrennanConroy BrennanConroy added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Apr 3, 2025
@halter73
Copy link
Member

halter73 commented Apr 7, 2025

Update: After implementing this, we discovered we need to make the callbacks nullable. The final approved API is:

namespace Microsoft.AspNetCore.Server.Kestrel.Https;

public class HttpsConnectionAdapterOptions
{
+    Action<ConnectionContext, ReadOnlySequence<byte>>? TlsClientHelloBytesCallback { get; set; }
}
namespace Microsoft.AspNetCore.Server.HttpSys;

public class HttpSysOptions
{
+    Action<IFeatureCollection, ReadOnlySpan<byte>>? TlsClientHelloBytesCallback { get; set; }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-httpsys
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants