Run saltutil.runner/saltutil.wheel as the master's configured user#69240
Run saltutil.runner/saltutil.wheel as the master's configured user#69240ggiesen wants to merge 1 commit into
Conversation
saltutil.runner and saltutil.wheel execute master-side functions inside the minion's process, which usually runs as root. Since the 3006 packages the master runs as the "salt" user by default, so these functions touched master-owned resources (the git_pillar/gitfs cache, the pki tree) as root -- leaving root-owned files behind and tripping git's safe.directory check. git_pillar.update invoked via saltutil.runner then failed with FileserverConfigError (saltstack#67716); the same class of failure affects other runner/wheel functions. When invoked from a more-privileged process these functions now drop to the master's configured user (master_opts["user"]) in a short-lived fork child before executing, returning the result to the parent. The user is changed only when one is configured, differs from the current user, and the process is root; otherwise behavior is unchanged. Note: the fork child uses multiprocessing.get_context("fork") directly. While importing multiprocessing was historically discouraged in Salt, it is now used widely in-tree -- salt/scripts.py sets the fork start method, salt/state.py uses get_context("fork"), and minion/master/gitfs import it. Adds unit tests for the user-selection logic, the privilege-dropping dispatch, and an end-to-end test that drops to an unprivileged user. Fixes saltstack#67716
a0834bf to
b17e307
Compare
|
Re-targeted to 3006.x (was master): #67716 is a bug fix, and 3006.x is the oldest supported branch with the affected Explicit behaviour-change note (flagging since this targets a maintenance branch): previously |
|
To be explicit given this is a behaviour change on a maintenance branch: if you'd rather it land on master (the next feature release) instead of 3006.x, I'm happy to rebase it there - just let me know. |
What does this PR do?
saltutil.runnerandsaltutil.wheelexecute master-side functions inside the minion's process, which usually runs asroot. Since 3006 the packaged Salt master runs as thesaltuser by default, so these functions touched master-owned resources (the git_pillar/gitfs cache, the pki tree) asroot-- leaving root-owned files behind in, and tripping git'ssafe.directorycheck on, the salt-owned master cache.This makes both functions drop to the master's configured user (
master_opts["user"]) before executing when invoked from a more-privileged process.What issues does this PR fix or reference?
Fixes #67716
Previous Behavior
salt <master-minion> saltutil.runner git_pillar.updatefailed withFileserverConfigError: Failed to load git_pillar/detected dubious ownership in repository at .../git_pillar/<name>/_, because the runner ran asrootagainst the salt-owned git_pillar cache.New Behavior
The runner/wheel function runs as the master's configured user.
Scope and risk
Changing the user that
saltutil.runner/saltutil.wheelexecute under is a deliberate behavior change in a core path, so it is intentionally contained: the user is changed only when a masteruseris configured, it differs from the current user, and the current process isroot. For the historical root-master case (current user == master user, or not root) the pathis byte-for-byte unchanged. In other words it is a no-op except in exactly the scenario that is currently broken -- a non-root (3006-default
salt) master with a colocated root minion.How this was tested
Because this is a fairly major architectural change, it was validated from several angles to build confidence that it both fixes the issue and does not regress unaffected calls.
Unit tests (
tests/pytests/unit/modules/test_saltutil.py): the user-selection logic (_master_user_runas) across root/non-root and matching/differing users; the dispatch from bothrunner()andwheel()(drops when needed, runs in-process otherwise); the fork+drop helper returns the result and propagates errors; and an end-to-end test that actually drops to an unprivileged user (runs under root in CI).Live, on a real 3006.25 install (in a container): master configured
user: salt(the packaged default) and running assalt, with a colocated minion running asroot. Each function was invoked as root viasalt-call --local, against a real git_pillar remote whose cache had been populated by (and is owned by) thesaltuser:getpass.getuser()rootsaltgit_pillar.updatedetected dubious ownership ... git_pillar/<name>/_->Falsegetpass.getuser()rootsaltfileserver.envs(regression)key.list_all(regression)So it both fixes the reported failure and leaves unaffected runner/wheel calls behaving identically.
Implementation note
The privilege drop runs in a short-lived fork child via
multiprocessing.get_context("fork"). I'm aware that importingmultiprocessingwas historically discouraged in Salt (I seem to recall @thatch45 said that importing multiprocessing was a fireable offence), but it is now used widely in-tree --salt/scripts.pysets the fork start method,salt/state.pyuses
get_context("fork"), andminion/master/gitfsimport it.Merge requirements satisfied?
Commits signed with GPG?
No