Revert "Proper webroot directory cleanup (#5453)" (#5574)

This reverts commit ad0a99a1f5.

certbot/plugins/util.py

@@ -6,24 +6,6 @@ from certbot import util
 
 logger = logging.getLogger(__name__)
 
-def get_prefixes(path):
-    """Retrieves all possible path prefixes of a path, in descending order
-    of length. For instance,
-        /a/b/c/ => ['/a/b/c/', '/a/b/c', '/a/b', '/a', '/']
-    :param str path: the path to break into prefixes
-
-    :returns: all possible path prefixes of given path in descending order
-    :rtype: `list` of `str`
-    """
-    prefix = path
-    prefixes = []
-    while len(prefix) > 0:
-        prefixes.append(prefix)
-        prefix, _ = os.path.split(prefix)
-        # break once we hit '/'
-        if prefix == prefixes[-1]:
-            break
-    return prefixes
 
 def path_surgery(cmd):
     """Attempt to perform PATH surgery to find cmd

certbot/plugins/util_test.py

@@ -5,14 +5,6 @@ import unittest
 import mock
 
 
-class GetPrefixTest(unittest.TestCase):
-    """Tests for certbot.plugins.get_prefixes."""
-    def test_get_prefix(self):
-        from certbot.plugins.util import get_prefixes
-        self.assertEqual(get_prefixes("/a/b/c/"), ['/a/b/c/', '/a/b/c', '/a/b', '/a', '/'])
-        self.assertEqual(get_prefixes("/"), ["/"])
-        self.assertEqual(get_prefixes("a"), ["a"])
-
 class PathSurgeryTest(unittest.TestCase):
     """Tests for certbot.plugins.path_surgery."""
 

certbot/plugins/webroot.py

@@ -18,7 +18,6 @@ from certbot import interfaces
 from certbot.display import util as display_util
 from certbot.display import ops
 from certbot.plugins import common
-from certbot.plugins import util
 
 
 logger = logging.getLogger(__name__)
@@ -66,8 +65,6 @@ to serve all files under specified web root ({0})."""
         super(Authenticator, self).__init__(*args, **kwargs)
         self.full_roots = {}
         self.performed = collections.defaultdict(set)
-        # stack of dirs successfully created by this authenticator
-        self._created_dirs = []
 
     def prepare(self):  # pylint: disable=missing-docstring
         pass
@@ -164,26 +161,27 @@ to serve all files under specified web root ({0})."""
             # Umask is used instead of chmod to ensure the client can also
             # run as non-root (GH #1795)
             old_umask = os.umask(0o022)
+
             try:
-                stat_path = os.stat(path)
+                # This is coupled with the "umask" call above because
-                for prefix in sorted(util.get_prefixes(self.full_roots[name]), key=len):
+                # os.makedirs's "mode" parameter may not always work:
-                    try:
+                # https://stackoverflow.com/questions/5231901/permission-problems-when-creating-a-dir-with-os-makedirs-python
-                        # This is coupled with the "umask" call above because
+                os.makedirs(self.full_roots[name], 0o0755)
-                        # os.mkdir's "mode" parameter may not always work:
+
-                        # https://docs.python.org/3/library/os.html#os.mkdir
+                # Set owner as parent directory if possible
-                        os.mkdir(prefix, 0o0755)
+                try:
-                        self._created_dirs.append(prefix)
+                    stat_path = os.stat(path)
-                        # Set owner as parent directory if possible
+                    os.chown(self.full_roots[name], stat_path.st_uid,
-                        try:
+                             stat_path.st_gid)
-                            os.chown(prefix, stat_path.st_uid, stat_path.st_gid)
+                except OSError as exception:
-                        except OSError as exception:
+                    logger.info("Unable to change owner and uid of webroot directory")
-                            logger.info("Unable to change owner and uid of webroot directory")
+                    logger.debug("Error was: %s", exception)
-                            logger.debug("Error was: %s", exception)
+
-                    except OSError as exception:
+            except OSError as exception:
-                        if exception.errno != errno.EEXIST:
+                if exception.errno != errno.EEXIST:
-                            raise errors.PluginError(
+                    raise errors.PluginError(
-                                "Couldn't create root for {0} http-01 "
+                        "Couldn't create root for {0} http-01 "
-                                "challenge responses: {1}", name, exception)
+                        "challenge responses: {1}", name, exception)
             finally:
                 os.umask(old_umask)
 
@@ -219,17 +217,16 @@ to serve all files under specified web root ({0})."""
                 os.remove(validation_path)
                 self.performed[root_path].remove(achall)
 
-        not_removed = []
+        for root_path, achalls in six.iteritems(self.performed):
-        while len(self._created_dirs) > 0:
+            if not achalls:
-            path = self._created_dirs.pop()
+                try:
-            try:
+                    os.rmdir(root_path)
-                os.rmdir(path)
+                    logger.debug("All challenges cleaned up, removing %s",
-            except OSError as exc:
+                                 root_path)
-                not_removed.insert(0, path)
+                except OSError as exc:
-                logger.info("Challenge directory %s was not empty, didn't remove", path)
+                    logger.info(
-                logger.debug("Error was: %s", exc)
+                        "Unable to clean up challenge directory %s", root_path)
-        self._created_dirs = not_removed
+                    logger.debug("Error was: %s", exc)
-        logger.debug("All challenges cleaned up")
 
 
 class _WebrootMapAction(argparse.Action):

certbot/plugins/webroot_test.py

@@ -36,8 +36,6 @@ class AuthenticatorTest(unittest.TestCase):
     def setUp(self):
         from certbot.plugins.webroot import Authenticator
         self.path = tempfile.mkdtemp()
-        self.partial_root_challenge_path = os.path.join(
-            self.path, ".well-known")
         self.root_challenge_path = os.path.join(
             self.path, ".well-known", "acme-challenge")
         self.validation_path = os.path.join(
@@ -201,35 +199,6 @@ class AuthenticatorTest(unittest.TestCase):
         self.auth.cleanup([self.achall])
         self.assertFalse(os.path.exists(self.validation_path))
         self.assertFalse(os.path.exists(self.root_challenge_path))
-        self.assertFalse(os.path.exists(self.partial_root_challenge_path))
-
-    def test_perform_cleanup_existing_dirs(self):
-        os.mkdir(self.partial_root_challenge_path)
-        self.auth.prepare()
-        self.auth.perform([self.achall])
-        self.auth.cleanup([self.achall])
-
-        # Ensure we don't "clean up" directories that previously existed
-        self.assertFalse(os.path.exists(self.validation_path))
-        self.assertFalse(os.path.exists(self.root_challenge_path))
-
-    def test_perform_cleanup_multiple_challenges(self):
-        bingo_achall = achallenges.KeyAuthorizationAnnotatedChallenge(
-            challb=acme_util.chall_to_challb(
-                challenges.HTTP01(token=b"bingo"), "pending"),
-            domain="thing.com", account_key=KEY)
-
-        bingo_validation_path = "YmluZ28"
-        os.mkdir(self.partial_root_challenge_path)
-        self.auth.prepare()
-        self.auth.perform([bingo_achall, self.achall])
-
-        self.auth.cleanup([self.achall])
-        self.assertFalse(os.path.exists(bingo_validation_path))
-        self.assertTrue(os.path.exists(self.root_challenge_path))
-        self.auth.cleanup([bingo_achall])
-        self.assertFalse(os.path.exists(self.validation_path))
-        self.assertFalse(os.path.exists(self.root_challenge_path))
 
     def test_cleanup_leftovers(self):
         self.auth.prepare()