upstream/certbot
1
0
Fork 0
mirror of https://github.com/certbot/certbot.git synced 2026-06-06 23:32:06 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

Address review comments

Browse source
This commit is contained in:
Joona Hoikkala 2020-04-09 02:42:26 +03:00
parent 895330e009
commit 57cd0c7d81
No known key found for this signature in database
GPG key ID: D5AA86BBF9B29A5C
3 changed files with 105 additions and 46 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
certbot-apache/certbot_apache/_internal/constants.py
View file

@ -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"""

92
certbot-apache/certbot_apache/_internal/prefetch_ocsp.py
View file

@ -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):

56
certbot-apache/tests/ocsp_prefetch_test.py
View file

@ -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")