Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting the default method of `ScrapyClientContextFactory` to `SSLv23_METHOD` #1629

Conversation

@starrify
Copy link
Contributor

@starrify starrify commented Dec 7, 2015

The upstream issue described in #194 has been fixed in OpenSSL: https://rt.openssl.org/Ticket/Display.html?id=2771 and also in Ubuntu: https://bugs.launchpad.net/ubuntu/+source/openssl/+bug/965371

Maybe it's time to introduce SSLv23_METHOD back again as it comes with more compatibility: https://docs.python.org/dev/library/ssl.html#ssl.wrap_socket

@codecov-io
Copy link

@codecov-io codecov-io commented Dec 7, 2015

Current coverage is 82.80%

Merging #1629 into master will decrease coverage by -0.13% as of 65f3c9b

Powered by Codecov. Updated on successful CI builds.

@redapple
Copy link
Contributor

@redapple redapple commented Feb 9, 2016

@starrify , I agree,
SSLv23_METHOD seems to be working much better in my tests on current open issues with tag "https" (#1764 (comment) , #1486 (comment) , #1435 (comment))

Another option to make the change technically more backward compatible is to define another context factory in scrapy/core/downloader/contextfactory.py, say TLSFlexibleContextFactory, and documenting that users can change the DOWNLOADER_CLIENTCONTEXTFACTORY setting to this other builtin class.

But I'm fine with making this the default, with a minimum openssl version in requirements.txt perhaps?

@@ -14,9 +14,7 @@ class ScrapyClientContextFactory(ClientContextFactory):
# and https://github.com/scrapy/scrapy/issues/981

This comment has been minimized.

@kmike

kmike Feb 16, 2016
Member

the list of issues should be updated or removed

@kmike
Copy link
Member

@kmike kmike commented Feb 16, 2016

Does anyone know which OpenSSL versions do have this bug?

@redapple
Copy link
Contributor

@redapple redapple commented Feb 16, 2016

@kmike , it seems to me that the initial problem was less a bug than issue with faulty servers, and OpenSSL implemented a few workarounds along the way to deal with them.

I've listed changelog changes related to TLS versions handling for OpenSSL and Ubuntu openssl:
https://docs.google.com/spreadsheets/d/1TLQAhAt2JsF9o-JfcH8D3ttCXHH5s0OczteXTg7OMK4/edit?usp=sharing

As far as I can read, Ubuntu Precise's openssl (1.0.1-4ubuntu5.27) was the version re-enabling TLS 1.2 by default, suggesting state of servers in the wild is good enough.

OpenSSL 1.0.1d looks like the last release with fixes to protocol version negotiation, before the rewrite in 1.1.0 (not released yet, Alpha 3 on 15-Feb-2016)

*) Version negotiation has been rewritten. In particular SSLv23_method(),
SSLv23_client_method() and SSLv23_server_method() have been deprecated,
and turned into macros which simply call the new preferred function names
TLS_method(), TLS_client_method() and TLS_server_method(). All new code
should use the new names instead. Also as part of this change the ssl23.h
header file has been removed.
[Matt Caswell]

@kmike
Copy link
Member

@kmike kmike commented Feb 17, 2016

@redapple a nice table!

Older OpenSSL versions are still in use. E.g. on OS X 10.11.3 (most recent OS X) I get the following:

> openssl version
OpenSSL 0.9.8zg 14 July 2015

(though Scrapy uses OpenSSL 1.0.2e 3 Dec 2015, likely via homebrew Python)

@redapple
Copy link
Contributor

@redapple redapple commented Feb 23, 2016

@kmike , @starrify ,
I'd go for #1794
What do you think?

@redapple
Copy link
Contributor

@redapple redapple commented Feb 24, 2016

Superseded by #1794

@redapple redapple closed this Feb 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.