Fix salt-minion supervisor leaving a privileged parent process (#68115)

changelog/68115.fixed.md

@@ -0,0 +1 @@
+Fixed ``salt-minion`` and ``salt-proxy`` leaving a privileged (root) keepalive supervisor process at the head of an otherwise unprivileged minion process tree when ``user`` is set to a non-root account. The supervisor now drops privileges to the configured user once the keepalive child has been spawned.

salt/scripts.py

@@ -206,6 +206,7 @@ def escalate_signal_to_process(
     # keep one minion subprocess running
     prev_sigint_handler = signal.getsignal(signal.SIGINT)
     prev_sigterm_handler = signal.getsignal(signal.SIGTERM)
+    supervisor_privileges_dropped = False
     while True:
         try:
             process = multiprocessing.Process(
@@ -230,6 +231,19 @@ def escalate_signal_to_process(
             minion.start()
             break
 
+        # Drop the supervisor's privileges now that the child has been
+        # forked. The child inherits root and performs its own
+        # verify_env/pidfile/check_user dance; this parent only needs
+        # to forward signals and restart on keepalive exits. Only do
+        # this once: subsequent loop iterations re-fork from an already
+        # unprivileged parent, which is fine because the dirs created
+        # by the first child already have the right ownership.
+        if not supervisor_privileges_dropped:
+            import salt.config
+
+            _supervisor_drop_privileges(salt.config.minion_config, "minion")
+            supervisor_privileges_dropped = True
+
         process.join()
 
         # Process exited or was terminated. Since we're going to try to restart
@@ -348,6 +362,7 @@ def salt_proxy():
         return
 
     # keep one minion subprocess running
+    supervisor_privileges_dropped = False
     while True:
         try:
             queue = multiprocessing.Queue()
@@ -360,6 +375,14 @@ def salt_proxy():
             target=proxy_minion_process, args=(queue,), name="ProxyMinion"
         )
         process.start()
+        # See the matching note in salt_minion(); the supervisor only
+        # needs root for the first fork's verify_env/pidfile setup, and
+        # leaving it as root afterwards is the bug described in #68115.
+        if not supervisor_privileges_dropped:
+            import salt.config
+
+            _supervisor_drop_privileges(salt.config.proxy_config, "proxy")
+            supervisor_privileges_dropped = True
         try:
             process.join()
             try:
@@ -611,6 +634,80 @@ def _get_onedir_env_path():
     return None
 
 
+def _supervisor_config_dir():
+    """
+    Return the salt config directory to use when the keepalive supervisor
+    reads minion or proxy options.
+
+    Respects ``-c``/``--config-dir`` on the command line first, then the
+    ``SALT_CONFIG_DIR`` env var, then falls back to the compiled-in
+    default. Keeping this lookup simple and ``argparse``-free avoids
+    pulling in the full option parser (which the child process already
+    runs) and keeps the parent's privilege drop cheap.
+    """
+    import salt.syspaths
+
+    argv = sys.argv[1:]
+    skip_next = False
+    for idx, arg in enumerate(argv):
+        if skip_next:
+            skip_next = False
+            continue
+        if arg in ("-c", "--config-dir") and idx + 1 < len(argv):
+            return argv[idx + 1]
+        if arg.startswith("--config-dir="):
+            return arg.split("=", 1)[1]
+        if arg.startswith("-c") and len(arg) > 2:
+            return arg[2:]
+    env_dir = os.environ.get("SALT_CONFIG_DIR")
+    if env_dir:
+        return env_dir
+    return salt.syspaths.CONFIG_DIR
+
+
+def _supervisor_drop_privileges(config_loader, config_name):
+    """
+    Drop privileges in the salt-minion / salt-proxy keepalive supervisor.
+
+    The supervisor forks a child process that runs the actual minion;
+    only the child needs root to perform ``verify_env`` chowns and to
+    create the pidfile, and the child drops to the configured user via
+    ``salt.cli.daemons.Minion._real_start``. The parent only needs to
+    forward signals to the child and restart it on
+    ``SALT_KEEPALIVE``-flavoured exits -- neither requires privileges.
+
+    Leaving the parent as root meant an unprivileged minion process tree
+    still had a privileged root process at its head, defeating the
+    purpose of configuring ``user: <something other than root>``. See
+    issue #68115.
+
+    ``config_loader`` is ``salt.config.minion_config`` or
+    ``salt.config.proxy_config``; ``config_name`` is the corresponding
+    config file basename (``minion`` or ``proxy``).
+    """
+    import salt.utils.user
+    import salt.utils.verify
+
+    config_dir = _supervisor_config_dir()
+    config_file = os.path.join(config_dir, config_name)
+    try:
+        opts = config_loader(config_file)
+    except Exception:  # pylint: disable=broad-except
+        log.debug(
+            "Supervisor could not load %s config from %s; skipping privilege drop",
+            config_name,
+            config_file,
+            exc_info=True,
+        )
+        return
+    user = opts.get("user")
+    if not user:
+        return
+    if user == salt.utils.user.get_user():
+        return
+    salt.utils.verify.check_user(user)
+
+
 def salt_pip(config_dir=None):
     """
     Proxy to current python's pip

tests/pytests/unit/test_scripts.py

@@ -1,6 +1,11 @@
+import sys
+
 import pytest
 
+import salt.defaults.exitcodes
+import salt.scripts
 from salt.scripts import _pip_args, _pip_environment
+from tests.support.mock import MagicMock, patch
 
 
 def test_pip_environment_no_pypath():
@@ -78,3 +83,124 @@ def test_pip_args_installing_with_target():
     assert pargs is not args
     assert args == ["install", "--target=/tmp/bartest"]
     assert pargs == ["install", "--target=/tmp/bartest"]
+
+
+@pytest.mark.skip_on_windows(
+    reason="The keepalive supervisor only exists on non-Windows minions"
+)
+def test_salt_minion_supervisor_drops_privileges(monkeypatch):
+    """
+    Regression test for #68115.
+
+    ``salt_minion()`` forks a ``MinionKeepAlive`` child process that
+    eventually drops privileges to the configured ``user`` inside
+    ``Minion._real_start``. The parent supervisor process, however, used
+    to retain its original (root) uid forever, leaving an unprivileged
+    minion with a privileged parent visible in the process table. The
+    parent must drop privileges itself once the keepalive child has been
+    spawned.
+    """
+    monkeypatch.setattr(sys, "argv", ["salt-minion"])
+
+    fake_process = MagicMock()
+    fake_process.pid = 4321
+    fake_process.exitcode = 0
+
+    fake_multiprocessing = MagicMock()
+    fake_multiprocessing.Process.return_value = fake_process
+
+    fake_opts = {"user": "saltuser"}
+
+    with patch("salt.utils.platform.is_windows", return_value=False), patch(
+        "salt.utils.debug.enable_sigusr1_handler"
+    ), patch("salt.utils.process.notify_systemd"), patch(
+        "salt.config.minion_config", return_value=fake_opts
+    ), patch(
+        "salt.utils.user.get_user", return_value="root"
+    ), patch(
+        "salt.utils.verify.check_user", return_value=True
+    ) as check_user, patch.dict(
+        sys.modules, {"multiprocessing": fake_multiprocessing}
+    ):
+        with pytest.raises(SystemExit):
+            salt.scripts.salt_minion()
+
+    fake_multiprocessing.Process.assert_called_once()
+    fake_process.start.assert_called_once()
+    # The supervisor parent must drop privileges after spawning the child.
+    check_user.assert_called_once_with("saltuser")
+
+
+@pytest.mark.skip_on_windows(
+    reason="The keepalive supervisor only exists on non-Windows minions"
+)
+def test_salt_minion_supervisor_skips_drop_when_already_user(monkeypatch):
+    """
+    If the supervisor is already running as the configured user (or as a
+    non-root user without ``user`` set), no privilege drop is attempted.
+    """
+    monkeypatch.setattr(sys, "argv", ["salt-minion"])
+
+    fake_process = MagicMock()
+    fake_process.pid = 4321
+    fake_process.exitcode = 0
+
+    fake_multiprocessing = MagicMock()
+    fake_multiprocessing.Process.return_value = fake_process
+
+    fake_opts = {"user": "saltuser"}
+
+    with patch("salt.utils.platform.is_windows", return_value=False), patch(
+        "salt.utils.debug.enable_sigusr1_handler"
+    ), patch("salt.utils.process.notify_systemd"), patch(
+        "salt.config.minion_config", return_value=fake_opts
+    ), patch(
+        "salt.utils.user.get_user", return_value="saltuser"
+    ), patch(
+        "salt.utils.verify.check_user", return_value=True
+    ) as check_user, patch.dict(
+        sys.modules, {"multiprocessing": fake_multiprocessing}
+    ):
+        with pytest.raises(SystemExit):
+            salt.scripts.salt_minion()
+
+    check_user.assert_not_called()
+
+
+@pytest.mark.skip_on_windows(
+    reason="The keepalive supervisor only exists on non-Windows proxies"
+)
+def test_salt_proxy_supervisor_drops_privileges(monkeypatch):
+    """
+    Regression test for #68115. The ``salt-proxy`` supervisor has the
+    same keepalive shape as ``salt-minion`` and must likewise drop
+    privileges after forking the child.
+    """
+    monkeypatch.setattr(sys, "argv", ["salt-proxy"])
+
+    fake_process = MagicMock()
+    fake_process.pid = 4321
+    fake_process.exitcode = 0
+
+    fake_queue = MagicMock()
+    fake_queue.get.return_value = 0
+
+    fake_multiprocessing = MagicMock()
+    fake_multiprocessing.Process.return_value = fake_process
+    fake_multiprocessing.Queue.return_value = fake_queue
+
+    fake_opts = {"user": "saltuser"}
+
+    with patch("salt.utils.platform.is_windows", return_value=False), patch(
+        "salt.config.proxy_config", return_value=fake_opts
+    ), patch("salt.utils.user.get_user", return_value="root"), patch(
+        "salt.utils.verify.check_user", return_value=True
+    ) as check_user, patch.dict(
+        sys.modules, {"multiprocessing": fake_multiprocessing}
+    ):
+        with pytest.raises(SystemExit):
+            salt.scripts.salt_proxy()
+
+    fake_multiprocessing.Process.assert_called_once()
+    fake_process.start.assert_called_once()
+    check_user.assert_called_once_with("saltuser")