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

Re-enable TLS 1.2 in cipher tests. #4735

Merged
merged 1 commit into from Aug 25, 2020
Merged

Re-enable TLS 1.2 in cipher tests. #4735

merged 1 commit into from Aug 25, 2020

Conversation

wRAR
Copy link
Contributor

@wRAR wRAR commented Aug 17, 2020

This should fix cipher tests on platforms where TLS 1.2 is the minimum supported version such as modern Debian. I don't remember why I disabled 1.2 initially as only 1.3 adds ciphers unconditionally (check openssl ciphers -s -tls1_2 CAMELLIA256-SHA and openssl ciphers -s -tls1_3 CAMELLIA256-SHA).

We'll need to do something else (and probably in Scrapy too, not just in tests) when TLS version reqs are even stricter.

Fixes #4726.

Note that tests/test_downloader_handlers.py::Https11CustomCiphers is fine when additional ciphers are enabled (so e.g. won't fail when SSL_OP_NO_TLSv1_3 is removed) while tests/test_webclient.py::WebClientCustomCiphersSSLTestCase also checks for an error when the ciphers don't match (and will fail without SSL_OP_NO_TLSv1_3 as in that case the custom ciphers will be ignored).

@wRAR wRAR added the CI label Aug 17, 2020
@codecov
Copy link

codecov bot commented Aug 17, 2020

Codecov Report

Merging #4735 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4735      +/-   ##
==========================================
- Coverage   87.17%   87.14%   -0.04%     
==========================================
  Files         160      160              
  Lines        9795     9795              
  Branches     1442     1442              
==========================================
- Hits         8539     8536       -3     
- Misses        995      996       +1     
- Partials      261      263       +2     
Impacted Files Coverage Δ
scrapy/utils/trackref.py 82.85% <0.00%> (-2.86%) ⬇️
scrapy/core/downloader/__init__.py 89.47% <0.00%> (-1.51%) ⬇️

Copy link
Member

@Gallaecio Gallaecio left a comment

Travis CI does not support Debian, but I wonder if we could use GitHub Actions (@elacuesta, you are the expert here!) to run the tests with Debian unstable through Docker.

@wRAR
Copy link
Contributor Author

wRAR commented Aug 18, 2020

Note that this should be run with the system libs (which isn't always possible). Not sure which OpenSSL is used in a tox venv but it works differently from the system one.

@elacuesta
Copy link
Member

elacuesta commented Aug 24, 2020

Seems like Debian is not among the available environments. It would appear to be possible to run our own self-hosted job runners, I'm not sure that's something we want to do though.

@wRAR wRAR merged commit 64e0ea4 into scrapy:master Aug 25, 2020
2 checks passed
@wRAR wRAR deleted the ciphers-tests branch Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failures on Debian Unstable
3 participants