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

tls, crypto: add DHE support #8272

Closed
wants to merge 2 commits into from
Closed

Conversation

shigeki
Copy link

@shigeki shigeki commented Aug 27, 2014

Per #8264, please review this, @indutny .

This also changes the default cipher list as DHE-RSA-AES128-SHA256 is added and !EDH is explicitly written to !EDH-RSA-DES:!EDH-DSS-DES.
In case of an invalid DH parameter file, it is sliently discarded.
To use auto DH parameter in a server and DHE key length check in a client, we need to wait for the next release of OpenSSL-1.0.2.

if (!bio)
return;

DH *dh = PEM_read_bio_DHparams(bio, NULL, NULL, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

DH* dh

@indutny
Copy link
Member

indutny commented Aug 27, 2014

@shigeki what's wrong with EDH ciphers?

Other than a few nits - LGTM.

@shigeki
Copy link
Author

shigeki commented Aug 27, 2014

@indutny Thanks for reviewing. It seems that!EDH also disables all DHE- cipher suites so I explicitly wrote them.

@indutny
Copy link
Member

indutny commented Aug 27, 2014

And why do we need to disable them?

@shigeki
Copy link
Author

shigeki commented Aug 27, 2014

I'm not sure the reason of the old commit. But it might come from prohibiting CBC by BEAST.
We should reconsider the default cipher list again in near future to add GCM and remove RC4 as described in http://tools.ietf.org/html/draft-ietf-uta-tls-bcp-02

@indutny
Copy link
Member

indutny commented Aug 27, 2014

Yeah, I already have a good cipher list for a v0.12. Let's remove !EDH anyway, and land your patch :)

shigeki pushed a commit to shigeki/node-v0.x-archive that referenced this pull request Aug 28, 2014
`!EDH` is also removed from the list in the discussion of nodejs#8272
@shigeki
Copy link
Author

shigeki commented Aug 28, 2014

@indutny The patch is revised with your comments. Also two changes are

  • add a new commit to change the default cipher for easy to track in the future.
  • 4096bit test is removed because it takes too long time to generate a key file

Please review this again.

Shigeki Ohtsu added 2 commits August 28, 2014 20:44
In case of an invalid DH parameter file, it is sliently discarded. To
use auto DH parameter in a server and DHE key length check in a
client, we need to wait for the next release of OpenSSL-1.0.2.
`!EDH` is also removed from the list in the discussion of nodejs#8272
@indutny
Copy link
Member

indutny commented Aug 28, 2014

LGTM, thank you!

indutny pushed a commit that referenced this pull request Aug 28, 2014
`!EDH` is also removed from the list in the discussion of #8272

Reviewed-By: Fedor Indutny <[email protected]>
@indutny
Copy link
Member

indutny commented Aug 28, 2014

Landed in 0dfedb7 and f6877f3, thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants