From 6c53f5d8ed2875a8ef17906d44db476bdbc580db Mon Sep 17 00:00:00 2001 From: sydneyli Date: Fri, 14 Jun 2019 13:44:50 -0700 Subject: [PATCH] Turn off session tickets for versions of Nginx that support it (#7092) * Turn off session tickets for versions of Nginx that support it In line with Mozilla's security recommendations. * Changelog. * Set version before installing config files * lint: remove unused import * windows testfix * another windows testfix? * Testing path of updating src file with old nginx * Fix windows, and make config update tests fail if update doesn't happen --- CHANGELOG.md | 2 +- certbot-nginx/MANIFEST.in | 1 + certbot-nginx/certbot_nginx/configurator.py | 29 ++++++++---- certbot-nginx/certbot_nginx/constants.py | 13 ++---- .../certbot_nginx/options-ssl-nginx-old.conf | 13 ++++++ .../certbot_nginx/options-ssl-nginx.conf | 1 + .../certbot_nginx/tests/configurator_test.py | 46 +++++++++++++++---- 7 files changed, 77 insertions(+), 28 deletions(-) create mode 100644 certbot-nginx/certbot_nginx/options-ssl-nginx-old.conf diff --git a/CHANGELOG.md b/CHANGELOG.md index 9919910f7..6e76970b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Added -* +* Turn off session tickets for nginx plugin by default ### Changed diff --git a/certbot-nginx/MANIFEST.in b/certbot-nginx/MANIFEST.in index 2daca6738..8707f9443 100644 --- a/certbot-nginx/MANIFEST.in +++ b/certbot-nginx/MANIFEST.in @@ -3,3 +3,4 @@ include README.rst recursive-include docs * recursive-include certbot_nginx/tests/testdata * include certbot_nginx/options-ssl-nginx.conf +include certbot_nginx/options-ssl-nginx-old.conf diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index 6da00e513..e078ad4cb 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -6,6 +6,8 @@ import subprocess import tempfile import time +import pkg_resources + import OpenSSL import zope.interface @@ -120,6 +122,14 @@ class NginxConfigurator(common.Installer): self.reverter.recovery_routine() + @property + def mod_ssl_conf_src(self): + """Full absolute path to SSL configuration file source.""" + config_filename = "options-ssl-nginx.conf" + if self.version < (1, 5, 9): + config_filename = "options-ssl-nginx-old.conf" + return pkg_resources.resource_filename("certbot_nginx", config_filename) + @property def mod_ssl_conf(self): """Full absolute path to SSL configuration file.""" @@ -130,6 +140,11 @@ class NginxConfigurator(common.Installer): """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 install_ssl_options_conf(self, options_ssl, options_ssl_digest): + """Copy Certbot's SSL options file into the system's config dir if required.""" + return common.install_version_controlled_file(options_ssl, options_ssl_digest, + self.mod_ssl_conf_src, constants.ALL_SSL_OPTIONS_HASHES) + # This is called in determine_authenticator and determine_installer def prepare(self): """Prepare the authenticator/installer. @@ -148,14 +163,14 @@ class NginxConfigurator(common.Installer): self.parser = parser.NginxParser(self.conf('server-root')) - install_ssl_options_conf(self.mod_ssl_conf, self.updated_mod_ssl_conf_digest) - - self.install_ssl_dhparams() - # Set Version if self.version is None: self.version = self.get_version() + self.install_ssl_options_conf(self.mod_ssl_conf, self.updated_mod_ssl_conf_digest) + + self.install_ssl_dhparams() + # Prevent two Nginx plugins from modifying a config at once try: util.lock_dir_until_exit(self.conf('server-root')) @@ -1131,12 +1146,6 @@ def nginx_restart(nginx_ctl, nginx_conf): time.sleep(1) -def install_ssl_options_conf(options_ssl, options_ssl_digest): - """Copy Certbot's SSL options file into the system's config dir if required.""" - return common.install_version_controlled_file(options_ssl, options_ssl_digest, - constants.MOD_SSL_CONF_SRC, constants.ALL_SSL_OPTIONS_HASHES) - - def _determine_default_server_root(): if os.environ.get("CERTBOT_DOCS") == "1": default_server_root = "%s or %s" % (constants.LINUX_SERVER_ROOT, diff --git a/certbot-nginx/certbot_nginx/constants.py b/certbot-nginx/certbot_nginx/constants.py index 41716db0f..cec7acaf5 100644 --- a/certbot-nginx/certbot_nginx/constants.py +++ b/certbot-nginx/certbot_nginx/constants.py @@ -1,8 +1,6 @@ """nginx plugin constants.""" import platform -import pkg_resources - FREEBSD_DARWIN_SERVER_ROOT = "/usr/local/etc/nginx" LINUX_SERVER_ROOT = "/etc/nginx" @@ -21,14 +19,13 @@ CLI_DEFAULTS = dict( MOD_SSL_CONF_DEST = "options-ssl-nginx.conf" """Name of the mod_ssl config file as saved in `IConfig.config_dir`.""" -MOD_SSL_CONF_SRC = pkg_resources.resource_filename( - "certbot_nginx", "options-ssl-nginx.conf") -"""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`.""" +SSL_OPTIONS_HASHES_NEW = [ + '63e2bddebb174a05c9d8a7cf2adf72f7af04349ba59a1a925fe447f73b2f1abf', +] +"""SHA256 hashes of the contents of versions of MOD_SSL_CONF_SRC for nginx >= 1.5.9""" ALL_SSL_OPTIONS_HASHES = [ '0f81093a1465e3d4eaa8b0c14e77b2a2e93568b0fc1351c2b87893a95f0de87c', @@ -37,7 +34,7 @@ ALL_SSL_OPTIONS_HASHES = [ '7f95624dd95cf5afc708b9f967ee83a24b8025dc7c8d9df2b556bbc64256b3ff', '394732f2bbe3e5e637c3fb5c6e980a1f1b90b01e2e8d6b7cff41dde16e2a756d', '4b16fec2bcbcd8a2f3296d886f17f9953ffdcc0af54582452ca1e52f5f776f16', -] +] + SSL_OPTIONS_HASHES_NEW """SHA256 hashes of the contents of all versions of MOD_SSL_CONF_SRC""" def os_constant(key): diff --git a/certbot-nginx/certbot_nginx/options-ssl-nginx-old.conf b/certbot-nginx/certbot_nginx/options-ssl-nginx-old.conf new file mode 100644 index 000000000..292d42984 --- /dev/null +++ b/certbot-nginx/certbot_nginx/options-ssl-nginx-old.conf @@ -0,0 +1,13 @@ +# 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. + +ssl_session_cache shared:le_nginx_SSL:1m; +ssl_session_timeout 1440m; + +ssl_protocols TLSv1 TLSv1.1 TLSv1.2; +ssl_prefer_server_ciphers on; + +ssl_ciphers "ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS"; diff --git a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf index 292d42984..57a332d2f 100644 --- a/certbot-nginx/certbot_nginx/options-ssl-nginx.conf +++ b/certbot-nginx/certbot_nginx/options-ssl-nginx.conf @@ -6,6 +6,7 @@ ssl_session_cache shared:le_nginx_SSL:1m; ssl_session_timeout 1440m; +ssl_session_tickets off; ssl_protocols TLSv1 TLSv1.1 TLSv1.2; ssl_prefer_server_ciphers on; diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index 6c3f4f0cf..5e9d61a44 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -13,7 +13,6 @@ from certbot import errors from certbot.compat import os 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.configurator import _redirect_block_for_domain @@ -883,12 +882,11 @@ class InstallSslOptionsConfTest(util.NginxTest): 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) + self.config.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) + return crypto_util.sha256sum(self.config.mod_ssl_conf_src) def _assert_current_file(self): self.assertTrue(os.path.isfile(self.config.mod_ssl_conf)) @@ -908,13 +906,33 @@ class InstallSslOptionsConfTest(util.NginxTest): self._call() self._assert_current_file() + def _mock_hash_except_ssl_conf_src(self, fake_hash): + # Write a bad file in place so that update tests fail if no update occurs. + # We're going to pretend this file (the currently installed conf file) + # actually hashes to `fake_hash` for the update tests. + with open(self.config.mod_ssl_conf, "w") as f: + f.write("bogus") + sha256 = crypto_util.sha256sum + def _hash(filename): + return sha256(filename) if filename == self.config.mod_ssl_conf_src else fake_hash + return _hash + def test_prev_file_updates_to_current(self): from certbot_nginx.constants import ALL_SSL_OPTIONS_HASHES - with mock.patch('certbot.crypto_util.sha256sum') as mock_sha256: - mock_sha256.return_value = ALL_SSL_OPTIONS_HASHES[0] + with mock.patch('certbot.crypto_util.sha256sum', + new=self._mock_hash_except_ssl_conf_src(ALL_SSL_OPTIONS_HASHES[0])): self._call() self._assert_current_file() + def test_prev_file_updates_to_current_old_nginx(self): + from certbot_nginx.constants import ALL_SSL_OPTIONS_HASHES, SSL_OPTIONS_HASHES_NEW + self.config.version = (1, 5, 8) + with mock.patch('certbot.crypto_util.sha256sum', + new=self._mock_hash_except_ssl_conf_src(ALL_SSL_OPTIONS_HASHES[0])): + self._call() + self._assert_current_file() + self.assertTrue(self._current_ssl_options_hash() not in SSL_OPTIONS_HASHES_NEW) + 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") @@ -922,7 +940,7 @@ class InstallSslOptionsConfTest(util.NginxTest): self._call() self.assertFalse(mock_logger.warning.called) self.assertTrue(os.path.isfile(self.config.mod_ssl_conf)) - self.assertEqual(crypto_util.sha256sum(constants.MOD_SSL_CONF_SRC), + self.assertEqual(crypto_util.sha256sum(self.config.mod_ssl_conf_src), self._current_ssl_options_hash()) self.assertNotEqual(crypto_util.sha256sum(self.config.mod_ssl_conf), self._current_ssl_options_hash()) @@ -937,7 +955,7 @@ class InstallSslOptionsConfTest(util.NginxTest): self.assertEqual(mock_logger.warning.call_args[0][0], "%s has been manually modified; updated file " "saved to %s. We recommend updating %s for security purposes.") - self.assertEqual(crypto_util.sha256sum(constants.MOD_SSL_CONF_SRC), + self.assertEqual(crypto_util.sha256sum(self.config.mod_ssl_conf_src), self._current_ssl_options_hash()) # only print warning once with mock.patch("certbot.plugins.common.logger") as mock_logger: @@ -950,6 +968,16 @@ class InstallSslOptionsConfTest(util.NginxTest): "Constants.ALL_SSL_OPTIONS_HASHES must be appended" " with the sha256 hash of self.config.mod_ssl_conf when it is updated.") + def test_old_nginx_version_uses_old_config(self): + self.config.version = (1, 5, 8) + self.assertEqual(os.path.basename(self.config.mod_ssl_conf_src), + "options-ssl-nginx-old.conf") + self._call() + self._assert_current_file() + self.config.version = (1, 5, 9) + self.assertEqual(os.path.basename(self.config.mod_ssl_conf_src), + "options-ssl-nginx.conf") + class DetermineDefaultServerRootTest(certbot_test_util.ConfigTestCase): """Tests for certbot_nginx.configurator._determine_default_server_root."""