From 680d99859722f5b7f4b38b40f3554a18845dab34 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Thu, 12 Jun 2025 11:02:22 -0700 Subject: [PATCH] Pass in dict of acme clients instead of acme so we can wait to initialize in some cases (#10337) Regression test fails on main with commit "add regression test" cherry-picked onto it ``` $ pytest certbot/src/certbot/_internal/tests/renewal_test.py ======================================================================= test session starts ======================================================================= platform darwin -- Python 3.12.8, pytest-8.3.5, pluggy-1.5.0 rootdir: /Users/erica/certbot configfile: pytest.ini plugins: anyio-4.9.0, cov-6.1.1, xdist-3.6.1 collected 27 items certbot/src/certbot/_internal/tests/renewal_test.py .....F..................... [100%] ============================================================================ FAILURES ============================================================================= ___________________________________________________________ RenewalTest.test_no_network_if_no_autorenew ___________________________________________________________ self = mock_autorenewal_enabled = , mock_client_network_get = unused_mock_display = > ??? E AssertionError: assert 1 == 0 E + where 1 = .call_count certbot/src/certbot/_internal/tests/renewal_test.py:260: AssertionError ===================================================================== short test summary info ===================================================================== FAILED certbot/src/certbot/_internal/tests/renewal_test.py::RenewalTest::test_no_network_if_no_autorenew - AssertionError: assert 1 == 0 ================================================================== 1 failed, 26 passed in 0.30s =================================================================== ``` --- certbot/CHANGELOG.md | 2 ++ certbot/src/certbot/_internal/main.py | 5 +-- certbot/src/certbot/_internal/renewal.py | 28 ++++++++-------- .../certbot/_internal/tests/renewal_test.py | 32 ++++++++++++++++--- 4 files changed, 45 insertions(+), 22 deletions(-) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 3626cbf44..782836b11 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -18,6 +18,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). * No longer checks ARI during certbot --dry-run, because --dry-run uses staging when used with let's encrypt but the cert was issued against the default server. This would emit a scary warning, even though the cert would renew successfully. +* Contacting the CA to check ARI is now skipped for certificate lineages that + have autorenew set to False. More details about these changes can be found on our GitHub repo. diff --git a/certbot/src/certbot/_internal/main.py b/certbot/src/certbot/_internal/main.py index 3130904bf..fcac33fd4 100644 --- a/certbot/src/certbot/_internal/main.py +++ b/certbot/src/certbot/_internal/main.py @@ -268,9 +268,10 @@ def _handle_identical_cert_request(config: configuration.NamespaceConfig, if is_key_type_changing: return "renew", lineage - acme = client.acme_from_config_key(config) + acme_clients = {} + acme_clients[config.server] = client.acme_from_config_key(config) - if renewal.should_renew(config, lineage, acme): + if renewal.should_renew(config, lineage, acme_clients): return "renew", lineage if config.reinstall: diff --git a/certbot/src/certbot/_internal/renewal.py b/certbot/src/certbot/_internal/renewal.py index 66cbea8d7..661c7a2fa 100644 --- a/certbot/src/certbot/_internal/renewal.py +++ b/certbot/src/certbot/_internal/renewal.py @@ -315,7 +315,7 @@ def _restore_str(name: str, value: str) -> Optional[str]: def should_renew(config: configuration.NamespaceConfig, lineage: storage.RenewableCert, - acme: acme_client.ClientV2) -> bool: + acme_clients: Dict[str, acme_client.ClientV2]) -> 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...") @@ -323,7 +323,7 @@ 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(lineage, acme): + if should_autorenew(config, lineage, acme_clients): logger.info("Certificate is due for renewal, auto-renewing...") return True display_util.notify("Certificate not yet due for renewal") @@ -350,7 +350,9 @@ def _default_renewal_time(cert_pem: bytes) -> datetime.datetime: return default_rt -def should_autorenew(lineage: storage.RenewableCert, acme: acme_client.ClientV2) -> bool: +def should_autorenew(config: configuration.NamespaceConfig, + lineage: storage.RenewableCert, + acme_clients: Dict[str, acme_client.ClientV2]) -> bool: """Should we now try to autorenew the most recent cert version? If ACME Renewal Info (ARI) is available in the directory, check that first, @@ -371,6 +373,12 @@ def should_autorenew(lineage: storage.RenewableCert, acme: acme_client.ClientV2) """ if lineage.autorenewal_is_enabled(): + # Don't initialize the acme client (making a network request) until + # we know we're actually going to have to check ARI + if config.server not in acme_clients: + acme_clients[config.server] = client.acme_from_config_key(config) + acme = acme_clients[config.server] + cert = lineage.version("cert", lineage.latest_common_version()) # Consider whether to attempt to autorenew this cert now @@ -587,7 +595,7 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list, # 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 = {} + acme_clients: Dict[str, acme_client.ClientV2] = {} for renewal_file in conf_files: display_util.notification("Processing " + renewal_file, pause=False) @@ -610,20 +618,10 @@ def handle_renewal_request(config: configuration.NamespaceConfig) -> Tuple[list, if not renewal_candidate: parse_failures.append(renewal_file) else: - # We check ARI against the server stored in the lineage config even if the user - # specified a different `--server` on the command line. That's the server that - # issued the existing certificate, so it's the only server that can respond to - # ARI requests for it. - server = lineage_config.server - if not server: - raise errors.Error(f"Renewal config for {lineage_config.names} has no server.") - if server not in acme_clients: - acme_clients[server] = client.acme_from_config_key(lineage_config) - renewal_candidate.ensure_deployed() from certbot._internal import main plugins = plugins_disco.PluginsRegistry.find_all() - if should_renew(lineage_config, renewal_candidate, acme_clients[server]): + if should_renew(lineage_config, renewal_candidate, acme_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 3ac70744b..39f826871 100644 --- a/certbot/src/certbot/_internal/tests/renewal_test.py +++ b/certbot/src/certbot/_internal/tests/renewal_test.py @@ -244,12 +244,30 @@ class RenewalTest(test_util.ConfigTestCase): assert expected_server != config.server assert mock_acme_from_config.call_args[0][0].server == expected_server + @test_util.patch_display_util() + @mock.patch('acme.client.ClientNetwork.get') + @mock.patch('certbot._internal.storage.RenewableCert.autorenewal_is_enabled') + def test_no_network_if_no_autorenew(self, mock_autorenewal_enabled, + mock_client_network_get, unused_mock_display): + from certbot._internal import renewal + mock_autorenewal_enabled.return_value = False + + test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf', ec=False) + + with mock.patch('time.sleep') as sleep: + renewal.handle_renewal_request(self.config) + + assert mock_client_network_get.call_count == 0 + + @mock.patch('acme.client.ClientV2') 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 with mock.patch('time.sleep') as sleep: - renewal.should_renew(self.config, mock.Mock(), mock_acme) + renewal.should_renew(self.config, mock.Mock(), acme_clients) assert mock_acme.renewal_time.call_count == 0 def test_default_renewal_time(self): @@ -280,6 +298,8 @@ class RenewalTest(test_util.ConfigTestCase): mock_acme = mock.MagicMock() future = datetime.datetime.now(pytz.UTC) + datetime.timedelta(days=100000) mock_acme.renewal_time.return_value = (future, future) + acme_clients = {} + acme_clients[self.config.server] = mock_acme mock_renewable_cert = mock.MagicMock() mock_renewable_cert.autorenewal_is_enabled.return_value = True @@ -327,7 +347,7 @@ class RenewalTest(test_util.ConfigTestCase): sometime = datetime.datetime.fromtimestamp(current_time, pytz.UTC) mock_datetime.datetime.now.return_value = sometime mock_renewable_cert.configuration = {"renew_before_expiry": interval} - assert renewal.should_autorenew(mock_renewable_cert, mock_acme) == result, f"at {current_time}, with config '{interval}', ari response in future, expected {result}" + 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}" mock_acme.renewal_time.return_value = (None, future) for (current_time, interval, result) in [ @@ -363,7 +383,7 @@ class RenewalTest(test_util.ConfigTestCase): sometime = datetime.datetime.fromtimestamp(current_time, pytz.UTC) mock_datetime.datetime.now.return_value = sometime mock_renewable_cert.configuration = {"renew_before_expiry": interval} - assert renewal.should_autorenew(mock_renewable_cert, mock_acme) == result, f"at {current_time}, with config '{interval}', no ari response, expected {result}" + assert renewal.should_autorenew(self.config, mock_renewable_cert, acme_clients) == result, f"at {current_time}, with config '{interval}', no ari response, expected {result}" @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') @mock.patch("certbot._internal.storage.RenewableCert.ocsp_revoked") @@ -374,16 +394,18 @@ class RenewalTest(test_util.ConfigTestCase): mock_acme = mock.MagicMock() future = datetime.datetime.now(pytz.UTC) + datetime.timedelta(seconds=1000) mock_acme.renewal_time.return_value = (future, future) + acme_clients = {} + acme_clients[self.config.server] = mock_acme # Autorenewal turned off mock_rc = mock.MagicMock() mock_rc.autorenewal_is_enabled.return_value = False - assert not renewal.should_autorenew(mock_rc, mock_acme) + assert not renewal.should_autorenew(self.config, mock_rc, acme_clients) mock_rc.autorenewal_is_enabled.return_value = True # Mandatory renewal on the basis of OCSP revocation mock_ocsp.return_value = True - assert renewal.should_autorenew(mock_rc, mock_acme) + assert renewal.should_autorenew(self.config, mock_rc, acme_clients) mock_ocsp.return_value = False class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase):