fix tls_(min|max)_version having no effect on openssl 1.1.0g or lower #6562
Conversation
distributed/security.py
Outdated
| _min_version[minimum_version] | _max_version[maximum_version] | ||
| ) | ||
| except KeyError: | ||
| raise RuntimeError("invalid TLSVersion selected") |
There was a problem hiding this comment.
setting maximum_version=MAXIMUM_SUPPORTED or maximum_version=MINIMUM_SUPPORTED or minimum_version=MAXIMUM_SUPPORTED isn't possible because we can't introspect the versions until 1.1.0.g anyway
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 15 files ±0 15 suites ±0 6h 20m 13s ⏱️ - 29m 25s For more details on these failures, see this check. Results for commit d7e25cd. ± Comparison against base commit 344868a. ♻️ This comment has been updated with latest results. |
| __all__ = ("Security",) | ||
|
|
||
|
|
||
| if sys.version_info >= (3, 10) or _OPENSSL_VERSION_INFO >= (1, 1, 0, 7): |
There was a problem hiding this comment.
Note if this is using newer APIs from OpenSSL, Python 3.10 is the only version I'm aware of doing that (from a different context). So checking the OpenSSL version may not be sufficient. In fact it may lead to issues as packagers may build with a newer OpenSSL even with old Python versions (as is the case in conda-forge)
There was a problem hiding this comment.
This should work with both newer openssl versions or newer python versions
There was a problem hiding this comment.
this is relying on new openssl features released in 1.1.0g and made available in Python 3.7
graingert
force-pushed
the
fix-insecure-cipher-suites
branch
from
bfbe501 to
5c55f89
Compare
It's very difficult to test this - as we'd need to find a docker image (centos:7) or similar to run the old version of openssl and compile a new version of python 3.7 instead of a test - I've simplified the code by a great deal by making distributed.security.Security immutable and then relying on earlier validation to support assignment of tls.TLSVersion.TLSv1_2 I have also deprecated support for openssl < 1.1.1 so we can remove this untested branch in the future |
graingert
requested review from
jcrist and
fjetter
and removed request for
jcrist
graingert
force-pushed
the
fix-insecure-cipher-suites
branch
from
5c55f89 to
825232e
Compare
distributed/security.py
Outdated
| raise AttributeError(f"Can't delete attribute {name!r}") | ||
|
|
||
| # __setstate__is needed for un-pickling frozen classes | ||
| def __setstate__(self, state): |
There was a problem hiding this comment.
making Security frozen broke pickle support:
https://github.com/dask/distributed/runs/6863709912?check_suite_focus=true#step:11:182
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "C:\Miniconda3\envs\dask-distributed\lib\multiprocessing\spawn.py", line 116, in spawn_main
exitcode = _main(fd, parent_sentinel)
File "C:\Miniconda3\envs\dask-distributed\lib\multiprocessing\spawn.py", line 126, in _main
self = reduction.pickle.load(from_parent)
File "d:\a\distributed\distributed\distributed\security.py", line 368, in __setattr__
raise AttributeError(f"Can't set attribute {name!r}")
AttributeError: Can't set attribute 'require_encryption'
maybe making it frozen isn't worth it?
There was a problem hiding this comment.
I've removed the commits making security.Security frozen from this PR
Python3.10 has dropped support for 1.1.1 https://peps.python.org/pep-0644/
graingert
force-pushed
the
fix-insecure-cipher-suites
branch
from
825232e to
d7e25cd
Compare
|
macos-latest 3.10 not c1 failed with #6395 |
| # The OP_NO_SSL* and OP_NO_TLS* become deprecated in favor of | ||
| # 'SSLContext.minimum_version' from Python 3.7 onwards, however | ||
| # this attribute is not available unless the ssl module is compiled | ||
| # with OpenSSL 1.1.0g or newer. | ||
| # https://docs.python.org/3.10/library/ssl.html#ssl.SSLContext.minimum_version | ||
| # https://docs.python.org/3.7/library/ssl.html#ssl.SSLContext.minimum_version |
There was a problem hiding this comment.
It's amazing that CPython chooses to deal with this issue merely by leaving a comment in the documentation. They could've raised if the attribute is set but the OpenSSL is too old
There was a problem hiding this comment.
There was a problem hiding this comment.
Interesting but not the same thing, is it? You apparently had a typo but we had the proper name. It could be something like
...
@property.setter
def minimal_version(self, value):
if openssl_version < 42:
raise
...There was a problem hiding this comment.
the fix suggested was to make ssl.SSLContext slotted, which would both prevent typos and use of correctly spelt attributes that are unsuported
fixes #6558
see also https://github.com/encode/httpx/blob/master/httpx/_compat.py#L23 and urllib3/urllib3#2636
pre-commit run --all-files