bpo-23749: Implement loop.start_tls()

[1st1]

https://bugs.python.org/issue23749

[asvetlov]

Add server_hostname parameter maybe?
At least create_connection has it, checking hostname match increases security.

[pitrou]

Yes, server_hostname should be added. Or perhaps a bunch of kwargs to allow passing arbitrary arguments to wrap_socket or wrap_bio.

[1st1]

Sure, will add it.

Or perhaps a bunch of kwargs to allow passing arbitrary arguments to wrap_socket or wrap_bio.

We don't do that in loop.create_connection or in loop.create_server, we explicitly pass params there. Most of the stuff can be configured through the SSLContext anyways.

[pitrou]

Yeah... I guess that could be deferred to another PR. However, the ssl module may add some other parameters along the way, and it would be nicer to inherit them automatically. For example, Python 3.6 got the session parameter: https://docs.python.org/3/library/ssl.html#ssl.SSLContext.wrap_socket

[1st1]

Sure. Can you make a PR after this one lands?

[asvetlov]

Clarification: asyncio doesn't use ssl.wrap_socket since Python 3.5

[pitrou]

Yes, but it uses ssl.wrap_bio which accepts similar arguments.

[asvetlov]

Looks good except minor note

[pitrou]

Is it necessary to rely on an implementation detail here?

[1st1]

Yes. Transports are intimately tied to event loops and are aware of each other private details. Another event loop, say uvloop, can't support asyncio Transport classes, and vice versa.

Currently, transports don't have APIs to notify the loop when their write buffers are drained. We probably need to design such a public API, but that's out of the scope of this PR.

That said, I think I'll add a private base class to share some functionality between Proactor and Selector transport classes. That will also make the code a bit prettier.

[pitrou]

Perhaps use transport.get_extra_info('socket')?

[asvetlov]

Since asyncio loop doesn't work uvloop transport (it's true for any loop implementation) using private attributes is perfectly fine from my perspective.

[pitrou]

Same question.

[pitrou]

Add docstrings in classes and important methods?

[1st1]

That's a private unittest helper class in Lib/test/test_asyncio/... I copied it from uvloop and I'm still in the process of refactoring it and adapting it to asyncio/Python needs. So I'll add docstrings later.

[1st1]

So after some investigation why loop.start_tls() did not work with the Proactor event loop, it turned out that creating a new transport is not the best approach. The Selector event loop supports that, but Proactor requires the old transport to be carefully detached from its underlying object. Doing that correctly is quite an elaborate task. Otherwise there are multiple races which all lead to mixed-up read and write events.

What I decided to do instead is to simply instantiate SSLProtocol directly in loop.start_tls(). That allowed me to pass an existing transport instance to it, eliminating the need to drain the buffer or creating a new transport. Now tests pass for the Proactor event loop on Windows.

The new approach is way safer than the previous one:

• No need to touch transports implementation.
• No need to drain the transport's write buffer -- SSLProtocol will simply write to the same buffer.
• Draining buffers is hard. The previous implementation did that incorrectly, and did not handle situations when a transport object is closed due to an error and write callback is never called.
• We reuse essentially the same code path that create_connection and create_server follow.