bpo-23749: Implement loop.start_tls() by 1st1 · Pull Request #5039 · python/cpython

1st1 · 2017-12-29T05:45:31Z

https://bugs.python.org/issue23749

asvetlov · 2017-12-29T07:40:11Z

        raise NotImplementedError

+    async def start_tls(self, transport, protocol, sslcontext, *,
+                        server_side=False,


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

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

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.

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

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

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

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

asvetlov

Looks good except minor note

pitrou · 2017-12-29T09:23:50Z

+                f'transport {self!r} is not supported by start_tls()')
+
+        if transport.get_write_buffer_size():
+            if transport._buffer_drained_fut is not None:


Is it necessary to rely on an implementation detail here?

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 · 2017-12-29T09:24:58Z

+
+        waiter = self.create_future()
+        app_transport = self._make_ssl_transport(
+            transport._sock, protocol, sslcontext, waiter,


Perhaps use transport.get_extra_info('socket')?

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 · 2017-12-29T09:25:44Z

+            transport._sock, protocol, sslcontext, waiter,
+            server_side=server_side,
+            ssl_handshake_timeout=ssl_handshake_timeout,
+            server=transport._server,


Same question.

pitrou · 2017-12-29T09:29:55Z

+import threading
+
+
+class FunctionalTestCaseMixin:


Add docstrings in classes and important methods?

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 · 2017-12-30T05:28:04Z

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.

1st1 self-assigned this Dec 29, 2017

1st1 requested a review from asvetlov December 29, 2017 05:45

the-knights-who-say-ni added the CLA signed label Dec 29, 2017

bedevere-bot added the awaiting merge label Dec 29, 2017

1st1 force-pushed the start_tls branch from af80627 to bf71f87 Compare December 29, 2017 05:46

1st1 changed the title ~~Start tls~~ bpo-23749: Implement loop.start_tls() Dec 29, 2017

asvetlov reviewed Dec 29, 2017

View reviewed changes

asvetlov approved these changes Dec 29, 2017

View reviewed changes

pitrou reviewed Dec 29, 2017

View reviewed changes

1st1 force-pushed the start_tls branch 2 times, most recently from c495ece to 64a4469 Compare December 30, 2017 04:52

bpo-23749: Implement loop.start_tls()

961b2b6

1st1 force-pushed the start_tls branch from 64a4469 to 961b2b6 Compare December 30, 2017 05:19

1st1 merged commit f111b3d into python:master Dec 30, 2017

bedevere-bot removed the awaiting merge label Dec 30, 2017

1st1 deleted the start_tls branch December 30, 2017 05:35

Uh oh!

Conversation

1st1 commented Dec 29, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asvetlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Dec 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

1st1 commented Dec 29, 2017 •

edited by bedevere-bot

Loading

1st1 Dec 29, 2017 •

edited

Loading

1st1 commented Dec 30, 2017 •

edited

Loading