From 68b96705a9e6f4904992cadbdb826928280f9a18 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 8 Nov 2018 10:29:46 -0500 Subject: [PATCH 1/8] Add more logging for this test --- distributed/comm/tests/test_comms.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/distributed/comm/tests/test_comms.py b/distributed/comm/tests/test_comms.py index a0155052942..3f4ae8fa1b9 100644 --- a/distributed/comm/tests/test_comms.py +++ b/distributed/comm/tests/test_comms.py @@ -580,8 +580,13 @@ def handle_comm(comm): listener.start() with pytest.raises(EnvironmentError) as excinfo: - yield connect(listener.contact_address, timeout=0.5, - connection_args={'ssl_context': bad_cli_ctx}) + try: + yield connect(listener.contact_address, timeout=0.5, + connection_args={'ssl_context': bad_cli_ctx}) + except Exception as e: + print(type(e), flush=True) + print(repr(e), flush=True) + raise e # The wrong error is reported on Python 2, see https://github.com/tornadoweb/tornado/pull/2028 if sys.version_info >= (3,) and os.name != 'nt': From 4939f89ebaef301b322c46bd93fa2e29b6a65589 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 8 Nov 2018 10:31:13 -0500 Subject: [PATCH 2/8] Check ciphers less aggresively for TLS1.3 ciphers When python is compiled against openssl 1.1 we have TLS1.3 ciphers in the mix as well. We should treat these appropriately --- distributed/tests/test_security.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/distributed/tests/test_security.py b/distributed/tests/test_security.py index 76003f3c73b..4eec7ad176f 100644 --- a/distributed/tests/test_security.py +++ b/distributed/tests/test_security.py @@ -201,7 +201,12 @@ def many_ciphers(ctx): ctx = d['ssl_context'] basic_checks(ctx) if sys.version_info >= (3, 6): - assert len(ctx.get_ciphers()) == 1 + supported_ciphers = ctx.get_ciphers() + tls_12_ciphers = [c for c in supported_ciphers if c['protocol'] == 'TLSv1.2'] + assert len(tls_12_ciphers) == 1 + tls_13_ciphers = [c for c in supported_ciphers if c['protocol'] == 'TLSv1.3'] + if len(tls_13_ciphers): + assert len(tls_13_ciphers) == 3 def test_listen_args(): @@ -255,7 +260,12 @@ def many_ciphers(ctx): ctx = d['ssl_context'] basic_checks(ctx) if sys.version_info >= (3, 6): - assert len(ctx.get_ciphers()) == 1 + supported_ciphers = ctx.get_ciphers() + tls_12_ciphers = [c for c in supported_ciphers if c['protocol'] == 'TLSv1.2'] + assert len(tls_12_ciphers) == 1 + tls_13_ciphers = [c for c in supported_ciphers if c['protocol'] == 'TLSv1.3'] + if len(tls_13_ciphers): + assert len(tls_13_ciphers) == 3 @gen_test() From 88130fc31b20e66e83db689331eb8d8a7a4d6919 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 8 Nov 2018 12:16:32 -0500 Subject: [PATCH 3/8] Attempt to mitigate the failed connection case --- distributed/comm/core.py | 2 ++ distributed/comm/tcp.py | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/distributed/comm/core.py b/distributed/comm/core.py index 319e5580874..5994743ce15 100644 --- a/distributed/comm/core.py +++ b/distributed/comm/core.py @@ -184,6 +184,8 @@ def _raise(error): comm = yield gen.with_timeout(timedelta(seconds=deadline - time()), future, quiet_exceptions=EnvironmentError) + except CommClosedError as e: + raise e except EnvironmentError as e: error = str(e) if time() < deadline: diff --git a/distributed/comm/tcp.py b/distributed/comm/tcp.py index 4db9ca2ad41..270f6ab0033 100644 --- a/distributed/comm/tcp.py +++ b/distributed/comm/tcp.py @@ -329,6 +329,13 @@ def connect(self, address, deserialize=True, **connection_args): stream = yield client.connect(ip, port, max_buffer_size=MAX_BUFFER_SIZE, **kwargs) + # Under certain circumstances tornado will have a closed connnection with an error and not raise + # a StreamClosedError. + # + # This occurs with tornado 5.x and openssl 1.1+ + if stream.closed() and stream.error: + raise StreamClosedError(stream.error) + except StreamClosedError as e: # The socket connect() call failed convert_stream_closed_error(self, e) From 4fcb1fdc7d9b7a46348f534ef14475459a96223c Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 8 Nov 2018 12:51:10 -0500 Subject: [PATCH 4/8] Introduced a FatalCommClosedError This deals with TLS errors that cannot be recovered. --- distributed/comm/core.py | 6 +++++- distributed/comm/tcp.py | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/distributed/comm/core.py b/distributed/comm/core.py index 5994743ce15..e11292aca84 100644 --- a/distributed/comm/core.py +++ b/distributed/comm/core.py @@ -21,6 +21,10 @@ class CommClosedError(IOError): pass +class FatalCommClosedError(CommClosedError): + pass + + class Comm(with_metaclass(ABCMeta)): """ A message-oriented communication object, representing an established @@ -184,7 +188,7 @@ def _raise(error): comm = yield gen.with_timeout(timedelta(seconds=deadline - time()), future, quiet_exceptions=EnvironmentError) - except CommClosedError as e: + except FatalCommClosedError as e: raise e except EnvironmentError as e: error = str(e) diff --git a/distributed/comm/tcp.py b/distributed/comm/tcp.py index 270f6ab0033..27e2bb675a6 100644 --- a/distributed/comm/tcp.py +++ b/distributed/comm/tcp.py @@ -24,7 +24,7 @@ from .registry import Backend, backends from .addressing import parse_host_port, unparse_host_port -from .core import Comm, Connector, Listener, CommClosedError +from .core import Comm, Connector, Listener, CommClosedError, FatalCommClosedError from .utils import (to_frames, from_frames, get_tcp_server_address, ensure_concrete_host) @@ -121,6 +121,10 @@ def convert_stream_closed_error(obj, exc): if exc.real_error is not None: # The stream was closed because of an underlying OS error exc = exc.real_error + import ssl + if isinstance(exc, ssl.SSLError): + if 'UNKNOWN_CA' in exc.reason: + raise FatalCommClosedError("in %s: %s: %s" % (obj, exc.__class__.__name__, exc)) raise CommClosedError("in %s: %s: %s" % (obj, exc.__class__.__name__, exc)) else: raise CommClosedError("in %s: %s" % (obj, exc)) From 6c07897f470f7cfc2b2d8efdfc2046deff9db757 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 8 Nov 2018 13:31:25 -0500 Subject: [PATCH 5/8] Relaxed forced cipher test --- distributed/tests/test_security.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/distributed/tests/test_security.py b/distributed/tests/test_security.py index 4eec7ad176f..7ebd414ca24 100644 --- a/distributed/tests/test_security.py +++ b/distributed/tests/test_security.py @@ -25,6 +25,14 @@ # Note this cipher uses RSA auth as this matches our test certs FORCED_CIPHER = 'ECDHE-RSA-AES128-GCM-SHA256' +TLS_13_CIPHERS = [ + 'TLS_AES_128_GCM_SHA256', + 'TLS_AES_256_GCM_SHA384', + 'TLS_CHACHA20_POLY1305_SHA256', + 'TLS_AES_128_CCM_SHA256', + 'TLS_AES_128_CCM_8_SHA256', +] + def test_defaults(): with new_config({}): @@ -316,7 +324,7 @@ def handle_comm(comm): comm = yield connect(listener.contact_address, connection_args=forced_cipher_sec.get_connection_args('worker')) cipher, _, _, = comm.extra_info['cipher'] - assert cipher == FORCED_CIPHER + assert cipher in [FORCED_CIPHER] + TLS_13_CIPHERS comm.abort() From 8f7b5496166b0eaf301b0f26ec6e4ac272c856af Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Thu, 8 Nov 2018 14:24:49 -0500 Subject: [PATCH 6/8] Remove debug code --- distributed/comm/tests/test_comms.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/distributed/comm/tests/test_comms.py b/distributed/comm/tests/test_comms.py index 3f4ae8fa1b9..a0155052942 100644 --- a/distributed/comm/tests/test_comms.py +++ b/distributed/comm/tests/test_comms.py @@ -580,13 +580,8 @@ def handle_comm(comm): listener.start() with pytest.raises(EnvironmentError) as excinfo: - try: - yield connect(listener.contact_address, timeout=0.5, - connection_args={'ssl_context': bad_cli_ctx}) - except Exception as e: - print(type(e), flush=True) - print(repr(e), flush=True) - raise e + yield connect(listener.contact_address, timeout=0.5, + connection_args={'ssl_context': bad_cli_ctx}) # The wrong error is reported on Python 2, see https://github.com/tornadoweb/tornado/pull/2028 if sys.version_info >= (3,) and os.name != 'nt': From ca90f8ae8bd25a8067e373fcca260bcfe8f7621e Mon Sep 17 00:00:00 2001 From: Matthew Rocklin Date: Thu, 8 Nov 2018 18:32:04 -0500 Subject: [PATCH 7/8] raise bare exception --- distributed/comm/core.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/distributed/comm/core.py b/distributed/comm/core.py index e11292aca84..82f2965d8ec 100644 --- a/distributed/comm/core.py +++ b/distributed/comm/core.py @@ -188,8 +188,8 @@ def _raise(error): comm = yield gen.with_timeout(timedelta(seconds=deadline - time()), future, quiet_exceptions=EnvironmentError) - except FatalCommClosedError as e: - raise e + except FatalCommClosedError: + raise except EnvironmentError as e: error = str(e) if time() < deadline: From 1021ced739f4b06f79c432a8ed5941485f0d19c0 Mon Sep 17 00:00:00 2001 From: Matthew Rocklin Date: Thu, 8 Nov 2018 18:33:11 -0500 Subject: [PATCH 8/8] avoid internal import ssl --- distributed/comm/tcp.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/distributed/comm/tcp.py b/distributed/comm/tcp.py index 27e2bb675a6..a838b08f360 100644 --- a/distributed/comm/tcp.py +++ b/distributed/comm/tcp.py @@ -121,8 +121,7 @@ def convert_stream_closed_error(obj, exc): if exc.real_error is not None: # The stream was closed because of an underlying OS error exc = exc.real_error - import ssl - if isinstance(exc, ssl.SSLError): + if ssl and isinstance(exc, ssl.SSLError): if 'UNKNOWN_CA' in exc.reason: raise FatalCommClosedError("in %s: %s: %s" % (obj, exc.__class__.__name__, exc)) raise CommClosedError("in %s: %s: %s" % (obj, exc.__class__.__name__, exc))