classification
Title: New SSL implementation based on ssl.MemoryBIO
Type: enhancement Stage: resolved
Components: asyncio Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: geertj, giampaolo.rodola, gvanrossum, haypo, pitrou, python-dev, sbt, yselivanov
Priority: normal Keywords: patch

Created on 2014-10-05 22:28 by pitrou, last changed 2015-03-23 15:00 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
sslproto.patch pitrou, 2014-10-06 00:57 review
sslproto3.patch pitrou, 2014-10-16 00:13 review
sslproto-4.patch haypo, 2015-01-08 09:28 review
Messages (28)
msg228628 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-05 22:28
Now that #21965 is implemented, it is possible to improve SSL support in asyncio by making it independent of how the underlying event loop works (e.g. whether it is a Unix-like reactor or a proactor).
msg228632 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-06 00:57
Here is a proof-of-concept patch. I've only tested it under Linux, but it should be possible to write a simple _make_ssl_transport() for the BaseProactorEventLoop.
msg228634 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2014-10-06 02:13
This is awesome news!

Since this is 3.5 only, I guess this means the end of my attempts to keep the asyncio source code identical in the Tulip repo (from which I occasionally create builds that work with Python 3.3) and in the 3.4 and 3.5 branches.  I guess that's okay, people should be switching to 3.5 anyway.  I'm not sure what approach to follow to keep the branches at least somewhat in sync -- perhaps tulip and 3.4 can still be kept identical, with changes merged from 3.4 into 3.5 (default) but 3.5 differing in some places?  Or perhaps the code can be kept identical with the exception of the sslproto.py file, and conditional import of the latter?
msg228652 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-06 10:14
>  Or perhaps the code can be kept identical with the exception of the
> sslproto.py file, and conditional import of the latter?

I think that's reasonable, yes. The _SelectorSslTransport is still there and can be used if the ssl module is not recent enough.
msg229507 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-16 00:13
Here is an updated patch. It hooks into the Proactor event loop (tested under Windows) and also adds a fallback for older Pythons (with tests).
msg229835 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-10-22 20:56
Does someone want to review this?
msg229916 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2014-10-24 08:20
I will try to take a look next week.
msg232169 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2014-12-05 01:00
From issue 22768:

"""
> Maybe
> transport.get_extra_info('socket').getpeercert(True)
> would be okay, no patch needed?

That will be problematic with issue22560. The clear-text socket object and the SSL object become unrelated, and it would be logical for get_extra_info('socket') to return the clear-text socket, so either a get_extra_info('ssl') would be needed, or we should expose the SSL properties directly as extra info members.
"""
msg233282 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-01 12:04
Ping :-)
msg233283 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-01 12:13
Note this could probably help https://twitter.com/icgood/status/549915951165358080, which Victor seems to care about :-)
msg233299 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-01-01 22:59
Maybe we should just accept this without review?  I really don't have time to review 600+ lines of code, sorry.
msg233300 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-02 01:11
> Maybe we should just accept this without review?  I really don't have time to review 600+ lines of code, sorry.

SSL/TLS is very important and the patch is large, a review is required. I posted a first review with a lot of comments.
msg233301 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-02 01:14
Sorry for the delay. I understood that the change targets the proactor event loop, and I was busy to fix annoying random bugs in this code (it's not done yet, see for example the issue #23095 for the most recent bug). Windows is not my favorite OS, I am less intersted to fix Windows specific bugs, but sometimes I try to fix them.

I still don't understand if the change is (or should be) specific to the proactor event loop or not. Antoine, can you please elaborate the rationale of your patch?

Is the "legacy" code only used on Python 3.4 and older? Is ssl.MemoryBIO always present in Python 3.5 and newer?

I would like to see benchmarks of memory BIO vs current code on Linux and Windows. Even if it would make the code simpler to always use memory BIO, I care of network performances. Maybe we may only use memory BIO for the proactor event loop?

Do you know if other applications use memory BIO for networking with SSL?

Did you try your patch on Python 3.3? On Linux and Windows?

Anyway, great job!
msg233302 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-02 01:15
> Note this could probably help https://twitter.com/icgood/status/549915951165358080, which Victor seems to care about :-)

Copy of the tweet: "@gvanrossum Will we be seeing TLS upgrade support (e.g. STARTTLS) soon in asyncio / tulip? All threads and issues on the subject seem stale."

How will this patch help to support STARTTLS? Could you please elaborate Antoine?
msg233303 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-02 01:47
FYI Twisted supports SSL with IOCP using pyOpenSSL 0.10 (released in 2009) or newer. The support is based on twisted.protocols.tls.TLSMemoryBIOFactory.

It looks like the memory BIO implementation is now preferred on all platforms. See the twisted.internet._newtls module:
"""
This module implements memory BIO based TLS support.  It is the preferred
implementation and will be used whenever pyOpenSSL 0.10 or newer is installed
(whenever L{twisted.protocols.tls} is importable).

@since: 11.1
"""
msg233306 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2015-01-02 04:21
Oh, I think I understand how this could help STARTTLS. Glyph once explained it to me. STARTTLS takes an existing non-TLS Transport and layers a TLS Transport on top of it. This requires the TLS layer to read/write from the underlying Transport using the standard Transport/Protocol interface (i.e. call transport.write() to write bytes, expect protocol.data_received() to be called when bytes are read). The existing (3.4) ssl module cannot do this because the TLS implementation needs to wrap the socket directly; but (presumably) the BIO-based TLS implementation can do this.
msg233311 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2015-01-02 11:35
> Antoine, can you please elaborate the rationale of your patch?

The patch adds SSL support for proactor-based event loops (any event loop supporting plain sockets, actually, so it could also work for libuv etc.).

> Is the "legacy" code only used on Python 3.4 and older? Is ssl.MemoryBIO always present in Python 3.5 and newer?

Yes and yes.

> I would like to see benchmarks of memory BIO vs current code on Linux and Windows.

Do you have such benchmarks?

> Maybe we may only use memory BIO for the proactor event loop?

It sounds better to exercise the same code path under all platforms.

> Did you try your patch on Python 3.3?

No.

> How will this patch help to support STARTTLS?

Guido explained this one :-)
msg233617 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-08 01:39
I updated sslproto3.patch with my remarks: sslproto-4.patch

Main differences with sslproto3.patch (unsorted):

* write_eof raises NotImplementedError
* fix write_buffer_size: use data, not offset
* use tuples in the write backlog
* data_received exits the loop when it gets an empty string (ignore following data, but an empty string must be at the end of the list according to Antoine)
* SSLPipe._write_backlog is now a deque (should be more efficient since we remove the head of the list in _process_write_backlog)
* always store and pass the server hostname, even if SNI is not supported
* deny calling shutdown() twice
* Remove ConnectionAbortedError special case: I would like to reproduce to understand it, and document it
* Set the default read parameter to 256 KB instead of 64 KB: reuse constant from selector_events.py
* SSLPipe: use a different attribute to store the shutdown callback

Remarks:

* sslproto looks to be based on gruvi/ssl.py of the gruvi project written by Geert
Jansen. We should mention the author in the commit.
* _process_write_backlog(): I don't understand why offset is set to 1 for handshake and shutdown
msg233618 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-08 01:44
I prefer to use the same code on all platforms. I don't like the idea of SSL bugs specific to Windows.

With this change, it becomes possible to support "STARTTLS". IMO supporting this feature is more important than performance, even if I only expect a low overflow of the new _SSLPipe layer, so the benchmark is not needed.
msg233631 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-08 09:28
Oh, I wrote the patch for Tulip. Patch regenerated to use Python paths.
msg233697 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-08 23:23
For STARTTLS, see also this issue:
https://code.google.com/p/tulip/issues/detail?id=79
msg233971 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-13 23:21
New changeset 432b817611f2 by Victor Stinner in branch '3.4':
Issue #22560: New SSL implementation based on ssl.MemoryBIO
https://hg.python.org/cpython/rev/432b817611f2
msg233972 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-13 23:25
I commited sslproto-4.patch to Python 3.4, Python 3.5 and Tulip with minor changes:

- remove write_eof(), as suggested by Yury
- define SSLProtocol after its transport in sslproto.py
- add a docstring to SSLProtocol
- test sslcontext with "is not None"
- _make_ssl_transport: waiter parameter is now optional

Thanks Antoine for this great contribution! Thanks also to Geert Jansen since sslproto.py looks to be based on his work.

Even if I ran the test suite on Python 3.3, 3.4 and 3.5 on Linux, and on Python 3.5 on Windows, I prefer to wait one or two days to see how buildbots appreciate this enhancement.
msg233973 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-13 23:32
New changeset b9fbbe7103e7 by Victor Stinner in branch 'default':
Issue #22560, asyncio doc: ProactorEventLoop now supports SSL!
https://hg.python.org/cpython/rev/b9fbbe7103e7
msg234041 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-14 22:35
While I tried to write an unit test to reproduce a bug (cancelled waiter), I noticed that the handshake callback of the protocol can be called indirectly from _process_write_backlog(). So _process_write_backlog() may be called indirectly from _process_write_backlog(), whereas this function doesn't support reentrant call.

A fix is to modify the handshake callback to schedule a call to _process_write_backlog(), instead of calling it immediatly.

Note: The shutdown callback doesn't have this issue, it only calls transport.close().
msg234064 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-15 08:42
New changeset fb3761de0d3c by Victor Stinner in branch '3.4':
Issue #22560: Fix SSLProtocol._on_handshake_complete()
https://hg.python.org/cpython/rev/fb3761de0d3c
msg234065 - (view) Author: Roundup Robot (python-dev) Date: 2015-01-15 08:45
New changeset 3c37825d85d3 by Victor Stinner in branch '3.4':
Issue #22560: Fix typo: call -> call_soon
https://hg.python.org/cpython/rev/3c37825d85d3
msg234066 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-01-15 08:46
Buildbots are happy. There is no remaining things to do, I close the issue.
History
Date User Action Args
2015-03-23 15:00:17berker.peksagsetstage: patch review -> resolved
2015-01-15 08:46:17hayposetstatus: open -> closed
resolution: fixed
messages: + msg234066
2015-01-15 08:45:24python-devsetmessages: + msg234065
2015-01-15 08:42:26python-devsetmessages: + msg234064
2015-01-14 22:35:20hayposetmessages: + msg234041
2015-01-13 23:32:04python-devsetmessages: + msg233973
2015-01-13 23:25:10hayposetmessages: + msg233972
2015-01-13 23:21:46python-devsetnosy: + python-dev
messages: + msg233971
2015-01-09 08:58:55hayposettitle: Add loop-agnostic SSL implementation to asyncio -> New SSL implementation based on ssl.MemoryBIO
2015-01-08 23:23:45hayposetmessages: + msg233697
2015-01-08 09:28:38hayposetfiles: - sslproto-4.patch
2015-01-08 09:28:29hayposetfiles: + sslproto-4.patch

messages: + msg233631
2015-01-08 01:44:42hayposetmessages: + msg233618
2015-01-08 01:39:23hayposetfiles: + sslproto-4.patch

messages: + msg233617
2015-01-02 11:35:13pitrousetmessages: + msg233311
2015-01-02 04:21:19gvanrossumsetmessages: + msg233306
2015-01-02 01:47:21hayposetmessages: + msg233303
2015-01-02 01:15:36hayposetmessages: + msg233302
2015-01-02 01:14:46hayposetmessages: + msg233301
2015-01-02 01:11:52hayposetmessages: + msg233300
2015-01-01 22:59:56gvanrossumsetmessages: + msg233299
2015-01-01 12:13:35pitrousetmessages: + msg233283
2015-01-01 12:04:43pitrousetmessages: + msg233282
2014-12-05 01:00:22pitrousetmessages: + msg232169
2014-10-24 08:20:11hayposetmessages: + msg229916
2014-10-22 20:56:15pitrousetmessages: + msg229835
2014-10-16 00:27:30pitrousetstage: patch review
2014-10-16 00:14:05pitrousetfiles: + sslproto3.patch

messages: + msg229507
2014-10-06 10:14:06pitrousetmessages: + msg228652
2014-10-06 02:13:24gvanrossumsetmessages: + msg228634
2014-10-06 00:57:52pitrousetfiles: + sslproto.patch

nosy: + sbt
messages: + msg228632

keywords: + patch
2014-10-05 22:28:32pitrousettype: enhancement
components: + asyncio
2014-10-05 22:28:22pitroucreate