On Tue, Oct 18, 2016 at 16:42:26 +0200, Pino Toscano wrote:
> 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)
No, this is exactly the split I was thinking about.
> > > 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.
Cool!
> > > + 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.
Hmm, I'm wondering whether that code was refactored or was wrong from
the beginning :).
Anyways I'll probably visit it and clean it up.
> > > + 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.)
Basically any call any function that uses the buffer invalidates the
pointer returned by virBufferCurrentContent (or should be treated as
such) which is prone to bugs when later modifying the code.
Most of the uses of the function are in special cases (or should be
removed).
> > > + 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.
Yep, I'll try to scrub them in a follow up.
> > > + 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?
So the callback is not guaranteed to be called at this point?
Resetting the error should not be necessary as you should not get into a
situation where the error was reported prior to entering this function.
Checking whether an error was reported is fine if you are not certain
that the callback worked.
> > > +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?
Sorry I probably got sidetracked and did not finish my thought.
My question was whether ssh_options_set accesses the pointer to 'sock'
right away and copies it. You are passing a pointer to a stack allocated
variable which will get out of scope later, thus the pointer should not
be accessed after the call to ssh_options_set returns.
It's used at least in two places in this patch.
>
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list