tls: throw an error on getLegacyCipher #14572

jasnell · 2015-04-08T22:50:58Z

throw an error on unknown getLegacyCipher input
rather than returning the default. Makes the
behavior more explicit and matches up with the
behavior on the command line switches.

jasnell · 2015-04-14T14:49:16Z

@misterdjules @mdawsonibm

misterdjules · 2015-04-14T18:23:18Z

Can we add a test to make sure that getLecagyCiphers throws/doesn't throw as expected?

per Julien's request: nodejs#14572 (comment)

jasnell · 2015-04-14T19:35:49Z

Done

per Julien's request: nodejs#14572 (comment)

misterdjules · 2015-04-14T23:20:58Z

@jasnell Do we want to throw an error on a call to getLegacyCiphers() when the first parameter passed is not a string (undefined, null or anything else that is not a string)? The error message would be different, instead of 'Error: Unknown legacy cipher list', it would be something like 'Error: getLegacyCiphers requires a string as first parameter".

Also, I think we miss the "full" documentation for the getLegacyCiphers function in the API docs, including what the parameter is, its type, etc. Currently it is only briefly mentioned in the Modifying the Default Cipher Suite section.

jasnell · 2015-04-15T02:32:17Z

@misterdjules ... PR updated to address feedback.

jasnell · 2015-04-15T03:07:32Z

#15445 @misterdjules

misterdjules · 2015-04-15T19:04:39Z

@jasnell Thank you, I think that's a significant improvement! 👍

mhdawson · 2015-04-16T13:25:29Z

One minor nit. Should the tests validate the type of the Error, ie a TypeError or an Error as appropriate ? Otherwise lgtm.

jasnell · 2015-04-22T02:55:22Z

/cc @mhdawson @misterdjules ok, please check the two additional commits. if these look good I'll squash the pr down and land.

misterdjules · 2015-04-22T17:29:23Z

test/simple/test-tls-cipher-list.js

+      // Test NODE_LEGACY_CIPHER_LIST takes precendence over --cipher-list
+      args.unshift('--cipher-list=XYZ');
+      if (env) options = {env:{"NODE_LEGACY_CIPHER_LIST": env}};
+      break;


We would probably need to have more tests than that to test the whole matrix of command line switches and environment variables. I would do that in a separate PR though, as this one is about ls: throw an error on getLegacyCipher.

misterdjules · 2015-04-22T17:31:54Z

@jasnell I think @mhdawson meant that we should check for the type of the error thrown by tls.getLegacyCiphers. I didn't see anything related to that in the latest two commits.

I would not add a lot to this PR so that we can reason about it easily and land something reasonably quickly. We can work on other aspects of ciphers list related changes in other PRs.

Thank you again very much for your work!

jasnell · 2015-04-22T17:42:47Z

@misterdjules after talking with @mhdawson today, I realize now that I missed an important aspect of your discussion with him regarding the order of precedence on the command line switches and environment variables, as well as the error type check in test test case. I'm going to make those additional changes and squash these PR's down this evening with the goal of landing them by Friday.

This commit squashes down several previous commits on this change to clean up before landing. There are several changes to the new cipher list command line switches: 1. It includes the type checking fix on getLegacyCiphers 2. It has the command line switches override the environment variables 3. It documents it order of precedence 4. Expands the test case to test for order of precedence

test that the type of error thrown is correct

jasnell · 2015-04-23T15:41:51Z

@misterdjules @mhdawson ... ok the changes discussed have been made and the multiple commits have been squashed down into two distinct commits.

Fix a minor typo in the tls.markdown and add additional precedence tests in test-tls-cipher-list

Per the feedback from Julien, refactor the test-tls-cipher-list test to avoid using the confusing switch statement. Add tests to ensure that using `--enable-legacy-cipher-list=v0.10.38` or `NODE_LEGACY_CIPHER_LIST=v0.10.38` disables the use of the default cipher list but setting the `--cipher-list` equal to the v0.10.38 list explicitly does not. Minor refactor to tls.js to ensure the above behavior.

misterdjules · 2015-05-05T00:11:30Z

lib/tls.js

@@ -1338,7 +1352,8 @@ exports.connect = function(/* [port, host], options, cb */) {
  var defaults = {
    rejectUnauthorized: '0' !== process.env.NODE_TLS_REJECT_UNAUTHORIZED
  };
-  if (DEFAULT_CIPHERS != _crypto.getLegacyCiphers('v0.10.38')) {
+  if (!usingV1038Ciphers()) {


Maybe add a comment to clarify why there's an exception for v0.10.38 here? Otherwise I'm concerned it's going to be difficult to understand a few months/years from now.

misterdjules · 2015-05-05T00:20:39Z

test/simple/test-tls-cipher-list.js

+    // test the right-most command line option takes precedence
+    doTest('XYZ',
+           [
+             '--cipher-list=XYZ',


Do we want another value for the first --cipher-list command line switch (e.g ABC)? Otherwise how do we know that the value for ciphers list was taken from the second --cipher-list and not the first one?

Yeah, actually that's just a typo... it should have been different

misterdjules · 2015-05-05T01:06:00Z

@jasnell Added a few comments, but it's definitely getting better, thank you!

Per the latest round of feedback from julien. Fix a couple of typos, add some explanatory text, refactor the test case.

misterdjules · 2015-05-07T22:53:40Z

test/simple/test-tls-cipher-list.js

@@ -19,36 +19,21 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.

-var spawn = require('child_process').spawn;
+var spawn  = require('child_process').spawn;


Style: the current style used does not align the = operator for assignment statements.

Per feedback from Julien.

jasnell · 2015-05-09T00:52:45Z

@misterdjules @mhdawson

misterdjules · 2015-05-09T00:55:16Z

@jasnell Thank you, I'll take a look on Sunday!

misterdjules · 2015-05-11T18:08:12Z

src/node_crypto.cc

@@ -4199,12 +4199,18 @@ const char* ToCString(const node::Utf8Value& value) {

 Handle<Value> DefaultCiphers(const Arguments& args) {
  HandleScope scope;
+  unsigned int len = args.Length();
+  if (len != 1 || !args[0]->IsString()) {
+    return ThrowException(Exception::TypeError(String::New("A single string parameter is required")));


Style: wrap at 80 columns.

jasnell · 2015-05-12T14:36:39Z

@misterdjules ... I'll refactor that last test to split things out more. No problem there. Will see if I can get that in this morning. Beyond that, are there any other remaining issues on this?

Refactor the default cipher list test to separate concerns a bit, make the test clearer based on Julien's feedback. Also, fix the 80-char wrap issue in the related node_crypto.cc DefaultCiphers method.

jasnell · 2015-05-12T17:18:55Z

@misterdjules

misterdjules · 2015-05-12T20:11:29Z

@jasnell I would like to come up with a good solution for #23947 too, and add support for the new command line switches in test/external/ssl-options. I can add that since I'm familiar with how this test works.

We don't have to rush it though. As we discussed during the latest weekly meeting, if we want to release v0.12.3 before these changes are ready, we can revert the work in progress in v0.12 and include them back in time for the next v0.12.4 release.

jasnell · 2015-05-12T20:13:21Z

Yes, I'm going to look at that one tonight again
On May 12, 2015 1:11 PM, "Julien Gilli" [email protected] wrote:

@jasnell https://github.com/jasnell I would like to come up with a good
solution for #23947 #23947 too, and
add support for the new command line switches in test/external/ssl-options.
I can add that since I'm familiar with how this test works.

We don't have to rush it though. As we discussed during the latest weekly
meeting, if we want to release v0.12.3 before these changes are ready, we
can revert the work in progress in v0.12 and include them back in time for
the next v0.12.4 release.

—
Reply to this email directly or view it on GitHub
#14572 (comment).

misterdjules · 2015-06-22T23:46:14Z

Moving to milestone v0.10.40, as v0.10.39 was released today.

misterdjules · 2015-06-30T22:44:39Z

Closing in favor of #25603.

tjfontaine added the in progress label Apr 8, 2015

Nodejs-Jenkins added pr v0.10 and removed in progress labels Apr 8, 2015

misterdjules added this to the 0.10.39 milestone Apr 14, 2015

misterdjules added the crypto label Apr 14, 2015

jasnell added a commit to jasnell/node-joyent that referenced this pull request Apr 14, 2015

test: add test to ensure getCipherList throws appropriately

faab984

per Julien's request: nodejs#14572 (comment)

jasnell added a commit to jasnell/node-joyent that referenced this pull request Apr 14, 2015

test: add test to ensure getCipherList throws appropriately

8496600

per Julien's request: nodejs#14572 (comment)

jasnell added a commit to jasnell/node-joyent that referenced this pull request Apr 14, 2015

test: add test to ensure getCipherList throws appropriately

276cdd5

per Julien's request: nodejs#14572 (comment)

jasnell force-pushed the v0.10-getLegacyCiphers branch from 80a485d to b068eb6 Compare April 15, 2015 02:49

misterdjules reviewed Apr 22, 2015
View reviewed changes

jasnell added 2 commits April 23, 2015 08:34

test: test-tls-cipher-list test error type

b450336

test that the type of error thrown is correct

jasnell force-pushed the v0.10-getLegacyCiphers branch from 0f02481 to b450336 Compare April 23, 2015 15:40

jasnell added 2 commits April 28, 2015 09:14

test: fix typo in tls.markdown doc and additional tests

2d1d2a0

Fix a minor typo in the tls.markdown and add additional precedence tests in test-tls-cipher-list

misterdjules reviewed May 5, 2015
View reviewed changes

test: additional refinements in the test

7f4d098

Per the latest round of feedback from julien. Fix a couple of typos, add some explanatory text, refactor the test case.

misterdjules reviewed May 7, 2015
View reviewed changes

test: additional round of edits on the test-tls-cipher-list.js

30f46c3

Per feedback from Julien.

jasnell force-pushed the v0.10-getLegacyCiphers branch from d9c9658 to 30f46c3 Compare May 9, 2015 00:48

misterdjules mentioned this pull request May 11, 2015

win,msi: broadcast WM_SETTINGCHANGE after install #25100

Closed

misterdjules reviewed May 11, 2015
View reviewed changes

test: refactor default cipher list test

8a61f11

Refactor the default cipher list test to separate concerns a bit, make the test clearer based on Julien's feedback. Also, fix the 80-char wrap issue in the related node_crypto.cc DefaultCiphers method.

jasnell mentioned this pull request May 22, 2015

[Converge] --cipher-list and --enable-legacy-cipher-list Command Line Switch nodejs/node-convergence-archive#39

Closed

misterdjules mentioned this pull request Jun 18, 2015

tls: revert disable RC4 and cipher lists changes #25511

Closed

misterdjules modified the milestones: 0.10.40, 0.10.39 Jun 22, 2015

misterdjules mentioned this pull request Jun 30, 2015

Deprecate RC4 ciphers from the default ciphers list #25603

Closed

misterdjules closed this Jun 30, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: throw an error on getLegacyCipher #14572

tls: throw an error on getLegacyCipher #14572

jasnell commented Apr 8, 2015

jasnell commented Apr 14, 2015

misterdjules commented Apr 14, 2015

jasnell commented Apr 14, 2015

misterdjules commented Apr 14, 2015

jasnell commented Apr 15, 2015

jasnell commented Apr 15, 2015

misterdjules commented Apr 15, 2015

mhdawson commented Apr 16, 2015

jasnell commented Apr 22, 2015

misterdjules Apr 22, 2015

misterdjules commented Apr 22, 2015

jasnell commented Apr 22, 2015

jasnell commented Apr 23, 2015

misterdjules May 5, 2015

misterdjules May 5, 2015

jasnell May 5, 2015

misterdjules commented May 5, 2015

misterdjules May 7, 2015

jasnell commented May 9, 2015

misterdjules commented May 9, 2015

misterdjules May 11, 2015

jasnell commented May 12, 2015

jasnell commented May 12, 2015

misterdjules commented May 12, 2015

jasnell commented May 12, 2015

misterdjules commented Jun 22, 2015

misterdjules commented Jun 30, 2015

tls: throw an error on getLegacyCipher #14572

tls: throw an error on getLegacyCipher #14572

Conversation

jasnell commented Apr 8, 2015

jasnell commented Apr 14, 2015

misterdjules commented Apr 14, 2015

jasnell commented Apr 14, 2015

misterdjules commented Apr 14, 2015

jasnell commented Apr 15, 2015

jasnell commented Apr 15, 2015

misterdjules commented Apr 15, 2015

mhdawson commented Apr 16, 2015

jasnell commented Apr 22, 2015

misterdjules Apr 22, 2015

Choose a reason for hiding this comment

misterdjules commented Apr 22, 2015

jasnell commented Apr 22, 2015

jasnell commented Apr 23, 2015

misterdjules May 5, 2015

Choose a reason for hiding this comment

misterdjules May 5, 2015

Choose a reason for hiding this comment

jasnell May 5, 2015

Choose a reason for hiding this comment

misterdjules commented May 5, 2015

misterdjules May 7, 2015

Choose a reason for hiding this comment

jasnell commented May 9, 2015

misterdjules commented May 9, 2015

misterdjules May 11, 2015

Choose a reason for hiding this comment

jasnell commented May 12, 2015

jasnell commented May 12, 2015

misterdjules commented May 12, 2015

jasnell commented May 12, 2015

misterdjules commented Jun 22, 2015

misterdjules commented Jun 30, 2015