acme: further deprecations (#9395)

* acme: deprecate acme.fields.Resource and .resource

* acme: deprecate .messages.OLD_ERROR_PREFIX

* acme: deprecate .messages.Directory.register

* acme: clean up deprecations

* dont use unscoped filterwarnings

* change deprecation approach for acme.fields

* warn on non-string keys in acme.messages.Directory

* remove leaked filterwarnings in BackwardsCompatibleClientV2Test

* remove non-string lookups of acme.messages.Directory

8 files changed, 211 insertions, 93 deletions

diff --git a/acme/acme/challenges.py b/acme/acme/challenges.py
index 534a60aad..194f19e47 100644
--- a/acme/acme/challenges.py
+++ b/acme/acme/challenges.py
@@ -56,7 +56,9 @@ class ChallengeResponse(ResourceMixin, TypeMixin, jose.TypedJSONObjectWithFields
     """ACME challenge response."""
     TYPES: Dict[str, Type['ChallengeResponse']] = {}
     resource_type = 'challenge'
-    resource: str = fields.resource(resource_type)
+    with warnings.catch_warnings():
+        warnings.filterwarnings('ignore', 'resource attribute in acme.fields', DeprecationWarning)
+        resource: str = fields.resource(resource_type)
 
 
 class UnrecognizedChallenge(Challenge):
diff --git a/acme/acme/client.py b/acme/acme/client.py
index be4c2e457..7d21b0fad 100644
--- a/acme/acme/client.py
+++ b/acme/acme/client.py
@@ -306,7 +306,7 @@ class Client(ClientBase):
 
         """
         new_reg = messages.NewRegistration() if new_reg is None else new_reg
-        response = self._post(self.directory[new_reg], new_reg)
+        response = self._post(self.directory['new-reg'], new_reg)
         # TODO: handle errors
         assert response.status_code == http_client.CREATED
 
@@ -612,7 +612,7 @@ class Client(ClientBase):
         :raises .ClientError: If revocation is unsuccessful.
 
         """
-        self._revoke(cert, rsn, self.directory[messages.Revocation])
+        self._revoke(cert, rsn, self.directory['revoke-cert'])
 
 
 class ClientV2(ClientBase):
diff --git a/acme/acme/fields.py b/acme/acme/fields.py
index 191231df2..8a1dc8462 100644
--- a/acme/acme/fields.py
+++ b/acme/acme/fields.py
@@ -1,8 +1,12 @@
 """ACME JSON fields."""
 import datetime
-from typing import Any
-
 import logging
+import sys
+from types import ModuleType
+from typing import Any
+from typing import cast
+from typing import List
+import warnings
 
 import josepy as jose
 import pyrfc3339
@@ -52,7 +56,11 @@ class RFC3339Field(jose.Field):
 
 
 class Resource(jose.Field):
-    """Resource MITM field."""
+    """Resource MITM field.
+
+    .. deprecated: 1.30.0
+
+    """
 
     def __init__(self, resource_type: str, *args: Any, **kwargs: Any) -> None:
         self.resource_type = resource_type
@@ -78,5 +86,40 @@ def rfc3339(json_name: str, omitempty: bool = False) -> Any:
 
 
 def resource(resource_type: str) -> Any:
-    """Generates a type-friendly Resource field."""
+    """Generates a type-friendly Resource field.
+
+    .. deprecated: 1.30.0
+
+    """
     return Resource(resource_type)
+
+
+# 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 _FieldsDeprecationModule: # pragma: no cover
+    """
+    Internal class delegating to a module, and displaying warnings when
+    module attributes deprecated in acme.fields are accessed.
+    """
+    def __init__(self, module: ModuleType) -> None:
+        self.__dict__['_module'] = module
+
+    def __getattr__(self, attr: str) -> None:
+        if attr in ('Resource', 'resource'):
+            warnings.warn('{0} attribute in acme.fields module is deprecated '
+                          'and will be removed soon.'.format(attr),
+                          DeprecationWarning, stacklevel=2)
+        return getattr(self._module, attr)
+
+    def __setattr__(self, attr: str, value: Any) -> None:
+        setattr(self._module, attr, value)
+
+    def __delattr__(self, attr: str) -> None:
+        delattr(self._module, attr)
+
+    def __dir__(self) -> List[str]:
+        return ['_module'] + dir(self._module)
+
+
+sys.modules[__name__] = cast(ModuleType, _FieldsDeprecationModule(sys.modules[__name__]))
diff --git a/acme/acme/messages.py b/acme/acme/messages.py
index fbb7738d0..cdefcfa4f 100644
--- a/acme/acme/messages.py
+++ b/acme/acme/messages.py
@@ -2,7 +2,9 @@
 import datetime
 from collections.abc import Hashable
 import json
+from types import ModuleType
 from typing import Any
+from typing import cast
 from typing import Dict
 from typing import Iterator
 from typing import List
@@ -14,6 +16,7 @@ from typing import Type
 from typing import TYPE_CHECKING
 from typing import TypeVar
 from typing import Union
+import sys
 import warnings
 
 import josepy as jose
@@ -24,7 +27,7 @@ from acme import fields
 from acme import jws
 from acme import util
 with warnings.catch_warnings():
-    warnings.filterwarnings("ignore", category=DeprecationWarning)
+    warnings.filterwarnings("ignore", ".*acme.mixins", category=DeprecationWarning)
     from acme.mixins import ResourceMixin
 
 if TYPE_CHECKING:
@@ -277,6 +280,10 @@ class Directory(jose.JSONDeSerializable):
     def register(cls,
                  resource_body_cls: Type[GenericHasResourceType]) -> Type[GenericHasResourceType]:
         """Register resource."""
+        warnings.warn(
+            "acme.messages.Directory.register is deprecated and will be removed in the next "
+            "major release of Certbot", DeprecationWarning, stacklevel=2
+        )
         resource_type = resource_body_cls.resource_type
         assert resource_type not in cls._REGISTERED_TYPES
         cls._REGISTERED_TYPES[resource_type] = resource_body_cls
@@ -295,6 +302,12 @@ class Directory(jose.JSONDeSerializable):
             raise AttributeError(str(error))
 
     def __getitem__(self, name: Union[str, HasResourceType, Type[HasResourceType]]) -> Any:
+        if not isinstance(name, str):
+            warnings.warn(
+                "Looking up acme.messages.Directory resources by non-string keys is deprecated "
+                "and will be removed in the next major release of Certbot",
+                DeprecationWarning, stacklevel=2
+            )
         try:
             return self._jobj[self._canon_key(name)]
         except KeyError:
@@ -462,19 +475,6 @@ class Registration(ResourceBody):
         return self._filter_contact(self.email_prefix)
 
 
-@Directory.register
-class NewRegistration(ResourceMixin, Registration):
-    """New registration."""
-    resource_type = 'new-reg'
-    resource: str = fields.resource(resource_type)
-
-
-class UpdateRegistration(ResourceMixin, Registration):
-    """Update registration."""
-    resource_type = 'reg'
-    resource: str = fields.resource(resource_type)
-
-
 class RegistrationResource(ResourceWithURI):
     """Registration Resource.
 
@@ -616,14 +616,14 @@ class Authorization(ResourceBody):
         """
         warnings.warn(
             "acme.messages.Authorization.combinations is deprecated and will be "
-            "removed in a future release.", DeprecationWarning)
+            "removed in a future release.", DeprecationWarning, stacklevel=2)
         return self._combinations
 
     @combinations.setter
     def combinations(self, combos: Tuple[Tuple[int, ...], ...]) -> None: # pragma: no cover
         warnings.warn(
             "acme.messages.Authorization.combinations is deprecated and will be "
-            "removed in a future release.", DeprecationWarning)
+            "removed in a future release.", DeprecationWarning, stacklevel=2)
         self._combinations = combos
 
     @property
@@ -635,22 +635,11 @@ class Authorization(ResourceBody):
         """
         warnings.warn(
             "acme.messages.Authorization.resolved_combinations is deprecated and will be "
-            "removed in a future release.", DeprecationWarning)
-        return tuple(tuple(self.challenges[idx] for idx in combo)
-                     for combo in self.combinations)  # pylint: disable=not-an-iterable
-
-
-@Directory.register
-class NewAuthorization(ResourceMixin, Authorization):
-    """New authorization."""
-    resource_type = 'new-authz'
-    resource: str = fields.resource(resource_type)
-
-
-class UpdateAuthorization(ResourceMixin, Authorization):
-    """Update authorization."""
-    resource_type = 'authz'
-    resource: str = fields.resource(resource_type)
+            "removed in a future release.", DeprecationWarning, stacklevel=2)
+        with warnings.catch_warnings():
+            warnings.filterwarnings('ignore', '.*combinations', DeprecationWarning)
+            return tuple(tuple(self.challenges[idx] for idx in combo)
+                        for combo in self.combinations)  # pylint: disable=not-an-iterable
 
 
 class AuthorizationResource(ResourceWithURI):
@@ -664,19 +653,6 @@ class AuthorizationResource(ResourceWithURI):
     new_cert_uri: str = jose.field('new_cert_uri', omitempty=True)
 
 
-@Directory.register
-class CertificateRequest(ResourceMixin, jose.JSONObjectWithFields):
-    """ACME new-cert request.
-
-    :ivar jose.ComparableX509 csr:
-        `OpenSSL.crypto.X509Req` wrapped in `.ComparableX509`
-
-    """
-    resource_type = 'new-cert'
-    resource: str = fields.resource(resource_type)
-    csr: jose.ComparableX509 = jose.field('csr', decoder=jose.decode_csr, encoder=jose.encode_csr)
-
-
 class CertificateResource(ResourceWithURI):
     """Certificate Resource.
 
@@ -690,21 +666,6 @@ class CertificateResource(ResourceWithURI):
     authzrs: Tuple[AuthorizationResource, ...] = jose.field('authzrs')
 
 
-@Directory.register
-class Revocation(ResourceMixin, jose.JSONObjectWithFields):
-    """Revocation message.
-
-    :ivar jose.ComparableX509 certificate: `OpenSSL.crypto.X509` wrapped in
-        `jose.ComparableX509`
-
-    """
-    resource_type = 'revoke-cert'
-    resource: str = fields.resource(resource_type)
-    certificate: jose.ComparableX509 = jose.field(
-        'certificate', decoder=jose.decode_cert, encoder=jose.encode_cert)
-    reason: int = jose.field('reason')
-
-
 class Order(ResourceBody):
     """Order Resource Body.
 
@@ -756,7 +717,98 @@ class OrderResource(ResourceWithURI):
                                                        omitempty=True)
 
 
-@Directory.register
-class NewOrder(Order):
-    """New order."""
-    resource_type = 'new-order'
+with warnings.catch_warnings():
+    warnings.filterwarnings("ignore", "acme.messages.Directory.register", DeprecationWarning)
+    warnings.filterwarnings("ignore", "resource attribute in acme.fields", DeprecationWarning)
+
+    @Directory.register
+    class NewOrder(Order):
+        """New order."""
+        resource_type = 'new-order'
+
+
+    @Directory.register
+    class Revocation(ResourceMixin, jose.JSONObjectWithFields):
+        """Revocation message.
+
+        :ivar jose.ComparableX509 certificate: `OpenSSL.crypto.X509` wrapped in
+            `jose.ComparableX509`
+
+        """
+        resource_type = 'revoke-cert'
+        resource: str = fields.resource(resource_type)
+        certificate: jose.ComparableX509 = jose.field(
+            'certificate', decoder=jose.decode_cert, encoder=jose.encode_cert)
+        reason: int = jose.field('reason')
+
+
+    @Directory.register
+    class CertificateRequest(ResourceMixin, jose.JSONObjectWithFields):
+        """ACME new-cert request.
+
+        :ivar jose.ComparableX509 csr:
+            `OpenSSL.crypto.X509Req` wrapped in `.ComparableX509`
+
+        """
+        resource_type = 'new-cert'
+        resource: str = fields.resource(resource_type)
+        csr: jose.ComparableX509 = jose.field('csr', decoder=jose.decode_csr,
+                                                encoder=jose.encode_csr)
+
+
+    @Directory.register
+    class NewAuthorization(ResourceMixin, Authorization):
+        """New authorization."""
+        resource_type = 'new-authz'
+        resource: str = fields.resource(resource_type)
+
+
+    class UpdateAuthorization(ResourceMixin, Authorization):
+        """Update authorization."""
+        resource_type = 'authz'
+        resource: str = fields.resource(resource_type)
+
+
+    @Directory.register
+    class NewRegistration(ResourceMixin, Registration):
+        """New registration."""
+        resource_type = 'new-reg'
+        resource: str = fields.resource(resource_type)
+
+
+    class UpdateRegistration(ResourceMixin, Registration):
+        """Update registration."""
+        resource_type = 'reg'
+        resource: str = fields.resource(resource_type)
+
+
+# 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 _MessagesDeprecationModule: # pragma: no cover
+    """
+    Internal class delegating to a module, and displaying warnings when
+    module attributes deprecated in acme.messages are accessed.
+    """
+    def __init__(self, module: ModuleType) -> None:
+        self.__dict__['_module'] = module
+
+    def __getattr__(self, attr: str) -> None:
+        if attr == 'OLD_ERROR_PREFIX':
+            warnings.warn('{0} attribute in acme.messages module is deprecated '
+                          'and will be removed soon.'.format(attr),
+                          DeprecationWarning, stacklevel=2)
+        return getattr(self._module, attr)
+
+    def __setattr__(self, attr: str, value: Any) -> None:
+        setattr(self._module, attr, value)
+
+    def __delattr__(self, attr: str) -> None:
+        delattr(self._module, attr)
+
+    def __dir__(self) -> List[str]:
+        return ['_module'] + dir(self._module)
+
+
+# Patching ourselves to warn about acme.messages.OLD_ERROR_PREFIX deprecation and planned removal.
+sys.modules[__name__] = cast(ModuleType, _MessagesDeprecationModule(sys.modules[__name__]))
diff --git a/acme/tests/client_test.py b/acme/tests/client_test.py
index 1d9aa27fe..e717f734a 100644
--- a/acme/tests/client_test.py
+++ b/acme/tests/client_test.py
@@ -3,11 +3,11 @@
 import copy
 import datetime
 import http.client as http_client
-import ipaddress
 import json
 import unittest
 from typing import Dict
 from unittest import mock
+import warnings
 
 import josepy as jose
 import OpenSSL
@@ -17,9 +17,17 @@ from acme import challenges
 from acme import errors
 from acme import jws as acme_jws
 from acme import messages
+from acme.client import ClientNetwork
+from acme.client import ClientV2
 from acme.mixins import VersionedLEACMEMixin
 import messages_test
 import test_util
+# Remove the following in Certbot 2.0:
+with warnings.catch_warnings():
+    warnings.filterwarnings('ignore', '.* in acme.client', DeprecationWarning)
+    from acme.client import BackwardsCompatibleClientV2
+    from acme.client import Client
+
 
 CERT_DER = test_util.load_vector('cert.der')
 CERT_SAN_PEM = test_util.load_vector('cert-san.pem')
@@ -87,12 +95,17 @@ class ClientTestBase(unittest.TestCase):
         # Reason code for revocation
         self.rsn = 1
 
-
 class BackwardsCompatibleClientV2Test(ClientTestBase):
     """Tests for  acme.client.BackwardsCompatibleClientV2."""
 
     def setUp(self):
         super().setUp()
+
+        # For some reason, required to suppress warnings on mock.patch('acme.client.Client')
+        self.warning_cap = warnings.catch_warnings()
+        self.warning_cap.__enter__()
+        warnings.filterwarnings('ignore', '.*acme.client', DeprecationWarning)
+
         # contains a loaded cert
         self.certr = messages.CertificateResource(
             body=messages_test.CERT)
@@ -114,15 +127,17 @@ class BackwardsCompatibleClientV2Test(ClientTestBase):
         self.orderr = messages.OrderResource(
             csr_pem=CSR_SAN_PEM)
 
+    def tearDown(self) -> None:
+        self.warning_cap.__exit__()
+        return super().tearDown()
+
     def _init(self):
         uri = 'http://www.letsencrypt-demo.org/directory'
-        from acme.client import BackwardsCompatibleClientV2
         return BackwardsCompatibleClientV2(net=self.net,
             key=KEY, server=uri)
 
     def test_init_downloads_directory(self):
         uri = 'http://www.letsencrypt-demo.org/directory'
-        from acme.client import BackwardsCompatibleClientV2
         BackwardsCompatibleClientV2(net=self.net,
             key=KEY, server=uri)
         self.net.get.assert_called_once_with(uri)
@@ -336,13 +351,11 @@ class ClientTest(ClientTestBase):
             uri='https://www.letsencrypt-demo.org/acme/cert/1',
             cert_chain_uri='https://www.letsencrypt-demo.org/ca')
 
-        from acme.client import Client
         self.client = Client(
             directory=self.directory, key=KEY, alg=jose.RS256, net=self.net)
 
     def test_init_downloads_directory(self):
         uri = 'http://www.letsencrypt-demo.org/directory'
-        from acme.client import Client
         self.client = Client(
             directory=uri, key=KEY, alg=jose.RS256, net=self.net)
         self.net.get.assert_called_once_with(uri)
@@ -351,7 +364,6 @@ class ClientTest(ClientTestBase):
     def test_init_without_net(self, mock_net):
         mock_net.return_value = mock.sentinel.net
         alg = jose.RS256
-        from acme.client import Client
         self.client = Client(
             directory=self.directory, key=KEY, alg=alg)
         mock_net.called_once_with(KEY, alg=alg, verify_ssl=True)
@@ -723,7 +735,6 @@ class ClientV2Test(ClientTestBase):
 
         self.directory = DIRECTORY_V2
 
-        from acme.client import ClientV2
         self.client = ClientV2(self.directory, self.net)
 
         self.new_reg = self.new_reg.update(terms_of_service_agreed=True)
@@ -948,7 +959,6 @@ class ClientNetworkTest(unittest.TestCase):
         self.verify_ssl = mock.MagicMock()
         self.wrap_in_jws = mock.MagicMock(return_value=mock.sentinel.wrapped)
 
-        from acme.client import ClientNetwork
         self.net = ClientNetwork(
             key=KEY, alg=jose.RS256, verify_ssl=self.verify_ssl,
             user_agent='acme-python-test')
@@ -1179,7 +1189,6 @@ class ClientNetworkWithMockedResponseTest(unittest.TestCase):
     """Tests for acme.client.ClientNetwork which mock out response."""
 
     def setUp(self):
-        from acme.client import ClientNetwork
         self.net = ClientNetwork(key=None, alg=None)
 
         self.response = mock.MagicMock(ok=True, status_code=http_client.OK)
@@ -1342,7 +1351,6 @@ class ClientNetworkSourceAddressBindingTest(unittest.TestCase):
         self.source_address = "8.8.8.8"
 
     def test_source_address_set(self):
-        from acme.client import ClientNetwork
         with mock.patch('warnings.warn') as mock_warn:
             net = ClientNetwork(key=None, alg=None, source_address=self.source_address)
             mock_warn.assert_called_once()
@@ -1353,7 +1361,6 @@ class ClientNetworkSourceAddressBindingTest(unittest.TestCase):
     def test_behavior_assumption(self):
         """This is a test that guardrails the HTTPAdapter behavior so that if the default for
         a Session() changes, the assumptions here aren't violated silently."""
-        from acme.client import ClientNetwork
         # Source address not specified, so the default adapter type should be bound -- this
         # test should fail if the default adapter type is changed by requests
         net = ClientNetwork(key=None, alg=None)
diff --git a/acme/tests/fields_test.py b/acme/tests/fields_test.py
index 4cc167f9c..76b215342 100644
--- a/acme/tests/fields_test.py
+++ b/acme/tests/fields_test.py
@@ -1,6 +1,7 @@
 """Tests for acme.fields."""
 import datetime
 import unittest
+import warnings
 
 import josepy as jose
 import pytz
@@ -58,8 +59,10 @@ class ResourceTest(unittest.TestCase):
     """Tests for acme.fields.Resource."""
 
     def setUp(self):
-        from acme.fields import Resource
-        self.field = Resource('x')
+        with warnings.catch_warnings():
+            warnings.filterwarnings('ignore', '.*Resource', DeprecationWarning)
+            from acme.fields import Resource
+            self.field = Resource('x')
 
     def test_decode_good(self):
         self.assertEqual('x', self.field.decode('x'))
diff --git a/acme/tests/messages_test.py b/acme/tests/messages_test.py
index cf7e7629a..782955fb4 100644
--- a/acme/tests/messages_test.py
+++ b/acme/tests/messages_test.py
@@ -2,6 +2,7 @@
 from typing import Dict
 import unittest
 from unittest import mock
+import warnings
 
 import josepy as jose
 
@@ -150,8 +151,10 @@ class DirectoryTest(unittest.TestCase):
     def test_getitem(self):
         self.assertEqual('reg', self.dir['new-reg'])
         from acme.messages import NewRegistration
-        self.assertEqual('reg', self.dir[NewRegistration])
-        self.assertEqual('reg', self.dir[NewRegistration()])
+        with warnings.catch_warnings():
+            warnings.filterwarnings('ignore', '.* non-string keys', DeprecationWarning)
+            self.assertEqual('reg', self.dir[NewRegistration])
+            self.assertEqual('reg', self.dir[NewRegistration()])
 
     def test_getitem_fails_with_key_error(self):
         self.assertRaises(KeyError, self.dir.__getitem__, 'foo')
@@ -407,10 +410,12 @@ class AuthorizationTest(unittest.TestCase):
         hash(Authorization.from_json(self.jobj_from))
 
     def test_resolved_combinations(self):
-        self.assertEqual(self.authz.resolved_combinations, (
-            (self.challbs[0],),
-            (self.challbs[1],),
-        ))
+        with warnings.catch_warnings():
+            warnings.filterwarnings('ignore', '.*resolved_combinations', DeprecationWarning)
+            self.assertEqual(self.authz.resolved_combinations, (
+                (self.challbs[0],),
+                (self.challbs[1],),
+            ))
 
 
 class AuthorizationResourceTest(unittest.TestCase):
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 04b25e98d..22c2ffdba 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -11,8 +11,14 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 ### Changed
 
 * `acme.client.ClientBase`, `acme.messages.Authorization.resolved_combinations`,
-  `acme.messages.Authorization.combinations` and `acme.mixins` are deprecated and
-  will be removed in a future release.
+  `acme.messages.Authorization.combinations`, `acme.mixins`, `acme.fields.resource`,
+  and `acme.fields.Resource` are deprecated and will be removed in a future release.
+* `acme.messages.OLD_ERROR_PREFIX` (`urn:acme:error:`) is deprecated and support for
+  the old ACME error prefix in Certbot will be removed in the next major release of
+  Certbot.
+* `acme.messages.Directory.register` is deprecated and will be removed in the next
+  major release of Certbot. Furthermore, `.Directory` will only support lookups
+  by the exact resource name string in the ACME directory  (e.g. `directory['newOrder']`).
 * The `certbot-dns-cloudxns` plugin is now deprecated and will be removed in the
   next major release of Certbot.
 * The `source_address` argument for `acme.client.ClientNetwork` is deprecated