From 53f8ad88dbc80b6d7c4e39932b9694c41c85a0b2 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 26 Jan 2020 15:42:01 +0200 Subject: [PATCH] Enable OCSP and revocation checking based on certificate and chain filepaths --- .../certbot_apache/_internal/prefetch_ocsp.py | 2 +- certbot/certbot/_internal/ocsp.py | 20 ++++++++--------- certbot/tests/ocsp_test.py | 22 ++++++++++++------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py index b6d587249..90923a789 100644 --- a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py +++ b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py @@ -115,7 +115,7 @@ class OCSPPrefetchMixin(object): self.config.work_dir, "ocsp", apache_util.certid_sha1_hex(cert_path)) handler = ocsp.RevocationChecker() - if not handler.revoked(cert_path, chain_path, ocsp_workfile): + if not handler.ocsp_revoked_cert(cert_path, chain_path, ocsp_workfile): # Guaranteed good response cache_path = os.path.join(self.config.config_dir, "ocsp", "ocsp_cache") # dbm.open automatically adds the file extension, it will be diff --git a/certbot/certbot/_internal/ocsp.py b/certbot/certbot/_internal/ocsp.py index 19bc3c96c..c16031f0f 100644 --- a/certbot/certbot/_internal/ocsp.py +++ b/certbot/certbot/_internal/ocsp.py @@ -65,21 +65,14 @@ class RevocationChecker(object): .. todo:: Make this a non-blocking call :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 + return self.ocsp_revoked_cert(cert.cert, cert.chain) - # 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 - return self.revoked(cert_path, chain_path) - - def revoked(self, cert_path, chain_path, response_file=None): + def ocsp_revoked_cert(self, cert_path, chain_path, response_file=None): # type: (str, str, Optional[str]) -> bool """Performs the OCSP revocation check @@ -90,6 +83,13 @@ class RevocationChecker(object): :rtype: bool """ + # 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 crypto_util.notAfter(cert_path) <= now: + return False + if self.broken: return False diff --git a/certbot/tests/ocsp_test.py b/certbot/tests/ocsp_test.py index 5779ef373..b550546c0 100644 --- a/certbot/tests/ocsp_test.py +++ b/certbot/tests/ocsp_test.py @@ -75,13 +75,14 @@ class OCSPTestOpenSSL(unittest.TestCase): self.assertEqual(checker.broken, True) @mock.patch('certbot._internal.ocsp._determine_ocsp_server') + @mock.patch('certbot._internal.ocsp.crypto_util.notAfter') @mock.patch('certbot.util.run_script') - def test_ocsp_revoked(self, mock_run, mock_determine): + def test_ocsp_revoked(self, mock_run, mock_na, 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) + mock_na.return_value = now + timedelta(hours=2) self.checker.broken = True mock_determine.return_value = ("", "") @@ -99,7 +100,7 @@ class OCSPTestOpenSSL(unittest.TestCase): self.assertEqual(mock_run.call_count, 2) # cert expired - cert_obj.target_expiry = now + mock_na.return_value = now mock_determine.return_value = ("", "") count_before = mock_determine.call_count self.assertEqual(self.checker.ocsp_revoked(cert_obj), False) @@ -152,21 +153,26 @@ class OSCPTestCryptography(unittest.TestCase): self.cert_obj = mock.MagicMock() self.cert_obj.cert = self.cert_path self.cert_obj.chain = self.chain_path + + def _call_expirymock(self, func, *args, **kwargs): + """Call function with mocked certificate expiry time""" now = pytz.UTC.fromutc(datetime.utcnow()) - self.cert_obj.target_expiry = now + timedelta(hours=2) + with mock.patch('certbot._internal.ocsp.crypto_util.notAfter') as mock_na: + mock_na.return_value = now + timedelta(hours=2) + return func(*args, **kwargs) @mock.patch('certbot._internal.ocsp._determine_ocsp_server') @mock.patch('certbot._internal.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_obj) + self._call_expirymock(self.checker.ocsp_revoked, self.cert_obj) mock_revoke.assert_called_once_with(self.cert_path, self.chain_path, 'http://example.com', None) def test_revoke(self): with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL): - revoked = self.checker.ocsp_revoked(self.cert_obj) + revoked = self._call_expirymock(self.checker.ocsp_revoked, self.cert_obj) self.assertTrue(revoked) def test_responder_is_issuer(self): @@ -176,7 +182,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_obj) + self._call_expirymock(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) @@ -191,7 +197,7 @@ class OSCPTestCryptography(unittest.TestCase): with _ocsp_mock(ocsp_lib.OCSPCertStatus.REVOKED, ocsp_lib.OCSPResponseStatus.SUCCESSFUL) as mocks: - self.checker.ocsp_revoked(self.cert_obj) + self._call_expirymock(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).