From 0990f12dd9f22597991f6a08594060deff0bb755 Mon Sep 17 00:00:00 2001 From: Greg Becker Date: Wed, 5 Jun 2019 08:15:47 +0900 Subject: modules: set permissions based on package configuration (#11337) Previously, module files were not set with the same permissions as the package installation. For world-readable packages, this would not cause a problem. For group readable packages, it does: ``` packages: mypackage: permissions: group: mygroup read: group write: group ``` In this case, the modulefile is unreadable by members of the group other than the one who installed it. Add logic to the modulefile writers to set the permissions based on the configuration in `packages.yaml` --- lib/spack/spack/hooks/permissions_setters.py | 58 ++++++---------------------- lib/spack/spack/modules/common.py | 5 +++ lib/spack/spack/test/modules/common.py | 40 ++++++++++++++++++- lib/spack/spack/test/permissions.py | 8 ++-- lib/spack/spack/util/file_permissions.py | 40 +++++++++++++++++++ 5 files changed, 99 insertions(+), 52 deletions(-) create mode 100644 lib/spack/spack/util/file_permissions.py (limited to 'lib') diff --git a/lib/spack/spack/hooks/permissions_setters.py b/lib/spack/spack/hooks/permissions_setters.py index 9ada697ad8..10d75906e2 100644 --- a/lib/spack/spack/hooks/permissions_setters.py +++ b/lib/spack/spack/hooks/permissions_setters.py @@ -4,54 +4,18 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import os -import stat - -from llnl.util.filesystem import chmod_x, chgrp - -from spack.package_prefs import get_package_permissions, get_package_group -from spack.package_prefs import get_package_dir_permissions -from spack.error import SpackError - - -def forall_files(path, fn, args, dir_args=None): - """Apply function to all files in directory, with file as first arg. - - Does not apply to the root dir. Does not apply to links""" - # os.walk explicitly set not to follow links - for root, dirs, files in os.walk(path, followlinks=False): - for d in dirs: - if not os.path.islink(os.path.join(root, d)): - if dir_args: - fn(os.path.join(root, d), *dir_args) - else: - fn(os.path.join(root, d), *args) - for f in files: - if not os.path.islink(os.path.join(root, f)): - fn(os.path.join(root, f), *args) - - -def chmod_real_entries(path, perms): - # Don't follow links so we don't change things outside the prefix - if not os.path.islink(path): - mode = os.stat(path).st_mode - perms |= mode & (stat.S_ISUID | stat.S_ISGID | stat.S_ISVTX) - if perms & stat.S_ISUID and perms & stat.S_IWGRP: - raise InvalidPermissionsError( - 'Attempting to set suid with world writable') - chmod_x(path, perms) +import spack.util.file_permissions as fp def post_install(spec): if not spec.external: - perms = get_package_permissions(spec) - dir_perms = get_package_dir_permissions(spec) - group = get_package_group(spec) - - forall_files(spec.prefix, chmod_real_entries, [perms], [dir_perms]) - - if group: - forall_files(spec.prefix, chgrp, [group]) - - -class InvalidPermissionsError(SpackError): - """Error class for invalid permission setters""" + fp.set_permissions_by_spec(spec.prefix, spec) + + # os.walk explicitly set not to follow links + for root, dirs, files in os.walk(spec.prefix, followlinks=False): + for d in dirs: + if not os.path.islink(os.path.join(root, d)): + fp.set_permissions_by_spec(os.path.join(root, d), spec) + for f in files: + if not os.path.islink(os.path.join(root, f)): + fp.set_permissions_by_spec(os.path.join(root, f), spec) diff --git a/lib/spack/spack/modules/common.py b/lib/spack/spack/modules/common.py index 62df108b50..9185e6bfc9 100644 --- a/lib/spack/spack/modules/common.py +++ b/lib/spack/spack/modules/common.py @@ -47,6 +47,7 @@ import spack.util.path import spack.util.environment import spack.error import spack.util.spack_yaml as syaml +import spack.util.file_permissions as fp #: config section for this file configuration = spack.config.get('modules') @@ -750,6 +751,10 @@ class BaseModuleFileWriter(object): with open(self.layout.filename, 'w') as f: f.write(text) + # Set the file permissions of the module to match that of the package + if os.path.exists(self.layout.filename): + fp.set_permissions_by_spec(self.layout.filename, self.spec) + def remove(self): """Deletes the module file.""" mod_file = self.layout.filename diff --git a/lib/spack/spack/test/modules/common.py b/lib/spack/spack/test/modules/common.py index bb61100739..5c32c34840 100644 --- a/lib/spack/spack/test/modules/common.py +++ b/lib/spack/spack/test/modules/common.py @@ -3,8 +3,12 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) - +import pytest +import os +import stat +import spack.spec import spack.modules.common +import spack.modules.tcl def test_update_dictionary_extending_list(): @@ -32,3 +36,37 @@ def test_update_dictionary_extending_list(): assert len(target['foo']) == 4 assert len(target['bar']) == 4 assert target['baz'] == 'foobaz' + + +@pytest.fixture() +def mock_module_filename(monkeypatch, tmpdir): + filename = str(tmpdir.join('module')) + monkeypatch.setattr(spack.modules.common.BaseFileLayout, + 'filename', + filename) + + yield filename + + +@pytest.fixture() +def mock_package_perms(monkeypatch): + perms = stat.S_IRGRP | stat.S_IWGRP + monkeypatch.setattr(spack.package_prefs, + 'get_package_permissions', + lambda spec: perms) + + yield perms + + +def test_modules_written_with_proper_permissions(mock_module_filename, + mock_package_perms, + mock_packages, config): + spec = spack.spec.Spec('mpileaks').concretized() + + # The code tested is common to all module types, but has to be tested from + # one. TCL picked at random + generator = spack.modules.tcl.TclModulefileWriter(spec) + generator.write() + + assert mock_package_perms & os.stat( + mock_module_filename).st_mode == mock_package_perms diff --git a/lib/spack/spack/test/permissions.py b/lib/spack/spack/test/permissions.py index 223b3e8319..b233c7a0fa 100644 --- a/lib/spack/spack/test/permissions.py +++ b/lib/spack/spack/test/permissions.py @@ -7,8 +7,8 @@ import os import pytest import stat -from spack.hooks.permissions_setters import ( - chmod_real_entries, InvalidPermissionsError +from spack.util.file_permissions import ( + set_permissions, InvalidPermissionsError ) import llnl.util.filesystem as fs @@ -20,7 +20,7 @@ def test_chmod_real_entries_ignores_suid_sgid(tmpdir): mode = os.stat(path).st_mode # adds a high bit we aren't concerned with perms = stat.S_IRWXU - chmod_real_entries(path, perms) + set_permissions(path, perms) assert os.stat(path).st_mode == mode | perms & ~stat.S_IXUSR @@ -32,4 +32,4 @@ def test_chmod_rejects_group_writable_suid(tmpdir): perms = stat.S_IRWXU | stat.S_IRWXG | stat.S_IRWXO with pytest.raises(InvalidPermissionsError): - chmod_real_entries(path, perms) + set_permissions(path, perms) diff --git a/lib/spack/spack/util/file_permissions.py b/lib/spack/spack/util/file_permissions.py new file mode 100644 index 0000000000..c732aad0ab --- /dev/null +++ b/lib/spack/spack/util/file_permissions.py @@ -0,0 +1,40 @@ +# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other +# Spack Project Developers. See the top-level COPYRIGHT file for details. +# +# SPDX-License-Identifier: (Apache-2.0 OR MIT) + +import os +import stat as st +import llnl.util.filesystem as fs +import spack.package_prefs as pp +from spack.error import SpackError + + +def set_permissions_by_spec(path, spec): + # Get permissions for spec + if os.path.isdir(path): + perms = pp.get_package_dir_permissions(spec) + else: + perms = pp.get_package_permissions(spec) + group = pp.get_package_group(spec) + + set_permissions(path, perms, group) + + +def set_permissions(path, perms, group=None): + # Preserve higher-order bits of file permissions + perms |= os.stat(path).st_mode & (st.S_ISUID | st.S_ISGID | st.S_ISVTX) + + # Do not let users create world writable suid binaries + if perms & st.S_ISUID and perms & st.S_IWGRP: + raise InvalidPermissionsError( + "Attepting to set suid with world writable") + + fs.chmod_x(path, perms) + + if group: + fs.chgrp(path, group) + + +class InvalidPermissionsError(SpackError): + """Error class for invalid permission setters""" -- cgit v1.2.3-60-g2f50