diff --git a/certbot/src/certbot/_internal/main.py b/certbot/src/certbot/_internal/main.py index c6e80fbc5..92f2b5966 100644 --- a/certbot/src/certbot/_internal/main.py +++ b/certbot/src/certbot/_internal/main.py @@ -268,10 +268,9 @@ def _handle_identical_cert_request(config: configuration.NamespaceConfig, if is_key_type_changing: return "renew", lineage - acme_clients = {} - acme_clients[config.server] = client.create_acme_client(config) + ari_clients = renewal.AriClientPool(config) - if renewal.should_renew(config, lineage, acme_clients): + if renewal.should_renew(config, lineage, ari_clients): return "renew", lineage if config.reinstall: diff --git a/certbot/src/certbot/_internal/renewal.py b/certbot/src/certbot/_internal/renewal.py index bdda72f6d..ad0bf52a5 100644 --- a/certbot/src/certbot/_internal/renewal.py +++ b/certbot/src/certbot/_internal/renewal.py @@ -57,6 +57,39 @@ BOOL_CONFIG_ITEMS = ["must_staple", "allow_subset_of_names", "reuse_key", CONFIG_ITEMS = set(itertools.chain( BOOL_CONFIG_ITEMS, INT_CONFIG_ITEMS, STR_CONFIG_ITEMS, ('pref_challs',))) +class AriClientPool: + """A cache of ACME clients for using in performing ACME Renewal Info (ARI) requests. + + During `certbot renew` we need to check ARI for many certificates, and usually these + are all issued by the same server. To avoid redundant directory fetches, we create + one acme.ClientV2 per server. + + This takes a command line configuration object so it can set the User-Agent header and + observe the --no-verify-ssl flag. + """ + def __init__(self, cli_config: configuration.NamespaceConfig): + self._verify_ssl = not cli_config.no_verify_ssl + self._user_agent = client.determine_user_agent(cli_config) + self._pool: Dict[str, acme_client.ClientV2] = {} + + def get(self, server: str) -> acme_client.ClientV2: + """ + Retrieve or create an ACME client for the specified server. + + Returns: + acme_client.ClientV2: The ACME client associated with the specified server. + """ + ari_client = self._pool.get(server, None) + if ari_client: + return ari_client + + net = acme_client.ClientNetwork(verify_ssl=self._verify_ssl, user_agent=self._user_agent) + directory = acme_client.ClientV2.get_directory(server, net) + ari_client = acme_client.ClientV2(directory, net) + + self._pool[server] = ari_client + return ari_client + def reconstitute(config: configuration.NamespaceConfig, full_path: str) -> Optional[storage.RenewableCert]: @@ -310,7 +343,7 @@ def _restore_str(name: str, value: str) -> Optional[str]: def should_renew(config: configuration.NamespaceConfig, lineage: storage.RenewableCert, - acme_clients: Dict[str, acme_client.ClientV2]) -> bool: + ari_clients: AriClientPool) -> bool: """Return true if any of the circumstances for automatic renewal apply.""" if config.renew_by_default: logger.debug("Auto-renewal forced with --force-renewal...") @@ -318,41 +351,33 @@ def should_renew(config: configuration.NamespaceConfig, if config.dry_run: logger.info("Certificate not due for renewal, but simulating renewal for dry run") return True - if should_autorenew(config, lineage, acme_clients): + if should_autorenew(lineage, ari_clients): logger.info("Certificate is due for renewal, auto-renewing...") return True display_util.notify("Certificate not yet due for renewal") return False -def _ari_renewal_time(config: configuration.NamespaceConfig, - lineage: storage.RenewableCert, - acme_clients: Dict[str, acme_client.ClientV2], - cert_pem: bytes, - )-> Optional[datetime.datetime]: +def _ari_renewal_time(lineage: storage.RenewableCert, + cert_pem: bytes, + ari_clients: AriClientPool) -> Optional[datetime.datetime]: """Return the ARI suggested renewal time if it's available.""" # For ARI requests, we want to use the ACME directory URL from which the - # cert was originally requested. Since `config.server` can be overridden on + # cert was originally requested. Since `NamespaceConfig.server` can be overridden on # the command line, we're using the server stored in the cert's renewal # conf, i.e. `lineage.server` # # Fixes https://github.com/certbot/certbot/issues/10339 if not lineage.server: - renewal_conf_file = storage.renewal_filename_for_lineagename(config, lineage.lineagename) logger.warning("Skipping ARI check because %s has no 'server' field. This issue will not " - "prevent certificate renewal", renewal_conf_file) + "prevent certificate renewal", lineage.configfile.filename) return None try: - # Creating a new ACME client makes a network request, so check if we have - # one cached for this cert's server already - if lineage.server not in acme_clients: - acme_clients[lineage.server] = \ - client.create_acme_client(config, server_override=lineage.server) - acme = acme_clients.get(lineage.server, None) + ari_client = ari_clients.get(lineage.server) # Attempt to get the ARI-defined renewal time - if acme: - return acme.renewal_time(cert_pem)[0] + if ari_client: + return ari_client.renewal_time(cert_pem)[0] except Exception: # pylint: disable=broad-except # We want to stop errors around ARI preventing renewal so we catch all exceptions here # with a warning asking users to tell us about any problems they are experiencing @@ -384,9 +409,8 @@ def _default_renewal_time(cert_pem: bytes) -> datetime.datetime: return default_rt -def should_autorenew(config: configuration.NamespaceConfig, - lineage: storage.RenewableCert, - acme_clients: Dict[str, acme_client.ClientV2]) -> bool: +def should_autorenew(lineage: storage.RenewableCert, + ari_clients: AriClientPool) -> bool: """Should we now try to autorenew the most recent cert version? If automatic renewal is disabled for the lineage, this function @@ -414,7 +438,7 @@ def should_autorenew(config: configuration.NamespaceConfig, with open(cert, 'rb') as f: cert_pem = f.read() - renewal_time = _ari_renewal_time(config, lineage, acme_clients, cert_pem) + renewal_time = _ari_renewal_time(lineage, cert_pem, ari_clients) now = datetime.datetime.now(datetime.timezone.utc) @@ -622,7 +646,7 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> None: # We initialize acme clients on a per-server basis, but most # lineages use the same server. Memoize clients here so we can # share the connection pool and reuse a single fetched directory. - acme_clients: Dict[str, acme_client.ClientV2] = {} + ari_clients = AriClientPool(config) for renewal_file in conf_files: display_util.notification("Processing " + renewal_file, pause=False) @@ -649,7 +673,7 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> None: renewal_candidate.ensure_deployed() from certbot._internal import main plugins = plugins_disco.PluginsRegistry.find_all() - if should_renew(lineage_config, renewal_candidate, acme_clients): + if should_renew(lineage_config, renewal_candidate, ari_clients): # Apply random sleep upon first renewal if needed if apply_random_sleep: sleep_time = random.uniform(1, 60 * 8) diff --git a/certbot/src/certbot/_internal/tests/renewal_test.py b/certbot/src/certbot/_internal/tests/renewal_test.py index ba1f5a379..0b35c45f0 100644 --- a/certbot/src/certbot/_internal/tests/renewal_test.py +++ b/certbot/src/certbot/_internal/tests/renewal_test.py @@ -217,14 +217,14 @@ class RenewalTest(test_util.ConfigTestCase): @test_util.patch_display_util() @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') - @mock.patch('certbot._internal.client.create_acme_client') + @mock.patch('certbot._internal.renewal.AriClientPool.get') @mock.patch('certbot._internal.main.renew_cert') @mock.patch("certbot._internal.renewal.datetime") - def test_renewal_via_ari(self, mock_datetime, mock_renew_cert, mock_acme_from_config, mock_set_by_user, unused_mock_display): + def test_renewal_via_ari(self, mock_datetime, mock_renew_cert, mock_ari_client_get, mock_set_by_user, unused_mock_display): mock_set_by_user.return_value = False from certbot._internal import renewal acme_client = mock.MagicMock() - mock_acme_from_config.return_value = acme_client + mock_ari_client_get.return_value = acme_client past = datetime.datetime(2025, 3, 19, 0, 0, 0, tzinfo=datetime.timezone.utc) now = datetime.datetime(2025, 4, 19, 0, 0, 0, tzinfo=datetime.timezone.utc) future = datetime.datetime(2025, 4, 19, 12, 0, 0, tzinfo=datetime.timezone.utc) @@ -242,7 +242,8 @@ class RenewalTest(test_util.ConfigTestCase): # the global default. expected_server = "https://acme-staging-v02.api.letsencrypt.org/directory" assert expected_server != config.server - assert mock_acme_from_config.call_args[0][0].server == expected_server + mock_ari_client_get.assert_called_once() + assert mock_ari_client_get.call_args[0][0] == expected_server @test_util.patch_display_util() @mock.patch('acme.client.ClientNetwork.get') @@ -263,10 +264,10 @@ class RenewalTest(test_util.ConfigTestCase): def test_dry_run_no_ari_call(self, mock_acme): from certbot._internal import renewal self.config.dry_run = True - acme_clients = {} - acme_clients[self.config.server] = mock_acme + ari_client_pool = mock.MagicMock() + ari_client_pool.get.return_value = mock_acme with mock.patch('time.sleep') as sleep: - renewal.should_renew(self.config, mock.Mock(), acme_clients) + renewal.should_renew(self.config, mock.Mock(), ari_client_pool) assert mock_acme.renewal_time.call_count == 0 def test_default_renewal_time(self): @@ -298,8 +299,8 @@ class RenewalTest(test_util.ConfigTestCase): mock_acme = mock.MagicMock() future = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(days=100000) mock_acme.renewal_time.return_value = (future, future) - acme_clients = {} - acme_clients[ari_server] = mock_acme + ari_client_pool = mock.MagicMock() + ari_client_pool.get.return_value = mock_acme mock_renewable_cert = mock.MagicMock() mock_renewable_cert.server = ari_server @@ -349,7 +350,7 @@ class RenewalTest(test_util.ConfigTestCase): sometime = datetime.datetime.fromtimestamp(current_time, datetime.timezone.utc) mock_datetime.datetime.now.return_value = sometime mock_renewable_cert.configuration = {"renew_before_expiry": interval} - assert renewal.should_autorenew(self.config, mock_renewable_cert, acme_clients) == result, f"at {current_time}, with config '{interval}', ari response in future, expected {result}" + assert renewal.should_autorenew(mock_renewable_cert, ari_client_pool) == result, f"at {current_time}, with config '{interval}', ari response in future, expected {result}" # Now, test cases where ARI either fails (returns `(None, _)`) or # the cert has no `server` value and ARI is skipped @@ -388,9 +389,9 @@ class RenewalTest(test_util.ConfigTestCase): mock_datetime.datetime.now.return_value = sometime mock_renewable_cert.configuration = {"renew_before_expiry": interval} mock_renewable_cert.server = ari_server - assert renewal.should_autorenew(self.config, mock_renewable_cert, acme_clients) == result, f"at {current_time}, with config '{interval}', no ari response, expected {result}" + assert renewal.should_autorenew(mock_renewable_cert, ari_client_pool) == result, f"at {current_time}, with config '{interval}', no ari response, expected {result}" mock_renewable_cert.server = None - assert renewal.should_autorenew(self.config, mock_renewable_cert, acme_clients) == result, f"at {current_time}, with config '{interval}', skipped ari, expected {result}" + assert renewal.should_autorenew(mock_renewable_cert, ari_client_pool) == result, f"at {current_time}, with config '{interval}', skipped ari, expected {result}" @mock.patch("certbot._internal.storage.RenewableCert.ocsp_revoked") def test_should_autorenew(self, mock_ocsp): @@ -400,77 +401,84 @@ class RenewalTest(test_util.ConfigTestCase): ari_server = "http://ari" future = datetime.datetime.now(datetime.timezone.utc) + datetime.timedelta(seconds=1000) mock_acme.renewal_time.return_value = (future, future) - acme_clients = {} - acme_clients[ari_server] = mock_acme + ari_client_pool = mock.MagicMock() + ari_client_pool.get.return_value = mock_acme mock_rc = mock.MagicMock() with mock.patch('certbot._internal.renewal.open', mock.mock_open(read_data=b'')): # Autorenewal turned off mock_rc.autorenewal_is_enabled.return_value = False mock_rc.server = ari_server - assert not renewal.should_autorenew(self.config, mock_rc, acme_clients) + assert not renewal.should_autorenew(mock_rc, ari_client_pool) mock_rc.server = None - assert not renewal.should_autorenew(self.config, mock_rc, acme_clients) + assert not renewal.should_autorenew(mock_rc, ari_client_pool) # Autorenewal turned on, mandatory renewal on the basis of OCSP # revocation mock_rc.autorenewal_is_enabled.return_value = True mock_ocsp.return_value = True - assert renewal.should_autorenew(self.config, mock_rc, acme_clients) + assert renewal.should_autorenew(mock_rc, ari_client_pool) mock_rc.server = None with mock.patch('certbot._internal.renewal.logger.warning') as mock_warning: - assert renewal.should_autorenew(self.config, mock_rc, acme_clients) + assert renewal.should_autorenew(mock_rc, ari_client_pool) # Ensure we warned about skipping ARI checks when server is None - assert any(call.args[0].startswith('Skipping ARI') for call in + assert any(call.args[0].startswith('Skipping ARI check') for call in mock_warning.call_args_list) - @mock.patch('certbot._internal.client.create_acme_client') @mock.patch('certbot._internal.storage.RenewableCert.ocsp_revoked') @mock.patch('acme.client.ClientV2.renewal_time') - def test_resilient_ari_directory_fetches(self, mock_renewal_time, mock_ocsp, mock_create_acme): + def test_resilient_ari_directory_fetches(self, mock_renewal_time, mock_ocsp): from certbot._internal import renewal from acme import messages ari_server = 'http://ari' - acme_clients = {} + ari_client_pool = mock.MagicMock() + ari_client_pool.get.side_effect = messages.Error() mock_rc = mock.MagicMock() mock_rc.server = ari_server mock_rc.autorenewal_is_enabled.return_value = True - mock_create_acme.side_effect = messages.Error() mock_ocsp.return_value = True with mock.patch('certbot._internal.renewal.open', mock.mock_open(read_data=b'')): with mock.patch('certbot._internal.renewal.logger') as mock_logger: - assert renewal.should_autorenew(self.config, mock_rc, acme_clients) + assert renewal.should_autorenew(mock_rc, ari_client_pool) + assert mock_renewal_time.call_count == 0 # Ensure we logged about skipping the ARI check and the underlying exception assert any('ARI' in call.args[0] for call in mock_logger.warning.call_args_list) assert any(call.kwargs.get('exc_info') for call in mock_logger.debug.call_args_list) - @mock.patch('certbot._internal.storage.RenewableCert.ocsp_revoked') def test_resilient_ari_check(self, mock_ocsp): from certbot._internal import renewal + from acme import messages + + rc_path = test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf') + renewable_cert = storage.RenewableCert(rc_path, self.config) - mock_acme = mock.MagicMock() ari_error = acme_errors.ARIError('some error', datetime.datetime.now()) - ari_server = 'http://ari' - mock_acme.renewal_time.side_effect = ari_error - acme_clients = {} - acme_clients[ari_server] = mock_acme - mock_rc = mock.MagicMock() - mock_rc.server = ari_server - mock_rc.autorenewal_is_enabled.return_value = True + ari_client_pool = MockAriClientPool(None, None) + ari_client_pool.mock_acme.renewal_time.side_effect = ari_error + mock_ocsp.return_value = True with mock.patch('certbot._internal.renewal.open', mock.mock_open(read_data=b'')): with mock.patch('certbot._internal.renewal.logger') as mock_logger: - assert renewal.should_autorenew(self.config, mock_rc, acme_clients) + assert renewal.should_autorenew(renewable_cert, ari_client_pool) # Ensure we logged about skipping the ARI check and the underlying exception assert any('ARI' in call.args[0] for call in mock_logger.warning.call_args_list) assert any(call.kwargs.get('exc_info') for call in mock_logger.debug.call_args_list) +class MockAriClientPool: + def __init__(self, renewal_time, retry_after): + self.mock_acme = mock.MagicMock() + self.mock_acme.renewal_time.return_value = (renewal_time, retry_after) + + def get(self, server): + return self.mock_acme + + class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): """Tests for certbot._internal.renewal.restore_required_config_elements.""" @classmethod