Check OCSP as part of determining if the certificate is due for renew…

…al (#7829)

Fixes #1028.

Doing this now because of https://community.letsencrypt.org/t/revoking-certain-certificates-on-march-4/.

The new `ocsp_revoked_by_paths` function  is taken from #7649 with the optional argument removed for now because it is unused.

This function was added in this PR because `storage.py` uses `self.latest_common_version()` to determine which certificate should be looked at for determining renewal status at https://github.com/certbot/certbot/blob/9f8e4507ad0cb3dbedb726dda4c46affb1eb7ad3/certbot/certbot/_internal/storage.py#L939-L947

I think this is unnecessary and you can just look at the currently linked certificate, but I don't think we should be changing the logic that code has always had now.

* Check OCSP status as part of determining to renew

* add integration tests

* add ocsp_revoked_by_paths

certbot-ci/certbot_integration_tests/certbot_tests/test_main.py

@@ -595,6 +595,23 @@ def test_ocsp_status_live(context):
     assert output.count('REVOKED') == 1, 'Expected {0} to be REVOKED'.format(cert)
 
 
+def test_ocsp_renew(context):
+    """Test that revoked certificates are renewed."""
+    # Obtain a certificate
+    certname = context.get_domain('ocsp-renew')
+    context.certbot(['--domains', certname])
+
+    # Test that "certbot renew" does not renew the certificate
+    assert_cert_count_for_lineage(context.config_dir, certname, 1)
+    context.certbot(['renew'], force_renew=False)
+    assert_cert_count_for_lineage(context.config_dir, certname, 1)
+
+    # Revoke the certificate and test that it does renew the certificate
+    context.certbot(['revoke', '--cert-name', certname, '--no-delete-after-revoke'])
+    context.certbot(['renew'], force_renew=False)
+    assert_cert_count_for_lineage(context.config_dir, certname, 2)
+
+
 def test_dry_run_deactivate_authzs(context):
     """Test that Certbot deactivates authorizations when performing a dry run"""
 

certbot/CHANGELOG.md

@@ -13,6 +13,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Changed
 
+* Certbot will now renew certificates early if they have been revoked according
+  to OCSP.
 * Fix acme module warnings when response Content-Type includes params (e.g. charset).
 * Fixed issue where webroot plugin would incorrectly raise `Read-only file system` 
   error when creating challenge directories (issue #7165).

certbot/certbot/_internal/storage.py

@@ -15,6 +15,7 @@
 from certbot import crypto_util
 from certbot import errors
 from certbot import interfaces
+from certbot import ocsp
 from certbot import util
 from certbot._internal import cli
 from certbot._internal import constants
@@ -882,27 +883,33 @@ def names(self):
         with open(target) as f:
             return crypto_util.get_names_from_cert(f.read())
 
-    def ocsp_revoked(self, version=None):
-        # pylint: disable=unused-argument
+    def ocsp_revoked(self, version):
         """Is the specified cert version revoked according to OCSP?
 
-        Also returns True if the cert version is declared as intended
-        to be revoked according to Let's Encrypt OCSP extensions.
-        (If no version is specified, uses the current version.)
-
-        This method is not yet implemented and currently always returns
-        False.
+        Also returns True if the cert version is declared as revoked
+        according to OCSP. If OCSP status could not be determined, False
+        is returned.
 
         :param int version: the desired version number
 
-        :returns: whether the certificate is or will be revoked
+        :returns: True if the certificate is revoked, otherwise, False
         :rtype: bool
 
         """
-        # XXX: This query and its associated network service aren't
-        # implemented yet, so we currently return False (indicating that the
-        # certificate is not revoked).
-        return False
+        cert_path = self.version("cert", version)
+        chain_path = self.version("chain", version)
+        # While the RevocationChecker should return False if it failed to
+        # determine the OCSP status, let's ensure we don't crash Certbot by
+        # catching all exceptions here.
+        try:
+            return ocsp.RevocationChecker().ocsp_revoked_by_paths(cert_path,
+                                                                  chain_path)
+        except Exception as e:  # pylint: disable=broad-except
+            logger.warning(
+                "An error occurred determining the OCSP status of %s.",
+                cert_path)
+            logger.debug(str(e))
+            return False
 
     def autorenewal_is_enabled(self):
         """Is automatic renewal enabled for this cert?

certbot/certbot/ocsp.py

@@ -68,8 +68,19 @@ def ocsp_revoked(self, cert):
         :rtype: bool
 
         """
-        cert_path, chain_path = cert.cert_path, cert.chain_path
+        return self.ocsp_revoked_by_paths(cert.cert_path, cert.chain_path)
 
+    def ocsp_revoked_by_paths(self, cert_path, chain_path):
+        # type: (str, str) -> bool
+        """Performs the OCSP revocation check
+
+        :param str cert_path: Certificate filepath
+        :param str chain_path: Certificate chain filepath
+
+        :returns: True if revoked; False if valid or the check failed or cert is expired.
+        :rtype: bool
+
+        """
         if self.broken:
             return False
 

certbot/tests/storage_test.py

@@ -672,10 +672,35 @@ def test_bad_kind(self):
             errors.CertStorageError,
             self.test_rc._update_link_to, "elephant", 17)
 
-    def test_ocsp_revoked(self):
-        # XXX: This is currently hardcoded to False due to a lack of an
-        #      OCSP server to test against.
-        self.assertFalse(self.test_rc.ocsp_revoked())
+    @mock.patch("certbot.ocsp.RevocationChecker.ocsp_revoked_by_paths")
+    def test_ocsp_revoked(self, mock_checker):
+        # Write out test files
+        for kind in ALL_FOUR:
+            self._write_out_kind(kind, 1)
+        version = self.test_rc.latest_common_version()
+        expected_cert_path = self.test_rc.version("cert", version)
+        expected_chain_path = self.test_rc.version("chain", version)
+
+        # Test with cert revoked
+        mock_checker.return_value = True
+        self.assertTrue(self.test_rc.ocsp_revoked(version))
+        self.assertEqual(mock_checker.call_args[0][0], expected_cert_path)
+        self.assertEqual(mock_checker.call_args[0][1], expected_chain_path)
+
+        # Test with cert not revoked
+        mock_checker.return_value = False
+        self.assertFalse(self.test_rc.ocsp_revoked(version))
+        self.assertEqual(mock_checker.call_args[0][0], expected_cert_path)
+        self.assertEqual(mock_checker.call_args[0][1], expected_chain_path)
+
+        # Test with error
+        mock_checker.side_effect = ValueError
+        with mock.patch("certbot._internal.storage.logger.warning") as logger:
+            self.assertFalse(self.test_rc.ocsp_revoked(version))
+        self.assertEqual(mock_checker.call_args[0][0], expected_cert_path)
+        self.assertEqual(mock_checker.call_args[0][1], expected_chain_path)
+        log_msg = logger.call_args[0][0]
+        self.assertIn("An error occurred determining the OCSP status", log_msg)
 
     def test_add_time_interval(self):
         from certbot._internal import storage