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.

Sign up for GitHub

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

fix tls_(min|max)_version having no effect on openssl 1.1.0g or lower #6562

Merged
merged 2 commits into from
Jun 15, 2022
Merged

fix tls_(min|max)_version having no effect on openssl 1.1.0g or lower #6562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fd36bdd
fix tls_(min|max)_version having no effect on openssl 1.1.0g or lower
graingert Jun 13, 2022
d7e25cd
deprecate support for openssl < 1.1.1
graingert Jun 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 60 additions & 3 deletions distributed/security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,68 @@
from __future__ import annotations

import datetime
import os
import sys
import tempfile
import warnings

try:
import ssl
except ImportError:
ssl = None # type: ignore

try:
from ssl import OPENSSL_VERSION_INFO as _OPENSSL_VERSION_INFO
except ImportError:
_OPENSSL_VERSION_INFO = (0, 0, 0, 0, 0)

import dask
from dask.widgets import get_template

__all__ = ("Security",)


if sys.version_info >= (3, 10) or _OPENSSL_VERSION_INFO >= (1, 1, 0, 7):
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

This should work with both newer openssl versions or newer python versions

Copy link
Member Author

Choose a reason for hiding this comment

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

this is relying on new openssl features released in 1.1.0g and made available in Python 3.7

# 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
Comment on lines +26 to +31
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@fjetter https://lwn.net/Articles/861910/

Copy link
Member

Choose a reason for hiding this comment

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

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
    ...

Copy link
Member Author

Choose a reason for hiding this comment

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

the fix suggested was to make ssl.SSLContext slotted, which would both prevent typos and use of correctly spelt attributes that are unsuported


# these _set_mimimun_version and _set_maximum_version depend on the validation
# already performed in `Security._set_tls_version_field`,
# and that they only apply to freshly created ssl.SSLContext instances in
# _get_tls_context
def _set_minimum_version(ctx: ssl.SSLContext, version: ssl.TLSVersion) -> None:
ctx.minimum_version = version

def _set_maximum_version(ctx: ssl.SSLContext, version: ssl.TLSVersion) -> None:
ctx.maximum_version = version

else:
graingert marked this conversation as resolved.
Show resolved Hide resolved

def _set_minimum_version(ctx: ssl.SSLContext, version: ssl.TLSVersion) -> None:
# if the ctx.maximum_version attribute is unsupported then we can infer
# that TLS 1.3 is not supported.
# _set_tls_version_field enforces that version is TLSVersion.TLSv1_2,
# or TLSVersion.TLSv1_3
if version is not ssl.TLSVersion.TLSv1_2:
raise ValueError(f"Unsupported TLS/SSL version: {version!r}")
ctx.options |= (
ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3 | ssl.OP_NO_TLSv1 | ssl.OP_NO_TLSv1_1
)

def _set_maximum_version(ctx: ssl.SSLContext, version: ssl.TLSVersion) -> None:
# if the ctx.maximum_version attribute is unsupported then we can infer
# that TLSv1_3 is not supported.
# _set_tls_version_field enforces that version is TLSVersion.TLSv1_2,
# TLSVersion.TLSv1_3, or None
# _get_tls_context enforces that version is not None
if version is not ssl.TLSVersion.TLSv1_2:
raise ValueError(f"Unsupported TLS/SSL version: {version!r}")


class Security:
"""Security configuration for a Dask cluster.

Expand Down Expand Up @@ -71,6 +121,12 @@ class Security:
)

def __init__(self, require_encryption=None, **kwargs):
if _OPENSSL_VERSION_INFO < (1, 1, 1):
warnings.warn(
f"support for {ssl.OPENSSL_VERSION} is deprecated,"
" and will be removed in a future release",
DeprecationWarning,
)
extra = set(kwargs).difference(self.__slots__)
if extra:
raise TypeError("Unknown parameters: %r" % sorted(extra))
Expand Down Expand Up @@ -251,10 +307,11 @@ def _get_tls_context(self, tls, purpose):
else:
ctx = ssl.create_default_context(purpose=purpose, cafile=ca)

if self.tls_min_version is not None:
ctx.minimum_version = self.tls_min_version
# the _set_tls_version_field method enforces that
# self.tls_min_version is TLSv1_2, or TLSv1_3
_set_minimum_version(ctx, self.tls_min_version)
if self.tls_max_version is not None:
ctx.maximum_version = self.tls_max_version
_set_maximum_version(ctx, self.tls_max_version)

cert_in_memory = "\n" in cert
key_in_memory = key is not None and "\n" in key
Expand Down
8 changes: 8 additions & 0 deletions distributed/tests/test_security.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,14 @@ def test_min_max_version_config_errors(field):
sec = Security()


def test_invalid_min_version_from_config_errors():
with dask.config.set({"distributed.comm.tls.min-version": None}):
with pytest.raises(
ValueError, match=r"tls_min_version=.* is not supported, expected one of .*"
):
Security(tls_min_version=ssl.TLSVersion.MINIMUM_SUPPORTED)


def test_kwargs():
c = {
"distributed.comm.tls.ca-file": "ca.pem",
Expand Down