From f72bf1bac84aa79f439ebd128816d63a6349041c Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sat, 11 Jul 2026 19:33:18 -0400 Subject: [PATCH 1/2] Stop the loader from shadowing top-level imports via sys.path (#69139) The LazyLoader put Salt's own source directories on the global sys.path while a module body executed (__populate_sys_path, and the fpath_dirname append in _load_module). Any bare import issued while that module ran could then resolve to a same-named single-file Salt module and get cached in sys.modules for the life of the process. That is what broke napalm on modern Salt. Loading salt/utils/napalm.py runs its top-level `import napalm`, which reaches ncclient.transport, which does a bare `import ssh` to detect the optional ssh-python/libssh package. With salt/utils on sys.path that bound to salt/utils/ssh.py (a plain module, not the ssh-python package), so ncclient's `from ssh.channel import Channel` raised "'ssh' is not a package", `import napalm` failed, HAS_NAPALM was False, and the napalm proxy/execution modules never passed their __virtual__ gate. This is the root cause behind the "Proxymodule napalm is missing an init()" reports in #69139 (which #69330 only improved the error message for). Salt-internal modules are imported via their fully-qualified salt.* names, so they never needed sys.path; only external/custom module dirs do, so a custom module's bare sibling imports keep resolving. Skip appending any directory under SALT_BASE_PATH in both __populate_sys_path and the fpath_dirname append. As a side effect, a module whose optional same-named dependency is not installed no longer "loads" by importing itself. --- changelog/69139.fixed.md | 1 + salt/loader/lazy.py | 33 ++++- tests/pytests/unit/loader/test_loader.py | 171 +++++++++++++++++++++++ 3 files changed, 204 insertions(+), 1 deletion(-) create mode 100644 changelog/69139.fixed.md diff --git a/changelog/69139.fixed.md b/changelog/69139.fixed.md new file mode 100644 index 000000000000..da56b0a16be5 --- /dev/null +++ b/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. diff --git a/salt/loader/lazy.py b/salt/loader/lazy.py index 677371f6734a..6928f6ad80df 100644 --- a/salt/loader/lazy.py +++ b/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": diff --git a/tests/pytests/unit/loader/test_loader.py b/tests/pytests/unit/loader/test_loader.py index ea0883b93889..f40351aa1fcb 100644 --- a/tests/pytests/unit/loader/test_loader.py +++ b/tests/pytests/unit/loader/test_loader.py @@ -6,7 +6,9 @@ """ import os +import pathlib import shutil +import sys import textwrap import pytest @@ -14,6 +16,7 @@ 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) From 3ca097a3ea0d1f57db70db266b7d946f5e163cec Mon Sep 17 00:00:00 2001 From: "Gary T. Giesen" Date: Sun, 12 Jul 2026 22:55:14 -0400 Subject: [PATCH 2/2] Add functional regression test for the loader sys.path shadow (#69139) The unit tests monkeypatch SALT_BASE_PATH and use a synthetic shadow file. This drives the real minion_mods + utils loaders against the real SALT_BASE_PATH and asserts, from inside a module body executed mid-load, that no Salt-internal directory is ever placed on sys.path -- the name-independent guarantee that protects every salt/utils and salt/modules collision (dns, napalm, git, pip, consul, ...), not just ssh. The concrete salt/utils/ssh.py shadow is also asserted when a real top-level ssh is absent. --- .../functional/loader/test_syspath_shadow.py | 139 ++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 tests/pytests/functional/loader/test_syspath_shadow.py diff --git a/tests/pytests/functional/loader/test_syspath_shadow.py b/tests/pytests/functional/loader/test_syspath_shadow.py new file mode 100644 index 000000000000..6ccdd77d4b73 --- /dev/null +++ b/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/.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