diff --git a/changelog/68115.fixed.md b/changelog/68115.fixed.md new file mode 100644 index 00000000000..5dfa7311a99 --- /dev/null +++ b/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. diff --git a/salt/scripts.py b/salt/scripts.py index 06a24160b41..f651cb02b87 100644 --- a/salt/scripts.py +++ b/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: ``. 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 diff --git a/tests/pytests/unit/test_scripts.py b/tests/pytests/unit/test_scripts.py index a636275e0de..217f7352ab8 100644 --- a/tests/pytests/unit/test_scripts.py +++ b/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")