Skip to content
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

feat: control SSL protocol version on tls jobs #2049

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

DeagleGross
Copy link
Contributor

@DeagleGross DeagleGross commented Jan 20, 2025

Adding an ability to specify the tls version for the job.

In case of Kestrel, added an option to do it on server side. In case of HTTP.SYS the only available option is to modify the underlying registry (which is unsafe and can fail / leave machine in a transient state). So the easiest is to explicitly set the SslProtocols property on the client causing server to establish a connection using specified tls protocol version.

the option load.httpClient.variables.sslProtocols is being contributed here: dotnet/crank#786 (current PR should be merged after the crank one)

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Are we going to add TLS1.3?
We should probably change the scenario names so tls1.2 and tls1.3 are distinguishable.

src/BenchmarksApps/TLS/Kestrel/Program.cs Outdated Show resolved Hide resolved
src/BenchmarksApps/TLS/Kestrel/Program.cs Outdated Show resolved Hide resolved
@DeagleGross
Copy link
Contributor Author

DeagleGross commented Jan 21, 2025

Are we going to add TLS1.3?

I think we should not add more jobs, but simply run TLS1.3 when we would want to compare the perf. It feels like we are adding too many jobs at a moment if we would want to multiply by TLS / OpenSSL version (x3 jobs).
what do you think?

Console.WriteLine("TLS: " + tlsHandshakeFeature.Protocol);
Console.WriteLine("-----");
}
await next();
Copy link
Member

Choose a reason for hiding this comment

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

Same 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants