From 8b7ce54c43677a7a8272929a438fd4124e43f7c1 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Wed, 3 Feb 2021 21:54:29 +1100 Subject: [PATCH 1/6] cli: redesign output when cert installation failed - Display a message when certificate installation begins. - Don't use IReporter, just log errors immediately if restart/rollback fails. - Prompt the user with a command to retry the installation process once they have fixed any underlying problems. --- certbot/certbot/_internal/client.py | 28 +++++++------ certbot/certbot/_internal/main.py | 8 +++- certbot/tests/client_test.py | 61 ++++++++++++++++++++--------- 3 files changed, 66 insertions(+), 31 deletions(-) diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index 5dc62580e..407d0090e 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -8,7 +8,6 @@ from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives.asymmetric.rsa import generate_private_key # type: ignore import josepy as jose import OpenSSL -import zope.component from acme import client as acme_client from acme import crypto_util as acme_crypto_util @@ -19,7 +18,6 @@ from acme.magic_typing import Optional import certbot from certbot import crypto_util from certbot import errors -from certbot import interfaces from certbot import util from certbot._internal import account from certbot._internal import auth_handler @@ -31,6 +29,7 @@ from certbot._internal import storage from certbot._internal.plugins import selection as plugin_selection from certbot.compat import os from certbot.display import ops as display_ops +from certbot.display import util as display_util logger = logging.getLogger(__name__) @@ -516,10 +515,11 @@ class Client(object): return abs_cert_path, abs_chain_path, abs_fullchain_path - def deploy_certificate(self, domains, privkey_path, + def deploy_certificate(self, cert_name, domains, privkey_path, cert_path, chain_path, fullchain_path): """Install certificate + :param str cert_name: name of the certificate lineage (optional) :param list domains: list of domains to install the certificate :param str privkey_path: path to certificate private key :param str cert_path: certificate file path (optional) @@ -533,7 +533,13 @@ class Client(object): chain_path = None if chain_path is None else os.path.abspath(chain_path) - msg = ("Unable to install the certificate") + display_util.notify("\nDeploying certificate") + + msg = f"Failed to install the certificate (using the {self.config.installer} plugin)." + if cert_name: + msg += (" Try again by running:\n\n" + f" {cli.cli_constants.cli_command} install --cert-name {cert_name}\n") + with error_handler.ErrorHandler(self._recovery_routine_with_msg, msg): for dom in domains: self.installer.deploy_cert( @@ -624,7 +630,7 @@ class Client(object): logger.warning("Enhancement %s was already set.", enhancement) except errors.PluginError: - logger.warning("Unable to set enhancement %s for %s", + logger.error("Unable to set enhancement %s for %s", enhancement, dom) raise @@ -637,8 +643,7 @@ class Client(object): """ self.installer.recovery_routine() - reporter = zope.component.getUtility(interfaces.IReporter) - reporter.add_message(success_msg, reporter.HIGH_PRIORITY) + display_util.notify(success_msg) def _rollback_and_restart(self, success_msg): """Rollback the most recent checkpoint and restart the webserver @@ -647,19 +652,18 @@ class Client(object): """ logger.critical("Rolling back to previous server configuration...") - reporter = zope.component.getUtility(interfaces.IReporter) try: self.installer.rollback_checkpoints() self.installer.restart() except: - reporter.add_message( + logger.error( "An error occurred and we failed to restore your config and " "restart your server. Please post to " "https://community.letsencrypt.org/c/help " - "with details about your configuration and this error you received.", - reporter.HIGH_PRIORITY) + "with details about your configuration and this error you received." + ) raise - reporter.add_message(success_msg, reporter.HIGH_PRIORITY) + display_util.notify(success_msg) def validate_key_csr(privkey, csr=None): diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index ab777e651..6fb972d92 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -801,7 +801,13 @@ def _install_cert(config, le_client, domains, lineage=None): path_provider = lineage if lineage else config assert path_provider.cert_path is not None - le_client.deploy_certificate(domains, path_provider.key_path, + cert_name = None + if isinstance(path_provider, storage.RenewableCert): + cert_name = path_provider.lineagename + elif path_provider.certname: + cert_name = path_provider.certname + + le_client.deploy_certificate(cert_name, domains, path_provider.key_path, path_provider.cert_path, path_provider.chain_path, path_provider.fullchain_path) le_client.enhance_config(domains, path_provider.chain_path) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index f058cb658..2d1ebb7f3 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -516,15 +516,16 @@ class ClientTest(ClientTestCommon): shutil.rmtree(tmp_path) - def test_deploy_certificate_success(self): + @test_util.patch_get_utility() + def test_deploy_certificate_success(self, mock_util): self.assertRaises(errors.Error, self.client.deploy_certificate, - ["foo.bar"], "key", "cert", "chain", "fullchain") + "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") installer = mock.MagicMock() self.client.installer = installer self.client.deploy_certificate( - ["foo.bar"], "key", "cert", "chain", "fullchain") + "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") installer.deploy_cert.assert_called_once_with( cert_path=os.path.abspath("cert"), chain_path=os.path.abspath("chain"), @@ -534,46 +535,63 @@ class ClientTest(ClientTestCommon): self.assertEqual(installer.save.call_count, 2) installer.restart.assert_called_once_with() - def test_deploy_certificate_failure(self): + @mock.patch('certbot._internal.client.display_util.notify') + @test_util.patch_get_utility() + def test_deploy_certificate_failure(self, mock_util, mock_notify): installer = mock.MagicMock() self.client.installer = installer + self.config.installer = "foobar" installer.deploy_cert.side_effect = errors.PluginError self.assertRaises(errors.PluginError, self.client.deploy_certificate, - ["foo.bar"], "key", "cert", "chain", "fullchain") + "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") installer.recovery_routine.assert_called_once_with() - def test_deploy_certificate_save_failure(self): + mock_notify.assert_any_call('\nDeploying certificate') + mock_notify.assert_any_call( + 'Failed to install the certificate (using the foobar plugin). ' + 'Try again by running:\n\n certbot install --cert-name foo.bar\n' + ) + + @test_util.patch_get_utility() + def test_deploy_certificate_save_failure(self, mock_util): installer = mock.MagicMock() self.client.installer = installer installer.save.side_effect = errors.PluginError self.assertRaises(errors.PluginError, self.client.deploy_certificate, - ["foo.bar"], "key", "cert", "chain", "fullchain") + "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") installer.recovery_routine.assert_called_once_with() + @mock.patch('certbot._internal.client.display_util.notify') @test_util.patch_get_utility() - def test_deploy_certificate_restart_failure(self, mock_get_utility): + def test_deploy_certificate_restart_failure(self, mock_get_utility, mock_notify): installer = mock.MagicMock() installer.restart.side_effect = [errors.PluginError, None] self.client.installer = installer self.assertRaises(errors.PluginError, self.client.deploy_certificate, - ["foo.bar"], "key", "cert", "chain", "fullchain") - self.assertEqual(mock_get_utility().add_message.call_count, 1) + "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + mock_notify.assert_called_with( + 'We were unable to install your certificate, however, we successfully restored ' + 'your server to its prior configuration.') installer.rollback_checkpoints.assert_called_once_with() self.assertEqual(installer.restart.call_count, 2) + @mock.patch('certbot._internal.client.logger') @test_util.patch_get_utility() - def test_deploy_certificate_restart_failure2(self, mock_get_utility): + def test_deploy_certificate_restart_failure2(self, mock_get_utility, mock_logger): installer = mock.MagicMock() installer.restart.side_effect = errors.PluginError installer.rollback_checkpoints.side_effect = errors.ReverterError self.client.installer = installer self.assertRaises(errors.PluginError, self.client.deploy_certificate, - ["foo.bar"], "key", "cert", "chain", "fullchain") - self.assertEqual(mock_get_utility().add_message.call_count, 1) + "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + self.assertEqual(mock_logger.error.call_count, 1) + self.assertIn( + 'An error occurred and we failed to restore your config', + mock_logger.error.call_args[0][0]) installer.rollback_checkpoints.assert_called_once_with() self.assertEqual(installer.restart.call_count, 1) @@ -660,7 +678,7 @@ class EnhanceConfigTest(ClientTestCommon): def test_enhance_failure(self): self.client.installer = mock.MagicMock() self.client.installer.enhance.side_effect = errors.PluginError - self._test_error() + self._test_error(enhance_error=True) self.client.installer.recovery_routine.assert_called_once_with() def test_save_failure(self): @@ -683,15 +701,22 @@ class EnhanceConfigTest(ClientTestCommon): self._test_error_with_rollback() def _test_error_with_rollback(self): - self._test_error() + self._test_error(restart_error=True) self.assertTrue(self.client.installer.restart.called) - def _test_error(self): + def _test_error(self, enhance_error=False, restart_error=False): self.config.redirect = True - with test_util.patch_get_utility() as mock_gu: + with mock.patch('certbot._internal.client.logger') as mock_logger, \ + test_util.patch_get_utility() as mock_gu: self.assertRaises( errors.PluginError, self._test_with_all_supported) - self.assertEqual(mock_gu().add_message.call_count, 1) + + if enhance_error: + self.assertEqual(mock_logger.error.call_count, 1) + self.assertIn('Unable to set enhancement', mock_logger.error.call_args_list[0][0][0]) + if restart_error: + mock_logger.critical.assert_called_with( + 'Rolling back to previous server configuration...') def _test_with_all_supported(self): if self.client.installer is None: From 7a1b713dc4958a4223e276b2d10f4efd3ffcadee Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Thu, 11 Feb 2021 21:56:51 +1100 Subject: [PATCH 2/6] derve cert name from cert_path, if possible --- certbot/certbot/_internal/cert_manager.py | 9 +++++++-- certbot/certbot/_internal/main.py | 8 +++++++- certbot/tests/cert_manager_test.py | 4 ++++ certbot/tests/client_test.py | 18 ++++++++++++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/certbot/certbot/_internal/cert_manager.py b/certbot/certbot/_internal/cert_manager.py index dfbe4b538..5135f4fee 100644 --- a/certbot/certbot/_internal/cert_manager.py +++ b/certbot/certbot/_internal/cert_manager.py @@ -221,9 +221,14 @@ def cert_path_to_lineage(cli_config): :raises `errors.Error`: If the specified cert path can't be matched to a lineage name. :raises `errors.OverlappingMatchFound`: If the matched lineage's archive is shared. """ + # cli_config.cert_path might be a (path, contents) tuple or just a path. + cert_path = cli_config.cert_path[0] \ + if isinstance(cli_config.cert_path, tuple) \ + else cli_config.cert_path + acceptable_matches = _acceptable_matches() match = match_and_check_overlaps(cli_config, acceptable_matches, - lambda x: cli_config.cert_path[0], lambda x: x.lineagename) + lambda x: cert_path, lambda x: x.lineagename) return match[0] @@ -254,7 +259,7 @@ def match_and_check_overlaps(cli_config, acceptable_matches, match_func, rv_func matched = _search_lineages(cli_config, find_matches, [], acceptable_matches) if not matched: - raise errors.Error("No match found for cert-path {0}!".format(cli_config.cert_path[0])) + raise errors.Error("No matching certificates found") elif len(matched) > 1: raise errors.OverlappingMatchFound() return matched diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index f3f98deb5..34fe7260e 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -800,11 +800,17 @@ def _install_cert(config, le_client, domains, lineage=None): path_provider = lineage if lineage else config assert path_provider.cert_path is not None - cert_name = None + cert_name: str = None if isinstance(path_provider, storage.RenewableCert): cert_name = path_provider.lineagename elif path_provider.certname: cert_name = path_provider.certname + else: + # Check if the cert path happens to be part of an existing lineage + try: + cert_name = cert_manager.cert_path_to_lineage(config) + except errors.Error: + pass le_client.deploy_certificate(cert_name, domains, path_provider.key_path, path_provider.cert_path, path_provider.chain_path, path_provider.fullchain_path) diff --git a/certbot/tests/cert_manager_test.py b/certbot/tests/cert_manager_test.py index b26c1f624..5ed2f3383 100644 --- a/certbot/tests/cert_manager_test.py +++ b/certbot/tests/cert_manager_test.py @@ -575,6 +575,10 @@ class CertPathToLineageTest(storage_test.BaseRenewableCertTest): self._archive_files(x, 'fullchain')] self.assertEqual('example.org', self._call(self.config)) + def test_only_path(self): + self.config.cert_path = self.fullchain + self.assertEqual('example.org', self._call(self.config)) + class MatchAndCheckOverlaps(storage_test.BaseRenewableCertTest): """Tests for certbot._internal.cert_manager.match_and_check_overlaps w/o overlapping diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 2d1ebb7f3..825281598 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -553,6 +553,24 @@ class ClientTest(ClientTestCommon): 'Try again by running:\n\n certbot install --cert-name foo.bar\n' ) + @mock.patch('certbot._internal.client.display_util.notify') + @test_util.patch_get_utility() + def test_deploy_certificae_failure_no_certname(self, mock_util, mock_notify): + installer = mock.MagicMock() + self.client.installer = installer + self.config.installer = "foobar" + + installer.deploy_cert.side_effect = errors.PluginError + self.assertRaises(errors.PluginError, self.client.deploy_certificate, + None, ["foo.bar"], "key", "cert", "chain", "fullchain") + installer.recovery_routine.assert_called_once_with() + + mock_notify.assert_any_call('\nDeploying certificate') + mock_notify.assert_any_call( + 'Failed to install the certificate (using the foobar plugin).' + ) + + @test_util.patch_get_utility() def test_deploy_certificate_save_failure(self, mock_util): installer = mock.MagicMock() From 49911afaa62ade1fca4c4ec407f817720cb354e3 Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Mon, 15 Feb 2021 17:13:03 +1100 Subject: [PATCH 3/6] fix type annotation --- certbot/certbot/_internal/main.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index 34fe7260e..9652ea71e 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -800,7 +800,7 @@ def _install_cert(config, le_client, domains, lineage=None): path_provider = lineage if lineage else config assert path_provider.cert_path is not None - cert_name: str = None + cert_name: Optional[str] = None if isinstance(path_provider, storage.RenewableCert): cert_name = path_provider.lineagename elif path_provider.certname: From fac77ea9f41ba6285c859ea3f7c88b9bf74b26da Mon Sep 17 00:00:00 2001 From: alexzorin Date: Mon, 22 Feb 2021 11:01:50 +1100 Subject: [PATCH 4/6] fix test name Co-authored-by: ohemorange --- certbot/tests/client_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 825281598..97380e351 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -555,7 +555,7 @@ class ClientTest(ClientTestCommon): @mock.patch('certbot._internal.client.display_util.notify') @test_util.patch_get_utility() - def test_deploy_certificae_failure_no_certname(self, mock_util, mock_notify): + def test_deploy_certificate_failure_no_certname(self, mock_util, mock_notify): installer = mock.MagicMock() self.client.installer = installer self.config.installer = "foobar" From d4a626bb8a11ff80fff4735c7613415f602a5a6e Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Thu, 4 Mar 2021 21:06:26 +1100 Subject: [PATCH 5/6] s/using the {} plugin/installer: {}/ --- certbot/certbot/_internal/client.py | 2 +- certbot/tests/client_test.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index cbcf69783..ba800e4cc 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -535,7 +535,7 @@ class Client: display_util.notify("\nDeploying certificate") - msg = f"Failed to install the certificate (using the {self.config.installer} plugin)." + msg = f"Failed to install the certificate (installer: {self.config.installer})." if cert_name: msg += (" Try again by running:\n\n" f" {cli.cli_constants.cli_command} install --cert-name {cert_name}\n") diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 97380e351..51a11c208 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -549,7 +549,7 @@ class ClientTest(ClientTestCommon): mock_notify.assert_any_call('\nDeploying certificate') mock_notify.assert_any_call( - 'Failed to install the certificate (using the foobar plugin). ' + 'Failed to install the certificate (installer: foobar). ' 'Try again by running:\n\n certbot install --cert-name foo.bar\n' ) @@ -567,7 +567,7 @@ class ClientTest(ClientTestCommon): mock_notify.assert_any_call('\nDeploying certificate') mock_notify.assert_any_call( - 'Failed to install the certificate (using the foobar plugin).' + 'Failed to install the certificate (installer: foobar).' ) From 64e3b31577726d1a29d3fd32a650620212767a9e Mon Sep 17 00:00:00 2001 From: Alex Zorin Date: Thu, 6 May 2021 17:34:32 +1000 Subject: [PATCH 6/6] get rid of leading newline on "Deploying [...]" --- certbot/certbot/_internal/client.py | 2 +- certbot/tests/client_test.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index ba800e4cc..1863e1b61 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -533,7 +533,7 @@ class Client: chain_path = None if chain_path is None else os.path.abspath(chain_path) - display_util.notify("\nDeploying certificate") + display_util.notify("Deploying certificate") msg = f"Failed to install the certificate (installer: {self.config.installer})." if cert_name: diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 51a11c208..4eb8ca9aa 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -547,7 +547,7 @@ class ClientTest(ClientTestCommon): "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") installer.recovery_routine.assert_called_once_with() - mock_notify.assert_any_call('\nDeploying certificate') + mock_notify.assert_any_call('Deploying certificate') mock_notify.assert_any_call( 'Failed to install the certificate (installer: foobar). ' 'Try again by running:\n\n certbot install --cert-name foo.bar\n' @@ -565,7 +565,7 @@ class ClientTest(ClientTestCommon): None, ["foo.bar"], "key", "cert", "chain", "fullchain") installer.recovery_routine.assert_called_once_with() - mock_notify.assert_any_call('\nDeploying certificate') + mock_notify.assert_any_call('Deploying certificate') mock_notify.assert_any_call( 'Failed to install the certificate (installer: foobar).' )