MagicStack / uvloop

Ultra fast asyncio event loop.
Apache License 2.0
10.37k stars 541 forks source link

Transport extra info "socket" is not upgraded to SSL after STARTTLS #55

Closed SamWhited closed 8 years ago

SamWhited commented 8 years ago

After performing STARTTLS, transp.get_extra_info("socket") still returns the original socket and not the SSL socket. In the builtin asyncio event loop the transports extra info is upgraded to point to the SSL socket. I am unsure if this happens after a direct TLS connection or not.

I also don't have an easy to reproduce example without requiring an account somewhere; using the latest version of slixmpp to connect to any XMPP server should attempt STARTTLS and reproduce the issue, but I'm afraid that's not the most helpful advice in a bug report. I'll try to put together a minimal working example.

1st1 commented 8 years ago

What do you use to STARTTLS? Can you provide a script and steps to reproduce the problem?

SamWhited commented 8 years ago

I was thinking that I couldn't provide a script without requiring that you get an account somewhere, but it doesn't really matter if auth fails; presumably (hopefully) STARTTLS happens before auth. One moment, I'll try to provide a minimal script.

1st1 commented 8 years ago

Also, can you reproduce this in vanilla asyncio? The problem is that there's no "SSL socket" in 3.5, we use SSL memory BIO, which means that the socket object stays the same.

SamWhited commented 8 years ago

Also, can you reproduce this in vanilla asyncio? The problem is that there's no "SSL socket" in 3.5, we use SSL memory BIO, which means that the socket object stays the same.

I can't; running with normal asyncio I get an ssl.SSLSocket object returned, but if I run with uvloop I always get a socket.socket (which doesn't have a getpeercert() method, so slixmpp panics).

Requires

slixmpp==1.2.1
uvloop==0.5.4

MWE

(note that this appears to be flakey; you may have to run it once or twice to see the issue)

#!/usr/bin/env python3

import asyncio
import uvloop

from slixmpp import ClientXMPP

if __name__ == "__main__":
    asyncio.set_event_loop_policy(uvloop.EventLoopPolicy())
    loop = asyncio.get_event_loop()

    client = ClientXMPP("fakeaccount@samwhited.com", "fakepassword")
    client.loop = loop
    client.connect()

    loop.run_forever()
SamWhited commented 8 years ago

For context, the starttls connection is being created in Slix with:

ssl_connect_routine = self.loop.create_connection(lambda: self, ssl=self.ssl_context,                            
                                                  sock=self.socket,                                              
                                                  server_hostname=self.default_domain)

See: https://github.com/poezio/slixmpp/blob/master/slixmpp/xmlstream/xmlstream.py#L472

SamWhited commented 8 years ago

Another related difference, it appears that the ssl_object extra info also contains the ssl.SSLSocket in asyncio's default event loop, but contains ssl.SSLObject in uvloop (which makes more sense, given the name). I'm not entirely sure these even count as a stable API that should be the same though (I didn't see them documented anywhere), but I don't know how else we'd get a reference to the socket or SSL socket, so I assume at least socket should have the same value in different implementations.

SamWhited commented 8 years ago

Sorry for all the spam, one more thing: This does appear to be a documented API (https://docs.python.org/3/library/asyncio-protocol.html), and it appears that uvloop is doing things right and asyncio's built in event loop (and slix) is doing it wrong. According to the docs, socket should always map to a socket.socket, and ssl_object should map to an SSLObject or an SSLSocket, so I'm inclined to beleive that uvloop is in the right. Closing this issue.

1st1 commented 8 years ago

According to the docs, socket should always map to a socket.socket, and ssl_object should map to an SSLObject or an SSLSocket, so I'm inclined to beleive that uvloop is in the right.

Right. One problem: I don't understand how can you have SSLSocket on 3.5.2 in asyncio at all.

SamWhited commented 8 years ago

I don't understand how can you have SSLSocket on 3.5.2 in asyncio at all.

I don't know much about it, but it appears to be some hackery slixmpp is doing to use the old SSL, no idea why. From their chat room:

10-06 12:44 <== <mathieui> but we use the old-style SSL implementation in asyncio
10-06 12:44 <== <mathieui> because I had issues with the new one

Related issues that I failed to find before:

1st1 commented 8 years ago

I don't know much about it, but it appears to be some hackery slixmpp is doing to use the old SSL, no idea why.

Got it. That explains why I couldn't see how this could happen in asyncio ;) Would you be able to ask them why are they doing this? Perhaps there is a bug that should be fixed ASAP, before Python 3.6 is out.

SamWhited commented 8 years ago

Perhaps there is a bug that should be fixed ASAP, before Python 3.6 is out.

I think it was just that the ssl_object wasn't being set on 3.4 (see the issues I posted, that regression has already been fixed). I'll try to get more info out of them though, and submit a patch if anything is required on the asyncio side.

EDIT: Update for issue historians: jank workaround for the 3.4 regression submitted: https://github.com/poezio/slixmpp/pull/5/files

1st1 commented 8 years ago

Cool! Thank a lot for investigating this.