Add lockfile (#4449)

* add lock_file

* cleanup lock file

* Add LockFile tests

* add lock_dir

* add lock_dir_until_exit

* add set_up_core_dir and move lock_dir_until_exit

* Move lock_and_call to certbot.test.util

* Add lock to Apache

* Add lock to the Nginx plugin

* Improve permissions error message

* sort plugins

* add test_prepare_order

* provide more actionable permissions error

* Document and catch use of OSError

* don't lock a directory twice

* add conditional dependency on ordereddict

* Add lock_test

* expand sorted plugins comment

* Add lock_test to lint

* make make_lineage more conventional and flexible

* enhance lock_test.py

* add lock_test to tox

* Readd success message

* make py26 happy

* add test_acquire_without_deletion

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',

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

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

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):

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', [], [])

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."""

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

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:

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):

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):

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__":

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

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'])
 

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)

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()

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,

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))
 
 

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

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()

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