Questions about recent changes in ciphers list

[misterdjules]

We have made some changes to the way default ciphers list are handled recently, and I have two questions about the current implementation:

1. Why does tls.connect do not define default.ciphers when DEFAULT_CIPHERS === _crypto.getLegacyCiphers('v0.10.38')? (As a side note, I think we want to use !== on that line). That makes me think that we may want to add support for the --enable-legacy-cipher-list and --cipher-list options to test/external/ssl-options to make sure that node behaves as we expect in this regard. We've had some interesting surprises in the past when running these tests.
2. When using both --cipher-list and --enable-legacy-cipher-list command line options, users are not warned about potential misuse. For instance, I can run node with:

./node --enable-legacy-cipher-list=v0.10.38 --cipher-list=foo

and think that foo will be the default ciphers list, but in fact it will be v0.10.38's default cipher list.
Basically, it seems that using --enable-legacy-cipher-list always overwrites any usage of --cipher-list. It doesn't seem to be mentioned in the documentation, and I think that if the two are mutually exclusive, node should exit with an error and not continue silently.

/cc @jasnell

[jasnell]

re: not setting default.ciphers when the DEFAULT_CIPHERS is equal to v0.10.38 in order to preserve the default behavior in v0.10.38. Before this change, the default.ciphers in v0.10.38 and before was not being set. The thought was to make sure that setting the legacy ciphers back to that default would revert to the original v0.10.38 behavior completely.

I can definitely add more documentation around this. Yes, --enable-legacy-cipher-list completely overrides the --cipher-list option. Those do not make any sense to use together.

[mhdawson]

Echoing the comment about not setting default ciphers on the client when we revert to v0.10.38. We wanted to make sure users could get back to the exact same behavior as in v0.10.38 and this seemed to be a reasonable way to allow that without overly complicating the command line options.

[misterdjules]

@jasnell @mdawsonibm Thank you for the clarification!

Not setting the default ciphers for tls.connect (and thus for https.request and https.get) seems like a security issue we'd like to fix, even when --enable-legacy-cipher-list=v0.10.38 is passed. I understand this could break some applications, but these applications can be fixed by specifying custom ciphers suite. What do you think?

If it does not make sense to use --enable-legacy-cipher-list and --cipher-list together, is it sensible to throw or exit with an error in that case?

[jasnell]

Possibly, but the current way that options are parsed vs. fallback on the environment variables makes that a bit tricky. You end up having to follow the bouncing ball a little bit. One item on my todo list is to slightly rework the way command line options are parsed in order to make this easier to manage. For now, I've documented the order of precedence on the options to make it clear exactly how they relate to one another.

[jasnell]

As for setting the default ciphers on tls.connect, I'm not sure. We address the security bug in v0.10.39 and beyond so we're covered there. The semantics of the --enable-legacy-cipher-list switch is to go back to the behavior in a specific version just in case your application breaks. Reverting the cipher list opens you up to security issues anyway so I don't see this as a significant issue.

[misterdjules]

Good points. I'm still not comfortable with not fixing it when using --enable-legacy-cipher-list=v0.1038. I would like to gather the opinion of @joyent/node-coreteam on this issue before we move forward with this.

[jasnell]

Btw, so it's clear, I will be squashing the multiple commits before
landing. :)
On Apr 15, 2015 12:24 PM, "Julien Gilli" [email protected] wrote:

Good points. I'm still not comfortable with not fixing it when using
--enable-legacy-cipher-list=v0.1038. I would like to gather the opinion
of @joyent/node-coreteam
https://github.com/orgs/joyent/teams/node-coreteam on this issue before
we move forward with this.

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

[misterdjules]

Regarding conflicting ciphers list environment variables/command line options, I created a gist that adds some checks and exits with an error message when conflicting options are used.

Here's the output for most of the use cases I had in mind with these changes:

➜  v0.12 git:(v0.10) ✗ ./node --enable-legacy-cipher-list=v0.10.38 --cipher-list=foo
--cipher-list cannot be used with --enable-legacy-cipher-list
➜  v0.12 git:(v0.10) ✗ ./node --cipher-list=foo --enable-legacy-cipher-list=v0.10.38
--enable-legacy-cipher-list cannot be used with --cipher-list
➜  v0.12 git:(v0.10) ✗ NODE_CIPHER_LIST=v0.10.38 ./node --enable-legacy-cipher-list=v0.10.38
NODE_CIPHER_LIST cannot be set when other cipher lists options are used
➜  v0.12 git:(v0.10) ✗ NODE_LEGACY_CIPHER_LIST=v0.10.38 ./node --cipher-list=foo            
NODE_LEGACY_CIPHER_LIST cannot be set when other cipher lists options are used
➜  v0.12 git:(v0.10) ✗ NODE_LEGACY_CIPHER_LIST=v0.10.38 NODE_CIPHER_LIST='foo' ./node                  
NODE_LEGACY_CIPHER_LIST cannot be set when other cipher lists options are used
➜  v0.12 git:(v0.10) ✗ NODE_CIPHER_LIST='foo' NODE_LEGACY_CIPHER_LIST=v0.10.38 ./node
NODE_LEGACY_CIPHER_LIST cannot be set when other cipher lists options are used
➜  v0.12 git:(v0.10) ✗ NODE_LEGACY_CIPHER_LIST=v0.10.39 ./node --enable-legacy-cipher-list=v0.10.38
NODE_LEGACY_CIPHER_LIST cannot be set when other cipher lists options are used
➜  v0.12 git:(v0.10) ✗ NODE_CIPHER_LIST='foo' ./node --cipher-list='bar'                 
NODE_CIPHER_LIST cannot be set when other cipher lists options are used
➜  v0.12 git:(v0.10) ✗

@jasnell What do you think?

EDIT: The gist is a proof of concept, not necessarily something we'd like to commit exactly as is.

[misterdjules]

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