[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]