Stop the loader from shadowing top-level imports via sys.path (#69139)

changelog/69139.fixed.md

@@ -0,0 +1 @@
+Fixed the module loader putting Salt's own source directories on ``sys.path`` while a module body executes. That let a single-file Salt module (for example ``salt/utils/ssh.py``) shadow a same-named top-level third-party package that a loaded module's import chain pulls in, and the shadow was cached in ``sys.modules`` for the life of the process. In practice this broke ``import napalm``: ncclient's bare ``import ssh`` (used to detect the optional ssh-python/libssh package) bound to ``salt/utils/ssh.py`` instead, so ``HAS_NAPALM`` was ``False`` and the napalm proxy/execution modules never loaded. Salt-internal directories are no longer added to ``sys.path``; only external/custom module directories are, so a custom module's sibling imports still resolve. As a side effect, a module whose optional same-named dependency is not installed no longer loads by importing itself.

salt/loader/lazy.py

@@ -65,6 +65,21 @@
 
 SALT_BASE_PATH = pathlib.Path(salt.syspaths.INSTALL_DIR).resolve()
 LOADED_BASE_NAME = "salt.loaded"
+
+
+def _is_salt_internal_path(path):
+    """
+    Return True if ``path`` is the Salt install directory or lives under it.
+
+    Uses a path-component boundary (not a raw string prefix), so a sibling
+    directory whose name merely begins with the Salt package name -- e.g. the
+    ``saltext.*`` extensions, which install next to the ``salt`` package in
+    site-packages -- is correctly treated as external.
+    """
+    salt_base = str(SALT_BASE_PATH)
+    return path == salt_base or path.startswith(salt_base + os.sep)
+
+
 PY3_PRE_EXT = re.compile(r"\.cpython-{}{}(\.opt-[1-9])?".format(*sys.version_info[:2]))
 
 # Will be set to pyximport module at runtime if cython is enabled in config.
@@ -746,6 +761,16 @@ def _reload_submodules(self, mod):
 
     def __populate_sys_path(self):
         for directory in self.extra_module_dirs:
+            # Never put a Salt-internal module dir (e.g. salt/utils) on
+            # sys.path. Internal modules are imported via their fully-qualified
+            # ``salt.*`` names, so they gain nothing from this, and their
+            # single-file modules (salt/utils/ssh.py, salt/utils/yaml.py, ...)
+            # would shadow same-named third-party/stdlib top-level packages for
+            # any bare import triggered while a module body runs -- which then
+            # gets cached in sys.modules for the life of the process. Only
+            # external (custom/extension) dirs need to be importable this way.
+            if _is_salt_internal_path(directory):
+                continue
             if directory not in sys.path:
                 sys.path.append(directory)
                 self._clean_module_dirs.append(directory)
@@ -804,7 +829,13 @@ def _load_module(self, name):
         fpath_appended = False
         try:
             self.__populate_sys_path()
-            if fpath_dirname not in sys.path:
+            # Only append external module dirs, so a custom module's bare
+            # sibling imports resolve. A Salt-internal dir (salt/modules,
+            # salt/utils, ...) must never go on sys.path: a file such as
+            # salt/modules/ssh.py would shadow a same-named third-party/stdlib
+            # top-level package for any bare import made while this module
+            # executes. Internal modules import siblings via ``salt.*`` names.
+            if not _is_salt_internal_path(fpath) and fpath_dirname not in sys.path:
                 sys.path.append(fpath_dirname)
                 fpath_appended = True
             if suffix == ".pyx":

tests/pytests/functional/loader/test_syspath_shadow.py

@@ -0,0 +1,139 @@
+"""
+Functional regression test for the loader ``sys.path`` shadowing fix (#69139).
+
+The unit tests in ``test_loader.py`` monkeypatch ``SALT_BASE_PATH`` to a
+synthetic tree and use a hand-made shadow file. This test exercises the real
+loader wiring end to end: a real ``minion_mods`` loader built with the real
+``utils`` loader (whose ``module_dirs`` really contains ``salt/utils``), against
+the real ``SALT_BASE_PATH``.
+
+The concern is not limited to what ships in a default Salt install. Any
+``salt/utils/<name>.py`` shadows a same-named top-level package that some loaded
+module's import chain pulls in, and the packages people actually install to
+drive Salt modules are the real victims -- e.g. ``salt/utils/dns.py`` vs
+dnspython's ``dns``, ``salt/utils/napalm.py`` vs ``napalm`` (the #69139
+neighbourhood itself), ``salt/utils/github.py`` vs PyGithub's ``github``,
+``salt/utils/slack.py`` vs ``slack``. The salt/modules side is worse still
+(``git`` vs GitPython, ``pip``, ``consul``, ``elasticsearch``, ...).
+
+Rather than pin one name's symptom, the probe records -- while its own body runs
+mid-load -- whether *any* Salt-internal directory is on ``sys.path`` at all.
+That is the root guarantee that protects the whole class of collisions, and it
+does not depend on which third-party packages happen to be installed in CI. The
+concrete ``salt/utils/ssh.py`` shadow (the reported #69139 case) is asserted in
+addition, when a real top-level ``ssh`` is absent.
+"""
+
+import copy
+import importlib.util
+import os
+import sys
+import textwrap
+
+import pytest
+
+import salt.loader
+from salt.loader.lazy import SALT_BASE_PATH
+
+
+@pytest.fixture
+def shadow_probe_dir(tmp_path):
+    """
+    An on-disk execution-module directory whose module, in its body (executed
+    while the loader has it open), records which Salt-internal directories are
+    on ``sys.path`` and whether a bare ``import ssh`` binds to
+    ``salt/utils/ssh.py``.
+    """
+    base = tmp_path / "shadow-mod-base"
+    (base / "modules").mkdir(parents=True)
+    (base / "modules" / "shadowprobe.py").write_text(
+        textwrap.dedent(
+            '''
+            """Regression probe for the loader sys.path shadow (#69139)."""
+            import os
+            import sys
+
+            from salt.loader.lazy import SALT_BASE_PATH
+
+            _base = str(SALT_BASE_PATH)
+            # Any Salt-internal dir visible on sys.path *right now* (mid-load)
+            # is a shadow vector: a bare ``import X`` for any X matching a
+            # salt/utils/*.py file (dns, napalm, github, slack, ssh, ...) would
+            # bind to the Salt file instead of the real package.
+            _leaked = [
+                p for p in sys.path if p == _base or p.startswith(_base + os.sep)
+            ]
+
+            # Concrete #69139 symptom: the real salt/utils/ssh.py shadowing a
+            # bare ``import ssh`` (napalm -> ncclient -> import ssh).
+            try:
+                import ssh as _ssh
+
+                _resolved = os.path.abspath(getattr(_ssh, "__file__", "") or "")
+                _ssh_shadowed = os.path.join("salt", "utils") in _resolved
+            except ImportError:
+                _ssh_shadowed = False
+
+
+            def leaked_internal_dirs():
+                return _leaked
+
+
+            def ssh_shadowed():
+                return _ssh_shadowed
+            '''
+        )
+    )
+    return str(base)
+
+
+def test_loader_never_leaks_salt_internal_dirs_onto_sys_path_69139(
+    minion_opts, shadow_probe_dir
+):
+    """
+    Build the real ``minion_mods`` loader with the real ``utils`` loader and
+    load a module that inspects ``sys.path`` from inside its own body. With the
+    fix, no Salt-internal directory is ever placed on ``sys.path``, so no
+    ``salt/utils/*.py`` (or ``salt/modules/*.py``) file can shadow a same-named
+    third-party package that any loaded module's import chain pulls in.
+
+    Regression guard: without the fix the loader appends ``salt/utils`` to
+    ``sys.path`` for the duration of the load, so the probe sees it there and a
+    bare ``import ssh`` binds to ``salt/utils/ssh.py`` -- the shadow then gets
+    cached in ``sys.modules`` for the life of the process, which is exactly what
+    broke napalm/ncclient loading.
+    """
+    opts = copy.deepcopy(minion_opts)
+    opts["module_dirs"] = [shadow_probe_dir]
+
+    saved_path = list(sys.path)
+    saved_ssh = sys.modules.pop("ssh", None)
+    try:
+        utils = salt.loader.utils(opts)
+        # Guard the test's own premise: the real salt/utils directory is among
+        # the extra module dirs the loader would otherwise leak onto sys.path,
+        # so an empty ``leaked_internal_dirs`` below is a real result and not a
+        # vacuous pass from the wiring having changed.
+        assert any(
+            os.path.basename(directory) == "utils" and str(SALT_BASE_PATH) in directory
+            for directory in utils.module_dirs
+        ), "premise broken: real salt/utils not found in utils.module_dirs"
+
+        funcs = salt.loader.minion_mods(opts, utils=utils)
+        assert (
+            "shadowprobe.leaked_internal_dirs" in funcs
+        ), "probe module failed to load"
+
+        # Root guarantee, name-independent: protects the whole class of
+        # collisions (dns, napalm, git, pip, consul, ...), not just ssh.
+        assert funcs["shadowprobe.leaked_internal_dirs"]() == []
+
+        # Concrete #69139 symptom, when a genuine top-level ``ssh`` is not
+        # installed (otherwise a successful import is not proof of a shadow).
+        if importlib.util.find_spec("ssh") is None:
+            assert funcs["shadowprobe.ssh_shadowed"]() is False
+    finally:
+        sys.path[:] = saved_path
+        sys.modules.pop("ssh", None)
+        if saved_ssh is not None:
+            sys.modules["ssh"] = saved_ssh

tests/pytests/unit/loader/test_loader.py

@@ -6,14 +6,17 @@
 """
 
 import os
+import pathlib
 import shutil
+import sys
 import textwrap
 
 import pytest
 
 import salt.exceptions
 import salt.loader
 import salt.loader.lazy
+from tests.support.mock import patch
 
 
 @pytest.fixture
@@ -96,3 +99,171 @@ def foobar():
     with pytest.helpers.temp_file("mymod.py", contents, directory=tmp_path):
         loader = salt.loader.LazyLoader([tmp_path], opts, pack={"__test__": "meh"})
         assert loader["mymod.foobar"]() == "meh"
+
+
+def test_loader_does_not_shadow_top_level_import(tmp_path):
+    """
+    Loading a Salt-internal module must not put its directory on sys.path.
+
+    Otherwise a same-named single-file Salt module (e.g. salt/utils/ssh.py)
+    shadows a real top-level third-party package that a loaded module's import
+    chain pulls in, and the shadow is cached in sys.modules for the life of the
+    process. This is the root cause behind #69139: ncclient's bare ``import
+    ssh`` (to detect ssh-python, which is normally not installed) bound to
+    salt/utils/ssh.py, which broke ``import napalm``.
+    """
+    # Fake SALT_BASE_PATH tree. ``shadowmod`` stands in for salt/utils/ssh.py:
+    # a plain single-file Salt module whose stem matches a top-level package a
+    # loaded module's import chain would probe for. ``importer`` stands in for
+    # salt.utils.napalm doing a bare third-party import while its body runs.
+    # There is no real ``shadowmod`` package installed (as ssh-python normally
+    # is not), so the only way ``import shadowmod`` can resolve is if the loader
+    # leaks the Salt-internal dir onto sys.path.
+    salt_root = tmp_path / "saltroot"
+    loaderdir = salt_root / "mymods"
+    loaderdir.mkdir(parents=True)
+    (loaderdir / "shadowmod.py").write_text("MARKER = 'salt-internal-shadow'\n")
+    (loaderdir / "importer.py").write_text(
+        "try:\n"
+        "    import shadowmod\n"
+        "    RESULT = getattr(shadowmod, 'MARKER', 'other')\n"
+        "except ImportError:\n"
+        "    RESULT = 'not-importable'\n\n\n"
+        "def result():\n"
+        "    return RESULT\n"
+    )
+
+    opts = {"optimization_order": [0]}
+    saved_path = list(sys.path)
+    try:
+        with patch.object(
+            salt.loader.lazy, "SALT_BASE_PATH", pathlib.Path(str(salt_root))
+        ):
+            loader = salt.loader.LazyLoader([str(loaderdir)], opts)
+            # With the fix, loaderdir (under SALT_BASE_PATH) is never appended to
+            # sys.path, so a bare ``import shadowmod`` from another loaded module
+            # cannot bind to loaderdir/shadowmod.py. Without the fix, loaderdir
+            # is appended and the bare import binds to the Salt-internal file.
+            assert loader["importer.result"]() == "not-importable"
+    finally:
+        sys.path[:] = saved_path
+        sys.modules.pop("shadowmod", None)
+
+
+def test_loader_self_named_module_not_self_shadowed_when_dep_absent(tmp_path):
+    """
+    A module named after its optional third-party dependency must not "load" by
+    importing itself when that dependency is absent.
+
+    This is the ethtool / dson / json5 class (32 salt modules bare-import their
+    own name). salt/modules/ethtool.py does ``import ethtool``; with the
+    module's own directory on sys.path that bound to the salt file itself, so
+    the module falsely "loaded" while being self-referential and non-functional
+    at runtime. With this fix it correctly does not load.
+    """
+    salt_root = tmp_path / "saltroot"
+    loaderdir = salt_root / "mymods"
+    loaderdir.mkdir(parents=True)
+    # No real "widget" package is installed, mirroring an absent optional dep.
+    (loaderdir / "widget.py").write_text(
+        "import widget\n\n\ndef ok():\n    return getattr(widget, 'REAL', 'self')\n"
+    )
+    opts = {"optimization_order": [0]}
+    saved_path = list(sys.path)
+    try:
+        with patch.object(
+            salt.loader.lazy, "SALT_BASE_PATH", pathlib.Path(str(salt_root))
+        ):
+            loader = salt.loader.LazyLoader([str(loaderdir)], opts)
+            # With the fix loaderdir is not on sys.path, so ``import widget``
+            # raises ImportError and widget.py does not load. Without the fix it
+            # self-imports and widget.ok shows up in the loader.
+            assert "widget.ok" not in loader
+    finally:
+        sys.path[:] = saved_path
+        sys.modules.pop("widget", None)
+
+
+def test_loader_self_named_module_loads_via_real_dep_when_present(tmp_path):
+    """
+    Companion to the above: when the real same-named dependency IS installed,
+    the module must still load and bind to the real package (not the salt file).
+    Confirms the fix does not break these modules in the normal case.
+    """
+    # A real external "widget" package -- stands in for the installed dependency.
+    site = tmp_path / "site"
+    (site / "widget").mkdir(parents=True)
+    (site / "widget" / "__init__.py").write_text("REAL = 'real-widget'\n")
+
+    salt_root = tmp_path / "saltroot"
+    loaderdir = salt_root / "mymods"
+    loaderdir.mkdir(parents=True)
+    (loaderdir / "widget.py").write_text(
+        "import widget\n\n\ndef ok():\n    return getattr(widget, 'REAL', 'self')\n"
+    )
+    opts = {"optimization_order": [0]}
+    saved_path = list(sys.path)
+    sys.path.insert(0, str(site))
+    try:
+        with patch.object(
+            salt.loader.lazy, "SALT_BASE_PATH", pathlib.Path(str(salt_root))
+        ):
+            loader = salt.loader.LazyLoader([str(loaderdir)], opts)
+            assert loader["widget.ok"]() == "real-widget"
+    finally:
+        sys.path[:] = saved_path
+        sys.modules.pop("widget", None)
+
+
+def test_loader_external_module_dir_still_on_sys_path(tmp_path):
+    """
+    External (custom) module dirs must still be added to sys.path so a custom
+    module's bare sibling import keeps resolving -- the fix only excludes
+    Salt-internal dirs.
+    """
+    extdir = tmp_path / "ext"
+    extdir.mkdir()
+    (extdir / "sibling.py").write_text("VALUE = 'sib'\n")
+    (extdir / "usesibling.py").write_text(
+        "import sibling\n\n\ndef ok():\n    return sibling.VALUE\n"
+    )
+    opts = {"optimization_order": [0]}
+    try:
+        # extdir is not under the real SALT_BASE_PATH, so it is appended and the
+        # bare ``import sibling`` resolves.
+        loader = salt.loader.LazyLoader([str(extdir)], opts)
+        assert loader["usesibling.ok"]() == "sib"
+    finally:
+        sys.modules.pop("sibling", None)
+
+
+def test_loader_external_dir_sharing_salt_prefix_still_appended(tmp_path):
+    """
+    A module directory whose path merely *begins* with the Salt install path --
+    e.g. a ``saltext.*`` extension installed sibling to the ``salt`` package in
+    site-packages (``.../site-packages/saltext/...`` vs ``.../site-packages/salt``)
+    -- must still be added to sys.path. A raw ``startswith`` guard would
+    misclassify it as internal because "saltext" starts with "salt"; the fix
+    uses a path-component boundary instead.
+    """
+    salt_root = tmp_path / "saltroot"
+    salt_root.mkdir()
+    # External dir that shares the "saltroot" textual prefix but is NOT under it.
+    extdir = tmp_path / "saltroot_ext"
+    extdir.mkdir()
+    assert str(extdir).startswith(str(salt_root))  # the boundary condition
+    (extdir / "sibling.py").write_text("VALUE = 'sib'\n")
+    (extdir / "usesibling.py").write_text(
+        "import sibling\n\n\ndef ok():\n    return sibling.VALUE\n"
+    )
+    opts = {"optimization_order": [0]}
+    try:
+        with patch.object(
+            salt.loader.lazy, "SALT_BASE_PATH", pathlib.Path(str(salt_root))
+        ):
+            loader = salt.loader.LazyLoader([str(extdir)], opts)
+            # extdir is a sibling of salt_root, not under it, so it must be
+            # appended and the bare sibling import must resolve.
+            assert loader["usesibling.ok"]() == "sib"
+    finally:
+        sys.modules.pop("sibling", None)