rpc: merge logic for generating remote SSH shell script

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@redhat.com>

diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index c8f2e3164266bbfb688d5f628c60eb548f58e400..d398d208809840e367ec9213507c402271f3c928 100644 (file)
--- 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 8d3e0176e0f136b9f9ee4af7c4b0d342564dccf0..9c76eaca5ec5944eb25c962b9086135badb8243c 100644 (file)
--- 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 778910b575def83ea7cd7d0b32e35b7510b5d823..0005de46f319009c0acf0e811e86035c332d5e13 100644 (file)
--- 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 90e0f7c47597c83d72fad284ed6032d119f06a80..ebdeadc4a0a790cb1c3f8926888767e37f56e10a 100644 (file)
--- a/src/rpc/virnetsocket.c
+++ b/src/rpc/virnetsocket.c
@@ -865,14 +865,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;
 
@@ -897,38 +894,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 f2b74f3ccbc6cab6094427e07ec70eaf3d2e0ef9..d39b2704809c05ebe7cd616236d9df912f3edca0 100644 (file)
--- 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 b8baca0c23b06889c8a2ae94d6c83ca7d6430991..e603cf2bf21fb41a37453a1b15a84b703d383ab8 100644 (file)
--- 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;
 
@@ -576,6 +578,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;"
@@ -636,6 +639,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;"
@@ -651,6 +655,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 '"