Skip to content

use shlex to parse/ escape stings for POSIX shells - #347

Open
weriomat wants to merge 2 commits into
serokell:masterfrom
weriomat:shlex
Open

use shlex to parse/ escape stings for POSIX shells#347
weriomat wants to merge 2 commits into
serokell:masterfrom
weriomat:shlex

Conversation

@weriomat

Copy link
Copy Markdown
Contributor

Use the shlex crate to split/ parse the ssh_opts instead of just splitting by ' '.
If shlex fails to parse/ join, we fall back to the old way.

Fixes #130. Might fix #343.

weriomat and others added 2 commits September 1, 2026 16:44
…tests

Problem: the shlex-based fix for serokell#130/serokell#343 duplicated the same
try_join/unwrap_or call at both NIX_SSHOPTS call sites in push.rs, and
shipped without any regression test covering either the ssh_opts
parsing side (lib.rs) or the ssh_opts joining side (push.rs).

Solution: extract parse_ssh_opts (lib.rs) and join_ssh_opts (push.rs)
as named, testable functions, and add unit tests reproducing the
broken tokenization from serokell#130 and the broken NIX_SSHOPTS join from
serokell#343 against the old naive space-split/space-join behavior.
@heitor-lassarote

Copy link
Copy Markdown
Member

Thank you for your work on this. I've rebased your branch (and resolved conflicts) and added some tests.

One note: I don't think this fixes #343. That failure happens during the direct SSH activation call, where each ssh_opts entry is already passed as its own argument, not through a joined string. The two spots this PR touches (the --ssh-opts CLI flag and NIX_SSHOPTS for remote builds) aren't on that code path. I'll leave #343 open separately.

CI is currently red. I didn't delve into it myself but I had Claude check. So please don't take its feedback at face value, just something that perhaps could help unblock this PR:


join_ssh_opts (used at both NIX_SSHOPTS call sites in push.rs) uses shlex::try_join, which quotes more aggressively than it needs to. It wraps something like StrictHostKeyChecking=no in single quotes even though there's no space to protect.

That's a problem because Nix's NIX_SSHOPTS parsing only became quote-aware in Nix 2.26. Before that, Nix split the string with tokenizeString, a plain whitespace split with no quote or escape handling at all (confirmed in Nix 2.24.12's own source, SSHMaster::addCommonSSHOpts). This repo's pinned nixpkgs ships Nix 2.24.12, so the quoted value comes through as one token, quotes included, and ssh chokes on it: Bad configuration option: 'stricthostkeychecking. That matches the failing jobs exactly.

It's not limited to ProxyCommand-style values either. Since shlex quotes almost anything containing =, it affects nearly every ssh option, which is why most of the VM tests fail, not just the one aimed at a specific edge case.

A quoting function that only wraps a value in quotes when it actually contains whitespace, instead of delegating to shlex's more conservative default, would fix the space-in-a-value case on Nix 2.26+ without changing anything for values that already worked, on any Nix version. Worth noting: even with that fix, a value with an embedded space still won't work on Nix below 2.26, since that Nix has no way to receive it correctly at all. That's a Nix-side limitation, not something fixable from here.


I don't have the bandwidth to implement that fix as part of this review, so flagging it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Working example of configuration with SSH and ProxyCommand wrong arguments given to ssh when some ssh argument specified in ssh-opts contains space

2 participants