diff --git a/acme/src/acme/_internal/tests/client_test.py b/acme/src/acme/_internal/tests/client_test.py index 419bbda93..42b910f1a 100644 --- a/acme/src/acme/_internal/tests/client_test.py +++ b/acme/src/acme/_internal/tests/client_test.py @@ -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): diff --git a/acme/src/acme/client.py b/acme/src/acme/client.py index ffb989f19..eea184619 100644 --- a/acme/src/acme/client.py +++ b/acme/src/acme/client.py @@ -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()) diff --git a/certbot/src/certbot/_internal/renewal.py b/certbot/src/certbot/_internal/renewal.py index 744bb4332..cc2bf2a1b 100644 --- a/certbot/src/certbot/_internal/renewal.py +++ b/certbot/src/certbot/_internal/renewal.py @@ -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 diff --git a/certbot/src/certbot/_internal/tests/renewal_test.py b/certbot/src/certbot/_internal/tests/renewal_test.py index 5ca6038fa..a5951e626 100644 --- a/certbot/src/certbot/_internal/tests/renewal_test.py +++ b/certbot/src/certbot/_internal/tests/renewal_test.py @@ -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")