From 7d35f95293b40159ef15f38eed448168e158ce56 Mon Sep 17 00:00:00 2001 From: Adrien Ferrand Date: Wed, 29 May 2019 00:16:12 +0200 Subject: [PATCH] Avoid to delete both webroot_map and webroot_path (#7095) * Always restore webroot_path in renewal config. * Add unit tests to ensure correct behavior * Add changelog * Add certbot as modified package --- CHANGELOG.md | 4 +++- certbot/plugins/webroot_test.py | 13 +++++++++++++ certbot/renewal.py | 10 +++++----- certbot/tests/renewal_test.py | 24 ++++++++++++++++++++++++ 4 files changed, 45 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cc3ff41f..8669d34a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,12 +16,14 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Fixed -* +* Renewal parameter `webroot_path` is always saved, avoiding some regressions + when `webroot` authenticator plugin is invoked with no challenge to perform. Despite us having broken lockstep, we are continuing to release new versions of all Certbot components during releases for the time being, however, the only package with changes other than its version number was: +* certbot * certbot-dns-rfc2136 More details about these changes can be found on our GitHub repo. diff --git a/certbot/plugins/webroot_test.py b/certbot/plugins/webroot_test.py index 3a902b91f..bca1045d8 100644 --- a/certbot/plugins/webroot_test.py +++ b/certbot/plugins/webroot_test.py @@ -295,6 +295,19 @@ class WebrootActionTest(unittest.TestCase): self.assertEqual( config.webroot_map[self.achall.domain], self.path) + def test_webroot_map_partial_without_perform(self): + # This test acknowledges the fact that webroot_map content will be partial if webroot + # plugin perform method is not invoked (corner case when all auths are already valid). + # To not be a problem, the webroot_path must always been conserved during renew. + # This condition is challenged by: + # certbot.tests.renewal_tests::RenewalTest::test_webroot_params_conservation + # See https://github.com/certbot/certbot/pull/7095 for details. + other_webroot_path = tempfile.mkdtemp() + args = self.parser.parse_args("-w {0} -d {1} -w {2} -d bar".format( + self.path, self.achall.domain, other_webroot_path).split()) + self.assertEqual(args.webroot_map, {self.achall.domain: self.path}) + self.assertEqual(args.webroot_path, [self.path, other_webroot_path]) + def _get_config_after_perform(self, config): from certbot.plugins.webroot import Authenticator auth = Authenticator(config, "webroot") diff --git a/certbot/renewal.py b/certbot/renewal.py index f932269b6..4b4a5a082 100644 --- a/certbot/renewal.py +++ b/certbot/renewal.py @@ -106,11 +106,11 @@ def _restore_webroot_config(config, renewalparams): restoring logic is not able to correctly parse it from the serialized form. """ - if "webroot_map" in renewalparams: - if not cli.set_by_cli("webroot_map"): - config.webroot_map = renewalparams["webroot_map"] - elif "webroot_path" in renewalparams: - logger.debug("Ancient renewal conf file without webroot-map, restoring webroot-path") + if "webroot_map" in renewalparams and not cli.set_by_cli("webroot_map"): + config.webroot_map = renewalparams["webroot_map"] + # To understand why webroot_path and webroot_map processing are not mutually exclusive, + # see https://github.com/certbot/certbot/pull/7095 + if "webroot_path" in renewalparams and not cli.set_by_cli("webroot_path"): wp = renewalparams["webroot_path"] if isinstance(wp, six.string_types): # prior to 0.1.0, webroot_path was a string wp = [wp] diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index dac585239..e33869e13 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -28,6 +28,29 @@ class RenewalTest(test_util.ConfigTestCase): renewal._restore_webroot_config(config, renewalparams) self.assertEqual(config.webroot_path, ['/var/www/']) + @mock.patch('certbot.renewal.cli.set_by_cli') + def test_webroot_params_conservation(self, mock_set_by_cli): + # For more details about why this test is important, see: + # certbot.plugins.webroot_test::WebrootActionTest::test_webroot_map_partial_without_perform + from certbot import renewal + mock_set_by_cli.return_value = False + + renewalparams = { + 'webroot_map': {'test.example.com': '/var/www/test'}, + 'webroot_path': ['/var/www/test', '/var/www/other'], + } + renewal._restore_webroot_config(self.config, renewalparams) # pylint: disable=protected-access + self.assertEqual(self.config.webroot_map, {'test.example.com': '/var/www/test'}) + self.assertEqual(self.config.webroot_path, ['/var/www/test', '/var/www/other']) + + renewalparams = { + 'webroot_map': {}, + 'webroot_path': '/var/www/test', + } + renewal._restore_webroot_config(self.config, renewalparams) # pylint: disable=protected-access + self.assertEqual(self.config.webroot_map, {}) + self.assertEqual(self.config.webroot_path, ['/var/www/test']) + class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): """Tests for certbot.renewal.restore_required_config_elements.""" @@ -89,5 +112,6 @@ class RestoreRequiredConfigElementsTest(test_util.ConfigTestCase): self.assertRaises( errors.Error, self._call, self.config, renewalparams) + if __name__ == "__main__": unittest.main() # pragma: no cover