Skip to content

Commit

Permalink
BREAKING: Change default paramiko unknown hosts policy to Warning
Browse files Browse the repository at this point in the history
* Upgrade static checkers and kill dead code
  • Loading branch information
penguinolog committed Dec 5, 2023
1 parent 1437d36 commit 6a93b64
Show file tree
Hide file tree
Showing 14 changed files with 163 additions and 134 deletions.
2 changes: 1 addition & 1 deletion .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ repos:

- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.1.5
rev: v0.1.7
hooks:
- id: ruff
args: [ --fix, --exit-non-zero-on-fix ]
Expand Down
6 changes: 3 additions & 3 deletions exec_helpers/_ssh_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,7 @@ def __connect(self) -> None:
with self.lock:
if self.__sock is not None:
self.__ssh = paramiko.SSHClient()
self.__ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
self.__ssh.set_missing_host_key_policy(paramiko.WarningPolicy())
self.auth.connect(
client=self.__ssh,
hostname=self.hostname,
Expand All @@ -779,7 +779,7 @@ def __get_client(self) -> paramiko.SSHClient:
"""

last_ssh_client: paramiko.SSHClient = paramiko.SSHClient()
last_ssh_client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
last_ssh_client.set_missing_host_key_policy(paramiko.WarningPolicy()) # noqa: S507,RUF100

config, auth = self.__conn_chain[0]

Expand All @@ -793,7 +793,7 @@ def __get_client(self) -> paramiko.SSHClient:

for config, auth in self.__conn_chain[1:]: # start has another logic, so do it out of cycle
ssh = paramiko.SSHClient()
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
ssh.set_missing_host_key_policy(paramiko.WarningPolicy()) # noqa: S507,RUF100

if config.proxyjump:
transport = last_ssh_client.get_transport()
Expand Down
14 changes: 7 additions & 7 deletions exec_helpers/_ssh_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,13 +222,13 @@ def from_ssh_config(
"""
return cls(
hostname=ssh_config["hostname"], # type: ignore[arg-type]
port=ssh_config.get("port", None), # type: ignore[arg-type]
user=ssh_config.get("user", None), # type: ignore[arg-type]
identityfile=ssh_config.get("identityfile", None), # type: ignore[arg-type]
proxycommand=ssh_config.get("proxycommand", None), # type: ignore[arg-type]
proxyjump=ssh_config.get("proxyjump", None), # type: ignore[arg-type]
controlpath=ssh_config.get("controlpath", None), # type: ignore[arg-type]
controlmaster=ssh_config.get("controlmaster", None), # type: ignore[arg-type]
port=ssh_config.get("port"), # type: ignore[arg-type]
user=ssh_config.get("user"), # type: ignore[arg-type]
identityfile=ssh_config.get("identityfile"), # type: ignore[arg-type]
proxycommand=ssh_config.get("proxycommand"), # type: ignore[arg-type]
proxyjump=ssh_config.get("proxyjump"), # type: ignore[arg-type]
controlpath=ssh_config.get("controlpath"), # type: ignore[arg-type]
controlmaster=ssh_config.get("controlmaster"), # type: ignore[arg-type]
)

@property
Expand Down
71 changes: 0 additions & 71 deletions exec_helpers/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -316,74 +316,3 @@ def __init__(
)
self.cmd: str = command
self.exceptions: dict[tuple[str, int], Exception] = exceptions


class StopExecution(Exception):
"""Stop execution without waiting for exit code."""

def __init__(
self,
trigger_line: bytes,
result: exec_result.ExecResult,
message: str = "StopExecution raised",
) -> None:
self.__trigger_line = trigger_line
self.__result = result
super().__init__(message)

@property
def trigger_line(self) -> bytes:
"""Trigger line for exception.
:return: original line triggered StopExecution as bytes
:rtype: bytes
"""
return self.__trigger_line

@property
def trigger_string(self) -> str:
"""Decoded trigger line for exception.
:return: original line triggered StopExecution as str
:rtype: str
"""
return self.__trigger_line.decode("utf-8", errors="backslashreplace")

@property
def result(self) -> exec_result.ExecResult:
"""Execution result object.
:return: execution result object
:rtype: exec_result.ExecResult
"""
return self.__result


class StopExecutionGraceful(StopExecution):
"""Stop execution without raising exception."""

def __init__(
self,
trigger_line: bytes,
result: exec_result.ExecResult,
) -> None:
super().__init__(
trigger_line=trigger_line,
result=result,
message="Graceful early stop of execution.",
)


class StopExecutionError(StopExecution, ExecHelperError):
"""Stop execution and raise exception."""

def __init__(
self,
trigger_line: bytes,
result: exec_result.ExecResult,
) -> None:
super().__init__(
trigger_line=trigger_line,
result=result,
message="Unexpected command output caused stop of execution.",
)
10 changes: 10 additions & 0 deletions flake8_requirements.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,14 @@
flake8
flake8-async
flake8-datetimez
flake8-type-checking
flake8-builtins
flake8-bugbear
flake8-debugger
flake8-executable
flake8-implicit-str-concat
flake8-simplify
flake8-comprehensions
flake8-import-conventions
pep8-naming
flake8-docstrings
2 changes: 1 addition & 1 deletion mypy_requirements.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
mypy>=0.910
mypy>=1.7.0
6 changes: 4 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,8 @@ extend-select = [
]
extend-ignore = [
# refactor rules (too many statements/arguments/branches)
"PLR0904", "PLR0911", "PLR0912", "PLR0913", "PLR0915", "PLR2004",
"PLR0904", "PLR0911", "PLR0912", "PLR0913", "PLR0915", "PLR0917", "PLR2004",
"PLR6301", # "maybe staticmethod"
"PLW3201", # Bad or misspelled dunder method name `__pretty_repr__`
"RET504", # Unnecessary variable assignment before return statement
"SIM108", # Use ternary operator,
"TRY003", # long messages prepare outside, ...
Expand All @@ -198,6 +197,9 @@ extend-ignore = [
force-single-line = true
known-third-party = ["logwrap", "paramiko", "tenacity", "pyyaml", "ruamel.yaml", "psutil"]

[tool.ruff.lint.pylint]
allow-dunder-method-names = ["__pretty_repr__", "__pretty_str__", "__rich_repr__"]

[tool.ruff.pydocstyle]
convention = "pep257"

Expand Down
6 changes: 3 additions & 3 deletions test/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ def paramiko_ssh_client(mocker, no_real_ssh_config) -> mock.MagicMock:


@pytest.fixture
def auto_add_policy(mocker) -> mock.MagicMock:
"""Minimal paramiko.AutoAddPolicy mock."""
return mocker.patch("paramiko.AutoAddPolicy", return_value="AutoAddPolicy")
def paramiko_keys_policy(mocker) -> mock.MagicMock:
"""Minimal paramiko.WarningPolicy mock."""
return mocker.patch("paramiko.WarningPolicy", return_value="WarningPolicy")


@pytest.fixture
Expand Down
32 changes: 25 additions & 7 deletions test/test_sftp.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@


@mock.patch("logging.getLogger", autospec=True)
@mock.patch("paramiko.AutoAddPolicy", autospec=True, return_value="AutoAddPolicy")
@mock.patch("paramiko.WarningPolicy", autospec=True, return_value="WarningPolicy")
@mock.patch("paramiko.SSHClient", autospec=True)
class TestSftp(unittest.TestCase):
@staticmethod
Expand All @@ -46,9 +46,10 @@ def prepare_sftp_file_tests(client):
open_sftp = mock.Mock(parent=_ssh, return_value=_sftp)
_ssh.attach_mock(open_sftp, "open_sftp")

with mock.patch("exec_helpers._ssh_helpers.SSH_CONFIG_FILE_SYSTEM", autospec=True) as conf_sys, mock.patch(
"exec_helpers._ssh_helpers.SSH_CONFIG_FILE_USER", autospec=True
) as conf_user:
with mock.patch(
"exec_helpers._ssh_helpers.SSH_CONFIG_FILE_SYSTEM",
autospec=True,
) as conf_sys, mock.patch("exec_helpers._ssh_helpers.SSH_CONFIG_FILE_USER", autospec=True) as conf_user:
conf_sys.exists.return_value = False
conf_user.exists.return_value = False

Expand Down Expand Up @@ -258,7 +259,16 @@ def test_open(self, client, *args):
@mock.patch("os.path.exists", autospec=True)
@mock.patch("exec_helpers.ssh.SSHClient.isdir")
@mock.patch("os.path.isdir", autospec=True)
def test_download(self, isdir, remote_isdir, exists, remote_exists, client, policy, _logger):
def test_download(
self,
isdir,
remote_isdir,
exists,
remote_exists,
client,
policy,
_logger,
):
ssh, _sftp = self.prepare_sftp_file_tests(client)
isdir.return_value = True
exists.side_effect = [True, False, False]
Expand All @@ -274,7 +284,12 @@ def test_download(self, isdir, remote_isdir, exists, remote_exists, client, poli
exists.assert_called_once_with(posixpath.join(target, os.path.basename(dst)))
remote_isdir.assert_called_once_with(dst)
remote_exists.assert_called_once_with(dst)
_sftp.assert_has_calls((mock.call.get(dst, posixpath.join(target, os.path.basename(dst))),))
_sftp.assert_has_calls(
mock.call.get(
dst,
posixpath.join(target, os.path.basename(dst)),
)
)

# Negative scenarios
# noinspection PyTypeChecker
Expand Down Expand Up @@ -327,6 +342,9 @@ def test_upload_dir(self, isdir, remote_isdir, walk, mkdir, exists, client, *arg
_sftp.assert_has_calls(
(
mock.call.unlink(expected_file),
mock.call.put(os.path.normpath(os.path.join(source, filename)), expected_file),
mock.call.put(
os.path.normpath(os.path.join(source, filename)),
expected_file,
),
)
)
41 changes: 34 additions & 7 deletions test/test_ssh_client_execute.py
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ def ssh_transport_channel(paramiko_ssh_client, chan_makefile, run_parameters):
def ssh(
paramiko_ssh_client,
ssh_transport_channel,
auto_add_policy,
paramiko_keys_policy,
ssh_auth_logger,
get_logger,
):
Expand All @@ -241,7 +241,7 @@ def ssh(
def ssh2(
paramiko_ssh_client,
ssh_transport_channel,
auto_add_policy,
paramiko_keys_policy,
ssh_auth_logger,
get_logger,
):
Expand Down Expand Up @@ -298,10 +298,20 @@ def get_patched_execute_async_retval(

@pytest.fixture
def execute(mocker, exec_result):
return mocker.patch("exec_helpers.ssh.SSHClient.execute", name="execute", return_value=exec_result)
return mocker.patch(
"exec_helpers.ssh.SSHClient.execute",
name="execute",
return_value=exec_result,
)


def test_002_execute(ssh, ssh_transport_channel, exec_result, run_parameters, get_logger) -> None:
def test_002_execute(
ssh,
ssh_transport_channel,
exec_result,
run_parameters,
get_logger,
) -> None:
kwargs = {}
if "get_pty" in run_parameters:
kwargs["get_pty"] = run_parameters["get_pty"]
Expand Down Expand Up @@ -401,10 +411,21 @@ def test_007_check_stderr(ssh, exec_result, get_logger, mocker) -> None:
log = ssh_logger.getChild(f"{host}:{port}")

if not exec_result.stderr:
assert ssh.check_stderr(command, stdin=exec_result.stdin, expected=[exec_result.exit_code]) == exec_result
assert (
ssh.check_stderr(
command,
stdin=exec_result.stdin,
expected=[exec_result.exit_code],
)
== exec_result
)
else:
with pytest.raises(exec_helpers.CalledProcessError) as e:
ssh.check_stderr(command, stdin=exec_result.stdin, expected=[exec_result.exit_code])
ssh.check_stderr(
command,
stdin=exec_result.stdin,
expected=[exec_result.exit_code],
)
exc: exec_helpers.CalledProcessError = e.value
assert exc.result == exec_result
assert exc.cmd == exec_result.cmd
Expand Down Expand Up @@ -473,7 +494,13 @@ def test_009_execute_together(ssh, ssh2, execute_async, exec_result, run_paramet
assert exc.results == {(host, port): exec_result, (host2, port): exec_result}


def test_010_execute_together_expected(ssh, ssh2, execute_async, exec_result, run_parameters):
def test_010_execute_together_expected(
ssh,
ssh2,
execute_async,
exec_result,
run_parameters,
):
remotes = [ssh, ssh2]

results = exec_helpers.SSHClient.execute_together(
Expand Down
17 changes: 13 additions & 4 deletions test/test_ssh_client_execute_special.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ def __call__(self, flags: str):
def ssh_transport_channel(paramiko_ssh_client, chan_makefile):
chan = mock.Mock(makefile=chan_makefile, closed=False)
chan_makefile.channel = chan
chan.attach_mock(mock.Mock(return_value=FakeFileStream(*stderr_src)), "makefile_stderr")
chan.attach_mock(
mock.Mock(return_value=FakeFileStream(*stderr_src)),
"makefile_stderr",
)
chan.configure_mock(exit_status=0)
chan.status_event.attach_mock(mock.Mock(return_value=True), "is_set")
open_session = mock.Mock(return_value=chan)
Expand All @@ -113,7 +116,7 @@ def ssh_transport_channel(paramiko_ssh_client, chan_makefile):
def ssh(
paramiko_ssh_client,
ssh_transport_channel,
auto_add_policy,
paramiko_keys_policy,
ssh_auth_logger,
get_logger,
):
Expand All @@ -128,7 +131,7 @@ def ssh(
def ssh2(
paramiko_ssh_client,
ssh_transport_channel,
auto_add_policy,
paramiko_keys_policy,
ssh_auth_logger,
get_logger,
):
Expand All @@ -141,7 +144,13 @@ def ssh2(

@pytest.fixture
def exec_result():
return exec_helpers.ExecResult(cmd=command, stdin=None, stdout=stdout_src, stderr=stderr_src, exit_code=0)
return exec_helpers.ExecResult(
cmd=command,
stdin=None,
stdout=stdout_src,
stderr=stderr_src,
exit_code=0,
)


def test_001_mask_command(ssh, get_logger) -> None:
Expand Down
Loading

0 comments on commit 6a93b64

Please sign in to comment.