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 3cb473072f0c..0d7b382e8869 100644 --- a/doc/ref/configuration/master.rst +++ b/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 +` 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 6e32516235a3..a9b1c60a5bef 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 @@ -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, 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/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 1ed2e588385b..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 @@ -62,6 +67,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 +87,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 +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) 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 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..57c276098329 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,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 # ---------------------------------------------------------------------------