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

Support TLS 1.3 #618

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

Support TLS 1.3 #618

wants to merge 13 commits into from

Conversation

evolutionise
Copy link
Contributor

@evolutionise evolutionise commented Nov 4, 2024

Background

We want to support TLS 1.3 for server/tentacle communication.

We currently explicitly specify supported SSL protocols in Halibut, Tentacle, and Server.

In the long term, we intend to explore following Microsoft recommendations and not configuring protocols explicitly and leaving the choice to the OS.

In the immediate term, we are just going to add TLS 1.3. to the configured set of supported protocols.

This PR will be followed by PRs to Tentacle and Server to update Halibut.

Results

This PR just adds SslProtocol.TLS13 to the set of supported protocols in the various places we configure protocols in Halibut.

The change has been tested in branch builds of Tentacle and Server and we have confirmed that:

  • TLS 1.3 is used for tentacle communication when supported by the OS.

Screenshot 2024-11-26 at 9 57 39 AM

  • TLS 1.2 is still used successfully when the platform does not support TLS 1.3.

image

How to review this PR

Is the current testing sufficient?

  • All automated tests pass.
  • We have manually tested using this configuration of Halibut in Tentacle and Server in cloud and local dev environments.

Quality ✔️

Pre-requisites

  • I have read How we use GitHub Issues for help deciding when and where it's appropriate to make an issue.
  • I have considered informing or consulting the right people, according to the ownership map.
  • I have considered appropriate testing for my change.

@gb-8 gb-8 changed the title what if we just like .... didn't ... specify tls versions tho? Support TLS 1.3 Nov 26, 2024
@gb-8 gb-8 marked this pull request as ready for review November 26, 2024 22:58
@gb-8 gb-8 requested a review from a team as a code owner November 26, 2024 22:58
@gb-8 gb-8 requested a review from LukeButters November 26, 2024 23:02
await ssl.AuthenticateAsClientAsync(
serviceEndpoint.BaseUri.Host,
new X509Certificate2Collection(),
SslProtocols.Tls | SslProtocols.Tls11 | SslProtocols.Tls12 | SslProtocols.Tls13,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: It might make sense to pull this list out to a single place and perhaps make it user configurable within the HalibutRuntimeBuilder

Copy link
Contributor

@LukeButters LukeButters left a comment

Choose a reason for hiding this comment

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

Approved although, Is it possible to write a test for this?

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.

3 participants