diff --git a/changelog/63477.fixed.md b/changelog/63477.fixed.md new file mode 100644 index 000000000000..d49f0e9794f9 --- /dev/null +++ b/changelog/63477.fixed.md @@ -0,0 +1 @@ +Fixed the wheel ``key.accept``, ``key.reject`` and ``key.delete`` functions (and their ``_dict`` variants) reporting ``success`` when the match named no key the operation could act on -- a non-existent minion, or a key present only in a state the command does not touch (for example rejecting an accepted key without ``include_accepted``). They now set a non-zero retcode (``success=False``); a key already in its target state remains an idempotent success. The runner/wheel retcode is also reset at the start of each call so a failure set on a reused client (the master's per-worker wheel client or a cached reactor client) no longer leaks into later calls. diff --git a/salt/client/mixins.py b/salt/client/mixins.py index 1e40e85c712e..0c62c5ec0dc5 100644 --- a/salt/client/mixins.py +++ b/salt/client/mixins.py @@ -384,6 +384,12 @@ def low(self, fun, low, print_event=True, full_return=False): self.functions.context_dict.clone ): func = self.functions[fun] + # Reset retcode so a value set by a previous call on a + # reused client (e.g. the master's ClearFuncs.wheel_, one + # per MWorker, or a cached reactor client) cannot leak into + # this call -- low() otherwise never clears it. + if isinstance(self.context, dict): + self.context["retcode"] = 0 try: data["return"] = func(*args, **kwargs) except TypeError as exc: diff --git a/salt/wheel/key.py b/salt/wheel/key.py index 29c9cca54a79..2cfb9ccc0703 100644 --- a/salt/wheel/key.py +++ b/salt/wheel/key.py @@ -97,6 +97,28 @@ def name_match(match): return skey.name_match(match) +def _fail_on_no_match(matched, keydirs): + """ + Give the calling wheel function a non-zero retcode -- and therefore + ``success=False`` instead of a silent success -- when ``matched`` names no + key the command can act on or that is already in its target state (#63477). + + ``matched`` is a ``status -> key-names`` mapping (``name_match``'s result + for a glob, or the caller-supplied ``match_dict``). ``keydirs`` is the set + of key states that count as success for the command: the states it moves + keys out of, plus the target state, so that re-asserting a key already in + its final state stays an idempotent success (important for ``saltmod.wheel`` + orchestration). A non-existent minion, or a key that only exists in a state + the command will not touch (e.g. rejecting an accepted key without + ``include_accepted``), is reported as a failure rather than a silent + success. + """ + if not isinstance(matched, dict) or not any( + matched.get(keydir) for keydir in keydirs + ): + __context__["retcode"] = 1 + + def accept(match, include_rejected=False, include_denied=False): """ Accept keys based on a glob match. Returns a dictionary. @@ -118,6 +140,12 @@ def accept(match, include_rejected=False, include_denied=False): {'minions': ['minion1']} """ with salt.key.get_key(__opts__) as skey: + keydirs = [skey.PEND, skey.ACC] + if include_rejected: + keydirs.append(skey.REJ) + if include_denied: + keydirs.append(skey.DEN) + _fail_on_no_match(skey.name_match(match), keydirs) return skey.accept( match, include_rejected=include_rejected, include_denied=include_denied ) @@ -158,6 +186,12 @@ def accept_dict(match, include_rejected=False, include_denied=False): {'minions': ['jerry', 'stuart', 'bob']} """ with salt.key.get_key(__opts__) as skey: + keydirs = [skey.PEND, skey.ACC] + if include_rejected: + keydirs.append(skey.REJ) + if include_denied: + keydirs.append(skey.DEN) + _fail_on_no_match(match, keydirs) return skey.accept( match_dict=match, include_rejected=include_rejected, @@ -178,6 +212,9 @@ def delete(match): {'jid': '20160826201244808521', 'tag': 'salt/wheel/20160826201244808521'} """ with salt.key.get_key(__opts__) as skey: + # delete acts on a key in any state, so any matching key is actionable + keydirs = [skey.ACC, skey.PEND, skey.REJ, skey.DEN] + _fail_on_no_match(skey.name_match(match), keydirs) return skey.delete_key(match) @@ -232,6 +269,8 @@ def delete_dict(match): ... ]) """ with salt.key.get_key(__opts__) as skey: + keydirs = [skey.ACC, skey.PEND, skey.REJ, skey.DEN] + _fail_on_no_match(match, keydirs) return skey.delete_key(match_dict=match) @@ -256,6 +295,12 @@ def reject(match, include_accepted=False, include_denied=False): {'jid': '20160826201244808521', 'tag': 'salt/wheel/20160826201244808521'} """ with salt.key.get_key(__opts__) as skey: + keydirs = [skey.PEND, skey.REJ] + if include_accepted: + keydirs.append(skey.ACC) + if include_denied: + keydirs.append(skey.DEN) + _fail_on_no_match(skey.name_match(match), keydirs) return skey.reject( match, include_accepted=include_accepted, include_denied=include_denied ) @@ -293,6 +338,12 @@ def reject_dict(match, include_accepted=False, include_denied=False): {'jid': '20160826201244808521', 'tag': 'salt/wheel/20160826201244808521'} """ with salt.key.get_key(__opts__) as skey: + keydirs = [skey.PEND, skey.REJ] + if include_accepted: + keydirs.append(skey.ACC) + if include_denied: + keydirs.append(skey.DEN) + _fail_on_no_match(match, keydirs) return skey.reject( match_dict=match, include_accepted=include_accepted, diff --git a/tests/pytests/unit/wheel/test_key.py b/tests/pytests/unit/wheel/test_key.py new file mode 100644 index 000000000000..cd154e92fcbe --- /dev/null +++ b/tests/pytests/unit/wheel/test_key.py @@ -0,0 +1,191 @@ +""" +Unit tests for the ``key`` wheel module (#63477): accept/reject/delete and +their ``_dict`` variants must report ``success=False`` when the match names no +key the command can act on or leave in its target state, while re-asserting a +key already in its target state stays an idempotent success. +""" + +import os + +import pytest + +import salt.config +import salt.crypt +import salt.wheel +import salt.wheel.key +from tests.support.mock import MagicMock, patch + + +@pytest.fixture +def context(): + return {} + + +def _run(func, context, name_match_result, *args, **kwargs): + """ + Call a wheel key function with ``salt.key.get_key`` mocked so ``name_match`` + returns ``name_match_result`` and ``__context__`` bound to ``context``. For + the ``_dict`` variants pass the match dict as the positional arg (they use + it directly and never call ``name_match``). + """ + skey = MagicMock() + skey.ACC, skey.PEND, skey.REJ, skey.DEN = ( + "minions", + "minions_pre", + "minions_rejected", + "minions_denied", + ) + skey.name_match.return_value = name_match_result + cm = MagicMock() + cm.__enter__.return_value = skey + cm.__exit__.return_value = False + with patch("salt.key.get_key", return_value=cm), patch.object( + salt.wheel.key, "__context__", context, create=True + ), patch.object(salt.wheel.key, "__opts__", {}, create=True): + func(*args, **kwargs) + return skey + + +def _rc(context): + return context.get("retcode", 0) + + +# --- a non-existent key (no match anywhere) always fails --------------------- + + +@pytest.mark.parametrize("func", ["accept", "reject", "delete"]) +def test_glob_no_match_sets_retcode(context, func): + _run(getattr(salt.wheel.key, func), context, {}, "invalid-minion") + assert _rc(context) == 1 + + +# --- a key already in the target state stays an idempotent success ----------- + + +def test_accept_pending_no_retcode(context): + _run(salt.wheel.key.accept, context, {"minions_pre": ["web1"]}, "web1") + assert _rc(context) == 0 + + +def test_accept_already_accepted_is_idempotent(context): + _run(salt.wheel.key.accept, context, {"minions": ["web1"]}, "web1") + assert _rc(context) == 0 + + +def test_reject_already_rejected_is_idempotent(context): + _run(salt.wheel.key.reject, context, {"minions_rejected": ["web1"]}, "web1") + assert _rc(context) == 0 + + +def test_delete_match_any_state_no_retcode(context): + _run(salt.wheel.key.delete, context, {"minions": ["web1"]}, "web1") + assert _rc(context) == 0 + + +# --- a key only in a state the command will not touch is a failure ----------- + + +def test_accept_rejected_key_without_include_sets_retcode(context): + # rejected key + default include_rejected=False -> nothing accepted + _run(salt.wheel.key.accept, context, {"minions_rejected": ["web1"]}, "web1") + assert _rc(context) == 1 + + +def test_accept_rejected_key_with_include_no_retcode(context): + _run( + salt.wheel.key.accept, + context, + {"minions_rejected": ["web1"]}, + "web1", + include_rejected=True, + ) + assert _rc(context) == 0 + + +def test_reject_accepted_key_without_include_sets_retcode(context): + # security-sharp: rejecting an accepted minion with default flags is a + # no-op; it must not report success (the admin is not actually locking it + # out). + _run(salt.wheel.key.reject, context, {"minions": ["web1"]}, "web1") + assert _rc(context) == 1 + + +def test_reject_accepted_key_with_include_no_retcode(context): + _run( + salt.wheel.key.reject, + context, + {"minions": ["web1"]}, + "web1", + include_accepted=True, + ) + assert _rc(context) == 0 + + +# --- the _dict variants get the same guard ---------------------------------- + + +def test_accept_dict_empty_sets_retcode(context): + _run(salt.wheel.key.accept_dict, context, {}, {}) + assert _rc(context) == 1 + + +def test_accept_dict_pending_no_retcode(context): + _run(salt.wheel.key.accept_dict, context, {}, {"minions_pre": ["web1"]}) + assert _rc(context) == 0 + + +def test_accept_dict_rejected_without_include_sets_retcode(context): + _run(salt.wheel.key.accept_dict, context, {}, {"minions_rejected": ["web1"]}) + assert _rc(context) == 1 + + +def test_reject_dict_accepted_without_include_sets_retcode(context): + _run(salt.wheel.key.reject_dict, context, {}, {"minions": ["web1"]}) + assert _rc(context) == 1 + + +def test_delete_dict_empty_sets_retcode(context): + _run(salt.wheel.key.delete_dict, context, {}, {}) + assert _rc(context) == 1 + + +def test_delete_dict_any_state_no_retcode(context): + _run(salt.wheel.key.delete_dict, context, {}, {"minions": ["web1"]}) + assert _rc(context) == 0 + + +# --- the failure retcode must not leak across calls on a reused client ------- + + +def test_retcode_does_not_leak_on_reused_client(tmp_path): + """ + ``low()`` resets retcode per call, so a failure retcode set by a no-match + ``key.accept`` on a reused ``WheelClient`` -- as used by the master's + ``ClearFuncs.wheel_`` and the reactor's cached client -- does not poison a + later successful call. Regression test for #63477. + """ + pki = tmp_path / "pki" + for sub in ("minions", "minions_pre", "minions_rejected", "minions_denied"): + (pki / sub).mkdir(parents=True) + salt.crypt.gen_keys(str(pki / "minions_pre"), "pend1", 2048) + os.replace( + str(pki / "minions_pre" / "pend1.pub"), str(pki / "minions_pre" / "pend1") + ) + os.remove(str(pki / "minions_pre" / "pend1.pem")) + opts = salt.config.master_config(None) + opts.update( + pki_dir=str(pki), + cachedir=str(tmp_path / "cache"), + sock_dir=str(tmp_path / "sock"), + key_logfile=str(tmp_path / "key.log"), + ) + + client = salt.wheel.WheelClient(opts) + no_match = client.cmd( + "key.accept", ["does-not-exist"], print_event=False, full_return=True + ) + assert no_match["success"] is False + # Same client: a genuinely successful accept must still report success. + accepted = client.cmd("key.accept", ["pend1"], print_event=False, full_return=True) + assert accepted["success"] is True + assert accepted["return"] == {"minions": ["pend1"]}