From 44f4743b515e62a0f63436ddd990b90b059ab990 Mon Sep 17 00:00:00 2001 From: Erica Portnoy Date: Tue, 23 May 2017 16:25:39 -0700 Subject: [PATCH 1/2] Mechanism for automatically updating options-ssl-apache.conf file * add file update mechanism + tests to apache * update with actual hashes, and update apache test to match since there aren't previous versions --- certbot-apache/certbot_apache/configurator.py | 26 +++--- certbot-apache/certbot_apache/constants.py | 10 +++ .../certbot_apache/tests/configurator_test.py | 83 ++++++++++++++++++- 3 files changed, 103 insertions(+), 16 deletions(-) diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 13b325d7f..24a2f3934 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -5,7 +5,6 @@ import fnmatch import logging import os import re -import shutil import socket import time @@ -145,6 +144,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): 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) + def prepare(self): """Prepare the authenticator/installer. @@ -195,7 +199,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Get all of the available vhosts self.vhosts = self.get_virtual_hosts() - install_ssl_options_conf(self.mod_ssl_conf) + install_ssl_options_conf(self.mod_ssl_conf, self.updated_mod_ssl_conf_digest) # Prevent two Apache plugins from modifying a config at once try: @@ -1981,19 +1985,11 @@ def _split_aug_path(vhost_path): return file_path, "/".join(reversed(internal_path)) -def install_ssl_options_conf(options_ssl): - """ - Copy Certbot's SSL options file into the system's config dir if - required. - """ +def install_ssl_options_conf(options_ssl, options_ssl_digest): + """Copy Certbot's SSL options file into the system's config dir if required.""" + # XXX if we ever try to enforce a local privilege boundary (eg, running # certbot for unprivileged users via setuid), this function will need # to be modified. - - # XXX if the user is in security-autoupdate mode, we should be willing to - # overwrite the options_ssl file at least if it's unmodified: - # https://github.com/letsencrypt/letsencrypt/issues/1123 - - # Check to make sure options-ssl.conf is installed - if not os.path.isfile(options_ssl): - shutil.copyfile(constants.os_constant("MOD_SSL_CONF_SRC"), options_ssl) + return common.install_ssl_options_conf(options_ssl, options_ssl_digest, + constants.os_constant("MOD_SSL_CONF_SRC"), constants.ALL_SSL_OPTIONS_HASHES, logger) diff --git a/certbot-apache/certbot_apache/constants.py b/certbot-apache/certbot_apache/constants.py index 3cfeb4dd6..0b96d392e 100644 --- a/certbot-apache/certbot_apache/constants.py +++ b/certbot-apache/certbot_apache/constants.py @@ -131,6 +131,16 @@ CLI_DEFAULTS = { MOD_SSL_CONF_DEST = "options-ssl-apache.conf" """Name of the mod_ssl config file as saved in `IConfig.config_dir`.""" + +UPDATED_MOD_SSL_CONF_DIGEST = ".updated-options-ssl-apache-conf-digest.txt" +"""Name of the hash of the updated or informed mod_ssl_conf as saved in `IConfig.config_dir`.""" + +ALL_SSL_OPTIONS_HASHES = [ + '2086bca02db48daf93468332543c60ac6acdb6f0b58c7bfdf578a5d47092f82a', + '4844d36c9a0f587172d9fa10f4f1c9518e3bcfa1947379f155e16a70a728c21a', +] +"""SHA256 hashes of the contents of previous versions of all versions of MOD_SSL_CONF_SRC""" + AUGEAS_LENS_DIR = pkg_resources.resource_filename( "certbot_apache", "augeas_lens") """Path to the Augeas lens directory""" diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 75589dce5..055feeece 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -12,6 +12,7 @@ import six # pylint: disable=unused-import from acme import challenges from certbot import achallenges +from certbot import crypto_util from certbot import errors from certbot.tests import acme_util @@ -826,8 +827,10 @@ class MultipleVhostsTest(util.ApacheTest): def test_install_ssl_options_conf(self): from certbot_apache.configurator import install_ssl_options_conf path = os.path.join(self.work_dir, "test_it") - install_ssl_options_conf(path) + other_path = os.path.join(self.work_dir, "other_test_it") + install_ssl_options_conf(path, other_path) self.assertTrue(os.path.isfile(path)) + self.assertTrue(os.path.isfile(other_path)) # TEST ENHANCEMENTS def test_supported_enhancements(self): @@ -1432,5 +1435,83 @@ class MultiVhostsTest(util.ApacheTest): mock.ANY) +class InstallSslOptionsConfTest(util.ApacheTest): + """Test that the options-ssl-nginx.conf file is installed and updated properly.""" + + def setUp(self): # pylint: disable=arguments-differ + super(InstallSslOptionsConfTest, self).setUp() + + self.config = util.get_apache_configurator( + self.config_path, self.vhost_path, self.config_dir, self.work_dir) + + def _call(self): + from certbot_apache.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): + return crypto_util.sha256sum(constants.os_constant("MOD_SSL_CONF_SRC")) + + def _assert_current_file(self): + self.assertTrue(os.path.isfile(self.config.mod_ssl_conf)) + 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 + 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_apache.constants import ALL_SSL_OPTIONS_HASHES + ALL_SSL_OPTIONS_HASHES.insert(0, "test_hash_does_not_match") + with mock.patch('certbot.crypto_util.sha256sum') as mock_sha256: + 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_apache.configurator.logger") as mock_logger: + self._call() + self.assertFalse(mock_logger.warning.called) + self.assertTrue(os.path.isfile(self.config.mod_ssl_conf)) + self.assertEqual(crypto_util.sha256sum(constants.os_constant("MOD_SSL_CONF_SRC")), + self._current_ssl_options_hash()) + self.assertNotEqual(crypto_util.sha256sum(self.config.mod_ssl_conf), + 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_apache.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.") + self.assertEqual(crypto_util.sha256sum(constants.os_constant("MOD_SSL_CONF_SRC")), + self._current_ssl_options_hash()) + # only print warning once + with mock.patch("certbot_apache.configurator.logger") as mock_logger: + self._call() + self.assertFalse(mock_logger.warning.called) + + def test_current_file_hash_in_all_hashes(self): + from certbot_apache.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 From 844c2d34389ebe104979a10b3715e127627c8aeb Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 1 Jun 2017 09:12:50 -0700 Subject: [PATCH 2/2] Finish work on #4718. * Update in response to changes in #4720. * Update ALL_SSL_OPTIONS_HASHES. * Add warning to Apache's SSL options files. --- .../certbot_apache/centos-options-ssl-apache.conf | 6 +++++- certbot-apache/certbot_apache/configurator.py | 2 +- certbot-apache/certbot_apache/constants.py | 4 ++++ certbot-apache/certbot_apache/options-ssl-apache.conf | 6 +++++- certbot-apache/certbot_apache/tests/configurator_test.py | 6 +++--- 5 files changed, 18 insertions(+), 6 deletions(-) diff --git a/certbot-apache/certbot_apache/centos-options-ssl-apache.conf b/certbot-apache/certbot_apache/centos-options-ssl-apache.conf index fbe8da0f2..17ae1be76 100644 --- a/certbot-apache/certbot_apache/centos-options-ssl-apache.conf +++ b/certbot-apache/certbot_apache/centos-options-ssl-apache.conf @@ -1,4 +1,8 @@ -# Baseline setting to Include for SSL sites +# This file contains important security parameters. If you modify this file +# manually, Certbot will be unable to automatically provide future security +# updates. Instead, Certbot will print and log an error message with a path to +# the up-to-date file that you will need to refer to when manually updating +# this file. SSLEngine on diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 24a2f3934..097c4ff42 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -1992,4 +1992,4 @@ def install_ssl_options_conf(options_ssl, options_ssl_digest): # certbot for unprivileged users via setuid), this function will need # to be modified. return common.install_ssl_options_conf(options_ssl, options_ssl_digest, - constants.os_constant("MOD_SSL_CONF_SRC"), constants.ALL_SSL_OPTIONS_HASHES, logger) + constants.os_constant("MOD_SSL_CONF_SRC"), constants.ALL_SSL_OPTIONS_HASHES) diff --git a/certbot-apache/certbot_apache/constants.py b/certbot-apache/certbot_apache/constants.py index 0b96d392e..7aced5dca 100644 --- a/certbot-apache/certbot_apache/constants.py +++ b/certbot-apache/certbot_apache/constants.py @@ -138,6 +138,10 @@ UPDATED_MOD_SSL_CONF_DIGEST = ".updated-options-ssl-apache-conf-digest.txt" ALL_SSL_OPTIONS_HASHES = [ '2086bca02db48daf93468332543c60ac6acdb6f0b58c7bfdf578a5d47092f82a', '4844d36c9a0f587172d9fa10f4f1c9518e3bcfa1947379f155e16a70a728c21a', + '5a922826719981c0a234b1fbcd495f3213e49d2519e845ea0748ba513044b65b', + '4066b90268c03c9ba0201068eaa39abbc02acf9558bb45a788b630eb85dadf27', + 'f175e2e7c673bd88d0aff8220735f385f916142c44aa83b09f1df88dd4767a88', + 'cfdd7c18d2025836ea3307399f509cfb1ebf2612c87dd600a65da2a8e2f2797b', ] """SHA256 hashes of the contents of previous versions of all versions of MOD_SSL_CONF_SRC""" diff --git a/certbot-apache/certbot_apache/options-ssl-apache.conf b/certbot-apache/certbot_apache/options-ssl-apache.conf index ec07a4ba3..950a02a8b 100644 --- a/certbot-apache/certbot_apache/options-ssl-apache.conf +++ b/certbot-apache/certbot_apache/options-ssl-apache.conf @@ -1,4 +1,8 @@ -# Baseline setting to Include for SSL sites +# This file contains important security parameters. If you modify this file +# manually, Certbot will be unable to automatically provide future security +# updates. Instead, Certbot will print and log an error message with a path to +# the up-to-date file that you will need to refer to when manually updating +# this file. SSLEngine on diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 055feeece..db04bfcd1 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -1480,7 +1480,7 @@ class InstallSslOptionsConfTest(util.ApacheTest): 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_apache.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)) @@ -1494,7 +1494,7 @@ class InstallSslOptionsConfTest(util.ApacheTest): 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_apache.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 " @@ -1502,7 +1502,7 @@ class InstallSslOptionsConfTest(util.ApacheTest): self.assertEqual(crypto_util.sha256sum(constants.os_constant("MOD_SSL_CONF_SRC")), self._current_ssl_options_hash()) # only print warning once - with mock.patch("certbot_apache.configurator.logger") as mock_logger: + with mock.patch("certbot.plugins.common.logger") as mock_logger: self._call() self.assertFalse(mock_logger.warning.called)