diff --git a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py index d8ff5d7c9..07d0f1088 100644 --- a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py +++ b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py @@ -34,7 +34,7 @@ from datetime import datetime import logging import time -from acme.magic_typing import Dict # pylint: disable=unused-import, no-name-in-module +from acme.magic_typing import Dict, Union # pylint: disable=unused-import, no-name-in-module from certbot import errors from certbot import ocsp @@ -90,7 +90,7 @@ class OCSPPrefetchMixin(object): """OCSPPrefetchMixin implements OCSP response prefetching""" def __init__(self, *args, **kwargs): - self._ocsp_prefetch = {} # type: Dict[str, str] + self._ocsp_prefetch = {} # type: Dict[str, Dict[str, Union[str, float]]] # This is required because of python super() call chain. # Additionally, mypy isn't able to figure the chain out and needs to be # disabled for this line. See https://github.com/python/mypy/issues/5887 @@ -121,16 +121,16 @@ class OCSPPrefetchMixin(object): "Apache OCSP stapling cache database.") raise errors.NotSupportedError(msg) - def _ocsp_refresh_needed(self, pf_obj): + def _ocsp_refresh_needed(self, lastupdate): """Refreshes OCSP response for a certificate if it's due - :param dict pf_obj: OCSP prefetch object from pluginstorage + :param int lastupdate: Last update timestamp from pluginstorage entry :returns: If OCSP response was updated :rtype: bool """ - ttl = pf_obj["lastupdate"] + constants.OCSP_INTERNAL_TTL + ttl = lastupdate + constants.OCSP_INTERNAL_TTL if ttl < time.time(): return True return False @@ -150,6 +150,9 @@ class OCSPPrefetchMixin(object): os.path.dirname(self._ocsp_work), apache_util.certid_sha1_hex(cert_path)) handler = ocsp.RevocationChecker() + if not os.path.isfile(cert_path): + raise OCSPCertificateError("Certificate has been removed from the system.") + if not handler.ocsp_revoked_by_paths(cert_path, chain_path, ocsp_workfile): # Guaranteed good response cert_sha = apache_util.certid_sha1(cert_path) @@ -159,6 +162,7 @@ class OCSPPrefetchMixin(object): else: logger.warning("Encountered an issue while trying to prefetch OCSP " "response for certificate: %s", cert_path) + raise OCSPCertificateError("Certificate has been revoked.") finally: try: os.remove(ocsp_workfile) @@ -251,14 +255,21 @@ class OCSPPrefetchMixin(object): """ status = { "lastupdate": time.time(), - "cert_path": cert_path, "chain_path": chain_path } - cert_id = apache_util.certid_sha1_hex(cert_path) - self._ocsp_prefetch[cert_id] = status + self._ocsp_prefetch[cert_path] = status self.storage.put("ocsp_prefetch", self._ocsp_prefetch) self.storage.save() + def _ocsp_prefetch_remove(self, cert_path): + """Removes OCSP prefetch configuration from PluginStorage object for + a certificate. + + :param str cert_path: Filesystem path to certificate + """ + self._ocsp_prefetch.pop(cert_path) + self.storage.put("ocsp_prefetch", self._ocsp_prefetch) + def _ocsp_prefetch_fetch_state(self): """ Populates the OCSP prefetch state from the pluginstorage. @@ -365,16 +376,24 @@ class OCSPPrefetchMixin(object): # No OCSP prefetching enabled for any certificate return - for pf in self._ocsp_prefetch.values(): - if self._ocsp_refresh_needed(pf): + # make a copy of the list of dictionary keys as we might remove items mid-iteration + for cert_path in list(self._ocsp_prefetch): + pf = self._ocsp_prefetch[cert_path] + if self._ocsp_refresh_needed(pf["lastupdate"]): try: - self._ocsp_refresh(pf["cert_path"], pf["chain_path"]) + self._ocsp_refresh(cert_path, pf["chain_path"]) + except OCSPCertificateError as err: + self._ocsp_prefetch_remove(cert_path) + msg = ("Error when trying to prefetch OCSP staple: {} " + + "OCSP prefetch functionality removed for the certificate").format(err) + logger.warning(msg) + continue except errors.PluginError as err: msg = "Encountered a issue when trying to renew OCSP staple: {}".format(err) logger.warning(msg) # Save the status to pluginstorage. Do this even if errored out to prevent # spurious errors when handling multiple renewals. - self._ocsp_prefetch_save(pf["cert_path"], pf["chain_path"]) + self._ocsp_prefetch_save(cert_path, pf["chain_path"]) def restart(self): """Reloads the Apache server. When restarting, Apache deletes @@ -404,4 +423,8 @@ class OCSPPrefetchMixin(object): self._ocsp_prefetch_restore_db() +class OCSPCertificateError(errors.PluginError): + """Error that prompts for removal of certificate from OCSP prefetch pool.""" + + OCSPPrefetchEnhancement.register(OCSPPrefetchMixin) # pylint: disable=no-member diff --git a/certbot-apache/tests/ocsp_prefetch_test.py b/certbot-apache/tests/ocsp_prefetch_test.py index 98d5d4662..f9ad569a2 100644 --- a/certbot-apache/tests/ocsp_prefetch_test.py +++ b/certbot-apache/tests/ocsp_prefetch_test.py @@ -4,6 +4,8 @@ from datetime import datetime from datetime import timedelta import json import sys +import tempfile +import time import unittest import mock @@ -89,7 +91,8 @@ class OCSPPrefetchTest(util.ApacheTest): self.config_path, self.vhost_path, self.config_dir, self.work_dir, os_info="debian") - self.lineage = mock.MagicMock(cert_path="cert", chain_path="chain") + _, self.tmp_certfile = tempfile.mkstemp() + self.lineage = mock.MagicMock(cert_path=self.tmp_certfile, chain_path="chain") self.config.parser.modules.add("headers_module") self.config.parser.modules.add("mod_headers.c") self.config.parser.modules.add("ssl_module") @@ -102,6 +105,9 @@ class OCSPPrefetchTest(util.ApacheTest): self.config._ensure_ocsp_dirs() self.db_path = os.path.join(self.work_dir, "ocsp", "ocsp_cache") + ".db" + def tearDown(self): + os.remove(self.tmp_certfile) + def _call_mocked(self, func, *args, **kwargs): """Helper method to call functins with mock stack""" @@ -154,7 +160,7 @@ class OCSPPrefetchTest(util.ApacheTest): ref_path = "certbot_apache._internal.override_debian.DebianConfigurator._ocsp_refresh" with mock.patch(ref_path): self.call_mocked_py2(self.config.enable_ocsp_prefetch, - self.lineage, ["ocspvhost.com"]) + self.lineage, ["ocspvhost.com"]) self.assertTrue(mock_enable.called) self.assertEqual(len(self.config._ocsp_prefetch), 1) @@ -211,35 +217,27 @@ class OCSPPrefetchTest(util.ApacheTest): self.call_mocked_py2(self.config.update_ocsp_prefetch, None) self.assertTrue(mock_ocsp.called) - def test_ocsp_prefetch_refresh_noop(self): - def ocsp_req_mock(_cert, _chain, workfile): - """Method to mock the OCSP request and write response to file""" - with open(workfile, 'w') as fh: - fh.write("MOCKRESPONSE") - return True - - ocsp_path = "certbot.ocsp.RevocationChecker.ocsp_revoked_by_paths" - with mock.patch(ocsp_path, side_effect=ocsp_req_mock): - self.call_mocked_py2(self.config.enable_ocsp_prefetch, - self.lineage, ["ocspvhost.com"]) + def test_ocsp_prefetch_refresh_cert_deleted(self): + self.config._ocsp_prefetch_save("nonexistent_cert_path", "irrelevant") self.assertEqual(len(self.config._ocsp_prefetch), 1) + rn_path = "certbot_apache._internal.override_debian.DebianConfigurator._ocsp_refresh_needed" + with mock.patch(rn_path) as mock_needed: + mock_needed.return_value = True + with mock.patch("certbot_apache._internal.prefetch_ocsp.logger.warning") as mock_log: + self.call_mocked_py2(self.config.update_ocsp_prefetch, None) + self.assertTrue(mock_log.called) + self.assertTrue("has been removed" in mock_log.call_args[0][0]) + self.assertEqual(len(self.config._ocsp_prefetch), 0) + + def test_ocsp_prefetch_refresh_noop(self): + self.config._ocsp_prefetch_save(self.lineage.cert_path, "irrelevant") refresh_path = "certbot_apache._internal.override_debian.DebianConfigurator._ocsp_refresh" with mock.patch(refresh_path) as mock_refresh: self.call_mocked_py2(self.config.update_ocsp_prefetch, None) self.assertFalse(mock_refresh.called) def test_ocsp_prefetch_refresh_error(self): - def ocsp_req_mock(_cert, _chain, workfile): - """Method to mock the OCSP request and write response to file""" - with open(workfile, 'w') as fh: - fh.write("MOCKRESPONSE") - return True - - ocsp_path = "certbot.ocsp.RevocationChecker.ocsp_revoked_by_paths" - with mock.patch(ocsp_path, side_effect=ocsp_req_mock): - self.call_mocked_py2(self.config.enable_ocsp_prefetch, - self.lineage, ["ocspvhost.com"]) - self.assertEqual(len(self.config._ocsp_prefetch), 1) + self.config._ocsp_prefetch_save(self.lineage.cert_path, "irrelevant") refresh_path = "certbot_apache._internal.override_debian.DebianConfigurator._ocsp_refresh" rn_path = "certbot_apache._internal.override_debian.DebianConfigurator._ocsp_refresh_needed" with mock.patch(refresh_path) as mock_refresh: @@ -293,8 +291,11 @@ class OCSPPrefetchTest(util.ApacheTest): with mock.patch(ocsp_path) as mock_ocsp: mock_ocsp.return_value = True with mock.patch(log_path) as mock_log: - self.call_mocked_py2(self.config.enable_ocsp_prefetch, - self.lineage, ["ocspvhost.com"]) + self.assertRaises(errors.PluginError, + self.call_mocked_py2, + self.config.enable_ocsp_prefetch, + self.lineage, ["ocspvhost.com"] + ) self.assertTrue(mock_log.called) self.assertTrue( "trying to prefetch OCSP" in mock_log.call_args[0][0])