Opened 3 years ago

Closed 2 years ago

#6586 enhancement closed fixed (fixed)

Support (and enable by default) ECDHE key-exchange

Reported by: jknight Owned by: hynek
Priority: normal Milestone:
Component: core Keywords:
Cc: zooko@… Branch: branches/ninja-in-ecdhe-6586
(github, coverage, patch, buildbot, log)
Author: hynek

Description (last modified by thijs)

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 3 years ago by thijs

  • Description modified (diff)

Fix description markup.

comment:2 Changed 3 years ago by exarkun

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 3 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 3 years ago by zooko

  • Cc zooko@… added

comment:5 Changed 2 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 2 years ago by hynek

  • Author set to hynek
  • Branch set to branches/ninja-in-ecdhe-6586

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

comment:7 Changed 2 years ago by hynek

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

refs #6586

comment:8 Changed 2 years ago by lvh

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

comment:9 Changed 2 years ago by hynek

  • 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 2 years ago by lvh

  • Owner set to lvh

comment:11 Changed 2 years ago by lvh

  • Keywords review removed
  • Owner changed from lvh to hynek

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 2 years ago by lvh

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

comment:13 Changed 2 years ago by hynek

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 2 years ago by hynek

  • Resolution set to fixed
  • Status changed from new to closed

(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.