Change acme.renewal_time to only check ARI, and not also a default time. Separate out default check and use that in should_autorenew instead. (#10309)

Fixes https://github.com/certbot/certbot/issues/10298. Replacement for #10301. --------- Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
2026-08-03 03:41:53 +02:00 · 2025-06-04 14:48:44 -07:00
parent 3cbe1288c9
commit 48f34938c6
4 changed files with 104 additions and 30 deletions
@@ -463,21 +463,21 @@ class ClientV2Test(unittest.TestCase):
            ClientV2.get_directory('https://example.com/dir', self.net).to_partial_json()

    def test_renewal_time_no_renewal_info(self):
-        # A directory with no 'renewalInfo' should result in default renewal periods.
+        # A directory with no 'renewalInfo' should result in None.
        self.client.directory = messages.Directory({})
        cert_pem = make_cert_for_renewal(
            not_before=datetime.datetime(2025, 3, 12, 00, 00, 00),
            not_after=datetime.datetime(2025, 3, 20, 00, 00, 00),
        )
        t, _ = self.client.renewal_time(cert_pem)
-        assert t == datetime.datetime(2025, 3, 16, 00, 00, 00, tzinfo=datetime.timezone.utc)
+        assert t == None

        cert_pem = make_cert_for_renewal(
            not_before=datetime.datetime(2025, 3, 12, 00, 00, 00),
            not_after=datetime.datetime(2025, 3, 30, 00, 00, 00),
        )
        t, _ = self.client.renewal_time(cert_pem)
-        assert t == datetime.datetime(2025, 3, 24, 00, 00, 00, tzinfo=datetime.timezone.utc)
+        assert t == None

    def test_renewal_time_with_renewal_info(self):
        from cryptography import x509
@@ -526,7 +526,7 @@ class ClientV2Test(unittest.TestCase):
        self.client.directory = messages.Directory({
            'renewalInfo': 'https://www.letsencrypt-demo.org/acme/renewal-info',
        })
-        # Failure to fetch the 'renewalInfo' URL should return default timings
+        # Failure to fetch the 'renewalInfo' URL should return None
        self.net.get.side_effect = requests.exceptions.RequestException

        cert_pem = make_cert_for_renewal(
@@ -534,14 +534,14 @@ class ClientV2Test(unittest.TestCase):
            not_after=datetime.datetime(2025, 3, 20, 00, 00, 00),
        )
        t, _ = self.client.renewal_time(cert_pem)
-        assert t == datetime.datetime(2025, 3, 16, 00, 00, 00, tzinfo=datetime.timezone.utc)
+        assert t == None

        cert_pem = make_cert_for_renewal(
            not_before=datetime.datetime(2025, 3, 12, 00, 00, 00),
            not_after=datetime.datetime(2025, 3, 30, 00, 00, 00),
        )
        t, _ = self.client.renewal_time(cert_pem)
-        assert t == datetime.datetime(2025, 3, 24, 00, 00, 00, tzinfo=datetime.timezone.utc)
+        assert t == None

    @mock.patch('acme.client.datetime')
    def test_renewal_time_returns_retry_after(self, dt_mock):
@@ -314,7 +314,8 @@ class ClientV2:
                raise e
        return self.poll_finalization(orderr, deadline, fetch_alternative_chains)

-    def renewal_time(self, cert_pem: bytes) -> Tuple[datetime.datetime, datetime.datetime]:
+    def renewal_time(self, cert_pem: bytes
+        ) -> Tuple[Optional[datetime.datetime], datetime.datetime]:
        """Return an appropriate time to attempt renewal of the certificate,
        and the next time to ask the ACME server for renewal info.

@@ -322,11 +323,15 @@ class ClientV2:
        based on a fetch of the renewal info resource for the certificate
        (https://www.ietf.org/archive/id/draft-ietf-acme-ari-08.html).

-        If there is no "renewalInfo" field, this function will fall back to
-        reasonable defaults based on the certificate lifetime.
+        If there is no "renewalInfo" field, this function will return a tuple of
+        None, and the next time to ask the ACME server for renewal info.

        This function may make other network calls in the future (e.g., OCSP
        or CRL).
+
+        :param bytes cert_pem: cert as pem file
+
+        :returns: Tuple of time to attempt renewal, next time to ask for renewal info
        """
        now = datetime.datetime.now()
        # https://www.ietf.org/archive/id/draft-ietf-acme-ari-08.html#section-4.3.3
@@ -334,24 +339,17 @@ class ClientV2:

        cert = x509.load_pem_x509_certificate(cert_pem)

-        not_before = cert.not_valid_before_utc
-        lifetime = cert.not_valid_after_utc - not_before
-        if lifetime.total_seconds() < 10 * 86400:
-            default_renewal_time = not_before + lifetime / 2
-        else:
-            default_renewal_time = not_before + lifetime * 2 / 3
-
        try:
            renewal_info_base_url = self.directory['renewalInfo']
        except KeyError:
-            return default_renewal_time, now + default_retry_after
+            return None, now + default_retry_after

        ari_url = renewal_info_base_url + '/' + _renewal_info_path_component(cert)
        try:
            resp = self.net.get(ari_url, content_type='application/json')
        except (requests.exceptions.RequestException, messages.Error) as error:
            logger.warning("failed to fetch renewal_info URL (%s): %s", ari_url, error)
-            return default_renewal_time, now + default_retry_after
+            return None, now + default_retry_after

        renewal_info: messages.RenewalInfo = messages.RenewalInfo.from_json(resp.json())

@@ -21,6 +21,7 @@ from cryptography.hazmat.backends import default_backend
 from cryptography.hazmat.primitives.asymmetric import ec
 from cryptography.hazmat.primitives.asymmetric import rsa
 from cryptography.hazmat.primitives.serialization import load_pem_private_key
+from cryptography import x509

 from acme import client as acme_client

@@ -328,6 +329,27 @@ def should_renew(config: configuration.NamespaceConfig,
    display_util.notify("Certificate not yet due for renewal")
    return False

+
+def _default_renewal_time(cert_pem: bytes) -> datetime.datetime:
+    """Return an reasonable default time to attempt renewal of the certificate
+    based on the certificate lifetime.
+
+    :param bytes cert_pem: cert as pem file
+
+    :returns: Time to attempt renewal
+    :rtype: `datetime.datetime`
+    """
+    cert = x509.load_pem_x509_certificate(cert_pem)
+
+    not_before = cert.not_valid_before_utc
+    lifetime = cert.not_valid_after_utc - not_before
+    if lifetime.total_seconds() < 10 * 86400:
+        default_rt = not_before + lifetime / 2
+    else:
+        default_rt = not_before + lifetime * 2 / 3
+
+    return default_rt
+
 def should_autorenew(lineage: storage.RenewableCert, acme: acme_client.ClientV2) -> bool:
    """Should we now try to autorenew the most recent cert version?

@@ -368,15 +390,21 @@ def should_autorenew(lineage: storage.RenewableCert, acme: acme_client.ClientV2)
            return True

        # The "renew_before_expiry" config field can make us renew earlier
-        # than the default.
+        # than the default. If ARI response was None and no "renew_before_expiry"
+        # is set, check against the default.
        config_interval = lineage.configuration.get("renew_before_expiry")
-        notAfter = crypto_util.notAfter(cert)
-        if (config_interval is not None and
-            notAfter < storage.add_time_interval(now, config_interval)):
-            logger.debug("Should renew, less than %s before certificate "
-                            "expiry %s.", config_interval,
-                            notAfter.strftime("%Y-%m-%d %H:%M:%S %Z"))
-            return True
+        if config_interval is not None:
+            notAfter = crypto_util.notAfter(cert)
+            if notAfter < storage.add_time_interval(now, config_interval):
+                logger.debug("Should renew, less than %s before certificate "
+                                "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
+        elif renewal_time is None:
+            default_renewal_time = _default_renewal_time(cert_pem)
+            if now > default_renewal_time:
+                return True

    return False

@@ -237,9 +237,18 @@ class RenewalTest(test_util.ConfigTestCase):

        mock_renew_cert.assert_called_once()

-    @mock.patch.object(configuration.NamespaceConfig, 'set_by_user')
+    def test_default_renewal_time(self):
+        from certbot._internal import renewal
+        cert_pem = make_cert_with_lifetime(datetime.datetime(2025, 3, 12, 00, 00, 00), 8)
+        t = renewal._default_renewal_time(cert_pem)
+        assert t == datetime.datetime(2025, 3, 16, 00, 00, 00, tzinfo=datetime.timezone.utc)
+
+        cert_pem = make_cert_with_lifetime(datetime.datetime(2025, 3, 12, 00, 00, 00), 18)
+        t = renewal._default_renewal_time(cert_pem)
+        assert t == datetime.datetime(2025, 3, 24, 00, 00, 00, tzinfo=datetime.timezone.utc)
+
    @mock.patch("certbot._internal.renewal.datetime")
-    def test_renew_before_expiry(self, mock_datetime, mock_set_by_user):
+    def test_renew_before_expiry(self, mock_datetime):
        """When neither OCSP nor the ACME client indicate it's time to renew,
           obey the renew_before_expiry config.
        """
@@ -263,7 +272,6 @@ class RenewalTest(test_util.ConfigTestCase):
        mock_renewable_cert.ocsp_revoked.return_value = False

        mock_datetime.timedelta = datetime.timedelta
-        mock_set_by_user.return_value = False

        with tempfile.NamedTemporaryFile() as tmp_cert:
            tmp_cert.close()  # close now because of compatibility issues on Windows
@@ -282,6 +290,10 @@ class RenewalTest(test_util.ConfigTestCase):
                    (1418472000, None, False),
                    # Times that should not renew
                    (1418472000, "4 days", False), (1418472000, "2 days", False),
+                    # 2014-12-16 20:00 (after the default renewal time but before expiry)
+                    # Times that should not renew
+                    (1418760000, None, False),
+                    (1418760000, "1 day", False),
                    # 2009-05-01 12:00:00+00:00 (about 5 years prior to expiry)
                    # Times that should result in autorenewal/autodeployment
                    (1241179200, "7 years", True),
@@ -300,7 +312,43 @@ 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}', expected {result}"
+                assert renewal.should_autorenew(mock_renewable_cert, mock_acme) == 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 [
+                    # 2014-12-13 12:00 (about 5 days prior to expiry)
+                    # Times that should result in autorenewal/autodeployment
+                    (1418472000, "2 months", True), (1418472000, "1 week", True),
+                    # With the "default" logic, this 7-day certificate should autorenew
+                    # at 3.5 days prior to expiry. We haven't reached that yet,
+                    # so don't renew.
+                    (1418472000, None, False),
+                    # Times that should not renew
+                    (1418472000, "4 days", False), (1418472000, "2 days", False),
+                    # 2014-12-16 20:00 (after the default renewal time but before expiry)
+                    # Times that should result in autorenewal/autodeployment
+                    (1418760000, None, True), # Note that this result is different from the above
+                    # Times that should not renew
+                    (1418760000, "1 day", False),
+                    # 2009-05-01 12:00:00+00:00 (about 5 years prior to expiry)
+                    # Times that should result in autorenewal/autodeployment
+                    (1241179200, "7 years", True),
+                    (1241179200, "11 years 2 months", True),
+                    # Times that should not renew
+                    (1241179200, "8 hours", False), (1241179200, "2 days", False),
+                    (1241179200, "40 days", False), (1241179200, "9 months", False),
+                    # 2015-01-01 (after expiry has already happened, so all
+                    #            intervals should cause autorenewal/autodeployment)
+                    (1420070400, "0 seconds", True),
+                    (1420070400, "10 seconds", True),
+                    (1420070400, "10 minutes", True),
+                    (1420070400, "10 weeks", True), (1420070400, "10 months", True),
+                    (1420070400, "10 years", True), (1420070400, "99 months", True),
+            ]:
+                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}"

    @mock.patch.object(configuration.NamespaceConfig, 'set_by_user')
    @mock.patch("certbot._internal.storage.RenewableCert.ocsp_revoked")