Improve OpenSSL ECDH support

[sruester]

BPO	32858
Nosy	@tiran, @alex, @dstufft, @sruester
PRs	#32858: Add OpenSSL ECDH functionality #5700 bpo-32858: Add method kxinfo to SSLObject #5707
Files	ssl-host-check.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tiran'
closed_at = <Date 2018-02-20.10:01:19.958>
created_at = <Date 2018-02-16.16:03:17.959>
labels = ['expert-SSL', 'type-feature', '3.8']
title = 'Improve OpenSSL ECDH support'
updated_at = <Date 2018-02-20.10:01:19.957>
user = 'https://github.com/sruester'

bugs.python.org fields:

activity = <Date 2018-02-20.10:01:19.957>
actor = 'sruester'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2018-02-20.10:01:19.958>
closer = 'sruester'
components = ['SSL']
creation = <Date 2018-02-16.16:03:17.959>
creator = 'sruester'
dependencies = []
files = ['47449']
hgrepos = []
issue_num = 32858
keywords = ['patch']
message_count = 10.0
messages = ['312237', '312238', '312241', '312243', '312244', '312245', '312248', '312347', '312348', '312407']
nosy_count = 5.0
nosy_names = ['janssen', 'christian.heimes', 'alex', 'dstufft', 'sruester']
pr_nums = ['5700', '5707']
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue32858'
versions = ['Python 3.8']

[sruester]

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.

[tiran]

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.

[tiran]

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.

[sruester]

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 :-)

[sruester]

Attached script shows usage

[tiran]

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.

[sruester]

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.

[sruester]

AppVeyor build failed for pull request 5707. It looks like there was a problem with the build environment.

[tiran]

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.