From 768c7cd9c09dc3161490934d3b69ace9c72937af Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Sun, 22 Nov 2015 15:16:50 +0100 Subject: [PATCH 1/6] Fix webroot permissions Take them from the parent directory where the webroot is.Should fix issue #1389 --- letsencrypt/plugins/webroot.py | 11 +++++++++++ letsencrypt/plugins/webroot_test.py | 12 ++++++++++++ 2 files changed, 23 insertions(+) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 4e18f5ca2..4686360a7 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -41,6 +41,7 @@ and reload your daemon. import errno import logging import os +import stat import zope.interface @@ -98,6 +99,16 @@ to serve all files under specified web root ({0}).""" self.full_roots[name]) try: os.makedirs(self.full_roots[name]) + # Set permissions as parent directory (GH #1389) + filemode = stat.S_IMODE(os.stat(path).st_mode) + os.chmod(self.full_roots[name]) + + # Make permissions valid for files, too + for root, dirs, files in os.walk(self.full_roots[name]): + for filename in files: + # No need for exec permissions + os.chmod(filename, filemode & ~stat.S_IEXEC) + except OSError as exception: if exception.errno != errno.EEXIST: raise errors.PluginError( diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index 902f74e9f..897c6993f 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -3,6 +3,7 @@ import os import shutil import tempfile import unittest +import stat import mock @@ -69,6 +70,17 @@ class AuthenticatorTest(unittest.TestCase): self.assertRaises(errors.PluginError, self.auth.prepare) os.chmod(self.path, 0o700) + def test_prepare_permissions(self): + + # Remove exec bit from permission check, so that it + # matches the file + parent_permissions = (stat.S_IMODE(os.stat(self.path)) & + ~stat.S_IEXEC) + + actual_permissions = stat.S_IMODE(os.stat(self.validation_path)) + + self.assertEqual(parent_permissions, actual_permissions) + def test_perform_cleanup(self): responses = self.auth.perform([self.achall]) self.assertEqual(1, len(responses)) From a71c3ed90cae9e31861c22dbd51e9b661737f29f Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Tue, 24 Nov 2015 10:13:46 +0100 Subject: [PATCH 2/6] Fix issues from review - Put chmod argument to os.chmod (oops) - Add permissions adjustments for challenge files, too --- letsencrypt/plugins/webroot.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 4686360a7..61eb87a88 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -101,13 +101,7 @@ to serve all files under specified web root ({0}).""" os.makedirs(self.full_roots[name]) # Set permissions as parent directory (GH #1389) filemode = stat.S_IMODE(os.stat(path).st_mode) - os.chmod(self.full_roots[name]) - - # Make permissions valid for files, too - for root, dirs, files in os.walk(self.full_roots[name]): - for filename in files: - # No need for exec permissions - os.chmod(filename, filemode & ~stat.S_IEXEC) + os.chmod(self.full_roots[name], filemode) except OSError as exception: if exception.errno != errno.EEXIST: @@ -132,6 +126,13 @@ to serve all files under specified web root ({0}).""" 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] + filemode = stat.S_IMODE(os.stat(parent_path).st_mode) + # Remove execution bit (not needed for this file) + os.chmod(path, filemode & ~stat.S_IEXEC) + return response def cleanup(self, achalls): # pylint: disable=missing-docstring From c7c1808ad1b29ec01b19057eaed9e5014cb9ff0d Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Tue, 24 Nov 2015 10:14:35 +0100 Subject: [PATCH 3/6] Add unit tests for webroot permissions handling Tested, pass. --- letsencrypt/plugins/webroot_test.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index 897c6993f..3fa7f2994 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -74,10 +74,11 @@ class AuthenticatorTest(unittest.TestCase): # Remove exec bit from permission check, so that it # matches the file - parent_permissions = (stat.S_IMODE(os.stat(self.path)) & + responses = self.auth.perform([self.achall]) + parent_permissions = (stat.S_IMODE(os.stat(self.path).st_mode) & ~stat.S_IEXEC) - actual_permissions = stat.S_IMODE(os.stat(self.validation_path)) + actual_permissions = stat.S_IMODE(os.stat(self.validation_path).st_mode) self.assertEqual(parent_permissions, actual_permissions) From a58c939c8df2857939c1088a713b1ad1b6bf3a6f Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Wed, 25 Nov 2015 14:26:00 +0100 Subject: [PATCH 4/6] Change ownership of the validation paths as well Match them with the parent directory they're in. --- letsencrypt/plugins/webroot.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/letsencrypt/plugins/webroot.py b/letsencrypt/plugins/webroot.py index 61eb87a88..cd13d1810 100644 --- a/letsencrypt/plugins/webroot.py +++ b/letsencrypt/plugins/webroot.py @@ -100,8 +100,15 @@ to serve all files under specified web root ({0}).""" try: os.makedirs(self.full_roots[name]) # Set permissions as parent directory (GH #1389) - filemode = stat.S_IMODE(os.stat(path).st_mode) + # We don't use the parameters in makedirs because it + # may not always work + # https://stackoverflow.com/questions/5231901/permission-problems-when-creating-a-dir-with-os-makedirs-python + 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) except OSError as exception: if exception.errno != errno.EEXIST: @@ -129,9 +136,11 @@ to serve all files under specified web root ({0}).""" # Set permissions as parent directory (GH #1389) parent_path = self.full_roots[achall.domain] - filemode = stat.S_IMODE(os.stat(parent_path).st_mode) + 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) return response From 2a5f539d9a830f0ace1a38a707e3dba2b232bc4f Mon Sep 17 00:00:00 2001 From: Luca Beltrame Date: Wed, 25 Nov 2015 14:26:51 +0100 Subject: [PATCH 5/6] Add tests for testing gid and uid with the webroot plugin They pass. --- letsencrypt/plugins/webroot_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index 3fa7f2994..862921d1d 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -81,6 +81,11 @@ class AuthenticatorTest(unittest.TestCase): actual_permissions = stat.S_IMODE(os.stat(self.validation_path).st_mode) self.assertEqual(parent_permissions, actual_permissions) + parent_gid = os.stat(self.path).st_gid + parent_uid = os.stat(self.path).st_uid + + self.assertEqual(os.stat(self.validation_path).st_gid, parent_gid) + self.assertEqual(os.stat(self.validation_path).st_uid, parent_uid) def test_perform_cleanup(self): responses = self.auth.perform([self.achall]) From 02d93e995a6d6a845282d240ef3c344a33eab7c8 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Tue, 1 Dec 2015 19:24:14 -0800 Subject: [PATCH 6/6] lint --- letsencrypt/plugins/webroot_test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/plugins/webroot_test.py b/letsencrypt/plugins/webroot_test.py index 862921d1d..e7f96b50d 100644 --- a/letsencrypt/plugins/webroot_test.py +++ b/letsencrypt/plugins/webroot_test.py @@ -74,7 +74,7 @@ class AuthenticatorTest(unittest.TestCase): # Remove exec bit from permission check, so that it # matches the file - responses = self.auth.perform([self.achall]) + self.auth.perform([self.achall]) parent_permissions = (stat.S_IMODE(os.stat(self.path).st_mode) & ~stat.S_IEXEC)