Navigation Menu

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

Jump to bottom

[MRG+1] Use OpenSSL default ciphers #2314

Merged
merged 1 commit into from Oct 17, 2016
Merged

[MRG+1] Use OpenSSL default ciphers #2314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions scrapy/core/downloader/contextfactory.py
View file
Open in desktop
Expand Up @@ -13,7 +13,7 @@
from twisted.web.client import BrowserLikePolicyForHTTPS
from twisted.web.iweb import IPolicyForHTTPS

from scrapy.core.downloader.tls import ScrapyClientTLSOptions
from scrapy.core.downloader.tls import ScrapyClientTLSOptions, DEFAULT_CIPHERS


@implementer(IPolicyForHTTPS)
Expand Down Expand Up @@ -46,7 +46,9 @@ def getCertificateOptions(self):
# not calling super(..., self).__init__
return CertificateOptions(verify=False,
method=getattr(self, 'method',
getattr(self, '_ssl_method', None)))
getattr(self, '_ssl_method', None)),
fixBrokenPeers=True,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea what it means, but in Twisted docs there is a note:

This option is now off by default, because it causes problems with connections between peers using OpenSSL 0.9.8a.

Is it required to turn it on, or is it turned on just in case? I'm fine with turning it on, but we should keep an eye on it then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I did not mention this in the PR description.
The story is that it adds the SSL_OP_ALL flag to the options like we had in scrapy<1.1 and what we set when Twisted<14
See https://www.openssl.org/docs/manmaster/ssl/SSL_CTX_set_options.html for what it does at OpenSSL level.
But we can leave it out for now, and have this option in the back of our minds if we still have issues in the field.
This could also be an option like ENABLE_OPENSSL_SERVER_BUG_WORKAROUND

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is fine to merge it as-is; the option looks obscure.

acceptableCiphers=DEFAULT_CIPHERS)

# kept for old-style HTTP/1.0 downloader context twisted calls,
# e.g. connectSSL()
Expand Down
3 changes: 3 additions & 0 deletions scrapy/core/downloader/tls.py
View file
Open in desktop
Expand Up @@ -28,6 +28,7 @@
SSL_CB_HANDSHAKE_START = 0x10
SSL_CB_HANDSHAKE_DONE = 0x20

from twisted.internet.ssl import AcceptableCiphers
from twisted.internet._sslverify import (ClientTLSOptions,
_maybeSetHostNameIndication,
verifyHostname,
Expand Down Expand Up @@ -60,6 +61,8 @@ def _identityVerifyingInfoCallback(self, connection, where, ret):
'from host "{}" (exception: {})'.format(
self._hostnameASCII, repr(e)))

DEFAULT_CIPHERS = AcceptableCiphers.fromOpenSSLCipherString('DEFAULT')

except ImportError:
# ImportError should not matter for older Twisted versions
# as the above is not used in the fallback ScrapyClientContextFactory
Expand Down