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.

Sign up for GitHub

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

Jump to bottom

TLS error callback called twice #23631

Closed
neroux opened this issue Oct 12, 2018 · 3 comments
Closed

TLS error callback called twice #23631

neroux opened this issue Oct 12, 2018 · 3 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@neroux
Copy link

neroux commented Oct 12, 2018

  • Version: v10.12.0 (issue appeared with 10.0.0, 9.11.2 was fine)
  • Platform: Linux 4.9.0-8-amd64 SMP Debian 4.9.110-3+deb9u6 (2018-10-08) x86_64 GNU/Linux
  • Subsystem: TLS

Running

./node -e "require('tls').connect({ host: '104.27.165.195', port: 443 }).on('error', error => { console.log(error); });"

on Debian 9 emits twice an error

{ Error: Client network socket disconnected before secure TLS connection was established
    at TLSSocket.onConnectEnd (_tls_wrap.js:1086:19)
    at Object.onceWrapper (events.js:273:13)
    at TLSSocket.emit (events.js:187:15)
    at endReadableNT (_stream_readable.js:1094:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)
  code: 'ECONNRESET',
  path: undefined,
  host: '104.27.165.195',
  port: 443,
  localAddress: undefined }

followed immediately by

[Error: 139716102896512:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:../deps/openssl/openssl/ssl/record/rec_layer_s3.c:1407:SSL alert number 40
]

It might be an issue with this server's particular TLS configuration, however it still shouldnt emit the error twice, and neither did so before 10.0.
@addaleax addaleax added the tls Issues and PRs related to the tls subsystem. label Oct 12, 2018
@cjihrig cjihrig mentioned this issue Oct 13, 2018
Merged
2 tasks
cjihrig added a commit to cjihrig/node that referenced this issue Oct 17, 2018
@cjihrig
tls: prevent multiple connection errors
b94ce57 
onConnectEnd(), which is called by TLSSocket, has a guard to
prevent being called multiple times, but it does not prevent the
OpenSSL error handler from being called, leading to multiple
error events. This commit adds that piece of missing logic.

PR-URL: nodejs#23636
Fixes: nodejs#23631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Oct 17, 2018
@cjihrig @jasnell
tls: prevent multiple connection errors
934eb7e 
onConnectEnd(), which is called by TLSSocket, has a guard to
prevent being called multiple times, but it does not prevent the
OpenSSL error handler from being called, leading to multiple
error events. This commit adds that piece of missing logic.

PR-URL: #23636
Fixes: #23631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this issue Oct 20, 2018
@cjihrig @addaleax
tls: prevent multiple connection errors
b5a5626 
onConnectEnd(), which is called by TLSSocket, has a guard to
prevent being called multiple times, but it does not prevent the
OpenSSL error handler from being called, leading to multiple
error events. This commit adds that piece of missing logic.

PR-URL: #23636
Fixes: #23631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@neroux
Copy link
Author

neroux commented Oct 23, 2018

This fix is not yet in 11.0, correct?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 23, 2018

According to https://nodejs.org/en/blog/release/v11.0.0/, it's in 11.0.0

@neroux
Copy link
Author

neroux commented Oct 23, 2018

True, I found it under its PR number. I was looking for the issue. Thanks.

MylesBorins pushed a commit that referenced this issue Oct 30, 2018
@cjihrig @MylesBorins
tls: prevent multiple connection errors
81dc241 
onConnectEnd(), which is called by TLSSocket, has a guard to
prevent being called multiple times, but it does not prevent the
OpenSSL error handler from being called, leading to multiple
error events. This commit adds that piece of missing logic.

PR-URL: #23636
Fixes: #23631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this issue Nov 28, 2018
@cjihrig @rvagg
tls: prevent multiple connection errors
17589a5 
onConnectEnd(), which is called by TLSSocket, has a guard to
prevent being called multiple times, but it does not prevent the
OpenSSL error handler from being called, leading to multiple
error events. This commit adds that piece of missing logic.

PR-URL: #23636
Fixes: #23631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
@cjihrig @MylesBorins
tls: prevent multiple connection errors
ed0a97a 
onConnectEnd(), which is called by TLSSocket, has a guard to
prevent being called multiple times, but it does not prevent the
OpenSSL error handler from being called, leading to multiple
error events. This commit adds that piece of missing logic.

PR-URL: #23636
Fixes: #23631
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Wyatt Preul <wpreul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@gluwer gluwer mentioned this issue Dec 21, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@addaleax @cjihrig @neroux