From: John Mulligan Date: Wed, 14 Feb 2024 16:35:57 +0000 (-0500) Subject: mgr/cephadm: make remote command execution auditable X-Git-Tag: v20.0.0~2361^2~3 X-Git-Url: http://git.apps.os.sepia.ceph.com/?a=commitdiff_plain;h=a811ef0d813570752aaffcf38dc18cc6778b72a9;p=ceph.git mgr/cephadm: make remote command execution auditable Update ssh.py and other code using it to only allow commands wrapped in particular python types as executables on the remote hosts. By using a specific type for remote executables we make the code more auditable, avoiding the possibility of executing arbitrary strings as commands with sudo. This is all enforced by mypy's type checking. The result is a list of commands that the cephadm mgr module may execute on a remote host using sudo: ``` $ git ls-files -z | xargs -0 grep 'RemoteExecutable(' -d skip -h | grep -v '(str)' | sed -e 's/.*RemoteExecutable(//' -e 's/)//' -e 's/,$//' 'which' '/usr/bin/cephadm' python 'chmod' 'ls' 'sysctl' 'chown' 'mkdir' 'mv' 'touch' 'rm' 'true' ``` Note that *python* is special as it is based on the output of which and may vary from OS to OS. The quoted items are used exactly as named. Only the binary at `/usr/bin/cephadm` _or_ the dynamically discovered python3 binary will be used. This depends on a configuration option for the cephadm module. Signed-off-by: John Mulligan --- diff --git a/src/pybind/mgr/cephadm/offline_watcher.py b/src/pybind/mgr/cephadm/offline_watcher.py index 2b7751dfc34d8..4aa07e2f584ae 100644 --- a/src/pybind/mgr/cephadm/offline_watcher.py +++ b/src/pybind/mgr/cephadm/offline_watcher.py @@ -4,6 +4,8 @@ from typing import List, Optional, TYPE_CHECKING import multiprocessing as mp import threading +from . import ssh + if TYPE_CHECKING: from cephadm.module import CephadmOrchestrator @@ -38,7 +40,8 @@ class OfflineHostWatcher(threading.Thread): def check_host(self, host: str) -> None: if host not in self.mgr.offline_hosts: try: - self.mgr.ssh.check_execute_command(host, ['true'], log_command=self.mgr.log_refresh_metadata) + rcmd = ssh.RemoteCommand(ssh.Executables.TRUE) + self.mgr.ssh.check_execute_command(host, rcmd, log_command=self.mgr.log_refresh_metadata) except Exception: logger.debug(f'OfflineHostDetector: detected {host} to be offline') # kick serve loop in case corrective action must be taken for offline host diff --git a/src/pybind/mgr/cephadm/serve.py b/src/pybind/mgr/cephadm/serve.py index 27775087d05d5..761174fc6c0f4 100644 --- a/src/pybind/mgr/cephadm/serve.py +++ b/src/pybind/mgr/cephadm/serve.py @@ -34,6 +34,7 @@ from mgr_util import format_bytes, verify_tls, get_cert_issuer_info, ServerConfi from . import utils from . import exchange +from . import ssh if TYPE_CHECKING: from cephadm.module import CephadmOrchestrator @@ -42,6 +43,9 @@ logger = logging.getLogger(__name__) REQUIRES_POST_ACTIONS = ['grafana', 'iscsi', 'prometheus', 'alertmanager', 'rgw', 'nvmeof'] +WHICH = ssh.RemoteExecutable('which') +CEPHADM_EXE = ssh.RemoteExecutable('/usr/bin/cephadm') + class CephadmServe: """ @@ -1271,7 +1275,7 @@ class CephadmServe: if path == '/etc/ceph/ceph.conf': continue self.log.info(f'Removing {host}:{path}') - cmd = ['rm', '-f', path] + cmd = ssh.RemoteCommand(ssh.Executables.RM, ['-f', path]) self.mgr.ssh.check_execute_command(host, cmd) updated_files = True self.mgr.cache.removed_client_file(host, path) @@ -1620,15 +1624,24 @@ class CephadmServe: if stdin and 'agent' not in str(entity): self.log.debug('stdin: %s' % stdin) - cmd = ['which', 'python3'] + cmd = ssh.RemoteCommand(WHICH, ['python3']) python = await self.mgr.ssh._check_execute_command(host, cmd, addr=addr) - cmd = [python, self.mgr.cephadm_binary_path] + final_args + # N.B. because the python3 executable is based on the results of the + # which command we can not know it ahead of time and must be converted + # into a RemoteExecutable. + cmd = ssh.RemoteCommand( + ssh.RemoteExecutable(python), + [self.mgr.cephadm_binary_path] + final_args + ) try: out, err, code = await self.mgr.ssh._execute_command( host, cmd, stdin=stdin, addr=addr) if code == 2: - ls_cmd = ['ls', self.mgr.cephadm_binary_path] + ls_cmd = ssh.RemoteCommand( + ssh.Executables.LS, + [self.mgr.cephadm_binary_path] + ) out_ls, err_ls, code_ls = await self.mgr.ssh._execute_command(host, ls_cmd, addr=addr, log_command=log_output) if code_ls == 2: @@ -1649,7 +1662,7 @@ class CephadmServe: elif self.mgr.mode == 'cephadm-package': try: - cmd = ['/usr/bin/cephadm'] + final_args + cmd = ssh.RemoteCommand(CEPHADM_EXE, final_args) out, err, code = await self.mgr.ssh._execute_command( host, cmd, stdin=stdin, addr=addr) except Exception as e: diff --git a/src/pybind/mgr/cephadm/ssh.py b/src/pybind/mgr/cephadm/ssh.py index 7460fc159d7b2..f0d507dfe8958 100644 --- a/src/pybind/mgr/cephadm/ssh.py +++ b/src/pybind/mgr/cephadm/ssh.py @@ -1,3 +1,4 @@ +import enum import logging import os import asyncio @@ -44,6 +45,77 @@ Host * """ +class RemoteExecutable(str): + pass + + +class RemoteCommand: + exe: RemoteExecutable + args: List[str] + + def __init__(self, exe: RemoteExecutable, args: Optional[List[str]] = None) -> None: + self.exe = exe + self.args = args or [] + + def __iter__(self) -> Iterator[str]: + yield str(self.exe) + for arg in self.args: + yield arg + + def quoted(self) -> Iterator[str]: + return (quote(a) for a in self) + + def __str__(self) -> str: + return " ".join(self.quoted()) + + def __repr__(self) -> str: + # handy when debugging tests + return f'({self.exe!r}, {self.args!r})' + + def __eq__(self, other: object) -> bool: + # handy when working with unit tests + if not isinstance(other, self.__class__): + return NotImplemented + return other.exe == self.exe and other.args == self.args + + +class RemoteSudoCommand(RemoteCommand): + use_sudo: bool = True + + def __init__( + self, exe: RemoteExecutable, args: List[str], use_sudo: bool = True + ) -> None: + super().__init__(exe, args) + self.use_sudo = use_sudo + + def __iter__(self) -> Iterator[str]: + if self.use_sudo: + yield 'sudo' + for a in super().__iter__(): + yield a + + @classmethod + def wrap( + cls, other: RemoteCommand, use_sudo: bool = True + ) -> 'RemoteSudoCommand': + return cls(other.exe, other.args, use_sudo) + + +class Executables(RemoteExecutable, enum.Enum): + CHMOD = RemoteExecutable('chmod') + CHOWN = RemoteExecutable('chown') + LS = RemoteExecutable('ls') + MKDIR = RemoteExecutable('mkdir') + MV = RemoteExecutable('mv') + RM = RemoteExecutable('rm') + SYSCTL = RemoteExecutable('sysctl') + TOUCH = RemoteExecutable('touch') + TRUE = RemoteExecutable('true') + + def __str__(self) -> str: + return self.value + + class EventLoopThread(Thread): def __init__(self) -> None: @@ -152,24 +224,27 @@ class SSHManager: async def _execute_command(self, host: str, - cmd_components: List[str], + cmd_components: RemoteCommand, stdin: Optional[str] = None, addr: Optional[str] = None, log_command: Optional[bool] = True, ) -> Tuple[str, str, int]: conn = await self._remote_connection(host, addr) - sudo_prefix = "sudo " if self.mgr.ssh_user != 'root' else "" - cmd = sudo_prefix + " ".join(quote(x) for x in cmd_components) + use_sudo = (self.mgr.ssh_user != 'root') + rcmd = RemoteSudoCommand.wrap(cmd_components, use_sudo=use_sudo) try: address = addr or self.mgr.inventory.get_addr(host) except Exception: address = host if log_command: - logger.debug(f'Running command: {cmd}') + logger.debug(f'Running command: {rcmd}') try: - r = await conn.run(f'{sudo_prefix}true', check=True, timeout=5) # host quick check - r = await conn.run(cmd, input=stdin) + test_cmd = RemoteSudoCommand( + Executables.TRUE, [], use_sudo=use_sudo + ) + r = await conn.run(str(test_cmd), check=True, timeout=5) # host quick check + r = await conn.run(str(rcmd), input=stdin) # handle these Exceptions otherwise you might get a weird error like # TypeError: __init__() missing 1 required positional argument: 'reason' (due to the asyncssh error interacting with raise_if_exception) except asyncssh.ChannelOpenError as e: @@ -179,13 +254,13 @@ class SSHManager: self.mgr.offline_hosts.add(host) raise HostConnectionError(f'Unable to reach remote host {host}. {str(e)}', host, address) except asyncssh.ProcessError as e: - msg = f"Cannot execute the command '{cmd}' on the {host}. {str(e.stderr)}." + msg = f"Cannot execute the command '{rcmd}' on the {host}. {str(e.stderr)}." logger.debug(msg) await self._reset_con(host) self.mgr.offline_hosts.add(host) raise HostConnectionError(msg, host, address) except Exception as e: - msg = f"Generic error while executing command '{cmd}' on the host {host}. {str(e)}." + msg = f"Generic error while executing command '{rcmd}' on the host {host}. {str(e)}." logger.debug(msg) await self._reset_con(host) self.mgr.offline_hosts.add(host) @@ -209,7 +284,7 @@ class SSHManager: def execute_command(self, host: str, - cmd: List[str], + cmd: RemoteCommand, stdin: Optional[str] = None, addr: Optional[str] = None, log_command: Optional[bool] = True @@ -219,7 +294,7 @@ class SSHManager: async def _check_execute_command(self, host: str, - cmd: List[str], + cmd: RemoteCommand, stdin: Optional[str] = None, addr: Optional[str] = None, log_command: Optional[bool] = True @@ -233,7 +308,7 @@ class SSHManager: def check_execute_command(self, host: str, - cmd: List[str], + cmd: RemoteCommand, stdin: Optional[str] = None, addr: Optional[str] = None, log_command: Optional[bool] = True, @@ -253,14 +328,22 @@ class SSHManager: try: cephadm_tmp_dir = f"/tmp/cephadm-{self.mgr._cluster_fsid}" dirname = os.path.dirname(path) - await self._check_execute_command(host, ['mkdir', '-p', dirname], addr=addr) - await self._check_execute_command(host, ['mkdir', '-p', cephadm_tmp_dir + dirname], addr=addr) + mkdir = RemoteCommand(Executables.MKDIR, ['-p', dirname]) + await self._check_execute_command(host, mkdir, addr=addr) + mkdir2 = RemoteCommand(Executables.MKDIR, ['-p', cephadm_tmp_dir + dirname]) + await self._check_execute_command(host, mkdir2, addr=addr) tmp_path = cephadm_tmp_dir + path + '.new' - await self._check_execute_command(host, ['touch', tmp_path], addr=addr) + touch = RemoteCommand(Executables.TOUCH, [tmp_path]) + await self._check_execute_command(host, touch, addr=addr) if self.mgr.ssh_user != 'root': assert self.mgr.ssh_user - await self._check_execute_command(host, ['chown', '-R', self.mgr.ssh_user, cephadm_tmp_dir], addr=addr) - await self._check_execute_command(host, ['chmod', str(644), tmp_path], addr=addr) + chown = RemoteCommand( + Executables.CHOWN, + ['-R', self.mgr.ssh_user, cephadm_tmp_dir] + ) + await self._check_execute_command(host, chown, addr=addr) + chmod = RemoteCommand(Executables.CHMOD, [str(644), tmp_path]) + await self._check_execute_command(host, chmod, addr=addr) with NamedTemporaryFile(prefix='cephadm-write-remote-file-') as f: os.fchmod(f.fileno(), 0o600) f.write(content) @@ -270,9 +353,15 @@ class SSHManager: await sftp.put(f.name, tmp_path) if uid is not None and gid is not None and mode is not None: # shlex quote takes str or byte object, not int - await self._check_execute_command(host, ['chown', '-R', str(uid) + ':' + str(gid), tmp_path], addr=addr) - await self._check_execute_command(host, ['chmod', oct(mode)[2:], tmp_path], addr=addr) - await self._check_execute_command(host, ['mv', tmp_path, path], addr=addr) + chown = RemoteCommand( + Executables.CHOWN, + ['-R', str(uid) + ':' + str(gid), tmp_path] + ) + await self._check_execute_command(host, chown, addr=addr) + chmod = RemoteCommand(Executables.CHMOD, [oct(mode)[2:], tmp_path]) + await self._check_execute_command(host, chmod, addr=addr) + mv = RemoteCommand(Executables.MV, [tmp_path, path]) + await self._check_execute_command(host, mv, addr=addr) except Exception as e: msg = f"Unable to write {host}:{path}: {e}" logger.exception(msg) diff --git a/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py b/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py index 66feaee319494..9db971f6f216e 100644 --- a/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py +++ b/src/pybind/mgr/cephadm/tests/test_tuned_profiles.py @@ -5,7 +5,7 @@ from cephadm.tuned_profiles import TunedProfileUtils, SYSCTL_DIR from cephadm.inventory import TunedProfileStore from ceph.utils import datetime_now from ceph.deployment.service_spec import TunedProfileSpec, PlacementSpec -from cephadm.ssh import SSHManager +from cephadm.ssh import SSHManager, RemoteCommand, Executables from orchestrator import HostSpec from typing import List, Dict @@ -148,10 +148,26 @@ class TestTunedProfiles: tp = TunedProfileUtils(mgr) tp._remove_stray_tuned_profiles('a', self.profiles_to_calls(tp, [self.tspec1, self.tspec2])) calls = [ - mock.call('a', ['ls', SYSCTL_DIR], log_command=False), - mock.call('a', ['rm', '-f', f'{SYSCTL_DIR}/p3-cephadm-tuned-profile.conf']), - mock.call('a', ['rm', '-f', f'{SYSCTL_DIR}/who-cephadm-tuned-profile.conf']), - mock.call('a', ['sysctl', '--system']) + mock.call( + 'a', RemoteCommand(Executables.LS, [SYSCTL_DIR]), log_command=False + ), + mock.call( + 'a', + RemoteCommand( + Executables.RM, + ['-f', f'{SYSCTL_DIR}/p3-cephadm-tuned-profile.conf'] + ) + ), + mock.call( + 'a', + RemoteCommand( + Executables.RM, + ['-f', f'{SYSCTL_DIR}/who-cephadm-tuned-profile.conf'] + ) + ), + mock.call( + 'a', RemoteCommand(Executables.SYSCTL, ['--system']) + ), ] _check_execute_command.assert_has_calls(calls, any_order=True) @@ -170,7 +186,9 @@ class TestTunedProfiles: profiles) tp = TunedProfileUtils(mgr) tp._write_tuned_profiles('a', self.profiles_to_calls(tp, [self.tspec1, self.tspec2])) - _check_execute_command.assert_called_with('a', ['sysctl', '--system']) + _check_execute_command.assert_called_with( + 'a', RemoteCommand(Executables.SYSCTL, ['--system']) + ) _write_remote_file.assert_called_with( 'a', f'{SYSCTL_DIR}/p2-cephadm-tuned-profile.conf', tp._profile_to_str(self.tspec2).encode('utf-8')) diff --git a/src/pybind/mgr/cephadm/tuned_profiles.py b/src/pybind/mgr/cephadm/tuned_profiles.py index 8ec30bd536be7..7a37d9379044b 100644 --- a/src/pybind/mgr/cephadm/tuned_profiles.py +++ b/src/pybind/mgr/cephadm/tuned_profiles.py @@ -3,6 +3,7 @@ from typing import Dict, List, TYPE_CHECKING from ceph.utils import datetime_now from .schedule import HostAssignment from ceph.deployment.service_spec import ServiceSpec, TunedProfileSpec +from . import ssh if TYPE_CHECKING: from cephadm.module import CephadmOrchestrator @@ -11,6 +12,8 @@ logger = logging.getLogger(__name__) SYSCTL_DIR = '/etc/sysctl.d' +SYSCTL_SYSTEM_CMD = ssh.RemoteCommand(ssh.Executables.SYSCTL, ['--system']) + class TunedProfileUtils(): def __init__(self, mgr: "CephadmOrchestrator") -> None: @@ -69,7 +72,7 @@ class TunedProfileUtils(): """ if self.mgr.cache.is_host_unreachable(host): return - cmd = ['ls', SYSCTL_DIR] + cmd = ssh.RemoteCommand(ssh.Executables.LS, [SYSCTL_DIR]) found_files = self.mgr.ssh.check_execute_command(host, cmd, log_command=self.mgr.log_refresh_metadata).split('\n') found_files = [s.strip() for s in found_files] profile_names: List[str] = sum([[*p] for p in profiles], []) # extract all profiles names @@ -81,11 +84,11 @@ class TunedProfileUtils(): continue if file not in expected_files: logger.info(f'Removing stray tuned profile file {file}') - cmd = ['rm', '-f', f'{SYSCTL_DIR}/{file}'] + cmd = ssh.RemoteCommand(ssh.Executables.RM, ['-f', f'{SYSCTL_DIR}/{file}']) self.mgr.ssh.check_execute_command(host, cmd) updated = True if updated: - self.mgr.ssh.check_execute_command(host, ['sysctl', '--system']) + self.mgr.ssh.check_execute_command(host, SYSCTL_SYSTEM_CMD) def _write_tuned_profiles(self, host: str, profiles: List[Dict[str, str]]) -> None: if self.mgr.cache.is_host_unreachable(host): @@ -99,5 +102,5 @@ class TunedProfileUtils(): self.mgr.ssh.write_remote_file(host, profile_filename, content.encode('utf-8')) updated = True if updated: - self.mgr.ssh.check_execute_command(host, ['sysctl', '--system']) + self.mgr.ssh.check_execute_command(host, SYSCTL_SYSTEM_CMD) self.mgr.cache.last_tuned_profile_update[host] = datetime_now()