tls: Update cipher-list defaults, add command-line switches #14383

jasnell · 2015-04-01T18:50:13Z

Pull over the io.js default cipher list updates
Adds new --cipher-list and --enable-legacy-cipher-list command line switches.
--cipher-list allows a new default cipher list to be specified
--enable-legacy-cipher-list tells node to use specific older cipher lists
Adds new NODE_CIPHER_LIST and NODE_LEGACY_CIPHER_LIST
environment variables as alternatives to the command line switches
Adds a new getLegacyCiphers method to the TLS module
A test case is included.

Port of io.js commit: nodejs/node@77f3586 Original commit message: This updates the default cipher suite to an more secure list, which prefers strong ciphers with Forward Secrecy. Additionally, it enables `honorCipherOrder` by default. Noteable effect of this change is that the insecure RC4 ciphers are disabled and that Chrome negotiates a more secure ECDHE cipher. Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> PR-URL: nodejs/node#826

jasnell · 2015-04-02T20:40:29Z

(This is just for master... separate PR's will be coming for backporting changes into
v.0.12.x and v0.10.x)

/cc @joyent/node-coreteam

jasnell · 2015-04-02T22:19:01Z

There will be just a couple more updates on this coming soon.. just a few minor late breaking tweaks...

Add command line switches and environment variables to override the default cipher suite in tls.js `--cipher-list` and `NODE_CIPHER_LIST` can be used to completely override the default cipher list with a given value. `--enable-legacy-cipher-list` and `NODE_LEGACY_CIPHER_LIST` can be used to reset the default cipher list back to a known legacy value shipped in prior Node.js releases A new `getLegacyCiphers` method on the tis module allows programmatic access to the old cipher list defaults.

trevnorris · 2015-04-03T20:25:08Z

src/node.cc

+         "                       set to v0.10.38 to use the v0.10.38 list,\n"
+         "                       set to v0.10.39 to use the v0.10.39 list.\n"
+         "                       set to v0.12.2 to use the v0.12.2 list.\n"
+         "                       set to v0.12.3 to use the v0.12.3 list.\n"


I think it would be enough to simply list possible options. users could probably figure out from context that each one corresponds to a given version.

;) one hopes, yes.
On Apr 3, 2015 1:25 PM, "Trevor Norris" [email protected] wrote:

In src/node.cc
#14383 (comment):

@@ -2929,6 +2929,12 @@ static void PrintHelp() {
#endif
" --enable-ssl2 enable ssl2\n"
" --enable-ssl3 enable ssl3\n"

" --cipher-list=val specify the default TLS cipher list\n"

" --enable-legacy-cipher-list=val \n"

" set to v0.10.38 to use the v0.10.38 list,\n"

" set to v0.10.39 to use the v0.10.39 list.\n"

" set to v0.12.2 to use the v0.12.2 list.\n"

" set to v0.12.3 to use the v0.12.3 list.\n"

I think it would be enough to simply list possible options. users could
probably figure out from context that each one corresponds to a given
version.

—
Reply to this email directly or view it on GitHub
https://github.com/joyent/node/pull/14383/files#r27753373.

trevnorris · 2015-04-03T20:27:51Z

Few small things, but looking good.

Based on commit feedback, make the PrintHelp for --enable-legacy-cipher-list less verbose.

Per the commit feedback, fix up style nits and pass in the isolate with the NODE_DEFINE_STRING_CONSTANT macro.

mhdawson · 2015-04-07T12:35:08Z

Reviewed for consistency with 0.12 and 0.10 patches looks good. Differences between them also make sense. This looks like changes from original io.js patch plus the changes for 0.10.X and 0.12.X. Will need to squash and ensure final commit comment is less than 50 char limit but otherwise lgtm.

misterdjules · 2015-04-07T23:02:50Z

src/node.cc

@@ -3040,6 +3047,20 @@ static void ParseArgs(int* argc,
    } else if (strcmp(arg, "--v8-options") == 0) {
      new_v8_argv[new_v8_argc] = "--help";
      new_v8_argc += 1;
+    } else if (strncmp(arg, "--cipher-list=", 14) == 0) {


Could this be done in src/node.js instead of in src/node.cc? It seems it would just be less error prone and shorter.

misterdjules · 2015-04-07T23:25:40Z

@jasnell Added some comments, sorry again for taking so long in reviewing this!

misterdjules · 2015-04-08T00:00:39Z

@jasnell Running the tests suite in test/external/ssl-options/test.js with these changes now, I'll keep you posted about the results.

misterdjules · 2015-04-08T00:41:54Z

@jasnell All tests in test/external/ssl-options/test.js pass.

jasnell · 2015-04-08T00:45:33Z

+1... my time tonight is disappearing quick so I will work on landing these
in the morning. Thank you very much for the additional review Julien. It's
very helpful

On Tue, Apr 7, 2015 at 5:42 PM, Julien Gilli [email protected]
wrote:

@jasnell https://github.com/jasnell All tests in
test/external/ssl-options/test.js pass.

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

typo and unnecessary options init

Port of io.js commit: nodejs/node@77f3586 Original commit message: This updates the default cipher suite to an more secure list, which prefers strong ciphers with Forward Secrecy. Additionally, it enables `honorCipherOrder` by default. Noteable effect of this change is that the insecure RC4 ciphers are disabled and that Chrome negotiates a more secure ECDHE cipher. Reviewed-By: James M Snell <[email protected]> PR-URL: #14383

Add command line switches and environment variables to override the default cipher suite in tls.js `--cipher-list` and `NODE_CIPHER_LIST` can be used to completely override the default cipher list with a given value. `--enable-legacy-cipher-list` and `NODE_LEGACY_CIPHER_LIST` can be used to reset the default cipher list back to a known legacy value shipped in prior Node.js releases A new `getLegacyCiphers` method on the tis module allows programmatic access to the old cipher list defaults. Reviewed-By: James M Snell <[email protected]> PR-URL: #14383

Based on commit feedback, make the PrintHelp for --enable-legacy-cipher-list less verbose. Reviewed-By: James M Snell <[email protected]> PR-URL: #14383

Per the commit feedback, fix up style nits and pass in the isolate with the NODE_DEFINE_STRING_CONSTANT macro. Reviewed-By: James M Snell <[email protected]> PR-URL: #14383

typo and unnecessary options init Reviewed-By: James M Snell <[email protected]> PR-URL: #14383

jasnell · 2015-04-08T20:33:46Z

Landed

Nodejs-Jenkins added pr master labels Apr 1, 2015

tjfontaine added in progress and removed master pr labels Apr 1, 2015

Nodejs-Jenkins added pr master labels Apr 1, 2015

jasnell added this to the 0.13.1 milestone Apr 1, 2015

jasnell self-assigned this Apr 1, 2015

jasnell force-pushed the tls-update-cipher-suite-with-switches branch from b72acf0 to 025dd75 Compare April 2, 2015 20:36

jasnell force-pushed the tls-update-cipher-suite-with-switches branch from 025dd75 to 008ccd9 Compare April 2, 2015 20:52

jasnell force-pushed the tls-update-cipher-suite-with-switches branch from 008ccd9 to 12c3a57 Compare April 2, 2015 23:21

trevnorris reviewed Apr 3, 2015
View reviewed changes

jasnell added 2 commits April 6, 2015 11:39

tls: make --enable-legacy-cipher-list less verbose

9329464

Based on commit feedback, make the PrintHelp for --enable-legacy-cipher-list less verbose.

tls: pass in isolate with define string constant + style nits

09160f5

Per the commit feedback, fix up style nits and pass in the isolate with the NODE_DEFINE_STRING_CONSTANT macro.

misterdjules reviewed Apr 7, 2015
View reviewed changes

misterdjules mentioned this pull request Apr 7, 2015

tls: more secure defaults #14194

Closed

This was referenced Apr 8, 2015

tls: disable RC4, add --cipher-list command line switch #14413

Closed

tls: disable RC4, add --cipher-list command line switch #14414

Closed

test: fixing a few nits in the test

a9f8196

typo and unnecessary options init

jasnell added a commit that referenced this pull request Apr 8, 2015

tls: make --enable-legacy-cipher-list less verbose

3f58ce6

Based on commit feedback, make the PrintHelp for --enable-legacy-cipher-list less verbose. Reviewed-By: James M Snell <[email protected]> PR-URL: #14383

jasnell added a commit that referenced this pull request Apr 8, 2015

test: fixing a few nits in the test

4d9c81b

typo and unnecessary options init Reviewed-By: James M Snell <[email protected]> PR-URL: #14383

jasnell closed this Apr 8, 2015

tjfontaine removed the in progress label Apr 8, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: Update cipher-list defaults, add command-line switches #14383

tls: Update cipher-list defaults, add command-line switches #14383

jasnell commented Apr 1, 2015

jasnell commented Apr 2, 2015

jasnell commented Apr 2, 2015

trevnorris Apr 3, 2015

jasnell Apr 3, 2015

trevnorris commented Apr 3, 2015

mhdawson commented Apr 7, 2015

misterdjules Apr 7, 2015

misterdjules commented Apr 7, 2015

misterdjules commented Apr 8, 2015

misterdjules commented Apr 8, 2015

jasnell commented Apr 8, 2015

jasnell commented Apr 8, 2015

tls: Update cipher-list defaults, add command-line switches #14383

tls: Update cipher-list defaults, add command-line switches #14383

Conversation

jasnell commented Apr 1, 2015

jasnell commented Apr 2, 2015

jasnell commented Apr 2, 2015

trevnorris Apr 3, 2015

Choose a reason for hiding this comment

jasnell Apr 3, 2015

Choose a reason for hiding this comment

trevnorris commented Apr 3, 2015

mhdawson commented Apr 7, 2015

misterdjules Apr 7, 2015

Choose a reason for hiding this comment

misterdjules commented Apr 7, 2015

misterdjules commented Apr 8, 2015

misterdjules commented Apr 8, 2015

jasnell commented Apr 8, 2015

jasnell commented Apr 8, 2015