Opened 6 years ago

Closed 3 years ago

#5014 task closed fixed (fixed)

Drop support for pyOpenSSL < 0.10

Reported by: Jean-Paul Calderone Owned by: Alex Gaynor
Priority: normal Milestone:
Component: core Keywords:
Cc: Thijs Triemstra, davidsarah, Hynek Schlawack, zooko@… Branch: branches/old-as-hell-openssl-5014
branch-diff, diff-cov, branch-cov, buildbot
Author: glyph

Description (last modified by Thijs Triemstra)

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 Gaynor 3 years ago.
t5014.2.diff (18.3 KB) - added by Alex Gaynor 3 years ago.
Same patch with a better topfile.
t5014.3.diff (18.3 KB) - added by Alex Gaynor 3 years ago.
Fixed!
t5014.4.diff (19.0 KB) - added by Alex Gaynor 3 years ago.
Updated INSTALL file.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by Jean-Paul Calderone

Milestone: Twisted-12.1

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

(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 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Description: modified (diff)

comment:4 Changed 5 years ago by Itamar Turner-Trauring

Milestone: Twisted-12.1

Removing 12.1 milestone, since #4974 is still outstanding.

comment:5 Changed 5 years ago by davidsarah

Cc: davidsarah added

comment:6 Changed 5 years ago by Hynek Schlawack

Cc: Hynek Schlawack added

comment:7 Changed 4 years ago by zooko

Cc: zooko@… added

Changed 3 years ago by Alex Gaynor

Attachment: t5014.diff added

comment:8 Changed 3 years ago by Alex Gaynor

Keywords: review added

Changed 3 years ago by Alex Gaynor

Attachment: t5014.2.diff added

Same patch with a better topfile.

comment:9 Changed 3 years ago by Adi Roiban

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

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

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

Attachment: t5014.3.diff added

Fixed!

comment:12 Changed 3 years ago by Adi Roiban

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

comment:13 Changed 3 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Alex Gaynor

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

Changed 3 years ago by Alex Gaynor

Attachment: t5014.4.diff added

Updated INSTALL file.

comment:14 Changed 3 years ago by Alex Gaynor

Keywords: review added

comment:15 Changed 3 years ago by Glyph

Author: glyph
Branch: branches/old-as-hell-openssl-5014

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

comment:16 Changed 3 years 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 ; Changed 3 years 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 3 years 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 3 years 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 3 years ago by Glyph

Good to land, I'll do so.

comment:21 Changed 3 years ago by Glyph

Resolution: fixed
Status: newclosed

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