From 73bd801f352fb9bad7fe8bc35c368f573c15a21e Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Fri, 16 Feb 2018 16:21:02 -0800 Subject: [PATCH 01/21] add and use request_authorizations --- acme/acme/client.py | 14 ++++++++++++++ certbot/auth_handler.py | 10 ++++++---- certbot/client.py | 23 ++++++++--------------- certbot/main.py | 2 +- 4 files changed, 29 insertions(+), 20 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index 1f4ae4fad..6b4d65233 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -671,6 +671,7 @@ class BackwardsCompatibleClientV2(object): self.client = Client(directory, key=key, net=net) else: self.client = ClientV2(directory, net=net) + self.orderr = None def __getattr__(self, name): if name in vars(self.client): @@ -705,6 +706,19 @@ class BackwardsCompatibleClientV2(object): regr = regr.update(terms_of_service_agreed=True) return self.client.new_account(regr) + def request_authorizations(self, csr_pem): + if self.acme_version == 1: + csr = OpenSSL.crypto.load_certificate_request(OpenSSL.crypto.FILETYPE_PEM, csr_pem) + # pylint: disable=protected-access + dnsNames = crypto_util._pyopenssl_cert_or_req_all_names(csr) + authorizations = [] + for domain in dnsNames: + authorizations.append(self.client.request_domain_challenges(domain)) + return authorizations + else: + self.orderr = self.client.new_order(csr_pem) + return self.orderr.authorizations + def _acme_version_from_directory(self, directory): if hasattr(directory, 'newNonce'): return 2 diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 5f520cbcb..4f88199e3 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -48,10 +48,10 @@ class AuthHandler(object): # List must be used to keep responses straight. self.achalls = [] - def get_authorizations(self, domains, best_effort=False): + def get_authorizations(self, csr_pem, best_effort=False): """Retrieve all authorizations for challenges. - :param list domains: Domains for authorization + :param list csr_pem: CSR containing domains for authorization :param bool best_effort: Whether or not all authorizations are required (this is useful in renewal) @@ -62,8 +62,10 @@ class AuthHandler(object): authorizations """ - for domain in domains: - self.authzr[domain] = self.acme.request_domain_challenges(domain) + authzrs = self.acme.request_authorizations(csr_pem) + for authzr in authzrs: + self.authzr[authzr.body.identifier.value] = authzr + domains = self.authzr.keys() self._choose_challenges(domains) config = zope.component.getUtility(interfaces.IConfig) diff --git a/certbot/client.py b/certbot/client.py index 67ee8f7fa..8e3ec6c62 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -235,13 +235,9 @@ class Client(object): else: self.auth_handler = None - def obtain_certificate_from_csr(self, domains, csr, authzr=None): + def obtain_certificate_from_csr(self, csr, authzr=None): """Obtain certificate. - Internal function with precondition that `domains` are - consistent with identifiers present in the `csr`. - - :param list domains: Domain names. :param .util.CSR csr: PEM-encoded Certificate Signing Request. The key used to generate this CSR can be different than `authkey`. @@ -261,10 +257,10 @@ class Client(object): if self.account.regr is None: raise errors.Error("Please register with the ACME server first.") - logger.debug("CSR: %s, domains: %s", csr, domains) + logger.debug("CSR: %s", csr) if authzr is None: - authzr = self.auth_handler.get_authorizations(domains) + authzr = self.auth_handler.get_authorizations(csr) certr = self.acme.request_issuance( jose.ComparableX509( @@ -307,13 +303,6 @@ class Client(object): :rtype: tuple """ - authzr = self.auth_handler.get_authorizations( - domains, - self.config.allow_subset_of_names) - - auth_domains = set(a.body.identifier.value for a in authzr) - domains = [d for d in domains if d in auth_domains] - # Create CSR from names if self.config.dry_run: key = util.Key(file=None, @@ -326,8 +315,12 @@ class Client(object): self.config.rsa_key_size, self.config.key_dir) csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) + authzr = self.auth_handler.get_authorizations( + csr, + self.config.allow_subset_of_names) + certr, chain = self.obtain_certificate_from_csr( - domains, csr, authzr=authzr) + csr, authzr=authzr) return certr, chain, key, csr diff --git a/certbot/main.py b/certbot/main.py index ff3758985..d01f68920 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -1064,7 +1064,7 @@ def _csr_get_and_save_cert(config, le_client): """ csr, _ = config.actual_csr - certr, chain = le_client.obtain_certificate_from_csr(config.domains, csr) + certr, chain = le_client.obtain_certificate_from_csr(csr) if config.dry_run: logger.debug( "Dry run: skipping saving certificate to %s", config.cert_path) From eaf739184cf517d5a3f5103caa072bb0cd39b4e9 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Fri, 16 Feb 2018 16:29:42 -0800 Subject: [PATCH 02/21] pass pem to auth_handler --- certbot/client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 8e3ec6c62..d7d2acb14 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -260,7 +260,7 @@ class Client(object): logger.debug("CSR: %s", csr) if authzr is None: - authzr = self.auth_handler.get_authorizations(csr) + authzr = self.auth_handler.get_authorizations(csr.data) certr = self.acme.request_issuance( jose.ComparableX509( @@ -316,7 +316,7 @@ class Client(object): csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) authzr = self.auth_handler.get_authorizations( - csr, + csr.data, self.config.allow_subset_of_names) certr, chain = self.obtain_certificate_from_csr( From ea2022588b4d95f1d14849b918b2cd3788cc6084 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Fri, 16 Feb 2018 16:32:49 -0800 Subject: [PATCH 03/21] add docstring --- acme/acme/client.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/acme/acme/client.py b/acme/acme/client.py index 6b4d65233..e7cf016bb 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -707,6 +707,16 @@ class BackwardsCompatibleClientV2(object): return self.client.new_account(regr) def request_authorizations(self, csr_pem): + """Request authorizations for the domains in csr_pem. + + Calls request_domain_challenges for each domain for V1, and + calls new_order and saves the result for V2. + + :param str csr_pem: A CSR in PEM format. + + :returns: List of Authorization Resources. + :rtype: list of `.AuthorizationResource` + """ if self.acme_version == 1: csr = OpenSSL.crypto.load_certificate_request(OpenSSL.crypto.FILETYPE_PEM, csr_pem) # pylint: disable=protected-access From 20d0b91c710bd8110aa5ee23082f45583e7789a4 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Fri, 16 Feb 2018 17:35:10 -0800 Subject: [PATCH 04/21] switch interface to new_order and remove best_effort flag --- acme/acme/client.py | 18 ++++++++---------- certbot/auth_handler.py | 27 ++++++++++----------------- certbot/client.py | 15 ++++----------- 3 files changed, 22 insertions(+), 38 deletions(-) diff --git a/acme/acme/client.py b/acme/acme/client.py index e7cf016bb..1838fab42 100644 --- a/acme/acme/client.py +++ b/acme/acme/client.py @@ -671,7 +671,6 @@ class BackwardsCompatibleClientV2(object): self.client = Client(directory, key=key, net=net) else: self.client = ClientV2(directory, net=net) - self.orderr = None def __getattr__(self, name): if name in vars(self.client): @@ -706,16 +705,16 @@ class BackwardsCompatibleClientV2(object): regr = regr.update(terms_of_service_agreed=True) return self.client.new_account(regr) - def request_authorizations(self, csr_pem): - """Request authorizations for the domains in csr_pem. + def new_order(self, csr_pem): + """Request a new Order object from the server. - Calls request_domain_challenges for each domain for V1, and - calls new_order and saves the result for V2. + If using ACMEv1, returns a dummy OrderResource with only + the authorizations field filled in. :param str csr_pem: A CSR in PEM format. - :returns: List of Authorization Resources. - :rtype: list of `.AuthorizationResource` + :returns: The newly created order. + :rtype: OrderResource """ if self.acme_version == 1: csr = OpenSSL.crypto.load_certificate_request(OpenSSL.crypto.FILETYPE_PEM, csr_pem) @@ -724,10 +723,9 @@ class BackwardsCompatibleClientV2(object): authorizations = [] for domain in dnsNames: authorizations.append(self.client.request_domain_challenges(domain)) - return authorizations + return messages.OrderResource(authorizations=authorizations) else: - self.orderr = self.client.new_order(csr_pem) - return self.orderr.authorizations + return self.client.new_order(csr_pem) def _acme_version_from_directory(self, directory): if hasattr(directory, 'newNonce'): diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 4f88199e3..825513329 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -48,12 +48,11 @@ class AuthHandler(object): # List must be used to keep responses straight. self.achalls = [] - def get_authorizations(self, csr_pem, best_effort=False): + def handle_authorizations(self, orderr): """Retrieve all authorizations for challenges. - :param list csr_pem: CSR containing domains for authorization - :param bool best_effort: Whether or not all authorizations are - required (this is useful in renewal) + :param acme.messages.OrderResource orderr: must have + authorizations filled in :returns: List of authorization resources :rtype: list @@ -62,7 +61,7 @@ class AuthHandler(object): authorizations """ - authzrs = self.acme.request_authorizations(csr_pem) + authzrs = orderr.authorizations for authzr in authzrs: self.authzr[authzr.body.identifier.value] = authzr domains = self.authzr.keys() @@ -80,7 +79,7 @@ class AuthHandler(object): 'Pass "-v" for more info about challenges.', pause=True) # Send all Responses - this modifies achalls - self._respond(resp, best_effort) + self._respond(resp) # Just make sure all decisions are complete. self.verify_authzr_complete() @@ -124,7 +123,7 @@ class AuthHandler(object): return resp - def _respond(self, resp, best_effort): + def _respond(self, resp): """Send/Receive confirmation of all challenges. .. note:: This method also cleans up the auth_handler state. @@ -137,7 +136,7 @@ class AuthHandler(object): # Check for updated status... try: - self._poll_challenges(chall_update, best_effort) + self._poll_challenges(chall_update) finally: # This removes challenges from self.achalls self._cleanup_challenges(active_achalls) @@ -169,7 +168,7 @@ class AuthHandler(object): return active_achalls def _poll_challenges( - self, chall_update, best_effort, min_sleep=3, max_rounds=15): + self, chall_update, min_sleep=3, max_rounds=15): """Wait for all challenge results to be determined.""" dom_to_check = set(chall_update.keys()) comp_domains = set() @@ -190,14 +189,8 @@ class AuthHandler(object): chall_update[domain].remove(achall) # We failed some challenges... damage control else: - if best_effort: - comp_domains.add(domain) - logger.warning( - "Challenge failed for domain %s", - domain) - else: - all_failed_achalls.update( - updated for _, updated in failed_achalls) + all_failed_achalls.update( + updated for _, updated in failed_achalls) if all_failed_achalls: _report_failed_challs(all_failed_achalls) diff --git a/certbot/client.py b/certbot/client.py index d7d2acb14..61e9db635 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -235,14 +235,12 @@ class Client(object): else: self.auth_handler = None - def obtain_certificate_from_csr(self, csr, authzr=None): + def obtain_certificate_from_csr(self, csr): """Obtain certificate. :param .util.CSR csr: PEM-encoded Certificate Signing Request. The key used to generate this CSR can be different than `authkey`. - :param list authzr: List of - :class:`acme.messages.AuthorizationResource` :returns: `.CertificateResource` and certificate chain (as returned by `.fetch_chain`). @@ -259,8 +257,8 @@ class Client(object): logger.debug("CSR: %s", csr) - if authzr is None: - authzr = self.auth_handler.get_authorizations(csr.data) + orderr = self.acme.new_order(csr.data) + authzr = self.auth_handler.handle_authorizations(orderr) certr = self.acme.request_issuance( jose.ComparableX509( @@ -315,12 +313,7 @@ class Client(object): self.config.rsa_key_size, self.config.key_dir) csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) - authzr = self.auth_handler.get_authorizations( - csr.data, - self.config.allow_subset_of_names) - - certr, chain = self.obtain_certificate_from_csr( - csr, authzr=authzr) + certr, chain = self.obtain_certificate_from_csr(csr) return certr, chain, key, csr From 68e24a8ea7eeb405592e40ed340baff6f3f3821b Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Fri, 16 Feb 2018 17:59:51 -0800 Subject: [PATCH 05/21] start test updates --- certbot/tests/auth_handler_test.py | 54 ++++++++++++++++-------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 32c4c0d3b..d424e59ca 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -57,8 +57,8 @@ class ChallengeFactoryTest(unittest.TestCase): errors.Error, self.handler._challenge_factory, "failure.com", [0]) -class GetAuthorizationsTest(unittest.TestCase): - """get_authorizations test. +class HandleAuthorizationsTest(unittest.TestCase): + """handle_authorizations test. This tests everything except for all functions under _poll_challenges. @@ -92,12 +92,11 @@ class GetAuthorizationsTest(unittest.TestCase): @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") def test_name1_tls_sni_01_1(self, mock_poll): - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES) - mock_poll.side_effect = self._validate_all - authzr = self.handler.get_authorizations(["0"]) + authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES) + mock_order = mock.MagicMock(authorizations=[authzr]) + authzr = self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_net.answer_challenge.call_count, 1) @@ -115,14 +114,13 @@ class GetAuthorizationsTest(unittest.TestCase): @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") def test_name1_tls_sni_01_1_http_01_1_dns_1(self, mock_poll): - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES, combos=False) - mock_poll.side_effect = self._validate_all self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) self.mock_auth.get_chall_pref.return_value.append(challenges.DNS01) - authzr = self.handler.get_authorizations(["0"]) + authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES) + mock_order = mock.MagicMock(authorizations=[authzr]) + authzr = self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_net.answer_challenge.call_count, 3) @@ -146,7 +144,11 @@ class GetAuthorizationsTest(unittest.TestCase): mock_poll.side_effect = self._validate_all - authzr = self.handler.get_authorizations(["0", "1", "2"]) + 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)] + mock_order = mock.MagicMock(authorizations=authzrs) + authzr = self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_net.answer_challenge.call_count, 3) @@ -169,31 +171,33 @@ class GetAuthorizationsTest(unittest.TestCase): def test_debug_challenges(self, mock_poll): zope.component.provideUtility( mock.Mock(debug_challenges=True), interfaces.IConfig) - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES) + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] + mock_order = mock.MagicMock(authorizations=authzrs) mock_poll.side_effect = self._validate_all - self.handler.get_authorizations(["0"]) + self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_net.answer_challenge.call_count, 1) self.assertEqual(self.mock_display.notification.call_count, 1) def test_perform_failure(self): - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES) + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] + mock_order = mock.MagicMock(authorizations=authzrs) + self.mock_auth.perform.side_effect = errors.AuthorizationError self.assertRaises( - errors.AuthorizationError, self.handler.get_authorizations, ["0"]) + errors.AuthorizationError, self.handler.handle_authorizations, mock_order) def test_no_domains(self): - self.assertRaises(errors.AuthorizationError, self.handler.get_authorizations, []) + mock_order = mock.MagicMock(authorizations=[]) + self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations, mock_order) @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") def test_preferred_challenge_choice(self, mock_poll): - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES) + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] + mock_order = mock.MagicMock(authorizations=authzrs) mock_poll.side_effect = self._validate_all self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) @@ -201,20 +205,20 @@ class GetAuthorizationsTest(unittest.TestCase): self.handler.pref_challs.extend((challenges.HTTP01.typ, challenges.DNS01.typ,)) - self.handler.get_authorizations(["0"]) + self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( self.mock_auth.cleanup.call_args[0][0][0].typ, "http-01") def test_preferred_challenges_not_supported(self): - self.mock_net.request_domain_challenges.side_effect = functools.partial( - gen_dom_authzr, challs=acme_util.CHALLENGES) + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] + mock_order = mock.MagicMock(authorizations=authzrs) self.handler.pref_challs.append(challenges.HTTP01.typ) self.assertRaises( - errors.AuthorizationError, self.handler.get_authorizations, ["0"]) + errors.AuthorizationError, self.handler.handle_authorizations, mock_order) - def _validate_all(self, unused_1, unused_2): + def _validate_all(self, unused_1): for dom in six.iterkeys(self.handler.authzr): azr = self.handler.authzr[dom] self.handler.authzr[dom] = acme_util.gen_authzr( From d6b4e2001b404b1f9bd4c3929e888726d583ee57 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 13:19:04 -0800 Subject: [PATCH 06/21] put back in best_effort code, with a todo for actually supporting it in ACMEv2 --- certbot/auth_handler.py | 22 +++++++++++++++------- certbot/client.py | 11 ++++++++--- certbot/tests/auth_handler_test.py | 2 +- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 825513329..662cadc65 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -48,11 +48,13 @@ class AuthHandler(object): # List must be used to keep responses straight. self.achalls = [] - def handle_authorizations(self, orderr): + def handle_authorizations(self, orderr, best_effort=False): """Retrieve all authorizations for challenges. :param acme.messages.OrderResource orderr: must have authorizations filled in + :param bool best_effort: Whether or not all authorizations are + required (this is useful in renewal) :returns: List of authorization resources :rtype: list @@ -79,7 +81,7 @@ class AuthHandler(object): 'Pass "-v" for more info about challenges.', pause=True) # Send all Responses - this modifies achalls - self._respond(resp) + self._respond(resp, best_effort) # Just make sure all decisions are complete. self.verify_authzr_complete() @@ -123,7 +125,7 @@ class AuthHandler(object): return resp - def _respond(self, resp): + def _respond(self, resp, best_effort): """Send/Receive confirmation of all challenges. .. note:: This method also cleans up the auth_handler state. @@ -136,7 +138,7 @@ class AuthHandler(object): # Check for updated status... try: - self._poll_challenges(chall_update) + self._poll_challenges(chall_update, best_effort) finally: # This removes challenges from self.achalls self._cleanup_challenges(active_achalls) @@ -168,7 +170,7 @@ class AuthHandler(object): return active_achalls def _poll_challenges( - self, chall_update, min_sleep=3, max_rounds=15): + self, chall_update, best_effort, min_sleep=3, max_rounds=15): """Wait for all challenge results to be determined.""" dom_to_check = set(chall_update.keys()) comp_domains = set() @@ -189,8 +191,14 @@ class AuthHandler(object): chall_update[domain].remove(achall) # We failed some challenges... damage control else: - all_failed_achalls.update( - updated for _, updated in failed_achalls) + if best_effort: + comp_domains.add(domain) + logger.warning( + "Challenge failed for domain %s", + domain) + else: + all_failed_achalls.update( + updated for _, updated in failed_achalls) if all_failed_achalls: _report_failed_challs(all_failed_achalls) diff --git a/certbot/client.py b/certbot/client.py index 61e9db635..5feea662d 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -235,12 +235,13 @@ class Client(object): else: self.auth_handler = None - def obtain_certificate_from_csr(self, csr): + def obtain_certificate_from_csr(self, csr, best_effort=False): """Obtain certificate. :param .util.CSR csr: PEM-encoded Certificate Signing Request. The key used to generate this CSR can be different than `authkey`. + :param bool best_effort: Whether or not all authorizations are required :returns: `.CertificateResource` and certificate chain (as returned by `.fetch_chain`). @@ -258,7 +259,11 @@ class Client(object): logger.debug("CSR: %s", csr) orderr = self.acme.new_order(csr.data) - authzr = self.auth_handler.handle_authorizations(orderr) + authzr = self.auth_handler.handle_authorizations(orderr, best_effort) + if best_effort: + # TODO: check if we passed all authorizations, and if not, + # create a new order and try again, possibly in a loop + pass certr = self.acme.request_issuance( jose.ComparableX509( @@ -313,7 +318,7 @@ class Client(object): self.config.rsa_key_size, self.config.key_dir) csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) - certr, chain = self.obtain_certificate_from_csr(csr) + certr, chain = self.obtain_certificate_from_csr(csr, self.config.allow_subset_of_names) return certr, chain, key, csr diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index d424e59ca..ea8b006c4 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -218,7 +218,7 @@ class HandleAuthorizationsTest(unittest.TestCase): self.assertRaises( errors.AuthorizationError, self.handler.handle_authorizations, mock_order) - def _validate_all(self, unused_1): + def _validate_all(self, unused_1, unused_2): for dom in six.iterkeys(self.handler.authzr): azr = self.handler.authzr[dom] self.handler.authzr[dom] = acme_util.gen_authzr( From 11f2f1e576243a255afbcd166b042cab2e2f1c4b Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 13:20:41 -0800 Subject: [PATCH 07/21] remove extra spaces --- certbot/auth_handler.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 662cadc65..47d806b94 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -194,8 +194,8 @@ class AuthHandler(object): if best_effort: comp_domains.add(domain) logger.warning( - "Challenge failed for domain %s", - domain) + "Challenge failed for domain %s", + domain) else: all_failed_achalls.update( updated for _, updated in failed_achalls) From a0e84e65ce9cfc041f341a6bedf36525e49fa1b2 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 14:29:04 -0800 Subject: [PATCH 08/21] auth_handler tests are happy --- certbot/tests/auth_handler_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index ea8b006c4..3633b673d 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -118,7 +118,7 @@ class HandleAuthorizationsTest(unittest.TestCase): self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) self.mock_auth.get_chall_pref.return_value.append(challenges.DNS01) - authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES) + authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=False) mock_order = mock.MagicMock(authorizations=[authzr]) authzr = self.handler.handle_authorizations(mock_order) From 76a0cbf9c23b9827c4bf94e9a6521a4bf049a466 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 14:43:12 -0800 Subject: [PATCH 09/21] client tests passing --- certbot/tests/client_test.py | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index a9a87b80b..b6cbca367 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -141,16 +141,17 @@ class ClientTest(ClientTestCommon): def _mock_obtain_certificate(self): self.client.auth_handler = mock.MagicMock() - self.client.auth_handler.get_authorizations.return_value = [None] + self.client.auth_handler.handle_authorizations.return_value = [None] self.acme.request_issuance.return_value = mock.sentinel.certr self.acme.fetch_chain.return_value = mock.sentinel.chain + self.acme.new_order.return_value = mock.sentinel.orderr def _check_obtain_certificate(self): - self.client.auth_handler.get_authorizations.assert_called_once_with( - self.eg_domains, + self.client.auth_handler.handle_authorizations.assert_called_once_with( + mock.sentinel.orderr, self.config.allow_subset_of_names) - authzr = self.client.auth_handler.get_authorizations() + authzr = self.client.auth_handler.handle_authorizations() self.acme.request_issuance.assert_called_once_with( jose.ComparableX509(OpenSSL.crypto.load_certificate_request( @@ -167,31 +168,19 @@ class ClientTest(ClientTestCommon): test_csr = util.CSR(form="pem", file=None, data=CSR_SAN) auth_handler = self.client.auth_handler - authzr = auth_handler.get_authorizations(self.eg_domains, False) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( - self.eg_domains, test_csr, - authzr=authzr)) + best_effort=False)) # and that the cert was obtained correctly self._check_obtain_certificate() - # Test for authzr=None - self.assertEqual( - (mock.sentinel.certr, mock.sentinel.chain), - self.client.obtain_certificate_from_csr( - self.eg_domains, - test_csr, - authzr=None)) - auth_handler.get_authorizations.assert_called_with(self.eg_domains) - # Test for no auth_handler self.client.auth_handler = None self.assertRaises( errors.Error, self.client.obtain_certificate_from_csr, - self.eg_domains, test_csr) mock_logger.warning.assert_called_once_with(mock.ANY) @@ -204,13 +193,10 @@ class ClientTest(ClientTestCommon): test_csr = util.CSR(form="der", file=None, data=CSR_SAN) auth_handler = self.client.auth_handler - authzr = auth_handler.get_authorizations(self.eg_domains, False) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( - self.eg_domains, - test_csr, - authzr=authzr)) + test_csr)) self.assertEqual(1, mock_get_utility().notification.call_count) @test_util.patch_get_utility() @@ -220,13 +206,10 @@ class ClientTest(ClientTestCommon): test_csr = util.CSR(form="der", file=None, data=CSR_SAN) auth_handler = self.client.auth_handler - authzr = auth_handler.get_authorizations(self.eg_domains, False) self.assertRaises( acme_errors.Error, self.client.obtain_certificate_from_csr, - self.eg_domains, - test_csr, - authzr=authzr) + test_csr) self.assertEqual(1, mock_get_utility().notification.call_count) @mock.patch("certbot.client.crypto_util") @@ -276,7 +259,7 @@ class ClientTest(ClientTestCommon): identifier=mock.MagicMock( value=domain)))) - self.client.auth_handler.get_authorizations.return_value = authzr + self.client.auth_handler.handle_authorizations.return_value = authzr with test_util.patch_get_utility(): result = self.client.obtain_certificate(self.eg_domains) From 3dfeb483ee853333b11e829ef7d985ebf5ad7269 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 14:49:23 -0800 Subject: [PATCH 10/21] lint --- certbot/tests/client_test.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index b6cbca367..570080e3b 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -166,7 +166,6 @@ class ClientTest(ClientTestCommon): mock_logger): self._mock_obtain_certificate() test_csr = util.CSR(form="pem", file=None, data=CSR_SAN) - auth_handler = self.client.auth_handler self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), @@ -191,7 +190,6 @@ class ClientTest(ClientTestCommon): self.acme.fetch_chain.side_effect = [acme_errors.Error, mock.sentinel.chain] test_csr = util.CSR(form="der", file=None, data=CSR_SAN) - auth_handler = self.client.auth_handler self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), @@ -204,7 +202,6 @@ class ClientTest(ClientTestCommon): self._mock_obtain_certificate() self.acme.fetch_chain.side_effect = acme_errors.Error test_csr = util.CSR(form="der", file=None, data=CSR_SAN) - auth_handler = self.client.auth_handler self.assertRaises( acme_errors.Error, From d6af978472d7519b615e8894903383610ab41269 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 14:52:11 -0800 Subject: [PATCH 11/21] remove if/pass --- certbot/client.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 5feea662d..65e85a159 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -260,10 +260,8 @@ class Client(object): orderr = self.acme.new_order(csr.data) authzr = self.auth_handler.handle_authorizations(orderr, best_effort) - if best_effort: - # TODO: check if we passed all authorizations, and if not, - # create a new order and try again, possibly in a loop - pass + # TODO: check if we passed all authorizations, and if not, + # create a new order and try again, possibly in a loop certr = self.acme.request_issuance( jose.ComparableX509( From d29c637bf94cf083ef3dc1648bed3697229b8025 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 15:36:35 -0800 Subject: [PATCH 12/21] support best_effort --- certbot/client.py | 29 +++++++++++++++++++++-------- certbot/tests/client_test.py | 31 ++++++++++++++++++++++++++----- 2 files changed, 47 insertions(+), 13 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 65e85a159..d00055eae 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -235,13 +235,13 @@ class Client(object): else: self.auth_handler = None - def obtain_certificate_from_csr(self, csr, best_effort=False): + def obtain_certificate_from_csr(self, csr, orderr=None): """Obtain certificate. :param .util.CSR csr: PEM-encoded Certificate Signing Request. The key used to generate this CSR can be different than `authkey`. - :param bool best_effort: Whether or not all authorizations are required + :param acme.messages.OrderResource orderr: contains authzrs :returns: `.CertificateResource` and certificate chain (as returned by `.fetch_chain`). @@ -258,10 +258,12 @@ class Client(object): logger.debug("CSR: %s", csr) - orderr = self.acme.new_order(csr.data) - authzr = self.auth_handler.handle_authorizations(orderr, best_effort) - # TODO: check if we passed all authorizations, and if not, - # create a new order and try again, possibly in a loop + if orderr is None: + orderr = self.acme.new_order(csr.data) + authzr = self.auth_handler.handle_authorizations(orderr) + else: + authzr = orderr.authorizations + certr = self.acme.request_issuance( jose.ComparableX509( @@ -316,9 +318,20 @@ class Client(object): self.config.rsa_key_size, self.config.key_dir) csr = crypto_util.init_save_csr(key, domains, self.config.csr_dir) - certr, chain = self.obtain_certificate_from_csr(csr, self.config.allow_subset_of_names) + orderr = self.acme.new_order(csr.data) + authzr = self.auth_handler.handle_authorizations(orderr, self.config.allow_subset_of_names) + auth_domains = set(a.body.identifier.value for a in authzr) + successful_domains = [d for d in domains if d in auth_domains] - return certr, chain, key, csr + if successful_domains != domains: + if not self.config.dry_run: + # TODO: delete keys + pass + return self.obtain_certificate(successful_domains) + else: + certr, chain = self.obtain_certificate_from_csr(csr, orderr) + + return certr, chain, key, csr # pylint: disable=no-member def obtain_and_enroll_certificate(self, domains, certname): diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 570080e3b..376bf5a90 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -134,6 +134,7 @@ class ClientTest(ClientTestCommon): self.config.allow_subset_of_names = False self.config.dry_run = False self.eg_domains = ["example.com", "www.example.com"] + self.eg_order = mock.MagicMock(authorizations=[None]) def test_init_acme_verify_ssl(self): net = self.acme_client.call_args[0][0] @@ -144,11 +145,11 @@ class ClientTest(ClientTestCommon): self.client.auth_handler.handle_authorizations.return_value = [None] self.acme.request_issuance.return_value = mock.sentinel.certr self.acme.fetch_chain.return_value = mock.sentinel.chain - self.acme.new_order.return_value = mock.sentinel.orderr + self.acme.new_order.return_value = self.eg_order def _check_obtain_certificate(self): self.client.auth_handler.handle_authorizations.assert_called_once_with( - mock.sentinel.orderr, + self.eg_order, self.config.allow_subset_of_names) authzr = self.client.auth_handler.handle_authorizations() @@ -166,15 +167,26 @@ class ClientTest(ClientTestCommon): mock_logger): self._mock_obtain_certificate() test_csr = util.CSR(form="pem", file=None, data=CSR_SAN) + auth_handler = self.client.auth_handler + orderr = self.acme.new_order(test_csr.data) + authzr = auth_handler.handle_authorizations(orderr, False) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( test_csr, - best_effort=False)) + orderr=orderr)) # and that the cert was obtained correctly self._check_obtain_certificate() + # Test for orderr=None + self.assertEqual( + (mock.sentinel.certr, mock.sentinel.chain), + self.client.obtain_certificate_from_csr( + test_csr, + orderr=None)) + auth_handler.handle_authorizations.assert_called_with(self.eg_order) + # Test for no auth_handler self.client.auth_handler = None self.assertRaises( @@ -190,11 +202,15 @@ class ClientTest(ClientTestCommon): self.acme.fetch_chain.side_effect = [acme_errors.Error, mock.sentinel.chain] test_csr = util.CSR(form="der", file=None, data=CSR_SAN) + auth_handler = self.client.auth_handler + orderr = self.acme.new_order(test_csr.data) + authzr = auth_handler.handle_authorizations(orderr, False) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( - test_csr)) + test_csr, + orderr=orderr)) self.assertEqual(1, mock_get_utility().notification.call_count) @test_util.patch_get_utility() @@ -202,11 +218,15 @@ class ClientTest(ClientTestCommon): self._mock_obtain_certificate() self.acme.fetch_chain.side_effect = acme_errors.Error test_csr = util.CSR(form="der", file=None, data=CSR_SAN) + auth_handler = self.client.auth_handler + orderr = self.acme.new_order(test_csr.data) + authzr = auth_handler.handle_authorizations(orderr, False) self.assertRaises( acme_errors.Error, self.client.obtain_certificate_from_csr, - test_csr) + test_csr, + orderr=orderr) self.assertEqual(1, mock_get_utility().notification.call_count) @mock.patch("certbot.client.crypto_util") @@ -256,6 +276,7 @@ class ClientTest(ClientTestCommon): identifier=mock.MagicMock( value=domain)))) + self.eg_order.authorizations = authzr self.client.auth_handler.handle_authorizations.return_value = authzr with test_util.patch_get_utility(): From 7c073dbcaf6d6789da01734f0a32951b18b3e4d6 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 15:38:18 -0800 Subject: [PATCH 13/21] lint --- certbot/tests/client_test.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 376bf5a90..f0ef077b2 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -170,7 +170,6 @@ class ClientTest(ClientTestCommon): auth_handler = self.client.auth_handler orderr = self.acme.new_order(test_csr.data) - authzr = auth_handler.handle_authorizations(orderr, False) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( @@ -205,7 +204,6 @@ class ClientTest(ClientTestCommon): auth_handler = self.client.auth_handler orderr = self.acme.new_order(test_csr.data) - authzr = auth_handler.handle_authorizations(orderr, False) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( @@ -221,7 +219,6 @@ class ClientTest(ClientTestCommon): auth_handler = self.client.auth_handler orderr = self.acme.new_order(test_csr.data) - authzr = auth_handler.handle_authorizations(orderr, False) self.assertRaises( acme_errors.Error, self.client.obtain_certificate_from_csr, From 051664a142a8c77121c79cb07307379a63d76874 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 15:39:30 -0800 Subject: [PATCH 14/21] lint --- certbot/tests/client_test.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index f0ef077b2..f10053616 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -201,7 +201,6 @@ class ClientTest(ClientTestCommon): self.acme.fetch_chain.side_effect = [acme_errors.Error, mock.sentinel.chain] test_csr = util.CSR(form="der", file=None, data=CSR_SAN) - auth_handler = self.client.auth_handler orderr = self.acme.new_order(test_csr.data) self.assertEqual( @@ -216,7 +215,6 @@ class ClientTest(ClientTestCommon): self._mock_obtain_certificate() self.acme.fetch_chain.side_effect = acme_errors.Error test_csr = util.CSR(form="der", file=None, data=CSR_SAN) - auth_handler = self.client.auth_handler orderr = self.acme.new_order(test_csr.data) self.assertRaises( From d5a90c5a6e58a068c0fbd8bf800f9953425a9693 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 15:43:27 -0800 Subject: [PATCH 15/21] delete key and csr before trying again --- certbot/client.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index d00055eae..404e1e0d9 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -325,8 +325,8 @@ class Client(object): if successful_domains != domains: if not self.config.dry_run: - # TODO: delete keys - pass + os.remove(key.file) + os.remove(csr.file) return self.obtain_certificate(successful_domains) else: certr, chain = self.obtain_certificate_from_csr(csr, orderr) From 26bcaff85cae0b38280f576ac732bd5a4744d2f8 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 15:59:58 -0800 Subject: [PATCH 16/21] add test for new_order for v2 --- acme/acme/client_test.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 11516c02f..1ba41cd7d 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -161,6 +161,21 @@ class BackwardsCompatibleClientV2Test(ClientTestBase): mock_client().register.assert_called_once_with(self.new_reg) mock_client().agree_to_tos.assert_not_called() + def test_new_order_v1(self): + self.response.json.return_value = DIRECTORY_V1.to_json() + with mock.patch('acme.client.Client') as mock_client: + client = self._init() + + def test_new_order_v2(self): + self.response.json.return_value = DIRECTORY_V2.to_json() + mock_csr_pem = mock.MagicMock() + with mock.patch('acme.client.ClientV2') as mock_client: + client = self._init() + client.new_order(mock_csr_pem) + mock_client().new_order.assert_called_once_with(mock_csr_pem) + + + class ClientTest(ClientTestBase): """Tests for acme.client.Client.""" From 65d0b9674cb4008e624c1a58cc7c8e4ee91fcdb6 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 16:01:35 -0800 Subject: [PATCH 17/21] Fix client test --- certbot/tests/client_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index f10053616..6b90eab83 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -170,6 +170,7 @@ class ClientTest(ClientTestCommon): auth_handler = self.client.auth_handler orderr = self.acme.new_order(test_csr.data) + auth_handler.handle_authorizations(orderr) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( From a7eadf88629d9cbba611731b9fe555bc0ba5cb84 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 16:08:46 -0800 Subject: [PATCH 18/21] add new order test for v1 --- acme/acme/client_test.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 1ba41cd7d..773d59aa2 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -161,10 +161,18 @@ class BackwardsCompatibleClientV2Test(ClientTestBase): mock_client().register.assert_called_once_with(self.new_reg) mock_client().agree_to_tos.assert_not_called() - def test_new_order_v1(self): + @mock.patch('OpenSSL.crypto.load_certificate_request') + @mock.patch('acme.crypto_util._pyopenssl_cert_or_req_all_names') + def test_new_order_v1(self, mock__pyopenssl_cert_or_req_all_names, + mock_load_certificate_request): self.response.json.return_value = DIRECTORY_V1.to_json() + mock__pyopenssl_cert_or_req_all_names.return_value = ['example.com', 'www.example.com'] + mock_csr_pem = mock.MagicMock() with mock.patch('acme.client.Client') as mock_client: + mock_client().request_domain_challenges.return_value = mock.sentinel.auth client = self._init() + orderr = client.new_order(mock_csr_pem) + self.assertEqual(orderr.authorizations, [mock.sentinel.auth, mock.sentinel.auth]) def test_new_order_v2(self): self.response.json.return_value = DIRECTORY_V2.to_json() From dea43e90b629f629a19e6f865fbac2930421a733 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 16:11:36 -0800 Subject: [PATCH 19/21] lint --- acme/acme/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/acme/client_test.py b/acme/acme/client_test.py index 773d59aa2..1b33ca5d7 100644 --- a/acme/acme/client_test.py +++ b/acme/acme/client_test.py @@ -164,7 +164,7 @@ class BackwardsCompatibleClientV2Test(ClientTestBase): @mock.patch('OpenSSL.crypto.load_certificate_request') @mock.patch('acme.crypto_util._pyopenssl_cert_or_req_all_names') def test_new_order_v1(self, mock__pyopenssl_cert_or_req_all_names, - mock_load_certificate_request): + unused_mock_load_certificate_request): self.response.json.return_value = DIRECTORY_V1.to_json() mock__pyopenssl_cert_or_req_all_names.return_value = ['example.com', 'www.example.com'] mock_csr_pem = mock.MagicMock() From df50f2d5fa3ddd70cfcd01cf63c305431c577d46 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 16:12:15 -0800 Subject: [PATCH 20/21] client test --- certbot/tests/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 6b90eab83..ecd77bdeb 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -170,7 +170,7 @@ class ClientTest(ClientTestCommon): auth_handler = self.client.auth_handler orderr = self.acme.new_order(test_csr.data) - auth_handler.handle_authorizations(orderr) + auth_handler.handle_authorizations(orderr, False) self.assertEqual( (mock.sentinel.certr, mock.sentinel.chain), self.client.obtain_certificate_from_csr( From d13a4ed18da3f2a0b8f88076bf15f778337259ea Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 20 Feb 2018 16:50:18 -0800 Subject: [PATCH 21/21] add tests for if partial auth success --- certbot/tests/client_test.py | 47 ++++++++++++++++++++++++++---------- 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index ecd77bdeb..f4a8a5c8a 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -147,10 +147,13 @@ class ClientTest(ClientTestCommon): self.acme.fetch_chain.return_value = mock.sentinel.chain self.acme.new_order.return_value = self.eg_order - def _check_obtain_certificate(self): - self.client.auth_handler.handle_authorizations.assert_called_once_with( - self.eg_order, - self.config.allow_subset_of_names) + def _check_obtain_certificate(self, auth_count=1): + if auth_count == 1: + self.client.auth_handler.handle_authorizations.assert_called_once_with( + self.eg_order, + self.config.allow_subset_of_names) + else: + self.assertEqual(self.client.auth_handler.handle_authorizations.call_count, auth_count) authzr = self.client.auth_handler.handle_authorizations() @@ -238,6 +241,21 @@ class ClientTest(ClientTestCommon): mock_crypto_util.init_save_csr.assert_called_once_with( mock.sentinel.key, self.eg_domains, self.config.csr_dir) + @mock.patch("certbot.client.crypto_util") + @mock.patch("os.remove") + def test_obtain_certificate_partial_success(self, mock_remove, mock_crypto_util): + csr = util.CSR(form="pem", file=mock.sentinel.csr_file, data=CSR_SAN) + key = util.CSR(form="pem", file=mock.sentinel.key_file, data=CSR_SAN) + mock_crypto_util.init_save_csr.return_value = csr + mock_crypto_util.init_save_key.return_value = key + + authzr = self._authzr_from_domains(["example.com"]) + self._test_obtain_certificate_common(key, csr, authzr_ret=authzr, auth_count=2) + + self.assertEqual(mock_crypto_util.init_save_key.call_count, 2) + self.assertEqual(mock_crypto_util.init_save_csr.call_count, 2) + self.assertEqual(mock_remove.call_count, 2) + @mock.patch("certbot.client.crypto_util") @mock.patch("certbot.client.acme_crypto_util") def test_obtain_certificate_dry_run(self, mock_acme_crypto, mock_crypto): @@ -255,22 +273,25 @@ class ClientTest(ClientTestCommon): mock_crypto.init_save_key.assert_not_called() mock_crypto.init_save_csr.assert_not_called() - def _test_obtain_certificate_common(self, key, csr): - self._mock_obtain_certificate() - - # return_value is essentially set to (None, None) in - # _mock_obtain_certificate(), which breaks this test. - # Thus fixed by the next line. - + def _authzr_from_domains(self, domains): authzr = [] # domain ordering should not be affected by authorization order - for domain in reversed(self.eg_domains): + for domain in reversed(domains): authzr.append( mock.MagicMock( body=mock.MagicMock( identifier=mock.MagicMock( value=domain)))) + return authzr + + def _test_obtain_certificate_common(self, key, csr, authzr_ret=None, auth_count=1): + self._mock_obtain_certificate() + + # return_value is essentially set to (None, None) in + # _mock_obtain_certificate(), which breaks this test. + # Thus fixed by the next line. + authzr = authzr_ret or self._authzr_from_domains(self.eg_domains) self.eg_order.authorizations = authzr self.client.auth_handler.handle_authorizations.return_value = authzr @@ -281,7 +302,7 @@ class ClientTest(ClientTestCommon): self.assertEqual( result, (mock.sentinel.certr, mock.sentinel.chain, key, csr)) - self._check_obtain_certificate() + self._check_obtain_certificate(auth_count) @mock.patch('certbot.client.Client.obtain_certificate') @mock.patch('certbot.storage.RenewableCert.new_lineage')