Opened 4 years ago

Closed 9 months ago

#5014 task closed fixed (fixed)

Drop support for pyOpenSSL < 0.10

Reported by: exarkun Owned by: Alex
Priority: normal Milestone:
Component: core Keywords:
Cc: thijs, davidsarah, hs@…, zooko@… Branch: branches/old-as-hell-openssl-5014
(diff, github, buildbot, log)
Author: glyph Launchpad Bug:

Description (last modified by thijs)

In #4854, we added support for using the memory bio features of pyOpenSSL >= 0.10 to implement IReactorSSL in all reactors.

In #4974, we will deprecate the support based on pyOpenSSL < 0.10.

After that deprecation has been in place for some time, we should remove support for working with pyOpenSSL < 0.10.

If #4974 is resolved for 11.1, then I think 12.1 would be the right time to do this. When we do, we can get rid of all of twisted/internet/_oldtls.py and the conditionals elsewhere which import that module when pyOpenSSL < 0.10 is in use.

Attachments (4)

t5014.diff (18.3 KB) - added by Alex 9 months ago.
t5014.2.diff (18.3 KB) - added by Alex 9 months ago.
Same patch with a better topfile.
t5014.3.diff (18.3 KB) - added by Alex 9 months ago.
Fixed!
t5014.4.diff (19.0 KB) - added by Alex 9 months ago.
Updated INSTALL file.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 4 years ago by exarkun

  • Milestone set to Twisted-12.1

comment:2 Changed 4 years ago by exarkun

(In [31537]) Merge protocol-ssl-4854-6

Author: exarkun
Reviewer: itamar, glyph, thijs
Fixes: #4854
Refs: #4974
Refs: #5014
Refs: #4455

Add an implementation of IReactorSSL and ITLSTransport which uses the memory
BIO APIs present in pyOpenSSL 0.10 and newer. This implementation will be preferred
by all reactors if the pyOpenSSL dependency is satisfied, otherwise the old
implementation will still be used.

This appears to have slightly better performance than the old implementation and
should avoid bugs like #4455.

comment:3 Changed 3 years ago by thijs

  • Cc thijs added
  • Description modified (diff)

comment:4 Changed 3 years ago by itamar

  • Milestone Twisted-12.1 deleted

Removing 12.1 milestone, since #4974 is still outstanding.

comment:5 Changed 3 years ago by davidsarah

  • Cc davidsarah added

comment:6 Changed 2 years ago by hynek

  • Cc hs@… added

comment:7 Changed 20 months ago by zooko

  • Cc zooko@… added

Changed 9 months ago by Alex

comment:8 Changed 9 months ago by Alex

  • Keywords review added

Changed 9 months ago by Alex

Same patch with a better topfile.

comment:9 Changed 9 months ago by adiroiban

The topfile is missing a 'R' from the start of the line.

Also, what do you think about starting the topfile with 'twisted.internet.tcp' to identify the place where changes were made.

Thanks!

comment:10 Changed 9 months ago by Alex

What do you mean "start the topfile with 'twisted.internet.tcp'", I was told the topfile should only mention the user visible changes, and not the internal details (i.e. oldtls).

comment:11 Changed 9 months ago by adiroiban

True. No need for 'twisted.internet.tcp'
There are still some typos in the message

Maybe used something based on example from: http://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles

Support for versions of pyOpenSSL older than 0.10 has been removed. Affected users should upgrade pyOpenSSL. 

Thanks!

Changed 9 months ago by Alex

Fixed!

comment:12 Changed 9 months ago by adiroiban

Thanks for the fix. Everything is fine by me and can be merged.

comment:13 Changed 9 months ago by exarkun

  • Keywords review removed
  • Owner set to Alex

The top-level INSTALL file still says that older versions of pyOpenSSL are supported.

Changed 9 months ago by Alex

Updated INSTALL file.

comment:14 Changed 9 months ago by Alex

  • Keywords review added

comment:15 Changed 9 months ago by glyph

  • Author set to glyph
  • Branch set to branches/old-as-hell-openssl-5014

(In [41985]) Branching to old-as-hell-openssl-5014.

comment:16 follow-up: Changed 9 months ago by radix

Looks good. I think it would be cool to say ", but version 0.14 is recommended for enhanced security." in the INSTALL and news files, but it's not imperative.

There are some trivial pyflakes and twistedchecker errors. It looks like behavioral tests are passing on all the builders, so please fix those and merge assuming all the rest of the builders are green.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 9 months ago by zooko

Replying to radix:

Looks good. I think it would be cool to say ", but version 0.14 is recommended for enhanced security." in the INSTALL and news files, but it's not imperative.

Could you be more specific about the "enhanced security"? For the Tahoe-LAFS project we're currently planning to pin our dependency on pyOpenSSL to < 0.14 in order to deal with https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2193 , and I'd like for us (and our users) to know exactly what security risks we might incur by doing that.

comment:18 in reply to: ↑ 17 Changed 9 months ago by glyph

Replying to zooko:

Replying to radix:

Looks good. I think it would be cool to say ", but version 0.14 is recommended for enhanced security." in the INSTALL and news files, but it's not imperative.

Could you be more specific about the "enhanced security"? For the Tahoe-LAFS project we're currently planning to pin our dependency on pyOpenSSL to < 0.14 in order to deal with https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2193 , and I'd like for us (and our users) to know exactly what security risks we might incur by doing that.

One thing is ECDHE key exchange. Another is that instead of a bunch of custom-written C code, pyOpenSSL is now almost entirely automatically generated from a verified set of static metadata, eliminating an entire class of potential bugs.

Pinning this dependency would IMHO be a serious mistake, but I'll discuss that with you on the relevant bug, not here.

comment:19 Changed 9 months ago by glyph

  • Keywords review removed

It would be good for INSTALL to mention the capabilities that 0.14 allows, but I think the NEWS file doesn't need to talk about "enhanced security"; it just describes the change being made here.

comment:20 Changed 9 months ago by glyph

Good to land, I'll do so.

comment:21 Changed 9 months ago by glyph

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

(In [41990]) Merge old-as-hell-openssl-5014: Drop support for older pyOpenSSL.

Author: Alex

Reviewer: exarkun, glyph

Fixes: #5014

Remove support for pyOpenSSL older than 0.10

Note: See TracTickets for help on using tickets.