From 83f7e72fefb8d9087a5ad488153a644e1b905572 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Tue, 10 Jul 2018 13:03:25 -0700 Subject: [PATCH] Update and delete registration with account reuse (#6174) * find the correct url when deactivating an acmev1 account on the acmev2 endpoint * set regr in ClientNetwork.account after deactivating on the server * update self.net.account * move logic into update_registration * return methods to their original order to please git * factor out common code * update test_fowarding to use a method that still gets forwarded * add acme module test coverage * pragma no cover on correct line * use previous regr uri * strip unnecessary items from regr before saving * add explanation to main.py * add extra check to client_test.py * use empty dict instead of empty string to indicate lack of body that we save to disk --- acme/acme/client.py | 26 +++++++++++++++++++++++++- acme/acme/client_test.py | 22 +++++++++++++++++++++- acme/acme/messages.py | 1 + certbot/account.py | 9 ++++++--- certbot/main.py | 5 +++++ 5 files changed, 58 insertions(+), 5 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index bdc07fb1c..a0bfe460d 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -50,7 +50,6 @@ class ClientBase(object): # pylint: disable=too-many-instance-attributes :ivar .ClientNetwork net: Client network. :ivar int acme_version: ACME protocol version. 1 or 2. """ - def __init__(self, directory, net, acme_version): """Initialize. @@ -588,6 +587,30 @@ class ClientV2(ClientBase): self.net.account = regr return regr + def update_registration(self, regr, update=None): + """Update registration. + + :param messages.RegistrationResource regr: Registration Resource. + :param messages.Registration update: Updated body of the + resource. If not provided, body will be taken from `regr`. + + :returns: Updated Registration Resource. + :rtype: `.RegistrationResource` + + """ + # https://github.com/certbot/certbot/issues/6155 + new_regr = self._get_v2_account(regr) + return super(ClientV2, self).update_registration(new_regr, update) + + def _get_v2_account(self, regr): + self.net.account = None + only_existing_reg = regr.body.update(only_return_existing=True) + response = self._post(self.directory['newAccount'], only_existing_reg) + updated_uri = response.headers['Location'] + new_regr = regr.update(uri=updated_uri) + self.net.account = new_regr + return new_regr + def new_order(self, csr_pem): """Request a new Order object from the server. @@ -910,6 +933,7 @@ class ClientNetwork(object): # pylint: disable=too-many-instance-attributes if acme_version == 2: kwargs["url"] = url # newAccount and revokeCert work without the kid + # newAccount must not have kid if self.account is not None: kwargs["kid"] = self.account["uri"] kwargs["key"] = self.key diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index f3018ed81..cd31c4ac3 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -139,7 +139,7 @@ class BackwardsCompatibleClientV2Test(ClientTestBase): client = self._init() self.assertEqual(client.directory, client.client.directory) self.assertEqual(client.key, KEY) - self.assertEqual(client.update_registration, client.client.update_registration) + self.assertEqual(client.deactivate_registration, client.client.deactivate_registration) self.assertRaises(AttributeError, client.__getattr__, 'nonexistent') self.assertRaises(AttributeError, client.__getattr__, 'new_account_and_tos') self.assertRaises(AttributeError, client.__getattr__, 'new_account') @@ -270,6 +270,13 @@ class BackwardsCompatibleClientV2Test(ClientTestBase): client.revoke(messages_test.CERT, self.rsn) mock_client().revoke.assert_called_once_with(messages_test.CERT, self.rsn) + def test_update_registration(self): + self.response.json.return_value = DIRECTORY_V1.to_json() + with mock.patch('acme.client.Client') as mock_client: + client = self._init() + client.update_registration(mock.sentinel.regr, None) + mock_client().update_registration.assert_called_once_with(mock.sentinel.regr, None) + class ClientTest(ClientTestBase): """Tests for acme.client.Client.""" @@ -789,6 +796,19 @@ class ClientV2Test(ClientTestBase): self.net.post.assert_called_once_with( self.directory["revokeCert"], mock.ANY, acme_version=2) + def test_update_registration(self): + # "Instance of 'Field' has no to_json/update member" bug: + # pylint: disable=no-member + self.response.headers['Location'] = self.regr.uri + self.response.json.return_value = self.regr.body.to_json() + self.assertEqual(self.regr, self.client.update_registration(self.regr)) + self.assertNotEqual(self.client.net.account, None) + self.assertEqual(self.client.net.post.call_count, 2) + self.assertTrue(DIRECTORY_V2.newAccount in self.net.post.call_args_list[0][0]) + + self.response.json.return_value = self.regr.body.update( + contact=()).to_json() + class MockJSONDeSerializable(jose.JSONDeSerializable): # pylint: disable=missing-docstring diff --git a/acme/acme/messages.py b/acme/acme/messages.py index 827a4dd11..5be458580 100644 --- a/acme/acme/messages.py +++ b/acme/acme/messages.py @@ -274,6 +274,7 @@ class Registration(ResourceBody): agreement = jose.Field('agreement', omitempty=True) status = jose.Field('status', omitempty=True) terms_of_service_agreed = jose.Field('termsOfServiceAgreed', omitempty=True) + only_return_existing = jose.Field('onlyReturnExisting', omitempty=True) phone_prefix = 'tel:' email_prefix = 'mailto:' diff --git a/certbot/account.py b/certbot/account.py index 5e9455048..2f261f759 100644 --- a/certbot/account.py +++ b/certbot/account.py @@ -270,9 +270,12 @@ class AccountFileStorage(interfaces.AccountStorage): if hasattr(acme.directory, "new-authz"): regr = RegistrationResourceWithNewAuthzrURI( new_authzr_uri=acme.directory.new_authz, - body=regr.body, - uri=regr.uri, - terms_of_service=regr.terms_of_service) + body={}, + uri=regr.uri) + else: + regr = messages.RegistrationResource( + body={}, + uri=regr.uri) regr_file.write(regr.json_dumps()) if not regr_only: with util.safe_open(self._key_path(account_dir_path), diff --git a/certbot/main.py b/certbot/main.py index 556722104..2c7ded3e2 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -735,8 +735,13 @@ def register(config, unused_plugins): cb_client = client.Client(config, acc, None, None, acme=acme) # We rely on an exception to interrupt this process if it didn't work. acc_contacts = ['mailto:' + email for email in config.email.split(',')] + prev_regr_uri = acc.regr.uri acc.regr = cb_client.acme.update_registration(acc.regr.update( body=acc.regr.body.update(contact=acc_contacts))) + # A v1 account being used as a v2 account will result in changing the uri to + # the v2 uri. Since it's the same object on disk, put it back to the v1 uri + # so that we can also continue to use the account object with acmev1. + acc.regr = acc.regr.update(uri=prev_regr_uri) account_storage.save_regr(acc, cb_client.acme) eff.handle_subscription(config) add_msg("Your e-mail address was updated to {0}.".format(config.email))