Skip to content

feat: support tls client hello bytes callback in Kestrel #61631

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

feat: support tls client hello bytes callback in Kestrel #61631

wants to merge 16 commits into from

Conversation

DeagleGross
Copy link
Member

Supporting TLS Client Hello callback in Kestrel

HTTP.SYS contribution was done in this PR

Description

Adding a new property to HttpsConnectionAdapterOptions - TlsClientHelloBytesCallback (added to public API).
It allows to subscribe to the TLS client hello message parsed from the ConnectionContext.Transport.Input:

options.TlsClientHelloBytesCallback = (connection, clientHelloBytes) =>
{
    Logger.LogDebug("[Received TlsClientHelloBytesCallback] Connection: {0}; TLS client hello buffer: {1}", connection.ConnectionId, clientHelloBytes.Length);
};

If property HttpsConnectionAdapterOptions.TlsClientHelloBytesCallback is set (not null), then new middleware is added before HttpsConnectionMiddleware.

The implementation is doing the following:

  1. waiting for data to come
  2. if there is enough data, try to parse TLS and determine if we need to invoke a callback
  3. if there is not enough data, we either wait (not end of the stream) or simply continue the middleware pipeline

Fixes #60805

@DeagleGross DeagleGross self-assigned this Apr 23, 2025
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 23, 2025
gfoidl
gfoidl reviewed Apr 23, 2025
View reviewed changes
Copy link
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -- just a small comment.

@build-analysis build-analysis bot mentioned this pull request Apr 23, 2025
Open
3 tasks
@DeagleGross
Copy link
Member Author

add tests for SSL 2.0 and SSL 3.0

@DeagleGross
Copy link
Member Author

add tests for SSL 2.0 and SSL 3.0

done

BrennanConroy
BrennanConroy reviewed Apr 29, 2025
View reviewed changes
src/Servers/Kestrel/Core/src/HttpsConnectionAdapterOptions.cs
/// <summary>
/// A callback to be invoked to get the TLS client hello bytes.
/// Null by default.
/// </summary>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

If you want to store the bytes from the ReadOnlySequence<byte>, copy them into a buffer that you control rather than keeping a reference to the ReadOnlySequence or ReadOnlyMemory instances.

src/Servers/Kestrel/test/InMemory.FunctionalTests/TestTransport/InMemoryTransportConnection.cs
@@ -91,7 +91,7 @@ public override async ValueTask DisposeAsync()
// This piece of code allows us to wait until the PipeReader has been awaited on.
// We need to wrap lots of layers (including the ValueTask) to gain visiblity into when
// the machinery for the await happens
private class ObservableDuplexPipe : IDuplexPipe
internal class ObservableDuplexPipe : IDuplexPipe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undo changes to this file

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
{
[Theory]
[MemberData(nameof(ValidClientHelloData))]
public Task OnTlsClientHelloAsync_ValidData(int id, byte[] packetBytes, bool nextMiddlewareInvoked)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nextMiddlewareInvoked is always true?

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
);

await writer.WriteAsync(new byte[1] { 0x16 });
var middlewareTask = Task.Run(() => middleware.OnTlsClientHelloAsync(transportConnection));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to use Task.Run?

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
Assert.False(tlsClientHelloCallbackInvoked);

// ensuring that we have read limited number of times
Assert.True(reader.ReadAsyncCounter is >= 2 && reader.ReadAsyncCounter is <= 5,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be 2-3?

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
{
var pipe = new Pipe();
var writer = pipe.Writer;
var reader = new ObservablePipeReader(pipe.Reader);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: not needed?

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
var middlewareTask = Task.Run(() => middleware.OnTlsClientHelloAsync(transportConnection));

var random = new Random();
await Task.Delay(millisecondsDelay: random.Next(25, 75));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delays should not be needed. If you need to deterministically do something then you might want to make use of TaskCompletionSource.

e.g.

TaskCompletionSource Tcs => _tcs;
async ValueTask<ReadResult> ReadAsync(CancellationToken ...)
{
    await _tcs.Task;
    _tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAscynchronously);
    var res = await _inner.ReadAsync();
    return res;
}

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
{
var pipe = new Pipe();
var writer = pipe.Writer;
var reader = new ObservablePipeReader(pipe.Reader);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unneeded?

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
}
}

private static byte[] valid_clientHelloHeader =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually use PascalCase for properties/fields in this repo

src/Servers/Kestrel/Core/test/TlsListenerMiddlewareTests.cs
=> RunTlsClientHelloCallbackTest_WithMultipleSegments(id, packets, nextMiddlewareInvoked, tlsClientHelloCallbackExpected: false);

[Fact]
public async Task RunTlsClientHelloCallbackTest_DeterministinglyReads()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you're trying to do the same thing as RunTlsClientHelloCallbackTest_WithMultipleSegments in this test, any reason this one should exist?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gfoidl gfoidl gfoidl left review comments

@BrennanConroy BrennanConroy BrennanConroy left review comments

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@JamesNK JamesNK Awaiting requested review from JamesNK JamesNK is a code owner

@mgravell mgravell Awaiting requested review from mgravell mgravell is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@DeagleGross DeagleGross

Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API Proposal: Expose TLS client hello message
3 participants
@DeagleGross @gfoidl @BrennanConroy