Title: Improve OpenSSL ECDH support
Type: enhancement Stage: resolved
Components: SSL Versions: Python 3.8
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: alex, christian.heimes, dstufft, janssen, sruester
Priority: normal Keywords: patch

Created on 2018-02-16 16:03 by sruester, last changed 2018-02-20 10:01 by sruester. This issue is now closed.

File name Uploaded Description Edit sruester, 2018-02-16 17:13
Pull Requests
URL Status Linked Edit
PR 5700 sruester, 2018-02-16 16:57
PR 5707 closed sruester, 2018-02-16 21:38
Messages (10)
msg312237 - (view) Author: sruester (sruester) * Date: 2018-02-16 16:03
Tested with OpenSSL v1.1.0g, Python does not support selection of curve Curve25519 with _ssl.ctx.set_ecdh_curve("X25519").

Additionally the DH key exchange parameters (which curve has been chosen, what DH bit size was used) are not available through any SSL or Context method.
msg312238 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-16 16:34
Please elaborate, how did you test that the curve is not support? Python calls SSL_CTX_set_ecdh_auto(self->ctx, 1) to auto configure curves.

>>> import ssl
>>> ssl = ssl.SSLContext()
>>> ssl.set_ecdh_curve('X25519')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ssl.SSLError: unknown group (_ssl.c:3954)

The error message means that EC_KEY_new_by_curve_name() does not support X25519's group.

Some notes:
* OpenSSL 1.0.2+ supports SSL_CTX_set1_curves_list() besides SSL_CTX_set_tmp_ecdh()
* OpenSSL has no API to get configured curves from a context.
* I'm not sure how useful SSL_get1_curves() and SSL_get_shared_curve() would be for a general audience. To reduce our maintenance burden, we only wrap functions that are useful or required.
msg312241 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-16 16:56
When I replace the current implementation of SSLContext.set_ecdh_curve() with an implementation based on SSL_CTX_set1_curves_list(), then I'm able to configure X25519 curve for ECDH.
msg312243 - (view) Author: sruester (sruester) * Date: 2018-02-16 17:09
With OpenSSL 1.1.0g, the Code

 int nid = OBJ_sn2nid("X25519");
 EC_KEY *key = EC_KEY_new_by_curve_name(nid);
 printf("id:%i  key:%p\n", nid, key);


 id:1034  key:(nil)

EC_KEY_new_by_curve_name is IMHO not the best option to define client side curves. It can only select a single curve to be offered to the server, and it does not (for whatever reason) support X25519 yet.
SSL_CTX_set1_curves_list() provides both, selection of multiple curves for the client's preference list and it supports X25519 out of the box.

Aside from this I am missing a method in SSLSocket to give me information about the key exchange (DH, ECDH, which curve was chosen, which bit size DH keys had, ...).

I prepared a pull request which addresses both. Please review and be gentle, it is my first pull request here :-)
msg312244 - (view) Author: sruester (sruester) * Date: 2018-02-16 17:13
Attached script shows usage
msg312245 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-16 17:26

I rejected your initial PR. In the past we added some cruft or badly designed features to the SSL module. I'm in the process of cleaning the module up. Any new feature or revised method should be designed carefully and added to PEP 543. The PEP defines a general TLS API that is less OpenSSL centric.

The ssl module is and will stay a thin wrapper around OpenSSL. But we are trying to implement new features in a general, abstract way that work with other TLS implementations like SecureTransport, SChannel, or NSS.
msg312248 - (view) Author: sruester (sruester) * Date: 2018-02-16 17:46
I'd really love to see kxinfo() or a similar method in the standard. I chose to implement it similar to cipher() which seemed to be a good idea then. If there are any objections, please let's discuss how that information can be made available otherwise.
If that's ok, I will open another pull request which only contains kxinfo or similar. It is, however, not sufficient without set_ecdh_curve's support for X25519 in some cases (my case ^^).

Changing the implementation of set_ecdh_curve seems necessary anyway, as it does not support X25519 at all, and it does not allow defining multiple curves.

Maybe we can do both, update PEP 543 to address the needs and implement it (in an OpenSSL centric way) for the current version.
msg312347 - (view) Author: sruester (sruester) * Date: 2018-02-19 11:35
AppVeyor build failed for pull request 5707. It looks like there was a problem with the build environment.
msg312348 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-19 12:19
Please split this issue into multiple issues, a bug report for the curve configuration bug and a feature request for kxinfo. The bug fix may land in 2.7, 3.6 and 3.7 while the new feature can only land in 3.8.

Before you start coding, let's figure out an API first. For instance I don't like "kxinfo" as method name. It's a) a cryptic name and b) technically wrong for TLS 1.3 and PFS suites. Although people refer to DH as key exchange protocol, it's really a key agreement protocol. kRSA is a key exchange protocol.
msg312407 - (view) Author: sruester (sruester) * Date: 2018-02-20 10:01
I agree, we shouldn't support that confusion. I opened two separate issues and and will close this one now.
Date User Action Args
2018-02-20 10:16:07christian.heimeslinkissue32883 superseder
2018-02-20 10:09:45christian.heimeslinkissue32882 superseder
2018-02-20 10:01:19sruestersetstatus: open -> closed
resolution: wont fix
messages: + msg312407

stage: patch review -> resolved
2018-02-19 12:19:03christian.heimessetmessages: + msg312348
2018-02-19 11:35:38sruestersetmessages: + msg312347
2018-02-16 21:38:31sruestersetpull_requests: + pull_request5495
2018-02-16 17:46:50sruestersetmessages: + msg312248
2018-02-16 17:26:59christian.heimessetmessages: + msg312245
2018-02-16 17:13:59sruestersetfiles: +

messages: + msg312244
2018-02-16 17:09:13sruestersetmessages: + msg312243
2018-02-16 16:57:54sruestersetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request5489
2018-02-16 16:56:53christian.heimessetmessages: + msg312241
2018-02-16 16:34:46christian.heimessetnosy: + janssen, christian.heimes, alex, dstufft
messages: + msg312238

assignee: christian.heimes
components: + SSL, - Library (Lib)
stage: needs patch
2018-02-16 16:03:17sruestercreate