11:39 a.m.
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
11:39 a.m.
New subject: [libvirt PATCH 01/11] virbuftest: Increase coverage
Test the behavior of virBufferEscapeShell for different types of
quotes as well as the empty string.
Signed-off-by: Andrea Bolognani <abologna(a)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
11:39 a.m.
New subject: [libvirt PATCH 02/11] virbuffer: Simplify virBufferEscapeShell()
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(a)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
11:39 a.m.
New subject: [libvirt PATCH 03/11] virnetsockettest: Drop unnecessary backslash
No need to escape a single quote.
Signed-off-by: Andrea Bolognani <abologna(a)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
11:39 a.m.
New subject: [libvirt PATCH 04/11] virnetsockettest: Move opening quote
Make this test case consistent with all the other ones.
Signed-off-by: Andrea Bolognani <abologna(a)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
11:39 a.m.
New subject: [libvirt PATCH 05/11] virnetsockettest: Improve indentation
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(a)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
11:39 a.m.
New subject: [libvirt PATCH 06/11] virnetclient: Improve spacing of ssh script
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(a)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
12:11 p.m.
New subject: [libvirt PATCH 06/11] virnetclient: Improve spacing of ssh script
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(a)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 :|
11:39 a.m.
New subject: [libvirt PATCH 07/11] virnetclient: Use 'if' consistently
This makes the generated script a bit shorter and removes an
unnecessary call to test.
Signed-off-by: Andrea Bolognani <abologna(a)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
11:46 a.m.
New subject: [libvirt PATCH 07/11] virnetclient: Use 'if' consistently
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(a)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 :|
3:56 a.m.
New subject: [libvirt PATCH 07/11] virnetclient: Use 'if' consistently
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
4:25 a.m.
New subject: [libvirt PATCH 07/11] virnetclient: Use 'if' consistently
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 :|
10:09 a.m.
New subject: [libvirt PATCH 07/11] virnetclient: Use 'if' consistently
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/d5aba75f3b5589e156afe...
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/27cee8da127c1d447c...
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
10:56 a.m.
New subject: [libvirt PATCH 07/11] virnetclient: Use 'if' consistently
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/d5aba75f3b5589e156afe...
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/27cee8da127c1d447c...
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 :|
11:58 a.m.
New subject: [libvirt PATCH 07/11] virnetclient: Use 'if' consistently
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/d5aba75f3b5589e156afe...
>
> 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/27cee8da127c1d447c...
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
11:39 a.m.
New subject: [libvirt PATCH 08/11] virnetsockettest: Tweak input for test 7
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(a)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
11:39 a.m.
New subject: [libvirt PATCH 09/11] virnetsockettest: Allow changing the proxy parameter
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(a)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
11:39 a.m.
New subject: [libvirt PATCH 10/11] virnetsockettest: Increase coverage
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(a)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
11:39 a.m.
New subject: [libvirt PATCH 11/11] virnetclient: Escape socket path
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(a)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
11:49 a.m.
New subject: [libvirt PATCH 11/11] virnetclient: Escape socket path
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(a)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 :|
3:21 a.m.
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(a)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
4:36 a.m.
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(a)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
1004
days inactive
1007
days old
21 comments
3 participants
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Prívozník