Repository: certbot/certbot

Issue:
Certbot drops webroot options from renewal configuration file
To reproduce this, I think you need Certbot 0.31.0+ and an ACME server that reuses authorizations.

## I ran this command and it produced this output:

Here is each command and the renewal configuration file it produces.

The plugin used first here is irrelevant, you just have to get a cert with a valid authz.
```
sudo certbot certonly --staging --webroot -w /var/www/html -d example.com
```
produces
```
# renew_before_expiry = 30 days
version = 0.34.0
archive_dir = /etc/letsencrypt/archive/example.com
cert = /etc/letsencrypt/live/example.com/cert.pem
privkey = /etc/letsencrypt/live/example.com/privkey.pem
chain = /etc/letsencrypt/live/example.com/chain.pem
fullchain = /etc/letsencrypt/live/example.com/fullchain.pem

# Options used in the renewal process
[renewalparams]
authenticator = webroot
account = 0652f690fd6e48af380ad6d6a1b4e792
webroot_path = /var/www/html,
server = https://acme-staging-v02.api.letsencrypt.org/directory
[[webroot_map]]
example.com = /var/www/html
```
This next command is the start of the problem. Notice the empty value from `webroot_map` when compared to the previous value.
```
sudo certbot certonly --staging --webroot -w /var/www/html -d example.com --renew-by-default
```
produces
```
# renew_before_expiry = 30 days
version = 0.34.0
archive_dir = /etc/letsencrypt/archive/example.com
cert = /etc/letsencrypt/live/example.com/cert.pem
privkey = /etc/letsencrypt/live/example.com/privkey.pem
chain = /etc/letsencrypt/live/example.com/chain.pem
fullchain = /etc/letsencrypt/live/example.com/fullchain.pem

# Options used in the renewal process
[renewalparams]
authenticator = webroot
account = 0652f690fd6e48af380ad6d6a1b4e792
webroot_path = /var/www/html,
server = https://acme-staging-v02.api.letsencrypt.org/directory
[[webroot_map]]
```
Finally, this command removes all references to the webroot from the renewal configuration file.
```
sudo certbot renew --force-renewal
```
produces
```
# renew_before_expiry = 30 days
version = 0.34.0
archive_dir = /etc/letsencrypt/archive/example.com
cert = /etc/letsencrypt/live/example.com/cert.pem
privkey = /etc/letsencrypt/live/example.com/privkey.pem
chain = /etc/letsencrypt/live/example.com/chain.pem
fullchain = /etc/letsencrypt/live/example.com/fullchain.pem

# Options used in the renewal process
[renewalparams]
authenticator = webroot
account = 0652f690fd6e48af380ad6d6a1b4e792
server = https://acme-staging-v02.api.letsencrypt.org/directory
```

## Certbot's behavior differed from what I expected because:

Certbot dropped the reference to the webroot path. This is bad because renewal will fail with:
```
Missing command line flag or config entry for this setting: Input the webroot for example.com
```
when authz are no longer available to be reused.

## What's going on?

While I haven't verified this, I think what's going on here is a combination of authz reuse (which Certbot understands as of version 0.31.0) and full parsing of the webroot options not happening until the challenge is preformed by the webroot plugin.

I'm imagining the 2nd command causes the value for `webroot_path` to be preserved because it is set on the command line. `webroot_map` is preserved because `webroot_path` is set on the command line and `webroot_map` is considered modified if `webroot_path` is set due to the code [here](https://github.com/certbot/certbot/blob/d391fb8876dd97d5f446bcd1667ecdce0c4b187c/certbot/cli.py#L141), however, `webroot_map` is empty because the webroot plugin never ran.

When `certbot renew --force-renewal` runs, I think `webroot_map` is initially set to it's default value and `webroot_path` is dropped due to the code [here](https://github.com/certbot/certbot/blob/d391fb8876dd97d5f446bcd1667ecdce0c4b187c/certbot/renewal.py#L103). After that, I think `webroot_map` is also dropped and not written to the renewal configuration file because it has its default value.

Discussion:
adferrand: If it solely for regression testing, I think a unit test will be sufficient. On that aspect, it is still a corner case and I would like to avoid invest time to build an integration test for corner cases that is also consuming resources (these tests are slow).

If it is about testing the capability of Certbot to reuse authorizations when it is supported by the acme server, then I agree about writing an integration tests. 

So, what do you have in mind? 
bmw: Adding some integration tests around authz reuse is probably a good idea, but I'm specifically interested in preventing regressions here.
adferrand: So, finally I do not implement the reconstruction of webroot_map in the webroot plugin during the prepare phase. Indeed, the cure is worse than the decease, as the process to move is potentially interactive, and change too much thinks in the workflow. I prefer to stay simple, and the copy of `webroot_path` alone is sufficient to fix the bug.
adferrand: @bmw, relevant unit tests have been added to renewal and webroot.
bmw: Whoops. Looks like the code from https://github.com/certbot/certbot/pull/7054 accidentally got pushed to this branch.
bmw: Also, don't forget to add a changelog entry about this.
adferrand: Fixed about the unwanted merge of #7054, and changelog updated.

Patch:
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/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]


Test patch:
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/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
diff --git a/tests/letstest/apache2_targets.yaml b/tests/letstest/apache2_targets.yaml
index dda4d4b1c..4da6abb15 100644
--- a/tests/letstest/apache2_targets.yaml
+++ b/tests/letstest/apache2_targets.yaml
@@ -1,8 +1,8 @@
 targets:
   #-----------------------------------------------------------------------------
   #Ubuntu
-  - ami: ami-064bd2d44a1d6c097
-    name: ubuntu18.10
+  - ami: ami-08ab45c4343f5f5c6
+    name: ubuntu19.04
     type: ubuntu
     virt: hvm
     user: ubuntu
diff --git a/tests/letstest/targets.yaml b/tests/letstest/targets.yaml
index d1eba43de..340fe6bf8 100644
--- a/tests/letstest/targets.yaml
+++ b/tests/letstest/targets.yaml
@@ -1,8 +1,8 @@
 targets:
   #-----------------------------------------------------------------------------
   #Ubuntu
-  - ami: ami-064bd2d44a1d6c097
-    name: ubuntu18.10
+  - ami: ami-08ab45c4343f5f5c6
+    name: ubuntu19.04
     type: ubuntu
     virt: hvm
     user: ubuntu