[libvirt PATCH 0/9] remote: introduce a custom netcat impl for ssh tunnelling
We have long had a problem with use of netcat for ssh tunnelling because there's no guarantee the UNIX socket path the client builds will match the UNIX socket path the remote host uses. We don't even allow session mode SSH tunnelling for this reason. We also can't easily auto-spawn libvirtd in session mode. With the introduction of modular daemons we also have potential for two completely different UNIX socket paths even for system mode, and the client can't know which to use. The solution to all these problems is to introduce a custom netcat impl. Instead passing the UNIX socket path, we pass the libvirt driver URI. The custom netcat then decides which socket path to use based on the remote build host environment. We still have to support netcat for interoperability with legacy libvirt versions, but we can default to the new virt-nc. Daniel P. Berrangé (9): rpc: merge logic for generating remote SSH shell script remote: split off enums into separate source file remote: split out function for parsing URI scheme remote: parse the remote transport string earlier remote: split out function for constructing socket path remote: extract logic for determining daemon to connect to remote: introduce virtd-nc helper binary rpc: switch order of args in virNetClientNewSSH rpc: use new virt-nc binary for remote tunnelling build-aux/syntax-check.mk | 2 +- docs/uri.html.in | 18 ++ po/POTFILES.in | 2 + src/libvirt_remote.syms | 1 + src/remote/Makefile.inc.am | 32 +++ src/remote/remote_driver.c | 323 +++++---------------------- src/remote/remote_nc.c | 424 ++++++++++++++++++++++++++++++++++++ src/remote/remote_sockets.c | 277 +++++++++++++++++++++++ src/remote/remote_sockets.h | 70 ++++++ src/rpc/virnetclient.c | 151 ++++++++----- src/rpc/virnetclient.h | 29 ++- src/rpc/virnetsocket.c | 37 +--- src/rpc/virnetsocket.h | 4 +- tests/virnetsockettest.c | 12 +- 14 files changed, 1018 insertions(+), 364 deletions(-) create mode 100644 src/remote/remote_nc.c create mode 100644 src/remote/remote_sockets.c create mode 100644 src/remote/remote_sockets.h -- 2.26.2
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 | 1 + src/rpc/virnetclient.c | 105 +++++++++++++++++++++------------------ src/rpc/virnetclient.h | 3 ++ src/rpc/virnetsocket.c | 37 +------------- src/rpc/virnetsocket.h | 3 +- tests/virnetsockettest.c | 9 +++- 6 files changed, 70 insertions(+), 88 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 1c5bef86a1..aee2b52bf6 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -391,28 +391,75 @@ 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, @@ -428,8 +475,6 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, { virNetSocketPtr sock = NULL; - virBuffer buf = VIR_BUFFER_INITIALIZER; - g_autofree char *nc = NULL; g_autofree char *command = NULL; g_autofree char *homedir = NULL; @@ -442,9 +487,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 +511,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 +522,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, @@ -518,8 +538,6 @@ virNetClientPtr virNetClientNewLibssh(const char *host, { virNetSocketPtr sock = NULL; - virBuffer buf = VIR_BUFFER_INITIALIZER; - g_autofree char *nc = NULL; g_autofree char *command = NULL; g_autofree char *homedir = NULL; @@ -556,18 +574,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 3ea863f625..6909a92a93 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -842,14 +842,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; - virBuffer buf = VIR_BUFFER_INITIALIZER; *retsock = NULL; @@ -874,38 +871,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 78fb9cbffd..842eb1bcfc 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 @@ -463,6 +464,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, @@ -470,9 +473,8 @@ static int testSocketSSH(const void *opaque) data->username, data->noTTY, data->noVerify, - data->netcat, data->keyfile, - data->path, + command, &csock) < 0) goto cleanup; @@ -570,6 +572,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;" @@ -630,6 +633,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;" @@ -645,6 +649,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
The remoteDriverTransport and remoteDriverMode enums are going to be needed by source files beyond the remote driver client. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/Makefile.inc.am | 2 ++ src/remote/remote_driver.c | 41 +----------------------------- src/remote/remote_sockets.c | 39 +++++++++++++++++++++++++++++ src/remote/remote_sockets.h | 50 +++++++++++++++++++++++++++++++++++++ 4 files changed, 92 insertions(+), 40 deletions(-) create mode 100644 src/remote/remote_sockets.c create mode 100644 src/remote/remote_sockets.h diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index 1b1be8340d..0ae97f4107 100644 --- a/src/remote/Makefile.inc.am +++ b/src/remote/Makefile.inc.am @@ -13,6 +13,8 @@ REMOTE_DRIVER_GENERATED = \ $(NULL) REMOTE_DRIVER_SOURCES = \ + remote/remote_sockets.c \ + remote/remote_sockets.h \ remote/remote_driver.c \ remote/remote_driver.h \ $(NULL) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 653c68472a..880fce6e62 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -38,6 +38,7 @@ #include "virbuffer.h" #include "remote_driver.h" #include "remote_protocol.h" +#include "remote_sockets.h" #include "lxc_protocol.h" #include "qemu_protocol.h" #include "viralloc.h" @@ -54,46 +55,6 @@ VIR_LOG_INIT("remote.remote_driver"); -typedef enum { - REMOTE_DRIVER_TRANSPORT_TLS, - REMOTE_DRIVER_TRANSPORT_UNIX, - REMOTE_DRIVER_TRANSPORT_SSH, - REMOTE_DRIVER_TRANSPORT_LIBSSH2, - REMOTE_DRIVER_TRANSPORT_EXT, - REMOTE_DRIVER_TRANSPORT_TCP, - REMOTE_DRIVER_TRANSPORT_LIBSSH, - - REMOTE_DRIVER_TRANSPORT_LAST, -} remoteDriverTransport; - -VIR_ENUM_DECL(remoteDriverTransport); -VIR_ENUM_IMPL(remoteDriverTransport, - REMOTE_DRIVER_TRANSPORT_LAST, - "tls", - "unix", - "ssh", - "libssh2", - "ext", - "tcp", - "libssh"); - -typedef enum { - /* Try to figure out the "best" choice magically */ - REMOTE_DRIVER_MODE_AUTO, - /* Always use the legacy libvirtd */ - REMOTE_DRIVER_MODE_LEGACY, - /* Always use the per-driver virt*d daemons */ - REMOTE_DRIVER_MODE_DIRECT, - - REMOTE_DRIVER_MODE_LAST -} remoteDriverMode; - -VIR_ENUM_DECL(remoteDriverMode); -VIR_ENUM_IMPL(remoteDriverMode, - REMOTE_DRIVER_MODE_LAST, - "auto", - "legacy", - "direct"); #if SIZEOF_LONG < 8 # define HYPER_TO_TYPE(_type, _to, _from) \ diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c new file mode 100644 index 0000000000..0662cbad14 --- /dev/null +++ b/src/remote/remote_sockets.c @@ -0,0 +1,39 @@ +/* + * remote_sockets.c: helpers for getting remote driver socket paths + * + * Copyright (C) 2007-2019 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include "remote_sockets.h" + +VIR_ENUM_IMPL(remoteDriverTransport, + REMOTE_DRIVER_TRANSPORT_LAST, + "tls", + "unix", + "ssh", + "libssh2", + "ext", + "tcp", + "libssh"); + +VIR_ENUM_IMPL(remoteDriverMode, + REMOTE_DRIVER_MODE_LAST, + "auto", + "legacy", + "direct"); diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h new file mode 100644 index 0000000000..1d4ae3f9c1 --- /dev/null +++ b/src/remote/remote_sockets.h @@ -0,0 +1,50 @@ +/* + * remote_sockets.h: helpers for getting remote driver socket paths + * + * Copyright (C) 2007-2020 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#pragma once + +#include "virenum.h" + +typedef enum { + REMOTE_DRIVER_TRANSPORT_TLS, + REMOTE_DRIVER_TRANSPORT_UNIX, + REMOTE_DRIVER_TRANSPORT_SSH, + REMOTE_DRIVER_TRANSPORT_LIBSSH2, + REMOTE_DRIVER_TRANSPORT_EXT, + REMOTE_DRIVER_TRANSPORT_TCP, + REMOTE_DRIVER_TRANSPORT_LIBSSH, + + REMOTE_DRIVER_TRANSPORT_LAST, +} remoteDriverTransport; + +VIR_ENUM_DECL(remoteDriverTransport); + +typedef enum { + /* Try to figure out the "best" choice magically */ + REMOTE_DRIVER_MODE_AUTO, + /* Always use the legacy libvirtd */ + REMOTE_DRIVER_MODE_LEGACY, + /* Always use the per-driver virt*d daemons */ + REMOTE_DRIVER_MODE_DIRECT, + + REMOTE_DRIVER_MODE_LAST +} remoteDriverMode; + +VIR_ENUM_DECL(remoteDriverMode); -- 2.26.2
The remoteSplitURISCheme method will be needed by source files beyond the remote driver client. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 25 ------------------------- src/remote/remote_sockets.c | 28 ++++++++++++++++++++++++++++ src/remote/remote_sockets.h | 6 ++++++ 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 880fce6e62..b84b72522a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -165,31 +165,6 @@ static void make_nonnull_domain_snapshot(remote_nonnull_domain_snapshot *snapsho /*----------------------------------------------------------------------*/ /* Helper functions for remoteOpen. */ -static int remoteSplitURIScheme(virURIPtr uri, - char **driver, - char **transport) -{ - char *p = strchr(uri->scheme, '+'); - - *driver = *transport = NULL; - - if (p) - *driver = g_strndup(uri->scheme, p - uri->scheme); - else - *driver = g_strdup(uri->scheme); - - if (p) { - *transport = g_strdup(p + 1); - - p = *transport; - while (*p) { - *p = g_ascii_tolower(*p); - p++; - } - } - - return 0; -} static int diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c index 0662cbad14..976124d0ed 100644 --- a/src/remote/remote_sockets.c +++ b/src/remote/remote_sockets.c @@ -37,3 +37,31 @@ VIR_ENUM_IMPL(remoteDriverMode, "auto", "legacy", "direct"); + + +int +remoteSplitURIScheme(virURIPtr uri, + char **driver, + char **transport) +{ + char *p = strchr(uri->scheme, '+'); + + *driver = *transport = NULL; + + if (p) + *driver = g_strndup(uri->scheme, p - uri->scheme); + else + *driver = g_strdup(uri->scheme); + + if (p) { + *transport = g_strdup(p + 1); + + p = *transport; + while (*p) { + *p = g_ascii_tolower(*p); + p++; + } + } + + return 0; +} diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h index 1d4ae3f9c1..bef3cdada9 100644 --- a/src/remote/remote_sockets.h +++ b/src/remote/remote_sockets.h @@ -21,6 +21,7 @@ #pragma once #include "virenum.h" +#include "viruri.h" typedef enum { REMOTE_DRIVER_TRANSPORT_TLS, @@ -48,3 +49,8 @@ typedef enum { } remoteDriverMode; VIR_ENUM_DECL(remoteDriverMode); + +int +remoteSplitURIScheme(virURIPtr uri, + char **driver, + char **transport); -- 2.26.2
We delay converting the remote transport string to enum form until fairly late. As a result we're doing string comparisons when we could be just doing enum comparisons. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- po/POTFILES.in | 1 + src/remote/remote_driver.c | 51 ++++++++++--------------------------- src/remote/remote_sockets.c | 35 +++++++++++++++++++++---- src/remote/remote_sockets.h | 2 +- 4 files changed, 45 insertions(+), 44 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index af52054aa4..8fd391a63a 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -180,6 +180,7 @@ @SRCDIR@/src/remote/remote_daemon_dispatch.c @SRCDIR@/src/remote/remote_daemon_stream.c @SRCDIR@/src/remote/remote_driver.c +@SRCDIR@/src/remote/remote_sockets.c @SRCDIR@/src/rpc/virkeepalive.c @SRCDIR@/src/rpc/virnetclient.c @SRCDIR@/src/rpc/virnetclientprogram.c diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b84b72522a..c39085951e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -863,12 +863,11 @@ static int doRemoteOpen(virConnectPtr conn, struct private_data *priv, const char *driver_str, - const char *transport_str, + remoteDriverTransport transport, virConnectAuthPtr auth G_GNUC_UNUSED, virConfPtr conf, unsigned int flags) { - int transport; #ifndef WIN32 g_autofree char *daemonPath = NULL; #endif @@ -903,34 +902,6 @@ doRemoteOpen(virConnectPtr conn, /* We handle *ALL* URIs here. The caller has rejected any * URIs we don't care about */ - if (conn->uri) { - if (!transport_str) { - if (conn->uri->server) - transport = REMOTE_DRIVER_TRANSPORT_TLS; - else - transport = REMOTE_DRIVER_TRANSPORT_UNIX; - } else { - if ((transport = remoteDriverTransportTypeFromString(transport_str)) < 0) { - virReportError(VIR_ERR_INVALID_ARG, "%s", - _("remote_open: transport in URL not recognised " - "(should be tls|unix|ssh|ext|tcp|libssh2|libssh)")); - return VIR_DRV_OPEN_ERROR; - } - - if (transport == REMOTE_DRIVER_TRANSPORT_UNIX && - conn->uri->server) { - virReportError(VIR_ERR_INVALID_ARG, - _("using unix socket and remote " - "server '%s' is not supported."), - conn->uri->server); - return VIR_DRV_OPEN_ERROR; - } - } - } else { - /* No URI, then must be probing so use UNIX socket */ - transport = REMOTE_DRIVER_TRANSPORT_UNIX; - } - /* Remote server defaults to "localhost" if not specified. */ if (conn->uri && conn->uri->port != 0) { port = g_strdup_printf("%d", conn->uri->port); @@ -1352,11 +1323,16 @@ remoteConnectOpen(virConnectPtr conn, int rflags = 0; const char *autostart = getenv("LIBVIRT_AUTOSTART"); char *driver = NULL; - char *transport = NULL; + remoteDriverTransport transport; + + if (conn->uri) { + if (remoteSplitURIScheme(conn->uri, &driver, &transport) < 0) + goto cleanup; + } else { + /* No URI, then must be probing so use UNIX socket */ + transport = REMOTE_DRIVER_TRANSPORT_UNIX; + } - if (conn->uri && - remoteSplitURIScheme(conn->uri, &driver, &transport) < 0) - goto cleanup; if (inside_daemon) { if (!conn->uri) { @@ -1398,12 +1374,12 @@ remoteConnectOpen(virConnectPtr conn, rflags |= VIR_DRV_OPEN_REMOTE_USER; /* - * Furthermore if no servername is given, and no +XXX - * transport is listed, or transport is unix, + * Furthermore if no servername is given, + * and the transport is unix, * and uid is unprivileged then auto-spawn a daemon. */ if (!conn->uri->server && - (transport == NULL || STREQ(transport, "unix")) && + (transport == REMOTE_DRIVER_TRANSPORT_UNIX) && (!autostart || STRNEQ(autostart, "0"))) { VIR_DEBUG("Try daemon autostart"); @@ -1438,7 +1414,6 @@ remoteConnectOpen(virConnectPtr conn, cleanup: VIR_FREE(driver); - VIR_FREE(transport); return ret; } diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c index 976124d0ed..cdc0a00293 100644 --- a/src/remote/remote_sockets.c +++ b/src/remote/remote_sockets.c @@ -21,6 +21,9 @@ #include <config.h> #include "remote_sockets.h" +#include "virerror.h" + +#define VIR_FROM_THIS VIR_FROM_REMOTE VIR_ENUM_IMPL(remoteDriverTransport, REMOTE_DRIVER_TRANSPORT_LAST, @@ -42,25 +45,47 @@ VIR_ENUM_IMPL(remoteDriverMode, int remoteSplitURIScheme(virURIPtr uri, char **driver, - char **transport) + remoteDriverTransport *transport) { char *p = strchr(uri->scheme, '+'); - *driver = *transport = NULL; - if (p) *driver = g_strndup(uri->scheme, p - uri->scheme); else *driver = g_strdup(uri->scheme); if (p) { - *transport = g_strdup(p + 1); + g_autofree char *tmp = g_strdup(p + 1); + int val; - p = *transport; + p = tmp; while (*p) { *p = g_ascii_tolower(*p); p++; } + + if ((val = remoteDriverTransportTypeFromString(tmp)) < 0) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("remote_open: transport in URL not recognised " + "(should be tls|unix|ssh|ext|tcp|libssh2|libssh)")); + return -1; + } + + if (val == REMOTE_DRIVER_TRANSPORT_UNIX && + uri->server) { + virReportError(VIR_ERR_INVALID_ARG, + _("using unix socket and remote " + "server '%s' is not supported."), + uri->server); + return -1; + } + + *transport = val; + } else { + if (uri->server) + *transport = REMOTE_DRIVER_TRANSPORT_TLS; + else + *transport = REMOTE_DRIVER_TRANSPORT_UNIX; } return 0; diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h index bef3cdada9..ade3feab88 100644 --- a/src/remote/remote_sockets.h +++ b/src/remote/remote_sockets.h @@ -53,4 +53,4 @@ VIR_ENUM_DECL(remoteDriverMode); int remoteSplitURIScheme(virURIPtr uri, char **driver, - char **transport); + remoteDriverTransport *transport); -- 2.26.2
The remoteGetUNIXSocketHelper method will be needed by source files beyond the remote driver client. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 129 +--------------------------------- src/remote/remote_sockets.c | 134 ++++++++++++++++++++++++++++++++++++ src/remote/remote_sockets.h | 8 +++ 3 files changed, 145 insertions(+), 126 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c39085951e..c7fd24625e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -697,131 +697,6 @@ remoteConnectSupportsFeatureUnlocked(virConnectPtr conn, } -static char * -remoteGetUNIXSocketHelper(remoteDriverTransport transport, - const char *sock_prefix, - unsigned int flags) -{ - char *sockname = NULL; - g_autofree char *userdir = NULL; - - if (flags & VIR_DRV_OPEN_REMOTE_USER) { - if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("Connecting to session instance without " - "socket path is not supported by the %s " - "transport"), - remoteDriverTransportTypeToString(transport)); - return NULL; - } - userdir = virGetUserRuntimeDirectory(); - - sockname = g_strdup_printf("%s/%s-sock", userdir, sock_prefix); - } else { - /* Intentionally do *NOT* use RUNSTATEDIR here. We might - * be connecting to a remote machine, and cannot assume - * the remote host has /run. The converse is ok though, - * any machine with /run will have a /var/run symlink. - * The portable option is to thus use $LOCALSTATEDIR/run - */ - sockname = g_strdup_printf("%s/run/libvirt/%s-%s", LOCALSTATEDIR, - sock_prefix, - flags & VIR_DRV_OPEN_REMOTE_RO ? "sock-ro" : "sock"); - } - - VIR_DEBUG("Built UNIX sockname %s for transport %s prefix %s flags=0x%x", - sockname, remoteDriverTransportTypeToString(transport), - sock_prefix, flags); - return sockname; -} - - -static char * -remoteGetUNIXSocket(remoteDriverTransport transport, - remoteDriverMode mode, - const char *driver, - char **daemon, - unsigned int flags) -{ - char *sock_name = NULL; - g_autofree char *direct_daemon = NULL; - g_autofree char *legacy_daemon = NULL; - g_autofree char *direct_sock_name = NULL; - g_autofree char *legacy_sock_name = NULL; - - if (driver) - direct_daemon = g_strdup_printf("virt%sd", driver); - - legacy_daemon = g_strdup("libvirtd"); - - if (driver && - !(direct_sock_name = remoteGetUNIXSocketHelper(transport, direct_daemon, flags))) - return NULL; - - if (!(legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", flags))) - return NULL; - - if (mode == REMOTE_DRIVER_MODE_AUTO) { - if (transport == REMOTE_DRIVER_TRANSPORT_UNIX) { - if (direct_sock_name && virFileExists(direct_sock_name)) { - mode = REMOTE_DRIVER_MODE_DIRECT; - } else if (virFileExists(legacy_sock_name)) { - mode = REMOTE_DRIVER_MODE_LEGACY; - } else if (driver) { - /* - * This constant comes from the configure script and - * maps to either the direct or legacy mode constant - */ - mode = REMOTE_DRIVER_MODE_DEFAULT; - } else { - mode = REMOTE_DRIVER_MODE_LEGACY; - } - } else { - mode = REMOTE_DRIVER_MODE_LEGACY; - } - } - - switch ((remoteDriverMode)mode) { - case REMOTE_DRIVER_MODE_LEGACY: - sock_name = g_steal_pointer(&legacy_sock_name); - *daemon = g_steal_pointer(&legacy_daemon); - break; - - case REMOTE_DRIVER_MODE_DIRECT: - if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("Cannot use direct socket mode for %s transport"), - remoteDriverTransportTypeToString(transport)); - return NULL; - } - - if (!direct_sock_name) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("Cannot use direct socket mode if no URI is set")); - return NULL; - } - - sock_name = g_steal_pointer(&direct_sock_name); - *daemon = g_steal_pointer(&direct_daemon); - break; - - case REMOTE_DRIVER_MODE_AUTO: - case REMOTE_DRIVER_MODE_LAST: - default: - virReportEnumRangeError(remoteDriverMode, mode); - return NULL; - } - - VIR_DEBUG("Chosen UNIX sockname %s daemon %s " - "for mode %s transport %s flags=0x%x", - sock_name, NULLSTR(*daemon), - remoteDriverModeTypeToString(mode), - remoteDriverTransportTypeToString(transport), - flags); - return sock_name; -} - - #ifndef WIN32 static const char * remoteGetDaemonPathEnv(void) @@ -1015,7 +890,9 @@ doRemoteOpen(virConnectPtr conn, case REMOTE_DRIVER_TRANSPORT_LIBSSH2: if (!sockname && !(sockname = remoteGetUNIXSocket(transport, mode, driver_str, - &daemon_name, flags))) + flags & VIR_DRV_OPEN_REMOTE_RO, + flags & VIR_DRV_OPEN_REMOTE_USER, + &daemon_name))) goto failed; break; diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c index cdc0a00293..28e02e24d5 100644 --- a/src/remote/remote_sockets.c +++ b/src/remote/remote_sockets.c @@ -22,9 +22,15 @@ #include "remote_sockets.h" #include "virerror.h" +#include "virlog.h" +#include "virfile.h" +#include "virutil.h" +#include "configmake.h" #define VIR_FROM_THIS VIR_FROM_REMOTE +VIR_LOG_INIT("remote.remote_sockets"); + VIR_ENUM_IMPL(remoteDriverTransport, REMOTE_DRIVER_TRANSPORT_LAST, "tls", @@ -90,3 +96,131 @@ remoteSplitURIScheme(virURIPtr uri, return 0; } + + +static char * +remoteGetUNIXSocketHelper(remoteDriverTransport transport, + const char *sock_prefix, + bool ro, + bool session) +{ + char *sockname = NULL; + g_autofree char *userdir = NULL; + + if (session) { + if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Connecting to session instance without " + "socket path is not supported by the %s " + "transport"), + remoteDriverTransportTypeToString(transport)); + return NULL; + } + userdir = virGetUserRuntimeDirectory(); + + sockname = g_strdup_printf("%s/%s-sock", userdir, sock_prefix); + } else { + /* Intentionally do *NOT* use RUNSTATEDIR here. We might + * be connecting to a remote machine, and cannot assume + * the remote host has /run. The converse is ok though, + * any machine with /run will have a /var/run symlink. + * The portable option is to thus use $LOCALSTATEDIR/run + */ + sockname = g_strdup_printf("%s/run/libvirt/%s-%s", LOCALSTATEDIR, + sock_prefix, + ro ? "sock-ro" : "sock"); + } + + VIR_DEBUG("Built UNIX sockname=%s for transport=%s " + "prefix=%s ro=%d session=%d", + sockname, remoteDriverTransportTypeToString(transport), + sock_prefix, ro, session); + return sockname; +} + + +char * +remoteGetUNIXSocket(remoteDriverTransport transport, + remoteDriverMode mode, + const char *driver, + bool ro, + bool session, + char **daemon) +{ + char *sock_name = NULL; + g_autofree char *direct_daemon = NULL; + g_autofree char *legacy_daemon = NULL; + g_autofree char *direct_sock_name = NULL; + g_autofree char *legacy_sock_name = NULL; + + if (driver) + direct_daemon = g_strdup_printf("virt%sd", driver); + + legacy_daemon = g_strdup("libvirtd"); + + if (driver && + !(direct_sock_name = remoteGetUNIXSocketHelper(transport, direct_daemon, ro, session))) + return NULL; + + if (!(legacy_sock_name = remoteGetUNIXSocketHelper(transport, "libvirt", ro, session))) + return NULL; + + if (mode == REMOTE_DRIVER_MODE_AUTO) { + if (transport == REMOTE_DRIVER_TRANSPORT_UNIX) { + if (direct_sock_name && virFileExists(direct_sock_name)) { + mode = REMOTE_DRIVER_MODE_DIRECT; + } else if (virFileExists(legacy_sock_name)) { + mode = REMOTE_DRIVER_MODE_LEGACY; + } else if (driver) { + /* + * This constant comes from the configure script and + * maps to either the direct or legacy mode constant + */ + mode = REMOTE_DRIVER_MODE_DEFAULT; + } else { + mode = REMOTE_DRIVER_MODE_LEGACY; + } + } else { + mode = REMOTE_DRIVER_MODE_LEGACY; + } + } + + switch ((remoteDriverMode)mode) { + case REMOTE_DRIVER_MODE_LEGACY: + sock_name = g_steal_pointer(&legacy_sock_name); + *daemon = g_steal_pointer(&legacy_daemon); + break; + + case REMOTE_DRIVER_MODE_DIRECT: + if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, + _("Cannot use direct socket mode for %s transport"), + remoteDriverTransportTypeToString(transport)); + return NULL; + } + + if (!direct_sock_name) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("Cannot use direct socket mode if no URI is set")); + return NULL; + } + + sock_name = g_steal_pointer(&direct_sock_name); + *daemon = g_steal_pointer(&direct_daemon); + break; + + case REMOTE_DRIVER_MODE_AUTO: + case REMOTE_DRIVER_MODE_LAST: + default: + virReportEnumRangeError(remoteDriverMode, mode); + return NULL; + } + + VIR_DEBUG("Chosen UNIX sockname=%s daemon=%s " + "for mode=%s transport=%s ro=%d session=%d", + sock_name, NULLSTR(*daemon), + remoteDriverModeTypeToString(mode), + remoteDriverTransportTypeToString(transport), + ro, session); + return sock_name; +} diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h index ade3feab88..64055f3d44 100644 --- a/src/remote/remote_sockets.h +++ b/src/remote/remote_sockets.h @@ -54,3 +54,11 @@ int remoteSplitURIScheme(virURIPtr uri, char **driver, remoteDriverTransport *transport); + +char * +remoteGetUNIXSocket(remoteDriverTransport transport, + remoteDriverMode mode, + const char *driver, + bool ro, + bool session, + char **daemon); -- 2.26.2
We'll shortly want to reuse code for determining whether to connect to the system or session daemon from places outside the remote driver client. Pulling it out into a self contained function facilitates reuse. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 51 ++++---------------------------- src/remote/remote_sockets.c | 59 +++++++++++++++++++++++++++++++++++++ src/remote/remote_sockets.h | 6 ++++ 3 files changed, 71 insertions(+), 45 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c7fd24625e..c2dcf20f91 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1198,7 +1198,8 @@ remoteConnectOpen(virConnectPtr conn, struct private_data *priv; int ret = VIR_DRV_OPEN_ERROR; int rflags = 0; - const char *autostart = getenv("LIBVIRT_AUTOSTART"); + bool user; + bool autostart; char *driver = NULL; remoteDriverTransport transport; @@ -1233,51 +1234,11 @@ remoteConnectOpen(virConnectPtr conn, if (flags & VIR_CONNECT_RO) rflags |= VIR_DRV_OPEN_REMOTE_RO; - /* - * User session daemon is used for - * - * - Any URI with /session suffix - * - Test driver, if a protocol is given - * - * provided we are running non-root - */ - if (conn->uri && - conn->uri->path && - conn->uri->scheme && - (STREQ(conn->uri->path, "/session") || - STRPREFIX(conn->uri->scheme, "test+")) && - geteuid() > 0) { - VIR_DEBUG("User session daemon required"); + remoteGetURIDaemonInfo(conn->uri, transport, &user, &autostart); + if (user) rflags |= VIR_DRV_OPEN_REMOTE_USER; - - /* - * Furthermore if no servername is given, - * and the transport is unix, - * and uid is unprivileged then auto-spawn a daemon. - */ - if (!conn->uri->server && - (transport == REMOTE_DRIVER_TRANSPORT_UNIX) && - (!autostart || - STRNEQ(autostart, "0"))) { - VIR_DEBUG("Try daemon autostart"); - rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART; - } - } - - /* - * If URI is NULL, then do a UNIX connection possibly auto-spawning - * unprivileged server and probe remote server for URI. - */ - if (!conn->uri) { - VIR_DEBUG("Auto-probe remote URI"); - if (geteuid() > 0) { - VIR_DEBUG("Auto-spawn user daemon instance"); - rflags |= VIR_DRV_OPEN_REMOTE_USER; - if (!autostart || - STRNEQ(autostart, "0")) - rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART; - } - } + if (autostart) + rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART; ret = doRemoteOpen(conn, priv, driver, transport, auth, conf, rflags); if (ret != VIR_DRV_OPEN_SUCCESS) { diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c index 28e02e24d5..854775f401 100644 --- a/src/remote/remote_sockets.c +++ b/src/remote/remote_sockets.c @@ -224,3 +224,62 @@ remoteGetUNIXSocket(remoteDriverTransport transport, ro, session); return sock_name; } + + +void +remoteGetURIDaemonInfo(virURIPtr uri, + remoteDriverTransport transport, + bool *session, + bool *autostart) +{ + const char *autostart_str = getenv("LIBVIRT_AUTOSTART"); + + *session = false; + *autostart = false; + + /* + * User session daemon is used for + * + * - Any URI with /session suffix + * - Test driver, if a protocol is given + * + * provided we are running non-root + */ + if (uri && + uri->path && + uri->scheme && + (STREQ(uri->path, "/session") || + STRPREFIX(uri->scheme, "test+")) && + geteuid() > 0) { + VIR_DEBUG("User session daemon required"); + *session = true; + + /* + * Furthermore if no servername is given, + * and the transport is unix, + * and uid is unprivileged then auto-spawn a daemon. + */ + if (!uri->server && + (transport == REMOTE_DRIVER_TRANSPORT_UNIX) && + (!autostart_str || + STRNEQ(autostart_str, "0"))) { + VIR_DEBUG("Try daemon autostart"); + *autostart = true; + } + } + + /* + * If URI is NULL, then do a UNIX connection possibly auto-spawning + * unprivileged server and probe remote server for URI. + */ + if (!uri) { + VIR_DEBUG("Auto-probe remote URI"); + if (geteuid() > 0) { + VIR_DEBUG("Auto-spawn user daemon instance"); + *session = true; + if (!autostart_str || + STRNEQ(autostart_str, "0")) + *autostart = true; + } + } +} diff --git a/src/remote/remote_sockets.h b/src/remote/remote_sockets.h index 64055f3d44..7526752835 100644 --- a/src/remote/remote_sockets.h +++ b/src/remote/remote_sockets.h @@ -62,3 +62,9 @@ remoteGetUNIXSocket(remoteDriverTransport transport, bool ro, bool session, char **daemon); + +void +remoteGetURIDaemonInfo(virURIPtr uri, + remoteDriverTransport transport, + bool *session, + bool *autostart); -- 2.26.2
When accessing libvirtd over a SSH tunnel, the remote driver must spawn the remote 'nc' process, pointing it to the libvirtd socket path. This is problematic for a number of reasons: - The socket path varies according to the --prefix chosen at build time. The remote client is seeing the local prefix, but what we need is the remote prefix - The socket path varies according to remote env variables, such as the XDG_RUNTIME_DIR location. Again we see the local XDG_RUNTIME_DIR value, but what we need is the remote value (if any) - We can not able to autospawn the libvirtd daemon for session mode access To address these problems this patch introduces the 'virtd-nc' helper program which takes the URI for the remote driver as a CLI parameter. It then figures out the socket path to connect to using the same code as the remote driver does on the remote host. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 2 +- po/POTFILES.in | 1 + src/remote/Makefile.inc.am | 30 +++ src/remote/remote_nc.c | 424 +++++++++++++++++++++++++++++++++++++ src/rpc/virnetsocket.h | 1 + 5 files changed, 457 insertions(+), 1 deletion(-) create mode 100644 src/remote/remote_nc.c diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk index d47a92b530..81b307ebe8 100644 --- a/build-aux/syntax-check.mk +++ b/build-aux/syntax-check.mk @@ -1967,7 +1967,7 @@ group-qemu-caps: # List all syntax-check exemptions: exclude_file_name_regexp--sc_avoid_strcase = ^tools/vsh\.h$$ -_src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon +_src1=libvirt-stream|qemu/qemu_monitor|util/vir(command|file|fdstream)|rpc/virnetsocket|lxc/lxc_controller|locking/lock_daemon|logging/log_daemon|remote/remote_nc _test1=shunloadtest|virnettlscontexttest|virnettlssessiontest|vircgroupmock|commandhelper exclude_file_name_regexp--sc_avoid_write = \ ^(src/($(_src1))|tools/virsh-console|tests/($(_test1)))\.c$$ diff --git a/po/POTFILES.in b/po/POTFILES.in index 8fd391a63a..8fa47ec276 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -180,6 +180,7 @@ @SRCDIR@/src/remote/remote_daemon_dispatch.c @SRCDIR@/src/remote/remote_daemon_stream.c @SRCDIR@/src/remote/remote_driver.c +@SRCDIR@/src/remote/remote_nc.c @SRCDIR@/src/remote/remote_sockets.c @SRCDIR@/src/rpc/virkeepalive.c @SRCDIR@/src/rpc/virnetclient.c diff --git a/src/remote/Makefile.inc.am b/src/remote/Makefile.inc.am index 0ae97f4107..2527cc193f 100644 --- a/src/remote/Makefile.inc.am +++ b/src/remote/Makefile.inc.am @@ -221,6 +221,8 @@ if WITH_LIBVIRTD sbin_PROGRAMS += libvirtd virtproxyd +libexec_PROGRAMS += virt-nc + augeas_DATA += \ remote/libvirtd.aug \ remote/virtproxyd.aug \ @@ -286,6 +288,34 @@ remote/virtproxyd.conf: remote/libvirtd.conf.in -e 's/[@]DAEMON_NAME[@]/virtproxyd/' \ $< > $@ +virt_nc_SOURCES = \ + remote/remote_sockets.h \ + remote/remote_sockets.c \ + remote/remote_nc.c \ + $(NULL) + +virt_nc_CFLAGS = \ + $(LIBXML_CFLAGS) \ + $(GLIB_CFLAGS) \ + $(WARN_CFLAGS) \ + $(PIE_CFLAGS) \ + -I$(srcdir)/access \ + -I$(srcdir)/rpc \ + $(NULL) + +virt_nc_LDFLAGS = \ + $(RELRO_LDFLAGS) \ + $(PIE_LDFLAGS) \ + $(NO_INDIRECT_LDFLAGS) \ + $(NO_UNDEFINED_LDFLAGS) \ + $(NULL) + +virt_nc_LDADD = \ + libvirt.la \ + $(LIBXML_LIBS) \ + $(NULL) + + INSTALL_DATA_DIRS += remote install-data-remote: diff --git a/src/remote/remote_nc.c b/src/remote/remote_nc.c new file mode 100644 index 0000000000..d304db1a04 --- /dev/null +++ b/src/remote/remote_nc.c @@ -0,0 +1,424 @@ +/* + * remote_nc.c: a netcat equivalent for remote driver tunnelling + * + * Copyright (C) 2020 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +#include <config.h> + +#include <unistd.h> + +#include "virnetsocket.h" +#include "viralloc.h" +#include "virlog.h" +#include "virgettext.h" +#include "virfile.h" + +#include "remote_sockets.h" + +#define VIR_FROM_THIS VIR_FROM_REMOTE + +VIR_LOG_INIT("remote.remote_nc"); + +struct virRemoteProxyBuffer { + size_t length; + size_t offset; + char *data; +}; + +typedef struct virRemoteProxy virRemoteProxy; +typedef virRemoteProxy *virRemoteProxyPtr; +struct virRemoteProxy { + bool quit; + virNetSocketPtr sock; + int stdinWatch; + int stdoutWatch; + + struct virRemoteProxyBuffer sockToTerminal; + struct virRemoteProxyBuffer terminalToSock; +}; + + +static void +virRemoteProxyShutdown(virRemoteProxyPtr proxy) +{ + if (proxy->sock) { + virNetSocketRemoveIOCallback(proxy->sock); + virNetSocketClose(proxy->sock); + virObjectUnref(proxy->sock); + proxy->sock = NULL; + } + VIR_FREE(proxy->sockToTerminal.data); + VIR_FREE(proxy->terminalToSock.data); + if (proxy->stdinWatch != -1) + virEventRemoveHandle(proxy->stdinWatch); + if (proxy->stdoutWatch != -1) + virEventRemoveHandle(proxy->stdoutWatch); + proxy->stdinWatch = -1; + proxy->stdoutWatch = -1; + if (!proxy->quit) + proxy->quit = true; +} + + +static void +virRemoteProxyEventOnSocket(virNetSocketPtr sock, + int events, void *opaque) +{ + virRemoteProxyPtr proxy = opaque; + + /* we got late event after proxy was shutdown */ + if (!proxy->sock) + return; + + if (events & VIR_EVENT_HANDLE_READABLE) { + size_t avail = proxy->sockToTerminal.length - + proxy->sockToTerminal.offset; + int got; + + if (avail < 1024) { + if (VIR_REALLOC_N(proxy->sockToTerminal.data, + proxy->sockToTerminal.length + 1024) < 0) { + virRemoteProxyShutdown(proxy); + return; + } + proxy->sockToTerminal.length += 1024; + avail += 1024; + } + + got = virNetSocketRead(sock, + proxy->sockToTerminal.data + + proxy->sockToTerminal.offset, + avail); + if (got == -2) + return; /* blocking */ + if (got == 0) { + VIR_DEBUG("EOF on socket, shutting down"); + virRemoteProxyShutdown(proxy); + return; + } + if (got < 0) { + virRemoteProxyShutdown(proxy); + return; + } + proxy->sockToTerminal.offset += got; + if (proxy->sockToTerminal.offset) + virEventUpdateHandle(proxy->stdoutWatch, + VIR_EVENT_HANDLE_WRITABLE); + } + + if (events & VIR_EVENT_HANDLE_WRITABLE && + proxy->terminalToSock.offset) { + ssize_t done; + size_t avail; + done = virNetSocketWrite(proxy->sock, + proxy->terminalToSock.data, + proxy->terminalToSock.offset); + if (done == -2) + return; /* blocking */ + if (done < 0) { + virRemoteProxyShutdown(proxy); + return; + } + memmove(proxy->terminalToSock.data, + proxy->terminalToSock.data + done, + proxy->terminalToSock.offset - done); + proxy->terminalToSock.offset -= done; + + avail = proxy->terminalToSock.length - proxy->terminalToSock.offset; + if (avail > 1024) { + ignore_value(VIR_REALLOC_N(proxy->terminalToSock.data, + proxy->terminalToSock.offset + 1024)); + proxy->terminalToSock.length = proxy->terminalToSock.offset + 1024; + } + } + if (!proxy->terminalToSock.offset) + virNetSocketUpdateIOCallback(proxy->sock, + VIR_EVENT_HANDLE_READABLE); + + if (events & VIR_EVENT_HANDLE_ERROR || + events & VIR_EVENT_HANDLE_HANGUP) { + virRemoteProxyShutdown(proxy); + } +} + + +static void +virRemoteProxyEventOnStdin(int watch G_GNUC_UNUSED, + int fd G_GNUC_UNUSED, + int events, + void *opaque) +{ + virRemoteProxyPtr proxy = opaque; + + /* we got late event after console was shutdown */ + if (!proxy->sock) + return; + + if (events & VIR_EVENT_HANDLE_READABLE) { + size_t avail = proxy->terminalToSock.length - + proxy->terminalToSock.offset; + int got; + + if (avail < 1024) { + if (VIR_REALLOC_N(proxy->terminalToSock.data, + proxy->terminalToSock.length + 1024) < 0) { + virRemoteProxyShutdown(proxy); + return; + } + proxy->terminalToSock.length += 1024; + avail += 1024; + } + + got = read(fd, + proxy->terminalToSock.data + + proxy->terminalToSock.offset, + avail); + if (got < 0) { + if (errno != EAGAIN) { + virReportSystemError(errno, "%s", _("cannot read from stdin")); + virRemoteProxyShutdown(proxy); + } + return; + } + if (got == 0) { + VIR_DEBUG("EOF on stdin, shutting down"); + virRemoteProxyShutdown(proxy); + return; + } + + proxy->terminalToSock.offset += got; + if (proxy->terminalToSock.offset) + virNetSocketUpdateIOCallback(proxy->sock, + VIR_EVENT_HANDLE_READABLE | + VIR_EVENT_HANDLE_WRITABLE); + } + + if (events & VIR_EVENT_HANDLE_ERROR) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error on stdin")); + virRemoteProxyShutdown(proxy); + return; + } + + if (events & VIR_EVENT_HANDLE_HANGUP) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdin")); + virRemoteProxyShutdown(proxy); + return; + } +} + + +static void +virRemoteProxyEventOnStdout(int watch G_GNUC_UNUSED, + int fd, + int events, + void *opaque) +{ + virRemoteProxyPtr proxy = opaque; + + /* we got late event after console was shutdown */ + if (!proxy->sock) + return; + + if (events & VIR_EVENT_HANDLE_WRITABLE && + proxy->sockToTerminal.offset) { + ssize_t done; + size_t avail; + done = write(fd, + proxy->sockToTerminal.data, + proxy->sockToTerminal.offset); + if (done < 0) { + if (errno != EAGAIN) { + virReportSystemError(errno, "%s", _("cannot write to stdout")); + virRemoteProxyShutdown(proxy); + } + return; + } + memmove(proxy->sockToTerminal.data, + proxy->sockToTerminal.data + done, + proxy->sockToTerminal.offset - done); + proxy->sockToTerminal.offset -= done; + + avail = proxy->sockToTerminal.length - proxy->sockToTerminal.offset; + if (avail > 1024) { + ignore_value(VIR_REALLOC_N(proxy->sockToTerminal.data, + proxy->sockToTerminal.offset + 1024)); + proxy->sockToTerminal.length = proxy->sockToTerminal.offset + 1024; + } + } + + if (!proxy->sockToTerminal.offset) + virEventUpdateHandle(proxy->stdoutWatch, 0); + + if (events & VIR_EVENT_HANDLE_ERROR) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("IO error stdout")); + virRemoteProxyShutdown(proxy); + return; + } + + if (events & VIR_EVENT_HANDLE_HANGUP) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("EOF on stdout")); + virRemoteProxyShutdown(proxy); + return; + } +} + + +static int +virRemoteProxyRun(virNetSocketPtr sock) +{ + int ret = -1; + virRemoteProxy proxy = { + .sock = sock, + .stdinWatch = -1, + .stdoutWatch = -1, + }; + + virEventRegisterDefaultImpl(); + + if ((proxy.stdinWatch = virEventAddHandle(STDIN_FILENO, + VIR_EVENT_HANDLE_READABLE, + virRemoteProxyEventOnStdin, + &proxy, + NULL)) < 0) + goto cleanup; + + if ((proxy.stdoutWatch = virEventAddHandle(STDOUT_FILENO, + 0, + virRemoteProxyEventOnStdout, + &proxy, + NULL)) < 0) + goto cleanup; + + if (virNetSocketAddIOCallback(proxy.sock, + VIR_EVENT_HANDLE_READABLE, + virRemoteProxyEventOnSocket, + &proxy, + NULL) < 0) + goto cleanup; + + while (!proxy.quit) + virEventRunDefaultImpl(); + + if (virGetLastErrorCode() != VIR_ERR_OK) + goto cleanup; + + ret = 0; + cleanup: + if (proxy.stdinWatch != -1) + virEventRemoveHandle(proxy.stdinWatch); + if (proxy.stdoutWatch != -1) + virEventRemoveHandle(proxy.stdoutWatch); + return ret; +} + +int main(int argc, char **argv) +{ + const char *uri_str = NULL; + g_autoptr(virURI) uri = NULL; + g_autofree char *driver = NULL; + remoteDriverTransport transport; + bool user = false; + bool autostart = false; + gboolean version = false; + gboolean readonly = false; + g_autofree char *sock_path = NULL; + g_autofree char *daemon_name = NULL; + g_autoptr(virNetSocket) sock = NULL; + GError *error = NULL; + g_autoptr(GOptionContext) context = NULL; + GOptionEntry entries[] = { + { "readonly", 'r', 0, G_OPTION_ARG_NONE, &readonly, "Connect read-only", NULL }, + { "version", 'V', 0, G_OPTION_ARG_NONE, &version, "Display version information", NULL }, + { NULL } + }; + + context = g_option_context_new("- libvirt socket proxy"); + g_option_context_add_main_entries(context, entries, PACKAGE); + if (!g_option_context_parse(context, &argc, &argv, &error)) { + g_printerr(_("option parsing failed: %s\n"), error->message); + exit(EXIT_FAILURE); + } + + if (version) { + g_print("%s (%s) %s\n", argv[0], PACKAGE_NAME, PACKAGE_VERSION); + exit(EXIT_SUCCESS); + } + + virSetErrorFunc(NULL, NULL); + virSetErrorLogPriorityFunc(NULL); + + if (virGettextInitialize() < 0 || + virErrorInitialize() < 0) { + g_printerr(_("%s: initialization failed\n"), argv[0]); + exit(EXIT_FAILURE); + } + + virFileActivateDirOverrideForProg(argv[0]); + + /* Initialize the log system */ + virLogSetFromEnv(); + + if (optind != (argc - 1)) { + g_printerr("%s: expected a URI\n", argv[0]); + exit(EXIT_FAILURE); + } + + uri_str = argv[optind]; + VIR_DEBUG("Using URI %s", uri_str); + + if (!(uri = virURIParse(uri_str))) { + g_printerr(("%s: cannot parse '%s': %s\n"), + argv[0], uri_str, virGetLastErrorMessage()); + exit(EXIT_FAILURE); + } + + if (remoteSplitURIScheme(uri, &driver, &transport) < 0) { + g_printerr(_("%s: cannot parse URI transport '%s': %s\n"), + argv[0], uri_str, virGetLastErrorMessage()); + exit(EXIT_FAILURE); + } + + if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) { + g_printerr(_("%s: unexpected URI transport '%s'\n"), + argv[0], uri_str); + exit(EXIT_FAILURE); + } + + remoteGetURIDaemonInfo(uri, transport, &user, &autostart); + + sock_path = remoteGetUNIXSocket(transport, + REMOTE_DRIVER_MODE_AUTO, + driver, + !!readonly, + user, + &daemon_name); + + if (virNetSocketNewConnectUNIX(sock_path, autostart, daemon_name, &sock) < 0) { + g_printerr(_("%s: cannot connect to '%s': %s\n"), + argv[0], sock_path, virGetLastErrorMessage()); + exit(EXIT_FAILURE); + } + + if (virRemoteProxyRun(sock) < 0) { + g_printerr(_("%s: could not proxy traffic: %s\n"), + argv[0], virGetLastErrorMessage()); + exit(EXIT_FAILURE); + } + + exit(EXIT_SUCCESS); +} diff --git a/src/rpc/virnetsocket.h b/src/rpc/virnetsocket.h index d39b270480..3996d264fb 100644 --- a/src/rpc/virnetsocket.h +++ b/src/rpc/virnetsocket.h @@ -34,6 +34,7 @@ typedef struct _virNetSocket virNetSocket; typedef virNetSocket *virNetSocketPtr; +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virNetSocket, virObjectUnref); typedef void (*virNetSocketIOFunc)(virNetSocketPtr sock, int events, -- 2.26.2
On 7/9/20 8:36 PM, Daniel P. Berrangé wrote:
When accessing libvirtd over a SSH tunnel, the remote driver must spawn the remote 'nc' process, pointing it to the libvirtd socket path. This is problematic for a number of reasons:
- The socket path varies according to the --prefix chosen at build time. The remote client is seeing the local prefix, but what we need is the remote prefix
- The socket path varies according to remote env variables, such as the XDG_RUNTIME_DIR location. Again we see the local XDG_RUNTIME_DIR value, but what we need is the remote value (if any)
- We can not able to autospawn the libvirtd daemon for session mode access
To address these problems this patch introduces the 'virtd-nc' helper program which takes the URI for the remote driver as a CLI parameter. It then figures out the socket path to connect to using the same code as the remote driver does on the remote host.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 2 +- po/POTFILES.in | 1 + src/remote/Makefile.inc.am | 30 +++ src/remote/remote_nc.c | 424 +++++++++++++++++++++++++++++++++++++ src/rpc/virnetsocket.h | 1 + 5 files changed, 457 insertions(+), 1 deletion(-) create mode 100644 src/remote/remote_nc.c
The spec file needs to be updated too. Michal
On Fri, Jul 10, 2020 at 02:04:00PM +0200, Michal Privoznik wrote:
On 7/9/20 8:36 PM, Daniel P. Berrangé wrote:
When accessing libvirtd over a SSH tunnel, the remote driver must spawn the remote 'nc' process, pointing it to the libvirtd socket path. This is problematic for a number of reasons:
- The socket path varies according to the --prefix chosen at build time. The remote client is seeing the local prefix, but what we need is the remote prefix
- The socket path varies according to remote env variables, such as the XDG_RUNTIME_DIR location. Again we see the local XDG_RUNTIME_DIR value, but what we need is the remote value (if any)
- We can not able to autospawn the libvirtd daemon for session mode access
To address these problems this patch introduces the 'virtd-nc' helper program which takes the URI for the remote driver as a CLI parameter. It then figures out the socket path to connect to using the same code as the remote driver does on the remote host.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- build-aux/syntax-check.mk | 2 +- po/POTFILES.in | 1 + src/remote/Makefile.inc.am | 30 +++ src/remote/remote_nc.c | 424 +++++++++++++++++++++++++++++++++++++ src/rpc/virnetsocket.h | 1 + 5 files changed, 457 insertions(+), 1 deletion(-) create mode 100644 src/remote/remote_nc.c
The spec file needs to be updated too.
Oh right, of course. 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 :|
Switch keyfile and netcat parameters, since the netcat path and socket path are a logical pair that belong together. This patches the other constructors. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- src/remote/remote_driver.c | 2 +- src/rpc/virnetclient.c | 2 +- src/rpc/virnetclient.h | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c2dcf20f91..c1f7a45aab 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -1010,8 +1010,8 @@ doRemoteOpen(virConnectPtr conn, username, !tty, !verify, - netcat ? netcat : "nc", keyfile, + netcat ? netcat : "nc", sockname))) goto failed; diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index aee2b52bf6..cd1bcc3ab3 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -441,8 +441,8 @@ virNetClientPtr virNetClientNewSSH(const char *nodename, const char *username, bool noTTY, bool noVerify, - const char *netcatPath, const char *keyfile, + const char *netcatPath, const char *socketPath) { virNetSocketPtr sock; diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h index 0005de46f3..6fdc370083 100644 --- a/src/rpc/virnetclient.h +++ b/src/rpc/virnetclient.h @@ -48,9 +48,9 @@ virNetClientPtr virNetClientNewSSH(const char *nodename, const char *username, bool noTTY, bool noVerify, - const char *netcat, const char *keyfile, - const char *path); + const char *netcat, + const char *socketPath); virNetClientPtr virNetClientNewLibSSH2(const char *host, const char *port, -- 2.26.2
This wires up support for using the new virt-nc binary with the ssh, libssh and libssh2 protocols. The new binary will be used preferentially if it is available in $PATH, otherwise we fall back to traditional netcat. The "proxy" URI parameter can be used to force use of netcat e.g. qemu+ssh://host/system?proxy=netcat or the disable fallback e.g. qemu+ssh://host/system?proxy=virt-nc With use of virt-nc, we can now support remote session URIs qemu+ssh://host/session and this will only use virt-nc, with no fallback. This also lets the libvirtd process be auto-started. Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/uri.html.in | 18 ++++++++++ src/remote/remote_driver.c | 30 +++++++++++++++- src/remote/remote_sockets.c | 8 ----- src/rpc/virnetclient.c | 70 ++++++++++++++++++++++++++++++------- src/rpc/virnetclient.h | 30 +++++++++++++--- tests/virnetsockettest.c | 7 ++-- 6 files changed, 136 insertions(+), 27 deletions(-) diff --git a/docs/uri.html.in b/docs/uri.html.in index 49f92773f8..5311579273 100644 --- a/docs/uri.html.in +++ b/docs/uri.html.in @@ -259,6 +259,24 @@ Note that parameter values must be <td colspan="2"/> <td> Example: <code>mode=direct</code> </td> </tr> + <tr> + <td> + <code>proxy</code> + </td> + <td>auto, virt, generic </td> + <td> + <dl> + <dt><code>auto</code></dt><dd>try virt-nc, fallback to netcat</dd> + <dt><code>netcat</code></dt><dd>only use netcat</dd> + <dt><code>virt-nc</code></dt><dd>only use virt-nc</dd> + </dl> + Can also be set in <code>libvirt.conf</code> as <code>remote_proxy</code> + </td> + </tr> + <tr> + <td colspan="2"/> + <td> Example: <code>proxy=virt-nc</code> </td> + </tr> <tr> <td> <code>command</code> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c1f7a45aab..83789a86a9 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -761,12 +761,14 @@ doRemoteOpen(virConnectPtr conn, g_autofree char *knownHosts = NULL; g_autofree char *mode_str = NULL; g_autofree char *daemon_name = NULL; + g_autofree char *proxy_str = NULL; bool sanity = true; bool verify = true; #ifndef WIN32 bool tty = true; #endif int mode; + int proxy; if (inside_daemon && !conn->uri->server) { mode = REMOTE_DRIVER_MODE_DIRECT; @@ -774,6 +776,14 @@ doRemoteOpen(virConnectPtr conn, mode = REMOTE_DRIVER_MODE_AUTO; } + /* Historically we didn't allow ssh tunnel with session mode, + * since we can't construct the accurate path remotely, + * so we can default to modern virt-nc */ + if (flags & VIR_DRV_OPEN_REMOTE_USER) + proxy = VIR_NET_CLIENT_PROXY_VIRT_NC; + else + proxy = VIR_NET_CLIENT_PROXY_NETCAT; + /* We handle *ALL* URIs here. The caller has rejected any * URIs we don't care about */ @@ -813,6 +823,7 @@ doRemoteOpen(virConnectPtr conn, EXTRACT_URI_ARG_STR("known_hosts_verify", knownHostsVerify); EXTRACT_URI_ARG_STR("tls_priority", tls_priority); EXTRACT_URI_ARG_STR("mode", mode_str); + EXTRACT_URI_ARG_STR("proxy", proxy_str); EXTRACT_URI_ARG_BOOL("no_sanity", sanity); EXTRACT_URI_ARG_BOOL("no_verify", verify); #ifndef WIN32 @@ -865,6 +876,14 @@ doRemoteOpen(virConnectPtr conn, (mode = remoteDriverModeTypeFromString(mode_str)) < 0) goto failed; + if (conf && !proxy_str && + virConfGetValueString(conf, "remote_proxy", &proxy_str) < 0) + goto failed; + + if (proxy_str && + (proxy = virNetClientProxyTypeFromString(proxy_str)) < 0) + goto failed; + /* Sanity check that nothing requested !direct mode by mistake */ if (inside_daemon && !conn->uri->server && mode != REMOTE_DRIVER_MODE_DIRECT) { virReportError(VIR_ERR_INVALID_ARG, "%s", @@ -949,8 +968,11 @@ doRemoteOpen(virConnectPtr conn, knownHosts, knownHostsVerify, sshauth, + proxy, netcat, sockname, + name, + flags & VIR_DRV_OPEN_REMOTE_RO, auth, conn->uri); if (!priv->client) @@ -970,8 +992,11 @@ doRemoteOpen(virConnectPtr conn, knownHosts, knownHostsVerify, sshauth, + proxy, netcat, sockname, + name, + flags & VIR_DRV_OPEN_REMOTE_RO, auth, conn->uri); if (!priv->client) @@ -1011,8 +1036,11 @@ doRemoteOpen(virConnectPtr conn, !tty, !verify, keyfile, + proxy, netcat ? netcat : "nc", - sockname))) + sockname, + name, + flags & VIR_DRV_OPEN_REMOTE_RO))) goto failed; priv->is_secure = 1; diff --git a/src/remote/remote_sockets.c b/src/remote/remote_sockets.c index 854775f401..7c69ed9e7f 100644 --- a/src/remote/remote_sockets.c +++ b/src/remote/remote_sockets.c @@ -108,14 +108,6 @@ remoteGetUNIXSocketHelper(remoteDriverTransport transport, g_autofree char *userdir = NULL; if (session) { - if (transport != REMOTE_DRIVER_TRANSPORT_UNIX) { - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, - _("Connecting to session instance without " - "socket path is not supported by the %s " - "transport"), - remoteDriverTransportTypeToString(transport)); - return NULL; - } userdir = virGetUserRuntimeDirectory(); sockname = g_strdup_printf("%s/%s-sock", userdir, sock_prefix); diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index cd1bcc3ab3..5939f74e62 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -50,6 +50,10 @@ enum { VIR_NET_CLIENT_MODE_COMPLETE, }; +VIR_ENUM_IMPL(virNetClientProxy, + VIR_NET_CLIENT_PROXY_LAST, + "auto", "netcat", "virt-nc"); + struct _virNetClientCall { int mode; @@ -414,20 +418,50 @@ virNetClientDoubleEscapeShell(const char *str) } char * -virNetClientSSHHelperCommand(const char *netcatPath, - const char *socketPath) +virNetClientSSHHelperCommand(virNetClientProxy proxy, + const char *netcatPath, + const char *socketPath, + const char *driverURI, + bool readonly) { g_autofree char *netcatPathSafe = virNetClientDoubleEscapeShell(netcatPath); + g_autofree char *driverURISafe = virNetClientDoubleEscapeShell(driverURI); + g_autofree char *nccmd = NULL; + g_autofree char *virtnccmd = NULL; - return g_strdup_printf( - "sh -c " - "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + nccmd = g_strdup_printf( + "if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" "else " - "ARG=;" + "ARG=;" "fi;" - "'%s' $ARG -U %s'", + "'%s' $ARG -U %s", netcatPathSafe, netcatPathSafe, socketPath); + + virtnccmd = g_strdup_printf("%s '%s'", + readonly ? "virt-nc -r" : "virt-nc", + driverURISafe); + + switch (proxy) { + case VIR_NET_CLIENT_PROXY_AUTO: + return g_strdup_printf("sh -c 'which virt-nc 1>/dev/null 2>&1; " + "if test $? = 0; then " + " %s; " + "else" + " %s; " + "fi'", virtnccmd, nccmd); + + case VIR_NET_CLIENT_PROXY_NETCAT: + return g_strdup_printf("sh -c '%s'", nccmd); + + case VIR_NET_CLIENT_PROXY_VIRT_NC: + return g_strdup_printf("sh -c '%s'", virtnccmd); + + case VIR_NET_CLIENT_PROXY_LAST: + default: + virReportEnumRangeError(virNetClientProxy, proxy); + return NULL; + } } @@ -442,8 +476,11 @@ virNetClientPtr virNetClientNewSSH(const char *nodename, bool noTTY, bool noVerify, const char *keyfile, + virNetClientProxy proxy, const char *netcatPath, - const char *socketPath) + const char *socketPath, + const char *driverURI, + bool readonly) { virNetSocketPtr sock; @@ -451,7 +488,8 @@ virNetClientPtr virNetClientNewSSH(const char *nodename, DEFAULT_VALUE(netcatPath, "nc"); - command = virNetClientSSHHelperCommand(netcatPath, socketPath); + command = virNetClientSSHHelperCommand(proxy, netcatPath, socketPath, + driverURI, readonly); if (virNetSocketNewConnectSSH(nodename, service, binary, username, noTTY, noVerify, keyfile, command, &sock) < 0) @@ -468,8 +506,11 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, const char *knownHostsPath, const char *knownHostsVerify, const char *authMethods, + virNetClientProxy proxy, const char *netcatPath, const char *socketPath, + const char *driverURI, + bool readonly, virConnectAuthPtr authPtr, virURIPtr uri) { @@ -511,7 +552,8 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, DEFAULT_VALUE(netcatPath, "nc"); DEFAULT_VALUE(knownHostsVerify, "normal"); - command = virNetClientSSHHelperCommand(netcatPath, socketPath); + command = virNetClientSSHHelperCommand(proxy, netcatPath, socketPath, + driverURI, readonly); if (virNetSocketNewConnectLibSSH2(host, port, family, @@ -531,8 +573,11 @@ virNetClientPtr virNetClientNewLibssh(const char *host, const char *knownHostsPath, const char *knownHostsVerify, const char *authMethods, + virNetClientProxy proxy, const char *netcatPath, const char *socketPath, + const char *driverURI, + bool readonly, virConnectAuthPtr authPtr, virURIPtr uri) { @@ -574,7 +619,8 @@ virNetClientPtr virNetClientNewLibssh(const char *host, DEFAULT_VALUE(netcatPath, "nc"); DEFAULT_VALUE(knownHostsVerify, "normal"); - command = virNetClientSSHHelperCommand(netcatPath, socketPath); + command = virNetClientSSHHelperCommand(proxy, netcatPath, socketPath, + driverURI, readonly); if (virNetSocketNewConnectLibssh(host, port, family, diff --git a/src/rpc/virnetclient.h b/src/rpc/virnetclient.h index 6fdc370083..76500e2c3f 100644 --- a/src/rpc/virnetclient.h +++ b/src/rpc/virnetclient.h @@ -30,9 +30,22 @@ #include "virobject.h" #include "viruri.h" +typedef enum { + VIR_NET_CLIENT_PROXY_AUTO, + VIR_NET_CLIENT_PROXY_NETCAT, + VIR_NET_CLIENT_PROXY_VIRT_NC, + + VIR_NET_CLIENT_PROXY_LAST, +} virNetClientProxy; + +VIR_ENUM_DECL(virNetClientProxy); + char * -virNetClientSSHHelperCommand(const char *netcatPath, - const char *socketPath); +virNetClientSSHHelperCommand(virNetClientProxy proxy, + const char *netcatPath, + const char *socketPath, + const char *driverURI, + bool readonly); virNetClientPtr virNetClientNewUNIX(const char *path, bool spawnDaemon, @@ -49,8 +62,11 @@ virNetClientPtr virNetClientNewSSH(const char *nodename, bool noTTY, bool noVerify, const char *keyfile, - const char *netcat, - const char *socketPath); + virNetClientProxy proxy, + const char *netcatPath, + const char *socketPath, + const char *driverURI, + bool readonly); virNetClientPtr virNetClientNewLibSSH2(const char *host, const char *port, @@ -60,8 +76,11 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, const char *knownHostsPath, const char *knownHostsVerify, const char *authMethods, + virNetClientProxy proxy, const char *netcatPath, const char *socketPath, + const char *driverURI, + bool readonly, virConnectAuthPtr authPtr, virURIPtr uri); @@ -73,8 +92,11 @@ virNetClientPtr virNetClientNewLibssh(const char *host, const char *knownHostsPath, const char *knownHostsVerify, const char *authMethods, + virNetClientProxy proxy, const char *netcatPath, const char *socketPath, + const char *driverURI, + bool readonly, virConnectAuthPtr authPtr, virURIPtr uri); diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index 842eb1bcfc..c6fbe479d7 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -464,8 +464,11 @@ 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); + g_autofree char *command = virNetClientSSHHelperCommand(VIR_NET_CLIENT_PROXY_AUTO, + data->netcat, + data->path, + "qemu:///session", + true); if (virNetSocketNewConnectSSH(data->nodename, data->service, -- 2.26.2
On 7/9/20 8:36 PM, Daniel P. Berrangé wrote:
This wires up support for using the new virt-nc binary with the ssh, libssh and libssh2 protocols.
The new binary will be used preferentially if it is available in $PATH, otherwise we fall back to traditional netcat.
The "proxy" URI parameter can be used to force use of netcat e.g.
qemu+ssh://host/system?proxy=netcat
or the disable fallback e.g.
qemu+ssh://host/system?proxy=virt-nc
With use of virt-nc, we can now support remote session URIs
qemu+ssh://host/session
and this will only use virt-nc, with no fallback. This also lets the libvirtd process be auto-started.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- docs/uri.html.in | 18 ++++++++++ src/remote/remote_driver.c | 30 +++++++++++++++- src/remote/remote_sockets.c | 8 ----- src/rpc/virnetclient.c | 70 ++++++++++++++++++++++++++++++------- src/rpc/virnetclient.h | 30 +++++++++++++--- tests/virnetsockettest.c | 7 ++-- 6 files changed, 136 insertions(+), 27 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index cd1bcc3ab3..5939f74e62 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -50,6 +50,10 @@ enum { VIR_NET_CLIENT_MODE_COMPLETE, };
+VIR_ENUM_IMPL(virNetClientProxy, + VIR_NET_CLIENT_PROXY_LAST, + "auto", "netcat", "virt-nc"); + struct _virNetClientCall { int mode;
@@ -414,20 +418,50 @@ virNetClientDoubleEscapeShell(const char *str) }
char * -virNetClientSSHHelperCommand(const char *netcatPath, - const char *socketPath) +virNetClientSSHHelperCommand(virNetClientProxy proxy, + const char *netcatPath, + const char *socketPath, + const char *driverURI, + bool readonly) { g_autofree char *netcatPathSafe = virNetClientDoubleEscapeShell(netcatPath); + g_autofree char *driverURISafe = virNetClientDoubleEscapeShell(driverURI); + g_autofree char *nccmd = NULL; + g_autofree char *virtnccmd = NULL;
- return g_strdup_printf( - "sh -c " - "'if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " - "ARG=-q0;" + nccmd = g_strdup_printf( + "if '%s' -q 2>&1 | grep \"requires an argument\" >/dev/null 2>&1; then " + "ARG=-q0;" "else " - "ARG=;" + "ARG=;" "fi;" - "'%s' $ARG -U %s'", + "'%s' $ARG -U %s", netcatPathSafe, netcatPathSafe, socketPath); + + virtnccmd = g_strdup_printf("%s '%s'", + readonly ? "virt-nc -r" : "virt-nc", + driverURISafe); + + switch (proxy) { + case VIR_NET_CLIENT_PROXY_AUTO: + return g_strdup_printf("sh -c 'which virt-nc 1>/dev/null 2>&1; " + "if test $? = 0; then " + " %s; " + "else" + " %s; " + "fi'", virtnccmd, nccmd); + + case VIR_NET_CLIENT_PROXY_NETCAT: + return g_strdup_printf("sh -c '%s'", nccmd); + + case VIR_NET_CLIENT_PROXY_VIRT_NC: + return g_strdup_printf("sh -c '%s'", virtnccmd); + + case VIR_NET_CLIENT_PROXY_LAST: + default: + virReportEnumRangeError(virNetClientProxy, proxy); + return NULL; + } }
This needs to be coupled with virnetsockettest update because the expected output of executed command changes. Michal
On Thu, 2020-07-09 at 19:36 +0100, Daniel P. Berrangé wrote:
This wires up support for using the new virt-nc binary with the ssh, libssh and libssh2 protocols.
The new binary will be used preferentially if it is available in $PATH, otherwise we fall back to traditional netcat.
The "proxy" URI parameter can be used to force use of netcat e.g.
qemu+ssh://host/system?proxy=netcat
or the disable fallback e.g.
qemu+ssh://host/system?proxy=virt-nc
With use of virt-nc, we can now support remote session URIs
qemu+ssh://host/session
and this will only use virt-nc, with no fallback. This also lets the libvirtd process be auto-started.
Just a couple of comments about the UI: would it make sense to use something like qemu+ssh://host/system?tunnelcmd=virt-tunnel instead? virt-nc could be seen as a bit of a misnomer, considering that (by design) it doesn't do nearly as much as the actual netcat does; as for the 'proxy' argument, I'm afraid it might lead people to believe it's used for HTTP proxying or some other form of proxying *between the client and the host*, whereas it's really something that only affects operations on the host itself - not to mention that we also have a virtproxyd now, further increasing the potential for confusion... I'd also be remiss if I didn't point out that this is definitely a change worth documenting in our release notes :) -- Andrea Bolognani / Red Hat / Virtualization
On Fri, Jul 10, 2020 at 07:21:47PM +0200, Andrea Bolognani wrote:
On Thu, 2020-07-09 at 19:36 +0100, Daniel P. Berrangé wrote:
This wires up support for using the new virt-nc binary with the ssh, libssh and libssh2 protocols.
The new binary will be used preferentially if it is available in $PATH, otherwise we fall back to traditional netcat.
The "proxy" URI parameter can be used to force use of netcat e.g.
qemu+ssh://host/system?proxy=netcat
or the disable fallback e.g.
qemu+ssh://host/system?proxy=virt-nc
With use of virt-nc, we can now support remote session URIs
qemu+ssh://host/session
and this will only use virt-nc, with no fallback. This also lets the libvirtd process be auto-started.
Just a couple of comments about the UI: would it make sense to use something like
qemu+ssh://host/system?tunnelcmd=virt-tunnel
instead? virt-nc could be seen as a bit of a misnomer, considering that (by design) it doesn't do nearly as much as the actual netcat does; as for the 'proxy' argument, I'm afraid it might lead people to believe it's used for HTTP proxying or some other form of proxying *between the client and the host*, whereas it's really something that only affects operations on the host itself - not to mention that we also have a virtproxyd now, further increasing the potential for confusion...
I chose proxy not tunnel, because SSH is providing the tunnel here. virt-nc is a proxy linking the tunnel to the daemon. virtproxyd is conceptually similar, again linking a libvirt client to the real daemon. I probably shouldn't mention "virt-nc" by name in the URI and instead have "proxy=netcat" vs "proxy=native", as users don't get to choose the actual binary here - they are providing an enum string, which gets mapped to the desired binary. 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 Wed, 2020-07-15 at 11:00 +0100, Daniel P. Berrangé wrote:
On Fri, Jul 10, 2020 at 07:21:47PM +0200, Andrea Bolognani wrote:
Just a couple of comments about the UI: would it make sense to use something like
qemu+ssh://host/system?tunnelcmd=virt-tunnel
instead? virt-nc could be seen as a bit of a misnomer, considering that (by design) it doesn't do nearly as much as the actual netcat does; as for the 'proxy' argument, I'm afraid it might lead people to believe it's used for HTTP proxying or some other form of proxying *between the client and the host*, whereas it's really something that only affects operations on the host itself - not to mention that we also have a virtproxyd now, further increasing the potential for confusion...
I chose proxy not tunnel, because SSH is providing the tunnel here. virt-nc is a proxy linking the tunnel to the daemon. virtproxyd is conceptually similar, again linking a libvirt client to the real daemon.
Mh, that makes sense but I'm still wary of using "proxy" due to the potential for confusion, since in this case the proxy is on the opposite side of the connection than one would probably expect it to be. Something like "remoteproxy" or "serverproxy", perhaps?
I probably shouldn't mention "virt-nc" by name in the URI and instead have "proxy=netcat" vs "proxy=native", as users don't get to choose the actual binary here - they are providing an enum string, which gets mapped to the desired binary.
Yeah, that's much better. Going back to the name of the command itself, since it's an internal implementation details, and as such it's not intended to be invoked by users and accordingly we're installing it under $(libexecdir) along with existing helpers, what about following the established naming convention and calling it 'libvirt_proxyhelper'? -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jul 15, 2020 at 02:25:14PM +0200, Andrea Bolognani wrote:
On Wed, 2020-07-15 at 11:00 +0100, Daniel P. Berrangé wrote:
On Fri, Jul 10, 2020 at 07:21:47PM +0200, Andrea Bolognani wrote:
Just a couple of comments about the UI: would it make sense to use something like
qemu+ssh://host/system?tunnelcmd=virt-tunnel
instead? virt-nc could be seen as a bit of a misnomer, considering that (by design) it doesn't do nearly as much as the actual netcat does; as for the 'proxy' argument, I'm afraid it might lead people to believe it's used for HTTP proxying or some other form of proxying *between the client and the host*, whereas it's really something that only affects operations on the host itself - not to mention that we also have a virtproxyd now, further increasing the potential for confusion...
I chose proxy not tunnel, because SSH is providing the tunnel here. virt-nc is a proxy linking the tunnel to the daemon. virtproxyd is conceptually similar, again linking a libvirt client to the real daemon.
Mh, that makes sense but I'm still wary of using "proxy" due to the potential for confusion, since in this case the proxy is on the opposite side of the connection than one would probably expect it to be. Something like "remoteproxy" or "serverproxy", perhaps?
I don't think there's really any problem of confusion here unless someone doesn't read the docs at all, in which case they won't even know about this parameter. So I don't think using a more verbose term is any benefit.
I probably shouldn't mention "virt-nc" by name in the URI and instead have "proxy=netcat" vs "proxy=native", as users don't get to choose the actual binary here - they are providing an enum string, which gets mapped to the desired binary.
Yeah, that's much better.
Going back to the name of the command itself, since it's an internal implementation details, and as such it's not intended to be invoked by users and accordingly we're installing it under $(libexecdir) along with existing helpers, what about following the established naming convention and calling it 'libvirt_proxyhelper'?
Installing it to libexecdir is actually a mistake in this version. It needs to be installed into bindir, as it must be present in $PATH. 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 Wed, 2020-07-15 at 14:25 +0100, Daniel P. Berrangé wrote:
On Wed, Jul 15, 2020 at 02:25:14PM +0200, Andrea Bolognani wrote:
Mh, that makes sense but I'm still wary of using "proxy" due to the potential for confusion, since in this case the proxy is on the opposite side of the connection than one would probably expect it to be. Something like "remoteproxy" or "serverproxy", perhaps?
I don't think there's really any problem of confusion here unless someone doesn't read the docs at all, in which case they won't even know about this parameter. So I don't think using a more verbose term is any benefit.
Okay.
Going back to the name of the command itself, since it's an internal implementation details, and as such it's not intended to be invoked by users and accordingly we're installing it under $(libexecdir) along with existing helpers, what about following the established naming convention and calling it 'libvirt_proxyhelper'?
Installing it to libexecdir is actually a mistake in this version. It needs to be installed into bindir, as it must be present in $PATH.
Why is that so? Is it because otherwise we can't guarantee that ssh on the remote end will find it, seeing as $(libexecdir) can be changed at configure time and is usually not part of $PATH anyway? If the binary will show up in $PATH, then I think it's even more important to ensure it's very apparent that it's for internal use and not to be invoked directly. Including a message explaining this in the help output as well as the usage message that is printed when a URI is not passed on the command line would be a great start. As for the name of the binary itself, longer and more descriptive is clearly better to avoid confusion. What about 'virt-proxy-helper'? -- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2020-07-15 at 16:11 +0200, Andrea Bolognani wrote:
On Wed, 2020-07-15 at 14:25 +0100, Daniel P. Berrangé wrote:
On Wed, Jul 15, 2020 at 02:25:14PM +0200, Andrea Bolognani wrote:
Mh, that makes sense but I'm still wary of using "proxy" due to the potential for confusion, since in this case the proxy is on the opposite side of the connection than one would probably expect it to be. Something like "remoteproxy" or "serverproxy", perhaps?
I don't think there's really any problem of confusion here unless someone doesn't read the docs at all, in which case they won't even know about this parameter. So I don't think using a more verbose term is any benefit.
Okay.
The other day I randomly realized the ssh-based transports already accept a 'netcat' URI parameter which can be used to point libvirt to a non-standard nc stand-in. With that in mind, is it really necessary to introduce another URI parameter? Can't we just reuse the existing one? -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Jul 20, 2020 at 07:18:55PM +0200, Andrea Bolognani wrote:
On Wed, 2020-07-15 at 16:11 +0200, Andrea Bolognani wrote:
On Wed, 2020-07-15 at 14:25 +0100, Daniel P. Berrangé wrote:
On Wed, Jul 15, 2020 at 02:25:14PM +0200, Andrea Bolognani wrote:
Mh, that makes sense but I'm still wary of using "proxy" due to the potential for confusion, since in this case the proxy is on the opposite side of the connection than one would probably expect it to be. Something like "remoteproxy" or "serverproxy", perhaps?
I don't think there's really any problem of confusion here unless someone doesn't read the docs at all, in which case they won't even know about this parameter. So I don't think using a more verbose term is any benefit.
Okay.
The other day I randomly realized the ssh-based transports already accept a 'netcat' URI parameter which can be used to point libvirt to a non-standard nc stand-in. With that in mind, is it really necessary to introduce another URI parameter? Can't we just reuse the existing one?
Any binary provided there needs to implement netcat syntax. The whole point of this work is that we don't want to use the netcat syntax since it is impossible to know the correct socket path. 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, 2020-07-20 at 18:21 +0100, Daniel P. Berrangé wrote:
On Mon, Jul 20, 2020 at 07:18:55PM +0200, Andrea Bolognani wrote:
The other day I randomly realized the ssh-based transports already accept a 'netcat' URI parameter which can be used to point libvirt to a non-standard nc stand-in. With that in mind, is it really necessary to introduce another URI parameter? Can't we just reuse the existing one?
Any binary provided there needs to implement netcat syntax. The whole point of this work is that we don't want to use the netcat syntax since it is impossible to know the correct socket path.
We could special-case binaries called 'virt-nc' and use our internal syntax for those. Having two separate URI parameters to control the same knob sounds like trouble, especially since you can mix and match: if you try to connect to qemu+ssh://host/system?proxy=virt-nc&netcat=my-cool-nc for example, what happens? As far as I can tell virt-nc will be used, but it's certainly not as obvious as it would be if everything was controlled by a single URI parameter. Actually, and I might be very well missing something because I looked at the code rather quickly, from what I can tell the default value for proxy will cause libvirt to always prefer virt-nc when available, which means that the URI qemu+ssh://host/system?netcat=my-cool-nc will suddenly stop using my-cool-nc and start using virt-nc after libvirt has been upgraded - a breaking change. -- Andrea Bolognani / Red Hat / Virtualization
On Mon, Jul 20, 2020 at 08:20:12PM +0200, Andrea Bolognani wrote:
On Mon, 2020-07-20 at 18:21 +0100, Daniel P. Berrangé wrote:
On Mon, Jul 20, 2020 at 07:18:55PM +0200, Andrea Bolognani wrote:
The other day I randomly realized the ssh-based transports already accept a 'netcat' URI parameter which can be used to point libvirt to a non-standard nc stand-in. With that in mind, is it really necessary to introduce another URI parameter? Can't we just reuse the existing one?
Any binary provided there needs to implement netcat syntax. The whole point of this work is that we don't want to use the netcat syntax since it is impossible to know the correct socket path.
We could special-case binaries called 'virt-nc' and use our internal syntax for those. Having two separate URI parameters to control the same knob sounds like trouble, especially since you can mix and match: if you try to connect to
qemu+ssh://host/system?proxy=virt-nc&netcat=my-cool-nc
for example, what happens? As far as I can tell virt-nc will be used, but it's certainly not as obvious as it would be if everything was controlled by a single URI parameter.
No, I really don't want to do magic based on the name of the binary. That is a recipe for long term pain. It never turns out well when we try to overload two distinct concepts onto a single tunable. That URL you illustrate should be reported as an error since it is using mutually exclusive args.
Actually, and I might be very well missing something because I looked at the code rather quickly, from what I can tell the default value for proxy will cause libvirt to always prefer virt-nc when available, which means that the URI
qemu+ssh://host/system?netcat=my-cool-nc
will suddenly stop using my-cool-nc and start using virt-nc after libvirt has been upgraded - a breaking change.
It will only stop using my-cool-nc if you have upgraded the remote host to have virt-nc installed, and your local host also has the libvirt supporting virt-nc. I'd consider that desirable, as netcat is redundant once both sides are upgraded. 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, 2020-07-20 at 19:36 +0100, Daniel P. Berrangé wrote:
On Mon, Jul 20, 2020 at 08:20:12PM +0200, Andrea Bolognani wrote:
We could special-case binaries called 'virt-nc' and use our internal syntax for those. Having two separate URI parameters to control the same knob sounds like trouble, especially since you can mix and match: if you try to connect to
qemu+ssh://host/system?proxy=virt-nc&netcat=my-cool-nc
for example, what happens? As far as I can tell virt-nc will be used, but it's certainly not as obvious as it would be if everything was controlled by a single URI parameter.
No, I really don't want to do magic based on the name of the binary. That is a recipe for long term pain. It never turns out well when we try to overload two distinct concepts onto a single tunable.
That URL you illustrate should be reported as an error since it is using mutually exclusive args.
Adding validation sounds good. We should also cross-reference the two parameters in the documentation, to make sure users changing one of them is aware of the other existing as well.
Actually, and I might be very well missing something because I looked at the code rather quickly, from what I can tell the default value for proxy will cause libvirt to always prefer virt-nc when available, which means that the URI
qemu+ssh://host/system?netcat=my-cool-nc
will suddenly stop using my-cool-nc and start using virt-nc after libvirt has been upgraded - a breaking change.
It will only stop using my-cool-nc if you have upgraded the remote host to have virt-nc installed, and your local host also has the libvirt supporting virt-nc. I'd consider that desirable, as netcat is redundant once both sides are upgraded.
If the user is explicitly asking for a specific netcat binary to be used, then we need to comply with that request, even if we think that virt-nc would be better. Doing otherwise has the potential to break the user's setup. Basically, when the netcat parameter is specified we should behave as if proxy=netcat had been specified as well. -- Andrea Bolognani / Red Hat / Virtualization
On 7/9/20 8:36 PM, Daniel P. Berrangé wrote:
We have long had a problem with use of netcat for ssh tunnelling because there's no guarantee the UNIX socket path the client builds will match the UNIX socket path the remote host uses. We don't even allow session mode SSH tunnelling for this reason. We also can't easily auto-spawn libvirtd in session mode.
With the introduction of modular daemons we also have potential for two completely different UNIX socket paths even for system mode, and the client can't know which to use.
The solution to all these problems is to introduce a custom netcat impl. Instead passing the UNIX socket path, we pass the libvirt driver URI. The custom netcat then decides which socket path to use based on the remote build host environment.
We still have to support netcat for interoperability with legacy libvirt versions, but we can default to the new virt-nc.
Daniel P. Berrangé (9): rpc: merge logic for generating remote SSH shell script remote: split off enums into separate source file remote: split out function for parsing URI scheme remote: parse the remote transport string earlier remote: split out function for constructing socket path remote: extract logic for determining daemon to connect to remote: introduce virtd-nc helper binary rpc: switch order of args in virNetClientNewSSH rpc: use new virt-nc binary for remote tunnelling
build-aux/syntax-check.mk | 2 +- docs/uri.html.in | 18 ++ po/POTFILES.in | 2 + src/libvirt_remote.syms | 1 + src/remote/Makefile.inc.am | 32 +++ src/remote/remote_driver.c | 323 +++++---------------------- src/remote/remote_nc.c | 424 ++++++++++++++++++++++++++++++++++++ src/remote/remote_sockets.c | 277 +++++++++++++++++++++++ src/remote/remote_sockets.h | 70 ++++++ src/rpc/virnetclient.c | 151 ++++++++----- src/rpc/virnetclient.h | 29 ++- src/rpc/virnetsocket.c | 37 +--- src/rpc/virnetsocket.h | 4 +- tests/virnetsockettest.c | 12 +- 14 files changed, 1018 insertions(+), 364 deletions(-) create mode 100644 src/remote/remote_nc.c create mode 100644 src/remote/remote_sockets.c create mode 100644 src/remote/remote_sockets.h
If you fix small problems I've raised then: Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Andrea Bolognani -
Daniel P. Berrangé -
Michal Privoznik