Fail wheel key.accept/reject/delete when nothing is acted on (#63477)

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.

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:

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,

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"]}