tls: add getProtocol() to TLS sockets#4995
Closed
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
mscdexforce-pushed the feature-tls-getprotocol branch from ab37728 to ecb98f4Compare
ContributorAuthor
mscdex commented Jan 31, 2016
targos added the semver-minor 
Member
There was a problem hiding this comment.
Can you try two or three protocol versions here?
ContributorAuthor
There was a problem hiding this comment.
Do you mean 2 or 3 separate connections each with different secureProtocol settings?
Member
bnoordhuis commented Jan 31, 2016
Huh, didn't realize that |
Contributor
There was a problem hiding this comment.
Can we do the end first and then do the assertion? If there is a test failure, this will not end right?
ContributorAuthor
There was a problem hiding this comment.
If the assertion fails, the process dies anyway since it throws the error.
Contributor
There was a problem hiding this comment.
Hmmm, that makes me think of another case. Will the protocol still be the same even after the connection ends?
ContributorAuthor
There was a problem hiding this comment.
No, getProtocol() will return null for disconnected client sockets, just like the other get*() methods on TLS sockets.
Contributor
There was a problem hiding this comment.
Cool, would it be better if we asserted that as well?
ContributorAuthor
mscdex commented Jan 31, 2016
@bnoordhuis Yeah I just left the |
mscdexforce-pushed the feature-tls-getprotocol branch 2 times, most recently from 814578d to fb67872Compare
Contributor
thefourtheye commented Jan 31, 2016
LGTM. Lets hear from @bnoordhuis as well. |
mscdexforce-pushed the feature-tls-getprotocol branch from fb67872 to 45e57d0CompareContributorAuthor
mscdex commented Jan 31, 2016
@bnoordhuis I added more tests. CI again: https://ci.nodejs.org/job/node-test-pull-request/1461/ |
Member
bnoordhuis commented Jan 31, 2016
LGTM |
Contributor
There was a problem hiding this comment.
Can this unknown case be tested?
ContributorAuthor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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.
mscdexforce-pushed the feature-tls-getprotocol branch from 45e57d0 to 94f9b2cCompareContributor
There was a problem hiding this comment.
I really wonder how this is bound to the socket object
ContributorAuthor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
I see. You are right. Thanks for clarifying :)
Contributor
thefourtheye commented Jan 31, 2016
LGTM stays if build is Green. |
ContributorAuthor
mscdex commented Jan 31, 2016
CI after test tweak: https://ci.nodejs.org/job/node-test-pull-request/1464/ |
Member
jasnell commented Feb 1, 2016
LGTM |
mscdex added a commit that referenced this pull request Feb 4, 2016
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]>
mscdex added a commit that referenced this pull request Feb 4, 2016
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]>
ContributorAuthor
mscdex commented Feb 4, 2016
rvagg pushed a commit that referenced this pull request Feb 10, 2016
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]>
rvagg pushed a commit that referenced this pull request Feb 10, 2016
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]>
rvagg added a commit that referenced this pull request Feb 21, 2016
* 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
rvagg added a commit that referenced this pull request Feb 21, 2016
* 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
rvagg added a commit that referenced this pull request Feb 23, 2016
* 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
rvagg added a commit that referenced this pull request Feb 24, 2016
* 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
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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]>
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.