Differences between standard SSL and Pyopenssl (ZeroReturnError, SysCallError)

[mthurman]

We use urllib3 via requests with pyopenssl so we can have SNI support in python 2.7.3.

Since we switched, we've noticed more errors from requests. Specifically:

• ZeroReturnError
• SysCallError: (104, 'Connection reset by peer')
• SysCallError: (-1, 'Unexpected EOF')

It seems like these should all be caught somehow in contrib/pyopenssl.py so at the requests level, we don't have to worry about pyopenssl being different than the standard ssl module.

Here's a few of the ideas I'm considering:

• in fileobject.read, everywhere we are catching WantReadError also catch ZeroReturnError and make sure that data is an empty string and continuing
• In inject_into_urllib3, setting connection.BaseSSLError = OpenSSL.SSL.Error

Based on my limited understanding of pyopenssl (and python's standard _ssl.c) I'm probably missing something in trying to unify these interfaces. If somebody with more familiarity with this stuff can point me in the right direction I can try to formalize this stuff into a pull request.

Here's a gist of how I started investigating the ZeroReturnError specifically: https://gist.github.com/mthurman/9963741

[shazow]

Sounds sensible to me. @t-8ch, any thoughts?

[t-8ch]

According to the pyOpenSSL docs ZeroReturnError means that the SSL connection has been closed cleanly. I think we should catch this and immediately return the empty string on read. The connection is closed and won't open by simply retrying. This seems cludgy and a proper exception would be better, but it seems this is how the stdlib behaves.

For the SysCallErrors there should be stdlib equivalents.

Tbh I really dislike the the monkeypatching. Maybe we could add a SSLEngine interface, which can be implemented with PyOpenSSL and used when constructing ConnectionPools

[shazow]

@t-8ch Wouldn't that just be the same as providing a different implementation of HTTPSConnectionPool (or ConnectionCls within it)?

I'm not a big fan of the monkeypatching either but I recall that it made the most sense at the time for some reason.

[t-8ch]

The engine would only have to provide wrap_socket and BaseSSLError. One could then do m = PoolManager(ssl_engine=urllib3.contrib.pyopenssl.SslEngine()). Practically inversion of control.
This way it is explicit where stuff can be switched.
The rest of the classes wouldn't be touched, so no reimplementation.
Requests could switch to:

try:
    ssl_engine = .packages.urllib3.contrib.pyopenssl.SSLEngine()
except ImportError:
    ssl_engine = .packages.urllib3.ssl_engine.SSLEngine()

[shazow]

Mmm I see what you mean.

What about just passing around the wrap_socket and assuming the BaseSSLError will be wrapped correctly? (Would it be easy enough to do for pyopenssl?)

An SSLEngine object seems to be a bit overkill here.

[t-8ch]

The SSLEngine wouldn't be much more (if at all) than;

SSLEngine = namedtuple('SSLEngine', ['wrap_socket', 'BaseSSLError'])

It wouldn't really matter, but I think it clearer for the users when they use a proper object instead of a single function.
And it's extensible should we need to add more stuff to it.

[shazow]

I feel the contrary, that it's confusing to have a proper object which has no actual behaviour except containing a callable. It may as well just be the callable. The whole notion of an SSLEngine sounds far more complex and unobvious than what it is (it took me a while to figure out that you just meant renaming the wrap_socket callable).

As for the exception, I feel that letting various implementations of wrap_socket provide their own exception that we want to catch breaks the abstraction of the "interface" pattern that we're proposing.

What do you think, @Lukasa?

[Lukasa]

Aha, you might find this relevant: a contributor to hyper just contributed this module. This is basically an attempt to unify the PyOpenSSL interface with the stdlib interface.

I think that doing the SSLEngine is a bit bizarre. If this problem can be solved by catching and rethrowing exceptions in wrap_socket then that's what should be done. If it can't, then I feel like a full compatibility layer is more appropriate. The SSLEngine paradigm works fine for the current situation, but I don't buy the 'extensibility' argument: seems like there are lots of possible extensions you might want to make that will be hairy in the SSLEngine.

No strong objection though, it's an acceptable solution to the current problem. =)

[shazow]

@Lukasa That's interesting indeed!

@alekstorm Any chance you'd be interested in making that a standalone package that we can vendor into urllib3?

Also wonder if there's any way we could further abstract the compatibility issues with Py2.

[Lukasa]

@alekstorm Having the ssl compatibility interface be standalone would be a really excellent idea, actually. If you feel like doing it I'll happily write up some docs for it, including how to get the best-possible SSL support for a given platform (really difficult to do right on OS X).

[alekstorm]

Sure, it would be my pleasure. I've gone ahead and registered the package as backports.ssl (following the backports namespace conventions), and uploaded the source to a new backports.ssl repo. Currently working on getting the urllib3 test suite to pass with the library monkey-patched in; I'll leave you to decide the best way to actually utilize it, of course.

@Lukasa docs would be much appreciated; I've currently only got a minimal README.rst.

@shazow The whole requests/urllib3 family seems to be doing a lot of vendoring, and I'm curious what the advantages are (besides avoiding the general mess that is Python package distribution), just for my own edification.

[shazow]

@alekstorm urllib3 generally only vendors Python 3 backports for compatibility purposes, you'll have to ask @kennethreitz for his reasons to vendor urllib3 and other libraries.

Generally, non-vendored dependencies for libraries that are core to many other libraries exhibit a bunch of problems such as version conflicting, the inability for themselves to be vendored, etc.

[alekstorm]

Got it, thanks. The urllib3 test suite now passes with backports.ssl (I've pushed my testing branch of urllib3 as test-backports.ssl), except for hanging on test_https.TestHTTPS_TLSv1, although I suspect that may be due to the Tornado-powered dummyserver using the wrong OpenSSL on my system.

@shazow, what are your conditions for pulling the trigger on vendoring it into urllib3? Should it pass the Python stdlib test suite (hard, but doable), or will you be satisfied with something else?

Sorry to clog this issue with more noise; would you like me to open a separate one?

[shazow]

@alekstorm Let's start with a PR which we can move the conversation to. I'm actually not familiar with the Python stdlib test suite--if it has stuff you think it should be passing, then yes please. If it's passing the urllib3 test suite, then I'm fairly happy with that too.

[lukas-gitl]

Any progress on this?

[sethmlarson]

@lukas-gitl Since it's been 3 years since this was originally reported it may be useful to open up a new issue and then link to this issue if you're experiencing this. :)

[IvanLauLinTiong]

pyOpenSSL is deprecated and will be removed in future release version 2.x (#2691).