From 18e6c6c2a883fcfae44a84524d0be57f99b2b357 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Mon, 23 Sep 2019 20:20:11 -0400 Subject: [PATCH] Don't send OCSP requests for expired certificates (#7387) Fixes #7152. * don't check ocsp if cert is expired when getting cert information * don't check ocsp if the cert is expired in ocsp_revoked * update tests * update changelog * move pytz import to the top of the test file --- CHANGELOG.md | 2 +- certbot/cert_manager.py | 2 +- certbot/ocsp.py | 20 ++++++++++---- certbot/tests/ocsp_test.py | 55 +++++++++++++++++++++++++------------- 4 files changed, 54 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 075da8376..f02e8fda3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Changed -* +* Don't send OCSP requests for expired certificates ### Fixed diff --git a/certbot/cert_manager.py b/certbot/cert_manager.py index 6d6d2e2e6..cd228ac12 100644 --- a/certbot/cert_manager.py +++ b/certbot/cert_manager.py @@ -262,7 +262,7 @@ def human_readable_cert_info(config, cert, skip_filter_checks=False): reasons.append('TEST_CERT') if cert.target_expiry <= now: reasons.append('EXPIRED') - if checker.ocsp_revoked(cert.cert, cert.chain): + elif checker.ocsp_revoked(cert): reasons.append('REVOKED') if reasons: diff --git a/certbot/ocsp.py b/certbot/ocsp.py index 0e35f023f..1cc1e7529 100644 --- a/certbot/ocsp.py +++ b/certbot/ocsp.py @@ -16,11 +16,13 @@ from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization from cryptography.hazmat.primitives import hashes # type: ignore from cryptography.exceptions import UnsupportedAlgorithm, InvalidSignature +import pytz import requests from acme.magic_typing import Optional, Tuple # pylint: disable=unused-import, no-name-in-module from certbot import crypto_util from certbot import errors +from certbot.storage import RenewableCert # pylint: disable=unused-import from certbot import util logger = logging.getLogger(__name__) @@ -48,21 +50,29 @@ class RevocationChecker(object): else: self.host_args = lambda host: ["Host", host] - def ocsp_revoked(self, cert_path, chain_path): - # type: (str, str) -> bool + def ocsp_revoked(self, cert): + # type: (RenewableCert) -> bool """Get revoked status for a particular cert version. .. todo:: Make this a non-blocking call - :param str cert_path: Path to certificate - :param str chain_path: Path to intermediate cert - :returns: True if revoked; False if valid or the check failed + :param `.storage.RenewableCert` cert: Certificate object + :returns: True if revoked; False if valid or the check failed or cert is expired. :rtype: bool """ + cert_path, chain_path = cert.cert, cert.chain + if self.broken: return False + # Let's Encrypt doesn't update OCSP for expired certificates, + # so don't check OCSP if the cert is expired. + # https://github.com/certbot/certbot/issues/7152 + now = pytz.UTC.fromutc(datetime.utcnow()) + if cert.target_expiry <= now: + return False + url, host = _determine_ocsp_server(cert_path) if not host or not url: return False diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index e8c1b9d03..680e2c2bb 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -16,6 +16,7 @@ try: except (ImportError, AttributeError): # pragma: no cover ocsp_lib = None # type: ignore import mock +import pytz from certbot import errors from certbot.tests import util as test_util @@ -72,21 +73,34 @@ class OCSPTestOpenSSL(unittest.TestCase): @mock.patch('certbot.ocsp._determine_ocsp_server') @mock.patch('certbot.util.run_script') def test_ocsp_revoked(self, mock_run, mock_determine): + now = pytz.UTC.fromutc(datetime.utcnow()) + cert_obj = mock.MagicMock() + cert_obj.cert = "x" + cert_obj.chain = "y" + cert_obj.target_expiry = now + timedelta(hours=2) + self.checker.broken = True mock_determine.return_value = ("", "") - self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) + self.assertEqual(self.checker.ocsp_revoked(cert_obj), False) self.checker.broken = False mock_run.return_value = tuple(openssl_happy[1:]) - self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) + self.assertEqual(self.checker.ocsp_revoked(cert_obj), False) self.assertEqual(mock_run.call_count, 0) mock_determine.return_value = ("http://x.co", "x.co") - self.assertEqual(self.checker.ocsp_revoked("blah.pem", "chain.pem"), False) + self.assertEqual(self.checker.ocsp_revoked(cert_obj), False) mock_run.side_effect = errors.SubprocessError("Unable to load certificate launcher") - self.assertEqual(self.checker.ocsp_revoked("x", "y"), False) + self.assertEqual(self.checker.ocsp_revoked(cert_obj), False) self.assertEqual(mock_run.call_count, 2) + # cert expired + cert_obj.target_expiry = now + mock_determine.return_value = ("", "") + count_before = mock_determine.call_count + self.assertEqual(self.checker.ocsp_revoked(cert_obj), False) + self.assertEqual(mock_determine.call_count, count_before) + def test_determine_ocsp_server(self): cert_path = test_util.vector_path('ocsp_certificate.pem') @@ -131,18 +145,23 @@ class OSCPTestCryptography(unittest.TestCase): self.checker = ocsp.RevocationChecker() self.cert_path = test_util.vector_path('ocsp_certificate.pem') self.chain_path = test_util.vector_path('ocsp_issuer_certificate.pem') + self.cert_obj = mock.MagicMock() + self.cert_obj.cert = self.cert_path + self.cert_obj.chain = self.chain_path + now = pytz.UTC.fromutc(datetime.utcnow()) + self.cert_obj.target_expiry = now + timedelta(hours=2) @mock.patch('certbot.ocsp._determine_ocsp_server') @mock.patch('certbot.ocsp._check_ocsp_cryptography') def test_ensure_cryptography_toggled(self, mock_revoke, mock_determine): mock_determine.return_value = ('http://example.com', 'example.com') - self.checker.ocsp_revoked(self.cert_path, self.chain_path) + self.checker.ocsp_revoked(self.cert_obj) mock_revoke.assert_called_once_with(self.cert_path, self.chain_path, 'http://example.com') def test_revoke(self): with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertTrue(revoked) def test_responder_is_issuer(self): @@ -152,7 +171,7 @@ class OSCPTestCryptography(unittest.TestCase): with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL) as mocks: mocks['mock_response'].return_value.responder_name = issuer.subject - self.checker.ocsp_revoked(self.cert_path, self.chain_path) + self.checker.ocsp_revoked(self.cert_obj) # Here responder and issuer are the same. So only the signature of the OCSP # response is checked (using the issuer/responder public key). self.assertEqual(mocks['mock_check'].call_count, 1) @@ -167,7 +186,7 @@ class OSCPTestCryptography(unittest.TestCase): with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL) as mocks: - self.checker.ocsp_revoked(self.cert_path, self.chain_path) + self.checker.ocsp_revoked(self.cert_obj) # Here responder and issuer are not the same. Two signatures will be checked then, # first to verify the responder cert (using the issuer public key), second to # to verify the OCSP response itself (using the responder public key). @@ -181,17 +200,17 @@ class OSCPTestCryptography(unittest.TestCase): # Server return an invalid HTTP response with _ocsp_mock(ocsp_lib.OCSPCertStatus.UNKNOWN, ocsp_lib.OCSPResponseStatus.SUCCESSFUL, http_status_code=400): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # OCSP response in invalid with _ocsp_mock(ocsp_lib.OCSPCertStatus.UNKNOWN, ocsp_lib.OCSPResponseStatus.UNAUTHORIZED): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # OCSP response is valid, but certificate status is unknown with _ocsp_mock(ocsp_lib.OCSPCertStatus.UNKNOWN, ocsp_lib.OCSPResponseStatus.SUCCESSFUL): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # The OCSP response says that the certificate is revoked, but certificate @@ -200,32 +219,32 @@ class OSCPTestCryptography(unittest.TestCase): with mock.patch('cryptography.x509.Extensions.get_extension_for_class', side_effect=x509.ExtensionNotFound( 'Not found', x509.AuthorityInformationAccessOID.OCSP)): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # OCSP response uses an unsupported signature. with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL, check_signature_side_effect=UnsupportedAlgorithm('foo')): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # OSCP signature response is invalid. with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL, check_signature_side_effect=InvalidSignature('foo')): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # Assertion error on OCSP response validity with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL, check_signature_side_effect=AssertionError('foo')): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # No responder cert in OCSP response with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL) as mocks: mocks['mock_response'].return_value.certificates = [] - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) # Responder cert is not signed by certificate issuer @@ -234,7 +253,7 @@ class OSCPTestCryptography(unittest.TestCase): cert = mocks['mock_response'].return_value.certificates[0] mocks['mock_response'].return_value.certificates[0] = mock.Mock( issuer='fake', subject=cert.subject) - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked) with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL): @@ -245,7 +264,7 @@ class OSCPTestCryptography(unittest.TestCase): with mock.patch('cryptography.x509.Extensions.get_extension_for_class', side_effect=x509.ExtensionNotFound( 'Not found', x509.AuthorityInformationAccessOID.OCSP)): - revoked = self.checker.ocsp_revoked(self.cert_path, self.chain_path) + revoked = self.checker.ocsp_revoked(self.cert_obj) self.assertFalse(revoked)