Re: [libvirt] [PATCH v2 2/2] libssh_transport: add new libssh-based transport

On Tuesday, 18 October 2016 15:15:07 CEST Peter Krempa wrote: > On Mon, Oct 17, 2016 at 16:24:53 +0200, Pino Toscano wrote: > > Implement a new libssh transport, which uses libssh to communicate with > > remote hosts, and use it in virNetSockets. > > > > This new transport supports all the common ssh authentication methods, > > making use of libvirt's auth callbacks for interaction with the user. > > > > Most of the functionalities and implementation are based on the libssh2 > > transport. > > --- > > config-post.h | 2 + > > configure.ac | 3 + > > include/libvirt/virterror.h | 2 + > > m4/virt-libssh.m4 | 26 + > > src/Makefile.am | 21 +- > > src/libvirt_libssh.syms | 22 + > > src/remote/remote_driver.c | 41 ++ > > src/rpc/virnetclient.c | 123 ++++ > > src/rpc/virnetclient.h | 13 + > > src/rpc/virnetlibsshsession.c | 1424 +++++++++++++++++++++++++++++++++++++++++ > > src/rpc/virnetlibsshsession.h | 80 +++ > > src/rpc/virnetsocket.c | 179 ++++++ > > src/rpc/virnetsocket.h | 13 + > > src/util/virerror.c | 9 +- > > 14 files changed, 1955 insertions(+), 3 deletions(-) > > create mode 100644 m4/virt-libssh.m4 > > create mode 100644 src/libvirt_libssh.syms > > create mode 100644 src/rpc/virnetlibsshsession.c > > create mode 100644 src/rpc/virnetlibsshsession.h > > Is it possible to split out the src/rpc/virnetlibsshsession.(ch) changes > (plus the ones necessary to compile it) from adding all the bits that > actually make use of it? This patch is very big. Yes, it is -- I was not sure how much should the changes be split, especially in cases like this when adding a new "thing" which is used only in a single place later on. Just to be sure, a reasonable split for this patch would be: 1) add libssh bits in virerror 2) add virnetlibsshsession.(ch) + autofoo stuff for libssh 3) add glue code in virNetSocket + virNetClient or were you thinking about something else? (no problems on my side)

> > diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c > > index 361dc1a..b296aac 100644 > > --- a/src/rpc/virnetclient.c > > +++ b/src/rpc/virnetclient.c > > @@ -505,6 +505,129 @@ virNetClientPtr virNetClientNewLibSSH2(const char *host, > > } > > #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; > > + virNetClientPtr ret = NULL; > > + > > + virBuffer buf = VIR_BUFFER_INITIALIZER; > > + char *nc = NULL; > > + char *command = NULL; > > + > > + char *homedir = virGetUserDirectory(); > > + char *confdir = virGetUserConfigDirectory(); > > + char *knownhosts = NULL; > > + char *privkey = NULL; > > + > > + /* Use default paths for known hosts an public keys if not provided */ > > So is libssh able to handle e.g. ECDSA keys in known hosts? Libssh2 was > not and truncated the known hosts file which was not acceptable. Yes, it is. For example in my tests I'm passing known_hosts=$HOME/.ssh/known_hosts as additional query item to the qemu+libssh URLs, and it works well.

> > + if (confdir) { > > + if (!knownHostsPath) { > > + if (virFileExists(confdir)) { > > + virBufferAsprintf(&buf, "%s/known_hosts", confdir); > > + if (!(knownhosts = virBufferContentAndReset(&buf))) > > Use virAsprintf instead of the two lines above. OK. Small side note: all the glue code in virNetSocket, virNetClient and remote_driver.c was basically a copy&paste from the libssh2 implementation, so all these fixes should be done there too.

> > + memset(&retr_passphrase, 0, sizeof(virConnectCredential)); > > + retr_passphrase.type = virCredTypeForPrompt(sess->cred, echo); > > + retr_passphrase.prompt = virBufferCurrentContent(&buff); > > This shouldn't really be used. Please get the content into a variable. Not a problem to change this, but could you please explain a bit more on the reasons? (So I can avoid doing the same again.)

> > + p = virStrncpy(buf, retr_passphrase.result, > > + retr_passphrase.resultlen, len); > > + VIR_FREE(retr_passphrase.result); > > Since this contains the passprhase, it should be overwritten. Please use > one of the VIR_DISPOSE... helpers. OK. Note that the libssh2 transport has similar issues.

> > + if (!p) { > > + virReportError(VIR_ERR_LIBSSH, "%s", > > + _("authentication buffer too long for provided " > > + "passphrase")); > > The passphrase is too long for the buffer. > > > + return -1; > > + } > > + > > + return 0; > > +} > > + > > +/* perform private key authentication > > + * > > + * returns SSH_AUTH_* values > > + */ > > +static int > > +virNetLibsshAuthenticatePrivkey(virNetLibsshSessionPtr sess, > > + virNetLibsshAuthMethodPtr priv) > > +{ > > + int err; > > + int ret; > > + char *tmp = NULL; > > + ssh_key public_key = NULL; > > + ssh_key private_key = NULL; > > + > > + VIR_DEBUG("sess=%p", sess); > > + > > + /* try open the key with the password set, first */ > > + ret = ssh_pki_import_privkey_file(priv->filename, priv->password, > > + virNetLibsshAuthenticatePrivkeyCb, > > + sess, &private_key); > > + if (ret == SSH_EOF) { > > + virReportError(VIR_ERR_AUTH_FAILED, > > + _("error while reading private key '%s'"), > > + priv->filename); > > Isn't this overwriting an error reported by the callback trying to open > the file? Not in this case, ... > > + err = SSH_AUTH_ERROR; > > + goto error; > > + } else if (ret == SSH_ERROR) { > > + virReportError(VIR_ERR_AUTH_FAILED, > > + _("error while opening private key '%s', wrong " > > + "passphrase?"), > > + priv->filename); > > Same here. ... but it can happen in this case, and when the callback fails. Would it be acceptable to call virResetLastError() before calling ssh_pki_import_privkey_file(), then check virGetLastError()->code to see whether an error was set and if not set the above one?

> > +int > > +virNetLibsshSessionConnect(virNetLibsshSessionPtr sess, > > + int sock) > > +{ > > + int ret; > > + const char *errmsg; > > + > > + VIR_DEBUG("sess=%p, sock=%d", sess, sock); > > + > > + if (!sess || sess->state != VIR_NET_LIBSSH_STATE_NEW) { > > + virReportError(VIR_ERR_LIBSSH, "%s", > > + _("Invalid virNetLibsshSessionPtr")); > > + return -1; > > + } > > + > > + virObjectLock(sess); > > + > > + /* check if configuration is valid */ > > + if ((ret = virNetLibsshValidateConfig(sess)) < 0) > > + goto error; > > + > > + /* read ~/.ssh/config */ > > + if ((ret = ssh_options_parse_config(sess->session, NULL)) < 0) > > + goto error; > > + > > + /* set the socket FD for the libssh session */ > > + if ((ret = ssh_options_set(sess->session, SSH_OPTIONS_FD, &sock)) < 0) > > Is this guaranteed to copy the socket number at call time? Otherwise > (similarly to the ones above will not work reliably). I don't understand this sentence (it seems truncated), can you please rephrase it?

All the rest of the issues have been fixed, waiting for the feeback wrt the splitting before sending v3. Thanks, -- Pino Toscano

-- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list