Re: [libvirt] [PATCHv2 1/4] libssh2_transport: add main libssh2 transport implementation

15 Aug 2012

On Sat, Aug 11, 2012 at 11:20:59PM +0200, Peter Krempa wrote:
...

diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
index 913fc5d..9c751b6 100644
--- a/include/libvirt/virterror.h
+++ b/include/libvirt/virterror.h
@@ -110,6 +110,7 @@ typedef enum {
     VIR_FROM_AUTH = 46,         /* Error from auth handling */
     VIR_FROM_DBUS = 47,         /* Error from DBus */
     VIR_FROM_PARALLELS = 48,    /* Error from Parallels */
+    VIR_FROM_LIBSSH = 49,       /* Error from libssh2 connection transport */
# ifdef VIR_ENUM_SENTINELS
     VIR_ERR_DOMAIN_LAST
@@ -279,6 +280,7 @@ typedef enum {
     VIR_ERR_BLOCK_COPY_ACTIVE = 83,     /* action prevented by block copy job */
     VIR_ERR_OPERATION_UNSUPPORTED = 84, /* The requested operation is not
                                            supported */
+    VIR_ERR_LIBSSH2_ERROR = 85,         /* error in libssh2 transport driver */
Using '_ERROR' as a suffix is redundant here - just call it VIR_ERR_LIBSSH2
...
diff --git a/src/rpc/virnetlibssh2session.c b/src/rpc/virnetlibssh2session.c
...
+struct _virNetLibSSH2AuthMethod {
+    virNetLibSSH2AuthMethods method;
+    const char *username;
+    const char *password;
+    const char *filename;
The strings here are all dynamically allocated and freed,
so these shouldn't be marked 'const'.
...
+struct _virNetLibSSH2Session {
+    virObject object;
+    virNetLibSSH2SessionState state;
+    virMutex lock;
+
+    /* libssh2 internal stuff */
+    LIBSSH2_SESSION *session;
+    LIBSSH2_CHANNEL *channel;
+    LIBSSH2_KNOWNHOSTS *knownHosts;
+    LIBSSH2_AGENT *agent;
+
+    /* for host key checking */
+    virNetLibSSH2HostkeyVerify hostKeyVerify;
+    const char *knownHostsFile;
+    const char *hostname;
Same as above - remove the const
...
+    int port;
+
+    /* authentication stuff */
+    virConnectAuthPtr cred;
+    virNetLibSSH2AuthCallbackError authCbErr;
+    size_t nauths;
+    virNetLibSSH2AuthMethodPtr *auths;
+
+    /* channel stuff */
+    const char *channelCommand;
Remove the const
...
+    int channelCommandReturnValue;
+
+    /* read cache */
+    char rbuf[VIR_NET_LIBSSH2_BUFFER_SIZE];
+    size_t bufUsed;
+    size_t bufStart;
+};
+
...
+static virClassPtr virNetLibSSH2SessionClass;
+static int
+virNetLibSSH2SessionOnceInit(void)
+{
+    if (!(virNetLibSSH2SessionClass = virClassNew("virNetLibSSH2Session",
+                                                  sizeof(virNetLibSSH2Session),
+                                                  virNetLibSSH2SessionDispose)))
+        return -1;
+
+    return 0;
+}
...
+static int
+virNetLibSSH2AuthenticatePrivkey(virNetLibSSH2SessionPtr sess,
+                                 virNetLibSSH2AuthMethodPtr priv)
+{
+    virConnectCredential retr_passphrase;
+    int i;
+    char *errmsg;
+    int ret;
+
+    /* try open the key with no password */
+    if ((ret = libssh2_userauth_publickey_fromfile(sess->session,
+                                                   priv->username,
+                                                   NULL,
+                                                   priv->filename,
+                                                   priv->password)) == 0)
+        return 0; /* success */
+
+    if (priv->password ||
+        ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED ||
+        ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED) {
+        libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
+        virReportError(VIR_ERR_AUTH_FAILED,
+                       _("authentication with private key '%s' "
+                         "has failed: %s"),
+                       priv->filename, errmsg);
+        return 1; /* auth failed */
+    }
+
+    /* request user's key password */
+    if (!sess->cred || !sess->cred->cb) {
+        virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                       _("No user interaction callback provided: "
+                         "Can't retrieve private key passphrase"));
+        return -1;
+    }
+
+    memset(&retr_passphrase, 0, sizeof(virConnectCredential));
+    retr_passphrase.type = -1;
+
+    for (i = 0; i < sess->cred->ncredtype; i++) {
+        if (sess->cred->credtype[i] == VIR_CRED_PASSPHRASE ||
+            sess->cred->credtype[i] == VIR_CRED_NOECHOPROMPT) {
+            retr_passphrase.type = sess->cred->credtype[i];
+            break;
+        }
+    }
+
+    if (retr_passphrase.type < 0) {
+        virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                       _("no suitable method to retrieve key passphrase"));
+        return -1;
+    }
+
+    if (virAsprintf((char **)&retr_passphrase.prompt,
+                    _("Passphrase for key '%s'"),
+                    priv->filename) < 0) {
+        virReportOOMError();
+        return -1;
+    }
+
+    if (sess->cred->cb(&retr_passphrase, 1, sess->cred->cbdata)) {
+        virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                       _("failed to retrieve private key passphrase: "
+                         "callback has failed"));
+        VIR_FREE(retr_passphrase.prompt);
+        return -1;
+    }
+
+    VIR_FREE(retr_passphrase.prompt);
+
+    ret = libssh2_userauth_publickey_fromfile(sess->session,
+                                              priv->username,
+                                              NULL,
+                                              priv->filename,
+                                              retr_passphrase.result);
+
+    VIR_FREE(retr_passphrase.result);
+
+    if (ret < 0) {
+        libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
+        virReportError(VIR_ERR_AUTH_FAILED,
+                       _("authentication with private key '%s' "
+                         "has failed: %s"),
+                       priv->filename, errmsg);
+
+        if (ret == LIBSSH2_ERROR_PUBLICKEY_UNRECOGNIZED ||
+            ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED)
+            return 1;
+        else
+            return -1;
+    }
+
+    return 0;
+}
+
+/* perform tunelled password authentication
+ *
+ * Returns: 0 on success
+ *          1 on authentication failure
+ *         -1 on error
+ */
+static int
+virNetLibSSH2AuthenticatePassword(virNetLibSSH2SessionPtr sess,
+                                  virNetLibSSH2AuthMethodPtr priv)
+{
+    char *errmsg;
+    int ret;
+
+    /* tunelled password authentication */
+    if ((ret = libssh2_userauth_password(sess->session,
+                                         priv->username,
+                                         priv->password)) < 0) {
+        libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
+        virReportError(VIR_ERR_AUTH_FAILED,
+                       _("tunelled password authentication failed: %s"),
+                       errmsg);
+
+        if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED)
+            return 1;
+        else
+            return -1;
+    }
+    /* auth success */
+    return 0;
+}
+
+/* perform keyboard interactive authentication
+ *
+ * Returns: 0 on success
+ *          1 on authentication failure
+ *         -1 on error
+ */
+static int
+virNetLibSSH2AuthenticateKeyboardInteractive(virNetLibSSH2SessionPtr sess,
+                                             virNetLibSSH2AuthMethodPtr priv)
+{
+    char *errmsg;
+    int ret;
+
+    if (!sess->cred || !sess->cred->cb) {
+        virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                       _("Can't perform keyboard-interactive authentication: "
+                         "Authentication callback not provided "));
+        return -1;
+    }
+
+    /* try the auth infinite number of times, the server should break the
+     * connection if maximum number of bad auth tries is exceeded */
+    while (priv->tries < 0 || priv->tries-- > 0) {
If this is supposed to be infinit as the comment suggests, then
the normal paradigm is

   while (1) {
...
+        ret = libssh2_userauth_keyboard_interactive(sess->session,
+                                                    priv->username,
+                                                    virNetLibSSH2KbIntCb);
+
+        /* check for errors while calling the callback */
+        switch (sess->authCbErr) {
+        case VIR_NET_LIBSSH2_AUTHCB_NO_METHOD:
+            virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                           _("no suitable method to retrieve "
+                             "authentication cretentials"));
+            return -1;
+        case VIR_NET_LIBSSH2_AUTHCB_OOM:
+            virReportOOMError();
+            return -1;
+        case VIR_NET_LIBSSH2_AUTHCB_RETR_ERR:
+            virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                           _("failed to retrieve credentials"));
+            return -1;
+        case VIR_NET_LIBSSH2_AUTHCB_OK:
+            /* everything went fine, let's continue */
+            break;
+        }
+
+        if (ret == 0)
+            /* authentication succeeded */
+            return 0;
+
+        if (ret == LIBSSH2_ERROR_AUTHENTICATION_FAILED)
+            continue; /* try again */
+
+        if (ret < 0) {
+            libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
+            virReportError(VIR_ERR_AUTH_FAILED,
+                           _("keyboard interactive authentication failed: %s"),
+                           errmsg);
+            return -1;
+        }
+    }
+    libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
+    virReportError(VIR_ERR_AUTH_FAILED,
+                   _("keyboard interactive authentication failed: %s"),
+                   errmsg);
+    return 1;
+}
...
+int
+virNetLibSSH2SessionAuthAddKeyboardAuth(virNetLibSSH2SessionPtr sess,
+                                        const char *username,
+                                        int tries)
+{
+    virNetLibSSH2AuthMethodPtr auth;
+    char *user = NULL;
+
+    if (!username) {
+        virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                       _("Username must be provided "
+                         "for ssh agent authentication"));
+        return -1;
+    }
+
+    virMutexLock(&sess->lock);
+
+    if (!(user = strdup(username)))
+        goto no_memory;
+
+
+    if (!(auth = virNetLibSSH2SessionAuthMethodNew(sess))) {
+        virReportOOMError();
+        virMutexUnlock(&sess->lock);
+        return -1;
'goto no_memory' otherwise you leak 'user'
...
+    }
+
+    auth->username = user;
+    auth->tries = tries;
+    auth->method = VIR_NET_LIBSSH2_AUTH_KEYBOARD_INTERACTIVE;
+
+    virMutexUnlock(&sess->lock);
+    return 0;
+
+no_memory:
+    VIR_FREE(user);
+    virReportOOMError();
+    virMutexUnlock(&sess->lock);
+    return -1;
+
+}
+
...
+int
+virNetLibSSH2SessionSetHostKeyVerification(virNetLibSSH2SessionPtr sess,
+                                           const char *hostname,
+                                           int port,
+                                           const char *hostsfile,
+                                           bool readonly,
+                                           virNetLibSSH2HostkeyVerify opt)
+{
+    char *errmsg;
+
+    virMutexLock(&sess->lock);
+
+    sess->port = port;
+    sess->hostKeyVerify = opt;
+
+    VIR_FREE(sess->hostname);
+
+    if (hostname && !(sess->hostname = strdup(hostname)))
+        goto no_memory;
+
+    /* load the known hosts file */
+    if (hostsfile) {
+        if (libssh2_knownhost_readfile(sess->knownHosts,
+                                       hostsfile,
+                                       LIBSSH2_KNOWNHOST_FILE_OPENSSH) < 0) {
+            libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
+            virReportError(VIR_ERR_LIBSSH2_ERROR,
+                           _("unable to load knownhosts file '%s': %s"),
+                           hostsfile, errmsg);
+        }
A return -1 or goto error missing here ?
...
+
+        /* set filename only if writing to the known hosts file is requested */
+
+        if (!readonly) {
+            VIR_FREE(sess->knownHostsFile);
+            if ((sess->knownHostsFile = strdup(hostsfile)) == NULL)
+                goto no_memory;
+        }
+    }
+
+    virMutexUnlock(&sess->lock);
+    return 0;
+
+no_memory:
+    virMutexUnlock(&sess->lock);
+    virReportOOMError();
+    return -1;
+}
+
...
+/* do a read from a ssh channel, used instead of normal read on socket */
+ssize_t
+virNetLibSSH2ChannelRead(virNetLibSSH2SessionPtr sess,
+                         char *buf,
+                         size_t len)
+{
+    ssize_t ret = -1;
+    ssize_t read_n = 0;
+
+    virMutexLock(&sess->lock);
+
+    if (sess->state != VIR_NET_LIBSSH2_STATE_HANDSHAKE_COMPLETE) {
+        if (sess->state == VIR_NET_LIBSSH2_STATE_ERROR_REMOTE)
+            virReportError(VIR_ERR_LIBSSH2_ERROR,
+                           _("Remote program terminated "
+                             "with non-zero code: %d"),
+                           sess->channelCommandReturnValue);
+        else
+            virReportError(VIR_ERR_LIBSSH2_ERROR, "%s",
+                           _("Tried to write socket in error state"));
+
+        virMutexUnlock(&sess->lock);
+        return -1;
+    }
+
+    if (sess->bufUsed > 0) {
+        /* copy the rest (or complete) internal buffer to the output buffer */
+        memcpy(buf,
+               sess->rbuf + sess->bufStart,
+               len > sess->bufUsed?sess->bufUsed:len);
Can you add whitespace around the '?' and ':'
...
+
+        if (len >= sess->bufUsed) {
+            read_n = sess->bufUsed;
+
+            sess->bufStart = 0;
+            sess->bufUsed = 0;
+        } else {
+            read_n = len;
+            sess->bufUsed -= len;
+            sess->bufStart += len;
+
+            goto success;
+        }
+    }
+
+    /* continue reading into the buffer supplied */
+    if (read_n < len) {
+        ret = libssh2_channel_read(sess->channel,
+                                   buf + read_n,
+                                   len - read_n);
+
+        if (ret == LIBSSH2_ERROR_EAGAIN)
+            goto success;
+
+        if (ret < 0)
+            goto error;
+
+        read_n += ret;
+    }
+
+    /* try to read something into the internal buffer */
+    if (sess->bufUsed == 0) {
+        ret = libssh2_channel_read(sess->channel,
+                                   sess->rbuf,
+                                   VIR_NET_LIBSSH2_BUFFER_SIZE);
+
+        if (ret == LIBSSH2_ERROR_EAGAIN)
+            goto success;
+
+        if (ret < 0)
+            goto error;
+
+        sess->bufUsed = ret;
+        sess->bufStart = 0;
+    }
+
+    if (read_n == 0) {
+        /* get rid of data in stderr stream */
+        ret = libssh2_channel_read_stderr(sess->channel,
+                                          sess->rbuf,
+                                          VIR_NET_LIBSSH2_BUFFER_SIZE - 1);
+        if (ret > 0) {
+            sess->rbuf[ret] = '\0';
+            VIR_DEBUG("flushing stderr, data='%s'",  sess->rbuf);
+        }
+    }
+
+    if (libssh2_channel_eof(sess->channel)) {
+        if (libssh2_channel_get_exit_status(sess->channel)) {
+            virReportError(VIR_ERR_LIBSSH2_ERROR,
+                           _("Remote command terminated with non-zero code: %d"),
+                           libssh2_channel_get_exit_status(sess->channel));
+            sess->channelCommandReturnValue = libssh2_channel_get_exit_status(sess->channel);
+            sess->state = VIR_NET_LIBSSH2_STATE_ERROR_REMOTE;
+            virMutexUnlock(&sess->lock);
+            return -1;
+        }
+
+        sess->state = VIR_NET_LIBSSH2_STATE_CLOSED;
+        virMutexUnlock(&sess->lock);
+        return -1;
+    }
+
+success:
+    virMutexUnlock(&sess->lock);
+    return read_n;
+
+error:
+    sess->state = VIR_NET_LIBSSH2_STATE_ERROR;
+    virMutexUnlock(&sess->lock);
+    return ret;
+}
+
...
diff --git a/src/rpc/virnetlibssh2session.h b/src/rpc/virnetlibssh2session.h
new file mode 100644
index 0000000..88d4307
--- /dev/null
+++ b/src/rpc/virnetlibssh2session.h
...
+# include "internal.h"
+
+typedef struct _virNetLibSSH2Session virNetLibSSH2Session;
+typedef virNetLibSSH2Session *virNetLibSSH2SessionPtr;
+
+virNetLibSSH2SessionPtr virNetLibSSH2SessionNew(void);
+void virNetLibSSH2SessionFree(virNetLibSSH2SessionPtr sess);
+
+typedef enum {
+    VIR_NET_LIBSSH2_HOSTKEY_VERIFY_NORMAL,
+    VIR_NET_LIBSSH2_HOSTKEY_VERIFY_AUTO_ADD,
+    VIR_NET_LIBSSH2_HOSTKEY_VERIFY_IGNORE
+} virNetLibSSH2HostkeyVerify;
+
+int virNetLibSSH2SessionSetChannelCommand(virNetLibSSH2SessionPtr sess,
+                                          const char *command);
+
+void virNetLibSSH2SessionAuthReset(virNetLibSSH2SessionPtr sess);
+
+int virNetLibSSH2SessionAuthSetCallback(virNetLibSSH2SessionPtr sess,
+                                        virConnectAuthPtr auth);
+
+int virNetLibSSH2SessionAuthAddPasswordAuth(virNetLibSSH2SessionPtr sess,
+                                            const char *username,
+                                            const char *password);
+
+int virNetLibSSH2SessionAuthAddAgentAuth(virNetLibSSH2SessionPtr sess,
+                                         const char *username);
+
+int virNetLibSSH2SessionAuthAddPrivKeyAuth(virNetLibSSH2SessionPtr sess,
+                                           const char *username,
+                                           const char *keyfile,
+                                           const char *password);
+
+int virNetLibSSH2SessionAuthAddKeyboardAuth(virNetLibSSH2SessionPtr sess,
+                                            const char *username,
+                                            int tries);
+
+int virNetLibSSH2SessionSetHostKeyVerification(virNetLibSSH2SessionPtr sess,
+                                               const char *hostname,
+                                               int port,
+                                               const char *hostsfile,
+                                               bool readonly,
+                                               virNetLibSSH2HostkeyVerify opt);
+
+int virNetLibSSH2SessionConnect(virNetLibSSH2SessionPtr sess,
+                                int sock);
+
+ssize_t virNetLibSSH2ChannelRead(virNetLibSSH2SessionPtr sess,
+                                 char *buf,
+                                 size_t len);
+
+ssize_t virNetLibSSH2ChannelWrite(virNetLibSSH2SessionPtr sess,
+                                  const char *buf,
+                                  size_t len);
+
+bool virNetLibSSH2SessionHasCachedData(virNetLibSSH2SessionPtr sess);
+
+#endif /* ___VIR_NET_LIBSSH2_SESSION_H_ */
I think I'd be inclined to change the filename and API names throughout
this patch to just use 'ssh' instead of 'libssh2'. eg

  virNetSSHSessionHasCachedData

and virnetsshsession.{c,h}

For comparison see the SASL or TLS code which uses a naming that is
independant of the implementation (Cyrus-SASL / GNU-TLS).

ACK if you rename the APIs and fix the couple of minor issues mentioned
inline

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

    

Re: [libvirt] [PATCHv2 1/4] libssh2_transport: add main libssh2 transport implementation

Daniel P. Berrange