From 65fe296da5b3647d8709cf1d3c3c4ac189eb7fab Mon Sep 17 00:00:00 2001 From: Eli Kogan-Wang Date: Fri, 6 Dec 2024 15:54:47 +0100 Subject: [PATCH] Implement shellSplitString for proper handling of NIX_SSHOPTS with spaces and quotes --- doc/manual/rl-next/nix-sshopts-parsing.md | 21 +++++ src/libstore/ssh.cc | 13 ++- src/libutil-tests/strings.cc | 107 +++++++++++++++++++++- src/libutil/strings.cc | 104 +++++++++++++++++++++ src/libutil/strings.hh | 7 ++ 5 files changed, 248 insertions(+), 4 deletions(-) create mode 100644 doc/manual/rl-next/nix-sshopts-parsing.md diff --git a/doc/manual/rl-next/nix-sshopts-parsing.md b/doc/manual/rl-next/nix-sshopts-parsing.md new file mode 100644 index 00000000000..65fe6f5625a --- /dev/null +++ b/doc/manual/rl-next/nix-sshopts-parsing.md @@ -0,0 +1,21 @@ +--- +synopsis: "Improved `NIX_SSHOPTS` parsing for better SSH option handling" +issues: [5181] +prs: [12020] +--- + +The parsing of the `NIX_SSHOPTS` environment variable has been improved to handle spaces and quotes correctly. +Previously, incorrectly split SSH options could cause failures in CLIs like `nix-copy-closure`, +especially when using complex ssh invocations such as `-o ProxyCommand="ssh -W %h:%p ..."`. + +This change introduces a `shellSplitString` function to ensure +that `NIX_SSHOPTS` is parsed in a manner consistent with shell +behavior, addressing common parsing errors. + +For example, the following now works as expected: + +```bash +export NIX_SSHOPTS='-o ProxyCommand="ssh -W %h:%p ..."' +``` + +This update improves the reliability of SSH-related operations using `NIX_SSHOPTS` across Nix CLIs. diff --git a/src/libstore/ssh.cc b/src/libstore/ssh.cc index dec733fd516..116a480bacc 100644 --- a/src/libstore/ssh.cc +++ b/src/libstore/ssh.cc @@ -41,8 +41,17 @@ void SSHMaster::addCommonSSHOpts(Strings & args) { auto state(state_.lock()); - for (auto & i : tokenizeString(getEnv("NIX_SSHOPTS").value_or(""))) - args.push_back(i); + std::string sshOpts = getEnv("NIX_SSHOPTS").value_or(""); + + try { + std::list opts = shellSplitString(sshOpts); + for (auto & i : opts) + args.push_back(i); + } catch (Error & e) { + e.addTrace({}, "while splitting NIX_SSHOPTS '%s'", sshOpts); + throw; + } + if (!keyFile.empty()) args.insert(args.end(), {"-i", keyFile}); if (!sshPublicHostKey.empty()) { diff --git a/src/libutil-tests/strings.cc b/src/libutil-tests/strings.cc index 8ceb1676760..206890bcf19 100644 --- a/src/libutil-tests/strings.cc +++ b/src/libutil-tests/strings.cc @@ -2,11 +2,10 @@ #include #include "strings.hh" +#include "error.hh" namespace nix { -using Strings = std::vector; - /* ---------------------------------------------------------------------------- * concatStringsSep * --------------------------------------------------------------------------*/ @@ -345,4 +344,108 @@ RC_GTEST_PROP(splitString, recoveredByConcatStringsSep, (const std::string & s)) RC_ASSERT(concatStringsSep("a", splitString(s, "a")) == s); } +/* ---------------------------------------------------------------------------- + * shellSplitString + * --------------------------------------------------------------------------*/ + +TEST(shellSplitString, empty) +{ + std::list expected = {}; + + ASSERT_EQ(shellSplitString(""), expected); +} + +TEST(shellSplitString, oneWord) +{ + std::list expected = {"foo"}; + + ASSERT_EQ(shellSplitString("foo"), expected); +} + +TEST(shellSplitString, oneWordQuotedWithSpaces) +{ + std::list expected = {"foo bar"}; + + ASSERT_EQ(shellSplitString("'foo bar'"), expected); +} + +TEST(shellSplitString, oneWordQuotedWithSpacesAndDoubleQuoteInSingleQuote) +{ + std::list expected = {"foo bar\""}; + + ASSERT_EQ(shellSplitString("'foo bar\"'"), expected); +} + +TEST(shellSplitString, oneWordQuotedWithDoubleQuotes) +{ + std::list expected = {"foo bar"}; + + ASSERT_EQ(shellSplitString("\"foo bar\""), expected); +} + +TEST(shellSplitString, twoWords) +{ + std::list expected = {"foo", "bar"}; + + ASSERT_EQ(shellSplitString("foo bar"), expected); +} + +TEST(shellSplitString, twoWordsWithSpacesAndQuotesQuoted) +{ + std::list expected = {"foo bar'", "baz\""}; + + ASSERT_EQ(shellSplitString("\"foo bar'\" 'baz\"'"), expected); +} + +TEST(shellSplitString, emptyArgumentsAreAllowedSingleQuotes) +{ + std::list expected = {"foo", "", "bar", "baz", ""}; + + ASSERT_EQ(shellSplitString("foo '' bar baz ''"), expected); +} + +TEST(shellSplitString, emptyArgumentsAreAllowedDoubleQuotes) +{ + std::list expected = {"foo", "", "bar", "baz", ""}; + + ASSERT_EQ(shellSplitString("foo \"\" bar baz \"\""), expected); +} + +TEST(shellSplitString, singleQuoteDoesNotUseEscapes) +{ + std::list expected = {"foo\\\"bar"}; + + ASSERT_EQ(shellSplitString("'foo\\\"bar'"), expected); +} + +TEST(shellSplitString, doubleQuoteDoesUseEscapes) +{ + std::list expected = {"foo\"bar"}; + + ASSERT_EQ(shellSplitString("\"foo\\\"bar\""), expected); +} + +TEST(shellSplitString, backslashEscapesSpaces) +{ + std::list expected = {"foo bar", "baz", "qux quux"}; + + ASSERT_EQ(shellSplitString("foo\\ bar baz qux\\ quux"), expected); +} + +TEST(shellSplitString, backslashEscapesQuotes) +{ + std::list expected = {"foo\"bar", "baz", "qux'quux"}; + + ASSERT_EQ(shellSplitString("foo\\\"bar baz qux\\'quux"), expected); +} + +TEST(shellSplitString, testUnbalancedQuotes) +{ + ASSERT_THROW(shellSplitString("foo'"), Error); + ASSERT_THROW(shellSplitString("foo\""), Error); + ASSERT_THROW(shellSplitString("foo'bar"), Error); + ASSERT_THROW(shellSplitString("foo\"bar"), Error); + ASSERT_THROW(shellSplitString("foo\"bar\\\""), Error); +} + } // namespace nix diff --git a/src/libutil/strings.cc b/src/libutil/strings.cc index c221a43c6f1..402b7ae98a3 100644 --- a/src/libutil/strings.cc +++ b/src/libutil/strings.cc @@ -4,6 +4,7 @@ #include "strings-inline.hh" #include "os-string.hh" +#include "error.hh" namespace nix { @@ -48,4 +49,107 @@ template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const s template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::set &); template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::vector &); +/** + * Shell split string: split a string into shell arguments, respecting quotes and backslashes. + * + * Used for NIX_SSHOPTS handling, which previously used `tokenizeString` and was broken by + * Arguments that need to be passed to ssh with spaces in them. + * + * Read https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html for the + * POSIX shell specification, which is technically what we are implementing here. + */ +std::list shellSplitString(std::string_view s) +{ + std::list result; + std::string current; + bool startedCurrent = false; + bool escaping = false; + + auto pushCurrent = [&]() { + if (startedCurrent) { + result.push_back(current); + current.clear(); + startedCurrent = false; + } + }; + + auto pushChar = [&](char c) { + current.push_back(c); + startedCurrent = true; + }; + + auto pop = [&]() { + auto c = s[0]; + s.remove_prefix(1); + return c; + }; + + auto inDoubleQuotes = [&]() { + startedCurrent = true; + // in double quotes, escaping with backslash is only effective for $, `, ", and backslash + while (!s.empty()) { + auto c = pop(); + if (escaping) { + switch (c) { + case '$': + case '`': + case '"': + case '\\': + pushChar(c); + break; + default: + pushChar('\\'); + pushChar(c); + break; + } + escaping = false; + } else if (c == '\\') { + escaping = true; + } else if (c == '"') { + return; + } else { + pushChar(c); + } + } + if (s.empty()) { + throw Error("unterminated double quote"); + } + }; + + auto inSingleQuotes = [&]() { + startedCurrent = true; + while (!s.empty()) { + auto c = pop(); + if (c == '\'') { + return; + } + pushChar(c); + } + if (s.empty()) { + throw Error("unterminated single quote"); + } + }; + + while (!s.empty()) { + auto c = pop(); + if (escaping) { + pushChar(c); + escaping = false; + } else if (c == '\\') { + escaping = true; + } else if (c == ' ' || c == '\t') { + pushCurrent(); + } else if (c == '"') { + inDoubleQuotes(); + } else if (c == '\'') { + inSingleQuotes(); + } else { + pushChar(c); + } + } + + pushCurrent(); + + return result; +} } // namespace nix diff --git a/src/libutil/strings.hh b/src/libutil/strings.hh index 533126be1e0..c4fd3daa194 100644 --- a/src/libutil/strings.hh +++ b/src/libutil/strings.hh @@ -71,4 +71,11 @@ extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view, extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::set &); extern template std::string dropEmptyInitThenConcatStringsSep(std::string_view, const std::vector &); +/** + * Shell split string: split a string into shell arguments, respecting quotes and backslashes. + * + * Used for NIX_SSHOPTS handling, which previously used `tokenizeString` and was broken by + * Arguments that need to be passed to ssh with spaces in them. + */ +std::list shellSplitString(std::string_view s); }