diff --git a/CHANGELOG.md b/CHANGELOG.md index 9761ad3e9..2cc823d1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Changed * Support for TLS-SNI-01 has been removed from all official Certbot plugins. +* Attributes related to the TLS-SNI-01 challenge in `acme.challenges` and `acme.standalone` + modules are deprecated and will be removed soon. * CLI flags `--tls-sni-01-port` and `--tls-sni-01-address` are now no-op, will generate a deprecation warning if used, and will be removed soon. * Options `tls-sni` and `tls-sni-01` in `--preferred-challenges` flag are now no-op, diff --git a/acme/acme/__init__.py b/acme/acme/__init__.py index d91072a3b..20c008d64 100644 --- a/acme/acme/__init__.py +++ b/acme/acme/__init__.py @@ -6,6 +6,7 @@ This module is an implementation of the `ACME protocol`_. """ import sys +import warnings # This code exists to keep backwards compatibility with people using acme.jose # before it became the standalone josepy package. @@ -20,3 +21,30 @@ for mod in list(sys.modules): # preserved (acme.jose.* is josepy.*) if mod == 'josepy' or mod.startswith('josepy.'): sys.modules['acme.' + mod.replace('josepy', 'jose', 1)] = sys.modules[mod] + + +# This class takes a similar approach to the cryptography project to deprecate attributes +# in public modules. See the _ModuleWithDeprecation class here: +# https://github.com/pyca/cryptography/blob/91105952739442a74582d3e62b3d2111365b0dc7/src/cryptography/utils.py#L129 +class _TLSSNI01DeprecationModule(object): + """ + Internal class delegating to a module, and displaying warnings when + attributes related to TLS-SNI-01 are accessed. + """ + def __init__(self, module): + self.__dict__['_module'] = module + + def __getattr__(self, attr): + if 'TLSSNI01' in attr: + warnings.warn('{0} attribute is deprecated, and will be removed soon.'.format(attr), + DeprecationWarning, stacklevel=2) + return getattr(self._module, attr) + + def __setattr__(self, attr, value): # pragma: no cover + setattr(self._module, attr, value) + + def __delattr__(self, attr): # pragma: no cover + delattr(self._module, attr) + + def __dir__(self): # pragma: no cover + return ['_module'] + dir(self._module) diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py index 6f2b3757b..36e7ab41c 100644 --- a/acme/acme/challenges.py +++ b/acme/acme/challenges.py @@ -4,7 +4,7 @@ import functools import hashlib import logging import socket -import warnings +import sys from cryptography.hazmat.primitives import hashes # type: ignore import josepy as jose @@ -15,6 +15,7 @@ import six from acme import errors from acme import crypto_util from acme import fields +from acme import _TLSSNI01DeprecationModule logger = logging.getLogger(__name__) @@ -515,8 +516,6 @@ class TLSSNI01(KeyAuthorizationChallenge): #n = jose.Field("n", encoder=int, decoder=int) def __init__(self, *args, **kwargs): - warnings.warn("TLS-SNI-01 is deprecated, and will stop working soon.", - DeprecationWarning, stacklevel=2) super(TLSSNI01, self).__init__(*args, **kwargs) def validation(self, account_key, **kwargs): @@ -641,3 +640,7 @@ class DNSResponse(ChallengeResponse): """ return chall.check_validation(self.validation, account_public_key) + + +# Patching ourselves to warn about TLS-SNI challenge deprecation and removal. +sys.modules[__name__] = _TLSSNI01DeprecationModule(sys.modules[__name__]) diff --git a/acme/acme/challenges_test.py b/acme/acme/challenges_test.py index 4b905c1e5..edfaa3423 100644 --- a/acme/acme/challenges_test.py +++ b/acme/acme/challenges_test.py @@ -1,6 +1,5 @@ """Tests for acme.challenges.""" import unittest -import warnings import josepy as jose import mock @@ -374,25 +373,16 @@ class TLSSNI01Test(unittest.TestCase): 'type': 'tls-sni-01', 'token': 'a82d5ff8ef740d12881f6d3c2277ab2e', } - - def _msg(self): from acme.challenges import TLSSNI01 - with warnings.catch_warnings(record=True) as warn: - warnings.simplefilter("always") - msg = TLSSNI01( - token=jose.b64decode('a82d5ff8ef740d12881f6d3c2277ab2e')) - assert warn is not None # using a raw assert for mypy - self.assertTrue(len(warn) == 1) - self.assertTrue(issubclass(warn[-1].category, DeprecationWarning)) - self.assertTrue('deprecated' in str(warn[-1].message)) - return msg + self.msg = TLSSNI01( + token=jose.b64decode('a82d5ff8ef740d12881f6d3c2277ab2e')) def test_to_partial_json(self): - self.assertEqual(self.jmsg, self._msg().to_partial_json()) + self.assertEqual(self.jmsg, self.msg.to_partial_json()) def test_from_json(self): from acme.challenges import TLSSNI01 - self.assertEqual(self._msg(), TLSSNI01.from_json(self.jmsg)) + self.assertEqual(self.msg, TLSSNI01.from_json(self.jmsg)) def test_from_json_hashable(self): from acme.challenges import TLSSNI01 @@ -407,10 +397,18 @@ class TLSSNI01Test(unittest.TestCase): @mock.patch('acme.challenges.TLSSNI01Response.gen_cert') def test_validation(self, mock_gen_cert): mock_gen_cert.return_value = ('cert', 'key') - self.assertEqual(('cert', 'key'), self._msg().validation( + self.assertEqual(('cert', 'key'), self.msg.validation( KEY, cert_key=mock.sentinel.cert_key)) mock_gen_cert.assert_called_once_with(key=mock.sentinel.cert_key) + def test_deprecation_message(self): + with mock.patch('acme.warnings.warn') as mock_warn: + from acme.challenges import TLSSNI01 + assert TLSSNI01 + self.assertEqual(mock_warn.call_count, 1) + self.assertTrue('deprecated' in mock_warn.call_args[0][0]) + + class TLSALPN01ResponseTest(unittest.TestCase): # pylint: disable=too-many-instance-attributes diff --git a/acme/acme/crypto_util.py b/acme/acme/crypto_util.py index c88cab943..c47e88e73 100644 --- a/acme/acme/crypto_util.py +++ b/acme/acme/crypto_util.py @@ -18,17 +18,14 @@ from acme.magic_typing import Callable, Union, Tuple, Optional logger = logging.getLogger(__name__) -# TLSSNI01 certificate serving and probing is not affected by SSL -# vulnerabilities: prober needs to check certificate for expected -# contents anyway. Working SNI is the only thing that's necessary for -# the challenge and thus scoping down SSL/TLS method (version) would -# cause interoperability issues: TLSv1_METHOD is only compatible with +# Default SSL method selected here is the most compatible, while secure +# SSL method: TLSv1_METHOD is only compatible with # TLSv1_METHOD, while SSLv23_METHOD is compatible with all other # methods, including TLSv2_METHOD (read more at # https://www.openssl.org/docs/ssl/SSLv23_method.html). _serve_sni # should be changed to use "set_options" to disable SSLv2 and SSLv3, # in case it's used for things other than probing/serving! -_DEFAULT_TLSSNI01_SSL_METHOD = SSL.SSLv23_METHOD # type: ignore +_DEFAULT_SSL_METHOD = SSL.SSLv23_METHOD # type: ignore class SSLSocket(object): # pylint: disable=too-few-public-methods @@ -40,7 +37,7 @@ class SSLSocket(object): # pylint: disable=too-few-public-methods :ivar method: See `OpenSSL.SSL.Context` for allowed values. """ - def __init__(self, sock, certs, method=_DEFAULT_TLSSNI01_SSL_METHOD): + def __init__(self, sock, certs, method=_DEFAULT_SSL_METHOD): self.sock = sock self.certs = certs self.method = method @@ -112,7 +109,7 @@ class SSLSocket(object): # pylint: disable=too-few-public-methods def probe_sni(name, host, port=443, timeout=300, - method=_DEFAULT_TLSSNI01_SSL_METHOD, source_address=('', 0)): + method=_DEFAULT_SSL_METHOD, source_address=('', 0)): """Probe SNI server for SSL certificate. :param bytes name: Byte string to send as the server name in the diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 7c82c8507..d8684603c 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -3,7 +3,7 @@ import six import json try: from collections.abc import Hashable # pylint: disable=no-name-in-module -except ImportError: +except ImportError: # pragma: no cover from collections import Hashable import josepy as jose diff --git a/acme/acme/standalone.py b/acme/acme/standalone.py index ff9159933..c82967897 100644 --- a/acme/acme/standalone.py +++ b/acme/acme/standalone.py @@ -17,6 +17,7 @@ import OpenSSL from acme import challenges from acme import crypto_util from acme.magic_typing import List # pylint: disable=unused-import, no-name-in-module +from acme import _TLSSNI01DeprecationModule logger = logging.getLogger(__name__) @@ -37,7 +38,7 @@ class TLSServer(socketserver.TCPServer): self.certs = kwargs.pop("certs", {}) self.method = kwargs.pop( # pylint: disable=protected-access - "method", crypto_util._DEFAULT_TLSSNI01_SSL_METHOD) + "method", crypto_util._DEFAULT_SSL_METHOD) self.allow_reuse_address = kwargs.pop("allow_reuse_address", True) socketserver.TCPServer.__init__(self, *args, **kwargs) @@ -296,5 +297,9 @@ def simple_tls_sni_01_server(cli_args, forever=True): server.handle_request() +# Patching ourselves to warn about TLS-SNI challenge deprecation and removal. +sys.modules[__name__] = _TLSSNI01DeprecationModule(sys.modules[__name__]) + + if __name__ == "__main__": sys.exit(simple_tls_sni_01_server(sys.argv)) # pragma: no cover diff --git a/acme/acme/standalone_test.py b/acme/acme/standalone_test.py index ee527782a..953df40d4 100644 --- a/acme/acme/standalone_test.py +++ b/acme/acme/standalone_test.py @@ -1,13 +1,15 @@ """Tests for acme.standalone.""" +import multiprocessing import os import shutil import socket import threading import tempfile import unittest +import time +from contextlib import closing from six.moves import http_client # pylint: disable=import-error -from six.moves import queue # pylint: disable=import-error from six.moves import socketserver # type: ignore # pylint: disable=import-error import josepy as jose @@ -16,6 +18,7 @@ import requests from acme import challenges from acme import crypto_util +from acme import errors from acme import test_util from acme.magic_typing import Set # pylint: disable=unused-import, no-name-in-module @@ -248,7 +251,6 @@ class HTTP01DualNetworkedServersTest(unittest.TestCase): self.assertFalse(self._test_http01(add=False)) -@test_util.broken_on_windows class TestSimpleTLSSNI01Server(unittest.TestCase): """Tests for acme.standalone.simple_tls_sni_01_server.""" @@ -263,35 +265,41 @@ class TestSimpleTLSSNI01Server(unittest.TestCase): shutil.copy(test_util.vector_path('rsa2048_key.pem'), os.path.join(localhost_dir, 'key.pem')) + with closing(socket.socket(socket.AF_INET, socket.SOCK_STREAM)) as sock: + sock.bind(('', 0)) + sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) + self.port = sock.getsockname()[1] + from acme.standalone import simple_tls_sni_01_server - self.thread = threading.Thread( - target=simple_tls_sni_01_server, kwargs={ - 'cli_args': ('filename',), - 'forever': False, - }, - ) + self.process = multiprocessing.Process(target=simple_tls_sni_01_server, + args=(['path', '-p', str(self.port)],)) self.old_cwd = os.getcwd() os.chdir(self.test_cwd) def tearDown(self): os.chdir(self.old_cwd) - self.thread.join() + if self.process.is_alive(): + self.process.terminate() shutil.rmtree(self.test_cwd) - @mock.patch('acme.standalone.logger') - def test_it(self, mock_logger): - # Use a Queue because mock objects aren't thread safe. - q = queue.Queue() # type: queue.Queue[int] - # Add port number to the queue. - mock_logger.info.side_effect = lambda *args: q.put(args[-1]) - self.thread.start() + @mock.patch('acme.standalone.TLSSNI01Server.handle_request') + def test_mock(self, handle): + from acme.standalone import simple_tls_sni_01_server + simple_tls_sni_01_server(cli_args=['path', '-p', str(self.port)], forever=False) + self.assertEqual(handle.call_count, 1) - # After the timeout, an exception is raised if the queue is empty. - port = q.get(timeout=5) - cert = crypto_util.probe_sni(b'localhost', b'0.0.0.0', port) + def test_live(self): + self.process.start() + cert = None + for _ in range(50): + time.sleep(0.1) + try: + cert = crypto_util.probe_sni(b'localhost', b'127.0.0.1', self.port) + break + except errors.Error: # pragma: no cover + pass self.assertEqual(jose.ComparableX509(cert), - test_util.load_comparable_cert( - 'rsa2048_cert.pem')) + test_util.load_comparable_cert('rsa2048_cert.pem')) if __name__ == "__main__": diff --git a/acme/acme/test_util.py b/acme/acme/test_util.py index f97614700..1a0b67056 100644 --- a/acme/acme/test_util.py +++ b/acme/acme/test_util.py @@ -4,7 +4,6 @@ """ import os -import sys import pkg_resources import unittest @@ -95,11 +94,3 @@ def skip_unless(condition, reason): # pragma: no cover return lambda cls: cls else: return lambda cls: None - -def broken_on_windows(function): - """Decorator to skip temporarily a broken test on Windows.""" - reason = 'Test is broken and ignored on windows but should be fixed.' - return unittest.skipIf( - sys.platform == 'win32' - and os.environ.get('SKIP_BROKEN_TESTS_ON_WINDOWS', 'true') == 'true', - reason)(function) diff --git a/pytest.ini b/pytest.ini index 49db7da09..2531e50d2 100644 --- a/pytest.ini +++ b/pytest.ini @@ -13,6 +13,6 @@ filterwarnings = error ignore:decodestring:DeprecationWarning - ignore:TLS-SNI-01:DeprecationWarning + ignore:(TLSSNI01|TLS-SNI-01):DeprecationWarning ignore:.*collections\.abc:DeprecationWarning ignore:The `color_scheme` argument is deprecated:DeprecationWarning:IPython.*