diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index dd61575d4..752ccc133 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -139,6 +139,11 @@ class NginxConfigurator(common.Plugin): """Full absolute path to SSL configuration file.""" return os.path.join(self.config.config_dir, constants.MOD_SSL_CONF_DEST) + @property + def updated_mod_ssl_conf_digest(self): + """Full absolute path to digest of updated SSL configuration file.""" + return os.path.join(self.config.config_dir, constants.UPDATED_MOD_SSL_CONF_DIGEST) + # This is called in determine_authenticator and determine_installer def prepare(self): """Prepare the authenticator/installer. @@ -156,7 +161,7 @@ class NginxConfigurator(common.Plugin): self.parser = parser.NginxParser(self.conf('server-root')) - install_ssl_options_conf(self.mod_ssl_conf) + install_ssl_options_conf(self.mod_ssl_conf, self.updated_mod_ssl_conf_digest) # Set Version if self.version is None: @@ -862,8 +867,38 @@ def nginx_restart(nginx_ctl, nginx_conf): time.sleep(1) -def install_ssl_options_conf(options_ssl): +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): - shutil.copyfile(constants.MOD_SSL_CONF_SRC, 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) diff --git a/certbot-nginx/certbot_nginx/constants.py b/certbot-nginx/certbot_nginx/constants.py index 8cf1f6bc9..765bdd7a8 100644 --- a/certbot-nginx/certbot_nginx/constants.py +++ b/certbot-nginx/certbot_nginx/constants.py @@ -17,6 +17,21 @@ MOD_SSL_CONF_SRC = pkg_resources.resource_filename( """Path to the nginx mod_ssl config file found in the Certbot distribution.""" +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 = [ + '0f81093a1465e3d4eaa8b0c14e77b2a2e93568b0fc1351c2b87893a95f0de87c', + '9a7b32c49001fed4cff8ad24353329472a50e86ade1ef9b2b9e43566a619612e', + 'a6d9f1c7d6b36749b52ba061fff1421f9a0a3d2cfdafbd63c05d06f65b990937', + '7f95624dd95cf5afc708b9f967ee83a24b8025dc7c8d9df2b556bbc64256b3ff', +] +"""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""" + def os_constant(key): # XXX TODO: In the future, this could return different constants # based on what OS we are running under. To see an diff --git a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf index e1839909d..7303f9bc6 100644 --- a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf +++ b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf @@ -1,3 +1,8 @@ +# This file contains important security parameters. If you modify this file manually, +# Certbot will be unable to automatically provide future security updates. +# Instead, you will need to manually update this file by referencing the contents of +# options-ssl-nginx.conf.new. + ssl_session_cache shared:le_nginx_SSL:1m; ssl_session_timeout 1440m; diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 906d276b1..215fe3165 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -11,9 +11,11 @@ from acme import challenges from acme import messages from certbot import achallenges +from certbot import crypto_util from certbot import errors from certbot.tests import util as certbot_test_util +from certbot_nginx import constants from certbot_nginx import obj from certbot_nginx import parser from certbot_nginx.tests import util @@ -537,6 +539,79 @@ class NginxConfiguratorTest(util.NginxTest): self.assertTrue(util.contains_at_depth( generated_conf, ['ssl_stapling_verify', 'on'], 2)) +class InstallSslOptionsConfTest(util.NginxTest): + """Test that the options-ssl-nginx.conf file is installed and updated properly.""" + + def setUp(self): + super(InstallSslOptionsConfTest, self).setUp() + + self.config = util.get_nginx_configurator( + self.config_path, self.config_dir, self.work_dir, self.logs_dir) + + def _call(self): + 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 _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) + + def test_no_file(self): + # prepare should have placed a file there + self._assert_current_file() + os.remove(self.config.mod_ssl_conf) + self.assertFalse(os.path.isfile(self.config.mod_ssl_conf)) + self._call() + self._assert_current_file() + + def test_current_file(self): + self._assert_current_file() + self._call() + self._assert_current_file() + + def test_prev_file_updates_to_current(self): + from certbot_nginx.constants import PREVIOUS_SSL_OPTIONS_HASHES + with mock.patch('certbot.crypto_util.sha256sum') as mock_sha256: + mock_sha256.return_value = PREVIOUS_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: + 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.assertNotEqual(crypto_util.sha256sum(self.config.mod_ssl_conf), + 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: + 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) + # only print warning once + with mock.patch("certbot_nginx.configurator.logger") as mock_logger: + self._call() + self.assertFalse(mock_logger.warning.called) + if __name__ == "__main__": unittest.main() # pragma: no cover diff --git a/certbot-nginx/certbot_nginx/tests/util.py b/certbot-nginx/certbot_nginx/tests/util.py index 4ab95374e..2ee38ec38 100644 --- a/certbot-nginx/certbot_nginx/tests/util.py +++ b/certbot-nginx/certbot_nginx/tests/util.py @@ -16,7 +16,6 @@ from certbot.tests import util as test_util from certbot.plugins import common -from certbot_nginx import constants from certbot_nginx import configurator from certbot_nginx import nginxparser @@ -30,10 +29,6 @@ class NginxTest(unittest.TestCase): # pylint: disable=too-few-public-methods "etc_nginx", "certbot_nginx.tests") self.logs_dir = tempfile.mkdtemp('logs') - self.ssl_options = common.setup_ssl_options( - self.config_dir, constants.MOD_SSL_CONF_SRC, - constants.MOD_SSL_CONF_DEST) - self.config_path = os.path.join(self.temp_dir, "etc_nginx") self.rsa512jwk = jose.JWKRSA.load(test_util.load_vector( diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 7de173568..2b2e7d0d8 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -4,6 +4,7 @@ is capable of handling the signatures. """ +import hashlib import logging import os @@ -353,3 +354,17 @@ def _notAfterBefore(cert_path, method): if six.PY3: timestamp_str = timestamp_str.decode('ascii') return pyrfc3339.parse(timestamp_str) + + +def sha256sum(filename): + """Compute a sha256sum of a file. + + :param str filename: path to the file whose hash will be computed + + :returns: sha256 digest of the file in hexadecimal + :rtype: str + """ + sha256 = hashlib.sha256() + with open(filename, 'rb') as f: + sha256.update(f.read()) + return sha256.hexdigest() diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index 8adf753d6..c678dc501 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -293,5 +293,14 @@ class NotAfterTest(unittest.TestCase): '2014-12-18T22:34:45+00:00') +class Sha256sumTest(unittest.TestCase): + """Tests for certbot.crypto_util.notAfter""" + + def test_sha256sum(self): + from certbot.crypto_util import sha256sum + self.assertEqual(sha256sum(CERT_PATH), + '914ffed8daf9e2c99d90ac95c77d54f32cbd556672facac380f0c063498df84e') + + if __name__ == '__main__': unittest.main() # pragma: no cover