From d641f062f294765d7cfd7e06de7f2ec9bee388ed Mon Sep 17 00:00:00 2001 From: alexzorin Date: Fri, 6 Jan 2023 09:24:58 +1100 Subject: [PATCH] limit challenge polling to 30 minutes (#9527) * limit challenge polling to 30 minutes * Fix docstring typo Co-authored-by: Brad Warren Co-authored-by: Brad Warren --- certbot/CHANGELOG.md | 4 ++- certbot/certbot/_internal/auth_handler.py | 16 ++++++---- certbot/tests/auth_handler_test.py | 36 +++++++++++++++++++++-- 3 files changed, 48 insertions(+), 8 deletions(-) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index f2ad5ba0a..7f39d3075 100644 --- a/certbot/CHANGELOG.md +++ b/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 diff --git a/certbot/certbot/_internal/auth_handler.py b/certbot/certbot/_internal/auth_handler.py index 05feaadc0..747520a7d 100644 --- a/certbot/certbot/_internal/auth_handler.py +++ b/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. diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 23d5b2ae2..548356897 100644 --- a/certbot/tests/auth_handler_test.py +++ b/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,