@shigeki @jasnell can you two take a look at this PR

LGTM

Ran test suite locally. 1 failing (but unrelated) test in make test. make test-addons ran cleanly

This is a patch that prevents a security misconfiguration, IMO that makes it not a semver-major.

Technically it is semver-major but it is one that should have minimal impact. The introduction of the new command line switch to revert to the prior behavior is also significant. That said, this would land in v0.12.x which is in a weird place in semver-land anyway.

LGTM

I'm not really happy about the --revert-dhe-server-limit commandline argument here and I think I'd prefer to make it something you can't change than introduce another API we have to care about into the future (commandline argument as an API). If we must have the argument then perhaps something more specific that doesn't use the term "revert", --allow-insecure-server-dhparam.

Also, I'd vote for semver-major, we can argue this is a bug or misconfiguration but in the spirit of the conservative approach to semver we've been taking we should hold off till the next major.

And, /cc @nodejs/crypto

@rvagg The only setups this is going to break is those that have been dangerously misconfigured. And they still have a way to get back the old behaviour. It looks like a sane thing to do.

An alternative non-breaking approach would be to reduce this to a big warning instead of an Error. But given that this part of code is generally one of the first things that the server does — anyone should notice the error immediately after the version update on the test setup.

@rvagg .. note that this does not print the --revert-dhe-server-limit in the node --help output or document it in any way. The intent would be for it to be supported strictly in the LTS branches and not something that would be supported going forward. The only place it would be documented is in the release notes as an indication that if this change breaks you, here's the workaround we're providing... which by the way is only supported in these specific LTS lines.

I'm good with --allow-insecure-server-dhparam as a name, tho.

oh, I see this is for 0.12, we are doing the right thing in v4+ I take it then?

@mhdawson , correct me if I'm wrong, but this fix would end up needing to go out in v4.x also (unfortunately). In v4.x it would be a semver-minor bump

I leave the decision of a new option name to native English speakers. I have no preference.

@mhdawson I have two comments here.

• Please add a rule of dh512.pem to Makefile as in https://github.com/nodejs/node/blob/master/test/fixtures/keys/Makefile#L261-L263
• There are not descriptions regarding to the new option. It should be noted in src/node.cc:PrintHelp(), doc/node.1 and doc/api/tls.markdown.

@jasnell It's already in v4.x with 1024 bits limit at https://github.com/nodejs/node/blob/v4.x/src/node_crypto.cc#L800-L806 . Do not be worry :-)

Note: this is at least semver-minor, because as @bnoordhuis mentioned recently, downgrading while using this option will cause an error.

@shigeki ... ah, that's right.. so hard to keep track sometimes :-/

@shigeki I added the requested change to the Makefile for the test fixtures.

In terms of documenting this the discussion I had with James was that we'd prefer not to add to the regular documentation. We don't want anybody to use it and it will not be in any later release. Instead we planned to just document it in the migration notes so that people are aware of it if they need to use it when upgrading but otherwise are less likely to use it. I think Jame's comment above echo's the same approach.

Does that make sense to you or do you still think we need to add it to printHelp() and tls.markdown ?

@rvagg updated to address wording comments and change name of the option

@mhdawson I'm okay for no doc. Someone in the future would have the same question as mine so please write its description in the commit log. Otherwise, LGTM after CI is fine.

Updated based on shigeki's comment to add to the commit comment and squashed.

Changes LGTM. Disregard my previous two comments, keep forgetting this is v0.12.

CI run here https://ci.nodejs.org/job/node-test-pull-request/781/

CI run is green on all but windows. Looking at earlier failures most of them are pre-existing. The only one that worries me a bit is "not ok 721 - test-tls-ticket-cluster.js" which failed 2/3 times and which I don't see in the previous run, going to start another CI run to see how many times it recreates. Don't think its related as I'd expect any failure to be 100% but more data can't hurt https://ci.nodejs.org/job/node-test-pull-request/782/

If that continues to fail, may want to pass it through the new stress test job in CI to see if it's flaky

From test https://github.com/nodejs/node/blob/v0.12/test/simple/test-tls-ticket-cluster.js
var keyFile = join(common.fixturesDir, 'agent.key');
var certFile = join(common.fixturesDir, 'agent.crt');
From https://github.com/nodejs/node/blob/v0.12/test/fixtures/agent.key

-----BEGIN RSA PRIVATE KEY-----
MIIEowIBAAKCAQEAz+LXZOjcQCJq3+ZKUFabj71oo/ex/XsBcFqtBThjjTw9CVEV

So looks to be an RSA key, don't see how that could be related to limiting DHE key size

No recreates in second run, only failures commonly seen on windows so seems unrelated. I think we are good to go CI run wise

@rvagg ... can we get one more review from you on this before landing?

I'm not entirely comfortable with squeezing this in to v0.12 cause of the breaking nature of it but I'll throw in an lgtm for the sake of secure-by-default and the low likelihood of anyone being impacted.

@mhdawson test-tls-ticket-cluster.js has nothing to do with DHE key exchange. Probably it was a timing issue in process fork and die.

FYI. Google is planning to deprecate DHE in Chrome because they consider that 1024bits DHE has not enough security and moving to ECDHE is better.
"Intent to deprecate: DHE-based cipher suites" https://groups.google.com/a/chromium.org/d/msg/security-dev/dYyhKHPnrI0/pNxx8vTKBAAJ

@rvagg ... noted, I have the same concerns but overall, LGTM. @mhdawson , can you go ahead and get this landed in v0.12-staging?

I'm not happy with adding another seldom used command line switch, but if you go ahead, please at add it to node.1 and the --help print, so it's properly documented.

not documenting it is intentional. This switch would only exist in v0.12
and is only for people who absolutely need it. There is no intent to
support it in any other version. The documentation would be the release
notes. By not putting it into any other documentation, we are not
committing to support it beyond the v0.12 branch
On Nov 22, 2015 12:15 PM, "silverwind" notifications@github.com wrote:

I'm not happy with adding another seldom used command line switch, but if
you go ahead, please at add it to the man page and the --help print, so
it's properly documented.

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

@jasnell alright, carry on!

I was out Friday afternoon so just see the comment from James to land, will do that now

landed caa16b4