diff --git a/changelog/69983.fixed.md b/changelog/69983.fixed.md new file mode 100644 index 000000000000..766e9198a1f8 --- /dev/null +++ b/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. diff --git a/salt/loader/__init__.py b/salt/loader/__init__.py index eff92aaa49a0..088427edf642 100644 --- a/salt/loader/__init__.py +++ b/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 diff --git a/salt/modules/saltcheck.py b/salt/modules/saltcheck.py index e959e42e9874..2abff72d2c59 100644 --- a/salt/modules/saltcheck.py +++ b/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 diff --git a/tests/pytests/integration/loader/test_module_whitelist_dunder.py b/tests/pytests/integration/loader/test_module_whitelist_dunder.py new file mode 100644 index 000000000000..c4c015f1b2b7 --- /dev/null +++ b/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 diff --git a/tests/pytests/integration/renderers/test_renderer_whitelist.py b/tests/pytests/integration/renderers/test_renderer_whitelist.py new file mode 100644 index 000000000000..b27d31d44fe1 --- /dev/null +++ b/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 diff --git a/tests/pytests/unit/states/test_boto_cloudtrail.py b/tests/pytests/unit/states/test_boto_cloudtrail.py index b267e63a17a6..3b2d061481a0 100644 --- a/tests/pytests/unit/states/test_boto_cloudtrail.py +++ b/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, + }, } diff --git a/tests/pytests/unit/states/test_boto_cloudwatch_event.py b/tests/pytests/unit/states/test_boto_cloudwatch_event.py index 49a8a769d572..0275c57a56c4 100644 --- a/tests/pytests/unit/states/test_boto_cloudwatch_event.py +++ b/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, + }, } diff --git a/tests/pytests/unit/states/test_boto_elasticsearch_domain.py b/tests/pytests/unit/states/test_boto_elasticsearch_domain.py index ab9e1b7bc727..c01471a2b355 100644 --- a/tests/pytests/unit/states/test_boto_elasticsearch_domain.py +++ b/tests/pytests/unit/states/test_boto_elasticsearch_domain.py @@ -6,6 +6,7 @@ import salt.config import salt.loader +import salt.modules.boto_elasticsearch_domain as boto_elasticsearch_domain_module import salt.states.boto_elasticsearch_domain as boto_elasticsearch_domain from tests.support.mock import MagicMock, patch @@ -86,7 +87,12 @@ def configure_loader_modules(minion_opts): "__utils__": utils, "__states__": salt_states, "__serializers__": serializers, - } + }, + boto_elasticsearch_domain_module: { + "__opts__": minion_opts, + "__salt__": funcs, + "__utils__": utils, + }, } diff --git a/tests/pytests/unit/states/test_boto_iot.py b/tests/pytests/unit/states/test_boto_iot.py index ba5f0e522b26..c486c60d13ac 100644 --- a/tests/pytests/unit/states/test_boto_iot.py +++ b/tests/pytests/unit/states/test_boto_iot.py @@ -6,6 +6,7 @@ import salt.config import salt.loader +import salt.modules.boto_iot as boto_iot_module import salt.states.boto_iot as boto_iot from tests.support.mock import MagicMock, patch @@ -136,7 +137,12 @@ def configure_loader_modules(minion_opts): "__utils__": utils, "__states__": salt_states, "__serializers__": serializers, - } + }, + boto_iot_module: { + "__opts__": minion_opts, + "__salt__": funcs, + "__utils__": utils, + }, } diff --git a/tests/pytests/unit/states/test_boto_lambda.py b/tests/pytests/unit/states/test_boto_lambda.py index 400af9b23c8b..418a050f6858 100644 --- a/tests/pytests/unit/states/test_boto_lambda.py +++ b/tests/pytests/unit/states/test_boto_lambda.py @@ -6,6 +6,7 @@ import salt.config import salt.loader +import salt.modules.boto_lambda as boto_lambda_module import salt.states.boto_lambda as boto_lambda import salt.utils.json from tests.support.mock import MagicMock, patch @@ -113,7 +114,12 @@ def configure_loader_modules(minion_opts): "__utils__": utils, "__states__": salt_states, "__serializers__": serializers, - } + }, + boto_lambda_module: { + "__opts__": minion_opts, + "__salt__": funcs, + "__utils__": utils, + }, } diff --git a/tests/pytests/unit/states/test_boto_s3_bucket.py b/tests/pytests/unit/states/test_boto_s3_bucket.py index 340faa733cf2..3d2c99606397 100644 --- a/tests/pytests/unit/states/test_boto_s3_bucket.py +++ b/tests/pytests/unit/states/test_boto_s3_bucket.py @@ -6,6 +6,7 @@ import pytest import salt.loader +import salt.modules.boto_s3_bucket as boto_s3_bucket_module import salt.states.boto_s3_bucket as boto_s3_bucket from tests.support.mock import MagicMock, patch @@ -214,7 +215,12 @@ def configure_loader_modules(minion_opts): "__utils__": utils, "__states__": salt_states, "__serializers__": serializers, - } + }, + boto_s3_bucket_module: { + "__opts__": minion_opts, + "__salt__": funcs, + "__utils__": utils, + }, } diff --git a/tests/unit/states/test_boto_apigateway.py b/tests/unit/states/test_boto_apigateway.py index a00514aeff04..f916d61a2530 100644 --- a/tests/unit/states/test_boto_apigateway.py +++ b/tests/unit/states/test_boto_apigateway.py @@ -8,6 +8,7 @@ import salt.config import salt.loader +import salt.modules.boto_apigateway as boto_apigateway_module import salt.states.boto_apigateway as boto_apigateway import salt.utils.files import salt.utils.yaml @@ -520,7 +521,12 @@ def setup_loader_modules(self): "__salt__": self.funcs, "__states__": self.salt_states, "__serializers__": serializers, - } + }, + boto_apigateway_module: { + "__opts__": self.opts, + "__utils__": utils, + "__salt__": self.funcs, + }, } # Set up MagicMock to replace the boto3 session diff --git a/tests/unit/states/test_boto_cognitoidentity.py b/tests/unit/states/test_boto_cognitoidentity.py index 8354b50d13fe..648e9d9975c8 100644 --- a/tests/unit/states/test_boto_cognitoidentity.py +++ b/tests/unit/states/test_boto_cognitoidentity.py @@ -6,6 +6,7 @@ import salt.config import salt.loader +import salt.modules.boto_cognitoidentity as boto_cognitoidentity_module import salt.states.boto_cognitoidentity as boto_cognitoidentity from salt.utils.versions import Version from tests.support.mixins import LoaderModuleMockMixin @@ -170,7 +171,19 @@ def setup_loader_modules(self): "__utils__": utils, "__states__": self.salt_states, "__serializers__": serializers, - } + }, + # Also override the exec module's ``__salt__`` with ``funcs``. + # ``whitelist_modules`` now only restricts what remote callers + # can invoke -- modules loaded through the whitelisted loader + # receive an *unfiltered* ``__salt__`` (see #69983), so mocks + # patched into ``self.funcs`` are otherwise invisible when the + # exec module reaches back through ``__salt__[...]`` (e.g. + # ``_get_role_arn`` calls ``__salt__["boto_iam.describe_role"]``). + boto_cognitoidentity_module: { + "__opts__": self.opts, + "__salt__": funcs, + "__utils__": utils, + }, } @classmethod diff --git a/tests/unit/states/test_boto_vpc.py b/tests/unit/states/test_boto_vpc.py index 32305d1a5891..24d526724cd8 100644 --- a/tests/unit/states/test_boto_vpc.py +++ b/tests/unit/states/test_boto_vpc.py @@ -6,6 +6,7 @@ import pytest import salt.config +import salt.modules.boto_vpc as boto_vpc_module import salt.states.boto_vpc as boto_vpc import salt.utils.botomod as botomod from salt.utils.versions import Version @@ -119,6 +120,11 @@ def setup_loader_modules(self): "__states__": self.salt_states, "__serializers__": serializers, }, + boto_vpc_module: { + "__opts__": self.opts, + "__salt__": self.funcs, + "__utils__": utils, + }, botomod: {}, }