Skip to content

net,tls: add abort signal support to connect#37735

Closed
Linkgoron wants to merge 1 commit intonodejs:masterfrom
Linkgoron:net-tls-add-connect-abort-signal
Closed

net,tls: add abort signal support to connect#37735
Linkgoron wants to merge 1 commit intonodejs:masterfrom
Linkgoron:net-tls-add-connect-abort-signal

Conversation

@Linkgoron
Copy link
Contributor

@Linkgoron Linkgoron commented Mar 13, 2021 •
edited
Loading

A previous PR added support for AbortSignal to the net.Socket constructor and net.connect, however there was some missing documentation. I've added documentation (albeit minor) - I'd be happy for more suggestions if there are any other places that need it added, and also added tests for net.Socket and net.connect.

In addition, I've extended the AbortSignal support from net.Socket to tls.TLSSocket as it was a relatively minor change, and added tests for tls.connect, tls.TLSSocket, http2.connect and https.Agent.

There are https.Agent and https.request tests that fail without #37730, so this PR is blocked on that one.

@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 Mar 13, 2021
@Linkgoron Linkgoron force-pushed the net-tls-add-connect-abort-signal branch 2 times, most recently from e099f3f to bcc1e4f Compare March 13, 2021 02:54
@aduh95 aduh95 added blocked PRs that are blocked by other issues or PRs. semver-minor PRs that contain new features and should be released in the next minor version. labels Mar 13, 2021
@Linkgoron Linkgoron force-pushed the net-tls-add-connect-abort-signal branch from bcc1e4f to 332ab4f Compare March 13, 2021 10:50
benjamingr
benjamingr reviewed Mar 13, 2021
View reviewed changes
benjamingr
benjamingr reviewed Mar 13, 2021
View reviewed changes
test/parallel/test-net-connect-abort-controller.js
await testConstructorPost();
await testConstructorPostTick();

// Killing the net.socket without connecting hangs the server.
Copy link
Member

@benjamingr benjamingr Mar 13, 2021

Choose a reason for hiding this comment

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

Probably easier to call socket.setTimeout with a low timeout - but probably what you did is also fine

benjamingr
benjamingr approved these changes Mar 13, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

LGTM after #37730 lands

benjamingr
benjamingr reviewed Mar 13, 2021
View reviewed changes
@Linkgoron Linkgoron force-pushed the net-tls-add-connect-abort-signal branch from 332ab4f to 1fd2961 Compare March 13, 2021 22:48
@Linkgoron
net,tls: add abort signal support to connect
ce73ba1 
Add documentation for net.connect AbortSignal,
and add the support to tls.connect as well
@Linkgoron Linkgoron force-pushed the net-tls-add-connect-abort-signal branch from 1fd2961 to ce73ba1 Compare March 20, 2021 20:04
@benjamingr benjamingr added the notable-change PRs with changes that should be highlighted in changelogs. label Mar 20, 2021
@Linkgoron Linkgoron removed the blocked PRs that are blocked by other issues or PRs. label Mar 20, 2021
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 20, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36886/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 22, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/36902/

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2021

CI: https://ci.nodejs.org/job/node-test-pull-request/37079/

@Linkgoron Linkgoron added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 1, 2021
@jasnell
Copy link
Member

jasnell commented Apr 1, 2021

Landed in f87c4d1

@jasnell jasnell closed this Apr 1, 2021
jasnell pushed a commit that referenced this pull request Apr 1, 2021
@Linkgoron @jasnell
net,tls: add abort signal support to connect
f87c4d1 
Add documentation for net.connect AbortSignal,
and add the support to tls.connect as well

PR-URL: #37735
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 4, 2021
@Linkgoron @MylesBorins
net,tls: add abort signal support to connect
669b81c 
Add documentation for net.connect AbortSignal,
and add the support to tls.connect as well

PR-URL: #37735
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins added a commit that referenced this pull request Apr 4, 2021
@MylesBorins
2021-04-06, Version 15.14.0 (Current)
28cf81b 
This is a security release

Vulnerabilities fixed:

Coming Soon

Other Notable changes:

fs:
  * (SEMVER-MINOR) add support for async iterators to `fsPromises.writeFile` (HiroyukiYagihashi) #37490
net:
  * (SEMVER-MINOR) allow net.BlockList to use net.SocketAddress objects (James M Snell) #37917
  * (SEMVER-MINOR) add SocketAddress class (James M Snell) #37917
  * (SEMVER-MINOR) make net.BlockList cloneable (James M Snell) #37917
net,tls:
  * (SEMVER-MINOR) add abort signal support to connect (Nitzan Uziely) #37735
readline:
  * (SEMVER-MINOR) add AbortSignal support to interface (Nitzan Uziely) #37932

PR-URL: TODO
@MylesBorins MylesBorins mentioned this pull request Apr 4, 2021
Merged
MylesBorins added a commit that referenced this pull request Apr 5, 2021
@MylesBorins
2021-04-06, Version 15.14.0 (Current)
02160fd 
Vulnerabilities fixed:

- **CVE-2021-3450**: OpenSSL - CA certificate check bypass with X509_V_FLAG_X509_STRICT (High)
- **CVE-2021-3449**: OpenSSL - NULL pointer deref in signature_algorithms processing (High)
- **CVE-2020-7774**: npm upgrade - Update y18n to fix Prototype-Pollution (High)

Other Notable changes:

fs:
  * (SEMVER-MINOR) add support for async iterators to `fsPromises.writeFile` (HiroyukiYagihashi) #37490
net:
  * (SEMVER-MINOR) allow net.BlockList to use net.SocketAddress objects (James M Snell) #37917
  * (SEMVER-MINOR) add SocketAddress class (James M Snell) #37917
  * (SEMVER-MINOR) make net.BlockList cloneable (James M Snell) #37917
net,tls:
  * (SEMVER-MINOR) add abort signal support to connect (Nitzan Uziely) #37735
readline:
  * (SEMVER-MINOR) add AbortSignal support to interface (Nitzan Uziely) #37932

PR-URL: #38084
MylesBorins added a commit that referenced this pull request Apr 6, 2021
@MylesBorins
2021-04-06, Version 15.14.0 (Current)
7f4f37c 
Notable Changes:

This is a security release.

Vulnerabilities fixed:

- **CVE-2021-3450**: OpenSSL - CA certificate check bypass with X509_V_FLAG_X509_STRICT (High)
- **CVE-2021-3449**: OpenSSL - NULL pointer deref in signature_algorithms processing (High)
- **CVE-2020-7774**: npm upgrade - Update y18n to fix Prototype-Pollution (High)

Other Notable changes:

fs:
  * (SEMVER-MINOR) add support for async iterators to `fsPromises.writeFile` (HiroyukiYagihashi) #37490
net:
  * (SEMVER-MINOR) allow net.BlockList to use net.SocketAddress objects (James M Snell) #37917
  * (SEMVER-MINOR) add SocketAddress class (James M Snell) #37917
  * (SEMVER-MINOR) make net.BlockList cloneable (James M Snell) #37917
net,tls:
  * (SEMVER-MINOR) add abort signal support to connect (Nitzan Uziely) #37735
readline:
  * (SEMVER-MINOR) add AbortSignal support to interface (Nitzan Uziely) #37932

PR-URL: #38084
MylesBorins added a commit that referenced this pull request Apr 6, 2021
@MylesBorins
2021-04-06, Version 15.14.0 (Current)
1a34e9c 
Notable Changes:

This is a security release.

Vulnerabilities fixed:

- **CVE-2021-3450**: OpenSSL - CA certificate check bypass with X509_V_FLAG_X509_STRICT (High)
- **CVE-2021-3449**: OpenSSL - NULL pointer deref in signature_algorithms processing (High)
- **CVE-2020-7774**: npm upgrade - Update y18n to fix Prototype-Pollution (High)

Other Notable changes:

fs:
  * (SEMVER-MINOR) add support for async iterators to `fsPromises.writeFile` (HiroyukiYagihashi) #37490
net:
  * (SEMVER-MINOR) allow net.BlockList to use net.SocketAddress objects (James M Snell) #37917
  * (SEMVER-MINOR) add SocketAddress class (James M Snell) #37917
  * (SEMVER-MINOR) make net.BlockList cloneable (James M Snell) #37917
net,tls:
  * (SEMVER-MINOR) add abort signal support to connect (Nitzan Uziely) #37735
readline:
  * (SEMVER-MINOR) add AbortSignal support to interface (Nitzan Uziely) #37932

PR-URL: #38084
MylesBorins added a commit that referenced this pull request Apr 6, 2021
@MylesBorins
2021-04-06, Version 15.14.0 (Current)
78b6f38 
Notable Changes:

This is a security release.

Vulnerabilities fixed:

- **CVE-2021-3450**: OpenSSL - CA certificate check bypass with X509_V_FLAG_X509_STRICT (High)
- **CVE-2021-3449**: OpenSSL - NULL pointer deref in signature_algorithms processing (High)
- **CVE-2020-7774**: npm upgrade - Update y18n to fix Prototype-Pollution (High)

Other Notable changes:

fs:
  * (SEMVER-MINOR) add support for async iterators to `fsPromises.writeFile` (HiroyukiYagihashi) #37490
net:
  * (SEMVER-MINOR) allow net.BlockList to use net.SocketAddress objects (James M Snell) #37917
  * (SEMVER-MINOR) add SocketAddress class (James M Snell) #37917
  * (SEMVER-MINOR) make net.BlockList cloneable (James M Snell) #37917
net,tls:
  * (SEMVER-MINOR) add abort signal support to connect (Nitzan Uziely) #37735
readline:
  * (SEMVER-MINOR) add AbortSignal support to interface (Nitzan Uziely) #37932

PR-URL: #38084
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Linkgoron @nodejs-github-bot @jasnell @benjamingr @targos @aduh95