Add pillar_mask_output config option and fix full-value pillar masking

changelog/98852.added.md

@@ -0,0 +1 @@
+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.

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.

doc/ref/configuration/master.rst

@@ -5662,6 +5662,33 @@ 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``
+
+Changes the *default* behavior of :py:func:`pillar.items
+<salt.modules.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
+<salt.modules.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
+
+    pillar_mask_output: True
+
 .. conf_master:: pillar_includes_override_sls
 
 ``pillar_includes_override_sls``

salt/config/__init__.py

@@ -698,6 +698,10 @@ def _gather_buffer_space():
         "pillar_source_merging_strategy": str,
         # Recursively merge lists by aggregating them instead of replacing them.
         "pillar_merge_lists": bool,
+        # 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,
         # How to merge multiple top files from multiple salt environments
@@ -1172,6 +1176,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 +1651,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,

salt/modules/pillar.py

@@ -254,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
 
@@ -297,7 +300,14 @@ 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:

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)

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
@@ -62,18 +67,29 @@ 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):
         pairs = ", ".join(f"{k!r}: {_masked_repr(v)}" for k, v in value.items())
         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)
 
@@ -244,7 +260,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``.
@@ -255,10 +272,12 @@ def serial(value, _seen=None):
     """
     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:
@@ -312,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)

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)
     )