diff --git a/certbot-apache/certbot_apache/_internal/configurator.py b/certbot-apache/certbot_apache/_internal/configurator.py index e97d9d81f..9f36d78ff 100644 --- a/certbot-apache/certbot_apache/_internal/configurator.py +++ b/certbot-apache/certbot_apache/_internal/configurator.py @@ -2505,6 +2505,11 @@ class ApacheConfigurator(common.Installer): version=".".join(str(i) for i in self.version)) ) + def auth_hint(self, failed_achalls): # pragma: no cover + return ("The Certificate Authority failed to verify the temporary Apache configuration " + "changes made by Certbot. Ensure that the listed domains point to this Apache " + "server and that it is accessible from the internet.") + ########################################################################### # Challenges Section ########################################################################### diff --git a/certbot-nginx/certbot_nginx/_internal/configurator.py b/certbot-nginx/certbot_nginx/_internal/configurator.py index 000d48e45..462531c55 100644 --- a/certbot-nginx/certbot_nginx/_internal/configurator.py +++ b/certbot-nginx/certbot_nginx/_internal/configurator.py @@ -1078,6 +1078,13 @@ class NginxConfigurator(common.Installer): version=".".join(str(i) for i in self.version)) ) + def auth_hint(self, failed_achalls): # pragma: no cover + return ( + "The Certificate Authority failed to verify the temporary nginx configuration changes " + "made by Certbot. Ensure the listed domains point to this nginx server and that it is " + "accessible from the internet." + ) + ################################################### # Wrapper functions for Reverter class (IInstaller) ################################################### diff --git a/certbot/certbot/_internal/auth_handler.py b/certbot/certbot/_internal/auth_handler.py index c2f323a36..4c285729e 100644 --- a/certbot/certbot/_internal/auth_handler.py +++ b/certbot/certbot/_internal/auth_handler.py @@ -15,6 +15,8 @@ from certbot import achallenges from certbot import errors from certbot import interfaces from certbot._internal import error_handler +from certbot.display import util as display_util +from certbot.plugins import common as plugin_common logger = logging.getLogger(__name__) @@ -173,7 +175,7 @@ class AuthHandler: # In case of failed authzrs, create a report to the user. if authzrs_failed_to_report: - _report_failed_authzrs(authzrs_failed_to_report, self.account.key) + self._report_failed_authzrs(authzrs_failed_to_report) if not best_effort: # Without best effort, having failed authzrs is critical and fail the process. raise errors.AuthorizationError('Some challenges have failed.') @@ -264,6 +266,29 @@ class AuthHandler: return achalls + def _report_failed_authzrs(self, failed_authzrs: List[messages.AuthorizationResource]) -> None: + """Notifies the user about failed authorizations.""" + problems: Dict[str, List[achallenges.AnnotatedChallenge]] = {} + failed_achalls = [challb_to_achall(challb, self.account.key, authzr.body.identifier.value) + for authzr in failed_authzrs for challb in authzr.body.challenges + if challb.error] + + for achall in failed_achalls: + problems.setdefault(achall.error.typ, []).append(achall) + + msg = [f"\nCertbot failed to authenticate some domains (authenticator: {self.auth.name})." + " The Certificate Authority reported these problems:"] + + for _, achalls in sorted(problems.items(), key=lambda item: item[0]): + msg.append(_generate_failed_chall_msg(achalls)) + + # auth_hint will only be called on authenticators that subclass + # plugin_common.Plugin. Refer to comment on that function. + if failed_achalls and isinstance(self.auth, plugin_common.Plugin): + msg.append(f"\nHint: {self.auth.auth_hint(failed_achalls)}\n") + + display_util.notify("".join(msg)) + def challb_to_achall(challb, account_key, domain): """Converts a ChallengeBody object to an AnnotatedChallenge. @@ -393,60 +418,13 @@ def _report_no_chall_path(challbs): raise errors.AuthorizationError(msg) -_ERROR_HELP_COMMON = ( - "To fix these errors, please make sure that your domain name was entered " - "correctly and the DNS A/AAAA record(s) for that domain contain(s) the " - "right IP address.") - - -_ERROR_HELP = { - "connection": - _ERROR_HELP_COMMON + " Additionally, please check that your computer " - "has a publicly routable IP address and that no firewalls are preventing " - "the server from communicating with the client. If you're using the " - "webroot plugin, you should also verify that you are serving files " - "from the webroot path you provided.", - "dnssec": - _ERROR_HELP_COMMON + " Additionally, if you have DNSSEC enabled for " - "your domain, please ensure that the signature is valid.", - "malformed": - "To fix these errors, please make sure that you did not provide any " - "invalid information to the client, and try running Certbot " - "again.", - "serverInternal": - "Unfortunately, an error on the ACME server prevented you from completing " - "authorization. Please try again later.", - "tls": - _ERROR_HELP_COMMON + " Additionally, please check that you have an " - "up-to-date TLS configuration that allows the server to communicate " - "with the Certbot client.", - "unauthorized": _ERROR_HELP_COMMON, - "unknownHost": _ERROR_HELP_COMMON, -} - - -def _report_failed_authzrs(failed_authzrs, account_key): - """Notifies the user about failed authorizations.""" - problems: Dict[str, List[achallenges.AnnotatedChallenge]] = {} - failed_achalls = [challb_to_achall(challb, account_key, authzr.body.identifier.value) - for authzr in failed_authzrs for challb in authzr.body.challenges - if challb.error] - - for achall in failed_achalls: - problems.setdefault(achall.error.typ, []).append(achall) - - reporter = zope.component.getUtility(interfaces.IReporter) - for achalls in problems.values(): - reporter.add_message(_generate_failed_chall_msg(achalls), reporter.MEDIUM_PRIORITY) - - def _generate_failed_chall_msg(failed_achalls): + # type: (List[achallenges.AnnotatedChallenge]) -> str """Creates a user friendly error message about failed challenges. :param list failed_achalls: A list of failed :class:`certbot.achallenges.AnnotatedChallenge` with the same error type. - :returns: A formatted error message for the client. :rtype: str @@ -455,14 +433,10 @@ def _generate_failed_chall_msg(failed_achalls): typ = error.typ if messages.is_acme_error(error): typ = error.code - msg = ["The following errors were reported by the server:"] + msg = [] for achall in failed_achalls: - msg.append("\n\nDomain: %s\nType: %s\nDetail: %s" % ( + msg.append("\n Domain: %s\n Type: %s\n Detail: %s\n" % ( achall.domain, typ, achall.error.detail)) - if typ in _ERROR_HELP: - msg.append("\n\n") - msg.append(_ERROR_HELP[typ]) - return "".join(msg) diff --git a/certbot/certbot/_internal/plugins/manual.py b/certbot/certbot/_internal/plugins/manual.py index dec73a1ed..7b97ce089 100644 --- a/certbot/certbot/_internal/plugins/manual.py +++ b/certbot/certbot/_internal/plugins/manual.py @@ -10,6 +10,7 @@ from certbot import errors from certbot import interfaces from certbot import reverter from certbot import util +from certbot._internal.cli import cli_constants from certbot._internal import hooks from certbot.compat import misc from certbot.compat import os @@ -125,6 +126,42 @@ permitted by DNS standards.) 'validation challenges either through shell scripts provided by ' 'the user or by performing the setup manually.') + def auth_hint(self, failed_achalls): + has_chall = lambda cls: any(isinstance(achall.chall, cls) for achall in failed_achalls) + + has_dns = has_chall(challenges.DNS01) + resource_names = { + challenges.DNS01: 'DNS TXT records', + challenges.HTTP01: 'challenge files', + challenges.TLSALPN01: 'TLS-ALPN certificates' + } + resources = ' and '.join(sorted([v for k, v in resource_names.items() if has_chall(k)])) + + if self.conf('auth-hook'): + return ( + 'The Certificate Authority failed to verify the {resources} created by the ' + '--manual-auth-hook. Ensure that this hook is functioning correctly{dns_hint}. ' + 'Refer to "{certbot} --help manual" and the Certbot User Guide.' + .format( + certbot=cli_constants.cli_command, + resources=resources, + dns_hint=( + ' and that it waits a sufficient duration of time for DNS propagation' + ) if has_dns else '' + ) + ) + else: + return ( + 'The Certificate Authority failed to verify the manually created {resources}. ' + 'Ensure that you created these in the correct location{dns_hint}.' + .format( + resources=resources, + dns_hint=( + ', or try waiting longer for DNS propagation on the next attempt' + ) if has_dns else '' + ) + ) + def get_chall_pref(self, domain): # pylint: disable=unused-argument,missing-function-docstring return [challenges.HTTP01, challenges.DNS01] diff --git a/certbot/certbot/_internal/plugins/webroot.py b/certbot/certbot/_internal/plugins/webroot.py index 3473d3c8e..80717bf93 100644 --- a/certbot/certbot/_internal/plugins/webroot.py +++ b/certbot/certbot/_internal/plugins/webroot.py @@ -61,6 +61,12 @@ to serve all files under specified web root ({0}).""" "file, it needs to be on a single line, like: webroot-map = " '{"example.com":"/var/www"}.') + def auth_hint(self, failed_achalls): # pragma: no cover + return ("The Certificate Authority failed to download the temporary challenge files " + "created by Certbot. Ensure that the listed domains serve their content from " + "the provided --webroot-path/-w and that files created there can be downloaded " + "from the internet.") + def get_chall_pref(self, domain): # pragma: no cover # pylint: disable=unused-argument,missing-function-docstring return [challenges.HTTP01] diff --git a/certbot/certbot/plugins/common.py b/certbot/certbot/plugins/common.py index 4c33acbab..3181d7b50 100644 --- a/certbot/certbot/plugins/common.py +++ b/certbot/certbot/plugins/common.py @@ -97,6 +97,33 @@ class Plugin: """Find a configuration value for variable ``var``.""" return getattr(self.config, self.dest(var)) + def auth_hint(self, failed_achalls): + # type: (List[achallenges.AnnotatedChallenge]) -> str + """Human-readable string to help the user troubleshoot the authenticator. + + Shown to the user if one or more of the attempted challenges were not a success. + + Should describe, in simple language, what the authenticator tried to do, what went + wrong and what the user should try as their "next steps". + + TODO: auth_hint belongs in IAuthenticator but can't be added until the next major + version of Certbot. For now, it lives in .Plugin and auth_handler will only call it + on authenticators that subclass .Plugin. For now, inherit from `.Plugin` to implement + and/or override the method. + + :param list failed_achalls: List of one or more failed challenges + (:class:`achallenges.AnnotatedChallenge` subclasses). + + :rtype str: + """ + # This is a fallback hint. Authenticators should implement their own auth_hint that + # addresses the specific mechanics of that authenticator. + challs = " and ".join(sorted({achall.typ for achall in failed_achalls})) + return ("The Certificate Authority couldn't exterally verify that the {name} plugin " + "completed the required {challs} challenges. Ensure the plugin is configured " + "correctly and that the changes it makes are accessible from the internet." + .format(name=self.name, challs=challs)) + class Installer(Plugin): """An installer base class with reverter and ssl_dhparam methods defined. diff --git a/certbot/certbot/plugins/dns_common.py b/certbot/certbot/plugins/dns_common.py index 4459ac0fa..359434778 100644 --- a/certbot/certbot/plugins/dns_common.py +++ b/certbot/certbot/plugins/dns_common.py @@ -37,6 +37,15 @@ class DNSAuthenticator(common.Plugin): help='The number of seconds to wait for DNS to propagate before asking the ACME server ' 'to verify the DNS record.') + def auth_hint(self, failed_achalls): + delay = self.conf('propagation-seconds') + return ( + 'The Certificate Authority failed to verify the DNS TXT records created by --{name}. ' + 'Ensure the above domains are hosted by this DNS provider, or try increasing ' + '--{name}-propagation-seconds (currently {secs} second{suffix}).' + .format(name=self.name, secs=delay, suffix='s' if delay != 1 else '') + ) + def get_chall_pref(self, unused_domain): # pylint: disable=missing-function-docstring return [challenges.DNS01] diff --git a/certbot/tests/auth_handler_test.py b/certbot/tests/auth_handler_test.py index 1f798c2d8..751c445fe 100644 --- a/certbot/tests/auth_handler_test.py +++ b/certbot/tests/auth_handler_test.py @@ -17,6 +17,7 @@ from certbot import achallenges from certbot import errors from certbot import interfaces from certbot import util +from certbot.plugins import common as plugin_common from certbot.tests import acme_util from certbot.tests import util as test_util @@ -327,7 +328,8 @@ class HandleAuthorizationsTest(unittest.TestCase): mock_order = mock.MagicMock(authorizations=authzrs) - with mock.patch('certbot._internal.auth_handler._report_failed_authzrs') as mock_report: + with mock.patch('certbot._internal.auth_handler.AuthHandler._report_failed_authzrs') \ + as mock_report: valid_authzr = self.handler.handle_authorizations(mock_order, True) # Because best_effort=True, we did not blow up. Instead ... @@ -474,10 +476,18 @@ class GenChallengePathTest(unittest.TestCase): class ReportFailedAuthzrsTest(unittest.TestCase): - """Tests for certbot._internal.auth_handler._report_failed_authzrs.""" + """Tests for certbot._internal.auth_handler.AuthHandler._report_failed_authzrs.""" # pylint: disable=protected-access + def setUp(self): + from certbot._internal.auth_handler import AuthHandler + + self.mock_auth = mock.MagicMock(spec=plugin_common.Plugin, name="buzz") + self.mock_auth.name = "buzz" + self.mock_auth.auth_hint.return_value = "the buzz hint" + self.handler = AuthHandler(self.mock_auth, mock.MagicMock(), mock.MagicMock(), []) + kwargs = { "chall": acme_util.HTTP01, "uri": "uri", @@ -504,21 +514,57 @@ class ReportFailedAuthzrsTest(unittest.TestCase): self.authzr2.body.identifier.value = 'foo.bar' self.authzr2.body.challenges = [http_01_diff] - @test_util.patch_get_utility() - def test_same_error_and_domain(self, mock_zope): - from certbot._internal import auth_handler + @mock.patch('certbot._internal.auth_handler.display_util.notify') + def test_same_error_and_domain(self, mock_notify): + self.handler._report_failed_authzrs([self.authzr1]) + mock_notify.assert_called_with( + '\n' + 'Certbot failed to authenticate some domains (authenticator: buzz). ' + 'The Certificate Authority reported these problems:\n' + ' Domain: example.com\n' + ' Type: tls\n' + ' Detail: detail\n' + '\n' + ' Domain: example.com\n' + ' Type: tls\n' + ' Detail: detail\n' + '\nHint: the buzz hint\n' + ) - auth_handler._report_failed_authzrs([self.authzr1], 'key') - call_list = mock_zope().add_message.call_args_list - self.assertEqual(len(call_list), 1) - self.assertIn("Domain: example.com\nType: tls\nDetail: detail", call_list[0][0][0]) + @mock.patch('certbot._internal.auth_handler.display_util.notify') + def test_different_errors_and_domains(self, mock_notify): + self.mock_auth.name = "quux" + self.mock_auth.auth_hint.return_value = "quuuuuux" + self.handler._report_failed_authzrs([self.authzr1, self.authzr2]) + mock_notify.assert_called_with( + '\n' + 'Certbot failed to authenticate some domains (authenticator: quux). ' + 'The Certificate Authority reported these problems:\n' + ' Domain: foo.bar\n' + ' Type: dnssec\n' + ' Detail: detail\n' + '\n' + ' Domain: example.com\n' + ' Type: tls\n' + ' Detail: detail\n' + '\n' + ' Domain: example.com\n' + ' Type: tls\n' + ' Detail: detail\n' + '\nHint: quuuuuux\n' + ) - @test_util.patch_get_utility() - def test_different_errors_and_domains(self, mock_zope): - from certbot._internal import auth_handler + @mock.patch('certbot._internal.auth_handler.display_util.notify') + def test_non_subclassed_authenticator(self, mock_notify): + """If authenticator not derived from common.Plugin, we shouldn't call .auth_hint""" + from certbot._internal.auth_handler import AuthHandler - auth_handler._report_failed_authzrs([self.authzr1, self.authzr2], 'key') - self.assertEqual(mock_zope().add_message.call_count, 2) + self.mock_auth = mock.MagicMock(name="quuz") + self.mock_auth.name = "quuz" + self.mock_auth.auth_hint.side_effect = Exception + self.handler = AuthHandler(self.mock_auth, mock.MagicMock(), mock.MagicMock(), []) + self.handler._report_failed_authzrs([self.authzr1]) + self.assertEqual(mock_notify.call_count, 1) def gen_auth_resp(chall_list): diff --git a/certbot/tests/plugins/common_test.py b/certbot/tests/plugins/common_test.py index 6eb5dbfce..376af507b 100644 --- a/certbot/tests/plugins/common_test.py +++ b/certbot/tests/plugins/common_test.py @@ -83,6 +83,13 @@ class PluginTest(unittest.TestCase): parser.add_argument.assert_called_once_with( "--mock-foo-bar", dest="different_to_foo_bar", x=1, y=None) + def test_fallback_auth_hint(self): + self.assertIn("the mock plugin completed the required dns-01 challenges", + self.plugin.auth_hint([acme_util.DNS01_A, acme_util.DNS01_A])) + self.assertIn("the mock plugin completed the required dns-01 and http-01 challenges", + self.plugin.auth_hint([acme_util.DNS01_A, acme_util.HTTP01_A, + acme_util.DNS01_A])) + class InstallerTest(test_util.ConfigTestCase): """Tests for certbot.plugins.common.Installer.""" diff --git a/certbot/tests/plugins/dns_common_test.py b/certbot/tests/plugins/dns_common_test.py index 12fac00d0..13e81aeb9 100644 --- a/certbot/tests/plugins/dns_common_test.py +++ b/certbot/tests/plugins/dns_common_test.py @@ -119,6 +119,12 @@ class DNSAuthenticatorTest(test_util.TempDirTestCase, dns_test_common.BaseAuthen credentials = self.auth._configure_credentials("credentials", "", {"test": ""}) self.assertEqual(credentials.conf("test"), "value") + def test_auth_hint(self): + self.assertIn( + 'try increasing --fake-propagation-seconds (currently 0 seconds).', + self.auth.auth_hint([mock.MagicMock()]) + ) + class CredentialsConfigurationTest(test_util.TempDirTestCase): class _MockLoggingHandler(logging.Handler): diff --git a/certbot/tests/plugins/manual_test.py b/certbot/tests/plugins/manual_test.py index c97e08fa6..198b0aa19 100644 --- a/certbot/tests/plugins/manual_test.py +++ b/certbot/tests/plugins/manual_test.py @@ -120,6 +120,35 @@ class AuthenticatorTest(test_util.TempDirTestCase): else: self.assertNotIn('CERTBOT_TOKEN', os.environ) + def test_auth_hint_hook(self): + self.config.manual_auth_hook = '/bin/true' + self.assertEqual( + self.auth.auth_hint([acme_util.DNS01_A, acme_util.HTTP01_A]), + 'The Certificate Authority failed to verify the DNS TXT records and challenge ' + 'files created by the --manual-auth-hook. Ensure that this hook is functioning ' + 'correctly and that it waits a sufficient duration of time for DNS propagation. ' + 'Refer to "certbot --help manual" and the Certbot User Guide.' + ) + self.assertEqual( + self.auth.auth_hint([acme_util.HTTP01_A]), + 'The Certificate Authority failed to verify the challenge files created by the ' + '--manual-auth-hook. Ensure that this hook is functioning correctly. Refer to ' + '"certbot --help manual" and the Certbot User Guide.' + ) + + def test_auth_hint_no_hook(self): + self.assertEqual( + self.auth.auth_hint([acme_util.DNS01_A, acme_util.HTTP01_A]), + 'The Certificate Authority failed to verify the manually created DNS TXT records ' + 'and challenge files. Ensure that you created these in the correct location, or ' + 'try waiting longer for DNS propagation on the next attempt.' + ) + self.assertEqual( + self.auth.auth_hint([acme_util.HTTP01_A, acme_util.HTTP01_A, acme_util.HTTP01_A]), + 'The Certificate Authority failed to verify the manually created challenge files. ' + 'Ensure that you created these in the correct location.' + ) + if __name__ == '__main__': unittest.main() # pragma: no cover