Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

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

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 2, 2015

Disable RC4 in the default cipher list

Add the --cipher-list command line switch and NODE_CIPHER_LIST
environment variable to completely override the default cipher list.

Add the --enable-legacy-cipher-list and NODE_LEGACY_CIPHER_LIST
environment variable to selectively enable the default cipher list from
previous node.js releases.

(Targets v0.12.2-release)

Disable RC4 in the default cipher list

Add the `--cipher-list` command line switch and `NODE_CIPHER_LIST`
environment variable to completely override the default cipher list.

Add the `--enable-legacy-cipher-list` and `NODE_LEGACY_CIPHER_LIST`
environment variable to selectively enable the default cipher list from
previous node.js releases.
@jasnell jasnell added this to the 0.12.3 milestone Apr 2, 2015
@jasnell jasnell self-assigned this Apr 2, 2015
@jasnell jasnell added the P-1 label Apr 2, 2015
@jasnell jasnell changed the title disable RC4, add --cipher-list command line switch tls: disable RC4, add --cipher-list command line switch Apr 2, 2015
@Fishrock123
Copy link

Why not just use one PR? You can keep pushing to it without creating extra noise..

@jasnell
Copy link
Member Author

jasnell commented Apr 2, 2015

There are three separate PR's for three separate branches (v0.10.38,
v0.12.2 and Master).

On Thu, Apr 2, 2015 at 4:35 PM, Jeremiah Senkpiel notifications@github.com
wrote:

Why not just use one PR? You can keep pushing to it without creating extra
noise..


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

@Fishrock123
Copy link

Ah, makes sense, but then again, isn't it usually just merged forwards from the latest (0.10.x in this case)?

@jasnell
Copy link
Member Author

jasnell commented Apr 3, 2015

Typically yes. In this case there are key differences in the patches that
make that a bit difficult to review. Separating them out like this allows
the changes to be reviewed in isolation. If the changes look good, I'll
work with Julian to land the changes in whatever way makes the most sense.
On Apr 2, 2015 5:02 PM, "Jeremiah Senkpiel" notifications@github.com
wrote:

Ah, makes sense, but then again, isn't it usually just merged forwards
from the latest (0.10. in this case)?


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

Per feedback on the commit, make the PrintHelp output for
--enable-legacy-cipher-list less verbose.
@mhdawson
Copy link
Member

mhdawson commented Apr 7, 2015

Looks consistent with 0.10.38 and where different makes sense. Will need to squash the two commits and shorten the title to fit under the 50 char limit. Otherwise lgtm.

@@ -3047,6 +3054,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) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paraphrasing my previous comment for the corresponding PR targeted to v0.10.x:

I'm wondering if we need to support a --cipher-list command line option in v0.12.x. Adding support for --enable-legacy-cipher-list already breaks the contract of not adding additional API in patch releases, and it seems sufficient to handle the use case of users not wanting to use the new default ciphers list.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intent on introducing --cipher-list in v0.10.x and v0.12.x is to avoid
the possibility of having to make these kinds of changes in the future. If,
for instance, another one of the default ciphers comes under scrutiny and
we have to switch it off, rather than having to bump the maintenance
release again we can simply tell folks to use the command line switch or
envar to override the list entirely and go from there.

On Tue, Apr 7, 2015 at 5:29 PM, Julien Gilli notifications@github.com
wrote:

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

@@ -3047,6 +3054,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) {

Paraphrasing my previous comment
#14413 (comment) for the
corresponding PR targeted to v0.10.x:

I'm wondering if we need to support a --cipher-list command line option
in v0.12.x. Adding support for --enable-legacy-cipher-list already breaks
the contract of not adding additional API in patch releases, and it seems
sufficient to handle the use case of users not wanting to use the new
default ciphers list.


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

@misterdjules
Copy link

@jasnell Same comments as in #14383, plus #14414 (comment).

Thank you very much again for doing this work 👍

jasnell added a commit that referenced this pull request Apr 8, 2015
Disable RC4 in the default cipher list

Add the `--cipher-list` command line switch and `NODE_CIPHER_LIST`
environment variable to completely override the default cipher list.

Add the `--enable-legacy-cipher-list` and `NODE_LEGACY_CIPHER_LIST`
environment variable to selectively enable the default cipher list from
previous node.js releases.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #14414
jasnell added a commit that referenced this pull request Apr 8, 2015
Per feedback on the commit, make the PrintHelp output for
--enable-legacy-cipher-list less verbose.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #14414
@jasnell
Copy link
Member Author

jasnell commented Apr 8, 2015

Landed in #f9291a9 and #b5737bb

@jasnell jasnell closed this Apr 8, 2015
jasnell added a commit to jasnell/node-joyent that referenced this pull request May 18, 2015
Disable RC4 in the default cipher list

Add the `--cipher-list` command line switch and `NODE_CIPHER_LIST`
environment variable to completely override the default cipher list.

Add the `--enable-legacy-cipher-list` and `NODE_LEGACY_CIPHER_LIST`
environment variable to selectively enable the default cipher list from
previous node.js releases.

Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#14414
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants