limit challenge polling to 30 minutes (#9527)

* limit challenge polling to 30 minutes

* Fix docstring typo

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

Co-authored-by: Brad Warren <bmw@users.noreply.github.com>

certbot/CHANGELOG.md

@@ -10,7 +10,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Changed
 
-*
+* Certbot will no longer respect very long challenge polling intervals, which may be suggested
+  by some ACME servers. Certbot will continue to wait up to 90 seconds by default, or up to a
+  total of 30 minutes if requested by the server via `Retry-After`.
 
 ### Fixed
 

certbot/certbot/_internal/auth_handler.py

@@ -55,7 +55,8 @@ class AuthHandler:
 
     def handle_authorizations(self, orderr: messages.OrderResource,
                               config: configuration.NamespaceConfig, best_effort: bool = False,
-                              max_retries: int = 30) -> List[messages.AuthorizationResource]:
+                              max_retries: int = 30,
+                              max_time_mins: float = 30) -> List[messages.AuthorizationResource]:
         """
         Retrieve all authorizations, perform all challenges required to validate
         these authorizations, then poll and wait for the authorization to be checked.
@@ -63,6 +64,7 @@ class AuthHandler:
         :param certbot.configuration.NamespaceConfig config: current Certbot configuration
         :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
+        :param float max_time_mins: maximum time (in minutes) to poll authorizations
         :returns: list of all validated authorizations
         :rtype: List
 
@@ -103,7 +105,7 @@ class AuthHandler:
 
             # Wait for authorizations to be checked.
             logger.info('Waiting for verification...')
-            self._poll_authorizations(authzrs, max_retries, best_effort)
+            self._poll_authorizations(authzrs, max_retries, max_time_mins, best_effort)
 
             # Keep validated authorizations only. If there is none, no certificate can be issued.
             authzrs_validated = [authzr for authzr in authzrs
@@ -143,11 +145,11 @@ class AuthHandler:
         return (deactivated, failed)
 
     def _poll_authorizations(self, authzrs: List[messages.AuthorizationResource], max_retries: int,
-                             best_effort: bool) -> None:
+                             deadline_minutes: float, best_effort: bool) -> None:
         """
         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.
+        (valid or invalid), or a maximum of retries, or the polling deadline is reached.
         """
         if not self.acme:
             raise errors.Error("No ACME client defined, cannot poll authorizations.")
@@ -156,6 +158,7 @@ class AuthHandler:
                                           Optional[Response]]] = {index: (authzr, None)
                             for index, authzr in enumerate(authzrs)}
         authzrs_failed_to_report = []
+        deadline = datetime.datetime.now() + datetime.timedelta(minutes=deadline_minutes)
         # Give an initial second to the ACME CA server to check the authorizations
         sleep_seconds: float = 1
         for _ in range(max_retries):
@@ -184,7 +187,7 @@ class AuthHandler:
             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:
+            if not authzrs_to_check or datetime.datetime.now() > deadline:
                 # Polling process is finished, we can leave the loop
                 break
 
@@ -196,6 +199,9 @@ class AuthHandler:
             retry_after = max(self.acme.retry_after(resp, 3)
                               for _, resp in authzrs_to_check.values()
                               if resp is not None)
+            # Whatever Retry-After the ACME server requests, the polling must not take
+            # longer than the overall deadline (https://github.com/certbot/certbot/issues/9526).
+            retry_after = min(retry_after, deadline)
             sleep_seconds = (retry_after - datetime.datetime.now()).total_seconds()
 
         # In case of failed authzrs, create a report to the user.

certbot/tests/auth_handler_test.py

@@ -1,5 +1,5 @@
 """Tests for certbot._internal.auth_handler."""
-import functools
+import datetime
 import logging
 import unittest
 
@@ -12,7 +12,6 @@ from acme import errors as acme_errors
 from acme import messages
 from certbot import achallenges
 from certbot import errors
-from certbot import util
 from certbot._internal.display import obj as display_obj
 from certbot.plugins import common as plugin_common
 from certbot.tests import acme_util
@@ -227,6 +226,39 @@ class HandleAuthorizationsTest(unittest.TestCase):
             self.handler.handle_authorizations(mock_order, self.mock_config, False, 1)
         self.assertIn('All authorizations were not finalized by the CA.', str(error.exception))
 
+    @mock.patch('certbot._internal.auth_handler.time.sleep')
+    def test_deadline_exceeded(self, mock_sleep):
+        authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)]
+        mock_order = mock.MagicMock(authorizations=authzrs)
+
+        orig_now = datetime.datetime.now
+        state = {'time_slept': 0}
+
+        def mock_sleep_effect(secs):
+            state['time_slept'] += secs
+        mock_sleep.side_effect = mock_sleep_effect
+
+        def mock_now_effect():
+            return orig_now() + datetime.timedelta(seconds=state["time_slept"])
+
+        # We will return STATUS_PENDING and ask Certbot to sleep for 20 minutes at a time.
+        interval = datetime.timedelta(minutes=20).seconds
+        self.mock_net.poll.side_effect = _gen_mock_on_poll(status=messages.STATUS_PENDING,
+                                                           wait_value=interval)
+
+        with self.assertRaises(errors.AuthorizationError) as error, \
+             mock.patch('certbot._internal.auth_handler.datetime.datetime') as mock_dt:
+            mock_dt.now.side_effect = mock_now_effect
+            # Polling will only proceed for 30 minutes at most, so the second 20 minute sleep
+            # should be truncated and the polling should be aborted.
+            self.handler.handle_authorizations(mock_order, self.mock_config, False)
+        self.assertIn('All authorizations were not finalized by the CA.', str(error.exception))
+
+        self.assertEqual(mock_sleep.call_count, 3) # 1s, 20m and 10m sleep
+        self.assertEqual(mock_sleep.call_args_list[0][0][0], 1)
+        self.assertAlmostEqual(mock_sleep.call_args_list[1][0][0], interval - 1, delta=1)
+        self.assertAlmostEqual(mock_sleep.call_args_list[2][0][0], interval/2 - 1, delta=1)
+
     def test_no_domains(self):
         mock_order = mock.MagicMock(authorizations=[])
         self.assertRaises(errors.AuthorizationError, self.handler.handle_authorizations,