From 13954a783a7d820d13f0e73d9c7f6b017fb1b6d1 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 7 Aug 2025 13:19:05 -0700 Subject: [PATCH] renewal: factor out AriClientPool (#10393) Part of #10355. This allows combining the NamespaceConfig object with a cache of ACME clients, so we don't have to make the whole large NamespaceConfig object available all the way down the renewal call stack. The new AriClientPool is responsible for instantiating ACME clients for the purpose of ARI fetching. It provides only a simple user agent, listing the Certbot version. The only CLI flag it observes is --no-verify-ssl. --- certbot/src/certbot/_internal/main.py | 5 +- certbot/src/certbot/_internal/renewal.py | 72 ++++++++++++------ .../certbot/_internal/tests/renewal_test.py | 76 ++++++++++--------- 3 files changed, 92 insertions(+), 61 deletions(-) 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