From 3ed3787bd8792d6ed5c58537a66425a503107721 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Sat, 2 Mar 2019 02:03:33 +0100 Subject: [PATCH] Implement Retry-After, and refactor authorization polling (#6766) Fixes #5789 This PR is about allowing Certbot to respect the Retry-After HTTP header that an ACME CA server can return to a client POSTing to a challenge, to instruct him and retry the request later. However, this feature was not easily implementable in the current code of certbot.auth_handler, because the code became really hard to read. In fact, @bmw was thinking that the code was really deceiving, and a lot of supposed functionalities declared in the comments were in fact not implemented or not functional. So I took the time to understand what was going on, and effectively, most of the code is in fact not usable or not used. Then I did a refactoring against the bare ACME spec about what to do to prepare challenges, instruct the ACME CA server to perform them, then polling regularly the authorization resources until they are decided (valid or invalid). And of course this implementation takes care of Retry-After ^^ I added a lot of comments in the new implementation, to explain what is going on for a future developer. The workflow I used is relying on the relationships between authorizations and challenges states as described in section 7.1.6 of the ACME spec draft: https://datatracker.ietf.org/doc/draft-ietf-acme-acme/ * Clean auth_handler a bit, and implement retry-after. * Remove a debug logger * Correct tests * Fix mypy and lint. Setup max retries and default retry after accordingly. * Ease a comparison in tests * Update documentation * Add tests * Adapt windows coverage threshold to the global LOC reduction * Update certbot/auth_handler.py Co-Authored-By: adferrand * Corrections under review * Correction under review * Update certbot/auth_handler.py Co-Authored-By: adferrand * Corrections under review * Update auth_handler_test.py * Reimplementing user readable report for failed authorizations * Fixes two tests * Fix another test + lint + mypy * Update auth_handler.py * Update auth_handler_test.py * Fix tests * Update certbot/auth_handler.py Co-Authored-By: adferrand * Raise directly the exception on polling timout * Improve interface documentation * Move the wait on top of the loop, to be used initially or after a new loop iteration. Do not wait for negative values. * Always display the report about failed authorizations. * Clarify an exception. * Return, instead of break * Use setdefault * Remove useless assertion * Adapt tests * Improve a test about retry after value. * Update certbot/auth_handler.py Co-Authored-By: adferrand * Add a complete test on best_effort * Add entry to the changelog * Gather all failed authzrs to be reported in one unique report in case of best_effort * Build complete warn/report/raise process about failed authzrs --- CHANGELOG.md | 3 + certbot/auth_handler.py | 392 ++++++++++------------------- certbot/interfaces.py | 13 +- certbot/plugins/common.py | 2 +- certbot/tests/auth_handler_test.py | 313 +++++++++-------------- tox.cover.py | 2 +- 6 files changed, 252 insertions(+), 473 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3eea1923d..764fc0946 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * The running of manual plugin hooks is now always included in Certbot's log output. * Tests execution for certbot, certbot-apache and certbot-nginx packages now relies on pytest. +* An ACME CA server may return a "Retry-After" HTTP header on authorization polling, as + specified in the ACME protocol, to indicate when the next polling should occur. Certbot now + reads this header if set and respect its value. * The `acme` module avoids sending the `keyAuthorization` field in the JWS payload when responding to a challenge as the field is not included in the current ACME protocol. To ease the migration path for ACME CA servers, diff --git a/certbot/auth_handler.py b/certbot/auth_handler.py index 3dfaaf26f..517b56351 100644 --- a/certbot/auth_handler.py +++ b/certbot/auth_handler.py @@ -1,29 +1,23 @@ """ACME AuthHandler.""" -import collections import logging import time +import datetime -import six import zope.component from acme import challenges from acme import messages # pylint: disable=unused-import, no-name-in-module -from acme.magic_typing import DefaultDict, Dict, List, Set, Collection +from acme.magic_typing import Dict, List # pylint: enable=unused-import, no-name-in-module from certbot import achallenges from certbot import errors from certbot import error_handler from certbot import interfaces - logger = logging.getLogger(__name__) -AnnotatedAuthzr = collections.namedtuple("AnnotatedAuthzr", ["authzr", "achalls"]) -"""Stores an authorization resource and its active annotated challenges.""" - - class AuthHandler(object): """ACME Authorization Handler for a client. @@ -47,242 +41,151 @@ class AuthHandler(object): self.account = account self.pref_challs = pref_challs - 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 - - :raises .AuthorizationError: If unable to retrieve all - authorizations - + def handle_authorizations(self, orderr, best_effort=False, max_retries=30): """ - aauthzrs = [AnnotatedAuthzr(authzr, []) - for authzr in orderr.authorizations] + Retrieve all authorizations, perform all challenges required to validate + these authorizations, then poll and wait for the authorization to be checked. + :param acme.messages.OrderResource orderr: must have authorizations filled in + :param bool best_effort: if True, not all authorizations need to be validated (eg. renew) + :param int max_retries: maximum number of retries to poll authorizations + :returns: list of all validated authorizations + :rtype: List - self._choose_challenges(aauthzrs) - config = zope.component.getUtility(interfaces.IConfig) - notify = zope.component.getUtility(interfaces.IDisplay).notification + :raises .AuthorizationError: If unable to retrieve all authorizations + """ + authzrs = orderr.authorizations[:] + if not authzrs: + raise errors.AuthorizationError('No authorization to handle.') - # While there are still challenges remaining... - while self._has_challenges(aauthzrs): - with error_handler.ExitHandler(self._cleanup_challenges, aauthzrs): - resp = self._solve_challenges(aauthzrs) - logger.info("Waiting for verification...") + # Retrieve challenges that need to be performed to validate authorizations. + achalls = self._choose_challenges(authzrs) + if not achalls: + return authzrs + + # Starting now, challenges will be cleaned at the end no matter what. + with error_handler.ExitHandler(self._cleanup_challenges, achalls): + # To begin, let's ask the authenticator plugin to perform all challenges. + try: + resps = self.auth.perform(achalls) + + # If debug is on, wait for user input before starting the verification process. + logger.info('Waiting for verification...') + config = zope.component.getUtility(interfaces.IConfig) if config.debug_challenges: + notify = zope.component.getUtility(interfaces.IDisplay).notification notify('Challenges loaded. Press continue to submit to CA. ' 'Pass "-v" for more info about challenges.', pause=True) + except errors.AuthorizationError as error: + logger.critical('Failure in setting up challenges.') + logger.info('Attempting to clean up outstanding challenges...') + raise error + # All challenges should have been processed by the authenticator. + assert len(resps) == len(achalls), 'Some challenges have not been performed.' - # Send all Responses - this modifies achalls - self._respond(aauthzrs, resp, best_effort) + # Inform the ACME CA server that challenges are available for validation. + for achall, resp in zip(achalls, resps): + self.acme.answer_challenge(achall.challb, resp) - # Just make sure all decisions are complete. - self.verify_authzr_complete(aauthzrs) + # Wait for authorizations to be checked. + self._poll_authorizations(authzrs, max_retries, best_effort) - # Only return valid authorizations - ret_val = [aauthzr.authzr for aauthzr in aauthzrs - if aauthzr.authzr.body.status == messages.STATUS_VALID] + # Keep validated authorizations only. If there is none, no certificate can be issued. + authzrs_validated = [authzr for authzr in authzrs + if authzr.body.status == messages.STATUS_VALID] + if not authzrs_validated: + raise errors.AuthorizationError('All challenges have failed.') - if not ret_val: - raise errors.AuthorizationError( - "Challenges failed for all domains") + return authzrs_validated - return ret_val + def _poll_authorizations(self, authzrs, max_retries, best_effort): + """ + Poll the ACME CA server, to wait for confirmation that authorizations have their challenges + all verified. The poll may occur several times, until all authorizations are checked + (valid or invalid), or after a maximum of retries. + """ + authzrs_to_check = {index: (authzr, None) + for index, authzr in enumerate(authzrs)} + authzrs_failed_to_report = [] + # Give an initial second to the ACME CA server to check the authorizations + sleep_seconds = 1 + for _ in range(max_retries): + # Wait for appropriate time (from Retry-After, initial wait, or no wait) + if sleep_seconds > 0: + time.sleep(sleep_seconds) + # Poll all updated authorizations. + authzrs_to_check = {index: self.acme.poll(authzr) for index, (authzr, _) + in authzrs_to_check.items()} + # Update the original list of authzr with the updated authzrs from server. + for index, (authzr, _) in authzrs_to_check.items(): + authzrs[index] = authzr - def _choose_challenges(self, aauthzrs): + # Gather failed authorizations + authzrs_failed = [authzr for authzr, _ in authzrs_to_check.values() + if authzr.body.status == messages.STATUS_INVALID] + for authzr_failed in authzrs_failed: + logger.warning('Challenge failed for domain %s', + authzr_failed.body.identifier.value) + # Accumulating all failed authzrs to build a consolidated report + # on them at the end of the polling. + authzrs_failed_to_report.extend(authzrs_failed) + + # Extract out the authorization already checked for next poll iteration. + # Poll may stop here because there is no pending authorizations anymore. + authzrs_to_check = {index: (authzr, resp) for index, (authzr, resp) + in authzrs_to_check.items() + if authzr.body.status == messages.STATUS_PENDING} + if not authzrs_to_check: + # Polling process is finished, we can leave the loop + break + + # Be merciful with the ACME server CA, check the Retry-After header returned, + # and wait this time before polling again in next loop iteration. + # From all the pending authorizations, we take the greatest Retry-After value + # to avoid polling an authorization before its relevant Retry-After value. + retry_after = max(self.acme.retry_after(resp, 3) + for _, resp in authzrs_to_check.values()) + sleep_seconds = (retry_after - datetime.datetime.now()).total_seconds() + + # In case of failed authzrs, create a report to the user. + if authzrs_failed_to_report: + _report_failed_authzrs(authzrs_failed_to_report, self.account.key) + if not best_effort: + # Without best effort, having failed authzrs is critical and fail the process. + raise errors.AuthorizationError('Some challenges have failed.') + + if authzrs_to_check: + # Here authzrs_to_check is still not empty, meaning we exceeded the max polling attempt. + raise errors.AuthorizationError('All authorizations were not finalized by the CA.') + + def _choose_challenges(self, authzrs): """ Retrieve necessary and pending challenges to satisfy server. NB: Necessary and already validated challenges are not retrieved, as they can be reused for a certificate issuance. """ - pending_authzrs = [aauthzr for aauthzr in aauthzrs - if aauthzr.authzr.body.status != messages.STATUS_VALID] + pending_authzrs = [authzr for authzr in authzrs + if authzr.body.status != messages.STATUS_VALID] + achalls = [] # type: List[achallenges.AnnotatedChallenge] if pending_authzrs: logger.info("Performing the following challenges:") - for aauthzr in pending_authzrs: - aauthzr_challenges = aauthzr.authzr.body.challenges + for authzr in pending_authzrs: + authzr_challenges = authzr.body.challenges if self.acme.acme_version == 1: - combinations = aauthzr.authzr.body.combinations + combinations = authzr.body.combinations else: - combinations = tuple((i,) for i in range(len(aauthzr_challenges))) + combinations = tuple((i,) for i in range(len(authzr_challenges))) path = gen_challenge_path( - aauthzr_challenges, - self._get_chall_pref(aauthzr.authzr.body.identifier.value), + authzr_challenges, + self._get_chall_pref(authzr.body.identifier.value), combinations) - aauthzr_achalls = self._challenge_factory( - aauthzr.authzr, path) - aauthzr.achalls.extend(aauthzr_achalls) + achalls.extend(self._challenge_factory(authzr, path)) - for aauthzr in aauthzrs: - for achall in aauthzr.achalls: - if isinstance(achall.chall, challenges.TLSSNI01): - logger.warning("TLS-SNI-01 is deprecated, and will stop working soon.") - return + if any(isinstance(achall.chall, challenges.TLSSNI01) for achall in achalls): + logger.warning("TLS-SNI-01 is deprecated, and will stop working soon.") - def _has_challenges(self, aauthzrs): - """Do we have any challenges to perform?""" - return any(aauthzr.achalls for aauthzr in aauthzrs) - - def _solve_challenges(self, aauthzrs): - """Get Responses for challenges from authenticators.""" - resp = [] # type: Collection[challenges.ChallengeResponse] - all_achalls = self._get_all_achalls(aauthzrs) - try: - if all_achalls: - resp = self.auth.perform(all_achalls) - except errors.AuthorizationError: - logger.critical("Failure in setting up challenges.") - logger.info("Attempting to clean up outstanding challenges...") - raise - - assert len(resp) == len(all_achalls) - - return resp - - def _get_all_achalls(self, aauthzrs): - """Return all active challenges.""" - all_achalls = [] # type: Collection[challenges.ChallengeResponse] - for aauthzr in aauthzrs: - all_achalls.extend(aauthzr.achalls) - return all_achalls - - def _respond(self, aauthzrs, resp, best_effort): - """Send/Receive confirmation of all challenges. - - .. note:: This method also cleans up the auth_handler state. - - """ - # TODO: chall_update is a dirty hack to get around acme-spec #105 - chall_update = dict() \ - # type: Dict[int, List[achallenges.KeyAuthorizationAnnotatedChallenge]] - self._send_responses(aauthzrs, resp, chall_update) - - # Check for updated status... - self._poll_challenges(aauthzrs, chall_update, best_effort) - - def _send_responses(self, aauthzrs, resps, chall_update): - """Send responses and make sure errors are handled. - - :param aauthzrs: authorizations and the selected annotated challenges - to try and perform - :type aauthzrs: `list` of `AnnotatedAuthzr` - :param resps: challenge responses from the authenticator where - each response at index i corresponds to the annotated - challenge at index i in the list returned by - :func:`_get_all_achalls` - :type resps: `collections.abc.Iterable` of - :class:`~acme.challenges.ChallengeResponse` or `False` or - `None` - :param dict chall_update: parameter that is updated to hold - aauthzr index to list of outstanding solved annotated challenges - - """ - active_achalls = [] - resps_iter = iter(resps) - for i, aauthzr in enumerate(aauthzrs): - for achall in aauthzr.achalls: - # This line needs to be outside of the if block below to - # ensure failed challenges are cleaned up correctly - active_achalls.append(achall) - - resp = next(resps_iter) - # Don't send challenges for None and False authenticator responses - if resp: - self.acme.answer_challenge(achall.challb, resp) - # TODO: answer_challenge returns challr, with URI, - # that can be used in _find_updated_challr - # comparisons... - chall_update.setdefault(i, []).append(achall) - - return active_achalls - - def _poll_challenges(self, aauthzrs, chall_update, - best_effort, min_sleep=3, max_rounds=30): - """Wait for all challenge results to be determined.""" - indices_to_check = set(chall_update.keys()) - comp_indices = set() - rounds = 0 - - while indices_to_check and rounds < max_rounds: - # TODO: Use retry-after... - time.sleep(min_sleep) - all_failed_achalls = set() # type: Set[achallenges.KeyAuthorizationAnnotatedChallenge] - for index in indices_to_check: - comp_achalls, failed_achalls = self._handle_check( - aauthzrs, index, chall_update[index]) - - if len(comp_achalls) == len(chall_update[index]): - comp_indices.add(index) - elif not failed_achalls: - for achall, _ in comp_achalls: - chall_update[index].remove(achall) - # We failed some challenges... damage control - else: - if best_effort: - comp_indices.add(index) - logger.warning( - "Challenge failed for domain %s", - aauthzrs[index].authzr.body.identifier.value) - else: - all_failed_achalls.update( - updated for _, updated in failed_achalls) - - if all_failed_achalls: - _report_failed_challs(all_failed_achalls) - raise errors.FailedChallenges(all_failed_achalls) - - indices_to_check -= comp_indices - comp_indices.clear() - rounds += 1 - - def _handle_check(self, aauthzrs, index, achalls): - """Returns tuple of ('completed', 'failed').""" - completed = [] - failed = [] - - original_aauthzr = aauthzrs[index] - updated_authzr, _ = self.acme.poll(original_aauthzr.authzr) - aauthzrs[index] = AnnotatedAuthzr(updated_authzr, original_aauthzr.achalls) - if updated_authzr.body.status == messages.STATUS_VALID: - return achalls, [] - - # Note: if the whole authorization is invalid, the individual failed - # challenges will be determined here... - for achall in achalls: - updated_achall = achall.update(challb=self._find_updated_challb( - updated_authzr, achall)) - - # This does nothing for challenges that have yet to be decided yet. - if updated_achall.status == messages.STATUS_VALID: - completed.append((achall, updated_achall)) - elif updated_achall.status == messages.STATUS_INVALID: - failed.append((achall, updated_achall)) - - return completed, failed - - def _find_updated_challb(self, authzr, achall): # pylint: disable=no-self-use - """Find updated challenge body within Authorization Resource. - - .. warning:: This assumes only one instance of type of challenge in - each challenge resource. - - :param .AuthorizationResource authzr: Authorization Resource - :param .AnnotatedChallenge achall: Annotated challenge for which - to get status - - """ - for authzr_challb in authzr.body.challenges: - if type(authzr_challb.chall) is type(achall.challb.chall): # noqa - return authzr_challb - raise errors.AuthorizationError( - "Target challenge not found in authorization resource") + return achalls def _get_chall_pref(self, domain): """Return list of challenge preferences. @@ -306,43 +209,15 @@ class AuthHandler(object): chall_prefs.extend(plugin_pref) return chall_prefs - def _cleanup_challenges(self, aauthzrs, achalls=None): + def _cleanup_challenges(self, achalls): """Cleanup challenges. - :param aauthzrs: authorizations and their selected annotated - challenges - :type aauthzrs: `list` of `AnnotatedAuthzr` :param achalls: annotated challenges to cleanup :type achalls: `list` of :class:`certbot.achallenges.AnnotatedChallenge` """ logger.info("Cleaning up challenges") - if achalls is None: - achalls = self._get_all_achalls(aauthzrs) - if achalls: - self.auth.cleanup(achalls) - for achall in achalls: - for aauthzr in aauthzrs: - if achall in aauthzr.achalls: - aauthzr.achalls.remove(achall) - break - - def verify_authzr_complete(self, aauthzrs): - """Verifies that all authorizations have been decided. - - :param aauthzrs: authorizations and their selected annotated - challenges - :type aauthzrs: `list` of `AnnotatedAuthzr` - - :returns: Whether all authzr are complete - :rtype: bool - - """ - for aauthzr in aauthzrs: - authzr = aauthzr.authzr - if (authzr.body.status != messages.STATUS_VALID and - authzr.body.status != messages.STATUS_INVALID): - raise errors.AuthorizationError("Incomplete authorizations") + self.auth.cleanup(achalls) def _challenge_factory(self, authzr, path): """Construct Namedtuple Challenges @@ -530,22 +405,19 @@ _ERROR_HELP = { } -def _report_failed_challs(failed_achalls): - """Notifies the user about failed challenges. +def _report_failed_authzrs(failed_authzrs, account_key): + """Notifies the user about failed authorizations.""" + problems = {} # type: Dict[str, List[achallenges.AnnotatedChallenge]] + failed_achalls = [challb_to_achall(challb, account_key, authzr.body.identifier.value) + for authzr in failed_authzrs for challb in authzr.body.challenges + if challb.error] - :param set failed_achalls: A set of failed - :class:`certbot.achallenges.AnnotatedChallenge`. - - """ - problems = collections.defaultdict(list)\ - # type: DefaultDict[str, List[achallenges.KeyAuthorizationAnnotatedChallenge]] for achall in failed_achalls: - if achall.error: - problems[achall.error.typ].append(achall) + problems.setdefault(achall.error.typ, []).append(achall) + reporter = zope.component.getUtility(interfaces.IReporter) - for achalls in six.itervalues(problems): - reporter.add_message( - _generate_failed_chall_msg(achalls), reporter.MEDIUM_PRIORITY) + for achalls in problems.values(): + reporter.add_message(_generate_failed_chall_msg(achalls), reporter.MEDIUM_PRIORITY) def _generate_failed_chall_msg(failed_achalls): diff --git a/certbot/interfaces.py b/certbot/interfaces.py index bb9a91b0f..bd91d2272 100644 --- a/certbot/interfaces.py +++ b/certbot/interfaces.py @@ -159,21 +159,14 @@ class IAuthenticator(IPlugin): :func:`get_chall_pref` only. :returns: `collections.Iterable` of ACME - :class:`~acme.challenges.ChallengeResponse` instances - or if the :class:`~acme.challenges.Challenge` cannot - be fulfilled then: - - ``None`` - Authenticator can perform challenge, but not at this time. - ``False`` - Authenticator will never be able to perform (error). - + :class:`~acme.challenges.ChallengeResponse` instances corresponding to each provided + :class:`~acme.challenges.Challenge`. :rtype: :class:`collections.Iterable` of :class:`acme.challenges.ChallengeResponse`, where responses are required to be returned in the same order as corresponding input challenges - :raises .PluginError: If challenges cannot be performed + :raises .PluginError: If some or all challenges cannot be performed """ diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index ee1af4978..c14129d87 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -351,7 +351,7 @@ class ChallengePerformer(object): def perform(self): """Perform all added challenges. - :returns: challenge respones + :returns: challenge responses :rtype: `list` of `acme.challenges.KeyAuthorizationChallengeResponse` diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index fe0ece12e..353c34da2 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -4,13 +4,11 @@ import logging import unittest import mock -import six import zope.component from acme import challenges from acme import client as acme_client from acme import messages -from acme.magic_typing import Dict # pylint: disable=unused-import, no-name-in-module from certbot import achallenges from certbot import errors @@ -82,6 +80,7 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.mock_account = mock.Mock(key=util.Key("file_path", "PEM")) self.mock_net = mock.MagicMock(spec=acme_client.Client) self.mock_net.acme_version = 1 + self.mock_net.retry_after.side_effect = acme_client.Client.retry_after self.handler = AuthHandler( self.mock_auth, self.mock_net, self.mock_account, []) @@ -95,23 +94,26 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p authzr = gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES, combos=combos) mock_order = mock.MagicMock(authorizations=[authzr]) - with mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") as mock_poll: - mock_poll.side_effect = self._validate_all + self.mock_net.poll.side_effect = _gen_mock_on_poll(retry=1, wait_value=30) + with mock.patch('certbot.auth_handler.time') as mock_time: authzr = self.handler.handle_authorizations(mock_order) - self.assertEqual(self.mock_net.answer_challenge.call_count, 1) + self.assertEqual(self.mock_net.answer_challenge.call_count, 1) - self.assertEqual(mock_poll.call_count, 1) - chall_update = mock_poll.call_args[0][1] - self.assertEqual(list(six.iterkeys(chall_update)), [0]) - self.assertEqual(len(chall_update.values()), 1) + self.assertEqual(self.mock_net.poll.call_count, 2) # Because there is one retry + self.assertEqual(mock_time.sleep.call_count, 2) + # Retry-After header is 30 seconds, but at the time sleep is invoked, several + # instructions are executed, and next pool is in less than 30 seconds. + self.assertTrue(mock_time.sleep.call_args_list[1][0][0] <= 30) + # However, assert that we did not took the default value of 3 seconds. + self.assertTrue(mock_time.sleep.call_args_list[1][0][0] > 3) - self.assertEqual(self.mock_auth.cleanup.call_count, 1) - # Test if list first element is TLSSNI01, use typ because it is an achall - self.assertEqual( - self.mock_auth.cleanup.call_args[0][0][0].typ, "tls-sni-01") + self.assertEqual(self.mock_auth.cleanup.call_count, 1) + # Test if list first element is TLSSNI01, use typ because it is an achall + self.assertEqual( + self.mock_auth.cleanup.call_args[0][0][0].typ, "tls-sni-01") - self.assertEqual(len(authzr), 1) + self.assertEqual(len(authzr), 1) def test_name1_tls_sni_01_1_acme_1(self): self._test_name1_tls_sni_01_1_common(combos=True) @@ -120,9 +122,8 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.mock_net.acme_version = 2 self._test_name1_tls_sni_01_1_common(combos=False) - @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - def test_name1_tls_sni_01_1_http_01_1_dns_1_acme_1(self, mock_poll): - mock_poll.side_effect = self._validate_all + def test_name1_tls_sni_01_1_http_01_1_dns_1_acme_1(self): + self.mock_net.poll.side_effect = _gen_mock_on_poll() self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) self.mock_auth.get_chall_pref.return_value.append(challenges.DNS01) @@ -132,10 +133,7 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.assertEqual(self.mock_net.answer_challenge.call_count, 3) - self.assertEqual(mock_poll.call_count, 1) - chall_update = mock_poll.call_args[0][1] - self.assertEqual(list(six.iterkeys(chall_update)), [0]) - self.assertEqual(len(chall_update.values()), 1) + self.assertEqual(self.mock_net.poll.call_count, 1) self.assertEqual(self.mock_auth.cleanup.call_count, 1) # Test if list first element is TLSSNI01, use typ because it is an achall @@ -145,10 +143,9 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p # Length of authorizations list self.assertEqual(len(authzr), 1) - @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - def test_name1_tls_sni_01_1_http_01_1_dns_1_acme_2(self, mock_poll): + def test_name1_tls_sni_01_1_http_01_1_dns_1_acme_2(self): self.mock_net.acme_version = 2 - mock_poll.side_effect = self._validate_all + self.mock_net.poll.side_effect = _gen_mock_on_poll() self.mock_auth.get_chall_pref.return_value.append(challenges.HTTP01) self.mock_auth.get_chall_pref.return_value.append(challenges.DNS01) @@ -158,10 +155,7 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.assertEqual(self.mock_net.answer_challenge.call_count, 1) - self.assertEqual(mock_poll.call_count, 1) - chall_update = mock_poll.call_args[0][1] - self.assertEqual(list(six.iterkeys(chall_update)), [0]) - self.assertEqual(len(chall_update.values()), 1) + self.assertEqual(self.mock_net.poll.call_count, 1) self.assertEqual(self.mock_auth.cleanup.call_count, 1) cleaned_up_achalls = self.mock_auth.cleanup.call_args[0][0] @@ -175,27 +169,18 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.mock_net.request_domain_challenges.side_effect = functools.partial( gen_dom_authzr, challs=acme_util.CHALLENGES, combos=combos) - authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES), gen_dom_authzr(domain="1", challs=acme_util.CHALLENGES), gen_dom_authzr(domain="2", challs=acme_util.CHALLENGES)] mock_order = mock.MagicMock(authorizations=authzrs) - with mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") as mock_poll: - mock_poll.side_effect = self._validate_all - authzr = self.handler.handle_authorizations(mock_order) + + self.mock_net.poll.side_effect = _gen_mock_on_poll() + authzr = self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_net.answer_challenge.call_count, 3) # Check poll call - self.assertEqual(mock_poll.call_count, 1) - chall_update = mock_poll.call_args[0][1] - self.assertEqual(len(list(six.iterkeys(chall_update))), 3) - self.assertTrue(0 in list(six.iterkeys(chall_update))) - self.assertEqual(len(chall_update[0]), 1) - self.assertTrue(1 in list(six.iterkeys(chall_update))) - self.assertEqual(len(chall_update[1]), 1) - self.assertTrue(2 in list(six.iterkeys(chall_update))) - self.assertEqual(len(chall_update[2]), 1) + self.assertEqual(self.mock_net.poll.call_count, 3) self.assertEqual(self.mock_auth.cleanup.call_count, 1) @@ -208,14 +193,13 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.mock_net.acme_version = 2 self._test_name3_tls_sni_01_3_common(combos=False) - @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - def test_debug_challenges(self, mock_poll): + def test_debug_challenges(self): zope.component.provideUtility( mock.Mock(debug_challenges=True), interfaces.IConfig) 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_net.poll.side_effect = _gen_mock_on_poll() self.handler.handle_authorizations(mock_order) @@ -231,6 +215,18 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.assertRaises( errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + def test_max_retries_exceeded(self): + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] + mock_order = mock.MagicMock(authorizations=authzrs) + + # We will return STATUS_PENDING twice before returning STATUS_VALID. + self.mock_net.poll.side_effect = _gen_mock_on_poll(retry=2) + + with self.assertRaises(errors.AuthorizationError) as error: + # We retry only once, so retries will be exhausted before STATUS_VALID is returned. + self.handler.handle_authorizations(mock_order, False, 1) + self.assertTrue('All authorizations were not finalized by the CA.' in str(error.exception)) + def test_no_domains(self): mock_order = mock.MagicMock(authorizations=[]) self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations, mock_order) @@ -244,9 +240,8 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.handler.pref_challs.extend((challenges.HTTP01.typ, challenges.DNS01.typ,)) - with mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") as mock_poll: - mock_poll.side_effect = self._validate_all - self.handler.handle_authorizations(mock_order) + self.mock_net.poll.side_effect = _gen_mock_on_poll() + self.handler.handle_authorizations(mock_order) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( @@ -290,11 +285,11 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.assertEqual( self.mock_auth.cleanup.call_args[0][0][0].typ, "tls-sni-01") - @mock.patch("certbot.auth_handler.AuthHandler._respond") - def test_respond_error(self, mock_respond): + def test_answer_error(self): + self.mock_net.answer_challenge.side_effect = errors.AuthorizationError + authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] mock_order = mock.MagicMock(authorizations=authzrs) - mock_respond.side_effect = errors.AuthorizationError self.assertRaises( errors.AuthorizationError, self.handler.handle_authorizations, mock_order) @@ -302,20 +297,52 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p self.assertEqual( self.mock_auth.cleanup.call_args[0][0][0].typ, "tls-sni-01") - @mock.patch("certbot.auth_handler.AuthHandler._poll_challenges") - @mock.patch("certbot.auth_handler.AuthHandler.verify_authzr_complete") - def test_incomplete_authzr_error(self, mock_verify, mock_poll): + def test_incomplete_authzr_error(self): authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)] mock_order = mock.MagicMock(authorizations=authzrs) - mock_verify.side_effect = errors.AuthorizationError - mock_poll.side_effect = self._validate_all + self.mock_net.poll.side_effect = _gen_mock_on_poll(status=messages.STATUS_INVALID) - self.assertRaises( - errors.AuthorizationError, self.handler.handle_authorizations, mock_order) + with test_util.patch_get_utility(): + with self.assertRaises(errors.AuthorizationError) as error: + self.handler.handle_authorizations(mock_order, False) + self.assertTrue('Some challenges have failed.' in str(error.exception)) self.assertEqual(self.mock_auth.cleanup.call_count, 1) self.assertEqual( self.mock_auth.cleanup.call_args[0][0][0].typ, "tls-sni-01") + def test_best_effort(self): + def _conditional_mock_on_poll(authzr): + """This mock will invalidate one authzr, and invalidate the other one""" + valid_mock = _gen_mock_on_poll(messages.STATUS_VALID) + invalid_mock = _gen_mock_on_poll(messages.STATUS_INVALID) + + if authzr.body.identifier.value == 'will-be-invalid': + return invalid_mock(authzr) + return valid_mock(authzr) + + # Two authzrs. Only one will be valid. + authzrs = [gen_dom_authzr(domain="will-be-valid", challs=acme_util.CHALLENGES), + gen_dom_authzr(domain="will-be-invalid", challs=acme_util.CHALLENGES)] + self.mock_net.poll.side_effect = _conditional_mock_on_poll + + mock_order = mock.MagicMock(authorizations=authzrs) + + with mock.patch('certbot.auth_handler._report_failed_authzrs') as mock_report: + valid_authzr = self.handler.handle_authorizations(mock_order, True) + + # Because best_effort=True, we did not blow up. Instead ... + self.assertEqual(len(valid_authzr), 1) # ... the valid authzr has been processed + self.assertEqual(mock_report.call_count, 1) # ... the invalid authzr has been reported + + self.mock_net.poll.side_effect = _gen_mock_on_poll(status=messages.STATUS_INVALID) + + with test_util.patch_get_utility(): + with self.assertRaises(errors.AuthorizationError) as error: + self.handler.handle_authorizations(mock_order, True) + + # Despite best_effort=True, process will fail because no authzr is valid. + self.assertTrue('All challenges have failed.' in str(error.exception)) + def test_validated_challenge_not_rerun(self): # With pending challenge, we expect the challenge to be tried, and fail. authzr = acme_util.gen_authzr( @@ -334,138 +361,26 @@ class HandleAuthorizationsTest(unittest.TestCase): # pylint: disable=too-many-p mock_order = mock.MagicMock(authorizations=[authzr]) self.handler.handle_authorizations(mock_order) - def _validate_all(self, aauthzrs, unused_1, unused_2): - for i, aauthzr in enumerate(aauthzrs): - azr = aauthzr.authzr - updated_azr = acme_util.gen_authzr( - messages.STATUS_VALID, - azr.body.identifier.value, - [challb.chall for challb in azr.body.challenges], - [messages.STATUS_VALID] * len(azr.body.challenges), - azr.body.combinations) - aauthzrs[i] = type(aauthzr)(updated_azr, aauthzr.achalls) - @mock.patch("certbot.auth_handler.logger") def test_tls_sni_logs(self, logger): self._test_name1_tls_sni_01_1_common(combos=True) self.assertTrue("deprecated" in logger.warning.call_args[0][0]) -class PollChallengesTest(unittest.TestCase): - # pylint: disable=protected-access - """Test poll challenges.""" +def _gen_mock_on_poll(status=messages.STATUS_VALID, retry=0, wait_value=1): + state = {'count': retry} - def setUp(self): - from certbot.auth_handler import challb_to_achall - from certbot.auth_handler import AuthHandler, AnnotatedAuthzr - - # Account and network are mocked... - self.mock_net = mock.MagicMock() - self.handler = AuthHandler( - None, self.mock_net, mock.Mock(key="mock_key"), []) - - self.doms = ["0", "1", "2"] - self.aauthzrs = [ - AnnotatedAuthzr(acme_util.gen_authzr( - messages.STATUS_PENDING, self.doms[0], - [acme_util.HTTP01, acme_util.TLSSNI01], - [messages.STATUS_PENDING] * 2, False), []), - AnnotatedAuthzr(acme_util.gen_authzr( - messages.STATUS_PENDING, self.doms[1], - acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), []), - AnnotatedAuthzr(acme_util.gen_authzr( - messages.STATUS_PENDING, self.doms[2], - acme_util.CHALLENGES, [messages.STATUS_PENDING] * 3, False), []) - ] - - self.chall_update = {} # type: Dict[int, achallenges.KeyAuthorizationAnnotatedChallenge] - for i, aauthzr in enumerate(self.aauthzrs): - self.chall_update[i] = [ - challb_to_achall(challb, mock.Mock(key="dummy_key"), self.doms[i]) - for challb in aauthzr.authzr.body.challenges] - - - @mock.patch("certbot.auth_handler.time") - def test_poll_challenges(self, unused_mock_time): - self.mock_net.poll.side_effect = self._mock_poll_solve_one_valid - self.handler._poll_challenges(self.aauthzrs, self.chall_update, False) - - for aauthzr in self.aauthzrs: - self.assertEqual(aauthzr.authzr.body.status, messages.STATUS_VALID) - - @mock.patch("certbot.auth_handler.time") - def test_poll_challenges_failure_best_effort(self, unused_mock_time): - self.mock_net.poll.side_effect = self._mock_poll_solve_one_invalid - self.handler._poll_challenges(self.aauthzrs, self.chall_update, True) - - for aauthzr in self.aauthzrs: - self.assertEqual(aauthzr.authzr.body.status, messages.STATUS_PENDING) - - @mock.patch("certbot.auth_handler.time") - @test_util.patch_get_utility() - def test_poll_challenges_failure(self, unused_mock_time, unused_mock_zope): - self.mock_net.poll.side_effect = self._mock_poll_solve_one_invalid - self.assertRaises( - errors.AuthorizationError, self.handler._poll_challenges, - self.aauthzrs, self.chall_update, False) - - @mock.patch("certbot.auth_handler.time") - def test_unable_to_find_challenge_status(self, unused_mock_time): - from certbot.auth_handler import challb_to_achall - self.mock_net.poll.side_effect = self._mock_poll_solve_one_valid - self.chall_update[0].append( - challb_to_achall(acme_util.DNS01_P, "key", self.doms[0])) - self.assertRaises( - errors.AuthorizationError, self.handler._poll_challenges, - self.aauthzrs, self.chall_update, False) - - def test_verify_authzr_failure(self): - self.assertRaises(errors.AuthorizationError, - self.handler.verify_authzr_complete, self.aauthzrs) - - def _mock_poll_solve_one_valid(self, authzr): - # Pending here because my dummy script won't change the full status. - # Basically it didn't raise an error and it stopped earlier than - # Making all challenges invalid which would make mock_poll_solve_one - # change authzr to invalid - return self._mock_poll_solve_one_chall(authzr, messages.STATUS_VALID) - - def _mock_poll_solve_one_invalid(self, authzr): - return self._mock_poll_solve_one_chall(authzr, messages.STATUS_INVALID) - - def _mock_poll_solve_one_chall(self, authzr, desired_status): - # pylint: disable=no-self-use - """Dummy method that solves one chall at a time to desired_status. - - When all are solved.. it changes authzr.status to desired_status - - """ - new_challbs = authzr.body.challenges - for challb in authzr.body.challenges: - if challb.status != desired_status: - new_challbs = tuple( - challb_temp if challb_temp != challb - else acme_util.chall_to_challb(challb.chall, desired_status) - for challb_temp in authzr.body.challenges - ) - break - - if all(test_challb.status == desired_status - for test_challb in new_challbs): - status_ = desired_status - else: - status_ = authzr.body.status - - new_authzr = messages.AuthorizationResource( - uri=authzr.uri, - body=messages.Authorization( - identifier=authzr.body.identifier, - challenges=new_challbs, - combinations=authzr.body.combinations, - status=status_, - ), - ) - return (new_authzr, "response") + def _mock(authzr): + state['count'] = state['count'] - 1 + effective_status = status if state['count'] < 0 else messages.STATUS_PENDING + updated_azr = acme_util.gen_authzr( + effective_status, + authzr.body.identifier.value, + [challb.chall for challb in authzr.body.challenges], + [effective_status] * len(authzr.body.challenges), + authzr.body.combinations) + return updated_azr, mock.MagicMock(headers={'Retry-After': str(wait_value)}) + return _mock class ChallbToAchallTest(unittest.TestCase): @@ -527,8 +442,8 @@ class GenChallengePathTest(unittest.TestCase): errors.AuthorizationError, self._call, challbs, prefs, None) -class ReportFailedChallsTest(unittest.TestCase): - """Tests for certbot.auth_handler._report_failed_challs.""" +class ReportFailedAuthzrsTest(unittest.TestCase): + """Tests for certbot.auth_handler._report_failed_authzrs.""" # pylint: disable=protected-access def setUp(self): @@ -542,31 +457,27 @@ class ReportFailedChallsTest(unittest.TestCase): # Prevent future regressions if the error type changes self.assertTrue(kwargs["error"].description is not None) - self.http01 = achallenges.KeyAuthorizationAnnotatedChallenge( - # pylint: disable=star-args - challb=messages.ChallengeBody(**kwargs), - domain="example.com", - account_key="key") + http_01 = messages.ChallengeBody(**kwargs) # pylint: disable=star-args kwargs["chall"] = acme_util.TLSSNI01 - self.tls_sni_same = achallenges.KeyAuthorizationAnnotatedChallenge( - # pylint: disable=star-args - challb=messages.ChallengeBody(**kwargs), - domain="example.com", - account_key="key") + tls_sni_01 = messages.ChallengeBody(**kwargs) # pylint: disable=star-args + + self.authzr1 = mock.MagicMock() + self.authzr1.body.identifier.value = 'example.com' + self.authzr1.body.challenges = [http_01, tls_sni_01] kwargs["error"] = messages.Error(typ="dnssec", detail="detail") - self.tls_sni_diff = achallenges.KeyAuthorizationAnnotatedChallenge( - # pylint: disable=star-args - challb=messages.ChallengeBody(**kwargs), - domain="foo.bar", - account_key="key") + tls_sni_01_diff = messages.ChallengeBody(**kwargs) # pylint: disable=star-args + + self.authzr2 = mock.MagicMock() + self.authzr2.body.identifier.value = 'foo.bar' + self.authzr2.body.challenges = [tls_sni_01_diff] @test_util.patch_get_utility() def test_same_error_and_domain(self, mock_zope): from certbot import auth_handler - auth_handler._report_failed_challs([self.http01, self.tls_sni_same]) + auth_handler._report_failed_authzrs([self.authzr1], 'key') call_list = mock_zope().add_message.call_args_list self.assertTrue(len(call_list) == 1) self.assertTrue("Domain: example.com\nType: tls\nDetail: detail" in call_list[0][0][0]) @@ -575,7 +486,7 @@ class ReportFailedChallsTest(unittest.TestCase): def test_different_errors_and_domains(self, mock_zope): from certbot import auth_handler - auth_handler._report_failed_challs([self.http01, self.tls_sni_diff]) + auth_handler._report_failed_authzrs([self.authzr1, self.authzr2], 'key') self.assertTrue(mock_zope().add_message.call_count == 2) diff --git a/tox.cover.py b/tox.cover.py index e323ba255..d0f97626a 100755 --- a/tox.cover.py +++ b/tox.cover.py @@ -12,7 +12,7 @@ DEFAULT_PACKAGES = [ 'certbot_dns_sakuracloud', 'certbot_nginx', 'certbot_postfix', 'letshelp_certbot'] COVER_THRESHOLDS = { - 'certbot': {'linux': 98, 'windows': 94}, + 'certbot': {'linux': 98, 'windows': 93}, 'acme': {'linux': 100, 'windows': 99}, 'certbot_apache': {'linux': 100, 'windows': 100}, 'certbot_dns_cloudflare': {'linux': 98, 'windows': 98},