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

Enable unsafe legacy renegotiation #5790

Merged
merged 3 commits into from
Jan 17, 2023
Merged

Enable unsafe legacy renegotiation #5790

merged 3 commits into from
Jan 17, 2023

Conversation

wRAR
Copy link
Member

@wRAR wRAR commented Jan 13, 2023

Fixes #5491.

I don't know if we can have a test for it: it would require a server configured in a certain way, I need to check if it's even possible when using OpenSSL 3 on the server side. Locally I tested it on https://dorotheum.com/ with "pyOpenSSL 21.0.0 ('OpenSSL 1.1.1j 16 Feb 2021'), cryptography 3.4.6" (no error and website is accessible with and without the change) and "pyOpenSSL 23.0.0 ('OpenSSL 3.0.7 1 Nov 2022'), cryptography 39.0.0" (website is accessible only with the change).

import OpenSSL._util as pyOpenSSLutil

from scrapy.utils.python import to_unicode


# 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)
Copy link
Member Author

Choose a reason for hiding this comment

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

We can't deprecate this without some magic (probably the least magic way is the module-level __getattr__).

If we decide to deprecate it, this line can be changed into SSL_OP_NO_TLSv1_3 = OpenSSL.SSL.OP_NO_TLSv1_3 and the deprecation will be done separately.

Copy link
Member

Choose a reason for hiding this comment

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

No strong opinion either way from me.

@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #5790 (fa4b123) into master (4af5a06) will decrease coverage by 0.13%.
The diff coverage is 100.00%.

❗ Current head fa4b123 differs from pull request most recent head 43ab8bd. Consider uploading reports for the commit 43ab8bd to get more accurate results

@@            Coverage Diff             @@
##           master    #5790      +/-   ##
==========================================
- Coverage   88.89%   88.75%   -0.14%     
==========================================
  Files         162      162              
  Lines       10982    10981       -1     
  Branches     1797     1796       -1     
==========================================
- Hits         9762     9746      -16     
- Misses        940      954      +14     
- Partials      280      281       +1     
Impacted Files Coverage Δ
scrapy/core/downloader/tls.py 97.05% <ø> (ø)
scrapy/core/downloader/contextfactory.py 87.50% <100.00%> (+0.46%) ⬆️
scrapy/utils/ssl.py 55.26% <100.00%> (+1.60%) ⬆️
scrapy/robotstxt.py 75.30% <0.00%> (-13.59%) ⬇️
scrapy/core/downloader/__init__.py 90.97% <0.00%> (-1.51%) ⬇️
scrapy/core/downloader/handlers/http11.py 92.97% <0.00%> (-1.01%) ⬇️
scrapy/core/http2/stream.py 91.32% <0.00%> (-0.58%) ⬇️

@wRAR
Copy link
Member Author

wRAR commented Jan 13, 2023

Yeah, I don't see which flags could I set on the mockserver SSL context so that it could reject normal OpenSSL 3 clients.

See also https://www.openssl.org/docs/manmaster/man3/SSL_get_secure_renegotiation_support.html#SECURE-RENEGOTIATION

@wRAR wRAR merged commit 2f12f5c into master Jan 17, 2023
@wRAR wRAR deleted the ssl-legacy-reneg branch January 17, 2023 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenSSL unsafe legacy renegotiation disabled error
2 participants