Make whitelist_modules only apply to remote dispatch (3006.x)

changelog/69983.fixed.md

@@ -0,0 +1 @@
+Fixed `whitelist_modules` so it only restricts what remote callers can invoke. Whitelisted modules can now compose with non-whitelisted modules via `__salt__[...]`, so a minion configured with `whitelist_modules: [test, mycompany, saltutil]` refuses `salt '*' cmd.run 'rm -rf /'` from the master while `mycompany.deploy` (which internally calls `__salt__["cmd.run"](...)`) still works.

salt/loader/__init__.py

@@ -316,27 +316,73 @@ def minion_mods(
     # TODO Publish documentation for module whitelisting
     if not whitelist:
         whitelist = opts.get("whitelist_modules", None)
+    # Both loaders must share the same ``__context__`` dict.  If we leave it
+    # as ``None`` LazyLoader.__init__ replaces it with a fresh ``{}`` in each
+    # loader's ``self.pack``, so writes made via one loader's
+    # NamedLoaderContext never reach reads made via the other's.
+    if context is None:
+        context = {}
+    pack = {
+        "__context__": context,
+        "__utils__": utils,
+        "__proxy__": proxy,
+        "__opts__": opts,
+        "__file_client__": file_client,
+    }
+    # Two-loader model: outer loader is whitelist-filtered for wire dispatch;
+    # inner ``salt_dunder`` is unfiltered and packed as ``__salt__`` inside
+    # every loaded module, so a whitelisted module can still compose with
+    # non-whitelisted modules via ``__salt__[...]``.
+    salt_dunder = LazyLoader(
+        _module_dirs(opts, "modules", "module"),
+        opts,
+        tag="module",
+        pack=pack,
+        loaded_base_name=loaded_base_name,
+        static_modules=static_modules,
+        extra_module_dirs=utils.module_dirs if utils else None,
+        pack_self="__salt__",
+    )
+    pack = dict(pack)
+    pack["__salt__"] = salt_dunder
     ret = LazyLoader(
         _module_dirs(opts, "modules", "module"),
         opts,
         tag="module",
-        pack={
-            "__context__": context,
-            "__utils__": utils,
-            "__proxy__": proxy,
-            "__opts__": opts,
-            "__file_client__": file_client,
-        },
+        pack=pack,
         whitelist=whitelist,
         loaded_base_name=loaded_base_name,
         static_modules=static_modules,
         extra_module_dirs=utils.module_dirs if utils else None,
-        pack_self="__salt__",
     )
 
+    # Test / callsite compatibility: ``patch.dict(ret, {...})`` was the way
+    # pre-split-loader tests injected mocks that both the wire-dispatch path
+    # AND internal ``__salt__[...]`` composition would see, because there was
+    # only one loader.  With the split, exec modules' ``__salt__`` is now the
+    # unfiltered inner ``salt_dunder`` and writes to ``ret`` don't reach it.
+    # Mirror writes made on ``ret`` into ``salt_dunder._dict`` so the classic
+    # ``patch.dict(ret, ...)`` idiom still works; reads through ``ret`` still
+    # go through ``_load()`` (which enforces the whitelist) so the security
+    # boundary at wire dispatch is preserved.
+    _salt_dunder = salt_dunder
+
+    class _WriteThroughLoader(type(ret)):  # noqa: N801
+        __module__ = type(ret).__module__
+
+        def __setitem__(self, key, val):
+            LazyLoader.__setitem__(self, key, val)
+            _salt_dunder._dict[key] = val
+
+        def __delitem__(self, key):
+            LazyLoader.__delitem__(self, key)
+            _salt_dunder._dict.pop(key, None)
+
+    ret.__class__ = _WriteThroughLoader
+
     # Allow the usage of salt dunder in utils modules.
     if utils and isinstance(utils, LazyLoader):
-        utils.pack["__salt__"] = ret
+        utils.pack["__salt__"] = salt_dunder
 
     # Load any provider overrides from the configuration file providers option
     #  Note: Providers can be pkg, service, user or group - not to be confused

salt/modules/saltcheck.py

@@ -290,15 +290,21 @@
 
 log = logging.getLogger(__name__)
 
-try:
-    __context__
-except NameError:
-    __context__ = {}
-__context__["global_scheck"] = None
-
 __virtualname__ = "saltcheck"
 
 
+def __init__(opts):
+    # Initialise ``global_scheck`` in the loader's ``__context__`` on every
+    # load, but only if no previous load has already populated it.  Doing
+    # this at module top-level would be unsafe: module-level code runs
+    # *before* the loader's pack loop binds ``__context__`` to the loader's
+    # ``NamedLoaderContext``, so a fresh dict created there is orphaned when
+    # the pack loop rewires ``__context__``.  It would also unconditionally
+    # reset the entry on every ``exec_module``, clobbering the ``SaltCheck``
+    # instance a running call has already stored.
+    __context__.setdefault("global_scheck", None)
+
+
 def __virtual__():
     """
     Set the virtual pkg module if not running as a proxy

tests/pytests/integration/loader/test_module_whitelist_dunder.py

@@ -0,0 +1,139 @@
+"""
+Integration tests for the split-loader behavior in ``salt.loader.minion_mods``.
+
+``minion_mods()`` returns a whitelist-filtered LazyLoader for remote
+dispatch, but packs an *unfiltered* loader as ``__salt__`` inside every
+loaded module.
+
+Effect on a whitelisted minion:
+  - Remote publishers can only invoke functions from whitelisted modules.
+  - A whitelisted module can still compose with non-whitelisted modules
+    via ``__salt__[...]``.
+"""
+
+import pytest
+
+from tests.conftest import FIPS_TESTRUN
+
+SECTEST_MODULE = """
+def run(cmd):
+    return __salt__["cmd.run"](cmd)
+"""
+
+
+@pytest.fixture
+def whitelisted_minion(salt_master):
+    """
+    A minion configured with ``whitelist_modules: [test, sectest, saltutil]``.
+    ``cmd`` is *deliberately absent* from the whitelist.
+    """
+    minion = salt_master.salt_minion_daemon(
+        "test-whitelist-dunder-minion",
+        overrides={
+            "whitelist_modules": [
+                "test",
+                "sectest",
+                "saltutil",
+                # Needed for the SLS-render tests below (state.template_str
+                # touches config/grains/pillar/slsutil during compilation).
+                "state",
+                "config",
+                "grains",
+                "pillar",
+                "slsutil",
+            ],
+            "fips_mode": FIPS_TESTRUN,
+            "encryption_algorithm": "OAEP-SHA224" if FIPS_TESTRUN else "OAEP-SHA1",
+            "signing_algorithm": (
+                "PKCS1v15-SHA224" if FIPS_TESTRUN else "PKCS1v15-SHA1"
+            ),
+        },
+    )
+    minion.after_terminate(
+        pytest.helpers.remove_stale_minion_key, salt_master, minion.id
+    )
+    with salt_master.state_tree.base.temp_file("_modules/sectest.py", SECTEST_MODULE):
+        with minion.started():
+            salt_cli = salt_master.salt_cli()
+            salt_cli.run("saltutil.sync_modules", minion_tgt=minion.id)
+            yield minion
+
+
+def test_whitelisted_function_returns(salt_cli, whitelisted_minion):
+    """
+    ``test.ping`` is on the whitelist and must return normally.
+    """
+    ret = salt_cli.run("test.ping", minion_tgt=whitelisted_minion.id)
+    assert ret.data is True
+
+
+def test_nonwhitelisted_function_is_blocked(salt_cli, whitelisted_minion):
+    """
+    ``cmd.run`` is *not* on the whitelist. Remote publish must not
+    execute it: the minion's outer (filtered) loader has no ``cmd``
+    entry, so the function is unavailable and the CLI reports either
+    "'cmd.run' is not available." or "Minion did not return" -- both
+    prove the whitelist rejected the call.
+    """
+    ret = salt_cli.run(
+        "cmd.run", "echo blocked", minion_tgt=whitelisted_minion.id, _timeout=15
+    )
+    data = str(ret.data or "")
+    assert "not available" in data or "did not return" in data
+
+
+def test_whitelisted_module_reaches_nonwhitelisted_via_dunder(
+    salt_cli, whitelisted_minion
+):
+    """
+    ``sectest`` is whitelisted; its ``run()`` internally calls
+    ``__salt__['cmd.run']``. Because the packed ``__salt__`` is the
+    *unfiltered* loader, the call succeeds even though direct remote
+    dispatch of ``cmd.run`` is blocked (previous test).
+    """
+    ret = salt_cli.run(
+        "sectest.run", "echo hello-from-dunder", minion_tgt=whitelisted_minion.id
+    )
+    assert ret.data == "hello-from-dunder"
+
+
+def test_sls_render_can_call_whitelisted_module(salt_cli, whitelisted_minion):
+    """
+    SLS files render on the minion with the whitelist-filtered loader
+    exposed as ``salt`` / ``__salt__``. A whitelisted module call inside
+    the template must render normally and the resulting state must run.
+    """
+    template = (
+        "{% set r = salt['test.echo']('hi-from-sls') %}\n"
+        "probe:\n"
+        "  test.nop:\n"
+        "    - name: {{ r }}\n"
+    )
+    ret = salt_cli.run("state.template_str", template, minion_tgt=whitelisted_minion.id)
+    # state.template_str returns a dict keyed by state chunk id.
+    assert isinstance(ret.data, dict)
+    key = next(iter(ret.data))
+    assert ret.data[key]["result"] is True
+    assert ret.data[key]["name"] == "hi-from-sls"
+
+
+def test_sls_render_cannot_call_nonwhitelisted_module(salt_cli, whitelisted_minion):
+    """
+    ``cmd`` is not on ``whitelist_modules``. A template that tries
+    ``salt['cmd.run'](...)`` must fail *at render time* -- the render
+    pipeline receives the same filtered loader that the wire dispatch
+    uses, not the unfiltered ``salt_dunder`` that execution modules see.
+
+    Jinja surfaces the missing key as ``UndefinedError: '...AliasedLoader
+    object' has no attribute 'cmd.run'``.
+    """
+    template = (
+        "{% set r = salt['cmd.run']('id') %}\n"
+        "probe:\n"
+        "  test.nop:\n"
+        "    - name: {{ r }}\n"
+    )
+    ret = salt_cli.run("state.template_str", template, minion_tgt=whitelisted_minion.id)
+    text = str(ret.data or ret.stdout)
+    assert "cmd.run" in text
+    assert "UndefinedError" in text or "no attribute" in text

tests/pytests/integration/renderers/test_renderer_whitelist.py

@@ -0,0 +1,99 @@
+"""
+Integration tests for the minion-side ``renderer_whitelist`` opt.
+
+Setting ``renderer_whitelist: [jinja, yaml]`` on a minion must prevent
+SLS files that request other renderers (``#!py``, ``#!pyobjects``,
+``#!pydsl``, ``#!mako``, ``#!wempy``) from rendering. Without the
+whitelist, a ``#!py`` SLS executes arbitrary Python on the minion
+during render -- so this is a real defense-in-depth boundary.
+"""
+
+import pytest
+
+from tests.conftest import FIPS_TESTRUN
+
+PY_SLS = """#!py
+def run():
+    return {"probe": {"test.nop": [{"name": "hi-from-py-sls"}]}}
+"""
+
+JINJA_SLS = (
+    "{% set r = salt['test.echo']('hi-from-jinja') %}\n"
+    "probe:\n"
+    "  test.nop:\n"
+    "    - name: {{ r }}\n"
+)
+
+
+@pytest.fixture
+def renderer_whitelisted_minion(salt_master):
+    """
+    Minion with ``renderer_whitelist: [jinja, yaml]``. Also whitelists
+    the execution modules that ``state.template_str`` needs internally
+    so we can drive rendering through a single top-level call.
+    """
+    minion = salt_master.salt_minion_daemon(
+        "test-renderer-whitelist-minion",
+        overrides={
+            "renderer_whitelist": ["jinja", "yaml"],
+            "whitelist_modules": [
+                "test",
+                "state",
+                "saltutil",
+                "config",
+                "grains",
+                "pillar",
+                "slsutil",
+            ],
+            "fips_mode": FIPS_TESTRUN,
+            "encryption_algorithm": "OAEP-SHA224" if FIPS_TESTRUN else "OAEP-SHA1",
+            "signing_algorithm": (
+                "PKCS1v15-SHA224" if FIPS_TESTRUN else "PKCS1v15-SHA1"
+            ),
+        },
+    )
+    minion.after_terminate(
+        pytest.helpers.remove_stale_minion_key, salt_master, minion.id
+    )
+    with minion.started():
+        yield minion
+
+
+def test_default_pipeline_still_renders(salt_cli, renderer_whitelisted_minion):
+    """
+    A plain SLS (no shebang) uses the default ``jinja|yaml`` pipe -- both
+    are on the whitelist, so rendering must succeed.
+    """
+    ret = salt_cli.run(
+        "state.template_str",
+        JINJA_SLS,
+        minion_tgt=renderer_whitelisted_minion.id,
+    )
+    assert isinstance(ret.data, dict), f"unexpected return: {ret.data!r}"
+    key = next(iter(ret.data))
+    assert ret.data[key]["result"] is True
+    assert ret.data[key]["name"] == "hi-from-jinja"
+
+
+def test_shebang_py_renderer_is_rejected(salt_cli, renderer_whitelisted_minion):
+    """
+    An SLS starting with ``#!py`` requests the ``py`` renderer, which is
+    NOT on the whitelist. ``check_render_pipe_str`` drops it, the render
+    pipe becomes empty, and ``state.template_str`` reports no data --
+    the arbitrary-Python-in-SLS attack surface is closed.
+
+    Also verifies via the minion log that the renderer was rejected
+    with the standard ``The renderer "..." is not available`` warning.
+    """
+    ret = salt_cli.run(
+        "state.template_str",
+        PY_SLS,
+        minion_tgt=renderer_whitelisted_minion.id,
+    )
+    # A rejected render returns falsy data (empty dict / empty list /
+    # error string). Positively assert the Python body did NOT execute:
+    # a successful #!py render would produce a ``probe`` state chunk
+    # named ``hi-from-py-sls``.
+    text = str(ret.data or "")
+    assert "hi-from-py-sls" not in text
+    assert "test.nop" not in text

tests/pytests/unit/states/test_boto_cloudtrail.py

@@ -5,6 +5,7 @@
 import pytest
 
 import salt.loader
+import salt.modules.boto_cloudtrail as boto_cloudtrail_module
 import salt.states.boto_cloudtrail as boto_cloudtrail
 from tests.support.mock import MagicMock, patch
 
@@ -98,7 +99,12 @@ def configure_loader_modules(minion_opts):
             "__utils__": utils,
             "__states__": salt_states,
             "__serializers__": serializers,
-        }
+        },
+        boto_cloudtrail_module: {
+            "__opts__": minion_opts,
+            "__salt__": funcs,
+            "__utils__": utils,
+        },
     }
 
 

tests/pytests/unit/states/test_boto_cloudwatch_event.py

@@ -5,6 +5,7 @@
 import pytest
 
 import salt.loader
+import salt.modules.boto_cloudwatch_event as boto_cloudwatch_event_module
 import salt.states.boto_cloudwatch_event as boto_cloudwatch_event
 from tests.support.mock import MagicMock, patch
 
@@ -92,7 +93,12 @@ def configure_loader_modules(minion_opts):
             "__utils__": utils,
             "__states__": salt_states,
             "__serializers__": serializers,
-        }
+        },
+        boto_cloudwatch_event_module: {
+            "__opts__": minion_opts,
+            "__salt__": funcs,
+            "__utils__": utils,
+        },
     }