Repository: certbot/certbot
Pull request: Add extra challenge info to `--debug-challenges`
Fixes #6042, I believe.

This PR outputs the URLs (`http-01`) and FQDNs (`dns-01`) used for the challenges and their expected values when Certbot is being run with the `--debug-challenges` option.

This allows users to double/triple-check their challenges before continuing the validation.

Certbot currently does not support the `tls-alpn-01` challenge, so I didn't include it.


Also, it seems the token isn't available in `auth_handler_test.py`. Or at least I can't find it. So I couldn't use the token for the `assertIn()` check, but I'm pretty sure this is enough, right?


Problem statement:
[Feature Request] Print output on how to validate challenges with --debug-challenges
Can you please add a parameter, that keeps certbot running after the challenge is served in standalone mode?
So that the internal webserver is still running, that would simplify debugging complex network issues.
Currently with "--debug-challenge --test-cert", one is asked to press return *before* the internal webserver is up. And after one pressed return it exites immediately after the challenge is completed or has failed.
Or just add a "--debug-internal-webserver" parameter that starts the internal webserver, serves a debug page, shows it's url and waits until it gets killed.

Hints from issue discussion:
This is a great idea, feel free to submit a PR adding this feature! We called our internal webserver "standalone", so the flag should maybe reference that.
I think there may have been a misunderstanding here.

When running with `--debug-challenges` using the standalone plugin, the internal webserver is running when asked to press enter to continue. I think the only difference between `--debug-challenges` and the feature implemented in #6063 is the former waits before telling Let's Encrypt to verify the challenge while the latter waits afterwards. Do we need flags for both or am I missing something else?
The difference is, that if you wait before, you don't get the debug output of the internal webserver. This is only outputted to the console if one waits afterwards. Also the Challenge URL is not printed to the console if you wait before it.
I think you only get output from the internal webserver in the case Let's Encrypt successfully reaches it. Depending on the issue you're trying to debug, you may not get that.

As for the challenge URL, the only place that's included in the case Let's Encrypt can't reach the internal webserver is in the error output from Let's Encrypt. I'm not confident this URL is included for every error case either.

Sorry for the false start on this and @ohemorange should chime in if she disagrees with my preferred approach here, but I think we should just solve the problem of not giving the user any information about how to validate the challenge themselves when `--debug-challenges` is used. This can include information like the challenge URL and allows the user to try and fix the problem before they start eating into rate limits imposed by the CA.
Yea, that would also solve my problem.
So to sum this up:
- I propose giving more debug output with `--debug-challenges` then.
- This output should include received queries and the corresponding result sent back (like when letsencrypt is responding to the challenge)
- Additional debug information needed: The full Challenge url and the expected token to verify the weberver output (this is useful if one has a reverse proxy, load balance, SNI based request routing, only route .whell-known/acme... towards letsencrypt or has excessive caching infrastructure in between to debug problems there)

For not hitting rate limits, one could simply use the staging server by using the testing parameters. Than you can spam the api more frequently :p

@bmw I would be happy if someone of you could implement this, as I'm not really experienced with the codebase and I currently don't have much time to get used to it.
I think the main thing that needs to be added when running with `--debug-challenges` for the HTTP challenge is the full challenge URL and the expected contents like you said.

The same output from the server that is printed when Let's Encrypt is querying the challenge should already be printed, but I'd be fine with adding more `DEBUG` level logging information here if you think something is missing.

While we should definitely do this, we probably won't be able to get to this for a while. @agowa338, if you're interested and available to work on it, I'm happy to answer any questions or give you pointers to what needs to be changed to speed along the process for you.
I'm happy to take this on
I've looked into the code and I'm not sure about how to access the expected challenge response. 

I'm assuming it's the `KeyAuthorizationChallengeResponse.key_authorization` attribute, and I have not found a way to access it from an `achall` object directly.
I found how to generate a new `key_autorization` with `KeyAuthorizationChallenge.validation()`, specifically: `achall.validation(achall.account_key)`, but I'm not sure that matches the one generated previously for the challenge.

Am I missing something?



You'll want to use `achall.validation(achall.account_key)`. See [this method](https://github.com/certbot/certbot/blob/3de3188dd6fe4c8c9952848c155a56944b55ec2d/certbot/plugins/manual.py#L209) in our manual plugin as an example.

Note that [support for TLS-SNI-01 challenges is deprecated](https://github.com/certbot/certbot/blob/master/CHANGELOG.md#changed-4) so I don't think we have to add instructions for how to manually verify those challenges.
I'm not 100% confident I've understood what was required here, so before opening a PR I want to double check.
Here's what I've implemented: https://github.com/larabr/certbot/commit/3d3d458065cc147f513c411ca51f10e706ebe455
Basically I add the URI and expected response to the printed error.

Commit message: Mention feature in --help output

Patch to review:
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index 9794e84268f..73d2a5a7cba 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -6,6 +6,9 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
 
 ### Added
 
+* When the `--debug-challenges` option is used in combination with `-v`, Certbot
+  now displays the challenge URLs (for `http-01` challenges) or FQDNs (for
+  `dns-01` challenges) and their expected return values.
 *
 
 ### Changed
diff --git a/certbot/certbot/_internal/auth_handler.py b/certbot/certbot/_internal/auth_handler.py
index 16b8dfdbe47..3bdaa3d9c38 100644
--- a/certbot/certbot/_internal/auth_handler.py
+++ b/certbot/certbot/_internal/auth_handler.py
@@ -87,9 +87,34 @@ def handle_authorizations(self, orderr: messages.OrderResource,
 
                 # If debug is on, wait for user input before starting the verification process.
                 if config.debug_challenges:
+                    msg = []
+                    if config.verbose_count > 0:
+                        http01_achalls = {}
+                        dns01_achalls = {}
+                        for achall in achalls:
+                            if isinstance(achall.chall, challenges.HTTP01):
+                                http01_achalls[achall.chall.uri(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                            if isinstance(achall.chall, challenges.DNS01):
+                                dns01_achalls[achall.validation_domain_name(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                        if http01_achalls:
+                            msg.append("\nThe following URLs should be accessible from the world "
+                                       "wide web and return the value mentioned:\n")
+                            for uri, token in http01_achalls.items():
+                                msg.append(f"{uri}: {token}")
+                        if dns01_achalls:
+                            msg.append("\nThe following FQDNs should return a TXT resource "
+                                       "record with the value mentioned:\n")
+                            for fqdn, token in dns01_achalls.items():
+                                msg.append(f"{fqdn}: {token}")
+                    else:
+                        msg = ['Pass "-v" for more info about challenges.']
                     display_util.notification(
-                        'Challenges loaded. Press continue to submit to CA. '
-                        'Pass "-v" for more info about challenges.', pause=True)
+                        'Challenges loaded. Press continue to submit to CA.\n' + '\n'.join(msg),
+                        pause=True)
             except errors.AuthorizationError as error:
                 logger.critical('Failure in setting up challenges.')
                 logger.info('Attempting to clean up outstanding challenges...')
diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py
index a69f9666680..d11a454b146 100644
--- a/certbot/certbot/_internal/cli/__init__.py
+++ b/certbot/certbot/_internal/cli/__init__.py
@@ -264,7 +264,9 @@ def prepare_and_parse_args(plugins: plugins_disco.PluginsRegistry, args: List[st
         [None, "certonly", "run"], "--debug-challenges", action="store_true",
         default=flag_default("debug_challenges"),
         help="After setting up challenges, wait for user input before "
-             "submitting to CA")
+             "submitting to CA. When used in combination with the `-v` "
+             "option, the challenge URLs or FQDNs and their expected "
+             "return values are shown.")
     helpful.add(
         "testing", "--no-verify-ssl", action="store_true",
         help=config_help("no_verify_ssl"),
diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py
index a94259a7991..fe9e22f06d3 100644
--- a/certbot/tests/auth_handler_test.py
+++ b/certbot/tests/auth_handler_test.py
@@ -3,6 +3,7 @@
 import logging
 import unittest
 
+from josepy import b64encode
 try:
     import mock
 except ImportError:  # pragma: no cover
@@ -72,13 +73,13 @@ def setUp(self):
         with mock.patch("zope.component.provideUtility"):
             display_obj.set_display(self.mock_display)
 
-        self.mock_auth = mock.MagicMock(name="ApacheConfigurator")
+        self.mock_auth = mock.MagicMock(name="Authenticator")
 
         self.mock_auth.get_chall_pref.return_value = [challenges.HTTP01]
 
         self.mock_auth.perform.side_effect = gen_auth_resp
 
-        self.mock_account = mock.Mock(key=util.Key("file_path", "PEM"))
+        self.mock_account = mock.MagicMock()
         self.mock_net = mock.MagicMock(spec=acme_client.ClientV2)
         self.mock_net.acme_version = 1
         self.mock_net.retry_after.side_effect = acme_client.ClientV2.retry_after
@@ -191,15 +192,73 @@ def test_name3_http_01_3_common_acme_2(self):
 
     def test_debug_challenges(self):
         config = mock.Mock(debug_challenges=True)
-        authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)]
+        config.verbose_count = 0
+        authzrs = [gen_dom_authzr(domain="0", challs=[acme_util.HTTP01])]
+        mock_order = mock.MagicMock(authorizations=authzrs)
+
+        account_key_thumbprint = b"foobarbaz"
+        self.mock_account.key.thumbprint.return_value = account_key_thumbprint
+
+        self.mock_net.poll.side_effect = _gen_mock_on_poll()
+
+        self.handler.handle_authorizations(mock_order, config)
+
+        self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
+        self.assertEqual(self.mock_display.notification.call_count, 1)
+        self.assertIn('Pass "-v" for more info',
+                      self.mock_display.notification.call_args[0][0])
+        self.assertNotIn(f"http://{authzrs[0].body.identifier.value}/.well-known/acme-challenge/" +
+                         b64encode(authzrs[0].body.challenges[0].chall.token).decode(),
+                         self.mock_display.notification.call_args[0][0])
+        self.assertNotIn(b64encode(account_key_thumbprint).decode(),
+                      self.mock_display.notification.call_args[0][0])
+
+    def test_debug_challenges_verbose_http01(self):
+        config = mock.Mock(debug_challenges=True)
+        config.verbose_count = 1
+        authzrs = [gen_dom_authzr(domain="0", challs=[acme_util.HTTP01])]
+        mock_order = mock.MagicMock(authorizations=authzrs)
+
+        account_key_thumbprint = b"foobarbaz"
+        self.mock_account.key.thumbprint.return_value = account_key_thumbprint
+
+        self.mock_net.poll.side_effect = _gen_mock_on_poll()
+
+        self.handler.handle_authorizations(mock_order, config)
+
+        self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
+        self.assertEqual(self.mock_display.notification.call_count, 1)
+        self.assertNotIn('Pass "-v" for more info',
+                         self.mock_display.notification.call_args[0][0])
+        self.assertIn(f"http://{authzrs[0].body.identifier.value}/.well-known/acme-challenge/" +
+                      b64encode(authzrs[0].body.challenges[0].chall.token).decode(),
+                      self.mock_display.notification.call_args[0][0])
+        self.assertIn(b64encode(account_key_thumbprint).decode(),
+                      self.mock_display.notification.call_args[0][0])
+
+    def test_debug_challenges_verbose_dns01(self):
+        config = mock.Mock(debug_challenges=True)
+        config.verbose_count = 1
+        authzrs = [gen_dom_authzr(domain="0", challs=[acme_util.DNS01])]
         mock_order = mock.MagicMock(authorizations=authzrs)
 
+        account_key_thumbprint = b"foobarbaz"
+        self.mock_account.key.thumbprint.return_value = account_key_thumbprint
+
         self.mock_net.poll.side_effect = _gen_mock_on_poll()
 
+        self.mock_auth.get_chall_pref.return_value = [challenges.DNS01]
+
         self.handler.handle_authorizations(mock_order, config)
 
         self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
         self.assertEqual(self.mock_display.notification.call_count, 1)
+        self.assertNotIn('Pass "-v" for more info',
+                         self.mock_display.notification.call_args[0][0])
+        self.assertIn(f"_acme-challenge.{authzrs[0].body.identifier.value}",
+                      self.mock_display.notification.call_args[0][0])
+        self.assertIn(authzrs[0].body.challenges[0].validation(self.mock_account.key),
+                      self.mock_display.notification.call_args[0][0])
 
     def test_perform_failure(self):
         authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)]


Code review comments:
diff_hunk: @@ -87,9 +87,34 @@ def handle_authorizations(self, orderr: messages.OrderResource,
 
                 # If debug is on, wait for user input before starting the verification process.
                 if config.debug_challenges:
+                    msg = []
+                    if config.verbose_count > 0:
+                        http01_achalls = {}
+                        dns01_achalls = {}
+                        for achall in achalls:
+                            if isinstance(achall.chall, challenges.HTTP01):
+                                http01_achalls[achall.chall.uri(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                            if isinstance(achall.chall, challenges.DNS01):
+                                dns01_achalls[achall.validation_domain_name(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                        if http01_achalls:
+                            msg.append("\nThe following URLs should be accessible from the world "
+                                       "wide web and return the value mentioned:\n")
+                            for uri, token in http01_achalls.items():
+                                msg.append(f"{uri}: {token}")
+                        if dns01_achalls:
+                            msg.append("\nThe following FQDNs should return a TXT resource "
path: certbot/certbot/_internal/auth_handler.py
text: @user1:
The leading `\n` here causes an excess newline when there are mixed HTTP/DNS challenges. It could be avoided by making it conditional on there being any `http01_achalls`.


@author:
This would only be an issue when Certbot actually would support mixed challenge types. Is that the case?

@user1:
    certbot certonly --manual -d example.com -d "*.example.com" --manual-auth-hook /bin/true --debug-challenges

is one way to produce a mixed challenge type scenario.

@author:
Ah, cool, I did not know that. I thought the challenge type function would prefer having all challenges of the same type when I looked at the code, but perhaps it gives priority to what preference the authenticator has.

Anyway, fixed the newline issue without requiring it to be conditional :slightly_smiling_face:

diff_hunk: @@ -191,15 +192,73 @@ def test_name3_http_01_3_common_acme_2(self):
 
     def test_debug_challenges(self):
         config = mock.Mock(debug_challenges=True)
-        authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)]
+        config.verbose_count = 0
path: certbot/tests/auth_handler_test.py
text: @user1:
```suggestion
```

diff_hunk: @@ -87,9 +87,34 @@ def handle_authorizations(self, orderr: messages.OrderResource,
 
                 # If debug is on, wait for user input before starting the verification process.
                 if config.debug_challenges:
+                    msg = []
+                    if config.verbose_count > 0:
+                        http01_achalls = {}
+                        dns01_achalls = {}
+                        for achall in achalls:
+                            if isinstance(achall.chall, challenges.HTTP01):
+                                http01_achalls[achall.chall.uri(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                            if isinstance(achall.chall, challenges.DNS01):
+                                dns01_achalls[achall.validation_domain_name(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                        if http01_achalls:
+                            msg.append("\nThe following URLs should be accessible from the world "
+                                       "wide web and return the value mentioned:\n")
+                            for uri, token in http01_achalls.items():
+                                msg.append(f"{uri}: {token}")
+                        if dns01_achalls:
+                            msg.append("\nThe following FQDNs should return a TXT resource "
+                                       "record with the value mentioned:\n")
+                            for fqdn, token in dns01_achalls.items():
+                                msg.append(f"{fqdn}: {token}")
path: certbot/certbot/_internal/auth_handler.py
text: @user1:
I think it's probably worth moving this block into its own function, which summarizes the challenges as a string.

diff_hunk: @@ -87,9 +87,34 @@ def handle_authorizations(self, orderr: messages.OrderResource,
 
                 # If debug is on, wait for user input before starting the verification process.
                 if config.debug_challenges:
+                    msg = []
+                    if config.verbose_count > 0:
+                        http01_achalls = {}
+                        dns01_achalls = {}
+                        for achall in achalls:
+                            if isinstance(achall.chall, challenges.HTTP01):
+                                http01_achalls[achall.chall.uri(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                            if isinstance(achall.chall, challenges.DNS01):
+                                dns01_achalls[achall.validation_domain_name(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                        if http01_achalls:
+                            msg.append("\nThe following URLs should be accessible from the world "
+                                       "wide web and return the value mentioned:\n")
path: certbot/certbot/_internal/auth_handler.py
text: @user1:
I appreciate the flavor here 😆 .

It's not a big deal, but do you think the message will get across to more people if we call it the "internet"?

@author:
Not sure if people wouldn't understand "world wide web", but it's probably good to err on the side of caution, thanks :slightly_smiling_face:

diff_hunk: @@ -87,9 +87,34 @@ def handle_authorizations(self, orderr: messages.OrderResource,
 
                 # If debug is on, wait for user input before starting the verification process.
                 if config.debug_challenges:
+                    msg = []
+                    if config.verbose_count > 0:
+                        http01_achalls = {}
+                        dns01_achalls = {}
+                        for achall in achalls:
+                            if isinstance(achall.chall, challenges.HTTP01):
+                                http01_achalls[achall.chall.uri(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                            if isinstance(achall.chall, challenges.DNS01):
+                                dns01_achalls[achall.validation_domain_name(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                        if http01_achalls:
+                            msg.append("\nThe following URLs should be accessible from the world "
+                                       "wide web and return the value mentioned:\n")
+                            for uri, token in http01_achalls.items():
+                                msg.append(f"{uri}: {token}")
path: certbot/certbot/_internal/auth_handler.py
text: @user1:
I think it's likely that people will accidentally copy-paste the trailing `:` in the URL without realizing. `\n` might be an alternative?

diff_hunk: @@ -191,15 +192,73 @@ def test_name3_http_01_3_common_acme_2(self):
 
     def test_debug_challenges(self):
         config = mock.Mock(debug_challenges=True)
-        authzrs = [gen_dom_authzr(domain="0", challs=acme_util.CHALLENGES)]
+        config.verbose_count = 0
+        authzrs = [gen_dom_authzr(domain="0", challs=[acme_util.HTTP01])]
+        mock_order = mock.MagicMock(authorizations=authzrs)
+
+        account_key_thumbprint = b"foobarbaz"
+        self.mock_account.key.thumbprint.return_value = account_key_thumbprint
+
+        self.mock_net.poll.side_effect = _gen_mock_on_poll()
+
+        self.handler.handle_authorizations(mock_order, config)
+
+        self.assertEqual(self.mock_net.answer_challenge.call_count, 1)
+        self.assertEqual(self.mock_display.notification.call_count, 1)
+        self.assertIn('Pass "-v" for more info',
+                      self.mock_display.notification.call_args[0][0])
+        self.assertNotIn(f"http://{authzrs[0].body.identifier.value}/.well-known/acme-challenge/" +
+                         b64encode(authzrs[0].body.challenges[0].chall.token).decode(),
+                         self.mock_display.notification.call_args[0][0])
+        self.assertNotIn(b64encode(account_key_thumbprint).decode(),
+                      self.mock_display.notification.call_args[0][0])
+
+    def test_debug_challenges_verbose_http01(self):
+        config = mock.Mock(debug_challenges=True)
+        config.verbose_count = 1
path: certbot/tests/auth_handler_test.py
text: @user1:
```suggestion
        config = mock.Mock(debug_challenges=True, verbose_count=1)
```

diff_hunk: @@ -87,9 +87,34 @@ def handle_authorizations(self, orderr: messages.OrderResource,
 
                 # If debug is on, wait for user input before starting the verification process.
                 if config.debug_challenges:
+                    msg = []
+                    if config.verbose_count > 0:
+                        http01_achalls = {}
+                        dns01_achalls = {}
+                        for achall in achalls:
+                            if isinstance(achall.chall, challenges.HTTP01):
+                                http01_achalls[achall.chall.uri(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                            if isinstance(achall.chall, challenges.DNS01):
+                                dns01_achalls[achall.validation_domain_name(achall.domain)] = (
+                                    achall.validation(achall.account_key) + "\n"
+                                )
+                        if http01_achalls:
+                            msg.append("\nThe following URLs should be accessible from the world "
+                                       "wide web and return the value mentioned:\n")
+                            for uri, token in http01_achalls.items():
+                                msg.append(f"{uri}: {token}")
+                        if dns01_achalls:
+                            msg.append("\nThe following FQDNs should return a TXT resource "
+                                       "record with the value mentioned:\n")
+                            for fqdn, token in dns01_achalls.items():
+                                msg.append(f"{fqdn}: {token}")
path: certbot/certbot/_internal/auth_handler.py
text: @user1:
If we change `:` for HTTP, the same change should be made here.

diff_hunk: @@ -191,15 +192,73 @@ def test_name3_http_01_3_common_acme_2(self):
 
     def test_debug_challenges(self):
         config = mock.Mock(debug_challenges=True)
path: certbot/tests/auth_handler_test.py
text: @user1:
```suggestion
        config = mock.Mock(debug_challenges=True, verbose_count=0)
```