From 0e237e1c0b2c27a05c85b5a88330dd0f5ca37615 Mon Sep 17 00:00:00 2001 From: Thomas Mayer Date: Sat, 3 Dec 2016 06:50:57 +0100 Subject: [PATCH 1/8] Preserve --must-staple in configuration for renewal (#3844) --- certbot/renewal.py | 12 ++++++++++++ certbot/storage.py | 1 + certbot/tests/cli_test.py | 5 +++++ certbot/tests/testdata/sample-renewal.conf | 1 + 4 files changed, 19 insertions(+) diff --git a/certbot/renewal.py b/certbot/renewal.py index aa39c5fad..206f042f6 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -33,6 +33,7 @@ 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"] def renewal_conf_files(config): @@ -190,6 +191,17 @@ def _restore_required_config_elements(config, renewalparams): raise errors.Error( "Expected a numeric value for {0}".format(config_item)) setattr(config.namespace, config_item, int_value) + # bool-valued items to add if they're present + for config_item in BOOL_CONFIG_ITEMS: + if config_item in renewalparams and not cli.set_by_cli(config_item): + config_value = renewalparams[config_item] + if config_value in ("True", "False"): + # bool("False") == True + # pylint: disable=eval-used + setattr(config.namespace, config_item, eval(config_value)) + else: + raise errors.Error( + "Expected 'True' or 'False' for {0}".format(config_item)) def should_renew(config, lineage): diff --git a/certbot/storage.py b/certbot/storage.py index 1fc13a5df..f90276260 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -154,6 +154,7 @@ def _relevant(option): plugins = list(plugins_disco.PluginsRegistry.find_all()) return (option in renewal.STR_CONFIG_ITEMS or option in renewal.INT_CONFIG_ITEMS + or option in renewal.BOOL_CONFIG_ITEMS or any(option.startswith(x + "_") for x in plugins)) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 54ae74f95..96e862404 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -735,6 +735,11 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods args = ["renew", "--dry-run", "-tvv"] self._test_renewal_common(True, [], args=args, should_renew=True) + def test_must_staple_renew(self): + self._make_lineage('sample-renewal.conf') + args = ["renew", "--must-staple"] + self._test_renewal_common(True, [], args=args, should_renew=True) + def test_quiet_renew(self): self._make_lineage('sample-renewal.conf') args = ["renew", "--dry-run"] diff --git a/certbot/tests/testdata/sample-renewal.conf b/certbot/tests/testdata/sample-renewal.conf index 08032af86..52b3ec45c 100644 --- a/certbot/tests/testdata/sample-renewal.conf +++ b/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]] From 7767f62a2ef9cd1cd4226a83221ec26dd2c7f65b Mon Sep 17 00:00:00 2001 From: Thomas Mayer Date: Sat, 3 Dec 2016 07:22:55 +0100 Subject: [PATCH 2/8] Remove recently added test (#3844) --- certbot/tests/cli_test.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 96e862404..54ae74f95 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -735,11 +735,6 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods args = ["renew", "--dry-run", "-tvv"] self._test_renewal_common(True, [], args=args, should_renew=True) - def test_must_staple_renew(self): - self._make_lineage('sample-renewal.conf') - args = ["renew", "--must-staple"] - self._test_renewal_common(True, [], args=args, should_renew=True) - def test_quiet_renew(self): self._make_lineage('sample-renewal.conf') args = ["renew", "--dry-run"] From 823cba55e32547b65bc8346f9195762ce9bcff41 Mon Sep 17 00:00:00 2001 From: Thomas Mayer Date: Wed, 7 Dec 2016 23:02:42 +0100 Subject: [PATCH 3/8] Avoid eval() (#3844) --- certbot/renewal.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/certbot/renewal.py b/certbot/renewal.py index 206f042f6..12fc9c8c6 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -196,9 +196,7 @@ def _restore_required_config_elements(config, renewalparams): if config_item in renewalparams and not cli.set_by_cli(config_item): config_value = renewalparams[config_item] if config_value in ("True", "False"): - # bool("False") == True - # pylint: disable=eval-used - setattr(config.namespace, config_item, eval(config_value)) + setattr(config.namespace, config_item, (config_value == "True")) else: raise errors.Error( "Expected 'True' or 'False' for {0}".format(config_item)) From 2bbf28b4b949dfed9aa8c59cf45e2d863c111f67 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 21 Dec 2016 15:49:46 -0800 Subject: [PATCH 4/8] refactor _restore_required_config_elements --- certbot/renewal.py | 67 +++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/certbot/renewal.py b/certbot/renewal.py index c9eca9a3a..c73b2d4d3 100644 --- a/certbot/renewal.py +++ b/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 @@ -157,30 +158,48 @@ 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(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_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): From 36c9c49ab994355f97805042844a0a76a83d6743 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 21 Dec 2016 16:12:41 -0800 Subject: [PATCH 5/8] restore allow_subset_of_names --- certbot/renewal.py | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/certbot/renewal.py b/certbot/renewal.py index c73b2d4d3..23dbe191b 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -27,10 +27,11 @@ logger = logging.getLogger(__name__) # file's renewalparams and actually used in the client configuration # during the renewal process. We have to record their types here because # the renewal configuration process loses this information. +BOOL_CONFIG_ITEMS = ["allow_subset_of_names"] +INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"] 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"] def _reconstitute(config, full_path): @@ -159,6 +160,7 @@ def _restore_required_config_elements(config, renewalparams): """ 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: @@ -167,6 +169,25 @@ def _restore_required_config_elements(config, renewalparams): 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. From 5119d099664aff903e3a3581b6d0cccd6aef1b22 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 21 Dec 2016 16:24:28 -0800 Subject: [PATCH 6/8] save allow_subset_of_names in renew conf --- certbot/renewal.py | 2 ++ certbot/storage.py | 5 ++--- certbot/tests/storage_test.py | 17 +++++++++++++---- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/certbot/renewal.py b/certbot/renewal.py index 23dbe191b..6678d0247 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -32,6 +32,8 @@ INT_CONFIG_ITEMS = ["rsa_key_size", "tls_sni_01_port", "http01_port"] STR_CONFIG_ITEMS = ["config_dir", "logs_dir", "work_dir", "user_agent", "server", "account", "authenticator", "installer", "standalone_supported_challenges", "renew_hook"] +CONFIG_ITEMS = set(itertools.chain( + BOOL_CONFIG_ITEMS, INT_CONFIG_ITEMS, STR_CONFIG_ITEMS)) def _reconstitute(config, full_path): diff --git a/certbot/storage.py b/certbot/storage.py index e4fc21a85..4095ba862 100644 --- a/certbot/storage.py +++ b/certbot/storage.py @@ -183,9 +183,8 @@ 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): diff --git a/certbot/tests/storage_test.py b/certbot/tests/storage_test.py index a1fda6535..f52f31d3d 100644 --- a/certbot/tests/storage_test.py +++ b/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): From efad646960c0a9d180e7480b13e17596c95cdd6a Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 21 Dec 2016 16:49:24 -0800 Subject: [PATCH 7/8] add restore_required_config_elements test --- certbot/renewal.py | 4 ++-- certbot/tests/renewal_test.py | 35 ++++++++++++++++++++++++++++++++--- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/certbot/renewal.py b/certbot/renewal.py index 6678d0247..b8559a0a3 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -72,7 +72,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( @@ -152,7 +152,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 diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index 8155595c2..6048a6342 100644 --- a/certbot/tests/renewal_test.py +++ b/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,43 @@ 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) + + +if __name__ == "__main__": + unittest.main() # pragma: no cover From f238781c10ca411dff74fab1e84d5a94570ff6c1 Mon Sep 17 00:00:00 2001 From: Seth Schoen Date: Wed, 4 Jan 2017 17:30:34 -0800 Subject: [PATCH 8/8] Add must_staple config test to parallel allow_subset_of_names test --- certbot/tests/renewal_test.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index 6048a6342..07c4eac00 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -53,6 +53,18 @@ class RestoreRequiredConfigElementsTest(unittest.TestCase): 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