Deprecate all tls-sni related objects in acme module (#6859)

This PR is a part of the tls-sni-01 removal plan described in #6849.

As `acme` is a library, we need to put some efforts to make a decent deprecation path before totally removing tls-sni in it. While initialization of `acme.challenges.TLSSNI01` was already creating deprecation warning, not all cases were covered.

For instance, and innocent call like this ...
```python
if not isinstance(challenge, acme.challenges.TLSSNI01):
    print('I am not using this TLS-SNI deprecated stuff, what could possibly go wrong?')
```
... would break if we suddenly remove all objects related to this challenge.

So, I use the _Deprecator Warning Machine, Let's Pacify this Technical Debt_ (Guido ®), to make `acme.challenges` and `acme.standalone` patch themselves, and display a deprecation warning on stderr for any access to the tls-sni challenge objects.

No dev should be able to avoid the deprecation warning. I set the deprecation warning in the idea to remove the code on `0.34.0`, but the exact deprecation window is open to discussion of course.

* Modules challenges and standalone patch themselves to generated deprecation warning when tls-sni related objects are accessed.

* Correct unit tests

* Correct lint

* Update challenges_test.py

* Correct lint

* Fix an error during tests

* Update coverage

* Use multiprocessing for coverage

* Add coverage

* Update test_util.py

* Factor the logic about global deprecation warning when accessing TLS-SNI-01 attributes

* Fix coverage

* Add comment for cryptography example.

* Use warnings.

* Add a changelog

* Fix deprecation during tests

* Reload

* Update acme/acme/__init__.py

Co-Authored-By: adferrand <adferrand@users.noreply.github.com>

* Update CHANGELOG.md

* Pick a random free port.

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,

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)

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__])

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
 

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

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

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

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__":

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)

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