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.