Fix salt-minion supervisor leaving a privileged parent process (#68115)#69435
Open
dwoz wants to merge 1 commit into
Open
Fix salt-minion supervisor leaving a privileged parent process (#68115)#69435dwoz wants to merge 1 commit into
dwoz wants to merge 1 commit into
Conversation
`salt_minion()` and `salt_proxy()` fork a long-lived keepalive child that becomes the actual minion (and drops privileges to the configured `user` inside `Minion._real_start`). The parent supervisor process, however, retained root forever -- it only forwards signals and restarts the child on SALT_KEEPALIVE exits, yet it stayed at the head of the otherwise-unprivileged minion process tree. After the keepalive child has been spawned, parse the minion (or proxy) config in the parent and call `salt.utils.verify.check_user()` so the supervisor itself drops to the configured user. The child has already been forked at that point, so it still inherits root and can do its own `verify_env` chowns and pidfile setup before its own `check_user` call. Only drop on the first iteration of the keepalive loop; later iterations fork from the now-unprivileged parent and reuse directories created by the first child. Fixes saltstack#68115
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
1 participant
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.
What does this PR do?
salt_minion()andsalt_proxy()insalt/scripts.pyfork a long-livedkeepalive child that becomes the actual minion (and drops privileges to
the configured
userinsideMinion._real_start). The parentsupervisor process, however, retained root forever — it only forwards
signals and restarts the child on
SALT_KEEPALIVEexits, but it stayedat the head of the otherwise-unprivileged minion process tree.
After the keepalive child has been spawned, the parent now parses the
minion (or proxy) config and calls
salt.utils.verify.check_user()todrop to the configured user. The child has already been forked at that
point, so it still inherits root and can do its own
verify_envchownsand pidfile setup before its own
check_usercall. The drop onlyhappens on the first iteration of the keepalive loop; later iterations
fork from the now-unprivileged parent and reuse directories created by
the first child.
What issues does this PR fix or reference?
Fixes #68115
Previous Behavior
With
user: salt(or any non-root user) in the minion config, thepsoutput showed a root-ownedsalt-minionprocess sitting at thehead of a tree of
salt-ownedMinionKeepAlive/MinionManagerchildren. The root parent never did anything that required root, but
its presence defeated the security expectation of
user:.New Behavior
The parent supervisor drops to the configured user immediately after
spawning the keepalive child. The entire
salt-minionprocess tree isowned by the configured user. Same for
salt-proxy.Merge requirements satisfied?
user:docs nowmatch reality)
changelog/68115.fixed.md)tests/pytests/unit/test_scripts.py)Commits signed with GPG?
No