From a811ef0d813570752aaffcf38dc18cc6778b72a9 Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Wed, 14 Feb 2024 11:35:57 -0500 Subject: [PATCH 1/4] 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 --- src/pybind/mgr/cephadm/offline_watcher.py | 5 +- src/pybind/mgr/cephadm/serve.py | 23 +++- src/pybind/mgr/cephadm/ssh.py | 127 +++++++++++++++--- .../mgr/cephadm/tests/test_tuned_profiles.py | 30 ++++- src/pybind/mgr/cephadm/tuned_profiles.py | 11 +- 5 files changed, 161 insertions(+), 35 deletions(-) diff --git a/src/pybind/mgr/cephadm/offline_watcher.py b/src/pybind/mgr/cephadm/offline_watcher.py index 2b7751dfc34..4aa07e2f584 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 27775087d05..761174fc6c0 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 7460fc159d7..f0d507dfe89 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 66feaee3194..9db971f6f21 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 8ec30bd536b..7a37d937904 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() From d9fb2840fc34534fc157cd2d9563c2c1a5ebe407 Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Fri, 8 Mar 2024 13:19:39 -0500 Subject: [PATCH 2/4] mgr/cephadm: add test to ensure the list of remote commands is known Add a test file to help ensure the audit of remote commands is kept up to date. Signed-off-by: John Mulligan --- .../cephadm/tests/test_remote_executables.py | 262 ++++++++++++++++++ 1 file changed, 262 insertions(+) create mode 100644 src/pybind/mgr/cephadm/tests/test_remote_executables.py diff --git a/src/pybind/mgr/cephadm/tests/test_remote_executables.py b/src/pybind/mgr/cephadm/tests/test_remote_executables.py new file mode 100644 index 00000000000..a98f81a8df1 --- /dev/null +++ b/src/pybind/mgr/cephadm/tests/test_remote_executables.py @@ -0,0 +1,262 @@ +#!/usr/bin/python3 +"""Test to ensure remote suodable executables are audited. + +This file can be used in one of two ways: +* as a "unit test" executed by pytest +* as a script that can report on or check expected remote executables + +It is designed to act as a method of ensuring that the executables that we run +on remote nodes under sudo are explicitly known. The types defined in ssh.py +act as both a way to audit the commands (via this script) and that we don't +lose track of what can be run - by relying on mypy. + +The unit test mode integrates into pytest for convenience, it really acts as a +static check that scans the source code of the cephadm mgr module. NOTE: the +file's test script mode is sensitive to it's location in the source tree. If +files get moved this script may need to be updated. + +The test asserts that the `EXPECTED` list matches all tools that may be +executed remotely under sudo. + +This file can also be run as a script. When supplied with a directory or list +of files it will read the sources and report on all remote executables found. +When run with the `--check` option it performs the same job as the unit test +mode but outputs a report in a more human readable format. + +If the commands the manager module can execute remotely change the `EXPECTED` +this must be updated to match to ensure the change is being done deliberately. +Any corresponding documentation should also be updated. + +Note that ideally the EXPECTED list should shrink and not grow. Any changes to +the list may cause administrators of the ceph cluster to have to make manual +changes to the system prior/during upgrade! +""" +import ast +import os +import pathlib +import sys + +ssh_py = 'ssh.py' +serve_py = 'serve.py' + +# IMPORTANT - please read the entire module docstring before changing +# the list below. +EXPECTED = [ + # - value | is_constant | filename - + # constant executables + ('/usr/bin/cephadm', True, serve_py), + ('chmod', True, ssh_py), + ('chown', True, ssh_py), + ('ls', True, ssh_py), + ('mkdir', True, ssh_py), + ('mv', True, ssh_py), + ('rm', True, ssh_py), + ('sysctl', True, ssh_py), + ('touch', True, ssh_py), + ('true', True, ssh_py), + ('which', True, serve_py), + # variable executables + ('python', False, serve_py), +] + + +def test_expected_remote_executables(): + import pytest + + if sys.version_info < (3, 8): + pytest.skip("python 3.8 or later required") + + self_path = pathlib.Path(__file__).resolve() + # test is sensitive to where it is in the source tree. it expects to be in + # a tests directory under the cephadm mgr module. if stuff gets moved + # around this test will likely start failing and need to be updated + remote_exes = find_sudoable_exes_in_files( + _file_paths([self_path.parent.parent]) + ) + unexpected, gone = diff_remote_exes(remote_exes) + unexpected_msgs = gone_msgs = [] + if unexpected or gone: + unexpected_msgs, gone_msgs = format_diff( + unexpected, gone, remote_exes + ) + assert not unexpected_msgs, unexpected_msgs + assert not gone_msgs, gone_msgs + + +def _essential(v): + """Convert a remote exe dict to a tuple with only the essential fields.""" + return (v["value"], v["is_constant"], v["filename"].split("/")[-1]) + + +def _names(node): + if isinstance(node, ast.Name): + return [node.id] + if isinstance(node, ast.Attribute): + vn = _names(node.value) + return vn + [node.attr] + if isinstance(node, ast.Call): + return _names(node.func) + if isinstance(node, ast.Constant): + return [repr(node.value)] + if isinstance(node, ast.JoinedStr): + return [f""] + if isinstance(node, ast.Subscript): + return [f""] + raise ValueError(f"_names: unexpected type: {node}") + + +def _arg_kind(node): + assert isinstance(node, ast.Call) + assert len(node.args) == 1 + arg = node.args[0] + if isinstance(arg, ast.Constant): + return str(arg.value), True + names = _names(arg) + return ".".join(names), False + + +class ExecutableVisitor(ast.NodeVisitor): + def __init__(self): + super().__init__() + self.remote_executables = [] + + def visit_Call(self, node): + names = _names(node) + if names[-1] == 'RemoteExecutable': + value, is_constant = _arg_kind(node) + self.remote_executables.append( + dict(value=value, is_constant=is_constant, lineno=node.lineno) + ) + self.generic_visit(node) + + +def find_sudoable_exes(tree): + ev = ExecutableVisitor() + ev.visit(tree) + return ev.remote_executables + + +def find_sudoable_exes_in_files(files): + out = [] + for file in files: + with open(file) as fh: + source = fh.read() + tree = ast.parse(source, fh.name) + rmes = find_sudoable_exes(tree) + for rme in rmes: + rme['filename'] = str(file) + out.extend(rmes) + return out + + +def diff_remote_exes(remote_exes): + expected = set(EXPECTED) + current = {_essential(v) for v in remote_exes} + unexpected = current - expected + gone = expected - current + return unexpected, gone + + +def format_diff(unexpected, gone, remote_exes): + current = {_essential(v): v for v in remote_exes} + unexpected_msgs = [] + for val, is_constant, fn in unexpected: + vn = 'constant' if is_constant else 'variable' + vq = repr(val) if is_constant else val + # info is needed for full filename/linenumber and is only relevant for + # found (unexpected) entries + info = current[(val, is_constant, fn)] + unexpected_msgs.append( + f'{vn} {vq} in {info["filename"]}:{info["lineno"]} not tracked' + ) + gone_msgs = [] + for val, is_constant, fn in gone: + vn = 'constant' if is_constant else 'variable' + vq = repr(val) if is_constant else val + gone_msgs.append(f"{vn} {vq} expected in {fn} not found") + return unexpected_msgs, gone_msgs + + +def report_remote_exes(remote_exes): + for rme in remote_exes: + clabel = 'CONSTANT' if rme["is_constant"] else "VARIABLE" + print( + "{clabel:10} {value:10} {filename}:{lineno}".format( + clabel=clabel, **rme + ) + ) + + +def report_compare_remote_exes(remote_exes): + import textwrap + + unexpected, gone = diff_remote_exes(remote_exes) + if not (unexpected or gone): + print('No issues detected') + sys.exit(0) + unexpected_msgs, gone_msgs = format_diff(unexpected, gone, remote_exes) + if unexpected_msgs: + desc = textwrap.wrap( + "One or more remote executable has been detected in the source" + " files that is not tracked in the test. If this change is" + " intended you must update the test AND update any corresponding" + " documentation.", + 76, + ) + for line in desc: + print(line) + print('-' * 76) + for msg in unexpected_msgs: + print(f'* {msg}') + if unexpected_msgs: + print() + if gone_msgs: + desc = textwrap.wrap( + "One or more remote executable that is expected to appear" + " in the source files has not been detected." + " If this change is intended you must update the test AND update" + " any corresponding documentation.", + 76, + ) + for line in desc: + print(line) + print('-' * 76) + for msg in gone_msgs: + print(f'* {msg}') + + sys.exit(1) + + +def _file_paths(src_paths): + files = set() + for path in src_paths: + if path.is_file(): + files.add(path) + continue + for d, ds, fs in os.walk(path): + if 'tests' in ds: + ds.remove('tests') + dpath = pathlib.Path(d) + for fn in fs: + if fn.endswith('.py'): + files.add(dpath / fn) + return files + + +def main(): + import argparse + + parser = argparse.ArgumentParser() + parser.add_argument('--check', action='store_true') + parser.add_argument('PATH', nargs='+', type=pathlib.Path) + cli = parser.parse_args() + + remote_exes = find_sudoable_exes_in_files(_file_paths(cli.PATH)) + if cli.check: + report_compare_remote_exes(remote_exes) + else: + report_remote_exes(remote_exes) + + +if __name__ == '__main__': + main() From e357d2f99a14969b6f1a089680a9f16dc3bb0e54 Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Thu, 14 Mar 2024 14:02:17 -0400 Subject: [PATCH 3/4] mgr/cephadm: add a simple unit test for RemoteCommand class Converting a remote command to something that other libs uses requires converting the enum to a string. Python behavior in the area varies across versions so add a unit test that verifies the conversion behaves as intended. Signed-off-by: John Mulligan --- src/pybind/mgr/cephadm/tests/test_ssh.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/pybind/mgr/cephadm/tests/test_ssh.py b/src/pybind/mgr/cephadm/tests/test_ssh.py index 29f01b6c797..44ef3d429b7 100644 --- a/src/pybind/mgr/cephadm/tests/test_ssh.py +++ b/src/pybind/mgr/cephadm/tests/test_ssh.py @@ -103,3 +103,14 @@ class TestWithSSH: class TestWithoutSSH: def test_can_run(self, cephadm_module: CephadmOrchestrator): assert cephadm_module.can_run() == (False, "loading asyncssh library:No module named 'asyncssh'") + + +def test_remote_command(): + from cephadm.ssh import RemoteCommand, Executables + + assert list(RemoteCommand(Executables.TRUE)) == ['true'] + assert list(RemoteCommand(Executables.RM, ['-rf', '/tmp/blat'])) == [ + 'rm', + '-rf', + '/tmp/blat', + ] From 18efb3b74249f7035bfa77ad6925328c16ab0ce9 Mon Sep 17 00:00:00 2001 From: John Mulligan Date: Fri, 8 Mar 2024 13:56:19 -0500 Subject: [PATCH 4/4] doc/cephdam: document limiting passwordless sudo commands Based on the previous commits making the remote executables auditable and explicit, document the admin's ability to restrict password-less sudo access to only the set of commands cephadm actually uses. Signed-off-by: John Mulligan --- doc/cephadm/operations.rst | 45 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/doc/cephadm/operations.rst b/doc/cephadm/operations.rst index c2c8f8b6d28..4ec28bc1c36 100644 --- a/doc/cephadm/operations.rst +++ b/doc/cephadm/operations.rst @@ -658,6 +658,51 @@ For example, to distribute configs to hosts with the ``bare_config`` label, run (See :ref:`orchestrator-cli-placement-spec` for more information about placement specs.) + +Limiting Password-less sudo Access +================================== + +By default, the cephadm install guide recommends enabling password-less +``sudo`` for the cephadm user. This option is the most flexible and +future-proof but may not be preferred in all environments. An administrator can +restrict ``sudo`` to only running an exact list of commands without password +access. Note that this list may change between Ceph versions and +administrators choosing this option should read the release notes and review +this list in the destination version of the Ceph documentation. If the list +differs one must extend the list of password-less ``sudo`` commands prior to +upgrade. + +Commands requiring password-less sudo support: + + - ``chmod`` + - ``chown`` + - ``ls`` + - ``mkdir`` + - ``mv`` + - ``rm`` + - ``sysctl`` + - ``touch`` + - ``true`` + - ``which`` (see note) + - ``/usr/bin/cephadm`` or python executable (see note) + +.. note:: Typically cephadm will execute ``which`` to determine what python3 + command is available and then use the command returned by ``which`` in + subsequent commands. + Before configuring ``sudo`` run ``which python3`` to determine what + python command to add to the ``sudo`` configuration. + In some rare configurations ``/usr/bin/cephadm`` will be used instead. + + +Configuring the ``sudoers`` file can be performed using a tool like ``visudo`` +and adding or replacing a user configuration line such as the following: + +.. code-block:: + + # assuming the cephadm user is named "ceph" + ceph ALL=(ALL) NOPASSWD:/usr/bin/chmod,/usr/bin/chown,/usr/bin/ls,/usr/bin/mkdir,/usr/bin/mv,/usr/bin/rm,/usr/sbin/sysctl,/usr/bin/touch,/usr/bin/true,/usr/bin/which,/usr/bin/cephadm,/usr/bin/python3 + + Purging a cluster =================