Re: [Qemu-devel] [PATCH v4] block: Add support for Secure Shell (ssh) block device.

[Stefan Hajnoczi]

On Wed, Mar 27, 2013 at 03:57:29PM +0000, Richard W.M. Jones wrote:
> From: "Richard W.M. Jones" <address@hidden>
> 
>   qemu-system-x86_64 -drive file=ssh://hostname/some/image
> 
> QEMU will ssh into 'hostname' and open '/some/image' which is made
> available as a standard block device.
> 
> You can specify a username (ssh://address@hidden/...) and/or a port number
> (ssh://host:port/...).  You can also use an alternate syntax using
> properties (file.user, file.host, file.port, file.path).
> 
> Current limitations:
> 
> - Authentication must be done without passwords or passphrases, using
>   ssh-agent.  Other authentication methods are not supported.
> 
> - Uses a single connection, instead of concurrent AIO with multiple
>   SSH connections.
> 
> This is implemented using libssh2 on the client side.  The server just
> requires a regular ssh daemon with sftp-server support.  Most ssh
> daemons on Unix/Linux systems will work out of the box.
> 
> Signed-off-by: Richard W.M. Jones <address@hidden>
> Cc: Stefan Hajnoczi <address@hidden>
> Cc: Kevin Wolf <address@hidden>
> ---
>  block/Makefile.objs |   1 +
>  block/ssh.c         | 878 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  configure           |  47 +++
>  qemu-doc.texi       |  40 +++
>  qemu-options.hx     |  12 +
>  5 files changed, 978 insertions(+)
>  create mode 100644 block/ssh.c

Please run qemu-iotests, see tests/qemu-iotests/check.  For example,
with NBD:

  $ cd tests/qemu-iotests
  $ QEMU_PROG=$HOME/qemu/x86_64-softmmu/qemu-system-x86_64 
PATH=$HOME/qemu:$PATH \
    ./check -nbd

A patch will be required to add -ssh support to ./check.

> +/* Wrappers around error_report which make sure to dump as much
> + * information from libssh2 as possible.
> + */
> +static void
> +session_error_report(BDRVSSHState *s, const char *fs, ...)
> +{
> +    va_list args;
> +
> +    va_start(args, fs);
> +
> +    if ((s)->session) {
> +        char *ssh_err;
> +        int ssh_err_len = sizeof ssh_err;

This variable is unused and initialization to sizeof ssh_err seems
wrong.  Please drop it and pass NULL to libssh2_session_last_error(3).

> +        int ssh_err_code;
> +
> +        libssh2_session_last_error((s)->session,
> +                                   &ssh_err, &ssh_err_len, 0);
> +        /* This is not an errno.  See <libssh2.h>. */
> +        ssh_err_code = libssh2_session_last_errno((s)->session);
> +
> +        error_vprintf(fs, args);
> +        error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
> +    }
> +    else {

QEMU tends to use } else { on a single line.

> +        error_vprintf(fs, args);
> +    }

In fact this else statement can be avoided:

error_vprintf(fs, args);
if (s->session) {
    ...
    error_printf(": %s (libssh2 error code: %d)", ssh_err, ssh_err_code);
}

> +
> +    va_end(args);
> +    error_printf("\n");
> +}
> +
> +static void
> +sftp_error_report(BDRVSSHState *s, const char *fs, ...)
> +{
> +    va_list args;
> +
> +    va_start(args, fs);
> +
> +    if ((s)->sftp) {
> +        char *ssh_err;
> +        int ssh_err_len = sizeof ssh_err;
> +        int ssh_err_code;
> +        unsigned long sftp_err_code;
> +
> +        libssh2_session_last_error((s)->session,
> +                                   &ssh_err, &ssh_err_len, 0);
> +        /* This is not an errno.  See <libssh2.h>. */
> +        ssh_err_code = libssh2_session_last_errno((s)->session);
> +        /* See <libssh2_sftp.h>. */
> +        sftp_err_code = libssh2_sftp_last_error((s)->sftp);
> +
> +        error_vprintf(fs, args);
> +        error_printf(": %s (libssh2 error code: %d, sftp error code: %lu)",
> +                     ssh_err, ssh_err_code, sftp_err_code);
> +    }
> +    else {
> +        error_vprintf(fs, args);
> +    }

Same comments apply to sftp_error_report().

> +
> +    va_end(args);
> +    error_printf("\n");
> +}
> +
> +#ifndef _WIN32
> +static const char *get_username(void)
> +{
> +    struct passwd *pwd;
> +
> +    pwd = getpwuid(geteuid ());
> +    if (!pwd) {
> +        error_report("failed to get current user name");
> +        return NULL;
> +    }
> +    return pwd->pw_name;
> +}
> +#else
> +static const char *get_username(void)
> +{
> +    error_report("on Windows, specify a remote user name in the URI");
> +    return NULL;
> +}
> +#endif

This is ugly since it's platform-specific and uses getpwuid(3) which is
not reentrant.

Does ssh(1) even use getpwuid(geteuid()) or does it check .ssh/config
and then getenv("USER")?  Perhaps we can just getenv("USER")?

> +
> +static int parse_uri(const char *filename, QDict *options)

Use Error **errp argument instead of error_report()?

> +{
> +    URI *uri;
> +    char *host = NULL;

Unused variable.

> +static int check_host_key(BDRVSSHState *s, const char *host, int port)
> +{
> +    int ret;
> +    const char *home;
> +    char *knh_file = NULL;
> +    LIBSSH2_KNOWNHOSTS *knh = NULL;
> +    const char *fingerprint;
> +    size_t len;
> +    int type;
> +
> +    knh = libssh2_knownhost_init(s->session);
> +    if (!knh) {
> +        ret = -EINVAL;
> +        session_error_report(s, "failed to initialize known hosts support");
> +        goto out;
> +    }
> +
> +    home = getenv("HOME");
> +    if (home) {
> +        knh_file = g_strdup_printf("%s/.ssh/known_hosts", home);
> +    } else {
> +        knh_file = g_strdup_printf("/root/.ssh/known_hosts");
> +    }

Windows support?

> +
> +    /* Read all known hosts from OpenSSH-style known_hosts file. */
> +    libssh2_knownhost_readfile(knh, knh_file, 
> LIBSSH2_KNOWNHOST_FILE_OPENSSH);
> +
> +    fingerprint = libssh2_session_hostkey(s->session, &len, &type);
> +    if (fingerprint) {
> +        struct libssh2_knownhost *found;
> +        int r;
> +
> +        r = libssh2_knownhost_checkp(knh, host, port, fingerprint, len,
> +                                     LIBSSH2_KNOWNHOST_TYPE_PLAIN|
> +                                     LIBSSH2_KNOWNHOST_KEYENC_RAW,
> +                                     &found);
> +        switch (r) {
> +        case LIBSSH2_KNOWNHOST_CHECK_MATCH:
> +            /* OK */
> +            DPRINTF("host key OK: %s", found->key);
> +            break;
> +        case LIBSSH2_KNOWNHOST_CHECK_MISMATCH:
> +            ret = -EINVAL;
> +            session_error_report(s, "host key does not match the one in 
> known_hosts (found key %s)",
> +                                 found->key);

Does the user know the offending known_hosts line?  ssh(1) normally says
something like "Mismatch with line ~/.ssh/known_hosts:35" so you know
which hostkey to drop if you wish to proceed.

> +            goto out;
> +        case LIBSSH2_KNOWNHOST_CHECK_NOTFOUND:
> +            ret = -EINVAL;
> +            session_error_report(s, "no host key was found in known_hosts");
> +            goto out;

ssh(1) prompts the user to accept the hostkey.  When QEMU fails like
this the user needs to run ssh(1) first to populate known_hosts?

> @@ -1166,6 +1171,7 @@ echo "  --disable-glusterfs      disable GlusterFS 
> backend"
>  echo "  --enable-gcov            enable test coverage analysis with gcov"
>  echo "  --gcov=GCOV              use specified gcov [$gcov_tool]"
>  echo "  --enable-tpm             enable TPM support"
> +echo "  --enable-libssh2         enable ssh block device support"

Most options seem to also document --disable-FOO.  Please do that.