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 +0000, Daniel P. Berrange wrote:
> > On Tue, Feb 21, 2017 at 05:21:46PM +0000, Ketan Nilangekar wrote:
> > > 
> > > 
> > > On 2/21/17, 5:59 AM, "Stefan Hajnoczi" <address@hidden> 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?
> > >     
> > >     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 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?

Adding support for a known insecure authentication scheme is a clear
no-go as that's pretty much immediate CVE terrority when we release it,
as you give people the illusion of security where none exists.

IMHO, any new network protocol that we add to QEMU should at least be
capable of having secure encryption & access control enabled, unless it
is a long term pre-existing standard - even those have pretty much all
been given strong security extensions over the years as it became clear
that internal networks are often just as hostile as public networks.
eg the work we did to add TLS to VNC in 2007, or more recently adding
TLS to NBD. SPICE by constrast as a modern protocol had TLS right from
the start.

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