tls: add getProtocol() to TLS sockets by mscdex · Pull Request #4995 · nodejs/node · GitHub

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.

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

Can you try two or three protocol versions here?

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

Correct.

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.

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?

Done

@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.

LGTM. Lets hear from @bnoordhuis as well.

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

LGTM

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.

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 :)

LGTM stays if build is Green.

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

LGTM

This commit adds a new method for TLS sockets that returns the negotiated protocol version. PR-URL: #4995 Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

* 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 <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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 <thechargingvolcano@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>

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

tls: add getProtocol() to TLS sockets by mscdex · Pull Request #4995 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: add getProtocol() to TLS sockets - #4995

tls: add getProtocol() to TLS sockets#4995mscdex wants to merge 2 commits intonodejs:masternodejs/node:masterfrom mscdex:feature-tls-getprotocolmscdex/io.js:feature-tls-getprotocolCopy head branch name to clipboard

Conversation

mscdex commented Jan 31, 2016

Uh oh!

mscdex commented Jan 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jan 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jan 31, 2016

Uh oh!

thefourtheye commented Jan 31, 2016

Uh oh!

mscdex commented Jan 31, 2016

Uh oh!

bnoordhuis commented Jan 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thefourtheye commented Jan 31, 2016

Uh oh!

mscdex commented Jan 31, 2016

Uh oh!

jasnell commented Feb 1, 2016

Uh oh!

mscdex commented Feb 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tls: add getProtocol() to TLS sockets#4995
mscdex wants to merge 2 commits into
nodejs:masterfrom
mscdex:feature-tls-getprotocol