Repository navigation
Fix SFTP after close, harden SSH and Subprocess execution, enforce the lint baseline - #174
Merged
Merged
Conversation
Formatter output only, so the next change shows only real edits to it: - Tables and keys are reordered and sorted by the tool. - Lists of values are sorted where order does not matter. - No value is added, removed or changed.
Raise the checker floors to the current releases and bring the project up to the baseline rule set. ruff and pyproject-fmt revs in pre-commit match the dependency groups; CI installs pylint and mypy from the same groups. - Enable the D (pydocstyle) and PIE rules; fix the docstrings they report. Tests ignore only the missing-docstring rules D1. - Fix ICN001 (datetime imported as dt), FURB107 and BLE001 (narrow noqa with a reason), pylint arguments-differ against Callable and a useless suppression. - Suppress S602 on Subprocess.Popen: running a shell command line is the purpose of the class (approved by the maintainer). - Add types-paramiko, types-psutil and types-pyyaml to the mypy group instead of --install-types; drop the psutil ignore_missing_imports. - Enable mypy ignore-without-code; strict comes from [tool.mypy], not flags. - Fix the "ommands" typo in the bandit tox env, so bandit now actually runs. - Remove the unused [tool.isort], the wheel build requirement, the dangling pep257 tox env, and options that restate a default. | what | from | to | | --- | --- | --- | | ruff | >=0.15.13 | >=0.17.3 | | mypy | >=2.1 | >=2.4.1 | | pylint | >=4.0.5 | >=4.1.3 | | pyproject-fmt | >=2.21.2 | >=2.30.3 | | prek | >=0.4.1 | >=0.5.7 | | ruff-pre-commit rev | v0.15.13 | v0.17.3 | | pyproject-fmt rev | v2.21.2 | v2.30.3 | | CI pylint | >=3.0.0 | dependency group |
SSHClient closes the paramiko client on the outermost context exit (and on reconnect()), leaving it without a transport. Commands reconnect through _ssh_transport, but SFTP called SSHClient.open_sftp() directly, so the first SFTP call after a close failed with: AttributeError: 'NoneType' object has no attribute 'open_sftp_client' - __connect_sftp opens the channel with _ssh_transport.open_sftp_client(), which reconnects under the client lock when the transport is missing. - A failed reconnect propagates as is; it is not reported as unavailable SFTP. - Add regression tests for SFTP after close and for a failed reconnect; the existing mocks provide open_sftp_client on the transport.
The repr of the SSH, Subprocess and async Subprocess execute contexts printed the command, the STDIN data and the environment as they are, although they commonly hold passwords and tokens. - The command is shown with the client's log_mask_re applied; ExecuteContext takes the rule as an optional argument. - STDIN is shown by its size and the environment by variable names only. - Add tests for the three contexts, including environments with mixed key types.
Subprocess and async_api.Subprocess always ran the command line through a shell. Add an opt-in shell argument (default True, nothing changes). - With shell=False the command must be a list of arguments: execute() raises TypeError for a string and ValueError for an empty list. - The command stays a joined string in logs and ExecResult.cmd and is split into arguments only when the process is created (Popen without a shell, asyncio.create_subprocess_exec). - chroot is applied to the arguments, without sh -c inside the chroot. - SSHClient is not changed: the remote side always uses a shell. - The execute contexts take use_shell, so the shell= keyword is not passed around (bandit reports B604 on it). - Accept the S603 finding on Popen: the arguments come from the caller.
A stalled or rejected connection attempt blocked connect for a long time and leaked paramiko's transport thread and socket. - SSHClient takes connect_timeout, default None (nothing changes). SSHAuth.connect takes timeout and passes it to paramiko as timeout, banner_timeout and auth_timeout. It is applied at every connection step: the direct connection, a caller socket, each jump host of a chain and the clients created by proxy_to and execute_through_host. A value that is not positive raises ValueError. - SSHAuth.connect closes the client after every failed attempt, before the next key is tried and before an error is raised. A caller socket is closed with the failed attempt, so no further key is tried on it. - The connection chain closes every client created for it, in reverse order, if a host of the chain cannot be reached. - A connection on a supplied socket is not retried: the failed attempt has closed it, so a retry would only fail with OSError and hide the first error.
Connecting to the own machine (127.0.0.1, localhost, the own host name) printed the paramiko WarningPolicy warning every time, because the local host is normally absent from known_hosts. Nothing that worked before may fail, so the host key is still checked by the configured policy. - Add is_local_host: the loopback range, localhost, own host name or its short form, and addresses of local interfaces. It does no name resolution, treats an unscoped link-local address as not local, and answers "not local" if the interface list cannot be read. - Add local_host_keys: the public keys of this machine's SSH server (/etc/ssh/ssh_host_*_key.pub), read on every call; unreadable and malformed files are skipped. - A direct connection to a local host (no proxy command, jump host or caller socket) uses LocalHostKeyPolicy: a server key equal to a host key of this machine is accepted, every other key goes to the configured policy. The default policy still warns and RejectPolicy still rejects, on any address and port. A known_hosts entry is checked first, as before. - A failed lookup of the local host or its keys leaves the configured policy. - Tests do not read the host keys of the machine running them.
pyrefly runs next to mypy: it reports different errors, and the team uses its language server, so a finding of it shows up in their editors. - [tool.pyrefly] checks the package and the tests for the project's minimum Python version; the version file written by setuptools-scm is not required. - Add the pyrefly dependency group and a pyrefly tox environment; the stub packages move to a types group shared with mypy. Add a PyRefly CI job that the Test job waits for. - Make the code and the tests pass it, without behaviour changes: format the ExecResult error messages with format_map; name the SSHAuthMapping item method parameters as UserDict does; drop the redundant explicit context manager bases of the execute contexts; build the execute results with _make, as their field types are narrowed by properties; mark the narrowing properties and the optional imports with pyrefly: ignore[...] and the reason; annotate dictionaries in the tests.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Collaborator
Coverage Report for CI Build 40902289121Coverage increased (+14.0%) to 67.537%Details
Uncovered Changes
Coverage Regressions2 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
close()reconnects instead of failing withAttributeErroron aNonetransport.SSHClient(connect_timeout=...)(TCP, banner, key exchange, authentication; also for jump hosts andproxy_to).Subprocess(shell=False)andasync_api.Subprocess(shell=False)execute argument lists without a shell (TypeErrorfor strings).DandPIEenforced,pyreflyadded (tox env, CI job),pyproject.tomlformatted bypyproject-fmt.# noqa: S602and# noqa: S603onPopen: running the command line is the purpose ofSubprocess.SSHAuthMappingitem methods usekey/itemparameter names; the own key of the local host no longer reaches the missing host key policy.--strict --install-types[tool.mypy], stubs in thetypesgroup