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

net: fix family autoselection SSL connection handling #48189

Conversation

ShogunPanda
Copy link
Contributor

This PR fixes SSL connection attempts by not attempting to restore SSL information (like server name) if such informations are not available anymore.

Fixes #48000.

@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 26, 2023
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 26, 2023
@nodejs-github-bot
Copy link
Collaborator

const { request } = require('https');

request(
'https://nodejs.org/en',
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this test go in test/internet/ if it's attempting to reach an internet host?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I didn't know about it. Fixed now.

const { request } = require('https');

request(
'https://nodejs.org/en',
Copy link
Member

Choose a reason for hiding this comment

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

If this is reaching out to the internet this should go in internet and not parallel. It should also be using a constant from common/internet to allow people to run the test suites in places that restrict access to some addresses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know about this. This is fixed now.

@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 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 force-pushed the fix-family-autoselection-ssl-connect branch from 7425b2d to 5150216 Compare May 29, 2023 11:01
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2023
@nodejs-github-bot
Copy link
Collaborator

@ShogunPanda
Copy link
Contributor Author

@nodejs/build I have a failure in GH suite but I don't think it's related to my changes. Can I ignore or do you first want to address it?

@ShogunPanda
Copy link
Contributor Author

Landed in 26450c5

ShogunPanda added a commit that referenced this pull request May 31, 2023
PR-URL: #48189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@targos
Copy link
Member

targos commented May 31, 2023

The test-internet failure was confirmed to be unrelated and we opened #48262 for it.

@ShogunPanda ShogunPanda deleted the fix-family-autoselection-ssl-connect branch June 1, 2023 20:50
targos pushed a commit that referenced this pull request Jun 4, 2023
PR-URL: #48189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@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
PR-URL: nodejs#48189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jun 30, 2023
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#48189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48189
Backport-URL:
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48189
Backport-PR-URL:
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@mhdawson mhdawson added the backport-open-v18.x Indicate that the PR has an open backport. label Aug 15, 2023
mhdawson pushed a commit to mhdawson/io.js that referenced this pull request Aug 15, 2023
PR-URL: nodejs#48189
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
ruyadorno pushed a commit that referenced this pull request Aug 17, 2023
PR-URL: #48189
Backport-PR-URL: #49183
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Marco Ippolito <[email protected]>
@ruyadorno ruyadorno added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. backport-open-v18.x Indicate that the PR has an open backport. labels Aug 17, 2023
@ruyadorno ruyadorno mentioned this pull request Aug 17, 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.

Assertion (wrap->ssl_) != nullptr failed in TLSWrap::GetServername
10 participants