From 69316d827ae8de19ab5321b2c171b900f917ac9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Thu, 31 Mar 2016 09:27:22 +0200 Subject: [PATCH 1/3] [util] Remove umask manipulation from makedirs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This causes a race condition: umask is process-wide setting, so any file created by another thread while the umask is set to 0 will have wrong permissions. This also removes the possible problem of not resetting the umask if exception is raised. Fixes: #239 Signed-off-by: Lubomír Sedlář --- pungi/util.py | 2 -- tests/test_util.py | 15 +++++++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pungi/util.py b/pungi/util.py index 857174ea..f4f5e335 100644 --- a/pungi/util.py +++ b/pungi/util.py @@ -138,13 +138,11 @@ def _doCheckSum(path, hash, logger): def makedirs(path, mode=0o775): - mask = os.umask(0) try: os.makedirs(path, mode=mode) except OSError as ex: if ex.errno != errno.EEXIST: raise - os.umask(mask) def rmtree(path, ignore_errors=False, onerror=None): diff --git a/tests/test_util.py b/tests/test_util.py index d730ba4c..97892b71 100755 --- a/tests/test_util.py +++ b/tests/test_util.py @@ -13,7 +13,7 @@ sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) from pungi import compose from pungi import util -from tests.helpers import touch +from tests.helpers import touch, PungiTestCase class TestGitRefResolver(unittest.TestCase): @@ -193,7 +193,7 @@ class TestFindOldCompose(unittest.TestCase): self.assertEqual(old, self.tmp_dir + '/Fedora-Rawhide-Base-1-20160229.0') -class TestHelpers(unittest.TestCase): +class TestHelpers(PungiTestCase): def test_process_args(self): self.assertEqual(util.process_args('--opt={}', None), []) self.assertEqual(util.process_args('--opt={}', []), []) @@ -201,6 +201,17 @@ class TestHelpers(unittest.TestCase): ['--opt=foo', '--opt=bar']) self.assertEqual(util.process_args('--opt={}', 'foo'), ['--opt=foo']) + def test_makedirs(self): + util.makedirs(self.topdir + '/foo/bar/baz') + self.assertTrue(os.path.isdir(self.topdir + '/foo/bar/baz')) + + def test_makedirs_on_existing(self): + os.makedirs(self.topdir + '/foo/bar/baz') + try: + util.makedirs(self.topdir + '/foo/bar/baz') + except OSError: + self.fail('makedirs raised exception on existing directory') + if __name__ == "__main__": unittest.main() From 490514e263521b60f955cbef09cf8c0fa3aeb3fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Thu, 31 Mar 2016 09:45:50 +0200 Subject: [PATCH 2/3] [checks] Add a check for too restrictive umask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If umask is set to something too high (>0022), a warning will be printed. It does not abort the compose though. Signed-off-by: Lubomír Sedlář --- bin/pungi-koji | 1 + pungi/checks.py | 10 ++++++++++ tests/test_checks.py | 31 +++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/bin/pungi-koji b/bin/pungi-koji index 1044ec1a..a0eba0e1 100755 --- a/bin/pungi-koji +++ b/bin/pungi-koji @@ -172,6 +172,7 @@ def main(): import pungi.checks if not pungi.checks.check(conf): sys.exit(1) + pungi.checks.check_umask(logger) if opts.target_dir: compose_dir = Compose.get_compose_dir(opts.target_dir, conf, compose_type=compose_type, compose_label=opts.label) diff --git a/pungi/checks.py b/pungi/checks.py index 6895b2ab..f0d8b5bc 100644 --- a/pungi/checks.py +++ b/pungi/checks.py @@ -102,6 +102,16 @@ def check(conf): return not fail +def check_umask(logger): + """Make sure umask is set to something reasonable. If not, log a warning.""" + mask = os.umask(0) + os.umask(mask) + + if mask > 0o022: + logger.warning('Unusually strict umask detected (0%03o), ' + 'expect files with broken permissions.', mask) + + def validate_options(conf, valid_options): errors = [] for i in valid_options: diff --git a/tests/test_checks.py b/tests/test_checks.py index a0a49e00..0e2a9e2b 100755 --- a/tests/test_checks.py +++ b/tests/test_checks.py @@ -143,5 +143,36 @@ class CheckDependenciesTestCase(unittest.TestCase): self.assertFalse(result) +class TestUmask(unittest.TestCase): + def setUp(self): + self.orig_umask = os.umask(0) + os.umask(self.orig_umask) + + def tearDown(self): + os.umask(self.orig_umask) + + def test_no_warning_with_0022(self): + os.umask(0o022) + logger = mock.Mock() + checks.check_umask(logger) + self.assertItemsEqual(logger.mock_calls, []) + + def test_no_warning_with_0000(self): + os.umask(0o000) + logger = mock.Mock() + checks.check_umask(logger) + self.assertItemsEqual(logger.mock_calls, []) + + def test_warning_with_0044(self): + os.umask(0o044) + logger = mock.Mock() + checks.check_umask(logger) + self.assertItemsEqual( + logger.mock_calls, + [mock.call.warning('Unusually strict umask detected (0%03o), ' + 'expect files with broken permissions.', 0o044)] + ) + + if __name__ == "__main__": unittest.main() From c5c22614898d62b2d08d7e995b9811c6e15658ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Thu, 31 Mar 2016 10:07:09 +0200 Subject: [PATCH 3/3] [pungi-wrapper] Remove duplicated code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is almost exact duplicate of util.makedirs. One copy should be enough. Signed-off-by: Lubomír Sedlář --- pungi/wrappers/pungi.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/pungi/wrappers/pungi.py b/pungi/wrappers/pungi.py index 4d686bb1..ccd5eb6f 100644 --- a/pungi/wrappers/pungi.py +++ b/pungi/wrappers/pungi.py @@ -15,10 +15,11 @@ # Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. -import errno import os import re +from .. import util + PACKAGES_RE = { "rpm": re.compile(r"^RPM(\((?P[^\)]+)\))?: (?:file://)?(?P/?[^ ]+)$"), @@ -43,11 +44,7 @@ class PungiWrapper(object): ks_path = os.path.abspath(ks_path) ks_dir = os.path.dirname(ks_path) - try: - os.makedirs(ks_dir) - except OSError as ex: - if ex.errno != errno.EEXIST: - raise + util.makedirs(ks_dir) kickstart = open(ks_path, "w")