Repository navigation
asyncio missing wrap_socket (starttls) #67937
Description
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/1st1'
closed_at = <Date 2018-05-28.16:40:26.762>
created_at = <Date 2015-03-23.13:59:59.558>
labels = ['type-feature', '3.7', 'expert-asyncio']
title = 'asyncio missing wrap_socket (starttls)'
updated_at = <Date 2018-05-28.22:47:51.658>
user = 'https://bugs.python.org/gc'bugs.python.org fields:
activity = <Date 2018-05-28.22:47:51.658>
actor = 'vstinner'
assignee = 'yselivanov'
closed = True
closed_date = <Date 2018-05-28.16:40:26.762>
closer = 'yselivanov'
components = ['asyncio']
creation = <Date 2015-03-23.13:59:59.558>
creator = 'gc'
dependencies = []
files = ['40866']
hgrepos = []
issue_num = 23749
keywords = ['patch']
message_count = 41.0
messages = ['239021', '239022', '239026', '242095', '242097', '242136', '242155', '242193', '242194', '242195', '242197', '242198', '242199', '245532', '248048', '248051', '248053', '248054', '252513', '253495', '253496', '253888', '265494', '269417', '269439', '276790', '278161', '278162', '293912', '301628', '309220', '309235', '309236', '309645', '309648', '310577', '311057', '311117', '311150', '317222', '317942']
nosy_count = 14.0
nosy_names = ['barry', 'siemer', 'vstinner', 'christian.heimes', 'asvetlov', 'alex.gronholm', 'python-dev', 'yselivanov', 'msornay', 'Elizacat', 'gc', 'sphawk', 'Alex Gr\xc3\xb6nholm', 'Frzk']
pr_nums = ['5039', '5409']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue23749'
versions = ['Python 3.7']
Activity
gc commentedon Mar 23, 2015
It's not possible to wrap a socket in tls. The StreamWriter object should have an option to start a tls negotiation using the SSLContext of the server.
This is needed for protocols the have a "start_tls" feature, for example the ldap protocol.
In a non async program it's very easy:
wrapped_socket = ssl_context.wrap_socket(connection.socket, server_side=True, do_handshake_on_connect=True)
there should be something similar in the StreamWriter interface:
yield from writer.wrap_socket()
Bye,
Giovanni
vstinner commentedon Mar 23, 2015
Yes, it's not supported yet. It was already requested in this issue:
https://code.google.com/p/tulip/issues/detail?id=79
asyncio got a new SSL implementation which makes possible to implement STARTTLS. Are you interested to implement it?
gc commentedon Mar 23, 2015
Thanks, I will look to the new implementation of ssl in 3.5, and try to adapt it for my project (sldap3). I'd like to help, but I'm not skilled in asynchronous programming so I'm not sure if I succeed.
Bye,
Giovanni
Elizacat commentedon Apr 27, 2015
What needs to be done to make this happen? I can try to implement it.
Elizacat commentedon Apr 27, 2015
For what it's worth, IRC has an optional STARTTLS extension which is implemented by some servers. IMAP and SMTP also have STARTTLS as a fundamental component of their protocols. As gc pointed out, LDAP also supports it.
IMO this is a pretty glaring omission.
gvanrossum commentedon Apr 27, 2015
We didn't do this originally because the 3.4 SSL module didn't have this functionality (or maybe it was 3.3 that didn't have this) but now that we do have it I'd be very happy if you could implement this!
I'm not sure what the right interface is, probably coroutine methods on StreamReader and StreamWriter that take an SSLContext object (and from then on the reader/writer is using SSL), but there also would have to be a lower-level way to do the same thing to a Transport. This would probably have to return a new Transport that uses the original, wrapped transport for reading/writing.
You probably should write a small test app that proves this works for real too. Perhaps start with a synchronous test app that uses the existing wrap_socket() and then work on the async interface until you can reproduce the same thing there.
Let us know if you need more information.
pitrou commentedon Apr 27, 2015
So you need to:
have an API to wrap a clear-text protocol in a SSL protocol (see e.g. BaseProactorEventLoop._make_ssl_transport()... note how there's a waiter argument, what should be done with that?)
be able to replace a protocol with another on the transport (perhaps using a new, optional Transport API?)
perhaps a higher-level API that combines the two
Also for convenience a protocol should probably be able to inspect whether it has *already* been wrapped.
Elizacat commentedon Apr 28, 2015
It seems pretty simple to just make a function that returns a new transport, something like "transport = yield from loop.ssl_wrap_transport(transport)". I'm not sure how to handle plaintext data left on the wire, though, unless that's not really a consideration (given most (all?) real-world protocols can (and usually do) wait for the SSL handshake before sending more data when STARTTLS has been requested).
For the higher-level API, I'm thinking "reader, writer = asyncio.ssl_wrap(reader, writer)" maybe? You can't have half-closed SSL connections, so you would have to pass them both in.
As for replacing the protocol but keeping the transport, what would be the semantics of that? I can't really think of how to do that one. I do know SMTP clears all state, but some protocols might not (IRC is a key example - this isn't usually a problem since you are supposed to negotiate it early on before you log onto the server), so this shouldn't be mandatory.
Elizacat commentedon Apr 28, 2015
Reading the source now (just woke up, sorry!), the new protocol thing makes sense. I'm not sure what to do with the waiter argument or how to handle that.
What I'm really trying to think of here is how to handle copying of state. I guess users will just have to do it by hand if they want to do that? I don't know if keeping the same protocol is practical and just wrapping the transport is a good idea :(.
pitrou commentedon Apr 28, 2015
The protocol is not really replaced, it's wrapped.
Before:
SocketTransport <- UserProtocol
After:
SocketTransport <- (asyncio.sslproto.SSLProtocol
<- asyncio.sslproto._SSLProtocolTransport) <- UserProtocol
That way, the same SocketTransport (but it could be something else, e.g. a pipe transport) is always bound to the event loop; we simply insert a processing layer in the chain between the original transport and the final protocol. There are two distinct objects so that the SocketTransport sees a protocol and the UserProtocol sees a transport; but those two objects work hand in hand.
gvanrossum commentedon Apr 28, 2015
That sounds like a good plan for the top-level APIs.
But I think we may need to think about low-level APIs that handle Transports and Protocols as well.
The design I had been thinking of does not do any socket-level manipulation (in fact it doesn't care if the Transport has a socket or some other way to connect to a networking peer) but it does require some cooperation of the Transport and Protocol.
Let me draw some ASCII art.
In the initial state (no ssl) we have an App embodied in a Protocol, talking to a Transport which abstracts away a network connection:
(I.e., when the app wants to write to the network, it calls transport.write(); when the network has data for the app, it calls protocol.data_received().)
In the final state (ssl established) I was thinking the picture would look like this (leaving the method names out):
App <=> Protocol [ ] HelperTransport <=> HelperProtocol [ ] Transport <=> Network
[ ----> ] [ ----> ]
Here the Protocol at the far left and the Transport at the far right are the same objects as in the first diagram; however we've inserted a pair of helpers that handle SSL. Once the SSL connection is flowing, a write() by the app gives the data to the helper, which gives it to the SSL library. When the SSL library wants to send some (encrypted etc.) data to the network it calls write() on the rightmost Transport (the original one, which still owns the network connection). Conversely, when data arrives over the network, it is still given to the rightmost Transport via its data_received() method. This Transport then gives it to the SSL library, which eventually decrypts it (etc.) and gives it to the helper, which then calls data_received() with the decrypted plaintext on the leftmost Protocol (i.e. the App).
The clever trick here is that the Protocol on the left still talks to a Transport, it's just a different Transport (owned by the helpr); similarly, the Transport on the right still talks to a Protocol, it's just a different one owned by the helper.
People have proposed general stacks of Protocol/Transport pairs, but so far I haven't seen much of a use case for that. This might be that use case. In order to switch the arrangements, the helper code (which is ultimately invoked by your loop.ssl_wrap_transport() call) must first create the HelperTransport and HelperProtocol halves of the SSL helper layer, and then call set_transport()/set_protocol() on the existing App Protocol and Network Transport to change their respective associated transport and protocol.
Note that the relationship between HelperTransport and HelperProtocol is not the same as that between associated Transport/Protocol pairs; the interface they use to talk to each other is completely internal to the implementation of the helper (and probably determined mostly by the needs of the underlying SSL BIO interface).
Hope this helps (and hope others on the issue agree).
--Guido
pitrou commentedon Apr 28, 2015
Good :-)
I'm not sure. Apparently it's used for create_connection(), so perhaps it's not necessary here? Perhaps Victor or Guido can give some insight...
Well, there shouldn't be any copying necessary. The user just continues using the same protocol instance; it just calls a different transport.
gvanrossum commentedon Apr 28, 2015
Looks like Antoine drew the same diagram but quicker. :-)
Regarding the waiter arg, you can leave that None if you don't need it. It is intended to give the caller a way to block (using event loop machinery) until the connection is ready. But if your caller doesn't need it that's fine.
24 remaining items
1st1 commentedon Dec 30, 2017
New changeset f111b3d by Yury Selivanov in branch 'master':
bpo-23749: Implement loop.start_tls() (bpo-5039)
f111b3d
warsaw commentedon Dec 30, 2017
@yselivanov - thanks for adding this, it's a huge win.
I think the feature is significant enough for a What's New entry.
1st1 commentedon Dec 30, 2017
Sure, Elvis and I will go through all NEWS items when it's time for what's new ;)
vstinner commentedon Jan 7, 2018
http://buildbot.python.org/all/#/builders/58/builds/435
Tests fail on x86 Windows7 3.x:
======================================================================
ERROR: test_start_tls_client_1 (test.test_asyncio.test_sslproto.ProactorStartTLSTests)
----------------------------------------------------------------------
======================================================================
ERROR: test_start_tls_client_1 (test.test_asyncio.test_sslproto.SelectorStartTLSTests)
----------------------------------------------------------------------
vstinner commentedon Jan 7, 2018
SelectorStartTLSTests failed once on x86 Tiger 3.x in build 453, but then passed, no idea why.
http://buildbot.python.org/all/#/builders/30/builds/453
======================================================================
ERROR: test_start_tls_client_1 (test.test_asyncio.test_sslproto.SelectorStartTLSTests)
----------------------------------------------------------------------
======================================================================
ERROR: test_start_tls_server_1 (test.test_asyncio.test_sslproto.SelectorStartTLSTests)
----------------------------------------------------------------------
vstinner commentedon Jan 24, 2018
I created a more specific issue: bpo-32645, test_asyncio: TLS tests fail on "x86 Windows7" buildbot.
1st1 commentedon Jan 29, 2018
New changeset 1e5b25b by Yury Selivanov in branch 'master':
bpo-23749: Make start-tls tests more stable on win7 buildbot (GH-5409)
1e5b25b
vstinner commentedon Jan 29, 2018
test_start_tls_server_1() still fails randomly. Example on AppVeyor on my PR 5423:
https://ci.appveyor.com/project/python/cpython/build/3.7build11472
ERROR: test_start_tls_server_1 (test.test_asyncio.test_sslproto.SelectorStartTLSTests)
----------------------------------------------------------------------
I also had this failure on my Windows 10 VM when running "python -m test -R 3:3 -v test_asyncio". I skipped the test to be able to debug bpo-32710.
1st1 commentedon Jan 29, 2018
I have a feeling that using threads+IO+asyncio makes the test too unstable on some Windows buildbots. I'll rewrite start-tls tests without using threads.
asvetlov commentedon May 21, 2018
Is the issue done?
vstinner commentedon May 28, 2018
I found a race condition in START TLS: bpo-33674. I'm fixing it (I'm just waiting to merge my PR which has already been approved).