From 3384db92b4fb2bce66d01ddf5365478a695caad6 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 27 Sep 2018 19:51:27 +0500 Subject: [PATCH 1/4] Add support for setting SSL ciphers. --- scrapy/core/downloader/contextfactory.py | 13 +++++++++---- scrapy/settings/default_settings.py | 1 + 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/scrapy/core/downloader/contextfactory.py b/scrapy/core/downloader/contextfactory.py index 127a246f5..89d2776ae 100644 --- a/scrapy/core/downloader/contextfactory.py +++ b/scrapy/core/downloader/contextfactory.py @@ -1,5 +1,5 @@ from OpenSSL import SSL -from twisted.internet.ssl import optionsForClientTLS, CertificateOptions, platformTrust +from twisted.internet.ssl import optionsForClientTLS, CertificateOptions, platformTrust, AcceptableCiphers from twisted.web.client import BrowserLikePolicyForHTTPS from twisted.web.iweb import IPolicyForHTTPS from zope.interface.declarations import implementer @@ -19,15 +19,20 @@ class ScrapyClientContextFactory(BrowserLikePolicyForHTTPS): understand the SSLv3, TLSv1, TLSv1.1 and TLSv1.2 protocols.' """ - def __init__(self, method=SSL.SSLv23_METHOD, tls_verbose_logging=False, *args, **kwargs): + def __init__(self, method=SSL.SSLv23_METHOD, tls_verbose_logging=False, tls_ciphers=None, *args, **kwargs): super(ScrapyClientContextFactory, self).__init__(*args, **kwargs) self._ssl_method = method self.tls_verbose_logging = tls_verbose_logging + if tls_ciphers: + self.tls_ciphers = AcceptableCiphers.fromOpenSSLCipherString(tls_ciphers) + else: + self.tls_ciphers = DEFAULT_CIPHERS @classmethod def from_settings(cls, settings, method=SSL.SSLv23_METHOD, *args, **kwargs): tls_verbose_logging = settings.getbool('DOWNLOADER_CLIENT_TLS_VERBOSE_LOGGING') - return cls(method=method, tls_verbose_logging=tls_verbose_logging, *args, **kwargs) + tls_ciphers = settings['DOWNLOADER_CLIENT_TLS_CIPHERS'] + return cls(method=method, tls_verbose_logging=tls_verbose_logging, tls_ciphers=tls_ciphers, *args, **kwargs) def getCertificateOptions(self): # setting verify=True will require you to provide CAs @@ -45,7 +50,7 @@ class ScrapyClientContextFactory(BrowserLikePolicyForHTTPS): method=getattr(self, 'method', getattr(self, '_ssl_method', None)), fixBrokenPeers=True, - acceptableCiphers=DEFAULT_CIPHERS) + acceptableCiphers=self.tls_ciphers) # kept for old-style HTTP/1.0 downloader context twisted calls, # e.g. connectSSL() diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py index 81fee543f..742c8e8a1 100644 --- a/scrapy/settings/default_settings.py +++ b/scrapy/settings/default_settings.py @@ -85,6 +85,7 @@ DOWNLOADER = 'scrapy.core.downloader.Downloader' DOWNLOADER_HTTPCLIENTFACTORY = 'scrapy.core.downloader.webclient.ScrapyHTTPClientFactory' DOWNLOADER_CLIENTCONTEXTFACTORY = 'scrapy.core.downloader.contextfactory.ScrapyClientContextFactory' +DOWNLOADER_CLIENT_TLS_CIPHERS = 'DEFAULT' DOWNLOADER_CLIENT_TLS_METHOD = 'TLS' # Use highest TLS/SSL protocol version supported by the platform, # also allowing negotiation DOWNLOADER_CLIENT_TLS_VERBOSE_LOGGING = False From ce281d890dfbd905922e16552fdcee34eb0d6c42 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Fri, 28 Sep 2018 20:17:12 +0500 Subject: [PATCH 2/4] Documentation for DOWNLOADER_CLIENT_TLS_CIPHERS. --- docs/topics/settings.rst | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index 12606fe47..c042d3f43 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -445,6 +445,24 @@ accepts a ``method`` parameter (this is the ``OpenSSL.SSL`` method mapping :setting:`DOWNLOADER_CLIENT_TLS_METHOD`) and a ``tls_verbose_logging`` parameter (``bool``). +.. setting:: DOWNLOADER_CLIENT_TLS_CIPHERS + +DOWNLOADER_CLIENT_TLS_CIPHERS +----------------------------- + +Default: ``'DEFAULT'`` + +Use this setting to customize the TLS/SSL ciphers used by the default +HTTP/1.1 downloader. + +The setting should contain a string in the `OpenSSL cipher list format`_, +these ciphers will be used as client ciphers. Changing this setting may be +necessary to access certain HTTPS websites: for example, you may need to use +``'DEFAULT:!DH'`` for a website with weak DH parameters or enable a +specific cipher that is not included in ``DEFAULT`` if a website requires it. + +.. _OpenSSL cipher list format: https://www.openssl.org/docs/manmaster/man1/ciphers.html#CIPHER-LIST-FORMAT + .. setting:: DOWNLOADER_CLIENT_TLS_METHOD DOWNLOADER_CLIENT_TLS_METHOD From 9a8edf2bf1172e8eec282a70ad46a9dacff76d62 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Thu, 27 Sep 2018 19:52:29 +0500 Subject: [PATCH 3/4] Tests for setting SSL ciphers. --- tests/mockserver.py | 10 +++++++ tests/test_downloader_handlers.py | 43 +++++++++++++++++++++++++- tests/test_webclient.py | 50 ++++++++++++++++++++++++++++++- 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/tests/mockserver.py b/tests/mockserver.py index 3fa4bc0f0..8be8a36bb 100644 --- a/tests/mockserver.py +++ b/tests/mockserver.py @@ -3,6 +3,8 @@ import sys, time, random, os, json from six.moves.urllib.parse import urlencode from subprocess import Popen, PIPE +from OpenSSL import SSL + from twisted.web.server import Site, NOT_DONE_YET from twisted.web.resource import Resource from twisted.web.static import File @@ -222,6 +224,14 @@ def ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.c ) +def broken_ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.crt', cipher_string='DEFAULT'): + factory = ssl_context_factory(keyfile, certfile) + ctx = factory.getContext() + ctx.set_options(SSL.OP_CIPHER_SERVER_PREFERENCE | SSL.OP_NO_TLSv1_2) + ctx.set_cipher_list(cipher_string) + return factory + + if __name__ == "__main__": root = Root() factory = Site(root) diff --git a/tests/test_downloader_handlers.py b/tests/test_downloader_handlers.py index efef4192c..5df2ffe45 100644 --- a/tests/test_downloader_handlers.py +++ b/tests/test_downloader_handlers.py @@ -39,7 +39,7 @@ from scrapy.utils.test import get_crawler, skip_if_no_boto from scrapy.utils.python import to_bytes from scrapy.exceptions import NotConfigured -from tests.mockserver import MockServer, ssl_context_factory, Echo +from tests.mockserver import MockServer, ssl_context_factory, Echo, broken_ssl_context_factory from tests.spiders import SingleRequestSpider @@ -553,6 +553,47 @@ class Https11InvalidDNSPattern(Https11TestCase): super(Https11InvalidDNSPattern, self).setUp() +class Https11BadCiphers(unittest.TestCase): + scheme = 'https' + download_handler_cls = HTTP11DownloadHandler + + keyfile = 'keys/localhost.key' + certfile = 'keys/localhost.crt' + + def setUp(self): + self.tmpname = self.mktemp() + os.mkdir(self.tmpname) + FilePath(self.tmpname).child("file").setContent(b"0123456789") + r = static.File(self.tmpname) + self.site = server.Site(r, timeout=None) + self.wrapper = WrappingFactory(self.site) + self.host = 'localhost' + self.port = reactor.listenSSL( + 0, self.wrapper, broken_ssl_context_factory(self.keyfile, self.certfile, cipher_string='CAMELLIA256-SHA'), + interface=self.host) + self.portno = self.port.getHost().port + self.download_handler = self.download_handler_cls( + Settings({'DOWNLOADER_CLIENT_TLS_CIPHERS': 'CAMELLIA256-SHA'})) + self.download_request = self.download_handler.download_request + + @defer.inlineCallbacks + def tearDown(self): + yield self.port.stopListening() + if hasattr(self.download_handler, 'close'): + yield self.download_handler.close() + shutil.rmtree(self.tmpname) + + def getURL(self, path): + return "%s://%s:%d/%s" % (self.scheme, self.host, self.portno, path) + + def test_download(self): + request = Request(self.getURL('file')) + d = self.download_request(request, Spider('foo')) + d.addCallback(lambda r: r.body) + d.addCallback(self.assertEqual, b"0123456789") + return d + + class Http11MockServerTestCase(unittest.TestCase): """HTTP 1.1 test case with MockServer""" diff --git a/tests/test_webclient.py b/tests/test_webclient.py index 766329b57..2ebe075ab 100644 --- a/tests/test_webclient.py +++ b/tests/test_webclient.py @@ -8,15 +8,18 @@ import shutil from twisted.trial import unittest from twisted.web import server, static, util, resource -from twisted.internet import reactor, defer +from twisted.internet import reactor, defer, ssl from twisted.test.proto_helpers import StringTransport from twisted.python.filepath import FilePath from twisted.protocols.policies import WrappingFactory from twisted.internet.defer import inlineCallbacks from scrapy.core.downloader import webclient as client +from scrapy.core.downloader.contextfactory import ScrapyClientContextFactory from scrapy.http import Request, Headers +from scrapy.settings import Settings from scrapy.utils.python import to_bytes, to_unicode +from tests.mockserver import ssl_context_factory, broken_ssl_context_factory def getPage(url, contextFactory=None, response_transform=None, *args, **kwargs): @@ -363,3 +366,48 @@ class WebClientTestCase(unittest.TestCase): self.assertEqual(content_encoding, EncodingResource.out_encoding) self.assertEqual( response.body.decode(content_encoding), to_unicode(original_body)) + + +class WebClientSSLTestCase(unittest.TestCase): + context_factory = None + + def _listen(self, site): + return reactor.listenSSL( + 0, site, + contextFactory=self.context_factory or ssl_context_factory(), + interface="127.0.0.1") + + def getURL(self, path): + return "https://127.0.0.1:%d/%s" % (self.portno, path) + + def setUp(self): + self.tmpname = self.mktemp() + os.mkdir(self.tmpname) + FilePath(self.tmpname).child("file").setContent(b"0123456789") + r = static.File(self.tmpname) + r.putChild(b"payload", PayloadResource()) + self.site = server.Site(r, timeout=None) + self.wrapper = WrappingFactory(self.site) + self.port = self._listen(self.wrapper) + self.portno = self.port.getHost().port + + @inlineCallbacks + def tearDown(self): + yield self.port.stopListening() + shutil.rmtree(self.tmpname) + + def testPayload(self): + s = "0123456789" * 10 + return getPage(self.getURL("payload"), body=s).addCallback( + self.assertEqual, to_bytes(s)) + + +class WebClientBrokenSSLTestCase(WebClientSSLTestCase): + context_factory = broken_ssl_context_factory(cipher_string='CAMELLIA256-SHA') + + def testPayload(self): + s = "0123456789" * 10 + settings = Settings({'DOWNLOADER_CLIENT_TLS_CIPHERS': 'CAMELLIA256-SHA'}) + return getPage(self.getURL("payload"), body=s, + contextFactory=ScrapyClientContextFactory(settings=settings)).addCallback(self.assertEqual, + to_bytes(s)) From aaa5229e5db4f1c014ae69e0fb9e1a933f4952b0 Mon Sep 17 00:00:00 2001 From: Andrey Rakhmatullin Date: Mon, 15 Jul 2019 17:47:06 +0500 Subject: [PATCH 4/4] Fixes and improvements for DOWNLOADER_CLIENT_TLS_CIPHERS. --- docs/topics/settings.rst | 5 +++-- scrapy/core/downloader/handlers/http11.py | 2 +- scrapy/utils/ssl.py | 5 +++++ tests/mockserver.py | 19 ++++++++----------- tests/test_downloader_handlers.py | 6 +++--- tests/test_webclient.py | 23 ++++++++++++++++------- 6 files changed, 36 insertions(+), 24 deletions(-) diff --git a/docs/topics/settings.rst b/docs/topics/settings.rst index c042d3f43..0cb81c43e 100644 --- a/docs/topics/settings.rst +++ b/docs/topics/settings.rst @@ -442,8 +442,9 @@ or even enable client-side authentication (and various other things). If you do use a custom ContextFactory, make sure its ``__init__`` method accepts a ``method`` parameter (this is the ``OpenSSL.SSL`` method mapping -:setting:`DOWNLOADER_CLIENT_TLS_METHOD`) and a ``tls_verbose_logging`` -parameter (``bool``). +:setting:`DOWNLOADER_CLIENT_TLS_METHOD`), a ``tls_verbose_logging`` +parameter (``bool``) and a ``tls_ciphers`` parameter (see +:setting:`DOWNLOADER_CLIENT_TLS_CIPHERS`). .. setting:: DOWNLOADER_CLIENT_TLS_CIPHERS diff --git a/scrapy/core/downloader/handlers/http11.py b/scrapy/core/downloader/handlers/http11.py index e72052afc..91b45a8fc 100644 --- a/scrapy/core/downloader/handlers/http11.py +++ b/scrapy/core/downloader/handlers/http11.py @@ -54,7 +54,7 @@ class HTTP11DownloadHandler(object): ) msg = """ '%s' does not accept `method` argument (type OpenSSL.SSL method,\ - e.g. OpenSSL.SSL.SSLv23_METHOD) and/or `tls_verbose_logging` argument.\ + e.g. OpenSSL.SSL.SSLv23_METHOD) and/or `tls_verbose_logging` argument and/or `tls_ciphers` argument.\ Please upgrade your context factory class to handle them or ignore them.""" % ( settings['DOWNLOADER_CLIENTCONTEXTFACTORY'],) warnings.warn(msg) diff --git a/scrapy/utils/ssl.py b/scrapy/utils/ssl.py index 632827471..02aed60ee 100644 --- a/scrapy/utils/ssl.py +++ b/scrapy/utils/ssl.py @@ -6,6 +6,11 @@ import OpenSSL._util as pyOpenSSLutil from scrapy.utils.python import to_native_str +# The OpenSSL symbol is present since 1.1.1 but it's not currently supported in any version of pyOpenSSL. +# Using the binding directly, as this code does, requires cryptography 2.4. +SSL_OP_NO_TLSv1_3 = getattr(pyOpenSSLutil.lib, 'SSL_OP_NO_TLSv1_3', 0) + + def ffi_buf_to_string(buf): return to_native_str(pyOpenSSLutil.ffi.string(buf)) diff --git a/tests/mockserver.py b/tests/mockserver.py index 8be8a36bb..77908284b 100644 --- a/tests/mockserver.py +++ b/tests/mockserver.py @@ -4,7 +4,6 @@ from six.moves.urllib.parse import urlencode from subprocess import Popen, PIPE from OpenSSL import SSL - from twisted.web.server import Site, NOT_DONE_YET from twisted.web.resource import Resource from twisted.web.static import File @@ -15,8 +14,8 @@ from twisted.web.util import redirectTo from twisted.internet import reactor, ssl from twisted.internet.task import deferLater - from scrapy.utils.python import to_bytes, to_unicode +from scrapy.utils.ssl import SSL_OP_NO_TLSv1_3 def getarg(request, name, default=None, type=None): @@ -217,18 +216,16 @@ class MockServer(): return host + path -def ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.crt'): - return ssl.DefaultOpenSSLContextFactory( +def ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.crt', cipher_string=None): + factory = ssl.DefaultOpenSSLContextFactory( os.path.join(os.path.dirname(__file__), keyfile), os.path.join(os.path.dirname(__file__), certfile), ) - - -def broken_ssl_context_factory(keyfile='keys/localhost.key', certfile='keys/localhost.crt', cipher_string='DEFAULT'): - factory = ssl_context_factory(keyfile, certfile) - ctx = factory.getContext() - ctx.set_options(SSL.OP_CIPHER_SERVER_PREFERENCE | SSL.OP_NO_TLSv1_2) - ctx.set_cipher_list(cipher_string) + if cipher_string: + ctx = factory.getContext() + # disabling TLS1.2+ because it unconditionally enables some strong ciphers + ctx.set_options(SSL.OP_CIPHER_SERVER_PREFERENCE | SSL.OP_NO_TLSv1_2 | SSL_OP_NO_TLSv1_3) + ctx.set_cipher_list(to_bytes(cipher_string)) return factory diff --git a/tests/test_downloader_handlers.py b/tests/test_downloader_handlers.py index 5df2ffe45..109469503 100644 --- a/tests/test_downloader_handlers.py +++ b/tests/test_downloader_handlers.py @@ -39,7 +39,7 @@ from scrapy.utils.test import get_crawler, skip_if_no_boto from scrapy.utils.python import to_bytes from scrapy.exceptions import NotConfigured -from tests.mockserver import MockServer, ssl_context_factory, Echo, broken_ssl_context_factory +from tests.mockserver import MockServer, ssl_context_factory, Echo from tests.spiders import SingleRequestSpider @@ -553,7 +553,7 @@ class Https11InvalidDNSPattern(Https11TestCase): super(Https11InvalidDNSPattern, self).setUp() -class Https11BadCiphers(unittest.TestCase): +class Https11CustomCiphers(unittest.TestCase): scheme = 'https' download_handler_cls = HTTP11DownloadHandler @@ -569,7 +569,7 @@ class Https11BadCiphers(unittest.TestCase): self.wrapper = WrappingFactory(self.site) self.host = 'localhost' self.port = reactor.listenSSL( - 0, self.wrapper, broken_ssl_context_factory(self.keyfile, self.certfile, cipher_string='CAMELLIA256-SHA'), + 0, self.wrapper, ssl_context_factory(self.keyfile, self.certfile, cipher_string='CAMELLIA256-SHA'), interface=self.host) self.portno = self.port.getHost().port self.download_handler = self.download_handler_cls( diff --git a/tests/test_webclient.py b/tests/test_webclient.py index 2ebe075ab..a81946490 100644 --- a/tests/test_webclient.py +++ b/tests/test_webclient.py @@ -6,9 +6,10 @@ import os import six import shutil +import OpenSSL.SSL from twisted.trial import unittest from twisted.web import server, static, util, resource -from twisted.internet import reactor, defer, ssl +from twisted.internet import reactor, defer from twisted.test.proto_helpers import StringTransport from twisted.python.filepath import FilePath from twisted.protocols.policies import WrappingFactory @@ -18,8 +19,9 @@ from scrapy.core.downloader import webclient as client from scrapy.core.downloader.contextfactory import ScrapyClientContextFactory from scrapy.http import Request, Headers from scrapy.settings import Settings +from scrapy.utils.misc import create_instance from scrapy.utils.python import to_bytes, to_unicode -from tests.mockserver import ssl_context_factory, broken_ssl_context_factory +from tests.mockserver import ssl_context_factory def getPage(url, contextFactory=None, response_transform=None, *args, **kwargs): @@ -402,12 +404,19 @@ class WebClientSSLTestCase(unittest.TestCase): self.assertEqual, to_bytes(s)) -class WebClientBrokenSSLTestCase(WebClientSSLTestCase): - context_factory = broken_ssl_context_factory(cipher_string='CAMELLIA256-SHA') +class WebClientCustomCiphersSSLTestCase(WebClientSSLTestCase): + # we try to use a cipher that is not enabled by default in OpenSSL + custom_ciphers = 'CAMELLIA256-SHA' + context_factory = ssl_context_factory(cipher_string=custom_ciphers) def testPayload(self): s = "0123456789" * 10 - settings = Settings({'DOWNLOADER_CLIENT_TLS_CIPHERS': 'CAMELLIA256-SHA'}) + settings = Settings({'DOWNLOADER_CLIENT_TLS_CIPHERS': self.custom_ciphers}) + client_context_factory = create_instance(ScrapyClientContextFactory, settings=settings, crawler=None) return getPage(self.getURL("payload"), body=s, - contextFactory=ScrapyClientContextFactory(settings=settings)).addCallback(self.assertEqual, - to_bytes(s)) + contextFactory=client_context_factory).addCallback(self.assertEqual, to_bytes(s)) + + def testPayloadDefaultCiphers(self): + s = "0123456789" * 10 + d = getPage(self.getURL("payload"), body=s, contextFactory=ScrapyClientContextFactory()) + return self.assertFailure(d, OpenSSL.SSL.Error)