From 581977c92d0b1969baabbf1b47f3ffd2c12e61e0 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 4 Aug 2025 13:48:12 -0700 Subject: [PATCH] try and improve should_autorenew comments (#10391) i wrote this in response to erica's thread at https://opensource.eff.org/eff-open-source/pl/xtuemgdti78xfx1hn9jwbrfrjy this hopefully helps some, but i think our logic here is unfortunately fairly complicated which is reflected in the code comments. feel free to suggest alternate wording or even just close this if you think our comments currently in main are good enough --- certbot/src/certbot/_internal/renewal.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/certbot/src/certbot/_internal/renewal.py b/certbot/src/certbot/_internal/renewal.py index 414b93c4c..f9d6a1aaa 100644 --- a/certbot/src/certbot/_internal/renewal.py +++ b/certbot/src/certbot/_internal/renewal.py @@ -389,14 +389,15 @@ def should_autorenew(config: configuration.NamespaceConfig, 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, - and renew if ARI indicates it is time, or if we are within the default - renweal window. + If automatic renewal is disabled for the lineage, this function + immediately returns False. - If the certificate has an OCSP URL, renew if it is revoked. - - If neither of the above is true, but the "renew_before_expiry" config - indicates it is time, renew. Otherwise, don't. + Otherwise, if any of ACME Renewal Info (ARI), OCSP, or the + renew_before_expiry config option indicates we should renew, we + return True. If none of those values say we should renew and at + least one of ARI or renew_before_expiry has an available value, we + return False. We otherwise decide whether to renew based on the + certificate's remaining lifetime. Note that this examines the numerically most recent cert version, not the currently deployed version. @@ -425,9 +426,7 @@ def should_autorenew(config: configuration.NamespaceConfig, logger.debug("Should renew, certificate is revoked.") return True - # The "renew_before_expiry" config field can make us renew earlier than the - # default. If ARI response was None and no "renew_before_expiry" is set, - # check against the default. + # If the renew_before_expiry config field is set, check if it says we should renew config_interval = lineage.configuration.get("renew_before_expiry") if config_interval is not None: notAfter = crypto_util.notAfter(cert) @@ -436,7 +435,7 @@ def should_autorenew(config: configuration.NamespaceConfig, "expiry %s.", config_interval, notAfter.strftime("%Y-%m-%d %H:%M:%S %Z")) return True - # Only use the default if we don't have an ARI response + # Only use the default if we don't have an ARI or renew_before_expiry value elif renewal_time is None: default_renewal_time = _default_renewal_time(cert_pem) if now > default_renewal_time: