tls: added ECDH ciphers support #5854
Conversation
|
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. Commit erikdubbelboer/node@93884e6 has the following error(s):
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
| @@ -443,6 +445,9 @@ Server.prototype.setOptions = function(options) { | |||
| if (options.secureProtocol) this.secureProtocol = options.secureProtocol; | |||
| if (options.crl) this.crl = options.crl; | |||
| if (options.ciphers) this.ciphers = options.ciphers; | |||
| if (typeof options.ecdhCurve !== 'undefined') { | |||
| this.ecdhCurve = options.ecdhCurve; | |||
| } | |||
There was a problem hiding this comment.
if this is too long for a one liner, then cool dropping it to the next, but remove the {}
|
they'll need to be squashed, and you'll need to "Commit message must indicate the subsystem this commit changes" |
|
I'll squash it to one commit and fix the commit messages once I have done the final modifications. |
|
I have fixed all style mistakes and squashed everything into one commit. I also added |
|
@erikdubbelboer Your changes seem to break a ton of tests, see the Jenkins report. Can you investigate? |
|
@bnoordhuis The Jenkins report times out so I am not able to investigate the exact errors. I had already run |
|
@bnoordhuis or @trevnorris is there someone I can contact about http://jenkins.nodejs.org/ ? It times out all the time so I am not able to check the last errors on this pull request on: http://jenkins.nodejs.org/job/node-pullrequest/901/testReport/ |
|
Looping in @tjfontaine. I can confirm the issue, the Jenkins link for your PR keeps timing out. |
|
Not just my PR, http://jenkins.nodejs.org times out for me as well. |
|
Sometimes I really dislike jenkins, anyway I triggered a new build and its result will be at http://jenkins.nodejs.org/job/node-pullrequest/903/ |
|
@tjfontaine thanks for the new build but this one also times out :( |
|
@tjfontaine other people's pull requests seem to be timing out as well. @bnoordhuis Since my last commit I think the only tests that fail are the ones that probably fail on master as well. But I guess you want to wait until Jenkins works again to see this? The problem had to with TLSv2 which uses a SHA512 digest. While the keys Makefile contains commands to generate 1024 bit keys the actual keys in the repository are 512 bit. A 512 bit RSA key is not large enough for SHA512 so this was giving errors in almost all https/tls tests. The fix was to do a |
|
@tjfontaine @bnoordhuis any update on this? |
|
@bnoordhuis At the moment both jenkins reports return a 404 but a couple of days ago I could actually watch one of them. All current errors have nothing to do with my changes and are present in other pull requests as well. I merged everything into once commit so I guess it's up to you now if you want to merge it or not. |
|
I've scheduled another build, to keep jenkins from grinding to a halt we're not keeping as much history for things |
|
@tjfontaine thanks. As I said before the errors in the build http://jenkins.nodejs.org/job/node-pullrequest/1070/testReport/ have nothing to do with my changes and are present in other builds as well. |
|
In light of recent NSA revelations what is the time frame for integrating this into a release? |
|
|
||
| This is required to support ECDH (Elliptic Curve Diffie-Hellman) ciphers. | ||
|
|
||
| Defaults to `prime256v1`. |
There was a problem hiding this comment.
This section could be less terse. Maybe add a short (really short) primer on what ECDH is, why you'd use it and what the purpose of the ecdhCurve parameter is. When I say short, I mean three, maybe four paragraphs tops.
|
I don't see anything objectionable at a quick glance (a more thorough review will have to wait, on paternity leave). I'm curious though why you regenerated all the keys and certs in test/fixtures. |
|
ya, that was my question as well, was it necessary for the test suite to move to 1024 keys? |
|
I have added some more information to the documentation and merged all upstream changes. It's really hard to explain what the curve parameter does without knowing the math involved. Nginx for example also defaults to prime256v1 and has a ssl_ecdh_curve configure command that is undocumented. More information about the usefulness of ECDH can be found in these blog post: The test suite needed to move to 1024 bit keys because for the ECDHE-RSA-AES128-SHA256 cipher a 512 bit key is too small and all tls based tests failed. One thing that has to be thought about before accepting this pull request is the default value. ECDH is a lot slower than RSA so setting up a new connection takes more time. Below I have included the results of the tls-connect benchmark (both done with the new 1024 bit keys). I have set the default to enable ECDH ciphers because I personally think a secure default is more important than a fast default. Without ECDH: With ECDH: |
|
LGTM, thanks Erik. I'm somewhat in dubio if ECDH should be enabled by default. The thing I worry about is releasing node.js v0.12 and people noticing a hefty drop in TLS performance without a clear indication why that is. A mention in the release notes probably won't help - J. Random Devops doesn't read them. I'm leaning towards making it opt-in, possibly with a strong recommendation in the docs. Thoughts? |
|
Well it is only the setup of a connection that is slower. Transferring data should stay exactly the same (if the ECDH and non-ECDH cipers used use the same encryption method). |
Yes, but a TLS handshake is already painfully slow - making it slower still is, well, you know where I'm going with this. |
|
Okay committers, last call. If there are no comments in the next 24 hours, I'm merging it as-is. |
|
|
||
| SecureContext* sc = WeakObject::Unwrap<SecureContext>(args.This()); | ||
|
|
||
| if (args.Length() != 1 || !args[0]->IsString()) |
There was a problem hiding this comment.
A similar issue #4317 supports multiple curves being defined here, is that an interesting feature that should be had?
There was a problem hiding this comment.
I'm wondering if what this pull request does really works. It seems to generate a fixed key for each curve and then uses these for each request. My understanding was that for forward security this key needs to be newly generated for each request. I could be wrong on this and the documentation on SSL_CTX_set_tmp_ecdh_callback is non-existent. The way I have implemented it is the same way as nginx and apache.
- switched test fixtures to 1024 bit keys
|
Any update on this? |
|
Landed in bb909ad. Thanks, Erik. |
|
|
||
| This is required to support ECDH (Elliptic Curve Diffie-Hellman) ciphers. | ||
| ECDH ciphers are a newer alternative to RSA. The advantages of ECDH over | ||
| RSA is that it offers [Forward secrecy]. Forward secrecy means that for an |
There was a problem hiding this comment.
I get confused here. PFS in TLS has to do with the initial key-exchange, not the encryption itself. Using ECDH for key exchange is not considered as perfectly forward secure, because the parameters do not change (this is only the case with ECDHE). Also, DHE is considered to offer PFS, but ECDH is not really better than DH. Once your (EC)DH parameters are exposed, encryption keys can be computed from previously collected traffic affecting all past and future connections.
Furthermore, RSA is not in competition for key-exchange, it is used to deliver the certificate(s). It's counterpart is not ECDH but ECDSA. Comparing ECDH and RSA is complete rubbish. You compare a key-exchange method with a cipher. ECDH is not a cipher!
Also, this wording gives the idea that PFS cannot be achieved with RSA, but again DHE_RSA is perfectly forward secure!
Please fix this paragraph.
There was a problem hiding this comment.
@FlowLo thank you! You've very interesting insights on this topic, may I ask you to open a pull request for this?
There was a problem hiding this comment.
I don't think that it was supported in node v0.10.x. Please try with 0.11.x, it was introduced in bb909.
Seems to be working fine for me. I've checked some articles on this topic and totally agree with your thoughts in the first comment. Only ephemeral Diffie-Hellman provides PFS. And EC* just makes it harder to crack. Please submit a PR to fix this!
There was a problem hiding this comment.
You are totally right, it runs with 0.11.10, sorry for bothering you! Will write up a PR now, cheers :)
|
also @indutny do you know if the crypto APIs like |
|
@maxogden it may, if you'll open an issue ;) |
|
Bitcoin uses I'm unclear as to why it's listed as an "unsafe" on that site.. it's suggested within the community that it was chosen because it has a low likelyhood of having a magic NSA backdoor http://www.reddit.com/r/Bitcoin/comments/1mbyj0/bitcoins_secured_by_nsa_designed_encryption_or/cc7v8w8 |
|
@kyledrake As long as it's available from OpenSSL it can be specified as an option. Also the people that produced the safecurves site evaluated more than just potential backdoors. I originally learned about it from this talk http://www.youtube.com/watch?v=HJB1mYEZPPA&t=43m47s |
This would allow closing of issues #4315 #4317 and #4710.