diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index d00e70a..2916e70 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -63,8 +63,8 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip + pip install --upgrade --group pylint pip install --upgrade -r CI_REQUIREMENTS.txt - pip install --upgrade "pylint >= 3.0.0" - name: Install develop run: | pip install -e . @@ -93,13 +93,36 @@ jobs: pip install -e . - name: Lint with MyPy run: | - mypy --strict --install-types --non-interactive exec_helpers + mypy -p exec_helpers + + PyRefly: + permissions: + contents: read + pull-requests: read + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + - name: Set up Python + uses: actions/setup-python@v7 + with: + python-version: '3.x' + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install --upgrade --group pyrefly + pip install --upgrade -r CI_REQUIREMENTS.txt + - name: Install develop + run: | + pip install -e . + - name: Lint with PyRefly + run: | + pyrefly check Test: permissions: contents: read pull-requests: read - needs: [PEP8, Formatting, PyLint, MyPy] + needs: [PEP8, Formatting, PyLint, MyPy, PyRefly] runs-on: ${{ matrix.os }} strategy: max-parallel: 6 diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 21398f0..82f9c0a 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -10,7 +10,7 @@ repos: - repo: https://github.com/astral-sh/ruff-pre-commit # Ruff version. - rev: v0.15.13 + rev: v0.17.3 hooks: # Run the linter. - id: ruff-check @@ -29,6 +29,6 @@ repos: - repo: https://github.com/tox-dev/pyproject-fmt # Use the sha / tag you want to point at # or use `pre-commit autoupdate` to get the latest version - rev: "v2.21.2" + rev: "v2.30.3" hooks: - id: pyproject-fmt diff --git a/README.rst b/README.rst index 88e4719..7985b71 100644 --- a/README.rst +++ b/README.rst @@ -403,7 +403,11 @@ Keyword arguments: - cwd - working directory. - env - environment variables dict. -.. note:: `shell=true` is always set. +.. note:: The command is executed by the shell by default. + `Subprocess(shell=False)` (and `async_api.Subprocess(shell=False)`) executes it directly instead: + the command must be a list of arguments (a string raises `TypeError`), + and no shell syntax, expansion or `sh -c` inside a chroot is involved. + `SSHClient` can not do this: the remote side always passes the command to a shell. async_api.Subprocess specific ----------------------------- diff --git a/doc/source/SSHClient.rst b/doc/source/SSHClient.rst index 98f9a3e..1d291a8 100644 --- a/doc/source/SSHClient.rst +++ b/doc/source/SSHClient.rst @@ -10,7 +10,7 @@ API: SSHClient and SSHAuth. SSHClient helper. - .. py:method:: __init__(host, port=22, username=None, password=None, *, auth=None, verbose=True, ssh_config=None, ssh_auth_map=None, sock=None, keepalive=1, allow_ssh_agent=True) + .. py:method:: __init__(host, port=22, username=None, password=None, *, auth=None, verbose=True, ssh_config=None, ssh_auth_map=None, sock=None, keepalive=1, allow_ssh_agent=True, connect_timeout=None) :param host: remote hostname :type host: ``str`` @@ -34,6 +34,9 @@ API: SSHClient and SSHAuth. :type keepalive: int | bool :param allow_ssh_agent: use SSH Agent if available :type allow_ssh_agent: bool + :param connect_timeout: timeout of the TCP connection, the SSH banner, the key exchange and the authentication, + applied to every connection step (a connection attempt is retried 3 times with a 3 s pause) + :type connect_timeout: float | None .. note:: auth has priority over username/password/private_keys .. note:: @@ -52,6 +55,7 @@ API: SSHClient and SSHAuth. .. versionchanged:: 7.0.0 keepalive_mode is removed .. versionchanged:: 7.4.0 return of keepalive_mode to prevent mix with keepalive period. Default is `False` .. versionchanged:: 8.0.0 expose SSH Agent usage override + .. versionchanged:: 8.2.1 added connect_timeout .. py:attribute:: log_mask_re diff --git a/doc/source/conf.py b/doc/source/conf.py index faf1d7a..1b465c9 100644 --- a/doc/source/conf.py +++ b/doc/source/conf.py @@ -1,3 +1,5 @@ +"""Sphinx documentation build configuration.""" + # exec-helpers documentation build configuration file, created by # sphinx-quickstart on Sun Oct 30 13:40:52 2016. # diff --git a/exec_helpers/__init__.py b/exec_helpers/__init__.py index f553f85..d1c1a13 100644 --- a/exec_helpers/__init__.py +++ b/exec_helpers/__init__.py @@ -16,14 +16,13 @@ from __future__ import annotations +import contextlib import importlib import typing import warnings -try: # noqa: SIM105,S314,RUF100 +with contextlib.suppress(ImportError): from ._version import version as __version__ # noqa: F401 -except ImportError: - pass if typing.TYPE_CHECKING: from collections.abc import Sequence diff --git a/exec_helpers/_helpers.py b/exec_helpers/_helpers.py index 617e3aa..367ae07 100644 --- a/exec_helpers/_helpers.py +++ b/exec_helpers/_helpers.py @@ -9,6 +9,7 @@ if typing.TYPE_CHECKING: from collections.abc import Iterable + from collections.abc import Mapping def string_bytes_bytearray_as_bytes(src: str | bytes | bytearray) -> bytes: @@ -96,3 +97,42 @@ def chroot_command(command: str, chroot_path: str | None = None, chroot_exe: str quoted_command = shlex.quote(command) return f"{chroot_exe} {chroot_dst} sh -c {shlex.quote(f'eval {quoted_command}')}" return command + + +def chroot_argv_command(command: str, chroot_path: str | None = None, chroot_exe: str | None = None) -> str: + """Prepare command for chroot execution without a shell, only the arguments are joined. + + :param command: Original command, a result of `shlex.join`. + :type command: str + :param chroot_path: chroot path. + :type chroot_path: str | None + :param chroot_exe: chroot executable, may carry its own arguments. + :type chroot_exe: str | None + :return: Command to be executed with chroot rules if applicable. + :rtype: str + """ + if chroot_path and chroot_path != "/": + return shlex.join([*shlex.split(chroot_exe or "chroot"), chroot_path.strip(), *shlex.split(command)]) + return command + + +def repr_stdin(stdin: bytes | None) -> str: + """Describe STDIN data for a debug string without exposing it. + + :param stdin: Data for STDIN. + :type stdin: bytes | None + :return: Size of the data, as the content can hold secrets. + :rtype: str + """ + return "None" if stdin is None else f"<{len(stdin)} bytes>" + + +def repr_env(env: Mapping[str, str] | None) -> str: + """Describe an environment for a debug string without exposing the values. + + :param env: Process environment. + :type env: Mapping[str, str] | None + :return: Variable names only, as the values can hold secrets. + :rtype: str + """ + return "None" if env is None else f"" diff --git a/exec_helpers/_ssh_base.py b/exec_helpers/_ssh_base.py index 88474b0..67adbc0 100644 --- a/exec_helpers/_ssh_base.py +++ b/exec_helpers/_ssh_base.py @@ -21,7 +21,7 @@ import concurrent.futures import contextlib import copy -import datetime +import datetime as dt import getpass import logging import pathlib @@ -94,6 +94,7 @@ class SshExecuteAsyncResult(api.ExecuteAsyncResult): __slots__ = () @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def interface(self) -> paramiko.Channel: """Override original NamedTuple with proper typing. @@ -103,6 +104,7 @@ def interface(self) -> paramiko.Channel: return super().interface # type: ignore[no-any-return] @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def stdin(self) -> paramiko.channel.ChannelStdinFile: """Override original NamedTuple with proper typing. @@ -117,6 +119,7 @@ def stdin(self) -> paramiko.channel.ChannelStdinFile: return super().stdin # type: ignore[return-value] @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def stderr(self) -> paramiko.channel.ChannelStderrFile | None: """Override original NamedTuple with proper typing. @@ -126,6 +129,7 @@ def stderr(self) -> paramiko.channel.ChannelStderrFile | None: return super().stderr @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def stdout(self) -> paramiko.channel.ChannelFile | None: """Override original NamedTuple with proper typing. @@ -135,7 +139,7 @@ def stdout(self) -> paramiko.channel.ChannelFile | None: return super().stdout -class _SSHExecuteContext(api.ExecuteContext, contextlib.AbstractContextManager[SshExecuteAsyncResult]): +class _SSHExecuteContext(api.ExecuteContext): """SSH Execute context.""" __slots__ = ( @@ -222,8 +226,8 @@ def __repr__(self) -> str: """ return ( f" SshExecuteAsyncResult: stderr = None _stdin: paramiko.channel.ChannelFile with chan.makefile("wb") as _stdin: - started = datetime.datetime.now(tz=datetime.UTC) + started = dt.datetime.now(tz=dt.UTC) if self.__sudo_mode: chan.exec_command(self.command) # nosec # Sanitize on caller side if not stdout.channel.closed: @@ -333,13 +337,8 @@ def __enter__(self) -> SshExecuteAsyncResult: res_stdout = None # noinspection PyArgumentList - return SshExecuteAsyncResult( - interface=chan, - stdin=_stdin, - stderr=stderr, - stdout=res_stdout, - started=started, - ) + # The fields are typed by properties of the subclass, which type checkers do not accept as constructor arguments + return SshExecuteAsyncResult._make((chan, _stdin, stderr, res_stdout, started)) def __exit__( self, @@ -458,6 +457,13 @@ class SSHClientBase(api.ExecHelper): :type allow_ssh_agent: bool :param missing_host_key_policy: Missing host key policy class. See paramiko.MissingHostKeyPolicy for details. :type missing_host_key_policy: type[paramiko.MissingHostKeyPolicy] + :param connect_timeout: Timeout in seconds for the TCP connection, the SSH banner, the key exchange and the + authentication of every connection step, including hosts reached through `proxy_to`. + An authentication timeout is reported as an authentication failure. By default: None + (paramiko defaults: no TCP and key exchange limit, banner 15 s, authentication 30 s). A connection attempt + is retried 3 times with a 3 s pause, so a hung host takes up to 3 times the timeout plus 6 s to report. + :type connect_timeout: float | None + :raises ValueError: connect_timeout is not positive. .. note:: auth has priority over username/password/private_keys. .. note:: @@ -477,12 +483,16 @@ class SSHClientBase(api.ExecHelper): .. versionchanged:: 7.4.0 Return of keepalive_mode to prevent mix with a keepalive period. Default is `False`. .. versionchanged:: 8.0.0 Expose SSH Agent usage override. .. versionchanged:: 8.1.4 Allow specifying missing_host_key_policy. + .. versionchanged:: 8.2.1 Added connect_timeout. + .. versionchanged:: 8.2.1 Direct connection to the local host accepts the host keys of this machine without + the unknown host key warning; a key of another server is handled by the missing host key policy as before. """ __slots__ = ( "__allow_agent", "__auth_mapping", "__conn_chain", + "__connect_timeout", "__hostname", "__keepalive_mode", "__keepalive_period", @@ -519,14 +529,18 @@ def __init__( keepalive: KeepAlivePeriodT = 1, allow_ssh_agent: bool = True, missing_host_key_policy: type[paramiko.MissingHostKeyPolicy] = paramiko.WarningPolicy, + connect_timeout: float | None = None, ) -> None: - """Main SSH Client helper.""" + """Initialize the SSH client helper.""" self.__sudo_mode = False self.__keepalive_period: int = int(keepalive) self.__keepalive_mode = False self.__verbose: bool = verbose self.__sock = sock self._missing_host_key_policy = missing_host_key_policy + if connect_timeout is not None and connect_timeout <= 0: + raise ValueError(f"connect_timeout should be positive, got {connect_timeout!r}") + self.__connect_timeout = connect_timeout self.__ssh: paramiko.SSHClient self.__sftp: paramiko.SFTPClient | None = None @@ -749,14 +763,30 @@ def _ssh(self) -> paramiko.SSHClient: """ return self.__ssh + def __connect(self) -> None: + """Open the connection, with retries unless a socket was supplied. + + A failed attempt closes the supplied socket, so there is nothing to retry on: + the next attempt would only fail on the closed socket and hide the original error. + """ + with self.lock: + if self.__sock is None: + self.__connect_with_retries() + else: + self.__connect_once() + @tenacity.retry( retry=RetryOnExceptions(retry_on=paramiko.SSHException, reraise=paramiko.AuthenticationException), stop=tenacity.stop.stop_after_attempt(3), wait=tenacity.wait.wait_fixed(3), reraise=True, ) - def __connect(self) -> None: - """Main method for connection open.""" + def __connect_with_retries(self) -> None: + """Open the connection, retry on the connection errors.""" + self.__connect_once() + + def __connect_once(self) -> None: + """Open the connection, one attempt.""" with self.lock: if self.__sock is not None: self.__ssh = paramiko.SSHClient() @@ -769,6 +799,7 @@ def __connect(self) -> None: log=self.__verbose, sock=self.__sock, allow_ssh_agent=self.allow_ssh_agent, + timeout=self.__connect_timeout, ) else: self.__ssh = self.__get_client() @@ -776,6 +807,27 @@ def __connect(self) -> None: transport: paramiko.Transport = self._ssh_transport transport.set_keepalive(1 if self.__keepalive_period else 0) # send keepalive packets + def __host_key_policy(self, hostname: str) -> paramiko.MissingHostKeyPolicy | type[paramiko.MissingHostKeyPolicy]: + """Select the missing host key policy for a host which is connected to directly. + + :param hostname: Host to connect to, without a proxy or a jump host. + :type hostname: str + :return: Configured policy; for the local host a policy, which accepts the keys of this machine first. + :rtype: paramiko.MissingHostKeyPolicy | type[paramiko.MissingHostKeyPolicy] + + The local host is normally absent from `known_hosts`, so its own keys are the reference. A key of another + server (any local address can be a container, a tunnel or another server) goes to the configured policy as + before, so nothing which worked fails because of it. Without readable keys the configured policy decides. + The own key is accepted without asking the configured policy, so that policy does not see it. + """ + try: + keys = _ssh_helpers.local_host_keys() if _ssh_helpers.is_local_host(hostname) else () + except OSError: # the answer only silences a warning, so a failure of the lookup must not fail the connection + keys = () + if keys: + return _ssh_helpers.LocalHostKeyPolicy(keys, fallback=self._missing_host_key_policy) + return self._missing_host_key_policy + def __get_client(self) -> paramiko.SSHClient: """Connect using connection chain information. @@ -785,12 +837,33 @@ def __get_client(self) -> paramiko.SSHClient: :raises RuntimeError: Unexpected state. :raises ConnectionError: Cannot get SSH transport. """ + opened: list[paramiko.SSHClient] = [] + try: + return self.__connect_chain(opened) + except BaseException: + for client in reversed(opened): # paramiko leaves the transports of a failed connect running + client.close() + raise + + def __connect_chain(self, opened: list[paramiko.SSHClient]) -> paramiko.SSHClient: + """Connect the hosts of the connection chain one by one. + + :param opened: Every client created here is added to the list at once, for the cleanup on failure. + :type opened: list[paramiko.SSHClient] + :return: Paramiko ssh connection object of the last host. + :rtype: paramiko.SSHClient + :raises ValueError: ProxyCommand found in a connection chain after the first host reached an. + :raises RuntimeError: Unexpected state. + :raises ConnectionError: Cannot get SSH transport. + """ + config, auth = self.__conn_chain[0] last_ssh_client: paramiko.SSHClient = paramiko.SSHClient() + opened.append(last_ssh_client) last_ssh_client.load_system_host_keys() - last_ssh_client.set_missing_host_key_policy(self._missing_host_key_policy) - - config, auth = self.__conn_chain[0] + last_ssh_client.set_missing_host_key_policy( + self._missing_host_key_policy if config.proxycommand else self.__host_key_policy(config.hostname) + ) auth.connect( last_ssh_client, @@ -798,10 +871,12 @@ def __get_client(self) -> paramiko.SSHClient: port=config.port or 22, sock=paramiko.ProxyCommand(config.proxycommand) if config.proxycommand else None, allow_ssh_agent=self.allow_ssh_agent, + timeout=self.__connect_timeout, ) for config, auth in self.__conn_chain[1:]: # start has another logic, so do it out of cycle ssh = paramiko.SSHClient() + opened.append(ssh) ssh.load_system_host_keys() ssh.set_missing_host_key_policy(self._missing_host_key_policy) @@ -820,6 +895,7 @@ def __get_client(self) -> paramiko.SSHClient: port=config.port or 22, sock=sock, allow_ssh_agent=self.allow_ssh_agent, + timeout=self.__connect_timeout, ) last_ssh_client = ssh continue @@ -831,10 +907,11 @@ def __get_client(self) -> paramiko.SSHClient: return last_ssh_client def __connect_sftp(self) -> None: - """SFTP connection opener.""" + """Open the SFTP channel, reconnecting the SSH session first if it was closed.""" with self.lock: + transport = self._ssh_transport # a failed reconnect is not "SFTP unavailable": let it propagate try: - self.__sftp = self.__ssh.open_sftp() + self.__sftp = transport.open_sftp_client() except paramiko.SSHException: self.logger.warning("SFTP enable failed! SSH only is accessible.") @@ -844,6 +921,11 @@ def _sftp(self) -> paramiko.sftp_client.SFTPClient: :rtype: paramiko.sftp_client.SFTPClient :raises paramiko.SSHException: SFTP connection failed. + :raises ConnectionError: SSH session was closed and the reconnect failed. + :raises paramiko.SSHException: SSH session was closed and the reconnect failed (the original exception). + + .. versionchanged:: 8.2.1 The SFTP channel is opened through the reconnecting transport getter, + so a closed SSH session is restored instead of failing with `AttributeError`. """ if self.__sftp is not None: return self.__sftp @@ -877,7 +959,7 @@ def __del__(self) -> None: """ try: self.__ssh.close() - except BaseException as e: # pragma: no cover # NOSONAR + except BaseException as e: # noqa: BLE001 # pragma: no cover # NOSONAR with contextlib.suppress(AttributeError): self.logger.debug(f"Exception in {self!s} destructor call: {e}") self.__sftp = None @@ -908,7 +990,7 @@ def sudo_mode(self) -> bool: @sudo_mode.setter def sudo_mode(self, mode: bool) -> None: - """Persistent sudo mode change for connection object. + """Change the persistent sudo mode of the connection object. :param mode: Sudo status: enabled | disabled. :type mode: bool @@ -1061,7 +1143,7 @@ def poll_streams() -> None: result.read_stderr(src=async_result.stderr, log=self.logger if log_stderr else None, verbose=verbose) def poll_pipes() -> None: - """Polling task for FIFO buffers.""" + """Poll FIFO buffers.""" while not async_result.interface.status_event.is_set(): time.sleep(0.1) if async_result.stdout or async_result.stderr: @@ -1155,6 +1237,7 @@ def open_execute_context( sudo_mode=self.sudo_mode, auth=self.auth, logger=self.logger, + log_mask_re=self.log_mask_re, **kwargs, ) @@ -1564,6 +1647,7 @@ def proxy_to( sock=sock, ssh_auth_map=ssh_auth_map if ssh_auth_map is not None else self.__auth_mapping, keepalive=int(keepalive), + connect_timeout=self.__connect_timeout, ) def execute_through_host( @@ -1787,7 +1871,7 @@ def get_result(remote: SSHClientBase) -> exec_result.ExecResult: results[remote.hostname, remote.port] = result if result.exit_code not in prep_expected: errors[remote.hostname, remote.port] = result - except Exception as e: + except Exception as e: # noqa: BLE001 # stored and reported to the caller raised_exceptions[remote.hostname, remote.port] = e if raised_exceptions: # always raise diff --git a/exec_helpers/_ssh_helpers.py b/exec_helpers/_ssh_helpers.py index 5f0f981..a543c20 100644 --- a/exec_helpers/_ssh_helpers.py +++ b/exec_helpers/_ssh_helpers.py @@ -3,16 +3,23 @@ from __future__ import annotations import functools +import ipaddress import pathlib +import socket import typing from collections.abc import Collection +from collections.abc import Iterable import paramiko +import psutil if typing.TYPE_CHECKING: # noinspection PyPackageRequirements import logwrap +LOCAL_HOST_KEYS_PATTERN = "ssh_host_*_key.pub" +LOCAL_HOST_KEYS_DIR = pathlib.Path("/etc/ssh") + SSHConfigDictLikeT = dict[str, str | int | bool | Collection[str]] SSHConfigsDictT = dict[str, SSHConfigDictLikeT] @@ -38,12 +45,100 @@ def _parse_ssh_config_file(file_path: pathlib.Path) -> paramiko.SSHConfig | None config = paramiko.SSHConfig() with file_path.open() as f_obj: config.parse(f_obj) - except Exception: + except Exception: # noqa: BLE001 # any parse failure means no usable config config = None return config +def is_local_host(hostname: str) -> bool: + """Check if the hostname points to the machine we are running on. + + :param hostname: Host name or IP address from the connection parameters. + :type hostname: str + :return: Loopback address, `localhost`, own host name or an address of a local interface. + :rtype: bool + + No name resolution is done: a name is local only if it is the own host name or its short form. + A link-local address without a scope id is ambiguous (any link has such addresses), so it is not local. + """ + name = hostname.strip("[]").lower() + try: + address = ipaddress.ip_address(name.partition("%")[0]) + except ValueError: + own_name = socket.gethostname().lower() + return name in {"localhost", own_name, own_name.partition(".")[0]} + + if address.is_loopback: + return True + if address.is_link_local and "%" not in name: + return False + try: + return any( + address == ipaddress.ip_address(addr.address.partition("%")[0]) + for addrs in psutil.net_if_addrs().values() + for addr in addrs + if addr.family in {socket.AF_INET, socket.AF_INET6} + ) + except (OSError, ValueError): # the answer only silences a warning: no verdict means "not local" + return False + + +def local_host_keys() -> tuple[paramiko.PKey, ...]: + """Read the public host keys of the SSH server of this machine. + + :return: Parsed keys, empty if there are none or they are not readable. + :rtype: tuple[paramiko.PKey, ...] + + The files are read on every call: a key rotation must not leave stale keys, and the read costs less than + the connection itself. + """ + keys: list[paramiko.PKey] = [] + for key_file in sorted(LOCAL_HOST_KEYS_DIR.glob(LOCAL_HOST_KEYS_PATTERN)): + try: + # known_hosts line format: " " + entry = paramiko.hostkeys.HostKeyEntry.from_line(f"local {key_file.read_text(encoding='ascii')}") + except (OSError, ValueError, paramiko.SSHException, paramiko.hostkeys.InvalidHostKey): + continue + if entry is not None and entry.key is not None: + keys.append(entry.key) + return tuple(keys) + + +class LocalHostKeyPolicy(paramiko.MissingHostKeyPolicy): + """Missing host key policy for the local host: accept the host keys of this machine, ask the fallback otherwise. + + :param keys: Public host keys of the SSH server of this machine. + :type keys: collections.abc.Iterable[paramiko.PKey] + :param fallback: Policy for any other key, so a server which is not this machine is treated as it was before. + :type fallback: paramiko.MissingHostKeyPolicy | type[paramiko.MissingHostKeyPolicy] + """ + + __slots__ = ("__fallback", "__keys") + + def __init__( + self, + keys: Iterable[paramiko.PKey], + fallback: paramiko.MissingHostKeyPolicy | type[paramiko.MissingHostKeyPolicy], + ) -> None: + """Remember the keys and the fallback policy.""" + self.__keys = frozenset(key.asbytes() for key in keys) + self.__fallback = fallback() if isinstance(fallback, type) else fallback + + def missing_host_key(self, client: paramiko.SSHClient, hostname: str, key: paramiko.PKey) -> None: + """Accept the key of this machine, ask the fallback policy about any other key. + + :param client: SSH client (low level). + :type client: paramiko.SSHClient + :param hostname: Name of the host in `known_hosts` format. + :type hostname: str + :param key: Key offered by the server. + :type key: paramiko.PKey + """ + if key.asbytes() not in self.__keys: + self.__fallback.missing_host_key(client, hostname, key) + + class SSHConfig: """Parsed SSH Config for creation connection.""" diff --git a/exec_helpers/api.py b/exec_helpers/api.py index 2b8f4d9..3502608 100644 --- a/exec_helpers/api.py +++ b/exec_helpers/api.py @@ -40,7 +40,7 @@ from . import _helpers if typing.TYPE_CHECKING: - import datetime + import datetime as dt from collections.abc import Sequence from types import TracebackType from typing import Self @@ -74,7 +74,7 @@ class ExecuteAsyncResult(typing.NamedTuple): stdin: typing.Any | None stderr: typing.Any | None stdout: typing.Any | None - started: datetime.datetime + started: dt.datetime class ExecuteContext(contextlib.AbstractContextManager[ExecuteAsyncResult], abc.ABC): @@ -82,6 +82,7 @@ class ExecuteContext(contextlib.AbstractContextManager[ExecuteAsyncResult], abc. __slots__ = ( "__command", + "__log_mask_re", "__logger", "__open_stderr", "__open_stdout", @@ -96,6 +97,7 @@ def __init__( open_stdout: bool = True, open_stderr: bool = True, logger: logging.Logger, + log_mask_re: LogMaskReT = None, **kwargs: typing.Any, ) -> None: """Execute async context manager. @@ -110,6 +112,8 @@ def __init__( :type open_stderr: bool :param logger: Logger instance. :type logger: logging.Logger + :param log_mask_re: Regex lookup rule to mask the command in the debug string. + :type log_mask_re: str | re.Pattern[str] | None :param kwargs: Additional parameters for call. :type kwargs: typing.Any """ @@ -118,6 +122,7 @@ def __init__( self.__open_stdout = open_stdout self.__open_stderr = open_stderr self.__logger = logger + self.__log_mask_re = log_mask_re if kwargs: self.__logger.warning(f"Unexpected arguments: {kwargs!r}.", stack_info=True) @@ -130,6 +135,15 @@ def logger(self) -> logging.Logger: """ return self.__logger + @property + def _masked_command(self) -> str: + """Command with the log mask applied, for debug output. + + :return: Command without the masked parts. + :rtype: str + """ + return _helpers.mask_command(self.__command.rstrip(), self.__log_mask_re) + @property def command(self) -> str: """Command for execution (fully formatted). @@ -310,7 +324,7 @@ def _chroot_path(self) -> None: @property def _chroot_exe(self) -> str | None: - """Exe for chroot + """Return the exe for chroot. :rtype: str | None .. versionadded:: 8.1.0 @@ -319,7 +333,7 @@ def _chroot_exe(self) -> str | None: @_chroot_exe.setter def _chroot_exe(self, new_state: str | None) -> None: - """Executable for chroot if set. + """Set the executable for chroot. :param new_state: New exe. :type new_state: str | None @@ -381,6 +395,16 @@ def __exit__( self.__context_count -= 1 self.lock.release() + def _command_to_string(self, command: CommandT) -> str: + """Convert the command to the string form used for execution and logs. + + :param command: Original command. + :type command: str | Iterable[str] + :return: Command as a single string. + :rtype: str + """ + return _helpers.cmd_to_string(command) + def _mask_command(self, cmd: str, log_mask_re: LogMaskReT = None) -> str: """Log command with masking and return parsed cmd. @@ -394,7 +418,6 @@ def _mask_command(self, cmd: str, log_mask_re: LogMaskReT = None) -> str: .. versionadded:: 1.2.0 """ - return _helpers.mask_command(cmd.rstrip(), self.log_mask_re, log_mask_re) def _prepare_command(self, cmd: str, chroot_path: str | None = None, chroot_exe: str | None = None) -> str: @@ -557,7 +580,7 @@ def execute( .. versionchanged:: 8.1.0 chroot_exe added. """ log_level: int = logging.INFO if verbose else logging.DEBUG - cmd = _helpers.cmd_to_string(command) + cmd = self._command_to_string(command) self._log_command_execute( command=cmd, log_mask_re=log_mask_re, @@ -588,7 +611,7 @@ def execute( self.logger.log(level=log_level, msg=f"Command {result.cmd!r} exit code: {result.exit_code!s}") return result - def __call__( + def __call__( # pylint: disable=arguments-differ # Callable[..., X] declares no fixed signature self, command: CommandT, verbose: bool = False, @@ -823,7 +846,7 @@ def _handle_stderr( expected: ExpectedExitCodesT, exception_class: CalledProcessErrorSubClassT, ) -> exec_result.ExecResult: - """Internal check_stderr logic (synchronous). + """Check STDERR of the result (synchronous). :param result: Execution result for validation. :type result: exec_result.ExecResult diff --git a/exec_helpers/async_api/api.py b/exec_helpers/async_api/api.py index 5a8d424..3f2ad36 100644 --- a/exec_helpers/async_api/api.py +++ b/exec_helpers/async_api/api.py @@ -69,6 +69,7 @@ class ExecuteContext(contextlib.AbstractAsyncContextManager[api.ExecuteAsyncResu __slots__ = ( "__command", + "__log_mask_re", "__logger", "__open_stderr", "__open_stdout", @@ -83,6 +84,7 @@ def __init__( open_stdout: bool = True, open_stderr: bool = True, logger: logging.Logger, + log_mask_re: LogMaskReT = None, **kwargs: typing.Any, ) -> None: """Execute async context manager. @@ -97,6 +99,8 @@ def __init__( :type open_stderr: bool :param logger: Logger instance. :type logger: logging.Logger + :param log_mask_re: Regex lookup rule to mask the command in the debug string. + :type log_mask_re: str | re.Pattern[str] | None :param kwargs: Additional parameters for call. :type kwargs: typing.Any """ @@ -105,6 +109,7 @@ def __init__( self.__open_stdout = open_stdout self.__open_stderr = open_stderr self.__logger = logger + self.__log_mask_re = log_mask_re if kwargs: self.__logger.warning(f"Unexpected arguments: {kwargs!r}.", stack_info=True) @@ -117,6 +122,15 @@ def logger(self) -> logging.Logger: """ return self.__logger + @property + def _masked_command(self) -> str: + """Command with the log mask applied, for debug output. + + :return: Command without the masked parts. + :rtype: str + """ + return _helpers.mask_command(self.__command.rstrip(), self.__log_mask_re) + @property def command(self) -> str: """Command for execution (fully formatted). @@ -272,7 +286,7 @@ def _chroot_path(self) -> None: @property def _chroot_exe(self) -> str | None: - """Exe for chroot + """Return the exe for chroot. :rtype: str | None .. versionadded:: 8.1.0 @@ -281,7 +295,7 @@ def _chroot_exe(self) -> str | None: @_chroot_exe.setter def _chroot_exe(self, new_state: str | None) -> None: - """Executable for chroot if set. + """Set the executable for chroot. :param new_state: New exe. :type new_state: str | None @@ -337,6 +351,16 @@ async def __aexit__( if self.__alock is not None: self.__alock.release() + def _command_to_string(self, command: CommandT) -> str: + """Convert the command to the string form used for execution and logs. + + :param command: Original command. + :type command: str | Iterable[str] + :return: Command as a single string. + :rtype: str + """ + return _helpers.cmd_to_string(command) + def _mask_command(self, cmd: str, log_mask_re: LogMaskReT = None) -> str: """Log command with masking and return parsed cmd. @@ -350,7 +374,6 @@ def _mask_command(self, cmd: str, log_mask_re: LogMaskReT = None) -> str: .. versionadded:: 1.2.0 """ - return _helpers.mask_command(cmd.rstrip(), self.log_mask_re, log_mask_re) def _prepare_command(self, cmd: str, chroot_path: str | None = None, chroot_exe: str | None = None) -> str: @@ -510,7 +533,7 @@ async def execute( .. versionchanged:: 8.1.0 chroot_exe added. """ log_level: int = logging.INFO if verbose else logging.DEBUG - cmd = _helpers.cmd_to_string(command) + cmd = self._command_to_string(command) self._log_command_execute( command=cmd, log_mask_re=log_mask_re, @@ -541,7 +564,8 @@ async def execute( self.logger.log(level=log_level, msg=f"Command {result.cmd!r} exit code: {result.exit_code!s}") return result - async def __call__( # pylint: disable=invalid-overridden-method + # Callable[..., X] declares no fixed signature, so arguments-differ is a false positive. + async def __call__( # pylint: disable=invalid-overridden-method,arguments-differ self, command: CommandT, verbose: bool = False, @@ -772,7 +796,7 @@ def _handle_stderr( expected: ExpectedExitCodesT, exception_class: CalledProcessErrorSubClassT, ) -> exec_result.ExecResult: - """Internal check_stderr logic (synchronous). + """Check STDERR of the result (synchronous). :param result: Execution result for validation. :type result: exec_result.ExecResult diff --git a/exec_helpers/async_api/exec_result.py b/exec_helpers/async_api/exec_result.py index a982053..9a07a3c 100644 --- a/exec_helpers/async_api/exec_result.py +++ b/exec_helpers/async_api/exec_result.py @@ -26,7 +26,7 @@ if typing.TYPE_CHECKING: from collections.abc import AsyncIterable -from exec_helpers import exec_result # pylint: disable=wrong-import-position +from exec_helpers import exec_result __all__ = ("ExecResult",) diff --git a/exec_helpers/async_api/subprocess.py b/exec_helpers/async_api/subprocess.py index 2aa8f94..facc194 100644 --- a/exec_helpers/async_api/subprocess.py +++ b/exec_helpers/async_api/subprocess.py @@ -22,10 +22,11 @@ import asyncio import contextlib import copy -import datetime +import datetime as dt import errno import logging import os +import shlex import typing import warnings @@ -36,6 +37,7 @@ from exec_helpers.async_api import api from exec_helpers.async_api import exec_result +from .. import _helpers from .. import _log_templates from .. import _subprocess_helpers @@ -106,12 +108,10 @@ def stdout(self) -> AsyncIterable[bytes] | None: # type: ignore[override] return super().stdout # type: ignore[return-value] -class _SubprocessExecuteContext( - api.ExecuteContext, contextlib.AbstractAsyncContextManager[SubprocessExecuteAsyncResult] -): +class _SubprocessExecuteContext(api.ExecuteContext): """Subprocess Execute context.""" - __slots__ = ("__cwd", "__env", "__process") + __slots__ = ("__cwd", "__env", "__process", "__shell") def __init__( self, @@ -122,6 +122,7 @@ def __init__( open_stderr: bool = True, cwd: CwdT = None, env: EnvT = None, + use_shell: bool = True, logger: logging.Logger, **kwargs: typing.Any, ) -> None: @@ -139,6 +140,9 @@ def __init__( :type cwd: str | bytes | pathlib.Path | None :param env: Defines the environment variables for the new process. :type env: Mapping[str | bytes, str | bytes] | None + :param use_shell: Run the command by the shell. + Otherwise, the command is split to arguments and executed directly. + :type use_shell: bool :param logger: Logger instance. :type logger: logging.Logger :param kwargs: Additional parameters for call. @@ -154,6 +158,7 @@ def __init__( ) self.__cwd = cwd self.__env = env + self.__shell = use_shell self.__process: asyncio.subprocess.Process | None = None def __repr__(self) -> str: @@ -164,12 +169,13 @@ def __repr__(self) -> str: """ return ( f"" ) @@ -183,18 +189,22 @@ async def __aenter__(self) -> SubprocessExecuteAsyncResult: The Command is executed only in the context manager to be sure that everything will be cleaned up properly. """ - started = datetime.datetime.now(tz=datetime.UTC) - - self.__process = await asyncio.create_subprocess_shell( - cmd=self.command, - stdout=asyncio.subprocess.PIPE if self.open_stdout else asyncio.subprocess.DEVNULL, - stderr=asyncio.subprocess.PIPE if self.open_stderr else asyncio.subprocess.DEVNULL, - stdin=asyncio.subprocess.PIPE, - cwd=self.__cwd, - env=self.__env, - universal_newlines=False, - **_subprocess_helpers.subprocess_kw, - ) + started = dt.datetime.now(tz=dt.UTC) + + kwargs: dict[str, typing.Any] = { + "stdout": asyncio.subprocess.PIPE if self.open_stdout else asyncio.subprocess.DEVNULL, + "stderr": asyncio.subprocess.PIPE if self.open_stderr else asyncio.subprocess.DEVNULL, + "stdin": asyncio.subprocess.PIPE, + "cwd": self.__cwd, + "env": self.__env, + "universal_newlines": False, + } + kwargs.update(_subprocess_helpers.subprocess_kw) + # Running a shell command line is the default mode of this class, `shell=False` is the opt-out. + if self.__shell: + self.__process = await asyncio.create_subprocess_shell(self.command, **kwargs) + else: + self.__process = await asyncio.create_subprocess_exec(*shlex.split(self.command), **kwargs) process = self.__process if self.stdin is not None: @@ -219,13 +229,8 @@ async def __aenter__(self) -> SubprocessExecuteAsyncResult: raise # noinspection PyArgumentList - return SubprocessExecuteAsyncResult( - interface=process, - stdin=None, - stderr=process.stderr, - stdout=process.stdout, - started=started, - ) + # The fields are typed by properties of the subclass, which type checkers do not accept as constructor arguments + return SubprocessExecuteAsyncResult._make((process, None, process.stderr, process.stdout, started)) async def __aexit__( self, @@ -249,24 +254,77 @@ class Subprocess(api.ExecHelper): :type log_mask_re: str | re.Pattern[str] | None :param logger: Logger instance to use. :type logger: logging.Logger + :param shell: Run commands by the shell. If disabled, commands are accepted only as a list of arguments and + are executed directly, so no shell syntax or expansion is applied to them (chroot included). + The low level `open_execute_context` takes the already joined command and splits it with `shlex`. + :type shell: bool .. versionchanged:: 3.1.0 Not singleton anymore. Only lock is shared between all instances. .. versionchanged:: 3.2.0 Logger can be enforced. .. versionchanged:: 4.1.0 Support chroot .. versionchanged:: 4.3.0 Lock is not shared anymore: allow parallel call of different instances. + .. versionchanged:: 8.2.1 Added shell option: commands can be executed without a shell. """ - __slots__ = () + __slots__ = ("__shell",) def __init__( self, log_mask_re: LogMaskReT = None, *, logger: logging.Logger = logging.getLogger(__name__), # noqa: B008 + shell: bool = True, ) -> None: """Subprocess helper with timeouts and lock-free FIFO.""" + self.__shell = shell super().__init__(logger=logger, log_mask_re=log_mask_re) + @property + def shell(self) -> bool: + """Run commands by the shell. + + :return: False if commands are executed directly, without a shell. + :rtype: bool + """ + return self.__shell + + def _command_to_string(self, command: CommandT) -> str: + """Convert the command to the string form used for execution and logs. + + :param command: Original command. + :type command: str | Iterable[str] + :return: Command as a single string. + :rtype: str + :raises TypeError: Command is a string, but shell is disabled. + :raises ValueError: Command has no arguments, but shell is disabled. + """ + if not self.__shell and isinstance(command, str): + raise TypeError("Without a shell the command is expected as a list of arguments, not as a string") + cmd = super()._command_to_string(command) + if not self.__shell and not cmd: + raise ValueError("Without a shell the command needs at least the program to execute") + return cmd + + def _prepare_command(self, cmd: str, chroot_path: str | None = None, chroot_exe: str | None = None) -> str: + """Prepare command: cover chroot and other cases. + + :param cmd: Main command. + :type cmd: str + :param chroot_path: Path to make chroot for execution. + :type chroot_path: str | None + :param chroot_exe: chroot exe override. + :type chroot_exe: str | None + :return: Final command, includes chroot, if required. + :rtype: str + """ + if self.__shell: + return super()._prepare_command(cmd, chroot_path=chroot_path, chroot_exe=chroot_exe) + return _helpers.chroot_argv_command( + cmd, + chroot_path=chroot_path or self._chroot_path, + chroot_exe=chroot_exe or self._chroot_exe, + ) + async def _exec_command( # type: ignore[override] self, command: str, @@ -400,7 +458,9 @@ def open_execute_context( open_stderr=open_stderr, cwd=cwd, env=env, + use_shell=self.__shell, logger=self.logger, + log_mask_re=self.log_mask_re, **kwargs, ) diff --git a/exec_helpers/exec_result.py b/exec_helpers/exec_result.py index 832cf4f..ddac9c8 100644 --- a/exec_helpers/exec_result.py +++ b/exec_helpers/exec_result.py @@ -19,7 +19,7 @@ from __future__ import annotations import contextlib -import datetime +import datetime as dt import functools import json import logging @@ -68,13 +68,13 @@ def _handle_deserialize( fmt: str, ) -> Callable[[Callable[[ExecResult], _T]], Callable[[ExecResult], _T]]: - """Decorator fabric for decoder getters. + """Create a decorator for decoder getters. :return: real decorator """ def decorator(method: Callable[[ExecResult], _T]) -> Callable[[ExecResult], _T]: - """Decorator for decoder getter. + """Decorate a decoder getter. :return: Wrapped to try/except getter. """ @@ -89,11 +89,11 @@ def wrapper(self: ExecResult) -> _T: return method(self) except Exception as exc: tmpl: str = f"{{self.cmd}} stdout is not valid {fmt}:\n{{stdout!r}}\n" - LOGGER.exception(tmpl.format(self=self, stdout=self.stdout_str)) + LOGGER.exception(tmpl.format_map({"self": self, "stdout": self.stdout_str})) - raise exceptions.DeserializeValueError(tmpl.format(self=self, stdout=self.stdout_brief)).with_traceback( - exc.__traceback__ - ) from exc + raise exceptions.DeserializeValueError( + tmpl.format_map({"self": self, "stdout": self.stdout_brief}) + ).with_traceback(exc.__traceback__) from exc return wrapper @@ -167,7 +167,7 @@ def __getitem__( raise TypeError(f"Unexpected key type: {item!r}") def __len__(self) -> int: # pragma: no cover - """Data len. + """Return the data length. :return: Strings count. :rtype: int @@ -218,7 +218,7 @@ def __init__( stderr: Iterable[bytes] | None = None, exit_code: ExitCodeT = proc_enums.INVALID, *, - started: datetime.datetime | None = None, + started: dt.datetime | None = None, ) -> None: """Command execution result. @@ -257,10 +257,10 @@ def __init__( self._stderr = () self.__exit_code: ExitCodeT = proc_enums.INVALID - self.__timestamp: datetime.datetime | None = None + self.__timestamp: dt.datetime | None = None self.exit_code = exit_code - self.__started: datetime.datetime | None = started + self.__started: dt.datetime | None = started # By default is none: self._stdout_str: str | None = None @@ -291,7 +291,7 @@ def stderr_lock(self) -> threading.RLock: return self.__stderr_lock @property - def timestamp(self) -> datetime.datetime | None: + def timestamp(self) -> dt.datetime | None: """Timestamp. :return: Exit code timestamp. @@ -307,7 +307,7 @@ def set_timestamp(self) -> None: .. versionadded:: 4.0.0 """ if self.timestamp is None: - self.__timestamp = datetime.datetime.now(tz=datetime.UTC) + self.__timestamp = dt.datetime.now(tz=dt.UTC) @classmethod def _get_brief(cls, data: tuple[bytes, ...]) -> str: @@ -558,7 +558,7 @@ def exit_code(self, new_val: ExitCodeT) -> None: with self.stdout_lock, self.stderr_lock: self.__exit_code = proc_enums.exit_code_to_enum(new_val) if self.__exit_code != proc_enums.INVALID: - self.__timestamp = datetime.datetime.now(tz=datetime.UTC) + self.__timestamp = dt.datetime.now(tz=dt.UTC) @property def ok(self) -> bool: @@ -627,7 +627,7 @@ def raise_for_status( raise exception_class(self, expected_codes) @property - def started(self) -> datetime.datetime | None: + def started(self) -> dt.datetime | None: """Timestamp of command start. :return: Timestamp from command start, if applicable. @@ -666,7 +666,7 @@ def stdout_yaml(self) -> typing.Any: if yaml.__with_libyaml__: # pragma: no cover return yaml.load(self.stdout_str, Loader=yaml.CSafeLoader) # nosec # Safe return yaml.safe_load(self.stdout_str) # pragma: no cover - return ruamel_yaml.YAML(typ="safe").load(self.stdout_str) # nosec # Safe + return ruamel_yaml.YAML(typ="safe").load(self.stdout_str) # nosec # pyrefly: ignore[missing-attribute] @property @_handle_deserialize("xml") @@ -694,7 +694,7 @@ def stdout_lxml(self) -> lxml.etree.Element: .. note:: Can be insecure. """ with self.stdout_lock: - return lxml.etree.fromstring(b"".join(self.stdout)) # nosec[B314] + return lxml.etree.fromstring(b"".join(self.stdout)) # nosec[B314] # pyrefly: ignore[missing-attribute] def __dir__(self) -> list[str]: """Override dir for IDE and as source for getitem checks. @@ -726,7 +726,7 @@ def __dir__(self) -> list[str]: return content def __getitem__(self, item: str) -> typing.Any: - """Dict like get data. + """Get data like from a dict. :param item: Key. :type item: str diff --git a/exec_helpers/ssh_auth.py b/exec_helpers/ssh_auth.py index e562f8b..d405fc7 100644 --- a/exec_helpers/ssh_auth.py +++ b/exec_helpers/ssh_auth.py @@ -115,7 +115,7 @@ def username(self) -> str | None: @staticmethod def __get_public_key(key: paramiko.PKey | None) -> str | None: - """Internal method for get public key from private. + """Get the public key from the private one. :param key: SSH private key. :type key: paramiko.PKey @@ -165,6 +165,7 @@ def connect( *, sock: paramiko.ProxyCommand | paramiko.Channel | socket.socket | None = None, allow_ssh_agent: bool = True, + timeout: float | None = None, ) -> None: """Connect the SSH client object using credentials. @@ -180,8 +181,13 @@ def connect( :type sock: paramiko.ProxyCommand | paramiko.Channel | socket.socket | None :param allow_ssh_agent: Use SSH Agent if available. :type allow_ssh_agent: bool + :param timeout: Timeout in seconds for the TCP connection, the SSH banner and the authentication. + :type timeout: float | None :raises PasswordRequiredException: No password has been set, but required. - :raises AuthenticationException: Authentication failed. + :raises AuthenticationException: Authentication failed (an authentication timeout is reported the same way). + :raises TimeoutError: TCP connection timeout. + :raises paramiko.SSHException: Banner or key exchange timeout. + :raises BaseException: Any other failure of a connection attempt; the client is closed first. """ kwargs: dict[str, typing.Any] = {} @@ -189,6 +195,8 @@ def connect( kwargs["passphrase"] = self.__passphrase if sock is not None: kwargs["sock"] = sock + if timeout is not None: + kwargs.update(timeout=timeout, banner_timeout=timeout, auth_timeout=timeout) for index, key in sorted(enumerate(self.__keys), key=lambda i_k: i_k[0] != self.__key_index): kwargs["pkey"] = key @@ -207,13 +215,20 @@ def connect( self.__key_index = index LOGGER.debug(f"Main key has been updated, public key is: \n{self.public_key}") except paramiko.PasswordRequiredException: + client.close() # paramiko leaves the transport of a failed connect running if self.__password is None: LOGGER.exception("No password has been set!") raise LOGGER.critical("Unexpected PasswordRequiredException, when password is set!") raise except (paramiko.AuthenticationException, paramiko.BadHostKeyException): + client.close() # this also closes the supplied sock, so it can not be used for the next key + if sock is not None: + break continue + except BaseException: + client.close() + raise else: return msg: str = "Connection using stored authentication info failed!" @@ -259,7 +274,7 @@ def __ne__(self, other: object) -> bool: return not self.__eq__(other) def __deepcopy__(self, memo: typing.Any) -> SSHAuth: - """Helper for copy.deepcopy. + """Copy the object for copy.deepcopy. :param memo: copy.deepcopy() memodict. :type memo: typing.Any @@ -364,33 +379,33 @@ def __init__( else: # pragma: no cover raise TypeError(f"Auth object have incorrect type: (got {auth!r})") - def __setitem__(self, hostname: str, auth: SSHAuth) -> None: + def __setitem__(self, key: str, item: SSHAuth) -> None: """Dict-like access. - :param hostname: Key - hostname. - :type hostname: str - :param auth: value - SSHAuth object. - :type auth: SSHAuth + :param key: Key - hostname. + :type key: str + :param item: value - SSHAuth object. + :type item: SSHAuth :raises TypeError: Key is not string or value is not SSHAuth. """ - if not isinstance(hostname, str): # pragma: no cover - raise TypeError(f"Hostname should be string only! Got: {hostname!r}") - if not isinstance(auth, SSHAuth): # pragma: no cover - raise TypeError(f"Value {auth!r} is not SSHAuth object!") - super().__setitem__(hostname.lower(), auth) + if not isinstance(key, str): # pragma: no cover + raise TypeError(f"Hostname should be string only! Got: {key!r}") + if not isinstance(item, SSHAuth): # pragma: no cover + raise TypeError(f"Value {item!r} is not SSHAuth object!") + super().__setitem__(key.lower(), item) - def __getitem__(self, hostname: str) -> SSHAuth: + def __getitem__(self, key: str) -> SSHAuth: """Dict-like access. - :param hostname: Key - hostname. - :type hostname: str + :param key: Key - hostname. + :type key: str :return: Associated SSHAuth object. :rtype: SSHAuth :raises TypeError: Key is not a string. """ - if not isinstance(hostname, str): # pragma: no cover - raise TypeError(f"Hostname should be string only! Got: {hostname!r}") - return super().__getitem__(hostname.lower()) + if not isinstance(key, str): # pragma: no cover + raise TypeError(f"Hostname should be string only! Got: {key!r}") + return super().__getitem__(key.lower()) @typing.overload def get_with_alt_hostname( @@ -398,8 +413,7 @@ def get_with_alt_hostname( hostname: str, *host_names: str, default: SSHAuth, - ) -> SSHAuth: - """Try to guess hostname with credentials.""" + ) -> SSHAuth: ... @typing.overload def get_with_alt_hostname( @@ -407,8 +421,7 @@ def get_with_alt_hostname( hostname: str, *host_names: str, default: None = None, - ) -> SSHAuth | None: - """Try to guess hostname with credentials.""" + ) -> SSHAuth | None: ... def get_with_alt_hostname( self, @@ -439,13 +452,13 @@ def get_with_alt_hostname( return self[host] return default - def __delitem__(self, hostname: str) -> None: + def __delitem__(self, key: str) -> None: """Dict-like access. - :param hostname: Key - hostname. - :type hostname: str + :param key: Key - hostname. + :type key: str :raises TypeError: Key is not a string. """ - if not isinstance(hostname, str): # pragma: no cover - raise TypeError(f"Hostname should be string only! Got: {hostname!r}") - super().__delitem__(hostname.lower()) + if not isinstance(key, str): # pragma: no cover + raise TypeError(f"Hostname should be string only! Got: {key!r}") + super().__delitem__(key.lower()) diff --git a/exec_helpers/subprocess.py b/exec_helpers/subprocess.py index 868591c..dfd8001 100644 --- a/exec_helpers/subprocess.py +++ b/exec_helpers/subprocess.py @@ -19,13 +19,13 @@ from __future__ import annotations import concurrent.futures -import contextlib import copy -import datetime +import datetime as dt import errno import logging import os import pathlib +import shlex import subprocess # nosec # Expected usage import typing import warnings @@ -37,6 +37,7 @@ from exec_helpers import exec_result from exec_helpers import proc_enums +from . import _helpers from . import _log_templates from . import _subprocess_helpers @@ -64,6 +65,7 @@ class SubprocessExecuteAsyncResult(api.ExecuteAsyncResult): __slots__ = () @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def interface(self) -> subprocess.Popen[bytes]: """Override original NamedTuple with proper typing. @@ -73,6 +75,7 @@ def interface(self) -> subprocess.Popen[bytes]: return super().interface # type: ignore[no-any-return] @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def stdin(self) -> typing.IO[bytes] | None: """Override original NamedTuple with proper typing. @@ -87,6 +90,7 @@ def stdin(self) -> typing.IO[bytes] | None: return super().stdin @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def stderr(self) -> typing.IO[bytes] | None: """Override original NamedTuple with proper typing. @@ -96,6 +100,7 @@ def stderr(self) -> typing.IO[bytes] | None: return super().stderr @property + # pyrefly: ignore[bad-override] # the property narrows the NamedTuple field type def stdout(self) -> typing.IO[bytes] | None: """Override original NamedTuple with proper typing. @@ -105,10 +110,10 @@ def stdout(self) -> typing.IO[bytes] | None: return super().stdout -class _SubprocessExecuteContext(api.ExecuteContext, contextlib.AbstractContextManager[SubprocessExecuteAsyncResult]): +class _SubprocessExecuteContext(api.ExecuteContext): """Subprocess Execute context.""" - __slots__ = ("__cwd", "__env", "__process") + __slots__ = ("__cwd", "__env", "__process", "__shell") def __init__( self, @@ -119,6 +124,7 @@ def __init__( open_stderr: bool = True, cwd: CwdT = None, env: EnvT = None, + use_shell: bool = True, logger: logging.Logger, **kwargs: typing.Any, ) -> None: @@ -136,6 +142,9 @@ def __init__( :type cwd: str | bytes | pathlib.Path | None :param env: Defines the environment variables for the new process. :type env: Mapping[str | bytes, str | bytes] | None + :param use_shell: Run the command by the shell. + Otherwise, the command is split to arguments and executed directly. + :type use_shell: bool :param logger: Logger instance. :type logger: logging.Logger :param kwargs: Additional parameters for call. @@ -151,6 +160,7 @@ def __init__( ) self.__cwd = cwd self.__env = env + self.__shell = use_shell self.__process: subprocess.Popen[bytes] | None = None def __repr__(self) -> str: @@ -161,12 +171,13 @@ def __repr__(self) -> str: """ return ( f"" ) @@ -180,14 +191,15 @@ def __enter__(self) -> SubprocessExecuteAsyncResult: The Command is executed only in the context manager to be sure that everything will be cleaned up properly. """ - started = datetime.datetime.now(tz=datetime.UTC) + started = dt.datetime.now(tz=dt.UTC) - self.__process = subprocess.Popen( - args=self.command, + # Running a shell command line is the default mode of this class, `shell=False` is the opt-out. + self.__process = subprocess.Popen( # noqa: S603 # the arguments come from the caller by design + args=self.command if self.__shell else shlex.split(self.command), stdout=subprocess.PIPE if self.open_stdout else subprocess.DEVNULL, stderr=subprocess.PIPE if self.open_stderr else subprocess.DEVNULL, stdin=subprocess.PIPE, - shell=True, + shell=self.__shell, cwd=self.__cwd, env=self.__env, universal_newlines=False, @@ -223,13 +235,8 @@ def __enter__(self) -> SubprocessExecuteAsyncResult: raise # noinspection PyArgumentList - return SubprocessExecuteAsyncResult( - interface=process, - stdin=None, - stderr=process.stderr, - stdout=process.stdout, - started=started, - ) + # The fields are typed by properties of the subclass, which type checkers do not accept as constructor arguments + return SubprocessExecuteAsyncResult._make((process, None, process.stderr, process.stdout, started)) def __exit__( self, @@ -250,25 +257,79 @@ class Subprocess(api.ExecHelper): :param log_mask_re: Regex lookup rule to mask command for logger. All MATCHED groups will be replaced by '<*masked*>'. :type log_mask_re: str | re.Pattern[str] | None + :param shell: Run commands by the shell. If disabled, commands are accepted only as a list of arguments and + are executed directly, so no shell syntax or expansion is applied to them (chroot included). + The low level `open_execute_context` takes the already joined command and splits it with `shlex`. + :type shell: bool .. versionchanged:: 1.2.0 log_mask_re regex rule for masking cmd. .. versionchanged:: 3.1.0 Not singleton anymore. Only lock is shared between all instances. .. versionchanged:: 3.2.0 Logger can be enforced. .. versionchanged:: 4.1.0 support chroot. .. versionchanged:: 4.3.0 Lock is not shared anymore: allow parallel call of different instances. + .. versionchanged:: 8.2.1 Added shell option: commands can be executed without a shell. """ def __init__( self, log_mask_re: LogMaskReT = None, + *, + shell: bool = True, ) -> None: """Subprocess helper with timeouts and lock-free FIFO.""" + self.__shell = shell mod_name = "exec_helpers" if self.__module__.startswith("exec_helpers") else self.__module__ super().__init__( logger=logging.getLogger(f"{mod_name}.{self.__class__.__name__}"), log_mask_re=log_mask_re, ) + @property + def shell(self) -> bool: + """Run commands by the shell. + + :return: False if commands are executed directly, without a shell. + :rtype: bool + """ + return self.__shell + + def _command_to_string(self, command: CommandT) -> str: + """Convert the command to the string form used for execution and logs. + + :param command: Original command. + :type command: str | Iterable[str] + :return: Command as a single string. + :rtype: str + :raises TypeError: Command is a string, but shell is disabled. + :raises ValueError: Command has no arguments, but shell is disabled. + """ + if not self.__shell and isinstance(command, str): + raise TypeError("Without a shell the command is expected as a list of arguments, not as a string") + cmd = super()._command_to_string(command) + if not self.__shell and not cmd: + raise ValueError("Without a shell the command needs at least the program to execute") + return cmd + + def _prepare_command(self, cmd: str, chroot_path: str | None = None, chroot_exe: str | None = None) -> str: + """Prepare command: cover chroot and other cases. + + :param cmd: Main command. + :type cmd: str + :param chroot_path: Path to make chroot for execution. + :type chroot_path: str | None + :param chroot_exe: chroot exe override. + :type chroot_exe: str | None + :return: Final command, includes chroot, if required. + :rtype: str + """ + if self.__shell: + return super()._prepare_command(cmd, chroot_path=chroot_path, chroot_exe=chroot_exe) + return _helpers.chroot_argv_command( + cmd, + chroot_path=chroot_path or self._chroot_path, + chroot_exe=chroot_exe or self._chroot_exe, + ) + def _exec_command( # type: ignore[override] self, command: str, @@ -420,7 +481,9 @@ def open_execute_context( open_stderr=open_stderr, cwd=cwd, env=env, + use_shell=self.__shell, logger=self.logger, + log_mask_re=self.log_mask_re, **kwargs, ) diff --git a/pyproject.toml b/pyproject.toml index 2a44aa9..6cd3026 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -5,7 +5,6 @@ build-backend = "setuptools.build_meta" requires = [ "setuptools>=80.3", "setuptools-scm[toml]>=10.0.5", - "wheel>=0.47.0", ] [project] @@ -17,18 +16,20 @@ license = "Apache-2.0" # Use SPDX classifier license-files = [ "LICENSE" ] requires-python = ">=3.11.0" dynamic = [ "classifiers", "dependencies", "version" ] -[[project.authors]] -name = "Aleksei Stepanov" -email = "penguinolog@gmail.com" [[project.maintainers]] name = "Aleksei Stepanov" email = "penguinolog@gmail.com" + [[project.maintainers]] name = "Antonio Esposito" email = "esposito.cloud@gmail.com" + [[project.maintainers]] name = "Dennis Dmitriev" email = "dis-xcom@gmail.com" +[[project.authors]] +name = "Aleksei Stepanov" +email = "penguinolog@gmail.com" [project.optional-dependencies] xml = [] lxml = [ "lxml>=4.6.2" ] @@ -50,6 +51,7 @@ dev = [ { include-group = "prek" }, { include-group = "pylint" }, { include-group = "pyproject-fmt" }, + { include-group = "pyrefly" }, { include-group = "pytest" }, { include-group = "ruff" }, { include-group = "tox" }, @@ -68,17 +70,23 @@ doc8 = [ "pygments>=2.20.0", ] mypy = [ - "mypy[faster-cache,reports]>=2.1", + "mypy[faster-cache,reports]>=2.4.1", "rich>=15.0.0", + { include-group = "types" }, ] prek = [ - "prek>=0.4.1", + "prek>=0.5.7", ] pylint = [ - "pylint>=4.0.5", + "pylint>=4.1.3", ] pyproject-fmt = [ - "pyproject-fmt>=2.21.2", + "pyproject-fmt>=2.30.3", +] +pyrefly = [ + "pyrefly>=1.4.0", + { include-group = "pytest" }, + { include-group = "types" }, ] pytest = [ "asynctest>=0.13", @@ -93,26 +101,31 @@ readme = [ { include-group = "build" }, ] ruff = [ - "ruff>=0.15.13", + "ruff>=0.17.3", ] tox = [ "tox>=4.53.1", ] +types = [ + "types-paramiko>=5.0.0", + "types-psutil>=7.2.2", + "types-pyyaml>=6.0.12", +] [tool.setuptools] zip-safe = false -[tool.setuptools.dynamic] -dependencies = { file = [ "requirements.txt" ] } -classifiers = { file = [ "classifiers.txt" ] } +[tool.setuptools.packages.find] +exclude = [ ".*", "bin", "doc*", "examples", "test*" ] +namespaces = false [tool.setuptools.package-data] exec_helpers = [ - "py.typed", "*.pyi", "*/*.pyi", + "py.typed", ] -[tool.setuptools.packages.find] -exclude = [ "doc*", "examples", "test*", "bin", ".*" ] -namespaces = false +[tool.setuptools.dynamic] +classifiers = { file = [ "classifiers.txt" ] } +dependencies = { file = [ "requirements.txt" ] } [tool.distutils.bdist_wheel] universal = 0 @@ -129,12 +142,20 @@ line-length = 120 output-format = "full" [tool.ruff.format] docstring-code-format = true +[tool.ruff.lint.isort] +force-single-line = true +known-third-party = [ "logwrap", "paramiko", "tenacity", "pyyaml", "ruamel.yaml", "psutil" ] +[tool.ruff.lint.pydocstyle] +convention = "pep257" +[tool.ruff.lint.pylint] +allow-dunder-method-names = [ "__pretty_repr__", "__pretty_str__", "__rich_repr__" ] [tool.ruff.lint] extend-select = [ "A", # flake8-builtins "ASYNC", # flake8-async "B", - "C4", # flake8-return, flake8-simplify, flake8-comprehensions + "C4", # flake8-comprehensions + "D", # pydocstyle "DTZ", # flake8-datetimez "E", "EXE", # flake8-bugbear, flake8-debugger, flake8-executable @@ -145,8 +166,10 @@ extend-select = [ "ICN", "ISC", # flake8-implicit-str-concat "PERF", - "PGH", # flake8-import-conventions, pygrep-hooks + "PGH", # pygrep-hooks + "PIE", # flake8-pie "PL", + "PTH", # flake8-use-pathlib "PYI", # flake8-pyi "Q", # quotes "RET", @@ -154,7 +177,6 @@ extend-select = [ "S", # flake8-bandit "SIM", "SLOT", # flake8-slots - "PTH", # flake8-use-pathlib "T10", "TC", # flake8-type-checking "TID", # flake8-tidy-imports @@ -173,30 +195,38 @@ extend-ignore = [ "PLR2004", "PLR6301", # "maybe staticmethod" "RET504", # Unnecessary variable assignment before return statement - "SIM108", # Use ternary operator, "S314", # use "defusedxml" which is stale - "TRY003", # long messages prepare outside, ... + "SIM108", # Use ternary operator, "TID252", # relative imports from parent modules + "TRY003", # long messages prepare outside, ... ] -[tool.ruff.lint.isort] -force-single-line = true -known-third-party = [ "logwrap", "paramiko", "tenacity", "pyyaml", "ruamel.yaml", "psutil" ] -[tool.ruff.lint.pydocstyle] -convention = "pep257" -[tool.ruff.lint.pylint] -allow-dunder-method-names = [ "__pretty_repr__", "__pretty_str__", "__rich_repr__" ] - -[tool.isort] -line_length = 120 -multi_line_output = 3 -force_single_line = true -known_third_party = [ "logwrap", "paramiko", "tenacity", "pyyaml", "ruamel.yaml", "psutil" ] [tool.pylint] +disable = [ + "broad-except", + "file-ignored", + "invalid-name", + "locally-disabled", + "logging-format-interpolation", + "logging-fstring-interpolation", + "similarities", + "suppressed-message", + "too-few-public-methods", + "too-many-ancestors", + "too-many-arguments", + "too-many-branches", + "too-many-instance-attributes", + "too-many-lines", + "too-many-locals", + "too-many-positional-arguments", + "too-many-public-methods", + "too-many-return-statements", + "too-many-statements" +] +enable = "all" extension-pkg-whitelist = [ "lxml.etree" ] -ignore = [ "CVS", "_version.py" ] +ignore = [ "_version.py", "CVS" ] jobs = 0 -py-version = "3.11" load-plugins = [ "pylint.extensions.docparams", "pylint.extensions.docstyle", @@ -209,30 +239,8 @@ load-plugins = [ "pylint.extensions.empty_comment", "pylint.extensions.dunder", ] -enable = "all" -disable = [ - "locally-disabled", - "file-ignored", - "suppressed-message", - "similarities", - "too-many-ancestors", - "too-few-public-methods", - "too-many-public-methods", - "too-many-return-statements", - "too-many-branches", - "too-many-arguments", - "too-many-positional-arguments", - "too-many-locals", - "too-many-statements", - "too-many-instance-attributes", - "too-many-lines", - "broad-except", - "logging-fstring-interpolation", - "logging-format-interpolation", - "invalid-name" -] max-line-length = 120 -reports = false +py-version = "3.11" [tool.pylint.dunder] good-dunder-names = [ "__pretty_repr__", @@ -266,29 +274,36 @@ collapse_tables = [] skip_wrap_for_keys = [] [tool.mypy] +enable_error_code = [ "ignore-without-code" ] strict = true -warn_unused_configs = true -warn_redundant_casts = true show_error_context = true show_column_numbers = true -show_error_codes = true pretty = true +warn_unused_configs = true +show_error_codes = true [[tool.mypy.overrides]] -module = [ "psutil", "lxml.*" ] +module = [ "lxml.*" ] ignore_missing_imports = true +[tool.pyrefly] +python-version = "3.11" +project-includes = [ "exec_helpers", "test" ] +# Written by setuptools-scm at build time +ignore-missing-imports = [ "exec_helpers._version" ] + [tool.pytest] [tool.pytest.ini_options] minversion = "6.0" -addopts = "-vvv -s -p no:django -p no:ipdb" testpaths = [ "test" ] -mock_use_standalone_module = false +addopts = "-vvv -s -p no:django -p no:ipdb" junit_family = "xunit2" asyncio_default_fixture_loop_scope = "function" +mock_use_standalone_module = false [tool.coverage] -[tool.coverage.json] -pretty_print = true +[tool.coverage.run] +omit = [ "test/*" ] +branch = true [tool.coverage.report] exclude_lines = [ # Exclude methods marked as abstract @@ -321,64 +336,67 @@ exclude_lines = [ # Don't complain if tests don't hit defensive assertion code: "raise NotImplementedError", ] -[tool.coverage.run] -branch = true -omit = [ "test/*" ] +[tool.coverage.json] +pretty_print = true [tool.tox] requires = [ "tox>=4.53", ] env_list = [ - "ruff", - "pylint", + "doc8", + "docs", "mypy", - "pep257", { prefix = "py3", start = 11, stop = 15 }, { prefix = "pypy3", start = 11, stop = 15 }, + "pylint", + "pyrefly", "readme", - "doc8", - "docs", + "ruff", ] skip_missing_interpreters = true [tool.tox.env] -py311.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -py312.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -py313.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -py314.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -py315.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -pypy311.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -pypy312.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -pypy313.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -pypy314.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] -pypy315.depends = [ "pylint", "mypy", "ruff", "ruff-format", "pep257", "bandit" ] +py311.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +py312.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +py313.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +py314.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +py315.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +pypy311.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +pypy312.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +pypy313.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +pypy314.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] +pypy315.depends = [ "bandit", "mypy", "pylint", "pyrefly", "ruff", "ruff-format" ] [tool.tox.env.bandit] dependency_groups = [ "bandit" ] -ommands = [ [ "bandit", "-r", "exec_helpers" ] ] +commands = [ [ "bandit", "-r", "exec_helpers" ] ] [tool.tox.env.doc8] package = "skip" dependency_groups = [ "doc8" ] commands = [ [ "doc8", "README.rst", "doc/source" ] ] [tool.tox.env.docs] -depends = [ "doc8", "readme" ] dependency_groups = [ "docs" ] commands = [ [ "sphinx-build", "doc/source/", "doc/build" ] ] +depends = [ "doc8", "readme" ] [tool.tox.env.install] dependency_groups = [] commands = [ [ "pip", "install", "./", "-vvv", "-U" ] ] [tool.tox.env.mypy] -depends = [ "ruff", "ruff-format", "pep257", "bandit" ] +deps = [ "-r {toxinidir}/CI_REQUIREMENTS.txt" ] dependency_groups = [ "mypy" ] -deps = ["-r {toxinidir}/CI_REQUIREMENTS.txt"] commands = [ - [ "mypy", "--strict", "--install-types", "--non-interactive", "--show-error-codes", "-p", "exec_helpers" ], + [ "mypy", "-p", "exec_helpers" ], ] - +depends = [ "bandit", "ruff", "ruff-format" ] [tool.tox.env.pylint] -depends = [ "ruff", "ruff-format", "pep257", "bandit" ] +deps = [ "-r {toxinidir}/CI_REQUIREMENTS.txt" ] dependency_groups = [ "pylint" ] -deps = ["-r {toxinidir}/CI_REQUIREMENTS.txt"] commands = [ [ "pylint", "exec_helpers" ] ] +depends = [ "bandit", "ruff", "ruff-format" ] +[tool.tox.env.pyrefly] +deps = [ "-r {toxinidir}/CI_REQUIREMENTS.txt" ] +dependency_groups = [ "pyrefly" ] +commands = [ [ "pyrefly", "check" ] ] +depends = [ "bandit", "ruff", "ruff-format" ] [tool.tox.env.readme] package = "skip" dependency_groups = [ "readme" ] @@ -386,12 +404,11 @@ commands = [ [ "python", "-m", "build", "-s" ], [ "twine", "check", "{tox_root}{/}dist{/}*" ], ] - [tool.tox.env.ruff] package = "skip" -depends = [ "ruff-format" ] dependency_groups = [ "ruff" ] commands = [ [ "ruff", "check", "." ] ] +depends = [ "ruff-format" ] [tool.tox.env.ruff-format] package = "skip" dependency_groups = [ "ruff" ] @@ -399,8 +416,9 @@ commands = [ [ "ruff", "format", "--check", "." ] ] [tool.tox.env.venv] commands = [ [ "{posargs}" ] ] [tool.tox.env_run_base] -recreate = true package = "editable" +dependency_groups = [ "pytest" ] +recreate = true pass_env = [ "http_proxy", "HTTP_PROXY", @@ -410,7 +428,6 @@ pass_env = [ "NO_PROXY", ] set_env = { PYTHONDONTWRITEBYTECODE = "1" } -dependency_groups = [ "pytest" ] commands = [ [ "pip", "freeze" ], [ "py.test" ], diff --git a/test/async_api/test_subprocess.py b/test/async_api/test_subprocess.py index fc9f949..8adc81b 100644 --- a/test/async_api/test_subprocess.py +++ b/test/async_api/test_subprocess.py @@ -43,7 +43,7 @@ def __init__(self, *args): self.__src = list(args) def __aiter__(self): - """Normally we iter over source.""" + """Iterate over the source normally.""" return self async def __anext__(self): @@ -130,7 +130,7 @@ async def read_stream(stream: FakeFileStream): def pytest_generate_tests(metafunc): - """Tests parametrization.""" + """Test parametrization.""" if "run_parameters" in metafunc.fixturenames: metafunc.parametrize( "run_parameters", @@ -150,7 +150,7 @@ def pytest_generate_tests(metafunc): @pytest.fixture def run_parameters(request): - """Tests configuration apply.""" + """Test configuration apply.""" return configs[request.param] @@ -222,7 +222,7 @@ def create_mock( @pytest.fixture def logger(mocker): - """Simple mock of logger instance.""" + """Mock a logger instance.""" return mocker.patch("exec_helpers.async_api.subprocess.Subprocess.logger", autospec=True) diff --git a/test/async_api/test_subprocess_special.py b/test/async_api/test_subprocess_special.py index 0304cf3..04e7084 100644 --- a/test/async_api/test_subprocess_special.py +++ b/test/async_api/test_subprocess_special.py @@ -44,7 +44,7 @@ def __init__(self, *args): self.__src = list(args) def __aiter__(self): - """Normally we iter over source.""" + """Iterate over the source normally.""" return self async def __anext__(self): @@ -94,7 +94,7 @@ class MockParameters(typing.NamedTuple): write: Exception | None = None stdin_close: Exception | None = None - ec: typing.Sequence[int | Exception] = (0,) + ec: typing.Sequence[int | Exception | None] = (0,) configs = { @@ -189,7 +189,7 @@ class MockParameters(typing.NamedTuple): def pytest_generate_tests(metafunc): - """Tests parametrization.""" + """Test parametrization.""" if "run_parameters" in metafunc.fixturenames: metafunc.parametrize( "run_parameters", @@ -216,7 +216,7 @@ def pytest_generate_tests(metafunc): @pytest.fixture def run_parameters(request): - """Tests configuration apply.""" + """Test configuration apply.""" return configs[request.param] diff --git a/test/conftest.py b/test/conftest.py index 0519783..8c40b50 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -21,6 +21,12 @@ import pytest +@pytest.fixture(autouse=True) +def no_real_local_host_keys(mocker): + """Do not allow real host SSH server keys usage.""" + mocker.patch("exec_helpers._ssh_helpers.local_host_keys", return_value=()) + + @pytest.fixture def no_real_ssh_config(mocker): """Do not allow real host ssh config usage.""" diff --git a/test/pyproject.toml b/test/pyproject.toml index c2d19d6..d296d47 100644 --- a/test/pyproject.toml +++ b/test/pyproject.toml @@ -5,6 +5,7 @@ package = false # Extend the `pyproject.toml` file in the parent directory. extend = "../pyproject.toml" lint.extend-ignore = [ + "D1", # a test name is its description "PLC2701", # private members import "PLR0911", "PLR0912", diff --git a/test/test_context_repr.py b/test/test_context_repr.py new file mode 100644 index 0000000..4e23e6b --- /dev/null +++ b/test/test_context_repr.py @@ -0,0 +1,81 @@ +# Copyright 2018 - 2026 Alexey Stepanov aka penguinolog. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Debug strings of execute contexts do not expose secrets.""" + +from __future__ import annotations + +import typing + +import exec_helpers +from exec_helpers import async_api + +SECRET = "s3cr3t-value" + + +def check_masked(text: str) -> None: + assert SECRET not in text + assert "<*masked*>" in text + assert f"stdin=<{len(SECRET)} bytes>" in text + + +def test_subprocess_context_repr(): + """Subprocess context hides the command mask, STDIN data and environment values.""" + ctx = exec_helpers.Subprocess(log_mask_re=r"--password (\S+)").open_execute_context( + f"tool --password {SECRET}", + stdin=SECRET, + env={"TOKEN": SECRET}, + ) + text = repr(ctx) + check_masked(text) + assert "env=" in text + + +def test_async_subprocess_context_repr(): + """Async subprocess context hides the command mask, STDIN data and environment values.""" + ctx = async_api.Subprocess(log_mask_re=r"--password (\S+)").open_execute_context( + f"tool --password {SECRET}", + stdin=SECRET, + env={"TOKEN": SECRET}, + ) + text = repr(ctx) + check_masked(text) + assert "env=" in text + + +def test_ssh_context_repr(paramiko_ssh_client, ssh_auth_logger): + """SSH context hides the command mask and STDIN data.""" + ssh = exec_helpers.SSHClient( + host="192.0.2.1", + auth=exec_helpers.SSHAuth(password="pass"), + ) + ssh.log_mask_re = r"--password (\S+)" + ctx = ssh.open_execute_context(f"tool --password {SECRET}", stdin=SECRET) + check_masked(repr(ctx)) + + +def test_context_repr_no_stdin(): + """Absent STDIN and environment are shown as None.""" + text = repr(exec_helpers.Subprocess().open_execute_context("true")) + assert "stdin=None" in text + assert "env=None" in text + + +def test_context_repr_mixed_env_keys(): + """Names of an environment with mixed key types are listed without an error.""" + env: dict[typing.Any, str] = {b"A": SECRET, "B": SECRET} + ctx = exec_helpers.Subprocess().open_execute_context("true", env=env) + text = repr(ctx) + assert SECRET not in text + assert "env=" in text diff --git a/test/test_exec_result.py b/test/test_exec_result.py index c5d521d..f6a4ef4 100644 --- a/test/test_exec_result.py +++ b/test/test_exec_result.py @@ -20,7 +20,7 @@ from __future__ import annotations -import datetime +import datetime as dt import unittest import xml.etree.ElementTree as ET from unittest import mock @@ -134,7 +134,7 @@ def test_setters(self): self.assertEqual(result.stderr, result["stderr"]) with self.assertRaises(TypeError): - result.exit_code = "code" + result.exit_code = "code" # pyrefly: ignore[bad-argument-type] # a wrong type is the point result.exit_code = 0 self.assertEqual(result.exit_code, 0) @@ -232,9 +232,9 @@ def test_stdin_bytearray(self): def test_started(self): """Test timestamp.""" - started = datetime.datetime.now(tz=datetime.UTC) + started = dt.datetime.now(tz=dt.UTC) result = exec_helpers.ExecResult(cmd, exit_code=0, started=started) - spent = (result.timestamp - started).seconds + spent = (result.timestamp - started).seconds # pyrefly: ignore[unsupported-operation] # set by exit_code self.assertIs(result.started, started) self.assertEqual( str(result), @@ -285,9 +285,9 @@ def test_indexed_lines_access(self): self.assertEqual(result.stdout_lines[0, ..., 2], "line0\n...\nline2") self.assertEqual(result.stdout_lines[:1, ..., 2], "line0\n...\nline2") with self.assertRaises(TypeError): - _ = result.stdout_lines["aaa"] + _ = result.stdout_lines["aaa"] # pyrefly: ignore[bad-index] # a wrong index is the point with self.assertRaises(TypeError): - _ = result.stdout_lines[1, "aaa"] + _ = result.stdout_lines[1, "aaa"] # pyrefly: ignore[bad-index] # a wrong index is the point def test_stdout_xml(self): """Test xml etree decode.""" @@ -302,7 +302,9 @@ def test_stdout_xml(self): def test_stdout_lxml(self): """Test lxml etree decode.""" result = exec_helpers.ExecResult("test", stdout=[b"\n", b"123\n"]) + # pyrefly: ignore[missing-attribute] # the test is skipped without lxml expect = lxml.etree.fromstring(b"\n123\n") + # pyrefly: ignore[missing-attribute] # the test is skipped without lxml self.assertEqual(lxml.etree.tostring(expect), lxml.etree.tostring(result.stdout_lxml)) @unittest.skipUnless(yaml is not None, "PyYAML parser should be installed") @@ -316,7 +318,7 @@ def test_stdout_yaml_pyyaml(self): def test_pretty_repr(self): """Test repr with logwrap.""" result = exec_helpers.ExecResult("test", stdout=[b"{test: data}"], stderr=[b"{test: stderr}"]) - pretty_repr = logwrap.pretty_repr(result) + pretty_repr = logwrap.pretty_repr(result) # pyrefly: ignore[missing-attribute] # skipped without logwrap self.assertEqual( f"ExecResult(\n" f" cmd='test',\n" diff --git a/test/test_sftp.py b/test/test_sftp.py index 7964b3a..9b5fa51 100644 --- a/test/test_sftp.py +++ b/test/test_sftp.py @@ -43,8 +43,8 @@ def prepare_sftp_file_tests(client): ssh_ = mock.Mock() client.return_value = ssh_ sftp = mock.Mock() - open_sftp = mock.Mock(parent=ssh_, return_value=sftp) - ssh_.attach_mock(open_sftp, "open_sftp") + open_sftp = mock.Mock(parent=ssh_.get_transport(), return_value=sftp) + ssh_.get_transport().attach_mock(open_sftp, "open_sftp_client") with ( mock.patch("exec_helpers._ssh_helpers.SSH_CONFIG_FILE_SYSTEM", autospec=True) as conf_sys, diff --git a/test/test_shell_option.py b/test/test_shell_option.py new file mode 100644 index 0000000..5d9cd16 --- /dev/null +++ b/test/test_shell_option.py @@ -0,0 +1,121 @@ +# Copyright 2018 - 2026 Alexey Stepanov aka penguinolog. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + + +"""Execution without a shell.""" + +from __future__ import annotations + +import shlex + +import pytest + +import exec_helpers +from exec_helpers import async_api + +ARGS = ["echo", "a b; c", "$HOME"] +COMMAND = shlex.join(ARGS) + + +@pytest.fixture +def async_spawn(mocker): + """Mock process creation of asyncio: shell and exec variants, the same finished process.""" + create_shell = mocker.patch("asyncio.create_subprocess_shell") + create_exec = mocker.patch("asyncio.create_subprocess_exec") + process = mocker.MagicMock(returncode=0, stdin=None, wait=mocker.AsyncMock()) + create_shell.return_value = create_exec.return_value = process + mocker.patch("exec_helpers._subprocess_helpers.kill_proc_tree") + return create_shell, create_exec + + +def test_shell_default(mocker): + """The shell is used by default and the command is passed as is.""" + popen = mocker.patch("subprocess.Popen", autospec=True) + + with exec_helpers.Subprocess().open_execute_context("echo a | cat"): + pass + + assert popen.call_args.kwargs["shell"] is True + assert popen.call_args.kwargs["args"] == "echo a | cat\n" + + +def test_shell_disabled(mocker): + """The command is split to the arguments and executed directly.""" + popen = mocker.patch("subprocess.Popen", autospec=True) + + with exec_helpers.Subprocess(shell=False).open_execute_context(COMMAND): + pass + + assert popen.call_args.kwargs["shell"] is False + assert popen.call_args.kwargs["args"] == ARGS + + +def test_shell_disabled_chroot(mocker): + """Chroot is applied to the arguments, without a shell inside.""" + popen = mocker.patch("subprocess.Popen", autospec=True) + + with exec_helpers.Subprocess(shell=False).open_execute_context(COMMAND, chroot_path="/mnt/target dir"): + pass + + assert popen.call_args.kwargs["args"] == ["chroot", "/mnt/target dir", *ARGS] + + +def test_shell_disabled_string_rejected(): + """A string command is not accepted without a shell: its shell syntax would silently change the meaning.""" + with pytest.raises(TypeError, match="list of arguments"): + exec_helpers.Subprocess(shell=False).execute("echo a | cat") + + +def test_shell_disabled_empty_command(): + """A command without a program is rejected with a clear error.""" + with pytest.raises(ValueError, match="at least the program"): + exec_helpers.Subprocess(shell=False).execute([]) + + +@pytest.mark.asyncio +async def test_async_shell_default(async_spawn): + """The shell is used by default.""" + create_shell, create_exec = async_spawn + + async with async_api.Subprocess().open_execute_context("echo a | cat"): + pass + + assert create_shell.call_args.args == ("echo a | cat\n",) + create_exec.assert_not_called() + + +@pytest.mark.asyncio +async def test_async_shell_disabled(async_spawn): + """The command is split to the arguments and executed directly.""" + create_shell, create_exec = async_spawn + + async with async_api.Subprocess(shell=False).open_execute_context(COMMAND, chroot_path="/mnt"): + pass + + assert create_exec.call_args.args == ("chroot", "/mnt", *ARGS) + create_shell.assert_not_called() + + +@pytest.mark.asyncio +async def test_async_shell_disabled_string_rejected(): + """A string command is not accepted without a shell.""" + with pytest.raises(TypeError, match="list of arguments"): + await async_api.Subprocess(shell=False).execute("echo a | cat") + + +@pytest.mark.asyncio +async def test_async_shell_disabled_empty_command(): + """A command without a program is rejected with a clear error.""" + with pytest.raises(ValueError, match="at least the program"): + await async_api.Subprocess(shell=False).execute([]) diff --git a/test/test_ssh_client_execute.py b/test/test_ssh_client_execute.py index 6f97854..d63c589 100644 --- a/test/test_ssh_client_execute.py +++ b/test/test_ssh_client_execute.py @@ -14,7 +14,7 @@ from __future__ import annotations -import datetime +import datetime as dt import logging from unittest import mock @@ -35,7 +35,7 @@ def __init__(self, *args): self.closed = False def __iter__(self): - """Normally we iter over source.""" + """Iterate over the source normally.""" for _ in range(len(self.__src)): yield self.__src.pop(0) @@ -153,7 +153,7 @@ def read_stream(stream: FakeFileStream) -> tuple[bytes, ...]: def pytest_generate_tests(metafunc): - """Tests parametrization.""" + """Test parametrization.""" if "run_parameters" in metafunc.fixturenames: metafunc.parametrize( "run_parameters", @@ -175,7 +175,7 @@ def pytest_generate_tests(metafunc): @pytest.fixture def run_parameters(request): - """Tests configuration apply.""" + """Test configuration apply.""" return configs[request.param] @@ -282,13 +282,8 @@ def get_patched_execute_async_retval( status_event.attach_mock(mock.Mock(), "wait") chan.attach_mock(status_event, "status_event") chan.configure_mock(exit_status=exit_code) - return SshExecuteAsyncResult( - interface=chan, - stdin=mock.Mock, - stdout=stdout_part, - stderr=stderr_part, - started=datetime.datetime.now(tz=datetime.UTC), - ) + # the fields are typed by properties of the class, so they are passed in the order of the fields + return SshExecuteAsyncResult._make((chan, mock.Mock, stderr_part, stdout_part, dt.datetime.now(tz=dt.UTC))) return mocker.patch( "exec_helpers.ssh.SSHClient._execute_async", diff --git a/test/test_ssh_client_execute_special.py b/test/test_ssh_client_execute_special.py index 70e9a08..0dbde80 100644 --- a/test/test_ssh_client_execute_special.py +++ b/test/test_ssh_client_execute_special.py @@ -15,7 +15,7 @@ from __future__ import annotations import base64 -import datetime +import datetime as dt import logging import threading from unittest import mock @@ -38,7 +38,7 @@ def __init__(self, *args): self.channel = None def __iter__(self): - """Normally we iter over source.""" + """Iterate over the source normally.""" for _ in range(len(self.__src)): yield self.__src.pop(0) @@ -253,13 +253,8 @@ def get_patched_execute_async_retval() -> SshExecuteAsyncResult: status_event.attach_mock(mock.Mock(), "wait") chan.attach_mock(status_event, "status_event") chan.configure_mock(exit_status=exit_code) - return SshExecuteAsyncResult( - interface=chan, - stdin=mock.Mock, - stdout=stdout_part, - stderr=stderr_part, - started=datetime.datetime.now(tz=datetime.UTC), - ) + # the fields are typed by properties of the class, so they are passed in the order of the fields + return SshExecuteAsyncResult._make((chan, mock.Mock, stderr_part, stdout_part, dt.datetime.now(tz=dt.UTC))) execute_async = mocker.patch( "exec_helpers.ssh.SSHClient._execute_async", diff --git a/test/test_ssh_client_execute_through_host.py b/test/test_ssh_client_execute_through_host.py index 616c575..710acb8 100644 --- a/test/test_ssh_client_execute_through_host.py +++ b/test/test_ssh_client_execute_through_host.py @@ -34,7 +34,7 @@ def __init__(self, *args): self.channel = None def __iter__(self): - """Normally we iter over source.""" + """Iterate over the source normally.""" for _ in range(len(self.__src)): yield self.__src.pop(0) diff --git a/test/test_ssh_client_init_basic.py b/test/test_ssh_client_init_basic.py index 7eb32b1..d02bfb5 100644 --- a/test/test_ssh_client_init_basic.py +++ b/test/test_ssh_client_init_basic.py @@ -14,6 +14,9 @@ from __future__ import annotations +import socket +import typing +import warnings from unittest import mock import paramiko @@ -46,6 +49,7 @@ def __iter__(self): yield self.__src.pop(0) +REAL_LOCAL_HOST_KEYS = exec_helpers._ssh_helpers.local_host_keys # the autouse fixture replaces the module attribute host = "127.0.0.1" @@ -76,7 +80,7 @@ def __iter__(self): def pytest_generate_tests(metafunc): - """Tests parametrization.""" + """Test parametrization.""" if "run_parameters" in metafunc.fixturenames: metafunc.parametrize( "run_parameters", @@ -94,7 +98,7 @@ def pytest_generate_tests(metafunc): @pytest.fixture def run_parameters(request): - """Tests configuration apply.""" + """Test configuration apply.""" return configs[request.param] @@ -142,7 +146,7 @@ def test_init_base( ssh_auth_logger.assert_not_called() sftp = ssh._sftp - assert sftp == paramiko_ssh_client().open_sftp() + assert sftp == paramiko_ssh_client().get_transport().open_sftp_client() assert ssh._ssh == paramiko_ssh_client() assert ssh.hostname == host assert ssh.port == port @@ -155,3 +159,248 @@ def test_init_base( assert ssh.ssh_config[host] == expected_config_dict[host] assert ssh.ssh_config == expected_config_dict assert ssh.ssh_config[host].hostname == host + + +@pytest.mark.parametrize( + ("hostname", "expected"), + ( + ("127.0.0.1", True), + ("127.1.2.3", True), + ("::1", True), + ("[::1]", True), + ("localhost", True), + ("LOCALHOST", True), + ("192.0.2.1", False), + ("remote.example.com", False), + ), +) +def test_is_local_host(hostname, expected): + """Local host detection does not depend on name resolution.""" + assert exec_helpers._ssh_helpers.is_local_host(hostname) is expected + + +def test_is_local_host_own_names_and_interfaces(mocker): + """Own host name, its short form and own interface addresses are local.""" + mocker.patch("socket.gethostname", return_value="Node001.cluster.local") + mocker.patch( + "psutil.net_if_addrs", + return_value={ + "eth0": [ + mock.Mock(family=socket.AF_INET, address="192.0.2.10"), + mock.Mock(family=socket.AF_INET6, address="2001:db8::10%eth0"), + mock.Mock(family=socket.AF_INET6, address="fe80::10%eth0"), + mock.Mock(family=socket.AF_PACKET, address="aa:bb:cc:dd:ee:ff"), + ] + }, + ) + + for local in ("node001", "node001.cluster.local", "192.0.2.10", "2001:db8::10", "fe80::10%eth0"): + assert exec_helpers._ssh_helpers.is_local_host(local) + for remote in ("node002", "node001.example.com", "192.0.2.11", "fe80::10"): + assert not exec_helpers._ssh_helpers.is_local_host(remote) + + +def test_is_local_host_interface_enumeration_failed(mocker): + """No verdict about interfaces means the host is not local.""" + mocker.patch("psutil.net_if_addrs", side_effect=OSError) + assert not exec_helpers._ssh_helpers.is_local_host("192.0.2.10") + + +def test_unknown_host_key_policy(paramiko_ssh_client, ssh_auth_logger): + """Warning policy stays by default and a custom policy is never replaced.""" + ssh_ = mock.call + + exec_helpers.SSHClient(host="192.0.2.1", auth=exec_helpers.SSHAuth()) + assert ssh_.set_missing_host_key_policy(paramiko.WarningPolicy) in paramiko_ssh_client.return_value.mock_calls + + paramiko_ssh_client.reset_mock() + exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(), missing_host_key_policy=paramiko.RejectPolicy) + assert ssh_.set_missing_host_key_policy(paramiko.RejectPolicy) in paramiko_ssh_client.return_value.mock_calls + + +@pytest.mark.parametrize("local_host", ("127.0.0.1", "127.0.0.2", "127.0.0.254", "localhost")) +@pytest.mark.parametrize("port", (22, 2222)) +def test_local_host_policy_selected(mocker, paramiko_ssh_client, ssh_auth_logger, local_host, port): + """Any local address and port gets the policy, which accepts the keys of this machine.""" + mocker.patch("exec_helpers._ssh_helpers.local_host_keys", return_value=(mock.Mock(),)) + + exec_helpers.SSHClient(host=local_host, port=port, auth=exec_helpers.SSHAuth()) + + (policy,) = paramiko_ssh_client.return_value.set_missing_host_key_policy.call_args.args + assert isinstance(policy, exec_helpers._ssh_helpers.LocalHostKeyPolicy) + + +@pytest.mark.parametrize("keys", ((), (mock.Mock(),))) +def test_local_host_policy_not_selected(mocker, paramiko_ssh_client, ssh_auth_logger, keys): + """A remote host and a local host without readable keys keep the configured policy.""" + mocker.patch("exec_helpers._ssh_helpers.local_host_keys", return_value=keys) + + for name in ("192.0.2.1", "remote.example.com") + (() if keys else (host,)): + paramiko_ssh_client.reset_mock() + exec_helpers.SSHClient(host=name, auth=exec_helpers.SSHAuth(), missing_host_key_policy=paramiko.RejectPolicy) + assert ( + mock.call.set_missing_host_key_policy(paramiko.RejectPolicy) in paramiko_ssh_client.return_value.mock_calls + ) + + +def test_local_host_keys_file_reading(mocker, tmp_path): + """Public keys of the SSH server are read, unreadable and broken files are skipped.""" + keys = [paramiko.RSAKey.generate(1024), paramiko.RSAKey.generate(1024)] + for number, key in enumerate(keys): + (tmp_path / f"ssh_host_rsa{number}_key.pub").write_text(f"{key.get_name()} {key.get_base64()} comment\n") + (tmp_path / "ssh_host_broken_key.pub").write_text("not a key\n") + (tmp_path / "ssh_host_truncated_key.pub").write_text("ssh-ed25519 AAAA\n") + (tmp_path / "ssh_host_directory_key.pub").mkdir() + mocker.patch("exec_helpers._ssh_helpers.LOCAL_HOST_KEYS_DIR", tmp_path) + + assert [key.get_base64() for key in REAL_LOCAL_HOST_KEYS()] == [key.get_base64() for key in keys] + + +@pytest.mark.parametrize("through", ("proxycommand", "proxyjump", "sock")) +def test_unknown_host_key_policy_behind_proxy(mocker, paramiko_ssh_client, ssh_auth_logger, through): + """The local host name behind a proxy or jump host is resolved elsewhere, so the warning stays.""" + mocker.patch("exec_helpers._ssh_helpers.local_host_keys", return_value=(mock.Mock(),)) + ssh_ = mock.call + kwargs: dict[str, typing.Any] = {} + config: dict[str, typing.Any] = {host: {"hostname": host}} + if through == "proxycommand": + config = {host: {"hostname": host, "proxycommand": "nc far-host 22"}} + elif through == "proxyjump": + config = { + "jump": {"hostname": "192.0.2.1"}, + host: {"hostname": host, "proxyjump": "jump"}, + } + kwargs["ssh_auth_map"] = {"192.0.2.1": exec_helpers.SSHAuth(), host: exec_helpers.SSHAuth()} + else: + kwargs["sock"] = mock.Mock() + + if through != "sock": + kwargs["ssh_config"] = config + exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(), **kwargs) + + calls = paramiko_ssh_client.return_value.mock_calls + assert ssh_.set_missing_host_key_policy(paramiko.WarningPolicy) in calls + assert not any( + isinstance(call.args[0], exec_helpers._ssh_helpers.LocalHostKeyPolicy) + for call in paramiko_ssh_client.return_value.set_missing_host_key_policy.call_args_list + ) + + +@pytest.mark.parametrize("through", ("direct", "sock", "proxyjump", "proxy_to")) +def test_connect_timeout_every_step(paramiko_ssh_client, ssh_auth_logger, through): + """The connect timeout is applied to every connection step.""" + kwargs: dict[str, typing.Any] = {} + if through == "sock": + kwargs["sock"] = mock.Mock() + elif through == "proxyjump": + kwargs["ssh_config"] = { + "jump": {"hostname": "192.0.2.1"}, + host: {"hostname": host, "proxyjump": "jump"}, + } + kwargs["ssh_auth_map"] = {"192.0.2.1": exec_helpers.SSHAuth(), host: exec_helpers.SSHAuth()} + + ssh = exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(), connect_timeout=7, **kwargs) + if through == "proxy_to": + ssh.proxy_to("192.0.2.2", auth=exec_helpers.SSHAuth()) + + connects = [call for call in paramiko_ssh_client.return_value.mock_calls if call[0] == "connect"] + assert len(connects) >= (2 if through in {"proxyjump", "proxy_to"} else 1) + for call in connects: + assert (call.kwargs["timeout"], call.kwargs["banner_timeout"], call.kwargs["auth_timeout"]) == (7, 7, 7) + + +@pytest.mark.parametrize("value", (0, -1)) +def test_connect_timeout_not_positive(paramiko_ssh_client, value): + """A timeout that disables waiting is rejected.""" + with pytest.raises(ValueError, match="connect_timeout"): + exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(), connect_timeout=value) + + +def test_local_host_key_policy(): + """The key of this machine is accepted, any other key goes to the fallback policy as it was before.""" + key = paramiko.RSAKey.generate(1024) + other = paramiko.RSAKey.generate(1024) + client = mock.Mock() + fallback = mock.Mock() + + policy = exec_helpers._ssh_helpers.LocalHostKeyPolicy((key,), fallback=fallback) + policy.missing_host_key(client, host, key) + fallback.missing_host_key.assert_not_called() + + policy.missing_host_key(client, host, other) + fallback.missing_host_key.assert_called_once_with(client, host, other) + + +def test_local_host_key_policy_fallback_class(): + """A fallback policy class is instantiated and still decides about the keys of other servers.""" + key = paramiko.RSAKey.generate(1024) + other = paramiko.RSAKey.generate(1024) + policy = exec_helpers._ssh_helpers.LocalHostKeyPolicy((key,), fallback=paramiko.RejectPolicy) + + client = mock.Mock() + + policy.missing_host_key(client, host, key) + with pytest.raises(paramiko.SSHException): + policy.missing_host_key(client, host, other) + + +def test_failed_chain_closes_every_reached_host(paramiko_ssh_client, ssh_auth_logger): + """A failure on a jump host chain closes the clients of all the hosts created so far.""" + first = mock.Mock() + second = mock.Mock() + second.connect.side_effect = TimeoutError + paramiko_ssh_client.side_effect = [first, second] + config: dict[str, typing.Any] = { + "jump": {"hostname": "192.0.2.1"}, + host: {"hostname": host, "proxyjump": "jump"}, + } + + with pytest.raises(TimeoutError): + exec_helpers.SSHClient( + host=host, + auth=exec_helpers.SSHAuth(), + ssh_config=config, + ssh_auth_map={"192.0.2.1": exec_helpers.SSHAuth(), host: exec_helpers.SSHAuth()}, + ) + + first.close.assert_called() + second.close.assert_called() + + +@pytest.mark.parametrize(("supplied_sock", "attempts"), ((False, 3), (True, 1))) +def test_connect_retries(paramiko_ssh_client, ssh_auth_logger, supplied_sock, attempts): + """A failed connection is retried, except on a supplied socket: the failed attempt has closed it.""" + paramiko_ssh_client.return_value.connect.side_effect = paramiko.SSHException("banner") + kwargs = {"sock": mock.Mock()} if supplied_sock else {} + + with pytest.raises(paramiko.SSHException, match="banner"): + exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(), **kwargs) + + assert paramiko_ssh_client.return_value.connect.call_count == attempts + + +def test_local_host_key_policy_warns_about_other_keys(): + """The default policy still warns about a key which is not a key of this machine, and not about the own one.""" + key = paramiko.RSAKey.generate(1024) + other = paramiko.RSAKey.generate(1024) + client = mock.Mock() + policy = exec_helpers._ssh_helpers.LocalHostKeyPolicy((key,), fallback=paramiko.WarningPolicy) + + with warnings.catch_warnings(): + warnings.simplefilter("error") + policy.missing_host_key(client, host, key) + + with pytest.warns(UserWarning, match="Unknown"): + policy.missing_host_key(client, host, other) + + +@pytest.mark.parametrize("failing", ("is_local_host", "local_host_keys")) +def test_local_host_lookup_failure_is_not_a_connection_failure(mocker, paramiko_ssh_client, ssh_auth_logger, failing): + """A failed lookup of the local host keys leaves the configured policy, the connection is made as before.""" + mocker.patch("exec_helpers._ssh_helpers.is_local_host", return_value=True) + mocker.patch("exec_helpers._ssh_helpers.local_host_keys", return_value=(mock.Mock(),)) + mocker.patch(f"exec_helpers._ssh_helpers.{failing}", side_effect=OSError) + + exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth()) + + assert mock.call.set_missing_host_key_policy(paramiko.WarningPolicy) in paramiko_ssh_client.return_value.mock_calls diff --git a/test/test_ssh_client_init_special.py b/test/test_ssh_client_init_special.py index 0907fde..0bed7d1 100644 --- a/test/test_ssh_client_init_special.py +++ b/test/test_ssh_client_init_special.py @@ -14,6 +14,7 @@ from __future__ import annotations +import typing from unittest import mock import paramiko @@ -79,7 +80,7 @@ def test_001_require_key( pkey = private_keys[0] - kwargs_no_key = { + kwargs_no_key: dict[str, typing.Any] = { "hostname": host, "pkey": None, "port": port, @@ -95,6 +96,7 @@ def test_001_require_key( ssh_.load_system_host_keys(), ssh_.set_missing_host_key_policy(paramiko.WarningPolicy), ssh_.connect(**kwargs_full), + ssh_.close(), # the failed attempt is closed before the next one ssh_.connect(**kwargs_no_key), ssh_.get_transport(), ssh_.get_transport().set_keepalive(1), @@ -134,7 +136,7 @@ def test_002_use_next_key( ssh_auth_logger.debug.assert_called_once_with(f"Main key has been updated, public key is: \n{ssh.auth.public_key}") - kwargs_no_key = { + kwargs_no_key: dict[str, typing.Any] = { "hostname": host, "pkey": None, "port": port, @@ -152,7 +154,9 @@ def test_002_use_next_key( ssh_.load_system_host_keys(), ssh_.set_missing_host_key_policy(paramiko.WarningPolicy), ssh_.connect(**kwargs_key_0), + ssh_.close(), ssh_.connect(**kwargs_key_1), + ssh_.close(), ssh_.connect(**kwargs_no_key), ssh_.get_transport(), ssh_.get_transport().set_keepalive(1), @@ -330,7 +334,7 @@ def test_010_context( assert ssh.auth == exec_helpers.SSHAuth() sftp = ssh._sftp - assert sftp == paramiko_ssh_client().open_sftp() + assert sftp == paramiko_ssh_client().get_transport().open_sftp_client() assert ssh._ssh == paramiko_ssh_client() @@ -355,7 +359,7 @@ def test_011_clear_failed( mock.Mock(side_effect=[Exception("Mocked SFTP close()"), mock.Mock()]), "close", ) - ssh_.attach_mock(mock.Mock(return_value=sftp_), "open_sftp") + ssh_.get_transport().attach_mock(mock.Mock(return_value=sftp_), "open_sftp_client") paramiko_ssh_client.return_value = ssh_ ssh_logger = get_logger(exec_helpers.SSHClient.__name__) @@ -372,7 +376,7 @@ def test_011_clear_failed( assert ssh.auth == exec_helpers.SSHAuth() sftp = ssh._sftp - assert sftp == paramiko_ssh_client().open_sftp() + assert sftp == paramiko_ssh_client().get_transport().open_sftp_client() assert ssh._ssh == paramiko_ssh_client() @@ -438,7 +442,7 @@ def test_013_no_sftp( open_sftp = mock.Mock(side_effect=paramiko.SSHException) ssh_ = mock.Mock() - ssh_.attach_mock(open_sftp, "open_sftp") + ssh_.get_transport().attach_mock(open_sftp, "open_sftp_client") paramiko_ssh_client.return_value = ssh_ ssh_logger = get_logger(exec_helpers.SSHClient.__name__) @@ -472,7 +476,7 @@ def test_014_sftp_repair( open_sftp = mock.Mock(side_effect=[paramiko.SSHException, sftp_, sftp_]) ssh_ = mock.Mock() - ssh_.attach_mock(open_sftp, "open_sftp") + ssh_.get_transport().attach_mock(open_sftp, "open_sftp_client") paramiko_ssh_client.return_value = ssh_ ssh_logger = get_logger(exec_helpers.SSHClient.__name__) @@ -499,3 +503,74 @@ def test_014_sftp_repair( sftp = ssh._sftp assert sftp == open_sftp() log.assert_has_calls((mock.call.debug("SFTP is not connected, try to connect..."),)) + + +class ClosableSSHClient: + """Mimic the paramiko behavior: a closed client has no transport, so SFTP can not be opened without a connect.""" + + def __init__(self) -> None: + self.closed = False + self.transport = mock.Mock() + self.sftp = mock.Mock() + self.connect = mock.Mock(side_effect=self.__connect) + self.close = mock.Mock(side_effect=self.__close) + self.transport.open_sftp_client.side_effect = self.__open_sftp_client + + def __connect(self, *args: object, **kwargs: object) -> None: + self.closed = False + + def __close(self) -> None: + self.closed = True + + def __open_sftp_client(self) -> mock.Mock: + if self.closed: + raise AttributeError("'NoneType' object has no attribute 'open_sftp_client'") + return self.sftp + + def get_transport(self) -> mock.Mock | None: + return None if self.closed else self.transport + + def set_missing_host_key_policy(self, *args: object) -> None: + """Ignore host key policy.""" + + def load_system_host_keys(self, *args: object) -> None: + """Ignore system host keys.""" + + +def test_015_sftp_after_close( + paramiko_ssh_client, + ssh_auth_logger, + get_logger, +): + """SFTP channel is opened after the closed SSH session was restored.""" + fake = ClosableSSHClient() + paramiko_ssh_client.return_value = fake + + ssh = exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(password=password)) + ssh.close() + assert not ssh.is_alive + assert fake.connect.call_count == 1 + + assert ssh._sftp == fake.sftp + assert ssh.is_alive + assert fake.connect.call_count == 2 + + +def test_016_sftp_after_close_reconnect_failed( + paramiko_ssh_client, + ssh_auth_logger, + get_logger, +): + """Failed reconnect is reported as is and no SFTP channel is opened.""" + fake = ClosableSSHClient() + paramiko_ssh_client.return_value = fake + + ssh = exec_helpers.SSHClient(host=host, auth=exec_helpers.SSHAuth(password=password)) + ssh.close() + fake.connect.side_effect = paramiko.SSHException("connect failed") + + with pytest.raises(paramiko.SSHException, match="connect failed"): + # noinspection PyStatementEffect + ssh._sftp # pylint: disable=pointless-statement # noqa: B018 + + fake.transport.open_sftp_client.assert_not_called() diff --git a/test/test_sshauth.py b/test/test_sshauth.py index 60aff31..19a1194 100644 --- a/test/test_sshauth.py +++ b/test/test_sshauth.py @@ -44,7 +44,7 @@ def get_internal_keys( keys: typing.Iterable[paramiko.RSAKey] | None = None, **kwargs, ): - int_keys = [] + int_keys: list[paramiko.RSAKey | None] = [] if key is not None: int_keys.append(key) if keys is not None: @@ -90,7 +90,7 @@ def get_internal_keys( def pytest_generate_tests(metafunc): - """Tests parametrization.""" + """Test parametrization.""" if "run_parameters" in metafunc.fixturenames: metafunc.parametrize( "run_parameters", @@ -108,7 +108,7 @@ def pytest_generate_tests(metafunc): @pytest.fixture def run_parameters(request): - """Tests configuration apply.""" + """Test configuration apply.""" return configs[request.param] @@ -163,3 +163,56 @@ def test_002_equality_copy(): assert auth3 is not copy.copy(auth3) assert auth3 == copy.deepcopy(auth3) assert auth3 is not copy.deepcopy(auth3) + + +def test_connect_timeout(mocker): + """The connect timeout limits the TCP connection, the banner and the authentication.""" + client = mocker.Mock() + auth = exec_helpers.SSHAuth(username="user", password="pass") + + auth.connect(client, hostname="192.0.2.1", timeout=5) + kwargs = client.connect.call_args.kwargs + assert (kwargs["timeout"], kwargs["banner_timeout"], kwargs["auth_timeout"]) == (5, 5, 5) + + client.reset_mock() + auth.connect(client, hostname="192.0.2.1") + kwargs = client.connect.call_args.kwargs + assert not {"timeout", "banner_timeout", "auth_timeout"} & set(kwargs) + + +@pytest.mark.parametrize("error", (TimeoutError, paramiko.SSHException, paramiko.PasswordRequiredException)) +def test_connect_failure_closes_client(mocker, error): + """The transport of a failed connection attempt is closed: paramiko leaves it running.""" + client = mocker.Mock() + client.connect.side_effect = error + auth = exec_helpers.SSHAuth(username="user", password="pass") + + with pytest.raises(error): + auth.connect(client, hostname="192.0.2.1") + + client.close.assert_called_once_with() + + +def test_connect_all_attempts_failed_closes_client(mocker): + """Every failed attempt of the authentication loop is closed.""" + client = mocker.Mock() + client.connect.side_effect = paramiko.AuthenticationException + auth = exec_helpers.SSHAuth(username="user", password="pass") + + with pytest.raises(paramiko.AuthenticationException): + auth.connect(client, hostname="192.0.2.1", log=False) + + assert client.close.call_count == client.connect.call_count >= 1 + + +def test_connect_supplied_sock_single_attempt(mocker): + """A supplied socket is closed with the failed attempt, so the next key is not tried on it.""" + client = mocker.Mock() + client.connect.side_effect = paramiko.AuthenticationException + auth = exec_helpers.SSHAuth(username="user", keys=[paramiko.RSAKey.generate(1024), paramiko.RSAKey.generate(1024)]) + + with pytest.raises(paramiko.AuthenticationException, match="failed"): + auth.connect(client, hostname="192.0.2.1", sock=mocker.Mock(), log=False) + + client.connect.assert_called_once() + client.close.assert_called_once_with() diff --git a/test/test_subprocess.py b/test/test_subprocess.py index 9c261f1..eba3014 100644 --- a/test/test_subprocess.py +++ b/test/test_subprocess.py @@ -42,7 +42,7 @@ def __init__(self, *args): self.closed = False def __iter__(self): - """Normally we iter over source.""" + """Iterate over the source normally.""" for _ in range(len(self.__src)): yield self.__src.pop(0) @@ -127,7 +127,7 @@ def read_stream(stream: FakeFileStream): def pytest_generate_tests(metafunc): - """Tests parametrization.""" + """Test parametrization.""" if "run_parameters" in metafunc.fixturenames: metafunc.parametrize( "run_parameters", @@ -147,7 +147,7 @@ def pytest_generate_tests(metafunc): @pytest.fixture def run_parameters(request): - """Tests configuration apply.""" + """Test configuration apply.""" return configs[request.param] diff --git a/test/test_subprocess_special.py b/test/test_subprocess_special.py index c465c18..45d1925 100644 --- a/test/test_subprocess_special.py +++ b/test/test_subprocess_special.py @@ -43,7 +43,7 @@ def __init__(self, *args): self.closed = False def __iter__(self): - """Normally we iter over source.""" + """Iterate over the source normally.""" for _ in range(len(self.__src)): val = self.__src.pop(0) if isinstance(val, bytes): @@ -183,7 +183,7 @@ class MockParameters(typing.NamedTuple): def pytest_generate_tests(metafunc): - """Tests parametrization.""" + """Test parametrization.""" if "run_parameters" in metafunc.fixturenames: metafunc.parametrize( "run_parameters", @@ -210,7 +210,7 @@ def pytest_generate_tests(metafunc): @pytest.fixture def run_parameters(request): - """Tests configuration apply.""" + """Test configuration apply.""" return configs[request.param]