Repository navigation
Rewrite asyncio/sslproto.py #158
Description
Problems with sslproto.py
-
The code is very complex and hard to reason about. At this point it's pretty much unmaintainable, many bugs on bugs.python.org linger unresolved just because we don't have capacity to debug them or review PRs.
-
It's very slow. SSL connections are 3-4x slower than regular connections.
-
It has many yet unresolved problems associated with SSL connection shutdown and cleanup:
-
We don't have enough functional tests for SSL in asyncio. Especially ones that test cancellation & timeouts thoroughly enough.
Requirements for the new implementation
-
Easy to understand code. Ideally, the state machine should be implemented right in the
SSLProtocol(so noSSLPipeabstraction). A good example of a protocol state machine is asyncpg'sCoreProtocol <-> Protocol <-> Connectionmechanism. -
Performance. Ideally, SSL connections shouldn't be more than 10% slower.
-
Tests. Nathaniel once mentioned that Trio's SSL tests are very robust and that he found bugs in CPython's ssl module with them. We maybe want to backport them to asyncio.
-
SSL tunneled through SSL should be naturally supported.
Suggested steps towards the new implementation
-
Try to benchmark the current
asyncio/sslproto.py. Maybe we'll be able to pinpoint the exact bottleneck that makes it so slow; what if there's a bug in ssl module? Most likely there's no single bottleneck, and the reason for poor performance is just its inefficient code. -
Implement a new SSL state machine/protocol in pure Python (I suggest to use new
asyncio.BufferedProtocolfrom the beginning, though.) Get it to the point that it passes asyncio's existing SSL tests. Add more tests. -
Once we have a working pure Python implementation, we can (a) contribute it to Python 3.8, (b) start optimizing it in Cython for uvloop.
cc @fantix
Activity
fantix commentedon May 31, 2018
@1st1 Thanks! I'm trying to quickly profile
asyncio/sslproto.py, shall follow up with results here.fantix commentedon May 31, 2018
Vanilla asyncio echo server on Python 3.7b3 without SSL:
With SSL:
I've repainted this in red (
sslproto.py) and green(_sslmodule):This gives a rough idea about the time portfolio:
sslproto.py._sslmodule is taking quite some time, 90% of which is onread()andwrite()of_ssl.SSLSocket, and 9% on_ssl.MemoryBIO.Top
tottimedetail data:fantix commentedon May 31, 2018
Just made a very simple PoC, the improvement is not significant. I'll keep trying on something else tomorrow, just posting the results here for now.
Just curious about Trio, which is using the same stdlib
_sslmodule (MemoryBIO,read()andwrite()):With SSL:
Without SSL:
1st1 commentedon May 31, 2018
Oh, that's unfortunate. I looked at
_SSLObject.readandwriteimplementations inModules/_ssl.cand it looks like they are just tiny wrappers of OpenSSL functions. We can probably save some overhead by supplying our own buffer toread, but I wouldn't expect it to give us any significant improvements.1st1 commentedon May 31, 2018
Maybe asyncio's read buffer is too small and we have too much overhead with
WANT_READ.elprans commentedon May 31, 2018
Throughput depends a lot on the chosen cipher. See e.g. https://jbp.io/2018/01/07/rustls-vs-openssl-performance-1.html, the difference can be quite large.
fantix commentedon Jun 1, 2018
All above tests are using cipher
ECDHE-RSA-AES256-GCM-SHA384, I'll quickly tryECDHE-RSA-AES128-GCM-SHA256.Using
ECDHE-RSA-AES128-GCM-SHA256didn't produce much difference,8.53vs8.47. Let me try to tweak the buffer size.1st1 commentedon Jun 1, 2018
Or you can try uvloop which has 256kb (or something like that) buffer size by default.
fantix commentedon Jun 7, 2018
Sorry for the delay! Tweaking buffer size had no luck. Actually:
socket.send()calls, and 1.2mil_SSLSocket.read()calls. Fromsslproto.pythis looks like half of the_SSLSocket.read()calls provided all decrypted data in one go, the other half simply raisedWANT_READto break the loop infeed_ssldata().I'm thinking of these TODOs next, please kindly advice:
uvloop/sslproto.pyxto see how it works.cpython/Modules/_ssl.c(triedltracea bit, result is not yet convincing).asyncio/sslproto.pywith new_ssl.cperhaps.(Similarly, quick-and-dirty PoCs first.)
1st1 commentedon Jun 7, 2018
That's true only for 3.7, but I get your point.
Let's re-sync after (1).
(2) and (3) will require us waiting until 3.8, considering your improvements to the
sslmodule are accepted.fantix commentedon Jun 7, 2018
Got it, thanks! Meanwhile there's also a possibility to make uvloop use pyOpenSSL, libuv-tls or similar libs directly (as a last way).
1st1 commentedon Jun 7, 2018
Yes, I was thinking about that too until I looked at
_ssl.cand saw a very thin wrapper around OpenSSL. I really don't think we can make it any faster. But this is just my first impression; maybe there are some faster OpenSSL APIs that we can use or something. After we have a betterSSLProtocolimplementation I'm totally open to profile Python's ssl module to see if it can be improved.1st1 commentedon Jun 7, 2018
Christian Heims also suggests trying to enable http://manpages.org/SSL_CTX_set_read_ahead/3
17 remaining items
1st1 commentedon Jun 12, 2018
Take your time. And... nice diagram! We'll probably want to have a digital version of it to put somewhere in the docs ;)
fantix commentedon Jun 13, 2018
Sure thing!
Looks like renegotiation is a bit awful - Python didn't expose it, OpenSSL didn't do it fully right, HTTP/2 forbid it, and TLS1.3 replaced it with something else. I'll try to get it working and tested with minimal effort - perhaps copying some tests from Trio as Nathaniel seemed already walked through this.
Eventually figured out how renegotiation works in Python, there are quite some implications:
Hellomessage (probably together with other regular data messages),SSLObject.read()is required to consume theHellomessage.SSLObject.read()will write a handshake message into theoutgoingbuffer.SSLObject.write()won't do - it doesn't touch theincomingbuffer at all. Therefore, callingSSLObject.write()beforeSSLObject.read()would only send app data as usual, with theHellomessage staying in theincomingbuffer.ssl.SSLSyscallErrorto callSSLObject.write()afterSSLObject.read()consumed theHellomessage, until the renegotiation is done.Therefore, I came up with this process diagram:
SSLObject.read().write_backlogbuffer).SSLObject.read()may write data into theoutgoingbuffer or complete a renegotiation, therefore we should always try (the blue arrow) to write data fromwrite_backloginto theSSLObjectwithout waiting for the next downstream write (missing from current cpython), and flush theoutgoingbuffer if any (present in current cpython).SSLprotocolindependent, water marks (high/low forincoming, and high/low foroutgoing+backlog) can be separately tweaked in scenarios like SSL over SSL over SSL.Meanwhile, I'm still trying to fit this into a state transitioning diagram - states probably make more sense with explicit handshake and shutdown included.
DO_HANDSHAKEandSHUTDOWN, then processed afterwards accordingly.Other than the 4 original states, I added 3:
PASSTHRUare used betweenget_buffer()andbuffer_updated()inUNWRAPPEDmode to ignore any environment changes (very unlikely), when app reading is not paused and we could just pass it through without buffering locally.NO_CONNECTIONis used so that we don't have to judgeif self._transport is None.1st1 commentedon Jun 19, 2018
Great progress! (GH doesn't send notifications on edit, so this is the first time I see the updates)
Is it so broken that it's impractical to support it? I mean it's a very niche requirement, so if its support comes at the expense of code clarity/maintainability I'd say let's not do it at all. Your call though.
Can you share some details on that? Is that new TLS 1.3 renegotiation protocol easier to implement? Maybe we can get away with supporting just that TLS 1.3 thing?
fantix commentedon Jun 20, 2018
Thanks! No worries, I didn't want to make too frequent noises, just about to ask for your review and next steps.
The renegotiation support is actually done in previous commit. It turns out that as far as we follow the process diagram to call OpenSSL functions with those implications considered, current TLS<=1.2 renegotiation shall just work. It's manually tested, a unit test would require pyOpenSSL installed, though I'm not sure if that is practical in cpython source code.
For the two renegotiation issues, 1) duplex traffic during renegotiation: it seems that OpenSSL had already fixed the issue, by preventing sending app data once it had sent the renegotiation Hello. I didn't find the actual fix, but verified the behavior (test code is a bit ugly), so it should just work fine now. 2) TLS 1.3 rekeying is still in OpenSSL 1.1.1 beta, so it is a future thing I think.
Another finding during fixing cpython tests is about canceling handshake. SSL/TLS has this
user_canceledalert to manually cancel an ongoing handshake. OpenSSL has this type too, but exposed no API (should usessl3_send_alert()to send auser_canceled). Therefore, we shall simply close the underlying connection in this case. (The state diagram above is updated with a few transitions toNO_CONNECTIONon errors)A side topic - we may be able to support user-random SSL wrap/unwrap in the same TCP connection without reconnecting (transitioning between
UNWRAPPEDandWRAPPEDmultiple times). Originalsslproto.pywon't stay inUNWRAPPEDstate for too much time, but still allowed unencrypted data passing through for an extremely short while after a successful shutdown before connection lost. (Also, current asyncio supported one-time upgrading by inserting anSSLProtocolbefore user protocol byloop.start_tls())The worry is about being unwrapped by the peer - application protocol needs to be aware or even in charge of the SSL downgrading, or sensitive data may be sent in plaintext by mistake. For now, an SSL
close_notifyalert coming from the peer is received as an empty read (sslobj.read()returnsb''). We may use similar pattern to notify the app protocol that this connection is being unwrapped - ifdata_received(b'')happens, then all data will be in plaintext afterwards. We could also allow user to configure the behavior of response toclose_notifyalerts, either close the connection directly, or downgrade immediately, or even let the user to choose when to respond to theclose_notifyalert with callingunwrap().I think this is getting too far, sorry for the long story! It might be better to just stay with closing the connection immediately as the first part of RFC 5246 7.2.1 stated, and leave the random wrap/unwrap to a new feature. If so, I'll simply clean up the pass through logic (also the one from original
sslproto.py), and make sure all traffic is always encrypted.fantix commentedon Jun 21, 2018
About the performance thing, I just realized that I’ve been comparing encryption time with loopback network time, which is probably not a practical anchor point. So I’ve compared the absolute throughput of OpenSSL(with this benchmark tool found in Elvis's reference) and
cpython/_ssl.c(with asyncio echo server andcProfile), the result is: 2059 MB/s vs 1390 MB/s (68%). I think this is a reasonable result, considering async overhead, BIO memory copy overhead and Pythonize overhead.This probably means that our initial problem was incomplete - SSL connections are 3-4x slower than regular connections when the network time is about the same as encryption/decryption time. If the encryption/decryption time was 10% of the network time, then current SSL implementation is supposed to be adding another 15% ~ 30% overhead.
Making the time consumed in the
_sslmodule as the anchor point (unit of one), the first PoCsslproto.pywas 34% faster (1.29 -> 0.85), and the decrypted data aggregation fix was 70% faster (0.52 -> 0.15) on 100KB echo server. In this special combo case,sslproto.pyis improved to only take about 10% of the time spent in_sslmodule, that is 6.5% extra overhead overall comparing to previous 15% ~ 30%, which is good enough I think.1st1 commentedon Jun 21, 2018
Right. This is great. Perhaps we'll be able to squeeze a few more %% when we cythonize the
sslproto.py, but seems that the performance isn't that bad after all.WIP: sslproto rewrite draft, refs MagicStack/uvloop#158
1st1 commentedon Aug 14, 2018
What's the latest news here?
fantix commentedon Aug 14, 2018
Fixed MagicStack#158, rewrite sslproto
Fixed MagicStack#158, rewrite sslproto
Fixed MagicStack#158, rewrite sslproto