Re: [Qemu-devel] [PATCH v8 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[Daniel P. Berrange]

On Thu, Feb 23, 2017 at 08:19:29PM -0800, ashish mittal wrote:
> Hi,
> 
> Just want to check if the following mechanism for accepting the secret
> password looks OK?
> 
> We have yet to internally discuss the semantics of how we plan to use
> the user-ID/password for authentication. This diff is just to
> understand how I am expected to accept the secret from the command
> line.
> 
> Example specifying the username and password:
> 
> $ ./qemu-io --trace enable=vxhs* --object
> secret,id=ashish,data=asd888d989s  -c 'read 66000 128k'
> 'json:{"server.host": "127.0.0.1", "server.port": "9999", "vdisk-id":
> "/test.raw", "driver": "vxhs", "user": "ashish"}'
> address@hidden:vxhs_open_vdiskid Opening vdisk-id /test.raw
> address@hidden:vxhs_get_creds SecretID ashish, Password
> asd888d989s   <==== NOTE!!!!!
> address@hidden:vxhs_open_hostinfo Adding host 127.0.0.1:9999
> to BDRVVXHSState
> address@hidden:vxhs_get_vdisk_stat vDisk /test.raw stat ioctl
> returned size 196616
> read 131072/131072 bytes at offset 66000
> 128 KiB, 1 ops; 0.0017 sec (72.844 MiB/sec and 582.7506 ops/sec)
> address@hidden:vxhs_close Closing vdisk /test.raw
> address@hidden qemu] 2017-02-23 20:01:45$
> 
> Example where username and password are missing:
> 
> address@hidden qemu] 2017-02-23 19:30:16$ ./qemu-io
> --trace enable=vxhs* -c 'read 66000 128k' 'json:{"server.host":
> "127.0.0.1", "server.port": "9999", "vdisk-id": "/test.raw", "driver":
> "vxhs"}'
> address@hidden:vxhs_open_vdiskid Opening vdisk-id /test.raw
> address@hidden:vxhs_get_creds SecretID user, Password (null)
> can't open device json:{"server.host": "127.0.0.1", "server.port":
> "9999", "vdisk-id": "/test.raw", "driver": "vxhs"}: No secret with id
> 'user'      <==== NOTE!!!!!
> address@hidden qemu] 2017-02-23 19:30:25$

It is close but not quite correct approach. You're overloading a
single property to provide two different things - the username
and as a key to lookup the password secret - you want different
properties.


The 'secret' object only needs to be used for data that must be
kept private. By convention the block drivers try to use a property
'password-secret' to reference the ID of the secret object.

So, as an example, if you had a username "fred" and passwd "letmein"
then a suitable syntax would be

 $ ./qemu-io \
    --object secret,id=vxhspasswd,data=letmein
    -c 'read 66000 128k'
    'json:{"server.host": "127.0.0.1", "server.port": "9999",
           "vdisk-id": "/test.raw", "driver": "vxhs",
           "user": "fred", "password-secret": "xvxhspasswd"}'

(obviously in real world, we'd not send across the password
in clear text in the data= parameter of the secret - we'd
use the file= parameter instead, but its fine for dev testing.

> diff --git a/block/vxhs.c b/block/vxhs.c
> index 4f0633e..f3e70ce 100644
> --- a/block/vxhs.c
> +++ b/block/vxhs.c
> @@ -17,6 +17,7 @@
>  #include "qemu/uri.h"
>  #include "qapi/error.h"
>  #include "qemu/uuid.h"
> +#include "crypto/secret.h"
> 
>  #define VXHS_OPT_FILENAME           "filename"
>  #define VXHS_OPT_VDISK_ID           "vdisk-id"
> @@ -136,6 +137,14 @@ static QemuOptsList runtime_opts = {
>              .type = QEMU_OPT_STRING,
>              .help = "UUID of the VxHS vdisk",
>          },
> +        {
> +            .name = "user",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        {
> +            .name = "password",
> +            .type = QEMU_OPT_STRING,
> +        },
>          { /* end of list */ }
>      },
>  };
> @@ -257,6 +266,8 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
>      const char *server_host_opt;
>      char *str = NULL;
>      int ret = 0;
> +    const char *user = NULL;
> +    const char *password = NULL;
> 
>      ret = vxhs_init_and_ref();
>      if (ret < 0) {
> @@ -320,6 +331,23 @@ static int vxhs_open(BlockDriverState *bs, QDict 
> *options,
>          goto out;
>      }
> 
> +    /* check if we got username via the options */
> +    user = qemu_opt_get(opts, "user");
> +    if (!user) {
> +        //trace_vxhs_get_creds(user, password);
> +        user = "user";

We should not default any login credentials - if no username was
provided we should simply not use any username - if the server
mandates a username this obviously means connection would fail
and that's ok.

> +        //return;
> +    }
> +
> +    password = qcrypto_secret_lookup_as_utf8(user, &local_err);

Instead of passing 'user' into this method, you need to call
qemu_opt_get(opts, "password-secret") and use that result

> +    if (local_err != NULL) {
> +        trace_vxhs_get_creds(user, password);
> +        qdict_del(backing_options, str);
> +        ret = -EINVAL;
> +        goto out;
> +    }
> +    trace_vxhs_get_creds(user, password);
> +

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|