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"}'
> 17396@1487908905.546890:vxhs_open_vdiskid Opening vdisk-id /test.raw
> 17396@1487908905.546969:vxhs_get_creds SecretID ashish, Password
> asd888d989s <==== NOTE!!!!!
> 17396@1487908905.546994:vxhs_open_hostinfo Adding host 127.0.0.1:9999
> to BDRVVXHSState
> 17396@1487908905.568370: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)
> 17396@1487908905.570917:vxhs_close Closing vdisk /test.raw
> [root@rhel72ga-build-upstream qemu] 2017-02-23 20:01:45$
>
> Example where username and password are missing:
>
> [root@rhel72ga-build-upstream 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"}'
> 16120@1487907025.251167:vxhs_open_vdiskid Opening vdisk-id /test.raw
> 16120@1487907025.251245: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!!!!!
> [root@rhel72ga-build-upstream 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/ :|
