Don't set just 1 curve for ECDHE.

[kroeckx]

This fixes track ticket https://tm.tl/9210.

Contributor Checklist:

• There is an associated ticket in Trac. Create a new one at https://twistedmatrix.com/trac/newticket
• I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
• I have updated the automated tests.

I would like to point out that I've never written any python code, so please at least check that what I'm doing makes sense, I'm really just guessing what I should do. I for instance have no idea whether self._lib.set_ecdh_auto exists at all. I did not test this with OpenSSL 1.0.2, but it seems to works with OpenSSL 1.1.0.

[glyph]

Hi @kroeckx - had you intended to submit this ticket for review? The trac ticket does not have a review keyword on it.

[kroeckx]

I think I've done so now.

[kroeckx]

Can someone please look at this? This fixes a problem preventing people from connecting to certain sites.

[richvdh]

@kroeckx could I suggest you add a news fragment as per https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles, which I think will fix the CI failure?

[markrwilliams]

Unfortunately the code in this PR isn't quite right. The situation is complicated because of the interactions of two separate libraries with shifting APIs:

OpenSSL

1. Versions before 1.0.2 allow configuring ECDH on Contexts by calling SSL_CTX_set_tmp_ecdh with a key representing the desired curve;
2. Version 1.0.2 introduced SSL_CTX_set_ecdh_auto, which automatically selects "the highest curve."
3. Version 1.1 removed SSL_CTX_set_ecdh_auto because Contexts automatically select the best curve.

pyOpenSSL

4. Version 17.0.0 and later automatically call SSL_CTX_set_ecdh_auto if it's available. There is no public API to confirm this occurred.

As @kroeckx's contribution demonstrates, the source of this bug is that Twisted always explicitly sets the ECDH curve and key, and thus implements only case 1 when it should support cases 1-3.

Twisted currently depends on pyOpenSSL 16.0.0 or later, so it cannot assume case 4. (An additional problem is that Twisted calls private APIs to set the ECDH curve on the Context; #928 fixes this.)

I think we'll have to check the version of OpenSSL cryptography is linked against as well the version of pyOpenSSL to achieve the intent of @kroeckx's contribution:

• if OpenSSL >= 1.1.0, Twisted should do nothing (case 3)
• if pyOpenSSL >= 17.0.0, Twisted should do nothing (cases 2 & 4)
• otherwise, Twisted should do what it does now (case 1)

[markrwilliams]

This bug affects Synapse as well: matrix-org/synapse#2350

[kroeckx]

My intention was to support OpenSSL 1.0.2+, because OpenSSL 1.0.1 is no longer supported. Since twisted now actually sets the wrong thing, we at least need to change twisted to support OpenSSL 1.1, and we could just do the support for OpenSSL 1.0.2+ regardless of the version of pyOpenSSL. But I guess in the long time if you could require >= 17, you could remove the other code.

Looking at the patch in pyOpenSSL, my call is probably wrong and I'll update it. It also only supports OpenSSL 1.0.2+

[kroeckx]

So I've just rebased and pushed a new version.

[markrwilliams]

I've stolen @alex's query from cryptography's issue tracking 1.0.1 deprecation and modified it to track the OpenSSL versions of Twisted installations:

SELECT
  STRFTIME_UTC_USEC(timestamp, "%Y-%m") AS yyyymm,
  ROUND(100 * SUM(CASE
        WHEN SUBSTR(REGEXP_EXTRACT(details.openssl_version, r"^OpenSSL ([^ ]+) "), 0, 5) = "0.9.8" THEN 1
        ELSE 0 END) / COUNT(*), 1) AS percent_098,
  ROUND(100 * SUM(CASE
        WHEN SUBSTR(REGEXP_EXTRACT(details.openssl_version, r"^OpenSSL ([^ ]+) "), 0, 5) = "1.0.0" THEN 1
        ELSE 0 END) / COUNT(*), 1) AS percent_100,
  ROUND(100 * SUM(CASE
        WHEN SUBSTR(REGEXP_EXTRACT(details.openssl_version, r"^OpenSSL ([^ ]+) "), 0, 5) = "1.0.1" THEN 1
        ELSE 0 END) / COUNT(*), 1) AS percent_101,
  ROUND(100 * SUM(CASE
        WHEN SUBSTR(REGEXP_EXTRACT(details.openssl_version, r"^OpenSSL ([^ ]+) "), 0, 5) = "1.0.2" THEN 1
        ELSE 0 END) / COUNT(*), 1) AS percent_102,
  ROUND(100 * SUM(CASE
        WHEN SUBSTR(REGEXP_EXTRACT(details.openssl_version, r"^OpenSSL ([^ ]+) "), 0, 5) = "1.1.0" THEN 1
        ELSE 0 END) / COUNT(*), 1) AS percent_110,
  COUNT(*) AS download_count
FROM
  TABLE_DATE_RANGE( [the-psf:pypi.downloads], DATE_ADD(CURRENT_TIMESTAMP(), -1, "year"), CURRENT_TIMESTAMP() )
WHERE
  details.openssl_version IS NOT NULL
  AND file.project = 'twisted'
GROUP BY
  yyyymm
ORDER BY
  yyyymm DESC
LIMIT
  100

Here's what BigQuery has to say:

Row	yyyymm	percent_098	percent_100	percent_101	percent_102	percent_110	download_count
1	2017-11	4.4	0.3	47.4	42.7	2.8	186559
2	2017-10	4.6	0.0	46.9	43.4	2.7	296765
3	2017-09	4.6	0.0	45.5	41.0	2.3	285888
4	2017-08	5.0	0.0	42.1	42.7	1.9	321476
5	2017-07	6.0	0.0	46.8	43.3	1.9	279008
6	2017-06	6.5	0.0	46.1	44.0	1.4	261729
7	2017-05	7.3	0.0	44.3	45.2	1.2	255016
8	2017-04	10.9	0.0	42.0	44.4	0.7	275824
9	2017-03	13.8	0.0	43.8	40.6	0.6	295205
10	2017-02	15.3	0.0	43.3	39.8	0.6	274493

It looks like half our users use OpenSSL 1.0.1. That unfortunately argues in favor of supporting it.

[alex]

It's worth noting that for users who obtained a cryptography wheel, they'll get an OpenSSL 1.1.0; so that's really a pessimistic number.

[kroeckx]

So I've pushed a version that checks the OpenSSL version.

[kroeckx]

So do you think this patch is ready, or do you want me to make other changes?

[codecov]

Codecov Report

Merging #927 into trunk will decrease coverage by 0.36%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##            trunk     #927      +/-   ##
==========================================
- Coverage    91.9%   91.54%   -0.37%     
==========================================
  Files         840      840              
  Lines      149182   149188       +6     
  Branches    13073    13074       +1     
==========================================
- Hits       137106   136572     -534     
- Misses       9979    10503     +524     
- Partials     2097     2113      +16

[adiroiban]

See the work from #928 . I think that the changes from here are in conflict with that PR.
@kroeckx please get in touch with @markrwilliams to see how to move this forward

Thanks!

[kroeckx]

I would like to get this fixed in Debian stable, which uses pyOpenSSL 16.2. I think the intention of #928 is to rely on a newer version of pyOpenSSL, but then there doesn't seem to be a version of pyOpenSSL yet that does the right thing. So I prefer to use this simple patch for now, which should make it easy for people to backport it.

[adiroiban]

@kroeckx my call was to get hold of @markrwilliams and agree on a way forward or at least leave your feedback / review on #928

As a reviewer, I have approved #928 ... so if you don't want it merged please leave a comment on that PR with your objection :)

There are already a lot of people involved in this PR, so I will hold my review for now :)

---

Regarding the lack of newer pyOpenSSL in distro X. If you want to use pyOpenSSL from distro X my answer is to also use the Twisted version provided by the distro.

And if you manage to get a newer version of Twisted in distro X you should also get a newer version of pyOpenSSL

This is just my view.

In Twisted we already support a lot of platforms / operations system and this is hard.
If we go with supporting multiple python libraries, things will just get harder.
We already have few resources available I don't know how we can make it.

[markrwilliams]

I would like to get this fixed in Debian stable, which uses pyOpenSSL 16.2. I think the intention of #928 is to rely on a newer version of pyOpenSSL, but then there doesn't seem to be a version of pyOpenSSL yet that does the right thing.

Twisted depends on pyOpenSSL 16.0.0, and set_tmp_ecdh has been around since at least 16.0.0. #928 removes old code that was intended to work with pyOpenSSL ~0.14. It will work fine with Debian.

[markrwilliams]

I was surprised to see that this was green, since Twisted ends up installing cryptography 2.1.3, which should include OpenSSL 1.1.

It turns out we have a Python 3.3 Travis environment with OpenSSL 1.0.1f The cryptography project doesn't ship manylinux1 wheels for Python 3.3, so this job builds cryptography 2.1.3 against OpenSSL 1.0.1 and runs the code in this block.

That's fine for this PR but bad in general because Python 3.3 has reached its end of life and Twisted will probably drop support for it. We'll need builders with OpenSSL 1.0.1 if we're going to support it going forward.

[markrwilliams]

The ; should go.

[markrwilliams]

I have no idea why GitHub thinks I closed this.