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

[ashish mittal]

Thanks!

I hope the following is in line with what you suggested -

We will error out in case either of username, secret-id, or password
are missing.

Good case, passing password via a file -
$ ./qemu-io --trace enable=vxhs* --object
secret,id=xvxhspasswd,file=/tmp/some/file/path  -c 'read 66000 128k'
'json:{"server.host": "127.0.0.1", "server.port": "9999", "vdisk-id":
"/test.raw", "driver": "vxhs", "user": "ashish",  "password-secret":
"xvxhspasswd"}'
address@hidden:vxhs_open_vdiskid Opening vdisk-id /test.raw

address@hidden:vxhs_get_creds User ashish, SecretID
xvxhspasswd, Password address@hidden   <=== ****  NOTE WILL NOT PRINT
PASSWORD IN FINAL CODE ****

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.0012 sec (99.049 MiB/sec and 792.3930 ops/sec)
address@hidden:vxhs_close Closing vdisk /test.raw


Bad case, missing user -
$ ./qemu-io --trace enable=vxhs* --object
secret,id=xvxhspasswd,data=/tmp/some/file/path  -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
can't open device json:{"server.host": "127.0.0.1", "server.port":
"9999", "vdisk-id": "/test.raw", "driver": "vxhs"}: please specify the
user for authenticating to target

diff --git a/block/vxhs.c b/block/vxhs.c
index 4f0633e..9b60ddf 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -17,12 +17,16 @@
 #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"
 #define VXHS_OPT_SERVER             "server"
 #define VXHS_OPT_HOST               "host"
 #define VXHS_OPT_PORT               "port"
+#define VXHS_OPT_USER               "user"
+#define VXHS_OPT_PASSWORD           "password"
+#define VXHS_OPT_SECRETID           "password-secret"
 #define VXHS_UUID_DEF "12345678-1234-1234-1234-123456789012"

 QemuUUID qemu_uuid __attribute__ ((weak));
@@ -136,6 +140,22 @@ static QemuOptsList runtime_opts = {
             .type = QEMU_OPT_STRING,
             .help = "UUID of the VxHS vdisk",
         },
+        {
+            .name = VXHS_OPT_USER,
+            .type = QEMU_OPT_STRING,
+            .help = "username for authentication to target",
+        },
+        {
+            .name = VXHS_OPT_PASSWORD,
+            .type = QEMU_OPT_STRING,
+            .help = "password for authentication to target",
+        },
+        {
+            .name = VXHS_OPT_SECRETID,
+            .type = QEMU_OPT_STRING,
+            .help = "ID of the secret providing password for"
+                    "authentication to target",
+        },
         { /* end of list */ }
     },
 };
@@ -257,6 +277,9 @@ 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 *secretid = NULL;
+    const char *password = NULL;

     ret = vxhs_init_and_ref();
     if (ret < 0) {
@@ -320,6 +343,35 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
         goto out;
     }

+    /* check if we got username and secretid via the options */
+    user = qemu_opt_get(opts, VXHS_OPT_USER);
+    if (!user) {
+        error_setg(&local_err, "please specify the user for authenticating to "
+                   "target");
+        qdict_del(backing_options, str);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID);
+    if (!secretid) {
+        error_setg(&local_err, "please specify the ID of the secret to be "
+                   "used for authenticating to target");
+        qdict_del(backing_options, str);
+        ret = -EINVAL;
+        goto out;
+    }
+
+    /* check if we got password via the --object argument */
+    password = qcrypto_secret_lookup_as_utf8(secretid, &local_err);
+    if (local_err != NULL) {
+        trace_vxhs_get_creds(user, secretid, password);
+        qdict_del(backing_options, str);
+        ret = -EINVAL;
+        goto out;
+    }
+    trace_vxhs_get_creds(user, secretid, password);
+
     s->vdisk_hostinfo.host = g_strdup(server_host_opt);

     s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,

Regards,
Ashish


On Fri, Feb 24, 2017 at 1:19 AM, Daniel P. Berrange <address@hidden> wrote:
> 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/ :|