-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Support for overriding OpenSSL ciphers #3442
Conversation
if ciphers_string: | ||
self._ssl_ciphers = AcceptableCiphers.fromOpenSSLCipherString(ciphers_string) | ||
else: | ||
self._ssl_ciphers = DEFAULT_CIPHERS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEFAULT_CIPHERS
is a singleton so I kept it as it maybe improves the performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it helps, as download handler always pass ciphers_string, which is "DEFAULT" by default.
tests/mockserver.py
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TLS 1.2 has it's own always enabled ciphers so I disable it.
Twisted recommends using CertificateOptions
instead of DefaultOpenSSLContextFactory
, and its API is cleaner, but it requires file-like objects for certs and also I don't know if all supported Twisted versions support this API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CertificateOptions
was there at least in twisted 9.0.0 (https://twistedmatrix.com/documents/9.0.0/api/twisted.internet.ssl.CertificateOptions.html), so it should be fine to use. But I have no idea what I'm talking about, or what is better in this particular case :)
tests/mockserver.py
Outdated
@@ -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'): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be renamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain what this code does? Does it 1) add cipher_string support, and 2) disables default ciphers which come from TLSv1_2? And the purpose is to create servers which use outdated ciphers not supported by current http clients by default?
If so, what do you think about
- removing this function,
- adding cipher_string argument to ssl_context_factory, None by default,
- when cipher_string is not None, ssl_context_factory becomes broken_ssl_context_factory,
- add a comment which explains why
ctx.set_options(SSL.OP_CIPHER_SERVER_PREFERENCE | SSL.OP_NO_TLSv1_2)
line exists.
tests/test_webclient.py
Outdated
self.assertEqual, to_bytes(s)) | ||
|
||
|
||
class WebClientBrokenSSLTestCase(WebClientSSLTestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it should be renamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BrokenSSL -> OldCiphers / InsecureCIphers?
Codecov Report
@@ Coverage Diff @@
## master #3442 +/- ##
==========================================
- Coverage 85.35% 85.22% -0.14%
==========================================
Files 167 167
Lines 9699 9794 +95
Branches 1453 1478 +25
==========================================
+ Hits 8279 8347 +68
- Misses 1162 1185 +23
- Partials 258 262 +4
|
Awesome, this is a much-needed feature which may solve quite a lot of issues in Scrapy bug tracker 👍 |
tests/mockserver.py
Outdated
@@ -3,6 +3,8 @@ | |||
from six.moves.urllib.parse import urlencode | |||
from subprocess import Popen, PIPE | |||
|
|||
from OpenSSL import SSL | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a nitpick: this import should be with twisted imports according to pep8 (3 groups: stdlib imports, third-party imports, internal imports)
interface=self.host) | ||
self.portno = self.port.getHost().port | ||
self.download_handler = self.download_handler_cls( | ||
Settings({'DOWNLOADER_CLIENT_TLS_CIPHERS': 'CAMELLIA256-SHA'})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please also add a test which shows that downloading doesn't work with default DOWNLOADER_CLIENT_TLS_CIPHERS, to make sure the other test is valid?
The tests fail on the "jessie" env, I tried different pyOpenSSL and cryptography versions and found that the tests only work on cryptography 2.0+ (IIRC). Though I don't know why the Travis test failed with |
@wRAR could you please check what cryptography versions are available in Debians and Ubuntus we care about? |
@kmike We currently don't set the version constraints for pyOpenSSL or cryptography on envs other than jessie (and trusty, which we don't test anymore). Note that cryptography 2.0 only works with pyOpenSSL 16.2+. The failing test expects a connection error with a non-default server cipher and the default client setting but the connection works on these older libraries. I didn't see anything in the cryptography 2.0 changelog and didn't try to investigate further. |
Thanks @wRAR! |
This adds the
DOWNLOADER_CLIENT_TLS_CIPHERS
setting which is then used inScrapyClientContextFactory
. There is a real-world example of when it may be needed in #3392 (the server offers a cipher that is rejected by the client so we need to not advertise it), there are other theoretical problems e.g. a server supports only ciphers that are supported by the client but disabled. I've constructed the second case in the tests because constructing the first case seems to be impossible with a modern OpenSSL.There are two tests, the one in
tests/test_webclient.py
usesScrapyClientContextFactory
directly (its base class also just tests the default context factory with the default SSL server) and the one intests/test_downloader_handlers.py
uses the setting and the download handler. I don't know if the first one is worth keeping. I also don't like the amount of duplicated code in the added test cases, maybe some reshuffling of base HTTP(S) test classes will be useful.