From 40449ed2747634ae77928cae45424e5031a66c5b Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 12:49:41 -0700 Subject: [PATCH 1/8] Add single _PERM_ERR_FMT string --- certbot/main.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/certbot/main.py b/certbot/main.py index be68d694e..8e47e736a 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -36,6 +36,12 @@ from certbot.display import util as display_util, ops as display_ops from certbot.plugins import disco as plugins_disco from certbot.plugins import selection as plug_sel + +_PERM_ERR_FMT = ("An error occurred while trying to create or modify {0}. To " + "run as non-root, set --config-dir, --logs-dir, and " + "--work-dir to writeable paths.") + + logger = logging.getLogger(__name__) From 9ae755ef4cf3f8f4978d7c5594d6d16ed835a1f5 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 12:53:09 -0700 Subject: [PATCH 2/8] simplify log file error handling --- certbot/main.py | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index 8e47e736a..d3f90926a 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -2,7 +2,6 @@ from __future__ import print_function import atexit import dialog -import errno import functools import logging.handlers import os @@ -602,13 +601,8 @@ def setup_log_file_handler(config, logfile, fmt): try: handler = logging.handlers.RotatingFileHandler( log_file_path, maxBytes=2 ** 20, backupCount=10) - except IOError as e: - if e.errno == errno.EACCES: - msg = ("Access denied writing to {0}. To run as non-root, set " + - "--logs-dir, --config-dir, --work-dir to writable paths.") - raise errors.Error(msg.format(log_file_path)) - else: - raise + except IOError: + raise errors.Error(_PERM_ERR_FMT.format(log_file_path)) # rotate on each invocation, rollover only possible when maxBytes # is nonzero and backupCount is nonzero, so we set maxBytes as big # as possible not to overrun in single CLI invocation (1MB). From f3c6bac31065a200e1a2b79579c907279d48af1b Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 13:02:28 -0700 Subject: [PATCH 3/8] stop spacing out --- certbot/tests/main_test.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 66cba64a3..d044a50b7 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1,10 +1,8 @@ """Tests for certbot.main.""" import unittest - import mock - from certbot import cli from certbot import configuration from certbot.plugins import disco as plugins_disco From 4f35f3fdf7e8631282d11d3c7a854770dd02dccf Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 13:07:49 -0700 Subject: [PATCH 4/8] Add SetupLogFileHandlerTest --- certbot/tests/main_test.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index d044a50b7..5f6723bd7 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1,10 +1,13 @@ """Tests for certbot.main.""" +import shutil +import tempfile import unittest import mock from certbot import cli from certbot import configuration +from certbot import errors from certbot.plugins import disco as plugins_disco @@ -42,5 +45,26 @@ class ObtainCertTest(unittest.TestCase): self.assertFalse(pause) +class SetupLogFileHandlerTest(unittest.TestCase): + """Tests for certbot.main.setup_log_file_handler.""" + + def setUp(self): + self.config = mock.Mock(spec_set=['logs_dir'], + logs_dir=tempfile.mkdtemp()) + + def tearDown(self): + shutil.rmtree(self.config.logs_dir) + + def _call(self, *args, **kwargs): + from certbot.main import setup_log_file_handler + return setup_log_file_handler(*args, **kwargs) + + @mock.patch('certbot.main.logging.handlers.RotatingFileHandler') + def test_ioerror(self, mock_handler): + mock_handler.side_effect = IOError + self.assertRaises(errors.Error, self._call, + self.config, "test.log", "%s") + + if __name__ == '__main__': unittest.main() # pragma: no cover From 48b7c01a5925e476f6b4197fa34370cc07d97607 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 14:06:15 -0700 Subject: [PATCH 5/8] bring make_or_verify_dir docstring up to date --- certbot/util.py | 1 + 1 file changed, 1 insertion(+) diff --git a/certbot/util.py b/certbot/util.py index 301fc669b..2b40a0f2c 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -95,6 +95,7 @@ def make_or_verify_dir(directory, mode=0o755, uid=0, strict=False): :param str directory: Path to a directory. :param int mode: Directory mode. :param int uid: Directory owner. + :param bool strict: require directory to be owned by current user :raises .errors.Error: if a directory already exists, but has wrong permissions or owner From d7772217032235337405f8e4b62a9970f057a8fc Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 14:17:19 -0700 Subject: [PATCH 6/8] write make_or_verify_core_dir --- certbot/main.py | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index d3f90926a..2a18aa528 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -702,6 +702,23 @@ def _handle_exception(exc_type, exc_value, trace, config): traceback.format_exception(exc_type, exc_value, trace))) +def make_or_verify_core_dir(directory, mode, uid, strict): + """Make sure directory exists with proper permissions. + + :param str directory: Path to a directory. + :param int mode: Directory mode. + :param int uid: Directory owner. + :param bool strict: require directory to be owned by current user + + :raises .errors.Error: if the directory cannot be made or verified + + """ + try: + util.make_or_verify_dir(directory, mode, uid, strict) + except OSError: + raise errors.Error(_PERM_ERR_FMT.format(directory)) + + def main(cli_args=sys.argv[1:]): """Command line argument parsing and main script execution.""" sys.excepthook = functools.partial(_handle_exception, config=None) @@ -712,16 +729,16 @@ def main(cli_args=sys.argv[1:]): config = configuration.NamespaceConfig(args) zope.component.provideUtility(config) - # Setup logging ASAP, otherwise "No handlers could be found for - # logger ..." TODO: this should be done before plugins discovery - for directory in config.config_dir, config.work_dir: - util.make_or_verify_dir( - directory, constants.CONFIG_DIRS_MODE, os.geteuid(), - "--strict-permissions" in cli_args) + make_or_verify_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, + os.geteuid(), config.strict_permissions) + make_or_verify_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, + os.geteuid(), config.strict_permissions) # TODO: logs might contain sensitive data such as contents of the # private key! #525 - util.make_or_verify_dir( - config.logs_dir, 0o700, os.geteuid(), "--strict-permissions" in cli_args) + make_or_verify_core_dir(config.logs_dir, 0o700, + os.geteuid(), config.strict_permissions) + # Setup logging ASAP, otherwise "No handlers could be found for + # logger ..." TODO: this should be done before plugins discovery setup_logging(config, _cli_log_handler, logfile='letsencrypt.log') cli.possible_deprecation_warning(config) From e598e907bdbfb69418e6909a5e0f8bc3eb1994e4 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 14:54:51 -0700 Subject: [PATCH 7/8] create MakeOrVerifyCoreDirTest --- certbot/tests/main_test.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 5f6723bd7..32df525f0 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1,4 +1,5 @@ """Tests for certbot.main.""" +import os import shutil import tempfile import unittest @@ -66,5 +67,30 @@ class SetupLogFileHandlerTest(unittest.TestCase): self.config, "test.log", "%s") +class MakeOrVerifyCoreDirTest(unittest.TestCase): + """Tests for certbot.main.make_or_verify_core_dir.""" + + def setUp(self): + self.dir = tempfile.mkdtemp() + + def tearDown(self): + shutil.rmtree(self.dir) + + def _call(self, *args, **kwargs): + from certbot.main import make_or_verify_core_dir + return make_or_verify_core_dir(*args, **kwargs) + + def test_success(self): + new_dir = os.path.join(self.dir, 'new') + self._call(new_dir, 0o700, os.geteuid(), False) + self.assertTrue(os.path.exists(new_dir)) + + @mock.patch('certbot.main.util.make_or_verify_dir') + def test_failure(self, mock_make_or_verify): + mock_make_or_verify.side_effect = OSError + self.assertRaises(errors.Error, self._call, + self.dir, 0o700, os.geteuid(), False) + + if __name__ == '__main__': unittest.main() # pragma: no cover From 9372914c67e189c00a4f6c4143011811b4a617d9 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 8 Jul 2016 15:51:31 -0700 Subject: [PATCH 8/8] Improve error message --- certbot/main.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/certbot/main.py b/certbot/main.py index 2a18aa528..8bccc524d 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -36,9 +36,10 @@ from certbot.plugins import disco as plugins_disco from certbot.plugins import selection as plug_sel -_PERM_ERR_FMT = ("An error occurred while trying to create or modify {0}. To " - "run as non-root, set --config-dir, --logs-dir, and " - "--work-dir to writeable paths.") +_PERM_ERR_FMT = os.linesep.join(( + "The following error was encountered:", "{0}", + "If running as non-root, set --config-dir, " + "--logs-dir, and --work-dir to writeable paths.")) logger = logging.getLogger(__name__) @@ -601,8 +602,8 @@ def setup_log_file_handler(config, logfile, fmt): try: handler = logging.handlers.RotatingFileHandler( log_file_path, maxBytes=2 ** 20, backupCount=10) - except IOError: - raise errors.Error(_PERM_ERR_FMT.format(log_file_path)) + except IOError as error: + raise errors.Error(_PERM_ERR_FMT.format(error)) # rotate on each invocation, rollover only possible when maxBytes # is nonzero and backupCount is nonzero, so we set maxBytes as big # as possible not to overrun in single CLI invocation (1MB). @@ -715,8 +716,8 @@ def make_or_verify_core_dir(directory, mode, uid, strict): """ try: util.make_or_verify_dir(directory, mode, uid, strict) - except OSError: - raise errors.Error(_PERM_ERR_FMT.format(directory)) + except OSError as error: + raise errors.Error(_PERM_ERR_FMT.format(error)) def main(cli_args=sys.argv[1:]):