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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list