Repository: certbot/certbot
Pull request: Fix add deprecated argument
Fixes https://github.com/certbot/certbot/issues/8495.

To further explain the problem here, `modify_kwargs_for_default_detection` as called in `add` is simplistic and doesn't always work. See https://github.com/certbot/certbot/issues/6164 for one other example.

In this case, were bitten by the code https://github.com/certbot/certbot/blob/d1e7404358c05734aaf436ef3c9d709029d62b09/certbot/certbot/_internal/cli/helpful.py#L393-L395

The action used for deprecated arguments isn't in `ZERO_ARG_ACTIONS` so it assumes that all deprecated flags take one parameter.

Rather than trying to fix this function (which I think can only realistically be fixed by https://github.com/certbot/certbot/issues/4493), I took the approach that was previously used in `HelpfulArgumentParser.add_deprecated_argument` of bypassing this extra logic entirely. I adapted that function to now call `HelpfulArgumentParser.add` as well for consistency and to make testing easier.

Problem statement:
manual-public-ip-logging-ok deprecation leads to error
I am regularly running Certbot (latest docker image) to obtain new certificates when necessary. Tonight the Certbot command failed, which seems to be related to the deprecation of `manual-public-ip-logging-ok` within version `1.11.0`. Seems like this flag now requires an argument, which it previously did not. This is easy to fix, as it just means to remove the `--manual-public-ip-logging-ok` flag and things run fine again.


```
Use of --manual-public-ip-logging-ok is deprecated.
Saving debug log to /var/log/letsencrypt/letsencrypt.log
Plugins selected: Authenticator manual, Installer None
usage: 
  certbot [SUBCOMMAND] [options] [-d DOMAIN] [-d DOMAIN] ...
Certbot can obtain and install HTTPS/TLS/SSL certificates.  By default,
it will attempt to use a webserver both for obtaining and installing the
certificate. 
certbot: error: argument --manual-public-ip-logging-ok: expected one argument
```

Hints from issue discussion:
Hi @st-h,

Could you please share the exact command which produces this error?
This is the command when it failed:
```
certbot certonly --manual --preferred-challenges dns-01 -m $ACME_EMAIL --manual-public-ip-logging-ok --agree-tos --no-bootstrap --non-interactive --manual-auth-hook './src/authenticate.sh' --manual-cleanup-hook './src/cleanup.sh' --deploy-hook './src/habitat.sh' --domains *.$DOMAIN,$DOMAIN --cert-name $DOMAIN --server $ACME_URL
```

This is the log output when getting the docker image.
```
Using docker image sha256:c3703e180159a0e6660a648f890a9ab8bc5317f8a20541fbd52dc656b9a5aee5 for certbot/certbot with digest certbot/certbot@sha256:86264d55274f9c9bcc59572e0394f9bbc84463c0ca0990bbb92d31a7122685b6 ...
```
I'm still having problems reproducing this.

Here is what I'm trying on the latest Docker release, adapted from your command (just replaced the env variables):

    $ sudo docker run --rm -it certbot/certbot:v1.10.0 certonly --manual --preferred-challenges dns-01 -m test@letsdebug.net --manual-public-ip-logging-ok --agree-tos --no-bootstrap --non-interactive --manual-auth-hook '/bin/true' --manual-cleanup-hook '/bin/true' --deploy-hook '/bin/true' --domains example.org --cert-name example.org --server https://acme-staging-v02.api.letsencrypt.org/directory
    Use of --manual-public-ip-logging-ok is deprecated.
    Saving debug log to /var/log/letsencrypt/letsencrypt.log
    Plugins selected: Authenticator manual, Installer None
    Account registered.
    Requesting a certificate for example.org
    Performing the following challenges:
    dns-01 challenge for example.org
    Running manual-auth-hook command: /bin/true
    Waiting for verification...
    Challenge failed for domain example.org
    dns-01 challenge for example.org
    Cleaning up challenges
    Running manual-cleanup-hook command: /bin/true
    Some challenges have failed.

    IMPORTANT NOTES:
    - The following errors were reported by the server:

      Domain: example.org
      Type:   dns
      Detail: DNS problem: NXDOMAIN looking up TXT for
      _acme-challenge.example.org - check that a DNS record exists for
      this domain

@alexzorin sorry, but I currently don't have an idea what might has been causing this then. (I am running this within gitlab CI, and retriggering the failed built produces the same error). Please feel free to close, though. It's fairly easy to fix in case someone has identical issues, so probably not worth the time to dig further.
I can reproduce it. The key is that the error will only happen if the certificate issuance is successful.

Apparently `storage.new_lineage` eventually triggers reparsing of all of the CLI arguments. `storage.new_lineage` triggers this error in `relevant_values` -> `cli.set_by_cli`:

https://github.com/certbot/certbot/blob/31b5f1310e1fd0331faaa7efbcb8cccdfee93e78/certbot/certbot/_internal/storage.py#L1056

The second reparsing of the CLI arguments is going wrong and somehow it thinks that `nargs=1`. I'm not sure why yet.

That's a bit weird. In my case the successful run after the failure did not renew the cert, but prints:
```
certificate not yet due for renewal; no action taken.
```
Just looked it up, the certs have been renewed a week ago. I run Certbot once a day. So, I shouldn't be seeing another renewal today. But probably the outcome of a noop and a renewal is the same in this case.
I'll be happy to re-run the failing build once this is fixed to verify.
The renewed certificate does get saved, but Certbot crashes while it is saving the renewal parameters file.
@alexzorin I am very certain that this hasn't been the case. When Certbot successfully renews a cert, it uploads it to a central storage. The cert then gets distributed to all the nodes that need it. In case Certbot crashes, the whole thing just aborts. But anyway, I suspect that this does not matter at all regarding the cause of this issue.
>When Certbot successfully renews a cert, it uploads it to a central storage. The cert then gets distributed to all the nodes that need it

Sorry, I should have been more clear. Certbot is crashing before the `--deploy-hook`, but after it has written the updated certificate files to `/etc/letsencrypt/{live,archive}/`. The "Congratulations!" never gets a chance to be printed either.
thanks for clarifying. But as Certbot renewed the cert a week ago, it shouldn't be renewing it again today... Even if it crashed after writing the new cert to the file system, the next run of Certbot would not know about that cert, because it would have never been saved to the storage. Each run starts with a clean file system. 

Yet, I don't think that matters much. I am certain when I rerun the failed build it will work fine after the issue with renewal is fixed.

Commit message: Add changelog entry

Patch to review:
diff --git a/certbot-ci/certbot_integration_tests/utils/certbot_call.py b/certbot-ci/certbot_integration_tests/utils/certbot_call.py
index a71c610e560..2ddaa41c8ad 100755
--- a/certbot-ci/certbot_integration_tests/utils/certbot_call.py
+++ b/certbot-ci/certbot_integration_tests/utils/certbot_call.py
@@ -92,6 +92,7 @@ def _prepare_args_env(certbot_args, directory_url, http_01_port, tls_alpn_01_por
         '--no-verify-ssl',
         '--http-01-port', str(http_01_port),
         '--https-port', str(tls_alpn_01_port),
+        '--manual-public-ip-logging-ok',
         '--config-dir', config_dir,
         '--work-dir', os.path.join(workspace, 'work'),
         '--logs-dir', os.path.join(workspace, 'logs'),
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 94dde1e11c4..f9813bba474 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -14,7 +14,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Fixed
 
-*
+* Fixed a bug in `certbot.util.add_deprecated_argument` that caused the
+  deprecated `--manual-public-ip-logging-ok` flag to crash Certbot in some
+  scenarios.
 
 More details about these changes can be found on our GitHub repo.
 
diff --git a/certbot/certbot/_internal/cli/helpful.py b/certbot/certbot/_internal/cli/helpful.py
index 141dd41a415..434e7104646 100644
--- a/certbot/certbot/_internal/cli/helpful.py
+++ b/certbot/certbot/_internal/cli/helpful.py
@@ -2,8 +2,10 @@
 from __future__ import print_function
 import argparse
 import copy
+import functools
 import glob
 import sys
+
 import configargparse
 import six
 import zope.component
@@ -356,6 +358,18 @@ def add(self, topics, *args, **kwargs):
         :param dict **kwargs: various argparse settings for this argument
 
         """
+        action = kwargs.get("action")
+        if action is util.DeprecatedArgumentAction:
+            # If the argument is deprecated through
+            # certbot.util.add_deprecated_argument, it is not shown in the help
+            # output and any value given to the argument is thrown away during
+            # argument parsing. Because of this, we handle this case early
+            # skipping putting the argument in different help topics and
+            # handling default detection since these actions aren't needed and
+            # can cause bugs like
+            # https://github.com/certbot/certbot/issues/8495.
+            self.parser.add_argument(*args, **kwargs)
+            return
 
         if isinstance(topics, list):
             # if this flag can be listed in multiple sections, try to pick the one
@@ -410,8 +424,22 @@ def add_deprecated_argument(self, argument_name, num_args):
         :param int nargs: Number of arguments the option takes.
 
         """
-        util.add_deprecated_argument(
-            self.parser.add_argument, argument_name, num_args)
+        # certbot.util.add_deprecated_arguments expects the normal add_argument
+        # interface provided by argparse. This is what is given including when
+        # certbot.util.add_deprecated_arguments is used by plugins, however, in
+        # that case the first argument to certbot.util.add_deprecated_arguments
+        # is certbot._internal.cli.HelpfulArgumentGroup.add_argument which
+        # internally calls the add method of this class.
+        #
+        # The difference between the add method of this class and the standard
+        # argparse add_argument method caused a bug in the past (see
+        # https://github.com/certbot/certbot/issues/8495) so we use the same
+        # code path here for consistency and to ensure it works. To do that, we
+        # wrap the add method in a similar way to
+        # HelpfulArgumentGroup.add_argument by providing a help topic (which in
+        # this case is set to None).
+        add_func = functools.partial(self.add, None)
+        util.add_deprecated_argument(add_func, argument_name, num_args)
 
     def add_group(self, topic, verbs=(), **kwargs):
         """Create a new argument group.
diff --git a/certbot/certbot/util.py b/certbot/certbot/util.py
index fd7b46f85c1..8db5ab34a8c 100644
--- a/certbot/certbot/util.py
+++ b/certbot/certbot/util.py
@@ -439,7 +439,7 @@ def safe_email(email):
     return False
 
 
-class _ShowWarning(argparse.Action):
+class DeprecatedArgumentAction(argparse.Action):
     """Action to log a warning when an argument is used."""
     def __call__(self, unused1, unused2, unused3, option_string=None):
         logger.warning("Use of %s is deprecated.", option_string)
@@ -458,16 +458,16 @@ def add_deprecated_argument(add_argument, argument_name, nargs):
     :param nargs: Value for nargs when adding the argument to argparse.
 
     """
-    if _ShowWarning not in configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE:
+    if DeprecatedArgumentAction not in configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE:
         # In version 0.12.0 ACTION_TYPES_THAT_DONT_NEED_A_VALUE was
         # changed from a set to a tuple.
         if isinstance(configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE, set):
             configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE.add(
-                _ShowWarning)
+                DeprecatedArgumentAction)
         else:
             configargparse.ACTION_TYPES_THAT_DONT_NEED_A_VALUE += (
-                _ShowWarning,)
-    add_argument(argument_name, action=_ShowWarning,
+                DeprecatedArgumentAction,)
+    add_argument(argument_name, action=DeprecatedArgumentAction,
                  help=argparse.SUPPRESS, nargs=nargs)
 
 
diff --git a/certbot/tests/helpful_test.py b/certbot/tests/helpful_test.py
index 292e5530440..1a5c2bea6c6 100644
--- a/certbot/tests/helpful_test.py
+++ b/certbot/tests/helpful_test.py
@@ -1,6 +1,11 @@
 """Tests for certbot.helpful_parser"""
 import unittest
 
+try:
+    import mock
+except ImportError: # pragma: no cover
+    from unittest import mock
+
 from certbot import errors
 from certbot._internal.cli import HelpfulArgumentParser
 from certbot._internal.cli import _DomainsAction
@@ -189,5 +194,16 @@ def test_parse_args_hosts_and_auto_hosts(self):
             arg_parser.parse_args()
 
 
+class TestAddDeprecatedArgument(unittest.TestCase):
+    """Tests for add_deprecated_argument method of HelpfulArgumentParser"""
+
+    @mock.patch.object(HelpfulArgumentParser, "modify_kwargs_for_default_detection")
+    def test_no_default_detection_modifications(self, mock_modify):
+        arg_parser = HelpfulArgumentParser(["run"], {}, detect_defaults=True)
+        arg_parser.add_deprecated_argument("--foo", 0)
+        arg_parser.parse_args()
+        mock_modify.assert_not_called()
+
+
 if __name__ == '__main__':
     unittest.main()  # pragma: no cover


Code review comments:
diff_hunk: @@ -410,8 +424,22 @@ def add_deprecated_argument(self, argument_name, num_args):
         :param int nargs: Number of arguments the option takes.
 
         """
-        util.add_deprecated_argument(
-            self.parser.add_argument, argument_name, num_args)
+        # certbot.util.add_deprecated_arguments expects the normal add_argument
+        # interface provided by argparse. This is what is given including when
+        # certbot.util.add_deprecated_arguments is used by plugins, however, in
+        # that case the first argument to certbot.util.add_deprecated_arguments
path: certbot/certbot/_internal/cli/helpful.py
text: @user1:
Oops, one minor typo in this explainer: `add_deprecated_arguments` -> `add_deprecated_argument`.

Otherwise LGTM.

@user1:
There's 2 other instances in that paragraph (sorry)

@author:
No worries. Thanks for catching them.