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

@@ -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:

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)

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