Add pillar_mask_output config option and fix full-value pillar masking#69812
Open
Akm0d wants to merge 5 commits into
Open
Add pillar_mask_output config option and fix full-value pillar masking#69812Akm0d wants to merge 5 commits into
Akm0d wants to merge 5 commits into
Conversation
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 <noreply@anthropic.com>
Per review feedback on PR saltstack#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 <noreply@anthropic.com>
Per maintainer feedback on PR saltstack#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 <noreply@anthropic.com>
twangboy
approved these changes
Jul 16, 2026
Contributor
|
You got some failing tests |
Contributor
|
I submitted duplicate PR for same pillar mask global override, closed it now |
twangboy
previously approved these changes
Jul 20, 2026
…_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.
…canning
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-<user>" 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
VCOPS-98852: harden pillar output masking (VCOPS-77716, VCOPS-84671).
What does this PR do?
What issues does this PR fix or reference?
Fixes
Previous Behavior
Remove this section if not relevant
New Behavior
Remove this section if not relevant
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes/No