Repository: certbot/certbot

Issue:
Conditionally depend on sphinx imgconverter?
This is needed in Sphinx>=1.6, but Sphinx this new may not be available in older distros. Can we configure Sphinx to work with both versions by conditionally depending on imgconverter? Is there another approach we can take entirely?

The relevant PR here is https://github.com/certbot/certbot/pull/6070/files.

Discussion:
sydneyli: @bmw, I've been having trouble getting `make -c docs html` to work with versions of Sphinx before 1.5, even before #6070 was landed. I think it's due to the version of `docutils` being pinned in `tools/dev_constraints.txt`, but I'll have to investigate some more. What versions of Sphinx are you seeing in older distros?
sydneyli: > Let me know when this PR is ready for review or if it already is!

It's ready as-is, as I don't think it would cause any regressions for older versions of Sphinx.
sydneyli: Actually, would you prefer that I lump in the fix for Sphinx < 1.5 in this PR, or create a new one for it? [This](https://github.com/sphinx-doc/sphinx/issues/3212) is one of the relevant bugs.
sydneyli: Ok, so [annotations](https://het.as.utexas.edu/HET/Software/Sphinx/ext/autodoc.html), which are used [here](https://github.com/certbot/certbot/blob/master/docs/api/constants.rst) aren't supported until Sphinx 1.2, so that's the oldest version of Sphinx we can depend on.
sydneyli: Ready for re-review.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4b10a5805..6d9447ae2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@ Certbot adheres to [Semantic Versioning](http://semver.org/).
 * Noninteractive renewals with `certbot renew` (those not started from a
   terminal) now randomly sleep 1-480 seconds before beginning work in
   order to spread out load spikes on the server side.
+* Added External Account Binding support in cli and acme library.
+  Command line arguments --eab-kid and --eab-hmac-key added.
 
 ### Changed
 
@@ -19,6 +21,7 @@ Certbot adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 
 * Update code and dependencies to clean up Resource and Deprecation Warnings.
+* Only depend on imgconverter extension for Sphinx >= 1.6
 
 Despite us having broken lockstep, we are continuing to release new versions of
 all Certbot components during releases for the time being, however, the only
diff --git a/acme/acme/client.py b/acme/acme/client.py
index 88938e999..41338e17e 100644
--- a/acme/acme/client.py
+++ b/acme/acme/client.py
@@ -757,6 +757,13 @@ class ClientV2(ClientBase):
         """
         return self._revoke(cert, rsn, self.directory['revokeCert'])
 
+    def external_account_required(self):
+        """Checks if ACME server requires External Account Binding authentication."""
+        if hasattr(self.directory, 'meta') and self.directory.meta.external_account_required:
+            return True
+        else:
+            return False
+
     def _post_as_get(self, *args, **kwargs):
         """
         Send GET request using the POST-as-GET protocol if needed.
@@ -919,6 +926,15 @@ class BackwardsCompatibleClientV2(object):
         else:
             return 1
 
+    def external_account_required(self):
+        """Checks if the server requires an external account for ACMEv2 servers.
+
+        Always return False for ACMEv1 servers, as it doesn't use External Account Binding."""
+        if self.acme_version == 1:
+            return False
+        else:
+            return self.client.external_account_required()
+
 
 class ClientNetwork(object):  # pylint: disable=too-many-instance-attributes
     """Wrapper around requests that signs POSTs for authentication.
diff --git a/acme/acme/messages.py b/acme/acme/messages.py
index 7e86b0c3b..4400a6c31 100644
--- a/acme/acme/messages.py
+++ b/acme/acme/messages.py
@@ -1,6 +1,7 @@
 """ACME protocol messages."""
 import collections
 import six
+import json
 
 import josepy as jose
 
@@ -8,6 +9,7 @@ from acme import challenges
 from acme import errors
 from acme import fields
 from acme import util
+from acme import jws
 
 OLD_ERROR_PREFIX = "urn:acme:error:"
 ERROR_PREFIX = "urn:ietf:params:acme:error:"
@@ -27,6 +29,7 @@ ERROR_CODES = {
     'tls': 'The server experienced a TLS error during domain verification',
     'unauthorized': 'The client lacks sufficient authorization',
     'unknownHost': 'The server could not resolve a domain name',
+    'externalAccountRequired': 'The server requires external account binding',
 }
 
 ERROR_TYPE_DESCRIPTIONS = dict(
@@ -176,6 +179,7 @@ class Directory(jose.JSONDeSerializable):
         _terms_of_service_v2 = jose.Field('termsOfService', omitempty=True)
         website = jose.Field('website', omitempty=True)
         caa_identities = jose.Field('caaIdentities', omitempty=True)
+        external_account_required = jose.Field('externalAccountRequired', omitempty=True)
 
         def __init__(self, **kwargs):
             kwargs = dict((self._internal_name(k), v) for k, v in kwargs.items())
@@ -258,6 +262,24 @@ class ResourceBody(jose.JSONObjectWithFields):
     """ACME Resource Body."""
 
 
+class ExternalAccountBinding(object):
+    """ACME External Account Binding"""
+
+    @classmethod
+    def from_data(cls, account_public_key, kid, hmac_key, directory):
+        """Create External Account Binding Resource from contact details, kid and hmac."""
+
+        key_json = json.dumps(account_public_key.to_partial_json()).encode()
+        decoded_hmac_key = jose.b64.b64decode(hmac_key)
+        url = directory["newAccount"]
+
+        eab = jws.JWS.sign(key_json, jose.jwk.JWKOct(key=decoded_hmac_key),
+                           jose.jwa.HS256, None,
+                           url, kid)
+
+        return eab.to_partial_json()
+
+
 class Registration(ResourceBody):
     """Registration Resource Body.
 
@@ -275,12 +297,13 @@ class Registration(ResourceBody):
     status = jose.Field('status', omitempty=True)
     terms_of_service_agreed = jose.Field('termsOfServiceAgreed', omitempty=True)
     only_return_existing = jose.Field('onlyReturnExisting', omitempty=True)
+    external_account_binding = jose.Field('externalAccountBinding', omitempty=True)
 
     phone_prefix = 'tel:'
     email_prefix = 'mailto:'
 
     @classmethod
-    def from_data(cls, phone=None, email=None, **kwargs):
+    def from_data(cls, phone=None, email=None, external_account_binding=None, **kwargs):
         """Create registration resource from contact details."""
         details = list(kwargs.pop('contact', ()))
         if phone is not None:
@@ -288,6 +311,10 @@ class Registration(ResourceBody):
         if email is not None:
             details.extend([cls.email_prefix + mail for mail in email.split(',')])
         kwargs['contact'] = tuple(details)
+
+        if external_account_binding:
+            kwargs['external_account_binding'] = external_account_binding
+
         return cls(**kwargs)
 
     def _filter_contact(self, prefix):
diff --git a/certbot/cli.py b/certbot/cli.py
index 882ddba86..8557b12c8 100644
--- a/certbot/cli.py
+++ b/certbot/cli.py
@@ -859,7 +859,9 @@ class HelpfulArgumentParser(object):
         if chosen_topic == "everything":
             chosen_topic = "run"
         if chosen_topic == "all":
-            return dict([(t, True) for t in self.help_topics])
+            # Addition of condition closes #6209 (removal of duplicate route53 option).
+            return dict([(t, True) if t != 'certbot-route53:auth' else (t, False)
+                         for t in self.help_topics])
         elif not chosen_topic:
             return dict([(t, False) for t in self.help_topics])
         else:
@@ -944,6 +946,18 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
              "specified or you already have a certificate with the same "
              "name. In the case of a name collision it will append a number "
              "like 0001 to the file path name. (default: Ask)")
+    helpful.add(
+        [None, "run", "certonly", "register"],
+        "--eab-kid", dest="eab_kid",
+        metavar="EAB_KID",
+        help="Key Identifier for External Account Binding"
+    )
+    helpful.add(
+        [None, "run", "certonly", "register"],
+        "--eab-hmac-key", dest="eab_hmac_key",
+        metavar="EAB_HMAC_KEY",
+        help="HMAC key for External Account Binding"
+    )
     helpful.add(
         [None, "run", "certonly", "manage", "delete", "certificates",
          "renew", "enhance"], "--cert-name", dest="certname",
diff --git a/certbot/client.py b/certbot/client.py
index e634b6bd9..38b77a772 100644
--- a/certbot/client.py
+++ b/certbot/client.py
@@ -203,9 +203,27 @@ def perform_registration(acme, config, tos_cb):
     :returns: Registration Resource.
     :rtype: `acme.messages.RegistrationResource`
     """
+
+    eab_credentials_supplied = config.eab_kid and config.eab_hmac_key
+    if eab_credentials_supplied:
+        account_public_key = acme.client.net.key.public_key()
+        eab = messages.ExternalAccountBinding.from_data(account_public_key=account_public_key,
+                                                        kid=config.eab_kid,
+                                                        hmac_key=config.eab_hmac_key,
+                                                        directory=acme.client.directory)
+    else:
+        eab = None
+
+    if acme.external_account_required():
+        if not eab_credentials_supplied:
+            msg = ("Server requires external account binding."
+                   " Please use --eab-kid and --eab-hmac-key.")
+            raise errors.Error(msg)
+
     try:
-        return acme.new_account_and_tos(messages.NewRegistration.from_data(email=config.email),
-            tos_cb)
+        newreg = messages.NewRegistration.from_data(email=config.email,
+                                                    external_account_binding=eab)
+        return acme.new_account_and_tos(newreg, tos_cb)
     except messages.Error as e:
         if e.code == "invalidEmail" or e.code == "invalidContact":
             if config.noninteractive_mode:
diff --git a/certbot/constants.py b/certbot/constants.py
index 5bf68589d..eb4105f82 100644
--- a/certbot/constants.py
+++ b/certbot/constants.py
@@ -68,6 +68,8 @@ CLI_DEFAULTS = dict(
     directory_hooks=True,
     reuse_key=False,
     disable_renew_updates=False,
+    eab_hmac_key=None,
+    eab_kid=None,
 
     # Subparsers
     num=None,
diff --git a/docs/conf.py b/docs/conf.py
index 2e6c5a9b7..c72d1c1cf 100644
--- a/docs/conf.py
+++ b/docs/conf.py
@@ -17,6 +17,8 @@ import os
 import re
 import sys
 
+import sphinx
+
 
 here = os.path.abspath(os.path.dirname(__file__))
 
@@ -33,14 +35,13 @@ sys.path.insert(0, os.path.abspath(os.path.join(here, '..')))
 # -- General configuration ------------------------------------------------
 
 # If your documentation needs a minimal Sphinx version, state it here.
-needs_sphinx = '1.0'
+needs_sphinx = '1.2'
 
 # Add any Sphinx extension module names here, as strings. They can be
 # extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
 # ones.
 extensions = [
     'sphinx.ext.autodoc',
-    'sphinx.ext.imgconverter',
     'sphinx.ext.intersphinx',
     'sphinx.ext.todo',
     'sphinx.ext.coverage',
@@ -48,6 +49,9 @@ extensions = [
     'repoze.sphinx.autointerface',
 ]
 
+if sphinx.version_info >= (1, 6):
+    extensions.append('sphinx.ext.imgconverter')
+
 autodoc_member_order = 'bysource'
 autodoc_default_flags = ['show-inheritance', 'private-members']
 
diff --git a/local-oldest-requirements.txt b/local-oldest-requirements.txt
index 03226fc84..2346300a3 100644
--- a/local-oldest-requirements.txt
+++ b/local-oldest-requirements.txt
@@ -1,1 +1,1 @@
-acme[dev]==0.26.0
+-e acme[dev]
diff --git a/setup.py b/setup.py
index f8f5feadc..382149054 100644
--- a/setup.py
+++ b/setup.py
@@ -31,7 +31,7 @@ version = meta['version']
 # specified here to avoid masking the more specific request requirements in
 # acme. See https://github.com/pypa/pip/issues/988 for more info.
 install_requires = [
-    'acme>=0.26.0',
+    'acme>=0.29.0.dev0',
     # We technically need ConfigArgParse 0.10.0 for Python 2.6 support, but
     # saying so here causes a runtime error against our temporary fork of 0.9.3
     # in which we added 2.6 support (see #2243), so we relax the requirement.
@@ -68,9 +68,10 @@ dev3_extras = [
 ]
 
 docs_extras = [
+    # If you have Sphinx<1.5.1, you need docutils<0.13.1
+    # https://github.com/sphinx-doc/sphinx/issues/3212
     'repoze.sphinx.autointerface',
-    # sphinx.ext.imgconverter
-    'Sphinx >=1.6',
+    'Sphinx>=1.2', # Annotation support
     'sphinx_rtd_theme',
 ]
 
diff --git a/tools/_release.sh b/tools/_release.sh
index dd08974ae..90c176b0e 100755
--- a/tools/_release.sh
+++ b/tools/_release.sh
@@ -265,6 +265,16 @@ if [ "$RELEASE_BRANCH" = candidate-"$version" ] ; then
     SetVersion "$nextversion".dev0
     letsencrypt-auto-source/build.py
     git add letsencrypt-auto-source/letsencrypt-auto
+    for pkg_dir in $SUBPKGS_NO_CERTBOT .
+    do
+      if [ -f "$pkg_dir/local-oldest-requirements.txt" ]; then
+        sed -i "s/-e acme\[dev\]/acme[dev]==$version/" "$pkg_dir/local-oldest-requirements.txt"
+        sed -i "s/-e acme/acme[dev]==$version/" "$pkg_dir/local-oldest-requirements.txt"
+        sed -i "s/-e \.\[dev\]/certbot[dev]==$version/" "$pkg_dir/local-oldest-requirements.txt"
+        sed -i "s/-e \./certbot[dev]==$version/" "$pkg_dir/local-oldest-requirements.txt"
+        git add "$pkg_dir/local-oldest-requirements.txt"
+      fi
+    done
     git diff
     git commit -m "Bump version to $nextversion"
 fi
diff --git a/tools/dev_constraints.txt b/tools/dev_constraints.txt
index 65182bcde..778012d31 100644
--- a/tools/dev_constraints.txt
+++ b/tools/dev_constraints.txt
@@ -14,7 +14,7 @@ coverage==4.4.2
 decorator==4.1.2
 dns-lexicon==2.7.14
 dnspython==1.15.0
-docutils==0.14
+docutils==0.12
 execnet==1.5.0
 future==0.16.0
 futures==3.1.1


Test patch:
diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py
index 8b6cda47e..33ae3886b 100644
--- a/acme/acme/client_test.py
+++ b/acme/acme/client_test.py
@@ -284,6 +284,37 @@ class BackwardsCompatibleClientV2Test(ClientTestBase):
             client.update_registration(mock.sentinel.regr, None)
         mock_client().update_registration.assert_called_once_with(mock.sentinel.regr, None)
 
+    # newNonce present means it will pick acme_version 2
+    def test_external_account_required_true(self):
+        self.response.json.return_value = messages.Directory({
+            'newNonce': 'http://letsencrypt-test.com/acme/new-nonce',
+            'meta': messages.Directory.Meta(external_account_required=True),
+        }).to_json()
+
+        client = self._init()
+
+        self.assertTrue(client.external_account_required())
+
+    # newNonce present means it will pick acme_version 2
+    def test_external_account_required_false(self):
+        self.response.json.return_value = messages.Directory({
+            'newNonce': 'http://letsencrypt-test.com/acme/new-nonce',
+            'meta': messages.Directory.Meta(external_account_required=False),
+        }).to_json()
+
+        client = self._init()
+
+        self.assertFalse(client.external_account_required())
+
+    def test_external_account_required_false_v1(self):
+        self.response.json.return_value = messages.Directory({
+            'meta': messages.Directory.Meta(external_account_required=False),
+        }).to_json()
+
+        client = self._init()
+
+        self.assertFalse(client.external_account_required())
+
 
 class ClientTest(ClientTestBase):
     """Tests for acme.client.Client."""
@@ -823,6 +854,23 @@ class ClientV2Test(ClientTestBase):
         self.response.json.return_value = self.regr.body.update(
             contact=()).to_json()
 
+    def test_external_account_required_true(self):
+        self.client.directory = messages.Directory({
+            'meta': messages.Directory.Meta(external_account_required=True)
+        })
+
+        self.assertTrue(self.client.external_account_required())
+
+    def test_external_account_required_false(self):
+        self.client.directory = messages.Directory({
+            'meta': messages.Directory.Meta(external_account_required=False)
+        })
+
+        self.assertFalse(self.client.external_account_required())
+
+    def test_external_account_required_default(self):
+        self.assertFalse(self.client.external_account_required())
+
     def test_post_as_get(self):
         with mock.patch('acme.client.ClientV2._authzr_from_response') as mock_client:
             mock_client.return_value = self.authzr2
diff --git a/acme/acme/messages_test.py b/acme/acme/messages_test.py
index 876fbe825..7efaaa1a3 100644
--- a/acme/acme/messages_test.py
+++ b/acme/acme/messages_test.py
@@ -174,6 +174,24 @@ class DirectoryTest(unittest.TestCase):
         self.assertTrue(result)
 
 
+class ExternalAccountBindingTest(unittest.TestCase):
+    def setUp(self):
+        from acme.messages import Directory
+        self.key = jose.jwk.JWKRSA(key=KEY.public_key())
+        self.kid = "kid-for-testing"
+        self.hmac_key = "hmac-key-for-testing"
+        self.dir = Directory({
+            'newAccount': 'http://url/acme/new-account',
+        })
+
+    def test_from_data(self):
+        from acme.messages import ExternalAccountBinding
+        eab = ExternalAccountBinding.from_data(self.key, self.kid, self.hmac_key, self.dir)
+
+        self.assertEqual(len(eab), 3)
+        self.assertEqual(sorted(eab.keys()), sorted(['protected', 'payload', 'signature']))
+
+
 class RegistrationTest(unittest.TestCase):
     """Tests for acme.messages.Registration."""
 
@@ -205,6 +223,22 @@ class RegistrationTest(unittest.TestCase):
             'mailto:admin@foo.com',
         ))
 
+    def test_new_registration_from_data_with_eab(self):
+        from acme.messages import NewRegistration, ExternalAccountBinding, Directory
+        key = jose.jwk.JWKRSA(key=KEY.public_key())
+        kid = "kid-for-testing"
+        hmac_key = "hmac-key-for-testing"
+        directory = Directory({
+            'newAccount': 'http://url/acme/new-account',
+        })
+        eab = ExternalAccountBinding.from_data(key, kid, hmac_key, directory)
+        reg = NewRegistration.from_data(email='admin@foo.com', external_account_binding=eab)
+        self.assertEqual(reg.contact, (
+            'mailto:admin@foo.com',
+        ))
+        self.assertEqual(sorted(reg.external_account_binding.keys()),
+                         sorted(['protected', 'payload', 'signature']))
+
     def test_phones(self):
         self.assertEqual(('1234',), self.reg.phones)
 
diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py
index 69ef16597..1448b86e7 100644
--- a/certbot/tests/cli_test.py
+++ b/certbot/tests/cli_test.py
@@ -431,6 +431,11 @@ class ParseTest(unittest.TestCase):  # pylint: disable=too-many-public-methods
         self.assertRaises(errors.Error, self.parse,
                           "--allow-subset-of-names -d *.example.org".split())
 
+    def test_route53_no_revert(self):
+        for help_flag in ['-h', '--help']:
+            for topic in ['all', 'plugins', 'dns-route53']:
+                self.assertFalse('certbot-route53:auth' in self._help_output([help_flag, topic]))
+
 
 class DefaultTest(unittest.TestCase):
     """Tests for certbot.cli._Default."""
diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py
index a4e70ce35..330529fc6 100644
--- a/certbot/tests/client_test.py
+++ b/certbot/tests/client_test.py
@@ -13,6 +13,8 @@ from certbot import util
 
 import certbot.tests.util as test_util
 
+from josepy import interfaces
+
 KEY = test_util.load_vector("rsa512_key.pem")
 CSR_SAN = test_util.load_vector("csr-san_512.pem")
 
@@ -64,9 +66,28 @@ class RegisterTest(test_util.ConfigTestCase):
         tos_cb = mock.MagicMock()
         return register(self.config, self.account_storage, tos_cb)
 
+    @staticmethod
+    def _public_key_mock():
+        m = mock.Mock(__class__=interfaces.JSONDeSerializable)
+        m.to_partial_json.return_value = '{"a": 1}'
+        return m
+
+    @staticmethod
+    def _new_acct_dir_mock():
+        return "/acme/new-account"
+
+    @staticmethod
+    def _true_mock():
+        return True
+
+    @staticmethod
+    def _false_mock():
+        return False
+
     def test_no_tos(self):
         with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") 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.eff.handle_subscription") as mock_handle:
                 with mock.patch("certbot.account.report_new_account"):
                     mock_client().new_account_and_tos.side_effect = errors.Error
@@ -78,7 +99,8 @@ class RegisterTest(test_util.ConfigTestCase):
                     self.assertTrue(mock_handle.called)
 
     def test_it(self):
-        with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2"):
+        with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+            mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot.account.report_new_account"):
                 with mock.patch("certbot.eff.handle_subscription"):
                     self._call()
@@ -91,6 +113,7 @@ class RegisterTest(test_util.ConfigTestCase):
         msg = "DNS problem: NXDOMAIN looking up MX for example.com"
         mx_err = messages.Error.with_code('invalidContact', detail=msg)
         with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+            mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot.eff.handle_subscription") as mock_handle:
                 mock_client().new_account_and_tos.side_effect = [mx_err, mock.MagicMock()]
                 self._call()
@@ -104,6 +127,7 @@ class RegisterTest(test_util.ConfigTestCase):
         msg = "DNS problem: NXDOMAIN looking up MX for example.com"
         mx_err = messages.Error.with_code('invalidContact', detail=msg)
         with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+            mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot.eff.handle_subscription"):
                 mock_client().new_account_and_tos.side_effect = [mx_err, mock.MagicMock()]
                 self.assertRaises(errors.Error, self._call)
@@ -115,7 +139,8 @@ class RegisterTest(test_util.ConfigTestCase):
     @mock.patch("certbot.client.logger")
     def test_without_email(self, mock_logger):
         with mock.patch("certbot.eff.handle_subscription") as mock_handle:
-            with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2"):
+            with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_clnt:
+                mock_clnt().external_account_required.side_effect = self._false_mock
                 with mock.patch("certbot.account.report_new_account"):
                     self.config.email = None
                     self.config.register_unsafely_without_email = True
@@ -129,6 +154,7 @@ class RegisterTest(test_util.ConfigTestCase):
     def test_dry_run_no_staging_account(self, _rep, mock_get_email):
         """Tests dry-run for no staging account, expect account created with no email"""
         with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+            mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot.eff.handle_subscription"):
                 with mock.patch("certbot.account.report_new_account"):
                     self.config.dry_run = True
@@ -138,11 +164,53 @@ class RegisterTest(test_util.ConfigTestCase):
                     # check Certbot created an account with no email. Contact should return empty
                     self.assertFalse(mock_client().new_account_and_tos.call_args[0][0].contact)
 
+    def test_with_eab_arguments(self):
+        with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+            mock_client().client.directory.__getitem__ = mock.Mock(
+                side_effect=self._new_acct_dir_mock
+            )
+            mock_client().external_account_required.side_effect = self._false_mock
+            with mock.patch("certbot.eff.handle_subscription"):
+                target = "certbot.client.messages.ExternalAccountBinding.from_data"
+                with mock.patch(target) as mock_eab_from_data:
+                    self.config.eab_kid = "test-kid"
+                    self.config.eab_hmac_key = "J2OAqW4MHXsrHVa_PVg0Y-L_R4SYw0_aL1le6mfblbE"
+                    self._call()
+
+                    self.assertTrue(mock_eab_from_data.called)
+
+    def test_without_eab_arguments(self):
+        with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+            mock_client().external_account_required.side_effect = self._false_mock
+            with mock.patch("certbot.eff.handle_subscription"):
+                target = "certbot.client.messages.ExternalAccountBinding.from_data"
+                with mock.patch(target) as mock_eab_from_data:
+                    self.config.eab_kid = None
+                    self.config.eab_hmac_key = None
+                    self._call()
+
+                    self.assertFalse(mock_eab_from_data.called)
+
+    def test_external_account_required_without_eab_arguments(self):
+        with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") 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.eff.handle_subscription"):
+                with mock.patch("certbot.client.messages.ExternalAccountBinding.from_data"):
+                    self.config.eab_kid = None
+                    self.config.eab_hmac_key = None
+
+                    self.assertRaises(errors.Error, self._call)
+
     def test_unsupported_error(self):
         from acme import messages
         msg = "Test"
         mx_err = messages.Error(detail=msg, typ="malformed", title="title")
         with mock.patch("certbot.client.acme_client.BackwardsCompatibleClientV2") as mock_client:
+            mock_client().client.directory.__getitem__ = mock.Mock(
+                side_effect=self._new_acct_dir_mock
+            )
+            mock_client().external_account_required.side_effect = self._false_mock
             with mock.patch("certbot.eff.handle_subscription") as mock_handle:
                 mock_client().new_account_and_tos.side_effect = [mx_err, mock.MagicMock()]
                 self.assertRaises(messages.Error, self._call)