From 1bb62eed4dd97067ae35e922db3669701dc0a3fe Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 10 Sep 2015 22:35:44 -0400 Subject: [PATCH 1/5] Started crash recovery mechanism --- letsencrypt/error_handler.py | 46 ++++++++++++++++++++++++++++++++++++ letsencrypt/interfaces.py | 11 +++++++++ 2 files changed, 57 insertions(+) create mode 100644 letsencrypt/error_handler.py diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py new file mode 100644 index 000000000..884c73927 --- /dev/null +++ b/letsencrypt/error_handler.py @@ -0,0 +1,46 @@ +"""Registers and calls cleanup functions in case of an error.""" +import os +import signal + + +_SIGNALS = [signal.SIGTERM] if os.name == "nt" else + [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, + signal.SIGXCPU, signal.SIGXFSZ, signal.SIGPWR,] + + +class ErrorHandler(): + """Registers and calls cleanup functions in case of an error.""" + def __init__(self, func=None): + self.funcs = [] + if func: + self.funcs.append(func) + + def __enter__(self): + self.set_signal_handlers() + + def __exit__(self, exec_type, exec_value, traceback): + if exec_value is not None: + self.cleanup() + self.reset_signal_handlers() + + def register(self, func): + """Registers func to be called if an error occurs.""" + self.funcs.append(func) + + def cleanup(self): + """Calls all registered functions.""" + while self.funcs: + self.funcs.pop()() + + def set_signal_handlers(self): + for signal_type in _SIGNALS: + signal.signal(signal_type, self._signal_handler) + + def reset_signal_handlers(self): + for signal_type in _SIGNALS: + signal.signal(signal_type, signal.SIG_DFL) + + def _signal_handler(self, signum, frame): + self.cleanup() + signal.signal(signal_type, signal.SIG_DFL) + os.kill(os.getpid(), signum) diff --git a/letsencrypt/interfaces.py b/letsencrypt/interfaces.py index f330e28ce..653b5685b 100644 --- a/letsencrypt/interfaces.py +++ b/letsencrypt/interfaces.py @@ -322,6 +322,17 @@ class IInstaller(IPlugin): """ + def recovery_routine(self): + """Revert configuration to most recent finalized checkpoint. + + Remove all changes (temporary and permanent) that have not been + finalized. This is useful to protect against crashes and other + execution interruptions. + + :raises .errors.PluginError: If unable to recover the configuration + + """ + def view_config_changes(): """Display all of the LE config changes. From aa216a96d4ec2ede40dda8dfea81330669dca150 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 22 Sep 2015 18:24:22 -0700 Subject: [PATCH 2/5] Finished error_handler --- letsencrypt/error_handler.py | 51 +++++++++++++++---------- letsencrypt/tests/error_handler_test.py | 25 ++++++++++++ 2 files changed, 56 insertions(+), 20 deletions(-) create mode 100644 letsencrypt/tests/error_handler_test.py diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index 884c73927..b82f49b5a 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -3,44 +3,55 @@ import os import signal -_SIGNALS = [signal.SIGTERM] if os.name == "nt" else - [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, - signal.SIGXCPU, signal.SIGXFSZ, signal.SIGPWR,] +_SIGNALS = ([signal.SIGTERM] if os.name == "nt" else + [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, + signal.SIGXCPU, signal.SIGXFSZ, signal.SIGPWR]) -class ErrorHandler(): +class ErrorHandler(object): """Registers and calls cleanup functions in case of an error.""" def __init__(self, func=None): - self.funcs = [] - if func: - self.funcs.append(func) + self.funcs = [func] if func else [] + self.prev_handlers = {} def __enter__(self): self.set_signal_handlers() def __exit__(self, exec_type, exec_value, traceback): if exec_value is not None: - self.cleanup() + 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 cleanup(self): - """Calls all registered functions.""" - while self.funcs: - self.funcs.pop()() + + def call_registered(self): + """Calls all functions in the order they were registered.""" + for func in self.funcs: + func() def set_signal_handlers(self): - for signal_type in _SIGNALS: - signal.signal(signal_type, self._signal_handler) + """Sets signal handlers for signals in _SIGNALS.""" + for signum in _SIGNALS: + prev_handler = signal.getsignal(signum) + # If prev_handler is None, the handler was set outside of Python + if prev_handler is not None: + self.prev_handlers[signum] = prev_handler + signal.signal(signum, self._signal_handler) def reset_signal_handlers(self): - for signal_type in _SIGNALS: - signal.signal(signal_type, signal.SIG_DFL) + """Resets signal handlers for signals in _SIGNALS.""" + for signum in self.prev_handlers: + signal.signal(signum, self.prev_handlers[signum]) + self.prev_handlers.clear() - def _signal_handler(self, signum, frame): - self.cleanup() - signal.signal(signal_type, signal.SIG_DFL) + def _signal_handler(self, signum, _): + """Calls registered functions and the previous signal handler. + + :param int signum: number of current signal + + """ + self.call_registered() + signal.signal(signum, self.prev_handlers[signum]) os.kill(os.getpid(), signum) diff --git a/letsencrypt/tests/error_handler_test.py b/letsencrypt/tests/error_handler_test.py new file mode 100644 index 000000000..6c6d02ec3 --- /dev/null +++ b/letsencrypt/tests/error_handler_test.py @@ -0,0 +1,25 @@ +"""Tests for letsencrypt.error_handler.""" +import unittest + +import mock + + +class ErrorHandlerTest(unittest.TestCase): + """Tests for letsencrypt.error_handler.""" + + def setUp(self): + from letsencrypt import error_handler + self.init_func = mock.MagicMock() + self.error_handler = error_handler.ErrorHandler(self.init_func) + + def test_context_manager(self): + try: + with self.error_handler: + raise ValueError + except ValueError: + pass + self.init_func.assert_called_once_with() + + +if __name__ == "__main__": + unittest.main() # pragma: no cover From 2b9f72fc29c2e3bf4b223f37f4e503037b82d548 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 23 Sep 2015 15:02:20 -0700 Subject: [PATCH 3/5] Finished basic crash recovery --- letsencrypt/auth_handler.py | 10 +++----- letsencrypt/client.py | 33 ++++++++++++++----------- letsencrypt/error_handler.py | 4 ++- letsencrypt/interfaces.py | 2 +- letsencrypt/tests/client_test.py | 33 +++++++++++++++++++++++++ letsencrypt/tests/error_handler_test.py | 27 +++++++++++++++++--- 6 files changed, 82 insertions(+), 27 deletions(-) diff --git a/letsencrypt/auth_handler.py b/letsencrypt/auth_handler.py index 6498a5c19..a285825dc 100644 --- a/letsencrypt/auth_handler.py +++ b/letsencrypt/auth_handler.py @@ -11,6 +11,7 @@ from acme import messages from letsencrypt import achallenges from letsencrypt import constants from letsencrypt import errors +from letsencrypt import error_handler from letsencrypt import interfaces @@ -106,17 +107,12 @@ class AuthHandler(object): """Get Responses for challenges from authenticators.""" cont_resp = [] dv_resp = [] - try: + logger.info("Attempting to set up challenges.") + with error_handler.ErrorHandler(self._cleanup_challenges): if self.cont_c: cont_resp = self.cont_auth.perform(self.cont_c) if self.dv_c: dv_resp = self.dv_auth.perform(self.dv_c) - # This will catch both specific types of errors. - except errors.AuthorizationError: - logger.critical("Failure in setting up challenges.") - logger.info("Attempting to clean up outstanding challenges...") - self._cleanup_challenges() - raise assert len(cont_resp) == len(self.cont_c) assert len(dv_resp) == len(self.dv_c) diff --git a/letsencrypt/client.py b/letsencrypt/client.py index 60eaea5a1..3f1f4900b 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -18,6 +18,7 @@ from letsencrypt import constants 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 @@ -364,16 +365,17 @@ class Client(object): chain_path = None if chain_path is None else os.path.abspath(chain_path) - for dom in domains: - # TODO: Provide a fullchain reference for installers like - # nginx that want it - self.installer.deploy_cert( - dom, os.path.abspath(cert_path), - os.path.abspath(privkey_path), chain_path) + with error_handler.ErrorHandler(self.installer.recovery_routine): + for dom in domains: + # TODO: Provide a fullchain reference for installers like + # nginx that want it + self.installer.deploy_cert( + dom, os.path.abspath(cert_path), + os.path.abspath(privkey_path), chain_path) - self.installer.save("Deployed Let's Encrypt Certificate") - # sites may have been enabled / final cleanup - self.installer.restart() + self.installer.save("Deployed Let's Encrypt Certificate") + # sites may have been enabled / final cleanup + self.installer.restart() def enhance_config(self, domains, redirect=None): """Enhance the configuration. @@ -399,6 +401,8 @@ class Client(object): if redirect is None: redirect = enhancements.ask("redirect") + # When support for more enhancements are added, the call to the + # plugin's `enhance` function should be wrapped by an ErrorHandler if redirect: self.redirect_to_ssl(domains) @@ -409,14 +413,13 @@ class Client(object): :type vhost: :class:`letsencrypt.interfaces.IInstaller` """ - for dom in domains: - try: + with error_handler.ErrorHandler(self.installer.recovery_routine): + for dom in domains: + logger.info("Attempting to perform redirect for %s", dom) self.installer.enhance(dom, "redirect") - except errors.PluginError: - logger.warn("Unable to perform redirect for %s", dom) - self.installer.save("Add Redirects") - self.installer.restart() + self.installer.save("Add Redirects") + self.installer.restart() def validate_key_csr(privkey, csr=None): diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index b82f49b5a..3fc948b54 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -11,8 +11,10 @@ _SIGNALS = ([signal.SIGTERM] if os.name == "nt" else class ErrorHandler(object): """Registers and calls cleanup functions in case of an error.""" def __init__(self, func=None): - self.funcs = [func] if func else [] + self.funcs = [] self.prev_handlers = {} + if func: + self.register(func) def __enter__(self): self.set_signal_handlers() diff --git a/letsencrypt/interfaces.py b/letsencrypt/interfaces.py index af145ab0a..a0d2eb97f 100644 --- a/letsencrypt/interfaces.py +++ b/letsencrypt/interfaces.py @@ -321,7 +321,7 @@ class IInstaller(IPlugin): """ - def recovery_routine(self): + def recovery_routine(): """Revert configuration to most recent finalized checkpoint. Remove all changes (temporary and permanent) that have not been diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py index 93fdf2cd3..0131d3c93 100644 --- a/letsencrypt/tests/client_test.py +++ b/letsencrypt/tests/client_test.py @@ -178,6 +178,39 @@ class ClientTest(unittest.TestCase): shutil.rmtree(tmp_path) + def test_deploy_certificate(self): + self.assertRaises(errors.Error, self.client.deploy_certificate, + ["foo.bar"], "key", "cert", "chain") + + installer = mock.MagicMock() + self.client.installer = installer + + self.client.deploy_certificate(["foo.bar"], "key", "cert", "chain") + installer.deploy_cert.assert_called_once_with( + "foo.bar", os.path.abspath("cert"), + os.path.abspath("key"), os.path.abspath("chain")) + self.assertTrue(installer.save.call_count == 1) + installer.restart.assert_called_once_with() + + @mock.patch("letsencrypt.client.enhancements") + def test_enhance_config(self, mock_enhancements): + self.assertRaises(errors.Error, + self.client.enhance_config, ["foo.bar"]) + + mock_enhancements.ask.return_value = True + installer = mock.MagicMock() + self.client.installer = installer + + self.client.enhance_config(["foo.bar"]) + installer.enhance.assert_called_once_with("foo.bar", "redirect") + self.assertTrue(installer.save.call_count == 1) + installer.restart.assert_called_once_with() + + installer.enhance.side_effect = errors.PluginError + self.assertRaises(errors.PluginError, + self.client.enhance_config, ["foo.bar"], True) + installer.recovery_routine.assert_called_once_with() + class RollbackTest(unittest.TestCase): """Tests for letsencrypt.client.rollback.""" diff --git a/letsencrypt/tests/error_handler_test.py b/letsencrypt/tests/error_handler_test.py index 6c6d02ec3..6927b32a0 100644 --- a/letsencrypt/tests/error_handler_test.py +++ b/letsencrypt/tests/error_handler_test.py @@ -1,25 +1,46 @@ """Tests for letsencrypt.error_handler.""" +import signal import unittest import mock +from letsencrypt import error_handler + class ErrorHandlerTest(unittest.TestCase): """Tests for letsencrypt.error_handler.""" def setUp(self): - from letsencrypt import error_handler self.init_func = mock.MagicMock() - self.error_handler = error_handler.ErrorHandler(self.init_func) + self.handler = error_handler.ErrorHandler(self.init_func) def test_context_manager(self): try: - with self.error_handler: + with self.handler: raise ValueError except ValueError: pass self.init_func.assert_called_once_with() + @mock.patch('letsencrypt.error_handler.os') + @mock.patch('letsencrypt.error_handler.signal') + def test_signal_handler(self, mock_signal, mock_os): + # pylint: disable=protected-access + mock_signal.getsignal.return_value = signal.SIG_DFL + self.handler.set_signal_handlers() + signal_handler = self.handler._signal_handler + for signum in error_handler._SIGNALS: + mock_signal.signal.assert_any_call(signum, signal_handler) + + signum = error_handler._SIGNALS[0] + signal_handler(signum, None) + self.init_func.assert_called_once_with() + mock_os.kill.assert_called_once_with(mock_os.getpid(), signum) + + self.handler.reset_signal_handlers() + for signum in error_handler._SIGNALS: + mock_signal.signal.assert_any_call(signum, signal.SIG_DFL) + if __name__ == "__main__": unittest.main() # pragma: no cover From 31e9519ef5af39550cd1d333d6c1ecd608f24221 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 23 Sep 2015 15:11:10 -0700 Subject: [PATCH 4/5] Updated null installer interface --- letsencrypt/plugins/null.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/letsencrypt/plugins/null.py b/letsencrypt/plugins/null.py index bc9565e5a..efe041cac 100644 --- a/letsencrypt/plugins/null.py +++ b/letsencrypt/plugins/null.py @@ -47,6 +47,9 @@ class Installer(common.Plugin): def rollback_checkpoints(self, rollback=1): pass # pragma: no cover + def recovery_routine(self): + pass # pragma: no cover + def view_config_changes(self): pass # pragma: no cover From fd0c51e48afef3fb618d5027d4420a921c00f9a3 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Thu, 24 Sep 2015 16:23:40 -0700 Subject: [PATCH 5/5] Incorporated Kuba's feedback and better defined corner cases --- letsencrypt/auth_handler.py | 14 ++++--- letsencrypt/client.py | 7 +++- letsencrypt/error_handler.py | 55 +++++++++++++++++++++---- letsencrypt/tests/client_test.py | 4 +- letsencrypt/tests/error_handler_test.py | 19 ++++++--- 5 files changed, 77 insertions(+), 22 deletions(-) diff --git a/letsencrypt/auth_handler.py b/letsencrypt/auth_handler.py index a285825dc..68aed510a 100644 --- a/letsencrypt/auth_handler.py +++ b/letsencrypt/auth_handler.py @@ -107,12 +107,16 @@ class AuthHandler(object): """Get Responses for challenges from authenticators.""" cont_resp = [] dv_resp = [] - logger.info("Attempting to set up challenges.") with error_handler.ErrorHandler(self._cleanup_challenges): - if self.cont_c: - cont_resp = self.cont_auth.perform(self.cont_c) - if self.dv_c: - dv_resp = self.dv_auth.perform(self.dv_c) + try: + if self.cont_c: + cont_resp = self.cont_auth.perform(self.cont_c) + if self.dv_c: + dv_resp = self.dv_auth.perform(self.dv_c) + except errors.AuthorizationError: + logger.critical("Failure in setting up challenges.") + logger.info("Attempting to clean up outstanding challenges...") + raise assert len(cont_resp) == len(self.cont_c) assert len(dv_resp) == len(self.dv_c) diff --git a/letsencrypt/client.py b/letsencrypt/client.py index 3f1f4900b..56d9b1fda 100644 --- a/letsencrypt/client.py +++ b/letsencrypt/client.py @@ -415,8 +415,11 @@ class Client(object): """ with error_handler.ErrorHandler(self.installer.recovery_routine): for dom in domains: - logger.info("Attempting to perform redirect for %s", dom) - self.installer.enhance(dom, "redirect") + try: + self.installer.enhance(dom, "redirect") + except errors.PluginError: + logger.warn("Unable to perform redirect for %s", dom) + raise self.installer.save("Add Redirects") self.installer.restart() diff --git a/letsencrypt/error_handler.py b/letsencrypt/error_handler.py index 3fc948b54..fedb66c0e 100644 --- a/letsencrypt/error_handler.py +++ b/letsencrypt/error_handler.py @@ -1,26 +1,58 @@ -"""Registers and calls cleanup functions in case of an error.""" +"""Registers functions to be called if an exception or signal occurs.""" +import logging import os import signal +import traceback +logger = logging.getLogger(__name__) + + +# _SIGNALS stores the signals that will be handled by the ErrorHandler. These +# signals were chosen as their default handler terminates the process and could +# potentially occur from inside Python. Signals such as SIGILL were not +# included as they could be a sign of something devious and we should terminate +# immediately. _SIGNALS = ([signal.SIGTERM] if os.name == "nt" else [signal.SIGTERM, signal.SIGHUP, signal.SIGQUIT, signal.SIGXCPU, signal.SIGXFSZ, signal.SIGPWR]) class ErrorHandler(object): - """Registers and calls cleanup functions in case of an error.""" + """Registers functions to be called if an exception or signal occurs. + + This class allows you to register functions that will be called when + an exception or signal is encountered. The class works best as a + context manager. For example: + + with ErrorHandler(cleanup_func): + do_something() + + If an exception is raised out of do_something, cleanup_func will be + called. The exception is not caught by the ErrorHandler. Similarly, + if a signal is encountered, cleanup_func is called followed by the + previously registered signal handler. + + Every registered function is attempted to be run to completion + exactly once. If a registered function raises an exception, it is + logged and the next function is called. If a (different) handled + signal occurs while calling a registered function, it is attempted + to be called again by the next signal handler. + + """ def __init__(self, func=None): self.funcs = [] self.prev_handlers = {} - if func: + if func is not None: self.register(func) def __enter__(self): self.set_signal_handlers() - def __exit__(self, exec_type, exec_value, traceback): + def __exit__(self, exec_type, exec_value, trace): if exec_value is not None: + logger.debug("Encountered exception:\n%s", "".join( + traceback.format_exception(exec_type, exec_value, trace))) self.call_registered() self.reset_signal_handlers() @@ -29,9 +61,15 @@ class ErrorHandler(object): self.funcs.append(func) def call_registered(self): - """Calls all functions in the order they were registered.""" - for func in self.funcs: - func() + """Calls all registered functions""" + logger.debug("Calling registered functions") + while self.funcs: + try: + self.funcs[-1]() + except Exception as error: # pylint: disable=broad-except + logger.error("Encountered exception during recovery") + logger.exception(error) + self.funcs.pop() def set_signal_handlers(self): """Sets signal handlers for signals in _SIGNALS.""" @@ -48,12 +86,13 @@ class ErrorHandler(object): signal.signal(signum, self.prev_handlers[signum]) self.prev_handlers.clear() - def _signal_handler(self, signum, _): + def _signal_handler(self, signum, unused_frame): """Calls registered functions and the previous signal handler. :param int signum: number of current signal """ + logger.debug("Singal %s encountered", signum) self.call_registered() signal.signal(signum, self.prev_handlers[signum]) os.kill(os.getpid(), signum) diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py index 0131d3c93..83cd54226 100644 --- a/letsencrypt/tests/client_test.py +++ b/letsencrypt/tests/client_test.py @@ -189,7 +189,7 @@ class ClientTest(unittest.TestCase): installer.deploy_cert.assert_called_once_with( "foo.bar", os.path.abspath("cert"), os.path.abspath("key"), os.path.abspath("chain")) - self.assertTrue(installer.save.call_count == 1) + self.assertEqual(installer.save.call_count, 1) installer.restart.assert_called_once_with() @mock.patch("letsencrypt.client.enhancements") @@ -203,7 +203,7 @@ class ClientTest(unittest.TestCase): self.client.enhance_config(["foo.bar"]) installer.enhance.assert_called_once_with("foo.bar", "redirect") - self.assertTrue(installer.save.call_count == 1) + self.assertEqual(installer.save.call_count, 1) installer.restart.assert_called_once_with() installer.enhance.side_effect = errors.PluginError diff --git a/letsencrypt/tests/error_handler_test.py b/letsencrypt/tests/error_handler_test.py index 6927b32a0..66acac930 100644 --- a/letsencrypt/tests/error_handler_test.py +++ b/letsencrypt/tests/error_handler_test.py @@ -4,15 +4,17 @@ import unittest import mock -from letsencrypt import error_handler - class ErrorHandlerTest(unittest.TestCase): """Tests for letsencrypt.error_handler.""" def setUp(self): + from letsencrypt import error_handler + self.init_func = mock.MagicMock() self.handler = error_handler.ErrorHandler(self.init_func) + # pylint: disable=protected-access + self.signals = error_handler._SIGNALS def test_context_manager(self): try: @@ -29,18 +31,25 @@ class ErrorHandlerTest(unittest.TestCase): mock_signal.getsignal.return_value = signal.SIG_DFL self.handler.set_signal_handlers() signal_handler = self.handler._signal_handler - for signum in error_handler._SIGNALS: + for signum in self.signals: mock_signal.signal.assert_any_call(signum, signal_handler) - signum = error_handler._SIGNALS[0] + signum = self.signals[0] signal_handler(signum, None) self.init_func.assert_called_once_with() mock_os.kill.assert_called_once_with(mock_os.getpid(), signum) self.handler.reset_signal_handlers() - for signum in error_handler._SIGNALS: + for signum in self.signals: mock_signal.signal.assert_any_call(signum, signal.SIG_DFL) + def test_bad_recovery(self): + bad_func = mock.MagicMock(side_effect=[ValueError]) + self.handler.register(bad_func) + self.handler.call_registered() + self.init_func.assert_called_once_with() + bad_func.assert_called_once_with() + if __name__ == "__main__": unittest.main() # pragma: no cover