classification
Title: [2.7] Backport ssl.MemoryBIO to Python 2.7 - PEP 546
Type: enhancement Stage:
Components: Extension Modules, SSL Versions: Python 2.7
process
Status: open Resolution:
Dependencies: 22569 29781 Superseder:
Assigned To: christian.heimes Nosy List: Arfrever, alex, benjamin.peterson, christian.heimes, dstufft, geertj, giampaolo.rodola, glyph, janssen, ncoghlan, vstinner
Priority: normal Keywords: patch

Created on 2014-10-05 20:25 by alex, last changed 2017-09-11 15:11 by vstinner.

Files
File name Uploaded Description Edit
memory-bio.diff alex, 2014-10-05 20:25 review
issue225593.diff alex, 2014-10-06 18:00
issue22559.diff alex, 2014-10-06 21:17
issue22559.diff alex, 2014-10-10 16:32
Pull Requests
URL Status Linked Edit
PR 2133 open vstinner, 2017-06-12 15:56
Messages (26)
msg228618 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-05 20:25
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?
msg228625 - (view) Author: Geert Jansen (geertj) * Date: 2014-10-05 22:21
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 #808164.
+        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.
msg228627 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-05 22:25
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?
msg228629 - (view) Author: Geert Jansen (geertj) * Date: 2014-10-05 22:49
> 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?
msg228630 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-05 22:54
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.
msg228631 - (view) Author: Geert Jansen (geertj) * Date: 2014-10-05 23:11
> 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.
msg228696 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-06 14:36
As suggested by Benjamin, I've filed issue22569 to add weakref support to _socket.socket; that will address this and further reduce teh diff with Python3.
msg228723 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-06 18:00
New patch works and passes all tests. It's on top of issue22569.
msg228736 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-06 21:17
New patch is the same, it just rebases the socket changes out since Benjamin landed that (thanks!)
msg229014 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-10 16:32
Updated patch cherry-picks in some of the documentation updates that were pushed by Victor.
msg229055 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-10-11 01:09
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.
msg229208 - (view) Author: Alex Gaynor (alex) * (Python committer) Date: 2014-10-13 02:02
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.
msg229244 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-10-13 13:57
We can reevaluate when we know when 2.7.10 will be released.
msg235782 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-02-12 00:59
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.
msg235803 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2015-02-12 08:06
> 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 #21965 (first commit: a79003f25a41).
msg251082 - (view) Author: Donald Stufft (dstufft) * (Python committer) Date: 2015-09-19 14:59
Now that 3.5 is out, does that mean we can sync 2.7 with the 3.5 ssl again and land this patch?
msg251087 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-09-19 15:51
I don't know or remember what the whole PEP says. In doubt, I suggest floating it by python-dev to get a more informed answer.
msg251165 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2015-09-20 11:38
Guido specifically asked for a new PEP for each resync that explicitly enumerated the changes being backported, after earlier drafts of PEP 466 requested blanket future permission for network security related backports.

That approach actually turns out to be better from a documentation and backport management perspective - I organised the "New features in maintenance releases" section of the Python 2.7 What's New by PEP number, and "we backported PEP 466" nicely describes the changes that were backport to RHEL 7.2.

A new PEP for resyncing with Python 3.5 can be a lot simpler than PEP 466 was, though - the basic rationale doesn't need to be repeated, and the PEP 466 backport already brought the implementations much closer together.
msg293512 - (view) Author: Glyph Lefkowitz (glyph) Date: 2017-05-11 17:38
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.
msg293515 - (view) Author: Geert Jansen (geertj) * Date: 2017-05-11 19:59
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.
msg295797 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-12 16:00
I converted issue22559.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!
msg295799 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-06-12 16:03
You have to include the changes from #29334, too.
msg295800 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-12 16:06
Christian Heimes: "You have to include the changes from #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.
msg295804 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-06-12 17:14
Sounds good to me! I'll take care of the other patch after you have committed your PR.
msg301469 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-09-06 13:47
#29781 must be backported to 2.7 to fix SSLSocket.version()
msg301877 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-11 15:11
ssl_io_loop() currently ignores the timeout parameter: https://github.com/python/cpython/pull/3500 fixes it in the master branch.
History
Date User Action Args
2017-09-11 15:11:52vstinnersetmessages: + msg301877
2017-09-06 13:47:01christian.heimessetdependencies: + SSLObject.version returns incorrect value before handshake.
messages: + msg301469
2017-06-12 17:14:10christian.heimessetmessages: + msg295804
2017-06-12 16:30:21pitrousetnosy: - pitrou
2017-06-12 16:07:42vstinnersettitle: [backport] ssl.MemoryBIO -> [2.7] Backport ssl.MemoryBIO to Python 2.7 - PEP 546
2017-06-12 16:06:52vstinnersetmessages: + msg295800
2017-06-12 16:03:59christian.heimessetmessages: + msg295799
2017-06-12 16:00:17vstinnersetmessages: + msg295797
2017-06-12 15:56:21vstinnersetpull_requests: + pull_request2187
2017-05-11 19:59:53geertjsetmessages: + msg293515
2017-05-11 17:38:59glyphsetnosy: + glyph
messages: + msg293512
2016-09-15 08:20:52christian.heimessetassignee: christian.heimes
type: enhancement
components: + Extension Modules, SSL
2015-09-20 11:38:40ncoghlansetmessages: + msg251165
2015-09-19 15:51:44pitrousetmessages: + msg251087
2015-09-19 14:59:07dstufftsetmessages: + msg251082
2015-02-12 08:06:49vstinnersetnosy: + vstinner
messages: + msg235803
2015-02-12 00:59:24ncoghlansetnosy: + ncoghlan
messages: + msg235782
2014-10-13 20:02:54Arfreversetnosy: + Arfrever
2014-10-13 13:57:51benjamin.petersonsetmessages: + msg229244
2014-10-13 02:02:06alexsetmessages: + msg229208
2014-10-11 01:09:19benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg229055
2014-10-10 16:32:22alexsetfiles: + issue22559.diff

messages: + msg229014
2014-10-06 21:17:54alexsetfiles: + issue22559.diff

messages: + msg228736
2014-10-06 18:00:10alexsetfiles: + issue225593.diff

messages: + msg228723
2014-10-06 14:36:26alexsetdependencies: + Add support for weakrefs to _socket.socket
messages: + msg228696
2014-10-05 23:11:00geertjsetmessages: + msg228631
2014-10-05 22:54:39alexsetmessages: + msg228630
2014-10-05 22:49:23geertjsetmessages: + msg228629
2014-10-05 22:25:10alexsetmessages: + msg228627
2014-10-05 22:21:57geertjsetmessages: + msg228625
2014-10-05 21:10:45geertjsetnosy: + geertj
2014-10-05 20:25:55alexcreate