Re: [Qemu-block] [PATCH v2 12/17] qcow2: convert QCow2 to use QCryptoBlock for encryption

[Daniel P. Berrange]

On Thu, Jan 21, 2016 at 09:56:11PM +0800, Fam Zheng wrote:
> On Thu, 01/21 10:50, Daniel P. Berrange wrote:
> > On Thu, Jan 21, 2016 at 05:54:23PM +0800, Fam Zheng wrote:
> > > On Wed, 01/20 17:38, Daniel P. Berrange wrote:
> > > > This converts the qcow2 driver to make use of the QCryptoBlock
> > > > APIs for encrypting image content. As well as continued support
> > > > for the legacy QCow2 encryption format, the appealing benefit
> > > > is that it enables support for the LUKS format inside qcow2.
> > > 
> > > FWIW, with today's QEMU, it's possible to stack format drivers on top of 
> > > each
> > > other.  In other words, even without this patch, we can make LUKS driver
> > > encrypt/decrypt the qcow2 payload, while keeping them completely 
> > > orthogonal.
> > 
> > Yep, that is certainly possible, and it is what is intended for using
> > LUKS with RBD, iSCSI, & other network drivers.
> > 
> > I think there is value in having LUKS integrated directly into qcow2
> > though. It means that given a qcow2 file you can 100% reliably
> > distinguish between a file created with the intention of QEMU managing
> > the LUKS encryption, from a file where the guest OS happens to have
> > set up LUKS encryption in its virtual disk. If you don't have this,
> > then given a random qcow2 file, you have to probe to see if LUKS is
> > present or not. Given the security issues we've had in the past with
> > raw images being turned into qcow2 images by a malicious guest writing
> > a qcow2 header, I feel that having explicitly integration LUKS support
> > in QCow is worthwhile as a concept.
> 
> Yes, I'm not objecting to explicit (read: mandatory) encryption in qcow2 in 
> any
> way, and extending the format spec for LUKS is a good thing.
> 
> I mentioned stacked drivers because I wonder how good we can do in reusing
> block/crypto.c code to achieve this (to save 500+ new code in qcow2). For
> example I have a rough idea along this:
> 
> * In qcow2 spec, define type "2" of crypt_method for LUKS encryption, and 
> state
>   that if this method is used, the payload must be LUKS format as defined in
>   cryptsetup's docs/on-disk-format.pdf, and driver will take care of
>   encrypting/decrypting data in a way that is transparent to guest.
> 
> * In qcow2_open, if header's crypt_method is LUKS, set a special flag in the
>   BlockDriverState to indicate that block layer should create an overlay 
> crypto
>   driver (which will be luks in this case), on top of this BDS. To do this we
>   need some modification to bdrv_open.
> 
> * When qcow2_open is done, block layer will then instantiate a LUKS driver,
>   which backed by the qcow2 BDS (as the BDS.file). This LUKS BDS will then be
>   attached to guest device.
> 
> * From the guest PoV, R/W reqs are served as if it's an ordinary qcow2; from
>   LUKS driver's PoV, it works properly formatted luks data, which is backed by
>   whatever it doesn't care, be it an iscsi target, rbd block, local file, or
>   a qcow2 image that has metadata; from the qcow2 driver's PoV, it only made
>   sure that a LUKS driver covers itself, at instantiation time.  Everything
>   else operates the same way as a non-encrypted qcow2.
> 
> * Of course, qcow2_create would need some similar changes to the block layer 
> as
>   to bdrv_open, but that shouldn't be too hard.
> 
> Makes sense?

Hmm, interesting idea. So essentially we'd be having a format driver
be able to inform the generic block code to insert an encryption
layer above it.  The downside is that it leaks details about use
of encryption out into the generic block code. The upside is that
it reduces code in the qcow2 impl.  I do wonder if that would lead
to more or less code overall, or whether the generic block layer
additions would cancel out the removals from qcow2.

BTW, we can probably reduce the amount of code in qcow2 regardless,
as about 1/2 of the addition is related to handling of the QemuOpts
<-> QCryptoBlockOpenOptions conversion, and I could share that
with the block/crypto.c driver to a greater extent than I do now.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|