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@xxxxxxxxxx +1-919-301-3266
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list