diff --git a/changelog/98852.added.md b/changelog/98852.added.md new file mode 100644 index 000000000000..4381bd453c03 --- /dev/null +++ b/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. 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 c62d6ce308b6..5b63d0dd805e 100644 --- a/doc/ref/configuration/master.rst +++ b/doc/ref/configuration/master.rst @@ -5670,6 +5670,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 +` 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 + + 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 42253f176a85..f36bc7bf43db 100644 --- a/salt/config/__init__.py +++ b/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 @@ -1173,6 +1177,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`` @@ -1648,6 +1653,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/modules/pillar.py b/salt/modules/pillar.py index 72a1edb6b6e8..4b518fba8ab0 100644 --- a/salt/modules/pillar.py +++ b/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: diff --git a/salt/utils/secret.py b/salt/utils/secret.py index 1ed2e588385b..5c8b840aa757 100644 --- a/salt/utils/secret.py +++ b/salt/utils/secret.py @@ -62,6 +62,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 +82,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) @@ -244,7 +255,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 +267,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 +326,12 @@ 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"]) diff --git a/tests/pytests/integration/cli/test_salt_call.py b/tests/pytests/integration/cli/test_salt_call.py index 60dcf61ff261..fdea2d214087 100644 --- a/tests/pytests/integration/cli/test_salt_call.py +++ b/tests/pytests/integration/cli/test_salt_call.py @@ -164,6 +164,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 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/unit/modules/test_pillar.py b/tests/pytests/unit/modules/test_pillar.py index 820a2a8d10e7..095e2bff0093 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,23 +136,88 @@ 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 +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") == 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 df29e9079e56..bc8c73c28b6c 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(): @@ -319,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 @@ -327,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, @@ -337,11 +373,22 @@ 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 + + # --------------------------------------------------------------------------- # mask_pillar ContextVar gates container repr # ---------------------------------------------------------------------------