Three parts of the code all build up the same SSH shell script
snippet for remote tunneling the RPC protocol, but in slightly
different ways. Combine them all into one helper method in the
virNetClient code, since this logic doesn't really belong in
the virNetSocket code.
Note that the this change means the shell snippet is passed to
the SSH binary as a single arg, instead of three separate args,
but this is functionally identical, as the three separate args
were combined into one already when passed to the remote system.
Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
---
src/libvirt_remote.syms | 1 +
src/rpc/virnetclient.c | 108 ++++++++++++++++++++-------------------
src/rpc/virnetclient.h | 3 ++
src/rpc/virnetsocket.c | 37 +-------------
src/rpc/virnetsocket.h | 3 +-
tests/virnetsockettest.c | 9 +++-
6 files changed, 69 insertions(+), 92 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 0018a0c41d..0b00bce1fa 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -42,6 +42,7 @@ virNetClientSendStream;
virNetClientSendWithReply;
virNetClientSetCloseCallback;
virNetClientSetTLSSession;
+virNetClientSSHHelperCommand;
# rpc/virnetclientprogram.h
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 441f1502a6..bd818df953 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -391,28 +391,74 @@ virNetClientPtr virNetClientNewTCP(const char *nodename,
return virNetClientNew(sock, nodename);
}
+
+/*
+ * The SSH Server uses shell to spawn the command we give
+ * it. Our command then invokes shell again. Thus we need
+ * to apply two levels of escaping, so that commands with
+ * whitespace in their path get correctly interpreted.
+ */
+static char *
+virNetClientDoubleEscapeShell(const char *str)
+{
+ virBuffer buf = VIR_BUFFER_INITIALIZER;
+ g_autofree char *tmp = NULL;
+
+ virBufferEscapeShell(&buf, str);
+
+ tmp = virBufferContentAndReset(&buf);
+
+ virBufferEscapeShell(&buf, tmp);
+
+ return virBufferContentAndReset(&buf);
+}
+
+char *
+virNetClientSSHHelperCommand(const char *netcatPath,
+ const char *socketPath)
+{
+ g_autofree char *netcatPathSafe = virNetClientDoubleEscapeShell(netcatPath);
+
+ return g_strdup_printf(
+ "sh -c "
+ "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
+ "ARG=-q0;"
+ "else "
+ "ARG=;"
+ "fi;"
+ "'%s' $ARG -U %s'",
+ netcatPathSafe, netcatPathSafe, socketPath);
+}
+
+
+#define DEFAULT_VALUE(VAR, VAL) \
+ if (!VAR) \
+ VAR = VAL;
+
virNetClientPtr virNetClientNewSSH(const char *nodename,
const char *service,
const char *binary,
const char *username,
bool noTTY,
bool noVerify,
- const char *netcat,
+ const char *netcatPath,
const char *keyfile,
- const char *path)
+ const char *socketPath)
{
virNetSocketPtr sock;
+ g_autofree char *command = NULL;
+
+ DEFAULT_VALUE(netcatPath, "nc");
+
+ command = virNetClientSSHHelperCommand(netcatPath, socketPath);
if (virNetSocketNewConnectSSH(nodename, service, binary, username, noTTY,
- noVerify, netcat, keyfile, path, &sock) < 0)
+ noVerify, keyfile, command, &sock) < 0)
return NULL;
return virNetClientNew(sock, NULL);
}
-#define DEFAULT_VALUE(VAR, VAL) \
- if (!VAR) \
- VAR = VAL;
virNetClientPtr virNetClientNewLibSSH2(const char *host,
const char *port,
int family,
@@ -427,11 +473,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
virURIPtr uri)
{
virNetSocketPtr sock = NULL;
-
- g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- g_autofree char *nc = NULL;
g_autofree char *command = NULL;
-
g_autofree char *homedir = NULL;
g_autofree char *confdir = NULL;
g_autofree char *knownhosts = NULL;
@@ -442,9 +484,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
knownhosts = g_strdup(knownHostsPath);
} else {
confdir = virGetUserConfigDirectory();
- virBufferAsprintf(&buf, "%s/known_hosts", confdir);
- if (!(knownhosts = virBufferContentAndReset(&buf)))
- return NULL;
+ knownhosts = g_strdup_printf("%s/known_hosts", confdir);
}
if (privkeyPath) {
@@ -468,26 +508,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
DEFAULT_VALUE(netcatPath, "nc");
DEFAULT_VALUE(knownHostsVerify, "normal");
- virBufferEscapeShell(&buf, netcatPath);
- if (!(nc = virBufferContentAndReset(&buf)))
- return NULL;
- virBufferEscapeShell(&buf, nc);
- VIR_FREE(nc);
- if (!(nc = virBufferContentAndReset(&buf)))
- return NULL;
-
- virBufferAsprintf(&buf,
- "sh -c "
- "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
- "ARG=-q0;"
- "else "
- "ARG=;"
- "fi;"
- "'%s' $ARG -U %s'",
- nc, nc, socketPath);
-
- if (!(command = virBufferContentAndReset(&buf)))
- return NULL;
+ command = virNetClientSSHHelperCommand(netcatPath, socketPath);
if (virNetSocketNewConnectLibSSH2(host, port,
family,
@@ -498,11 +519,7 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host,
return virNetClientNew(sock, NULL);
}
-#undef DEFAULT_VALUE
-#define DEFAULT_VALUE(VAR, VAL) \
- if (!VAR) \
- VAR = VAL;
virNetClientPtr virNetClientNewLibssh(const char *host,
const char *port,
int family,
@@ -517,11 +534,7 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
virURIPtr uri)
{
virNetSocketPtr sock = NULL;
-
- g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
- g_autofree char *nc = NULL;
g_autofree char *command = NULL;
-
g_autofree char *homedir = NULL;
g_autofree char *confdir = NULL;
g_autofree char *knownhosts = NULL;
@@ -556,18 +569,7 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
DEFAULT_VALUE(netcatPath, "nc");
DEFAULT_VALUE(knownHostsVerify, "normal");
- virBufferEscapeShell(&buf, netcatPath);
- if (!(nc = virBufferContentAndReset(&buf)))
- return NULL;
- virBufferEscapeShell(&buf, nc);
- VIR_FREE(nc);
- if (!(nc = virBufferContentAndReset(&buf)))
- return NULL;
-
- command = g_strdup_printf("sh -c "
- "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
- "ARG=-q0;" "else " "ARG=;" "fi;" "'%s' $ARG -U %s'", nc, nc,
- socketPath);
+ command = virNetClientSSHHelperCommand(netcatPath, socketPath);
if (virNetSocketNewConnectLibssh(host, port,
family,
diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h
index 778910b575..0005de46f3 100644
--- a/src/rpc/virnetclient.h
+++ b/src/rpc/virnetclient.h
@@ -30,6 +30,9 @@
#include "virobject.h"
#include "viruri.h"
+char *
+virNetClientSSHHelperCommand(const char *netcatPath,
+ const char *socketPath);
virNetClientPtr virNetClientNewUNIX(const char *path,
bool spawnDaemon,
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c
index 977d2875fe..485e8229a8 100644
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -862,14 +862,11 @@ int virNetSocketNewConnectSSH(const char *nodename,
const char *username,
bool noTTY,
bool noVerify,
- const char *netcat,
const char *keyfile,
- const char *path,
+ const char *command,
virNetSocketPtr *retsock)
{
- char *quoted;
virCommandPtr cmd;
- g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
*retsock = NULL;
@@ -894,38 +891,8 @@ int virNetSocketNewConnectSSH(const char *nodename,
if (noVerify)
virCommandAddArgList(cmd, "-o", "StrictHostKeyChecking=no", NULL);
- if (!netcat)
- netcat = "nc";
-
- virCommandAddArgList(cmd, "--", nodename, "sh", "-c", NULL);
-
- virBufferEscapeShell(&buf, netcat);
- quoted = virBufferContentAndReset(&buf);
+ virCommandAddArgList(cmd, "--", nodename, command, NULL);
- virBufferEscapeShell(&buf, quoted);
- VIR_FREE(quoted);
- quoted = virBufferContentAndReset(&buf);
-
- /*
- * This ugly thing is a shell script to detect availability of
- * the -q option for 'nc': debian and suse based distros need this
- * flag to ensure the remote nc will exit on EOF, so it will go away
- * when we close the connection tunnel. If it doesn't go away, subsequent
- * connection attempts will hang.
- *
- * Fedora's 'nc' doesn't have this option, and defaults to the desired
- * behavior.
- */
- virCommandAddArgFormat(cmd,
- "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then "
- "ARG=-q0;"
- "else "
- "ARG=;"
- "fi;"
- "'%s' $ARG -U %s'",
- quoted, quoted, path);
-
- VIR_FREE(quoted);
return virNetSocketNewConnectCommand(cmd, retsock);
}
diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h
index f2b74f3ccb..d39b270480 100644
--- a/src/rpc/virnetsocket.h
+++ b/src/rpc/virnetsocket.h
@@ -78,9 +78,8 @@ int virNetSocketNewConnectSSH(const char *nodename,
const char *username,
bool noTTY,
bool noVerify,
- const char *netcat,
const char *keyfile,
- const char *path,
+ const char *command,
virNetSocketPtr *addr);
int virNetSocketNewConnectLibSSH2(const char *host,
diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c
index 96c582216c..31fe1f86f4 100644
--- a/tests/virnetsockettest.c
+++ b/tests/virnetsockettest.c
@@ -32,6 +32,7 @@
#include "virstring.h"
#include "rpc/virnetsocket.h"
+#include "rpc/virnetclient.h"
#define VIR_FROM_THIS VIR_FROM_RPC
@@ -468,6 +469,8 @@ static int testSocketSSH(const void *opaque)
virNetSocketPtr csock = NULL; /* Client socket */
int ret = -1;
char buf[1024];
+ g_autofree char *command = virNetClientSSHHelperCommand(data->netcat,
+ data->path);
if (virNetSocketNewConnectSSH(data->nodename,
data->service,
@@ -475,9 +478,8 @@ static int testSocketSSH(const void *opaque)
data->username,
data->noTTY,
data->noVerify,
- data->netcat,
data->keyfile,
- data->path,
+ command,
&csock) < 0)
goto cleanup;
@@ -575,6 +577,7 @@ mymain(void)
struct testSSHData sshData1 = {
.nodename = "somehost",
.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;"
@@ -635,6 +638,7 @@ mymain(void)
struct testSSHData sshData5 = {
.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 "
"ARG=-q0;"
@@ -650,6 +654,7 @@ mymain(void)
struct testSSHData sshData6 = {
.nodename = "example.com",
.path = "/tmp/socket",
+ .netcat = "nc",
.keyfile = "/root/.ssh/example_key",
.noVerify = true,
.expectOut = "-i /root/.ssh/example_key -T -e none -o StrictHostKeyChecking=no -- example.com sh -c '"
--
2.26.2
