From b20e9dc51596f787b0e0c6c2d4d21485e8d670c9 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Jan 2024 18:48:53 +0100 Subject: [PATCH 1/5] env-util: add new setenvf() helper And convert some pieces of code over. --- src/basic/env-util.c | 31 ++++++++++++++++++++++++++----- src/basic/env-util.h | 2 ++ src/journal/cat.c | 10 ++++------ src/userdb/userdbd-manager.c | 8 ++++---- 4 files changed, 36 insertions(+), 15 deletions(-) diff --git a/src/basic/env-util.c b/src/basic/env-util.c index 23a128bfd1..22eb3308e4 100644 --- a/src/basic/env-util.c +++ b/src/basic/env-util.c @@ -1012,8 +1012,8 @@ int putenv_dup(const char *assignment, bool override) { } int setenv_systemd_exec_pid(bool update_only) { - char str[DECIMAL_STR_MAX(pid_t)]; const char *e; + int r; /* Update $SYSTEMD_EXEC_PID=pid except when '*' is set for the variable. */ @@ -1024,10 +1024,9 @@ int setenv_systemd_exec_pid(bool update_only) { if (streq_ptr(e, "*")) return 0; - xsprintf(str, PID_FMT, getpid_cached()); - - if (setenv("SYSTEMD_EXEC_PID", str, 1) < 0) - return -errno; + r = setenvf("SYSTEMD_EXEC_PID", /* overwrite= */ 1, PID_FMT, getpid_cached()); + if (r < 0) + return r; return 1; } @@ -1122,3 +1121,25 @@ int set_full_environment(char **env) { return 0; } + +int setenvf(const char *name, bool overwrite, const char *valuef, ...) { + _cleanup_free_ char *value = NULL; + va_list ap; + int r; + + assert(name); + + if (!valuef) + return RET_NERRNO(unsetenv(name)); + + va_start(ap, valuef); + DISABLE_WARNING_FORMAT_NONLITERAL; + r = vasprintf(&value, valuef, ap); + REENABLE_WARNING; + va_end(ap); + + if (r < 0) + return -ENOMEM; + + return RET_NERRNO(setenv(name, value, overwrite)); +} diff --git a/src/basic/env-util.h b/src/basic/env-util.h index 8e77cc71d6..332efcf1b7 100644 --- a/src/basic/env-util.h +++ b/src/basic/env-util.h @@ -80,3 +80,5 @@ int getenv_path_list(const char *name, char ***ret_paths); int getenv_steal_erase(const char *name, char **ret); int set_full_environment(char **env); + +int setenvf(const char *name, bool overwrite, const char *valuef, ...) _printf_(3,4); diff --git a/src/journal/cat.c b/src/journal/cat.c index 609ddbaf6b..0325add12f 100644 --- a/src/journal/cat.c +++ b/src/journal/cat.c @@ -12,6 +12,7 @@ #include "alloc-util.h" #include "build.h" +#include "env-util.h" #include "fd-util.h" #include "format-util.h" #include "main-func.h" @@ -157,7 +158,6 @@ static int run(int argc, char *argv[]) { if (argc <= optind) (void) execl("/bin/cat", "/bin/cat", NULL); else { - _cleanup_free_ char *s = NULL; struct stat st; if (fstat(STDERR_FILENO, &st) < 0) @@ -165,11 +165,9 @@ static int run(int argc, char *argv[]) { "Failed to fstat(%s): %m", FORMAT_PROC_FD_PATH(STDERR_FILENO)); - if (asprintf(&s, DEV_FMT ":" INO_FMT, (dev_t)st.st_dev, st.st_ino) < 0) - return log_oom(); - - if (setenv("JOURNAL_STREAM", s, /* overwrite = */ true) < 0) - return log_error_errno(errno, "Failed to set environment variable JOURNAL_STREAM: %m"); + r = setenvf("JOURNAL_STREAM", /* overwrite = */ true, DEV_FMT ":" INO_FMT, (dev_t) st.st_dev, st.st_ino); + if (r < 0) + return log_error_errno(r, "Failed to set environment variable JOURNAL_STREAM: %m"); (void) execvp(argv[optind], argv + optind); } diff --git a/src/userdb/userdbd-manager.c b/src/userdb/userdbd-manager.c index bdaaab2c05..73b6d72026 100644 --- a/src/userdb/userdbd-manager.c +++ b/src/userdb/userdbd-manager.c @@ -5,6 +5,7 @@ #include "sd-daemon.h" #include "common-signal.h" +#include "env-util.h" #include "fd-util.h" #include "fs-util.h" #include "mkdir.h" @@ -157,7 +158,6 @@ static int start_one_worker(Manager *m) { if (r < 0) return log_error_errno(r, "Failed to fork new worker child: %m"); if (r == 0) { - char pids[DECIMAL_STR_MAX(pid_t)]; /* Child */ if (m->listen_fd == 3) { @@ -175,9 +175,9 @@ static int start_one_worker(Manager *m) { safe_close(m->listen_fd); } - xsprintf(pids, PID_FMT, pid); - if (setenv("LISTEN_PID", pids, 1) < 0) { - log_error_errno(errno, "Failed to set $LISTEN_PID: %m"); + r = setenvf("LISTEN_PID", /* overwrite= */ true, PID_FMT, pid); + if (r < 0) { + log_error_errno(r, "Failed to set $LISTEN_PID: %m"); _exit(EXIT_FAILURE); } From 3b1e80f7cb49971b98552f064c494f98b6243505 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Jan 2024 22:25:22 +0100 Subject: [PATCH 2/5] process-util: turn off O_NONBLOCK on stdio fds when rearranging fds We often create our fds O_NONBLOCK, but when we want to invoke some program with them as stdin/stdout/stderr we really should turn it off again. --- src/basic/fd-util.c | 13 +++++++++++++ src/basic/fd-util.h | 2 ++ src/basic/process-util.c | 3 +++ 3 files changed, 18 insertions(+) diff --git a/src/basic/fd-util.c b/src/basic/fd-util.c index 1a279690d2..38866ebb78 100644 --- a/src/basic/fd-util.c +++ b/src/basic/fd-util.c @@ -170,6 +170,19 @@ int fd_nonblock(int fd, bool nonblock) { return RET_NERRNO(fcntl(fd, F_SETFL, nflags)); } +int stdio_disable_nonblock(void) { + int ret = 0; + + /* stdin/stdout/stderr really should have O_NONBLOCK, which would confuse apps if left on, as + * write()s might unexpectedly fail with EAGAIN. */ + + RET_GATHER(ret, fd_nonblock(STDIN_FILENO, false)); + RET_GATHER(ret, fd_nonblock(STDOUT_FILENO, false)); + RET_GATHER(ret, fd_nonblock(STDERR_FILENO, false)); + + return ret; +} + int fd_cloexec(int fd, bool cloexec) { int flags, nflags; diff --git a/src/basic/fd-util.h b/src/basic/fd-util.h index 4bdd61fe54..6a1143b4f3 100644 --- a/src/basic/fd-util.h +++ b/src/basic/fd-util.h @@ -62,6 +62,8 @@ DEFINE_TRIVIAL_CLEANUP_FUNC_FULL(DIR*, closedir, NULL); #define _cleanup_close_pair_ _cleanup_(close_pairp) int fd_nonblock(int fd, bool nonblock); +int stdio_disable_nonblock(void); + int fd_cloexec(int fd, bool cloexec); int fd_cloexec_many(const int fds[], size_t n_fds, bool cloexec); diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 4d5c01d2cf..0f6cace426 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1667,6 +1667,9 @@ int safe_fork_full( log_full_errno(prio, r, "Failed to rearrange stdio fds: %m"); _exit(EXIT_FAILURE); } + + /* Turn off O_NONBLOCK on the fdio fds, in case it was left on */ + stdio_disable_nonblock(); } else { r = make_null_stdio(); if (r < 0) { From 07dca3c4b013bf9de440436142986408b4a3537d Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Jan 2024 18:49:12 +0100 Subject: [PATCH 3/5] varlink: turn off O_NONBLOCK in exec: transport If we invoke our own varlink implementation we'll turn on O_NONBLOCK right-away again, hence there is little point in turning it off, however, I generally think we probably should always path this fd as blocking, since that is the default after all, and invoked processes might want to use it like that. Or to see this differently: I think the varlink fd passed for activation in many ways is similar to and as fundamental as stdin/stderr/stdout, hence should probably be synchronous by default. --- src/shared/varlink.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/shared/varlink.c b/src/shared/varlink.c index 2b40c7f3bd..e24da3f893 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -450,6 +450,10 @@ int varlink_connect_exec(Varlink **ret, const char *_command, char **_argv) { if (socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0, pair) < 0) return log_debug_errno(errno, "Failed to allocate AF_UNIX socket pair: %m"); + r = fd_nonblock(pair[1], false); + if (r < 0) + return log_debug_errno(r, "Failed to disable O_NONBLOCK for varlink socket: %m"); + r = safe_fork_full( "(sd-vlexec)", /* stdio_fds= */ NULL, From a1bb30de7f5d9c4f7f01061240d984a786d5d00c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Jan 2024 22:26:17 +0100 Subject: [PATCH 4/5] varlink: add "ssh:" transport This uses openssh 9.4's -W support for AF_UNIX. Unfortunately older versions don't work with this, and I couldn#t figure a way that would work for older versions too, would not be racy and where we'd still could keep track of the forked off ssh process. Unfortunately, on older versions -W will just hang (because it tries to resolve the AF_UNIX path as regular host name), which sucks, but hopefully this issue will go away sooner or later on its own, as distributions update. Fedora is still stuck at 9.3 at the time of posting this (even on Fedora), even though 9.4, 9.5, 9.6 have all already been released by now. Example: varlinkctl call -j ssh:root@somehost:/run/systemd/io.systemd.Credentials io.systemd.Credentials.Encrypt '{"text":"foobar"}' --- docs/ENVIRONMENT.md | 5 ++ man/varlinkctl.xml | 7 +- src/shared/varlink.c | 113 ++++++++++++++++++++++---- test/TEST-74-AUX-UTILS/test.sh | 2 + test/units/testsuite-74.varlinkctl.sh | 26 ++++++ 5 files changed, 136 insertions(+), 17 deletions(-) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index 0113fd59fa..e3daabe3bc 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -610,3 +610,8 @@ SYSTEMD_HOME_DEBUG_SUFFIX=foo \ latter two via the environment variable unless `systemd-storagetm` is invoked to expose a single device only, since those identifiers better should be kept unique. + +Tools using the Varlink protocol, such as `varlinkctl`: + +* `$SYSTEMD_SSH` – the ssh binary to invoke when the `ssh:` transport is + used. May be a filename (which is searched for in `$PATH`) or absolute path. diff --git a/man/varlinkctl.xml b/man/varlinkctl.xml index 77acaab0f5..eff49af349 100644 --- a/man/varlinkctl.xml +++ b/man/varlinkctl.xml @@ -71,11 +71,16 @@ A Varlink service reference starting with the unix: string, followed - by an absolute AF_UNIX path, or by @ and an arbitrary string + by an absolute AF_UNIX socket path, or by @ and an arbitrary string (the latter for referencing sockets in the abstract namespace). A Varlink service reference starting with the exec: string, followed by an absolute path of a binary to execute. + + A Varlink service reference starting with the ssh: string, followed + by an SSH host specification, followed by :, followed by an absolute + AF_UNIX socket path. (This requires OpenSSH 9.4 or newer on the server side, + abstract namespace sockets are not supported.) For convenience these two simpler (redundant) service address syntaxes are also supported: diff --git a/src/shared/varlink.c b/src/shared/varlink.c index e24da3f893..67ed765233 100644 --- a/src/shared/varlink.c +++ b/src/shared/varlink.c @@ -511,39 +511,120 @@ int varlink_connect_exec(Varlink **ret, const char *_command, char **_argv) { return 0; } +static int varlink_connect_ssh(Varlink **ret, const char *where) { + _cleanup_close_pair_ int pair[2] = EBADF_PAIR; + _cleanup_(sigkill_waitp) pid_t pid = 0; + int r; + + assert_return(ret, -EINVAL); + assert_return(where, -EINVAL); + + /* Connects to an SSH server via OpenSSH 9.4's -W switch to connect to a remote AF_UNIX socket. For + * now we do not expose this function directly, but only via varlink_connect_url(). */ + + const char *ssh = secure_getenv("SYSTEMD_SSH") ?: "ssh"; + if (!path_is_valid(ssh)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "SSH path is not valid, refusing: %s", ssh); + + const char *e = strchr(where, ':'); + if (!e) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "SSH specification lacks a : separator between host and path, refusing: %s", where); + + _cleanup_free_ char *h = strndup(where, e - where); + if (!h) + return log_oom_debug(); + + _cleanup_free_ char *c = strdup(e + 1); + if (!c) + return log_oom_debug(); + + if (!path_is_absolute(c)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Remote AF_UNIX socket path is not absolute, refusing: %s", c); + + _cleanup_free_ char *p = NULL; + r = path_simplify_alloc(c, &p); + if (r < 0) + return r; + + if (!path_is_normalized(p)) + return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Specified path is not normalized, refusing: %s", p); + + log_debug("Forking off SSH child process '%s -W %s %s'.", ssh, p, h); + + if (socketpair(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC|SOCK_NONBLOCK, 0, pair) < 0) + return log_debug_errno(errno, "Failed to allocate AF_UNIX socket pair: %m"); + + r = safe_fork_full( + "(sd-vlssh)", + /* stdio_fds= */ (int[]) { pair[1], pair[1], STDERR_FILENO }, + /* except_fds= */ NULL, + /* n_except_fds= */ 0, + FORK_RESET_SIGNALS|FORK_CLOSE_ALL_FDS|FORK_DEATHSIG_SIGTERM|FORK_REOPEN_LOG|FORK_LOG|FORK_RLIMIT_NOFILE_SAFE|FORK_REARRANGE_STDIO, + &pid); + if (r < 0) + return log_debug_errno(r, "Failed to spawn process: %m"); + if (r == 0) { + /* Child */ + + execlp(ssh, "ssh", "-W", p, h, NULL); + log_debug_errno(errno, "Failed to invoke %s: %m", ssh); + _exit(EXIT_FAILURE); + } + + pair[1] = safe_close(pair[1]); + + Varlink *v; + r = varlink_new(&v); + if (r < 0) + return log_debug_errno(r, "Failed to create varlink object: %m"); + + v->fd = TAKE_FD(pair[0]); + v->af = AF_UNIX; + v->exec_pid = TAKE_PID(pid); + varlink_set_state(v, VARLINK_IDLE_CLIENT); + + *ret = v; + return 0; +} + int varlink_connect_url(Varlink **ret, const char *url) { _cleanup_free_ char *c = NULL; const char *p; - bool exec; + enum { + SCHEME_UNIX, + SCHEME_EXEC, + SCHEME_SSH, + } scheme; int r; assert_return(ret, -EINVAL); assert_return(url, -EINVAL); - // FIXME: Add support for vsock:, ssh-exec:, ssh-unix: URL schemes here. (The latter with OpenSSH - // 9.4's -W switch for referencing remote AF_UNIX sockets.) + // FIXME: Maybe add support for vsock: and ssh-exec: URL schemes here. - /* The Varlink URL scheme is a bit underdefined. We support only the unix: transport for now, plus an - * exec: transport we made up ourselves. Strictly speaking this shouldn't even be called URL, since - * it has nothing to do with Internet URLs by RFC. */ + /* The Varlink URL scheme is a bit underdefined. We support only the spec-defined unix: transport for + * now, plus exec:, ssh: transports we made up ourselves. Strictly speaking this shouldn't even be + * called "URL", since it has nothing to do with Internet URLs by RFC. */ p = startswith(url, "unix:"); if (p) - exec = false; - else { - p = startswith(url, "exec:"); - if (!p) - return log_debug_errno(SYNTHETIC_ERRNO(EPROTONOSUPPORT), "URL scheme not supported."); - - exec = true; - } + scheme = SCHEME_UNIX; + else if ((p = startswith(url, "exec:"))) + scheme = SCHEME_EXEC; + else if ((p = startswith(url, "ssh:"))) + scheme = SCHEME_SSH; + else + return log_debug_errno(SYNTHETIC_ERRNO(EPROTONOSUPPORT), "URL scheme not supported."); /* The varlink.org reference C library supports more than just file system paths. We might want to * support that one day too. For now simply refuse that. */ if (p[strcspn(p, ";?#")] != '\0') return log_debug_errno(SYNTHETIC_ERRNO(EPROTONOSUPPORT), "URL parameterization with ';', '?', '#' not supported."); - if (exec || p[0] != '@') { /* no validity checks for abstract namespace */ + if (scheme == SCHEME_SSH) + return varlink_connect_ssh(ret, p); + + if (scheme == SCHEME_EXEC || p[0] != '@') { /* no path validity checks for abstract namespace sockets */ if (!path_is_absolute(p)) return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Specified path not absolute, refusing."); @@ -556,7 +637,7 @@ int varlink_connect_url(Varlink **ret, const char *url) { return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Specified path is not normalized, refusing."); } - if (exec) + if (scheme == SCHEME_EXEC) return varlink_connect_exec(ret, c, NULL); return varlink_connect_address(ret, c ?: p); diff --git a/test/TEST-74-AUX-UTILS/test.sh b/test/TEST-74-AUX-UTILS/test.sh index e3eb62f198..2d17630d29 100755 --- a/test/TEST-74-AUX-UTILS/test.sh +++ b/test/TEST-74-AUX-UTILS/test.sh @@ -25,6 +25,8 @@ test_append_files() { install_mdadm generate_module_dependencies fi + + image_install socat } do_test "$@" diff --git a/test/units/testsuite-74.varlinkctl.sh b/test/units/testsuite-74.varlinkctl.sh index 5a962699c7..d97de3f604 100755 --- a/test/units/testsuite-74.varlinkctl.sh +++ b/test/units/testsuite-74.varlinkctl.sh @@ -53,6 +53,32 @@ if [[ -x /usr/lib/systemd/systemd-pcrextend ]]; then varlinkctl introspect /usr/lib/systemd/systemd-pcrextend io.systemd.PCRExtend fi +# SSH transport +SSHBINDIR="$(mktemp -d)" + +rm_rf_sshbindir() { + rm -rf "$SSHBINDIR" +} + +trap rm_rf_sshbindir EXIT + +# Create a fake "ssh" binary that validates everything works as expected +cat > "$SSHBINDIR"/ssh <<'EOF' +#!/bin/sh + +set -xe + +test "$1" = "-W" +test "$2" = "/run/systemd/journal/io.systemd.journal" +test "$3" = "foobar" + +exec socat - UNIX-CONNECT:/run/systemd/journal/io.systemd.journal +EOF + +chmod +x "$SSHBINDIR"/ssh + +SYSTEMD_SSH="$SSHBINDIR/ssh" varlinkctl info ssh:foobar:/run/systemd/journal/io.systemd.journal + # Go through all varlink sockets we can find under /run/systemd/ for some extra coverage find /run/systemd/ -name "io.systemd*" -type s | while read -r socket; do varlinkctl info "$socket" From bcb1bb379e8b888dc67494910ac9172c5cf32da6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 8 Jan 2024 23:06:48 +0100 Subject: [PATCH 5/5] sd-bus: also intrepret $SYSTEMD_SSH env var To make things symmetric to the $SYSTEMD_SSH logic that the varlink transport supports, let's also honour such a variable in sd-bus when picking ssh transport. --- docs/ENVIRONMENT.md | 3 ++- src/libsystemd/sd-bus/sd-bus.c | 12 +++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index e3daabe3bc..c96e6db85e 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -611,7 +611,8 @@ SYSTEMD_HOME_DEBUG_SUFFIX=foo \ to expose a single device only, since those identifiers better should be kept unique. -Tools using the Varlink protocol, such as `varlinkctl`: +Tools using the Varlink protocol (such as `varlinkctl`) or sd-bus (such as +`busctl`): * `$SYSTEMD_SSH` – the ssh binary to invoke when the `ssh:` transport is used. May be a filename (which is searched for in `$PATH`) or absolute path. diff --git a/src/libsystemd/sd-bus/sd-bus.c b/src/libsystemd/sd-bus/sd-bus.c index f036a49c64..86b626595e 100644 --- a/src/libsystemd/sd-bus/sd-bus.c +++ b/src/libsystemd/sd-bus/sd-bus.c @@ -1486,9 +1486,15 @@ interpret_port_as_machine_old_syntax: return -ENOMEM; } - a = strjoin("unixexec:path=ssh,argv1=-xT", p ? ",argv2=-p,argv3=" : "", strempty(p), - ",argv", p ? "4" : "2", "=--,argv", p ? "5" : "3", "=", e, - ",argv", p ? "6" : "4", "=systemd-stdio-bridge", c); + const char *ssh = secure_getenv("SYSTEMD_SSH") ?: "ssh"; + _cleanup_free_ char *ssh_escaped = bus_address_escape(ssh); + if (!ssh_escaped) + return -ENOMEM; + + a = strjoin("unixexec:path=", ssh_escaped, ",argv1=-xT", + p ? ",argv2=-p,argv3=" : "", strempty(p), + ",argv", p ? "4" : "2", "=--,argv", p ? "5" : "3", "=", e, + ",argv", p ? "6" : "4", "=systemd-stdio-bridge", c); if (!a) return -ENOMEM;