From 6d32460b13680d3ab250494ff517f02018b75e37 Mon Sep 17 00:00:00 2001 From: Maksim Shulga Date: Mon, 3 Aug 2026 17:44:34 +0000 Subject: [PATCH] Run the long-lived process as the configured non-root user (#31) * Run the long-lived process as the configured non-root user start.py used to stay root for its whole life; only the individual seaf-cli/seaf-daemon subprocess calls were unprivileged, via a per-call runuser wrapper. The orchestrating Python process itself, including the RPC socket to seaf-daemon and the watch_status() loop, ran as root the entire time. main() now drops privileges in-process (dsc.misc.drop_privileges, using os.initgroups/setgid/setuid) right after setup_uid()/create_dir() - which still need root to remap the seafile user's UID/GID and chown /dsc - and before SeafileClient is constructed. Since the process is already the target user by the time any seaf-cli call happens, runuser is removed from user_cmd(); it also could not be invoked from an already non-root process. Verified against the real image, not just mocked unit tests: a running container's PID 1 shows Uid: 1000 1000 1000 1000 in /proc/1/status, and tests/smoke.sh gained a check exercising setup_uid/create_dir/ drop_privileges together against the image's real /etc/passwd. * Isolate os.environ mutation in drop_privileges test drop_privileges() writes HOME/USER/LOGNAME directly to os.environ, and the test asserting that didn't restore them, leaking state into later tests. monkeypatch.delenv registers the keys so its teardown restores them regardless of who wrote to os.environ. --------- Co-authored-by: Maxim Shulga --- IMPROVEMENTS.md | 11 +++++++++- dsc/misc.py | 33 ++++++++++++++++++++++------ start.py | 3 ++- tests/smoke.sh | 12 +++++++++++ tests/test_commands.py | 49 +++++++++++++++++++++++++++++++++++++----- 5 files changed, 94 insertions(+), 14 deletions(-) diff --git a/IMPROVEMENTS.md b/IMPROVEMENTS.md index 1250af4..a0766e8 100644 --- a/IMPROVEMENTS.md +++ b/IMPROVEMENTS.md @@ -189,7 +189,16 @@ Acceptance criteria: ### Runtime hardening -- [ ] Run the long-lived process as the configured non-root user after setup. +- [x] Run the long-lived process as the configured non-root user after setup. + `start.py` used to stay root for its whole life; only the individual + `seaf-cli`/`seaf-daemon` subprocess calls were unprivileged (via + `runuser`). Now `main()` drops privileges in-process + (`dsc.misc.drop_privileges`, using `os.initgroups`/`setgid`/`setuid`) + right after `setup_uid()`/`create_dir()`, and before `SeafileClient` or + the daemon/RPC/watch loop start. `runuser` is gone from `user_cmd()` + since it can't be invoked from an already non-root process anyway. + Verified against the real image (not just mocked unit tests): a running + container's PID 1 shows `Uid: 1000 1000 1000 1000` in `/proc/1/status`. - [ ] Reject UID or GID zero unless a documented use case requires it. - [ ] Add a health check for the daemon and RPC endpoint. - [ ] Handle temporary RPC failures and daemon restarts without corrupting diff --git a/dsc/misc.py b/dsc/misc.py index 6744b74..037ac3c 100644 --- a/dsc/misc.py +++ b/dsc/misc.py @@ -42,17 +42,36 @@ def create_dir(dir_path: str): def user_cmd(argv: list) -> list: """ - Wrap a command so that it runs as the seafile user. + Return a command as-is, having already validated it is a plain argument + list and never a shell string. - ``runuser --`` execs the command directly. ``su -c`` would hand it to a - login shell instead, which turns every server-provided value in the - command into shell code. Like ``su``, ``runuser`` sets HOME, USER and a - default PATH for the target user, so seaf-cli still finds its config and - its own wrapper. + The process itself runs as the seafile user for its whole life after + drop_privileges() (see start.py), so subprocess calls no longer need to + be individually re-targeted with ``runuser``. """ if not isinstance(argv, list): raise TypeError(f"Command must be a list of arguments, got {type(argv).__name__}") - return ["runuser", "-u", DEFAULT_USERNAME, "--"] + argv + return argv + + +def drop_privileges(uid: int, gid: int): + """ + Permanently switch the current process to the given UID/GID. + + Must run after setup_uid()/create_dir(), which need root, and before any + other code, since everything from here on (the daemon, the RPC socket, + the sync loop) runs as this user for the rest of the process's life. + Order matters: supplementary groups and the GID must be set before the + UID is dropped, since giving up root removes the permission to change + them. + """ + user_pwinfo = pwd.getpwnam(DEFAULT_USERNAME) + os.initgroups(DEFAULT_USERNAME, gid) + os.setgid(gid) + os.setuid(uid) + os.environ["HOME"] = user_pwinfo.pw_dir + os.environ["USER"] = DEFAULT_USERNAME + os.environ["LOGNAME"] = DEFAULT_USERNAME def hide_secrets(cmd: list, *secrets: str) -> list: diff --git a/start.py b/start.py index cf1b409..d630758 100755 --- a/start.py +++ b/start.py @@ -9,7 +9,7 @@ import sys from dsc import SeafileClient, const from dsc.config import env_bool, env_int, env_str from dsc.errors import ConfigError, DscError, GracefulShutdown -from dsc.misc import setup_uid, create_dir +from dsc.misc import setup_uid, create_dir, drop_privileges from dsc.paths import plan_lib_dirs from dsc.secrets import resolve_secret @@ -148,6 +148,7 @@ def main(): setup_uid(args.uid, args.gid) create_dir(const.DEFAULT_APP_DIR) + drop_privileges(args.uid, args.gid) client = SeafileClient(args.server, args.username, password, const.DEFAULT_APP_DIR, token=token) diff --git a/tests/smoke.sh b/tests/smoke.sh index 5918cc9..7def2f6 100755 --- a/tests/smoke.sh +++ b/tests/smoke.sh @@ -150,6 +150,18 @@ print(secret, 'PASSWORD' in os.environ)" 2>&1 | tail -1) check "secret file read, password absent from the environment" \ "s3cret-from-file False" "$out" +echo "== the process drops root after setup and reaches the target uid/gid" +out=$(run "$IMAGE" python3 -c " +import os +from dsc.misc import setup_uid, create_dir, drop_privileges +from dsc import const +setup_uid(1000, 1000) +create_dir(const.DEFAULT_APP_DIR) +drop_privileges(1000, 1000) +print(os.getuid(), os.getgid(), os.environ['USER'])" 2>&1 | tail -1) +check "process runs as the target non-root user after drop_privileges" \ + "1000 1000 seafile" "$out" + echo "== a default container gets the documented defaults" out=$(run "$IMAGE" python3 -c " import start diff --git a/tests/test_commands.py b/tests/test_commands.py index c442197..65730cf 100644 --- a/tests/test_commands.py +++ b/tests/test_commands.py @@ -2,15 +2,15 @@ import argparse import json +import os import subprocess import pytest from dsc import client as client_module -from dsc import const from dsc.errors import ConfigError, DaemonError from dsc.client import SeafileClient -from dsc.misc import hide_secrets, user_cmd +from dsc.misc import hide_secrets, user_cmd, drop_privileges # Values that would be interpreted by a shell if a command were ever joined # into a string and handed to ``su -c``. @@ -56,9 +56,11 @@ def calls(monkeypatch): return recorded -def test_user_cmd_uses_runuser_without_a_shell(): +def test_user_cmd_returns_the_command_unchanged(): + # The process itself runs as the seafile user after drop_privileges(), + # so no per-call re-targeting is needed any more. argv = user_cmd(["seaf-cli", "status"]) - assert argv == ["runuser", "-u", const.DEFAULT_USERNAME, "--", "seaf-cli", "status"] + assert argv == ["seaf-cli", "status"] def test_user_cmd_never_invokes_a_shell(): @@ -74,6 +76,43 @@ def test_user_cmd_rejects_a_string(): user_cmd("seaf-cli status") +class FakePwInfo: + pw_dir = "/dsc" + + +@pytest.fixture +def fake_pwnam(monkeypatch): + monkeypatch.setattr("pwd.getpwnam", lambda name: FakePwInfo()) + + +def test_drop_privileges_sets_groups_before_gid_before_uid(monkeypatch, fake_pwnam): + """Order matters: root is needed for each step, and is gone after setuid.""" + calls = [] + monkeypatch.setattr("os.initgroups", lambda user, gid: calls.append(("initgroups", user, gid))) + monkeypatch.setattr("os.setgid", lambda gid: calls.append(("setgid", gid))) + monkeypatch.setattr("os.setuid", lambda uid: calls.append(("setuid", uid))) + + drop_privileges(1000, 1000) + + assert [c[0] for c in calls] == ["initgroups", "setgid", "setuid"] + + +def test_drop_privileges_sets_the_target_environment(monkeypatch, fake_pwnam): + monkeypatch.setattr("os.initgroups", lambda user, gid: None) + monkeypatch.setattr("os.setgid", lambda gid: None) + monkeypatch.setattr("os.setuid", lambda uid: None) + # drop_privileges() writes os.environ directly, not through monkeypatch, + # so register the keys with monkeypatch first to have it restore them. + for key in ("HOME", "USER", "LOGNAME"): + monkeypatch.delenv(key, raising=False) + + drop_privileges(1000, 1000) + + assert os.environ["USER"] == "seafile" + assert os.environ["LOGNAME"] == "seafile" + assert os.environ["HOME"] + + @pytest.mark.parametrize("secret", SHELL_METACHARACTERS) def test_sync_passes_the_token_as_one_unquoted_argument(client, calls, secret): client._SeafileClient__token = secret @@ -154,7 +193,7 @@ def test_configure_passes_option_values_as_arguments(client, calls, monkeypatch) client.configure(args, check_for_daemon=False) argvs = [argv for argv, _ in calls] - prefix = ["runuser", "-u", const.DEFAULT_USERNAME, "--", "seaf-cli", "config"] + prefix = ["seaf-cli", "config"] assert prefix + ["-k", "upload_limit"] in argvs assert prefix + ["-k", "upload_limit", "-v", "100"] in argvs # An option that seaf-cli does not know about is never forwarded.