Issue32858
Created on 2018-02-16 16:03 by sruester, last changed 2022-04-11 14:58 by admin. This issue is now closed.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| ssl-host-check.py | 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) * ![]() |
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) * ![]() |
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);
gives
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) * ![]() |
Date: 2018-02-16 17:26 | |
Thanks! 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) * ![]() |
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 https://bugs.python.org/issue32882 and https://bugs.python.org/issue32883 and will close this one now. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022-04-11 14:58:57 | admin | set | github: 77039 |
| 2018-02-20 10:16:07 | christian.heimes | link | issue32883 superseder |
| 2018-02-20 10:09:45 | christian.heimes | link | issue32882 superseder |
| 2018-02-20 10:01:19 | sruester | set | status: open -> closed resolution: wont fix messages: + msg312407 stage: patch review -> resolved |
| 2018-02-19 12:19:03 | christian.heimes | set | messages: + msg312348 |
| 2018-02-19 11:35:38 | sruester | set | messages: + msg312347 |
| 2018-02-16 21:38:31 | sruester | set | pull_requests: + pull_request5495 |
| 2018-02-16 17:46:50 | sruester | set | messages: + msg312248 |
| 2018-02-16 17:26:59 | christian.heimes | set | messages: + msg312245 |
| 2018-02-16 17:13:59 | sruester | set | files:
+ ssl-host-check.py messages: + msg312244 |
| 2018-02-16 17:09:13 | sruester | set | messages: + msg312243 |
| 2018-02-16 16:57:54 | sruester | set | keywords:
+ patch stage: needs patch -> patch review pull_requests: + pull_request5489 |
| 2018-02-16 16:56:53 | christian.heimes | set | messages: + msg312241 |
| 2018-02-16 16:34:46 | christian.heimes | set | nosy:
+ janssen, christian.heimes, alex, dstufft messages: + msg312238 assignee: christian.heimes |
| 2018-02-16 16:03:17 | sruester | create | |
