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

[Daniel P. Berrange]

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