Update options-ssl-nginx.conf inprepare if it hasn't been manually modified (#4689)

Fixes #4559.

* Update options-ssl-nginx.conf in prepare, if it hasn't been modified.

* add previous options-ssl-nginx.conf hashes

* InstallSslOptionsConfTest

* remove .new file and only print warning once

* save digest to /etc/letsencrypt

* add comment reminding devs to update hashes

* add comment and test for sha256sum

* treat hash file as text file because python3

* move constants and rename hidden digest file

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)

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

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;
 

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

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(

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()

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