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 +0000, Daniel P. Berrange wrote:
> On Wed, Feb 22, 2017 at 02:09:20PM +0000, Stefan Hajnoczi wrote:
> > On Tue, Feb 21, 2017 at 11:33:53AM +0000, Daniel P. Berrange wrote:
> > > On Tue, Feb 21, 2017 at 10:59:18AM +0000, 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 <address@hidden> 
> > > > > wrote:
> > > > > > On Sat, Feb 18, 2017 at 12:30:31AM +0000, Ketan Nilangekar wrote:
> > > > > >> On 2/17/17, 1:42 PM, "Jeff Cody" <address@hidden> 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:

https://github.com/VeritasHyperScale/libqnio/blob/securify/src/lib/qnio/utils.c#L81

That instanceid is the UUID passed in during the initial call to iio_init().

Also, does QEMU make any promises about qemu_uuid either being 0 or
undefined for qemu-img and qemu-io in the future?  If that assumption
changes in the future, it would also break the scheme in the these patches.

-Jeff