Merge pull request #3849 from thomaszbz/dev/3844-preserve-must-staple

Preserve --must-staple in configuration for renewal (#3844)
together with updated version of PR #3948 Save allow_subset_of_names in renewal conf files

certbot/renewal.py

@@ -1,6 +1,7 @@
 """Functionality for autorenewal and associated juggling of configurations"""
 from __future__ import print_function
 import copy
+import itertools
 import logging
 import os
 import traceback
@@ -30,6 +31,10 @@ STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent",
                     "server", "account", "authenticator", "installer",
                     "standalone_supported_challenges", "renew_hook"]
 INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"]
+BOOL_CONFIG_ITEMS = ["must_staple", "allow_subset_of_names"]
+
+CONFIG_ITEMS = set(itertools.chain(
+    BOOL_CONFIG_ITEMS, INT_CONFIG_ITEMS, STR_CONFIG_ITEMS))
 
 
 def _reconstitute(config, full_path):
@@ -68,7 +73,7 @@ def _reconstitute(config, full_path):
     # Now restore specific values along with their data types, if
     # those elements are present.
     try:
-        _restore_required_config_elements(config, renewalparams)
+        restore_required_config_elements(config, renewalparams)
         _restore_plugin_configs(config, renewalparams)
     except (ValueError, errors.Error) as error:
         logger.warning(
@@ -148,7 +153,7 @@ def _restore_plugin_configs(config, renewalparams):
                     setattr(config.namespace, config_item, cast(config_value))
 
 
-def _restore_required_config_elements(config, renewalparams):
+def restore_required_config_elements(config, renewalparams):
     """Sets non-plugin specific values in config from renewalparams
 
     :param configuration.NamespaceConfig config: configuration for the
@@ -157,30 +162,69 @@ def _restore_required_config_elements(config, renewalparams):
         configuration file that defines this lineage
 
     """
-    # string-valued items to add if they're present
-    for config_item in STR_CONFIG_ITEMS:
-        if config_item in renewalparams and not cli.set_by_cli(config_item):
-            value = renewalparams[config_item]
-            # Unfortunately, we've lost type information from ConfigObj,
-            # so we don't know if the original was NoneType or str!
-            if value == "None":
-                value = None
-            setattr(config.namespace, config_item, value)
-    # int-valued items to add if they're present
-    for config_item in INT_CONFIG_ITEMS:
-        if config_item in renewalparams and not cli.set_by_cli(config_item):
-            config_value = renewalparams[config_item]
-            # the default value for http01_port was None during private beta
-            if config_item == "http01_port" and config_value == "None":
-                logger.info("updating legacy http01_port value")
-                int_value = cli.flag_default("http01_port")
-            else:
-                try:
-                    int_value = int(config_value)
-                except ValueError:
-                    raise errors.Error(
-                        "Expected a numeric value for {0}".format(config_item))
-            setattr(config.namespace, config_item, int_value)
+
+    required_items = itertools.chain(
+        six.moves.zip(BOOL_CONFIG_ITEMS, itertools.repeat(_restore_bool)),
+        six.moves.zip(INT_CONFIG_ITEMS, itertools.repeat(_restore_int)),
+        six.moves.zip(STR_CONFIG_ITEMS, itertools.repeat(_restore_str)))
+    for item_name, restore_func in required_items:
+        if item_name in renewalparams and not cli.set_by_cli(item_name):
+            value = restore_func(item_name, renewalparams[item_name])
+            setattr(config.namespace, item_name, value)
+
+
+def _restore_bool(name, value):
+    """Restores an boolean key-value pair from a renewal config file.
+
+    :param str name: option name
+    :param str value: option value
+
+    :returns: converted option value to be stored in the runtime config
+    :rtype: bool
+
+    :raises errors.Error: if value can't be converted to an bool
+
+    """
+    lowercase_value = value.lower()
+    if lowercase_value not in ("true", "false"):
+        raise errors.Error(
+            "Expected True or False for {0} but found {1}".format(name, value))
+    return lowercase_value == "true"
+
+
+def _restore_int(name, value):
+    """Restores an integer key-value pair from a renewal config file.
+
+    :param str name: option name
+    :param str value: option value
+
+    :returns: converted option value to be stored in the runtime config
+    :rtype: int
+
+    :raises errors.Error: if value can't be converted to an int
+
+    """
+    if name == "http01_port" and value == "None":
+        logger.info("updating legacy http01_port value")
+        return cli.flag_default("http01_port")
+
+    try:
+        return int(value)
+    except ValueError:
+        raise errors.Error("Expected a numeric value for {0}".format(name))
+
+
+def _restore_str(unused_name, value):
+    """Restores an string key-value pair from a renewal config file.
+
+    :param str unused_name: option name
+    :param str value: option value
+
+    :returns: converted option value to be stored in the runtime config
+    :rtype: str or None
+
+    """
+    return None if value == "None" else value
 
 
 def should_renew(config, lineage):

certbot/storage.py

@@ -183,9 +183,9 @@ def _relevant(option):
     from certbot import renewal
     from certbot.plugins import disco as plugins_disco
     plugins = list(plugins_disco.PluginsRegistry.find_all())
-    return (option in renewal.STR_CONFIG_ITEMS
-            or option in renewal.INT_CONFIG_ITEMS
-            or any(option.startswith(x + "_") for x in plugins))
+
+    return (option in renewal.CONFIG_ITEMS or
+            any(option.startswith(x + "_") for x in plugins))
 
 
 def relevant_values(all_values):

certbot/tests/renewal_test.py

@@ -5,6 +5,7 @@ import unittest
 import tempfile
 
 from certbot import configuration
+from certbot import errors
 from certbot import storage
 
 from certbot.tests import util
@@ -15,15 +16,55 @@ class RenewalTest(unittest.TestCase):
         self.tmp_dir = tempfile.mkdtemp()
         self.config_dir = os.path.join(self.tmp_dir, 'config')
 
-    @mock.patch("certbot.cli.set_by_cli")
+    @mock.patch('certbot.cli.set_by_cli')
     def test_ancient_webroot_renewal_conf(self, mock_set_by_cli):
         mock_set_by_cli.return_value = False
         rc_path = util.make_lineage(self, 'sample-renewal-ancient.conf')
         args = mock.MagicMock(account=None, email=None, webroot_path=None)
         config = configuration.NamespaceConfig(args)
         lineage = storage.RenewableCert(rc_path, config)
-        renewalparams = lineage.configuration["renewalparams"]
+        renewalparams = lineage.configuration['renewalparams']
         # pylint: disable=protected-access
         from certbot import renewal
         renewal._restore_webroot_config(config, renewalparams)
-        self.assertEqual(config.webroot_path, ["/var/www/"])
+        self.assertEqual(config.webroot_path, ['/var/www/'])
+
+
+class RestoreRequiredConfigElementsTest(unittest.TestCase):
+    """Tests for certbot.renewal.restore_required_config_elements."""
+    def setUp(self):
+        self.config = mock.MagicMock()
+
+    @classmethod
+    def _call(cls, *args, **kwargs):
+        from certbot.renewal import restore_required_config_elements
+        return restore_required_config_elements(*args, **kwargs)
+
+    @mock.patch('certbot.renewal.cli.set_by_cli')
+    def test_allow_subset_of_names_success(self, mock_set_by_cli):
+        mock_set_by_cli.return_value = False
+        self._call(self.config, {'allow_subset_of_names': 'True'})
+        self.assertTrue(self.config.namespace.allow_subset_of_names is True)
+
+    @mock.patch('certbot.renewal.cli.set_by_cli')
+    def test_allow_subset_of_names_failure(self, mock_set_by_cli):
+        mock_set_by_cli.return_value = False
+        renewalparams = {'allow_subset_of_names': 'maybe'}
+        self.assertRaises(
+            errors.Error, self._call, self.config, renewalparams)
+
+    @mock.patch('certbot.renewal.cli.set_by_cli')
+    def test_must_staple_success(self, mock_set_by_cli):
+        mock_set_by_cli.return_value = False
+        self._call(self.config, {'must_staple': 'True'})
+        self.assertTrue(self.config.namespace.must_staple is True)
+
+    @mock.patch('certbot.renewal.cli.set_by_cli')
+    def test_must_staple_failure(self, mock_set_by_cli):
+        mock_set_by_cli.return_value = False
+        renewalparams = {'must_staple': 'maybe'}
+        self.assertRaises(
+            errors.Error, self._call, self.config, renewalparams)
+
+if __name__ == "__main__":
+    unittest.main()  # pragma: no cover

certbot/tests/storage_test.py

@@ -551,7 +551,8 @@ class RenewableCertTests(BaseRenewableCertTest):
 
         from certbot.storage import relevant_values
         with mock.patch("certbot.cli.helpful_parser", mock_parser):
-            return relevant_values(values)
+            # make a copy to ensure values isn't modified
+            return relevant_values(values.copy())
 
     def test_relevant_values(self):
         """Test that relevant_values() can reject an irrelevant value."""
@@ -567,10 +568,18 @@ class RenewableCertTests(BaseRenewableCertTest):
     def test_relevant_values_nondefault(self):
         """Test that relevant_values() can retain a non-default value."""
         values = {"rsa_key_size": 12}
-        # A copy is given to _test_relevant_values_common
-        # to make sure values isn't modified by the method
         self.assertEqual(
-            self._test_relevant_values_common(values.copy()), values)
+            self._test_relevant_values_common(values), values)
+
+    def test_relevant_values_bool(self):
+        values = {"allow_subset_of_names": True}
+        self.assertEqual(
+            self._test_relevant_values_common(values), values)
+
+    def test_relevant_values_str(self):
+        values = {"authenticator": "apache"}
+        self.assertEqual(
+            self._test_relevant_values_common(values), values)
 
     @mock.patch("certbot.storage.relevant_values")
     def test_new_lineage(self, mock_rv):

certbot/tests/testdata/sample-renewal.conf

@@ -73,4 +73,5 @@ tls_sni_01_port = 443
 logs_dir = /var/log/letsencrypt
 apache_vhost_root = /etc/apache2/sites-available
 configurator = None
+must_staple = True
 [[webroot_map]]