diff --git a/acme/setup.py b/acme/setup.py index cb12df51f..8d8d1a049 100644 --- a/acme/setup.py +++ b/acme/setup.py @@ -29,7 +29,10 @@ install_requires = [ # env markers cause problems with older pip and setuptools if sys.version_info < (2, 7): - install_requires.append('argparse') + install_requires.extend([ + 'argparse', + 'ordereddict', + ]) dev_extras = [ 'nose', diff --git a/certbot-apache/certbot_apache/configurator.py b/certbot-apache/certbot_apache/configurator.py index 39d25619d..b82506dd2 100644 --- a/certbot-apache/certbot_apache/configurator.py +++ b/certbot-apache/certbot_apache/configurator.py @@ -197,6 +197,14 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): install_ssl_options_conf(self.mod_ssl_conf) + # Prevent two Apache plugins from modifying a config at once + try: + util.lock_dir_until_exit(self.conf("server-root")) + except (OSError, errors.LockError): + logger.debug("Encountered error:", exc_info=True) + raise errors.PluginError( + "Unable to lock %s", self.conf("server-root")) + def _check_aug_version(self): """ Checks that we have recent enough version of libaugeas. If augeas version is recent enough, it will support case insensitive diff --git a/certbot-apache/certbot_apache/tests/configurator_test.py b/certbot-apache/certbot_apache/tests/configurator_test.py index 45e701bd5..3e12bf60a 100644 --- a/certbot-apache/certbot_apache/tests/configurator_test.py +++ b/certbot-apache/certbot_apache/tests/configurator_test.py @@ -95,6 +95,23 @@ class MultipleVhostsTest(util.ApacheTest): self.assertRaises( errors.NotSupportedError, self.config.prepare) + def test_prepare_locked(self): + server_root = self.config.conf("server-root") + self.config.config_test = mock.Mock() + os.remove(os.path.join(server_root, ".certbot.lock")) + certbot_util.lock_and_call(self._test_prepare_locked, server_root) + + @mock.patch("certbot_apache.parser.ApacheParser") + @mock.patch("certbot_apache.configurator.util.exe_exists") + def _test_prepare_locked(self, unused_parser, unused_exe_exists): + try: + self.config.prepare() + except errors.PluginError as err: + err_msg = str(err) + self.assertTrue("lock" in err_msg) + self.assertTrue(self.config.conf("server-root") in err_msg) + else: # pragma: no cover + self.fail("Exception wasn't raised!") def test_add_parser_arguments(self): # pylint: disable=no-self-use from certbot_apache.configurator import ApacheConfigurator diff --git a/certbot-nginx/certbot_nginx/configurator.py b/certbot-nginx/certbot_nginx/configurator.py index afa701a75..8608fb66a 100644 --- a/certbot-nginx/certbot_nginx/configurator.py +++ b/certbot-nginx/certbot_nginx/configurator.py @@ -162,6 +162,14 @@ class NginxConfigurator(common.Plugin): if self.version is None: self.version = self.get_version() + # Prevent two Nginx plugins from modifying a config at once + try: + util.lock_dir_until_exit(self.conf('server-root')) + except (OSError, errors.LockError): + logger.debug('Encountered error:', exc_info=True) + raise errors.PluginError( + 'Unable to lock %s', self.conf('server-root')) + # Entry point in main.py for installing cert def deploy_cert(self, domain, cert_path, key_path, chain_path=None, fullchain_path=None): diff --git a/certbot-nginx/certbot_nginx/tests/configurator_test.py b/certbot-nginx/certbot_nginx/tests/configurator_test.py index b9e70cd59..69f728b53 100644 --- a/certbot-nginx/certbot_nginx/tests/configurator_test.py +++ b/certbot-nginx/certbot_nginx/tests/configurator_test.py @@ -12,6 +12,7 @@ from acme import messages from certbot import achallenges from certbot import errors +from certbot.tests import util as certbot_test_util from certbot_nginx import obj from certbot_nginx import parser @@ -65,6 +66,23 @@ class NginxConfiguratorTest(util.NginxTest): self.config.prepare() self.assertEqual((1, 6, 2), self.config.version) + def test_prepare_locked(self): + server_root = self.config.conf("server-root") + self.config.config_test = mock.Mock() + os.remove(os.path.join(server_root, ".certbot.lock")) + certbot_test_util.lock_and_call(self._test_prepare_locked, server_root) + + @mock.patch("certbot_nginx.configurator.util.exe_exists") + def _test_prepare_locked(self, unused_exe_exists): + try: + self.config.prepare() + except errors.PluginError as err: + err_msg = str(err) + self.assertTrue("lock" in err_msg) + self.assertTrue(self.config.conf("server-root") in err_msg) + else: # pragma: no cover + self.fail("Exception wasn't raised!") + @mock.patch("certbot_nginx.configurator.socket.gethostbyaddr") def test_get_all_names(self, mock_gethostbyaddr): mock_gethostbyaddr.return_value = ('155.225.50.69.nephoscale.net', [], []) diff --git a/certbot/errors.py b/certbot/errors.py index 6d191404c..551add61c 100644 --- a/certbot/errors.py +++ b/certbot/errors.py @@ -33,6 +33,10 @@ class SignalExit(Error): """A Unix signal was received while in the ErrorHandler context manager.""" +class LockError(Error): + """File locking error.""" + + # Auth Handler Errors class AuthorizationError(Error): """Authorization error.""" diff --git a/certbot/lock.py b/certbot/lock.py new file mode 100644 index 000000000..5f59cc090 --- /dev/null +++ b/certbot/lock.py @@ -0,0 +1,139 @@ +"""Implements file locks for locking files and directories in UNIX.""" +import errno +import fcntl +import logging +import os + +from certbot import errors + +logger = logging.getLogger(__name__) + + +def lock_dir(dir_path): + """Place a lock file on the directory at dir_path. + + The lock file is placed in the root of dir_path with the name + .certbot.lock. + + :param str dir_path: path to directory + + :returns: the locked LockFile object + :rtype: LockFile + + :raises errors.LockError: if unable to acquire the lock + + """ + return LockFile(os.path.join(dir_path, '.certbot.lock')) + + +class LockFile(object): + """A UNIX lock file. + + This lock file is released when the locked file is closed or the + process exits. It cannot be used to provide synchronization between + threads. It is based on the lock_file package by Martin Horcicka. + + """ + def __init__(self, path): + """Initialize and acquire the lock file. + + :param str path: path to the file to lock + + :raises errors.LockError: if unable to acquire the lock + + """ + super(LockFile, self).__init__() + self._path = path + self._fd = None + + self.acquire() + + def acquire(self): + """Acquire the lock file. + + :raises errors.LockError: if lock is already held + :raises OSError: if unable to open or stat the lock file + + """ + while self._fd is None: + # Open the file + fd = os.open(self._path, os.O_CREAT | os.O_WRONLY, 0o600) + try: + self._try_lock(fd) + if self._lock_success(fd): + self._fd = fd + finally: + # Close the file if it is not the required one + if self._fd is None: + os.close(fd) + + def _try_lock(self, fd): + """Try to acquire the lock file without blocking. + + :param int fd: file descriptor of the opened file to lock + + """ + try: + fcntl.lockf(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) + except IOError as err: + if err.errno in (errno.EACCES, errno.EAGAIN): + logger.debug( + "A lock on %s is held by another process.", self._path) + raise errors.LockError( + "Another instance of Certbot is already running.") + raise + + def _lock_success(self, fd): + """Did we successfully grab the lock? + + Because this class deletes the locked file when the lock is + released, it is possible another process removed and recreated + the file between us opening the file and acquiring the lock. + + :param int fd: file descriptor of the opened file to lock + + :returns: True if the lock was successfully acquired + :rtype: bool + + """ + try: + stat1 = os.stat(self._path) + except OSError as err: + if err.errno == errno.ENOENT: + return False + raise + + stat2 = os.fstat(fd) + # If our locked file descriptor and the file on disk refer to + # the same device and inode, they're the same file. + return stat1.st_dev == stat2.st_dev and stat1.st_ino == stat2.st_ino + + def __repr__(self): + repr_str = '{0}({1}) <'.format(self.__class__.__name__, self._path) + if self._fd is None: + repr_str += 'released>' + else: + repr_str += 'acquired>' + return repr_str + + def release(self): + """Remove, close, and release the lock file.""" + # It is important the lock file is removed before it's released, + # otherwise: + # + # process A: open lock file + # process B: release lock file + # process A: lock file + # process A: check device and inode + # process B: delete file + # process C: open and lock a different file at the same path + # + # Calling os.remove on a file that's in use doesn't work on + # Windows, but neither does locking with fcntl. + try: + os.remove(self._path) + finally: + try: + os.close(self._fd) + finally: + self._fd = None diff --git a/certbot/log.py b/certbot/log.py index 7660846a6..c7bc867f1 100644 --- a/certbot/log.py +++ b/certbot/log.py @@ -131,7 +131,7 @@ def setup_log_file_handler(config, logfile, fmt): """ # TODO: logs might contain sensitive data such as contents of the # private key! #525 - util.make_or_verify_core_dir( + util.set_up_core_dir( config.logs_dir, 0o700, os.geteuid(), config.strict_permissions) log_file_path = os.path.join(config.logs_dir, logfile) try: diff --git a/certbot/main.py b/certbot/main.py index 023c09aee..eafcf49dd 100644 --- a/certbot/main.py +++ b/certbot/main.py @@ -696,10 +696,10 @@ def renew(config, unused_plugins): def make_or_verify_needed_dirs(config): """Create or verify existence of config and work directories""" - util.make_or_verify_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, - os.geteuid(), config.strict_permissions) - util.make_or_verify_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, - os.geteuid(), config.strict_permissions) + util.set_up_core_dir(config.config_dir, constants.CONFIG_DIRS_MODE, + os.geteuid(), config.strict_permissions) + util.set_up_core_dir(config.work_dir, constants.CONFIG_DIRS_MODE, + os.geteuid(), config.strict_permissions) def set_displayer(config): diff --git a/certbot/plugins/disco.py b/certbot/plugins/disco.py index a17f8d7b3..6bf4bd369 100644 --- a/certbot/plugins/disco.py +++ b/certbot/plugins/disco.py @@ -12,6 +12,12 @@ from certbot import constants from certbot import errors from certbot import interfaces +try: + from collections import OrderedDict +except ImportError: # pragma: no cover + # OrderedDict was added in Python 2.7 + from ordereddict import OrderedDict # pylint: disable=import-error + logger = logging.getLogger(__name__) @@ -168,7 +174,11 @@ class PluginsRegistry(collections.Mapping): """Plugins registry.""" def __init__(self, plugins): - self._plugins = plugins + # plugins are sorted so the same order is used between runs. + # This prevents deadlock caused by plugins acquiring a lock + # and ensures at least one concurrent Certbot instance will run + # successfully. + self._plugins = OrderedDict(sorted(six.iteritems(plugins))) @classmethod def find_all(cls): diff --git a/certbot/plugins/disco_test.py b/certbot/plugins/disco_test.py index 6c3c39dca..220b902b3 100644 --- a/certbot/plugins/disco_test.py +++ b/certbot/plugins/disco_test.py @@ -1,4 +1,6 @@ """Tests for certbot.plugins.disco.""" +import functools +import string import unittest import mock @@ -182,12 +184,17 @@ class PluginEntryPointTest(unittest.TestCase): class PluginsRegistryTest(unittest.TestCase): """Tests for certbot.plugins.disco.PluginsRegistry.""" - def setUp(self): + @classmethod + def _create_new_registry(cls, plugins): from certbot.plugins.disco import PluginsRegistry - self.plugin_ep = mock.MagicMock(name="mock") + return PluginsRegistry(plugins) + + def setUp(self): + self.plugin_ep = mock.MagicMock() + self.plugin_ep.name = "mock" self.plugin_ep.__hash__.side_effect = TypeError - self.plugins = {"mock": self.plugin_ep} - self.reg = PluginsRegistry(self.plugins) + self.plugins = {self.plugin_ep.name: self.plugin_ep} + self.reg = self._create_new_registry(self.plugins) def test_find_all(self): from certbot.plugins.disco import PluginsRegistry @@ -207,9 +214,8 @@ class PluginsRegistryTest(unittest.TestCase): self.assertEqual(["mock"], list(self.reg)) def test_len(self): + self.assertEqual(0, len(self._create_new_registry({}))) self.assertEqual(1, len(self.reg)) - self.plugins.clear() - self.assertEqual(0, len(self.reg)) def test_init(self): self.plugin_ep.init.return_value = "baz" @@ -217,14 +223,11 @@ class PluginsRegistryTest(unittest.TestCase): self.plugin_ep.init.assert_called_once_with("bar") def test_filter(self): - self.plugins.update({ - "foo": "bar", - "bar": "foo", - "baz": "boo", - }) self.assertEqual( - {"foo": "bar", "baz": "boo"}, - self.reg.filter(lambda p_ep: str(p_ep).startswith("b"))) + self.plugins, + self.reg.filter(lambda p_ep: p_ep.name.startswith("m"))) + self.assertEqual( + {}, self.reg.filter(lambda p_ep: p_ep.name.startswith("b"))) def test_ifaces(self): self.plugin_ep.ifaces.return_value = True @@ -246,6 +249,17 @@ class PluginsRegistryTest(unittest.TestCase): self.assertEqual(["baz"], self.reg.prepare()) self.plugin_ep.prepare.assert_called_once_with() + def test_prepare_order(self): + order = [] + plugins = dict( + (c, mock.MagicMock(prepare=functools.partial(order.append, c))) + for c in string.ascii_letters) + reg = self._create_new_registry(plugins) + reg.prepare() + # order of prepare calls must be sorted to prevent deadlock + # caused by plugins acquiring locks during prepare + self.assertEqual(order, sorted(string.ascii_letters)) + def test_available(self): self.plugin_ep.available = True # pylint: disable=protected-access @@ -265,11 +279,12 @@ class PluginsRegistryTest(unittest.TestCase): repr(self.reg)) def test_str(self): + self.assertEqual("No plugins", str(self._create_new_registry({}))) self.plugin_ep.__str__ = lambda _: "Mock" - self.plugins["foo"] = "Mock" - self.assertEqual("Mock\n\nMock", str(self.reg)) - self.plugins.clear() - self.assertEqual("No plugins", str(self.reg)) + self.assertEqual("Mock", str(self.reg)) + plugins = {self.plugin_ep.name: self.plugin_ep, "foo": "Bar"} + reg = self._create_new_registry(plugins) + self.assertEqual("Bar\n\nMock", str(reg)) if __name__ == "__main__": diff --git a/certbot/tests/lock_test.py b/certbot/tests/lock_test.py new file mode 100644 index 000000000..e1a4f8c8a --- /dev/null +++ b/certbot/tests/lock_test.py @@ -0,0 +1,116 @@ +"""Tests for certbot.lock.""" +import functools +import multiprocessing +import os +import unittest + +import mock + +from certbot import errors +from certbot.tests import util as test_util + + +class LockDirTest(test_util.TempDirTestCase): + """Tests for certbot.lock.lock_dir.""" + @classmethod + def _call(cls, *args, **kwargs): + from certbot.lock import lock_dir + return lock_dir(*args, **kwargs) + + def test_it(self): + assert_raises = functools.partial( + self.assertRaises, errors.LockError, self._call, self.tempdir) + lock_path = os.path.join(self.tempdir, '.certbot.lock') + test_util.lock_and_call(assert_raises, lock_path) + + +class LockFileTest(test_util.TempDirTestCase): + """Tests for certbot.lock.LockFile.""" + @classmethod + def _call(cls, *args, **kwargs): + from certbot.lock import LockFile + return LockFile(*args, **kwargs) + + def setUp(self): + super(LockFileTest, self).setUp() + self.lock_path = os.path.join(self.tempdir, 'test.lock') + + def test_acquire_without_deletion(self): + # acquire the lock in another process but don't delete the file + child = multiprocessing.Process(target=self._call, + args=(self.lock_path,)) + child.start() + child.join() + self.assertEqual(child.exitcode, 0) + self.assertTrue(os.path.exists(self.lock_path)) + + # Test we're still able to properly acquire and release the lock + self.test_removed() + + def test_contention(self): + assert_raises = functools.partial( + self.assertRaises, errors.LockError, self._call, self.lock_path) + test_util.lock_and_call(assert_raises, self.lock_path) + + def test_locked_repr(self): + lock_file = self._call(self.lock_path) + locked_repr = repr(lock_file) + self._test_repr_common(lock_file, locked_repr) + self.assertTrue('acquired' in locked_repr) + + def test_released_repr(self): + lock_file = self._call(self.lock_path) + lock_file.release() + released_repr = repr(lock_file) + self._test_repr_common(lock_file, released_repr) + self.assertTrue('released' in released_repr) + + def _test_repr_common(self, lock_file, lock_repr): + self.assertTrue(lock_file.__class__.__name__ in lock_repr) + self.assertTrue(self.lock_path in lock_repr) + + def test_race(self): + should_delete = [True, False] + stat = os.stat + + def delete_and_stat(path): + """Wrap os.stat and maybe delete the file first.""" + if path == self.lock_path and should_delete.pop(0): + os.remove(path) + return stat(path) + + with mock.patch('certbot.lock.os.stat') as mock_stat: + mock_stat.side_effect = delete_and_stat + self._call(self.lock_path) + self.assertFalse(should_delete) + + def test_removed(self): + lock_file = self._call(self.lock_path) + lock_file.release() + self.assertFalse(os.path.exists(self.lock_path)) + + @mock.patch('certbot.lock.fcntl.lockf') + def test_unexpected_lockf_err(self, mock_lockf): + msg = 'hi there' + mock_lockf.side_effect = IOError(msg) + try: + self._call(self.lock_path) + except IOError as err: + self.assertTrue(msg in str(err)) + else: # pragma: no cover + self.fail('IOError not raised') + + @mock.patch('certbot.lock.os.stat') + def test_unexpected_stat_err(self, mock_stat): + msg = 'hi there' + mock_stat.side_effect = OSError(msg) + try: + self._call(self.lock_path) + except OSError as err: + self.assertTrue(msg in str(err)) + else: # pragma: no cover + self.fail('OSError not raised') + + +if __name__ == "__main__": + unittest.main() # pragma: no cover diff --git a/certbot/tests/main_test.py b/certbot/tests/main_test.py index 562c4bb9d..23cff7edd 100644 --- a/certbot/tests/main_test.py +++ b/certbot/tests/main_test.py @@ -792,12 +792,12 @@ class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-me print(lf.read()) def test_renew_verb(self): - test_util.make_lineage(self, 'sample-renewal.conf') + test_util.make_lineage(self.config_dir, 'sample-renewal.conf') args = ["renew", "--dry-run", "-tvv"] self._test_renewal_common(True, [], args=args, should_renew=True) def test_quiet_renew(self): - test_util.make_lineage(self, 'sample-renewal.conf') + test_util.make_lineage(self.config_dir, 'sample-renewal.conf') args = ["renew", "--dry-run"] _, _, stdout = self._test_renewal_common(True, [], args=args, should_renew=True) out = stdout.getvalue() @@ -809,13 +809,13 @@ class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-me self.assertEqual("", out) def test_renew_hook_validation(self): - test_util.make_lineage(self, 'sample-renewal.conf') + test_util.make_lineage(self.config_dir, 'sample-renewal.conf') args = ["renew", "--dry-run", "--post-hook=no-such-command"] self._test_renewal_common(True, [], args=args, should_renew=False, error_expected=True) def test_renew_no_hook_validation(self): - test_util.make_lineage(self, 'sample-renewal.conf') + test_util.make_lineage(self.config_dir, 'sample-renewal.conf') args = ["renew", "--dry-run", "--post-hook=no-such-command", "--disable-hook-validation"] with mock.patch("certbot.hooks.post_hook"): @@ -825,7 +825,8 @@ class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-me @mock.patch("certbot.cli.set_by_cli") def test_ancient_webroot_renewal_conf(self, mock_set_by_cli): mock_set_by_cli.return_value = False - rc_path = test_util.make_lineage(self, 'sample-renewal-ancient.conf') + rc_path = test_util.make_lineage( + self.config_dir, 'sample-renewal-ancient.conf') args = mock.MagicMock(account=None, config_dir=self.config_dir, logs_dir=self.logs_dir, work_dir=self.work_dir, email=None, webroot_path=None) @@ -846,7 +847,7 @@ class MainTest(test_util.TempDirTestCase): # pylint: disable=too-many-public-me self._test_renewal_common(False, [], args=args, should_renew=False, error_expected=True) def test_renew_with_certname(self): - test_util.make_lineage(self, 'sample-renewal.conf') + test_util.make_lineage(self.config_dir, 'sample-renewal.conf') self._test_renewal_common(True, [], should_renew=True, args=['renew', '--dry-run', '--cert-name', 'sample-renewal']) diff --git a/certbot/tests/renewal_test.py b/certbot/tests/renewal_test.py index de3efe39c..869e6b104 100644 --- a/certbot/tests/renewal_test.py +++ b/certbot/tests/renewal_test.py @@ -21,7 +21,8 @@ class RenewalTest(util.TempDirTestCase): @mock.patch('certbot.cli.set_by_cli') def test_ancient_webroot_renewal_conf(self, mock_set_by_cli): mock_set_by_cli.return_value = False - rc_path = util.make_lineage(self, 'sample-renewal-ancient.conf') + rc_path = util.make_lineage( + self.config_dir, 'sample-renewal-ancient.conf') args = mock.MagicMock(account=None, config_dir=self.config_dir, logs_dir="logs", work_dir="work", email=None, webroot_path=None) diff --git a/certbot/tests/util.py b/certbot/tests/util.py index d58834335..76e3d5846 100644 --- a/certbot/tests/util.py +++ b/certbot/tests/util.py @@ -3,6 +3,7 @@ .. warning:: This module is not part of the public API. """ +import multiprocessing import os import pkg_resources import shutil @@ -13,12 +14,14 @@ from cryptography.hazmat.backends import default_backend from cryptography.hazmat.primitives import serialization import mock import OpenSSL +from six.moves import reload_module # pylint: disable=import-error from acme import jose from certbot import constants from certbot import interfaces from certbot import storage +from certbot import util from certbot.display import util as display_util @@ -106,12 +109,13 @@ def skip_unless(condition, reason): # pragma: no cover return lambda cls: None -def make_lineage(self, testfile): +def make_lineage(config_dir, testfile): """Creates a lineage defined by testfile. This creates the archive, live, and renewal directories if necessary and creates a simple lineage. + :param str config_dir: path to the configuration directory :param str testfile: configuration file to base the lineage on :returns: path to the renewal conf file for the created lineage @@ -121,11 +125,11 @@ def make_lineage(self, testfile): lineage_name = testfile[:-len('.conf')] conf_dir = os.path.join( - self.config_dir, constants.RENEWAL_CONFIGS_DIR) + config_dir, constants.RENEWAL_CONFIGS_DIR) archive_dir = os.path.join( - self.config_dir, constants.ARCHIVE_DIR, lineage_name) + config_dir, constants.ARCHIVE_DIR, lineage_name) live_dir = os.path.join( - self.config_dir, constants.LIVE_DIR, lineage_name) + config_dir, constants.LIVE_DIR, lineage_name) for directory in (archive_dir, conf_dir, live_dir,): if not os.path.exists(directory): @@ -140,11 +144,11 @@ def make_lineage(self, testfile): os.symlink(os.path.join(archive_dir, '{0}1.pem'.format(kind)), os.path.join(live_dir, '{0}.pem'.format(kind))) - conf_path = os.path.join(self.config_dir, conf_dir, testfile) + conf_path = os.path.join(config_dir, conf_dir, testfile) with open(vector_path(testfile)) as src: with open(conf_path, 'w') as dst: dst.writelines( - line.replace('MAGICDIR', self.config_dir) for line in src) + line.replace('MAGICDIR', config_dir) for line in src) return conf_path @@ -241,3 +245,47 @@ class TempDirTestCase(unittest.TestCase): def tearDown(self): shutil.rmtree(self.tempdir) + + +def lock_and_call(func, lock_path): + """Grab a lock for lock_path and call func. + + :param callable func: object to call after acquiring the lock + :param str lock_path: path to file or directory to lock + + """ + # Reload module to reset internal _LOCKS dictionary + reload_module(util) + + # start child and wait for it to grab the lock + cv = multiprocessing.Condition() + cv.acquire() + child_args = (cv, lock_path,) + child = multiprocessing.Process(target=hold_lock, args=child_args) + child.start() + cv.wait() + + # call func and terminate the child + func() + cv.notify() + cv.release() + child.join() + assert child.exitcode == 0 + + +def hold_lock(cv, lock_path): # pragma: no cover + """Acquire a file lock at lock_path and wait to release it. + + :param multiprocessing.Condition cv: condition for syncronization + :param str lock_path: path to the file lock + + """ + from certbot import lock + if os.path.isdir(lock_path): + my_lock = lock.lock_dir(lock_path) + else: + my_lock = lock.LockFile(lock_path) + cv.acquire() + cv.notify() + cv.wait() + my_lock.release() diff --git a/certbot/tests/util_test.py b/certbot/tests/util_test.py index f021c04cf..59a4f10b2 100644 --- a/certbot/tests/util_test.py +++ b/certbot/tests/util_test.py @@ -2,11 +2,13 @@ import argparse import errno import os +import shutil import stat import unittest import mock import six +from six.moves import reload_module # pylint: disable=import-error from certbot import errors import certbot.tests.util as test_util @@ -73,19 +75,52 @@ class ExeExistsTest(unittest.TestCase): self.assertFalse(self._call("exe")) -class MakeOrVerifyCoreDirTest(test_util.TempDirTestCase): +class LockDirUntilExit(test_util.TempDirTestCase): + """Tests for certbot.util.lock_dir_until_exit.""" + @classmethod + def _call(cls, *args, **kwargs): + from certbot.util import lock_dir_until_exit + return lock_dir_until_exit(*args, **kwargs) + + def setUp(self): + super(LockDirUntilExit, self).setUp() + # reset global state from other tests + import certbot.util + reload_module(certbot.util) + + @mock.patch('certbot.util.logger') + @mock.patch('certbot.util.atexit_register') + def test_it(self, mock_register, mock_logger): + subdir = os.path.join(self.tempdir, 'subdir') + os.mkdir(subdir) + self._call(self.tempdir) + self._call(subdir) + self._call(subdir) + + self.assertEqual(mock_register.call_count, 1) + registered_func = mock_register.call_args[0][0] + shutil.rmtree(subdir) + registered_func() # exception not raised + # logger.debug is only called once because the second call + # to lock subdir was ignored because it was already locked + self.assertEqual(mock_logger.debug.call_count, 1) + + +class SetUpCoreDirTest(test_util.TempDirTestCase): """Tests for certbot.util.make_or_verify_core_dir.""" def _call(self, *args, **kwargs): - from certbot.util import make_or_verify_core_dir - return make_or_verify_core_dir(*args, **kwargs) + from certbot.util import set_up_core_dir + return set_up_core_dir(*args, **kwargs) - def test_success(self): + @mock.patch('certbot.util.lock_dir_until_exit') + def test_success(self, mock_lock): new_dir = os.path.join(self.tempdir, 'new') self._call(new_dir, 0o700, os.geteuid(), False) self.assertTrue(os.path.exists(new_dir)) + self.assertEqual(mock_lock.call_count, 1) - @mock.patch('certbot.main.util.make_or_verify_dir') + @mock.patch('certbot.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, diff --git a/certbot/util.py b/certbot/util.py index 55a75097f..1cbef7e80 100644 --- a/certbot/util.py +++ b/certbot/util.py @@ -20,6 +20,13 @@ import configargparse from certbot import constants from certbot import errors +from certbot import lock + +try: + from collections import OrderedDict +except ImportError: # pragma: no cover + # OrderedDict was added in Python 2.7 + from ordereddict import OrderedDict # pylint: disable=import-error logger = logging.getLogger(__name__) @@ -47,6 +54,11 @@ PERM_ERR_FMT = os.linesep.join(( # Stores importing process ID to be used by atexit_register() _INITIAL_PID = os.getpid() +# Maps paths to locked directories to their lock object. All locks in +# the dict are attempted to be cleaned up at program exit. If the +# program exits before the lock is cleaned up, it is automatically +# released, but the file isn't deleted. +_LOCKS = OrderedDict() def run_script(params, log=logger.error): @@ -103,20 +115,47 @@ def exe_exists(exe): return False -def make_or_verify_core_dir(directory, mode, uid, strict): - """Make sure directory exists with proper permissions. +def lock_dir_until_exit(dir_path): + """Lock the directory at dir_path until program exit. + + :param str dir_path: path to directory + + :raises errors.LockError: if the lock is held by another process + + """ + if not _LOCKS: # this is the first lock to be released at exit + atexit_register(_release_locks) + + if dir_path not in _LOCKS: + _LOCKS[dir_path] = lock.lock_dir(dir_path) + + +def _release_locks(): + for dir_lock in six.itervalues(_LOCKS): + try: + dir_lock.release() + except: # pylint: disable=bare-except + msg = 'Exception occurred releasing lock: {0!r}'.format(dir_lock) + logger.debug(msg, exc_info=True) + + +def set_up_core_dir(directory, mode, uid, strict): + """Ensure directory exists with proper permissions and is locked. :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.LockError: if the directory cannot be locked :raises .errors.Error: if the directory cannot be made or verified """ try: make_or_verify_dir(directory, mode, uid, strict) + lock_dir_until_exit(directory) except OSError as error: + logger.debug("Exception was:", exc_info=True) raise errors.Error(PERM_ERR_FMT.format(error)) diff --git a/setup.py b/setup.py index b0cf57810..b7697dbd3 100644 --- a/setup.py +++ b/setup.py @@ -57,7 +57,10 @@ install_requires = [ # env markers cause problems with older pip and setuptools if sys.version_info < (2, 7): - install_requires.append('argparse') + install_requires.extend([ + 'argparse', + 'ordereddict', + ]) dev_extras = [ # Pin astroid==1.3.5, pylint==1.4.2 as a workaround for #289 diff --git a/tests/lock_test.py b/tests/lock_test.py new file mode 100644 index 000000000..4bb2865b4 --- /dev/null +++ b/tests/lock_test.py @@ -0,0 +1,238 @@ +"""Tests to ensure the lock order is preserved.""" +import atexit +import functools +import logging +import os +import re +import shutil +import subprocess +import sys +import tempfile + +from certbot import lock +from certbot import util + +from certbot.tests import util as test_util + + +logger = logging.getLogger(__name__) + + +def main(): + """Run the lock tests.""" + dirs, base_cmd = set_up() + for subcommand in ('certonly', 'install', 'renew', 'run',): + logger.info('Testing subcommand: %s', subcommand) + test_command(base_cmd + [subcommand], dirs) + logger.info('Lock test ran successfully.') + + +def set_up(): + """Prepare tests to be run. + + Logging is set up and temporary directories are set up to contain a + basic Certbot and Nginx configuration. The directories are returned + in the order they should be locked by Certbot. If the Nginx plugin + is expected to work on the system, the Nginx directory is included, + otherwise, it is not. + + A Certbot command is also created that uses the temporary + directories. The returned command can be used to test different + subcommands by appending the desired command to the end. + + :returns: directories and command + :rtype: `tuple` of `list` + + """ + logging.basicConfig(format='%(message)s', level=logging.INFO) + config_dir, logs_dir, work_dir, nginx_dir = set_up_dirs() + command = set_up_command(config_dir, logs_dir, work_dir, nginx_dir) + + dirs = [logs_dir, config_dir, work_dir] + # Travis and Circle CI set CI to true so we + # will always test Nginx's lock during CI + if os.environ.get('CI') == 'true' or util.exe_exists('nginx'): + dirs.append(nginx_dir) + else: + logger.warning('Skipping Nginx lock tests') + + return dirs, command + + +def set_up_dirs(): + """Set up directories for tests. + + A temporary directory is created to contain the config, log, work, + and nginx directories. A sample renewal configuration is created in + the config directory and a basic Nginx config is placed in the Nginx + directory. The temporary directory containing all of these + directories is deleted when the program exits. + + :return value: config, log, work, and nginx directories + :rtype: `tuple` of `str` + + """ + temp_dir = tempfile.mkdtemp() + logger.debug('Created temporary directory: %s', temp_dir) + atexit.register(functools.partial(shutil.rmtree, temp_dir)) + + config_dir = os.path.join(temp_dir, 'config') + logs_dir = os.path.join(temp_dir, 'logs') + work_dir = os.path.join(temp_dir, 'work') + nginx_dir = os.path.join(temp_dir, 'nginx') + + for directory in (config_dir, logs_dir, work_dir, nginx_dir,): + os.mkdir(directory) + + test_util.make_lineage(config_dir, 'sample-renewal.conf') + set_up_nginx_dir(nginx_dir) + + return config_dir, logs_dir, work_dir, nginx_dir + + +def set_up_nginx_dir(root_path): + """Create a basic Nginx configuration in nginx_dir. + + :param str root_path: where the Nginx server root should be placed + + """ + # Get the root of the git repository + repo_root = check_call('git rev-parse --show-toplevel'.split()).strip() + conf_script = os.path.join( + repo_root, 'certbot-nginx', 'tests', 'boulder-integration.conf.sh') + # boulder-integration.conf.sh uses the root environment variable as + # the Nginx server root when writing paths + os.environ['root'] = root_path + with open(os.path.join(root_path, 'nginx.conf'), 'w') as f: + f.write(check_call(['/bin/sh', conf_script])) + del os.environ['root'] + + +def set_up_command(config_dir, logs_dir, work_dir, nginx_dir): + """Build the Certbot command to run for testing. + + You can test different subcommands by appending the desired command + to the returned list. + + :param str config_dir: path to the configuration directory + :param str logs_dir: path to the logs directory + :param str work_dir: path to the work directory + :param str nginx_dir: path to the nginx directory + + :returns: certbot command to execute for testing + :rtype: `list` of `str` + + """ + return ( + 'certbot --cert-path {0} --key-path {1} --config-dir {2} ' + '--logs-dir {3} --work-dir {4} --nginx-server-root {5} --debug ' + '--force-renewal --nginx --verbose '.format( + test_util.vector_path('cert.pem'), + test_util.vector_path('rsa512_key.pem'), + config_dir, logs_dir, work_dir, nginx_dir).split()) + + +def test_command(command, directories): + """Assert Certbot acquires locks in a specific order. + + command is run repeatedly testing that Certbot acquires locks on + directories in the order they appear in the parameter directories. + + :param list command: Certbot command to execute + :param list directories: list of directories Certbot should fail + to acquire the lock on in sorted order + + """ + locks = [lock.lock_dir(directory) for directory in directories] + for dir_path, dir_lock in zip(directories, locks): + check_error(command, dir_path) + dir_lock.release() + + +def check_error(command, dir_path): + """Run command and verify it fails to acquire the lock for dir_path. + + :param str command: certbot command to run + :param str dir_path: path to directory containing the lock Certbot + should fail on + + """ + ret, out, err = subprocess_call(command) + if ret == 0: + report_failure("Certbot didn't exit with a nonzero status!", out, err) + + match = re.search("Please see the logfile '(.*)' for more details", err) + if match is not None: + # Get error output from more verbose logfile + with open(match.group(1)) as f: + err = f.read() + + pattern = 'A lock on {0}.* is held by another process'.format(dir_path) + if not re.search(pattern, err): + err_msg = 'Directory path {0} not in error output!'.format(dir_path) + report_failure(err_msg, out, err) + + +def check_call(args): + """Simple imitation of subprocess.check_call. + + This function is only available in subprocess in Python 2.7+. + + :param list args: program and it's arguments to be run + + :returns: stdout output + :rtype: str + + """ + exit_code, out, err = subprocess_call(args) + if exit_code: + report_failure('Command exited with a nonzero status!', out, err) + return out + + +def report_failure(err_msg, out, err): + """Report a subprocess failure and exit. + + :param str err_msg: error message to report + :param str out: stdout output + :param str err: stderr output + + """ + logger.fatal(err_msg) + log_output(logging.INFO, out, err) + sys.exit(err_msg) + + +def subprocess_call(args): + """Run a command with subprocess and return the result. + + :param list args: program and it's arguments to be run + + :returns: return code, stdout output, stderr output + :rtype: tuple + + """ + process = subprocess.Popen(args, stdout=subprocess.PIPE, + stderr=subprocess.PIPE, universal_newlines=True) + out, err = process.communicate() + logger.debug('Return code was %d', process.returncode) + log_output(logging.DEBUG, out, err) + return process.returncode, out, err + + +def log_output(level, out, err): + """Logs stdout and stderr output at the requested level. + + :param int level: logging level to use + :param str out: stdout output + :param str err: stderr output + + """ + if out: + logger.log(level, 'Stdout output was:\n%s', out) + if err: + logger.log(level, 'Stderr output was:\n%s', err) + + +if __name__ == "__main__": + main() diff --git a/tox.ini b/tox.ini index d393bb610..33cacf061 100644 --- a/tox.ini +++ b/tox.ini @@ -23,6 +23,7 @@ commands = nosetests -v certbot_nginx --processes=-1 pip install -e letshelp-certbot nosetests -v letshelp_certbot --processes=-1 + python tests/lock_test.py setenv = PYTHONPATH = {toxinidir} @@ -59,7 +60,7 @@ basepython = python2.7 # continue, but tox return code will reflect previous error commands = pip install -q -e acme[dev] -e .[dev] -e certbot-apache -e certbot-nginx -e certbot-compatibility-test -e letshelp-certbot - pylint --reports=n --rcfile=.pylintrc acme/acme certbot certbot-apache/certbot_apache certbot-nginx/certbot_nginx certbot-compatibility-test/certbot_compatibility_test letshelp-certbot/letshelp_certbot + pylint --reports=n --rcfile=.pylintrc acme/acme certbot certbot-apache/certbot_apache certbot-nginx/certbot_nginx certbot-compatibility-test/certbot_compatibility_test letshelp-certbot/letshelp_certbot tests/lock_test.py [testenv:mypy] basepython = python3.4