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

[ashish mittal]

On Wed, Mar 22, 2017 at 5:03 PM, ashish mittal  wrote:
> On Mon, Mar 20, 2017 at 5:55 AM, Daniel P. Berrange  
> wrote:
>> On Fri, Mar 17, 2017 at 06:44:56PM -0700, ashish mittal wrote:
>>> On Thu, Mar 16, 2017 at 5:29 PM, ashish mittal  wrote:
>>> > On Mon, Mar 13, 2017 at 2:57 AM, Daniel P. Berrange  
>>> > wrote:
>>> >> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
>>> >>> 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.
>>> >>
>>> >> It looks like option 1 here is the way to go. Just report an error if
>>> >> there are multiple creds provided for the same host and they don't
>>> >> match.
>>> >>
>>> >
>>> > I have made changes to implement option 1 in the library (branch
>>> > per_channel_ssl).
>>> > Can you please help review it?
>>> > https://github.com/VeritasHyperScale/libqnio/compare/per_channel_ssl
>>> >
>>> > Here's the changelog:
>>> > (1) Changed code to not use instance UUID for setting up SSL context.
>>> > (2) User is supposed to pass the cacert, client_key and client_cert
>>> > files to iio_open(). These will be used to set up a per-channel 
>>> > secure SSL
>>> > connection to the server. All three values are needed to set up a
>>> > secure connection.
>>> > (3) If the secure channel to a particular host is already open, other
>>> > block device connections to the same host will have to provide
>>> > TLS/SSL credentials that match the original one.
>>> > (4) Set default locations for trusted client CA certificates
>>> >  based on user specified cacert file.
>>> >
>>> > NB - I have given steps to test SSL communication (using the supplied
>>> > test client/server programs) in the commit log. I have not tested
>>> > using qemu binary yet. Will run more tests in the days to come.
>>> >
>>> > qemu vxhs patch changes should be pretty straightforward, given that
>>> > we have already discussed how to implement passing --object
>>> > tls-creds-x509 per block device.
>>> >
>>> > Thanks!
>>> >
>>>
>>> Update -
>>> (1) Successfully tested SSL communication using qemu-io and the test server.
>>> (2) Minor changes to per_channel_ssl branch.
>>> (3) Created a pull request.
>>>
>>> Please review per convenience. Thanks!
>>
>> IIUC, on that branch the 'is_secure()' method is still looking for the
>> directory /var/lib/libvxhs/secure to exist on the host. If that is not
>> present, then it appears to be silently ignoring the SSL certs passed
>> in from QEMU.
>>
>> IMHO it should enable TLS when 'cacert' passed to iio_open is not NULL,
>> not relying on a magic directory to exist.
>>
>
> I have made changes per above to the library. Please see commits -
>
> https://github.com/VeritasHyperScale/libqnio/commit/6c3261e9c9bb1350f4433a1ae4fcd98f7692cf39
> https://github.com/VeritasHyperScale/libqnio/commit/502c74278457e6ac86a4ee4ad9102e56ff3be5d4
>
> Commit log: Enable secure mode based on the SSL/TLS args passed in iio_open()
>
> (1) Do not use /var/lib/libvxhs/secure to enable secure SSL mode on
> the client side.
> (2) Instead, enable SSL mode if the user passes TLS/SSL creds for
> the block device on the qemu CLI.
>
> Will be posting v10 qemu vxhs patch soon. v10 will work with the
> latest library changes, and will support passing tls-creds-x509 creds
> for every vxhs block device.
>

I have posted v10 patches today. I've had to make one change in
addition to what has been discussed. Basically I cannot use
qcrypto_tls_creds_get_path() because its definition is under a #ifdef
CONFIG_GNUTLS. Therefore my builds were failing whenever "gnutls" was
not configured. I've replaced it as below -


diff --git a/block/vxhs.c b/block/vxhs.c
index 0aa7849..44c0ecd 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -19,7 +19,6 @@
 #include 

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

[ashish mittal]

On Mon, Mar 20, 2017 at 5:55 AM, Daniel P. Berrange  wrote:
> On Fri, Mar 17, 2017 at 06:44:56PM -0700, ashish mittal wrote:
>> On Thu, Mar 16, 2017 at 5:29 PM, ashish mittal  wrote:
>> > On Mon, Mar 13, 2017 at 2:57 AM, Daniel P. Berrange  
>> > wrote:
>> >> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
>> >>> 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.
>> >>
>> >> It looks like option 1 here is the way to go. Just report an error if
>> >> there are multiple creds provided for the same host and they don't
>> >> match.
>> >>
>> >
>> > I have made changes to implement option 1 in the library (branch
>> > per_channel_ssl).
>> > Can you please help review it?
>> > https://github.com/VeritasHyperScale/libqnio/compare/per_channel_ssl
>> >
>> > Here's the changelog:
>> > (1) Changed code to not use instance UUID for setting up SSL context.
>> > (2) User is supposed to pass the cacert, client_key and client_cert
>> > files to iio_open(). These will be used to set up a per-channel secure 
>> > SSL
>> > connection to the server. All three values are needed to set up a
>> > secure connection.
>> > (3) If the secure channel to a particular host is already open, other
>> > block device connections to the same host will have to provide
>> > TLS/SSL credentials that match the original one.
>> > (4) Set default locations for trusted client CA certificates
>> >  based on user specified cacert file.
>> >
>> > NB - I have given steps to test SSL communication (using the supplied
>> > test client/server programs) in the commit log. I have not tested
>> > using qemu binary yet. Will run more tests in the days to come.
>> >
>> > qemu vxhs patch changes should be pretty straightforward, given that
>> > we have already discussed how to implement passing --object
>> > tls-creds-x509 per block device.
>> >
>> > Thanks!
>> >
>>
>> Update -
>> (1) Successfully tested SSL communication using qemu-io and the test server.
>> (2) Minor changes to per_channel_ssl branch.
>> (3) Created a pull request.
>>
>> Please review per convenience. Thanks!
>
> IIUC, on that branch the 'is_secure()' method is still looking for the
> directory /var/lib/libvxhs/secure to exist on the host. If that is not
> present, then it appears to be silently ignoring the SSL certs passed
> in from QEMU.
>
> IMHO it should enable TLS when 'cacert' passed to iio_open is not NULL,
> not relying on a magic directory to exist.
>

I have made changes per above to the library. Please see commits -

https://github.com/VeritasHyperScale/libqnio/commit/6c3261e9c9bb1350f4433a1ae4fcd98f7692cf39
https://github.com/VeritasHyperScale/libqnio/commit/502c74278457e6ac86a4ee4ad9102e56ff3be5d4

Commit log: Enable secure mode based on the SSL/TLS args passed in iio_open()

(1) Do not use /var/lib/libvxhs/secure to enable secure SSL mode on
the client side.
(2) Instead, enable SSL mode if the user passes TLS/SSL creds for
the block device on the qemu CLI.

Will be posting v10 qemu vxhs patch soon. v10 will work with the
latest library changes, and will support passing tls-creds-x509 creds
for every vxhs block device.


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

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

[Daniel P. Berrange]

On Fri, Mar 17, 2017 at 06:44:56PM -0700, ashish mittal wrote:
> On Thu, Mar 16, 2017 at 5:29 PM, ashish mittal  wrote:
> > On Mon, Mar 13, 2017 at 2:57 AM, Daniel P. Berrange  
> > wrote:
> >> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
> >>> 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.
> >>
> >> It looks like option 1 here is the way to go. Just report an error if
> >> there are multiple creds provided for the same host and they don't
> >> match.
> >>
> >
> > I have made changes to implement option 1 in the library (branch
> > per_channel_ssl).
> > Can you please help review it?
> > https://github.com/VeritasHyperScale/libqnio/compare/per_channel_ssl
> >
> > Here's the changelog:
> > (1) Changed code to not use instance UUID for setting up SSL context.
> > (2) User is supposed to pass the cacert, client_key and client_cert
> > files to iio_open(). These will be used to set up a per-channel secure 
> > SSL
> > connection to the server. All three values are needed to set up a
> > secure connection.
> > (3) If the secure channel to a particular host is already open, other
> > block device connections to the same host will have to provide
> > TLS/SSL credentials that match the original one.
> > (4) Set default locations for trusted client CA certificates
> >  based on user specified cacert file.
> >
> > NB - I have given steps to test SSL communication (using the supplied
> > test client/server programs) in the commit log. I have not tested
> > using qemu binary yet. Will run more tests in the days to come.
> >
> > qemu vxhs patch changes should be pretty straightforward, given that
> > we have already discussed how to implement passing --object
> > tls-creds-x509 per block device.
> >
> > Thanks!
> >
> 
> Update -
> (1) Successfully tested SSL communication using qemu-io and the test server.
> (2) Minor changes to per_channel_ssl branch.
> (3) Created a pull request.
> 
> Please review per convenience. Thanks!

IIUC, on that branch the 'is_secure()' method is still looking for the
directory /var/lib/libvxhs/secure to exist on the host. If that is not
present, then it appears to be silently ignoring the SSL certs passed
in from QEMU.

IMHO it should enable TLS when 'cacert' passed to iio_open is not NULL,
not relying on a magic directory to exist.

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

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

[ashish mittal]

On Thu, Mar 16, 2017 at 5:29 PM, ashish mittal  wrote:
> On Mon, Mar 13, 2017 at 2:57 AM, Daniel P. Berrange  
> wrote:
>> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
>>> 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.
>>
>> It looks like option 1 here is the way to go. Just report an error if
>> there are multiple creds provided for the same host and they don't
>> match.
>>
>
> I have made changes to implement option 1 in the library (branch
> per_channel_ssl).
> Can you please help review it?
> https://github.com/VeritasHyperScale/libqnio/compare/per_channel_ssl
>
> Here's the changelog:
> (1) Changed code to not use instance UUID for setting up SSL context.
> (2) User is supposed to pass the cacert, client_key and client_cert
> files to iio_open(). These will be used to set up a per-channel secure SSL
> connection to the server. All three values are needed to set up a
> secure connection.
> (3) If the secure channel to a particular host is already open, other
> block device connections to the same host will have to provide
> TLS/SSL credentials that match the original one.
> (4) Set default locations for trusted client CA certificates
>  based on user specified cacert file.
>
> NB - I have given steps to test SSL communication (using the supplied
> test client/server programs) in the commit log. I have not tested
> using qemu binary yet. Will run more tests in the days to come.
>
> qemu vxhs patch changes should be pretty straightforward, given that
> we have already discussed how to implement passing --object
> tls-creds-x509 per block device.
>
> Thanks!
>

Update -
(1) Successfully tested SSL communication using qemu-io and the test server.
(2) Minor changes to per_channel_ssl branch.
(3) Created a pull request.

Please review per convenience. Thanks!

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

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

[ashish mittal]

On Mon, Mar 13, 2017 at 2:57 AM, Daniel P. Berrange  wrote:
> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
>> 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.
>
> It looks like option 1 here is the way to go. Just report an error if
> there are multiple creds provided for the same host and they don't
> match.
>

I have made changes to implement option 1 in the library (branch
per_channel_ssl).
Can you please help review it?
https://github.com/VeritasHyperScale/libqnio/compare/per_channel_ssl

Here's the changelog:
(1) Changed code to not use instance UUID for setting up SSL context.
(2) User is supposed to pass the cacert, client_key and client_cert
files to iio_open(). These will be used to set up a per-channel secure SSL
connection to the server. All three values are needed to set up a
secure connection.
(3) If the secure channel to a particular host is already open, other
block device connections to the same host will have to provide
TLS/SSL credentials that match the original one.
(4) Set default locations for trusted client CA certificates
 based on user specified cacert file.

NB - I have given steps to test SSL communication (using the supplied
test client/server programs) in the commit log. I have not tested
using qemu binary yet. Will run more tests in the days to come.

qemu vxhs patch changes should be pretty straightforward, given that
we have already discussed how to implement passing --object
tls-creds-x509 per block device.

Thanks!

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

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

[Daniel P. Berrange]

On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
> 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.

It looks like option 1 here is the way to go. Just report an error if
there are multiple creds provided for the same host and they don't
match.

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

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

[Daniel P. Berrange]

On Fri, Mar 10, 2017 at 07:04:38PM -0800, ashish mittal wrote:
> On Wed, Mar 8, 2017 at 10:11 AM, Daniel P. Berrange  
> wrote:
> > On Wed, Mar 08, 2017 at 09:59:32AM -0800, ashish mittal wrote:
> >> On Wed, Mar 8, 2017 at 5:04 AM, Ketan Nilangekar
> >>  wrote:
> >> >
> >> >
> >> >> On Mar 8, 2017, at 1:13 AM, Daniel P. Berrange  
> >> >> wrote:
> >> >>
> >> >>> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
> >> >>> 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?
> >> >>
> >> >> I don't think I'm in favour of such an approach, as it forces a single
> >> >> QEMU process to use the same privileges for all disks it uses.
> >> >>
> >> >> Consider an example where a QEMU process has two disks, one shared
> >> >> readonly disk and one exclusive writable disk, both on the same host.
> >> >>
> >> >
> >> > This is not a use case for VxHS as a solution. We do not support sharing 
> >> > of vdisks across QEMU instances.
> >> >
> >> > Vxhs library was thus not designed to open different connections for 
> >> > individual vdisks within a QEMU instance.
> >> >
> >> > Implementing this will involve rewrite of significant parts of libvxhs 
> >> > client and server. Is this a new requirement for acceptance into QEMU?
> >> >
> >> >
> >> >> It is reasonable as an administrator to want to use different 
> >> >> credentials
> >> >> for each of these. ie, they might have a set of well known credentials 
> >> >> to
> >> >> authenticate to get access to the read-only disk, but have a different 
> >> >> set
> >> >> of strictly limited access credentials to get access to the writable 
> >> >> disk
> >> >>
> >> >> Trying to re-use the same connection for multiple cause prevents QEMU 
> >> >> from
> >> >> authenticating with different credentials per disk, so I don't think 
> >> >> that
> >> >> is a good approach - each disk should have totally independant state.
> >> >>
> >>
> >> libvxhs does not make any claim to fit all the general purpose
> >> use-cases. It was purpose-built to be the communication channel for
> >> our block device service. As such, we do not need/support all the
> >> general use-cases. For the same reason we changed the name of the
> >> library from linqnio to libvxhs (v8 changelog, #2).
> >
> > I raise these kind of points because they are relevant to apps like
> > OpenStack, against which you've proposed VHXS support. OpenStack
> > intends to allow a single volume to be shared by multiple guests,
> > so declare that out of scope you're crippling certain use cases
> > within openstack. Of course you're free to make such a decision,
> > but it makes VHXS a less compelling technology to use IMHO.
> >
> 
> Fair point! Sharing of the same disk across multiple guests would
> require significant work from our side, and we would like to evaluate
> that after OpenStack starts supporting the feature. Would adding this
> support now, be a prerequisite for merging vxhs code to qemu?

No, it isn't a requirement - just a (strong-ish) suggestion. We just need
to ensure that the CLI syntax allows us to support it in the future without
backwards incompatible changes to the CLI.

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

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

[ashish mittal]

On Wed, Mar 8, 2017 at 10:11 AM, Daniel P. Berrange  wrote:
> On Wed, Mar 08, 2017 at 09:59:32AM -0800, ashish mittal wrote:
>> On Wed, Mar 8, 2017 at 5:04 AM, Ketan Nilangekar
>>  wrote:
>> >
>> >
>> >> On Mar 8, 2017, at 1:13 AM, Daniel P. Berrange  
>> >> wrote:
>> >>
>> >>> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
>> >>> 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?
>> >>
>> >> I don't think I'm in favour of such an approach, as it forces a single
>> >> QEMU process to use the same privileges for all disks it uses.
>> >>
>> >> Consider an example where a QEMU process has two disks, one shared
>> >> readonly disk and one exclusive writable disk, both on the same host.
>> >>
>> >
>> > This is not a use case for VxHS as a solution. We do not support sharing 
>> > of vdisks across QEMU instances.
>> >
>> > Vxhs library was thus not designed to open different connections for 
>> > individual vdisks within a QEMU instance.
>> >
>> > Implementing this will involve rewrite of significant parts of libvxhs 
>> > client and server. Is this a new requirement for acceptance into QEMU?
>> >
>> >
>> >> It is reasonable as an administrator to want to use different credentials
>> >> for each of these. ie, they might have a set of well known credentials to
>> >> authenticate to get access to the read-only disk, but have a different set
>> >> of strictly limited access credentials to get access to the writable disk
>> >>
>> >> Trying to re-use the same connection for multiple cause prevents QEMU from
>> >> authenticating with different credentials per disk, so I don't think that
>> >> is a good approach - each disk should have totally independant state.
>> >>
>>
>> libvxhs does not make any claim to fit all the general purpose
>> use-cases. It was purpose-built to be the communication channel for
>> our block device service. As such, we do not need/support all the
>> general use-cases. For the same reason we changed the name of the
>> library from linqnio to libvxhs (v8 changelog, #2).
>
> I raise these kind of points because they are relevant to apps like
> OpenStack, against which you've proposed VHXS support. OpenStack
> intends to allow a single volume to be shared by multiple guests,
> so declare that out of scope you're crippling certain use cases
> within openstack. Of course you're free to make such a decision,
> but it makes VHXS a less compelling technology to use IMHO.
>

Fair point! Sharing of the same disk across multiple guests would
require significant work from our side, and we would like to evaluate
that after OpenStack starts supporting the feature. Would adding this
support now, be a prerequisite for merging vxhs code to qemu?

>> Having dedicated communication channels for each device, or sharing
>> the channels between multiple devices, should both be acceptable
>> choices. The latter, IO multiplexing, is also a widely adopted IO
>> model. It just happens to fit our use-cases better.
>>
>> Binding access control to a communication channel would prevent
>> anybody from using the latter approach. Having a separate way to allow
>> access-control would permit the use of latter also.
>
> Sharing access control across multiple disks does not fit in effectively
> with the model used by apps that manage QEMU. Libvirt, and apps above
> libvirt, like OpenStack, oVirt, and things like Kubernetes all represent
> the information required to connect to a network block device, on a
> per-disk basis - there's no sense of having some information that is
> shared across all disks associated with a VM.
>
> So from the POV of modelling this in QEMU, all information needs to be
> specified against the individual -drive / -blockdev. If you really must,
> you will just have to reject configurations which imply talking to the
> same host, with incompatible parameters. Better would be to dynamically
> determine if you can re-use connections, vs spawn new connection based
> on the config.
>

Would this be a prerequisite for merging vxhs?

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

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

[Daniel P. Berrange]

On Wed, Mar 08, 2017 at 09:59:32AM -0800, ashish mittal wrote:
> On Wed, Mar 8, 2017 at 5:04 AM, Ketan Nilangekar
>  wrote:
> >
> >
> >> On Mar 8, 2017, at 1:13 AM, Daniel P. Berrange  wrote:
> >>
> >>> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
> >>> 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?
> >>
> >> I don't think I'm in favour of such an approach, as it forces a single
> >> QEMU process to use the same privileges for all disks it uses.
> >>
> >> Consider an example where a QEMU process has two disks, one shared
> >> readonly disk and one exclusive writable disk, both on the same host.
> >>
> >
> > This is not a use case for VxHS as a solution. We do not support sharing of 
> > vdisks across QEMU instances.
> >
> > Vxhs library was thus not designed to open different connections for 
> > individual vdisks within a QEMU instance.
> >
> > Implementing this will involve rewrite of significant parts of libvxhs 
> > client and server. Is this a new requirement for acceptance into QEMU?
> >
> >
> >> It is reasonable as an administrator to want to use different credentials
> >> for each of these. ie, they might have a set of well known credentials to
> >> authenticate to get access to the read-only disk, but have a different set
> >> of strictly limited access credentials to get access to the writable disk
> >>
> >> Trying to re-use the same connection for multiple cause prevents QEMU from
> >> authenticating with different credentials per disk, so I don't think that
> >> is a good approach - each disk should have totally independant state.
> >>
> 
> libvxhs does not make any claim to fit all the general purpose
> use-cases. It was purpose-built to be the communication channel for
> our block device service. As such, we do not need/support all the
> general use-cases. For the same reason we changed the name of the
> library from linqnio to libvxhs (v8 changelog, #2).

I raise these kind of points because they are relevant to apps like
OpenStack, against which you've proposed VHXS support. OpenStack
intends to allow a single volume to be shared by multiple guests,
so declare that out of scope you're crippling certain use cases
within openstack. Of course you're free to make such a decision,
but it makes VHXS a less compelling technology to use IMHO.

> Having dedicated communication channels for each device, or sharing
> the channels between multiple devices, should both be acceptable
> choices. The latter, IO multiplexing, is also a widely adopted IO
> model. It just happens to fit our use-cases better.
> 
> Binding access control to a communication channel would prevent
> anybody from using the latter approach. Having a separate way to allow
> access-control would permit the use of latter also.

Sharing access control across multiple disks does not fit in effectively
with the model used by apps that manage QEMU. Libvirt, and apps above
libvirt, like OpenStack, oVirt, and things like Kubernetes all represent
the information required to connect to a network block device, on a
per-disk basis - there's no sense of having some information that is
shared across all disks associated with a VM.

So from the POV of modelling this in QEMU, all information needs to be
specified against the individual -drive / -blockdev. If you really must,
you will just have to reject configurations which imply talking to the
same host, with incompatible parameters. Better would be to dynamically
determine if you can re-use connections, vs spawn new connection based
on the config.

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

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

[ashish mittal]

On Wed, Mar 8, 2017 at 5:04 AM, Ketan Nilangekar
 wrote:
>
>
>> On Mar 8, 2017, at 1:13 AM, Daniel P. Berrange  wrote:
>>
>>> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
>>> 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?
>>
>> I don't think I'm in favour of such an approach, as it forces a single
>> QEMU process to use the same privileges for all disks it uses.
>>
>> Consider an example where a QEMU process has two disks, one shared
>> readonly disk and one exclusive writable disk, both on the same host.
>>
>
> This is not a use case for VxHS as a solution. We do not support sharing of 
> vdisks across QEMU instances.
>
> Vxhs library was thus not designed to open different connections for 
> individual vdisks within a QEMU instance.
>
> Implementing this will involve rewrite of significant parts of libvxhs client 
> and server. Is this a new requirement for acceptance into QEMU?
>
>
>> It is reasonable as an administrator to want to use different credentials
>> for each of these. ie, they might have a set of well known credentials to
>> authenticate to get access to the read-only disk, but have a different set
>> of strictly limited access credentials to get access to the writable disk
>>
>> Trying to re-use the same connection for multiple cause prevents QEMU from
>> authenticating with different credentials per disk, so I don't think that
>> is a good approach - each disk should have totally independant state.
>>

libvxhs does not make any claim to fit all the general purpose
use-cases. It was purpose-built to be the communication channel for
our block device service. As such, we do not need/support all the
general use-cases. For the same reason we changed the name of the
library from linqnio to libvxhs (v8 changelog, #2).

Having dedicated communication channels for each device, or sharing
the channels between multiple devices, should both be acceptable
choices. The latter, IO multiplexing, is also a widely adopted IO
model. It just happens to fit our use-cases better.

Binding access control to a communication channel would prevent
anybody from using the latter approach. Having a separate way to allow
access-control would permit the use of latter also.

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

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

[Ketan Nilangekar]

> On Mar 8, 2017, at 1:13 AM, Daniel P. Berrange  wrote:
> 
>> On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
>> 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?
> 
> I don't think I'm in favour of such an approach, as it forces a single
> QEMU process to use the same privileges for all disks it uses.
> 
> Consider an example where a QEMU process has two disks, one shared
> readonly disk and one exclusive writable disk, both on the same host.
> 

This is not a use case for VxHS as a solution. We do not support sharing of 
vdisks across QEMU instances.

Vxhs library was thus not designed to open different connections for individual 
vdisks within a QEMU instance.

Implementing this will involve rewrite of significant parts of libvxhs client 
and server. Is this a new requirement for acceptance into QEMU?


> It is reasonable as an administrator to want to use different credentials
> for each of these. ie, they might have a set of well known credentials to
> authenticate to get access to the read-only disk, but have a different set
> of strictly limited access credentials to get access to the writable disk
> 
> Trying to re-use the same connection for multiple cause prevents QEMU from
> authenticating with different credentials per disk, so I don't think that
> is a good approach - each disk should have totally independant state.
> 
>> 
>> 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,
> 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/ :|

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

[Daniel P. Berrange]

On Tue, Mar 07, 2017 at 05:27:55PM -0800, ashish mittal wrote:
> 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?

I don't think I'm in favour of such an approach, as it forces a single
QEMU process to use the same privileges for all disks it uses.

Consider an example where a QEMU process has two disks, one shared
readonly disk and one exclusive writable disk, both on the same host.

It is reasonable as an administrator to want to use different credentials
for each of these. ie, they might have a set of well known credentials to
authenticate to get access to the read-only disk, but have a different set
of strictly limited access credentials to get access to the writable disk

Trying to re-use the same connection for multiple cause prevents QEMU from
authenticating with different credentials per disk, so I don't think that
is a good approach - each disk should have totally independant state.

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

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  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  
>> 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:
>>
>> [amittal2@camshaft 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": "",
>> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'
>> 15116@1488758101.084355:vxhs_open_vdiskid Opening vdisk-id /test.raw
>> 15116@1488758101.084396: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 
>> 15116@1488758101.084402:vxhs_open_hostinfo Adding host 127.0.0.1:
>> to BDRVVXHSState
>> 15116@1488758101.092060: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)
>> 15116@1488758101.094643:vxhs_close Closing vdisk /test.raw
>> [amittal2@camshaft 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(_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 

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  
> 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:
> 
> [amittal2@camshaft 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": "",
> "vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'
> 15116@1488758101.084355:vxhs_open_vdiskid Opening vdisk-id /test.raw
> 15116@1488758101.084396: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 
> 15116@1488758101.084402:vxhs_open_hostinfo Adding host 127.0.0.1:
> to BDRVVXHSState
> 15116@1488758101.092060: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)
> 15116@1488758101.094643:vxhs_close Closing vdisk /test.raw
> [amittal2@camshaft 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(_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, , _key,
> +   _cert, _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/ :|

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

[ashish mittal]

On Wed, Mar 1, 2017 at 1:18 AM, Daniel P. Berrange  wrote:
> On Tue, Feb 28, 2017 at 02:51:39PM -0800, ashish mittal wrote:
>> On Mon, Feb 27, 2017 at 1:22 AM, Daniel P. Berrange  
>> wrote:
>
>> >> +ret = -EINVAL;
>> >> +goto out;
>> >> +}
>> >> +
>> >> +secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID);
>> >> +if (!secretid) {
>> >> +error_setg(_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, _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,
>>
>> 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?
>
> 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.

Here are the files I had to create -
$ ll /etc/pki/qemu/vxhs
total 12
-r--r--r--. 1 root root 2094 Mar  4 18:02 ca-cert.pem
-r--r--r--. 1 root root 1899 Mar  4 18:00 client-cert.pem
-r--r--r--. 1 root root 1679 Mar  4 17:59 client-key.pem

>> (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.
>
> Yep, I think that's the minimum required change to make this mergable.
>
>> (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.
>
> That's correct - you can assume file permissions protect the cert
> files. I would expect the syntax to work as follows
>
>   $QEMU  -object 
> tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client \
>  -drive  driver=vxhs,...other..opts...,tls-creds=tls0
>
> When you see the 'tls-creds' flag, you can lookup the corresponding
> QCryptoTLSCredsX509 object and extract the 'dir' property from it.
>

The code diff below works as above. Please verify.

> 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.

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


Example CLI accepting the new TLS credentials:

[amittal2@camshaft 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": "",
"vdisk-id": "/test.raw", "driver": "vxhs", "tls-creds":"tls0"}'
15116@1488758101.084355:vxhs_open_vdiskid Opening vdisk-id /test.raw
15116@1488758101.084396: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 
15116@1488758101.084402:vxhs_open_hostinfo Adding host 127.0.0.1:
to BDRVVXHSState
15116@1488758101.092060: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)
15116@1488758101.094643:vxhs_close Closing vdisk /test.raw
[amittal2@camshaft qemu] 2017-03-05 15:55:01$

NB - I am passing client-key and client-cert to iio_open() here.
libqnio changes to work with these new args via iio_open() will
follow.

diff --git a/block/trace-events 

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

[Daniel P. Berrange]

On Tue, Feb 28, 2017 at 02:51:39PM -0800, ashish mittal wrote:
> On Mon, Feb 27, 2017 at 1:22 AM, Daniel P. Berrange  
> wrote:

> >> +ret = -EINVAL;
> >> +goto out;
> >> +}
> >> +
> >> +secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID);
> >> +if (!secretid) {
> >> +error_setg(_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, _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,
> 
> 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?

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 

> (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.

Yep, I think that's the minimum required change to make this mergable.

> (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.

That's correct - you can assume file permissions protect the cert
files. I would expect the syntax to work as follows

  $QEMU  -object tls-creds-x509,id=tls0,dir=/etc/pki/qemu/vxhs,endpoint=client \
 -drive  driver=vxhs,...other..opts...,tls-creds=tls0

When you see the 'tls-creds' flag, you can lookup the corresponding
QCryptoTLSCredsX509 object and extract the 'dir' property from it.

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

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

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  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": "", "vdisk-id":
>> "/test.raw", "driver": "vxhs", "user": "ashish",  "password-secret":
>> "xvxhspasswd"}'
>> 1132@1487977829.151064:vxhs_open_vdiskid Opening vdisk-id /test.raw
>>
>> 1132@1487977829.151141:vxhs_get_creds User ashish, SecretID
>> xvxhspasswd, Password Str0ngP@ssw0rd   <===   NOTE WILL NOT PRINT
>> PASSWORD IN FINAL CODE 
>>
>> 1132@1487977829.151168:vxhs_open_hostinfo Adding host 127.0.0.1:
>> to BDRVVXHSState
>> 1132@1487977829.173062: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)
>> 1132@1487977829.175141: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": "", "vdisk-id":
>> "/test.raw", "driver": "vxhs"}'
>> 1310@1487978547.771234:vxhs_open_vdiskid Opening vdisk-id /test.raw
>> can't open device json:{"server.host": "127.0.0.1", "server.port":
>> "", "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(_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(_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, _err);
>> +if (local_err != NULL) {
>> +trace_vxhs_get_creds(user, secretid, password);
>> +qdict_del(backing_options, str);
>> +ret = 

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

[Daniel P. Berrange]

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

> 
> 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": "", "vdisk-id":
> "/test.raw", "driver": "vxhs", "user": "ashish",  "password-secret":
> "xvxhspasswd"}'
> 1132@1487977829.151064:vxhs_open_vdiskid Opening vdisk-id /test.raw
> 
> 1132@1487977829.151141:vxhs_get_creds User ashish, SecretID
> xvxhspasswd, Password Str0ngP@ssw0rd   <===   NOTE WILL NOT PRINT
> PASSWORD IN FINAL CODE 
> 
> 1132@1487977829.151168:vxhs_open_hostinfo Adding host 127.0.0.1:
> to BDRVVXHSState
> 1132@1487977829.173062: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)
> 1132@1487977829.175141: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": "", "vdisk-id":
> "/test.raw", "driver": "vxhs"}'
> 1310@1487978547.771234:vxhs_open_vdiskid Opening vdisk-id /test.raw
> can't open device json:{"server.host": "127.0.0.1", "server.port":
> "", "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(_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

> +ret = -EINVAL;
> +goto out;
> +}
> +
> +secretid = qemu_opt_get(opts, VXHS_OPT_SECRETID);
> +if (!secretid) {
> +error_setg(_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, _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-

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": "", "vdisk-id":
"/test.raw", "driver": "vxhs", "user": "ashish",  "password-secret":
"xvxhspasswd"}'
1132@1487977829.151064:vxhs_open_vdiskid Opening vdisk-id /test.raw

1132@1487977829.151141:vxhs_get_creds User ashish, SecretID
xvxhspasswd, Password Str0ngP@ssw0rd   <===   NOTE WILL NOT PRINT
PASSWORD IN FINAL CODE 

1132@1487977829.151168:vxhs_open_hostinfo Adding host 127.0.0.1:
to BDRVVXHSState
1132@1487977829.173062: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)
1132@1487977829.175141: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": "", "vdisk-id":
"/test.raw", "driver": "vxhs"}'
1310@1487978547.771234:vxhs_open_vdiskid Opening vdisk-id /test.raw
can't open device json:{"server.host": "127.0.0.1", "server.port":
"", "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(_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(_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, _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  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 

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

[Daniel P. Berrange]

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": "", "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:
> 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": "", "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":
> "", "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": "",
   "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, _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  

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

[ashish mittal]

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": "", "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:
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": "", "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":
"", "vdisk-id": "/test.raw", "driver": "vxhs"}: No secret with id
'user'  < NOTE!
[root@rhel72ga-build-upstream qemu] 2017-02-23 19:30:25$


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";
+//return;
+}
+
+password = qcrypto_secret_lookup_as_utf8(user, _err);
+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);
+
 s->vdisk_hostinfo.host = g_strdup(server_host_opt);

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

Thanks,
Ashish

On Wed, Feb 22, 2017 at 6:44 AM, Jeff Cody  wrote:
> On Wed, Feb 22, 2017 at 02:22:30PM +, Daniel P. Berrange wrote:
>> On Wed, Feb 22, 2017 at 02:09:20PM +, Stefan Hajnoczi wrote:
>> > On Tue, Feb 21, 2017 at 11:33:53AM +, Daniel P. Berrange wrote:
>> > > On Tue, Feb 21, 2017 at 10:59:18AM +, Stefan Hajnoczi wrote:
>> > > > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
>> > > > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi 
>> > > > >  wrote:
>> > > > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
>> > > > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
>> > > > > >>
>> > > > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
>> > > > > >> > Hi,
>> > > > > >> >
>> > > > > >> > I am getting the following error with checkpatch.pl
>> > > > > >> >
>> > > > > >> > ERROR: externs should be avoided in .c files
>> > > > > >> > #78: FILE: block/vxhs.c:28:
>> > > > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
>> > > > > >> >
>> > > > > >> > Is there any way to get around this, or does it mean that I 
>> > > > > >> would have
>> > > > > >> > to add a vxhs.h just for this one entry?
>> > > > > >> >
>> > > > > >>
>> > > > > >> I remain skeptical on the use of the qemu_uuid as a way to 
>> > > > > >> select the TLS
>> > > > > >> cert.
>> > > > > >>
>> > > > > >> [ketan]
>> > > > > >> Is there another identity that can be used 

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

[Jeff Cody]

On Wed, Feb 22, 2017 at 02:22:30PM +, Daniel P. Berrange wrote:
> On Wed, Feb 22, 2017 at 02:09:20PM +, Stefan Hajnoczi wrote:
> > On Tue, Feb 21, 2017 at 11:33:53AM +, Daniel P. Berrange wrote:
> > > On Tue, Feb 21, 2017 at 10:59:18AM +, Stefan Hajnoczi wrote:
> > > > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > > > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  
> > > > > wrote:
> > > > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > > > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > > > > >>
> > > > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > > > > >> > Hi,
> > > > > >> >
> > > > > >> > I am getting the following error with checkpatch.pl
> > > > > >> >
> > > > > >> > ERROR: externs should be avoided in .c files
> > > > > >> > #78: FILE: block/vxhs.c:28:
> > > > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > > > > >> >
> > > > > >> > Is there any way to get around this, or does it mean that I 
> > > > > >> would have
> > > > > >> > to add a vxhs.h just for this one entry?
> > > > > >> >
> > > > > >>
> > > > > >> I remain skeptical on the use of the qemu_uuid as a way to 
> > > > > >> select the TLS
> > > > > >> cert.
> > > > > >>
> > > > > >> [ketan]
> > > > > >> Is there another identity that can be used for uniquely 
> > > > > >> identifying instances?
> > > > > >> The requirement was to enforce vdisk access to owner instances.
> > > > > >
> > > > > > The qemu_uuid weak attribute looks suspect.  What is going to 
> > > > > > provide a
> > > > > > strong qemu_uuid symbol?
> > > > > >
> > > > > > Why aren't configuration parameters like the UUID coming from the 
> > > > > > QEMU
> > > > > > command-line?
> > > > > >
> > > > > > Stefan
> > > > > 
> > > > > UUID will in fact come from the QEMU command line. VxHS is not doing
> > > > > anything special here. It will just use the value already available to
> > > > > qemu-kvm process.
> > > > > 
> > > > > QemuUUID qemu_uuid;
> > > > > bool qemu_uuid_set;
> > > > > 
> > > > > Both the above are defined in vl.c. vl.c will provide the strong
> > > > > symbol when available. There are certain binaries that do not get
> > > > > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > > > > affect for such binaries, and in this case, the default VXHS UUID will
> > > > > get picked up. I had, in a previous email, explained how we plan to
> > > > > use the default UUID. In the regular case, the VxHS controller will
> > > > > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > > > > may choose to grant temporary access to specific vdisks for these
> > > > > binaries depending on the workflow.
> > > > 
> > > > That idea sounds like a security problem.  During this time window
> > > > anyone could use the default UUID to access the data?
> > > 
> > > Any use of the VM UUID as a means to control authorization on the
> > > server side is a security flaw, as this is a public value which
> > > cannot be trusted on its own.
> > > 
> > > There needs to be some kind of authentication step to verify the
> > > reported identity, eg a password associated with the VM UUID that
> > > is validated before trusting the VM UUID.
> > > 
> > > Alternatively there needs to be a completely separate UUID, unrelated
> > > to the VM UUID, which is treated as a private value (eg uses the
> > > '-object secret' framework in QEMU)
> > 
> > I thought the UUID is used to select the TLS client certificate and
> > associated private key.  So the UUID provides authorization although
> > what really matters is the client certificate, not the actual value of
> > the UUID.
> 
> The message shown a few replies earlier said:
> 
>   "VxHS controller will not allow access to the default UUID (non qemu-kvm)
>binaries, but it may choose to grant temporary access to specific
>vdisks"
> 
> which suggests the VxHS server is making authorization decisions based
> on UUID, but perhaps this is incorrect interpretation and it really is
> making decisions based on the x509 cert identity or something else ?
> 
> 
> In any case hardcoding a policy of using the UUID to select a cert path
> is a flawed design. We can't assume that everyone deploying QEMU is going
> to be willing to configure a separate certificate per QEMU VM instance
> launched. People's CA management policies are often so burdensome that
> it will be impractical to generate a new cert for VMs on the fly. So we
> should expect that many people will just deploy one cert per host, with
> the cert being statically created at the time they setup the host. Thus
> we need to just be able to specify certs used explicitly when adding a
> disk to QEMU, so we can support different deployment models for cert
> usage
>

I do believe it is using the UUID to select the cert/key files; from
libqnio:

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

[Stefan Hajnoczi]

On Tue, Feb 21, 2017 at 05:21:46PM +, Ketan Nilangekar wrote:
> 
> 
> On 2/21/17, 5:59 AM, "Stefan Hajnoczi"  wrote:
> 
> On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  
> wrote:
> > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > >>
> > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > >> > Hi,
> > >> >
> > >> > I am getting the following error with checkpatch.pl
> > >> >
> > >> > ERROR: externs should be avoided in .c files
> > >> > #78: FILE: block/vxhs.c:28:
> > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > >> >
> > >> > Is there any way to get around this, or does it mean that I 
> would have
> > >> > to add a vxhs.h just for this one entry?
> > >> >
> > >>
> > >> I remain skeptical on the use of the qemu_uuid as a way to 
> select the TLS
> > >> cert.
> > >>
> > >> [ketan]
> > >> Is there another identity that can be used for uniquely identifying 
> instances?
> > >> The requirement was to enforce vdisk access to owner instances.
> > >
> > > The qemu_uuid weak attribute looks suspect.  What is going to provide 
> a
> > > strong qemu_uuid symbol?
> > >
> > > Why aren't configuration parameters like the UUID coming from the QEMU
> > > command-line?
> > >
> > > Stefan
> > 
> > UUID will in fact come from the QEMU command line. VxHS is not doing
> > anything special here. It will just use the value already available to
> > qemu-kvm process.
> > 
> > QemuUUID qemu_uuid;
> > bool qemu_uuid_set;
> > 
> > Both the above are defined in vl.c. vl.c will provide the strong
> > symbol when available. There are certain binaries that do not get
> > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > affect for such binaries, and in this case, the default VXHS UUID will
> > get picked up. I had, in a previous email, explained how we plan to
> > use the default UUID. In the regular case, the VxHS controller will
> > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > may choose to grant temporary access to specific vdisks for these
> > binaries depending on the workflow.
> 
> That idea sounds like a security problem.  During this time window
> anyone could use the default UUID to access the data?
> 
> Just make the UUID (or TLS client certificate file) a command-line
> parameter that qemu-system, qemu-img, and other tools accept (e.g.
> qemu-img via the --image-opts/--object syntax).
> 
> [Ketan]
> Sounds fair. Would it be ok to take this up after the driver is merged for 
> the upcoming QEMU release?

No, because the current command-line interface isn't production-ready.
QEMU's command-line interface must stay backwards compatible forever so
merging the incomplete code now will create problems.

Start by tackling the comments that Daniel Berrange made 9 days ago
about hardcoded paths in libvxhs:
https://github.com/VeritasHyperScale/libqnio/pull/12#discussion_r100812640

As a consequence of resolving that issue, the QEMU block driver will
need new command-line parameters so it can pass in the configuration.
Then qemu_uuid will not be needed anymore because the command-line
parameters should already specify everything that libvxhs needs.

Stefan


signature.asc
Description: PGP signature

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

[Daniel P. Berrange]

On Wed, Feb 22, 2017 at 02:09:20PM +, Stefan Hajnoczi wrote:
> On Tue, Feb 21, 2017 at 11:33:53AM +, Daniel P. Berrange wrote:
> > On Tue, Feb 21, 2017 at 10:59:18AM +, Stefan Hajnoczi wrote:
> > > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  
> > > > wrote:
> > > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > > > >>
> > > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > > > >> > Hi,
> > > > >> >
> > > > >> > I am getting the following error with checkpatch.pl
> > > > >> >
> > > > >> > ERROR: externs should be avoided in .c files
> > > > >> > #78: FILE: block/vxhs.c:28:
> > > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > > > >> >
> > > > >> > Is there any way to get around this, or does it mean that I 
> > > > >> would have
> > > > >> > to add a vxhs.h just for this one entry?
> > > > >> >
> > > > >>
> > > > >> I remain skeptical on the use of the qemu_uuid as a way to 
> > > > >> select the TLS
> > > > >> cert.
> > > > >>
> > > > >> [ketan]
> > > > >> Is there another identity that can be used for uniquely identifying 
> > > > >> instances?
> > > > >> The requirement was to enforce vdisk access to owner instances.
> > > > >
> > > > > The qemu_uuid weak attribute looks suspect.  What is going to provide 
> > > > > a
> > > > > strong qemu_uuid symbol?
> > > > >
> > > > > Why aren't configuration parameters like the UUID coming from the QEMU
> > > > > command-line?
> > > > >
> > > > > Stefan
> > > > 
> > > > UUID will in fact come from the QEMU command line. VxHS is not doing
> > > > anything special here. It will just use the value already available to
> > > > qemu-kvm process.
> > > > 
> > > > QemuUUID qemu_uuid;
> > > > bool qemu_uuid_set;
> > > > 
> > > > Both the above are defined in vl.c. vl.c will provide the strong
> > > > symbol when available. There are certain binaries that do not get
> > > > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > > > affect for such binaries, and in this case, the default VXHS UUID will
> > > > get picked up. I had, in a previous email, explained how we plan to
> > > > use the default UUID. In the regular case, the VxHS controller will
> > > > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > > > may choose to grant temporary access to specific vdisks for these
> > > > binaries depending on the workflow.
> > > 
> > > That idea sounds like a security problem.  During this time window
> > > anyone could use the default UUID to access the data?
> > 
> > Any use of the VM UUID as a means to control authorization on the
> > server side is a security flaw, as this is a public value which
> > cannot be trusted on its own.
> > 
> > There needs to be some kind of authentication step to verify the
> > reported identity, eg a password associated with the VM UUID that
> > is validated before trusting the VM UUID.
> > 
> > Alternatively there needs to be a completely separate UUID, unrelated
> > to the VM UUID, which is treated as a private value (eg uses the
> > '-object secret' framework in QEMU)
> 
> I thought the UUID is used to select the TLS client certificate and
> associated private key.  So the UUID provides authorization although
> what really matters is the client certificate, not the actual value of
> the UUID.

The message shown a few replies earlier said:

  "VxHS controller will not allow access to the default UUID (non qemu-kvm)
   binaries, but it may choose to grant temporary access to specific
   vdisks"

which suggests the VxHS server is making authorization decisions based
on UUID, but perhaps this is incorrect interpretation and it really is
making decisions based on the x509 cert identity or something else ?


In any case hardcoding a policy of using the UUID to select a cert path
is a flawed design. We can't assume that everyone deploying QEMU is going
to be willing to configure a separate certificate per QEMU VM instance
launched. People's CA management policies are often so burdensome that
it will be impractical to generate a new cert for VMs on the fly. So we
should expect that many people will just deploy one cert per host, with
the cert being statically created at the time they setup the host. Thus
we need to just be able to specify certs used explicitly when adding a
disk to QEMU, so we can support different deployment models for cert
usage

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

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

[Stefan Hajnoczi]

On Tue, Feb 21, 2017 at 11:33:53AM +, Daniel P. Berrange wrote:
> On Tue, Feb 21, 2017 at 10:59:18AM +, Stefan Hajnoczi wrote:
> > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  
> > > wrote:
> > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > > >>
> > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > > >> > Hi,
> > > >> >
> > > >> > I am getting the following error with checkpatch.pl
> > > >> >
> > > >> > ERROR: externs should be avoided in .c files
> > > >> > #78: FILE: block/vxhs.c:28:
> > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > > >> >
> > > >> > Is there any way to get around this, or does it mean that I 
> > > >> would have
> > > >> > to add a vxhs.h just for this one entry?
> > > >> >
> > > >>
> > > >> I remain skeptical on the use of the qemu_uuid as a way to select 
> > > >> the TLS
> > > >> cert.
> > > >>
> > > >> [ketan]
> > > >> Is there another identity that can be used for uniquely identifying 
> > > >> instances?
> > > >> The requirement was to enforce vdisk access to owner instances.
> > > >
> > > > The qemu_uuid weak attribute looks suspect.  What is going to provide a
> > > > strong qemu_uuid symbol?
> > > >
> > > > Why aren't configuration parameters like the UUID coming from the QEMU
> > > > command-line?
> > > >
> > > > Stefan
> > > 
> > > UUID will in fact come from the QEMU command line. VxHS is not doing
> > > anything special here. It will just use the value already available to
> > > qemu-kvm process.
> > > 
> > > QemuUUID qemu_uuid;
> > > bool qemu_uuid_set;
> > > 
> > > Both the above are defined in vl.c. vl.c will provide the strong
> > > symbol when available. There are certain binaries that do not get
> > > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > > affect for such binaries, and in this case, the default VXHS UUID will
> > > get picked up. I had, in a previous email, explained how we plan to
> > > use the default UUID. In the regular case, the VxHS controller will
> > > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > > may choose to grant temporary access to specific vdisks for these
> > > binaries depending on the workflow.
> > 
> > That idea sounds like a security problem.  During this time window
> > anyone could use the default UUID to access the data?
> 
> Any use of the VM UUID as a means to control authorization on the
> server side is a security flaw, as this is a public value which
> cannot be trusted on its own.
> 
> There needs to be some kind of authentication step to verify the
> reported identity, eg a password associated with the VM UUID that
> is validated before trusting the VM UUID.
> 
> Alternatively there needs to be a completely separate UUID, unrelated
> to the VM UUID, which is treated as a private value (eg uses the
> '-object secret' framework in QEMU)

I thought the UUID is used to select the TLS client certificate and
associated private key.  So the UUID provides authorization although
what really matters is the client certificate, not the actual value of
the UUID.

Stefan


signature.asc
Description: PGP signature

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

[Daniel P. Berrange]

On Tue, Feb 21, 2017 at 02:25:53PM -0500, Jeff Cody wrote:
> On Tue, Feb 21, 2017 at 06:56:24PM +, Daniel P. Berrange wrote:
> > On Tue, Feb 21, 2017 at 01:06:58PM -0500, Jeff Cody wrote:
> > > On Tue, Feb 21, 2017 at 05:39:12PM +, Daniel P. Berrange wrote:
> > > > On Tue, Feb 21, 2017 at 05:21:46PM +, Ketan Nilangekar wrote:
> > > > > 
> > > > > 
> > > > > On 2/21/17, 5:59 AM, "Stefan Hajnoczi"  wrote:
> > > > > 
> > > > > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > > > > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi 
> > > > >  wrote:
> > > > > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar 
> > > > > wrote:
> > > > > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > > > > > >>
> > > > > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal 
> > > > > wrote:
> > > > > > >> > Hi,
> > > > > > >> >
> > > > > > >> > I am getting the following error with checkpatch.pl
> > > > > > >> >
> > > > > > >> > ERROR: externs should be avoided in .c files
> > > > > > >> > #78: FILE: block/vxhs.c:28:
> > > > > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > > > > > >> >
> > > > > > >> > Is there any way to get around this, or does it mean 
> > > > > that I would have
> > > > > > >> > to add a vxhs.h just for this one entry?
> > > > > > >> >
> > > > > > >>
> > > > > > >> I remain skeptical on the use of the qemu_uuid as a way 
> > > > > to select the TLS
> > > > > > >> cert.
> > > > > > >>
> > > > > > >> [ketan]
> > > > > > >> Is there another identity that can be used for uniquely 
> > > > > identifying instances?
> > > > > > >> The requirement was to enforce vdisk access to owner 
> > > > > instances.
> > > > > > >
> > > > > > > The qemu_uuid weak attribute looks suspect.  What is going to 
> > > > > provide a
> > > > > > > strong qemu_uuid symbol?
> > > > > > >
> > > > > > > Why aren't configuration parameters like the UUID coming from 
> > > > > the QEMU
> > > > > > > command-line?
> > > > > > >
> > > > > > > Stefan
> > > > > > 
> > > > > > UUID will in fact come from the QEMU command line. VxHS is not 
> > > > > doing
> > > > > > anything special here. It will just use the value already 
> > > > > available to
> > > > > > qemu-kvm process.
> > > > > > 
> > > > > > QemuUUID qemu_uuid;
> > > > > > bool qemu_uuid_set;
> > > > > > 
> > > > > > Both the above are defined in vl.c. vl.c will provide the strong
> > > > > > symbol when available. There are certain binaries that do not 
> > > > > get
> > > > > > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > > > > > affect for such binaries, and in this case, the default VXHS 
> > > > > UUID will
> > > > > > get picked up. I had, in a previous email, explained how we 
> > > > > plan to
> > > > > > use the default UUID. In the regular case, the VxHS controller 
> > > > > will
> > > > > > not allow access to the default UUID (non qemu-kvm) binaries, 
> > > > > but it
> > > > > > may choose to grant temporary access to specific vdisks for 
> > > > > these
> > > > > > binaries depending on the workflow.
> > > > > 
> > > > > That idea sounds like a security problem.  During this time window
> > > > > anyone could use the default UUID to access the data?
> > > > > 
> > > > > Just make the UUID (or TLS client certificate file) a command-line
> > > > > parameter that qemu-system, qemu-img, and other tools accept (e.g.
> > > > > qemu-img via the --image-opts/--object syntax).
> > > > > 
> > > > > [Ketan]
> > > > > Sounds fair. Would it be ok to take this up after the driver is
> > > > > merged for the upcoming QEMU release?
> > > > 
> > > > I don't think we can merge code with known security flaws, particularly
> > > > if fixing these flaws will involve adding and/or changing command line
> > > > parameters for the block driver.
> > > >
> > > 
> > > We do support some protocols, such as gluster, that do not have robust
> > > authentication frameworks over tcp/ip.  Of course, these protocols have 
> > > been
> > > in as a driver for several years (and, gluster does support unix sockets).
> > 
> > NB, gluster *does* have secure access control. It uses the verified x509
> > certificate identity as a token against which access control rules are
> > placed on volumes.
> > 
> > It isn't authentication in the traditional sense most people think of it,
> > but it does provide a secure authorization facility.
> >
> 
> Good point, thanks for the clarification.
> 
> > > We seem to be establishing a rule for QEMU, that is "no new protocol 
> > > drivers
> > > without secure authentication".  That is a good thing. The existence of
> > > current protocol drivers that 

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

[Jeff Cody]

On Tue, Feb 21, 2017 at 06:56:24PM +, Daniel P. Berrange wrote:
> On Tue, Feb 21, 2017 at 01:06:58PM -0500, Jeff Cody wrote:
> > On Tue, Feb 21, 2017 at 05:39:12PM +, Daniel P. Berrange wrote:
> > > On Tue, Feb 21, 2017 at 05:21:46PM +, Ketan Nilangekar wrote:
> > > > 
> > > > 
> > > > On 2/21/17, 5:59 AM, "Stefan Hajnoczi"  wrote:
> > > > 
> > > > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > > > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi 
> > > >  wrote:
> > > > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar 
> > > > wrote:
> > > > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > > > > >>
> > > > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal 
> > > > wrote:
> > > > > >> > Hi,
> > > > > >> >
> > > > > >> > I am getting the following error with checkpatch.pl
> > > > > >> >
> > > > > >> > ERROR: externs should be avoided in .c files
> > > > > >> > #78: FILE: block/vxhs.c:28:
> > > > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > > > > >> >
> > > > > >> > Is there any way to get around this, or does it mean 
> > > > that I would have
> > > > > >> > to add a vxhs.h just for this one entry?
> > > > > >> >
> > > > > >>
> > > > > >> I remain skeptical on the use of the qemu_uuid as a way to 
> > > > select the TLS
> > > > > >> cert.
> > > > > >>
> > > > > >> [ketan]
> > > > > >> Is there another identity that can be used for uniquely 
> > > > identifying instances?
> > > > > >> The requirement was to enforce vdisk access to owner instances.
> > > > > >
> > > > > > The qemu_uuid weak attribute looks suspect.  What is going to 
> > > > provide a
> > > > > > strong qemu_uuid symbol?
> > > > > >
> > > > > > Why aren't configuration parameters like the UUID coming from 
> > > > the QEMU
> > > > > > command-line?
> > > > > >
> > > > > > Stefan
> > > > > 
> > > > > UUID will in fact come from the QEMU command line. VxHS is not 
> > > > doing
> > > > > anything special here. It will just use the value already 
> > > > available to
> > > > > qemu-kvm process.
> > > > > 
> > > > > QemuUUID qemu_uuid;
> > > > > bool qemu_uuid_set;
> > > > > 
> > > > > Both the above are defined in vl.c. vl.c will provide the strong
> > > > > symbol when available. There are certain binaries that do not get
> > > > > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > > > > affect for such binaries, and in this case, the default VXHS UUID 
> > > > will
> > > > > get picked up. I had, in a previous email, explained how we plan 
> > > > to
> > > > > use the default UUID. In the regular case, the VxHS controller 
> > > > will
> > > > > not allow access to the default UUID (non qemu-kvm) binaries, but 
> > > > it
> > > > > may choose to grant temporary access to specific vdisks for these
> > > > > binaries depending on the workflow.
> > > > 
> > > > That idea sounds like a security problem.  During this time window
> > > > anyone could use the default UUID to access the data?
> > > > 
> > > > Just make the UUID (or TLS client certificate file) a command-line
> > > > parameter that qemu-system, qemu-img, and other tools accept (e.g.
> > > > qemu-img via the --image-opts/--object syntax).
> > > > 
> > > > [Ketan]
> > > > Sounds fair. Would it be ok to take this up after the driver is
> > > > merged for the upcoming QEMU release?
> > > 
> > > I don't think we can merge code with known security flaws, particularly
> > > if fixing these flaws will involve adding and/or changing command line
> > > parameters for the block driver.
> > >
> > 
> > We do support some protocols, such as gluster, that do not have robust
> > authentication frameworks over tcp/ip.  Of course, these protocols have been
> > in as a driver for several years (and, gluster does support unix sockets).
> 
> NB, gluster *does* have secure access control. It uses the verified x509
> certificate identity as a token against which access control rules are
> placed on volumes.
> 
> It isn't authentication in the traditional sense most people think of it,
> but it does provide a secure authorization facility.
>

Good point, thanks for the clarification.

> > We seem to be establishing a rule for QEMU, that is "no new protocol drivers
> > without secure authentication".  That is a good thing. The existence of
> > current protocol drivers that don't meet that criteria is potentially
> > confusing for new contributors, however.  (As a side note to myself -- this
> > is probably a good thing to add to the wiki, if it is not there already).
> 
> It's been my goal to fix / enhance everything in QEMU that uses network and
> does not have secure encryption + 

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

[Daniel P. Berrange]

On Tue, Feb 21, 2017 at 01:06:58PM -0500, Jeff Cody wrote:
> On Tue, Feb 21, 2017 at 05:39:12PM +, Daniel P. Berrange wrote:
> > On Tue, Feb 21, 2017 at 05:21:46PM +, Ketan Nilangekar wrote:
> > > 
> > > 
> > > On 2/21/17, 5:59 AM, "Stefan Hajnoczi"  wrote:
> > > 
> > > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi 
> > >  wrote:
> > > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > > > >>
> > > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal 
> > > wrote:
> > > > >> > Hi,
> > > > >> >
> > > > >> > I am getting the following error with checkpatch.pl
> > > > >> >
> > > > >> > ERROR: externs should be avoided in .c files
> > > > >> > #78: FILE: block/vxhs.c:28:
> > > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > > > >> >
> > > > >> > Is there any way to get around this, or does it mean that 
> > > I would have
> > > > >> > to add a vxhs.h just for this one entry?
> > > > >> >
> > > > >>
> > > > >> I remain skeptical on the use of the qemu_uuid as a way to 
> > > select the TLS
> > > > >> cert.
> > > > >>
> > > > >> [ketan]
> > > > >> Is there another identity that can be used for uniquely 
> > > identifying instances?
> > > > >> The requirement was to enforce vdisk access to owner instances.
> > > > >
> > > > > The qemu_uuid weak attribute looks suspect.  What is going to 
> > > provide a
> > > > > strong qemu_uuid symbol?
> > > > >
> > > > > Why aren't configuration parameters like the UUID coming from the 
> > > QEMU
> > > > > command-line?
> > > > >
> > > > > Stefan
> > > > 
> > > > UUID will in fact come from the QEMU command line. VxHS is not doing
> > > > anything special here. It will just use the value already available 
> > > to
> > > > qemu-kvm process.
> > > > 
> > > > QemuUUID qemu_uuid;
> > > > bool qemu_uuid_set;
> > > > 
> > > > Both the above are defined in vl.c. vl.c will provide the strong
> > > > symbol when available. There are certain binaries that do not get
> > > > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > > > affect for such binaries, and in this case, the default VXHS UUID 
> > > will
> > > > get picked up. I had, in a previous email, explained how we plan to
> > > > use the default UUID. In the regular case, the VxHS controller will
> > > > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > > > may choose to grant temporary access to specific vdisks for these
> > > > binaries depending on the workflow.
> > > 
> > > That idea sounds like a security problem.  During this time window
> > > anyone could use the default UUID to access the data?
> > > 
> > > Just make the UUID (or TLS client certificate file) a command-line
> > > parameter that qemu-system, qemu-img, and other tools accept (e.g.
> > > qemu-img via the --image-opts/--object syntax).
> > > 
> > > [Ketan]
> > > Sounds fair. Would it be ok to take this up after the driver is
> > > merged for the upcoming QEMU release?
> > 
> > I don't think we can merge code with known security flaws, particularly
> > if fixing these flaws will involve adding and/or changing command line
> > parameters for the block driver.
> >
> 
> We do support some protocols, such as gluster, that do not have robust
> authentication frameworks over tcp/ip.  Of course, these protocols have been
> in as a driver for several years (and, gluster does support unix sockets).

NB, gluster *does* have secure access control. It uses the verified x509
certificate identity as a token against which access control rules are
placed on volumes.

It isn't authentication in the traditional sense most people think of it,
but it does provide a secure authorization facility.

> We seem to be establishing a rule for QEMU, that is "no new protocol drivers
> without secure authentication".  That is a good thing. The existence of
> current protocol drivers that don't meet that criteria is potentially
> confusing for new contributors, however.  (As a side note to myself -- this
> is probably a good thing to add to the wiki, if it is not there already).

It's been my goal to fix / enhance everything in QEMU that uses network and
does not have secure encryption + access control facilities. eg by adding
TLS support to the NBD driver, and providing the secure mechanism for feeding
passwords into QEMU for things like curl, iscsi, etc. We're getting pretty
close to having at least the option to use encryption + access control via
TLS certs or SASL on every key network based feature in QEMU.

> I think a non-secure scheme is 

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

[Ketan Nilangekar]

On 2/21/17, 5:59 AM, "Stefan Hajnoczi"  wrote:

On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  
wrote:
> > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> >>
> >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> >> > Hi,
> >> >
> >> > I am getting the following error with checkpatch.pl
> >> >
> >> > ERROR: externs should be avoided in .c files
> >> > #78: FILE: block/vxhs.c:28:
> >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> >> >
> >> > Is there any way to get around this, or does it mean that I 
would have
> >> > to add a vxhs.h just for this one entry?
> >> >
> >>
> >> I remain skeptical on the use of the qemu_uuid as a way to select 
the TLS
> >> cert.
> >>
> >> [ketan]
> >> Is there another identity that can be used for uniquely identifying 
instances?
> >> The requirement was to enforce vdisk access to owner instances.
> >
> > The qemu_uuid weak attribute looks suspect.  What is going to provide a
> > strong qemu_uuid symbol?
> >
> > Why aren't configuration parameters like the UUID coming from the QEMU
> > command-line?
> >
> > Stefan
> 
> UUID will in fact come from the QEMU command line. VxHS is not doing
> anything special here. It will just use the value already available to
> qemu-kvm process.
> 
> QemuUUID qemu_uuid;
> bool qemu_uuid_set;
> 
> Both the above are defined in vl.c. vl.c will provide the strong
> symbol when available. There are certain binaries that do not get
> linked with vl.c (e.g. qemu-img). The weak symbol will come into
> affect for such binaries, and in this case, the default VXHS UUID will
> get picked up. I had, in a previous email, explained how we plan to
> use the default UUID. In the regular case, the VxHS controller will
> not allow access to the default UUID (non qemu-kvm) binaries, but it
> may choose to grant temporary access to specific vdisks for these
> binaries depending on the workflow.

That idea sounds like a security problem.  During this time window
anyone could use the default UUID to access the data?

Just make the UUID (or TLS client certificate file) a command-line
parameter that qemu-system, qemu-img, and other tools accept (e.g.
qemu-img via the --image-opts/--object syntax).

[Ketan]
Sounds fair. Would it be ok to take this up after the driver is merged for the 
upcoming QEMU release?

Stefan

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

[Jeff Cody]

On Tue, Feb 21, 2017 at 05:39:12PM +, Daniel P. Berrange wrote:
> On Tue, Feb 21, 2017 at 05:21:46PM +, Ketan Nilangekar wrote:
> > 
> > 
> > On 2/21/17, 5:59 AM, "Stefan Hajnoczi"  wrote:
> > 
> > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  
> > wrote:
> > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > > >>
> > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > > >> > Hi,
> > > >> >
> > > >> > I am getting the following error with checkpatch.pl
> > > >> >
> > > >> > ERROR: externs should be avoided in .c files
> > > >> > #78: FILE: block/vxhs.c:28:
> > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > > >> >
> > > >> > Is there any way to get around this, or does it mean that I 
> > would have
> > > >> > to add a vxhs.h just for this one entry?
> > > >> >
> > > >>
> > > >> I remain skeptical on the use of the qemu_uuid as a way to 
> > select the TLS
> > > >> cert.
> > > >>
> > > >> [ketan]
> > > >> Is there another identity that can be used for uniquely 
> > identifying instances?
> > > >> The requirement was to enforce vdisk access to owner instances.
> > > >
> > > > The qemu_uuid weak attribute looks suspect.  What is going to 
> > provide a
> > > > strong qemu_uuid symbol?
> > > >
> > > > Why aren't configuration parameters like the UUID coming from the 
> > QEMU
> > > > command-line?
> > > >
> > > > Stefan
> > > 
> > > UUID will in fact come from the QEMU command line. VxHS is not doing
> > > anything special here. It will just use the value already available to
> > > qemu-kvm process.
> > > 
> > > QemuUUID qemu_uuid;
> > > bool qemu_uuid_set;
> > > 
> > > Both the above are defined in vl.c. vl.c will provide the strong
> > > symbol when available. There are certain binaries that do not get
> > > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > > affect for such binaries, and in this case, the default VXHS UUID will
> > > get picked up. I had, in a previous email, explained how we plan to
> > > use the default UUID. In the regular case, the VxHS controller will
> > > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > > may choose to grant temporary access to specific vdisks for these
> > > binaries depending on the workflow.
> > 
> > That idea sounds like a security problem.  During this time window
> > anyone could use the default UUID to access the data?
> > 
> > Just make the UUID (or TLS client certificate file) a command-line
> > parameter that qemu-system, qemu-img, and other tools accept (e.g.
> > qemu-img via the --image-opts/--object syntax).
> > 
> > [Ketan]
> > Sounds fair. Would it be ok to take this up after the driver is
> > merged for the upcoming QEMU release?
> 
> I don't think we can merge code with known security flaws, particularly
> if fixing these flaws will involve adding and/or changing command line
> parameters for the block driver.
>

We do support some protocols, such as gluster, that do not have robust
authentication frameworks over tcp/ip.  Of course, these protocols have been
in as a driver for several years (and, gluster does support unix sockets).

We seem to be establishing a rule for QEMU, that is "no new protocol drivers
without secure authentication".  That is a good thing. The existence of
current protocol drivers that don't meet that criteria is potentially
confusing for new contributors, however.  (As a side note to myself -- this
is probably a good thing to add to the wiki, if it is not there already).

I think a non-secure scheme is worse than no scheme at all, because it
becomes relied upon and promises something it cannot deliver.  In that vein,
would you object to a vxhs protocol driver that did no authentication at all
(similar to gluster), or do you think the above rule is a new hard rule for
protocol drivers?


Jeff

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

[Daniel P. Berrange]

On Tue, Feb 21, 2017 at 05:21:46PM +, Ketan Nilangekar wrote:
> 
> 
> On 2/21/17, 5:59 AM, "Stefan Hajnoczi"  wrote:
> 
> On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  
> wrote:
> > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > >>
> > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > >> > Hi,
> > >> >
> > >> > I am getting the following error with checkpatch.pl
> > >> >
> > >> > ERROR: externs should be avoided in .c files
> > >> > #78: FILE: block/vxhs.c:28:
> > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > >> >
> > >> > Is there any way to get around this, or does it mean that I 
> would have
> > >> > to add a vxhs.h just for this one entry?
> > >> >
> > >>
> > >> I remain skeptical on the use of the qemu_uuid as a way to 
> select the TLS
> > >> cert.
> > >>
> > >> [ketan]
> > >> Is there another identity that can be used for uniquely identifying 
> instances?
> > >> The requirement was to enforce vdisk access to owner instances.
> > >
> > > The qemu_uuid weak attribute looks suspect.  What is going to provide 
> a
> > > strong qemu_uuid symbol?
> > >
> > > Why aren't configuration parameters like the UUID coming from the QEMU
> > > command-line?
> > >
> > > Stefan
> > 
> > UUID will in fact come from the QEMU command line. VxHS is not doing
> > anything special here. It will just use the value already available to
> > qemu-kvm process.
> > 
> > QemuUUID qemu_uuid;
> > bool qemu_uuid_set;
> > 
> > Both the above are defined in vl.c. vl.c will provide the strong
> > symbol when available. There are certain binaries that do not get
> > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > affect for such binaries, and in this case, the default VXHS UUID will
> > get picked up. I had, in a previous email, explained how we plan to
> > use the default UUID. In the regular case, the VxHS controller will
> > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > may choose to grant temporary access to specific vdisks for these
> > binaries depending on the workflow.
> 
> That idea sounds like a security problem.  During this time window
> anyone could use the default UUID to access the data?
> 
> Just make the UUID (or TLS client certificate file) a command-line
> parameter that qemu-system, qemu-img, and other tools accept (e.g.
> qemu-img via the --image-opts/--object syntax).
> 
> [Ketan]
> Sounds fair. Would it be ok to take this up after the driver is
> merged for the upcoming QEMU release?

I don't think we can merge code with known security flaws, particularly
if fixing these flaws will involve adding and/or changing command line
parameters for the block driver.

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

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

[Daniel P. Berrange]

On Tue, Feb 21, 2017 at 10:59:18AM +, Stefan Hajnoczi wrote:
> On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  wrote:
> > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> > >>
> > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > >> > Hi,
> > >> >
> > >> > I am getting the following error with checkpatch.pl
> > >> >
> > >> > ERROR: externs should be avoided in .c files
> > >> > #78: FILE: block/vxhs.c:28:
> > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > >> >
> > >> > Is there any way to get around this, or does it mean that I would 
> > >> have
> > >> > to add a vxhs.h just for this one entry?
> > >> >
> > >>
> > >> I remain skeptical on the use of the qemu_uuid as a way to select 
> > >> the TLS
> > >> cert.
> > >>
> > >> [ketan]
> > >> Is there another identity that can be used for uniquely identifying 
> > >> instances?
> > >> The requirement was to enforce vdisk access to owner instances.
> > >
> > > The qemu_uuid weak attribute looks suspect.  What is going to provide a
> > > strong qemu_uuid symbol?
> > >
> > > Why aren't configuration parameters like the UUID coming from the QEMU
> > > command-line?
> > >
> > > Stefan
> > 
> > UUID will in fact come from the QEMU command line. VxHS is not doing
> > anything special here. It will just use the value already available to
> > qemu-kvm process.
> > 
> > QemuUUID qemu_uuid;
> > bool qemu_uuid_set;
> > 
> > Both the above are defined in vl.c. vl.c will provide the strong
> > symbol when available. There are certain binaries that do not get
> > linked with vl.c (e.g. qemu-img). The weak symbol will come into
> > affect for such binaries, and in this case, the default VXHS UUID will
> > get picked up. I had, in a previous email, explained how we plan to
> > use the default UUID. In the regular case, the VxHS controller will
> > not allow access to the default UUID (non qemu-kvm) binaries, but it
> > may choose to grant temporary access to specific vdisks for these
> > binaries depending on the workflow.
> 
> That idea sounds like a security problem.  During this time window
> anyone could use the default UUID to access the data?

Any use of the VM UUID as a means to control authorization on the
server side is a security flaw, as this is a public value which
cannot be trusted on its own.

There needs to be some kind of authentication step to verify the
reported identity, eg a password associated with the VM UUID that
is validated before trusting the VM UUID.

Alternatively there needs to be a completely separate UUID, unrelated
to the VM UUID, which is treated as a private value (eg uses the
'-object secret' framework in QEMU)

> Just make the UUID (or TLS client certificate file) a command-line
> parameter that qemu-system, qemu-img, and other tools accept (e.g.
> qemu-img via the --image-opts/--object syntax).

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

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

[Stefan Hajnoczi]

On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
> On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi  wrote:
> > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> >>
> >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> >> > Hi,
> >> >
> >> > I am getting the following error with checkpatch.pl
> >> >
> >> > ERROR: externs should be avoided in .c files
> >> > #78: FILE: block/vxhs.c:28:
> >> > +QemuUUID qemu_uuid __attribute__ ((weak));
> >> >
> >> > Is there any way to get around this, or does it mean that I would 
> >> have
> >> > to add a vxhs.h just for this one entry?
> >> >
> >>
> >> I remain skeptical on the use of the qemu_uuid as a way to select the 
> >> TLS
> >> cert.
> >>
> >> [ketan]
> >> Is there another identity that can be used for uniquely identifying 
> >> instances?
> >> The requirement was to enforce vdisk access to owner instances.
> >
> > The qemu_uuid weak attribute looks suspect.  What is going to provide a
> > strong qemu_uuid symbol?
> >
> > Why aren't configuration parameters like the UUID coming from the QEMU
> > command-line?
> >
> > Stefan
> 
> UUID will in fact come from the QEMU command line. VxHS is not doing
> anything special here. It will just use the value already available to
> qemu-kvm process.
> 
> QemuUUID qemu_uuid;
> bool qemu_uuid_set;
> 
> Both the above are defined in vl.c. vl.c will provide the strong
> symbol when available. There are certain binaries that do not get
> linked with vl.c (e.g. qemu-img). The weak symbol will come into
> affect for such binaries, and in this case, the default VXHS UUID will
> get picked up. I had, in a previous email, explained how we plan to
> use the default UUID. In the regular case, the VxHS controller will
> not allow access to the default UUID (non qemu-kvm) binaries, but it
> may choose to grant temporary access to specific vdisks for these
> binaries depending on the workflow.

That idea sounds like a security problem.  During this time window
anyone could use the default UUID to access the data?

Just make the UUID (or TLS client certificate file) a command-line
parameter that qemu-system, qemu-img, and other tools accept (e.g.
qemu-img via the --image-opts/--object syntax).

Stefan


signature.asc
Description: PGP signature

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 20, 2017 at 3:02 AM, Stefan Hajnoczi  wrote:
> On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
>> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
>>
>> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
>> > Hi,
>> >
>> > I am getting the following error with checkpatch.pl
>> >
>> > ERROR: externs should be avoided in .c files
>> > #78: FILE: block/vxhs.c:28:
>> > +QemuUUID qemu_uuid __attribute__ ((weak));
>> >
>> > Is there any way to get around this, or does it mean that I would have
>> > to add a vxhs.h just for this one entry?
>> >
>>
>> I remain skeptical on the use of the qemu_uuid as a way to select the TLS
>> cert.
>>
>> [ketan]
>> Is there another identity that can be used for uniquely identifying 
>> instances?
>> The requirement was to enforce vdisk access to owner instances.
>
> The qemu_uuid weak attribute looks suspect.  What is going to provide a
> strong qemu_uuid symbol?
>
> Why aren't configuration parameters like the UUID coming from the QEMU
> command-line?
>
> Stefan

UUID will in fact come from the QEMU command line. VxHS is not doing
anything special here. It will just use the value already available to
qemu-kvm process.

QemuUUID qemu_uuid;
bool qemu_uuid_set;

Both the above are defined in vl.c. vl.c will provide the strong
symbol when available. There are certain binaries that do not get
linked with vl.c (e.g. qemu-img). The weak symbol will come into
affect for such binaries, and in this case, the default VXHS UUID will
get picked up. I had, in a previous email, explained how we plan to
use the default UUID. In the regular case, the VxHS controller will
not allow access to the default UUID (non qemu-kvm) binaries, but it
may choose to grant temporary access to specific vdisks for these
binaries depending on the workflow.

Regards,
Ashish

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

[Stefan Hajnoczi]

On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> 
> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > Hi,
> > 
> > I am getting the following error with checkpatch.pl
> > 
> > ERROR: externs should be avoided in .c files
> > #78: FILE: block/vxhs.c:28:
> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > 
> > Is there any way to get around this, or does it mean that I would have
> > to add a vxhs.h just for this one entry?
> >
> 
> I remain skeptical on the use of the qemu_uuid as a way to select the TLS
> cert.
> 
> [ketan]
> Is there another identity that can be used for uniquely identifying instances?
> The requirement was to enforce vdisk access to owner instances.

The qemu_uuid weak attribute looks suspect.  What is going to provide a
strong qemu_uuid symbol?

Why aren't configuration parameters like the UUID coming from the QEMU
command-line?

Stefan


signature.asc
Description: PGP signature

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

[Daniel P. Berrange]

On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
> 
> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > Hi,
> > 
> > I am getting the following error with checkpatch.pl
> > 
> > ERROR: externs should be avoided in .c files
> > #78: FILE: block/vxhs.c:28:
> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > 
> > Is there any way to get around this, or does it mean that I would have
> > to add a vxhs.h just for this one entry?
> >
> 
> I remain skeptical on the use of the qemu_uuid as a way to select the TLS
> cert.
> 
> [ketan]
> Is there another identity that can be used for uniquely identifying instances?
> The requirement was to enforce vdisk access to owner instances.

The UUID is a bad way to do any kind of access control as QEMU could simply
lie about its UUID.

If the server needs to identify the client to do access control you need
something non-spoofable. In the absence of having an authentication protocol
built into the libqnio protocol, the best you could do would be to use the
TLS client certificate distinguished name. QEMU can't lie about that without
having access to the other certificate file - which would be blocked by
SELinux

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

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

[Daniel P. Berrange]

On Fri, Feb 17, 2017 at 04:42:15PM -0500, Jeff Cody wrote:
> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> > Hi,
> > 
> > I am getting the following error with checkpatch.pl
> > 
> > ERROR: externs should be avoided in .c files
> > #78: FILE: block/vxhs.c:28:
> > +QemuUUID qemu_uuid __attribute__ ((weak));
> > 
> > Is there any way to get around this, or does it mean that I would have
> > to add a vxhs.h just for this one entry?
> >
> 
> I remain skeptical on the use of the qemu_uuid as a way to select the TLS
> cert.

Yes, we should not be hardcoding arbitrary path lookup policies like that
in QEMU. The libqnio API should allow QEMU to specify what paths it wants
to use for certs directly. That allows the admin the flexibility to decide
their own policy for where to put certs and the policy on which certs are
used for which purpose.

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

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

[Ketan Nilangekar]

On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:

On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> Hi,
> 
> I am getting the following error with checkpatch.pl
> 
> ERROR: externs should be avoided in .c files
> #78: FILE: block/vxhs.c:28:
> +QemuUUID qemu_uuid __attribute__ ((weak));
> 
> Is there any way to get around this, or does it mean that I would have
> to add a vxhs.h just for this one entry?
>

I remain skeptical on the use of the qemu_uuid as a way to select the TLS
cert.

[ketan]
Is there another identity that can be used for uniquely identifying instances?
The requirement was to enforce vdisk access to owner instances.


But that aside, and looking at it strictly from a checkpatch.pl point of
view, I think one could make a case that if there is no header file, the
error could be ignored.

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

[Jeff Cody]

On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
> Hi,
> 
> I am getting the following error with checkpatch.pl
> 
> ERROR: externs should be avoided in .c files
> #78: FILE: block/vxhs.c:28:
> +QemuUUID qemu_uuid __attribute__ ((weak));
> 
> Is there any way to get around this, or does it mean that I would have
> to add a vxhs.h just for this one entry?
>

I remain skeptical on the use of the qemu_uuid as a way to select the TLS
cert.

But that aside, and looking at it strictly from a checkpatch.pl point of
view, I think one could make a case that if there is no header file, the
error could be ignored.

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

[ashish mittal]

Hi,

I am getting the following error with checkpatch.pl

ERROR: externs should be avoided in .c files
#78: FILE: block/vxhs.c:28:
+QemuUUID qemu_uuid __attribute__ ((weak));

Is there any way to get around this, or does it mean that I would have
to add a vxhs.h just for this one entry?

Thanks,
Ashish

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

[ashish mittal]

On Tue, Feb 14, 2017 at 12:51 PM, Jeff Cody  wrote:
> On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
>> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
>> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
>> >> From: Ashish Mittal 
>> >>
>> >> Source code for the qnio library that this code loads can be downloaded 
>> >> from:
>> >> https://github.com/VeritasHyperScale/libqnio.git
>> >>
>> >> Sample command line using JSON syntax:
>> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
>> >> 0.0.0.0:0
>> >> -k en-us -vga cirrus -device 
>> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
>> >> -msg timestamp=on
>> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
>> >> "server":{"host":"172.172.17.4","port":""}}'
>> >>
>> >> Sample command line using URI syntax:
>> >> qemu-img convert -f raw -O raw -n
>> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
>> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>> >>
>
> [...]
>
>> >> +#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_UUID_DEF "12345678-1234-1234-1234-123456789012"
>> >
>> > What is this?  It is not a valid UUID; is the value significant?
>> >
>>
>> This value gets passed to libvxhs for binaries like qemu-io, qemu-img
>> that do not have an Instance ID. We can use this default ID to control
>> access to specific vdisks by these binaries. qemu-kvm will pass the
>> actual instance ID, and therefore will not use this default value.
>>
>> Will reply to other queries soon.
>>
>
> If you are going to call it a UUID, it should adhere to the RFC 4122 spec.
> You can easily generate a compliant UUID with uuidgen.  However:
>
> Can you explain more about how you are using this to control access by
> qemu-img and qemu-io?  Looking at libqnio, it looks like this is used to
> determine at runtime which TLS certs to use based off of a
> pathname/filename, which is then how I presume you are controlling access.
> See Daniel's email regarding TLS certificates.
>

(1) The default behavior would be to disallow access to any vdisks by
the non qemu-kvm binaries. qemu-kvm would use the actual instance ID
for authentication.
(2) Depending on the workflow, HyperScale controller can choose to
grant *temporary* access to specific vdisks by qemu-img, qemu-io
binaries (identified by the default VXHS_UUID_DEF above).
(3) This information, described in #2, would be communicated by the
HyperScale controller to the actual proprietary VxHS server (running
on each compute) that does the authentication/SSL.
(4) The HyperScale controller, in this way, can grant/revoke access
for specific vdisks not just to clients with VXHS_UUID_DEF instance
ID, but also the actual VM instances.

> [...]
>
>> >> +
>> >> +static void bdrv_vxhs_init(void)
>> >> +{
>> >> +char out[37];
>
> Additional point: this should be sized as UUID_FMT_LEN + 1, not 37, but I
> suspect this code is changing anyways.
>
>> >> +
>> >> +if (qemu_uuid_is_null(_uuid)) {
>> >> +lib_init_failed = iio_init(QNIO_VERSION, vxhs_iio_callback, 
>> >> VXHS_UUID_DEF);
>> >> +} else {
>> >> +qemu_uuid_unparse(_uuid, out);
>> >> +lib_init_failed = iio_init(QNIO_VERSION, vxhs_iio_callback, out);
>> >> +}
>> >> +
>> >
>> > [1]
>> >
>> > Can you explain what is going on here with the qemu_uuid check?
>> >

(1) qemu_uuid_is_null(_uuid) is true for qemu-io, qemu-img that
do not define a instance ID. We end up using the default VXHS_UUID_DEF
ID for them, and authenticating them as described above.

(2) For the other case 'else', we convert the uuid to a char * using
qemu_uuid_unparse(), and pass the resulting char * uuid in variable
'out' to libvxhs.

>> >
>> > You also can't do this here.  This init function is just to register the
>> > driver (e.g. populate the BlockDriver list).  You shouldn't be doing
>> > anything other than the bdrv_register() call here.
>> >
>> > Since you want to run this iio_init only once, I would recommend doing it 
>> > in
>> > the vxhs_open() call, and using a ref counter.  That way, you can also call
>> > iio_fini() to finish releasing resources once the last device is closed.
>> >
>> > This was actually a suggestion I had before, which you then incorporated
>> > into v6, but it appears all the refcnt code has gone away for v7/v8.
>> >
>> >
>> >> +bdrv_register(_vxhs);
>> >> +}
>> >> +

Will consider this in the next patch.

Regards,
Ashish

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

[Jeff Cody]

On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
> >> From: Ashish Mittal 
> >>
> >> Source code for the qnio library that this code loads can be downloaded 
> >> from:
> >> https://github.com/VeritasHyperScale/libqnio.git
> >>
> >> Sample command line using JSON syntax:
> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
> >> 0.0.0.0:0
> >> -k en-us -vga cirrus -device 
> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
> >> -msg timestamp=on
> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
> >> "server":{"host":"172.172.17.4","port":""}}'
> >>
> >> Sample command line using URI syntax:
> >> qemu-img convert -f raw -O raw -n
> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
> >>

[...]

> >> +#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_UUID_DEF "12345678-1234-1234-1234-123456789012"
> >
> > What is this?  It is not a valid UUID; is the value significant?
> >
> 
> This value gets passed to libvxhs for binaries like qemu-io, qemu-img
> that do not have an Instance ID. We can use this default ID to control
> access to specific vdisks by these binaries. qemu-kvm will pass the
> actual instance ID, and therefore will not use this default value.
> 
> Will reply to other queries soon.
>

If you are going to call it a UUID, it should adhere to the RFC 4122 spec.
You can easily generate a compliant UUID with uuidgen.  However:

Can you explain more about how you are using this to control access by
qemu-img and qemu-io?  Looking at libqnio, it looks like this is used to
determine at runtime which TLS certs to use based off of a
pathname/filename, which is then how I presume you are controlling access.
See Daniel's email regarding TLS certificates.

[...]

> >> +
> >> +static void bdrv_vxhs_init(void)
> >> +{
> >> +char out[37];

Additional point: this should be sized as UUID_FMT_LEN + 1, not 37, but I
suspect this code is changing anyways.

> >> +
> >> +if (qemu_uuid_is_null(_uuid)) {
> >> +lib_init_failed = iio_init(QNIO_VERSION, vxhs_iio_callback, 
> >> VXHS_UUID_DEF);
> >> +} else {
> >> +qemu_uuid_unparse(_uuid, out);
> >> +lib_init_failed = iio_init(QNIO_VERSION, vxhs_iio_callback, out);
> >> +}
> >> +
> >
> > [1]
> >
> > Can you explain what is going on here with the qemu_uuid check?
> >
> >
> > You also can't do this here.  This init function is just to register the
> > driver (e.g. populate the BlockDriver list).  You shouldn't be doing
> > anything other than the bdrv_register() call here.
> >
> > Since you want to run this iio_init only once, I would recommend doing it in
> > the vxhs_open() call, and using a ref counter.  That way, you can also call
> > iio_fini() to finish releasing resources once the last device is closed.
> >
> > This was actually a suggestion I had before, which you then incorporated
> > into v6, but it appears all the refcnt code has gone away for v7/v8.
> >
> >
> >> +bdrv_register(_vxhs);
> >> +}
> >> +

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

[Jeff Cody]

On Thu, Feb 09, 2017 at 04:27:07PM -0800, ashish mittal wrote:
> On Thu, Feb 9, 2017 at 10:45 AM, ashish mittal  wrote:
> > On Thu, Feb 9, 2017 at 10:08 AM, ashish mittal  wrote:
> >> On Thu, Feb 9, 2017 at 8:50 AM, Jeff Cody  wrote:
> >>> On Thu, Feb 09, 2017 at 08:14:38AM -0800, ashish mittal wrote:
>  On Thu, Feb 9, 2017 at 6:32 AM, Jeff Cody  wrote:
>  > On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
>  >> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
>  >> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
>  >> >> From: Ashish Mittal 
>  >> >>
>  >> >> Source code for the qnio library that this code loads can be 
>  >> >> downloaded from:
>  >> >> https://github.com/VeritasHyperScale/libqnio.git
>  >> >>
>  >> >> Sample command line using JSON syntax:
>  >> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S 
>  >> >> -vnc 0.0.0.0:0
>  >> >> -k en-us -vga cirrus -device 
>  >> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
>  >> >> -msg timestamp=on
>  >> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
>  >> >> "server":{"host":"172.172.17.4","port":""}}'
>  >> >>
>  >> >> Sample command line using URI syntax:
>  >> >> qemu-img convert -f raw -O raw -n
>  >> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
>  >> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>  >> >>
>  >> >
>  >> > I don't know if I am using the qnio_server test server correctly or 
>  >> > not, but
>  >> > when I run qemu-io from the command line I get an i/o error.  When 
>  >> > I run the
>  >> > qemu-iotests, I get a segfault.
>  >> >
>  >> > Were you able to run qemu-iotests with these patches?
>  >> >
>  >> > Here is how I am invoking qnio_server:
>  >> >
>  >> > # qnio_server  -d 
>  >> > /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch
>  >> >  -v
>  >> >
>  >> >
>  >>
>  >> I ran full qemu-iotests and qemu-io manually with the test server on
>  >> version 7 patches. Ran qemu-io manually with the test server on
>  >> version 8, but the libvxhs code is undergoing a lot of checkins. Will
>  >> test again tomorrow and get back.
>  >>
>  >
>  > Does my invocation above look correct, for running the qemu-iotests?
> 
>  qemu-iotest starts the server internally. The server does not have to
>  be started explicitly before running qemu-iotest. The server must not
>  be running before you begin the test  because then the tests would
>  find port  busy.
>  Running make install on libvxhs copies the test server to the location
>  where the qemu-iotests expect to find it.
> >>>
> >>>
> >>> OK, thanks.  I tried that too, and I also tried against the branch
> >>> "ashish_securify_changes" for libqnio.  I still have qemu-iotests giving 
> >>> me
> >>> a segfault.
> >>>
> >>>
> >>
> >> Qemu patch v7 and v8 do not have many changes. I did successfully run
> >> iotests with v7 and libqnio master. I'm guessing there could be
> >> something in the 'securify' library branch that's causing this. I will
> >> rebuild and retest.
> >
> > Confirmed that there is a problem and qemu-iotest is segfaulting. Will
> > get back with a fix.
> 
> Checked in some changes to libqnio. Could you please try it again with
> the latest 'securify' branch?

Thanks - I just tried it, and can confirm that all 26 tests pass with raw:

# ./check -vxhs -raw

[...]

Passed all 26 tests


-Jeff

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

[ashish mittal]

On Thu, Feb 9, 2017 at 10:45 AM, ashish mittal  wrote:
> On Thu, Feb 9, 2017 at 10:08 AM, ashish mittal  wrote:
>> On Thu, Feb 9, 2017 at 8:50 AM, Jeff Cody  wrote:
>>> On Thu, Feb 09, 2017 at 08:14:38AM -0800, ashish mittal wrote:
 On Thu, Feb 9, 2017 at 6:32 AM, Jeff Cody  wrote:
 > On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
 >> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
 >> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
 >> >> From: Ashish Mittal 
 >> >>
 >> >> Source code for the qnio library that this code loads can be 
 >> >> downloaded from:
 >> >> https://github.com/VeritasHyperScale/libqnio.git
 >> >>
 >> >> Sample command line using JSON syntax:
 >> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
 >> >> 0.0.0.0:0
 >> >> -k en-us -vga cirrus -device 
 >> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
 >> >> -msg timestamp=on
 >> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
 >> >> "server":{"host":"172.172.17.4","port":""}}'
 >> >>
 >> >> Sample command line using URI syntax:
 >> >> qemu-img convert -f raw -O raw -n
 >> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
 >> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
 >> >>
 >> >
 >> > I don't know if I am using the qnio_server test server correctly or 
 >> > not, but
 >> > when I run qemu-io from the command line I get an i/o error.  When I 
 >> > run the
 >> > qemu-iotests, I get a segfault.
 >> >
 >> > Were you able to run qemu-iotests with these patches?
 >> >
 >> > Here is how I am invoking qnio_server:
 >> >
 >> > # qnio_server  -d 
 >> > /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch 
 >> > -v
 >> >
 >> >
 >>
 >> I ran full qemu-iotests and qemu-io manually with the test server on
 >> version 7 patches. Ran qemu-io manually with the test server on
 >> version 8, but the libvxhs code is undergoing a lot of checkins. Will
 >> test again tomorrow and get back.
 >>
 >
 > Does my invocation above look correct, for running the qemu-iotests?

 qemu-iotest starts the server internally. The server does not have to
 be started explicitly before running qemu-iotest. The server must not
 be running before you begin the test  because then the tests would
 find port  busy.
 Running make install on libvxhs copies the test server to the location
 where the qemu-iotests expect to find it.
>>>
>>>
>>> OK, thanks.  I tried that too, and I also tried against the branch
>>> "ashish_securify_changes" for libqnio.  I still have qemu-iotests giving me
>>> a segfault.
>>>
>>>
>>
>> Qemu patch v7 and v8 do not have many changes. I did successfully run
>> iotests with v7 and libqnio master. I'm guessing there could be
>> something in the 'securify' library branch that's causing this. I will
>> rebuild and retest.
>
> Confirmed that there is a problem and qemu-iotest is segfaulting. Will
> get back with a fix.

Checked in some changes to libqnio. Could you please try it again with
the latest 'securify' branch?
Thanks!

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

[ashish mittal]

On Thu, Feb 9, 2017 at 10:08 AM, ashish mittal  wrote:
> On Thu, Feb 9, 2017 at 8:50 AM, Jeff Cody  wrote:
>> On Thu, Feb 09, 2017 at 08:14:38AM -0800, ashish mittal wrote:
>>> On Thu, Feb 9, 2017 at 6:32 AM, Jeff Cody  wrote:
>>> > On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
>>> >> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
>>> >> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
>>> >> >> From: Ashish Mittal 
>>> >> >>
>>> >> >> Source code for the qnio library that this code loads can be 
>>> >> >> downloaded from:
>>> >> >> https://github.com/VeritasHyperScale/libqnio.git
>>> >> >>
>>> >> >> Sample command line using JSON syntax:
>>> >> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
>>> >> >> 0.0.0.0:0
>>> >> >> -k en-us -vga cirrus -device 
>>> >> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
>>> >> >> -msg timestamp=on
>>> >> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
>>> >> >> "server":{"host":"172.172.17.4","port":""}}'
>>> >> >>
>>> >> >> Sample command line using URI syntax:
>>> >> >> qemu-img convert -f raw -O raw -n
>>> >> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
>>> >> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>>> >> >>
>>> >> >
>>> >> > I don't know if I am using the qnio_server test server correctly or 
>>> >> > not, but
>>> >> > when I run qemu-io from the command line I get an i/o error.  When I 
>>> >> > run the
>>> >> > qemu-iotests, I get a segfault.
>>> >> >
>>> >> > Were you able to run qemu-iotests with these patches?
>>> >> >
>>> >> > Here is how I am invoking qnio_server:
>>> >> >
>>> >> > # qnio_server  -d 
>>> >> > /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch -v
>>> >> >
>>> >> >
>>> >>
>>> >> I ran full qemu-iotests and qemu-io manually with the test server on
>>> >> version 7 patches. Ran qemu-io manually with the test server on
>>> >> version 8, but the libvxhs code is undergoing a lot of checkins. Will
>>> >> test again tomorrow and get back.
>>> >>
>>> >
>>> > Does my invocation above look correct, for running the qemu-iotests?
>>>
>>> qemu-iotest starts the server internally. The server does not have to
>>> be started explicitly before running qemu-iotest. The server must not
>>> be running before you begin the test  because then the tests would
>>> find port  busy.
>>> Running make install on libvxhs copies the test server to the location
>>> where the qemu-iotests expect to find it.
>>
>>
>> OK, thanks.  I tried that too, and I also tried against the branch
>> "ashish_securify_changes" for libqnio.  I still have qemu-iotests giving me
>> a segfault.
>>
>>
>
> Qemu patch v7 and v8 do not have many changes. I did successfully run
> iotests with v7 and libqnio master. I'm guessing there could be
> something in the 'securify' library branch that's causing this. I will
> rebuild and retest.

Confirmed that there is a problem and qemu-iotest is segfaulting. Will
get back with a fix.

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

[ashish mittal]

On Thu, Feb 9, 2017 at 8:50 AM, Jeff Cody  wrote:
> On Thu, Feb 09, 2017 at 08:14:38AM -0800, ashish mittal wrote:
>> On Thu, Feb 9, 2017 at 6:32 AM, Jeff Cody  wrote:
>> > On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
>> >> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
>> >> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
>> >> >> From: Ashish Mittal 
>> >> >>
>> >> >> Source code for the qnio library that this code loads can be 
>> >> >> downloaded from:
>> >> >> https://github.com/VeritasHyperScale/libqnio.git
>> >> >>
>> >> >> Sample command line using JSON syntax:
>> >> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
>> >> >> 0.0.0.0:0
>> >> >> -k en-us -vga cirrus -device 
>> >> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
>> >> >> -msg timestamp=on
>> >> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
>> >> >> "server":{"host":"172.172.17.4","port":""}}'
>> >> >>
>> >> >> Sample command line using URI syntax:
>> >> >> qemu-img convert -f raw -O raw -n
>> >> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
>> >> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>> >> >>
>> >> >
>> >> > I don't know if I am using the qnio_server test server correctly or 
>> >> > not, but
>> >> > when I run qemu-io from the command line I get an i/o error.  When I 
>> >> > run the
>> >> > qemu-iotests, I get a segfault.
>> >> >
>> >> > Were you able to run qemu-iotests with these patches?
>> >> >
>> >> > Here is how I am invoking qnio_server:
>> >> >
>> >> > # qnio_server  -d 
>> >> > /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch -v
>> >> >
>> >> >
>> >>
>> >> I ran full qemu-iotests and qemu-io manually with the test server on
>> >> version 7 patches. Ran qemu-io manually with the test server on
>> >> version 8, but the libvxhs code is undergoing a lot of checkins. Will
>> >> test again tomorrow and get back.
>> >>
>> >
>> > Does my invocation above look correct, for running the qemu-iotests?
>>
>> qemu-iotest starts the server internally. The server does not have to
>> be started explicitly before running qemu-iotest. The server must not
>> be running before you begin the test  because then the tests would
>> find port  busy.
>> Running make install on libvxhs copies the test server to the location
>> where the qemu-iotests expect to find it.
>
>
> OK, thanks.  I tried that too, and I also tried against the branch
> "ashish_securify_changes" for libqnio.  I still have qemu-iotests giving me
> a segfault.
>
>

Qemu patch v7 and v8 do not have many changes. I did successfully run
iotests with v7 and libqnio master. I'm guessing there could be
something in the 'securify' library branch that's causing this. I will
rebuild and retest.

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

[Jeff Cody]

On Thu, Feb 09, 2017 at 08:14:38AM -0800, ashish mittal wrote:
> On Thu, Feb 9, 2017 at 6:32 AM, Jeff Cody  wrote:
> > On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
> >> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
> >> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
> >> >> From: Ashish Mittal 
> >> >>
> >> >> Source code for the qnio library that this code loads can be downloaded 
> >> >> from:
> >> >> https://github.com/VeritasHyperScale/libqnio.git
> >> >>
> >> >> Sample command line using JSON syntax:
> >> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
> >> >> 0.0.0.0:0
> >> >> -k en-us -vga cirrus -device 
> >> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
> >> >> -msg timestamp=on
> >> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
> >> >> "server":{"host":"172.172.17.4","port":""}}'
> >> >>
> >> >> Sample command line using URI syntax:
> >> >> qemu-img convert -f raw -O raw -n
> >> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
> >> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
> >> >>
> >> >
> >> > I don't know if I am using the qnio_server test server correctly or not, 
> >> > but
> >> > when I run qemu-io from the command line I get an i/o error.  When I run 
> >> > the
> >> > qemu-iotests, I get a segfault.
> >> >
> >> > Were you able to run qemu-iotests with these patches?
> >> >
> >> > Here is how I am invoking qnio_server:
> >> >
> >> > # qnio_server  -d 
> >> > /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch -v
> >> >
> >> >
> >>
> >> I ran full qemu-iotests and qemu-io manually with the test server on
> >> version 7 patches. Ran qemu-io manually with the test server on
> >> version 8, but the libvxhs code is undergoing a lot of checkins. Will
> >> test again tomorrow and get back.
> >>
> >
> > Does my invocation above look correct, for running the qemu-iotests?
> 
> qemu-iotest starts the server internally. The server does not have to
> be started explicitly before running qemu-iotest. The server must not
> be running before you begin the test  because then the tests would
> find port  busy.
> Running make install on libvxhs copies the test server to the location
> where the qemu-iotests expect to find it.


OK, thanks.  I tried that too, and I also tried against the branch
"ashish_securify_changes" for libqnio.  I still have qemu-iotests giving me
a segfault.

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

[ashish mittal]

On Thu, Feb 9, 2017 at 6:32 AM, Jeff Cody  wrote:
> On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
>> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
>> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
>> >> From: Ashish Mittal 
>> >>
>> >> Source code for the qnio library that this code loads can be downloaded 
>> >> from:
>> >> https://github.com/VeritasHyperScale/libqnio.git
>> >>
>> >> Sample command line using JSON syntax:
>> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
>> >> 0.0.0.0:0
>> >> -k en-us -vga cirrus -device 
>> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
>> >> -msg timestamp=on
>> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
>> >> "server":{"host":"172.172.17.4","port":""}}'
>> >>
>> >> Sample command line using URI syntax:
>> >> qemu-img convert -f raw -O raw -n
>> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
>> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>> >>
>> >
>> > I don't know if I am using the qnio_server test server correctly or not, 
>> > but
>> > when I run qemu-io from the command line I get an i/o error.  When I run 
>> > the
>> > qemu-iotests, I get a segfault.
>> >
>> > Were you able to run qemu-iotests with these patches?
>> >
>> > Here is how I am invoking qnio_server:
>> >
>> > # qnio_server  -d 
>> > /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch -v
>> >
>> >
>>
>> I ran full qemu-iotests and qemu-io manually with the test server on
>> version 7 patches. Ran qemu-io manually with the test server on
>> version 8, but the libvxhs code is undergoing a lot of checkins. Will
>> test again tomorrow and get back.
>>
>
> Does my invocation above look correct, for running the qemu-iotests?

qemu-iotest starts the server internally. The server does not have to
be started explicitly before running qemu-iotest. The server must not
be running before you begin the test  because then the tests would
find port  busy.
Running make install on libvxhs copies the test server to the location
where the qemu-iotests expect to find it.

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

[Jeff Cody]

On Thu, Feb 09, 2017 at 01:24:58AM -0800, ashish mittal wrote:
> On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
> > On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
> >> From: Ashish Mittal 
> >>
> >> Source code for the qnio library that this code loads can be downloaded 
> >> from:
> >> https://github.com/VeritasHyperScale/libqnio.git
> >>
> >> Sample command line using JSON syntax:
> >> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 
> >> 0.0.0.0:0
> >> -k en-us -vga cirrus -device 
> >> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
> >> -msg timestamp=on
> >> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
> >> "server":{"host":"172.172.17.4","port":""}}'
> >>
> >> Sample command line using URI syntax:
> >> qemu-img convert -f raw -O raw -n
> >> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
> >> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
> >>
> >
> > I don't know if I am using the qnio_server test server correctly or not, but
> > when I run qemu-io from the command line I get an i/o error.  When I run the
> > qemu-iotests, I get a segfault.
> >
> > Were you able to run qemu-iotests with these patches?
> >
> > Here is how I am invoking qnio_server:
> >
> > # qnio_server  -d 
> > /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch -v
> >
> >
> 
> I ran full qemu-iotests and qemu-io manually with the test server on
> version 7 patches. Ran qemu-io manually with the test server on
> version 8, but the libvxhs code is undergoing a lot of checkins. Will
> test again tomorrow and get back.
>

Does my invocation above look correct, for running the qemu-iotests?

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

[ashish mittal]

On Wed, Feb 8, 2017 at 10:29 PM, Jeff Cody  wrote:
> On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
>> From: Ashish Mittal 
>>
>> Source code for the qnio library that this code loads can be downloaded from:
>> https://github.com/VeritasHyperScale/libqnio.git
>>
>> Sample command line using JSON syntax:
>> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 0.0.0.0:0
>> -k en-us -vga cirrus -device 
>> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
>> -msg timestamp=on
>> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
>> "server":{"host":"172.172.17.4","port":""}}'
>>
>> Sample command line using URI syntax:
>> qemu-img convert -f raw -O raw -n
>> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
>> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>>
>
> I don't know if I am using the qnio_server test server correctly or not, but
> when I run qemu-io from the command line I get an i/o error.  When I run the
> qemu-iotests, I get a segfault.
>
> Were you able to run qemu-iotests with these patches?
>
> Here is how I am invoking qnio_server:
>
> # qnio_server  -d 
> /home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch -v
>
>

I ran full qemu-iotests and qemu-io manually with the test server on
version 7 patches. Ran qemu-io manually with the test server on
version 8, but the libvxhs code is undergoing a lot of checkins. Will
test again tomorrow and get back.

>
>> Signed-off-by: Ashish Mittal 
>> ---
>> TODO:
>> (1) valgrind report to follow soon.
>>
>> v8 changelog:
>> (1) Security implementation for libqnio present in branch 'securify'.
>> Please use 'securify' branch for building libqnio and testing
>> with this patch.
>> (2) Renamed libqnio to libvxhs.
>> (3) Pass instance ID to libvxhs for SSL authentication.
>>
>> v7 changelog:
>> (1) IO failover code has moved out to the libqnio library.
>> (2) Fixes for issues reported by Stefan on v6.
>> (3) Incorporated the QEMUBH patch provided by Stefan.
>> This is a replacement for the pipe mechanism used earlier.
>> (4) Fixes to the buffer overflows reported in libqnio.
>> (5) Input validations in vxhs.c to prevent any buffer overflows for
>> arguments passed to libqnio.
>>
>> v6 changelog:
>> (1) Added qemu-iotests for VxHS as a new patch in the series.
>> (2) Replaced release version from 2.8 to 2.9 in block-core.json.
>>
>> v5 changelog:
>> (1) Incorporated v4 review comments.
>>
>> v4 changelog:
>> (1) Incorporated v3 review comments on QAPI changes.
>> (2) Added refcounting for device open/close.
>> Free library resources on last device close.
>>
>> v3 changelog:
>> (1) Added QAPI schema for the VxHS driver.
>>
>> v2 changelog:
>> (1) Changes done in response to v1 comments.
>>
>>  block/Makefile.objs  |   2 +
>>  block/trace-events   |  16 ++
>>  block/vxhs.c | 499 
>> +++
>>  configure|  41 +
>>  qapi/block-core.json |  20 ++-
>>  5 files changed, 576 insertions(+), 2 deletions(-)
>>  create mode 100644 block/vxhs.c
>>
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index c6bd14e..75675b4 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -19,6 +19,7 @@ block-obj-$(CONFIG_LIBNFS) += nfs.o
>>  block-obj-$(CONFIG_CURL) += curl.o
>>  block-obj-$(CONFIG_RBD) += rbd.o
>>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
>> +block-obj-$(CONFIG_VXHS) += vxhs.o
>>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
>>  block-obj-$(CONFIG_LIBSSH2) += ssh.o
>>  block-obj-y += accounting.o dirty-bitmap.o
>> @@ -39,6 +40,7 @@ rbd.o-cflags   := $(RBD_CFLAGS)
>>  rbd.o-libs := $(RBD_LIBS)
>>  gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
>>  gluster.o-libs := $(GLUSTERFS_LIBS)
>> +vxhs.o-libs:= $(VXHS_LIBS)
>>  ssh.o-cflags   := $(LIBSSH2_CFLAGS)
>>  ssh.o-libs := $(LIBSSH2_LIBS)
>>  archipelago.o-libs := $(ARCHIPELAGO_LIBS)
>> diff --git a/block/trace-events b/block/trace-events
>> index 0bc5c0a..d5eca93 100644
>> --- a/block/trace-events
>> +++ b/block/trace-events
>> @@ -110,3 +110,19 @@ qed_aio_write_data(void *s, void *acb, int ret, 
>> uint64_t offset, size_t len) "s
>>  qed_aio_write_prefill(void *s, void *acb, uint64_t start, size_t len, 
>> uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
>>  qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, 
>> uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
>>  qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t 
>> len) "s %p acb %p ret %d offset %"PRIu64" len %zu"
>> +
>> +# block/vxhs.c
>> +vxhs_iio_callback(int error) "ctx is NULL: error %d"
>> +vxhs_iio_callback_chnfail(int err, int error) "QNIO channel failed, no i/o 
>> %d, %d"
>> +vxhs_iio_callback_unknwn(int opcode, int err) "unexpected opcode %d, 

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

[Jeff Cody]

On Wed, Feb 08, 2017 at 09:23:33PM -0800, Ashish Mittal wrote:
> From: Ashish Mittal 
> 
> Source code for the qnio library that this code loads can be downloaded from:
> https://github.com/VeritasHyperScale/libqnio.git
> 
> Sample command line using JSON syntax:
> ./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 0.0.0.0:0
> -k en-us -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
> -msg timestamp=on
> 'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
> "server":{"host":"172.172.17.4","port":""}}'
> 
> Sample command line using URI syntax:
> qemu-img convert -f raw -O raw -n
> /var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
> vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0
>

I don't know if I am using the qnio_server test server correctly or not, but
when I run qemu-io from the command line I get an i/o error.  When I run the
qemu-iotests, I get a segfault.

Were you able to run qemu-iotests with these patches?

Here is how I am invoking qnio_server:

# qnio_server  -d 
/home/jcody/work/redhat/upstream/qemu-kvm/tests/qemu-iotests/scratch -v



> Signed-off-by: Ashish Mittal 
> ---
> TODO:
> (1) valgrind report to follow soon.
> 
> v8 changelog:
> (1) Security implementation for libqnio present in branch 'securify'.
> Please use 'securify' branch for building libqnio and testing
> with this patch.
> (2) Renamed libqnio to libvxhs.
> (3) Pass instance ID to libvxhs for SSL authentication.
> 
> v7 changelog:
> (1) IO failover code has moved out to the libqnio library.
> (2) Fixes for issues reported by Stefan on v6.
> (3) Incorporated the QEMUBH patch provided by Stefan.
> This is a replacement for the pipe mechanism used earlier.
> (4) Fixes to the buffer overflows reported in libqnio.
> (5) Input validations in vxhs.c to prevent any buffer overflows for 
> arguments passed to libqnio.
> 
> v6 changelog:
> (1) Added qemu-iotests for VxHS as a new patch in the series.
> (2) Replaced release version from 2.8 to 2.9 in block-core.json.
> 
> v5 changelog:
> (1) Incorporated v4 review comments.
> 
> v4 changelog:
> (1) Incorporated v3 review comments on QAPI changes.
> (2) Added refcounting for device open/close.
> Free library resources on last device close.
> 
> v3 changelog:
> (1) Added QAPI schema for the VxHS driver.
> 
> v2 changelog:
> (1) Changes done in response to v1 comments.
> 
>  block/Makefile.objs  |   2 +
>  block/trace-events   |  16 ++
>  block/vxhs.c | 499 
> +++
>  configure|  41 +
>  qapi/block-core.json |  20 ++-
>  5 files changed, 576 insertions(+), 2 deletions(-)
>  create mode 100644 block/vxhs.c
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index c6bd14e..75675b4 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -19,6 +19,7 @@ block-obj-$(CONFIG_LIBNFS) += nfs.o
>  block-obj-$(CONFIG_CURL) += curl.o
>  block-obj-$(CONFIG_RBD) += rbd.o
>  block-obj-$(CONFIG_GLUSTERFS) += gluster.o
> +block-obj-$(CONFIG_VXHS) += vxhs.o
>  block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
>  block-obj-$(CONFIG_LIBSSH2) += ssh.o
>  block-obj-y += accounting.o dirty-bitmap.o
> @@ -39,6 +40,7 @@ rbd.o-cflags   := $(RBD_CFLAGS)
>  rbd.o-libs := $(RBD_LIBS)
>  gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
>  gluster.o-libs := $(GLUSTERFS_LIBS)
> +vxhs.o-libs:= $(VXHS_LIBS)
>  ssh.o-cflags   := $(LIBSSH2_CFLAGS)
>  ssh.o-libs := $(LIBSSH2_LIBS)
>  archipelago.o-libs := $(ARCHIPELAGO_LIBS)
> diff --git a/block/trace-events b/block/trace-events
> index 0bc5c0a..d5eca93 100644
> --- a/block/trace-events
> +++ b/block/trace-events
> @@ -110,3 +110,19 @@ qed_aio_write_data(void *s, void *acb, int ret, uint64_t 
> offset, size_t len) "s
>  qed_aio_write_prefill(void *s, void *acb, uint64_t start, size_t len, 
> uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
>  qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, 
> uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
>  qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) 
> "s %p acb %p ret %d offset %"PRIu64" len %zu"
> +
> +# block/vxhs.c
> +vxhs_iio_callback(int error) "ctx is NULL: error %d"
> +vxhs_iio_callback_chnfail(int err, int error) "QNIO channel failed, no i/o 
> %d, %d"
> +vxhs_iio_callback_unknwn(int opcode, int err) "unexpected opcode %d, errno 
> %d"
> +vxhs_aio_rw_invalid(int req) "Invalid I/O request iodir %d"
> +vxhs_aio_rw_ioerr(char *guid, int iodir, uint64_t size, uint64_t off, void 
> *acb, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d size = %lu 
> offset = %lu ACB = %p. Error = %d, errno = %d"
> +vxhs_get_vdisk_stat_err(char *guid, int ret, int err) "vDisk (%s) stat ioctl 
> failed, ret = %d, errno = %d"
> +vxhs_get_vdisk_stat(char 

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

[Ashish Mittal]

From: Ashish Mittal 

Source code for the qnio library that this code loads can be downloaded from:
https://github.com/VeritasHyperScale/libqnio.git

Sample command line using JSON syntax:
./x86_64-softmmu/qemu-system-x86_64 -name instance-0008 -S -vnc 0.0.0.0:0
-k en-us -vga cirrus -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x5
-msg timestamp=on
'json:{"driver":"vxhs","vdisk-id":"c3e9095a-a5ee-4dce-afeb-2a59fb387410",
"server":{"host":"172.172.17.4","port":""}}'

Sample command line using URI syntax:
qemu-img convert -f raw -O raw -n
/var/lib/nova/instances/_base/0c5eacd5ebea5ed914b6a3e7b18f1ce734c386ad
vxhs://192.168.0.1:/c6718f6b-0401-441d-a8c3-1f0064d75ee0

Signed-off-by: Ashish Mittal 
---
TODO:
(1) valgrind report to follow soon.

v8 changelog:
(1) Security implementation for libqnio present in branch 'securify'.
Please use 'securify' branch for building libqnio and testing
with this patch.
(2) Renamed libqnio to libvxhs.
(3) Pass instance ID to libvxhs for SSL authentication.

v7 changelog:
(1) IO failover code has moved out to the libqnio library.
(2) Fixes for issues reported by Stefan on v6.
(3) Incorporated the QEMUBH patch provided by Stefan.
This is a replacement for the pipe mechanism used earlier.
(4) Fixes to the buffer overflows reported in libqnio.
(5) Input validations in vxhs.c to prevent any buffer overflows for 
arguments passed to libqnio.

v6 changelog:
(1) Added qemu-iotests for VxHS as a new patch in the series.
(2) Replaced release version from 2.8 to 2.9 in block-core.json.

v5 changelog:
(1) Incorporated v4 review comments.

v4 changelog:
(1) Incorporated v3 review comments on QAPI changes.
(2) Added refcounting for device open/close.
Free library resources on last device close.

v3 changelog:
(1) Added QAPI schema for the VxHS driver.

v2 changelog:
(1) Changes done in response to v1 comments.

 block/Makefile.objs  |   2 +
 block/trace-events   |  16 ++
 block/vxhs.c | 499 +++
 configure|  41 +
 qapi/block-core.json |  20 ++-
 5 files changed, 576 insertions(+), 2 deletions(-)
 create mode 100644 block/vxhs.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index c6bd14e..75675b4 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -19,6 +19,7 @@ block-obj-$(CONFIG_LIBNFS) += nfs.o
 block-obj-$(CONFIG_CURL) += curl.o
 block-obj-$(CONFIG_RBD) += rbd.o
 block-obj-$(CONFIG_GLUSTERFS) += gluster.o
+block-obj-$(CONFIG_VXHS) += vxhs.o
 block-obj-$(CONFIG_ARCHIPELAGO) += archipelago.o
 block-obj-$(CONFIG_LIBSSH2) += ssh.o
 block-obj-y += accounting.o dirty-bitmap.o
@@ -39,6 +40,7 @@ rbd.o-cflags   := $(RBD_CFLAGS)
 rbd.o-libs := $(RBD_LIBS)
 gluster.o-cflags   := $(GLUSTERFS_CFLAGS)
 gluster.o-libs := $(GLUSTERFS_LIBS)
+vxhs.o-libs:= $(VXHS_LIBS)
 ssh.o-cflags   := $(LIBSSH2_CFLAGS)
 ssh.o-libs := $(LIBSSH2_LIBS)
 archipelago.o-libs := $(ARCHIPELAGO_LIBS)
diff --git a/block/trace-events b/block/trace-events
index 0bc5c0a..d5eca93 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -110,3 +110,19 @@ qed_aio_write_data(void *s, void *acb, int ret, uint64_t 
offset, size_t len) "s
 qed_aio_write_prefill(void *s, void *acb, uint64_t start, size_t len, uint64_t 
offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
 qed_aio_write_postfill(void *s, void *acb, uint64_t start, size_t len, 
uint64_t offset) "s %p acb %p start %"PRIu64" len %zu offset %"PRIu64
 qed_aio_write_main(void *s, void *acb, int ret, uint64_t offset, size_t len) 
"s %p acb %p ret %d offset %"PRIu64" len %zu"
+
+# block/vxhs.c
+vxhs_iio_callback(int error) "ctx is NULL: error %d"
+vxhs_iio_callback_chnfail(int err, int error) "QNIO channel failed, no i/o %d, 
%d"
+vxhs_iio_callback_unknwn(int opcode, int err) "unexpected opcode %d, errno %d"
+vxhs_aio_rw_invalid(int req) "Invalid I/O request iodir %d"
+vxhs_aio_rw_ioerr(char *guid, int iodir, uint64_t size, uint64_t off, void 
*acb, int ret, int err) "IO ERROR (vDisk %s) FOR : Read/Write = %d size = %lu 
offset = %lu ACB = %p. Error = %d, errno = %d"
+vxhs_get_vdisk_stat_err(char *guid, int ret, int err) "vDisk (%s) stat ioctl 
failed, ret = %d, errno = %d"
+vxhs_get_vdisk_stat(char *vdisk_guid, uint64_t vdisk_size) "vDisk %s stat 
ioctl returned size %lu"
+vxhs_qnio_iio_open(const char *ip) "Failed to connect to storage agent on 
host-ip %s"
+vxhs_complete_aio(void *acb, uint64_t ret) "aio failed acb %p ret %ld"
+vxhs_parse_uri_filename(const char *filename) "URI passed via 
bdrv_parse_filename %s"
+vxhs_qemu_init_vdisk(const char *vdisk_id) "vdisk-id from json %s"
+vxhs_parse_uri_hostinfo(int num, char *host, int port) "Host %d: IP %s, Port 
%d"
+vxhs_qemu_init(char *of_vsa_addr, int port) "Adding host %s:%d to 
BDRVVXHSState"
+vxhs_close(char *vdisk_guid) "Closing vdisk %s"
diff --git a/block/vxhs.c b/block/vxhs.c
new file mode