Repository: certbot/certbot
Pull request: Fix change detection on mutable values
Fixes https://github.com/certbot/certbot/issues/9825.

If we don't like this, as a workaround we could directly set `argument_sources` inside the webroot plugin which is I believe the only place in our first party code where currently we have a problem here, but I personally think we need a better long term solution than that. This is my crack at writing a longer term solution myself.

I think I'm pretty happy with the end result here, but I wrote this in one long programming session so it could definitely use another set of eyes and neurons to make sure I didn't miss anything significant and it seems sane enough though.

Problem statement:
Webroot path is not saved when entered interactively, only when given on command line
## My operating system is (include version):

Debian 12

## I installed Certbot with (snap, OS package manager, pip, certbot-auto, etc):

Docker

## I ran this command and it produced this output:

I first ran:

```
# docker run --rm -it [...volume storage setup...] certbot/certbot:v2.7.3 certonly --webroot
```

As expected, Certbot prompted for the webroot path. However, when I tried to renew, I got an error:

```
# docker run --rm -it [...] certbot/certbot:v2.7.3 renew --dry-run --no-random-sleep-on-renew
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
Processing /etc/letsencrypt/renewal/[domain].conf
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - 
Simulating renewal of an existing certificate for [domain]
Failed to renew certificate [domain] with error: Missing command line flag or config entry for this setting:
Input the webroot for [domain]:
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
All simulated renewals failed. The following certificates could not be renewed:
  /etc/letsencrypt/live/[domain]/fullchain.pem (failure)
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
1 renew failure(s), 0 parse failure(s)
```

I checked the configuration file under `/etc/letsencrypt/renewal`, and it seems that the path I entered was not saved, only the authenticator option.

I tried again with the path on the command line (after revoking the previous certificate):

```
# docker run --rm -it [...] certonly --webroot --webroot-path /srv/certbot-challenges
```

This time the path is saved to the configuration file, and renewal succeeds:

```
# docker run --rm -it [...] certbot/certbot:v2.7.3 renew --dry-run --no-random-sleep-on-renew
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Processing /etc/letsencrypt/renewal/[domain].conf
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Simulating renewal of an existing certificate for [domain]

- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Congratulations, all simulated renewals succeeded:
  /etc/letsencrypt/live/[domain]/fullchain.pem (success)
- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
```

## Certbot's behavior differed from what I expected because:

I expect the path to be saved in both cases. It doesn't make sense to break renewal like this just because I didn't include the path on the command line when I first obtained the certificate.

## Here is a Certbot log showing the issue (if available):

See output above.

## Here is the relevant nginx server block or Apache virtualhost for the domain I am configuring:

I have nginx set up to serve from the alternative webroot path for the challenges, but I believe this is not relevant to the bug I'm reporting.

Hints from issue discussion:
Thanks so much for the detailed bug report. I was able to reproduce this and we'll get this fixed ASAP.

Commit message: add changelog entry

Patch to review:
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 729f2146a9e..658fd210357 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -14,6 +14,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
+* Fixed a bug introduced in version 2.7.0 that caused interactively entered
+  webroot plugin values to not be saved for renewal.
 * Fixed a bug introduced in version 2.7.0 of our Lexicon based DNS plugins that
   caused them to fail to find the DNS zone that needs to be modified in some
   cases.
diff --git a/certbot/certbot/_internal/cli/helpful.py b/certbot/certbot/_internal/cli/helpful.py
index 2f3b4554cd4..73b4b316da0 100644
--- a/certbot/certbot/_internal/cli/helpful.py
+++ b/certbot/certbot/_internal/cli/helpful.py
@@ -165,6 +165,7 @@ def _usage_string(self, plugins: Iterable[str], help_arg: Union[str, bool]) -> s
     def remove_config_file_domains_for_renewal(self, config: NamespaceConfig) -> None:
         """Make "certbot renew" safe if domains are set in cli.ini."""
         # Works around https://github.com/certbot/certbot/issues/4096
+        assert config.argument_sources is not None
         if (config.argument_sources['domains'] == ArgumentSource.CONFIG_FILE and
                 self.verb == "renew"):
             config.domains = []
diff --git a/certbot/certbot/_internal/tests/configuration_test.py b/certbot/certbot/_internal/tests/configuration_test.py
index 8d88bb16af8..8daed4fbff4 100644
--- a/certbot/certbot/_internal/tests/configuration_test.py
+++ b/certbot/certbot/_internal/tests/configuration_test.py
@@ -165,17 +165,22 @@ def test_set_by_user_runtime_overrides(self):
 
     def test_set_by_user_exception(self):
         from certbot.configuration import NamespaceConfig
-        
+
         # a newly created NamespaceConfig has no argument sources dict, so an
         # exception is raised
         config = NamespaceConfig(self.config.namespace)
         with pytest.raises(RuntimeError):
             config.set_by_user('whatever')
-        
+
         # now set an argument sources dict
         config.set_argument_sources({})
         assert not config.set_by_user('whatever')
 
+    def test_set_by_user_mutables(self):
+        assert not self.config.set_by_user('domains')
+        self.config.domains.append('example.org')
+        assert self.config.set_by_user('domains')
+
 
 if __name__ == '__main__':
     sys.exit(pytest.main(sys.argv[1:] + [__file__]))  # pragma: no cover
diff --git a/certbot/certbot/configuration.py b/certbot/certbot/configuration.py
index 4720d7ecc8d..32fa05232ba 100644
--- a/certbot/certbot/configuration.py
+++ b/certbot/certbot/configuration.py
@@ -66,7 +66,8 @@ def __init__(self, namespace: argparse.Namespace) -> None:
         self.namespace: argparse.Namespace
         # Avoid recursion loop because of the delegation defined in __setattr__
         object.__setattr__(self, 'namespace', namespace)
-        object.__setattr__(self, 'argument_sources', None)
+        object.__setattr__(self, '_argument_sources', None)
+        object.__setattr__(self, '_previously_accessed_mutables', {})
 
         self.namespace.config_dir = os.path.abspath(self.namespace.config_dir)
         self.namespace.work_dir = os.path.abspath(self.namespace.work_dir)
@@ -90,7 +91,7 @@ def set_argument_sources(self, argument_sources: Dict[str, ArgumentSource]) -> N
         """
 
         # Avoid recursion loop because of the delegation defined in __setattr__
-        object.__setattr__(self, 'argument_sources', argument_sources)
+        object.__setattr__(self, '_argument_sources', argument_sources)
 
 
     def set_by_user(self, var: str) -> bool:
@@ -145,15 +146,48 @@ def _mark_runtime_override(self, name: str) -> None:
         """
         If an argument_sources dict was set, overwrites an argument's source to
         be ArgumentSource.RUNTIME. Used when certbot sets an argument's values
-        at runtime.
+        at runtime. This also clears the modified value from
+        _previously_accessed_mutables since it is no longer needed.
         """
-        if self.argument_sources is not None:
-            self.argument_sources[name] = ArgumentSource.RUNTIME
+        if self._argument_sources is not None:
+            self._argument_sources[name] = ArgumentSource.RUNTIME
+            if name in self._previously_accessed_mutables:
+                del self._previously_accessed_mutables[name]
+
+    @property
+    def argument_sources(self) -> Optional[Dict[str, ArgumentSource]]:
+        """Returns _argument_sources after handling any changes to accessed mutable values."""
+        # We keep values in _previously_accessed_mutables until we've detected a modification to try
+        # to provide up-to-date information when argument_sources is accessed. Once a mutable object
+        # has been accessed, it can be modified at any time if a reference to it was kept somewhere
+        # else.
+
+        # We copy _previously_accessed_mutables because _mark_runtime_override modifies it.
+        for name, prev_value in self._previously_accessed_mutables.copy().items():
+            current_value = getattr(self.namespace, name)
+            if current_value != prev_value:
+                self._mark_runtime_override(name)
+        return self._argument_sources
 
     # Delegate any attribute not explicitly defined to the underlying namespace object.
+    #
+    # If any mutuable namespace attributes are explicitly defined in the future, you'll probably
+    # want to take an approach like the one used in __getattr__ and the argument_sources property.
 
     def __getattr__(self, name: str) -> Any:
-        return getattr(self.namespace, name)
+        arg_sources = self.argument_sources
+        value = getattr(self.namespace, name)
+        if arg_sources is not None:
+            # If the requested attribute was already modified at runtime, we don't need to track any
+            # future changes.
+            if name not in arg_sources or arg_sources[name] != ArgumentSource.RUNTIME:
+                # If name is already in _previously_accessed_mutables, we don't need to make a copy
+                # of it again. If its value was changed, this would have been caught while preparing
+                # the return value of the property self.argument_sources accessed earlier in this
+                # function.
+                if name not in self._previously_accessed_mutables and not _is_immutable(value):
+                    self._previously_accessed_mutables[name] = copy.deepcopy(value)
+        return value
 
     def __setattr__(self, name: str, value: Any) -> None:
         self._mark_runtime_override(name)
@@ -425,9 +459,10 @@ def __deepcopy__(self, _memo: Any) -> 'NamespaceConfig':
         # Work around https://bugs.python.org/issue1515 for py26 tests :( :(
         new_ns = copy.deepcopy(self.namespace)
         new_config = type(self)(new_ns)
-        if self.set_argument_sources is not None:
-            new_sources = copy.deepcopy(self.argument_sources)
-            new_config.set_argument_sources(new_sources)
+        # Avoid recursion loop because of the delegation defined in __setattr__
+        object.__setattr__(new_config, '_argument_sources', copy.deepcopy(self.argument_sources))
+        object.__setattr__(new_config, '_previously_accessed_mutables',
+                           copy.deepcopy(self._previously_accessed_mutables))
         return new_config
 
 
@@ -450,3 +485,15 @@ def _check_config_sanity(config: NamespaceConfig) -> None:
         for domain in config.namespace.domains:
             # This may be redundant, but let's be paranoid
             util.enforce_domain_sanity(domain)
+
+
+def _is_immutable(value: Any) -> bool:
+    """Is value of an immutable type?"""
+    if isinstance(value, tuple):
+        # tuples are only immutable if all contained values are immutable.
+        return all(_is_immutable(subvalue) for subvalue in value)
+    for immutable_type in (int, float, complex, str, bytes, bool, frozenset,):
+        if isinstance(value, immutable_type):
+            return True
+    # The last case we consider here is None which is also immutable.
+    return value is None
diff --git a/certbot/certbot/tests/util.py b/certbot/certbot/tests/util.py
index bbe2ffe9c13..5d078cfbdb8 100644
--- a/certbot/certbot/tests/util.py
+++ b/certbot/certbot/tests/util.py
@@ -1,6 +1,7 @@
 """Test utilities."""
 import atexit
 from contextlib import ExitStack
+import copy
 from importlib import reload as reload_module
 import io
 import logging
@@ -403,7 +404,8 @@ class ConfigTestCase(TempDirTestCase):
     def setUp(self) -> None:
         super().setUp()
         self.config = configuration.NamespaceConfig(
-            mock.MagicMock(**constants.CLI_DEFAULTS),
+            # We make a copy here so any mutable values from CLI_DEFAULTS do not get modified.
+            mock.MagicMock(**copy.deepcopy(constants.CLI_DEFAULTS)),
         )
         self.config.set_argument_sources({})
         self.config.namespace.verb = "certonly"


Code review comments:
diff_hunk: @@ -145,15 +146,48 @@ def _mark_runtime_override(self, name: str) -> None:
         """
         If an argument_sources dict was set, overwrites an argument's source to
         be ArgumentSource.RUNTIME. Used when certbot sets an argument's values
-        at runtime.
+        at runtime. This also clears the modified value from
+        _previously_accessed_mutables since it is no longer needed.
         """
-        if self.argument_sources is not None:
-            self.argument_sources[name] = ArgumentSource.RUNTIME
+        if self._argument_sources is not None:
+            self._argument_sources[name] = ArgumentSource.RUNTIME
+            if name in self._previously_accessed_mutables:
+                del self._previously_accessed_mutables[name]
+
+    @property
+    def argument_sources(self) -> Optional[Dict[str, ArgumentSource]]:
+        """Returns _argument_sources after handling any changes to accessed mutable values."""
+        # We keep values in _previously_accessed_mutables until we've detected a modification to try
+        # to provide up-to-date information when argument_sources is accessed. Once a mutable object
+        # has been accessed, it can be modified at any time if a reference to it was kept somewhere
+        # else.
+
+        # We copy _previously_accessed_mutables because _mark_runtime_override modifies it.
+        for name, prev_value in self._previously_accessed_mutables.copy().items():
+            current_value = getattr(self.namespace, name)
+            if current_value != prev_value:
+                self._mark_runtime_override(name)
+        return self._argument_sources
 
     # Delegate any attribute not explicitly defined to the underlying namespace object.
+    #
+    # If any mutuable namespace attributes are explicitly defined in the future, you'll probably
path: certbot/certbot/configuration.py
text: @user1:
nit: `mutable` typo