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

[ashish mittal]

Thanks! There is one more input I need some help with!

VxHS network library opens a fixed number of connection channels to a
given host, and all the vdisks (that connect to the same host) share
these connection channels.

Therefore, we need to open secure channels to a specific target host
only once for the first vdisk that connects to that host. All the
other vdisks that connect to the same target host will share the same
set of secure communication channels.

I hope the above scheme is acceptable?

If yes, then we have a couple of options to implement this:

(1) Accept the TLS credentials per vdisk using the previously
discussed --object tls-creds-x509 mechanism. In this case, if more
than one vdisk have the same host info, then we will use only the
first one's creds to set up the secure connection, and ignore the
others. vdisks that connect to different target hosts will use their
individual tls-creds-x509 to set up the secure channels. This is, of
course, not relevant for qemu-img/qemu-io as they can open only one
vdisk at a time.

(2) Instead of having a per-vdisk --object tls-creds-x509, have a
single such argument on the command line for vxhs block device code to
consume - if that is possible! One way to achieve this could be the
user/password authentication we discussed earlier, which we could use
to pass the directory where cert/keys are kept.

(3) Use the instance UUID, when available, to lookup the cert files
per instance (i.e. for qemu-kvm), and use the --object tls-creds-x509
mechanism, when instance UUID is NULL (i.e. qemu-io, qemu-img etc).
The cert/key files are anyway protected by file permissions in either
case, so I guess there is no additional security provided by either
method.

Regards,
Ashish

On Mon, Mar 6, 2017 at 1:23 AM, Daniel P. Berrange <address@hidden> wrote:
> On Sun, Mar 05, 2017 at 04:26:05PM -0800, ashish mittal wrote:
>> On Wed, Mar 1, 2017 at 1:18 AM, Daniel P. Berrange <address@hidden> wrote:
>> >
>> > Yes, that's how other parts of QEMU deal with SSL
>> >
>> > NB, QEMU needs to pass 3 paths to libqnoio - the client cert, the
>> > client key, and the certificate authority certlist
>> >
>>
>> I see that the QEMU TLS code internally does expect to find cacert,
>> and errors out if this is missing. I did have to create one and place
>> it in the dir path where we are keeping the client key,cert files. The
>> code diff below requires all the three files.
>
> Yes, the client cert/key have to be paired with the correct CA cert. We cannot
> assume that the default CA cert bundle contains the right CA, for use with the
> cert/key. The default CA bundle contains all the public commercial CAs, and in
> general a QEMU deployment will never use any of those - the site will use a
> private internal only CA. While you could add the private CA to the global CA
> bundle, that is not desirable from a security POV, as it opens the deployment
> to risk from a rogue CA.
>
>> > The include/crypto/tlscredsx509.h file has constants defined for the
>> > standard filenames - eg you would concatenate the directory with
>> > the constants QCRYPTO_TLS_CREDS_X509_CLIENT_KEY.
>> >
>> > This gives the filenames you can then pass to libqnio
>> >
>>
>> I am using function qcrypto_tls_creds_get_path() to achieve the same.
>> Hope this is OK.
>
> Yep, that's fine.
>
>> Example CLI accepting the new TLS credentials:
>>
>> address@hidden qemu] 2017-03-05 15:54:55$ ./qemu-io --trace
>> enable=vxhs* --object
>> tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client -c 'read
>> 66000 128k' 'json:{"server.host": "127.0.0.1", "server.port": "9999",
>> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'
>> address@hidden:vxhs_open_vdiskid Opening vdisk-id /test.raw
>> address@hidden:vxhs_get_creds cacert
>> /etc/pki/qemu/vxhs/ca-cert.pem, client_key
>> /etc/pki/qemu/vxhs/client-key.pem, client_cert
>> /etc/pki/qemu/vxhs/client-cert.pem   <===== !!!! 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 1048576
>> read 131072/131072 bytes at offset 66000
>> 128 KiB, 1 ops; 0.0006 sec (188.537 MiB/sec and 1508.2956 ops/sec)
>> address@hidden:vxhs_close Closing vdisk /test.raw
>> address@hidden qemu] 2017-03-05 15:55:01$
>
> That looks ok.
>
>> @@ -315,33 +374,39 @@ static int vxhs_open(BlockDriverState *bs, QDict 
>> *options,
>>      if (strlen(server_host_opt) > MAXHOSTNAMELEN) {
>>          error_setg(&local_err, "server.host cannot be more than %d 
>> characters",
>>                     MAXHOSTNAMELEN);
>> -        qdict_del(backing_options, str);
>>          ret = -EINVAL;
>>          goto out;
>>      }
>>
>> -    s->vdisk_hostinfo.host = g_strdup(server_host_opt);
>> +    /* check if we got tls-creds via the --object argument */
>> +    s->tlscredsid = g_strdup(qemu_opt_get(opts, "tls-creds"));
>> +    if (s->tlscredsid) {
>> +        vxhs_get_tls_creds(s->tlscredsid, &cacert, &client_key,
>> +                           &client_cert, &local_err);
>> +        if (local_err != NULL) {
>> +            ret = -EINVAL;
>> +            goto out;
>> +        }
>> +        trace_vxhs_get_creds(cacert, client_key, client_cert);
>> +    }
>>
>> +    s->vdisk_hostinfo.host = g_strdup(server_host_opt);
>>      s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,
>>                                                            VXHS_OPT_PORT),
>>                                                            NULL, 0);
>>
>>      trace_vxhs_open_hostinfo(s->vdisk_hostinfo.host,
>> -                         s->vdisk_hostinfo.port);
>> -
>> -    /* free the 'server.' entries allocated by previous call to
>> -     * qdict_extract_subqdict()
>> -     */
>> -    qdict_del(backing_options, str);
>> +                             s->vdisk_hostinfo.port);
>>
>>      of_vsa_addr = g_strdup_printf("of://%s:%d",
>> -                                s->vdisk_hostinfo.host,
>> -                                s->vdisk_hostinfo.port);
>> +                                  s->vdisk_hostinfo.host,
>> +                                  s->vdisk_hostinfo.port);
>>
>>      /*
>>       * Open qnio channel to storage agent if not opened before.
>>       */
>> -    dev_handlep = iio_open(of_vsa_addr, s->vdisk_guid, 0);
>> +    dev_handlep = iio_open(of_vsa_addr, s->vdisk_guid, 0,
>> +                           client_key, client_cert);
>
> You need to pass  ca_cert into this method too.
>
>
> 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/ :|