From 1ef71376f2e2b41229471501e91448858f2df61d Mon Sep 17 00:00:00 2001 From: Tamara Dahlgren <35777542+tldahlgren@users.noreply.github.com> Date: Wed, 16 Oct 2019 14:55:37 -0700 Subject: Bugfix: stage directory permissions and cleaning (#12733) * This updates stage names to use "spack-stage-" as a prefix. This avoids removing non-Spack directories in "spack clean" as c141e99 did (in this case so long as they don't contain the prefix "spack-stage-"), and also addresses a follow-up issue where Spack stage directories were not removed. * Spack now does more-stringent checking of expected permissions for staging directories. For a given stage root that includes a user component, all directories before the user component that are created by Spack are expected to match the permissions of their parent; the user component and all deeper directories are expected to be accessible to the user (read/write/execute). --- lib/spack/docs/config_yaml.rst | 7 +- lib/spack/docs/tutorial_configuration.rst | 9 +- lib/spack/llnl/util/filesystem.py | 69 +++++++++++++ lib/spack/spack/package.py | 5 +- lib/spack/spack/stage.py | 91 ++++++++++------- lib/spack/spack/test/cmd/env.py | 4 +- lib/spack/spack/test/conftest.py | 25 ++++- lib/spack/spack/test/llnl/util/filesystem.py | 24 +++++ lib/spack/spack/test/stage.py | 146 +++++++++++++++++++++------ 9 files changed, 297 insertions(+), 83 deletions(-) diff --git a/lib/spack/docs/config_yaml.rst b/lib/spack/docs/config_yaml.rst index 53398f55c2..525f829c9e 100644 --- a/lib/spack/docs/config_yaml.rst +++ b/lib/spack/docs/config_yaml.rst @@ -91,9 +91,10 @@ the selected ``build_stage`` path. .. warning:: We highly recommend specifying ``build_stage`` paths that distinguish between staging and other activities to ensure ``spack clean`` does not inadvertently remove unrelated files. - This can be accomplished by using a combination of ``spack`` and or - ``stage`` in each path as shown in the default settings and documented - examples. + Spack prepends ``spack-stage-`` to temporary staging directory names to + reduce this risk. Using a combination of ``spack`` and or ``stage`` in + each specified path, as shown in the default settings and documented + examples, will add another layer of protection. By default, Spack's ``build_stage`` is configured like this: diff --git a/lib/spack/docs/tutorial_configuration.rst b/lib/spack/docs/tutorial_configuration.rst index 565a6c02b5..879bb94967 100644 --- a/lib/spack/docs/tutorial_configuration.rst +++ b/lib/spack/docs/tutorial_configuration.rst @@ -856,10 +856,11 @@ from this file system with the following ``config.yaml``: It is important to distinguish the build stage directory from other directories in your scratch space to ensure ``spack clean`` does not - inadvertently remove unrelated files. This can be accomplished by - including a combination of ``spack`` and or ``stage`` in each path - as shown in the default settings and documented examples. See - :ref:`config-yaml` for details. + inadvertently remove unrelated files. Spack prepends ``spack-stage-`` + to temporary staging directory names to reduce this risk. Using a + combination of ``spack`` and or ``stage`` in each specified path, as + shown in the default settings and documented examples, will add + another layer of protection. See :ref:`config-yaml` for details. On systems with compilers that absolutely *require* environment variables diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index 06925bec48..0af9c915b6 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -49,6 +49,8 @@ __all__ = [ 'is_exe', 'join_path', 'mkdirp', + 'partition_path', + 'prefixes', 'remove_dead_links', 'remove_if_dead_link', 'remove_linked_tree', @@ -1608,3 +1610,70 @@ def search_paths_for_executables(*path_hints): executable_paths.append(bin_dir) return executable_paths + + +def partition_path(path, entry=None): + """ + Split the prefixes of the path at the first occurrence of entry and + return a 3-tuple containing a list of the prefixes before the entry, a + string of the prefix ending with the entry, and a list of the prefixes + after the entry. + + If the entry is not a node in the path, the result will be the prefix list + followed by an empty string and an empty list. + """ + paths = prefixes(path) + + if entry is not None: + # Derive the index of entry within paths, which will correspond to + # the location of the entry in within the path. + try: + entries = path.split(os.sep) + i = entries.index(entry) + if '' in entries: + i -= 1 + return paths[:i], paths[i], paths[i + 1:] + except ValueError: + pass + + return paths, '', [] + + +def prefixes(path): + """ + Returns a list containing the path and its ancestors, top-to-bottom. + + The list for an absolute path will not include an ``os.sep`` entry. + For example, assuming ``os.sep`` is ``/``, given path ``/ab/cd/efg`` + the resulting paths will be, in order: ``/ab``, ``/ab/cd``, and + ``/ab/cd/efg`` + + The list for a relative path starting ``./`` will not include ``.``. + For example, path ``./hi/jkl/mn`` results in a list with the following + paths, in order: ``./hi``, ``./hi/jkl``, and ``./hi/jkl/mn``. + + Parameters: + path (str): the string used to derive ancestor paths + + Returns: + A list containing ancestor paths in order and ending with the path + """ + if not path: + return [] + + parts = path.strip(os.sep).split(os.sep) + if path.startswith(os.sep): + parts.insert(0, os.sep) + paths = [os.path.join(*parts[:i + 1]) for i in range(len(parts))] + + try: + paths.remove(os.sep) + except ValueError: + pass + + try: + paths.remove('.') + except ValueError: + pass + + return paths diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index b6a2d2de8d..dbe6e38fa6 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -56,7 +56,7 @@ from llnl.util.tty.log import log_output from llnl.util.tty.color import colorize from spack.filesystem_view import YamlFilesystemView from spack.util.executable import which -from spack.stage import Stage, ResourceStage, StageComposite +from spack.stage import stage_prefix, Stage, ResourceStage, StageComposite from spack.util.environment import dump_environment from spack.util.package_hash import package_hash from spack.version import Version @@ -753,7 +753,8 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)): mp = spack.mirror.mirror_archive_path(self.spec, fetcher) # Construct a path where the stage should build.. s = self.spec - stage_name = "%s-%s-%s" % (s.name, s.version, s.dag_hash()) + stage_name = "{0}{1}-{2}-{3}".format(stage_prefix, s.name, s.version, + s.dag_hash()) def download_search(): dynamic_fetcher = fs.from_list_url(self) diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py index 537a97b10e..6b27d37adf 100644 --- a/lib/spack/spack/stage.py +++ b/lib/spack/spack/stage.py @@ -4,7 +4,6 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) import os -import re import stat import sys import errno @@ -17,7 +16,7 @@ from six.moves.urllib.parse import urljoin import llnl.util.tty as tty from llnl.util.filesystem import mkdirp, can_access, install, install_tree -from llnl.util.filesystem import remove_linked_tree +from llnl.util.filesystem import partition_path, remove_linked_tree import spack.paths import spack.caches @@ -34,40 +33,63 @@ from spack.util.crypto import prefix_bits, bit_length _source_path_subdir = 'spack-src' # The temporary stage name prefix. -_stage_prefix = 'spack-stage-' +stage_prefix = 'spack-stage-' def _create_stage_root(path): """Create the stage root directory and ensure appropriate access perms.""" assert path.startswith(os.path.sep) and len(path.strip()) > 1 - curr_dir = os.path.sep - parts = path.strip(os.path.sep).split(os.path.sep) - for i, part in enumerate(parts): - curr_dir = os.path.join(curr_dir, part) - if not os.path.exists(curr_dir): - rest = os.path.join(os.path.sep, *parts[i + 1:]) - user_parts = rest.partition(os.path.sep + getpass.getuser()) - if len(user_parts[0]) > 0: - # Ensure access controls of subdirs created above `$user` - # inherit from the parent and share the group. - stats = os.stat(os.path.dirname(curr_dir)) - curr_dir = ''.join([curr_dir, user_parts[0]]) - mkdirp(curr_dir, group=stats.st_gid, mode=stats.st_mode) - - user_subdirs = ''.join(user_parts[1:]) - if len(user_subdirs) > 0: - # Ensure access controls of subdirs from `$user` on down are - # restricted to the user. - curr_dir = ''.join([curr_dir, user_subdirs]) - mkdirp(curr_dir, mode=stat.S_IRWXU) - - assert os.getuid() == os.stat(curr_dir).st_uid - break - - if not can_access(path): - raise OSError(errno.EACCES, - 'Cannot access %s: Permission denied' % curr_dir) + err_msg = 'Cannot create stage root {0}: Access to {1} is denied' + + user_uid = os.getuid() + + # Obtain lists of ancestor and descendant paths of the $user node, if any. + group_paths, user_node, user_paths = partition_path(path, + getpass.getuser()) + + for p in group_paths: + if not os.path.exists(p): + # Ensure access controls of subdirs created above `$user` inherit + # from the parent and share the group. + par_stat = os.stat(os.path.dirname(p)) + mkdirp(p, group=par_stat.st_gid, mode=par_stat.st_mode) + + p_stat = os.stat(p) + if par_stat.st_gid != p_stat.st_gid: + tty.warn("Expected {0} to have group {1}, but it is {2}" + .format(p, par_stat.st_gid, p_stat.st_gid)) + + if par_stat.st_mode & p_stat.st_mode != par_stat.st_mode: + tty.warn("Expected {0} to support mode {1}, but it is {2}" + .format(p, par_stat.st_mode, p_stat.st_mode)) + + if not can_access(p): + raise OSError(errno.EACCES, err_msg.format(path, p)) + + # Add the path ending with the $user node to the user paths to ensure paths + # from $user (on down) meet the ownership and permission requirements. + if user_node: + user_paths.insert(0, user_node) + + for p in user_paths: + # Ensure access controls of subdirs from `$user` on down are + # restricted to the user. + if not os.path.exists(p): + mkdirp(p, mode=stat.S_IRWXU) + + p_stat = os.stat(p) + if p_stat.st_mode & stat.S_IRWXU != stat.S_IRWXU: + tty.error("Expected {0} to support mode {1}, but it is {2}" + .format(p, stat.S_IRWXU, p_stat.st_mode)) + + raise OSError(errno.EACCES, err_msg.format(path, p)) + else: + p_stat = os.stat(p) + + if user_uid != p_stat.st_uid: + tty.warn("Expected user {0} to own {1}, but it is owned by {2}" + .format(user_uid, p, p_stat.st_uid)) def _first_accessible_path(paths): @@ -106,7 +128,7 @@ def _resolve_paths(candidates): # Remove the extra `$user` node from a `$tempdir/$user` entry for # hosts that automatically append `$user` to `$tempdir`. if path.startswith(os.path.join('$tempdir', '$user')) and tmp_has_usr: - path = os.path.join('$tempdir', path[15:]) + path = path.replace("/$user", "", 1) # Ensure the path is unique per user. can_path = sup.canonicalize_path(path) @@ -250,7 +272,7 @@ class Stage(object): # TODO: temporary stage area in _stage_root. self.name = name if name is None: - self.name = _stage_prefix + next(tempfile._get_candidate_names()) + self.name = stage_prefix + next(tempfile._get_candidate_names()) self.mirror_path = mirror_path # Use the provided path or construct an optionally named stage path. @@ -680,11 +702,8 @@ def purge(): """Remove all build directories in the top-level stage path.""" root = get_stage_root() if os.path.isdir(root): - # TODO: Figure out a "standard" way to identify the hash length - # TODO: Should this support alternate base represenations? - dir_expr = re.compile(r'.*-[0-9a-f]{32}$') for stage_dir in os.listdir(root): - if re.match(dir_expr, stage_dir) or stage_dir == '.lock': + if stage_dir.startswith(stage_prefix) or stage_dir == '.lock': stage_path = os.path.join(root, stage_dir) remove_linked_tree(stage_path) diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py index b7f3e5e325..232da811a3 100644 --- a/lib/spack/spack/test/cmd/env.py +++ b/lib/spack/spack/test/cmd/env.py @@ -17,6 +17,7 @@ import spack.environment as ev from spack.cmd.env import _env_create from spack.spec import Spec from spack.main import SpackCommand +from spack.stage import stage_prefix from spack.spec_list import SpecListError from spack.test.conftest import MockPackage, MockPackageMultiRepo @@ -576,7 +577,8 @@ def test_stage(mock_stage, mock_fetch, install_mockery): def check_stage(spec): spec = Spec(spec).concretized() for dep in spec.traverse(): - stage_name = "%s-%s-%s" % (dep.name, dep.version, dep.dag_hash()) + stage_name = "{0}{1}-{2}-{3}".format(stage_prefix, dep.name, + dep.version, dep.dag_hash()) assert os.path.isdir(os.path.join(root, stage_name)) check_stage('mpileaks') diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index 7795303eda..969e2471e4 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -5,6 +5,7 @@ import collections import copy +import errno import inspect import itertools import os @@ -41,6 +42,14 @@ from spack.spec import Spec from spack.version import Version +@pytest.fixture +def no_path_access(monkeypatch): + def _can_access(path, perms): + return False + + monkeypatch.setattr(os, 'access', _can_access) + + # # Disable any activate Spack environment BEFORE all tests # @@ -184,23 +193,29 @@ def working_env(): @pytest.fixture(scope='function', autouse=True) def check_for_leftover_stage_files(request, mock_stage, ignore_stage_files): - """Ensure that each test leaves a clean stage when done. + """ + Ensure that each (mock_stage) test leaves a clean stage when done. - This can be disabled for tests that are expected to dirty the stage - by adding:: + Tests that are expected to dirty the stage can disable the check by + adding:: @pytest.mark.disable_clean_stage_check - to tests that need it. + and the associated stage files will be removed. """ stage_path = mock_stage yield files_in_stage = set() - if os.path.exists(stage_path): + try: stage_files = os.listdir(stage_path) files_in_stage = set(stage_files) - ignore_stage_files + except OSError as err: + if err.errno == errno.ENOENT: + pass + else: + raise if 'disable_clean_stage_check' in request.keywords: # clean up after tests that are expected to be dirty diff --git a/lib/spack/spack/test/llnl/util/filesystem.py b/lib/spack/spack/test/llnl/util/filesystem.py index cdc50f9df2..f6d8845800 100644 --- a/lib/spack/spack/test/llnl/util/filesystem.py +++ b/lib/spack/spack/test/llnl/util/filesystem.py @@ -312,6 +312,30 @@ def test_headers_directory_setter(): assert hl.directories == [] +@pytest.mark.parametrize('path,entry,expected', [ + ('/tmp/user/root', None, + (['/tmp', '/tmp/user', '/tmp/user/root'], '', [])), + ('/tmp/user/root', 'tmp', ([], '/tmp', ['/tmp/user', '/tmp/user/root'])), + ('/tmp/user/root', 'user', (['/tmp'], '/tmp/user', ['/tmp/user/root'])), + ('/tmp/user/root', 'root', (['/tmp', '/tmp/user'], '/tmp/user/root', [])), + ('relative/path', None, (['relative', 'relative/path'], '', [])), + ('relative/path', 'relative', ([], 'relative', ['relative/path'])), + ('relative/path', 'path', (['relative'], 'relative/path', [])) +]) +def test_partition_path(path, entry, expected): + assert fs.partition_path(path, entry) == expected + + +@pytest.mark.parametrize('path,expected', [ + ('', []), + ('/tmp/user/dir', ['/tmp', '/tmp/user', '/tmp/user/dir']), + ('./some/sub/dir', ['./some', './some/sub', './some/sub/dir']), + ('another/sub/dir', ['another', 'another/sub', 'another/sub/dir']) +]) +def test_prefixes(path, expected): + assert fs.prefixes(path) == expected + + @pytest.mark.regression('7358') @pytest.mark.parametrize('regex,replacement,filename,keyword_args', [ (r"\", "", 'x86_cpuid_info.c', {}), diff --git a/lib/spack/spack/test/stage.py b/lib/spack/spack/test/stage.py index cb3204bc5f..66b358435f 100644 --- a/lib/spack/spack/test/stage.py +++ b/lib/spack/spack/test/stage.py @@ -4,6 +4,7 @@ # SPDX-License-Identifier: (Apache-2.0 OR MIT) """Test that the Stage class works correctly.""" +import errno import os import collections import shutil @@ -13,7 +14,7 @@ import getpass import pytest -from llnl.util.filesystem import mkdirp, touch, working_dir +from llnl.util.filesystem import mkdirp, partition_path, touch, working_dir import spack.paths import spack.stage @@ -354,23 +355,34 @@ def check_stage_dir_perms(prefix, path): # Ensure the path's subdirectories -- to `$user` -- have their parent's # perms while those from `$user` on are owned and restricted to the # user. - status = os.stat(prefix) - gid = status.st_gid - uid = os.getuid() + assert path.startswith(prefix) + user = getpass.getuser() - parts = path[len(prefix) + 1:].split(os.path.sep) - have_user = False - for part in parts: - if part == user: - have_user = True - prefix = os.path.join(prefix, part) - prefix_status = os.stat(prefix) - if not have_user: - assert gid == prefix_status.st_gid - assert status.st_mode == prefix_status.st_mode - else: - assert uid == status.st_uid - assert status.st_mode & stat.S_IRWXU == stat.S_IRWXU + prefix_status = os.stat(prefix) + uid = os.getuid() + + # Obtain lists of ancestor and descendant paths of the $user node, if any. + # + # Skip processing prefix ancestors since no guarantee they will be in the + # required group (e.g. $TEMPDIR on HPC machines). + skip = prefix if prefix.endswith(os.sep) else prefix + os.sep + group_paths, user_node, user_paths = partition_path(path.replace(skip, ""), + user) + + for p in group_paths: + p_status = os.stat(os.path.join(prefix, p)) + assert p_status.st_gid == prefix_status.st_gid + assert p_status.st_mode == prefix_status.st_mode + + # Add the path ending with the $user node to the user paths to ensure paths + # from $user (on down) meet the ownership and permission requirements. + if user_node: + user_paths.insert(0, user_node) + + for p in user_paths: + p_status = os.stat(os.path.join(prefix, p)) + assert uid == p_status.st_uid + assert p_status.st_mode & stat.S_IRWXU == stat.S_IRWXU @pytest.mark.usefixtures('mock_packages') @@ -643,7 +655,7 @@ class TestStage(object): def test_first_accessible_path(self, tmpdir): """Test _first_accessible_path names.""" - spack_dir = tmpdir.join('spack-test-fap') + spack_dir = tmpdir.join('paths') name = str(spack_dir) files = [os.path.join(os.path.sep, 'no', 'such', 'path'), name] @@ -671,9 +683,75 @@ class TestStage(object): # Cleanup shutil.rmtree(str(name)) + def test_create_stage_root(self, tmpdir, no_path_access): + """Test _create_stage_root permissions.""" + test_dir = tmpdir.join('path') + test_path = str(test_dir) + + try: + if getpass.getuser() in str(test_path).split(os.sep): + # Simply ensure directory created if tmpdir includes user + spack.stage._create_stage_root(test_path) + assert os.path.exists(test_path) + + p_stat = os.stat(test_path) + assert p_stat.st_mode & stat.S_IRWXU == stat.S_IRWXU + else: + # Ensure an OS Error is raised on created, non-user directory + with pytest.raises(OSError) as exc_info: + spack.stage._create_stage_root(test_path) + + assert exc_info.value.errno == errno.EACCES + finally: + try: + shutil.rmtree(test_path) + except OSError: + pass + + @pytest.mark.nomockstage + def test_create_stage_root_bad_uid(self, tmpdir, monkeypatch): + """ + Test the code path that uses an existing user path -- whether `$user` + in `$tempdir` or not -- and triggers the generation of the UID + mismatch warning. + + This situation can happen with some `config:build_stage` settings + for teams using a common service account for installing software. + """ + orig_stat = os.stat + + class MinStat: + st_mode = -1 + st_uid = -1 + + def _stat(path): + p_stat = orig_stat(path) + + fake_stat = MinStat() + fake_stat.st_mode = p_stat.st_mode + return fake_stat + + user_dir = tmpdir.join(getpass.getuser()) + user_dir.ensure(dir=True) + user_path = str(user_dir) + + # TODO: If we could guarantee access to the monkeypatch context + # function (i.e., 3.6.0 on), the call and assertion could be moved + # to a with block, such as: + # + # with monkeypatch.context() as m: + # m.setattr(os, 'stat', _stat) + # spack.stage._create_stage_root(user_path) + # assert os.stat(user_path).st_uid != os.getuid() + monkeypatch.setattr(os, 'stat', _stat) + spack.stage._create_stage_root(user_path) + + # The following check depends on the patched os.stat as a poor + # substitute for confirming the generated warnings. + assert os.stat(user_path).st_uid != os.getuid() + def test_resolve_paths(self): """Test _resolve_paths.""" - assert spack.stage._resolve_paths([]) == [] # resolved path without user appends user @@ -686,19 +764,24 @@ class TestStage(object): paths = [os.path.join(os.path.sep, 'spack-{0}'.format(user), 'stage')] assert spack.stage._resolve_paths(paths) == paths - # resolve paths where user - tmp = '$tempdir' - can_tmpdir = canonicalize_path(tmp) - temp_has_user = user in can_tmpdir.split(os.sep) - paths = [os.path.join(tmp, 'stage'), os.path.join(tmp, '$user')] - can_paths = [canonicalize_path(p) for p in paths] + tempdir = '$tempdir' + can_tempdir = canonicalize_path(tempdir) + user = getpass.getuser() + temp_has_user = user in can_tempdir.split(os.sep) + paths = [os.path.join(tempdir, 'stage'), + os.path.join(tempdir, '$user'), + os.path.join(tempdir, '$user', '$user'), + os.path.join(tempdir, '$user', 'stage', '$user')] + res_paths = [canonicalize_path(p) for p in paths] if temp_has_user: - can_paths[1] = can_tmpdir + res_paths[1] = can_tempdir + res_paths[2] = os.path.join(can_tempdir, user) + res_paths[3] = os.path.join(can_tempdir, 'stage', user) else: - can_paths[0] = os.path.join(can_paths[0], user) + res_paths[0] = os.path.join(res_paths[0], user) - assert spack.stage._resolve_paths(paths) == can_paths + assert spack.stage._resolve_paths(paths) == res_paths def test_get_stage_root_bad_path(self, clear_stage_root): """Ensure an invalid stage path root raises a StageError.""" @@ -711,9 +794,9 @@ class TestStage(object): assert spack.stage._stage_root is None @pytest.mark.parametrize( - 'path,purged', [('stage-1234567890abcdef1234567890abcdef', True), - ('stage-abcdef12345678900987654321fedcba', True), - ('stage-a1b2c3', False)]) + 'path,purged', [('spack-stage-1234567890abcdef1234567890abcdef', True), + ('spack-stage-anything-goes-here', True), + ('stage-spack', False)]) def test_stage_purge(self, tmpdir, clear_stage_root, path, purged): """Test purging of stage directories.""" stage_dir = tmpdir.join('stage') @@ -737,7 +820,6 @@ class TestStage(object): def test_get_stage_root_in_spack(self, clear_stage_root): """Ensure an instance path is an accessible build stage path.""" - base = canonicalize_path(os.path.join('$spack', '.spack-test-stage')) mkdirp(base) test_path = tempfile.mkdtemp(dir=base) -- cgit v1.2.3-70-g09d2