Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make ALL cipher string match ECDHE ciphers #91

Merged
merged 2 commits into from
May 4, 2016

Conversation

jgaxn
Copy link

@jgaxn jgaxn commented May 3, 2016

MRI includes ECDHE ciphers under the ALL cipher string and this replicates that behavior. This doesn't fully solve jruby/jruby#1774 but it at least makes ECDHE ciphers available under ALL. There are other issues with CipherStrings (see: jruby/jruby#1738 (comment)) that need to be worked out, but this fixes my current use case.

The following ciphers are added to the ALL cipher string:

[93] pry(main)> jruby_ciphers_new - jruby_ciphers_old
=> ["ECDHE-ECDSA-AES256-SHA",
 "ECDHE-RSA-AES256-SHA",
 "ECDH-ECDSA-AES256-SHA",
 "ECDH-RSA-AES256-SHA",
 "ECDHE-ECDSA-AES128-SHA256",
 "ECDHE-RSA-AES128-SHA256",
 "ECDH-ECDSA-AES128-SHA256",
 "ECDH-RSA-AES128-SHA256",
 "ECDHE-ECDSA-AES128-SHA",
 "ECDHE-RSA-AES128-SHA",
 "ECDH-ECDSA-AES128-SHA",
 "ECDH-RSA-AES128-SHA",
 "ECDHE-ECDSA-DES-CBC3-SHA",
 "ECDHE-RSA-DES-CBC3-SHA",
 "ECDH-ECDSA-DES-CBC3-SHA",
 "ECDH-RSA-DES-CBC3-SHA",
 "AECDH-AES256-SHA",
 "AECDH-AES128-SHA",
 "AECDH-DES-CBC3-SHA"]

I also verified that no new ciphers were added that aren't already included by MRI

[94] pry(main)> jruby_ciphers_old - mri_ciphers
=> ["DES-CBC-SHA", "EDH-RSA-DES-CBC-SHA", "ADH-DES-CBC-SHA", "EXP-DES-CBC-SHA", "EXP-EDH-RSA-DES-CBC-SHA", "EXP-EDH-DSS-DES-CBC-SHA", "EXP-ADH-DES-CBC-SHA"]
[95] pry(main)> jruby_ciphers_new - mri_ciphers
=> ["DES-CBC-SHA", "EDH-RSA-DES-CBC-SHA", "ADH-DES-CBC-SHA", "EXP-DES-CBC-SHA", "EXP-EDH-RSA-DES-CBC-SHA", "EXP-EDH-DSS-DES-CBC-SHA", "EXP-ADH-DES-CBC-SHA"]

This matches the MRI behavior.
@kares
Copy link
Member

kares commented May 4, 2016

Thanks John, could we please have a test-case (e.g. somewhere under: https://github.com/jruby/jruby-openssl/tree/master/src/test/ruby/ssl). Would likely run it on MRI sporadically to make sure its ~ same.

@jgaxn
Copy link
Author

jgaxn commented May 4, 2016

@kares Thanks for your feedback. I added a test case that checks for the ciphers we expect to see with the ALL cipher string on both jruby and MRI.

@kares kares merged commit c90d284 into jruby:master May 4, 2016
@jgaxn
Copy link
Author

jgaxn commented May 4, 2016

@kares: Thanks for merging! Is there a plan for the next release? We have a client waiting on this functionality and we'd like to know if we should wait for the release or fork it ourselves.

@jgaxn jgaxn deleted the cipherstrings_all_ecdhe branch May 4, 2016 20:06
@kares kares added this to the 0.9.17 milestone Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants