Make ACMEv1 deprecation warnings scarier (#9015)

Fixes https://github.com/certbot/certbot/issues/6844.

This PR does two things:

1. Changes ACMEv1 deprecation warnings from `PendingDeprecationWarning` to `DeprecationWarning`.
2. Changes the ACMEv1 deprecation warnings to be on references to the class themselves. This is the approach taken in https://github.com/certbot/certbot/pull/8989, the PRs linked there, and the `cryptography` code in the code comment. I think this approach warns in more cases and I updated our unit tests to avoid hitting these warnings.

acme/acme/client.py

@@ -1,4 +1,7 @@
 """ACME client API."""
+# pylint: disable=too-many-lines
+# This pylint disable can be deleted once the deprecated ACMEv1 code is
+# removed.
 import base64
 import collections
 import datetime
@@ -7,7 +10,9 @@ import heapq
 import http.client as http_client
 import logging
 import re
+import sys
 import time
+from types import ModuleType
 from typing import cast
 from typing import Dict
 from typing import List
@@ -250,8 +255,6 @@ class Client(ClientBase):
             URI from which the resource will be downloaded.
 
         """
-        warnings.warn("acme.client.Client (ACMEv1) is deprecated, "
-                      "use acme.client.ClientV2 instead.", PendingDeprecationWarning)
         self.key = key
         if net is None:
             net = ClientNetwork(key, alg=alg, verify_ssl=verify_ssl)
@@ -834,8 +837,6 @@ class BackwardsCompatibleClientV2:
     """
 
     def __init__(self, net, key, server):
-        warnings.warn("acme.client.BackwardsCompatibleClientV2 is deprecated, use "
-                      "acme.client.ClientV2 instead.", PendingDeprecationWarning)
         directory = messages.Directory.from_json(net.get(server).json())
         self.acme_version = self._acme_version_from_directory(directory)
         self.client: Union[Client, ClientV2]
@@ -1239,3 +1240,35 @@ class ClientNetwork:
         response = self._check_response(response, content_type=content_type)
         self._add_nonce(response)
         return response
+
+
+# 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 _ClientDeprecationModule:
+    """
+    Internal class delegating to a module, and displaying warnings when attributes
+    related to deprecated attributes in the acme.client module.
+    """
+    def __init__(self, module):
+        self.__dict__['_module'] = module
+
+    def __getattr__(self, attr):
+        if attr in ('Client', 'BackwardsCompatibleClientV2'):
+            warnings.warn('The {0} attribute in acme.client 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)
+
+
+# Patching ourselves to warn about deprecation and planned removal of some elements in the module.
+sys.modules[__name__] = cast(ModuleType, _ClientDeprecationModule(sys.modules[__name__]))

certbot/certbot/_internal/client.py

@@ -39,8 +39,7 @@ def acme_from_config_key(config, key, regr=None):
                                     user_agent=determine_user_agent(config))
 
     with warnings.catch_warnings():
-        # TODO: full removal of ACMEv1 support: https://github.com/certbot/certbot/issues/6844
+        warnings.simplefilter("ignore", DeprecationWarning)
-        warnings.simplefilter("ignore", PendingDeprecationWarning)
 
         client = acme_client.BackwardsCompatibleClientV2(net, key, config.server)
         if client.acme_version == 1:

certbot/tests/auth_handler_test.py

@@ -79,9 +79,9 @@ class HandleAuthorizationsTest(unittest.TestCase):
         self.mock_auth.perform.side_effect = gen_auth_resp
 
         self.mock_account = mock.Mock(key=util.Key("file_path", "PEM"))
-        self.mock_net = mock.MagicMock(spec=acme_client.Client)
+        self.mock_net = mock.MagicMock(spec=acme_client.ClientV2)
         self.mock_net.acme_version = 1
-        self.mock_net.retry_after.side_effect = acme_client.Client.retry_after
+        self.mock_net.retry_after.side_effect = acme_client.ClientV2.retry_after
 
         self.handler = AuthHandler(
             self.mock_auth, self.mock_net, self.mock_account, [])
@@ -165,9 +165,6 @@ class HandleAuthorizationsTest(unittest.TestCase):
         self.assertEqual(len(authzr), 1)
 
     def _test_name3_http_01_3_common(self, combos):
-        self.mock_net.request_domain_challenges.side_effect = functools.partial(
-            gen_dom_authzr, challs=acme_util.CHALLENGES, combos=combos)
-
         authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES),
                    gen_dom_authzr(domain="1", challs=acme_util.CHALLENGES),
                    gen_dom_authzr(domain="2", challs=acme_util.CHALLENGES)]

certbot/tests/client_test.py

@@ -1,4 +1,5 @@
 """Tests for certbot._internal.client."""
+import contextlib
 import platform
 import shutil
 import tempfile
@@ -92,8 +93,17 @@ class RegisterTest(test_util.ConfigTestCase):
     def _false_mock():
         return False
 
+    @staticmethod
+    @contextlib.contextmanager
+    def _patched_acme_client():
+        # This function is written this way to avoid deprecation warnings that
+        # are raised when BackwardsCompatibleClientV2 is accessed on the real
+        # acme.client module.
+        with mock.patch('certbot._internal.client.acme_client') as mock_acme_client:
+            yield mock_acme_client.BackwardsCompatibleClientV2
+
     def test_no_tos(self):
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client.new_account_and_tos().terms_of_service = "http://tos"
             mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot._internal.eff.prepare_subscription") as mock_prepare:
@@ -107,7 +117,7 @@ class RegisterTest(test_util.ConfigTestCase):
 
     @test_util.patch_display_util()
     def test_it(self, unused_mock_get_utility):
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot._internal.eff.handle_subscription"):
                 self._call()
@@ -118,7 +128,7 @@ class RegisterTest(test_util.ConfigTestCase):
         self.config.noninteractive_mode = False
         msg = "DNS problem: NXDOMAIN looking up MX for example.com"
         mx_err = messages.Error.with_code('invalidContact', detail=msg)
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot._internal.eff.prepare_subscription") as mock_prepare:
                 mock_client().new_account_and_tos.side_effect = [mx_err, mock.MagicMock()]
@@ -131,7 +141,7 @@ class RegisterTest(test_util.ConfigTestCase):
         self.config.noninteractive_mode = True
         msg = "DNS problem: NXDOMAIN looking up MX for example.com"
         mx_err = messages.Error.with_code('invalidContact', detail=msg)
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot._internal.eff.handle_subscription"):
                 mock_client().new_account_and_tos.side_effect = [mx_err, mock.MagicMock()]
@@ -144,8 +154,8 @@ class RegisterTest(test_util.ConfigTestCase):
     @mock.patch("certbot._internal.client.logger")
     def test_without_email(self, mock_logger):
         with mock.patch("certbot._internal.eff.prepare_subscription") as mock_prepare:
-            with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_clnt:
+            with self._patched_acme_client() as mock_client:
-                mock_clnt().external_account_required.side_effect = self._false_mock
+                mock_client().external_account_required.side_effect = self._false_mock
                 self.config.email = None
                 self.config.register_unsafely_without_email = True
                 self.config.dry_run = False
@@ -156,7 +166,7 @@ class RegisterTest(test_util.ConfigTestCase):
     @mock.patch("certbot._internal.client.display_ops.get_email")
     def test_dry_run_no_staging_account(self, mock_get_email):
         """Tests dry-run for no staging account, expect account created with no email"""
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot._internal.eff.handle_subscription"):
                 self.config.dry_run = True
@@ -168,7 +178,7 @@ class RegisterTest(test_util.ConfigTestCase):
 
     @test_util.patch_display_util()
     def test_with_eab_arguments(self, unused_mock_get_utility):
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().client.directory.__getitem__ = mock.Mock(
                 side_effect=self._new_acct_dir_mock
             )
@@ -184,7 +194,7 @@ class RegisterTest(test_util.ConfigTestCase):
 
     @test_util.patch_display_util()
     def test_without_eab_arguments(self, unused_mock_get_utility):
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot._internal.eff.handle_subscription"):
                 target = "certbot._internal.client.messages.ExternalAccountBinding.from_data"
@@ -196,7 +206,7 @@ class RegisterTest(test_util.ConfigTestCase):
                     self.assertIs(mock_eab_from_data.called, False)
 
     def test_external_account_required_without_eab_arguments(self):
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().client.net.key.public_key = mock.Mock(side_effect=self._public_key_mock)
             mock_client().external_account_required.side_effect = self._true_mock
             with mock.patch("certbot._internal.eff.handle_subscription"):
@@ -210,7 +220,7 @@ class RegisterTest(test_util.ConfigTestCase):
         from acme import messages
         msg = "Test"
         mx_err = messages.Error.with_code("malformed", detail=msg, title="title")
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+        with self._patched_acme_client() as mock_client:
             mock_client().client.directory.__getitem__ = mock.Mock(
                 side_effect=self._new_acct_dir_mock
             )
@@ -232,9 +242,10 @@ class ClientTestCommon(test_util.ConfigTestCase):
         self.account = mock.MagicMock(**{"key.pem": KEY})
 
         from certbot._internal.client import Client
-        with mock.patch("certbot._internal.client.acme_client.BackwardsCompatibleClientV2") as acme:
+        with mock.patch("certbot._internal.client.acme_client") as acme:
-            self.acme_client = acme
+            self.acme_client = acme.BackwardsCompatibleClientV2
-            self.acme = acme.return_value = mock.MagicMock()
+            self.acme = self.acme_client.return_value = mock.MagicMock()
+            self.client_network = acme.ClientNetwork
             self.client = Client(
                 config=self.config, account_=self.account,
                 auth=None, installer=None)
@@ -255,8 +266,7 @@ class ClientTest(ClientTestCommon):
             csr_pem=mock.sentinel.csr_pem)
 
     def test_init_acme_verify_ssl(self):
-        net = self.acme_client.call_args[0][0]
+        self.assertIs(self.client_network.call_args[1]['verify_ssl'], True)
-        self.assertIs(net.verify_ssl, True)
 
     def _mock_obtain_certificate(self):
         self.client.auth_handler = mock.MagicMock()

certbot/tests/main_test.py

@@ -323,12 +323,12 @@ class RevokeTest(test_util.TempDirTestCase):
         self.tmp_cert_path = os.path.abspath(os.path.join(self.tempdir, 'cert_512.pem'))
 
         patches = [
-            mock.patch('acme.client.BackwardsCompatibleClientV2'),
+            mock.patch('certbot._internal.client.acme_client'),
             mock.patch('certbot._internal.client.Client'),
             mock.patch('certbot._internal.main._determine_account'),
             mock.patch('certbot._internal.main.display_ops.success_revocation')
         ]
-        self.mock_acme_client = patches[0].start()
+        self.mock_acme_client = patches[0].start().BackwardsCompatibleClientV2
         patches[1].start()
         self.mock_determine_account = patches[2].start()
         self.mock_success_revoke = patches[3].start()
@@ -708,11 +708,10 @@ class MainTest(test_util.ConfigTestCase):
     @mock.patch('certbot._internal.eff.handle_subscription')
     @mock.patch('certbot._internal.log.post_arg_parse_setup')
     @mock.patch('certbot._internal.main._report_new_cert')
-    @mock.patch('certbot._internal.main.client.acme_client.Client')
     @mock.patch('certbot._internal.main._determine_account')
     @mock.patch('certbot._internal.main.client.Client.obtain_and_enroll_certificate')
     @mock.patch('certbot._internal.main._get_and_save_cert')
-    def test_user_agent(self, gsc, _obt, det, _client, _, __, ___):
+    def test_user_agent(self, gsc, _obt, det, _, __, ___):
         # Normally the client is totally mocked out, but here we need more
         # arguments to automate it...
         args = ["--standalone", "certonly", "-m", "none@none.com",
@@ -720,7 +719,8 @@ class MainTest(test_util.ConfigTestCase):
         det.return_value = mock.MagicMock(), None
         gsc.return_value = mock.MagicMock()
 
-        with mock.patch('certbot._internal.main.client.acme_client.ClientNetwork') as acme_net:
+        with mock.patch('certbot._internal.main.client.acme_client') as acme_client:
+            acme_net = acme_client.ClientNetwork
             self._call_no_clientmock(args)
             os_ver = util.get_os_info_ua()
             ua = acme_net.call_args[1]["user_agent"]
@@ -730,7 +730,8 @@ class MainTest(test_util.ConfigTestCase):
             if "linux" in plat.lower():
                 self.assertIn(util.get_os_info_ua(), ua)
 
-        with mock.patch('certbot._internal.main.client.acme_client.ClientNetwork') as acme_net:
+        with mock.patch('certbot._internal.main.client.acme_client') as acme_client:
+            acme_net = acme_client.ClientNetwork
             ua = "bandersnatch"
             args += ["--user-agent", ua]
             self._call_no_clientmock(args)