diff --git a/certbot-apache/certbot_apache/_internal/apache_util.py b/certbot-apache/certbot_apache/_internal/apache_util.py index f6cb00e3c..27c169a65 100644 --- a/certbot-apache/certbot_apache/_internal/apache_util.py +++ b/certbot-apache/certbot_apache/_internal/apache_util.py @@ -1,9 +1,12 @@ """ Utility functions for certbot-apache plugin """ import binascii +import hashlib import struct +import shutil import time from certbot import crypto_util +from certbot import errors from certbot import util from certbot.compat import os @@ -49,6 +52,53 @@ def certid_sha1(cert_path): return crypto_util.cert_sha1_fingerprint(cert_path) +def safe_copy(source, target): + """Copies a file, while verifying the target integrity + with the source. Retries twice if the initial + copy fails. + + :param str source: File path of the source file + :param str target: File path of the target file + + :raises: .errors.PluginError: If file cannot be + copied or the target file hash does not match + with the source file. + """ + for _ in range(3): + try: + shutil.copy2(source, target) + except IOError as e: + emsg = "Could not copy {} to {}: {}".format( + source, target, e + ) + raise errors.PluginError(emsg) + try: + source_hash = _file_hash(source) + target_hash = _file_hash(target) + except IOError: + continue + if source_hash == target_hash: + return + raise errors.PluginError( + "Safe copy failed. The file integrity does not match" + ) + + +def _file_hash(filepath): + """Helper function for safe_copy that calculates a + sha-256 hash of file. + + :param str filepath: Path of file to calculate hash for + + :returns: File sha-256 hash + :rtype: str + """ + fhash = hashlib.sha256() + with open(filepath, 'rb') as fh: + fhash.update(fh.read()) + return fhash.hexdigest() + + def get_mod_deps(mod_name): """Get known module dependencies. diff --git a/certbot-apache/certbot_apache/_internal/configurator.py b/certbot-apache/certbot_apache/_internal/configurator.py index c53350946..bb99d8e7d 100644 --- a/certbot-apache/certbot_apache/_internal/configurator.py +++ b/certbot-apache/certbot_apache/_internal/configurator.py @@ -1752,7 +1752,7 @@ class ApacheConfigurator(common.Installer): if prefetch: if "socache_dbm_module" not in self.parser.modules: self.enable_mod("socache_dbm") - cache_path = os.path.join(self.config.config_dir, "ocsp", "ocsp_cache.db") + cache_path = os.path.join(self.config.work_dir, "ocsp", "ocsp_cache.db") cache_dir = ["dbm:"+cache_path] else: if "socache_shmcb_module" not in self.parser.modules: diff --git a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py index ad27c6cab..c95f7e703 100644 --- a/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py +++ b/certbot-apache/certbot_apache/_internal/prefetch_ocsp.py @@ -30,20 +30,18 @@ class DBMHandler(object): def __enter__(self): """Open the DBM file and return the filehandle""" - if not os.path.isfile(self.filename + ".db"): - raise errors.PluginError( - "The OCSP stapling cache DBM file wasn't created by Apache.") try: import bsddb self.bsddb = True - cache_path = self.filename + ".db" try: - self.database = bsddb.hashopen(cache_path, self.filemode) + self.database = bsddb.hashopen(self.filename, self.filemode) except Exception: raise errors.PluginError("Unable to open dbm database file.") except ImportError: # Python3 doesn't have bsddb module, so we use dbm.ndbm instead import dbm + if self.filename.endswith(".db"): + self.filename = self.filename[:-3] try: self.database = dbm.ndbm.open(self.filename, self.filemode) # pylint: disable=no-member except Exception: @@ -63,7 +61,6 @@ class OCSPPrefetchMixin(object): def __init__(self, *args, **kwargs): self._ocsp_prefetch = {} # type: Dict[str, str] - self._ocsp_dbm_bsddb = False # 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 @@ -124,7 +121,7 @@ class OCSPPrefetchMixin(object): 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") + 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)) @@ -139,16 +136,26 @@ class OCSPPrefetchMixin(object): return def _write_to_dbm(self, filename, key, value): - """Helper method to write an OCSP response cache value to DBM + """Helper method to write an OCSP response cache value to DBM. :param filename: DBM database filename :param bytes key: Database key name :param bytes value: Database entry value """ + tmp_file = os.path.join( + self.config.work_dir, + "ocsp_work", + "tmp_" + os.path.basename(filename) + ) - with DBMHandler(filename, 'w') as db: + apache_util.safe_copy(filename, tmp_file) + + with DBMHandler(tmp_file, 'w') as db: db[key] = value + shutil.copy2(tmp_file, filename) + os.remove(tmp_file) + def _read_dbm(self, filename): """Helper method for reading the dbm using context manager. Used for tests. diff --git a/certbot-apache/tests/ocsp_prefetch_test.py b/certbot-apache/tests/ocsp_prefetch_test.py index 6e2f2b19e..2eee50a38 100644 --- a/certbot-apache/tests/ocsp_prefetch_test.py +++ b/certbot-apache/tests/ocsp_prefetch_test.py @@ -3,8 +3,8 @@ import base64 from datetime import datetime from datetime import timedelta import json -import unittest import sys +import unittest import mock # six is used in mock.patch() @@ -98,18 +98,15 @@ class OCSPPrefetchTest(util.ApacheTest): self.vh_truth = util.get_vh_truth( self.temp_dir, "debian_apache_2_4/multiple_vhosts") self.config._ensure_ocsp_dirs() - self.db_path = os.path.join(self.work_dir, "ocsp", "ocsp_cache") - self.db_fullpath = self.db_path + ".db" + self.db_path = os.path.join(self.work_dir, "ocsp", "ocsp_cache") + ".db" def _call_mocked(self, func, *args, **kwargs): """Helper method to call functins with mock stack""" - db_fullpath = self.db_path + ".db" - def mock_restart(): """Mock ApacheConfigurator.restart that creates the dbm file""" # Mock the Apache dbm file creation - open(db_fullpath, 'a').close() + open(self.db_path, 'a').close() ver_path = "certbot_apache._internal.configurator.ApacheConfigurator.get_version" res_path = "certbot_apache._internal.prefetch_ocsp.OCSPPrefetchMixin.restart" @@ -234,11 +231,11 @@ class OCSPPrefetchTest(util.ApacheTest): def ocsp_del_db(): """Side effect of _reload() that deletes the DBM file, like Apache does when restarting""" - os.remove(self.db_fullpath) - self.assertFalse(os.path.isfile(self.db_fullpath)) + os.remove(self.db_path) + self.assertFalse(os.path.isfile(self.db_path)) # Make sure that the db file exists - open(self.db_fullpath, 'a').close() + open(self.db_path, 'a').close() self.call_mocked_py2(self.config._write_to_dbm, self.db_path, b'mock_key', b'mock_value') # Mock OCSP prefetch dict to signify that there should be a db @@ -300,7 +297,7 @@ class OCSPPrefetchTest(util.ApacheTest): self.config._ensure_ocsp_prefetch_compatibility) def test_ocsp_prefetch_open_dbm_no_file(self): - open(self.db_fullpath, 'a').close() + open(self.db_path, 'a').close() db_not_exists = self.db_path+"nonsense" self.call_mocked_py2(self.config._write_to_dbm, self.db_path, b'k', b'v') self.assertRaises(errors.PluginError, @@ -310,7 +307,7 @@ class OCSPPrefetchTest(util.ApacheTest): b'k', b'v') def test_ocsp_prefetch_py2_open_file_error(self): - open(self.db_fullpath, 'a').close() + open(self.db_path, 'a').close() mock_db = mock.MagicMock() mock_db.hashopen.side_effect = Exception("error") sys.modules["bsddb"] = mock_db @@ -320,7 +317,7 @@ class OCSPPrefetchTest(util.ApacheTest): b'k', b'v') def test_ocsp_prefetch_py3_open_file_error(self): - open(self.db_fullpath, 'a').close() + open(self.db_path, 'a').close() mock_db = mock.MagicMock() mock_db.ndbm.open.side_effect = Exception("error") sys.modules["dbm"] = mock_db @@ -332,7 +329,7 @@ class OCSPPrefetchTest(util.ApacheTest): def test_ocsp_prefetch_open_close_py2_noerror(self): expected_val = b'whatever_value' - open(self.db_fullpath, 'a').close() + open(self.db_path, 'a').close() self.call_mocked_py2( self.config._write_to_dbm, self.db_path, b'key', expected_val @@ -342,7 +339,7 @@ class OCSPPrefetchTest(util.ApacheTest): def test_ocsp_prefetch_open_close_py3_noerror(self): expected_val = b'whatever_value' - open(self.db_fullpath, 'a').close() + open(self.db_path, 'a').close() self.call_mocked_py3( self.config._write_to_dbm, self.db_path, b'key', expected_val @@ -350,6 +347,31 @@ class OCSPPrefetchTest(util.ApacheTest): db2 = self.call_mocked_py3(self.config._read_dbm, self.db_path) self.assertEqual(db2[b'key'], expected_val) + def test_ocsp_prefetch_safe_open_hash_mismatch(self): + import random + def mock_hash(_filepath): + # shound not be the same value twice + return str(random.getrandbits(1024)) + + open(self.db_path, 'a').close() + with mock.patch("certbot_apache._internal.apache_util._file_hash", side_effect=mock_hash): + self.assertRaises( + errors.PluginError, + self.call_mocked_py3, + self.config._write_to_dbm, self.db_path, + b'anything', b'irrelevant' + ) + + def test_ocsp_prefetch_safe_open_hash_fail_open(self): + open(self.db_path, 'a').close() + with mock.patch("certbot_apache._internal.apache_util._file_hash", side_effect=IOError): + self.assertRaises( + errors.PluginError, + self.call_mocked_py3, + self.config._write_to_dbm, self.db_path, + 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)