From 69e1c6285989bda8dbb88d23e1b37085e51210f4 Mon Sep 17 00:00:00 2001 From: Filip Ochnik Date: Tue, 16 Feb 2016 20:33:22 +0800 Subject: [PATCH 1/5] Add test for cleaning up acme-challenge --- letsencrypt/plugins/webroot_test.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index e3f926c7f..4899c94e6 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -30,8 +30,10 @@ class AuthenticatorTest(unittest.TestCase): def setUp(self): from letsencrypt.plugins.webroot import Authenticator self.path = tempfile.mkdtemp() + self.root_challenge_path = os.path.join( + self.path, ".well-known", "acme-challenge") self.validation_path = os.path.join( - self.path, ".well-known", "acme-challenge", + self.root_challenge_path, "ZXZhR3hmQURzNnBTUmIyTEF2OUlaZjE3RHQzanV4R0orUEN0OTJ3citvQQ") self.config = mock.MagicMock(webroot_path=self.path, webroot_map={"thing.com": self.path}) @@ -137,6 +139,7 @@ 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)) if __name__ == "__main__": From 780c9ce2aedef97e85135c86816e8ccb093f4dc4 Mon Sep 17 00:00:00 2001 From: Filip Ochnik Date: Tue, 16 Feb 2016 20:36:46 +0800 Subject: [PATCH 2/5] Refactor path logic in webroot plugin --- letsencrypt/plugins/webroot.py | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 3f5bc6d28..82a6e20a7 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -97,7 +97,7 @@ to serve all files under specified web root ({0}).""" assert self.full_roots, "Webroot plugin appears to be missing webroot map" return [self._perform_single(achall) for achall in achalls] - def _path_for_achall(self, achall): + def _get_root_path(self, achall): try: path = self.full_roots[achall.domain] except KeyError: @@ -106,19 +106,23 @@ to serve all files under specified web root ({0}).""" if not os.path.exists(path): raise errors.PluginError("Mysteriously missing path {0} for domain: {1}" .format(path, achall.domain)) - return os.path.join(path, achall.chall.encode("token")) + return path + + def _get_validation_path(self, root_path, achall): + return os.path.join(root_path, achall.chall.encode("token")) def _perform_single(self, achall): response, validation = achall.response_and_validation() - path = self._path_for_achall(achall) - logger.debug("Attempting to save validation to %s", path) + root_path = self._get_root_path(achall) + validation_path = self._get_validation_path(root_path, achall) + logger.debug("Attempting to save validation to %s", validation_path) # Change permissions to be world-readable, owner-writable (GH #1795) old_umask = os.umask(0o022) try: - with open(path, "w") as validation_file: + with open(validation_path, "w") as validation_file: validation_file.write(validation.encode()) finally: os.umask(old_umask) @@ -127,6 +131,7 @@ to serve all files under specified web root ({0}).""" def cleanup(self, achalls): # pylint: disable=missing-docstring for achall in achalls: - path = self._path_for_achall(achall) - logger.debug("Removing %s", path) - os.remove(path) + root_path = self._get_root_path(achall) + validation_path = self._get_validation_path(root_path, achall) + logger.debug("Removing %s", validation_path) + os.remove(validation_path) From 9b5ff7bcd725592b97fcd86521f99bb5b46eb97f Mon Sep 17 00:00:00 2001 From: Filip Ochnik Date: Tue, 16 Feb 2016 20:46:42 +0800 Subject: [PATCH 3/5] Remove acme-challenge after cleaning up all challenges --- letsencrypt/plugins/webroot.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 82a6e20a7..b774a39ed 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -2,8 +2,10 @@ import errno import logging import os +from collections import defaultdict import zope.interface +import six from acme import challenges @@ -44,6 +46,7 @@ to serve all files under specified web root ({0}).""" def __init__(self, *args, **kwargs): super(Authenticator, self).__init__(*args, **kwargs) self.full_roots = {} + self.performed = defaultdict(set) def prepare(self): # pylint: disable=missing-docstring path_map = self.conf("map") @@ -127,6 +130,8 @@ to serve all files under specified web root ({0}).""" finally: os.umask(old_umask) + self.performed[root_path].add(achall) + return response def cleanup(self, achalls): # pylint: disable=missing-docstring @@ -135,3 +140,10 @@ to serve all files under specified web root ({0}).""" validation_path = self._get_validation_path(root_path, achall) logger.debug("Removing %s", validation_path) os.remove(validation_path) + self.performed[root_path].remove(achall) + + for root_path, achalls in six.iteritems(self.performed): + if not achalls: + logger.debug("All challenges cleaned up, removing %s", + root_path) + os.rmdir(root_path) From 4d9f487e893de6c40136078dbdae0d56c2cbb264 Mon Sep 17 00:00:00 2001 From: Filip Ochnik Date: Wed, 17 Feb 2016 17:04:10 +0800 Subject: [PATCH 4/5] Handle rmdir failure --- letsencrypt/plugins/webroot.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index b774a39ed..49f779bb8 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -144,6 +144,13 @@ to serve all files under specified web root ({0}).""" for root_path, achalls in six.iteritems(self.performed): if not achalls: - logger.debug("All challenges cleaned up, removing %s", - root_path) - os.rmdir(root_path) + try: + os.rmdir(root_path) + logger.debug("All challenges cleaned up, removing %s", + root_path) + except OSError as exc: + if exc.errno == errno.ENOTEMPTY: + logger.debug("Challenges cleaned up but %s not empty", + root_path) + else: + raise From 0554163ff974b2b2385dd82e57892d1119a3e7fa Mon Sep 17 00:00:00 2001 From: Filip Ochnik Date: Wed, 17 Feb 2016 17:21:25 +0800 Subject: [PATCH 5/5] Additional tests for webroot cleanup --- letsencrypt/plugins/webroot_test.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index 4899c94e6..7a34b3fcc 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -141,6 +141,32 @@ class AuthenticatorTest(unittest.TestCase): 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() + self.auth.perform([self.achall]) + + leftover_path = os.path.join(self.root_challenge_path, 'leftover') + os.mkdir(leftover_path) + + self.auth.cleanup([self.achall]) + self.assertFalse(os.path.exists(self.validation_path)) + self.assertTrue(os.path.exists(self.root_challenge_path)) + + os.rmdir(leftover_path) + + @mock.patch('os.rmdir') + def test_cleanup_oserror(self, mock_rmdir): + self.auth.prepare() + self.auth.perform([self.achall]) + + os_error = OSError() + os_error.errno = errno.EACCES + mock_rmdir.side_effect = os_error + + self.assertRaises(OSError, self.auth.cleanup, [self.achall]) + self.assertFalse(os.path.exists(self.validation_path)) + self.assertTrue(os.path.exists(self.root_challenge_path)) + if __name__ == "__main__": unittest.main() # pragma: no cover