Skip to content

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

Open
ggiesen wants to merge 1 commit into
saltstack:3006.xfrom
ggiesen:fix-63477-key-accept
Open

Fail wheel key.accept/reject/delete when nothing is acted on (#63477)#69780
ggiesen wants to merge 1 commit into
saltstack:3006.xfrom
ggiesen:fix-63477-key-accept

Conversation

@ggiesen

@ggiesen ggiesen commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Makes the wheel key.accept/reject/delete functions (and their _dict variants) report success=False when the match names no key the operation can act on, instead of a silent success. Also resets the runner/wheel retcode per call so a failure cannot leak across calls on a reused client.

What issues does this PR fix or reference?

Fixes #63477

Previous Behavior

wheel key.accept <match> returned success=True with an empty result even when <match> named no key that could be accepted -- most obviously a non-existent minion (the reported case), but also a key that exists only in a state the command does not touch. Since callers, and saltmod.wheel orchestration, treat success as "the operation did what I asked", this is a misleading and security-relevant false positive. The sharpest case is key.reject <accepted-minion> with default flags: reject only moves pending keys unless include_accepted is set, so it does nothing, yet reports success -- an operator believes a minion is locked out when it is still fully authorized.

New Behavior

accept/reject/delete (and accept_dict/reject_dict/delete_dict) set a non-zero retcode -- so success=False -- unless the match names a key in a state the command acts on, or already in its target state. Re-asserting a key already in its target state (for example accepting an already-accepted minion) stays an idempotent success, which matters for saltmod.wheel in orchestration.

SyncClientMixin.low() never reset the retcode, and two paths reuse one long-lived client -- the master's per-MWorker ClearFuncs.wheel_ and the reactor's cached Wheel -- so a failure retcode set by one call would otherwise persist and make later unrelated calls report success=False. The retcode is now reset at the start of each low() call, matching what the minion already does per job.

Verified with a real WheelClient against a temp PKI:

  • accept a non-existent minion -> success=False; a pending or already-accepted minion -> success=True; a rejected-only key without include_rejected -> success=False.
  • reject an accepted minion without include_accepted -> success=False.
  • reused client: a no-match accept followed by a valid accept -> the valid accept still reports success (no leak).

Merge requirements satisfied?

  • Docs - n/a
  • Changelog - changelog/63477.fixed.md
  • Tests written/updated - tests/pytests/unit/wheel/test_key.py (per-command condition, the _dict variants, and the reused-client leak regression; all pin-proofed against baseline)

Commits signed with GPG?

No

wheel key.accept (and reject/delete, plus their _dict variants) returned
success=True even when the match named no key the command could act on: a
non-existent minion, or a key present only in a state the command does not
touch (e.g. rejecting an already-accepted minion without include_accepted).
That is a misleading, security-relevant false positive.

Set a non-zero retcode (success=False) unless the match names a key in a
state the command acts on, or already in its target state (so re-asserting a
key stays idempotent for saltmod.wheel orchestration). Also reset the retcode
at the start of each runner/wheel low() call so a failure set on a reused
client -- the master's per-MWorker ClearFuncs.wheel_ or a cached reactor
client -- does not leak into later calls.

Fixes saltstack#63477
@ggiesen ggiesen requested a review from a team as a code owner July 11, 2026 17:44
@dwoz dwoz added the test:full Run the full test suite label Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants