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

[MRG+1] Use OpenSSL default ciphers #2314

Merged
merged 1 commit into from Oct 17, 2016
Merged

[MRG+1] Use OpenSSL default ciphers #2314

merged 1 commit into from Oct 17, 2016

Conversation

@redapple
Copy link
Contributor

@redapple redapple commented Oct 7, 2016

Twisted default TLS options restricts the ciphers list a bit -- "a secure default"
https://github.com/twisted/twisted/blob/e38cc25a67747899c6984d6ebaa8d3d134799415/src/twisted/internet/_sslverify.py#L1861

We want to be a bit more permissive with Scrapy
(at least as permissive as Scrapy 1.0 was, and which used a default OpenSSL Context)

See https://www.openssl.org/docs/manmaster/apps/ciphers.html#CIPHER_STRINGS

OpenSSL's DEFAULT seems to be reasonable enough.

Fixes #2311

Twisted default TLS options restricts the ciphers list a bit -- "a secure default"
https://github.com/twisted/twisted/blob/e38cc25a67747899c6984d6ebaa8d3d134799415/src/twisted/internet/_sslverify.py#L1861

We want to be a bit more permissive with Scrapy
(at least as permissive as Scrapy 1.0 was, and which used a default OpenSSL Context)

See https://www.openssl.org/docs/manmaster/apps/ciphers.html#CIPHER_STRINGS

OpenSSL's 'DEFAULT' seems to be reasonable enough.

Fixes #2311
@redapple redapple added this to the v1.2.1 milestone Oct 7, 2016
@redapple redapple mentioned this pull request Oct 7, 2016
@codecov-io
Copy link

@codecov-io codecov-io commented Oct 7, 2016

Current coverage is 83.31% (diff: 100%)

Merging #2314 into master will increase coverage by 3.35%

@@             master      #2314   diff @@
==========================================
  Files           161        161          
  Lines          8719       8721     +2   
  Methods           0          0          
  Messages          0          0          
  Branches       1284       1284          
==========================================
+ Hits           6972       7266   +294   
+ Misses         1496       1204   -292   
  Partials        251        251          

Powered by Codecov. Last update 3235bfe...c341137

@@ -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,

This comment has been minimized.

@kmike

kmike Oct 17, 2016
Member

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.

This comment has been minimized.

@redapple

redapple Oct 17, 2016
Author Contributor

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

This comment has been minimized.

@kmike

kmike Oct 17, 2016
Member

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

@kmike
Copy link
Member

@kmike kmike commented Oct 17, 2016

Looks good, a nice find!

@kmike kmike changed the title Use OpenSSL default ciphers [MRG+1] Use OpenSSL default ciphers Oct 17, 2016
@kmike kmike merged commit 7e20725 into scrapy:master Oct 17, 2016
3 checks passed
3 checks passed
codecov/patch 100% of diff hit (target 79.96%)
Details
codecov/project 83.31% (+3.35%) compared to 3235bfe
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@redapple redapple deleted the redapple:tls-ciphers branch Feb 23, 2017
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

3 participants