tls: add getProtocol() to TLS sockets #4995

mscdex · 2016-01-31T06:37:15Z

These commits add a new method for TLS sockets that returns the negotiated protocol version and fix a documentation problem. tlsSocket.getCipher() is documented as returning the current protocol version, but that is incorrect. It merely returns the first protocol version in which that cipher first appeared/was supported.

mscdex · 2016-01-31T06:51:56Z

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

bnoordhuis · 2016-01-31T09:44:36Z

test/parallel/test-tls-getprotocol.js

+const options = {
+  key: fs.readFileSync(common.fixturesDir + '/keys/agent2-key.pem'),
+  cert: fs.readFileSync(common.fixturesDir + '/keys/agent2-cert.pem'),
+  secureProtocol: 'TLSv1_2_method'


Can you try two or three protocol versions here?

Do you mean 2 or 3 separate connections each with different secureProtocol settings?

bnoordhuis · 2016-01-31T09:47:04Z

Huh, didn't realize that socket.getCipher() calls SSL_CIPHER_get_version(), that's effectively always going to return TLSv1/SSLv3. Guess it doesn't hurt anybody but it's not very useful either.

thefourtheye · 2016-01-31T12:02:05Z

test/parallel/test-tls-getprotocol.js

+    rejectUnauthorized: false
+  }, function() {
+    assert.strictEqual(this.getProtocol(), 'TLSv1.2');
+    this.end();


Can we do the end first and then do the assertion? If there is a test failure, this will not end right?

If the assertion fails, the process dies anyway since it throws the error.

Hmmm, that makes me think of another case. Will the protocol still be the same even after the connection ends?

No, getProtocol() will return null for disconnected client sockets, just like the other get*() methods on TLS sockets.

Cool, would it be better if we asserted that as well?

mscdex · 2016-01-31T13:46:44Z

@bnoordhuis Yeah I just left the protocol property returned by getCipher() as-is for better backwards compatibility. I do agree it's not particularly useful though. Also it's kind of odd to have unrelated information like the current TLS protocol version returned with the cipher info.

thefourtheye · 2016-01-31T15:06:40Z

LGTM. Lets hear from @bnoordhuis as well.

mscdex · 2016-01-31T21:11:17Z

@bnoordhuis I added more tests. CI again: https://ci.nodejs.org/job/node-test-pull-request/1461/

bnoordhuis · 2016-01-31T21:34:21Z

LGTM

thefourtheye · 2016-01-31T22:38:54Z

doc/api/tls.markdown

+### tlsSocket.getProtocol()
+
+Returns a string containing the negotiated SSL/TLS protocol version of the
+current connection. `'unknown'` will be returned for connected sockets that have


Can this unknown case be tested?

Not very easily/reliably as it would probably have to amount to polling or maybe some kind of crazy monkey patching. Even at the stage in which an SNI callback is called, the protocol information is already available.

Oh, okay then. Since we claim that it would return unknown, I thought that it would have been better to have a test to confirm that.

This commit adds a new method for TLS sockets that returns the negotiated protocol version.

getCipher() actually includes the protocol version that the cipher was first supported and *not* the negotiated protocol of the current connection.

thefourtheye · 2016-01-31T22:58:54Z

test/parallel/test-tls-getprotocol.js

+      secureProtocol: v.secureProtocol
+    }, common.mustCall(function() {
+      assert.strictEqual(this.getProtocol(), v.version);
+      this.on('end', common.mustCall(function() {


I really wonder how this is bound to the socket object

The callback passed to tls.connect() is just added as an event handler for the socket's secureConnect event and all EventEmitter event handlers have this set to the EventEmitter instance when the event handlers are called.

I see. You are right. Thanks for clarifying :)

thefourtheye · 2016-01-31T23:14:30Z

LGTM stays if build is Green.

mscdex · 2016-01-31T23:21:30Z

CI after test tweak: https://ci.nodejs.org/job/node-test-pull-request/1464/

jasnell · 2016-02-01T17:33:11Z

LGTM

This commit adds a new method for TLS sockets that returns the negotiated protocol version. PR-URL: #4995 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

getCipher() actually includes the protocol version that the cipher was first supported and *not* the negotiated protocol of the current connection. PR-URL: #4995 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

mscdex · 2016-02-04T02:38:35Z

Landed in 2c357a7 and c41c093.

This commit adds a new method for TLS sockets that returns the negotiated protocol version. PR-URL: #4995 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

getCipher() actually includes the protocol version that the cipher was first supported and *not* the negotiated protocol of the current connection. PR-URL: #4995 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354

* buffer: - You can now supply an encoding argument when filling a Buffer Buffer#fill(string[, start[, end]][, encoding]), supplying an existing Buffer will also work with Buffer#fill(buffer[, start[, end]]). See the API documentation for details on how this works. (Trevor Norris) #4935 - Buffer#indexOf() no longer requires a byteOffset argument if you also wish to specify an encoding: Buffer#indexOf(val[, byteOffset][, encoding]). (Trevor Norris) #4803 * child_process: spawn() and spawnSync() now support a 'shell' option to allow for optional execution of the given command inside a shell. If set to true, cmd.exe will be used on Windows and /bin/sh elsewhere. A path to a custom shell can also be passed to override these defaults. On Windows, this option allows .bat. and .cmd files to be executed with spawn() and spawnSync(). (Colin Ihrig) #4598 * http_parser: Update to http-parser 2.6.2 to fix an unintentionally strict limitation of allowable header characters. (James M Snell) #5237 * dgram: socket.send() now supports accepts an array of Buffers or Strings as the first argument. See the API docs for details on how this works. (Matteo Collina) #4374 * http: Fix a bug where handling headers will mistakenly trigger an 'upgrade' event where the server is just advertising its protocols. This bug can prevent HTTP clients from communicating with HTTP/2 enabled servers. (Fedor Indutny) #4337 * net: Added a listening Boolean property to net and http servers to indicate whether the server is listening for connections. (José Moreira) #4743 * node: The C++ node::MakeCallback() API is now reentrant and calling it from inside another MakeCallback() call no longer causes the nextTick queue or Promises microtask queue to be processed out of order. (Trevor Norris) #4507 * tls: Add a new tlsSocket.getProtocol() method to get the negotiated TLS protocol version of the current connection. (Brian White) #4995 * vm: Introduce new 'produceCachedData' and 'cachedData' options to new vm.Script() to interact with V8's code cache. When a new vm.Script object is created with the 'produceCachedData' set to true a Buffer with V8's code cache data will be produced and stored in cachedData property of the returned object. This data in turn may be supplied back to another vm.Script() object with a 'cachedData' option if the supplied source is the same. Successfully executing a script from cached data can speed up instantiation time. See the API docs for details. (Fedor Indutny) #4777 * performance: Improvements in: - process.nextTick() (Ruben Bridgewater) #5092 - path module (Brian White) #5123 - querystring module (Brian White) #5012 - streams module when processing small chunks (Matteo Collina) #4354 PR-URL: #5295

This commit adds a new method for TLS sockets that returns the negotiated protocol version. PR-URL: nodejs#4995 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

getCipher() actually includes the protocol version that the cipher was first supported and *not* the negotiated protocol of the current connection. PR-URL: nodejs#4995 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

mscdex added the tls Issues and PRs related to the tls subsystem. label Jan 31, 2016

mscdex force-pushed the feature-tls-getprotocol branch from ab37728 to ecb98f4 Compare January 31, 2016 06:48

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 31, 2016

bnoordhuis reviewed Jan 31, 2016
View reviewed changes

thefourtheye reviewed Jan 31, 2016
View reviewed changes

mscdex force-pushed the feature-tls-getprotocol branch 2 times, most recently from 814578d to fb67872 Compare January 31, 2016 14:48

mscdex force-pushed the feature-tls-getprotocol branch from fb67872 to 45e57d0 Compare January 31, 2016 21:10

thefourtheye reviewed Jan 31, 2016
View reviewed changes

mscdex added 2 commits January 31, 2016 17:53

tls: add getProtocol() to TLS sockets

0c6e523

This commit adds a new method for TLS sockets that returns the negotiated protocol version.

doc: correct tlsSocket.getCipher() description

94f9b2c

getCipher() actually includes the protocol version that the cipher was first supported and *not* the negotiated protocol of the current connection.

mscdex force-pushed the feature-tls-getprotocol branch from 45e57d0 to 94f9b2c Compare January 31, 2016 22:55

thefourtheye reviewed Jan 31, 2016
View reviewed changes

mscdex closed this Feb 4, 2016

mscdex deleted the feature-tls-getprotocol branch February 4, 2016 02:44

rvagg mentioned this pull request Feb 8, 2016

Release proposal: v5.5.1 (Stable) #5141

Closed

rvagg mentioned this pull request Feb 18, 2016

Release proposal: 5.7.0 (Stable) #5295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: add getProtocol() to TLS sockets #4995

tls: add getProtocol() to TLS sockets #4995

mscdex commented Jan 31, 2016

mscdex commented Jan 31, 2016

bnoordhuis Jan 31, 2016

mscdex Jan 31, 2016

bnoordhuis Jan 31, 2016

bnoordhuis commented Jan 31, 2016

thefourtheye Jan 31, 2016

mscdex Jan 31, 2016

thefourtheye Jan 31, 2016

mscdex Jan 31, 2016

thefourtheye Jan 31, 2016

mscdex Jan 31, 2016

mscdex commented Jan 31, 2016

thefourtheye commented Jan 31, 2016

mscdex commented Jan 31, 2016

bnoordhuis commented Jan 31, 2016

thefourtheye Jan 31, 2016

mscdex Jan 31, 2016

thefourtheye Jan 31, 2016

thefourtheye Jan 31, 2016

mscdex Jan 31, 2016

thefourtheye Jan 31, 2016

thefourtheye commented Jan 31, 2016

mscdex commented Jan 31, 2016

jasnell commented Feb 1, 2016

mscdex commented Feb 4, 2016

tls: add getProtocol() to TLS sockets #4995

tls: add getProtocol() to TLS sockets #4995

Conversation

mscdex commented Jan 31, 2016

mscdex commented Jan 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented Jan 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 31, 2016

thefourtheye commented Jan 31, 2016

mscdex commented Jan 31, 2016

bnoordhuis commented Jan 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thefourtheye commented Jan 31, 2016

mscdex commented Jan 31, 2016

jasnell commented Feb 1, 2016

mscdex commented Feb 4, 2016