Don't use pyOpenSSL unless no SNI is detected

[sethmlarson]

This is part 1 on removing requests[security] extra, for now we can still support SNI-less installations of Python but instead of unconditional monkey-patching we only patch if we detect either no ssl module or no SNI support.

Related: #5267
cc @tiran

An additional thought I had was if we wanted to warn users about the complete removal of requests[security] I don't think there's a way to detect if you were installed via requests pyopenssl or requests[security] beyond registering a name on PyPI like requests-security-extra, trying to import it, and if we do then we know we were installed via requests[security] so throw a deprecation warning? Otherwise our "end-goal" could be to have requests[security] be empty?

[nateprewitt]

I think this looks pretty straight forward and I agree we want to start moving away from PyOpenSSL by default. The only Python versions we'd expect to still be using pyopenssl after this would be <2.7.9, right? I'm think I'm fine approving but would like a second peek from Tiran or @sigmavirus24 if possible.

As for requests[security], I'd rather not have the end state be a no-op placeholder if we can avoid it. The package idea is a little clunky but seems like a clever approach to getting things fully removed. If it's just placing a onetime dummy package in PyPI that we use to set a flag for the DeprecationWarning, I don't see issues with that.

[sethmlarson]

I already have the name registered FYI: https://pypi.org/project/requests-security-extra
Can detect with import requests_security_extra. If others agree I'll add that to this PR as well.

Something like:

try:
    import requests_security_extra
    warnings.warn(
        "Installing 'requests[security]' is deprecated and will be removed in a future version of Requests. "
        "After changing your pip installs to remove [security] you can run "
        "'python -m pip uninstall requests-security-extra' to silence this DeprecationWarning",
        category=DeprecationWarning
    )
except ImportError:
    pass

[sethmlarson]

What I'll also say if we don't necessarily need to deprecate the extra since the worst that happens with the latest pip if the dist doesn't include the extra you asked for is to warn the user. So maybe that isn't as necessary as I originally thought?

[sigmavirus24]

So if we're doing better feature detection of the cases when we need to inject PyOpenSSL, why break backwards compatibility by removing requests[security]? We should also be documenting this change in behaviour as well as how folks can get back to the old behaviour (by calling inject_openssl or whatever it's called) themselves. I understand all of @tiran's objections but I don't think deprecating the extra is a reasonable or workable solution for everyone. Beyond that, distro packagers have their solution where necessary. __init__.py doesn't change so drastically that carrying that patch forward is difficult or opprobrious.

This was never meant to be a perfect solution, it was just the best solution within the constraints of an art project that reluctantly tried to be secure. I fear y'all will have more headaches trying to remove this than not and your ability to have a functioning inbox is important to me (and it should be important to you too)

[sethmlarson]

@sigmavirus24 That's true, we can keep the extra as-is. As far as this patch looks now though you're in favor?

Can definitely add a changelog entry.

[tiran]

The proposed PR is a good compromise between backwards compatibility and my requested change. My main pain point was the fact that requests changed behavior depending on the presence of a 3rd party package and that PyOpenSSL is not compatible with W^X memory protection of modern CPUs.

How about documenting requests[security] as deprecated then? You could remove at the same time as Python 2 support in the distant future.

[sigmavirus24]

How about documenting requests[security] as deprecated then? You could remove at the same time as Python 2 support in the distant future.

That seems like a great idea. By that point Requests should also just be supporting something like 3.6+ or 3.7+ so there should be no reason for PyOpenSSL (ideally 🤞 )

My main pain point was the fact that requests changed behavior depending on the presence of a 3rd party package and that PyOpenSSL is not compatible with W^X memory protection of modern CPUs.

Right, last night I fell asleep wondering "Why does @tiran even care? No one does rpm install python-requests[security] and then I realized that something else might depend on PyOpenSSL and that would cause the problem.

[sigmavirus24]

One question but otherwise LGTM

[sigmavirus24]

GitHub is rendering this oddly. Is this a tab or some kind of mixed white space?

[sethmlarson]

That is a strange render but as far as I can tell they're all spaces.

[tiran]

Right, last night I fell asleep wondering "Why does @tiran even care? No one does rpm install python-requests[security] and then I realized that something else might depend on PyOpenSSL and that would cause the problem.

I care because PyOpenSSL doesn't play nice with security hardening and inject_into_urllib3 can cause bugs that are hard to debug.

The main system stacks at $JOB includes a Python webservice that runs on Apache and mod_wsgi. An SELinux policy prevents Apache to create memory pages with mmap that are both writeable and executable. In general it's a terrible idea because it allows an attacker to run arbitrary native code easily. This all works fine with default set of packages that do not include PyOpenSSL.

But some $CUSTOMERS also install additional packages. One of the packages depends on PyOpenSSL. As soon as PyOpenSSL is present and Apache is restarted, requests starts to behave differently, triggers SELinux violations, and the main API service is broken. The Apache service often runs for days, weeks or longer without restart. There is no apparent causation between the problem and PyOpenSSL. First time we had a lot of "fun" figuring out the cause of the problem...

I came up with this workaround: freeipa/freeipa@dea059d

[sethmlarson]

@tiran Yikes! 😱

[nateprewitt]

Ok cool, it sounds like we’ve got agreement this is a decent compromise to at least get us moving in the right direction.

[nateprewitt]

Actually can we get a quick change log added for this too, @sethmlarson?

[sethmlarson]

@nateprewitt Sure thing, it'll be done in half an hour. :)

[sethmlarson]

@nateprewitt Done, let me know if you want to change the verbage.

[nateprewitt]

Perfect 👌

[sethmlarson]

Thanks all for the reviews! 🎉

[saper]

I just wanted to let you know that I am using PyOpenSSL integration not because of Python 2 issues but because the ability to use client-side keys and certificates provided as raw bytes instead of files (via https://www.pyopenssl.org/en/stable/api/ssl.html#OpenSSL.SSL.Context.use_certificate and https://www.pyopenssl.org/en/stable/api/ssl.html#OpenSSL.SSL.Context.use_privatekey) which seems to be difficult otherwise.

[sethmlarson]

@saper Thanks for bringing that use-case to my attention! If you still need pyOpenSSL support you can manually call urllib3.contrib.pyopenssl.inject_into_urllib3()

[saper]

@sethmlarson Yes, this is what I ultimately did because I only had pyopenssl.OpenSSL.SSL.Context monkeypatched in. It took me few hours to track this down to the unexpected update (I was getting SSL certificate verification errors). Leaving the project working in the afternoon and coming back in the evening and have everything broken by itself can be frustrating :)

Time to pin the dependencies I guess :/ Big thanks for documenting it clearly here though - once I realized the release was out 6 hours earlier I figured it out very quickly.

[tiran]

I just wanted to let you know that I am using PyOpenSSL integration not because of Python 2 issues but because the ability to use client-side keys and certificates provided as raw bytes instead of files (via pyopenssl.org/en/stable/api/ssl.html#OpenSSL.SSL.Context.use_certificate and pyopenssl.org/en/stable/api/ssl.html#OpenSSL.SSL.Context.use_privatekey) which seems to be difficult otherwise.

You can load certs and keys from a raw bytes with Python's ssl module, too. Just dump them into a temporary directory, load them from file, and remove the directory after you have loaded the files. Python ensures that a temporary directory is only accessible by the current effective user. This makes the approach safe. There is no risk when the key is secured with PKCS#8 PBE2.

There is a minimal risk if the key is not encrypted, you are running multiple untrusted processes under the same EUID and you have partly hardened the system. You can mitigate the risk by implementing privilege separation.

In all other cases loading private keys from raw bytes is less secure than loading it from files! OpenSSL does a far better job than Python when it comes to managing and cleaning memory that holds sensitive data. Just consider this example:

$ python3
>>> import os
>>> os.getpid()
2258735
>>> s = "MY SUPER SECRET KEY"

$ gcore -o /tmp/dump 2258735
$ strings /tmp/dump.2258735 | grep "MY SUPER SECRET KEY"
 = "MY SUPER SECRET KEY"
>> s = "MY SUPER SECRET KEY"
 = "MY SUPER SECRET KEY
s = "MY SUPER SECRET KEY"
MY SUPER SECRET KEY

[saper]

@tiran Sure, I know I can't trust anything like Python too much. I am not sure I always have a writable filesystem and other things (just being fed with PKCS#12 files). For proper key security I'd need to go the PKCS#11 route, but that has other issues (like poor library support).