[libvirt PATCH 00/11] virnetclient: Cleanups and improvement
I initially started looking into this because of https://gitlab.com/libvirt/libvirt/-/issues/273 I have now convinced myself that we don't need to change the way we quote things, but in the process I have accumulated several improvements and one bug fix. Andrea Bolognani (11): virbuftest: Increase coverage virbuffer: Simplify virBufferEscapeShell() virnetsockettest: Drop unnecessary backslash virnetsockettest: Move opening quote virnetsockettest: Improve indentation virnetclient: Improve spacing of ssh script virnetclient: Use 'if' consistently virnetsockettest: Tweak input for test 7 virnetsockettest: Allow changing the proxy parameter virnetsockettest: Increase coverage virnetclient: Escape socket path src/rpc/virnetclient.c | 20 +++--- src/util/virbuffer.c | 17 ++--- tests/virbuftest.c | 9 ++- tests/virnetsockettest.c | 149 ++++++++++++++++++++++++++++----------- 4 files changed, 133 insertions(+), 62 deletions(-) -- 2.34.1
Test the behavior of virBufferEscapeShell for different types of quotes as well as the empty string. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/virbuftest.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/tests/virbuftest.c b/tests/virbuftest.c index 977e0883a6..79bd0cae8e 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -20,7 +20,8 @@ static int testBufAutoIndent(const void *data G_GNUC_UNUSED) g_auto(virBuffer) bufinit = VIR_BUFFER_INITIALIZER; virBuffer *buf = &bufinit; const char expected[] = - " 1\n 2\n 3\n 4\n 5\n 6\n 7\n &\n 8\n 9\n 10\n ' 11'\n"; + " 1\n 2\n 3\n 4\n 5\n 6\n 7\n &\n 8\n 9\n 10\n" + " ' 11'\n ''\\''12'\n '\"13'\n ''\n"; g_autofree char *result = NULL; int ret = 0; @@ -85,6 +86,12 @@ static int testBufAutoIndent(const void *data G_GNUC_UNUSED) virBufferAddChar(buf, '\n'); virBufferEscapeShell(buf, " 11"); virBufferAddChar(buf, '\n'); + virBufferEscapeShell(buf, "'12"); + virBufferAddChar(buf, '\n'); + virBufferEscapeShell(buf, "\"13"); + virBufferAddChar(buf, '\n'); + virBufferEscapeShell(buf, ""); + virBufferAddChar(buf, '\n'); result = virBufferContentAndReset(buf); if (!result || STRNEQ(result, expected)) { -- 2.34.1
We can exit early when the input is an empty string, and we can avoid storing the string length in a variable since we only use that information once. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/util/virbuffer.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index a4834174a1..19cf775a8c 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -552,7 +552,6 @@ virBufferURIEncodeString(virBuffer *buf, const char *str) void virBufferEscapeShell(virBuffer *buf, const char *str) { - int len; g_autofree char *escaped = NULL; char *out; const char *cur; @@ -560,21 +559,19 @@ virBufferEscapeShell(virBuffer *buf, const char *str) if ((buf == NULL) || (str == NULL)) return; - /* Only quote if str includes shell metacharacters. */ - if (*str && !strpbrk(str, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~")) { - virBufferAdd(buf, str, -1); + if (!*str) { + virBufferAddLit(buf, "''"); return; } - if (*str) { - len = strlen(str); - - escaped = g_malloc0_n(len + 1, 4); - } else { - virBufferAddLit(buf, "''"); + /* Only quote if str includes shell metacharacters. */ + if (!strpbrk(str, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~")) { + virBufferAdd(buf, str, -1); return; } + escaped = g_malloc0_n(strlen(str) + 1, 4); + cur = str; out = escaped; -- 2.34.1
No need to escape a single quote. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/virnetsockettest.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 447cbee89c..0928f7056a 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -664,7 +664,7 @@ mymain(void) .netcat = "/tmp/fo o/nc", .path = "/tmp/socket", .expectOut = "-T -e none -- somehost sh -c '" - "if \'''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if '''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0;" "else " "ARG=;" -- 2.34.1
Make this test case consistent with all the other ones. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/virnetsockettest.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 0928f7056a..429c4e5f9d 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -630,8 +630,8 @@ mymain(void) .nodename = "crashyhost", .path = "/tmp/socket", .netcat = "nc", - .expectOut = "-T -e none -- crashyhost sh -c " - "'if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + .expectOut = "-T -e none -- crashyhost sh -c '" + "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0;" "else " "ARG=;" -- 2.34.1
Having the actual script indented and the closing quote on a separate line, like sh -c ' if foo; then bar; fi ' makes things more readable and easier to scan visually. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/virnetsockettest.c | 78 +++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 36 deletions(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 429c4e5f9d..d6bcfc93d1 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -570,12 +570,13 @@ mymain(void) .path = "/tmp/socket", .netcat = "nc", .expectOut = "-T -e none -- somehost sh -c '" - "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" - "else " - "ARG=;" - "fi;" - "'nc' $ARG -U /tmp/socket'\n", + "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" + "else " + "ARG=;" + "fi;" + "'nc' $ARG -U /tmp/socket" + "'\n", }; if (virTestRun("SSH test 1", testSocketSSH, &sshData1) < 0) ret = -1; @@ -589,12 +590,13 @@ mymain(void) .noVerify = false, .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o BatchMode=yes -- somehost sh -c '" - "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" - "else " - "ARG=;" - "fi;" - "'netcat' $ARG -U /tmp/socket'\n", + "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" + "else " + "ARG=;" + "fi;" + "'netcat' $ARG -U /tmp/socket" + "'\n", }; if (virTestRun("SSH test 2", testSocketSSH, &sshData2) < 0) ret = -1; @@ -608,12 +610,13 @@ mymain(void) .noVerify = true, .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o StrictHostKeyChecking=no -- somehost sh -c '" - "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" - "else " - "ARG=;" - "fi;" - "'netcat' $ARG -U /tmp/socket'\n", + "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" + "else " + "ARG=;" + "fi;" + "'netcat' $ARG -U /tmp/socket" + "'\n", }; if (virTestRun("SSH test 3", testSocketSSH, &sshData3) < 0) ret = -1; @@ -631,12 +634,13 @@ mymain(void) .path = "/tmp/socket", .netcat = "nc", .expectOut = "-T -e none -- crashyhost sh -c '" - "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" - "else " - "ARG=;" - "fi;" - "'nc' $ARG -U /tmp/socket'\n", + "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" + "else " + "ARG=;" + "fi;" + "'nc' $ARG -U /tmp/socket" + "'\n", .dieEarly = true, }; if (virTestRun("SSH test 5", testSocketSSH, &sshData5) < 0) @@ -649,12 +653,13 @@ mymain(void) .keyfile = "/root/.ssh/example_key", .noVerify = true, .expectOut = "-i /root/.ssh/example_key -T -e none -o StrictHostKeyChecking=no -- example.com sh -c '" - "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" - "else " - "ARG=;" - "fi;" - "'nc' $ARG -U /tmp/socket'\n", + "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" + "else " + "ARG=;" + "fi;" + "'nc' $ARG -U /tmp/socket" + "'\n", }; if (virTestRun("SSH test 6", testSocketSSH, &sshData6) < 0) ret = -1; @@ -664,12 +669,13 @@ mymain(void) .netcat = "/tmp/fo o/nc", .path = "/tmp/socket", .expectOut = "-T -e none -- somehost sh -c '" - "if '''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" - "else " - "ARG=;" - "fi;" - "'''\\''/tmp/fo o/nc'\\'''' $ARG -U /tmp/socket'\n", + "if '''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" + "else " + "ARG=;" + "fi;" + "'''\\''/tmp/fo o/nc'\\'''' $ARG -U /tmp/socket" + "'\n", }; VIR_WARNINGS_RESET if (virTestRun("SSH test 7", testSocketSSH, &sshData7) < 0) -- 2.34.1
This results in the generated script having consistent spacing throughout, instead of having repeated whitespace in some parts and commands that are separated by a semicolon and no spacing at all in others. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/rpc/virnetclient.c | 12 ++++++------ tests/virnetsockettest.c | 36 ++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 2998551cd8..cbefa8f11f 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -436,10 +436,10 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, nccmd = g_strdup_printf( "if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'%s' $ARG -U %s", netcatPathSafe, netcatPathSafe, socketPath); @@ -451,9 +451,9 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, case VIR_NET_CLIENT_PROXY_AUTO: return g_strdup_printf("sh -c 'which virt-ssh-helper 1>/dev/null 2>&1; " "if test $? = 0; then " - " %s; " - "else" - " %s; " + "%s; " + "else " + "%s; " "fi'", helpercmd, nccmd); case VIR_NET_CLIENT_PROXY_NETCAT: diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index d6bcfc93d1..c1d3e39856 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -571,10 +571,10 @@ mymain(void) .netcat = "nc", .expectOut = "-T -e none -- somehost sh -c '" "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'nc' $ARG -U /tmp/socket" "'\n", }; @@ -591,10 +591,10 @@ mymain(void) .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o BatchMode=yes -- somehost sh -c '" "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'netcat' $ARG -U /tmp/socket" "'\n", }; @@ -611,10 +611,10 @@ mymain(void) .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o StrictHostKeyChecking=no -- somehost sh -c '" "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'netcat' $ARG -U /tmp/socket" "'\n", }; @@ -635,10 +635,10 @@ mymain(void) .netcat = "nc", .expectOut = "-T -e none -- crashyhost sh -c '" "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'nc' $ARG -U /tmp/socket" "'\n", .dieEarly = true, @@ -654,10 +654,10 @@ mymain(void) .noVerify = true, .expectOut = "-i /root/.ssh/example_key -T -e none -o StrictHostKeyChecking=no -- example.com sh -c '" "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'nc' $ARG -U /tmp/socket" "'\n", }; @@ -670,10 +670,10 @@ mymain(void) .path = "/tmp/socket", .expectOut = "-T -e none -- somehost sh -c '" "if '''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'''\\''/tmp/fo o/nc'\\'''' $ARG -U /tmp/socket" "'\n", }; -- 2.34.1
On Fri, Feb 11, 2022 at 06:39:14PM +0100, Andrea Bolognani wrote:
This results in the generated script having consistent spacing throughout, instead of having repeated whitespace in some parts and commands that are separated by a semicolon and no spacing at all in others.
This is going to break apps that use ssh authorized_keys to strictly control the command libvirt invokves, so given that it has no functional benefit, we should not do this.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/rpc/virnetclient.c | 12 ++++++------ tests/virnetsockettest.c | 36 ++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 24 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 2998551cd8..cbefa8f11f 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -436,10 +436,10 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy,
nccmd = g_strdup_printf( "if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'%s' $ARG -U %s", netcatPathSafe, netcatPathSafe, socketPath);
@@ -451,9 +451,9 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, case VIR_NET_CLIENT_PROXY_AUTO: return g_strdup_printf("sh -c 'which virt-ssh-helper 1>/dev/null 2>&1; " "if test $? = 0; then " - " %s; " - "else" - " %s; " + "%s; " + "else " + "%s; " "fi'", helpercmd, nccmd);
case VIR_NET_CLIENT_PROXY_NETCAT: diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index d6bcfc93d1..c1d3e39856 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -571,10 +571,10 @@ mymain(void) .netcat = "nc", .expectOut = "-T -e none -- somehost sh -c '" "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'nc' $ARG -U /tmp/socket" "'\n", }; @@ -591,10 +591,10 @@ mymain(void) .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o BatchMode=yes -- somehost sh -c '" "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'netcat' $ARG -U /tmp/socket" "'\n", }; @@ -611,10 +611,10 @@ mymain(void) .path = "/tmp/socket", .expectOut = "-p 9000 -l fred -T -e none -o StrictHostKeyChecking=no -- somehost sh -c '" "if 'netcat' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'netcat' $ARG -U /tmp/socket" "'\n", }; @@ -635,10 +635,10 @@ mymain(void) .netcat = "nc", .expectOut = "-T -e none -- crashyhost sh -c '" "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'nc' $ARG -U /tmp/socket" "'\n", .dieEarly = true, @@ -654,10 +654,10 @@ mymain(void) .noVerify = true, .expectOut = "-i /root/.ssh/example_key -T -e none -o StrictHostKeyChecking=no -- example.com sh -c '" "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'nc' $ARG -U /tmp/socket" "'\n", }; @@ -670,10 +670,10 @@ mymain(void) .path = "/tmp/socket", .expectOut = "-T -e none -- somehost sh -c '" "if '''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + "ARG=-q0; " "else " - "ARG=;" - "fi;" + "ARG=; " + "fi; " "'''\\''/tmp/fo o/nc'\\'''' $ARG -U /tmp/socket" "'\n", }; -- 2.34.1
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
This makes the generated script a bit shorter and removes an unnecessary call to test. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/rpc/virnetclient.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index cbefa8f11f..7e7e9d52a6 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -449,8 +449,7 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, switch (proxy) { case VIR_NET_CLIENT_PROXY_AUTO: - return g_strdup_printf("sh -c 'which virt-ssh-helper 1>/dev/null 2>&1; " - "if test $? = 0; then " + return g_strdup_printf("sh -c 'if which virt-ssh-helper >/dev/null 2>&1; then " "%s; " "else " "%s; " -- 2.34.1
On Fri, Feb 11, 2022 at 06:39:15PM +0100, Andrea Bolognani wrote:
This makes the generated script a bit shorter and removes an unnecessary call to test.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/rpc/virnetclient.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index cbefa8f11f..7e7e9d52a6 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -449,8 +449,7 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy,
switch (proxy) { case VIR_NET_CLIENT_PROXY_AUTO: - return g_strdup_printf("sh -c 'which virt-ssh-helper 1>/dev/null 2>&1; " - "if test $? = 0; then " + return g_strdup_printf("sh -c 'if which virt-ssh-helper >/dev/null 2>&1; then " "%s; " "else " "%s; "
I understand the motivation, but please don't change this. Applications like OpenStack have configured ssh authorized_keys files with the specific command that libvirt invokes. So changes like this will break their SSH configs. We caused this pain when we first introduced the virt-ssh-helper, but at least that was giving them a functional improvement and they could use a URI parameter to force the old command string. This change is just prettiness for no functional improvement so is not worth breaking apps for. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Fri, Feb 11, 2022 at 05:46:31PM +0000, Daniel P. Berrangé wrote:
- return g_strdup_printf("sh -c 'which virt-ssh-helper 1>/dev/null 2>&1; " - "if test $? = 0; then " + return g_strdup_printf("sh -c 'if which virt-ssh-helper >/dev/null 2>&1; then " "%s; " "else " "%s; "
I understand the motivation, but please don't change this. Applications like OpenStack have configured ssh authorized_keys files with the specific command that libvirt invokes. So changes like this will break their SSH configs. We caused this pain when we first introduced the virt-ssh-helper, but at least that was giving them a functional improvement and they could use a URI parameter to force the old command string. This change is just prettiness for no functional improvement so is not worth breaking apps for.
Can you please provide pointers to the OpenStack implementation of this and the issue that resulted from introducing virt-ssh-helper? AFAICT the only way to restrict what commands a user can run after successfully authenticating is to specify command=... before the corresponding key in authorized_keys and I don't see how this change, or indeed the one that happened when virt-ssh-helper was added, could interfere with that. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Feb 14, 2022 at 01:56:17AM -0800, Andrea Bolognani wrote:
On Fri, Feb 11, 2022 at 05:46:31PM +0000, Daniel P. Berrangé wrote:
- return g_strdup_printf("sh -c 'which virt-ssh-helper 1>/dev/null 2>&1; " - "if test $? = 0; then " + return g_strdup_printf("sh -c 'if which virt-ssh-helper >/dev/null 2>&1; then " "%s; " "else " "%s; "
I understand the motivation, but please don't change this. Applications like OpenStack have configured ssh authorized_keys files with the specific command that libvirt invokes. So changes like this will break their SSH configs. We caused this pain when we first introduced the virt-ssh-helper, but at least that was giving them a functional improvement and they could use a URI parameter to force the old command string. This change is just prettiness for no functional improvement so is not worth breaking apps for.
Can you please provide pointers to the OpenStack implementation of this and the issue that resulted from introducing virt-ssh-helper?
I don't know where the code is. I just know that they were broken by our changes in this area.
AFAICT the only way to restrict what commands a user can run after successfully authenticating is to specify command=... before the corresponding key in authorized_keys and I don't see how this change, or indeed the one that happened when virt-ssh-helper was added, could interfere with that.
The command that was listed in the authorized_keys file no longer matched what libvirt was actually invoking, so it was rightly rejected. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Feb 14, 2022 at 10:25:48AM +0000, Daniel P. Berrangé wrote:
I understand the motivation, but please don't change this. Applications like OpenStack have configured ssh authorized_keys files with the specific command that libvirt invokes. So changes like this will break their SSH configs. We caused this pain when we first introduced the virt-ssh-helper, but at least that was giving them a functional improvement and they could use a URI parameter to force the old command string. This change is just prettiness for no functional improvement so is not worth breaking apps for.
Can you please provide pointers to the OpenStack implementation of this and the issue that resulted from introducing virt-ssh-helper?
I don't know where the code is. I just know that they were broken by our changes in this area.
AFAICT the only way to restrict what commands a user can run after successfully authenticating is to specify command=... before the corresponding key in authorized_keys and I don't see how this change, or indeed the one that happened when virt-ssh-helper was added, could interfere with that.
The command that was listed in the authorized_keys file no longer matched what libvirt was actually invoking, so it was rightly rejected.
I have found some of the issues filed at the time https://bugs.launchpad.net/tripleo/+bug/1918250 https://bugzilla.redhat.com/show_bug.cgi?id=1936804 and the corresponding fix https://github.com/rdo-packages/nova-distgit/commit/d5aba75f3b5589e156afeec9... The detection logic, as currently implemented, is a bit fragile, but updating it so that it keeps working even as we make minor adjustments to our ssh tunnel script wasn't particularly difficult https://github.com/andreabolognani/nova-distgit/commit/27cee8da127c1d447cfb6... I have posted a new version of this series which cleans up things further and actually addresses the original GitLab issue https://listman.redhat.com/archives/libvir-list/2022-February/msg00544.html but I will of course not push it until the nova-migration-wrapper changes mentioned above have been proposed and accepted, which I intend to try next. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Feb 14, 2022 at 08:09:57AM -0800, Andrea Bolognani wrote:
On Mon, Feb 14, 2022 at 10:25:48AM +0000, Daniel P. Berrangé wrote:
I understand the motivation, but please don't change this. Applications like OpenStack have configured ssh authorized_keys files with the specific command that libvirt invokes. So changes like this will break their SSH configs. We caused this pain when we first introduced the virt-ssh-helper, but at least that was giving them a functional improvement and they could use a URI parameter to force the old command string. This change is just prettiness for no functional improvement so is not worth breaking apps for.
Can you please provide pointers to the OpenStack implementation of this and the issue that resulted from introducing virt-ssh-helper?
I don't know where the code is. I just know that they were broken by our changes in this area.
AFAICT the only way to restrict what commands a user can run after successfully authenticating is to specify command=... before the corresponding key in authorized_keys and I don't see how this change, or indeed the one that happened when virt-ssh-helper was added, could interfere with that.
The command that was listed in the authorized_keys file no longer matched what libvirt was actually invoking, so it was rightly rejected.
I have found some of the issues filed at the time
https://bugs.launchpad.net/tripleo/+bug/1918250 https://bugzilla.redhat.com/show_bug.cgi?id=1936804
and the corresponding fix
https://github.com/rdo-packages/nova-distgit/commit/d5aba75f3b5589e156afeec9...
The detection logic, as currently implemented, is a bit fragile, but updating it so that it keeps working even as we make minor adjustments to our ssh tunnel script wasn't particularly difficult
https://github.com/andreabolognani/nova-distgit/commit/27cee8da127c1d447cfb6...
It isn't about whether it is difficult or not though. It is showing that the changes in libvirt are creating a compatibility problem for existing application code, and that any changes we make here will require further changes in such applications. I've not been explicit about back compatibility in this area, as we didn't realize apps would be sensitive to changes. Now we know that, I don't think we should be knowingly making further changes that are likely to break apps.
I have posted a new version of this series which cleans up things further and actually addresses the original GitLab issue
https://listman.redhat.com/archives/libvir-list/2022-February/msg00544.html
but I will of course not push it until the nova-migration-wrapper changes mentioned above have been proposed and accepted, which I intend to try next.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Mon, Feb 14, 2022 at 04:56:37PM +0000, Daniel P. Berrangé wrote:
On Mon, Feb 14, 2022 at 08:09:57AM -0800, Andrea Bolognani wrote:
Can you please provide pointers to the OpenStack implementation of this and the issue that resulted from introducing virt-ssh-helper?
I don't know where the code is. I just know that they were broken by our changes in this area.
I have found some of the issues filed at the time
https://bugs.launchpad.net/tripleo/+bug/1918250 https://bugzilla.redhat.com/show_bug.cgi?id=1936804
and the corresponding fix
https://github.com/rdo-packages/nova-distgit/commit/d5aba75f3b5589e156afeec9...
The detection logic, as currently implemented, is a bit fragile, but updating it so that it keeps working even as we make minor adjustments to our ssh tunnel script wasn't particularly difficult
https://github.com/andreabolognani/nova-distgit/commit/27cee8da127c1d447cfb6...
It isn't about whether it is difficult or not though. It is showing that the changes in libvirt are creating a compatibility problem for existing application code, and that any changes we make here will require further changes in such applications. I've not been explicit about back compatibility in this area, as we didn't realize apps would be sensitive to changes. Now we know that, I don't think we should be knowingly making further changes that are likely to break apps.
I think it's unlikely that other management applications are performing this kind of naive string matching on our ssh tunnel script, as evidenced by the fact that there haven't been widespread breakages reported at the time virt-ssh-helper was introduced. While obviously not definitive proof of this, a search for "virt-ssh-helper" on GitHub https://github.com/search?q=virt-ssh-helper&type=code only finds the OpenStack code that was affected by this; searching for "nc ARG -U" https://github.com/search?q=%22nc+ARG+-U%22&type=code brings up OpenStack again and a project called "virt-access" which hasn't seen a single update in 7 years. So it seems reasonable to conclude that, as long as we update OpenStack before merging these changes, they're not going to break any deployment. I've also just realized that the OpenStack fix mentioned above is less than two weeks old, so there's probably a fair chance that it hasn't made it into a release yet and we can replace the current approach with the more resilient one I implemented in a completely seamless manner. -- Andrea Bolognani / Red Hat / Virtualization
The important part of the value we assign to "netcat" is that it contains whitespace, so drop everything else to highlight this fact. Change the path to the socket so that it also contains whitespace: this will not actually work outside of the test suite at the moment, but we're going to fix that in an upcoming commit. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/virnetsockettest.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index c1d3e39856..557d781605 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -666,15 +666,15 @@ mymain(void) struct testSSHData sshData7 = { .nodename = "somehost", - .netcat = "/tmp/fo o/nc", - .path = "/tmp/socket", + .netcat = "n c", + .path = "/tmp/sock et", .expectOut = "-T -e none -- somehost sh -c '" - "if '''\\''/tmp/fo o/nc'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "if '''\\''n c'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " "ARG=-q0; " "else " "ARG=; " "fi; " - "'''\\''/tmp/fo o/nc'\\'''' $ARG -U /tmp/socket" + "'''\\''n c'\\'''' $ARG -U /tmp/sock et" "'\n", }; VIR_WARNINGS_RESET -- 2.34.1
Currently the test cases all follow the proxy=auto behavior, but we want to add coverage for other proxy modes as well. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/virnetsockettest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 557d781605..36f6993ba5 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -441,6 +441,7 @@ struct testSSHData { const char *username; bool noTTY; bool noVerify; + virNetClientProxy proxy; const char *netcat; const char *keyfile; const char *path; @@ -456,7 +457,7 @@ static int testSocketSSH(const void *opaque) virNetSocket *csock = NULL; /* Client socket */ int ret = -1; char buf[1024]; - g_autofree char *command = virNetClientSSHHelperCommand(VIR_NET_CLIENT_PROXY_AUTO, + g_autofree char *command = virNetClientSSHHelperCommand(data->proxy, data->netcat, data->path, "qemu:///session", -- 2.34.1
Add test cases for quotes appearing in the netcat and socket parameters, for the default behavior of proxy=auto where virt-ssh-helper is used if available, and for proxy=native. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- tests/virnetsockettest.c | 62 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 36f6993ba5..b0e26bc2b2 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -678,10 +678,70 @@ mymain(void) "'''\\''n c'\\'''' $ARG -U /tmp/sock et" "'\n", }; - VIR_WARNINGS_RESET if (virTestRun("SSH test 7", testSocketSSH, &sshData7) < 0) ret = -1; + struct testSSHData sshData8 = { + .nodename = "somehost", + .netcat = "n'c", + .path = "/tmp/sock'et", + .expectOut = "-T -e none -- somehost sh -c '" + "if '''\\''n'\\''\\'\\'''\\''c'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0; " + "else " + "ARG=; " + "fi; " + "'''\\''n'\\''\\'\\'''\\''c'\\'''' $ARG -U /tmp/sock'et" + "'\n", + }; + if (virTestRun("SSH test 8", testSocketSSH, &sshData8) < 0) + ret = -1; + + struct testSSHData sshData9 = { + .nodename = "somehost", + .netcat = "n\"c", + .path = "/tmp/sock\"et", + .expectOut = "-T -e none -- somehost sh -c '" + "if '''\\''n\"c'\\'''' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0; " + "else " + "ARG=; " + "fi; " + "'''\\''n\"c'\\'''' $ARG -U /tmp/sock\"et" + "'\n", + }; + if (virTestRun("SSH test 9", testSocketSSH, &sshData9) < 0) + ret = -1; + + struct testSSHData sshData10 = { + .nodename = "somehost", + .path = "/tmp/socket", + .expectOut = "-T -e none -- somehost sh -c '" + "if which virt-ssh-helper >/dev/null 2>&1; then " + "virt-ssh-helper -r 'qemu:///session'; " + "else " + "if 'nc' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0; " + "else " + "ARG=; " + "fi; " + "'nc' $ARG -U /tmp/socket; " + "fi" + "'\n" + }; + if (virTestRun("SSH test 10", testSocketSSH, &sshData10) < 0) + ret = -1; + + struct testSSHData sshData11 = { + .nodename = "somehost", + .proxy = VIR_NET_CLIENT_PROXY_NATIVE, + .expectOut = "-T -e none -- somehost sh -c '" + "virt-ssh-helper -r 'qemu:///session'" + "'\n" + }; + if (virTestRun("SSH test 11", testSocketSSH, &sshData11) < 0) + ret = -1; + VIR_WARNINGS_RESET #endif return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.34.1
Just like the name of the netcat command and the connection URI, the socket path is a user-provided piece of information that might contain characters that have special meaning for the shell, and as such should be escaped. Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/rpc/virnetclient.c | 5 +++-- tests/virnetsockettest.c | 18 +++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 7e7e9d52a6..563cffff85 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -422,6 +422,7 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, { g_autofree char *netcatPathSafe = virNetClientDoubleEscapeShell(netcatPath ? netcatPath : "nc"); g_autofree char *driverURISafe = virNetClientDoubleEscapeShell(driverURI); + g_autofree char *socketPathSafe = virNetClientDoubleEscapeShell(socketPath); g_autofree char *nccmd = NULL; g_autofree char *helpercmd = NULL; @@ -440,8 +441,8 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, "else " "ARG=; " "fi; " - "'%s' $ARG -U %s", - netcatPathSafe, netcatPathSafe, socketPath); + "'%s' $ARG -U '%s'", + netcatPathSafe, netcatPathSafe, socketPathSafe); helpercmd = g_strdup_printf("virt-ssh-helper%s'%s'", readonly ? " -r " : " ", diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index b0e26bc2b2..d79027d637 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -576,7 +576,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "'nc' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 1", testSocketSSH, &sshData1) < 0) @@ -596,7 +596,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'netcat' $ARG -U /tmp/socket" + "'netcat' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 2", testSocketSSH, &sshData2) < 0) @@ -616,7 +616,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'netcat' $ARG -U /tmp/socket" + "'netcat' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 3", testSocketSSH, &sshData3) < 0) @@ -640,7 +640,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "'nc' $ARG -U '/tmp/socket'" "'\n", .dieEarly = true, }; @@ -659,7 +659,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "'nc' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 6", testSocketSSH, &sshData6) < 0) @@ -675,7 +675,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'''\\''n c'\\'''' $ARG -U /tmp/sock et" + "'''\\''n c'\\'''' $ARG -U '''\\''/tmp/sock et'\\''''" "'\n", }; if (virTestRun("SSH test 7", testSocketSSH, &sshData7) < 0) @@ -691,7 +691,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'''\\''n'\\''\\'\\'''\\''c'\\'''' $ARG -U /tmp/sock'et" + "'''\\''n'\\''\\'\\'''\\''c'\\'''' $ARG -U '''\\''/tmp/sock'\\''\\'\\'''\\''et'\\''''" "'\n", }; if (virTestRun("SSH test 8", testSocketSSH, &sshData8) < 0) @@ -707,7 +707,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'''\\''n\"c'\\'''' $ARG -U /tmp/sock\"et" + "'''\\''n\"c'\\'''' $ARG -U '''\\''/tmp/sock\"et'\\''''" "'\n", }; if (virTestRun("SSH test 9", testSocketSSH, &sshData9) < 0) @@ -725,7 +725,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket; " + "'nc' $ARG -U '/tmp/socket'; " "fi" "'\n" }; -- 2.34.1
On Fri, Feb 11, 2022 at 06:39:19PM +0100, Andrea Bolognani wrote:
Just like the name of the netcat command and the connection URI, the socket path is a user-provided piece of information that might contain characters that have special meaning for the shell, and as such should be escaped.
Signed-off-by: Andrea Bolognani <abologna@redhat.com> --- src/rpc/virnetclient.c | 5 +++-- tests/virnetsockettest.c | 18 +++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 7e7e9d52a6..563cffff85 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -422,6 +422,7 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, { g_autofree char *netcatPathSafe = virNetClientDoubleEscapeShell(netcatPath ? netcatPath : "nc"); g_autofree char *driverURISafe = virNetClientDoubleEscapeShell(driverURI); + g_autofree char *socketPathSafe = virNetClientDoubleEscapeShell(socketPath); g_autofree char *nccmd = NULL; g_autofree char *helpercmd = NULL;
@@ -440,8 +441,8 @@ virNetClientSSHHelperCommand(virNetClientProxy proxy, "else " "ARG=; " "fi; " - "'%s' $ARG -U %s", - netcatPathSafe, netcatPathSafe, socketPath); + "'%s' $ARG -U '%s'", + netcatPathSafe, netcatPathSafe, socketPathSafe);
helpercmd = g_strdup_printf("virt-ssh-helper%s'%s'", readonly ? " -r " : " ",
Again to avoid breaking apps we really want to minimize the change here. IIUC the escaping should be a no-op for any scenario that previously worked. So we if we avoid using '' unless the path contains whitespace we should be OK IIUC
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index b0e26bc2b2..d79027d637 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -576,7 +576,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "'nc' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 1", testSocketSSH, &sshData1) < 0) @@ -596,7 +596,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'netcat' $ARG -U /tmp/socket" + "'netcat' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 2", testSocketSSH, &sshData2) < 0) @@ -616,7 +616,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'netcat' $ARG -U /tmp/socket" + "'netcat' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 3", testSocketSSH, &sshData3) < 0) @@ -640,7 +640,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "'nc' $ARG -U '/tmp/socket'" "'\n", .dieEarly = true, }; @@ -659,7 +659,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket" + "'nc' $ARG -U '/tmp/socket'" "'\n", }; if (virTestRun("SSH test 6", testSocketSSH, &sshData6) < 0) @@ -675,7 +675,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'''\\''n c'\\'''' $ARG -U /tmp/sock et" + "'''\\''n c'\\'''' $ARG -U '''\\''/tmp/sock et'\\''''" "'\n", }; if (virTestRun("SSH test 7", testSocketSSH, &sshData7) < 0) @@ -691,7 +691,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'''\\''n'\\''\\'\\'''\\''c'\\'''' $ARG -U /tmp/sock'et" + "'''\\''n'\\''\\'\\'''\\''c'\\'''' $ARG -U '''\\''/tmp/sock'\\''\\'\\'''\\''et'\\''''" "'\n", }; if (virTestRun("SSH test 8", testSocketSSH, &sshData8) < 0) @@ -707,7 +707,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'''\\''n\"c'\\'''' $ARG -U /tmp/sock\"et" + "'''\\''n\"c'\\'''' $ARG -U '''\\''/tmp/sock\"et'\\''''" "'\n", }; if (virTestRun("SSH test 9", testSocketSSH, &sshData9) < 0) @@ -725,7 +725,7 @@ mymain(void) "else " "ARG=; " "fi; " - "'nc' $ARG -U /tmp/socket; " + "'nc' $ARG -U '/tmp/socket'; " "fi" "'\n" }; -- 2.34.1
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 2/11/22 18:39, Andrea Bolognani wrote:
I initially started looking into this because of
https://gitlab.com/libvirt/libvirt/-/issues/273
I have now convinced myself that we don't need to change the way we quote things, but in the process I have accumulated several improvements and one bug fix.
Andrea Bolognani (11): virbuftest: Increase coverage virbuffer: Simplify virBufferEscapeShell() virnetsockettest: Drop unnecessary backslash virnetsockettest: Move opening quote virnetsockettest: Improve indentation virnetclient: Improve spacing of ssh script virnetclient: Use 'if' consistently virnetsockettest: Tweak input for test 7 virnetsockettest: Allow changing the proxy parameter virnetsockettest: Increase coverage virnetclient: Escape socket path
src/rpc/virnetclient.c | 20 +++--- src/util/virbuffer.c | 17 ++--- tests/virbuftest.c | 9 ++- tests/virnetsockettest.c | 149 ++++++++++++++++++++++++++++----------- 4 files changed, 133 insertions(+), 62 deletions(-)
For patches 1-5,8-10: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> You'll need to tweak the expected output in some patches, because patches 6 and 7 are missing now, but that's easy to do. Michal
On Mon, Feb 14, 2022 at 10:21:57AM +0100, Michal Prívozník wrote:
On 2/11/22 18:39, Andrea Bolognani wrote:
Andrea Bolognani (11): virbuftest: Increase coverage virbuffer: Simplify virBufferEscapeShell() virnetsockettest: Drop unnecessary backslash virnetsockettest: Move opening quote virnetsockettest: Improve indentation virnetclient: Improve spacing of ssh script virnetclient: Use 'if' consistently virnetsockettest: Tweak input for test 7 virnetsockettest: Allow changing the proxy parameter virnetsockettest: Increase coverage virnetclient: Escape socket path
For patches 1-5,8-10:
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
You'll need to tweak the expected output in some patches, because patches 6 and 7 are missing now, but that's easy to do.
Done. Thanks a lot for the review :) -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Prívozník