Opened 4 years ago

Closed 3 years ago

#6586 enhancement closed fixed (fixed)

Support (and enable by default) ECDHE key-exchange

Reported by: jknight Owned by: Hynek Schlawack
Priority: normal Milestone:
Component: core Keywords:
Cc: zooko@… Branch: branches/ninja-in-ecdhe-6586
branch-diff, diff-cov, branch-cov, buildbot
Author: hynek

Description (last modified by Thijs Triemstra)

Twisted's SSL support should enable ECDHE key exchange ciphersuites, to provide perfect forward secrecy. Then clients which support it can't have their traffic passively sniffed even if an adversary compromises your server's private key.

I think that in order for it to be usable -- even though the ciphersuites are enabled by default -- you need to call an extra function on the context.

It looks like this is how to do it in C:

+#ifdef NID_X9_62_prime256v1
+    EC_KEY *ecdh = NULL;
+    ecdh = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
+    SSL_CTX_set_tmp_ecdh(ctx,ecdh);
+    EC_KEY_free(ecdh);
+    LOG("{core} ECDH Initialized with NIST P-256\n");

But I don't really know anything more about it.

Change History (14)

comment:1 Changed 4 years ago by Thijs Triemstra

Description: modified (diff)

Fix description markup.

comment:2 Changed 4 years ago by Jean-Paul Calderone

Do you have a reference to an explanation of the security properties of ECDHE?

Separately, pyOpenSSL doesn't expose the C APIs used in the diff in the description. Those will need to be exposed before Twisted can do anything with them.

comment:3 Changed 4 years ago by jknight

This looks like a nice explanation of the issues:

Twisted could support DHE too, since it's supported in more versions of openssl than ECDHE. It too requires annoying configuration options to be set, including the generation of a "dh paramaters" blob that has to be loaded. And it's slow. Basically the only reason to consider using it is compatibility with older versions of ssl implementations.

ECDHE support in python:

DHE support in python:

comment:4 Changed 4 years ago by zooko

Cc: zooko@… added

comment:5 Changed 3 years ago by lvh

It turns out that it's possible, and, in fact, quite easy, to add best-effort ECDHE support to Twisted *right now*, if the environment happens to have a new enough version of cryptography/openssl installed. This is in the interest of having A-grade TLS in Twisted ASAP (14.0.0), and does not compromise future efforts to do so through PyOpenSSL.

We should of course eventually use the PyOpenSSL API once that exists, but that can be done in a separate ticket, which I will file right now. The branch hynek is creating adds support without introducing any new public APIs that we have to maintain. This way, we get improved security right now, without impeding any future tickets.

This would have really cool advantages such as magically Twisted being one of the only reasonable ways you can serve WSGI applications and get EC-based PFS (uwsgi does it too, but only with a bunch of gnarly C code), and *the* only way you can get a reasonably programmable one. That means that suddenly we have a great carrot for people to use Twisted: twistd web is a lot nicer than having to install and configure nginx as a reverse proxy, which is the default way of doing it right now.

comment:6 Changed 3 years ago by Hynek Schlawack

Author: hynek
Branch: branches/ninja-in-ecdhe-6586

(In [41883]) Branching to ninja-in-ecdhe-6586.

comment:7 Changed 3 years ago by Hynek Schlawack

(In [41884]) Add best-effort ECDHE support to CertificateOptions

refs #6586

comment:8 Changed 3 years ago by lvh

I have filed that ticket about eventually using the PyOpenSSL API whenever PyOpenSSL supports it: #7033.

comment:9 Changed 3 years ago by Hynek Schlawack

Keywords: review added

Welcome to the ecc future where the bots are green and the crypto is pretty!

This patch tries in a best-effort fashion to give us the benefits of ECDHE without adding public APIs or options.

Laurens said everything important, if this lands, our TLS server story becomes top-notch.

comment:10 Changed 3 years ago by lvh

Owner: set to lvh

comment:11 Changed 3 years ago by lvh

Keywords: review removed
Owner: changed from lvh to Hynek Schlawack

There is a pydoctor failure: twisted.internet._sslverify._OpenSSLECCurve.addECKeyToContext: invalid ref to OpenSSL.SSL.Context but that looks pretty bogus to me since OpenSSL.SSL.Context is obviously a thing. It also appears in other already existing places; I am assuming this is perhaps because that isn't supposed to work at all, or the API builder just doesn't have PyOpenSSL installed? I will file a ticket for that.

There is a twistedchecker failure that you should probably address: W9207:1223,0:FakeLib: Missing a blank line before epytext markups

All the other twistedchecker failures are bogus, because it's complaining that some method names don't follow the Twisted coding standard, but that's just because they're OpenSSL methods/functions:

C0103:1238,4:FakeLib.OBJ_sn2nid: Invalid name "OBJ_sn2nid" (should match ((([a-z])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$|(__[a-z]+__)$|(\_.+)$|(test\_.+)$|((telnet_|sync_|_fromString_|auth_|perspective_|handleStatus_|_parseSVNEntries_|removeTrigger_|_process_|get_|xmlrpc_|iac_|macro_|handle_|ctcpQuery_|visitNode_a_|ctcpReply_|parse_|channel_|_toString_|observe_|func_|async_|spew_|dcc_|_stat_|convert_|state_|ftp_|global_|packet_|start_|ext_|_dataReceived_|class_|opt_|do_|request_|render_|ssh_|_unjelly_|soap_|agentc_|cmd_|key_|generate_|view_|ssh_USERAUTH_PK_OK_|end_|oscar_|type_|irc_|search_|remote_|linemode_|decode_|response_|login_|visitNode_).+)$)
C0103:1251,4:FakeLib.EC_KEY_new_by_curve_name: Invalid name "EC_KEY_new_by_curve_name" (should match ((([a-z])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$|(__[a-z]+__)$|(\_.+)$|(test\_.+)$|((telnet_|sync_|_fromString_|auth_|perspective_|handleStatus_|_parseSVNEntries_|removeTrigger_|_process_|get_|xmlrpc_|iac_|macro_|handle_|ctcpQuery_|visitNode_a_|ctcpReply_|parse_|channel_|_toString_|observe_|func_|async_|spew_|dcc_|_stat_|convert_|state_|ftp_|global_|packet_|start_|ext_|_dataReceived_|class_|opt_|do_|request_|render_|ssh_|_unjelly_|soap_|agentc_|cmd_|key_|generate_|view_|ssh_USERAUTH_PK_OK_|end_|oscar_|type_|irc_|search_|remote_|linemode_|decode_|response_|login_|visitNode_).+)$)
C0103:1266,4:FakeLib.EC_KEY_free: Invalid name "EC_KEY_free" (should match ((([a-z])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$|(__[a-z]+__)$|(\_.+)$|(test\_.+)$|((telnet_|sync_|_fromString_|auth_|perspective_|handleStatus_|_parseSVNEntries_|removeTrigger_|_process_|get_|xmlrpc_|iac_|macro_|handle_|ctcpQuery_|visitNode_a_|ctcpReply_|parse_|channel_|_toString_|observe_|func_|async_|spew_|dcc_|_stat_|convert_|state_|ftp_|global_|packet_|start_|ext_|_dataReceived_|class_|opt_|do_|request_|render_|ssh_|_unjelly_|soap_|agentc_|cmd_|key_|generate_|view_|ssh_USERAUTH_PK_OK_|end_|oscar_|type_|irc_|search_|remote_|linemode_|decode_|response_|login_|visitNode_).+)$)
C0103:1282,4:FakeLib.SSL_CTX_set_tmp_ecdh: Invalid name "SSL_CTX_set_tmp_ecdh" (should match ((([a-z])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$|(__[a-z]+__)$|(\_.+)$|(test\_.+)$|((telnet_|sync_|_fromString_|auth_|perspective_|handleStatus_|_parseSVNEntries_|removeTrigger_|_process_|get_|xmlrpc_|iac_|macro_|handle_|ctcpQuery_|visitNode_a_|ctcpReply_|parse_|channel_|_toString_|observe_|func_|async_|spew_|dcc_|_stat_|convert_|state_|ftp_|global_|packet_|start_|ext_|_dataReceived_|class_|opt_|do_|request_|render_|ssh_|_unjelly_|soap_|agentc_|cmd_|key_|generate_|view_|ssh_USERAUTH_PK_OK_|end_|oscar_|type_|irc_|search_|remote_|linemode_|decode_|response_|login_|visitNode_).+)$)
C0103:1393,8:TestECCurve.test_nonECbinding: Invalid name "OBJ_sn2nid" (should match ((([a-z_])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$)
C0103:1423,8:TestECCurve.test_keyFails: Invalid name "EC_KEY_new_by_curve_name" (should match ((([a-z_])|([a-z]+_[a-z]))[a-zA-Z0-9]+)$)

... so I'm ignoring those.

WinXP buildbot fails because of that ridiculous ENOSPC bug, so whatever.

LGTM. Please merge.

comment:12 Changed 3 years ago by lvh

I'm sorry, I should probably clarify: please merge after addressing that one epytext markup thing.

comment:13 Changed 3 years ago by Hynek Schlawack

Thanks lvh. The twistedchecker error is bogus, because the docstring in question looks like this:

    An introspectable fake of cryptography's lib object.
    @ivar _createdKey: A set of keys that have been created by this instance.
    @type _createdKey: L{set} of L{FakeKey}
    @cvar NID_undef: A symbolic constant for undefined NIDs.
    @type NID_undef: L{FakeNID}

Therefore merging.

comment:14 Changed 3 years ago by Hynek Schlawack

Resolution: fixed
Status: newclosed

(In [41888]) Merge ninja-in-ecdhe-6586

Author: hynek Reviewer: lvh Fixes: #6586

OpenSSLCertificateOptions now tries best-effort to support ECDHE for servers.

Note: See TracTickets for help on using tickets.