This repository has been archived by the owner. It is now read-only.

Allow tls.connect to start on server-side sockets and add tls.start as a simple wrapper for tls.connect #3653

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@Rush

Rush commented Jul 6, 2012

I have found a piece of code floating on the internet to initiate a TLS function in the middle of a TCP connection, but the code in question has not functioned well. I decided to look into tls.js module whether it can do it all with some minor adjustment.

There was a previous pull request for this function joyent#848
However, current state of connect does not allow to start TLS arbitralily on a server-side socket.

Thus, I propose a very simple change to tls.connect allowing to say whether we want a server side socket via options.asServer but also I add a very nice intuitive wrapper called tls.start and two tests.

tls.start is in the same style as child_process.fork so I think it fits together inside node.

I am willing to update documention if a green light is given to this improvements.

@@ -1119,7 +1119,8 @@ exports.connect = function(/* [port, host], options, cb */) {
var sslcontext = crypto.createCredentials(options);
convertNPNProtocols(options.NPNProtocols, this);
- var pair = new SecurePair(sslcontext, false, true,
+
+ var pair = new SecurePair(sslcontext, options.asServer?true:false, true,

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Style.

+exports.start = function(socket /* [options], [asServer], cb */) {
+ var cb, options = {}, asServer;
+
+ if(!socket) {

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Style.

@@ -0,0 +1,46 @@
+var assert = require('assert');
+var net = require('net');
+var common = require('../common');

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Should be the first require() statement. This file needs the copyright boilerplate as well (copy it from one of the other tests).

@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Should be the first require() statement. This file needs the copyright boilerplate as well (copy it from one of the other tests).

+ // allow asServer to be passed either through argument
+ // or through options
+ asServer: asServer || options.asServer
+ }, options || {});

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

The || {} is superfluous, _extend() handles that.

@bnoordhuis

bnoordhuis Aug 2, 2012

Member

The || {} is superfluous, _extend() handles that.

This comment has been minimized.

Show comment Hide comment
@Rush

Rush Aug 3, 2012

Honestly I was basing this on line 1111 of the same file, in the connect function. Could I remove the "|| {}" there as well?

@Rush

Rush Aug 3, 2012

Honestly I was basing this on line 1111 of the same file, in the connect function. Could I remove the "|| {}" there as well?

+ var encryptedStream = tls.start(socket, serverOptions, true, function() {
+ serverSecurelyConnected = true;
+ });
+}).listen(9999);

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Use common.PORT here.

@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Use common.PORT here.

+ client.end();
+ });
+
+});

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Move this test to test/internet/

@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Move this test to test/internet/

+ var encryptedStream = tls.start(client, function() {
+ clientSecurelyConnected = true;
+ if(serverSecurelyConnected) {
+ process.exit(0);

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Close the client/server, don't exit.

@bnoordhuis

bnoordhuis Aug 2, 2012

Member

Close the client/server, don't exit.

+
+setTimeout(function() {
+ assert.fail("tls.start failed, client has not been able to upgrade connection");
+}, 100);

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis Aug 2, 2012

Member

This is timing sensitive. Use process.on('exit', ...).

@bnoordhuis

bnoordhuis Aug 2, 2012

Member

This is timing sensitive. Use process.on('exit', ...).

@Nodejs-Jenkins

This comment has been minimized.

Show comment Hide comment
@Nodejs-Jenkins

Nodejs-Jenkins Mar 13, 2013

Can one of the admins verify this patch?

Can one of the admins verify this patch?

@tjfontaine

This comment has been minimized.

Show comment Hide comment
@tjfontaine

tjfontaine Feb 18, 2014

This should be implemented in the current unstable, thanks!

This should be implemented in the current unstable, thanks!

@tjfontaine tjfontaine closed this Feb 18, 2014

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Dec 15, 2015

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Finally, change the behavior of --abort-on-uncaught-exception so that,
if the domain within which the error is thrown has no error handler, but
a domain further up the domains stack has one, the process will not
abort.

Fixes #3607 and #3653.

PR: #3654
PR-URL: nodejs/node#3654
Reviewed-By: Chris Dickinson <chris@neversaw.us>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Dec 17, 2015

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

PR: #3887
PR-URL: nodejs/node#3887
Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Dec 17, 2015

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

PR: #3885
PR-URL: nodejs/node#3885
Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Dec 17, 2015

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes #3607 and #3653.

PR: #3884
PR-URL: nodejs/node#3884
Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Dec 18, 2015

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes #3607 and #3653.

PR: #3884
PR-URL: nodejs/node#3884
Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Jan 11, 2016

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Fixes #3607 and #3653.

PR: #3884
PR-URL: nodejs/node#3884
Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Jan 13, 2016

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an `'uncaughtException'` event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an `'uncaughtException'` event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and `--abort-on-uncaught-exception` is used.

Finally, change the behavior of --abort-on-uncaught-exception so that,
if the domain within which the error is thrown has no error handler, but
a domain further up the domains stack has one, the process will not
abort.

Fixes #3607 and #3653.

PR: #3654
PR-URL: nodejs/node#3654
Reviewed-By: Chris Dickinson <chris@neversaw.us>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Feb 15, 2016

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

PR: #3885
PR-URL: nodejs/node#3885
Reviewed-By: James M Snell <jasnell@gmail.com>

richardlau pushed a commit to ibmruntimes/node that referenced this pull request Feb 15, 2016

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

PR: #3887
PR-URL: nodejs/node#3887
Reviewed-By: James M Snell <jasnell@gmail.com>

jBarz added a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

PR: #3885
PR-URL: nodejs/node#3885
Reviewed-By: James M Snell <jasnell@gmail.com>

jBarz added a commit to ibmruntimes/node that referenced this pull request Feb 16, 2017

domains: fix handling of uncaught exceptions
Fix node exiting due to an exception being thrown rather than emitting
an 'uncaughtException' event on the process object when:
1. no error handler is set on the domain within which an error is thrown
2. an 'uncaughtException' event listener is set on the process

Also fix an issue where the process would not abort in the proper
function call if an error is thrown within a domain with no error
handler and --abort-on-uncaught-exception is used.

Fixes #3607 and #3653.

PR: #3885
PR-URL: nodejs/node#3885
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.