-
Notifications
You must be signed in to change notification settings - Fork 422
Yet another ecdhe PR #89
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
Changes from all commits
7b8d57a
a683fc0
12dc084
2c04e86
807853c
d5419e2
479878d
32f016f
b4e5c8d
9bca0ed
76a6133
f05a273
4064ea1
63e99fe
e8b2d30
f5df243
5fb416a
61d7d39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
*.py[co] | ||
build | ||
dist | ||
*.egg-info | ||
*.egg-info |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,6 +124,19 @@ class _memoryview(object): | |
SSL_CB_HANDSHAKE_START = _lib.SSL_CB_HANDSHAKE_START | ||
SSL_CB_HANDSHAKE_DONE = _lib.SSL_CB_HANDSHAKE_DONE | ||
|
||
_Cryptography_HAS_EC = _lib.Cryptography_HAS_EC | ||
ELLIPTIC_CURVE_DESCRIPTIONS = {} # In case there's no EC support | ||
if _Cryptography_HAS_EC: | ||
_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 = dict( | ||
(_ffi.string(_lib.OBJ_nid2sn(c.nid)).decode('ascii'), | ||
_ffi.string(c.comment).decode('utf-8')) | ||
for c in _curves) | ||
del _num_curves | ||
del _curves | ||
|
||
|
||
class Error(Exception): | ||
""" | ||
|
@@ -217,6 +230,37 @@ def _asFileDescriptor(obj): | |
|
||
|
||
|
||
class ECNotAvailable(ValueError): | ||
""" | ||
Raised if a request for an elliptic curve fails because OpenSSL | ||
is compiled without elliptic curve support. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although we say EC everywhere (I personally prefer ECC), to proper term is “elliptic curve cryptography”. |
||
""" | ||
def __init__(self): | ||
ValueError.__init__(self, "OpenSSL is compiled without EC support") | ||
|
||
|
||
|
||
class UnknownObject(ValueError): | ||
""" | ||
Raised if OpenSSL does not recognize the requested object. | ||
""" | ||
def __init__(self, sn): | ||
ValueError.__init__(self, "OpenSSL does not recognize %r" % sn) | ||
self.sn = sn | ||
|
||
|
||
|
||
class UnsupportedEllipticCurve(ValueError): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Above, you write EC_NotAvailable but here Unsupported_EllipticCurve. UnsupportedECCurve would be more consistent I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or UnsupportedEC. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You’ll have to decide what EC stands for and use it consistently. Apparently it’s used as a synonym for elliptic curve cryptography, so using it as an abbreviation for elliptic curve would be confusing (how would you differentiate between ECNotAvailable and UnsupportedEC?). Do you see the problem?. |
||
""" | ||
Raised if OpenSSL does not support the requested elliptic curve. | ||
""" | ||
def __init__(self, sn): | ||
ValueError.__init__( | ||
self, "OpenSSL does not support the elliptic curve %r" % sn) | ||
self.sn = sn | ||
|
||
|
||
|
||
def SSLeay_version(type): | ||
""" | ||
Return a string describing the version of OpenSSL in use. | ||
|
@@ -598,6 +642,35 @@ def load_tmp_dh(self, dhfile): | |
_lib.SSL_CTX_set_tmp_dh(self._context, dh) | ||
|
||
|
||
def set_tmp_ecdh_curve(self, curve_name): | ||
""" | ||
Select a curve to use for ECDHE key exchange. | ||
|
||
The valid values of *curve_name* are the keys in | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
:py:data:OpenSSL.SSL.ELLIPTIC_CURVE_DESCRIPTIONS. | ||
|
||
Raises a subclass of ``ValueError`` if the linked OpenSSL was | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use |
||
not compiled with elliptical curve support or the specified | ||
curve is not available. You can check the specific subclass, | ||
but, in general, you should just handle ``ValueError``. | ||
|
||
:param curve_name: The 'short name' of a curve, e.g. 'prime256v1' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for scare quotes. I also like to put a period at the end of my sentences. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like to put verbs in my sentences ;) |
||
:type curve_name: str | ||
:return: None | ||
""" | ||
if _lib.Cryptography_HAS_EC: | ||
nid = _lib.OBJ_sn2nid(curve_name.encode('ascii')) | ||
if nid == _lib.NID_undef: | ||
raise UnknownObject(curve_name) | ||
ecdh = _lib.EC_KEY_new_by_curve_name(nid) | ||
if ecdh == _ffi.NULL: | ||
raise UnsupportedEllipticCurve(sn) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
_lib.SSL_CTX_set_tmp_ecdh(self._context, ecdh) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call can fail, apparently - memory allocation failure, broken parameters in the ecdh object, etc. |
||
_lib.EC_KEY_free(ecdh) | ||
else: | ||
raise ECNotAvailable() | ||
|
||
|
||
def set_cipher_list(self, cipher_list): | ||
""" | ||
Change the cipher list | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,9 @@ | |
SESS_CACHE_OFF, SESS_CACHE_CLIENT, SESS_CACHE_SERVER, SESS_CACHE_BOTH, | ||
SESS_CACHE_NO_AUTO_CLEAR, SESS_CACHE_NO_INTERNAL_LOOKUP, | ||
SESS_CACHE_NO_INTERNAL_STORE, SESS_CACHE_NO_INTERNAL) | ||
from OpenSSL.SSL import ( | ||
_Cryptography_HAS_EC, ELLIPTIC_CURVE_DESCRIPTIONS, | ||
ECNotAvailable, UnknownObject) | ||
|
||
from OpenSSL.SSL import ( | ||
Error, SysCallError, WantReadError, WantWriteError, ZeroReturnError) | ||
|
@@ -1172,6 +1175,65 @@ def test_load_tmp_dh(self): | |
# XXX What should I assert here? -exarkun | ||
|
||
|
||
def test_set_tmp_ecdh_curve(self): | ||
""" | ||
:py:obj:`Context.set_tmp_ecdh_curve` sets the elliptic | ||
curve for Diffie-Hellman to the specified named curve. | ||
""" | ||
context = Context(TLSv1_METHOD) | ||
for curve in ELLIPTIC_CURVE_DESCRIPTIONS.keys(): | ||
context.set_tmp_ecdh_curve(curve) # Must not throw. | ||
|
||
|
||
def test_set_tmp_ecdh_curve_bad_sn(self): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These two cases (and those in the next test function) should be artificially created and tested they work properly. You’ll have to monkeypatch a bit I’m afraid. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On a HAS_EC build, all but the !HAS_EC case are tested. I can monkeypatch for !HAS_EC. |
||
""" | ||
:py:obj:`Context.set_tmp_ecdh_curve` raises | ||
:py:obj:`UnknownObject` if passed a curve_name that OpenSSL | ||
does not recognize and EC is available. It raises | ||
:py:obj:`ECNotAvailable` if EC is not available at all. | ||
""" | ||
context = Context(TLSv1_METHOD) | ||
try: | ||
context.set_tmp_ecdh_curve('not_an_elliptic_curve') | ||
except ECNotAvailable: | ||
self.assertFalse(_Cryptography_HAS_EC) | ||
except UnknownObject: | ||
self.assertTrue(_Cryptography_HAS_EC) | ||
else: | ||
self.assertFalse(True) | ||
|
||
|
||
def test_set_tmp_ecdh_curve_not_a_curve(self): | ||
""" | ||
:py:obj:`Context.set_tmp_ecdh_curve` raises | ||
:py:obj:`UnsupportedEllipticCurve` if passed a curve_name that | ||
OpenSSL cannot instantiate as an elliptic curve. It raises | ||
:py:obj:`ECNotAvailable` if EC is not available at all. | ||
""" | ||
context = Context(TLSv1_METHOD) | ||
try: | ||
context.set_tmp_ecdh_curve('sha256') | ||
except ECNotAvailable: | ||
self.assertFalse(_Cryptography_HAS_EC) | ||
except UnknownObject: | ||
self.assertTrue(_Cryptography_HAS_EC) | ||
else: | ||
self.assertFalse(True) | ||
|
||
|
||
def test_has_curve_descriptions(self): | ||
""" | ||
If the underlying cryptography bindings claim to have elliptic | ||
curve support, there should be at least one curve. | ||
|
||
(In theory there could be an OpenSSL that violates this | ||
assumption. If so, this test will fail and we'll find out.) | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for an extra line nitpick :) |
||
""" | ||
if _Cryptography_HAS_EC: | ||
self.assertNotEqual(len(ELLIPTIC_CURVE_DESCRIPTIONS), 0) | ||
|
||
|
||
def test_set_cipher_list_bytes(self): | ||
""" | ||
:py:obj:`Context.set_cipher_list` accepts a :py:obj:`bytes` naming the | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,6 +110,17 @@ Context, Connection. | |
.. versionadded:: 0.14 | ||
|
||
|
||
.. py:data:: ELLIPTIC_CURVE_DESCRIPTIONS | ||
|
||
A dictionary mapping short names of elliptic curves to textual | ||
descriptions. This dictionary contains exactly the set of curves | ||
supported by the OpenSSL build in use. | ||
|
||
The keys are the curve names that can be passed into | ||
Constants used with :py:meth:`Context.set_tmp_ecdh_curve` to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this wording a bit odd. “Constants”? It should at the very least be lower-case but it still feels a bit hand-wavy. |
||
specify which elliptical curve should be used for ECDHE key exchange. | ||
|
||
|
||
.. py:data:: OPENSSL_VERSION_NUMBER | ||
|
||
An integer giving the version number of the OpenSSL library used to build this | ||
|
@@ -316,6 +327,21 @@ Context objects have the following methods: | |
|
||
Load parameters for Ephemeral Diffie-Hellman from *dhfile*. | ||
|
||
.. py:method:: Context.set_tmp_ecdh_curve(curve_name) | ||
|
||
Select a curve to use for ECDHE key exchange. | ||
|
||
The valid values of *curve_name* are the keys in | ||
:py:data:`ELLIPTIC_CURVE_DESCRIPTIONS`. | ||
|
||
Raises a subclass of ``ValueError`` if the linked OpenSSL was not | ||
compiled with elliptical curve support or the specified curve is | ||
not available. You can check the specific subclass, but, in | ||
general, you should just handle ``ValueError``. | ||
|
||
:param curve_name: The 'short name' of a curve, e.g. 'prime256v1' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for scare quotes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They're real quotes that happen to be scary. OpenSSL talks about "short name[s]" and "curve name[s]". The latter is, confusingly, a number. That being said, the quotes aren't really necessary, so I'll remove them. |
||
:type curve_name: str | ||
:return: None | ||
|
||
.. py:method:: Context.set_app_data(data) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if there's a good reason to expose the descriptions here. They don't seem to have particularly meaningful values. If someone wants to learn more about a particular curve they'll probably have better luck looking around for info on it on their own? So maybe this should just be a set.
Also I'm not sure this is the right API to expose even the names of the supported curves. It seems like an increase in the disorder of the module interface. Curves actually seems more crypto related than SSL. In the interest of getting some ECDHE support into pyOpenSSL though, maybe the right thing to do for now is just make this private and then sort out the rest of the API details relating to it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no problem with making it be a set. It would also be easy to move it into OpenSSL.crypto, or to rename it to _ELLIPTIC_CURVES or _ELLIPTIC_CURVE_DESCRIPTIONS, or to remove it entirely.