diff --git a/letsencrypt-apache/letsencrypt_apache/augeas_configurator.py b/letsencrypt-apache/letsencrypt_apache/augeas_configurator.py index b0b15d649..9e0948f12 100644 --- a/letsencrypt-apache/letsencrypt_apache/augeas_configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/augeas_configurator.py @@ -73,7 +73,8 @@ class AugeasConfigurator(common.Plugin): This function first checks for save errors, if none are found, all configuration changes made will be saved. According to the - function parameters. + function parameters. If an exception is raised, a new checkpoint + was not created. :param str title: The title of the save. If a title is given, the configuration will be saved as a new checkpoint and put in a @@ -82,8 +83,9 @@ class AugeasConfigurator(common.Plugin): :param bool temporary: Indicates whether the changes made will be quickly reversed in the future (ie. challenges) - :raises .errors.PluginError: If there was an error in Augeas, in an - attempt to save the configuration, or an error creating a checkpoint + :raises .errors.PluginError: If there was an error in Augeas, in + an attempt to save the configuration, or an error creating a + checkpoint """ save_state = self.aug.get("/augeas/save") @@ -122,16 +124,16 @@ class AugeasConfigurator(common.Plugin): except errors.ReverterError as err: raise errors.PluginError(str(err)) + self.aug.set("/augeas/save", save_state) + self.save_notes = "" + self.aug.save() + if title and not temporary: try: self.reverter.finalize_checkpoint(title) except errors.ReverterError as err: raise errors.PluginError(str(err)) - self.aug.set("/augeas/save", save_state) - self.save_notes = "" - self.aug.save() - def _log_save_errors(self, ex_errs): """Log errors due to bad Augeas save. diff --git a/letsencrypt/client.py b/letsencrypt/client.py index 8a0ad6af4..0f0ecdc6d 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -5,6 +5,7 @@ import os from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives.asymmetric import rsa import OpenSSL +import zope.component from acme import client as acme_client from acme import jose @@ -18,6 +19,7 @@ from letsencrypt import continuity_auth from letsencrypt import crypto_util from letsencrypt import errors from letsencrypt import error_handler +from letsencrypt import interfaces from letsencrypt import le_util from letsencrypt import reverter from letsencrypt import storage @@ -330,26 +332,13 @@ class Client(object): self.installer.save("Deployed Let's Encrypt Certificate") - # sites may have been enabled / final cleanup - with error_handler.ErrorHandler(self._rollback_and_restart): + msg = ("We were unable to install your certificate, " + "however, we successfully restored your " + "server to its prior configuration.") + with error_handler.ErrorHandler(self._rollback_and_restart, msg): + # sites may have been enabled / final cleanup self.installer.restart() - def _rollback_and_restart(self): - """Rollback the most recent checkpoint and restart the webserver""" - logger.critical("Rolling back to previous server configuration...") - try: - self.installer.rollback_checkpoints() - self.installer.restart() - except: - # TODO: suggest letshelp-letsencypt here - logger.critical("Failure to rollback config " - "changes and restart your server") - logger.critical("Please submit a bug report to " - "https://github.com/letsencrypt/letsencrypt") - raise - logger.critical("Rollback successful; your server has " - "been restarted with your old configuration") - def enhance_config(self, domains, redirect=None): """Enhance the configuration. @@ -386,7 +375,9 @@ class Client(object): :type vhost: :class:`letsencrypt.interfaces.IInstaller` """ - with error_handler.ErrorHandler(self.installer.recovery_routine): + msg = ("We were unable to set up a redirect for your server, " + "however, we successfully installed your certificate.") + with error_handler.ErrorHandler(self._recovery_routine_with_msg, msg): for dom in domains: try: self.installer.enhance(dom, "redirect") @@ -395,8 +386,41 @@ class Client(object): raise self.installer.save("Add Redirects") + + with error_handler.ErrorHandler(self._rollback_and_restart, msg): self.installer.restart() + def _recovery_routine_with_msg(self, success_msg): + """Calls the installer's recovery routine and prints success_msg + + :param str success_msg: message to show on successful recovery + + """ + self.installer.recovery_routine() + reporter = zope.component.getUtility(interfaces.IReporter) + reporter.add_message(success_msg, reporter.HIGH_PRIORITY) + + def _rollback_and_restart(self, success_msg): + """Rollback the most recent checkpoint and restart the webserver + + :param str success_msg: message to show on successful rollback + + """ + logger.critical("Rolling back to previous server configuration...") + reporter = zope.component.getUtility(interfaces.IReporter) + try: + self.installer.rollback_checkpoints() + self.installer.restart() + except: + # TODO: suggest letshelp-letsencypt here + reporter.add_message( + "An error occured and we failed to restore your config and " + "restart your server. Please submit a bug report to " + "https://github.com/letsencrypt/letsencrypt", + reporter.HIGH_PRIORITY) + raise + reporter.add_message(success_msg, reporter.HIGH_PRIORITY) + def validate_key_csr(privkey, csr=None): """Validate Key and CSR files. diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index 8b0eb7c8b..431e677a1 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -1,4 +1,5 @@ """Registers functions to be called if an exception or signal occurs.""" +import functools import logging import os import signal @@ -40,11 +41,11 @@ class ErrorHandler(object): to be called again by the next signal handler. """ - def __init__(self, func=None): + def __init__(self, func=None, *args, **kwargs): self.funcs = [] self.prev_handlers = {} if func is not None: - self.register(func) + self.register(func, *args, **kwargs) def __enter__(self): self.set_signal_handlers() @@ -57,9 +58,13 @@ class ErrorHandler(object): self.call_registered() self.reset_signal_handlers() - def register(self, func): - """Registers func to be called if an error occurs.""" - self.funcs.append(func) + def register(self, func, *args, **kwargs): + """Sets func to be called with *args and **kwargs during cleanup + + :param function func: function to be called in case of an error + + """ + self.funcs.append(functools.partial(func, *args, **kwargs)) def call_registered(self): """Calls all registered functions""" diff --git a/letsencrypt/interfaces.py b/letsencrypt/interfaces.py index 987fdc25e..c8a725fde 100644 --- a/letsencrypt/interfaces.py +++ b/letsencrypt/interfaces.py @@ -298,7 +298,8 @@ class IInstaller(IPlugin): Both title and temporary are needed because a save may be intended to be permanent, but the save is not ready to be a full - checkpoint + checkpoint. If an exception is raised, it is assumed a new + checkpoint was not created. :param str title: The title of the save. If a title is given, the configuration will be saved as a new checkpoint and put in a diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py index 4a61194f3..f8da90f36 100644 --- a/letsencrypt/tests/client_test.py +++ b/letsencrypt/tests/client_test.py @@ -148,7 +148,7 @@ class ClientTest(unittest.TestCase): shutil.rmtree(tmp_path) - def test_deploy_certificate(self): + def test_deploy_certificate_success(self): self.assertRaises(errors.Error, self.client.deploy_certificate, ["foo.bar"], "key", "cert", "chain", "fullchain") @@ -166,17 +166,38 @@ class ClientTest(unittest.TestCase): self.assertEqual(installer.save.call_count, 2) installer.restart.assert_called_once_with() - def test_deploy_certificate_restart_failure_with_recovery(self): + def test_deploy_certificate_failure(self): + installer = mock.MagicMock() + self.client.installer = installer + + installer.deploy_cert.side_effect = errors.PluginError + self.assertRaises(errors.PluginError, self.client.deploy_certificate, + ["foo.bar"], "key", "cert", "chain", "fullchain") + installer.recovery_routine.assert_called_once_with() + + def test_deploy_certificate_save_failure(self): + 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") + installer.recovery_routine.assert_called_once_with() + + @mock.patch("letsencrypt.client.zope.component.getUtility") + def test_deploy_certificate_restart_failure(self, mock_get_utility): 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) installer.rollback_checkpoints.assert_called_once_with() self.assertEqual(installer.restart.call_count, 2) - def test_deploy_certificate_restart_failure_without_recovery(self): + @mock.patch("letsencrypt.client.zope.component.getUtility") + def test_deploy_certificate_restart_failure2(self, mock_get_utility): installer = mock.MagicMock() installer.restart.side_effect = errors.PluginError installer.rollback_checkpoints.side_effect = errors.ReverterError @@ -184,6 +205,7 @@ class ClientTest(unittest.TestCase): self.assertRaises(errors.PluginError, self.client.deploy_certificate, ["foo.bar"], "key", "cert", "chain", "fullchain") + self.assertEqual(mock_get_utility().add_message.call_count, 1) installer.rollback_checkpoints.assert_called_once_with() self.assertEqual(installer.restart.call_count, 1) @@ -201,10 +223,68 @@ class ClientTest(unittest.TestCase): self.assertEqual(installer.save.call_count, 1) installer.restart.assert_called_once_with() + def test_enhance_config_no_installer(self): + self.assertRaises(errors.Error, + self.client.enhance_config, ["foo.bar"]) + + @mock.patch("letsencrypt.client.zope.component.getUtility") + @mock.patch("letsencrypt.client.enhancements") + def test_enhance_config_enhance_failure(self, mock_enhancements, + mock_get_utility): + mock_enhancements.ask.return_value = True + installer = mock.MagicMock() + self.client.installer = installer installer.enhance.side_effect = errors.PluginError + self.assertRaises(errors.PluginError, self.client.enhance_config, ["foo.bar"], True) installer.recovery_routine.assert_called_once_with() + self.assertEqual(mock_get_utility().add_message.call_count, 1) + + @mock.patch("letsencrypt.client.zope.component.getUtility") + @mock.patch("letsencrypt.client.enhancements") + def test_enhance_config_save_failure(self, mock_enhancements, + mock_get_utility): + mock_enhancements.ask.return_value = True + installer = mock.MagicMock() + self.client.installer = installer + installer.save.side_effect = errors.PluginError + + self.assertRaises(errors.PluginError, + self.client.enhance_config, ["foo.bar"], True) + installer.recovery_routine.assert_called_once_with() + self.assertEqual(mock_get_utility().add_message.call_count, 1) + + @mock.patch("letsencrypt.client.zope.component.getUtility") + @mock.patch("letsencrypt.client.enhancements") + def test_enhance_config_restart_failure(self, mock_enhancements, + mock_get_utility): + mock_enhancements.ask.return_value = True + installer = mock.MagicMock() + self.client.installer = installer + installer.restart.side_effect = [errors.PluginError, None] + + self.assertRaises(errors.PluginError, + self.client.enhance_config, ["foo.bar"], True) + self.assertEqual(mock_get_utility().add_message.call_count, 1) + installer.rollback_checkpoints.assert_called_once_with() + self.assertEqual(installer.restart.call_count, 2) + + @mock.patch("letsencrypt.client.zope.component.getUtility") + @mock.patch("letsencrypt.client.enhancements") + def test_enhance_config_restart_failure2(self, mock_enhancements, + mock_get_utility): + mock_enhancements.ask.return_value = True + installer = mock.MagicMock() + self.client.installer = installer + installer.restart.side_effect = errors.PluginError + installer.rollback_checkpoints.side_effect = errors.ReverterError + + self.assertRaises(errors.PluginError, + self.client.enhance_config, ["foo.bar"], True) + self.assertEqual(mock_get_utility().add_message.call_count, 1) + installer.rollback_checkpoints.assert_called_once_with() + self.assertEqual(installer.restart.call_count, 1) class RollbackTest(unittest.TestCase): diff --git a/letsencrypt/tests/error_handler_test.py b/letsencrypt/tests/error_handler_test.py index c92f12435..7fbdcffd8 100644 --- a/letsencrypt/tests/error_handler_test.py +++ b/letsencrypt/tests/error_handler_test.py @@ -13,7 +13,11 @@ class ErrorHandlerTest(unittest.TestCase): from letsencrypt import error_handler self.init_func = mock.MagicMock() - self.handler = error_handler.ErrorHandler(self.init_func) + self.init_args = set((42,)) + self.init_kwargs = {'foo': 'bar'} + self.handler = error_handler.ErrorHandler(self.init_func, + *self.init_args, + **self.init_kwargs) # pylint: disable=protected-access self.signals = error_handler._SIGNALS @@ -23,7 +27,8 @@ class ErrorHandlerTest(unittest.TestCase): raise ValueError except ValueError: pass - self.init_func.assert_called_once_with() + self.init_func.assert_called_once_with(*self.init_args, + **self.init_kwargs) @mock.patch('letsencrypt.error_handler.os') @mock.patch('letsencrypt.error_handler.signal') @@ -37,7 +42,8 @@ class ErrorHandlerTest(unittest.TestCase): signum = self.signals[0] signal_handler(signum, None) - self.init_func.assert_called_once_with() + self.init_func.assert_called_once_with(*self.init_args, + **self.init_kwargs) mock_os.kill.assert_called_once_with(mock_os.getpid(), signum) self.handler.reset_signal_handlers() @@ -48,7 +54,8 @@ class ErrorHandlerTest(unittest.TestCase): bad_func = mock.MagicMock(side_effect=[ValueError]) self.handler.register(bad_func) self.handler.call_registered() - self.init_func.assert_called_once_with() + self.init_func.assert_called_once_with(*self.init_args, + **self.init_kwargs) bad_func.assert_called_once_with() def test_sysexit_ignored(self):