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>

src/libvirt_remote.syms

@@ -42,6 +42,7 @@ virNetClientSendStream;
 virNetClientSendWithReply;
 virNetClientSetCloseCallback;
 virNetClientSetTLSSession;
+virNetClientSSHHelperCommand;
 
 
 # rpc/virnetclientprogram.h

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,101 +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;
-    g_autofree char *privkey = NULL;
-
-    /* Use default paths for known hosts an public keys if not provided */
-    if (knownHostsPath) {
-        knownhosts = g_strdup(knownHostsPath);
-    } else {
-        confdir = virGetUserConfigDirectory();
-        virBufferAsprintf(&buf, "%s/known_hosts", confdir);
-        if (!(knownhosts = virBufferContentAndReset(&buf)))
-            return NULL;
-    }
-
-    if (privkeyPath) {
-        privkey = g_strdup(privkeyPath);
-    } else {
-        homedir = virGetUserDirectory();
-        if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
-            return NULL;
-    }
-
-    if (!authMethods) {
-        if (privkey)
-            authMethods = "agent,privkey,password,keyboard-interactive";
-        else
-            authMethods = "agent,password,keyboard-interactive";
-    }
-
-    DEFAULT_VALUE(host, "localhost");
-    DEFAULT_VALUE(port, "22");
-    DEFAULT_VALUE(username, "root");
-    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;
-
-    if (virNetSocketNewConnectLibSSH2(host, port,
-                                      family,
-                                      username, privkey,
-                                      knownhosts, knownHostsVerify, authMethods,
-                                      command, authPtr, uri, &sock) != 0)
-        return NULL;
-
-   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,
-                                      const char *username,
-                                      const char *privkeyPath,
-                                      const char *knownHostsPath,
-                                      const char *knownHostsVerify,
-                                      const char *authMethods,
-                                      const char *netcatPath,
-                                      const char *socketPath,
-                                      virConnectAuthPtr authPtr,
-                                      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 +508,68 @@ virNetClientPtr virNetClientNewLibssh(const char *host,
     DEFAULT_VALUE(netcatPath, "nc");
     DEFAULT_VALUE(knownHostsVerify, "normal");
 
-    virBufferEscapeShell(&buf, netcatPath);
+    command = virNetClientSSHHelperCommand(netcatPath, socketPath);
-    if (!(nc = virBufferContentAndReset(&buf)))
+
-        return NULL;
+    if (virNetSocketNewConnectLibSSH2(host, port,
-    virBufferEscapeShell(&buf, nc);
+                                      family,
-    VIR_FREE(nc);
+                                      username, privkey,
-    if (!(nc = virBufferContentAndReset(&buf)))
+                                      knownhosts, knownHostsVerify, authMethods,
+                                      command, authPtr, uri, &sock) != 0)
         return NULL;
 
-    command = g_strdup_printf("sh -c "
+   return virNetClientNew(sock, NULL);
-                              "'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);
+virNetClientPtr virNetClientNewLibssh(const char *host,
+                                      const char *port,
+                                      int family,
+                                      const char *username,
+                                      const char *privkeyPath,
+                                      const char *knownHostsPath,
+                                      const char *knownHostsVerify,
+                                      const char *authMethods,
+                                      const char *netcatPath,
+                                      const char *socketPath,
+                                      virConnectAuthPtr authPtr,
+                                      virURIPtr uri)
+{
+    virNetSocketPtr sock = NULL;
+    g_autofree char *command = NULL;
+    g_autofree char *homedir = NULL;
+    g_autofree char *confdir = NULL;
+    g_autofree char *knownhosts = NULL;
+    g_autofree char *privkey = NULL;
+
+    /* Use default paths for known hosts an public keys if not provided */
+    if (knownHostsPath) {
+        knownhosts = g_strdup(knownHostsPath);
+    } else {
+        confdir = virGetUserConfigDirectory();
+        knownhosts = g_strdup_printf("%s/known_hosts", confdir);
+    }
+
+    if (privkeyPath) {
+        privkey = g_strdup(privkeyPath);
+    } else {
+        homedir = virGetUserDirectory();
+        if (virNetClientFindDefaultSshKey(homedir, &privkey) < 0)
+            return NULL;
+    }
+
+    if (!authMethods) {
+        if (privkey)
+            authMethods = "agent,privkey,password,keyboard-interactive";
+        else
+            authMethods = "agent,password,keyboard-interactive";
+    }
+
+    DEFAULT_VALUE(host, "localhost");
+    DEFAULT_VALUE(port, "22");
+    DEFAULT_VALUE(username, "root");
+    DEFAULT_VALUE(netcatPath, "nc");
+    DEFAULT_VALUE(knownHostsVerify, "normal");
+
+    command = virNetClientSSHHelperCommand(netcatPath, socketPath);
 
     if (virNetSocketNewConnectLibssh(host, port,
                                      family,

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,

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)
+    virCommandAddArgList(cmd, "--", nodename, command, NULL);
-        netcat = "nc";
 
-    virCommandAddArgList(cmd, "--", nodename, "sh", "-c", NULL);
-
-    virBufferEscapeShell(&buf, netcat);
-    quoted = virBufferContentAndReset(&buf);
-
-    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);
 }
 

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,

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 '"