Fix/whitelist internal loaders - #70250
Open
dwoz wants to merge 10 commits into
Open
Conversation
``salt.utils.schedule.Schedule`` is an internal Salt subsystem that must be able to call ``timezone.get_offset`` (in ``__singleton_init__``) and ``config.merge`` (in ``option``) for its own bookkeeping regardless of the operator's ``whitelist_modules`` setting. With a strict whitelist that omits ``config`` or ``timezone``, both lookups previously KeyError'd on the wire-filtered ``self.functions``. ``salt.loader.minion_mods`` already exposes the unfiltered inner LazyLoader as ``ret._dunder_salt`` on the outer wire-filtered loader (the substrate landed with PR saltstack#69983; PR saltstack#70192 extended it to the state stack). This patch continues the propagation into the scheduler: a new ``Schedule._dunder_salt`` property reads through the inner loader for the two internal-helper call sites while user-configured scheduled jobs keep dispatching through ``self.functions`` (line 861 / 880 etc.) and stay whitelist-gated. The property falls back to ``self.functions`` when the inner loader attribute is absent (salt-ssh ``FunctionWrapper``, tests that pass a plain ``dict``, or any wire-loader that did not set ``_dunder_salt``), so existing callers and the scheduler-test conftest fixture that hands in ``{"test.ping": ping}`` are unaffected. This also automatically picks up the freshly-regenerated inner loader in ``handle_func``'s subprocess-spawn path (line 738-740), since the property re-reads ``self.functions._dunder_salt`` on every access. Regression tests in ``tests/pytests/unit/utils/scheduler/test_whitelist_dunder.py`` cover: * ``_dunder_salt`` returns the inner loader when present * ``_dunder_salt`` falls back to ``self.functions`` when the attribute is missing or ``None`` * ``option()`` dispatches ``config.merge`` through the inner loader and works when the outer wire loader lacks ``config.merge`` * ``option()`` falls back to ``opts.get`` when neither loader has ``config.merge`` * ``time_offset`` is populated from the inner ``timezone.get_offset`` and defaults to ``"0000"`` when the inner call raises
Companion to the unit tests in ``tests/pytests/unit/utils/scheduler/test_whitelist_dunder.py`` -- these exercise the real ``salt.loader.minion_mods`` two-loader split (PR saltstack#69983) end-to-end with a real ``Schedule`` on top, so the fix is proven to work against actual loaders rather than only against plain-dict / mock fixtures. The 6 functional tests cover: * Preconditions: with ``whitelist_modules: [test, grains]``, the real wire-facing outer loader omits ``config.merge`` and ``timezone.get_offset`` (and the inner unfiltered loader still has them). * ``Schedule._dunder_salt`` on a real Schedule resolves to the same inner loader object exposed by ``minion_mods``. * ``Schedule.__singleton_init__`` populates ``self.time_offset`` by actually calling ``timezone.get_offset`` through the inner loader (``os_family`` grain is seeded so the function progresses past its grain check and shells out to ``date +%z``; the 5-char sign-prefixed return value distinguishes a real read from the 4-char ``"0000"`` fallback string that the ``try/except`` produces on failure). * ``Schedule.option`` dispatches ``config.merge`` through the inner loader end-to-end. * Salt-ssh / plain-dict backcompat: constructing Schedule with a plain ``dict`` (no ``_dunder_salt`` attribute) still works and exercises the ``self.opts.get`` / ``"0000"`` fallbacks.
Continues the two-loader model that shipped in ``salt.loader.minion_mods`` (PR saltstack#69983) and was extended to the state stack in PR saltstack#70192, applying the same pattern to three more internal Salt subsystems. Each of these is internal machinery -- operator-configured minion/master daemons or a hard-coded diagnostic path -- so requiring the operator to add helper execution modules (``config``, ``status``, ``mine``, ``event``, ``pillar``, ``sys``, ...) to their ``whitelist_modules`` was a maintenance treadmill that gave no security value, since these callers are never dispatched over the wire in the first place. Changes: * ``salt.loader.beacons`` -- the shipped beacons (status, sh, service, etc.) call ``__salt__[f"status.{func}"]``, ``status.procs()`` and friends. The factory now packs ``functions._dunder_salt`` as ``__salt__`` when the outer wire loader carries it, falling back to ``functions`` for salt-ssh FunctionWrapper / plain-dict fixtures. * ``salt.loader.engines`` -- shipped engines (slack, webhook, sqs, etc.) call ``__salt__["event.send"]`` / ``__salt__["pillar.get"]``. Same getattr-with-fallback pattern. * ``sys.doc`` error paths in ``salt/cli/caller.py``, ``salt/minion.py``, ``salt/metaproxy/proxy.py``, and ``salt/metaproxy/deltaproxy.py`` -- when a user-requested function is not on the wire loader, the "did you mean" documentation lookup uses ``sys.doc``. Under a strict whitelist that omits ``sys``, this error path itself KeyError'd; the operator got a traceback instead of the intended fallback message. All four callsites now resolve ``sys.doc`` through the inner loader via ``_sys_loader = getattr(functions, "_dunder_salt", None) or functions``; the user-facing membership check (``fun not in self.minion.functions``) stays on the wire loader and continues to be whitelist-gated. Zero caller-site changes required for beacons/engines: the loader factory does the getattr internally. Salt-ssh FunctionWrapper and plain-dict test fixtures continue to work unchanged (the ``_dunder_salt`` attribute is simply absent, and the fallback returns the original functions). Regression tests in ``tests/pytests/unit/loader/test_subsystem_whitelist_dunder.py`` cover: * beacons factory: passthrough of inner loader when present, fallback to ``functions`` when the ``_dunder_salt`` attribute is missing or explicitly ``None``, and end-to-end verification that ``status.procs`` is NOT on the wire loader but IS on the beacon's ``__salt__`` pack. * engines factory: same three cases, verified with ``event.send``. * sys.doc error path: the getattr-and-dispatch pattern used by all four callsites is exercised (inner-loader path, missing-attr fallback, explicit-None fallback). * Anti-regression parametrised test: asserts each of the four source files (``salt/cli/caller.py``, ``salt/minion.py``, ``salt/metaproxy/proxy.py``, ``salt/metaproxy/deltaproxy.py``) contains the ``_sys_loader["sys.doc"](...)`` invocation and does NOT contain the pre-fix direct ``functions["sys.doc"](...)`` form. Guards against a future single-site revert.
``salt.minion.Minion.process_beacons`` re-reads the ``beacons`` config
on every scheduler tick via ``functions["config.merge"]`` -- another
minion-internal ``config.merge`` lookup that KeyError'd on the wire
loader under a strict ``whitelist_modules`` that omitted ``config``.
Same shape as the ``Schedule.option`` and sys.doc error-path fixes.
Add unit + functional coverage for the process_beacons routing:
* unit: test_process_beacons_dispatches_config_merge_via_inner_loader
* unit: test_process_beacons_config_merge_falls_back_when_dunder_missing
* unit: test_process_beacons_callsite_routes_through_inner_loader
(anti-regression: asserts the getattr-fallback pattern is present
in salt/minion.py and the pre-fix direct dispatch is not)
Also add functional coverage for the beacons + engines + sys.doc fixes
already on this branch:
* tests/pytests/functional/loader/test_subsystem_whitelist_dunder.py
(9 tests) -- real minion_mods with narrow whitelist_modules,
real salt.loader.beacons + salt.loader.engines, load real beacon
and engine modules and verify their __salt__ reaches
non-whitelisted execution modules (status.procs, cmd.run_bg,
sys.doc).
``salt.minion.Minion.setup_scheduler`` (and its metaproxy variants)
inject four Salt-internal scheduled entries -- ``__mine_interval``,
``__master_alive_*``, ``__master_failback``, ``__ping_master`` -- into
every minion's scheduler at startup. These entries are internal
machinery, not user-controlled dispatch, but on the current wire
loader they were still gated by ``whitelist_modules`` in two places:
1. ``salt.utils.schedule.Schedule.handle_func`` looked up the
scheduled function through ``self.functions[func]`` -- the
wire-filtered outer loader. When ``__mine_interval`` fired on a
minion whose operator omitted ``mine`` from ``whitelist_modules``,
the tick raised ``KeyError: 'mine.update'``. Silent hourly
failure.
2. ``setup_scheduler`` gated the ``__mine_interval`` INJECTION on
``"mine.update" in self.functions`` (three sites -- ``salt/minion.py``,
``salt/metaproxy/proxy.py``, ``salt/metaproxy/deltaproxy.py``).
Under the same whitelist, ``__mine_interval`` was never added at
all -- ``mine.update`` simply never ran on that minion.
Fix uses the ``__``-prefix schedule-key convention that Salt already
follows for its internal entries: ``handle_func`` picks
``dispatch_functions = getattr(self.functions, "_dunder_salt", None) or
self.functions`` when ``data["name"]`` starts with ``__``, and keeps
the wire loader for operator-configured entries. The three injection
gates apply the getattr-with-fallback pattern directly.
Operator-configured schedule entries (no ``__`` prefix) keep
dispatching through the wire-filtered outer loader, so
``whitelist_modules`` remains an effective defense-in-depth gate on
operator-controlled scheduling. Salt-ssh ``FunctionWrapper`` and
plain-dict test fixtures are unaffected -- the fallback returns
``functions`` when ``_dunder_salt`` is absent.
Regression tests in
``tests/pytests/unit/utils/scheduler/test_whitelist_dunder.py``:
* ``test_handle_func_internal_job_dispatches_via_inner_loader`` --
``__``-prefixed schedule key routes through inner loader.
* ``test_handle_func_operator_configured_job_stays_on_wire_loader`` --
non-``__`` key stays on outer; a non-whitelisted exec module on
the inner loader is NOT reachable from operator-scheduled
dispatch.
* ``test_handle_func_internal_job_falls_back_when_dunder_missing`` --
plain-dict backcompat.
* ``test_handle_func_dispatch_selector_source_pattern_present`` --
anti-regression: asserts the selector code is present in
``salt/utils/schedule.py``.
* ``test_mine_update_injection_gates_route_through_inner_loader``
(parametrised over the three files) -- anti-regression: asserts
the getattr-fallback pattern is present in each injection file
and the pre-fix direct dispatch is not.
Boots a real salt-master + salt-minion pair with a strict
``whitelist_modules`` that omits every one of the internal-composition
helpers our fix targets (``config``, ``status``, ``mine``, ``sys``,
``timezone``, ``event``, ``pillar``) and exercises the fix through
the CLI end-to-end.
* ``tests/pytests/integration/loader/test_subsystem_whitelist_dunder.py``
- test_whitelisted_minion_dispatches_test_ping: sanity that a
minion under this narrow whitelist boots and can dispatch a
whitelisted function through the master; if any of
beacons-loader / process_beacons / setup_scheduler / engine
loader / sys.doc error path had regressed on startup, the
minion would either fail to start or drop off the master.
- test_beacons_list_reflects_configured_status_beacon: a
``status`` beacon is configured; ``beacons.list`` must reflect
it (proving the beacons factory + Minion.process_beacons +
config.merge routing all worked end-to-end).
- test_missing_function_error_path_does_not_traceback: dispatching
a nonexistent function returns a docs / not-available message
rather than a Python traceback naming sys.doc as the KeyError
target.
* ``tests/pytests/integration/utils/test_scheduler_whitelist_dunder.py``
- test_schedule_list_succeeds_under_narrow_whitelist: schedule.list
returns cleanly without traceback / KeyError, proving
Schedule.option() / config.merge routing works over the wire.
- test_saltutil_running_succeeds_under_narrow_whitelist:
saltutil.running (touched by scheduler ticks) returns cleanly.
``Schedule.eval`` (invoked at 1Hz via the minion's ``schedule``
periodic callback) and ``Schedule.run_job`` both check whether the
scheduled function exists on ``self.functions`` before dispatching.
Pre-fix, Salt-internal ``__``-prefixed jobs like ``__mine_interval``
routed this check through the wire-filtered outer loader. Under a
strict ``whitelist_modules`` that omitted ``mine`` / ``status``, the
check emitted ``log.info("Invalid function: mine.update in scheduled
job __mine_interval.")`` at every eval tick -- 1Hz log spam that
mimicked a KeyError but was actually just noisy INFO logging. The
downstream dispatch through ``handle_func`` already routed correctly
via the previously-added ``__``-prefix selector, so the *actual*
scheduled function still fired; the spam was cosmetic but obscuring.
This patch mirrors the ``handle_func`` selector on both pre-dispatch
presence checks: for ``__``-prefixed schedule keys the presence check
routes through the unfiltered inner loader; operator-configured
entries stay on the wire loader so ``whitelist_modules`` remains an
effective defense-in-depth gate on operator-controlled scheduling.
Reported by Daniel: on the mgmt-vc, dropping ``mine`` from
``whitelist_modules`` produced a 1Hz stream of these Invalid-function
log lines from the ``MultiMinionProcessManager MinionProcessManager``
subprocess -- restoring ``mine`` silenced them. Post-fix, the
whitelist can safely omit ``mine`` (plus the already-safe ``config``,
``status``, ``timezone``) without the log spam.
Regression tests in
``tests/pytests/unit/utils/scheduler/test_whitelist_dunder.py``:
* test_eval_presence_check_internal_job_uses_inner_loader
* test_eval_presence_check_operator_job_stays_on_wire_loader
* test_eval_and_run_job_presence_checks_use_prefix_selector
(parametrised anti-regression that greps for the selector in the
source at both callsites).
…llseye Cluster A (14 distros): tests/pytests/functional/loader/test_subsystem_ whitelist_dunder.py::test_beacon_module_can_reach_non_whitelisted_exec force-loaded salt.beacons.sh to inspect its populated __globals__["__salt__"] and prove the beacon loader packs the unfiltered inner dunder. salt.beacons.sh has __virtual__() that returns False when strace isn't on PATH -- CI images don't ship strace, so the module never lands in beacons._dict and the test KeyErrored on "sh.beacon" across every Linux runner. Swap to salt.beacons. status, whose __virtual__ unconditionally returns the virtualname and which uses the same __salt__["status.<func>"] dispatch pattern the test exercises. Cluster B (Debian 11 functional zeromq 2): tests/pytests/functional/modules/ test_aptpkg.py::test_aptpkg_remove_wildcard installs nginx-light 1.18.0-6.1+ deb11u8 from bullseye-security, but that .deb has been rewound off the mirror while the Packages file still advertises it (404). Debian 11 reached EOL 2026-08-31; add a cited-reason skipif gated to bullseye only so bookworm and trixie continue to enforce. Cluster C (Test Package Debian 11 Arm64 downgrade 3007.14): pkg-install of salt-* .debs pulled net-tools_...+deb11u2 from the security pool which is also 404 (same EOL rewind cause). The x86 twin debian-11-pkg was already enabled: false in cicd/shared-gh-workflows-context.yml; flip the arm64 counterpart to match for symmetry. Cluster A is PR-70250-introduced (the whitelist_dunder test file is new in this PR). Clusters B and C are pre-existing 3008.x infrastructure failures also visible on nightly runs; addressed here because saltstack#70250 owns the test-hygiene surface those bullseye jobs live in.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fix several internal only loaders to use a loader that is unrestricted by the whitelist.