From a020de1e50c4a7fb7198cea57e08036a13003425 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Thu, 17 Jul 2025 12:18:25 -0700 Subject: [PATCH] Make ACME directory fetches for ARI checks resilient (#10358) Fixes https://github.com/certbot/certbot/issues/10342 When doing ARI checks in acme.renewal_time, we catch RequestException and return a default value. That's so an unavailable ARI server doesn't cause issues. Before we get to acme.renewal_time, we have to create an ACME client, and in the process fetch a directory. We should make the directory fetch similarly resilient. --------- Co-authored-by: Brad Warren --- certbot/CHANGELOG.md | 9 +++++---- certbot/src/certbot/_internal/renewal.py | 13 +++++++++---- .../certbot/_internal/tests/renewal_test.py | 19 +++++++++++++++++++ 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index f3d0b959d..99f4f4312 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -10,13 +10,14 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Changed -* +* Catches and ignores errors during the directory fetch for ARI checking so that these + errors do not hinder the actual certificate issuance ### Fixed -* Certbot now always uses the server value from the renewal configuration file - for ARI checks instead of the server value from the current invocation of - Certbot. This helps prevent ARI requests from going to the wrong server if +* Certbot now always uses the server value from the renewal configuration file + for ARI checks instead of the server value from the current invocation of + Certbot. This helps prevent ARI requests from going to the wrong server if the user changes CAs. * Previously, we claimed to set FAILED_DOMAINS and RENEWED_DOMAINS env variables for use by post-hooks when certificate renewals fail, but we were not actually setting them. Now, we are. diff --git a/certbot/src/certbot/_internal/renewal.py b/certbot/src/certbot/_internal/renewal.py index 6f3e30ec7..6b1268758 100644 --- a/certbot/src/certbot/_internal/renewal.py +++ b/certbot/src/certbot/_internal/renewal.py @@ -385,12 +385,17 @@ def should_autorenew(config: configuration.NamespaceConfig, # 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[lineage.server] + try: + acme_clients[lineage.server] = \ + client.create_acme_client(config, server_override=lineage.server) + except Exception as error: # pylint: disable=broad-except + logger.info("Unable to connect to %s to request ACME Renewal Information (ARI). " + "Error was: %s", lineage.server, error) + acme = acme_clients.get(lineage.server, None) # Attempt to get the ARI-defined renewal time - renewal_time, _ = acme.renewal_time(cert_pem) + if acme: + renewal_time, _ = acme.renewal_time(cert_pem) else: logger.info("Certificate has no 'server' field configured, unable to " "perform ACME Renewal Information (ARI) request.") diff --git a/certbot/src/certbot/_internal/tests/renewal_test.py b/certbot/src/certbot/_internal/tests/renewal_test.py index 4dde0aaca..4899fdbf2 100644 --- a/certbot/src/certbot/_internal/tests/renewal_test.py +++ b/certbot/src/certbot/_internal/tests/renewal_test.py @@ -420,6 +420,25 @@ class RenewalTest(test_util.ConfigTestCase): mock_rc.server = None assert renewal.should_autorenew(self.config, mock_rc, acme_clients) + @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): + from certbot._internal import renewal + from acme import messages + + ari_server = 'http://ari' + acme_clients = {} + 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'')): + assert renewal.should_autorenew(self.config, mock_rc, acme_clients) + assert mock_renewal_time.call_count == 0 + class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): """Tests for certbot._internal.renewal.restore_required_config_elements."""