psf / requests

A simple, yet elegant, HTTP library.
https://requests.readthedocs.io/en/latest/
Apache License 2.0
52.13k stars 9.32k forks source link

Allow to disable SSL compression. #1853

Closed chmouel closed 10 years ago

chmouel commented 10 years ago

For certain use cases disabling SSL compression gets us better performances while staying secure, it would be nice if requests allow to do that

Lukasa commented 10 years ago

I don't believe we offer SSL compression.

I tested the following scenarios on my local machine (Windows 7) against a webserver I control (here, running nginx v1.4.1 with default settings):

So it looks like we never negotiate TLS compression. Can you confirm that you've ever seen a situation where we are using TLS compression, ideally with a reproducible test case?

Lukasa commented 10 years ago

This finding is backed up by the uber-bitchy HowsMySSL website that also states that Requests doesn't offer TLS compression on my machine.

Lukasa commented 10 years ago

Also, @t-8ch, our resident TLS guru, can you confirm that this is/isn't related to specific installs of OpenSSL?

chmouel commented 10 years ago

This has spun up from this openstack swiftclient review: https://review.openstack.org/#/c/33473/ I have pinged thomas if he can look over it and there is actually a pull request on urllib3 (that i have just seen before opening the bug here) to actually disable it https://github.com/shazow/urllib3/pull/309 but we may need the requests part as well

bugsduggan commented 10 years ago

Here's some numbers relating TLS compression tunings and speed-ups. It's targetted at OpenStack Swift but the principles are applicable. http://tomleaman.co.uk/swift_perf6.pdf

Lukasa commented 10 years ago

To be clear, I accept the numbers, my question is about whether we have the problem to begin with. =)

Lukasa commented 10 years ago

To follow up with the linked urllib3 issue, I see tls_compression_supported == False for my copies of Requests on Windows.

Lukasa commented 10 years ago

I also get False in Python 2.6 on my CentOS box.

t-8ch commented 10 years ago

Nginx disables SSL compression since 1.2.2 (07/2012). Looking at the SSL client hello with wireshark requests announces support for DEFLATE compression. (openssl version 1.0.1f).

Given the fact, that SSL compression is a security risk (CRIME) we are trying to disable it altogether in urllib3, see the linked issue. Unfortunately the standard ssl module on older pythons doesn't allow us to do this.

@chmouel @bugsduggan Wouldn't it be useful to disable SSL compression on the openstack-swift server-side as the performance impact would also hit other API clients besides the official one?

Lukasa commented 10 years ago

@t-8ch Brilliant, if urllib3 disables it altogether that'll make me very happy, as we won't have to worry about the API. =) Give me a shout on that issue if you think I can help.

chmouel commented 10 years ago

Yeah I was reading the bug report, when we are talking old version of python how old is that?

chmouel commented 10 years ago

@t-8ch sorry my answer was a bit short, we could indeed disable SSL comprssion in swift (and openstack in general) but that's not how people usally runs it they would have usually things like (since this is how we advise to run it) pound/nginx doing that for us.

dbrgn commented 10 years ago

On Arch Linux and both Python 3.3 and 2.7:

>>> import requests
>>> r = requests.get('https://www.howsmyssl.com/a/check')
>>> data = r.json()
>>> data['tls_compression_supported']
True

I think this is something that has to do with OpenSSL defaults. Unfortunately it doesn't seem to be easily possible to change the defaults before Python 3.2... For py3.2+ this should now soon be fixed in urllib3, where it's explicitly disabled. (I actually created that PR because of requests...)

I'm not sure if we can do something about it from a higher level. But the default should be "off".

Lukasa commented 10 years ago

I doubt this is something Requests can do any easier that urllib3. For the moment, we'll close this issue, but everyone should keep a close eye on urllib3 and revisit this when a decision has been made over there.