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

tls: reapply servername on happy eyeballs connect #48255

Merged
merged 1 commit into from
Jun 2, 2023

Conversation

indutny
Copy link
Member

@indutny indutny commented May 30, 2023

When establishing a TLS connection to a server with autoSelectFamily set to true, the net.Socket will call [kWrapConnectedHandle]() to reinitialize the socket (in case if it got broken during previous connect attempts). Unfortunately, prior to this patch this resulted in a brand new TLSWrap instance being created for the socket. While most of the configuration of TLSWrap is restored, the servername was sadly dropped and not reinitalized.

With this patch servername will be reinitialized if there are tls.connect options present on the TLSSocket instance, making it possible to connect with "Happy Eyeballs" to TLS servers that require the servername extension.

When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.

With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem. labels May 30, 2023
@indutny
Copy link
Member Author

indutny commented May 30, 2023

cc @nodejs/net @nodejs/crypto

@indutny
Copy link
Member Author

indutny commented May 30, 2023

I'm not sure what the process is nowadays, but could this be backported to 18.x.y, please?

@tniessen tniessen requested a review from ShogunPanda May 30, 2023 19:28
@richardlau richardlau added the lts-watch-v18.x PRs that may need to be released in v18.x. label May 30, 2023
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

On a second look, I noticed kWrapConnectedHandle disappeared in #46587.
It seems like I already reinitialize the server name now (there is a little bug but I'm already fixing it in #48189).

@indutny Do we still need this?

@indutny
Copy link
Member Author

indutny commented May 31, 2023

@ShogunPanda ah, that's great! We still use Node 18, so we didn't get the fix there. Perhaps the PR here could be used as a backport of your work?

@ShogunPanda
Copy link
Contributor

ShogunPanda commented May 31, 2023

Yeah, the fixes are stuck in main due to #48000, unfortunately.
This is also making people angry in 20.x.
I think this two PRs are not conflicting. Do you agree? If yes we can land them both to eventually speed up backporting.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ShogunPanda ShogunPanda added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 2, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 2, 2023
@nodejs-github-bot nodejs-github-bot merged commit 2fca7ea into nodejs:main Jun 2, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 2fca7ea

targos pushed a commit that referenced this pull request Jun 4, 2023
When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.

With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.

PR-URL: #48255
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@targos targos mentioned this pull request Jun 4, 2023
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.

With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.

PR-URL: nodejs#48255
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.

With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.

PR-URL: #48255
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.

With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.

PR-URL: nodejs#48255
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.

With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.

PR-URL: nodejs#48255
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
When establishing a TLS connection to a server with `autoSelectFamily`
set to `true`, the `net.Socket` will call `[kWrapConnectedHandle]()` to
reinitialize the socket (in case if it got broken during previous
connect attempts). Unfortunately, prior to this patch this resulted in a
brand new `TLSWrap` instance being created for the socket. While most of
the configuration of `TLSWrap` is restored, the `servername` was sadly
dropped and not reinitalized.

With this patch `servername` will be reinitialized if there are
`tls.connect` options present on the `TLSSocket` instance, making it
possible to connect with "Happy Eyeballs" to TLS servers that require
the servername extension.

PR-URL: nodejs#48255
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed lts-watch-v18.x PRs that may need to be released in v18.x. labels Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v18.x PRs backported to the v18.x-staging branch. needs-ci PRs that need a full CI run. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants