From 84d98a6e8ee148891d6f35364e82b4843bfc8e28 Mon Sep 17 00:00:00 2001 From: "Mark A. Grondona" Date: Sat, 2 Nov 2024 22:48:17 +0000 Subject: [PATCH] imp: remove kill subcommand Problem: The `flux-imp kill` subcommand is no longer necessary, since both IMP exec and run subcommands now have a privleged IMP parent process via which signals may be forwarded. Remove the `kill` subcommand and associated tests. --- src/imp/Makefile.am | 1 - src/imp/impcmd-list.c | 5 -- src/imp/kill.c | 185 --------------------------------------- t/Makefile.am | 1 - t/t2001-imp-kill.t | 196 ------------------------------------------ 5 files changed, 388 deletions(-) delete mode 100644 src/imp/kill.c delete mode 100755 t/t2001-imp-kill.t diff --git a/src/imp/Makefile.am b/src/imp/Makefile.am index 31ac9761..42d170fa 100644 --- a/src/imp/Makefile.am +++ b/src/imp/Makefile.am @@ -62,7 +62,6 @@ IMP_SOURCES = \ signals.h \ cgroup.c \ cgroup.h \ - kill.c \ run.c \ exec/user.h \ exec/user.c \ diff --git a/src/imp/impcmd-list.c b/src/imp/impcmd-list.c index 231c1b32..af576f4b 100644 --- a/src/imp/impcmd-list.c +++ b/src/imp/impcmd-list.c @@ -18,8 +18,6 @@ extern int imp_casign_unprivileged (struct imp_state *imp, struct kv *); extern int imp_casign_privileged (struct imp_state *imp, struct kv *); extern int imp_exec_unprivileged (struct imp_state *imp, struct kv *); extern int imp_exec_privileged (struct imp_state *imp, struct kv *); -extern int imp_kill_unprivileged (struct imp_state *imp, struct kv *); -extern int imp_kill_privileged (struct imp_state *imp, struct kv *); extern int imp_run_unprivileged (struct imp_state *imp, struct kv *); extern int imp_run_privileged (struct imp_state *imp, struct kv *); @@ -41,9 +39,6 @@ struct impcmd impcmd_list[] = { { "exec", imp_exec_unprivileged, imp_exec_privileged }, - { "kill", - imp_kill_unprivileged, - imp_kill_privileged }, { "run", imp_run_unprivileged, imp_run_privileged }, diff --git a/src/imp/kill.c b/src/imp/kill.c deleted file mode 100644 index 5bdd0e14..00000000 --- a/src/imp/kill.c +++ /dev/null @@ -1,185 +0,0 @@ -/************************************************************\ - * Copyright 2020 Lawrence Livermore National Security, LLC - * (c.f. AUTHORS, NOTICE.LLNS, COPYING) - * - * This file is part of the Flux resource manager framework. - * For details, see https://github.com/flux-framework. - * - * SPDX-License-Identifier: LGPL-3.0 -\************************************************************/ - -/* flux-imp kill - signal tasks on behalf of requestor when authorized - * - * PURPOSE: - * - * Allow a non-privileged Flux instance to signal processes in - * jobs running as different users. - * - * OPERATION: - * - * The IMP kill command currently works under the assumption that the - * multiuser instance is running under systemd with Delegate=yes. This - * setting directs systemd to delegate ownership of the flux.service - * cgroup to the user under which Flux is runnng, e.g. "flux". - * - * Since all Flux jobs will be executed within this cgroup or a child, - * flux-imp kill may authorize signal delivery to any task where - * the task's cgroup is owned by the requesting user. - * - * These assumptions hold even if Flux is using a systemd user instance - * to launch multi-user jobs, since the systemd instance spawning jobs - * will also be running under a delegated cgroup. - * - */ - -#if HAVE_CONFIG_H -#include -#endif - -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include - -#include "src/libutil/kv.h" -#include "src/libutil/strlcpy.h" - -#include "imp_log.h" -#include "imp_state.h" -#include "impcmd.h" -#include "privsep.h" -#include "pidinfo.h" - -/* Return true if the user executing the IMP is allowed to run - * 'flux-imp kill'. This is the same set of users allowed to run - * 'flux-imp exec', so look in exec.allowed-users. - */ -static bool imp_kill_allowed (const cf_t *conf) -{ - struct passwd * pwd = getpwuid (getuid ()); - const cf_t *exec = cf_get_in (conf, "exec"); - - if (pwd && exec) - return cf_array_contains (cf_get_in (exec, "allowed-users"), - pwd->pw_name); - return false; -} - -static void check_and_kill_process (struct imp_state *imp, pid_t pid, int sig) -{ - uid_t user = getuid (); - struct pid_info *p = NULL; - - if (!imp_kill_allowed (imp->conf)) - imp_die (1, "kill command not allowed"); - - if (!(p = pid_info_create ((pid_t) pid))) - imp_die (1, "kill: failed to initialize pid info: %s", - strerror (errno)); - if (sig == 0) { - printf ("pid=%ju owner=%ju cg_path=%s cg_owner=%ju", - (uintmax_t) pid, - (uintmax_t) p->pid_owner, - p->cg_path, - (uintmax_t) p->cg_owner); - pid_info_destroy (p); - return; - } - - /* Check if pid is in pids cgroup owned by IMP user */ - if (p->cg_owner != user - && p->pid_owner != user) - imp_die (1, - "kill: refusing request from uid=%ju to kill pid %jd (owner=%ju)", - (uintmax_t) user, - (intmax_t) pid, - (uintmax_t) p->cg_owner); - - /* If pid is owned by root and is a flux-imp process, then deliver - * signal to child process instead (presumably a job shell) - */ - if (p->pid_owner == 0 - && strcmp (p->command, "flux-imp") == 0) { - int count = pid_kill_children (pid, sig); - if (count < 0) - imp_die (1, - "kill: failed to signal flux-imp children: %s", - strerror (errno)); - else if (count == 0) - imp_die (1, "kill: killed 0 flux-imp children"); - } - else if (kill (pid, sig) < 0) - imp_die (1, "kill: %jd sig=%ju: %s", - (intmax_t) pid, - (uintmax_t) sig, - strerror (errno)); - - pid_info_destroy (p); -} - - -/* Read pid and signal from the privsep pipe, then check if user - * is allowed to kill the target process. - */ -int imp_kill_privileged (struct imp_state *imp, struct kv *kv) -{ - int64_t pid; - int64_t signum; - - if (kv_get (kv, "pid", KV_INT64, &pid) < 0) - imp_die (1, "kill: failed to get pid"); - if (kv_get (kv, "signal", KV_INT64, &signum) < 0) - imp_die (1, "kill: failed to get signal"); - - check_and_kill_process (imp, pid, signum); - return 0; -} - -/* Unprivileged process reads signal and pid from cmdline and - * sends to parent over privsep pipe. If not running privileged, - * try killing as requesting user (used for testing). - */ -int imp_kill_unprivileged (struct imp_state *imp, struct kv *kv) -{ - char *p = NULL; - int64_t pid = 0; - int64_t signum = -1; - - if (imp->argc < 4) - imp_die (1, "kill: Usage flux-imp kill SIGNAL PID"); - - errno = 0; - if ((signum = strtol (imp->argv[2], &p, 10)) < 0 - || errno != 0 - || *p != '\0') - imp_die (1, "kill: invalid SIGNAL %s", imp->argv[2]); - - /* PID of 0 is explicitly forbidden here as it could be used - * to inadvertently kill our parent. - */ - if ((pid = strtol (imp->argv[3], &p, 10)) == 0 - || *p != '\0') - imp_die (1, "kill: invalid PID %s", imp->argv[3]); - - if (kv_put (kv, "pid", KV_INT64, pid) < 0) - imp_die (1, "kill: kv_put pid: %s", strerror (errno)); - if (kv_put (kv, "signal", KV_INT64, signum) < 0) - imp_die (1, "kill: kv_put signum: %s", strerror (errno)); - - if (!imp->ps) - check_and_kill_process (imp, pid, signum); - else if (privsep_write_kv (imp->ps, kv) < 0) - imp_die (1, "kill: failed to communicate with privsep parent"); - - return 0; -} - -/* vi: ts=4 sw=4 expandtab - */ diff --git a/t/Makefile.am b/t/Makefile.am index 6693203f..36845bd3 100644 --- a/t/Makefile.am +++ b/t/Makefile.am @@ -21,7 +21,6 @@ TESTSCRIPTS = \ t1002-sign-munge.t \ t1003-sign-curve.t \ t2000-imp-exec.t \ - t2001-imp-kill.t \ t2002-imp-run.t \ t2003-imp-exec-pam.t diff --git a/t/t2001-imp-kill.t b/t/t2001-imp-kill.t deleted file mode 100755 index 796e95a3..00000000 --- a/t/t2001-imp-kill.t +++ /dev/null @@ -1,196 +0,0 @@ -#!/bin/sh -# - -test_description='IMP kill basic functionality test - -Basic flux-imp kill failure handling -' - -# Append --logfile option if FLUX_TESTS_LOGFILE is set in environment: -test -n "$FLUX_TESTS_LOGFILE" && set -- "$@" --logfile -. `dirname $0`/sharness.sh - -flux_imp=${SHARNESS_BUILD_DIRECTORY}/src/imp/flux-imp - -sign=${SHARNESS_BUILD_DIRECTORY}/t/src/sign - -fake_imp_input() { - printf '{"J":"%s"}' $(echo $1 | $sign) -} - -echo "# Using ${flux_imp}" - -test_expect_success 'flux-imp kill: returns error when run with no args' ' - test_must_fail $flux_imp kill -' -test_expect_success 'flux-imp kill: returns error when run with 1 arg' ' - test_must_fail $flux_imp kill 15 -' -test_expect_success 'flux-imp kill: returns error with pid=0' ' - test_must_fail $flux_imp kill 15 0 -' -test_expect_success 'flux-imp kill: returns error with invalid signal' ' - test_must_fail $flux_imp kill -15 0 -' -test_expect_success 'flux-imp kill: checks exec.allowed-users' ' - name=wronguser && - cat <<-EOF >${name}.toml && - [exec] - allowed-users = [] - EOF - ( export FLUX_IMP_CONFIG_PATTERN=${name}.toml && - test_must_fail $flux_imp kill 15 1234 >${name}.log 2>&1 - ) && - test_debug "cat ${name}.log" && - grep "kill command not allowed" ${name}.log && unset name -' -test_expect_success SUDO 'flux-imp kill: sudo: user must be in allowed-users' ' - name=wrongusersudo && - cat <<-EOF >${name}.toml && - allow-sudo = true - [exec] - allowed-users = [] - EOF - test_must_fail $SUDO FLUX_IMP_CONFIG_PATTERN=${name}.toml \ - $flux_imp kill 15 1234 >${name}.log 2>&1 && - test_debug "cat ${name}.log" && - grep -i "kill command not allowed" ${name}.log && unset name -' - -test "$chain_lint" = "t" || test_set_prereq NO_CHAIN_LINT - - -test "$(stat -fc %T /sys/fs/cgroup 2>/dev/null)" = "cgroup2fs" \ - -o "$(stat -fc %T /sys/fs/cgroup/unified 2>/dev/null)" = "cgroup2fs" \ - -o "$(stat -fc %T /sys/fs/cgroup/systemd 2>/dev/null)" = "cgroup2fs" \ - -o "$(stat -fc %T /sys/fs/cgroup/systemd 2>/dev/null)" = "cgroupfs" \ - && test_set_prereq SYSTEMD_CGROUP - -test_expect_success NO_CHAIN_LINT,SYSTEMD_CGROUP 'flux-imp kill: works in unpriv mode' ' - name=unpriv && - cat <<-EOF >${name}.toml - allow-sudo = true - [exec] - allowed-users = [ "$(whoami)" ] - EOF - sleep 300 & pid=$! && - FLUX_IMP_CONFIG_PATTERN=${name}.toml \ - $flux_imp kill 15 $pid >${name}.log 2>&1 && - test_debug "cat ${name}.log" && - test_expect_code 143 wait $pid -' -test_expect_success NO_CHAIN_LINT,SYSTEMD_CGROUP,SUDO 'flux-imp kill: works with sudo' ' - name=sudo && - cat <<-EOF >${name}.toml - allow-sudo = true - [exec] - allowed-users = [ "$(whoami)" ] - EOF - sleep 300 & pid=$! && - $SUDO FLUX_IMP_CONFIG_PATTERN=${name}.toml \ - $flux_imp kill 15 $pid >${name}.log 2>&1 && - test_expect_code 143 wait $pid -' -test_expect_success SYSTEMD_CGROUP 'flux-imp kill: fails for nonexistent pid' ' - name=pid-noexist && - cat <<-EOF >${name}.toml && - allow-sudo = true - [exec] - allowed-users = [ "$(whoami)" ] - EOF - for pid in `seq 10000 12000`; do - kill -s 0 ${pid} >/dev/null 2>&1 || break - done && - ( export FLUX_IMP_CONFIG_PATTERN=${name}.toml && - test_must_fail $flux_imp kill 15 $pid >${name}.log 2>&1 - ) && - test_debug "cat ${name}.log" && - grep "No such file or directory" ${name}.log -' -test_expect_success SUDO,SYSTEMD_CGROUP 'flux-imp kill: fails for nonexistent pid under sudo' ' - name=noexist-pid-sudo && - cat <<-EOF >${name}.toml && - allow-sudo = true - [exec] - allowed-users = [ "$(whoami)" ] - EOF - for pid in `seq 10000 12000`; do - kill -s 0 ${pid} >/dev/null 2>&1 || break - done && - test_must_fail $SUDO FLUX_IMP_CONFIG_PATTERN=${name}.toml \ - $flux_imp kill 15 $pid >${name}.log 2>&1 && - test_debug "cat ${name}.log" && - grep "No such file or directory" ${name}.log -' -test_expect_success 'create config for flux-imp exec and signer' ' - cat <<-EOF >sign-none.toml - allow-sudo = true - [sign] - max-ttl = 30 - default-type = "none" - allowed-types = [ "none" ] - [exec] - allowed-users = [ "$(whoami)" ] - allowed-shells = [ "$(pwd)/sleeper.sh" ] - allow-unprivileged-exec = true - EOF -' - -# Need a setuid IMP for the following kill tests to work. This is required -# because on most systemd systems, sudo may migrate children to a root-owned -# cgroup, and `flux-imp kill` uses the ownership of the containing cgroup -# to determine authorization to signal a process -test_expect_success SUDO 'attempt to create setuid copy of flux-imp for testing' ' - sudo cp $flux_imp . && - sudo chmod 4755 ./flux-imp -' - -# In case tests are running on filesystem with nosuid, check that permissions -# of flux-imp are actually setuid, and if so set SUID_ENABLED prereq -# -test -n "$(find ./flux-imp -perm 4755)" && test_set_prereq SUID_ENABLED - -# In order for the following kill tests to work, the owner of the current -# cgroup must be the current user running the test. -# -test "$(FLUX_IMP_CONFIG_PATTERN=sign-none.toml ./flux-imp kill 0 $$ \ - | sed 's/.*cg_owner=//')" = "$(id -u)" && test_set_prereq USER_CGROUP - -waitfile() -{ - count=0 - while ! grep "$2" $1 >/dev/null 2>&1; do - sleep 0.2 - count=$(($count + 1)) - test $count -gt 20 && break - done - grep "$2" $1 >/dev/null -} - -test_expect_success NO_CHAIN_LINT,SUID_ENABLED,USER_CGROUP \ -'flux-imp kill signals children of another flux-imp' ' - cat <<-EOF >sleeper.sh && - #!/bin/sh - echo "\$@" - printf "\$PPID\n" >$(pwd)/sleeper.pid - /bin/sleep "\$@" & - pid=\$! && - trap "echo got SIGTERM; kill \$pid" TERM - wait - echo sleep exited with \$? >&2 - EOF - chmod +x sleeper.sh && - export FLUX_IMP_CONFIG_PATTERN=sign-none.toml && - fake_imp_input foo | \ - ./flux-imp exec $(pwd)/sleeper.sh 30 >sleeper.out 2>&1 & - pid=$! && - waitfile sleeper.pid ".*" && - test_debug "echo calling flux-imp kill $(cat sleeper.pid)" && - FLUX_IMP_CONFIG_PATTERN=$(pwd)/unpriv.toml \ - ./flux-imp kill 15 $(cat sleeper.pid) && - wait $pid && - test_debug "echo waiting for output file" && - grep "got SIGTERM" sleeper.out -' - -test_done