Add fancier ECDHE support #57

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
6 participants

amluto commented Mar 5, 2014

This is based on #9, but it adds more curve definitions and ELLIPTIC_CURVE_DESCRIPTIONS. The latter lists curves that are actually supported on the system's OpenSSL.

alex and others added some commits Jan 17, 2014

Merge branch 'master' into ecdhe-support
Conflicts:
	.gitignore
	OpenSSL/test/test_ssl.py
Expose all of the EC curve name constants
It would be great if there were a clean way to enumerate them rather
than just listing them like this, but I don't know of one.
Add SSL.ELLIPTIC_CURVE_DESCRIPTIONS to expose the actual supported cu…
…rves

Different OpenSSL builds support different curves.  Determine the
supported curves at startup and expose the list.

amluto commented Mar 5, 2014

This depends on pyca/cryptography#738

@amluto amluto referenced this pull request Mar 5, 2014

Closed

Expose support for using ecdhe with SSL connections #9

3 of 3 tasks complete

amluto commented Mar 5, 2014

The cryptography dependency has been merged.

amluto commented Mar 5, 2014

Looks like Travis CI needs to be asked to try again once cryptography makes a release.

OpenSSL/SSL.py
+ _num_curves = _lib.EC_get_builtin_curves(_ffi.NULL, 0)
+ _curves = _ffi.new('EC_builtin_curve[]', _num_curves)
+ if _lib.EC_get_builtin_curves(_curves, _num_curves) == _num_curves:
+ ELLIPTIC_CURVE_DESCRIPTIONS = {c.nid : _ffi.string(c.comment)
@hynek

hynek Mar 11, 2014

Contributor

dict comprehension are Python 2.7+. Try dict((key, value) for (key, value) in sequence) instead.

OpenSSL/SSL.py
@@ -594,6 +749,26 @@ def load_tmp_dh(self, dhfile):
_lib.SSL_CTX_set_tmp_dh(self._context, dh)
+ def set_tmp_ecdh_by_curve_name(self, curve_name):
+ """
+ Configure this connection to people to use Elliptical Curve
@hynek

hynek Mar 11, 2014

Contributor

This neither about connections (it’s a context) nor about people. :) How about just “Configure curve for ECDHE key exchanges.”?

OpenSSL/SSL.py
+ Configure this connection to people to use Elliptical Curve
+ Diffie-Hellman key exchanges.
+
+ :param curve_name: One of the named curve constants.
@hynek

hynek Mar 11, 2014

Contributor

this needs a :type curve_name:

amluto commented Mar 12, 2014

I updated the branch.

Owner

reaperhulk commented Mar 14, 2014

@amluto, we're considering not adding any more NID bindings to cryptography and instead asking users to use OBJ_sn2nid to obtain the NID mappings. This decision won't affect this PR since we already landed the expanded NIDs you require, but would come up if you decided to support the brainpool curves present in OpenSSL 1.0.2. @hynek asked me to drop a comment here to get your thoughts on this, but to avoid polluting this PR feel free to comment over on pyca/cryptography#778

@amluto amluto referenced this pull request in pyca/cryptography Mar 14, 2014

Closed

Support for brainpool curves in OpenSSL 1.0.2 #778

amluto commented Mar 14, 2014

I didn't know about OBJ_sn2nid and OBJ_nid2sn. Let me adjust the branch to use strings instead of numbers -- I like that better.

amluto added some commits Mar 14, 2014

Identify elliptic curves by short name, not NID
Using NIDs is awkward and requires updating pyOpenSSL every time a new
curve is added.  This approach avoids needing to update pyOpenSSL
each time a new curve is added, and it results in more readable code
and a more readable dict ELLIPTIC_CURVE_DESCRIPTIONS.

amluto commented Mar 14, 2014

I updated the pull request. I renamed the function, since it no longer uses a "curve name" (which, in OpenSSL parlance, is a number, not really a name).

Owner

lvh commented Mar 14, 2014

This is only still failing because cryptography hasn't made a release yet, right?

This works for me (PyPy 2.2.1, cryptography 3.0-dev1, OpenSSL 1.0.1e).
Anything missing to get it merged?

Other than adjusting setup.py:

install_requires=["cryptography>=0.3", "six>=1.5.2"],
Owner

lvh commented Mar 17, 2014

I think that version of cryptography has to be actually released before we can merge this :) Specifically we can't merge things with red buildbots ;)

Owner

lvh commented Mar 17, 2014

I've just merged the updated Travis configuration that will test this against cryptography master, so I will now re-run the build.

awesome! I'd love to retire a bunch of hacks and take this into production ..

Owner

lvh commented Mar 17, 2014

Likewise, although in Twisted trunk you should actually already get ECDHE (with nistp256 only though) even without this branch.

Seems like crypography 0.2.2 is still missing

AttributeError: 'FFILibrary' object has no attribute 'EC_get_builtin_curves'
Owner

lvh commented Mar 17, 2014

Yes. I can't figure out how to convince Travis to build against cryptography master. I've asked in their IRC channel. It may be necessary for @amluto to merge master back into his branch.

@oberstet oberstet referenced this pull request in crossbario/crossbar Mar 17, 2014

Closed

TLS hardening #34

Owner

lvh commented Mar 17, 2014

Yeah. Heard back from the Travis CI people. @amluto: could you please merge pyca/pyopenssl master back into your branch? That way we can at least see if it works on the buildbots with cryptography master :)

@lvh I have merged master and @amluto 's ecdeh branch into https://github.com/oberstet/pyopenssl/tree/ecdhe

Will that work for Travis testing?

Owner

lvh commented Mar 19, 2014

Only if you submit it as a PR.

done: #71

Owner

lvh commented Mar 22, 2014

So, recap from the #71 experiment: everything works with everything as trunk; we need cryptography 0.3 to be released and to depend on it in master, and then have master merged into this branch.

Owner

alex commented Mar 27, 2014

We've done a release now.

@lvh lvh referenced this pull request Mar 28, 2014

Closed

Require cryptography>=0.3 #81

Owner

lvh commented Mar 28, 2014

I've made reffed PR to require that new release, once someone (presumably @exarkun) merges it and after that @amluto merges master back into this branch, we can get Travis to run the tests (which almost certainly will pass), and then we can finally get this to land :-)

Awesome. If @amluto is busy with other stuff: why can't we just merge master onto amluto's branch plus the updated require on another branch (as I did 10 days ago for prelimiary testing) ourselves? In the end, this is Git .. version history is preserved. Why would @amluto be required to perform these mechanical acts?

Owner

lvh commented Mar 29, 2014

There's no reason. Since @hynek thought the branch that needed the new cryptography should add the requirement, I'll make a new PR.

Owner

lvh commented Mar 29, 2014

Closing in favor of #82.

@lvh lvh closed this Mar 29, 2014

@amluto amluto referenced this pull request Apr 4, 2014

Closed

Yet another ecdhe PR #89

amluto commented Apr 4, 2014

See #89, and feel free to close it if you merge into here.

@exarkun exarkun referenced this pull request Apr 19, 2014

Merged

Ecdhe #101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment