diff --git a/certbot-apache/certbot_apache/_internal/constants.py b/certbot-apache/certbot_apache/_internal/constants.py index 358449bb7..6bac521c0 100644 --- a/certbot-apache/certbot_apache/_internal/constants.py +++ b/certbot-apache/certbot_apache/_internal/constants.py @@ -70,8 +70,5 @@ MANAGED_COMMENT = "DO NOT REMOVE - Managed by Certbot" MANAGED_COMMENT_ID = MANAGED_COMMENT+", VirtualHost id: {0}" """Managed by Certbot comments and the VirtualHost identification template""" -OCSP_APACHE_TTL = 432000 -"""Apache TTL for OCSP response in seconds: 5 days""" - OCSP_INTERNAL_TTL = 86400 """Internal TTL for OCSP response in seconds: 1 day""" diff --git a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py index afc2f398c..d8ff5d7c9 100644 --- a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py +++ b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py @@ -95,12 +95,13 @@ class OCSPPrefetchMixin(object): # 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 super(OCSPPrefetchMixin, self).__init__(*args, **kwargs) # type: ignore + self._ocsp_store = os.path.join(self.config.work_dir, "ocsp", "ocsp_cache.db") + self._ocsp_work = os.path.join(self.config.work_dir, "ocsp_work", "ocsp_cache.db") def _ensure_ocsp_dirs(self): """Makes sure that the OCSP directory paths exist.""" - ocsp_work = os.path.join(self.config.work_dir, "ocsp_work") - ocsp_save = os.path.join(self.config.work_dir, "ocsp") - for path in [ocsp_work, ocsp_save]: + for path in [os.path.dirname(self._ocsp_work), + os.path.dirname(self._ocsp_store)]: if not os.path.isdir(path): filesystem.makedirs(path, 0o755) @@ -121,7 +122,7 @@ class OCSPPrefetchMixin(object): raise errors.NotSupportedError(msg) def _ocsp_refresh_needed(self, pf_obj): - """Refreshes OCSP response for a certiifcate if it's due + """Refreshes OCSP response for a certificate if it's due :param dict pf_obj: OCSP prefetch object from pluginstorage @@ -144,25 +145,26 @@ class OCSPPrefetchMixin(object): """ self._ensure_ocsp_dirs() - ocsp_workfile = os.path.join( - self.config.work_dir, "ocsp_work", - apache_util.certid_sha1_hex(cert_path)) - handler = ocsp.RevocationChecker() - if not handler.ocsp_revoked_by_paths(cert_path, chain_path, ocsp_workfile): - # Guaranteed good response - cache_path = os.path.join(self.config.work_dir, "ocsp", "ocsp_cache.db") - cert_sha = apache_util.certid_sha1(cert_path) - # dbm.open automatically adds the file extension - self._write_to_dbm(cache_path, cert_sha, self._ocsp_response_dbm(ocsp_workfile)) - else: - logger.warning("Encountered an issue while trying to prefetch OCSP " - "response for certificate: %s", cert_path) - # Clean up try: - os.remove(ocsp_workfile) - except OSError: - # The OCSP workfile did not exist because of an OCSP response fetching error - return + ocsp_workfile = os.path.join( + os.path.dirname(self._ocsp_work), + apache_util.certid_sha1_hex(cert_path)) + handler = ocsp.RevocationChecker() + if not handler.ocsp_revoked_by_paths(cert_path, chain_path, ocsp_workfile): + # Guaranteed good response + cert_sha = apache_util.certid_sha1(cert_path) + # dbm.open automatically adds the file extension + self._write_to_dbm(self._ocsp_store, cert_sha, + self._ocsp_response_dbm(ocsp_workfile)) + else: + logger.warning("Encountered an issue while trying to prefetch OCSP " + "response for certificate: %s", cert_path) + finally: + try: + os.remove(ocsp_workfile) + except OSError: + # The OCSP workfile did not exist because of an OCSP response fetching error + pass def _write_to_dbm(self, filename, key, value): """Helper method to write an OCSP response cache value to DBM. @@ -172,8 +174,7 @@ class OCSPPrefetchMixin(object): :param bytes value: Database entry value """ tmp_file = os.path.join( - self.config.work_dir, - "ocsp_work", + os.path.dirname(self._ocsp_work), "tmp_" + os.path.basename(filename) ) @@ -199,13 +200,28 @@ class OCSPPrefetchMixin(object): :returns: TTL in seconds. :rtype: int """ - + # hour in seconds + hour = 3600 + suberror = "" if next_update is not None: now = datetime.utcnow() res_ttl = int((next_update - now).total_seconds()) if res_ttl > 0: - return res_ttl/2 - return constants.OCSP_APACHE_TTL + safe_ttl = res_ttl - 30 * hour + if safe_ttl > hour: + # Use nextUpdate - 30h if it's over an hour from now + return safe_ttl + else: + suberror = ("OCSP response nextUpdate timestamp too " + "early: {}. Certbot cannot ensure a safe TTL" + "for OCSP staple prefeching.").format(next_update) + else: + suberror = ("OCSP response nextUpdate timestamp too " + "early: {}").format(next_update) + else: + suberror = ("OCSP response nextUpdate not provided with response. " + "Staple should not be prefetched.") + raise errors.PluginError(suberror) def _ocsp_response_dbm(self, workfile): """Creates a dbm entry for OCSP response data @@ -262,11 +278,11 @@ class OCSPPrefetchMixin(object): Erroring out here would prevent any restarts done by Apache plugin. """ self._ensure_ocsp_dirs() - cache_path = os.path.join(self.config.work_dir, "ocsp", "ocsp_cache.db") try: apache_util.safe_copy( - cache_path, - os.path.join(self.config.work_dir, "ocsp_work", "ocsp_cache.db")) + self._ocsp_store, + self._ocsp_work + ) except errors.PluginError: logger.debug("Encountered an issue while trying to backup OCSP dbm file") @@ -277,12 +293,11 @@ class OCSPPrefetchMixin(object): prevent other critical functions that need to be carried out for Apache httpd. + Erroring out here would prevent any restarts done by Apache plugin. """ self._ensure_ocsp_dirs() - cache_path = os.path.join(self.config.work_dir, "ocsp", "ocsp_cache.db") - work_file_path = os.path.join(self.config.work_dir, "ocsp_work", "ocsp_cache.db") try: - filesystem.replace(work_file_path, cache_path) + filesystem.replace(self._ocsp_work, self._ocsp_store) except IOError: logger.debug("Encountered an issue when trying to restore OCSP dbm file") @@ -338,7 +353,7 @@ class OCSPPrefetchMixin(object): self.recovery_routine() self.restart() msg = ("Encountered an error while trying to enable OCSP prefetch " - "enhancement: %s\nOCSP prefetch was not enabled.") + "enhancement: %s\nOCSP prefetch was not enabled.") raise errors.PluginError(msg % str(err)) def update_ocsp_prefetch(self, _unused_lineage): @@ -352,8 +367,13 @@ class OCSPPrefetchMixin(object): for pf in self._ocsp_prefetch.values(): if self._ocsp_refresh_needed(pf): - self._ocsp_refresh(pf["cert_path"], pf["chain_path"]) - # Save the status to pluginstorage + try: + self._ocsp_refresh(pf["cert_path"], pf["chain_path"]) + 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"]) def restart(self): diff --git a/certbot-apache/tests/ocsp_prefetch_test.py b/certbot-apache/tests/ocsp_prefetch_test.py index a687d047c..98d5d4662 100644 --- a/certbot-apache/tests/ocsp_prefetch_test.py +++ b/certbot-apache/tests/ocsp_prefetch_test.py @@ -228,6 +228,28 @@ class OCSPPrefetchTest(util.ApacheTest): 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) + 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: + mock_refresh.side_effect = errors.PluginError("Could not update") + with mock.patch("certbot_apache._internal.prefetch_ocsp.logger.warning") as mock_log: + with mock.patch(rn_path) as mock_needed: + mock_needed.return_value = True + self.call_mocked_py2(self.config.update_ocsp_prefetch, None) + self.assertTrue(mock_log.called) + @mock.patch("certbot_apache._internal.configurator.ApacheConfigurator.config_test") def test_ocsp_prefetch_backup_db(self, _mock_test): def ocsp_del_db(): @@ -374,14 +396,34 @@ class OCSPPrefetchTest(util.ApacheTest): b'anything', b'irrelevant' ) - @mock.patch("certbot_apache._internal.constants.OCSP_APACHE_TTL", 1234) - def test_ttl(self): - self.assertEqual(self.config._ocsp_ttl(None), 1234) - next_update = datetime.today() + timedelta(days=6) + def test_ttl_no_nextupdate(self): + self.assertRaises(errors.PluginError, + self.config._ocsp_ttl, + None) + + def test_ttl_nextupdate_in_past(self): + next_update = datetime.utcnow() - timedelta(hours=1) + self.assertRaises(errors.PluginError, + self.config._ocsp_ttl, + next_update) + + def test_ttl_nextupdate_not_enough_leeway(self): + next_update = datetime.utcnow() + timedelta(hours=29) + self.assertRaises(errors.PluginError, + self.config._ocsp_ttl, + next_update) + + def test_ttl_ok(self): + next_update = datetime.utcnow() + timedelta(hours=32) ttl = self.config._ocsp_ttl(next_update) - # ttl should be roughly 3 days - self.assertTrue(ttl > 86400*2) - self.assertTrue(ttl < 86400*4) + self.assertTrue(ttl > 7100 and ttl < 7201) + + def test_ttl(self): + next_update = datetime.utcnow() + timedelta(days=6) + ttl = self.config._ocsp_ttl(next_update) + # ttl should be 30h from next_update + self.assertTrue(ttl < timedelta(days=4, hours=19).total_seconds()) + self.assertTrue(ttl > timedelta(days=4, hours=17).total_seconds()) @mock.patch("certbot_apache._internal.prefetch_ocsp.OCSPPrefetchMixin._ocsp_prefetch_fetch_state") @mock.patch("certbot_apache._internal.configurator.ApacheConfigurator.config_test")