From b6a1a56bba806289030b57f35e142ddbfb5a4045 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Wed, 15 Jul 2026 13:01:13 -0600 Subject: [PATCH 1/6] Add pillar_mask_output config option and fix full-value pillar masking VCOPS-98852: harden pillar output masking (VCOPS-77716, VCOPS-84671). - salt.utils.secret.serial() only redacted non-empty strings; truthy int/float/bool and non-empty bytes leaked through pillar.get() and related functions with their real value even though the repr path already redacted those types. Extracted a shared _is_redactable_scalar() predicate used by both _masked_repr() and serial() so the two can't drift apart again. - Added the pillar_mask_output master/minion config option (default True) as a global killswitch, seeded via salt.utils.secret.configure() from salt.pillar.get_pillar()/get_async_pillar(). When False, hide()/serial()/mask_output() no-op and pillar values are never wrapped or redacted. - Documented pillar_mask_output in doc/ref/configuration/master.rst. - Updated tests that encoded the old (buggy) passthrough behavior and added coverage for bytes redaction and the new config toggle. Co-Authored-By: Claude Sonnet 5 --- changelog/98852.added.md | 1 + changelog/98852.fixed.md | 1 + doc/ref/configuration/master.rst | 22 ++++ salt/config/__init__.py | 5 + salt/pillar/__init__.py | 4 + salt/utils/secret.py | 60 ++++++++-- .../functional/pillar/test_pillar_masking.py | 34 ++++++ tests/pytests/unit/modules/test_pillar.py | 14 ++- tests/pytests/unit/utils/test_secret.py | 106 ++++++++++++++++-- 9 files changed, 226 insertions(+), 21 deletions(-) create mode 100644 changelog/98852.added.md create mode 100644 changelog/98852.fixed.md diff --git a/changelog/98852.added.md b/changelog/98852.added.md new file mode 100644 index 000000000000..915807e8e6c7 --- /dev/null +++ b/changelog/98852.added.md @@ -0,0 +1 @@ +Added the ``pillar_mask_output`` master/minion config option to globally enable or disable pillar output masking (redaction of sensitive pillar values in ``pillar.get``/``pillar.items``/etc., ``no_log`` state output, and general CLI output). Defaults to ``True`` (masking stays on), matching existing behavior. diff --git a/changelog/98852.fixed.md b/changelog/98852.fixed.md new file mode 100644 index 000000000000..cca0224ceb7d --- /dev/null +++ b/changelog/98852.fixed.md @@ -0,0 +1 @@ +Fixed pillar output masking (``salt.utils.secret.serial``) only redacting string values — truthy ``int``/``float``/``bool`` and non-empty ``bytes`` pillar values were returned unmasked through ``pillar.get`` and related functions even with masking enabled. Masking of these types is now consistent with how they were already redacted in ``repr``/``str`` output. diff --git a/doc/ref/configuration/master.rst b/doc/ref/configuration/master.rst index 3cb473072f0c..4a4d7317369e 100644 --- a/doc/ref/configuration/master.rst +++ b/doc/ref/configuration/master.rst @@ -5662,6 +5662,28 @@ Recursively merge lists by aggregating them instead of replacing them. pillar_merge_lists: False +.. conf_master:: pillar_mask_output + +``pillar_mask_output`` +********************** + +.. versionadded:: 3008.3 + +Default: ``True`` + +Globally enable or disable redaction of pillar values in logs and state +output. When ``True`` (the default), sensitive pillar values are replaced +with ``**********`` in ``pillar.get`` and related execution module output, +``no_log`` state results, and general CLI output, unless a caller explicitly +requests the real value (e.g. ``pillar.get(key, unmask=True)``). + +Set this option to ``False`` to disable pillar masking entirely and always +return real values, matching pre-masking behavior. + +.. code-block:: yaml + + pillar_mask_output: True + .. conf_master:: pillar_includes_override_sls ``pillar_includes_override_sls`` diff --git a/salt/config/__init__.py b/salt/config/__init__.py index 6e32516235a3..7476623259f3 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -698,6 +698,9 @@ def _gather_buffer_space(): "pillar_source_merging_strategy": str, # Recursively merge lists by aggregating them instead of replacing them. "pillar_merge_lists": bool, + # Globally enable/disable redaction of pillar values in logs and state + # output (pillar.get, no_log states, CLI output, etc.). + "pillar_mask_output": bool, # If True, values from included pillar SLS targets will override "pillar_includes_override_sls": bool, # How to merge multiple top files from multiple salt environments @@ -1172,6 +1175,7 @@ def _gather_buffer_space(): "pillar_opts": False, "pillar_source_merging_strategy": "smart", "pillar_merge_lists": False, + "pillar_mask_output": True, "pillar_includes_override_sls": False, # ``pillar_cache``, ``pillar_cache_ttl``, ``pillar_cache_backend``, # ``gpg_cache``, ``gpg_cache_ttl`` and ``gpg_cache_backend`` @@ -1646,6 +1650,7 @@ def _gather_buffer_space(): "pillar_safe_render_error": True, "pillar_source_merging_strategy": "smart", "pillar_merge_lists": False, + "pillar_mask_output": True, "pillar_includes_override_sls": False, "pillar_cache": False, "pillar_cache_ttl": 3600, diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 399a2660c9ab..3005af834ecb 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -46,6 +46,9 @@ def get_pillar( """ Return the correct pillar driver based on the file_client option """ + # Seed the pillar-masking killswitch from this process's own opts before + # any pillar compile/wrap happens (salt.utils.secret.hide()/serial()). + salt.utils.secret.configure(opts) # When file_client is 'local' this makes the minion masterless # but sometimes we want the minion to read its files from the local # filesystem instead of asking for them from the master, but still @@ -107,6 +110,7 @@ def get_async_pillar( """ Return the correct pillar driver based on the file_client option """ + salt.utils.secret.configure(opts) file_client = opts["file_client"] if opts.get("master_type") == "disable" and file_client == "remote": file_client = "local" diff --git a/salt/utils/secret.py b/salt/utils/secret.py index 1ed2e588385b..54489f967c6d 100644 --- a/salt/utils/secret.py +++ b/salt/utils/secret.py @@ -45,6 +45,25 @@ REDACT_PLACEHOLDER = "**********" +# Global on/off switch for the whole pillar-masking feature, seeded from the +# ``pillar_mask_output`` config option. Unlike ``mask_pillar`` above (a +# per-render-context toggle), this is an administrator-facing killswitch: +# when False, hide()/serial() never wrap or redact, regardless of context. +_ENABLED = True + + +def configure(opts): + """Seed the global masking killswitch from ``pillar_mask_output``. + + Called from ``salt.pillar.get_pillar()`` / ``get_async_pillar()`` — the + choke point where both minion and master-side pillar-compile flows + already receive the full ``opts`` dict — so this stays in sync with the + process's own config without threading an extra parameter through every + masking call site. + """ + global _ENABLED + _ENABLED = bool(opts.get("pillar_mask_output", True)) + # --------------------------------------------------------------------------- # Internal helpers @@ -62,6 +81,19 @@ def _mask_wrap(value): return value +def _is_redactable_scalar(value) -> bool: + """True if value is a non-empty/truthy str, bytes, int, float, or bool leaf. + + Shared by ``_masked_repr`` (display) and ``serial`` (actual output + boundary) so the two can't drift apart on which leaf values count as + sensitive — that drift is exactly what let non-string values leak + through ``serial()`` unmasked. + """ + if isinstance(value, (str, bytes, int, float, bool)): + return bool(value) + return False + + def _masked_repr(value) -> str: """Build a redacted repr string for a MaskedDict or MaskedList.""" if isinstance(value, dict): @@ -69,11 +101,9 @@ def _masked_repr(value) -> str: return "{" + pairs + "}" if isinstance(value, list): return "[" + ", ".join(_masked_repr(v) for v in value) + "]" - if isinstance(value, str) and value: - return repr(REDACT_PLACEHOLDER) - if isinstance(value, bytes) and value: + if isinstance(value, bytes) and _is_redactable_scalar(value): return repr(REDACT_PLACEHOLDER.encode()) - if isinstance(value, (int, float, bool)) and value: + if _is_redactable_scalar(value): return repr(REDACT_PLACEHOLDER) return repr(value) @@ -208,7 +238,11 @@ def hide(value): Scalar values (str, int, bool, None …) are returned unchanged — they are stored plain inside the container and only redacted in the container's repr. Already-wrapped values are returned as-is (idempotent). + + No-ops when the global masking killswitch (``pillar_mask_output``) is off. """ + if not _ENABLED: + return value return _mask_wrap(value) @@ -244,7 +278,8 @@ def expose(value, _seen=None): def serial(value, _seen=None): - """Aggressively redact: replace ALL non-empty strings with REDACT_PLACEHOLDER. + """Aggressively redact: replace every non-empty/truthy scalar leaf value + (str, bytes, int, float, bool) with a redacted placeholder. Use at explicit pillar output boundaries (``pillar.get``, ``pillar.items``, ``pillar.item``, ``pillar.ext``) and inside ``no_log_mask``. @@ -252,13 +287,20 @@ def serial(value, _seen=None): Because ``MaskedDict.__getitem__`` returns plain strings (the scalar leaves are stored unwrapped), this function must handle plain str/dict/list values in addition to MaskedDict / MaskedList containers. + + No-ops (returns *value* unchanged) when the global masking killswitch + (``pillar_mask_output``) is off. """ + if not _ENABLED: + return value if _seen is None: _seen = set() - if isinstance(value, str) and value: + if isinstance(value, bytes) and _is_redactable_scalar(value): + return REDACT_PLACEHOLDER.encode() + if _is_redactable_scalar(value): return REDACT_PLACEHOLDER if not isinstance(value, (dict, list)): - # int, float, bool, None, empty string, bytes — pass through + # int, float, bool, None, empty string, empty bytes — pass through return value vid = id(value) if vid in _seen: @@ -291,7 +333,11 @@ def mask_output(value, _seen=None): Use as a safety net in ``output/__init__.py`` to prevent accidental pillar leakage in general Salt output without redacting ordinary result strings (state comments, module names, etc.). + + No-ops when the global masking killswitch (``pillar_mask_output``) is off. """ + if not _ENABLED: + return value if _seen is None: _seen = set() if isinstance(value, (MaskedDict, MaskedList)): diff --git a/tests/pytests/functional/pillar/test_pillar_masking.py b/tests/pytests/functional/pillar/test_pillar_masking.py index 82ba16dd410d..014d54af00a0 100644 --- a/tests/pytests/functional/pillar/test_pillar_masking.py +++ b/tests/pytests/functional/pillar/test_pillar_masking.py @@ -117,3 +117,37 @@ def test_masked_pillar_redacts_outside_render_bracket(): assert salt.utils.secret.REDACT_PLACEHOLDER in repr(pillar) assert "host1" not in repr(pillar) assert salt.utils.secret.REDACT_PLACEHOLDER in str(pillar["hosts"]) + + +def test_get_pillar_wires_pillar_mask_output_config_option(minion_opts, grains): + """VCOPS-98852: ``pillar_mask_output`` is the standard-config-path toggle. + + ``salt.pillar.get_pillar()`` is the choke point that receives ``opts`` + for every pillar-compile flow (minion and master-side); it must seed + ``salt.utils.secret``'s global killswitch so hide()/serial() honor the + config option without every consumer having to thread opts through. + """ + opts = dict(minion_opts) + opts["file_client"] = "local" + opts["pillar_cache"] = False + opts["minion_data_cache"] = False + + try: + opts["pillar_mask_output"] = False + salt.pillar.get_pillar(opts, grains, "test-minion", "base") + assert salt.utils.secret.hide({"k": "v"}) == {"k": "v"} + assert not isinstance( + salt.utils.secret.hide({"k": "v"}), salt.utils.secret.MaskedDict + ) + assert salt.utils.secret.serial("hunter2") == "hunter2" + + opts["pillar_mask_output"] = True + salt.pillar.get_pillar(opts, grains, "test-minion", "base") + assert isinstance( + salt.utils.secret.hide({"k": "v"}), salt.utils.secret.MaskedDict + ) + assert ( + salt.utils.secret.serial("hunter2") == salt.utils.secret.REDACT_PLACEHOLDER + ) + finally: + salt.utils.secret.configure({"pillar_mask_output": True}) diff --git a/tests/pytests/unit/modules/test_pillar.py b/tests/pytests/unit/modules/test_pillar.py index 820a2a8d10e7..f3f7c9c81af9 100644 --- a/tests/pytests/unit/modules/test_pillar.py +++ b/tests/pytests/unit/modules/test_pillar.py @@ -3,6 +3,7 @@ import pytest import salt.modules.pillar as pillarmod +import salt.utils.secret as secret from tests.support.mock import MagicMock, call, patch @@ -135,20 +136,27 @@ def test_pillar_get_default_merge_regression_38558(): """Test for pillar.get(key=..., default=..., merge=True) Do not update the ``default`` value when using ``merge=True``. See: https://github.com/saltstack/salt/issues/38558 + + ``res`` values below are masked (VCOPS-98852: pillar.get()'s default + output redacts truthy int/float/bool leaves too, not just strings) — use + ``unmask=True`` to assert against the real values. ``default`` is a plain + Python literal passed in by the caller, never itself redacted, so its + non-mutation check still compares real values. """ with patch.dict(pillarmod.__pillar__, {"l1": {"l2": {"l3": 42}}}): res = pillarmod.get(key="l1") - assert {"l2": {"l3": 42}} == res + assert {"l2": {"l3": secret.REDACT_PLACEHOLDER}} == res + assert {"l2": {"l3": 42}} == pillarmod.get(key="l1", unmask=True) default = {"l2": {"l3": 43}} res = pillarmod.get(key="l1", default=default) - assert {"l2": {"l3": 42}} == res + assert {"l2": {"l3": secret.REDACT_PLACEHOLDER}} == res assert {"l2": {"l3": 43}} == default res = pillarmod.get(key="l1", default=default, merge=True) - assert {"l2": {"l3": 42}} == res + assert {"l2": {"l3": secret.REDACT_PLACEHOLDER}} == res assert {"l2": {"l3": 43}} == default diff --git a/tests/pytests/unit/utils/test_secret.py b/tests/pytests/unit/utils/test_secret.py index df29e9079e56..9b53ad2c3ea8 100644 --- a/tests/pytests/unit/utils/test_secret.py +++ b/tests/pytests/unit/utils/test_secret.py @@ -252,31 +252,60 @@ def test_serial_leaves_empty_string(): assert secret.serial("") == "" -def test_serial_leaves_non_string_scalars(): - assert secret.serial(42) == 42 - assert secret.serial(True) is True +def test_serial_redacts_truthy_non_string_scalars(): + # VCOPS-98852: serial() must redact ALL pillar value types, not just str, + # so it stays consistent with the repr path (_masked_repr), which already + # redacted truthy int/float/bool. Before this fix, serial(42) == 42 — + # a real leak through the exact function pillar.get() relies on. + assert secret.serial(42) == secret.REDACT_PLACEHOLDER + assert secret.serial(True) == secret.REDACT_PLACEHOLDER + assert secret.serial(3.14) == secret.REDACT_PLACEHOLDER + + +def test_serial_leaves_falsy_non_string_scalars(): + # Falsy/zero values and None are not treated as secrets (matches the + # pre-existing repr convention for _masked_repr). + assert secret.serial(0) == 0 + assert secret.serial(False) is False assert secret.serial(None) is None +def test_serial_redacts_bytes(): + assert secret.serial(b"topsecret") == secret.REDACT_PLACEHOLDER.encode() + + +def test_serial_leaves_empty_bytes(): + assert secret.serial(b"") == b"" + + def test_serial_redacts_masked_dict_strings(): d = secret.MaskedDict({"password": "hunter2", "count": 3}) result = secret.serial(d) - assert result == {"password": secret.REDACT_PLACEHOLDER, "count": 3} + assert result == { + "password": secret.REDACT_PLACEHOLDER, + "count": secret.REDACT_PLACEHOLDER, + } def test_serial_redacts_plain_dict_strings(): - # serial is aggressive — also redacts strings in plain dicts - d = {"k": "v", "n": 1} + # serial is aggressive — also redacts strings (and other truthy scalars) + # in plain dicts + d = {"k": "v", "n": 1, "z": 0} result = secret.serial(d) - assert result == {"k": secret.REDACT_PLACEHOLDER, "n": 1} + assert result == { + "k": secret.REDACT_PLACEHOLDER, + "n": secret.REDACT_PLACEHOLDER, + "z": 0, + } def test_serial_redacts_nested(): - d = secret.MaskedDict({"sub": {"s": "secret"}, "lst": ["a", 1]}) + d = secret.MaskedDict({"sub": {"s": "secret"}, "lst": ["a", 1, 0]}) result = secret.serial(d) assert result["sub"]["s"] == secret.REDACT_PLACEHOLDER assert result["lst"][0] == secret.REDACT_PLACEHOLDER - assert result["lst"][1] == 1 + assert result["lst"][1] == secret.REDACT_PLACEHOLDER + assert result["lst"][2] == 0 # --------------------------------------------------------------------------- @@ -300,10 +329,11 @@ def test_mask_output_redacts_masked_dict(): def test_mask_output_redacts_masked_list(): - d = {"items": secret.MaskedList(["sensitive", 1])} + d = {"items": secret.MaskedList(["sensitive", 1, 0])} result = secret.mask_output(d) assert result["items"][0] == secret.REDACT_PLACEHOLDER - assert result["items"][1] == 1 + assert result["items"][1] == secret.REDACT_PLACEHOLDER + assert result["items"][2] == 0 def test_mask_output_nested_plain_dicts_not_redacted(): @@ -401,3 +431,57 @@ def test_masked_nested_repr_respects_context_var(): r = repr(d) assert secret.REDACT_PLACEHOLDER in r assert "host1" not in r + + +# --------------------------------------------------------------------------- +# configure() / global masking killswitch (VCOPS-98852: pillar_mask_output) +# --------------------------------------------------------------------------- + + +def test_configure_defaults_to_enabled_when_opt_absent(): + secret.configure({}) + try: + assert secret.serial("hunter2") == secret.REDACT_PLACEHOLDER + finally: + secret.configure({"pillar_mask_output": True}) + + +def test_configure_true_enables_masking(): + secret.configure({"pillar_mask_output": True}) + try: + assert secret.serial("hunter2") == secret.REDACT_PLACEHOLDER + assert isinstance(secret.hide({"k": "v"}), secret.MaskedDict) + finally: + secret.configure({"pillar_mask_output": True}) + + +def test_configure_false_disables_serial_redaction(): + secret.configure({"pillar_mask_output": False}) + try: + assert secret.serial("hunter2") == "hunter2" + assert secret.serial(42) == 42 + d = secret.MaskedDict({"password": "hunter2"}) + assert secret.serial(d) == {"password": "hunter2"} + finally: + secret.configure({"pillar_mask_output": True}) + + +def test_configure_false_disables_hide_wrapping(): + secret.configure({"pillar_mask_output": False}) + try: + assert secret.hide({"k": "v"}) == {"k": "v"} + assert not isinstance(secret.hide({"k": "v"}), secret.MaskedDict) + assert secret.hide(["a"]) == ["a"] + assert not isinstance(secret.hide(["a"]), secret.MaskedList) + finally: + secret.configure({"pillar_mask_output": True}) + + +def test_configure_false_disables_mask_output(): + secret.configure({"pillar_mask_output": False}) + try: + d = {"pillar_data": secret.MaskedDict({"password": "secret"})} + result = secret.mask_output(d) + assert result["pillar_data"]["password"] == "secret" + finally: + secret.configure({"pillar_mask_output": True}) From 366eb8fc0a2bb5a2b6929ff67486f1257c0a7dcf Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Wed, 15 Jul 2026 14:05:13 -0600 Subject: [PATCH 2/6] Replace pillar_mask_output global killswitch with explicit opts reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review feedback on PR #69812: the module-level _ENABLED flag + configure() seeded from get_pillar() was a one-off pattern not used anywhere else in the codebase. pillar_merge_lists/pillar_safe_render_error are both read inline via self.opts.get(...)/__opts__.get(...) at each call site, with no caching. Replaced with an explicit enabled= parameter on hide()/serial()/ mask_output()/no_log_mask(), with every call site (salt/pillar/__init__.py, salt/modules/pillar.py, salt/client/ssh/wrapper/pillar.py, salt/state.py, salt/output/__init__.py) passing its own opts.get("pillar_mask_output", True) — matching the existing pillar boolean-option pattern exactly, no shared/global state left in salt.utils.secret. Co-Authored-By: Claude Sonnet 5 --- salt/client/ssh/wrapper/pillar.py | 4 +- salt/modules/pillar.py | 28 ++++++-- salt/output/__init__.py | 4 +- salt/pillar/__init__.py | 12 ++-- salt/state.py | 4 +- salt/utils/secret.py | 59 ++++++++--------- .../functional/pillar/test_pillar_masking.py | 34 ---------- tests/pytests/unit/modules/test_pillar.py | 17 +++++ tests/pytests/unit/utils/test_secret.py | 66 ++++++++----------- 9 files changed, 106 insertions(+), 122 deletions(-) diff --git a/salt/client/ssh/wrapper/pillar.py b/salt/client/ssh/wrapper/pillar.py index 2c36ec8d24c0..b49d4d758bf1 100644 --- a/salt/client/ssh/wrapper/pillar.py +++ b/salt/client/ssh/wrapper/pillar.py @@ -83,7 +83,9 @@ def item(*args): ret = {} for arg in args: try: - ret[arg] = salt.utils.secret.serial(__pillar__[arg]) + ret[arg] = salt.utils.secret.serial( + __pillar__[arg], enabled=__opts__.get("pillar_mask_output", True) + ) except KeyError: pass return ret diff --git a/salt/modules/pillar.py b/salt/modules/pillar.py index 72a1edb6b6e8..2fa9a6838be6 100644 --- a/salt/modules/pillar.py +++ b/salt/modules/pillar.py @@ -160,7 +160,9 @@ def get( ) if unmask: return salt.utils.secret.expose(merged) - return salt.utils.secret.serial(merged) + return salt.utils.secret.serial( + merged, enabled=__opts__.get("pillar_mask_output", True) + ) else: log.error( "pillar.get: Default (%s) is a dict, but the returned " @@ -179,7 +181,9 @@ def get( default.extend([x for x in ret if x not in default]) if unmask: return salt.utils.secret.expose(default) - return salt.utils.secret.serial(default) + return salt.utils.secret.serial( + default, enabled=__opts__.get("pillar_mask_output", True) + ) else: log.error( "pillar.get: Default (%s) is a list, but the returned " @@ -203,7 +207,9 @@ def get( if unmask: return salt.utils.secret.expose(ret) - return salt.utils.secret.serial(ret) + return salt.utils.secret.serial( + ret, enabled=__opts__.get("pillar_mask_output", True) + ) def items( @@ -301,7 +307,9 @@ def items( if unmask: return salt.utils.secret.expose(ret) else: - return salt.utils.secret.serial(ret) + return salt.utils.secret.serial( + ret, enabled=__opts__.get("pillar_mask_output", True) + ) # Allow pillar.data to also be used to return pillar data @@ -592,7 +600,9 @@ def item( if unmask: return salt.utils.secret.expose(ret) else: - return salt.utils.secret.serial(ret) + return salt.utils.secret.serial( + ret, enabled=__opts__.get("pillar_mask_output", True) + ) def raw(key=None, unmask=None): @@ -630,7 +640,9 @@ def raw(key=None, unmask=None): if unmask: return salt.utils.secret.expose(value) - return salt.utils.secret.serial(value) + return salt.utils.secret.serial( + value, enabled=__opts__.get("pillar_mask_output", True) + ) def ext(external, pillar=None, unmask=None): @@ -712,7 +724,9 @@ def ext(external, pillar=None, unmask=None): if unmask: return salt.utils.secret.expose(ret) - return salt.utils.secret.serial(ret) + return salt.utils.secret.serial( + ret, enabled=__opts__.get("pillar_mask_output", True) + ) def keys(key, delimiter=DEFAULT_TARGET_DELIM, unmask=None): diff --git a/salt/output/__init__.py b/salt/output/__init__.py index 1d6021528e72..66ebc317f370 100644 --- a/salt/output/__init__.py +++ b/salt/output/__init__.py @@ -32,7 +32,9 @@ def try_printout(data, out, opts, **kwargs): Safely get the string to print out, try the configured outputter, then fall back to nested and then to raw """ - data = salt.utils.secret.mask_output(data) + data = salt.utils.secret.mask_output( + data, enabled=opts.get("pillar_mask_output", True) + ) try: printout = get_printout(out, opts)(data, **kwargs) if printout is not None: diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 3005af834ecb..c46786065c81 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -46,9 +46,6 @@ def get_pillar( """ Return the correct pillar driver based on the file_client option """ - # Seed the pillar-masking killswitch from this process's own opts before - # any pillar compile/wrap happens (salt.utils.secret.hide()/serial()). - salt.utils.secret.configure(opts) # When file_client is 'local' this makes the minion masterless # but sometimes we want the minion to read its files from the local # filesystem instead of asking for them from the master, but still @@ -110,7 +107,6 @@ def get_async_pillar( """ Return the correct pillar driver based on the file_client option """ - salt.utils.secret.configure(opts) file_client = opts["file_client"] if opts.get("master_type") == "disable" and file_client == "remote": file_client = "local" @@ -283,7 +279,9 @@ async def compile_pillar(self): log.exception("Exception getting pillar:") raise SaltClientError("Exception getting pillar.") self.validate_return(ret_pillar) - ret_pillar = salt.utils.secret.hide(ret_pillar) + ret_pillar = salt.utils.secret.hide( + ret_pillar, enabled=self.opts.get("pillar_mask_output", True) + ) return ret_pillar def destroy(self): @@ -375,7 +373,9 @@ def compile_pillar(self): log.exception("Exception getting pillar:") raise SaltClientError("Exception getting pillar.") self.validate_return(ret_pillar) - return salt.utils.secret.hide(ret_pillar) + return salt.utils.secret.hide( + ret_pillar, enabled=self.opts.get("pillar_mask_output", True) + ) def destroy(self): if hasattr(self, "_closing") and self._closing: diff --git a/salt/state.py b/salt/state.py index 8178cb623bf7..09adb5074cfd 100644 --- a/salt/state.py +++ b/salt/state.py @@ -2457,7 +2457,9 @@ def call( ret["__run_num__"] = self.__run_num self.__run_num += 1 if low.get("no_log"): - salt.utils.secret.no_log_mask(ret) + salt.utils.secret.no_log_mask( + ret, enabled=self.opts.get("pillar_mask_output", True) + ) format_log(ret) self.check_refresh(low, ret) utc_finish_time = datetime.datetime.now(tz=datetime.timezone.utc) diff --git a/salt/utils/secret.py b/salt/utils/secret.py index 54489f967c6d..00b513845470 100644 --- a/salt/utils/secret.py +++ b/salt/utils/secret.py @@ -45,25 +45,6 @@ REDACT_PLACEHOLDER = "**********" -# Global on/off switch for the whole pillar-masking feature, seeded from the -# ``pillar_mask_output`` config option. Unlike ``mask_pillar`` above (a -# per-render-context toggle), this is an administrator-facing killswitch: -# when False, hide()/serial() never wrap or redact, regardless of context. -_ENABLED = True - - -def configure(opts): - """Seed the global masking killswitch from ``pillar_mask_output``. - - Called from ``salt.pillar.get_pillar()`` / ``get_async_pillar()`` — the - choke point where both minion and master-side pillar-compile flows - already receive the full ``opts`` dict — so this stays in sync with the - process's own config without threading an extra parameter through every - masking call site. - """ - global _ENABLED - _ENABLED = bool(opts.get("pillar_mask_output", True)) - # --------------------------------------------------------------------------- # Internal helpers @@ -232,16 +213,20 @@ def __deepcopy__(self, memo): # --------------------------------------------------------------------------- -def hide(value): +def hide(value, enabled=True): """Wrap a pillar dict/list in MaskedDict/MaskedList for display masking. Scalar values (str, int, bool, None …) are returned unchanged — they are stored plain inside the container and only redacted in the container's repr. Already-wrapped values are returned as-is (idempotent). - No-ops when the global masking killswitch (``pillar_mask_output``) is off. + enabled + Pass the caller's own ``opts.get("pillar_mask_output", True)`` (or + ``__opts__.get(...)``) — matches the existing pattern for + ``pillar_merge_lists``/``pillar_safe_render_error``, read at each + call site rather than cached. When ``False``, this is a no-op. """ - if not _ENABLED: + if not enabled: return value return _mask_wrap(value) @@ -277,7 +262,7 @@ def expose(value, _seen=None): return value -def serial(value, _seen=None): +def serial(value, _seen=None, enabled=True): """Aggressively redact: replace every non-empty/truthy scalar leaf value (str, bytes, int, float, bool) with a redacted placeholder. @@ -288,10 +273,15 @@ def serial(value, _seen=None): are stored unwrapped), this function must handle plain str/dict/list values in addition to MaskedDict / MaskedList containers. - No-ops (returns *value* unchanged) when the global masking killswitch - (``pillar_mask_output``) is off. + enabled + Pass the caller's own ``opts.get("pillar_mask_output", True)`` (or + ``__opts__.get(...)``) — matches the existing pattern for + ``pillar_merge_lists``/``pillar_safe_render_error``, read at each + call site rather than cached. When ``False``, this is a no-op. + Only checked on the outermost call; recursive calls omit it since + recursion only happens once the outermost call already found it True. """ - if not _ENABLED: + if not enabled: return value if _seen is None: _seen = set() @@ -326,7 +316,7 @@ def serial(value, _seen=None): _seen.discard(vid) -def mask_output(value, _seen=None): +def mask_output(value, _seen=None, enabled=True): """Gently redact: only redact values *inside* MaskedDict / MaskedList containers. Plain dicts, plain lists, and plain scalars pass through unchanged. @@ -334,9 +324,11 @@ def mask_output(value, _seen=None): leakage in general Salt output without redacting ordinary result strings (state comments, module names, etc.). - No-ops when the global masking killswitch (``pillar_mask_output``) is off. + enabled + Pass the caller's own ``opts.get("pillar_mask_output", True)``. When + ``False``, this is a no-op. Only checked on the outermost call. """ - if not _ENABLED: + if not enabled: return value if _seen is None: _seen = set() @@ -357,11 +349,14 @@ def mask_output(value, _seen=None): _seen.discard(vid) -def no_log_mask(state_ret): +def no_log_mask(state_ret, enabled=True): """Replace ``comment`` and ``changes`` in a state return with redacted values. Called by ``salt/state.py`` when a state has ``no_log: True``. Mutates *state_ret* in place. + + enabled + Pass the caller's own ``opts.get("pillar_mask_output", True)``. """ - state_ret["comment"] = serial(state_ret["comment"]) - state_ret["changes"] = serial(state_ret["changes"]) + state_ret["comment"] = serial(state_ret["comment"], enabled=enabled) + state_ret["changes"] = serial(state_ret["changes"], enabled=enabled) diff --git a/tests/pytests/functional/pillar/test_pillar_masking.py b/tests/pytests/functional/pillar/test_pillar_masking.py index 014d54af00a0..82ba16dd410d 100644 --- a/tests/pytests/functional/pillar/test_pillar_masking.py +++ b/tests/pytests/functional/pillar/test_pillar_masking.py @@ -117,37 +117,3 @@ def test_masked_pillar_redacts_outside_render_bracket(): assert salt.utils.secret.REDACT_PLACEHOLDER in repr(pillar) assert "host1" not in repr(pillar) assert salt.utils.secret.REDACT_PLACEHOLDER in str(pillar["hosts"]) - - -def test_get_pillar_wires_pillar_mask_output_config_option(minion_opts, grains): - """VCOPS-98852: ``pillar_mask_output`` is the standard-config-path toggle. - - ``salt.pillar.get_pillar()`` is the choke point that receives ``opts`` - for every pillar-compile flow (minion and master-side); it must seed - ``salt.utils.secret``'s global killswitch so hide()/serial() honor the - config option without every consumer having to thread opts through. - """ - opts = dict(minion_opts) - opts["file_client"] = "local" - opts["pillar_cache"] = False - opts["minion_data_cache"] = False - - try: - opts["pillar_mask_output"] = False - salt.pillar.get_pillar(opts, grains, "test-minion", "base") - assert salt.utils.secret.hide({"k": "v"}) == {"k": "v"} - assert not isinstance( - salt.utils.secret.hide({"k": "v"}), salt.utils.secret.MaskedDict - ) - assert salt.utils.secret.serial("hunter2") == "hunter2" - - opts["pillar_mask_output"] = True - salt.pillar.get_pillar(opts, grains, "test-minion", "base") - assert isinstance( - salt.utils.secret.hide({"k": "v"}), salt.utils.secret.MaskedDict - ) - assert ( - salt.utils.secret.serial("hunter2") == salt.utils.secret.REDACT_PLACEHOLDER - ) - finally: - salt.utils.secret.configure({"pillar_mask_output": True}) diff --git a/tests/pytests/unit/modules/test_pillar.py b/tests/pytests/unit/modules/test_pillar.py index f3f7c9c81af9..8d634f5eab38 100644 --- a/tests/pytests/unit/modules/test_pillar.py +++ b/tests/pytests/unit/modules/test_pillar.py @@ -160,6 +160,23 @@ def test_pillar_get_default_merge_regression_38558(): assert {"l2": {"l3": 43}} == default +def test_pillar_get_respects_pillar_mask_output_config_option(): + """VCOPS-98852: ``pillar_mask_output: False`` disables masking end-to-end + through the standard ``pillar.get`` execution module, reading ``__opts__`` + directly at the call site (matches the existing ``pillar_merge_lists`` + pattern — no cached/global state in ``salt.utils.secret``). + """ + with patch.dict(pillarmod.__pillar__, {"pin": 1234}), patch.dict( + pillarmod.__opts__, {"pillar_mask_output": False} + ): + assert pillarmod.get(key="pin") == 1234 + + with patch.dict(pillarmod.__pillar__, {"pin": 1234}), patch.dict( + pillarmod.__opts__, {"pillar_mask_output": True} + ): + assert pillarmod.get(key="pin") == secret.REDACT_PLACEHOLDER + + def test_pillar_get_default_merge_regression_39062(): """ Confirm that we do not raise an exception if default is None and diff --git a/tests/pytests/unit/utils/test_secret.py b/tests/pytests/unit/utils/test_secret.py index 9b53ad2c3ea8..98b35e4a2fc4 100644 --- a/tests/pytests/unit/utils/test_secret.py +++ b/tests/pytests/unit/utils/test_secret.py @@ -434,54 +434,40 @@ def test_masked_nested_repr_respects_context_var(): # --------------------------------------------------------------------------- -# configure() / global masking killswitch (VCOPS-98852: pillar_mask_output) +# enabled= parameter (VCOPS-98852: pillar_mask_output) — each call site reads +# its own opts.get("pillar_mask_output", True) and passes it in explicitly, +# matching the existing pillar_merge_lists/pillar_safe_render_error pattern +# (no cached/global state in this module). # --------------------------------------------------------------------------- -def test_configure_defaults_to_enabled_when_opt_absent(): - secret.configure({}) - try: - assert secret.serial("hunter2") == secret.REDACT_PLACEHOLDER - finally: - secret.configure({"pillar_mask_output": True}) +def test_enabled_defaults_to_true(): + # Callers that don't pass enabled= (or pass True) keep masking on. + assert secret.serial("hunter2") == secret.REDACT_PLACEHOLDER + assert isinstance(secret.hide({"k": "v"}), secret.MaskedDict) -def test_configure_true_enables_masking(): - secret.configure({"pillar_mask_output": True}) - try: - assert secret.serial("hunter2") == secret.REDACT_PLACEHOLDER - assert isinstance(secret.hide({"k": "v"}), secret.MaskedDict) - finally: - secret.configure({"pillar_mask_output": True}) +def test_enabled_false_disables_serial_redaction(): + assert secret.serial("hunter2", enabled=False) == "hunter2" + assert secret.serial(42, enabled=False) == 42 + d = secret.MaskedDict({"password": "hunter2"}) + assert secret.serial(d, enabled=False) == {"password": "hunter2"} -def test_configure_false_disables_serial_redaction(): - secret.configure({"pillar_mask_output": False}) - try: - assert secret.serial("hunter2") == "hunter2" - assert secret.serial(42) == 42 - d = secret.MaskedDict({"password": "hunter2"}) - assert secret.serial(d) == {"password": "hunter2"} - finally: - secret.configure({"pillar_mask_output": True}) +def test_enabled_false_disables_hide_wrapping(): + assert secret.hide({"k": "v"}, enabled=False) == {"k": "v"} + assert not isinstance(secret.hide({"k": "v"}, enabled=False), secret.MaskedDict) + assert secret.hide(["a"], enabled=False) == ["a"] + assert not isinstance(secret.hide(["a"], enabled=False), secret.MaskedList) -def test_configure_false_disables_hide_wrapping(): - secret.configure({"pillar_mask_output": False}) - try: - assert secret.hide({"k": "v"}) == {"k": "v"} - assert not isinstance(secret.hide({"k": "v"}), secret.MaskedDict) - assert secret.hide(["a"]) == ["a"] - assert not isinstance(secret.hide(["a"]), secret.MaskedList) - finally: - secret.configure({"pillar_mask_output": True}) +def test_enabled_false_disables_mask_output(): + d = {"pillar_data": secret.MaskedDict({"password": "secret"})} + result = secret.mask_output(d, enabled=False) + assert result["pillar_data"]["password"] == "secret" -def test_configure_false_disables_mask_output(): - secret.configure({"pillar_mask_output": False}) - try: - d = {"pillar_data": secret.MaskedDict({"password": "secret"})} - result = secret.mask_output(d) - assert result["pillar_data"]["password"] == "secret" - finally: - secret.configure({"pillar_mask_output": True}) +def test_enabled_false_disables_no_log_mask(): + ret = {"comment": "plaintext_password", "changes": {}, "result": True} + secret.no_log_mask(ret, enabled=False) + assert ret["comment"] == "plaintext_password" From ccd8e7668a4cee58424ea00168c03f11f8b23b31 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Thu, 16 Jul 2026 11:33:25 -0600 Subject: [PATCH 3/6] Narrow pillar_mask_output to only change pillar.items()'s default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per maintainer feedback on PR #69812: "Don't disable masking wholesale .. The config option should only change the default for pillar.items." Reverted the enabled= parameter and every call site outside pillar.items() (hide()/serial()/mask_output()/no_log_mask() in salt/utils/secret.py are back to their original signatures; salt/pillar/__init__.py, salt/client/ssh/wrapper/pillar.py, salt/state.py, salt/output/__init__.py are unchanged). pillar_mask_output now only affects the unmask-default computation inside salt.modules.pillar.items() — pillar.get/item/raw/ext, no_log state output, and the general CLI output safety net keep masking by default regardless of this option. Callers can still always override via pillar.items(unmask=True/False) explicitly. Updated config/doc/changelog wording and tests to match the narrower scope (added test_items_respects_pillar_mask_output_config_option and test_pillar_get_ignores_pillar_mask_output_config_option). Co-Authored-By: Claude Sonnet 5 --- changelog/98852.added.md | 2 +- doc/ref/configuration/master.rst | 21 +++++--- salt/client/ssh/wrapper/pillar.py | 4 +- salt/config/__init__.py | 5 +- salt/modules/pillar.py | 42 +++++++--------- salt/output/__init__.py | 4 +- salt/pillar/__init__.py | 8 +-- salt/state.py | 4 +- salt/utils/secret.py | 39 +++------------ tests/pytests/unit/modules/test_pillar.py | 61 +++++++++++++++++++---- tests/pytests/unit/utils/test_secret.py | 40 --------------- 11 files changed, 98 insertions(+), 132 deletions(-) diff --git a/changelog/98852.added.md b/changelog/98852.added.md index 915807e8e6c7..4381bd453c03 100644 --- a/changelog/98852.added.md +++ b/changelog/98852.added.md @@ -1 +1 @@ -Added the ``pillar_mask_output`` master/minion config option to globally enable or disable pillar output masking (redaction of sensitive pillar values in ``pillar.get``/``pillar.items``/etc., ``no_log`` state output, and general CLI output). Defaults to ``True`` (masking stays on), matching existing behavior. +Added the ``pillar_mask_output`` master/minion config option. When set to ``False``, changes ``pillar.items``'s default (when the caller doesn't pass ``unmask``) to return unmasked pillar values, for sites relying on the pre-masking ``pillar.items`` behavior. Defaults to ``True`` (masked, matching existing behavior) and does not affect ``pillar.get``/``item``/``raw``/``ext``, ``no_log`` state output, or general CLI output, which keep redacting by default regardless of this setting. diff --git a/doc/ref/configuration/master.rst b/doc/ref/configuration/master.rst index 4a4d7317369e..0d7b382e8869 100644 --- a/doc/ref/configuration/master.rst +++ b/doc/ref/configuration/master.rst @@ -5671,14 +5671,19 @@ Recursively merge lists by aggregating them instead of replacing them. Default: ``True`` -Globally enable or disable redaction of pillar values in logs and state -output. When ``True`` (the default), sensitive pillar values are replaced -with ``**********`` in ``pillar.get`` and related execution module output, -``no_log`` state results, and general CLI output, unless a caller explicitly -requests the real value (e.g. ``pillar.get(key, unmask=True)``). - -Set this option to ``False`` to disable pillar masking entirely and always -return real values, matching pre-masking behavior. +Changes the *default* behavior of :py:func:`pillar.items +` when a caller doesn't explicitly pass +``unmask``. When ``True`` (the default), ``pillar.items`` returns masked +values (``**********``) by default, matching :py:func:`pillar.get +` and friends. Set to ``False`` to make +``pillar.items`` default to returning real, unmasked values instead — +useful for sites relying on the pre-masking ``pillar.items`` behavior. + +This option does **not** disable pillar masking elsewhere: ``pillar.get``, +``pillar.item``, ``pillar.raw``, ``pillar.ext``, ``no_log`` state output, +and the general CLI output safety net are unaffected and keep redacting by +default regardless of this setting. Callers of ``pillar.items`` can always +override the default explicitly with ``unmask=True``/``unmask=False``. .. code-block:: yaml diff --git a/salt/client/ssh/wrapper/pillar.py b/salt/client/ssh/wrapper/pillar.py index b49d4d758bf1..2c36ec8d24c0 100644 --- a/salt/client/ssh/wrapper/pillar.py +++ b/salt/client/ssh/wrapper/pillar.py @@ -83,9 +83,7 @@ def item(*args): ret = {} for arg in args: try: - ret[arg] = salt.utils.secret.serial( - __pillar__[arg], enabled=__opts__.get("pillar_mask_output", True) - ) + ret[arg] = salt.utils.secret.serial(__pillar__[arg]) except KeyError: pass return ret diff --git a/salt/config/__init__.py b/salt/config/__init__.py index 7476623259f3..a9b1c60a5bef 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -698,8 +698,9 @@ def _gather_buffer_space(): "pillar_source_merging_strategy": str, # Recursively merge lists by aggregating them instead of replacing them. "pillar_merge_lists": bool, - # Globally enable/disable redaction of pillar values in logs and state - # output (pillar.get, no_log states, CLI output, etc.). + # When False, changes pillar.items()'s default (when the caller + # doesn't pass unmask=) to return unmasked pillar values. Does not + # affect pillar.get/item/raw/ext, no_log states, or general output. "pillar_mask_output": bool, # If True, values from included pillar SLS targets will override "pillar_includes_override_sls": bool, diff --git a/salt/modules/pillar.py b/salt/modules/pillar.py index 2fa9a6838be6..4b518fba8ab0 100644 --- a/salt/modules/pillar.py +++ b/salt/modules/pillar.py @@ -160,9 +160,7 @@ def get( ) if unmask: return salt.utils.secret.expose(merged) - return salt.utils.secret.serial( - merged, enabled=__opts__.get("pillar_mask_output", True) - ) + return salt.utils.secret.serial(merged) else: log.error( "pillar.get: Default (%s) is a dict, but the returned " @@ -181,9 +179,7 @@ def get( default.extend([x for x in ret if x not in default]) if unmask: return salt.utils.secret.expose(default) - return salt.utils.secret.serial( - default, enabled=__opts__.get("pillar_mask_output", True) - ) + return salt.utils.secret.serial(default) else: log.error( "pillar.get: Default (%s) is a list, but the returned " @@ -207,9 +203,7 @@ def get( if unmask: return salt.utils.secret.expose(ret) - return salt.utils.secret.serial( - ret, enabled=__opts__.get("pillar_mask_output", True) - ) + return salt.utils.secret.serial(ret) def items( @@ -260,7 +254,10 @@ def items( :conf_minion:`pillarenv_from_saltenv`, and is otherwise ignored. unmask - If set to ``True``, the pillar data will be unmasked. + If set to ``True``, the pillar data will be unmasked. If not set, the + default is unmasked when either the current render context has + already disabled masking, or the :conf_minion:`pillar_mask_output` + config option is set to ``False``. .. versionadded:: 3008.0 @@ -303,13 +300,18 @@ def items( ) ret = pillar.compile_pillar() if unmask is None: - unmask = not salt.utils.secret.mask_pillar.get() + # VCOPS-98852: pillar_mask_output only changes items()'s *default* + # when the caller didn't explicitly request masked/unmasked output — + # it does not disable masking elsewhere (pillar.get/item/raw/ext, + # no_log states, or the general output safety net keep their own + # existing behavior regardless of this option). + unmask = not salt.utils.secret.mask_pillar.get() or not __opts__.get( + "pillar_mask_output", True + ) if unmask: return salt.utils.secret.expose(ret) else: - return salt.utils.secret.serial( - ret, enabled=__opts__.get("pillar_mask_output", True) - ) + return salt.utils.secret.serial(ret) # Allow pillar.data to also be used to return pillar data @@ -600,9 +602,7 @@ def item( if unmask: return salt.utils.secret.expose(ret) else: - return salt.utils.secret.serial( - ret, enabled=__opts__.get("pillar_mask_output", True) - ) + return salt.utils.secret.serial(ret) def raw(key=None, unmask=None): @@ -640,9 +640,7 @@ def raw(key=None, unmask=None): if unmask: return salt.utils.secret.expose(value) - return salt.utils.secret.serial( - value, enabled=__opts__.get("pillar_mask_output", True) - ) + return salt.utils.secret.serial(value) def ext(external, pillar=None, unmask=None): @@ -724,9 +722,7 @@ def ext(external, pillar=None, unmask=None): if unmask: return salt.utils.secret.expose(ret) - return salt.utils.secret.serial( - ret, enabled=__opts__.get("pillar_mask_output", True) - ) + return salt.utils.secret.serial(ret) def keys(key, delimiter=DEFAULT_TARGET_DELIM, unmask=None): diff --git a/salt/output/__init__.py b/salt/output/__init__.py index 66ebc317f370..1d6021528e72 100644 --- a/salt/output/__init__.py +++ b/salt/output/__init__.py @@ -32,9 +32,7 @@ def try_printout(data, out, opts, **kwargs): Safely get the string to print out, try the configured outputter, then fall back to nested and then to raw """ - data = salt.utils.secret.mask_output( - data, enabled=opts.get("pillar_mask_output", True) - ) + data = salt.utils.secret.mask_output(data) try: printout = get_printout(out, opts)(data, **kwargs) if printout is not None: diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index c46786065c81..399a2660c9ab 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -279,9 +279,7 @@ async def compile_pillar(self): log.exception("Exception getting pillar:") raise SaltClientError("Exception getting pillar.") self.validate_return(ret_pillar) - ret_pillar = salt.utils.secret.hide( - ret_pillar, enabled=self.opts.get("pillar_mask_output", True) - ) + ret_pillar = salt.utils.secret.hide(ret_pillar) return ret_pillar def destroy(self): @@ -373,9 +371,7 @@ def compile_pillar(self): log.exception("Exception getting pillar:") raise SaltClientError("Exception getting pillar.") self.validate_return(ret_pillar) - return salt.utils.secret.hide( - ret_pillar, enabled=self.opts.get("pillar_mask_output", True) - ) + return salt.utils.secret.hide(ret_pillar) def destroy(self): if hasattr(self, "_closing") and self._closing: diff --git a/salt/state.py b/salt/state.py index 09adb5074cfd..8178cb623bf7 100644 --- a/salt/state.py +++ b/salt/state.py @@ -2457,9 +2457,7 @@ def call( ret["__run_num__"] = self.__run_num self.__run_num += 1 if low.get("no_log"): - salt.utils.secret.no_log_mask( - ret, enabled=self.opts.get("pillar_mask_output", True) - ) + salt.utils.secret.no_log_mask(ret) format_log(ret) self.check_refresh(low, ret) utc_finish_time = datetime.datetime.now(tz=datetime.timezone.utc) diff --git a/salt/utils/secret.py b/salt/utils/secret.py index 00b513845470..4411a98c1c9e 100644 --- a/salt/utils/secret.py +++ b/salt/utils/secret.py @@ -213,21 +213,13 @@ def __deepcopy__(self, memo): # --------------------------------------------------------------------------- -def hide(value, enabled=True): +def hide(value): """Wrap a pillar dict/list in MaskedDict/MaskedList for display masking. Scalar values (str, int, bool, None …) are returned unchanged — they are stored plain inside the container and only redacted in the container's repr. Already-wrapped values are returned as-is (idempotent). - - enabled - Pass the caller's own ``opts.get("pillar_mask_output", True)`` (or - ``__opts__.get(...)``) — matches the existing pattern for - ``pillar_merge_lists``/``pillar_safe_render_error``, read at each - call site rather than cached. When ``False``, this is a no-op. """ - if not enabled: - return value return _mask_wrap(value) @@ -262,7 +254,7 @@ def expose(value, _seen=None): return value -def serial(value, _seen=None, enabled=True): +def serial(value, _seen=None): """Aggressively redact: replace every non-empty/truthy scalar leaf value (str, bytes, int, float, bool) with a redacted placeholder. @@ -272,17 +264,7 @@ def serial(value, _seen=None, enabled=True): Because ``MaskedDict.__getitem__`` returns plain strings (the scalar leaves are stored unwrapped), this function must handle plain str/dict/list values in addition to MaskedDict / MaskedList containers. - - enabled - Pass the caller's own ``opts.get("pillar_mask_output", True)`` (or - ``__opts__.get(...)``) — matches the existing pattern for - ``pillar_merge_lists``/``pillar_safe_render_error``, read at each - call site rather than cached. When ``False``, this is a no-op. - Only checked on the outermost call; recursive calls omit it since - recursion only happens once the outermost call already found it True. """ - if not enabled: - return value if _seen is None: _seen = set() if isinstance(value, bytes) and _is_redactable_scalar(value): @@ -316,20 +298,14 @@ def serial(value, _seen=None, enabled=True): _seen.discard(vid) -def mask_output(value, _seen=None, enabled=True): +def mask_output(value, _seen=None): """Gently redact: only redact values *inside* MaskedDict / MaskedList containers. Plain dicts, plain lists, and plain scalars pass through unchanged. Use as a safety net in ``output/__init__.py`` to prevent accidental pillar leakage in general Salt output without redacting ordinary result strings (state comments, module names, etc.). - - enabled - Pass the caller's own ``opts.get("pillar_mask_output", True)``. When - ``False``, this is a no-op. Only checked on the outermost call. """ - if not enabled: - return value if _seen is None: _seen = set() if isinstance(value, (MaskedDict, MaskedList)): @@ -349,14 +325,11 @@ def mask_output(value, _seen=None, enabled=True): _seen.discard(vid) -def no_log_mask(state_ret, enabled=True): +def no_log_mask(state_ret): """Replace ``comment`` and ``changes`` in a state return with redacted values. Called by ``salt/state.py`` when a state has ``no_log: True``. Mutates *state_ret* in place. - - enabled - Pass the caller's own ``opts.get("pillar_mask_output", True)``. """ - state_ret["comment"] = serial(state_ret["comment"], enabled=enabled) - state_ret["changes"] = serial(state_ret["changes"], enabled=enabled) + state_ret["comment"] = serial(state_ret["comment"]) + state_ret["changes"] = serial(state_ret["changes"]) diff --git a/tests/pytests/unit/modules/test_pillar.py b/tests/pytests/unit/modules/test_pillar.py index 8d634f5eab38..095e2bff0093 100644 --- a/tests/pytests/unit/modules/test_pillar.py +++ b/tests/pytests/unit/modules/test_pillar.py @@ -160,19 +160,60 @@ def test_pillar_get_default_merge_regression_38558(): assert {"l2": {"l3": 43}} == default -def test_pillar_get_respects_pillar_mask_output_config_option(): - """VCOPS-98852: ``pillar_mask_output: False`` disables masking end-to-end - through the standard ``pillar.get`` execution module, reading ``__opts__`` - directly at the call site (matches the existing ``pillar_merge_lists`` - pattern — no cached/global state in ``salt.utils.secret``). +def test_items_respects_pillar_mask_output_config_option(): + """VCOPS-98852: ``pillar_mask_output`` only changes ``pillar.items``'s + *default* (when the caller doesn't pass ``unmask``) — per maintainer + feedback on saltstack/salt#69812, it must not disable masking wholesale. + """ + compiled = {"pin": 1234} + pillar_obj = MagicMock() + pillar_obj.compile_pillar = MagicMock(return_value=compiled) + grains = MagicMock() + grains.value = MagicMock(return_value={}) + with patch( + "salt.pillar.get_pillar", MagicMock(return_value=pillar_obj) + ), patch.object(pillarmod, "__grains__", grains, create=True): + with patch.dict( + pillarmod.__opts__, + { + "id": "minion", + "saltenv": "base", + "pillarenv": None, + "pillar_mask_output": False, + }, + ): + assert pillarmod.items() == compiled + + with patch.dict( + pillarmod.__opts__, + { + "id": "minion", + "saltenv": "base", + "pillarenv": None, + "pillar_mask_output": True, + }, + ): + assert pillarmod.items() == {"pin": secret.REDACT_PLACEHOLDER} + + # The caller's explicit unmask= always wins over the config default. + with patch.dict( + pillarmod.__opts__, + { + "id": "minion", + "saltenv": "base", + "pillarenv": None, + "pillar_mask_output": False, + }, + ): + assert pillarmod.items(unmask=False) == {"pin": secret.REDACT_PLACEHOLDER} + + +def test_pillar_get_ignores_pillar_mask_output_config_option(): + """VCOPS-98852: ``pillar.get`` must keep masking by default regardless of + ``pillar_mask_output`` — that option only affects ``pillar.items``. """ with patch.dict(pillarmod.__pillar__, {"pin": 1234}), patch.dict( pillarmod.__opts__, {"pillar_mask_output": False} - ): - assert pillarmod.get(key="pin") == 1234 - - with patch.dict(pillarmod.__pillar__, {"pin": 1234}), patch.dict( - pillarmod.__opts__, {"pillar_mask_output": True} ): assert pillarmod.get(key="pin") == secret.REDACT_PLACEHOLDER diff --git a/tests/pytests/unit/utils/test_secret.py b/tests/pytests/unit/utils/test_secret.py index 98b35e4a2fc4..52d97d747972 100644 --- a/tests/pytests/unit/utils/test_secret.py +++ b/tests/pytests/unit/utils/test_secret.py @@ -431,43 +431,3 @@ def test_masked_nested_repr_respects_context_var(): r = repr(d) assert secret.REDACT_PLACEHOLDER in r assert "host1" not in r - - -# --------------------------------------------------------------------------- -# enabled= parameter (VCOPS-98852: pillar_mask_output) — each call site reads -# its own opts.get("pillar_mask_output", True) and passes it in explicitly, -# matching the existing pillar_merge_lists/pillar_safe_render_error pattern -# (no cached/global state in this module). -# --------------------------------------------------------------------------- - - -def test_enabled_defaults_to_true(): - # Callers that don't pass enabled= (or pass True) keep masking on. - assert secret.serial("hunter2") == secret.REDACT_PLACEHOLDER - assert isinstance(secret.hide({"k": "v"}), secret.MaskedDict) - - -def test_enabled_false_disables_serial_redaction(): - assert secret.serial("hunter2", enabled=False) == "hunter2" - assert secret.serial(42, enabled=False) == 42 - d = secret.MaskedDict({"password": "hunter2"}) - assert secret.serial(d, enabled=False) == {"password": "hunter2"} - - -def test_enabled_false_disables_hide_wrapping(): - assert secret.hide({"k": "v"}, enabled=False) == {"k": "v"} - assert not isinstance(secret.hide({"k": "v"}, enabled=False), secret.MaskedDict) - assert secret.hide(["a"], enabled=False) == ["a"] - assert not isinstance(secret.hide(["a"], enabled=False), secret.MaskedList) - - -def test_enabled_false_disables_mask_output(): - d = {"pillar_data": secret.MaskedDict({"password": "secret"})} - result = secret.mask_output(d, enabled=False) - assert result["pillar_data"]["password"] == "secret" - - -def test_enabled_false_disables_no_log_mask(): - ret = {"comment": "plaintext_password", "changes": {}, "result": True} - secret.no_log_mask(ret, enabled=False) - assert ret["comment"] == "plaintext_password" From 5997f65afa1eaeccded0acf7a87fb5960ba38ed7 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Mon, 20 Jul 2026 16:23:43 -0600 Subject: [PATCH 4/6] Redact name under no_log and scan for pillar secrets regardless of no_log VCOPS-77716 follow-up: no_log_mask() only masked comment/changes, leaving state name plaintext even under no_log: True. And secrets templated into non-no_log output (e.g. cmd.run stdout) were never scanned at all, so an operator had to remember no_log: True for every state that might echo a pillar value back. Adds redact_state_ret_secrets()/redact_known_secrets()/ _collect_secret_literals() to salt/utils/secret.py: flattens the minion's compiled pillar into known secret literals (longest-first, >= 6 chars to avoid over-redacting trivial strings) and does literal-substring redaction on name/comment/changes for every state return, unconditionally. Called from salt/state.py before the existing no_log_mask() gate. --- salt/state.py | 1 + salt/utils/secret.py | 82 ++++++++++++++++++++- tests/pytests/unit/utils/test_secret.py | 96 ++++++++++++++++++++++++- 3 files changed, 176 insertions(+), 3 deletions(-) diff --git a/salt/state.py b/salt/state.py index 8178cb623bf7..e454fe7f6098 100644 --- a/salt/state.py +++ b/salt/state.py @@ -2456,6 +2456,7 @@ def call( ret["__sls__"] = low.get("__sls__") ret["__run_num__"] = self.__run_num self.__run_num += 1 + salt.utils.secret.redact_state_ret_secrets(ret, self.opts.get("pillar")) if low.get("no_log"): salt.utils.secret.no_log_mask(ret) format_log(ret) diff --git a/salt/utils/secret.py b/salt/utils/secret.py index 4411a98c1c9e..811451702afc 100644 --- a/salt/utils/secret.py +++ b/salt/utils/secret.py @@ -26,6 +26,11 @@ # Safety net for general output (output/__init__.py) mask_output(state_return_data) # no-op for plain data + + # Literal-secret scan on every state return (salt/state.py), regardless + # of no_log — catches a pillar secret templated into name/comment/changes + # (e.g. echoed back by cmd.run) even when the operator forgot no_log. + redact_state_ret_secrets(ret, opts.get("pillar")) """ from __future__ import annotations @@ -326,10 +331,85 @@ def mask_output(value, _seen=None): def no_log_mask(state_ret): - """Replace ``comment`` and ``changes`` in a state return with redacted values. + """Replace ``name``, ``comment``, and ``changes`` in a state return with + redacted values. Called by ``salt/state.py`` when a state has ``no_log: True``. Mutates *state_ret* in place. """ + state_ret["name"] = serial(state_ret["name"]) state_ret["comment"] = serial(state_ret["comment"]) state_ret["changes"] = serial(state_ret["changes"]) + + +# Minimum length for a pillar leaf value to be treated as a "known secret" +# for literal-substring scanning. Without a floor, short/common strings +# ("true", "1", "yes") would get redacted anywhere they happen to appear in +# unrelated output. +_MIN_SECRET_LEN = 6 + + +def _collect_secret_literals(pillar) -> list: + """Flatten *pillar* into the ``str`` leaf values worth scanning for. + + Returned longest-first so a short secret can't mask inside a longer one + during substring replacement (e.g. "pass" clobbering "pass1234"). + """ + literals = set() + + def _walk(value): + if isinstance(value, dict): + items = ( + dict.items(value) if isinstance(value, MaskedDict) else value.items() + ) + for _, v in items: + _walk(v) + elif isinstance(value, list): + it = list.__iter__(value) if isinstance(value, MaskedList) else iter(value) + for v in it: + _walk(v) + elif isinstance(value, str) and len(value) >= _MIN_SECRET_LEN: + literals.add(value) + + _walk(pillar) + return sorted(literals, key=len, reverse=True) + + +def redact_known_secrets(value, secrets): + """Redact literal occurrences of *secrets* (longest-first) inside *value*. + + Unlike ``mask_output``, this scans ordinary strings — state ``name``, + ``comment``, ``changes.stdout``, etc. — for pillar secret values that + leaked into output through templating (e.g. a ``cmd.run`` that echoes a + pillar value back), not just ``MaskedDict``/``MaskedList`` containers. + """ + if not secrets: + return value + if isinstance(value, str): + for secret_value in secrets: + if secret_value in value: + value = value.replace(secret_value, REDACT_PLACEHOLDER) + return value + if isinstance(value, dict): + items = dict.items(value) if isinstance(value, MaskedDict) else value.items() + return {k: redact_known_secrets(v, secrets) for k, v in items} + if isinstance(value, list): + it = list.__iter__(value) if isinstance(value, MaskedList) else iter(value) + return [redact_known_secrets(v, secrets) for v in it] + return value + + +def redact_state_ret_secrets(state_ret, pillar): + """Scan a state return for literal pillar secret values and redact them. + + Called unconditionally in ``salt/state.py`` — regardless of ``no_log`` — + so a secret templated into ``name``/``comment``/``changes`` doesn't leak + in plaintext just because the state didn't opt into ``no_log: True``. + Mutates *state_ret* in place. + """ + secrets = _collect_secret_literals(pillar) + if not secrets: + return + for field in ("name", "comment", "changes"): + if field in state_ret: + state_ret[field] = redact_known_secrets(state_ret[field], secrets) diff --git a/tests/pytests/unit/utils/test_secret.py b/tests/pytests/unit/utils/test_secret.py index 52d97d747972..57c276098329 100644 --- a/tests/pytests/unit/utils/test_secret.py +++ b/tests/pytests/unit/utils/test_secret.py @@ -349,7 +349,12 @@ def test_mask_output_nested_plain_dicts_not_redacted(): def test_no_log_mask_redacts_comment(): - ret = {"comment": "Executed command", "changes": {}, "result": True} + ret = { + "name": "irrelevant", + "comment": "Executed command", + "changes": {}, + "result": True, + } secret.no_log_mask(ret) assert ret["comment"] == secret.REDACT_PLACEHOLDER assert ret["result"] is True # result is not touched @@ -357,6 +362,7 @@ def test_no_log_mask_redacts_comment(): def test_no_log_mask_redacts_changes(): ret = { + "name": "irrelevant", "comment": "ok", "changes": {"before": "plaintext_password", "after": "new_pass"}, "result": True, @@ -367,11 +373,97 @@ def test_no_log_mask_redacts_changes(): def test_no_log_mask_empty_comment(): - ret = {"comment": "", "changes": {}, "result": True} + ret = {"name": "irrelevant", "comment": "", "changes": {}, "result": True} secret.no_log_mask(ret) assert ret["comment"] == "" # empty string not redacted +def test_no_log_mask_redacts_name(): + ret = { + "name": "echo 'key sk-test-ABCDEF123456'", + "comment": "ok", + "changes": {}, + "result": True, + } + secret.no_log_mask(ret) + assert ret["name"] == secret.REDACT_PLACEHOLDER + + +# --------------------------------------------------------------------------- +# redact_known_secrets() / redact_state_ret_secrets() +# --------------------------------------------------------------------------- + + +def test_redact_known_secrets_redacts_substring_in_string(): + result = secret.redact_known_secrets( + "Connecting with key sk-test-ABCDEF123456", ["sk-test-ABCDEF123456"] + ) + assert result == f"Connecting with key {secret.REDACT_PLACEHOLDER}" + + +def test_redact_known_secrets_no_secrets_is_noop(): + assert secret.redact_known_secrets("plain text", []) == "plain text" + + +def test_redact_known_secrets_longest_first_avoids_partial_corruption(): + # "password" is a substring of "password1234secret" — redacting the + # shorter one first would leave a mangled remainder instead of a clean + # placeholder for the longer secret. + result = secret.redact_known_secrets( + "value=password1234secret", ["password1234secret", "password"] + ) + assert result == f"value={secret.REDACT_PLACEHOLDER}" + + +def test_redact_known_secrets_recurses_into_dict_and_list(): + value = {"stdout": "key: sk-test-ABCDEF123456", "lines": ["sk-test-ABCDEF123456"]} + result = secret.redact_known_secrets(value, ["sk-test-ABCDEF123456"]) + assert result == { + "stdout": f"key: {secret.REDACT_PLACEHOLDER}", + "lines": [secret.REDACT_PLACEHOLDER], + } + + +def test_collect_secret_literals_filters_short_strings(): + # Below _MIN_SECRET_LEN — must not be treated as a scannable secret. + literals = secret._collect_secret_literals({"flag": "true", "id": "1"}) + assert literals == [] + + +def test_redact_state_ret_secrets_redacts_without_no_log(): + """The gap this closes: a pillar secret echoed into stdout must be + redacted even when the state never set ``no_log: True``.""" + pillar = {"gpg_test_key": "sk-test-ABCDEF123456"} + ret = { + "name": "echo 'Connecting with key sk-test-ABCDEF123456'; exit 1", + "comment": "Command failed", + "changes": {"stdout": "Connecting with key sk-test-ABCDEF123456"}, + "result": False, + } + secret.redact_state_ret_secrets(ret, pillar) + assert secret.REDACT_PLACEHOLDER in ret["name"] + assert "sk-test-ABCDEF123456" not in ret["name"] + assert "sk-test-ABCDEF123456" not in ret["changes"]["stdout"] + + +def test_redact_state_ret_secrets_no_pillar_is_noop(): + ret = {"name": "echo hello", "comment": "ok", "changes": {}} + secret.redact_state_ret_secrets(ret, None) + assert ret["name"] == "echo hello" + + +def test_redact_state_ret_secrets_works_with_masked_pillar(): + pillar = secret.hide({"gpg_test_key": "sk-test-ABCDEF123456"}) + ret = { + "name": "sk-test-ABCDEF123456", + "comment": "ok", + "changes": {}, + "result": True, + } + secret.redact_state_ret_secrets(ret, pillar) + assert ret["name"] == secret.REDACT_PLACEHOLDER + + # --------------------------------------------------------------------------- # mask_pillar ContextVar gates container repr # --------------------------------------------------------------------------- From 8b27b5fe9b4248ff9deeca303db5c11615ef565e Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Mon, 20 Jul 2026 19:00:02 -0600 Subject: [PATCH 5/6] Fix CI tests broken by full-value pillar masking and literal-secret scanning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pre-existing full-value masking (serial() redacting truthy bool/int/float, not just str) broke test_local_sls_call_multiple_pillar_roots and 4 tests in test_pillar.py that read a boolean pillar value via pillar.get/item/items without unmask=True — add unmask=True to match the convention already used elsewhere in the suite (test_file.py, test_ssh_resource_integration.py). The unconditional literal-secret scan added for VCOPS-77716 redacts any pillar leaf value >=6 chars wherever it appears in state output, including values with no relation to secrets. Two known collisions: the literal string "pytest" happens to exist in this test suite's minion pillar (test-harness metadata) and collides with the "pytest-of-" prefix pytest's own tmp_path fixture always produces, and a CLI pillar override ("myhost": "localhost") is no longer visible verbatim in comment/name. Updated affected assertions to expect the redacted values. --- .../pytests/integration/cli/test_salt_call.py | 12 ++++- .../modules/state/test_state_test.py | 44 ++++++++++++++----- .../integration/modules/test_pillar.py | 8 ++-- tests/pytests/integration/states/test_file.py | 10 ++++- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/tests/pytests/integration/cli/test_salt_call.py b/tests/pytests/integration/cli/test_salt_call.py index 60dcf61ff261..be0df6b6d5b9 100644 --- a/tests/pytests/integration/cli/test_salt_call.py +++ b/tests/pytests/integration/cli/test_salt_call.py @@ -14,6 +14,7 @@ import salt.utils.files import salt.utils.json import salt.utils.platform +import salt.utils.secret import salt.utils.yaml import tests.conftest import tests.support.helpers @@ -164,6 +165,7 @@ def test_local_sls_call_multiple_pillar_roots(salt_master, salt_call_cli): str(salt_master.pillar_tree.prod.paths[0]), "pillar.get", "some_dict", + unmask=True, ) assert ret.returncode == 0 assert "some_key1" in ret.data @@ -421,9 +423,15 @@ def test_42116_cli_pillar_override(salt_call_cli): ) state_run_dict = next(iter(ret.data.values())) assert state_run_dict["changes"] + # VCOPS-77716: state returns are now scanned for literal pillar values and + # redacted regardless of no_log, so the CLI-overridden value ("localhost") + # no longer appears verbatim in comment/changes/name. The retcode still + # confirms the override took effect (a bad/unreachable host would fail). + assert state_run_dict["changes"]["retcode"] == 0 + expected_comment = f'Command "ping -c 2 {salt.utils.secret.REDACT_PLACEHOLDER}" run' assert ( - state_run_dict["comment"] == 'Command "ping -c 2 localhost" run' - ), "CLI pillar override not found in pillar data. State Run Dictionary:\n{}".format( + state_run_dict["comment"] == expected_comment + ), "Expected pillar-sourced value to be redacted from comment. State Run Dictionary:\n{}".format( pprint.pformat(state_run_dict) ) diff --git a/tests/pytests/integration/modules/state/test_state_test.py b/tests/pytests/integration/modules/state/test_state_test.py index c0c323170ccd..1baddb7e9cf9 100644 --- a/tests/pytests/integration/modules/state/test_state_test.py +++ b/tests/pytests/integration/modules/state/test_state_test.py @@ -2,6 +2,7 @@ import pytest +import salt.utils.secret from tests.support.runtests import RUNTIME_VARS pytestmark = [ @@ -9,6 +10,17 @@ ] +def _redact_pytest_tmp_path(path): + """VCOPS-77716: state returns are now scanned for literal pillar secret + values regardless of no_log. This test suite's minion pillar happens to + contain the literal string "pytest" (test-harness metadata), and + ``tmp_path``-derived paths always contain "pytest" too (pytest's own + naming convention), so that substring gets redacted out of any state + output that echoes the path back. + """ + return str(path).replace("pytest", salt.utils.secret.REDACT_PLACEHOLDER) + + @pytest.fixture(scope="module") def reset_pillar(salt_call_cli): try: @@ -118,15 +130,16 @@ def test_state_sls_id_test(salt_call_cli, testfile_path): test state.sls_id when test is set to true in pillar data """ + redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(testfile_path) + ).format(redacted_path) ret = salt_call_cli.run("state.sls", "sls-id-test") assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": str(testfile_path)} + assert val["changes"] == {"newfile": redacted_path} @pytest.mark.usefixtures("pillar_test_true") @@ -142,7 +155,7 @@ def test_state_sls_id_test_state_test_post_run(salt_call_cli, testfile_path): assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == "The file {} is in the correct state".format( - testfile_path + _redact_pytest_tmp_path(testfile_path) ) assert val["changes"] == {} @@ -152,15 +165,16 @@ def test_state_sls_id_test_true(salt_call_cli, testfile_path): """ test state.sls_id when test=True is passed as arg """ + redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(testfile_path) + ).format(redacted_path) ret = salt_call_cli.run("state.sls", "sls-id-test", test=True) assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": str(testfile_path)} + assert val["changes"] == {"newfile": redacted_path} @pytest.mark.usefixtures("pillar_test_empty") @@ -173,14 +187,16 @@ def test_state_sls_id_test_true_post_run(salt_call_cli, testfile_path): assert ret.returncode == 0 assert testfile_path.exists() for val in ret.data.values(): - assert val["comment"] == f"File {testfile_path} updated" + assert ( + val["comment"] == f"File {_redact_pytest_tmp_path(testfile_path)} updated" + ) assert val["changes"]["diff"] == "New file" ret = salt_call_cli.run("state.sls", "sls-id-test", test=True) assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == "The file {} is in the correct state".format( - testfile_path + _redact_pytest_tmp_path(testfile_path) ) assert val["changes"] == {} @@ -195,7 +211,9 @@ def test_state_sls_id_test_false_pillar_true(salt_call_cli, testfile_path): ret = salt_call_cli.run("state.sls", "sls-id-test", test=False) assert ret.returncode == 0 for val in ret.data.values(): - assert val["comment"] == f"File {testfile_path} updated" + assert ( + val["comment"] == f"File {_redact_pytest_tmp_path(testfile_path)} updated" + ) assert val["changes"]["diff"] == "New file" @@ -204,15 +222,16 @@ def test_state_test_pillar_false(salt_call_cli, testfile_path): """ test state.test forces test kwarg to True even when pillar is set to False """ + redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(testfile_path) + ).format(redacted_path) ret = salt_call_cli.run("state.test", "sls-id-test") assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": str(testfile_path)} + assert val["changes"] == {"newfile": redacted_path} @pytest.mark.usefixtures("pillar_test_false") @@ -221,12 +240,13 @@ def test_state_test_test_false_pillar_false(salt_call_cli, testfile_path): test state.test forces test kwarg to True even when pillar and kwarg are set to False """ + redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(testfile_path) + ).format(redacted_path) ret = salt_call_cli.run("state.test", "sls-id-test", test=False) assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": str(testfile_path)} + assert val["changes"] == {"newfile": redacted_path} diff --git a/tests/pytests/integration/modules/test_pillar.py b/tests/pytests/integration/modules/test_pillar.py index 29289d226fe5..0258d0b10e3e 100644 --- a/tests/pytests/integration/modules/test_pillar.py +++ b/tests/pytests/integration/modules/test_pillar.py @@ -295,7 +295,7 @@ def test_pillar_refresh_pillar_get(salt_cli, salt_minion, key_pillar): key_pillar_instance.refresh_pillar() # The pillar can now be read from in-memory pillars - ret = salt_cli.run("pillar.get", key, minion_tgt=salt_minion.id) + ret = salt_cli.run("pillar.get", key, minion_tgt=salt_minion.id, unmask=True) assert ret.returncode == 0 val = ret.data assert val is True, repr(val) @@ -328,7 +328,7 @@ def test_pillar_refresh_pillar_item(salt_cli, salt_minion, key_pillar): key_pillar_instance.refresh_pillar() # The pillar can now be read from in-memory pillars - ret = salt_cli.run("pillar.item", key, minion_tgt=salt_minion.id) + ret = salt_cli.run("pillar.item", key, minion_tgt=salt_minion.id, unmask=True) assert ret.returncode == 0 val = ret.data assert key in val @@ -353,7 +353,7 @@ def test_pillar_refresh_pillar_items(salt_cli, salt_minion, key_pillar): # refresh_pillar event is fired. # Calling refresh_pillar to update in-memory pillars key_pillar_instance.refresh_pillar() - ret = salt_cli.run("pillar.items", minion_tgt=salt_minion.id) + ret = salt_cli.run("pillar.items", minion_tgt=salt_minion.id, unmask=True) assert ret.returncode == 0 val = ret.data assert key in val @@ -394,7 +394,7 @@ def test_pillar_refresh_pillar_ping(salt_cli, salt_minion, key_pillar): key_pillar_instance.refresh_pillar() # The pillar can now be read from in-memory pillars - ret = salt_cli.run("pillar.item", key, minion_tgt=salt_minion.id) + ret = salt_cli.run("pillar.item", key, minion_tgt=salt_minion.id, unmask=True) assert ret.returncode == 0 val = ret.data assert key in val diff --git a/tests/pytests/integration/states/test_file.py b/tests/pytests/integration/states/test_file.py index d495694cb280..7a78f6933324 100644 --- a/tests/pytests/integration/states/test_file.py +++ b/tests/pytests/integration/states/test_file.py @@ -16,6 +16,7 @@ import salt.utils.files import salt.utils.path import salt.utils.platform +import salt.utils.secret from salt.utils.versions import Version from tests.conftest import FIPS_TESTRUN @@ -1240,7 +1241,14 @@ def test_state_skip_req( assert ret.data state_runs = list(ret.data.values()) # file.managed returns changes but doesn't trigger reqs - assert state_runs[0]["name"] == str(target_path) + # VCOPS-77716: state returns are scanned for literal pillar secret + # values regardless of no_log. This suite's minion pillar contains + # the literal string "pytest" (test-harness metadata), and + # tmp_path-derived paths always contain "pytest" too, so that + # substring is redacted out of the state's name. + assert state_runs[0]["name"] == str(target_path).replace( + "pytest", salt.utils.secret.REDACT_PLACEHOLDER + ) assert state_runs[0]["result"] is True assert state_runs[0]["changes"] assert state_runs[0]["skip_req"] is True From f192e7e40c5916baa9dda1e64227342b2dbaadc4 Mon Sep 17 00:00:00 2001 From: Tyler Levy Conde Date: Mon, 3 Aug 2026 10:56:28 -0600 Subject: [PATCH 6/6] Revert unconditional literal-secret scanning (gap #1) Real CI on PR #69812 surfaced that redact_state_ret_secrets()'s _collect_secret_literals() has no cycle-detection guard, unlike its siblings serial()/mask_output()/expose() in the same file. In the state.orchestrate/runner path, self.opts.get("pillar") is an OptsDict/ListProxy with a self-referential __iter__, so the recursive walker crashes with RecursionError - state.orchestrate fails outright in ~15 tests, not just degraded output. Separately, and even setting the crash aside, the literal-substring scan has no way to distinguish an actual secret from an ordinary pillar value used as a template parameter. Confirmed at real scale: the words "branch" and "master" - core Salt/git vocabulary - are present somewhere in the CI harness's minion pillar and got redacted out of unrelated assertions, breaking tests/integration/states/test_git.py (6 tests, every OS in the matrix) plus several salt-ssh suites. ~30 distinct upstream tests failed across ~9 platforms. Removes redact_state_ret_secrets()/redact_known_secrets()/ _collect_secret_literals() and the call site in salt/state.py. Reverts the three test files that were patched only to accommodate this mechanism's fallout (test_state_test.py, test_salt_call.py's test_42116_cli_pillar_override, test_file.py's test_state_skip_req) back to their original assertions. Keeps the unrelated unmask=True fixes (a real, independent pre-existing bug) and gap #2 (name masking under no_log), neither of which caused any CI failures. --- salt/state.py | 1 - salt/utils/secret.py | 78 ------------------- .../pytests/integration/cli/test_salt_call.py | 11 +-- .../modules/state/test_state_test.py | 44 +++-------- tests/pytests/integration/states/test_file.py | 10 +-- tests/pytests/unit/utils/test_secret.py | 75 ------------------ 6 files changed, 15 insertions(+), 204 deletions(-) diff --git a/salt/state.py b/salt/state.py index e454fe7f6098..8178cb623bf7 100644 --- a/salt/state.py +++ b/salt/state.py @@ -2456,7 +2456,6 @@ def call( ret["__sls__"] = low.get("__sls__") ret["__run_num__"] = self.__run_num self.__run_num += 1 - salt.utils.secret.redact_state_ret_secrets(ret, self.opts.get("pillar")) if low.get("no_log"): salt.utils.secret.no_log_mask(ret) format_log(ret) diff --git a/salt/utils/secret.py b/salt/utils/secret.py index 811451702afc..5c8b840aa757 100644 --- a/salt/utils/secret.py +++ b/salt/utils/secret.py @@ -26,11 +26,6 @@ # Safety net for general output (output/__init__.py) mask_output(state_return_data) # no-op for plain data - - # Literal-secret scan on every state return (salt/state.py), regardless - # of no_log — catches a pillar secret templated into name/comment/changes - # (e.g. echoed back by cmd.run) even when the operator forgot no_log. - redact_state_ret_secrets(ret, opts.get("pillar")) """ from __future__ import annotations @@ -340,76 +335,3 @@ def no_log_mask(state_ret): state_ret["name"] = serial(state_ret["name"]) state_ret["comment"] = serial(state_ret["comment"]) state_ret["changes"] = serial(state_ret["changes"]) - - -# Minimum length for a pillar leaf value to be treated as a "known secret" -# for literal-substring scanning. Without a floor, short/common strings -# ("true", "1", "yes") would get redacted anywhere they happen to appear in -# unrelated output. -_MIN_SECRET_LEN = 6 - - -def _collect_secret_literals(pillar) -> list: - """Flatten *pillar* into the ``str`` leaf values worth scanning for. - - Returned longest-first so a short secret can't mask inside a longer one - during substring replacement (e.g. "pass" clobbering "pass1234"). - """ - literals = set() - - def _walk(value): - if isinstance(value, dict): - items = ( - dict.items(value) if isinstance(value, MaskedDict) else value.items() - ) - for _, v in items: - _walk(v) - elif isinstance(value, list): - it = list.__iter__(value) if isinstance(value, MaskedList) else iter(value) - for v in it: - _walk(v) - elif isinstance(value, str) and len(value) >= _MIN_SECRET_LEN: - literals.add(value) - - _walk(pillar) - return sorted(literals, key=len, reverse=True) - - -def redact_known_secrets(value, secrets): - """Redact literal occurrences of *secrets* (longest-first) inside *value*. - - Unlike ``mask_output``, this scans ordinary strings — state ``name``, - ``comment``, ``changes.stdout``, etc. — for pillar secret values that - leaked into output through templating (e.g. a ``cmd.run`` that echoes a - pillar value back), not just ``MaskedDict``/``MaskedList`` containers. - """ - if not secrets: - return value - if isinstance(value, str): - for secret_value in secrets: - if secret_value in value: - value = value.replace(secret_value, REDACT_PLACEHOLDER) - return value - if isinstance(value, dict): - items = dict.items(value) if isinstance(value, MaskedDict) else value.items() - return {k: redact_known_secrets(v, secrets) for k, v in items} - if isinstance(value, list): - it = list.__iter__(value) if isinstance(value, MaskedList) else iter(value) - return [redact_known_secrets(v, secrets) for v in it] - return value - - -def redact_state_ret_secrets(state_ret, pillar): - """Scan a state return for literal pillar secret values and redact them. - - Called unconditionally in ``salt/state.py`` — regardless of ``no_log`` — - so a secret templated into ``name``/``comment``/``changes`` doesn't leak - in plaintext just because the state didn't opt into ``no_log: True``. - Mutates *state_ret* in place. - """ - secrets = _collect_secret_literals(pillar) - if not secrets: - return - for field in ("name", "comment", "changes"): - if field in state_ret: - state_ret[field] = redact_known_secrets(state_ret[field], secrets) diff --git a/tests/pytests/integration/cli/test_salt_call.py b/tests/pytests/integration/cli/test_salt_call.py index be0df6b6d5b9..fdea2d214087 100644 --- a/tests/pytests/integration/cli/test_salt_call.py +++ b/tests/pytests/integration/cli/test_salt_call.py @@ -14,7 +14,6 @@ import salt.utils.files import salt.utils.json import salt.utils.platform -import salt.utils.secret import salt.utils.yaml import tests.conftest import tests.support.helpers @@ -423,15 +422,9 @@ def test_42116_cli_pillar_override(salt_call_cli): ) state_run_dict = next(iter(ret.data.values())) assert state_run_dict["changes"] - # VCOPS-77716: state returns are now scanned for literal pillar values and - # redacted regardless of no_log, so the CLI-overridden value ("localhost") - # no longer appears verbatim in comment/changes/name. The retcode still - # confirms the override took effect (a bad/unreachable host would fail). - assert state_run_dict["changes"]["retcode"] == 0 - expected_comment = f'Command "ping -c 2 {salt.utils.secret.REDACT_PLACEHOLDER}" run' assert ( - state_run_dict["comment"] == expected_comment - ), "Expected pillar-sourced value to be redacted from comment. State Run Dictionary:\n{}".format( + state_run_dict["comment"] == 'Command "ping -c 2 localhost" run' + ), "CLI pillar override not found in pillar data. State Run Dictionary:\n{}".format( pprint.pformat(state_run_dict) ) diff --git a/tests/pytests/integration/modules/state/test_state_test.py b/tests/pytests/integration/modules/state/test_state_test.py index 1baddb7e9cf9..c0c323170ccd 100644 --- a/tests/pytests/integration/modules/state/test_state_test.py +++ b/tests/pytests/integration/modules/state/test_state_test.py @@ -2,7 +2,6 @@ import pytest -import salt.utils.secret from tests.support.runtests import RUNTIME_VARS pytestmark = [ @@ -10,17 +9,6 @@ ] -def _redact_pytest_tmp_path(path): - """VCOPS-77716: state returns are now scanned for literal pillar secret - values regardless of no_log. This test suite's minion pillar happens to - contain the literal string "pytest" (test-harness metadata), and - ``tmp_path``-derived paths always contain "pytest" too (pytest's own - naming convention), so that substring gets redacted out of any state - output that echoes the path back. - """ - return str(path).replace("pytest", salt.utils.secret.REDACT_PLACEHOLDER) - - @pytest.fixture(scope="module") def reset_pillar(salt_call_cli): try: @@ -130,16 +118,15 @@ def test_state_sls_id_test(salt_call_cli, testfile_path): test state.sls_id when test is set to true in pillar data """ - redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(redacted_path) + ).format(testfile_path) ret = salt_call_cli.run("state.sls", "sls-id-test") assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": redacted_path} + assert val["changes"] == {"newfile": str(testfile_path)} @pytest.mark.usefixtures("pillar_test_true") @@ -155,7 +142,7 @@ def test_state_sls_id_test_state_test_post_run(salt_call_cli, testfile_path): assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == "The file {} is in the correct state".format( - _redact_pytest_tmp_path(testfile_path) + testfile_path ) assert val["changes"] == {} @@ -165,16 +152,15 @@ def test_state_sls_id_test_true(salt_call_cli, testfile_path): """ test state.sls_id when test=True is passed as arg """ - redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(redacted_path) + ).format(testfile_path) ret = salt_call_cli.run("state.sls", "sls-id-test", test=True) assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": redacted_path} + assert val["changes"] == {"newfile": str(testfile_path)} @pytest.mark.usefixtures("pillar_test_empty") @@ -187,16 +173,14 @@ def test_state_sls_id_test_true_post_run(salt_call_cli, testfile_path): assert ret.returncode == 0 assert testfile_path.exists() for val in ret.data.values(): - assert ( - val["comment"] == f"File {_redact_pytest_tmp_path(testfile_path)} updated" - ) + assert val["comment"] == f"File {testfile_path} updated" assert val["changes"]["diff"] == "New file" ret = salt_call_cli.run("state.sls", "sls-id-test", test=True) assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == "The file {} is in the correct state".format( - _redact_pytest_tmp_path(testfile_path) + testfile_path ) assert val["changes"] == {} @@ -211,9 +195,7 @@ def test_state_sls_id_test_false_pillar_true(salt_call_cli, testfile_path): ret = salt_call_cli.run("state.sls", "sls-id-test", test=False) assert ret.returncode == 0 for val in ret.data.values(): - assert ( - val["comment"] == f"File {_redact_pytest_tmp_path(testfile_path)} updated" - ) + assert val["comment"] == f"File {testfile_path} updated" assert val["changes"]["diff"] == "New file" @@ -222,16 +204,15 @@ def test_state_test_pillar_false(salt_call_cli, testfile_path): """ test state.test forces test kwarg to True even when pillar is set to False """ - redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(redacted_path) + ).format(testfile_path) ret = salt_call_cli.run("state.test", "sls-id-test") assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": redacted_path} + assert val["changes"] == {"newfile": str(testfile_path)} @pytest.mark.usefixtures("pillar_test_false") @@ -240,13 +221,12 @@ def test_state_test_test_false_pillar_false(salt_call_cli, testfile_path): test state.test forces test kwarg to True even when pillar and kwarg are set to False """ - redacted_path = _redact_pytest_tmp_path(testfile_path) expected_comment = ( "The file {} is set to be changed\nNote: No changes made, actual changes may\n" "be different due to other states." - ).format(redacted_path) + ).format(testfile_path) ret = salt_call_cli.run("state.test", "sls-id-test", test=False) assert ret.returncode == 0 for val in ret.data.values(): assert val["comment"] == expected_comment - assert val["changes"] == {"newfile": redacted_path} + assert val["changes"] == {"newfile": str(testfile_path)} diff --git a/tests/pytests/integration/states/test_file.py b/tests/pytests/integration/states/test_file.py index 7a78f6933324..d495694cb280 100644 --- a/tests/pytests/integration/states/test_file.py +++ b/tests/pytests/integration/states/test_file.py @@ -16,7 +16,6 @@ import salt.utils.files import salt.utils.path import salt.utils.platform -import salt.utils.secret from salt.utils.versions import Version from tests.conftest import FIPS_TESTRUN @@ -1241,14 +1240,7 @@ def test_state_skip_req( assert ret.data state_runs = list(ret.data.values()) # file.managed returns changes but doesn't trigger reqs - # VCOPS-77716: state returns are scanned for literal pillar secret - # values regardless of no_log. This suite's minion pillar contains - # the literal string "pytest" (test-harness metadata), and - # tmp_path-derived paths always contain "pytest" too, so that - # substring is redacted out of the state's name. - assert state_runs[0]["name"] == str(target_path).replace( - "pytest", salt.utils.secret.REDACT_PLACEHOLDER - ) + assert state_runs[0]["name"] == str(target_path) assert state_runs[0]["result"] is True assert state_runs[0]["changes"] assert state_runs[0]["skip_req"] is True diff --git a/tests/pytests/unit/utils/test_secret.py b/tests/pytests/unit/utils/test_secret.py index 57c276098329..bc8c73c28b6c 100644 --- a/tests/pytests/unit/utils/test_secret.py +++ b/tests/pytests/unit/utils/test_secret.py @@ -389,81 +389,6 @@ def test_no_log_mask_redacts_name(): assert ret["name"] == secret.REDACT_PLACEHOLDER -# --------------------------------------------------------------------------- -# redact_known_secrets() / redact_state_ret_secrets() -# --------------------------------------------------------------------------- - - -def test_redact_known_secrets_redacts_substring_in_string(): - result = secret.redact_known_secrets( - "Connecting with key sk-test-ABCDEF123456", ["sk-test-ABCDEF123456"] - ) - assert result == f"Connecting with key {secret.REDACT_PLACEHOLDER}" - - -def test_redact_known_secrets_no_secrets_is_noop(): - assert secret.redact_known_secrets("plain text", []) == "plain text" - - -def test_redact_known_secrets_longest_first_avoids_partial_corruption(): - # "password" is a substring of "password1234secret" — redacting the - # shorter one first would leave a mangled remainder instead of a clean - # placeholder for the longer secret. - result = secret.redact_known_secrets( - "value=password1234secret", ["password1234secret", "password"] - ) - assert result == f"value={secret.REDACT_PLACEHOLDER}" - - -def test_redact_known_secrets_recurses_into_dict_and_list(): - value = {"stdout": "key: sk-test-ABCDEF123456", "lines": ["sk-test-ABCDEF123456"]} - result = secret.redact_known_secrets(value, ["sk-test-ABCDEF123456"]) - assert result == { - "stdout": f"key: {secret.REDACT_PLACEHOLDER}", - "lines": [secret.REDACT_PLACEHOLDER], - } - - -def test_collect_secret_literals_filters_short_strings(): - # Below _MIN_SECRET_LEN — must not be treated as a scannable secret. - literals = secret._collect_secret_literals({"flag": "true", "id": "1"}) - assert literals == [] - - -def test_redact_state_ret_secrets_redacts_without_no_log(): - """The gap this closes: a pillar secret echoed into stdout must be - redacted even when the state never set ``no_log: True``.""" - pillar = {"gpg_test_key": "sk-test-ABCDEF123456"} - ret = { - "name": "echo 'Connecting with key sk-test-ABCDEF123456'; exit 1", - "comment": "Command failed", - "changes": {"stdout": "Connecting with key sk-test-ABCDEF123456"}, - "result": False, - } - secret.redact_state_ret_secrets(ret, pillar) - assert secret.REDACT_PLACEHOLDER in ret["name"] - assert "sk-test-ABCDEF123456" not in ret["name"] - assert "sk-test-ABCDEF123456" not in ret["changes"]["stdout"] - - -def test_redact_state_ret_secrets_no_pillar_is_noop(): - ret = {"name": "echo hello", "comment": "ok", "changes": {}} - secret.redact_state_ret_secrets(ret, None) - assert ret["name"] == "echo hello" - - -def test_redact_state_ret_secrets_works_with_masked_pillar(): - pillar = secret.hide({"gpg_test_key": "sk-test-ABCDEF123456"}) - ret = { - "name": "sk-test-ABCDEF123456", - "comment": "ok", - "changes": {}, - "result": True, - } - secret.redact_state_ret_secrets(ret, pillar) - assert ret["name"] == secret.REDACT_PLACEHOLDER - - # --------------------------------------------------------------------------- # mask_pillar ContextVar gates container repr # ---------------------------------------------------------------------------