From e591a7666c84c41f7a67cf62fb15a72fe087f9ed Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Tue, 26 Jan 2016 12:29:49 -0500 Subject: [PATCH 1/2] Guard reverter invocations and wrap in correct exceptions --- .../letsencrypt_nginx/configurator.py | 48 ++++++++++++++----- .../tests/configurator_test.py | 25 ++++++++++ 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index efa7e08b4..88fa66843 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -190,6 +190,12 @@ class NginxConfigurator(common.Plugin): ", ".join(str(addr) for addr in vhost.addrs))) self.save_notes += "\tssl_certificate %s\n" % fullchain_path self.save_notes += "\tssl_certificate_key %s\n" % key_path + if len(stapling_directives) > 0: + self.save_notes += "\tssl_trusted_certificate %s\n" % chain_path + self.save_notes += "\tssl_stapling on\n" + self.save_notes += "\tssl_stapling_verify on\n" + + ####################### # Vhost parsing methods @@ -514,18 +520,26 @@ class NginxConfigurator(common.Plugin): """ save_files = set(self.parser.parsed.keys()) - # Create Checkpoint - if temporary: - self.reverter.add_to_temp_checkpoint( - save_files, self.save_notes) - else: - self.reverter.add_to_checkpoint(save_files, + try: + # Create Checkpoint + if temporary: + self.reverter.add_to_temp_checkpoint( + save_files, self.save_notes) + else: + self.reverter.add_to_checkpoint(save_files, self.save_notes) + except errors.ReverterError as err: + raise errors.PluginError(str(err)) + + self.save_notes = "" # Change 'ext' to something else to not override existing conf files self.parser.filedump(ext='') if title and not temporary: - self.reverter.finalize_checkpoint(title) + try: + self.reverter.finalize_checkpoint(title) + except errors.ReverterError as err: + raise errors.PluginError(str(err)) return True @@ -535,12 +549,18 @@ class NginxConfigurator(common.Plugin): Reverts all modified files that have not been saved as a checkpoint """ - self.reverter.recovery_routine() + try: + self.reverter.recovery_routine() + except errors.ReverterError as err: + raise errors.PluginError(str(err)) self.parser.load() def revert_challenge_config(self): """Used to cleanup challenge configurations.""" - self.reverter.revert_temporary_config() + try: + self.reverter.revert_temporary_config() + except errors.ReverterError as err: + raise errors.PluginError(str(err)) self.parser.load() def rollback_checkpoints(self, rollback=1): @@ -549,12 +569,18 @@ class NginxConfigurator(common.Plugin): :param int rollback: Number of checkpoints to revert """ - self.reverter.rollback_checkpoints(rollback) + try: + self.reverter.rollback_checkpoints(rollback) + except errors.ReverterError as err: + raise errors.PluginError(str(err)) self.parser.load() def view_config_changes(self): """Show all of the configuration changes that have taken place.""" - self.reverter.view_config_changes() + try: + self.reverter.view_config_changes() + except errors.ReverterError as err: + raise errors.PluginError(str(err)) ########################################################################### # Challenges Section for IAuthenticator diff --git a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py index 4fce33079..4d15d6a75 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py +++ b/letsencrypt-nginx/letsencrypt_nginx/tests/configurator_test.py @@ -371,6 +371,31 @@ class NginxConfiguratorTest(util.NginxTest): mock_run_script.side_effect = errors.SubprocessError self.assertRaises(errors.MisconfigurationError, self.config.config_test) + @mock.patch("letsencrypt.reverter.Reverter.recovery_routine") + def test_recovery_routine_throws_error_from_reverter(self, mock_recovery_routine): + mock_recovery_routine.side_effect = errors.ReverterError("foo") + self.assertRaises(errors.PluginError, self.config.recovery_routine) + + @mock.patch("letsencrypt.reverter.Reverter.view_config_changes") + def test_view_config_changes_throws_error_from_reverter(self, mock_view_config_changes): + mock_view_config_changes.side_effect = errors.ReverterError("foo") + self.assertRaises(errors.PluginError, self.config.view_config_changes) + + @mock.patch("letsencrypt.reverter.Reverter.rollback_checkpoints") + def test_rollback_checkpoints_throws_error_from_reverter(self, mock_rollback_checkpoints): + mock_rollback_checkpoints.side_effect = errors.ReverterError("foo") + self.assertRaises(errors.PluginError, self.config.rollback_checkpoints) + + @mock.patch("letsencrypt.reverter.Reverter.revert_temporary_config") + def test_revert_challenge_config_throws_error_from_reverter(self, mock_revert_temporary_config): + mock_revert_temporary_config.side_effect = errors.ReverterError("foo") + self.assertRaises(errors.PluginError, self.config.revert_challenge_config) + + @mock.patch("letsencrypt.reverter.Reverter.add_to_checkpoint") + def test_save_throws_error_from_reverter(self, mock_add_to_checkpoint): + mock_add_to_checkpoint.side_effect = errors.ReverterError("foo") + self.assertRaises(errors.PluginError, self.config.save) + def test_get_snakeoil_paths(self): # pylint: disable=protected-access cert, key = self.config._get_snakeoil_paths() From 15182d5aa487c2843502894cad851a4d31487a00 Mon Sep 17 00:00:00 2001 From: Ola Bini Date: Wed, 27 Jan 2016 09:06:56 -0500 Subject: [PATCH 2/2] Add comments about the exceptions raised --- .../letsencrypt_nginx/configurator.py | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/letsencrypt-nginx/letsencrypt_nginx/configurator.py b/letsencrypt-nginx/letsencrypt_nginx/configurator.py index 88fa66843..b0a1e76cd 100644 --- a/letsencrypt-nginx/letsencrypt_nginx/configurator.py +++ b/letsencrypt-nginx/letsencrypt_nginx/configurator.py @@ -517,6 +517,10 @@ class NginxConfigurator(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 + an attempt to save the configuration, or an error creating a + checkpoint + """ save_files = set(self.parser.parsed.keys()) @@ -548,6 +552,8 @@ class NginxConfigurator(common.Plugin): Reverts all modified files that have not been saved as a checkpoint + :raises .errors.PluginError: If unable to recover the configuration + """ try: self.reverter.recovery_routine() @@ -556,7 +562,11 @@ class NginxConfigurator(common.Plugin): self.parser.load() def revert_challenge_config(self): - """Used to cleanup challenge configurations.""" + """Used to cleanup challenge configurations. + + :raises .errors.PluginError: If unable to revert the challenge config. + + """ try: self.reverter.revert_temporary_config() except errors.ReverterError as err: @@ -568,6 +578,9 @@ class NginxConfigurator(common.Plugin): :param int rollback: Number of checkpoints to revert + :raises .errors.PluginError: If there is a problem with the input or + the function is unable to correctly revert the configuration + """ try: self.reverter.rollback_checkpoints(rollback) @@ -576,7 +589,12 @@ class NginxConfigurator(common.Plugin): self.parser.load() def view_config_changes(self): - """Show all of the configuration changes that have taken place.""" + """Show all of the configuration changes that have taken place. + + :raises .errors.PluginError: If there is a problem while processing + the checkpoints directories. + + """ try: self.reverter.view_config_changes() except errors.ReverterError as err: