starred/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-07-28 08:45:19 +02:00
Code Issues Packages Projects Releases Wiki Activity

Pass in dict of acme clients instead of acme so we can wait to initialize in some cases (#10337)

Browse Source 
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 = <certbot._internal.tests.renewal_test.RenewalTest testMethod=test_no_network_if_no_autorenew>
mock_autorenewal_enabled = <MagicMock name='autorenewal_is_enabled' id='4378096224'>, mock_client_network_get = <MagicMock name='get' id='4378087008'>
unused_mock_display = <certbot.tests.util.FreezableMock object at 0x104eb4f50>

>   ???
E   AssertionError: assert 1 == 0
E    +  where 1 = <MagicMock name='get' id='4378087008'>.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 ===================================================================

```
This commit is contained in:
ohemorange
2025-06-12 11:02:22 -07:00
committed by GitHub
parent 31599bad83
commit 680d998597
4 changed files with 45 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
certbot/CHANGELOG.md
+2
View File
@@ -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.
certbot/src/certbot/_internal/main.py
+3 -2
View File
@@ -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:
certbot/src/certbot/_internal/renewal.py
+13 -15
View File
@@ -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)
certbot/src/certbot/_internal/tests/renewal_test.py
+27 -5
View File
@@ -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):