[libvirt] [PATCH 2/5] libssh2_transport: add main libssh2 transport implementation

[Eric Blake]

On 08/03/2012 08:03 AM, Peter Krempa wrote:
> This patch adds helper functions to libssh2 that enable us to use
> libssh2 in conjunction with libvirt-native virNetSockets instead of
> using a spawned "ssh" client process.
> 
> This implemetation supports tunneled plaintext, keyboard-interactive,

s/implemtation/implementation/

> private key, ssh agent based and null authentication. Libvirt's Auth
> callback is used for interaction with the user. (Keyboard interactive
> 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 "known_hosts" 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.
> 
> As a bonus, this should (untested) add SSH support to libvirt clients
> running on Windows.

>  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])

s/wether/whether/

> +++ b/src/Makefile.am
> @@ -1508,6 +1508,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

Doing it this way requires a separate .syms file.  How hard would it be
to provide stub symbols, so that we always compile and link against the
file, but the file is a no-op when HAVE_LIBSSH2 is not available?

> +++ b/src/rpc/virnetlibsshcontext.c

> +/* 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)
> +{

prompts is marked 'const', but...

> +    /* 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';

...you are modifying it in-place by using strrchr to cast away const.
Is that an issue where you could cause a SEGV, and should be strdup'ing
the prompt before modifying it?


> +/* 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)

Is the return always negative on error?

> +            /* 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, "%02X", (unsigned char) keyhash[i]);

Is the cast there to avoid unintentional sign-extension because
keyhash[i] might be signed?  If so, you could avoid the cast by telling
asprintf that you are passing 1 byte in the first place with the 'hh'
modifier.

> +                if (i != 15)
> +                    virBufferAddChar(&buff, ':');
> +            }

Slightly more efficient to use:

for (i = 0; i < 16; i++)
    virBufferAsprintf(&buff, "%02hhX:", keyhash[i]);
virBufferTrim(&buff, ":", 1);

> +
> +            if (STRNEQ_NULLABLE(askKey.result, _("yes"))) {
> +                virReportError(VIR_ERR_LIBSSH_ERROR,
> +                               _("SSH host key for '%s' (%s) was not accepted"),

When parsing user input, don't you want a case-insensitive comparison?
Also, what about users that want to type 'y' instead of 'yes'?  And is
comparing to a translated string wise?  Without consulting LC_MESSAGES
for the proper regex for the user's chosen locale, I'd almost feel safer
with a check hard-coded to English 'y' and 'n' rather than to the
arbitrary language _("yes").

I'm not quite sure how I would test all of the code, but the bulk of it
looked sane by just glancing over it.  Having not specifically coded
with libssh2, I can't say if you were using the library API correctly
without spending a lot longer on the review; but if it is possible to
easily test the results, that would go a long way to convince me that
the code itself is doing the right thing.

-- 
Eric Blake   eblake at redhat.com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120806/61410f35/attachment-0001.sig>