[2.7] Backport ssl.MemoryBIO to Python 2.7 - PEP 546

@ncoghlan

BPO	22559
Nosy	@ncoghlan, @vstinner, @giampaolo, @tiran, @benjaminp, @glyph, @alex, @zware, @dstufft
PRs	bpo-22559: Implementation of the PEP 546: Backport MemoryBIO #2133
Dependencies	bpo-22569: Add support for weakrefs to _socket.socket bpo-29781: SSLObject.version returns incorrect value before handshake.
Files	memory-bio.diff issue225593.diff issue22559.diff issue22559.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/tiran'
closed_at = <Date 2020-04-27.03:26:46.304>
created_at = <Date 2014-10-05.20:25:55.967>
labels = ['extension-modules', 'expert-SSL', 'type-feature']
title = '[2.7] Backport ssl.MemoryBIO to Python 2.7 - PEP 546'
updated_at = <Date 2020-04-27.03:26:46.303>
user = 'https://github.com/alex'

bugs.python.org fields:

activity = <Date 2020-04-27.03:26:46.303>
actor = 'zach.ware'
assignee = 'christian.heimes'
closed = True
closed_date = <Date 2020-04-27.03:26:46.304>
closer = 'zach.ware'
components = ['Extension Modules', 'SSL']
creation = <Date 2014-10-05.20:25:55.967>
creator = 'alex'
dependencies = ['22569', '29781']
files = ['36820', '36827', '36829', '36868']
hgrepos = []
issue_num = 22559
keywords = ['patch']
message_count = 27.0
messages = ['228618', '228625', '228627', '228629', '228630', '228631', '228696', '228723', '228736', '229014', '229055', '229208', '229244', '235782', '235803', '251082', '251087', '251165', '293512', '293515', '295797', '295799', '295800', '295804', '301469', '301877', '367366']
nosy_count = 12.0
nosy_names = ['geertj', 'ncoghlan', 'janssen', 'vstinner', 'giampaolo.rodola', 'christian.heimes', 'benjamin.peterson', 'glyph', 'Arfrever', 'alex', 'zach.ware', 'dstufft']
pr_nums = ['2133']
priority = 'normal'
resolution = 'rejected'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22559'
versions = ['Python 2.7']

Attached patch is a first-cut at a backport patch. Note that it is not quite a 1-1 with the original:

The SSL module backport added a new field for the Python-level "SSLSocket" reference (ssl_sock), which was a different object from the _socket.socket (PySocketObject *) object. This is all effectively supplanted by the new owner field, so this patch does that as well.

The patch looks basically complete to me, but the tests hang for some reason: in a test where the client side hangs up (test_check_hostname) the server side of the socket blocks in a call to read(). I'm not sure why.

Antoine, or anyone else who worked on the original patch, did you run into any issues like this, or have any other insights into what might be causing it?

It seems that SSLSocket.close() doesn't actually close the socket, and that's why the server side read() blocks.

It's a bit of a mystery to me how socket.close(), which is called by SSLSocket to do the actual close, is supposed to work. I don't see any calls to _sock.close() in there..

If I add the following 3 lines to socket.py then it works (but there's a few unexpected EOF errors in return).

diff -r ae64614b66b7 Lib/socket.py
--- a/Lib/socket.py     Sat Oct 04 18:24:32 2014 -0400
+++ b/Lib/socket.py     Sun Oct 05 18:16:51 2014 -0400
@@ -192,6 +192,9 @@
     def close(self, _closedsocket=_closedsocket,
               _delegate_methods=_delegate_methods, setattr=setattr):
         # This function should not reference any globals. See issue python/cpython#39247.
+        if hasattr(self._sock, '_dummy'):
+            return
+        self._sock.close()
         self._sock = _closedsocket()
         dummy = self._sock._dummy
         for method in _delegate_methods:

I'm probably overlooking something b/c I can't imagine socket.close() being a no-op.

Right, socket._socketobject mearly nulls out the reference to _socket.socket, and lets reference counting take care of the rest.

I've more of less got this figured out:

When do_handshake() raises an exception (say, a CertificateError), then a reference to a traceback is stored for sys.exc_info()
This traceback holds a reference to a frame where ssl.SSLObject is self
ssl.SSLObject holds a reference to _ssl._SSLSocket
Which holds a reference to _socket.socket

This is avoided on Python3 because exceptions don't stick around, adding a sys.exc_clear() to that test causes it to not hang.

It seems like ssl.SSLSocket.close() should probably explicitly close the SSLObject somehow? I think this problem would appear on Python3 if you caught the exception manually and kept a reference to it?

Right, socket._socketobject mearly nulls out the reference to _socket.socket, and lets reference counting take care of the rest.

Ugh this is bad... I thought close() was exactly there when you don't want to depend on refcounting for cleanup.

When do_handshake() raises an exception (say, a CertificateError), then a reference to a traceback is stored for sys.exc_info()

This traceback holds a reference to a frame where ssl.SSLObject is self

ssl.SSLObject holds a reference to _ssl._SSLSocket

Which holds a reference to _socket.socket

On Python 3.x the last one above is a weakref.

It seems like ssl.SSLSocket.close() should probably explicitly close the SSLObject somehow? I think this problem would appear on Python3 if you caught the exception manually and kept a reference to it?

On Python 3.x socket.close() does a real close() on the socket, it seems. (thought it appears to have an app-level refcount for makefile()). I agree this is the best way but it seems very scary to make that change for 2.7.

I think that closing the socket in SSLSocket.close(), as you suggest, would work (using socket._sock.close()), or or maybe you can make the "Socket" member in _SSLSocket a weakref?

Unfortunately it can't be a weakref in python2 :-(

In Python3 socket._socketobject *subclasses* _socket.socket, so when we pass "self" to stuff, it's has the right C-level fields but it's also a Python-levle object so it can have a weakref.

In Python2 socket._socketobject composes with _socket.socket, so we pass "self._sock", that way it has teh right C-level fields. Unfortunately taking a weakref of _socket.socket is not allowed.

In Python2 socket._socketobject composes with _socket.socket, so we pass "self._sock", that way it has teh right C-level fields. Unfortunately taking a weakref of _socket.socket is not allowed.

I see, and agree that making it weakref-able would be a bad idea for a minor release.

As suggested by Benjamin, I've filed bpo-22569 to add weakref support to _socket.socket; that will address this and further reduce teh diff with Python3.

New patch works and passes all tests. It's on top of bpo-22569.

New patch is the same, it just rebases the socket changes out since Benjamin landed that (thanks!)

Updated patch cherry-picks in some of the documentation updates that were pushed by Victor.

Since this is such a new feature (not even released in 3.x), I don't think we should put it in 2.7.9.

Would you be ok with it going into 2.7.10? The biggest argument in favor of this is that it significantly reduces the diff between 2.x and 3.x's SSL module, specifically it removes the one major difference between the two of them.

We can reevaluate when we know when 2.7.10 will be released.

While I agree with the idea of backporting this at some point after it has been published in a 3.x release, Guido also specifically requested that we *not* treat PEP-466 as blanket permission to backport other network security features to Python 2.7. (I originally had that kind of wording in the PEP, and he convinced me to take it out to ensure we considered each future feature backport request on its own merits)

That means 2.7.x should track the 3.4.x maintenance branch until after 3.5 is released, and then it should start tracking the 3.5.x maintenance branch.

However, any new 3.5 ssl features will need their own backporting PEP, rather than relying on the previous approval in PEP-466 to backport ssl features from 3.4.

Since this is such a new feature (not even released in 3.x), I don't think we should put it in 2.7.9.

While ssl.MemoryBIO would be very useful on Windows for Trollius (to support SSL with the IOCP event loop), I also consider it as a new feature. It's a little bit borderline between feature and security fix.

Maybe it should be discussed on python-dev?

Note: MemoryBIO was added by issue bpo-21965 (first commit: a79003f25a41).

For what it's worth, it would still be great if this could be merged. It could help a lot with 2/3 migrations of async code that uses Twisted and wants to adopt some asyncio parts. Right now the stdlib SSL module is de-facto useless in those scenarios and depending on pyOpenSSL is pretty much the only choice, and the lack of availability of a memory BIO on 2.x is the biggest blocker.

Glyph, if this is just for Twisted you could ship the "sslcompat" module that I'm shipping with Gruvi. It backports the async IO stuff and a few other pieces through an extension module.

https://github.com/geertj/gruvi/blob/master/src/sslcompat.c

and

https://github.com/geertj/gruvi/blob/master/lib/gruvi/sslcompat.py

But having this in the stdlib itself is much cleaner of course.

I converted bpo-22559.diff to a pull request. I only had one conflict!

--- Lib/ssl.py
+++ Lib/ssl.py
@@ -727,11 +857,7 @@ class SSLSocket(socket):
         if not self._sslobj:
             raise ValueError("Read on closed or unwrapped SSL socket.")
         try:
-            if buffer is not None:
-                v = self._sslobj.read(len, buffer)
-            else:
-                v = self._sslobj.read(len or 1024)
-            return v
+            return self._sslobj.read(len, buffer)
         except SSLError as x:
             if x.args[0] == SSL_ERROR_EOF and self.suppress_ragged_eofs:
                 if buffer is not None:

I fixed it by using "return self._sslobj.read(len, buffer)".

I added a second commit to synchronize Lib/ssl.py with master:

add selected_alpn_protocol() method, needed by test_ssl
change how the default size of read() is defined
change how send()/write result is reported: handle SSL_ERROR_WANT_READ/SSL_ERROR_WANT_WRITE

I tagged my PR as [WIP] since I'm not sure if I backported bpo-20951 "SSLSocket.send() returns 0 for non-blocking socket" by mistake or not :-)

Please review the PR carefully!

You have to include the changes from bpo-29334, too.

Christian Heimes: "You have to include the changes from bpo-29334, too."

Right now, we are only allow to squash a patch serie into an unique patch. I would prefer to keep the fix of bpo-29334 as a separated commit, so merge it after MemoryBIO is merged. The change seems to change different lines, so it will be fine.

Sounds good to me! I'll take care of the other patch after you have committed your PR.

bpo-29781 must be backported to 2.7 to fix SSLSocket.version()

ssl_io_loop() currently ignores the timeout parameter: #3500 fixes it in the master branch.

With PEP-546 withdrawn/rejected and 2.7 at EOL, I'm closing the issue.

Navigation Menu

Repository navigation

[2.7] Backport ssl.MemoryBIO to Python 2.7 - PEP 546 #66749

4 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

[2.7] Backport ssl.MemoryBIO to Python 2.7 - PEP 546 #66749

Description

Activity

alex commented on Oct 5, 2014

geertj commented on Oct 5, 2014

alex commented on Oct 5, 2014

geertj commented on Oct 5, 2014

alex commented on Oct 5, 2014

geertj commented on Oct 5, 2014

alex commented on Oct 6, 2014

alex commented on Oct 6, 2014

alex commented on Oct 6, 2014

alex commented on Oct 10, 2014

benjaminp commented on Oct 11, 2014

alex commented on Oct 13, 2014

benjaminp commented on Oct 13, 2014

ncoghlan commented on Feb 12, 2015

vstinner commented on Feb 12, 2015

4 remaining items

glyph commented on May 11, 2017

geertj commented on May 11, 2017

vstinner commented on Jun 12, 2017

tiran commented on Jun 12, 2017

vstinner commented on Jun 12, 2017

tiran commented on Jun 12, 2017

tiran commented on Sep 6, 2017

vstinner commented on Sep 11, 2017

zware commented on Apr 27, 2020

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions