From bc8897d0039d4d53c5f38f2bbffa381a01e1fdd1 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Thu, 13 Aug 2026 22:24:57 -0500 Subject: [PATCH 1/7] Added --ask-pass, --password-file and --switch-user-command options Ticket: ENT-14418 Changelog: title Signed-off-by: Nick Anderson Co-Authored-By: Claude Opus 5 (1M context) --- .gitignore | 2 + README.md | 34 +++++++ cf_remote/aramid.py | 56 ++++++++--- cf_remote/args.py | 31 ++++++ cf_remote/main.py | 9 ++ cf_remote/ssh.py | 142 +++++++++++++++++++++++++- tests/docker/sudo/Dockerfile | 26 +++++ tests/shell/002_sudo_password.sh | 125 +++++++++++++++++++++++ tests/shell/all.sh | 1 + tests/test_ssh.py | 164 +++++++++++++++++++++++++++++++ 10 files changed, 573 insertions(+), 17 deletions(-) create mode 100644 tests/docker/sudo/Dockerfile create mode 100755 tests/shell/002_sudo_password.sh diff --git a/.gitignore b/.gitignore index 8069798..4612d59 100644 --- a/.gitignore +++ b/.gitignore @@ -7,3 +7,5 @@ dist /.venv __pycache__/ **/.DS_STORE +/tests/docker/sudo/id_test +/tests/docker/sudo/id_test.pub diff --git a/README.md b/README.md index ec0de06..6667e08 100644 --- a/README.md +++ b/README.md @@ -8,6 +8,7 @@ Commands for provisioning hosts in the cloud (AWS or GCP) are also available. - cf-remote requires python 3.6 or greater. - SSH must be configured in such a way that cf-remote can login without a password. +- The account cf-remote logs in as must be root or be able to `sudo`. Passwordless sudo is not required, see [Switching user on the remote hosts](#switching-user-on-the-remote-hosts). - An sftp server for transferring files on UNIX hosts. e.g. openssh-sftp-server for debian-based distributions. ## Installation @@ -184,6 +185,39 @@ If you have more than one key in `~/.ssh` you may need to specify which key `cf- $ export CF_REMOTE_SSH_KEY="~/.ssh/id_rsa.pub" ``` +### Switching user on the remote hosts + +Most of what `cf-remote` does needs root, so unless it logs in as root it runs commands through `sudo`. +If `sudo` asks for a password, use `--ask-pass` (`-K`) and `cf-remote` prompts for it once and uses it for all the hosts in the run: + +``` +$ cf-remote --ask-pass install --clients ubuntu@10.0.0.5 +Password for switching user: +``` + +The password is written to the standard input of the `ssh` process, so it is never part of a command line and doesn't show up in the process list, in the shell history on the target host, or in the output of `--log-level DEBUG`. +It is only sent to hosts where switching user actually asks for a password. + +Where there is nobody to answer a prompt, such as in a script or a CI job, put the password on the first line of a file and point `--password-file` at it: + +``` +$ cf-remote --password-file ~/.cf-remote-password install --clients ubuntu@10.0.0.5 +``` + +`cf-remote` refuses to read the file if others can read it, the same way `ssh` refuses to use a private key with too generous permissions, so `chmod 600` it first. + +Use `--switch-user-command` if `sudo` is not what you want to switch user with: + +``` +$ cf-remote --ask-pass --switch-user-command "doas /bin/sh -c" info -H bsd-host +``` + +The command to run is appended as a single quoted argument. +The default is `sudo bash -c`, or `sudo -S -p '' bash -c` with `--ask-pass`, since `sudo` only reads the password from standard input when it is given `-S`. + +A password can only reach a command that reads it from standard input, which in practice means `sudo -S` and the tools that copy its interface, such as `dzdo -S`. +`doas` and `su` read from a terminal instead, so they work with `--switch-user-command` where they need no password, but cannot be given one by `cf-remote`. + ### Working on the local host `cf-remote` can work on the local host when the target host is `localhost`. In this case, it executes commands locally without connecting over SSH. diff --git a/cf_remote/aramid.py b/cf_remote/aramid.py index c822f48..4ce8d42 100644 --- a/cf_remote/aramid.py +++ b/cf_remote/aramid.py @@ -114,11 +114,46 @@ def _get_put_method_args(method, host, src, dst): ) +def _popen(args, stdin_input=None): + """Start a process, optionally writing 'stdin_input' to its standard input + + Anything we send this way (a password for switching user) is small enough + to fit in the pipe buffer, so writing it up front cannot block. Standard + input is left alone (inherited) when there is nothing to send. + + An empty string is not the same as `None`: it gives the command a pipe + with nothing in it, so anything waiting for input sees EOF at once rather + than blocking on the terminal 'cf-remote' was started from. + + The pipe is deliberately left open here, 'communicate()' closes it (and + with it, sends the EOF a command waiting for more input needs). + """ + proc = subprocess.Popen( + args, + stdin=(subprocess.PIPE if stdin_input is not None else None), + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + universal_newlines=True, + ) + if stdin_input: + assert proc.stdin is not None + try: + proc.stdin.write(stdin_input) + proc.stdin.flush() + except BrokenPipeError: + # The process is already gone, its exit code tells the story + pass + return proc + + class _Task: - def __init__(self, host, proc, action=None, retries=0): # TODO: timeout=60 + def __init__( + self, host, proc, action=None, retries=0, stdin_input=None + ): # TODO: timeout=60 self.host = host self.proc = proc self.action = action + self.stdin_input = stdin_input self._max_retries = retries self._retries = retries self.stdout = "" @@ -143,12 +178,7 @@ def communicate(self, timeout=1, ignore_failed=False): if self._retries > 0: # wait for the rest of timeout (if any) and restart the process time.sleep(max(timeout - (time.time() - start), 0)) - self.proc = subprocess.Popen( - self.proc.args, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - universal_newlines=True, - ) + self.proc = _popen(self.proc.args, self.stdin_input) self._retries -= 1 return False else: @@ -305,6 +335,7 @@ def execute( ignore_failed=False, echo=True, echo_cmd=False, + stdin_input=None, ): # TODO: parallel=False """Execute command on remote hosts (in parallel) @@ -321,6 +352,9 @@ def execute( :param bool echo: whether to echo the output (STDOUT first followed by STDERR) of the given commands :param bool echo_cmd: whether to echo the commands run on the hosts + :param str stdin_input: data to write to the standard input of the commands, + for example a password for switching user. If `None`, + standard input is inherited from `cf-remote` itself. :return: results of commands executed on the given hosts :rtype: dict(:class:`Host` -> list(:class:`ExecutionResult`)) @@ -340,18 +374,16 @@ def execute( port_args = [] if host.port != _DEFAULT_SSH_PORT: port_args += ["-p", str(host.port)] - proc = subprocess.Popen( + proc = _popen( ["ssh"] + DEFAULT_SSH_ARGS + port_args + host.extra_ssh_args + [host.login] + [commands[i]], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - universal_newlines=True, + stdin_input=stdin_input, ) - task = _Task(host, proc, commands[i], retries=retries) + task = _Task(host, proc, commands[i], retries=retries, stdin_input=stdin_input) host.tasks.append(task) tasks.append(task) diff --git a/cf_remote/args.py b/cf_remote/args.py index da480eb..fafeb78 100644 --- a/cf_remote/args.py +++ b/cf_remote/args.py @@ -2,6 +2,7 @@ import os import sys +from cf_remote import ssh from cf_remote.utils import cache @@ -279,6 +280,35 @@ def add_connect_args(sp: argparse.ArgumentParser) -> None: @cache +def add_switch_user_args(ap: argparse.ArgumentParser) -> None: + password_source = ap.add_mutually_exclusive_group() + password_source.add_argument( + "--ask-pass", + "-K", + help="Prompt for the password used to switch user on the remote hosts." + + " The password is asked for once and used for all hosts", + action="store_true", + ) + password_source.add_argument( + "--password-file", + help="Read the password used to switch user from the first line of a" + + " file, for use where there is nobody to answer --ask-pass." + + " The file must not be readable by others", + type=str, + ) + ap.add_argument( + "--switch-user-command", + help="Command used to run commands as another (privileged) user." + + " The command to run is appended as a single quoted argument." + + " Defaults to '%s', or '%s' when --ask-pass is used" + % ( + ssh.DEFAULT_SWITCH_USER_COMMAND, + ssh.DEFAULT_SWITCH_USER_COMMAND_WITH_PASSWORD, + ), + type=str, + ) + + def get_arg_parser() -> argparse.ArgumentParser: ap = argparse.ArgumentParser( description="Spooky CFEngine at a distance", @@ -299,6 +329,7 @@ def get_arg_parser() -> argparse.ArgumentParser: type=str, const=True, ) + add_switch_user_args(ap) command_help_hint = ( "Commands (use %s COMMAND --help to get more info)" diff --git a/cf_remote/main.py b/cf_remote/main.py index 403f92a..c77800c 100644 --- a/cf_remote/main.py +++ b/cf_remote/main.py @@ -1,9 +1,11 @@ +import getpass import os import sys import re import socket from cf_remote import log +from cf_remote import ssh from cf_remote import version from cf_remote import commands, paths from cf_remote.args import get_arg_parser @@ -348,6 +350,13 @@ def _main() -> int: log.set_level(args.log_level) validate_args(args) + if args.switch_user_command: + ssh.set_switch_user_command(args.switch_user_command) + if args.ask_pass: + ssh.set_switch_user_password(getpass.getpass("Password for switching user: ")) + elif args.password_file: + ssh.set_switch_user_password(ssh.read_switch_user_password(args.password_file)) + exit_code = run_command_with_args(args.command, args) assert type(exit_code) is int return exit_code diff --git a/cf_remote/ssh.py b/cf_remote/ssh.py index 6238527..0ff8d62 100644 --- a/cf_remote/ssh.py +++ b/cf_remote/ssh.py @@ -41,6 +41,95 @@ def _check_reachable( ) +DEFAULT_SWITCH_USER_COMMAND = "sudo bash -c" +"""Command used to run commands as another (privileged) user""" + +DEFAULT_SWITCH_USER_COMMAND_WITH_PASSWORD = "sudo -S -p '' bash -c" +"""Same, but reading the password from standard input instead of a terminal""" + +_switch_user_command = None +_switch_user_password = None + + +def set_switch_user_command(command): + global _switch_user_command + _switch_user_command = command + + +def set_switch_user_password(password): + global _switch_user_password + _switch_user_password = password + + +def read_switch_user_password(path): + """Read the password for switching user from the first line of a file + + Refuses to read a file others can read, the same way ssh refuses to use a + private key with too generous permissions. + """ + path = os.path.expanduser(path) + if not os.path.isfile(path): + raise CFRUserError("Password file '%s' does not exist" % path) + + if os.name == "posix" and (os.stat(path).st_mode & 0o077): + raise CFRUserError( + "Password file '%s' is readable by others, run" + " 'chmod 600 %s' before using it" % (path, path) + ) + + try: + with open(path, "r") as f: + line = f.readline() + except OSError as e: + raise CFRUserError("Cannot read password file '%s': %s" % (path, e)) + + # Only the newline the editor added, a password may well end in a space + return line.rstrip("\r\n") + + +def get_switch_user_password(): + return _switch_user_password + + +def get_switch_user_command(): + if _switch_user_command is not None: + return _switch_user_command + if _switch_user_password is not None: + return DEFAULT_SWITCH_USER_COMMAND_WITH_PASSWORD + return DEFAULT_SWITCH_USER_COMMAND + + +def switch_user(cmd): + """Wrap 'cmd' so that it runs as another (privileged) user""" + return "%s '%s'" % (get_switch_user_command(), cmd) + + +def _switch_user_needs_password(connection): + """Check whether switching user on this host requires a password + + Only interesting when we actually have a password to send. Sending it + when it isn't needed would leave it on the standard input of the command + we are running instead. + + 'sudo -n' answers this without ever attempting to authenticate. Asking by + letting an attempt fail instead would count towards the failed attempts + that pam_faillock locks accounts out over, once per host and run. + """ + if get_switch_user_password() is None: + return False + + if _switch_user_command is not None: + # A command we didn't pick has no 'sudo -n' to ask with, so run it + # with nothing on standard input: one that wants a password fails + # right away rather than taking ours. Assuming it wants one instead + # would hand the password to whatever runs when it doesn't. + return ( + connection.run(switch_user("true"), hide=True, stdin_input="").retcode != 0 + ) + + return connection.run("sudo -n true", hide=True).retcode != 0 + + class LocalConnection: is_local = True ssh_user = None @@ -49,13 +138,17 @@ class LocalConnection: def __init__(self): self.ssh_user = pwd.getpwuid(os.getuid()).pw_name self.needs_sudo = self.run("echo $UID", hide=True).stdout.strip() != "0" + self.switch_user_needs_password = ( + self.needs_sudo and _switch_user_needs_password(self) + ) - def run(self, command, hide=False): + def run(self, command, hide=False, stdin_input=None): # to maintain Python 3.5/3.6 compatability the following are used: # stdout=PIPE, stderr=STDOUT instead of capture_output=True # universal_newlines=True instead of text=True result = subprocess.run( command, + input=stdin_input, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, shell=True, @@ -113,6 +206,9 @@ def __init__(self, host, user, connect_kwargs=None, port=aramid._DEFAULT_SSH_POR ) self.needs_sudo = self.run("echo $UID", hide=True).stdout.strip() != "0" + self.switch_user_needs_password = ( + self.needs_sudo and _switch_user_needs_password(self) + ) log.debug("Connection initialized") def __del__(self): @@ -123,7 +219,7 @@ def __del__(self): ): self._ssh_control_master.send_signal(signal.SIGTERM) - def run(self, command, hide=False): + def run(self, command, hide=False, stdin_input=None): extra_ssh_args = [] if self._connect_kwargs and "key_filename" in self._connect_kwargs: extra_ssh_args.extend(["-i", self._connect_kwargs["key_filename"]]) @@ -138,7 +234,9 @@ def run(self, command, hide=False): extra_ssh_args.extend(["-oControlPath=%s" % self._control_path]) ahost = aramid.Host(self.ssh_host, self.ssh_user, self.ssh_port, extra_ssh_args) - results = aramid.execute([ahost], command, echo=(not hide)) + results = aramid.execute( + [ahost], command, echo=(not hide), stdin_input=stdin_input + ) return results[ahost][0] def put(self, src, hide=False): @@ -308,16 +406,47 @@ def ssh_cmd(connection, cmd, errors=False, needs_pty=True) -> Union[str, None]: return None +def _switch_user_hint(connection, result): + """Explain a switch user failure caused by the password, if that's what it is""" + output = (result.stdout or "") + (result.stderr or "") + output = output.lower() + + if "try again" in output or "incorrect password" in output: + return "Password for switching user was rejected on '%s'" % connection.ssh_host + + needs_password = ( + "a terminal is required" in output + or "a password is required" in output + or "no tty present" in output + ) + if needs_password: + if get_switch_user_password() is None: + return ( + "Switching user requires a password on '%s'," + " rerun with --ask-pass to be prompted for it" % connection.ssh_host + ) + return ( + "Switching user asked for a password on '%s' after reporting that" + " it didn't need one, so none was sent" % connection.ssh_host + ) + + return None + + def ssh_sudo(connection, cmd, errors=False, needs_pty=False): assert connection + stdin_input = None if connection.needs_sudo: - cmd = "sudo bash -c '%s'" % cmd + cmd = switch_user(cmd) + password = get_switch_user_password() + if connection.switch_user_needs_password and password is not None: + stdin_input = password + "\n" if needs_pty: cmd = 'script -qec "%s" /dev/null' % cmd - result = connection.run(cmd, hide=True) + result = connection.run(cmd, hide=True, stdin_input=stdin_input) if result.retcode == 0: output = result.stdout.strip("\n") @@ -325,6 +454,9 @@ def ssh_sudo(connection, cmd, errors=False, needs_pty=False): return output else: msg = "Sudo command unexpectedly exited: '%s' [%d]" % (cmd, result.retcode) + hint = _switch_user_hint(connection, result) + if hint: + log.error(hint) if errors: print(result.stdout if result.stdout is not None else "") print(result.stderr if result.stderr is not None else "") diff --git a/tests/docker/sudo/Dockerfile b/tests/docker/sudo/Dockerfile new file mode 100644 index 0000000..908285b --- /dev/null +++ b/tests/docker/sudo/Dockerfile @@ -0,0 +1,26 @@ +FROM debian:12-slim + +# openssh-server depends on dbus and systemd which is a lot, so use dropbear instead +RUN apt update -y && apt install -y dropbear openssh-sftp-server sudo + +# 'cftest' may sudo, but has to type a password to do it +RUN useradd -m -s /bin/bash cftest && echo 'cftest:cftestpw' | chpasswd +RUN echo 'cftest ALL=(ALL:ALL) ALL' > /etc/sudoers.d/cftest && chmod 440 /etc/sudoers.d/cftest + +# 'cfnopass' may sudo without one, to check that we don't send a password +# where none is wanted (it would end up on the standard input of the command +# we are running instead) +RUN useradd -m -s /bin/bash cfnopass +RUN echo 'cfnopass ALL=(ALL:ALL) NOPASSWD: ALL' > /etc/sudoers.d/cfnopass \ + && chmod 440 /etc/sudoers.d/cfnopass + +# Log in over SSH with a key, so that the account password is only ever used +# for sudo, which is what we are testing +COPY id_test.pub /tmp/id_test.pub +RUN for u in cftest cfnopass; do \ + install -d -m 700 -o $u -g $u /home/$u/.ssh && \ + install -m 600 -o $u -g $u /tmp/id_test.pub /home/$u/.ssh/authorized_keys; \ + done && rm /tmp/id_test.pub + +# run dropbear sshd in foreground (-F), log to stderr (-E), create host keys (-R) +CMD [ "dropbear", "-F", "-E", "-R" ] diff --git a/tests/shell/002_sudo_password.sh b/tests/shell/002_sudo_password.sh new file mode 100755 index 0000000..82ce898 --- /dev/null +++ b/tests/shell/002_sudo_password.sh @@ -0,0 +1,125 @@ +#!/usr/bin/env bash +# Check that cf-remote can switch user on hosts where sudo asks for a password, +# which is the normal case in environments that don't hand out NOPASSWD sudo. +set -ex +set -o pipefail + +if ! docker info >/dev/null 2>&1; then + echo "--- SKIP: docker is not available" + exit 0 +fi + +# The build context lives with the other docker fixtures +dir=$(dirname "$0")/../docker/sudo +name=cf-remote-sudo-password-test-host +port=8823 +password=cftestpw +pwfile=$(mktemp) + +# Leave nothing behind, including on Ctrl+C: a container holding the port, a +# throwaway SSH key, a password file and an ssh-agent +cleanup () { + ssh-agent -k >/dev/null 2>&1 || true + docker rm -f "$name" >/dev/null 2>&1 || true + rm -f "$dir/id_test" "$dir/id_test.pub" "$pwfile" +} +trap cleanup EXIT INT TERM + +out="" +# Run cf-remote with a password on stdin, keeping the output in $out. Some of +# the cases below are expected to fail, so the exit code is not what we assert +# on, the output is. +run_cfr () { + local pw="$1"; shift + set +e + out=$(printf '%s\n' "$pw" | "$@" 2>&1) + set -e + printf '%s\n' "$out" +} + +assert_output () { # assert_output + if printf '%s\n' "$out" | grep -q "$@"; then + echo "ok: output matched '$*'" + else + echo "FAIL: output did not match '$*'" + exit 1 + fi +} + +# SSH logs in with a key, so the account password is only used by sudo +rm -f "$dir/id_test" "$dir/id_test.pub" +ssh-keygen -t ed25519 -N "" -f "$dir/id_test" -q +docker build -t "$name" "$dir" +docker run -d -p "$port":22 --name "$name" "$name" + +# scp doesn't take the key from CF_REMOTE_SSH_KEY, so use an agent +eval "$(ssh-agent -s)" +ssh-add "$dir/id_test" + +# The port is published, so the host is reachable on the loopback address. +# 'hostname -i' can answer with several addresses, which would not be. +host=127.0.0.1 +ready=no +for _ in $(seq 30); do + if ssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + -p "$port" cftest@"$host" true; then + ready=yes + break + fi + sleep 1 +done +if [ "$ready" != yes ]; then + echo "FAIL: '$name' never accepted an SSH connection on port $port" + exit 1 +fi + +echo "=== without --ask-pass: should say what is wrong ===" +run_cfr "" cf-remote sudo -H cftest@"$host":"$port" 'id -un' +assert_output -- "--ask-pass" + +echo "=== with --ask-pass: should run as root ===" +run_cfr "$password" cf-remote --ask-pass sudo -H cftest@"$host":"$port" 'id -un' +assert_output "root" + +echo "=== with a wrong password: should say it was rejected ===" +run_cfr "definitely-not-the-password" \ + cf-remote --ask-pass sudo -H cftest@"$host":"$port" 'id -un' +assert_output -i "rejected" + +echo "=== NOPASSWD host: the password must not reach the command ===" +run_cfr "$password" cf-remote --ask-pass sudo -H cfnopass@"$host":"$port" 'cat' +if printf '%s\n' "$out" | grep -q "$password"; then + echo "FAIL: password ended up on the standard input of the command" + exit 1 +fi +echo "ok: no password sent where none was needed" + +echo "=== --switch-user-command is used as given ===" +run_cfr "$password" cf-remote --ask-pass \ + --switch-user-command "sudo -S -p '' /bin/sh -c" \ + sudo -H cftest@"$host":"$port" 'readlink /proc/$$/exe' +assert_output "sh" + +echo "=== a command carrying a quote of its own survives ===" +quoted_payload='echo "it'"'"'s fine"' +run_cfr "$password" cf-remote --ask-pass sudo -H cftest@"$host":"$port" "$quoted_payload" +assert_output "it's fine" + +echo "=== --password-file needs nobody to answer a prompt ===" +chmod 600 "$pwfile" +printf '%s\n' "$password" > "$pwfile" +run_cfr "" cf-remote --password-file "$pwfile" sudo -H cftest@"$host":"$port" 'id -un' +assert_output "root" + +echo "=== a password file others can read is refused ===" +chmod 644 "$pwfile" +run_cfr "" cf-remote --password-file "$pwfile" sudo -H cftest@"$host":"$port" 'id -un' +assert_output "readable by others" + +echo "=== install with a sudo password ===" +chmod 600 "$pwfile" +run_cfr "$password" cf-remote --ask-pass install --edition community \ + --clients cftest@"$host":"$port" +assert_output "successfully installed" + +echo "=== all sudo password tests passed ===" diff --git a/tests/shell/all.sh b/tests/shell/all.sh index 3202762..c714503 100644 --- a/tests/shell/all.sh +++ b/tests/shell/all.sh @@ -44,6 +44,7 @@ run_test() { } run_test tests/shell/001_migrate_dirs.sh +run_test tests/shell/002_sudo_password.sh # Summary _suite_end=$(date +%s) diff --git a/tests/test_ssh.py b/tests/test_ssh.py index d9eef27..120f059 100644 --- a/tests/test_ssh.py +++ b/tests/test_ssh.py @@ -1,4 +1,9 @@ +import pytest + +from cf_remote import ssh +from cf_remote.aramid import ExecutionResult from cf_remote.ssh import auto_connect, ssh_cmd +from cf_remote.utils import CFRUserError # for debugging, uncomment the following two lines # from cf_remote import log @@ -21,3 +26,162 @@ def test_ssh_localhost(): def test_failed_command(): nope("localhost") + + +@pytest.fixture(autouse=True) +def reset_switch_user(): + """Keep the switch user settings from leaking between tests""" + yield + ssh.set_switch_user_command(None) + ssh.set_switch_user_password(None) + + +def test_switch_user_default(): + assert ssh.switch_user("cf-agent -K") == "sudo bash -c 'cf-agent -K'" + + +def test_switch_user_with_password(): + # With a password to send, sudo has to read it from standard input + ssh.set_switch_user_password("hunter2") + assert ssh.switch_user("cf-agent -K") == "sudo -S -p '' bash -c 'cf-agent -K'" + + +def test_switch_user_command_overrides_default(): + ssh.set_switch_user_command("doas -n /bin/sh -c") + assert ssh.switch_user("cf-agent -K") == "doas -n /bin/sh -c 'cf-agent -K'" + + # ... also when a password is given, then it's up to the user to make the + # command read it from standard input + ssh.set_switch_user_password("hunter2") + assert ssh.switch_user("cf-agent -K") == "doas -n /bin/sh -c 'cf-agent -K'" + + +def _password_file(tmp_path, content, mode=0o600): + path = tmp_path / "password" + path.write_text(content) + path.chmod(mode) + return str(path) + + +def test_password_file_drops_the_trailing_newline(tmp_path): + path = _password_file(tmp_path, "hunter2\n") + assert ssh.read_switch_user_password(path) == "hunter2" + + +def test_password_file_keeps_the_rest_of_the_line(tmp_path): + # A password may well end in a space, only the editor's newline goes + path = _password_file(tmp_path, "hunter2 \r\nignored second line\n") + assert ssh.read_switch_user_password(path) == "hunter2 " + + +def test_password_file_readable_by_others_is_refused(tmp_path): + path = _password_file(tmp_path, "hunter2\n", mode=0o644) + with pytest.raises(CFRUserError, match="readable by others"): + ssh.read_switch_user_password(path) + + +def test_missing_password_file_is_refused(tmp_path): + with pytest.raises(CFRUserError, match="does not exist"): + ssh.read_switch_user_password(str(tmp_path / "nope")) + + +class FakeConnection: + ssh_host = "somehost" + needs_sudo = False + switch_user_needs_password = False + + def __init__(self, retcode=0): + self.retcode = retcode + self.commands = [] + + def run(self, command, hide=False, stdin_input=None): + self.commands.append((command, stdin_input)) + return ExecutionResult(command, self.retcode, "", "") + + +def test_no_password_means_no_asking(): + connection = FakeConnection() + assert ssh._switch_user_needs_password(connection) is False + assert connection.commands == [] + + +def test_asking_never_attempts_authentication(): + # A failed attempt is what pam_faillock counts, so this must not make one + ssh.set_switch_user_password("hunter2") + + connection = FakeConnection(retcode=1) + assert ssh._switch_user_needs_password(connection) is True + assert connection.commands == [("sudo -n true", None)] + + connection = FakeConnection(retcode=0) + assert ssh._switch_user_needs_password(connection) is False + assert connection.commands == [("sudo -n true", None)] + + +def test_password_goes_on_standard_input(): + ssh.set_switch_user_password("hunter2") + connection = FakeConnection() + connection.needs_sudo = True + connection.switch_user_needs_password = True + + ssh.ssh_sudo(connection, "id -un") + assert connection.commands == [("sudo -S -p '' bash -c 'id -un'", "hunter2\n")] + + +def test_password_is_withheld_where_it_isnt_needed(): + # Otherwise it ends up on the standard input of the command instead + ssh.set_switch_user_password("hunter2") + connection = FakeConnection() + connection.needs_sudo = True + connection.switch_user_needs_password = False + + ssh.ssh_sudo(connection, "id -un") + assert connection.commands == [("sudo -S -p '' bash -c 'id -un'", None)] + + +def test_own_switch_user_command_is_asked_with_empty_input(): + # Assuming it wants a password would hand the password to whatever runs + # when it doesn't, so ask, with nothing it could mistake for one + ssh.set_switch_user_password("hunter2") + ssh.set_switch_user_command("doas /bin/sh -c") + + connection = FakeConnection(retcode=0) + assert ssh._switch_user_needs_password(connection) is False + assert connection.commands == [("doas /bin/sh -c 'true'", "")] + + connection = FakeConnection(retcode=1) + assert ssh._switch_user_needs_password(connection) is True + + +def _failure(stderr): + return ExecutionResult("some command", 1, "", stderr) + + +def test_switch_user_hint_suggests_ask_pass(): + result = _failure("sudo: a terminal is required to read the password") + hint = ssh._switch_user_hint(FakeConnection(), result) + assert hint is not None + assert "--ask-pass" in hint + assert "somehost" in hint + + +def test_switch_user_hint_reports_rejected_password(): + ssh.set_switch_user_password("hunter2") + result = _failure("Sorry, try again.\nsudo: 1 incorrect password attempt") + hint = ssh._switch_user_hint(FakeConnection(), result) + assert hint is not None + assert "rejected" in hint + + +def test_switch_user_hint_reports_password_that_was_never_sent(): + # Asking said no password was needed, but the command disagreed + ssh.set_switch_user_password("hunter2") + result = _failure("sudo: a password is required") + hint = ssh._switch_user_hint(FakeConnection(), result) + assert hint is not None + assert "none was sent" in hint + + +def test_switch_user_hint_ignores_unrelated_failures(): + result = _failure("dpkg: error processing archive") + assert ssh._switch_user_hint(FakeConnection(), result) is None From 8f998eca43e4bfa4d96bc973abc357be86dfb618 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 14 Aug 2026 10:57:10 -0500 Subject: [PATCH 2/7] Quoted the command switching user instead of wrapping it in quotes Ticket: ENT-14418 Changelog: title Signed-off-by: Nick Anderson Co-Authored-By: Claude Opus 5 (1M context) --- cf_remote/ssh.py | 14 ++++++++++---- tests/test_ssh.py | 11 ++++++++++- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/cf_remote/ssh.py b/cf_remote/ssh.py index 0ff8d62..762ffd1 100644 --- a/cf_remote/ssh.py +++ b/cf_remote/ssh.py @@ -1,5 +1,6 @@ import os import pwd +import shlex import shutil import signal import socket @@ -100,8 +101,13 @@ def get_switch_user_command(): def switch_user(cmd): - """Wrap 'cmd' so that it runs as another (privileged) user""" - return "%s '%s'" % (get_switch_user_command(), cmd) + """Wrap 'cmd' so that it runs as another (privileged) user + + 'cmd' is quoted rather than wrapped in quotes: a command containing a + quote of its own would otherwise end the wrapping early and the remote + shell would run something else, or nothing at all. + """ + return "%s %s" % (get_switch_user_command(), shlex.quote(cmd)) def _switch_user_needs_password(connection): @@ -386,7 +392,7 @@ def ssh_cmd(connection, cmd, errors=False, needs_pty=True) -> Union[str, None]: assert connection if needs_pty: - cmd = 'script -qec "%s" /dev/null' % cmd + cmd = "script -qec %s /dev/null" % shlex.quote(cmd) result = connection.run(cmd, hide=True) if result.retcode == 0: @@ -444,7 +450,7 @@ def ssh_sudo(connection, cmd, errors=False, needs_pty=False): stdin_input = password + "\n" if needs_pty: - cmd = 'script -qec "%s" /dev/null' % cmd + cmd = "script -qec %s /dev/null" % shlex.quote(cmd) result = connection.run(cmd, hide=True, stdin_input=stdin_input) diff --git a/tests/test_ssh.py b/tests/test_ssh.py index 120f059..631d3c1 100644 --- a/tests/test_ssh.py +++ b/tests/test_ssh.py @@ -1,3 +1,5 @@ +import shlex + import pytest from cf_remote import ssh @@ -56,6 +58,13 @@ def test_switch_user_command_overrides_default(): assert ssh.switch_user("cf-agent -K") == "doas -n /bin/sh -c 'cf-agent -K'" +def test_switch_user_survives_quotes_in_the_command(): + # A command carrying quotes of its own must not end the wrapping early. + # Splitting it back the way a shell would proves it arrives in one piece. + for cmd in ("echo it's fine", 'echo "double"', "echo 'mixed \"quotes\"'"): + assert shlex.split(ssh.switch_user(cmd))[-1] == cmd + + def _password_file(tmp_path, content, mode=0o600): path = tmp_path / "password" path.write_text(content) @@ -147,7 +156,7 @@ def test_own_switch_user_command_is_asked_with_empty_input(): connection = FakeConnection(retcode=0) assert ssh._switch_user_needs_password(connection) is False - assert connection.commands == [("doas /bin/sh -c 'true'", "")] + assert connection.commands == [("doas /bin/sh -c true", "")] connection = FakeConnection(retcode=1) assert ssh._switch_user_needs_password(connection) is True From ccb872d6f1c244e1e766d8981fab92838c818e98 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 14 Aug 2026 11:12:19 -0500 Subject: [PATCH 3/7] Kept the parser memoized when adding the switch user options Ticket: ENT-14418 Changelog: none Signed-off-by: Nick Anderson Co-Authored-By: Claude Opus 5 (1M context) --- cf_remote/args.py | 2 +- tests/test_args.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 tests/test_args.py diff --git a/cf_remote/args.py b/cf_remote/args.py index fafeb78..30321f5 100644 --- a/cf_remote/args.py +++ b/cf_remote/args.py @@ -279,7 +279,6 @@ def add_connect_args(sp: argparse.ArgumentParser) -> None: ) -@cache def add_switch_user_args(ap: argparse.ArgumentParser) -> None: password_source = ap.add_mutually_exclusive_group() password_source.add_argument( @@ -309,6 +308,7 @@ def add_switch_user_args(ap: argparse.ArgumentParser) -> None: ) +@cache def get_arg_parser() -> argparse.ArgumentParser: ap = argparse.ArgumentParser( description="Spooky CFEngine at a distance", diff --git a/tests/test_args.py b/tests/test_args.py new file mode 100644 index 0000000..d3fc26a --- /dev/null +++ b/tests/test_args.py @@ -0,0 +1,31 @@ +import argparse + +from cf_remote.args import add_switch_user_args, get_arg_parser + + +def test_switch_user_args_are_added_to_every_parser(): + # Memoizing this would key on parsers that look alike, so a second one + # would quietly get none of these and reading them would raise + for _ in range(2): + ap = argparse.ArgumentParser(description="Spooky CFEngine at a distance") + add_switch_user_args(ap) + args = ap.parse_args([]) + assert args.ask_pass is False + assert args.password_file is None + assert args.switch_user_command is None + + +def test_switch_user_options_reach_the_real_parser(): + args = get_arg_parser().parse_args(["info", "-H", "somehost"]) + assert args.ask_pass is False + assert args.password_file is None + assert args.switch_user_command is None + + +def test_password_sources_are_mutually_exclusive(capsys): + ap = get_arg_parser() + try: + ap.parse_args(["--ask-pass", "--password-file", "/tmp/x", "info", "-H", "h"]) + except SystemExit: + pass + assert "not allowed with argument" in capsys.readouterr().err From 1d46e96c1d6464a505eee08962635b66586e484e Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Fri, 14 Aug 2026 11:15:13 -0500 Subject: [PATCH 4/7] Let one function decide whether a password is needed Ticket: ENT-14418 Changelog: none Signed-off-by: Nick Anderson Co-Authored-By: Claude Opus 5 (1M context) --- cf_remote/ssh.py | 11 +++++------ tests/test_ssh.py | 14 +++++++++++--- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/cf_remote/ssh.py b/cf_remote/ssh.py index 762ffd1..cb111a3 100644 --- a/cf_remote/ssh.py +++ b/cf_remote/ssh.py @@ -121,6 +121,9 @@ def _switch_user_needs_password(connection): letting an attempt fail instead would count towards the failed attempts that pam_faillock locks accounts out over, once per host and run. """ + if not connection.needs_sudo: + return False + if get_switch_user_password() is None: return False @@ -144,9 +147,7 @@ class LocalConnection: def __init__(self): self.ssh_user = pwd.getpwuid(os.getuid()).pw_name self.needs_sudo = self.run("echo $UID", hide=True).stdout.strip() != "0" - self.switch_user_needs_password = ( - self.needs_sudo and _switch_user_needs_password(self) - ) + self.switch_user_needs_password = _switch_user_needs_password(self) def run(self, command, hide=False, stdin_input=None): # to maintain Python 3.5/3.6 compatability the following are used: @@ -212,9 +213,7 @@ def __init__(self, host, user, connect_kwargs=None, port=aramid._DEFAULT_SSH_POR ) self.needs_sudo = self.run("echo $UID", hide=True).stdout.strip() != "0" - self.switch_user_needs_password = ( - self.needs_sudo and _switch_user_needs_password(self) - ) + self.switch_user_needs_password = _switch_user_needs_password(self) log.debug("Connection initialized") def __del__(self): diff --git a/tests/test_ssh.py b/tests/test_ssh.py index 631d3c1..99fe35b 100644 --- a/tests/test_ssh.py +++ b/tests/test_ssh.py @@ -96,7 +96,7 @@ def test_missing_password_file_is_refused(tmp_path): class FakeConnection: ssh_host = "somehost" - needs_sudo = False + needs_sudo = True switch_user_needs_password = False def __init__(self, retcode=0): @@ -114,6 +114,16 @@ def test_no_password_means_no_asking(): assert connection.commands == [] +def test_root_is_never_asked(): + # Nothing to switch to, so no round trip and nothing to send + ssh.set_switch_user_password("hunter2") + connection = FakeConnection() + connection.needs_sudo = False + + assert ssh._switch_user_needs_password(connection) is False + assert connection.commands == [] + + def test_asking_never_attempts_authentication(): # A failed attempt is what pam_faillock counts, so this must not make one ssh.set_switch_user_password("hunter2") @@ -130,7 +140,6 @@ def test_asking_never_attempts_authentication(): def test_password_goes_on_standard_input(): ssh.set_switch_user_password("hunter2") connection = FakeConnection() - connection.needs_sudo = True connection.switch_user_needs_password = True ssh.ssh_sudo(connection, "id -un") @@ -141,7 +150,6 @@ def test_password_is_withheld_where_it_isnt_needed(): # Otherwise it ends up on the standard input of the command instead ssh.set_switch_user_password("hunter2") connection = FakeConnection() - connection.needs_sudo = True connection.switch_user_needs_password = False ssh.ssh_sudo(connection, "id -un") From 4721a76c2900027125e19f2de1005e875a1f0623 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Tue, 18 Aug 2026 12:30:01 -0500 Subject: [PATCH 5/7] Passed how to switch user as a parameter instead of keeping it in globals Ticket: ENT-14418 Changelog: none Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Nick Anderson --- cf_remote/commands.py | 47 ++++++----- cf_remote/main.py | 51 ++++++++---- cf_remote/remote.py | 17 ++-- cf_remote/ssh.py | 176 +++++++++++++++++++++++++----------------- tests/test_ssh.py | 82 ++++++++++---------- 5 files changed, 220 insertions(+), 153 deletions(-) diff --git a/cf_remote/commands.py b/cf_remote/commands.py index 53ecbc7..5e7347b 100644 --- a/cf_remote/commands.py +++ b/cf_remote/commands.py @@ -53,12 +53,12 @@ from cf_remote import cloud_data -def info(hosts, users=None): +def info(hosts, users=None, switch_user=None): assert hosts log.debug("hosts='{}'".format(hosts)) errors = 0 for host in hosts: - data = get_info(host, users=users) + data = get_info(host, users=users, switch_user=switch_user) if data: print_info(data) else: @@ -66,11 +66,17 @@ def info(hosts, users=None): return errors -def run(hosts, command, users=None, sudo=False, raw=False): +def run(hosts, command, users=None, sudo=False, raw=False, switch_user=None): assert hosts errors = 0 for host in hosts: - lines = run_command(host=host, command=command, users=users, sudo=sudo) + lines = run_command( + host=host, + command=command, + users=users, + sudo=sudo, + switch_user=switch_user, + ) if lines is None: log.error("Command: '{}'\nFailed on host: '{}'".format(command, host)) errors += 1 @@ -96,15 +102,15 @@ def run(hosts, command, users=None, sudo=False, raw=False): return errors -def sudo(hosts, command, users=None, raw=False): - return run(hosts, command, users, sudo=True, raw=raw) +def sudo(hosts, command, users=None, raw=False, switch_user=None): + return run(hosts, command, users, sudo=True, raw=raw, switch_user=switch_user) -def scp(hosts, files, users=None): +def scp(hosts, files, users=None, switch_user=None): errors = 0 for host in hosts: for file in files: - errors += transfer_file(host, file, users) + errors += transfer_file(host, file, users, switch_user=switch_user) return errors @@ -186,7 +192,8 @@ def install( edition=None, remote_download=False, trust_keys=None, - insecure=False + insecure=False, + switch_user=None ): assert hubs or clients assert not (hubs and clients and package) @@ -258,6 +265,7 @@ def install( insecure=insecure, demo_salt=salt, demo_sha=sha, + switch_user=switch_user, ) ) @@ -294,6 +302,7 @@ def install( show_info=show_host_info, remote_download=remote_download, trust_keys=trust_keys, + switch_user=switch_user, ) ) @@ -926,14 +935,14 @@ def show(ansible_inventory): return 0 -def uninstall(hosts, purge=False): +def uninstall(hosts, purge=False, switch_user=None): errors = 0 for host in hosts: - errors += uninstall_host(host, purge=purge) + errors += uninstall_host(host, purge=purge, switch_user=switch_user) return errors -def deploy_tarball(hubs, tarball): +def deploy_tarball(hubs, tarball, switch_user=None): assert os.path.isfile(tarball) if not tarball.endswith((".tgz", ".tar.gz")): @@ -944,7 +953,7 @@ def deploy_tarball(hubs, tarball): errors = 0 for hub in hubs: - errors += deploy_masterfiles(hub, tarball) + errors += deploy_masterfiles(hub, tarball, switch_user=switch_user) return errors @@ -965,7 +974,7 @@ def _get_hubs(): return hubs -def deploy(hubs, masterfiles): +def deploy(hubs, masterfiles, switch_user=None): if not hubs: hubs = _get_hubs() if hubs: @@ -1006,7 +1015,7 @@ def deploy(hubs, masterfiles): masterfiles = masterfiles.rstrip("/") if os.path.isfile(masterfiles): - return deploy_tarball(hubs, masterfiles) + return deploy_tarball(hubs, masterfiles, switch_user=switch_user) if masterfiles.endswith((".tgz", ".tar.gz")): if not os.path.exists(masterfiles): @@ -1054,10 +1063,10 @@ def deploy(hubs, masterfiles): above = directory[0 : -len("/masterfiles")] os.system("rm -rf %s" % tarball) os.system("tar -czf %s -C %s masterfiles" % (tarball, above)) - return deploy_tarball(hubs, tarball) + return deploy_tarball(hubs, tarball, switch_user=switch_user) -def agent(hosts, bootstrap=None): +def agent(hosts, bootstrap=None, switch_user=None): if bootstrap and len(bootstrap) > 1: raise CFRExitError( "Cannot boostrap {} to {}. Cannot bootstrap to more than one host.".format( @@ -1066,7 +1075,7 @@ def agent(hosts, bootstrap=None): ) for host in hosts: - data = get_info(host) + data = get_info(host, switch_user=switch_user) if not data["agent"]: raise CFRExitError("CFEngine not installed on {}".format(host)) @@ -1078,7 +1087,7 @@ def agent(hosts, bootstrap=None): command = " ".join(args) - output = run_command(host, command, sudo=True) + output = run_command(host, command, sudo=True, switch_user=switch_user) if output: print(output) diff --git a/cf_remote/main.py b/cf_remote/main.py index c77800c..0440026 100644 --- a/cf_remote/main.py +++ b/cf_remote/main.py @@ -44,9 +44,24 @@ def get_args(): return args -def run_command_with_args(command, args) -> int: +def _switch_user_from_args(args) -> ssh.SwitchUser: + """Build how to switch user on the remote hosts from the command line options + + Asks for the password here, once, rather than per host: the commands below + get a finished object to hand to the connections they make. + """ + password = None + if args.ask_pass: + password = getpass.getpass("Password for switching user: ") + elif args.password_file: + password = ssh.read_switch_user_password(args.password_file) + + return ssh.SwitchUser(command=args.switch_user_command, password=password) + + +def run_command_with_args(command, args, switch_user=None) -> int: if command == "info": - return commands.info(args.hosts, None) + return commands.info(args.hosts, None, switch_user=switch_user) elif command == "install": if args.trust_keys: trust_keys = args.trust_keys.split(",") @@ -67,10 +82,11 @@ def run_command_with_args(command, args) -> int: remote_download=args.remote_download, trust_keys=trust_keys, insecure=args.insecure, + switch_user=switch_user, ) elif command == "uninstall": all_hosts = (args.hosts or []) + (args.hub or []) + (args.clients or []) - return commands.uninstall(all_hosts, purge=args.purge) + return commands.uninstall(all_hosts, purge=args.purge, switch_user=switch_user) elif command == "packages": log.warning( "packages command is deprecated, please use the new command: download" @@ -97,15 +113,23 @@ def run_command_with_args(command, args) -> int: allow_expired=args.allow_expired, ) elif command == "run": - return commands.run(hosts=args.hosts, raw=args.raw, command=args.remote_command) + return commands.run( + hosts=args.hosts, + raw=args.raw, + command=args.remote_command, + switch_user=switch_user, + ) elif command == "save": return commands.save(hosts=args.hosts, role=args.role, name=args.name) elif command == "sudo": return commands.sudo( - hosts=args.hosts, raw=args.raw, command=args.remote_command + hosts=args.hosts, + raw=args.raw, + command=args.remote_command, + switch_user=switch_user, ) elif command == "scp": - return commands.scp(hosts=args.hosts, files=args.args) + return commands.scp(hosts=args.hosts, files=args.args, switch_user=switch_user) elif command == "spawn": if args.list_platforms: return commands.list_platforms() @@ -167,9 +191,9 @@ def run_command_with_args(command, args) -> int: group_name = args.name if args.name else None return commands.destroy(group_name) elif command == "deploy": - return commands.deploy(args.hub, args.masterfiles) + return commands.deploy(args.hub, args.masterfiles, switch_user=switch_user) elif command == "agent": - return commands.agent(args.hosts, args.bootstrap) + return commands.agent(args.hosts, args.bootstrap, switch_user=switch_user) elif command == "connect": return commands.connect_cmd(args.hosts) else: @@ -350,14 +374,9 @@ def _main() -> int: log.set_level(args.log_level) validate_args(args) - if args.switch_user_command: - ssh.set_switch_user_command(args.switch_user_command) - if args.ask_pass: - ssh.set_switch_user_password(getpass.getpass("Password for switching user: ")) - elif args.password_file: - ssh.set_switch_user_password(ssh.read_switch_user_password(args.password_file)) - - exit_code = run_command_with_args(args.command, args) + exit_code = run_command_with_args( + args.command, args, switch_user=_switch_user_from_args(args) + ) assert type(exit_code) is int return exit_code diff --git a/cf_remote/remote.py b/cf_remote/remote.py index 66b7a6e..c7770a5 100644 --- a/cf_remote/remote.py +++ b/cf_remote/remote.py @@ -135,15 +135,17 @@ def print_info(data): print() -def transfer_file(host, file, users=None, connection=None): +def transfer_file(host, file, users=None, connection=None, switch_user=None): assert not users or len(users) == 1 if users: host = users[0] + "@" + host - return scp(file=file, remote=host, connection=connection) + return scp(file=file, remote=host, connection=connection, switch_user=switch_user) @auto_connect -def run_command(host, command, *, users=None, connection=None, sudo=False): +def run_command( + host, command, *, users=None, connection=None, sudo=False, switch_user=None +): if sudo: return ssh_sudo(connection, command, errors=True) return ssh_cmd(connection, command, errors=True) @@ -208,7 +210,7 @@ def get_package_tags(os_release=None, redhat_release=None): @auto_connect -def get_info(host, *, users=None, connection=None): +def get_info(host, *, users=None, connection=None, switch_user=None): assert connection is not None log.debug("Getting info about '{}'".format(host)) user, host = connection.ssh_user, connection.ssh_host @@ -663,7 +665,8 @@ def install_host( trust_keys=None, insecure=False, demo_salt=None, - demo_sha=None + demo_sha=None, + switch_user=None ): data = get_info(host, connection=connection) if show_info: @@ -782,7 +785,7 @@ def errors(self): @auto_connect -def uninstall_host(host, *, connection=None, purge=False): +def uninstall_host(host, *, connection=None, purge=False, switch_user=None): data = get_info(host, connection=connection) print_info(data) @@ -807,7 +810,7 @@ def uninstall_host(host, *, connection=None, purge=False): @auto_connect -def deploy_masterfiles(host, tarball, *, connection=None): +def deploy_masterfiles(host, tarball, *, connection=None, switch_user=None): data = get_info(host, connection=connection) print("\nDeploying to:") print_info(data) diff --git a/cf_remote/ssh.py b/cf_remote/ssh.py index cb111a3..5e4ecd3 100644 --- a/cf_remote/ssh.py +++ b/cf_remote/ssh.py @@ -48,19 +48,6 @@ def _check_reachable( DEFAULT_SWITCH_USER_COMMAND_WITH_PASSWORD = "sudo -S -p '' bash -c" """Same, but reading the password from standard input instead of a terminal""" -_switch_user_command = None -_switch_user_password = None - - -def set_switch_user_command(command): - global _switch_user_command - _switch_user_command = command - - -def set_switch_user_password(password): - global _switch_user_password - _switch_user_password = password - def read_switch_user_password(path): """Read the password for switching user from the first line of a file @@ -88,55 +75,82 @@ def read_switch_user_password(path): return line.rstrip("\r\n") -def get_switch_user_password(): - return _switch_user_password - +class SwitchUser: + """How to run commands as another (privileged) user on the remote hosts -def get_switch_user_command(): - if _switch_user_command is not None: - return _switch_user_command - if _switch_user_password is not None: - return DEFAULT_SWITCH_USER_COMMAND_WITH_PASSWORD - return DEFAULT_SWITCH_USER_COMMAND - - -def switch_user(cmd): - """Wrap 'cmd' so that it runs as another (privileged) user - - 'cmd' is quoted rather than wrapped in quotes: a command containing a - quote of its own would otherwise end the wrapping early and the remote - shell would run something else, or nothing at all. + Built once from the command line options and passed to the connections it + applies to. Nothing here changes after that, so the settings of a run + cannot be read before they are complete, and a test can make one of these + without having to put anything back afterwards. """ - return "%s %s" % (get_switch_user_command(), shlex.quote(cmd)) - - -def _switch_user_needs_password(connection): - """Check whether switching user on this host requires a password - Only interesting when we actually have a password to send. Sending it - when it isn't needed would leave it on the standard input of the command - we are running instead. - - 'sudo -n' answers this without ever attempting to authenticate. Asking by - letting an attempt fail instead would count towards the failed attempts - that pam_faillock locks accounts out over, once per host and run. - """ - if not connection.needs_sudo: - return False - - if get_switch_user_password() is None: - return False - - if _switch_user_command is not None: - # A command we didn't pick has no 'sudo -n' to ask with, so run it - # with nothing on standard input: one that wants a password fails - # right away rather than taking ours. Assuming it wants one instead - # would hand the password to whatever runs when it doesn't. - return ( - connection.run(switch_user("true"), hide=True, stdin_input="").retcode != 0 - ) + def __init__(self, command=None, password=None): + """ + :param str command: command to run commands as another user with, the + command to run is appended as a single quoted + argument. `None` picks a default depending on + whether there is a password to send. + :param str password: password to send to :param:`command`, or `None` + when there is none to send. + """ + self._command = command + self._password = password + + @property + def password(self): + return self._password + + @property + def is_command_given(self): + """Whether the command is one we were given rather than one we picked""" + return self._command is not None + + @property + def command(self): + if self._command is not None: + return self._command + if self._password is not None: + return DEFAULT_SWITCH_USER_COMMAND_WITH_PASSWORD + return DEFAULT_SWITCH_USER_COMMAND + + def wrap(self, cmd): + """Wrap 'cmd' so that it runs as another (privileged) user + + 'cmd' is quoted rather than wrapped in quotes: a command containing a + quote of its own would otherwise end the wrapping early and the remote + shell would run something else, or nothing at all. + """ + return "%s %s" % (self.command, shlex.quote(cmd)) + + def needs_password_on(self, connection): + """Check whether switching user on this host requires a password + + Only interesting when we actually have a password to send. Sending it + when it isn't needed would leave it on the standard input of the + command we are running instead. + + 'sudo -n' answers this without ever attempting to authenticate. Asking + by letting an attempt fail instead would count towards the failed + attempts that pam_faillock locks accounts out over, once per host and + run. + """ + if not connection.needs_sudo: + return False + + if self._password is None: + return False + + if self.is_command_given: + # A command we didn't pick has no 'sudo -n' to ask with, so run it + # with nothing on standard input: one that wants a password fails + # right away rather than taking ours. Assuming it wants one instead + # would hand the password to whatever runs when it doesn't. + return ( + connection.run(self.wrap("true"), hide=True, stdin_input="").retcode + != 0 + ) - return connection.run("sudo -n true", hide=True).retcode != 0 + return connection.run("sudo -n true", hide=True).retcode != 0 class LocalConnection: @@ -144,10 +158,11 @@ class LocalConnection: ssh_user = None ssh_host = "localhost" - def __init__(self): + def __init__(self, switch_user=None): self.ssh_user = pwd.getpwuid(os.getuid()).pw_name + self.switch_user = switch_user or SwitchUser() self.needs_sudo = self.run("echo $UID", hide=True).stdout.strip() != "0" - self.switch_user_needs_password = _switch_user_needs_password(self) + self.switch_user_needs_password = self.switch_user.needs_password_on(self) def run(self, command, hide=False, stdin_input=None): # to maintain Python 3.5/3.6 compatability the following are used: @@ -175,7 +190,14 @@ def put(self, src, hide=False): class Connection: - def __init__(self, host, user, connect_kwargs=None, port=aramid._DEFAULT_SSH_PORT): + def __init__( + self, + host, + user, + connect_kwargs=None, + port=aramid._DEFAULT_SSH_PORT, + switch_user=None, + ): log.debug( "Initializing Connection: host '%s' user '%s' port '%s'" % (host, user, port) @@ -184,6 +206,7 @@ def __init__(self, host, user, connect_kwargs=None, port=aramid._DEFAULT_SSH_POR self.ssh_host = host self.ssh_port = port self.ssh_user = user + self.switch_user = switch_user or SwitchUser() self._connect_kwargs = connect_kwargs self._ssh_control_master = None @@ -213,7 +236,7 @@ def __init__(self, host, user, connect_kwargs=None, port=aramid._DEFAULT_SSH_POR ) self.needs_sudo = self.run("echo $UID", hide=True).stdout.strip() != "0" - self.switch_user_needs_password = _switch_user_needs_password(self) + self.switch_user_needs_password = self.switch_user.needs_password_on(self) log.debug("Connection initialized") def __del__(self): @@ -294,7 +317,7 @@ def get_state_from_host(host): return data -def connect(host, users=None): +def connect(host, users=None, switch_user=None): log.debug("Connecting to '%s'" % host) log.debug("users= '%s'" % users) @@ -329,7 +352,11 @@ def connect(host, users=None): if key: connect_kwargs["key_filename"] = os.path.expanduser(key) c = Connection( - host=host, user=user, port=port, connect_kwargs=connect_kwargs + host=host, + user=user, + port=port, + connect_kwargs=connect_kwargs, + switch_user=switch_user, ) c.ssh_user = user c.ssh_host = host @@ -351,16 +378,23 @@ def connect(host, users=None): # Requires that first positional argument is host # and connection should be a keyword argument with default None # Uses a context manager (with) to ensure connections are closed +# +# A 'switch_user' keyword argument, like 'users', is read here to make the +# connection with. A connection we are given already carries the one it was +# made with, so it is only of interest when we make one ourselves. def auto_connect(func): log.debug("Building config file") _build_ssh_config() def connect_wrapper(host, *args, **kwargs): + switch_user = kwargs.get("switch_user") if not kwargs.get("connection"): if host == "localhost": - kwargs["connection"] = LocalConnection() + kwargs["connection"] = LocalConnection(switch_user=switch_user) return func(host, *args, **kwargs) - with connect(host, users=kwargs.get("users")) as connection: + with connect( + host, users=kwargs.get("users"), switch_user=switch_user + ) as connection: assert connection kwargs["connection"] = connection return func(host, *args, **kwargs) @@ -369,9 +403,9 @@ def connect_wrapper(host, *args, **kwargs): return connect_wrapper -def scp(file, remote, connection=None, rename=None, hide=False): +def scp(file, remote, connection=None, rename=None, hide=False, switch_user=None): if not connection: - with connect(remote) as connection: + with connect(remote, switch_user=switch_user) as connection: scp(file, remote, connection, rename, hide=hide) else: if not hide: @@ -425,7 +459,7 @@ def _switch_user_hint(connection, result): or "no tty present" in output ) if needs_password: - if get_switch_user_password() is None: + if connection.switch_user.password is None: return ( "Switching user requires a password on '%s'," " rerun with --ask-pass to be prompted for it" % connection.ssh_host @@ -443,8 +477,8 @@ def ssh_sudo(connection, cmd, errors=False, needs_pty=False): stdin_input = None if connection.needs_sudo: - cmd = switch_user(cmd) - password = get_switch_user_password() + cmd = connection.switch_user.wrap(cmd) + password = connection.switch_user.password if connection.switch_user_needs_password and password is not None: stdin_input = password + "\n" diff --git a/tests/test_ssh.py b/tests/test_ssh.py index 99fe35b..2d03096 100644 --- a/tests/test_ssh.py +++ b/tests/test_ssh.py @@ -30,39 +30,44 @@ def test_failed_command(): nope("localhost") -@pytest.fixture(autouse=True) -def reset_switch_user(): - """Keep the switch user settings from leaking between tests""" - yield - ssh.set_switch_user_command(None) - ssh.set_switch_user_password(None) - - def test_switch_user_default(): - assert ssh.switch_user("cf-agent -K") == "sudo bash -c 'cf-agent -K'" + assert ssh.SwitchUser().wrap("cf-agent -K") == "sudo bash -c 'cf-agent -K'" def test_switch_user_with_password(): # With a password to send, sudo has to read it from standard input - ssh.set_switch_user_password("hunter2") - assert ssh.switch_user("cf-agent -K") == "sudo -S -p '' bash -c 'cf-agent -K'" + switch_user = ssh.SwitchUser(password="hunter2") + assert switch_user.wrap("cf-agent -K") == "sudo -S -p '' bash -c 'cf-agent -K'" def test_switch_user_command_overrides_default(): - ssh.set_switch_user_command("doas -n /bin/sh -c") - assert ssh.switch_user("cf-agent -K") == "doas -n /bin/sh -c 'cf-agent -K'" + switch_user = ssh.SwitchUser(command="doas -n /bin/sh -c") + assert switch_user.wrap("cf-agent -K") == "doas -n /bin/sh -c 'cf-agent -K'" # ... also when a password is given, then it's up to the user to make the # command read it from standard input - ssh.set_switch_user_password("hunter2") - assert ssh.switch_user("cf-agent -K") == "doas -n /bin/sh -c 'cf-agent -K'" + switch_user = ssh.SwitchUser(command="doas -n /bin/sh -c", password="hunter2") + assert switch_user.wrap("cf-agent -K") == "doas -n /bin/sh -c 'cf-agent -K'" + + +def test_switch_user_settings_do_not_leak_between_connections(): + # Two hosts in the same run can be reached with different settings, and + # one of these cannot change what another one already answers + plain = ssh.SwitchUser() + with_password = ssh.SwitchUser(password="hunter2") + + assert plain.password is None + assert plain.command == "sudo bash -c" + assert with_password.password == "hunter2" + assert with_password.command == "sudo -S -p '' bash -c" def test_switch_user_survives_quotes_in_the_command(): # A command carrying quotes of its own must not end the wrapping early. # Splitting it back the way a shell would proves it arrives in one piece. + switch_user = ssh.SwitchUser() for cmd in ("echo it's fine", 'echo "double"', "echo 'mixed \"quotes\"'"): - assert shlex.split(ssh.switch_user(cmd))[-1] == cmd + assert shlex.split(switch_user.wrap(cmd))[-1] == cmd def _password_file(tmp_path, content, mode=0o600): @@ -99,9 +104,10 @@ class FakeConnection: needs_sudo = True switch_user_needs_password = False - def __init__(self, retcode=0): + def __init__(self, retcode=0, switch_user=None): self.retcode = retcode self.commands = [] + self.switch_user = switch_user or ssh.SwitchUser() def run(self, command, hide=False, stdin_input=None): self.commands.append((command, stdin_input)) @@ -110,36 +116,34 @@ def run(self, command, hide=False, stdin_input=None): def test_no_password_means_no_asking(): connection = FakeConnection() - assert ssh._switch_user_needs_password(connection) is False + assert connection.switch_user.needs_password_on(connection) is False assert connection.commands == [] def test_root_is_never_asked(): # Nothing to switch to, so no round trip and nothing to send - ssh.set_switch_user_password("hunter2") - connection = FakeConnection() + connection = FakeConnection(switch_user=ssh.SwitchUser(password="hunter2")) connection.needs_sudo = False - assert ssh._switch_user_needs_password(connection) is False + assert connection.switch_user.needs_password_on(connection) is False assert connection.commands == [] def test_asking_never_attempts_authentication(): # A failed attempt is what pam_faillock counts, so this must not make one - ssh.set_switch_user_password("hunter2") + switch_user = ssh.SwitchUser(password="hunter2") - connection = FakeConnection(retcode=1) - assert ssh._switch_user_needs_password(connection) is True + connection = FakeConnection(retcode=1, switch_user=switch_user) + assert switch_user.needs_password_on(connection) is True assert connection.commands == [("sudo -n true", None)] - connection = FakeConnection(retcode=0) - assert ssh._switch_user_needs_password(connection) is False + connection = FakeConnection(retcode=0, switch_user=switch_user) + assert switch_user.needs_password_on(connection) is False assert connection.commands == [("sudo -n true", None)] def test_password_goes_on_standard_input(): - ssh.set_switch_user_password("hunter2") - connection = FakeConnection() + connection = FakeConnection(switch_user=ssh.SwitchUser(password="hunter2")) connection.switch_user_needs_password = True ssh.ssh_sudo(connection, "id -un") @@ -148,8 +152,7 @@ def test_password_goes_on_standard_input(): def test_password_is_withheld_where_it_isnt_needed(): # Otherwise it ends up on the standard input of the command instead - ssh.set_switch_user_password("hunter2") - connection = FakeConnection() + connection = FakeConnection(switch_user=ssh.SwitchUser(password="hunter2")) connection.switch_user_needs_password = False ssh.ssh_sudo(connection, "id -un") @@ -159,15 +162,14 @@ def test_password_is_withheld_where_it_isnt_needed(): def test_own_switch_user_command_is_asked_with_empty_input(): # Assuming it wants a password would hand the password to whatever runs # when it doesn't, so ask, with nothing it could mistake for one - ssh.set_switch_user_password("hunter2") - ssh.set_switch_user_command("doas /bin/sh -c") + switch_user = ssh.SwitchUser(command="doas /bin/sh -c", password="hunter2") - connection = FakeConnection(retcode=0) - assert ssh._switch_user_needs_password(connection) is False + connection = FakeConnection(retcode=0, switch_user=switch_user) + assert switch_user.needs_password_on(connection) is False assert connection.commands == [("doas /bin/sh -c true", "")] - connection = FakeConnection(retcode=1) - assert ssh._switch_user_needs_password(connection) is True + connection = FakeConnection(retcode=1, switch_user=switch_user) + assert switch_user.needs_password_on(connection) is True def _failure(stderr): @@ -183,18 +185,18 @@ def test_switch_user_hint_suggests_ask_pass(): def test_switch_user_hint_reports_rejected_password(): - ssh.set_switch_user_password("hunter2") + connection = FakeConnection(switch_user=ssh.SwitchUser(password="hunter2")) result = _failure("Sorry, try again.\nsudo: 1 incorrect password attempt") - hint = ssh._switch_user_hint(FakeConnection(), result) + hint = ssh._switch_user_hint(connection, result) assert hint is not None assert "rejected" in hint def test_switch_user_hint_reports_password_that_was_never_sent(): # Asking said no password was needed, but the command disagreed - ssh.set_switch_user_password("hunter2") + connection = FakeConnection(switch_user=ssh.SwitchUser(password="hunter2")) result = _failure("sudo: a password is required") - hint = ssh._switch_user_hint(FakeConnection(), result) + hint = ssh._switch_user_hint(connection, result) assert hint is not None assert "none was sent" in hint From cd4fa304074231a32fd93468c778a04da6dc2f49 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Tue, 18 Aug 2026 12:30:18 -0500 Subject: [PATCH 6/7] Let communicate() write the standard input of the commands Ticket: ENT-14418 Changelog: none Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Nick Anderson --- cf_remote/aramid.py | 44 ++++++++++++++++---------------- tests/test_aramid.py | 60 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 23 deletions(-) create mode 100644 tests/test_aramid.py diff --git a/cf_remote/aramid.py b/cf_remote/aramid.py index 4ce8d42..0af204f 100644 --- a/cf_remote/aramid.py +++ b/cf_remote/aramid.py @@ -115,35 +115,26 @@ def _get_put_method_args(method, host, src, dst): def _popen(args, stdin_input=None): - """Start a process, optionally writing 'stdin_input' to its standard input - - Anything we send this way (a password for switching user) is small enough - to fit in the pipe buffer, so writing it up front cannot block. Standard - input is left alone (inherited) when there is nothing to send. - - An empty string is not the same as `None`: it gives the command a pipe - with nothing in it, so anything waiting for input sees EOF at once rather - than blocking on the terminal 'cf-remote' was started from. - - The pipe is deliberately left open here, 'communicate()' closes it (and - with it, sends the EOF a command waiting for more input needs). + """Start a process, giving it a pipe on standard input if we have input for it + + The data itself is handed to 'Popen.communicate()' by + ':meth:`_Task.communicate`' rather than written here: writing to + 'proc.stdin' directly risks a deadlock, because the process can fill its + stdout or stderr pipe and stop reading while we are still blocked writing + to it. Standard input is left alone (inherited) when there is nothing to + send. + + An empty string is not the same as `None`: it gives the command a pipe that + is closed with nothing in it, so anything waiting for input sees EOF at + once rather than blocking on the terminal 'cf-remote' was started from. """ - proc = subprocess.Popen( + return subprocess.Popen( args, stdin=(subprocess.PIPE if stdin_input is not None else None), stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True, ) - if stdin_input: - assert proc.stdin is not None - try: - proc.stdin.write(stdin_input) - proc.stdin.flush() - except BrokenPipeError: - # The process is already gone, its exit code tells the story - pass - return proc class _Task: @@ -154,6 +145,7 @@ def __init__( self.proc = proc self.action = action self.stdin_input = stdin_input + self._input_given = False self._max_retries = retries self._retries = retries self.stdout = "" @@ -165,8 +157,13 @@ def __init__( def communicate(self, timeout=1, ignore_failed=False): start = time.time() + # 'communicate()' keeps writing what the first call handed it, and + # raises if a later one hands it the same input again. Timing out is + # normal here, so only the first call gets it. + stdin_input = None if self._input_given else self.stdin_input + self._input_given = True try: - out, err = self.proc.communicate(timeout=timeout) + out, err = self.proc.communicate(input=stdin_input, timeout=timeout) except subprocess.TimeoutExpired: log.debug("Connection timed out") return False @@ -179,6 +176,7 @@ def communicate(self, timeout=1, ignore_failed=False): # wait for the rest of timeout (if any) and restart the process time.sleep(max(timeout - (time.time() - start), 0)) self.proc = _popen(self.proc.args, self.stdin_input) + self._input_given = False # a new process needs it again self._retries -= 1 return False else: diff --git a/tests/test_aramid.py b/tests/test_aramid.py new file mode 100644 index 0000000..42ea8bb --- /dev/null +++ b/tests/test_aramid.py @@ -0,0 +1,60 @@ +from cf_remote import aramid + +# A process that writes more than a pipe buffer holds before it reads anything, +# so it stops reading while there is still output to collect. Writing to +# 'proc.stdin' ourselves is what deadlocks on that, which is why the input goes +# to 'communicate()' instead. +_TALKS_BEFORE_LISTENING = ["sh", "-c", "yes ohnoes | head -c 200000; cat"] + +# Slow enough that collecting the output takes more than one call +_SLOW_TO_ANSWER = ["sh", "-c", "sleep 0.5; cat"] + + +def _run(args, stdin_input=None, timeout=0.01): + """Run 'args' the way :func:`aramid.execute` runs a command on a host + + :return: the result and how many times collecting it timed out + """ + proc = aramid._popen(args, stdin_input=stdin_input) + task = aramid._Task( + aramid.Host("somehost"), proc, " ".join(args), stdin_input=stdin_input + ) + timeouts = 0 + while not task.communicate(timeout=timeout): + timeouts += 1 + return task.get_result(), timeouts + + +def test_input_arrives_and_no_output_is_lost(): + result, _ = _run(_TALKS_BEFORE_LISTENING, stdin_input="hunter2\n") + + assert result.retcode == 0 + assert len(result.stdout) == 200000 + len("hunter2\n") + assert result.stdout.endswith("hunter2\n") + + +def test_input_is_only_handed_over_once(): + # 'communicate()' keeps writing what the first call gave it and raises if a + # later one gives it the same input again, and timing out is normal here + result, timeouts = _run(_SLOW_TO_ANSWER, stdin_input="hunter2\n") + + assert timeouts > 0, "the command answered too quickly to test anything" + assert result.retcode == 0 + assert result.stdout == "hunter2\n" + + +def test_empty_input_closes_standard_input(): + # An empty string is not the same as None: the command gets a pipe that is + # closed with nothing in it, so it sees EOF instead of blocking on the + # terminal cf-remote was started from + result, _ = _run(["sh", "-c", "cat"], stdin_input="") + + assert result.retcode == 0 + assert result.stdout == "" + + +def test_output_and_exit_code_come_back_without_input(): + result, _ = _run(["sh", "-c", "echo hello; exit 3"]) + + assert result.retcode == 3 + assert result.stdout == "hello\n" From dafd6d47f5ed66c614adcd0ff741b222e860b633 Mon Sep 17 00:00:00 2001 From: Nick Anderson Date: Tue, 18 Aug 2026 12:30:20 -0500 Subject: [PATCH 7/7] Checked that the password stays out of the debug log too Ticket: ENT-14418 Changelog: none Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Nick Anderson --- tests/shell/002_sudo_password.sh | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/tests/shell/002_sudo_password.sh b/tests/shell/002_sudo_password.sh index 82ce898..1971289 100755 --- a/tests/shell/002_sudo_password.sh +++ b/tests/shell/002_sudo_password.sh @@ -46,6 +46,16 @@ assert_output () { # assert_output fi } +# The password must never be printed, not even with --log-level debug, so check +# for it in everything cf-remote wrote rather than only in the command output +refute_password () { # refute_password + if printf '%s\n' "$out" | grep -q "$password"; then + echo "FAIL: password showed up in the output: $1" + exit 1 + fi + echo "ok: no password in the output ($1)" +} + # SSH logs in with a key, so the account password is only used by sudo rm -f "$dir/id_test" "$dir/id_test.pub" ssh-keygen -t ed25519 -N "" -f "$dir/id_test" -q @@ -81,18 +91,23 @@ echo "=== with --ask-pass: should run as root ===" run_cfr "$password" cf-remote --ask-pass sudo -H cftest@"$host":"$port" 'id -un' assert_output "root" +echo "=== with --log-level debug: the password must stay out of the log ===" +run_cfr "$password" cf-remote --log-level debug --ask-pass \ + sudo -H cftest@"$host":"$port" 'id -un' +assert_output "root" +assert_output "\[DEBUG\]" # ... it really was a debug run +refute_password "the password was sent to this host" + echo "=== with a wrong password: should say it was rejected ===" run_cfr "definitely-not-the-password" \ cf-remote --ask-pass sudo -H cftest@"$host":"$port" 'id -un' assert_output -i "rejected" -echo "=== NOPASSWD host: the password must not reach the command ===" -run_cfr "$password" cf-remote --ask-pass sudo -H cfnopass@"$host":"$port" 'cat' -if printf '%s\n' "$out" | grep -q "$password"; then - echo "FAIL: password ended up on the standard input of the command" - exit 1 -fi -echo "ok: no password sent where none was needed" +echo "=== NOPASSWD host: the password must not reach the command or the log ===" +run_cfr "$password" cf-remote --log-level debug --ask-pass \ + sudo -H cfnopass@"$host":"$port" 'cat' +assert_output "\[DEBUG\]" # ... it really was a debug run +refute_password "no password was needed on this host" echo "=== --switch-user-command is used as given ===" run_cfr "$password" cf-remote --ask-pass \