[libvirt] [PATCH RFCv2 1/5] libssh2_transport: add main libssh2 transport implementation
Michal Privoznik
mprivozn at redhat.com
Wed Jan 18 14:58:45 UTC 2012
On 04.01.2012 00:47, Peter Krempa wrote:
> This patch adds helper functions to libssh2 that enable us to use
> libssh2 in conjunction with libvirt-native virNetSocket-s instead of
> using a spawned "ssh" client process.
>
> This implemetation supports tunneled plaintext, keyboard-interactive,
> private key, ssh agent based and null authentication. Libvirt's Auth
> callback is used for interaction with the user. (Keyboar interactive
s/Keyboar/Keyboard/
> authentication, adding of host keys, private key passphrases). This
> enables seamless integration into the application using libvirt, as no
> helpers as "ssh-askpass" are needed.
>
> Reading and writing of OpenSSH style "knownHosts" files is supported.
>
> Communication is done using SSH exec channel, where the user may specify
> arbitrary command to be executed on the remote side and reads and writes
> to/from stdin/out are sent through the ssh channel. Usage of stderr is
> not supported (by this code).
>
> As a bonus, this should (untested) add SSH support to libvirt clients
> running on Windows.
>
> TODO:
> - add default paths for private key and known hosts file on unix systems
>
> * configure.ac: Add checking for correct version and version dependent
> define
> * po/POTFILES.in: Add to translation.
> * src/Makefile.am: specify paths to new files
> * src/rpc/virnetlibsshcontext.c: Helpers for keeping ssh session context
> * src/rpc/virnetlibsshcontext.h: headers for above
> ---
> configure.ac | 40 ++-
> include/libvirt/virterror.h | 4 +
> po/POTFILES.in | 1 +
> src/Makefile.am | 9 +
> src/rpc/virnetlibsshcontext.c | 1132 +++++++++++++++++++++++++++++++++++++++++
> src/rpc/virnetlibsshcontext.h | 76 +++
> src/util/virterror.c | 14 +
> 7 files changed, 1272 insertions(+), 4 deletions(-)
> create mode 100644 src/rpc/virnetlibsshcontext.c
> create mode 100644 src/rpc/virnetlibsshcontext.h
>
> diff --git a/configure.ac b/configure.ac
> index 46a9129..ed668d3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -73,6 +73,7 @@ OPENWSMAN_REQUIRED="2.2.3"
> LIBPCAP_REQUIRED="1.0.0"
> LIBNL_REQUIRED="1.1"
> LIBSSH2_REQUIRED="1.0"
> +LIBSSH2_TRANSPORT_REQUIRED="1.3"
> LIBBLKID_REQUIRED="2.17"
>
> dnl Checks for C compiler.
> @@ -331,6 +332,8 @@ AC_ARG_WITH([remote],
> AC_HELP_STRING([--with-remote], [add remote driver support @<:@default=yes@:>@]),[],[with_remote=yes])
> AC_ARG_WITH([libvirtd],
> AC_HELP_STRING([--with-libvirtd], [add libvirtd support @<:@default=yes@:>@]),[],[with_libvirtd=yes])
> +AC_ARG_WITH([libssh2_transport],
> + AC_HELP_STRING([--with-libssh2_transport], [libssh2 location @<:@default=check@:>@]),[],[with_libssh2_transport=check])
>
> dnl
> dnl in case someone want to build static binaries
> @@ -1476,29 +1479,58 @@ AM_CONDITIONAL([WITH_UML], [test "$with_uml" = "yes"])
>
>
> dnl
> -dnl check for libssh2 (PHYP)
> +dnl check for libssh2 (PHYP and libssh2 transport)
> dnl
>
> LIBSSH2_CFLAGS=""
> LIBSSH2_LIBS=""
>
> -if test "$with_phyp" = "yes" || test "$with_phyp" = "check"; then
> +if test "$with_phyp" = "yes" || test "$with_phyp" = "check" ||
> + test "$with_libssh2_transport" = "yes" || test "$with_libssh2_transport" = "check"; then
> PKG_CHECK_MODULES([LIBSSH2], [libssh2 >= $LIBSSH2_REQUIRED], [
> - with_phyp=yes
> + if test "$with_phyp" = "check"; then
> + with_phyp=yes
> + fi
> + if $PKG_CONFIG "libssh2 >= $LIBSSH2_TRANSPORT_REQUIRED"; then
> + if test "$with_libssh2_transport" = "check"; then
> + with_libssh2_transport=yes
> + fi
> + else
> + if test "$with_libssh2_transport" = "check"; then
> + with_libssh2_transport=no
> + AC_MSG_NOTICE([libssh2 >= $LIBSSH2_TRANSPORT_REQUIRED is required for libssh2 transport])
> + fi
> + if test "$with_libssh2_transport" = "yes"; then
> + AC_MSG_ERROR([libssh2 >= $LIBSSH2_TRANSPORT_REQUIRED is required for libssh2 transport])
> + fi
> + fi
> ], [
> if test "$with_phyp" = "check"; then
> with_phyp=no
> AC_MSG_NOTICE([libssh2 is required for Phyp driver, disabling it])
> - else
> + fi
> + if test "$with_phyp" = "yes"; then
> AC_MSG_ERROR([libssh2 >= $LIBSSH2_REQUIRED is required for Phyp driver])
> fi
> + if test "$with_libssh2_transport" = "check"; then
> + with_libssh2_transport=no
> + AC_MSG_NOTICE([libssh2 >= $LIBSSH2_TRANSPORT_REQUIRED is required for libssh2 transport])
> + fi
> + if test "$with_libssh2_transport" = "yes"; then
> + AC_MSG_ERROR([libssh2 >= $LIBSSH2_TRANSPORT_REQUIRED is required for libssh2 transport])
> + fi
> ])
> fi
>
> if test "$with_phyp" = "yes"; then
> AC_DEFINE_UNQUOTED([WITH_PHYP], 1, [whether IBM HMC / IVM driver is enabled])
> fi
> +if test "$with_libssh2_transport" = "yes"; then
> + AC_DEFINE_UNQUOTED([HAVE_LIBSSH2], 1, [wether libssh2 transport is enabled])
> +fi
> +
> AM_CONDITIONAL([WITH_PHYP],[test "$with_phyp" = "yes"])
> +AM_CONDITIONAL([HAVE_LIBSSH2], [test "$with_libssh2_transport" = "yes"])
>
> AC_SUBST([LIBSSH2_CFLAGS])
> AC_SUBST([LIBSSH2_LIBS])
> diff --git a/include/libvirt/virterror.h b/include/libvirt/virterror.h
> index e896d67..d190a37 100644
> --- a/include/libvirt/virterror.h
> +++ b/include/libvirt/virterror.h
> @@ -85,6 +85,7 @@ typedef enum {
> VIR_FROM_LOCKING = 42, /* Error from lock manager */
> VIR_FROM_HYPERV = 43, /* Error from Hyper-V driver */
> VIR_FROM_CAPABILITIES = 44, /* Error from capabilities */
> + VIR_FROM_LIBSSH = 45, /* Error from libssh2 connection transport */
> } virErrorDomain;
>
>
> @@ -243,6 +244,9 @@ typedef enum {
> risky domain snapshot revert */
> VIR_ERR_OPERATION_ABORTED = 78, /* operation on a domain was
> canceled/aborted by user */
> + VIR_ERR_LIBSSH_REMOTE_COMMAND = 79, /* remote command of a libssh2
> + connection failed */
> + VIR_ERR_LIBSSH_ERROR = 80, /* error in libssh2 transport driver */
> } virErrorNumber;
>
> /**
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 3e8359a..3113414 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -77,6 +77,7 @@ src/rpc/virkeepalive.c
> src/rpc/virnetclient.c
> src/rpc/virnetclientprogram.c
> src/rpc/virnetclientstream.c
> +src/rpc/virnetlibsshcontext.c
> src/rpc/virnetmessage.c
> src/rpc/virnetsaslcontext.c
> src/rpc/virnetsocket.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 93bf54c..d5b91cf 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1392,6 +1392,13 @@ libvirt_net_rpc_la_SOURCES = \
> rpc/virnettlscontext.h rpc/virnettlscontext.c \
> rpc/virkeepaliveprotocol.h rpc/virkeepaliveprotocol.c \
> rpc/virkeepalive.h rpc/virkeepalive.c
> +if HAVE_LIBSSH2
> +libvirt_net_rpc_la_SOURCES += \
> + rpc/virnetlibsshcontext.h rpc/virnetlibsshcontext.c
> +else
> +EXTRA_DIST += \
> + rpc/virnetlibsshcontext.h rpc/virnetlibsshcontext.c
> +endif
> if HAVE_SASL
> libvirt_net_rpc_la_SOURCES += \
> rpc/virnetsaslcontext.h rpc/virnetsaslcontext.c
> @@ -1402,11 +1409,13 @@ endif
> libvirt_net_rpc_la_CFLAGS = \
> $(GNUTLS_CFLAGS) \
> $(SASL_CFLAGS) \
> + $(LIBSSH2_CFLAGS) \
> $(XDR_CFLAGS) \
> $(AM_CFLAGS)
> libvirt_net_rpc_la_LDFLAGS = \
> $(GNUTLS_LIBS) \
> $(SASL_LIBS) \
> + $(LIBSSH2_LIBS)\
> $(AM_LDFLAGS) \
> $(CYGWIN_EXTRA_LDFLAGS) \
> $(MINGW_EXTRA_LDFLAGS)
> diff --git a/src/rpc/virnetlibsshcontext.c b/src/rpc/virnetlibsshcontext.c
> new file mode 100644
> index 0000000..0769f4c
> --- /dev/null
> +++ b/src/rpc/virnetlibsshcontext.c
> @@ -0,0 +1,1132 @@
> +/*
> + * virnetlibsshcontext.c: libssh network transport provider
> + *
> + * Copyright (C) 2011 Red Hat, Inc.
s/2011/2012/ and likewise in .h counterpart
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Author: Peter Krempa <pkrempa at redhat.com>
> + */
> +
> +#include <config.h>
> +#include <libssh2.h>
> +#include <libssh2_publickey.h>
> +
> +#include "virnetlibsshcontext.h"
> +
> +#include "internal.h"
> +#include "buf.h"
> +#include "memory.h"
> +#include "logging.h"
> +#include "configmake.h"
> +#include "threads.h"
> +#include "util.h"
> +#include "virterror_internal.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_LIBSSH
> +#define virSSHError(code, ...) \
> + virReportErrorHelper(VIR_FROM_THIS, code, __FILE__, \
> + __FUNCTION__, __LINE__, __VA_ARGS__)
> +
> +#define VIR_NET_LIBSSH_DEFAULT_KNOWN_HOSTS_PATH NULL
> +
> +static const char vir_libssh2_key_comment[] = "added by libvirt libssh transport";
> +#define VIR_NET_LIBSSH_BUFFER_SIZE 512
> +
> +typedef enum {
> + VIR_NET_LIBSSH_STATE_NEW,
> + VIR_NET_LIBSSH_STATE_HANDSHAKE_COMPLETE,
> + VIR_NET_LIBSSH_STATE_AUTH_CALLBACK_ERROR,
> + VIR_NET_LIBSSH_STATE_CLOSED,
> + VIR_NET_LIBSSH_STATE_ERROR,
> + VIR_NET_LIBSSH_STATE_ERROR_REMOTE,
> +} virNetLibSSHSessionState;
> +
> +typedef enum {
> + VIR_NET_LIBSSH_AUTHCB_OK,
> + VIR_NET_LIBSSH_AUTHCB_NO_METHOD,
> + VIR_NET_LIBSSH_AUTHCB_OOM,
> + VIR_NET_LIBSSH_AUTHCB_RETR_ERR,
> +} virNetLibSSHAuthCallbackError;
> +
> +struct _virNetLibSSHSession {
> + virNetLibSSHSessionState state;
> + virMutex lock;
> +
> + /* libssh2 internal stuff */
> + LIBSSH2_SESSION *session;
> + LIBSSH2_CHANNEL *channel;
> + LIBSSH2_KNOWNHOSTS *knownHosts;
> + LIBSSH2_AGENT *agent;
> +
> + /* for host key checking */
> + virNetLibSSHHostkeyVerify hostKeyVerify;
> + const char *knownHostsFile;
> + const char *hostname;
> + int port;
> +
> + /* authentication stuff */
> + const char *username;
> + const char *password;
> + const char *privkey;
> + virConnectAuthPtr cred;
> + virNetLibSSHAuthCallbackError authCbErr;
> +
> + /* channel stuff */
> + const char *channelCommand;
> + int channelCommandReturnValue;
> +
> + /* read cache */
> + char rbuf[VIR_NET_LIBSSH_BUFFER_SIZE];
> + size_t bufUsed;
> + size_t bufStart;
For consistency sake I'd rather use bufferLenght and bufferOffset.
> +};
> +
> +/* keyboard interactive authentication callback */
> +static void virNetLibSSHKbIntCb(const char *name ATTRIBUTE_UNUSED,
> + int name_len ATTRIBUTE_UNUSED,
> + const char *instruction ATTRIBUTE_UNUSED,
> + int instruction_len ATTRIBUTE_UNUSED,
> + int num_prompts,
> + const LIBSSH2_USERAUTH_KBDINT_PROMPT *prompts,
> + LIBSSH2_USERAUTH_KBDINT_RESPONSE *responses,
> + void **opaque)
> +{
> + virNetLibSSHSessionPtr priv = *opaque;
> + virConnectCredentialPtr askcred = NULL;
> + int i;
> + int credtype_echo = -1;
> + int credtype_noecho = -1;
> + char *tmp;
> +
> + priv->authCbErr = VIR_NET_LIBSSH_AUTHCB_OK;
> +
> + /* find credential type for asking passwords */
> + for (i = 0; i < priv->cred->ncredtype; i++) {
> + if (priv->cred->credtype[i] == VIR_CRED_PASSPHRASE ||
> + priv->cred->credtype[i] == VIR_CRED_NOECHOPROMPT)
> + credtype_noecho = priv->cred->credtype[i];
> +
> + if (priv->cred->credtype[i] == VIR_CRED_ECHOPROMPT)
> + credtype_echo = priv->cred->credtype[i];
> + }
> +
> + if (credtype_echo < 0 || credtype_noecho < 0) {
> + priv->authCbErr = VIR_NET_LIBSSH_AUTHCB_NO_METHOD;
> + return;
> + }
> +
> + if (VIR_ALLOC_N(askcred, num_prompts) < 0) {
> + priv->authCbErr = VIR_NET_LIBSSH_AUTHCB_OOM;
> + return;
> + }
> +
> + /* fill data structures for auth callback */
> + for (i = 0; i < num_prompts; i++) {
> + /* remove colon and trailing spaces from prompts, as default behavior
> + * of libvirt's auth callback is to add them */
> + if ((tmp = strrchr(prompts[i].text, ':')))
> + *tmp = '\0';
> +
> + askcred[i].prompt = prompts[i].text;
> + askcred[i].type = prompts[i].echo?credtype_echo:credtype_noecho;
Add spaces around '?' and ':';
> + }
> +
> + /* retrieve responses using the auth callback */
> + if (priv->cred->cb(askcred, num_prompts, priv->cred->cbdata)) {
> + priv->authCbErr = VIR_NET_LIBSSH_AUTHCB_RETR_ERR;
> + goto cleanup;
> + }
> +
> + /* copy retrieved data back */
> + for (i = 0; i < num_prompts; i++) {
> + responses[i].text = askcred[i].result;
> + askcred[i].result = NULL; /* steal the pointer */
> + responses[i].length = askcred[i].resultlen;
> + }
> +
> +cleanup:
> + if (askcred)
> + for (i = 0; i < num_prompts; i++)
> + VIR_FREE(askcred[i].result);
> +
> + VIR_FREE(askcred);
> +
> + return;
Anyway, I am wondering if we could perhaps tweak virRequestPassword from
authhelper.c to use it instead of this. As it seems to me they are doing
pretty the same. Same applies for virNetLibSSHAuthenticate.
> +}
> +
> +/* check session host keys
> + *
> + * this function checks the known host database and verifies the key
> + * errors are raised in this func
> + *
> + * return value: 0 on success, not 0 otherwise
> + */
> +static int virNetLibSSHCheckHostKey(virNetLibSSHSessionPtr sess)
> +{
> + int ret;
> + const char *key;
> + const char *keyhash;
> + int keyType;
> + size_t keyLength;
> + char *errmsg;
> + virBuffer buff = VIR_BUFFER_INITIALIZER;
> + virConnectCredential askKey;
> + struct libssh2_knownhost *knownHostEntry = NULL;
> + int i;
> + char *hostnameStr = NULL;
> +
> + if (sess->hostKeyVerify == VIR_NET_LIBSSH_HOSTKEY_VERIFY_IGNORE)
> + return 0;
> +
> + /* get the key */
> + key = libssh2_session_hostkey(sess->session, &keyLength, &keyType);
> + if (!key) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("Failed to retrieve ssh host key: %s"),
> + errmsg);
> + return -1;
> + }
> +
> + /* verify it */
> + ret = libssh2_knownhost_checkp(sess->knownHosts,
> + sess->hostname,
> + sess->port,
> + key,
> + keyLength,
> + LIBSSH2_KNOWNHOST_TYPE_PLAIN |
> + LIBSSH2_KNOWNHOST_KEYENC_RAW,
> + &knownHostEntry);
> +
> + switch (ret) {
> + case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
> + /* key was not found, query to add it to database */
> + if (sess->hostKeyVerify == VIR_NET_LIBSSH_HOSTKEY_VERIFY_NORMAL) {
> + /* ask to add the key */
> + if (!sess->cred || !sess->cred->cb) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("No user interaction callback provided: "
> + "Can't verify the session host key"));
> + return -1;
> + }
> +
> + /* prepare data for the callback */
> + memset(&askKey, 0, sizeof(virConnectCredential));
> +
> + for (i = 0; i < sess->cred->ncredtype; i++) {
> + if (sess->cred->credtype[i] == VIR_CRED_ECHOPROMPT) {
> + i = -1;
> + break;
> + }
> + }
> +
> + if (i > 0) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("no suitable method to retrieve "
> + "authentication cretentials"));
s/cretentials/credentials/
> + return -1;
> + }
> +
> + /* calculate remote key hash, using MD5 algorithm that is
> + * usual in OpenSSH. The returned value should *NOT* be freed*/
> + if (!(keyhash = libssh2_hostkey_hash(sess->session,
> + LIBSSH2_HOSTKEY_HASH_MD5))) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("failed to calculate ssh host key hash"));
> + return -1;
> + }
> + /* format the host key into a nice userfriendly string.
> + * Sadly, there's no constant to describe the hash length, so
> + * we have to use a *MAGIC* constant. */
> + for (i = 0; i < 16; i++) {
> + virBufferAsprintf(&buff, "%2X", (unsigned char) keyhash[i]);
s/%2X/%02X/
as we want chunks to be eventually prepended with 0's.
In addition, I'd like to see these magic constants as #define even
they'd be used only once.
> + if (i != 15)
> + virBufferAddChar(&buff, ':');
> + }
> +
> + if (virBufferError(&buff) != 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + keyhash = virBufferContentAndReset(&buff);
> +
> + askKey.type = VIR_CRED_ECHOPROMPT;
> + if (virAsprintf((char **)&askKey.prompt,
> + _("Accept SSH host key with hash '%s' for "
> + "host '%s:%d' (%s/%s)?"),
> + keyhash,
> + sess->hostname, sess->port,
> + _("yes"), _("no")) < 0) {
Is there any reason for having "yes" and "no" separately and not
directly in the 1st string?
> + virReportOOMError();
> + VIR_FREE(keyhash);
> + return -1;
> + }
> +
> + if (sess->cred->cb(&askKey, 1, sess->cred->cbdata)) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("failed to retrieve decision to accept host key"));
> + VIR_FREE(askKey.prompt);
> + VIR_FREE(keyhash);
> + return -1;
> + }
> +
> + VIR_FREE(askKey.prompt);
> +
> + if (STRNEQ_NULLABLE(askKey.result, _("yes"))) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("SSH host key for '%s' (%s) was not accepted"),
> + sess->hostname, keyhash);
> + VIR_FREE(keyhash);
> + VIR_FREE(askKey.result);
> + return -1;
> + }
> + VIR_FREE(keyhash);
> + VIR_FREE(askKey.result);
> + }
> +
> + /* VIR_NET_LIBSSH_HOSTKEY_VERIFY_AUTO_ADD */
> + /* convert key type, as libssh is using different enums type for
> + * getting the key and different for adding ... */
> + switch (keyType) {
> + case LIBSSH2_HOSTKEY_TYPE_RSA:
> + keyType = LIBSSH2_KNOWNHOST_KEY_SSHRSA;
> + break;
> + case LIBSSH2_HOSTKEY_TYPE_DSS:
> + keyType = LIBSSH2_KNOWNHOST_KEY_SSHDSS;
> +
> + case LIBSSH2_HOSTKEY_TYPE_UNKNOWN:
Since we have default right after, this case is pretty useless.
> + default:
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("unsupported SSH key type"));
> + return -1;
> + }
However, whole switch is awful. But that's libssh's fault if they use
different values for get and add.
> +
> +
> + /* add the key to the DB and save it, if applicable */
> + /* construct a "[hostname]:port" string to have the hostkey bound
> + * to port number */
> + virBufferAsprintf(&buff, "[%s]:%d", sess->hostname, sess->port);
> + if (virBufferError(&buff) != 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + hostnameStr = virBufferContentAndReset(&buff);
This can be joined with the former if, as virBufferContentAndReset()
returns NULL if buffer has an error.
> +
> + if (libssh2_knownhost_addc(sess->knownHosts,
> + hostnameStr,
> + NULL,
> + key,
> + keyLength,
> + vir_libssh2_key_comment,
> + strlen(vir_libssh2_key_comment),
> + LIBSSH2_KNOWNHOST_TYPE_PLAIN |
> + LIBSSH2_KNOWNHOST_KEYENC_RAW |
> + keyType,
> + NULL) < 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("unable to add SSH host key for host '%s': %s"),
> + hostnameStr, errmsg);
> + VIR_FREE(hostnameStr);
> + return -1;
> + }
> +
> + VIR_FREE(hostnameStr);
> +
> + /* write the host key file - if applicable */
> + if (sess->knownHostsFile) {
> + if (libssh2_knownhost_writefile(sess->knownHosts,
> + sess->knownHostsFile,
> + LIBSSH2_KNOWNHOST_FILE_OPENSSH) < 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("failed to write known_host file '%s': %s"),
> + sess->knownHostsFile,
> + errmsg);
> + return -1;
> + }
> + }
> + /* key was accepted and added */
> + return 0;
> +
> + case LIBSSH2_KNOWNHOST_CHECK_MATCH:
> + /* host key matches */
> + return 0;
> +
> + case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
> + /* host key verification failed */
> + // TODO: Better message!
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("!!! SSH HOST KEY VERIFICATION FAILED !!!: "
> + "Failed to authenticate identity of host '%s:%d' "
> + "with host key '%s'"),
> + sess->hostname, sess->port,
> + knownHostEntry->key);
> + return -1;
> +
> + case LIBSSH2_KNOWNHOST_CHECK_FAILURE:
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("failed to validate SSH host key: %s"),
> + errmsg);
> + return -1;
> +
> + default: /* should never happen (tm) */
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "Unknown error value");
> + return -1;
> + }
> +
> + return -1;
> +}
> +
> +/* select auth method and authenticate */
> +static int virNetLibSSHAuthenticate(virNetLibSSHSessionPtr sess)
> +{
> + char *auth_list;
> + char *errmsg;
> + int ret;
> +
> + /* retrieval of private key passphrase */
> + virConnectCredential retr_passphrase;
> + int i;
> +
> + /* for ssh agent based authentication */
> + struct libssh2_agent_publickey *agent_identity;
> + struct libssh2_agent_publickey *agent_prev_identity = NULL;
> +
> +
> + /* obtain list of supported auth methods */
> + auth_list = libssh2_userauth_list(sess->session,
> + sess->username,
> + strlen(sess->username));
> + if (!auth_list) {
> + /* unlikely event, authentication succeeded with NONE as method */
> + if (libssh2_userauth_authenticated(sess->session) == 1)
> + return 0;
> +
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("couldn't retrieve authentication methods list: %s"),
> + errmsg);
> + return -1;
> + }
> +
> + /* go through supported auth methods and try the first one, that works */
> + if (strstr(auth_list, "publickey")) {
> + /* try ssh agent authentication */
> + if (libssh2_agent_connect(sess->agent) < 0) {
> + VIR_DEBUG("Failed to connect to ssh agent");
> + goto agent_error;
> + }
> +
> + if (libssh2_agent_list_identities(sess->agent) < 0) {
> + VIR_DEBUG("Failed to list ssh agent identities");
> + goto agent_error;
> + }
> +
> + while ((ret = libssh2_agent_get_identity(sess->agent,
> + &agent_identity,
> + agent_prev_identity)) == 0) {
> + if ((ret = libssh2_agent_userauth(sess->agent,
> + sess->username,
> + agent_identity)) == 0) {
> + VIR_DEBUG("Agent based authentication succeeded");
> + return 0;
> + }
> +
> + if (ret != LIBSSH2_ERROR_AUTHENTICATION_FAILED) {
In fact, ssh agent might be giving us some keys which don't belong to
the server we are loggin in. In this case
LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED is returned. So check ret for this
value as well.
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_AUTH_FAILED,
> + _("failed to authenticate using SSH agent: %s"),
> + errmsg);
> + return -1;
> + }
> + /* authentication has failed, try next key */
> + }
> +
> + /* if there are no more keys in the agent, the identity retrieval
> + * function returns > 1 */
> + if (ret < 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_AUTH_FAILED,
> + _("failed to authenticate using SSH agent: %s"),
> + errmsg);
> + return -1;
> + }
> +
> + /* no suitable ssh key provided by ssh agent */
> +agent_error:
> + /* regular public key authentication */
> + if (sess->privkey) {
> + /* try open the key with no password */
> + if ((ret = libssh2_userauth_publickey_fromfile(sess->session,
> + sess->username,
> + NULL,
> + sess->privkey,
> + "")) == 0)
"" or NULL, whatever.
> + return 0; /* success */
> +
> + if (ret != LIBSSH2_ERROR_FILE) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_AUTH_FAILED,
> + _("authentication with public key '%s' has failed: %s"),
> + sess->privkey, errmsg);
> + return -1;
> + }
> +
> + /* request user's key password */
> + if (!sess->cred || !sess->cred->cb) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("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) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("no suitable method to retrieve "
> + "key passphrase"));
> + return -1;
> + }
> +
> + if (virAsprintf((char **)&retr_passphrase.prompt,
> + _("Passphrase for key '%s'"),
> + sess->privkey) < 0) {
> + virReportOOMError();
> + return -1;
> + }
> +
> + if (sess->cred->cb(&retr_passphrase, 1, sess->cred->cbdata)) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("failed to retrieve private key passphrase: "
> + "callback has failed"));
> + VIR_FREE(retr_passphrase.prompt);
> + return -1;
> + }
> +
> + VIR_FREE(retr_passphrase.prompt);
> +
> + if (libssh2_userauth_publickey_fromfile(sess->session,
> + sess->username,
> + NULL,
> + sess->privkey,
> + retr_passphrase.result) < 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_AUTH_FAILED,
> + _("authentication with private key '%s' has failed: %s"),
> + sess->privkey, errmsg);
> +
> + VIR_FREE(retr_passphrase.result);
> + return -1;
> + }
> +
> + VIR_FREE(retr_passphrase.result);
> + return 0;
> + }
> + }
> +
> + /* tunelled password authentication */
> + if (sess->password && strstr(auth_list, "password")) {
> + if ((ret = libssh2_userauth_password(sess->session,
> + sess->username,
> + sess->password)) !=
> + LIBSSH2_ERROR_AUTHENTICATION_FAILED) {
> + if (ret < 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_AUTH_FAILED,
> + _("tunelled password authentication failed: %s"),
> + errmsg);
> + }
> +
> + /* auth success */
> + return 0;
> + }
> + /* authentication has failed, try keyboard interactive */
> + }
> +
> + /* keyboard interactive authentication */
> + if (sess->cred &&
> + sess->cred->cb &&
> + strstr(auth_list, "keyboard-interactive")) {
> +
> + /* try the auth infinite number of times, the server should break the
> + * connection if maximum number of bad auth tries is exceeded */
> + while (true) {
> + ret = libssh2_userauth_keyboard_interactive(sess->session,
> + sess->username,
> + virNetLibSSHKbIntCb);
> +
> + /* check for errors while calling the callback */
> + switch (sess->authCbErr) {
> + case VIR_NET_LIBSSH_AUTHCB_NO_METHOD:
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("no suitable method to retrieve "
> + "authentication cretentials"));
> + return -1;
> + case VIR_NET_LIBSSH_AUTHCB_OOM:
> + virReportOOMError();
> + return -1;
> + case VIR_NET_LIBSSH_AUTHCB_RETR_ERR:
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("failed to retrieve credentials"));
> + return -1;
> + case VIR_NET_LIBSSH_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 */
Yeah, I'd try again but not endlessly. Whatabout giving users a limit
for the maximum number of retries? Or should we depend fully on server
how much it will allows?
> +
> + if (ret < 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_AUTH_FAILED,
> + _("keyboard interactive authentication failed: %s"),
> + errmsg);
> + return -1;
> + }
> + }
> + }
> +
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("No suitable authentication method found"));
> +
> + return -1;
> +}
Can we let libssh to actually parse sshconfigs for us? As for some hosts
I use sss-agent, for some I am using keybord-interactive. And this code
deosn'y play nice with this.
> +
> +/* open channel */
> +static int virNetLibSSHOpenChannel(virNetLibSSHSessionPtr sess)
> +{
> + char *errmsg;
> +
> + sess->channel = libssh2_channel_open_session(sess->session);
> + if (!sess->channel) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("failed to open ssh channel: %s"),
> + errmsg);
> + return -1;
> + }
> +
> + if (libssh2_channel_exec(sess->channel, sess->channelCommand) != 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_REMOTE_COMMAND,
> + _("failed to execute command '%s': %s"),
> + sess->channelCommand,
> + errmsg);
> + return -1;
> + }
> +
> + /* nonblocking mode - currently does nothing*/
> + libssh2_channel_set_blocking(sess->channel, 0);
> +
> + /* channel open */
> + return 0;
> +}
> +
> +/* validate if all required parameters are configured */
> +static int virNetLibSSHValidateConfig(const virNetLibSSHSessionPtr sess)
> +{
> + if (!sess->username) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("Username not provided"));
> + return -1;
> + }
> +
> + if (sess->hostKeyVerify != VIR_NET_LIBSSH_HOSTKEY_VERIFY_IGNORE) {
> + if (!sess->hostname) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("Hostname is needed for host key verification"));
> + return -1;
> + }
> + }
> +
> + /* everything ok */
> + return 0;
> +}
> +
> +/* ### PUBLIC API ### */
> +int virNetLibSSHSessionSetAuthCallback(virNetLibSSHSessionPtr sess,
> + virConnectAuthPtr auth)
> +{
> + sess->cred = auth;
> + return 0;
> +}
Any reason for returning int as this *always* succeeds?
> +
> +int virNetLibSSHSessionSetChannelCommand(virNetLibSSHSessionPtr sess,
> + const char *command)
> +{
> + virMutexLock(&sess->lock);
> +
> + if (command) {
> + VIR_FREE(sess->channelCommand);
This has different semantics than the two following functions. In here,
we set channelCommand iff there was passed one. However, the two
following functions allows us to pass NULL and thus erase any value set.
> + if ((sess->channelCommand = strdup(command)) == NULL) {
> + virReportOOMError();
> + virMutexUnlock(&sess->lock);
> + return -1;
> + }
> + }
> +
> + virMutexUnlock(&sess->lock);
> + return 0;
> +}
> +
> +int virNetLibSSHSessionSetCredentials(virNetLibSSHSessionPtr sess,
> + const char *username,
> + const char *password)
> +{
> + virMutexLock(&sess->lock);
> +
> + VIR_FREE(sess->username);
> + VIR_FREE(sess->password);
> +
> + if (username &&
> + !(sess->username = strdup(username)))
> + goto oom;
> +
> + if (password &&
> + !(sess->password = strdup(password)))
> + goto oom;
> +
> + virMutexUnlock(&sess->lock);
> + return 0;
> +
> +oom:
> + virReportOOMError();
> + virMutexUnlock(&sess->lock);
> + return -1;
> +}
> +
> +int virNetLibSSHSessionSetPrivateKey(virNetLibSSHSessionPtr sess,
> + const char *keyfile)
> +{
> + virMutexLock(&sess->lock);
> +
> + VIR_FREE(sess->privkey);
> +
> + if (keyfile) {
> + if ((sess->privkey = strdup(keyfile)) == NULL) {
> + virReportOOMError();
> + virMutexUnlock(&sess->lock);
> + return -1;
> + }
> + }
> +
> + virMutexUnlock(&sess->lock);
> + return 0;
> +}
> +
> +int virNetLibSSHSessionSetHostKeyVerification(virNetLibSSHSessionPtr sess,
> + const char *hostname,
> + const int port,
> + const char *hostsfile,
> + bool readonly,
> + virNetLibSSHHostkeyVerify opt)
> +{
> + char *errmsg;
> +
> + virMutexLock(&sess->lock);
> +
> + sess->port = port;
> + sess->hostKeyVerify = opt;
> +
> + if (hostname) {
> + VIR_FREE(sess->hostname);
> + if ((sess->hostname = strdup(hostname)) == NULL) {
> + virReportOOMError();
> + goto error;
> + }
> + }
> +
> + /* 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);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("unable to load knownhosts file '%s': %s"),
> + hostsfile, errmsg);
> + }
> +
> + /* set filename only if writing to the known hosts file is requested */
> +
> + if (!readonly) {
> + VIR_FREE(sess->knownHostsFile);
> + if ((sess->knownHostsFile = strdup(hostsfile)) == NULL) {
> + virReportOOMError();
> + goto error;
> + }
> + }
> + }
> +
> + virMutexUnlock(&sess->lock);
> + return 0;
> +
> +error:
> + virMutexUnlock(&sess->lock);
> + return -1;
> +}
> +
> +/* allocate and initialize a ssh session object */
> +virNetLibSSHSessionPtr virNetLibSSHSessionNew(void)
> +{
> + virNetLibSSHSessionPtr sess;
> + if (VIR_ALLOC(sess) < 0)
> + return NULL;
> +
> + /* initialize internal structures */
> + if (virMutexInit(&sess->lock) < 0) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("Failed to initialize mutex"));
> + goto error;
> + }
> +
> + /* initialize session data, use the internal data for callbacks
> + * and stick to default memory management functions */
> + if (!(sess->session = libssh2_session_init_ex(NULL,
> + NULL,
> + NULL,
> + (void *)sess))) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("Failed to initialize libssh2 session"));
> + goto error;
> + }
> +
> + if (!(sess->knownHosts = libssh2_knownhost_init(sess->session))) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("Failed to initialize libssh2 known hosts table"));
> + goto error;
> + }
> +
> + if (!(sess->agent = libssh2_agent_init(sess->session))) {
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("Failed to initialize libssh2 agent handle"));
> + goto error;
> + }
> +
> + VIR_DEBUG("virNetLibSSHSessionPtr=0x%p, LIBSSH2_SESSION *=0x%p",
Is there any reason for '0x' just before '%p' format modifier?
'%p' will prepend '0x' anyway.
> + sess,
> + sess->session);
> +
> + /* set blocking mode for libssh2 until handshake is complete */
> + libssh2_session_set_blocking(sess->session, 1);
> +
> + /* default states for config variables */
> + sess->state = VIR_NET_LIBSSH_STATE_NEW;
> + sess->hostKeyVerify = VIR_NET_LIBSSH_HOSTKEY_VERIFY_IGNORE;
> +
> + return sess;
> +
> +error:
> + libssh2_agent_free(sess->agent);
> + libssh2_knownhost_free(sess->knownHosts);
> + libssh2_session_free(sess->session);
> + VIR_FREE(sess);
> + return NULL;
> +}
> +
> +/* close session and free internal data */
> +void virNetLibSSHSessionFree(virNetLibSSHSessionPtr sess)
> +{
> + VIR_DEBUG("sess=0x%p", sess);
> +
> + if (!sess)
> + return;
> +
> + virMutexLock(&sess->lock);
> +
> + if (sess->channel) {
> + libssh2_channel_send_eof(sess->channel);
> + libssh2_channel_close(sess->channel);
> + libssh2_channel_free(sess->channel);
> + }
> +
> + libssh2_knownhost_free(sess->knownHosts);
> + libssh2_agent_free(sess->agent);
> +
> + if (sess->session) {
> + libssh2_session_disconnect(sess->session,
> + "libvirt: virNetLibSSHSessionFree()");
> + libssh2_session_free(sess->session);
> + }
> +
> + VIR_FREE(sess->username);
> + VIR_FREE(sess->password);
> + VIR_FREE(sess->privkey);
> + VIR_FREE(sess->channelCommand);
> + VIR_FREE(sess->hostname);
> + VIR_FREE(sess->knownHostsFile);
> +
Missing virMutexUnlock(&sess->lock); virMutexDestroy(&sess->lock);
I wonder if we want to create onliners for this as we have in the rest
of libvirt code:
void virNetLibSSHSessionLock(virNetLibSSHSessionPtr sess)
{
virMutexLock(&sess->lock);
}
void virNetLibSSHSessionUnlock(virNetLibSSHSessionPtr sess)
{
virMutexUnlock(&sess->lock);
}
> + VIR_FREE(sess);
> +}
> +
> +int virNetLibSSHSessionConnect(virNetLibSSHSessionPtr sess,
> + int sock)
> +{
> + int ret;
> + char *errmsg;
> +
> + VIR_DEBUG("sess=0x%p, sock=%d", sess, sock);
> +
> + if (!sess || sess->state != VIR_NET_LIBSSH_STATE_NEW) {
I think that check for session state should be done after locking the
session. Otherwise if two threads try to connect at the same time,
they'll connect twise. I know it is not likely now, but who knows what's
happen next?
> + virSSHError(VIR_ERR_LIBSSH_ERROR, "%s",
> + _("Invalid virNetLibSSHSessionPtr"));
> + return -1;
> + }
> +
> + virMutexLock(&sess->lock);
> +
> + /* check if configuration is valid */
> + if ((ret = virNetLibSSHValidateConfig(sess)) < 0)
> + goto error;
> +
> + /* open session */
> + ret = libssh2_session_handshake(sess->session, sock);
> + /* libssh2 is in blocking mode, so EAGAIN will never happen */
> + if (ret < 0) {
> + libssh2_session_last_error(sess->session, &errmsg, NULL, 0);
> + virSSHError(VIR_ERR_NO_CONNECT,
> + _("SSH session handshake failed: %s"),
> + errmsg);
> + goto error;
> + }
> +
> + /* verify the SSH host key */
> + if ((ret = virNetLibSSHCheckHostKey(sess)) != 0)
> + goto error;
> +
> + /* authenticate */
> + if ((ret = virNetLibSSHAuthenticate(sess)) != 0)
> + goto error;
> +
> + /* open channel */
> + if ((ret = virNetLibSSHOpenChannel(sess)) != 0)
> + goto error;
> +
> + /* all set */
> + /* switch to nonblocking mode and return */
> + libssh2_session_set_blocking(sess->session, 0);
> + sess->state = VIR_NET_LIBSSH_STATE_HANDSHAKE_COMPLETE;
> +
> + virMutexUnlock(&sess->lock);
> + return ret;
> +error:
> + sess->state = VIR_NET_LIBSSH_STATE_ERROR;
> + virMutexUnlock(&sess->lock);
> + return ret;
> +}
> +
> +/* do a read from a ssh channel, used instead of normal read on socket */
> +ssize_t
> +virNetLibSSHChannelRead(virNetLibSSHSessionPtr sess,
> + char *buf,
> + size_t len)
> +{
> + ssize_t ret = -1;
> + ssize_t read_n = 0;
> +
> + virMutexLock(&sess->lock);
> +
> + if (sess->state != VIR_NET_LIBSSH_STATE_HANDSHAKE_COMPLETE) {
> + if (sess->state == VIR_NET_LIBSSH_STATE_ERROR_REMOTE)
> + virSSHError(VIR_ERR_LIBSSH_REMOTE_COMMAND,
> + _("Remote program terminated with non-zero code: %d"),
> + sess->channelCommandReturnValue);
> + else
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("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);
> +
> + 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_LIBSSH_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_LIBSSH_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)) {
> + virSSHError(VIR_ERR_LIBSSH_REMOTE_COMMAND,
> + _("Remote program 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_LIBSSH_STATE_ERROR_REMOTE;
> + virMutexUnlock(&sess->lock);
> + return -1;
> + }
> +
> + sess->state = VIR_NET_LIBSSH_STATE_CLOSED;
> + virMutexUnlock(&sess->lock);
> + return -1;
> + }
> +
> +success:
> + virMutexUnlock(&sess->lock);
> + return read_n;
> +
> +error:
> + sess->state = VIR_NET_LIBSSH_STATE_ERROR;
> + virMutexUnlock(&sess->lock);
> + return ret;
> +}
> +
> +ssize_t
> +virNetLibSSHChannelWrite(virNetLibSSHSessionPtr sess,
> + const char *buf,
> + size_t len)
> +{
> + ssize_t ret;
> +
> + virMutexLock(&sess->lock);
> +
> + if (sess->state != VIR_NET_LIBSSH_STATE_HANDSHAKE_COMPLETE) {
> + if (sess->state == VIR_NET_LIBSSH_STATE_ERROR_REMOTE)
> + virSSHError(VIR_ERR_LIBSSH_REMOTE_COMMAND,
> + _("Remote program terminated with non-zero code: %d"),
> + sess->channelCommandReturnValue);
> + else
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("Tried to write socket in error state"));
> + ret = -1;
> + goto cleanup;
> + }
> +
> + if (libssh2_channel_eof(sess->channel)) {
> + if (libssh2_channel_get_exit_status(sess->channel)) {
> + virSSHError(VIR_ERR_LIBSSH_REMOTE_COMMAND,
> + _("Remote program terminated with non-zero code: %d"),
> + libssh2_channel_get_exit_status(sess->channel));
> + sess->state = VIR_NET_LIBSSH_STATE_ERROR_REMOTE;
> + sess->channelCommandReturnValue = libssh2_channel_get_exit_status(sess->channel);
> +
> + ret = -1;
> + goto cleanup;
> + }
> +
> + sess->state = VIR_NET_LIBSSH_STATE_CLOSED;
> + ret = -1;
> + goto cleanup;
> + }
> +
> + ret = libssh2_channel_write(sess->channel, buf, len);
> + if (ret == LIBSSH2_ERROR_EAGAIN) {
> + ret = 0;
> + goto cleanup;
> + }
> +
> + if (ret < 0) {
> + char *msg;
> + sess->state = VIR_NET_LIBSSH_STATE_ERROR;
> + libssh2_session_last_error(sess->session, &msg, NULL, 0);
> + virSSHError(VIR_ERR_LIBSSH_ERROR,
> + _("write failed: %s"), msg);
> + }
> +
> +cleanup:
> + virMutexUnlock(&sess->lock);
> + return ret;
> +}
> +
> +bool virNetLibSSHHasCachedData(virNetLibSSHSessionPtr sess)
> +{
> + bool ret;
> +
> + if (!sess)
> + return false;
> +
> + virMutexLock(&sess->lock);
> +
> + ret = sess->bufUsed > 0;
> +
> + virMutexUnlock(&sess->lock);
> + return ret;
> +}
> diff --git a/src/rpc/virnetlibsshcontext.h b/src/rpc/virnetlibsshcontext.h
> new file mode 100644
> index 0000000..98a022c
> --- /dev/null
> +++ b/src/rpc/virnetlibsshcontext.h
> @@ -0,0 +1,76 @@
> +/*
> + * virnetlibsshcontext.h: libssh transport provider
> + *
> + * Copyright (C) 2011 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Author: Peter Krempa <pkrempa at redhat.com>
> + */
> +#ifndef __VIR_NET_LIBSSH_CONTEXT_H__
> +# define __VIR_NET_LIBSSH_CONTEXT_H__
> +
> +# include "internal.h"
> +
> +typedef struct _virNetLibSSHSession virNetLibSSHSession;
> +typedef virNetLibSSHSession *virNetLibSSHSessionPtr;
> +
> +virNetLibSSHSessionPtr virNetLibSSHSessionNew(void);
> +void virNetLibSSHSessionFree(virNetLibSSHSessionPtr sess);
> +
> +typedef enum {
> + VIR_NET_LIBSSH_HOSTKEY_VERIFY_NORMAL,
> + VIR_NET_LIBSSH_HOSTKEY_VERIFY_AUTO_ADD,
> + VIR_NET_LIBSSH_HOSTKEY_VERIFY_IGNORE
> +} virNetLibSSHHostkeyVerify;
> +
> +int virNetLibSSHSessionSetAuthCallback(virNetLibSSHSessionPtr sess,
> + virConnectAuthPtr auth);
> +
> +int virNetLibSSHSessionSetChannelCommand(virNetLibSSHSessionPtr sess,
> + const char *command);
> +
> +int virNetLibSSHSessionSetCredentials(virNetLibSSHSessionPtr sess,
> + const char *username,
> + const char *password);
> +
> +int virNetLibSSHSessionSetPrivateKey(virNetLibSSHSessionPtr sess,
> + const char *keyfile);
> +
> +int virNetLibSSHSessionSetHostKeyVerification(virNetLibSSHSessionPtr sess,
> + const char *hostname,
> + const int port,
> + const char *hostsfile,
> + bool readonly,
> + virNetLibSSHHostkeyVerify opt);
> +
> +int virNetLibSSHSessionConnect(virNetLibSSHSessionPtr sess,
> + int sock);
> +
> +ssize_t
> +virNetLibSSHChannelRead(virNetLibSSHSessionPtr sess,
> + char *buf,
> + size_t len);
> +
> +ssize_t
> +virNetLibSSHChannelWrite(virNetLibSSHSessionPtr sess,
> + const char *buf,
> + size_t len);
> +
> +bool
> +virNetLibSSHHasCachedData(virNetLibSSHSessionPtr sess);
> +
> +
> +#endif /* __VIR_NET_LIBSSH_CONTEXT_H__ */
> diff --git a/src/util/virterror.c b/src/util/virterror.c
> index 380dc56..4ed404f 100644
> --- a/src/util/virterror.c
> +++ b/src/util/virterror.c
> @@ -178,6 +178,9 @@ static const char *virErrorDomainName(virErrorDomain domain) {
> case VIR_FROM_CAPABILITIES:
> dom = "Capabilities ";
> break;
> + case VIR_FROM_LIBSSH:
> + dom = "LibSSH2 ";
> + break;
> }
> return(dom);
> }
> @@ -1219,6 +1222,17 @@ virErrorMsg(virErrorNumber error, const char *info)
> else
> errmsg = _("operation aborted: %s");
> break;
> + case VIR_ERR_LIBSSH_REMOTE_COMMAND:
> + if (info == NULL)
> + errmsg = _("remote command terminated abnormally");
> + else
> + errmsg = _("remote command terminated abnormally: %s");
> + break;
> + case VIR_ERR_LIBSSH_ERROR:
> + if (info == NULL)
> + errmsg = _("LibSSH2 internal error");
> + else
> + errmsg = _("LibSSH2 internal error: %s");
> }
> return (errmsg);
> }
One thing - I'd suggest to rewrite the code (esp. in
virnetlibsshcontext.c) and collapse error paths with goto; I mean this
structure:
func(args) {
int ret = -1;
virMutexLock();
if (operation() < 0) {
reportError();
goto cleanup;
}
...
ret = 0;
cleanup:
virMutexUnlock();
return ret;
}
instead of:
func(args) {
int ret = 0;
virMutexLock();
if (operation() < 0) {
reportError();
virMutexUnlock();
return -1;
}
...
virMutexUnlock();
return ret;
}
In some part of the code you stick perfectly to the latter style, but
I'd gladly see this on the rest as well, e.g.
virNetLibSSHCheckHostKey(), virNetLibSSHAuthenticate().
Overall, some memleaks are hard to spot as libssh documentation tries to
avoid telling users if caller should or should not free() returned
value. And I am not into diggin over libssh code :P
But the feeling after reading this is good.
More information about the libvir-list
mailing list