Quote scp paths in salt-ssh to support paths with spaces - #70085
Open
twangboy wants to merge 5 commits into
Open
Quote scp paths in salt-ssh to support paths with spaces#70085twangboy wants to merge 5 commits into
twangboy wants to merge 5 commits into
Conversation
twangboy
force-pushed
the
fix/61338/3008.x
branch
from
August 18, 2026 20:51
4298c8c to
824c544
Compare
twangboy
force-pushed
the
fix/61338/3008.x
branch
from
August 25, 2026 16:38
824c544 to
22eb329
Compare
twangboy
force-pushed
the
fix/61338/3008.x
branch
from
August 31, 2026 03:45
5fd8ca9 to
7767bd9
Compare
Shell.send() built the scp command as an unquoted f-string, so any space in the local or remote path (e.g. a master cachedir/root_dir containing a space) caused shlex.split() to break it into bogus extra arguments and fail the transfer. This affected every send() caller: thin/relenv tarball deploy, ext_mods tarball, pre-flight script, shim transfer, and the relenv minion-config transfer. Quote local and host:remote with shlex.quote() in Shell.send(), and apply the same treatment to the related unquoted "test -d"/"test -f" remote checks in Single.check_thin_dir() and Single.cmd_block(). Add a unit test covering the command string built by Shell.send(), and an integration test that overrides a roster's thin_dir with a space-containing path and drives a real salt-ssh deploy against it. Fixes saltstack#61338
salt.client.ssh.Single.__init__ set self.minion_opts["__master_opts__"] = self.context["master_opts"] for relenv targets, embedding the master's entire own config (395 keys in a typical test master) into the minion config file shipped to and read by salt-call on the remote target. __master_opts__ is a master-side-only convention: every other reader of it (salt/client/ssh/wrapper/cmdmod.py, cp.py, publish.py, salt/client/ssh/state.py, salt/roster/__init__.py) pulls it from the Python wrapper opts dict while running on the master; nothing on the remote target ever reads it back out of its own minion config. self.context["master_opts"] is an alias for the master's own opts, which gets mutated as nested Single/wrapper calls restore/adjust the master cachedir (saltstack#69605, saltstack#68458). Embedding it in the relenv minion config meant that config grew, unbounded, with every nested Single created during a single state run, until it exceeded the kernel's ARG_MAX and the ssh invocation failed with "Argument list too long". Fixes saltstack#70186.
twangboy
force-pushed
the
fix/61338/3008.x
branch
from
August 31, 2026 15:09
7767bd9 to
55ae4a0
Compare
This was referenced Sep 1, 2026
Single.deploy() and deploy_ext() discarded Shell.send()'s return value and always reported success, even when the scp transfer failed. A failed transfer was invisible until the shim's next request for a redeploy, and cmd_block()'s retry after that redeploy request called itself again with no guard, recursing indefinitely: each cycle re-deployed, re-ran the shim, failed again, and recursed again. What looked like salt-ssh hanging for >90s (see saltstack#70204) was actually dozens of these deploy/shim cycles running back to back until the test timeout tripped. deploy()/deploy_ext() now check Shell.send()'s retcode and report failure instead of swallowing it, and cmd_block() bails out with an error on a second consecutive deploy failure instead of recursing into a third attempt. Fixes saltstack#70204.
test_thin_dir_with_space put the space in the *remote* thin_dir, which is a strictly harder property than saltstack#61338 ever asked for: the reporter's space was in the master's root_dir (a local path), with a space-free default remote thin_dir. Remote-path spaces can't work through OpenSSH's legacy (pre-9.0) scp/rcp protocol, which hands the remote path to a remote shell that word-splits it -- that's a separate, unsupported case, not this fix. What actually happened when that test ran on OpenSSH < 9.0 targets wasn't a hang either: each scp returned in under a second, but the transfer silently failed to land in the space-containing remote directory, and salt-ssh's deploy-retry loop spun 40+ times before pytest's 90s timeout cut it off (now fixed separately). The 'flaky(max_runs=4)' marking added on top of it didn't help, since the failure was deterministic, not intermittent, on every affected distro. Replace it with test_send_local_path_with_space, which drives a real scp transfer through Shell.send() with a space in the *local* path -- the actual saltstack#61338 regression -- against the sshd already used by this module, rather than mocking _run_cmd() like the existing unit test. This exercises the real _cmd_str()/_split_cmd()/scp path that the staging-fix regression (since reverted) slipped through. Also tighten the existing unit test's assertion to check the exact trailing scp arguments, so a regression that adds bogus arguments (as the reverted staging fix did) fails loudly instead of passing on an 'in split_cmd' membership check.
The previous CI run (attempt 2) hit a GitHub Actions startup_failure before any jobs ran. Attempt 1 of that same run had already completed successfully for this PR's purposes: all 4 previously-failing jobs (Photon OS 4 Arm64/fips, Ubuntu 22.04 Arm64 tcp/zeromq) passed, and the 5 jobs the reverted staging fix had broken (Photon OS 5 x4, Debian 12 Arm64) stayed green. The only failures were pre-existing, unrelated flakiness (a dead upstream apt repo on EOL Debian 11, and the known event-listener memory-leak test flake). Re-triggering to get a clean recorded run.
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
1 participant
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.
What does this PR do?
Shell.send() built the scp command as an unquoted f-string, so any space in the local or remote path (e.g. a master cachedir/root_dir containing a space) caused shlex.split() to break it into bogus extra arguments and fail the transfer. This affected every send() caller: thin/relenv tarball deploy, ext_mods tarball, pre-flight script, shim transfer, and the relenv minion-config transfer.
Quote local and host:remote with shlex.quote() in Shell.send(), and apply the same treatment to the related unquoted "test -d"/"test -f" remote checks in Single.check_thin_dir() and Single.cmd_block().
Add a unit test covering the command string built by Shell.send(), and an integration test that overrides a roster's thin_dir with a space-containing path and drives a real salt-ssh deploy against it.
What issues does this PR fix or reference?
Fixes #61338
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes