mirror of
https://github.com/certbot/certbot.git
synced 2026-06-09 00:32:12 -04:00
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
This commit is contained in:
alexzorin
GitHub
parent
2e31b1ca41
commit
a02223a97f
4 changed files with 140 additions and 78 deletions
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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',
|
||||
|
|
|
|||
|
|
@ -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')
|
||||
|
|
|
|||
Loading…
Reference in a new issue