From edf3a4ed732e1123cf257bd9c999c7ca6e468fb4 Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Mon, 7 Dec 2015 10:49:24 +0100 Subject: [PATCH 1/6] Make webroot usable also when running as non-root (GH #1795) Thanks to @aburch's suggestions, the logic has been changed: - Set umask before creating folders and files - Leverage os.makedirs' mode option in conjunction with umask The program still tries to change owner / group, but in case of errors it continues gracefully. Tests have been updated, and they pass. --- letsencrypt/plugins/webroot.py | 51 +++++++++++++++++++---------- letsencrypt/plugins/webroot_test.py | 13 +++++--- 2 files changed, 42 insertions(+), 22 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 0b81d45b5..392d1fc2c 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -60,23 +60,38 @@ to serve all files under specified web root ({0}).""" logger.debug("Creating root challenges validation dir at %s", self.full_roots[name]) try: - os.makedirs(self.full_roots[name]) - # Set permissions as parent directory (GH #1389) - # We don't use the parameters in makedirs because it - # may not always work + # Change the permissiosn to be writable (GH #1389) + # We also set umask because os.makedirs's mode parameter does + # not always work: # https://stackoverflow.com/questions/5231901/permission-problems-when-creating-a-dir-with-os-makedirs-python + # We set the umask instead of going the chmod route to ensure the client + # can also run as non-root (GH #1795) + stat_path = os.stat(path) - filemode = stat.S_IMODE(stat_path.st_mode) - os.chmod(self.full_roots[name], filemode) - # Set owner and group, too - os.chown(self.full_roots[name], stat_path.st_uid, - stat_path.st_gid) + old_umask = os.umask(0o022) + os.makedirs(self.full_roots[name], 0o0755) + + # Set owner as parent directory if possible + + try: + stat_path = os.stat(path) + os.chown(self.full_roots[name], stat_path.st_uid, + stat_path.st_gid) + except OSError as exception: + if exception.errno == errno.EACCES: + logger.debug("Insufficient permissions to change owner and uid - ignoring") + else: + raise errors.PluginError( + "Couldn't create root for {0} http-01 " + "challenge responses: {1}", name, exception) except OSError as exception: if exception.errno != errno.EEXIST: raise errors.PluginError( "Couldn't create root for {0} http-01 " "challenge responses: {1}", name, exception) + finally: + os.umask(old_umask) def perform(self, achalls): # pylint: disable=missing-docstring assert self.full_roots, "Webroot plugin appears to be missing webroot map" @@ -95,18 +110,18 @@ to serve all files under specified web root ({0}).""" 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) - with open(path, "w") as validation_file: - validation_file.write(validation.encode()) - # Set permissions as parent directory (GH #1389) - parent_path = self.full_roots[achall.domain] - stat_parent_path = os.stat(parent_path) - filemode = stat.S_IMODE(stat_parent_path.st_mode) - # Remove execution bit (not needed for this file) - os.chmod(path, filemode & ~stat.S_IEXEC) - os.chown(path, stat_parent_path.st_uid, stat_parent_path.st_gid) + old_umask = os.umask(0o022) + + try: + with open(path, "w") as validation_file: + # Change permissions to be world-readable, owner-writable (GH #1795) + validation_file.write(validation.encode()) + finally: + os.umask(old_umask) return response diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index e7f96b50d..2e88c1756 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -75,12 +75,17 @@ class AuthenticatorTest(unittest.TestCase): # Remove exec bit from permission check, so that it # matches the file self.auth.perform([self.achall]) - parent_permissions = (stat.S_IMODE(os.stat(self.path).st_mode) & - ~stat.S_IEXEC) + path_permissions = stat.S_IMODE(os.stat(self.validation_path).st_mode) + self.assertEqual(path_permissions, 0o644) - actual_permissions = stat.S_IMODE(os.stat(self.validation_path).st_mode) + # Check permissions of the directories + + for dirpath, dirnames, filenames in os.walk(self.path): + for directory in dirnames: + full_path = os.path.join(dirpath, directory) + dir_permissions = stat.S_IMODE(os.stat(full_path).st_mode) + self.assertEqual(dir_permissions, 0o755) - self.assertEqual(parent_permissions, actual_permissions) parent_gid = os.stat(self.path).st_gid parent_uid = os.stat(self.path).st_uid From 2b942d97b27eab9ef8f5fee263a5dbbd95b432f8 Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Mon, 7 Dec 2015 11:17:29 +0100 Subject: [PATCH 2/6] Address review comments - move the umask call before the try/except block - move comment in _prepare_single to the umask call Simplify the code comments, too. Tests still pass. --- letsencrypt/plugins/webroot.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 392d1fc2c..c4072c3f9 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -59,16 +59,19 @@ to serve all files under specified web root ({0}).""" logger.debug("Creating root challenges validation dir at %s", self.full_roots[name]) + + # Change the permissiosn to be writable (GH #1389) + # Umask is used instead of chmod to ensure the client can also + # run as non-root (GH #1795) + old_umask = os.umask(0o022) + try: - # Change the permissiosn to be writable (GH #1389) - # We also set umask because os.makedirs's mode parameter does - # not always work: - # https://stackoverflow.com/questions/5231901/permission-problems-when-creating-a-dir-with-os-makedirs-python - # We set the umask instead of going the chmod route to ensure the client - # can also run as non-root (GH #1795) stat_path = os.stat(path) - old_umask = os.umask(0o022) + # This is coupled with the "umask" call above because + # os.makedirs's "mode" parameter may not always work: + # https://stackoverflow.com/questions/5231901/permission-problems-when-creating-a-dir-with-os-makedirs-python + os.makedirs(self.full_roots[name], 0o0755) # Set owner as parent directory if possible @@ -114,11 +117,11 @@ to serve all files under specified web root ({0}).""" path = self._path_for_achall(achall) logger.debug("Attempting to save validation to %s", path) + # Change permissions to be world-readable, owner-writable (GH #1795) old_umask = os.umask(0o022) try: with open(path, "w") as validation_file: - # Change permissions to be world-readable, owner-writable (GH #1795) validation_file.write(validation.encode()) finally: os.umask(old_umask) From 74927613e9e9f9fa02f0dfbe2a2f75707a850b5d Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 11 Dec 2015 18:03:52 -0800 Subject: [PATCH 3/6] Fixed lint issues --- letsencrypt/plugins/webroot.py | 5 ++--- letsencrypt/plugins/webroot_test.py | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index c4072c3f9..b4c877c78 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -2,7 +2,6 @@ import errno import logging import os -import stat import zope.interface @@ -105,10 +104,10 @@ to serve all files under specified web root ({0}).""" path = self.full_roots[achall.domain] except IndexError: raise errors.PluginError("Missing --webroot-path for domain: {1}" - .format(achall.domain)) + .format(achall.domain)) if not os.path.exists(path): raise errors.PluginError("Mysteriously missing path {0} for domain: {1}" - .format(path, achall.domain)) + .format(path, achall.domain)) return os.path.join(path, achall.chall.encode("token")) def _perform_single(self, achall): diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index 2e88c1756..2ebbf01a6 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -48,7 +48,7 @@ class AuthenticatorTest(unittest.TestCase): def test_add_parser_arguments(self): add = mock.MagicMock() self.auth.add_parser_arguments(add) - self.assertEqual(0, add.call_count) # became 0 when we moved the args to cli.py! + self.assertEqual(0, add.call_count) # args moved to cli.py! def test_prepare_bad_root(self): self.config.webroot_path = os.path.join(self.path, "null") @@ -80,7 +80,7 @@ class AuthenticatorTest(unittest.TestCase): # Check permissions of the directories - for dirpath, dirnames, filenames in os.walk(self.path): + for dirpath, dirnames, _ in os.walk(self.path): for directory in dirnames: full_path = os.path.join(dirpath, directory) dir_permissions = stat.S_IMODE(os.stat(full_path).st_mode) From 2d525594663ecf5037cd31a4b6f67bcb54d2e516 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 11 Dec 2015 18:12:46 -0800 Subject: [PATCH 4/6] Cleanup comment --- letsencrypt/plugins/webroot.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index b4c877c78..075930c8b 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -59,8 +59,8 @@ to serve all files under specified web root ({0}).""" logger.debug("Creating root challenges validation dir at %s", self.full_roots[name]) - # Change the permissiosn to be writable (GH #1389) - # Umask is used instead of chmod to ensure the client can also + # Change the permissions to be writable (GH #1389) + # Umask is used instead of chmod to ensure the client can also # run as non-root (GH #1795) old_umask = os.umask(0o022) From d45865a60110f9df383b58d307b06cea6f7cb42a Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 11 Dec 2015 19:14:23 -0800 Subject: [PATCH 5/6] Cleanup --- letsencrypt/plugins/webroot.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 075930c8b..0679bc349 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -65,16 +65,12 @@ to serve all files under specified web root ({0}).""" old_umask = os.umask(0o022) try: - - stat_path = os.stat(path) # This is coupled with the "umask" call above because # os.makedirs's "mode" parameter may not always work: # https://stackoverflow.com/questions/5231901/permission-problems-when-creating-a-dir-with-os-makedirs-python - os.makedirs(self.full_roots[name], 0o0755) # Set owner as parent directory if possible - try: stat_path = os.stat(path) os.chown(self.full_roots[name], stat_path.st_uid, From 1a7dd76288204a6bc1c979149162fa5440b04156 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 11 Dec 2015 19:31:50 -0800 Subject: [PATCH 6/6] Added test coverage --- letsencrypt/plugins/webroot_test.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index 2ebbf01a6..9f5b6bba8 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -1,9 +1,10 @@ """Tests for letsencrypt.plugins.webroot.""" +import errno import os import shutil +import stat import tempfile import unittest -import stat import mock @@ -35,7 +36,6 @@ class AuthenticatorTest(unittest.TestCase): self.config = mock.MagicMock(webroot_path=self.path, webroot_map={"thing.com": self.path}) self.auth = Authenticator(self.config, "webroot") - self.auth.prepare() def tearDown(self): shutil.rmtree(self.path) @@ -70,7 +70,18 @@ class AuthenticatorTest(unittest.TestCase): self.assertRaises(errors.PluginError, self.auth.prepare) os.chmod(self.path, 0o700) + @mock.patch("letsencrypt.plugins.webroot.os.chown") + def test_failed_chown_eacces(self, mock_chown): + mock_chown.side_effect = OSError(errno.EACCES, "msg") + self.auth.prepare() # exception caught and logged + + @mock.patch("letsencrypt.plugins.webroot.os.chown") + def test_failed_chown_not_eacces(self, mock_chown): + mock_chown.side_effect = OSError() + self.assertRaises(errors.PluginError, self.auth.prepare) + def test_prepare_permissions(self): + self.auth.prepare() # Remove exec bit from permission check, so that it # matches the file @@ -93,6 +104,7 @@ class AuthenticatorTest(unittest.TestCase): self.assertEqual(os.stat(self.validation_path).st_uid, parent_uid) def test_perform_cleanup(self): + self.auth.prepare() responses = self.auth.perform([self.achall]) self.assertEqual(1, len(responses)) self.assertTrue(os.path.exists(self.validation_path))