-
Notifications
You must be signed in to change notification settings - Fork 10.3k
feat: fetch TLS client hello message from HTTP.SYS #60806
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR adds an API to fetch the TLS client hello message by introducing a new interface (ITlsAccessFeature) along with sample code changes and necessary updates in the underlying HttpSys and native interop code. Key changes include:
- Introducing ITlsAccessFeature and its implementation to expose raw TLS client hello message bytes.
- Updating the HttpSys request processing to retrieve, log, and expose the TLS client hello message.
- Adding support in HttpApi interop and configuring HTTP service settings to enable caching of the client hello message.
Reviewed Changes
File | Description |
---|---|
src/Servers/HttpSys/samples/TlsFeaturesObserve/Program.cs | Updated sample to invoke new TLS client hello message caching configuration. |
src/Servers/HttpSys/samples/TlsFeaturesObserve/Startup.cs | Sample updated to use ITlsAccessFeature for fetching TLS client hello bytes. |
src/Servers/Connections.Abstractions/src/Features/ITlsAccessFeature.cs | New API interface for accessing TLS client hello message bytes. |
src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs | Integrated methods to retrieve and parse TLS client hello message bytes. |
src/Servers/HttpSys/src/NativeInterop/HttpApi.cs | Updated interop definitions to include HttpSetServiceConfiguration. |
Others | Minor updates in logging, error codes, and feature collection to support the new API. |
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (4)
src/Servers/HttpSys/samples/TlsFeaturesObserve/Program.cs:11
- [nitpick] The namespace 'TlsFeatureObserve' is inconsistent with the folder name 'TlsFeaturesObserve'. Consider aligning the namespace with the folder structure to improve clarity.
namespace TlsFeatureObserve;
src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs:245
- [nitpick] A magic number (11) is used inline for the TLS client hello property. Consider using a named constant to enhance readability and maintainability.
11 /* HTTP_REQUEST_PROPERTY.HttpRequestPropertyTlsClientHello */
src/Servers/HttpSys/src/LoggerEventIds.cs:62
- [nitpick] The logger event name 'TlsClientHelloParseError' is defined here but the LoggerMessage in RequestContext.Log.cs uses 'TlsClientHelloRetrieveError'. Ensure consistent naming for clarity.
public const int TlsClientHelloParseError = 55;
src/Servers/HttpSys/src/RequestProcessing/RequestContext.cs:227
- [nitpick] The implementation of 'GetTlsClientHelloMessageBytes()' appears similar to other copies in the code. Consider refactoring the duplicate logic into a shared utility method to improve maintainability.
internal unsafe byte[]? GetTlsClientHelloMessageBytes()
880651a
to
c9431ec
Compare
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/14315380132 |
@DeagleGross an error occurred while backporting to "release/8.0", please check the run log for details! Error: @DeagleGross is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=DeagleGross |
/backport to release/8.0 |
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/14315397681 |
|
||
internal void InvokeTlsClientHelloCallback(IFeatureCollection features, Request request) | ||
{ | ||
if (_connectionIdsTlsClientHelloCallbackInvokedMap.TryGetValue(request.UConnectionId, out _)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think UConnectionId
varies per request on HTTP/2, see comment here: https://source.dot.net/#Microsoft.AspNetCore.Server.HttpSys/RequestProcessing/Request.cs,48.
The DisconnectToken
that's used to fire the ConnectionClosed()
callback behaves the same way - it calls HttpWaitForDisconnectEx()
, which returns at the end of every request for HTTP/2, not at the end of the connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ye, I also thought about it. I will re-do now tracking RawConnectionId
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So since I dont see a way to follow on any http.sys notification/subscription around connection lifetime (RawConnectionId
, not UConnectionId
), I changed approach to TTL based (basically keep it in cache while connection alive, if no requests come in some interval (10 minutes as hardcoded value), then we remove the connectionId from cache).
I am going to sync with @BrennanConroy to discuss it offline
using TlsFeatureObserve; | ||
using TlsFeaturesObserve.HttpSys; | ||
|
||
HttpSysConfigurator.ConfigureCacheTlsClientHello(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fetching TLS client hello feature from HTTP.SYS works only if a pre-configuration was done for ssl cert binding and netsh ssl config beforehand. User would need to set service configuration via HttpSetServiceConfiguration with HTTP_SERVICE_CONFIG_SSL_FLAG_ENABLE_CACHE_CLIENT_HELLO
flag.
That is and should not be a part of ASP.NET app, but I added that for "sample app" for simplicity of reproduction.
src/Servers/HttpSys/samples/TlsFeaturesObserve/HttpSys/HttpSysConfigurator.cs
Outdated
Show resolved
Hide resolved
[StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] | ||
public struct HTTP_SERVICE_CONFIG_SSL_PARAM | ||
{ | ||
public int SslHashLength; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc comments here as in the class below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, these are all documented in the http.sys docs, e.g. https://learn.microsoft.com/en-us/windows/win32/api/http/ns-http-http_service_config_ssl_param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we point to that somewhere? Not sure how frequently users interact with this type but having some sort of reference even for us as code maintainers might be helpful.
Expose API to get the TLS client hello message from http.sys
A new callback added to
HttpSysOptions
should invoke a callback with the raw bytes of TLS client hello message, and it should be happening once per connection.Fixes #60805