diff --git a/certbot/main.py b/certbot/main.py index be68d694e..8bccc524d 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 @@ -36,6 +35,13 @@ 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 = 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__) @@ -596,13 +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 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 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). @@ -702,6 +703,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 as error: + raise errors.Error(_PERM_ERR_FMT.format(error)) + + 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 +730,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) diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 66cba64a3..32df525f0 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -1,12 +1,14 @@ """Tests for certbot.main.""" +import os +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 @@ -44,5 +46,51 @@ 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") + + +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 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