From 1d9fc8dccf103abe6cf1fbe83c58ab7b66791190 Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Mon, 9 Jun 2025 11:44:04 -0700 Subject: [PATCH] renewal: use lineage-specific server for ARI (#10307) Previously, we were constructing an ACME client for ARI checking that used the global value for `server`, not the one recorded in a lineage's renewal file. This resulted in errors in the logs and failure to observe ARI for lineages that used a non-default `--server` (e.g. staging or non-Let's Encrypt CAs). --------- Co-authored-by: ohemorange --- .../certbot_tests/test_main.py | 7 +++++++ .../certbot_integration_tests/utils/certbot_call.py | 10 +++++++--- certbot/CHANGELOG.md | 6 +++++- certbot/src/certbot/_internal/renewal.py | 6 +++++- certbot/src/certbot/_internal/tests/renewal_test.py | 13 ++++++++++--- 5 files changed, 34 insertions(+), 8 deletions(-) diff --git a/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py b/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py index 0cfaad752..6f5ee4ebc 100644 --- a/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py +++ b/certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py @@ -329,6 +329,13 @@ def test_renew_when_ari_says_its_time(context: IntegrationTestsContext) -> None: } })) + # For the renew call only, avoid passing `--server` to the `certbot` command, so + # we fall back on the hardcoded default of `https://acme-v02.api.letsencrypt.org`. + # No requests should be made to that URL because the lineage has a baked-in Pebble + # URL in its config from the issuance earlier in this test case. If there's a bug + # an ARI _is_ called against that URL it will fail because Let's Encrypt doesn't + # know about certificates issued by Pebble. + context.directory_url = None context.certbot(['renew', '--deploy-hook', misc.echo('deploy', context.hook_probe)], force_renew=False) diff --git a/certbot-ci/src/certbot_integration_tests/utils/certbot_call.py b/certbot-ci/src/certbot_integration_tests/utils/certbot_call.py index 89210be9a..337b44bf3 100755 --- a/certbot-ci/src/certbot_integration_tests/utils/certbot_call.py +++ b/certbot-ci/src/certbot_integration_tests/utils/certbot_call.py @@ -6,6 +6,7 @@ import subprocess import sys from typing import Dict from typing import List +from typing import Optional from typing import Tuple import certbot_integration_tests @@ -13,7 +14,7 @@ import certbot_integration_tests from certbot_integration_tests.utils.constants import * -def certbot_test(certbot_args: List[str], directory_url: str, http_01_port: int, +def certbot_test(certbot_args: List[str], directory_url: Optional[str], http_01_port: int, tls_alpn_01_port: int, config_dir: str, workspace: str, force_renew: bool = True) -> Tuple[str, str]: """ @@ -81,7 +82,7 @@ def _prepare_environ(workspace: str) -> Dict[str, str]: return new_environ -def _prepare_args_env(certbot_args: List[str], directory_url: str, http_01_port: int, +def _prepare_args_env(certbot_args: List[str], directory_url: Optional[str], http_01_port: int, tls_alpn_01_port: int, config_dir: str, workspace: str, force_renew: bool) -> Tuple[List[str], Dict[str, str]]: @@ -90,9 +91,12 @@ def _prepare_args_env(certbot_args: List[str], directory_url: str, http_01_port: if force_renew: additional_args.append('--renew-by-default') + if directory_url: + additional_args.extend(['--server', directory_url]) + + command = [ 'certbot', - '--server', directory_url, '--no-verify-ssl', '--http-01-port', str(http_01_port), '--https-port', str(tls_alpn_01_port), diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 0be1a807a..adc0a46ab 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -6,7 +6,11 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Added -* +* ACME Renewal Info (ARI) support. https://datatracker.ietf.org/doc/draft-ietf-acme-ari/ + `certbot renew` will automatically check ARI when using an ACME server that supports it, + and may renew early based on the ARI information. For Let's Encrypt certificates this + will typically cause renewal at around 2/3rds of the certificate's lifetime, even if + the renew_before_expiry field of a lineage renewal config is set a later date. ### Changed diff --git a/certbot/src/certbot/_internal/renewal.py b/certbot/src/certbot/_internal/renewal.py index cc2bf2a1b..fa907d32e 100644 --- a/certbot/src/certbot/_internal/renewal.py +++ b/certbot/src/certbot/_internal/renewal.py @@ -610,11 +610,15 @@ 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(config) + acme_clients[server] = client.acme_from_config_key(lineage_config) renewal_candidate.ensure_deployed() from certbot._internal import main diff --git a/certbot/src/certbot/_internal/tests/renewal_test.py b/certbot/src/certbot/_internal/tests/renewal_test.py index a5951e626..72ef7c34a 100644 --- a/certbot/src/certbot/_internal/tests/renewal_test.py +++ b/certbot/src/certbot/_internal/tests/renewal_test.py @@ -216,10 +216,12 @@ class RenewalTest(test_util.ConfigTestCase): assert lineage_config.key_type == 'rsa' @test_util.patch_display_util() + @mock.patch.object(configuration.NamespaceConfig, 'set_by_user') @mock.patch('certbot._internal.client.acme_from_config_key') @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, unused_mock_display): + def test_renewal_via_ari(self, mock_datetime, mock_renew_cert, mock_acme_from_config, 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 @@ -230,12 +232,17 @@ class RenewalTest(test_util.ConfigTestCase): acme_client.renewal_time.return_value = past, future test_util.make_lineage(self.config.config_dir, 'sample-renewal.conf', ec=False) - lineage_config = copy.deepcopy(self.config) + config = configuration.NamespaceConfig(self.config) with mock.patch('time.sleep') as sleep: - renewal.handle_renewal_request(lineage_config) + renewal.handle_renewal_request(config) mock_renew_cert.assert_called_once() + # This value comes from `sample-renewal.conf` and is different than + # 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 def test_default_renewal_time(self): from certbot._internal import renewal