Turn off session tickets for versions of Nginx that support it (certbot#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

Author: sydneyli

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
 

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

certbot-nginx/certbot_nginx/configurator.py

@@ -6,6 +6,8 @@
 import tempfile
 import time
 
+import pkg_resources
+
 import OpenSSL
 import zope.interface
 
@@ -120,6 +122,14 @@ def __init__(self, *args, **kwargs):
 
         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 @@ 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 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 @@ def prepare(self):
 
         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,

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 @@
 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 @@
     '7f95624dd95cf5afc708b9f967ee83a24b8025dc7c8d9df2b556bbc64256b3ff',
     '394732f2bbe3e5e637c3fb5c6e980a1f1b90b01e2e8d6b7cff41dde16e2a756d',
     '4b16fec2bcbcd8a2f3296d886f17f9953ffdcc0af54582452ca1e52f5f776f16',
-]
+] + SSL_OPTIONS_HASHES_NEW
 """SHA256 hashes of the contents of all versions of MOD_SSL_CONF_SRC"""
 
 def os_constant(key):

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";

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;

certbot-nginx/certbot_nginx/tests/configurator_test.py

@@ -13,7 +13,6 @@
 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 @@ def setUp(self):
             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,12 +906,32 @@ def test_current_file(self):
         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:
@@ -922,7 +940,7 @@ def test_manually_modified_current_file_does_not_update(self):
             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 @@ def test_manually_modified_past_file_warns(self):
             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 @@ def test_current_file_hash_in_all_hashes(self):
             "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."""