Repository: certbot/certbot

Issue:
Certbot should respect Retry-After when polling authzs

Discussion:
bmw: @jsha, one change being made in this PR is we no longer sleep before beginning to poll authorizations.

Simplified prior pseudocode was:
```
Respond to challenges
while authorizations are pending:
    sleep 3s
    poll authorizations
```
Pseudocode for this PR is:
```
Respond to challenges
while true:
    poll authorizations
    if no pending authorizations:
        break
    if retry-after is set:
        sleep maximum retry-after value
    else:
        sleep 3s
```
Do you have concerns about us removing this initial sleep?
adferrand: I added back the formatted report for failed authorizations.
jsha: > Do you have concerns about us removing this initial sleep?

It would probably be better to stick it back in since it will almost never result in a "valid" result. You're welcome to make it shorter if you like -- I think most validations should complete in under 1s in ideal circumstances.
adferrand: I added a first second before the first polling. 
adferrand: And changelog updated.
adferrand: I have a failure on a test unrelated on this PR, on for Python 3 tests only (on 2.7 it is passing). I do not know what is going on.
bmw: The test that failed is testing some particularly nasty code which uses global state. I'm surprised your PR is consistently reproducing the error but I'll write a quick patch which should hopefully fix it.
bmw: See #6799.
adferrand: OK @bmw, your fix is fixing my PR!

Patch:
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]
-
-        self._choose_challenges(aauthzrs)
-        config = zope.component.getUtility(interfaces.IConfig)
-        notify = zope.component.getUtility(interfaces.IDisplay).notification
-
-        # 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 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
+
+        :raises .AuthorizationError: If unable to retrieve all authorizations
+        """
+        authzrs = orderr.authorizations[:]
+        if not authzrs:
+            raise errors.AuthorizationError('No authorization to handle.')
+
+        # 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)
-
-        # Just make sure all decisions are complete.
-        self.verify_authzr_complete(aauthzrs)
+            # 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)
 
-        # Only return valid authorizations
-        ret_val = [aauthzr.authzr for aauthzr in aauthzrs
-                   if aauthzr.authzr.body.status == messages.STATUS_VALID]
+            # Wait for authorizations to be checked.
+            self._poll_authorizations(authzrs, max_retries, best_effort)
 
-        if not ret_val:
-            raise errors.AuthorizationError(
-                "Challenges failed for all domains")
+            # 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.')
 
-        return ret_val
+            return authzrs_validated
 
-    def _choose_challenges(self, aauthzrs):
+    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
+
+            # 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)
-
-        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
-
-    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
+            achalls.extend(self._challenge_factory(authzr, path))
 
-        assert len(resp) == len(all_achalls)
+        if any(isinstance(achall.chall, challenges.TLSSNI01) for achall in achalls):
+            logger.warning("TLS-SNI-01 is deprecated, and will stop working soon.")
 
-        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/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},


Test patch:
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)