From fc097de5ffc9294d59fab674d1f0a400a0d2b2d0 Mon Sep 17 00:00:00 2001 From: ohemorange Date: Thu, 1 Jun 2017 09:04:48 -0700 Subject: [PATCH] Refactor nginx file update mechanism in preparation for working with apache plugin (#4720) * move install_ssl_options_conf functionality to common * add no cover * compute current hash instead of saving * make current hash be computed; switch to list of all canonical hashes * put message directly into assertion * don't pass logger * add docstring * Add unit tests for certbot.plugins.common.install_ssl_options_conf --- certbot-apache/certbot_apache/tests/util.py | 4 - certbot-nginx/certbot_nginx/configurator.py | 36 +-------- certbot-nginx/certbot_nginx/constants.py | 8 +- .../certbot_nginx/tests/configurator_test.py | 36 +++++---- certbot/plugins/common.py | 58 ++++++++++++-- certbot/plugins/common_test.py | 75 +++++++++++++++++++ 6 files changed, 151 insertions(+), 66 deletions(-) diff --git a/certbot-apache/certbot_apache/tests/util.py b/certbot-apache/certbot_apache/tests/util.py index d40152ad5..2f51ec016 100644 --- a/certbot-apache/certbot_apache/tests/util.py +++ b/certbot-apache/certbot_apache/tests/util.py @@ -32,10 +32,6 @@ class ApacheTest(unittest.TestCase): # pylint: disable=too-few-public-methods test_dir=test_dir, pkg="certbot_apache.tests") - self.ssl_options = common.setup_ssl_options( - self.config_dir, constants.os_constant("MOD_SSL_CONF_SRC"), - constants.MOD_SSL_CONF_DEST) - self.config_path = os.path.join(self.temp_dir, config_root) self.vhost_path = os.path.join(self.temp_dir, vhost_root) diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 752ccc133..63f659453 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -2,7 +2,6 @@ import logging import os import re -import shutil import socket import subprocess import tempfile @@ -869,36 +868,5 @@ def nginx_restart(nginx_ctl, nginx_conf): def install_ssl_options_conf(options_ssl, options_ssl_digest): """Copy Certbot's SSL options file into the system's config dir if required.""" - def _write_current_hash(): - with open(options_ssl_digest, "w") as f: - f.write(constants.CURRENT_SSL_OPTIONS_HASH) - - def _install_current_file(): - shutil.copyfile(constants.MOD_SSL_CONF_SRC, options_ssl) - _write_current_hash() - - # Check to make sure options-ssl.conf is installed - if not os.path.isfile(options_ssl): - _install_current_file() - return - # there's already a file there. if it exactly matches a previous file hash, - # we can update it. otherwise, print a warning once per new version. - active_file_digest = crypto_util.sha256sum(options_ssl) - if active_file_digest in constants.PREVIOUS_SSL_OPTIONS_HASHES: # safe to update - _install_current_file() - elif active_file_digest == constants.CURRENT_SSL_OPTIONS_HASH: # already up to date - return - else: # has been manually modified, not safe to update - # did they modify the current version or an old version? - if os.path.isfile(options_ssl_digest): - with open(options_ssl_digest, "r") as f: - saved_digest = f.read() - # they modified it after we either installed or told them about this version, so return - if saved_digest == constants.CURRENT_SSL_OPTIONS_HASH: - return - # there's a new version but we couldn't update the file, or they deleted the digest. - # save the current digest so we only print this once, and print a warning - _write_current_hash() - logger.warning("%s has been manually modified; updated ssl configuration options " - "saved to %s. We recommend updating %s for security purposes.", - options_ssl, constants.MOD_SSL_CONF_SRC, options_ssl) + return common.install_ssl_options_conf(options_ssl, options_ssl_digest, + constants.MOD_SSL_CONF_SRC, constants.ALL_SSL_OPTIONS_HASHES) diff --git a/certbot-nginx/certbot_nginx/constants.py b/certbot-nginx/certbot_nginx/constants.py index 765bdd7a8..a74f97662 100644 --- a/certbot-nginx/certbot_nginx/constants.py +++ b/certbot-nginx/certbot_nginx/constants.py @@ -21,16 +21,14 @@ UPDATED_MOD_SSL_CONF_DIGEST = ".updated-options-ssl-nginx-conf-digest.txt" """Name of the hash of the updated or informed mod_ssl_conf as saved in `IConfig.config_dir`.""" -PREVIOUS_SSL_OPTIONS_HASHES = [ +ALL_SSL_OPTIONS_HASHES = [ '0f81093a1465e3d4eaa8b0c14e77b2a2e93568b0fc1351c2b87893a95f0de87c', '9a7b32c49001fed4cff8ad24353329472a50e86ade1ef9b2b9e43566a619612e', 'a6d9f1c7d6b36749b52ba061fff1421f9a0a3d2cfdafbd63c05d06f65b990937', '7f95624dd95cf5afc708b9f967ee83a24b8025dc7c8d9df2b556bbc64256b3ff', + '394732f2bbe3e5e637c3fb5c6e980a1f1b90b01e2e8d6b7cff41dde16e2a756d', ] -"""SHA256 hashes of the contents of previous versions of MOD_SSL_CONF_SRC""" - -CURRENT_SSL_OPTIONS_HASH = '394732f2bbe3e5e637c3fb5c6e980a1f1b90b01e2e8d6b7cff41dde16e2a756d' -"""SHA256 hash of the current contents of MOD_SSL_CONF_SRC""" +"""SHA256 hashes of the contents of all versions of MOD_SSL_CONF_SRC""" def os_constant(key): # XXX TODO: In the future, this could return different constants diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 215fe3165..1f9d3e253 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -552,14 +552,14 @@ class InstallSslOptionsConfTest(util.NginxTest): from certbot_nginx.configurator import install_ssl_options_conf install_ssl_options_conf(self.config.mod_ssl_conf, self.config.updated_mod_ssl_conf_digest) + def _current_ssl_options_hash(self): + from certbot_nginx.constants import MOD_SSL_CONF_SRC + return crypto_util.sha256sum(MOD_SSL_CONF_SRC) + def _assert_current_file(self): - """If this is failing, remember that constants.PREVIOUS_SSL_OPTIONS_HASHES and - constants.CURRENT_SSL_OPTIONS_HASH must be updated when self.config.mod_ssl_conf - is updated. Add CURRENT_SSL_OPTIONS_HASH to PREVIOUS_SSL_OPTIONS_HASHES and set - CURRENT_SSL_OPTIONS_HASH to the hash of the updated self.config.mod_ssl_conf.""" self.assertTrue(os.path.isfile(self.config.mod_ssl_conf)) - from certbot_nginx.constants import CURRENT_SSL_OPTIONS_HASH - self.assertEqual(crypto_util.sha256sum(self.config.mod_ssl_conf), CURRENT_SSL_OPTIONS_HASH) + self.assertEqual(crypto_util.sha256sum(self.config.mod_ssl_conf), + self._current_ssl_options_hash()) def test_no_file(self): # prepare should have placed a file there @@ -575,43 +575,47 @@ class InstallSslOptionsConfTest(util.NginxTest): self._assert_current_file() def test_prev_file_updates_to_current(self): - from certbot_nginx.constants import PREVIOUS_SSL_OPTIONS_HASHES + from certbot_nginx.constants import ALL_SSL_OPTIONS_HASHES with mock.patch('certbot.crypto_util.sha256sum') as mock_sha256: - mock_sha256.return_value = PREVIOUS_SSL_OPTIONS_HASHES[0] + mock_sha256.return_value = ALL_SSL_OPTIONS_HASHES[0] self._call() self._assert_current_file() def test_manually_modified_current_file_does_not_update(self): with open(self.config.mod_ssl_conf, "a") as mod_ssl_conf: mod_ssl_conf.write("a new line for the wrong hash\n") - with mock.patch("certbot_nginx.configurator.logger") as mock_logger: + with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() self.assertFalse(mock_logger.warning.called) self.assertTrue(os.path.isfile(self.config.mod_ssl_conf)) - from certbot_nginx.constants import CURRENT_SSL_OPTIONS_HASH self.assertEqual(crypto_util.sha256sum(constants.MOD_SSL_CONF_SRC), - CURRENT_SSL_OPTIONS_HASH) + self._current_ssl_options_hash()) self.assertNotEqual(crypto_util.sha256sum(self.config.mod_ssl_conf), - CURRENT_SSL_OPTIONS_HASH) + self._current_ssl_options_hash()) def test_manually_modified_past_file_warns(self): with open(self.config.mod_ssl_conf, "a") as mod_ssl_conf: mod_ssl_conf.write("a new line for the wrong hash\n") with open(self.config.updated_mod_ssl_conf_digest, "w") as f: f.write("hashofanoldversion") - with mock.patch("certbot_nginx.configurator.logger") as mock_logger: + with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() self.assertEqual(mock_logger.warning.call_args[0][0], "%s has been manually modified; updated ssl configuration options " "saved to %s. We recommend updating %s for security purposes.") - from certbot_nginx.constants import CURRENT_SSL_OPTIONS_HASH self.assertEqual(crypto_util.sha256sum(constants.MOD_SSL_CONF_SRC), - CURRENT_SSL_OPTIONS_HASH) + self._current_ssl_options_hash()) # only print warning once - with mock.patch("certbot_nginx.configurator.logger") as mock_logger: + with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() self.assertFalse(mock_logger.warning.called) + def test_current_file_hash_in_all_hashes(self): + from certbot_nginx.constants import ALL_SSL_OPTIONS_HASHES + self.assertTrue(self._current_ssl_options_hash() in ALL_SSL_OPTIONS_HASHES, + "Constants.ALL_SSL_OPTIONS_HASHES must be appended" + " with the sha256 hash of self.config.mod_ssl_conf when it is updated.") + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot/plugins/common.py b/certbot/plugins/common.py index 75c5f1cef..aa58e86cc 100644 --- a/certbot/plugins/common.py +++ b/certbot/plugins/common.py @@ -1,4 +1,5 @@ """Plugin common functions.""" +import logging import os import re import shutil @@ -11,9 +12,12 @@ import zope.interface from acme.jose import util as jose_util from certbot import constants +from certbot import crypto_util from certbot import interfaces from certbot import util +logger = logging.getLogger(__name__) + def option_namespace(name): """ArgumentParser options namespace (prefix of all options).""" @@ -262,17 +266,57 @@ class TLSSNI01(object): return response +def install_ssl_options_conf(options_ssl, options_ssl_digest, mod_ssl_conf_src, + all_ssl_options_hashes): + """Copy Certbot's SSL options file into the system's config dir if required. + + :param str options_ssl: destination path for file containing ssl options + :param str options_ssl_digest: path to save a digest of options_ssl in + :param str mod_ssl_conf_src: path to file containing ssl options found in distribution + :param list all_ssl_options_hashes: hashes of every released version of options_ssl + """ + current_ssl_options_hash = crypto_util.sha256sum(mod_ssl_conf_src) + + def _write_current_hash(): + with open(options_ssl_digest, "w") as f: + f.write(current_ssl_options_hash) + + def _install_current_file(): + shutil.copyfile(mod_ssl_conf_src, options_ssl) + _write_current_hash() + + # Check to make sure options-ssl.conf is installed + if not os.path.isfile(options_ssl): + _install_current_file() + return + # there's already a file there. if it's up to date, do nothing. if it's not but + # it matches a known file hash, we can update it. + # otherwise, print a warning once per new version. + active_file_digest = crypto_util.sha256sum(options_ssl) + if active_file_digest == current_ssl_options_hash: # already up to date + return + elif active_file_digest in all_ssl_options_hashes: # safe to update + _install_current_file() + else: # has been manually modified, not safe to update + # did they modify the current version or an old version? + if os.path.isfile(options_ssl_digest): + with open(options_ssl_digest, "r") as f: + saved_digest = f.read() + # they modified it after we either installed or told them about this version, so return + if saved_digest == current_ssl_options_hash: + return + # there's a new version but we couldn't update the file, or they deleted the digest. + # save the current digest so we only print this once, and print a warning + _write_current_hash() + logger.warning("%s has been manually modified; updated ssl configuration options " + "saved to %s. We recommend updating %s for security purposes.", + options_ssl, mod_ssl_conf_src, options_ssl) + + # test utils used by certbot_apache/certbot_nginx (hence # "pragma: no cover") TODO: this might quickly lead to dead code (also # c.f. #383) -def setup_ssl_options(config_dir, src, dest): # pragma: no cover - """Move the ssl_options into position and return the path.""" - option_path = os.path.join(config_dir, dest) - shutil.copyfile(src, option_path) - return option_path - - def dir_setup(test_dir, pkg): # pragma: no cover """Setup the directories necessary for the configurator.""" def expanded_tempdir(prefix): diff --git a/certbot/plugins/common_test.py b/certbot/plugins/common_test.py index 8154b255a..7eb5e5ced 100644 --- a/certbot/plugins/common_test.py +++ b/certbot/plugins/common_test.py @@ -11,6 +11,7 @@ from acme import challenges from acme import jose from certbot import achallenges +from certbot import crypto_util from certbot.tests import acme_util from certbot.tests import util as test_util @@ -221,5 +222,79 @@ class TLSSNI01Test(unittest.TestCase): OpenSSL.crypto.dump_privatekey(OpenSSL.crypto.FILETYPE_PEM, key)) +class InstallSslOptionsConfTest(test_util.TempDirTestCase): + """Tests for certbot.plugins.common.install_ssl_options_conf.""" + + def setUp(self): + super(InstallSslOptionsConfTest, self).setUp() + self.source_path = os.path.join(self.tempdir, "options-ssl-source.conf") + self.dest_path = os.path.join(self.tempdir, "options-ssl-dest.conf") + self.hash_path = os.path.join(self.tempdir, ".options-ssl-conf.txt") + with open(self.source_path, "w") as f: + f.write("test directive\n") + self.hashes = ["someotherhash", + "anotheroldhash", + crypto_util.sha256sum(self.source_path)] + + def _call(self): + from certbot.plugins.common import install_ssl_options_conf + install_ssl_options_conf(self.dest_path, + self.hash_path, + self.source_path, + self.hashes) + + def _current_ssl_options_hash(self): + return crypto_util.sha256sum(self.source_path) + + def _assert_current_file(self): + self.assertTrue(os.path.isfile(self.dest_path)) + self.assertEqual(crypto_util.sha256sum(self.dest_path), + self._current_ssl_options_hash()) + + def test_no_file(self): + self.assertFalse(os.path.isfile(self.dest_path)) + self._call() + self._assert_current_file() + + def test_current_file(self): + self._call() + self._assert_current_file() + + def test_prev_file_updates_to_current(self): + with mock.patch('certbot.crypto_util.sha256sum') as mock_sha256: + mock_sha256.return_value = self.hashes[0] + self._call() + self._assert_current_file() + + def test_manually_modified_current_file_does_not_update(self): + self._call() + with open(self.dest_path, "a") as mod_ssl_conf: + mod_ssl_conf.write("a new line for the wrong hash\n") + with mock.patch("certbot.plugins.common.logger") as mock_logger: + self._call() + self.assertFalse(mock_logger.warning.called) + self.assertTrue(os.path.isfile(self.dest_path)) + self.assertEqual(crypto_util.sha256sum(self.source_path), + self._current_ssl_options_hash()) + self.assertNotEqual(crypto_util.sha256sum(self.dest_path), + self._current_ssl_options_hash()) + + def test_manually_modified_past_file_warns(self): + with open(self.dest_path, "a") as mod_ssl_conf: + mod_ssl_conf.write("a new line for the wrong hash\n") + with open(self.hash_path, "w") as f: + f.write("hashofanoldversion") + with mock.patch("certbot.plugins.common.logger") as mock_logger: + self._call() + self.assertEqual(mock_logger.warning.call_args[0][0], + "%s has been manually modified; updated ssl configuration options " + "saved to %s. We recommend updating %s for security purposes.") + self.assertEqual(crypto_util.sha256sum(self.source_path), + self._current_ssl_options_hash()) + # only print warning once + with mock.patch("certbot.plugins.common.logger") as mock_logger: + self._call() + self.assertFalse(mock_logger.warning.called) + if __name__ == "__main__": unittest.main() # pragma: no cover