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

[ashish mittal]

On Mon, Feb 27, 2017 at 1:22 AM, Daniel P. Berrange <address@hidden> wrote:
> On Fri, Feb 24, 2017 at 03:30:21PM -0800, ashish mittal wrote:
>> Thanks!
>>
>> I hope the following is in line with what you suggested -
>
> Yes, that looks suitable for password auth
>
Thanks!

>>
>> 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);
>
> Not sure why you're deleting this ? Likewise the 2 cases below too
>

Picked it up from qemu_gluster_parse_json(). I will get rid of them in
the final version.

>> +        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,
> 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/ :|

The next thing we need consensus on, is to decide exactly what
additional information to pass.

(1) Current security implementation in VxHS uses the SSL key and
certificate files. Do you think we can achieve all the intended
security goals if we pass these two files paths (and names?) from the
command line?

(2) If we are OK to continue with the present security scheme (using
key and cert), then the only additional change needed might be to
accept these files on the command line.

(3) If we decide to rely on file permissions and SELinux policy to
protect these key/cert files, then we don't need to pass the file
names as a secret, instead, passing them as regular qemu_opt_get()
options might be enough.

Let us know what you think about the above?

Thanks,
Ashish