From a02223a97fe0c710448fb6028e6c9b4548349ee9 Mon Sep 17 00:00:00 2001 From: alexzorin Date: Thu, 27 May 2021 08:16:12 +1000 Subject: [PATCH] cli: later printing of renewal and install retry advice (#8860) * later printing of renewal and install retry advice Move printing of advice for automated renewal, and retrying installation in case of failure, towards the end of `run` and `certonly`. Also adds some renewal advice for the --csr case (no autorenewal). * update renewal advice for preconfigured-renewal * rewrite in terms of "NEXT STEPS" for run/certonly * fix lint * re-add "Could not install certificate" * update --csr renewal advice * rewrite non-preconfigured-renewal renewal advice --- certbot/certbot/_internal/client.py | 14 +-- certbot/certbot/_internal/main.py | 134 +++++++++++++++++++++------- certbot/tests/client_test.py | 34 ++----- certbot/tests/main_test.py | 36 ++++++-- 4 files changed, 140 insertions(+), 78 deletions(-) diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py index d90c0254b..c9a240de3 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -516,11 +516,9 @@ class Client: return abs_cert_path, abs_chain_path, abs_fullchain_path - def deploy_certificate(self, cert_name, domains, privkey_path, - cert_path, chain_path, fullchain_path): + def deploy_certificate(self, 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) @@ -536,11 +534,7 @@ class Client: display_util.notify("Deploying certificate") - msg = f"Failed to install the certificate (installer: {self.config.installer})." - if cert_name: - msg += (" Try again after fixing errors by running:\n\n" - f" {cli.cli_constants.cli_command} install --cert-name {cert_name}\n") - + msg = "Could not install certificate" with error_handler.ErrorHandler(self._recovery_routine_with_msg, msg): for dom in domains: self.installer.deploy_cert( @@ -616,9 +610,7 @@ class Client: """ - msg = ("We were unable to set up enhancement %s for your server, " - "however, we successfully installed your certificate." - % (enhancement)) + msg = f"Could not set up {enhancement} enhancement" with error_handler.ErrorHandler(self._recovery_routine_with_msg, msg): for dom in domains: try: diff --git a/certbot/certbot/_internal/main.py b/certbot/certbot/_internal/main.py index d1f7019d6..496ce382c 100644 --- a/certbot/certbot/_internal/main.py +++ b/certbot/certbot/_internal/main.py @@ -469,6 +469,68 @@ def _find_domains_or_certname(config, installer, question=None): return domains, certname +def _report_next_steps(config: interfaces.IConfig, installer_err: Optional[errors.Error], + lineage: Optional[storage.RenewableCert], + new_or_renewed_cert: bool = True) -> None: + """Displays post-run/certonly advice to the user about renewal and installation. + + The output varies by runtime configuration and any errors encountered during installation. + + :param config: Configuration object + :type config: interfaces.IConfig + + :param installer_err: The installer/enhancement error encountered, if any. + :type error: Optional[errors.Error] + + :param lineage: The resulting certificate lineage from the issuance, if any. + :type lineage: Optional[storage.RenewableCert] + + :param bool new_or_renewed_cert: Whether the verb execution resulted in a certificate + being saved (created or renewed). + + """ + steps: List[str] = [] + + # If the installation or enhancement raised an error, show advice on trying again + if installer_err: + steps.append( + "The certificate was saved, but could not be installed (installer: " + f"{config.installer}). After fixing the error shown below, try installing it again " + f"by running:\n {cli.cli_command} install --cert-name " + f"{_cert_name_from_config_or_lineage(config, lineage)}" + ) + + # If a certificate was obtained or renewed, show applicable renewal advice + if new_or_renewed_cert: + if config.csr: + steps.append( + "Certificates created using --csr will not be renewed automatically by Certbot. " + "You will need to renew the certificate before it expires, by running the same " + "Certbot command again.") + elif not config.preconfigured_renewal: + steps.append( + "The certificate will need to be renewed before it expires. Certbot can " + "automatically renew the certificate in the background, but you may need " + "to take steps to enable that functionality. " + "See https://certbot.eff.org/docs/using.html#automated-renewals for " + "instructions.") + + if not steps: + return + + # TODO: refactor ANSI escapes during https://github.com/certbot/certbot/issues/8848 + (bold_on, bold_off) = [c if sys.stdout.isatty() and not config.quiet else '' \ + for c in (util.ANSI_SGR_BOLD, util.ANSI_SGR_RESET)] + + print(bold_on, '\n', 'NEXT STEPS:', bold_off, sep='') + for step in steps: + display_util.notify(f"- {step}") + + # If there was an installer error, segregate the error output with a trailing newline + if installer_err: + print() + + def _report_new_cert(config, cert_path, fullchain_path, key_path=None): # type: (interfaces.IConfig, Optional[str], Optional[str], Optional[str]) -> None """Reports the creation of a new certificate to the user. @@ -499,18 +561,13 @@ def _report_new_cert(config, cert_path, fullchain_path, key_path=None): ("\nSuccessfully received certificate.\n" "Certificate is saved at: {cert_path}\n{key_msg}" "This certificate expires on {expiry}.\n" - "These files will be updated when the certificate renews.\n{renew_msg}{nl}").format( + "These files will be updated when the certificate renews.{renewal_msg}{nl}").format( cert_path=fullchain_path, expiry=crypto_util.notAfter(cert_path).date(), key_msg="Key is saved at: {}\n".format(key_path) if key_path else "", - renew_msg="Certbot will automatically renew this certificate in the background." - if config.preconfigured_renewal else - (f'Run "{cli.cli_constants.cli_command} renew" to renew ' - "expiring certificates. " - "We recommend setting up a scheduled task for renewal; see " - "https://certbot.eff.org/docs/using.html#automated-renewals " - "for instructions."), - nl="\n" if config.verb == "run" else "" # visually split output if also deploying + renewal_msg="\nCertbot has set up a scheduled task to automatically renew this " + "certificate in the background." if config.preconfigured_renewal else "", + nl="\n" if config.verb == "run" else "" # Normalize spacing across verbs ) ) @@ -813,6 +870,21 @@ def update_account(config, unused_plugins): return None +def _cert_name_from_config_or_lineage(config: interfaces.IConfig, + lineage: Optional[storage.RenewableCert]) -> Optional[str]: + if lineage: + return lineage.lineagename + elif config.certname: + return config.certname + try: + cert_name = cert_manager.cert_path_to_lineage(config) + return cert_name + except errors.Error: + pass + + return None + + def _install_cert(config, le_client, domains, lineage=None): """Install a cert @@ -835,20 +907,8 @@ 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: Optional[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) + le_client.deploy_certificate(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) @@ -1216,15 +1276,27 @@ def run(config, plugins): if should_get_cert: _report_new_cert(config, cert_path, fullchain_path, key_path) - _install_cert(config, le_client, domains, new_lineage) + # The installer error, if any, is being stored as a value here, in order to first print + # relevant advice in a nice way, before re-raising the error for normal processing. + installer_err: Optional[errors.Error] = None + try: + _install_cert(config, le_client, domains, new_lineage) - if enhancements.are_requested(config) and new_lineage: - enhancements.enable(new_lineage, domains, installer, config) + if enhancements.are_requested(config) and new_lineage: + enhancements.enable(new_lineage, domains, installer, config) - if lineage is None or not should_get_cert: - display_ops.success_installation(domains) - else: - display_ops.success_renewal(domains) + if lineage is None or not should_get_cert: + display_ops.success_installation(domains) + else: + display_ops.success_renewal(domains) + except errors.Error as e: + installer_err = e + finally: + _report_next_steps(config, installer_err, new_lineage, + new_or_renewed_cert=should_get_cert) + # If the installer did fail, re-raise the error to bail out + if installer_err: + raise installer_err _suggest_donation_if_appropriate(config) eff.handle_subscription(config, le_client.account) @@ -1327,6 +1399,7 @@ def certonly(config, plugins): if config.csr: cert_path, chain_path, fullchain_path = _csr_get_and_save_cert(config, le_client) _csr_report_new_cert(config, cert_path, chain_path, fullchain_path) + _report_next_steps(config, None, None) _suggest_donation_if_appropriate(config) eff.handle_subscription(config, le_client.account) return @@ -1345,6 +1418,7 @@ def certonly(config, plugins): fullchain_path = lineage.fullchain_path if lineage else None key_path = lineage.key_path if lineage else None _report_new_cert(config, cert_path, fullchain_path, key_path) + _report_next_steps(config, None, lineage, new_or_renewed_cert=should_get_cert) _suggest_donation_if_appropriate(config) eff.handle_subscription(config, le_client.account) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index b42e6992e..8d9811fa3 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -521,13 +521,12 @@ class ClientTest(ClientTestCommon): @test_util.patch_get_utility() def test_deploy_certificate_success(self, mock_util): self.assertRaises(errors.Error, self.client.deploy_certificate, - "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + ["foo.bar"], "key", "cert", "chain", "fullchain") installer = mock.MagicMock() self.client.installer = installer - self.client.deploy_certificate( - "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + self.client.deploy_certificate(["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"), @@ -546,31 +545,10 @@ class ClientTest(ClientTestCommon): installer.deploy_cert.side_effect = errors.PluginError self.assertRaises(errors.PluginError, self.client.deploy_certificate, - "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + ["foo.bar"], "key", "cert", "chain", "fullchain") installer.recovery_routine.assert_called_once_with() mock_notify.assert_any_call('Deploying certificate') - mock_notify.assert_any_call( - 'Failed to install the certificate (installer: foobar). ' - 'Try again after fixing errors 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_certificate_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('Deploying certificate') - mock_notify.assert_any_call( - 'Failed to install the certificate (installer: foobar).' - ) @test_util.patch_get_utility() @@ -580,7 +558,7 @@ class ClientTest(ClientTestCommon): installer.save.side_effect = errors.PluginError self.assertRaises(errors.PluginError, self.client.deploy_certificate, - "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + ["foo.bar"], "key", "cert", "chain", "fullchain") installer.recovery_routine.assert_called_once_with() @mock.patch('certbot._internal.client.display_util.notify') @@ -591,7 +569,7 @@ class ClientTest(ClientTestCommon): self.client.installer = installer self.assertRaises(errors.PluginError, self.client.deploy_certificate, - "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + ["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.') @@ -607,7 +585,7 @@ class ClientTest(ClientTestCommon): self.client.installer = installer self.assertRaises(errors.PluginError, self.client.deploy_certificate, - "foo.bar", ["foo.bar"], "key", "cert", "chain", "fullchain") + ["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', diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 42e514fea..166b29dea 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -112,6 +112,7 @@ class RunTest(test_util.ConfigTestCase): mock.patch('certbot._internal.main._report_new_cert'), mock.patch('certbot._internal.main._find_cert'), mock.patch('certbot._internal.eff.handle_subscription'), + mock.patch('certbot._internal.main._report_next_steps') ] self.mock_auth = patches[0].start() @@ -122,6 +123,7 @@ class RunTest(test_util.ConfigTestCase): self.mock_report_cert = patches[5].start() self.mock_find_cert = patches[6].start() self.mock_subscription = patches[7].start() + self.mock_report_next_steps = patches[8].start() for patch in patches: self.addCleanup(patch.stop) @@ -139,6 +141,8 @@ class RunTest(test_util.ConfigTestCase): self.mock_find_cert.return_value = True, None self._call() self.mock_success_installation.assert_called_once_with([self.domain]) + self.mock_report_next_steps.assert_called_once_with(mock.ANY, None, mock.ANY, + new_or_renewed_cert=True) def test_reinstall_success(self): self.mock_auth.return_value = mock.Mock() @@ -161,6 +165,18 @@ class RunTest(test_util.ConfigTestCase): main.run, self.config, plugins) + @mock.patch('certbot._internal.main._install_cert') + def test_cert_success_install_error(self, mock_install_cert): + mock_install_cert.side_effect = errors.PluginError("Fake installation error") + self.mock_auth.return_value = mock.Mock() + self.mock_find_cert.return_value = True, None + self.assertRaises(errors.PluginError, self._call) + + # Next steps should contain both renewal advice and installation error + self.mock_report_next_steps.assert_called_once_with( + mock.ANY, mock_install_cert.side_effect, mock.ANY, new_or_renewed_cert=True) + # The final success message shouldn't be shown + self.mock_success_installation.assert_not_called() class CertonlyTest(unittest.TestCase): """Tests for certbot._internal.main.certonly.""" @@ -198,13 +214,14 @@ class CertonlyTest(unittest.TestCase): def _assert_no_pause(self, message, pause=True): # pylint: disable=unused-argument self.assertIs(pause, False) + @mock.patch('certbot._internal.main._report_next_steps') @mock.patch('certbot._internal.cert_manager.lineage_for_certname') @mock.patch('certbot._internal.cert_manager.domains_for_certname') @mock.patch('certbot._internal.renewal.renew_cert') @mock.patch('certbot._internal.main._handle_unexpected_key_type_migration') @mock.patch('certbot._internal.main._report_new_cert') def test_find_lineage_for_domains_and_certname(self, mock_report_cert, - mock_handle_type, mock_renew_cert, mock_domains, mock_lineage): + mock_handle_type, mock_renew_cert, mock_domains, mock_lineage, mock_report_next_steps): domains = ['example.com', 'test.org'] mock_domains.return_value = domains mock_lineage.names.return_value = domains @@ -216,6 +233,8 @@ class CertonlyTest(unittest.TestCase): self.assertEqual(mock_renew_cert.call_count, 1) self.assertEqual(mock_report_cert.call_count, 1) self.assertEqual(mock_handle_type.call_count, 1) + mock_report_next_steps.assert_called_once_with( + mock.ANY, None, mock.ANY, new_or_renewed_cert=True) # user confirms updating lineage with new domains self._call(('certonly --webroot -d example.com -d test.com ' @@ -231,12 +250,13 @@ class CertonlyTest(unittest.TestCase): self.assertRaises(errors.ConfigurationError, self._call, 'certonly --webroot -d example.com -d test.com --cert-name example.com'.split()) + @mock.patch('certbot._internal.main._report_next_steps') @mock.patch('certbot._internal.cert_manager.domains_for_certname') @mock.patch('certbot.display.ops.choose_names') @mock.patch('certbot._internal.cert_manager.lineage_for_certname') @mock.patch('certbot._internal.main._report_new_cert') def test_find_lineage_for_domains_new_certname(self, mock_report_cert, - mock_lineage, mock_choose_names, mock_domains_for_certname): + mock_lineage, mock_choose_names, mock_domains_for_certname, unused_mock_report_next_steps): mock_lineage.return_value = None # no lineage with this name but we specified domains so create a new cert @@ -1823,7 +1843,8 @@ class ReportNewCertTest(unittest.TestCase): 'Key is saved at: /path/to/privkey.pem\n' 'This certificate expires on 1970-01-01.\n' 'These files will be updated when the certificate renews.\n' - 'Certbot will automatically renew this certificate in the background.' + 'Certbot has set up a scheduled task to automatically renew this ' + 'certificate in the background.' ) def test_report_no_key(self): @@ -1836,7 +1857,8 @@ class ReportNewCertTest(unittest.TestCase): 'Certificate is saved at: /path/to/fullchain.pem\n' 'This certificate expires on 1970-01-01.\n' 'These files will be updated when the certificate renews.\n' - 'Certbot will automatically renew this certificate in the background.' + 'Certbot has set up a scheduled task to automatically renew this ' + 'certificate in the background.' ) def test_report_no_preconfigured_renewal(self): @@ -1849,13 +1871,9 @@ class ReportNewCertTest(unittest.TestCase): 'Certificate is saved at: /path/to/fullchain.pem\n' 'Key is saved at: /path/to/privkey.pem\n' 'This certificate expires on 1970-01-01.\n' - 'These files will be updated when the certificate renews.\n' - 'Run "certbot renew" to renew expiring certificates. We recommend setting up a ' - 'scheduled task for renewal; see https://certbot.eff.org/docs/using.html#automated' - '-renewals for instructions.' + 'These files will be updated when the certificate renews.' ) - def test_csr_report(self): self._call_csr(mock.Mock(dry_run=False), '/path/to/cert.pem', '/path/to/chain.pem', '/path/to/fullchain.pem')