Re: [RFC 0/8] Support generic Luks encryption

[Daniel P . Berrangé]

On Tue, Dec 05, 2023 at 01:32:51AM +0800, Yong Huang wrote:
> On Tue, Dec 5, 2023 at 12:51 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Tue, Dec 05, 2023 at 12:41:16AM +0800, Yong Huang wrote:
> > > On Tue, Dec 5, 2023 at 12:24 AM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > On Tue, Dec 05, 2023 at 12:06:17AM +0800, Hyman Huang wrote:
> > > > > This functionality was motivated by the following to-do list seen
> > > > > in crypto documents:
> > > > > https://wiki.qemu.org/Features/Block/Crypto
> > > > >
> > > > > The last chapter says we should "separate header volume":
> > > > >
> > > > > The LUKS format has ability to store the header in a separate volume
> > > > > from the payload. We should extend the LUKS driver in QEMU to support
> > > > > this use case.
> > > > >
> > > > > As a proof-of-concept, I've created this patchset, which I've named
> > > > > the Gluks: generic luks. As their name suggests, they offer
> > encryption
> > > > > for any format that QEMU theoretically supports.
> > > >
> > > > I don't see the point in creating a new driver.
> > > >
> > > > I would expect detached header support to be implemented via an
> > > > optional new 'header' field in the existing driver. ie
> > > >
> > > > diff --git a/qapi/block-core.json b/qapi/block-core.json
> > > > index ca390c5700..48d1f2a974 100644
> > > > --- a/qapi/block-core.json
> > > > +++ b/qapi/block-core.json
> > > > @@ -3352,11 +3352,15 @@
> > > >  #     decryption key (since 2.6). Mandatory except when doing a
> > > >  #     metadata-only probe of the image.
> > > >  #
> > > > +# @header: optional reference to the location of a blockdev
> > > > +#     storing a detached LUKS heaer
> > > > +#
> > > >  # Since: 2.9
> > > >  ##
> > > >  { 'struct': 'BlockdevOptionsLUKS',
> > > >    'base': 'BlockdevOptionsGenericFormat',
> > > > -  'data': { '*key-secret': 'str' } }
> > > > +  'data': { '*key-secret': 'str',
> > > > +            "*header-file': 'BlockdevRef'} }
> > > >
> > > >  ##
> > > >  # @BlockdevOptionsGenericCOWFormat:
> > > > @@ -4941,9 +4945,18 @@
> > > >  #
> > > >  # Driver specific image creation options for LUKS.
> > > >  #
> > > > -# @file: Node to create the image format on
> > > > +# @file: Node to create the image format on. Mandatory
> > > > +#     unless a detached header file is specified using
> > > > +#     @header.
> > > >  #
> > > > -# @size: Size of the virtual disk in bytes
> > > > +# @size: Size of the virtual disk in bytes.  Mandatory
> > > > +#     unless a detached header file is specified using
> > > > +#     @header.
> > > > +#
> > > > +# @header: optional reference to the location of a blockdev
> > > > +#     storing a detached LUKS heaer. The @file option is
> > > > +#     is optional when this is given, unless it is desired
> > > > +#     to perform pre-allocation
> > > >  #
> > > >  # @preallocation: Preallocation mode for the new image (since: 4.2)
> > > >  #     (default: off; allowed values: off, metadata, falloc, full)
> > > > @@ -4952,8 +4965,9 @@
> > > >  ##
> > > >  { 'struct': 'BlockdevCreateOptionsLUKS',
> > > >    'base': 'QCryptoBlockCreateOptionsLUKS',
> > > > -  'data': { 'file':             'BlockdevRef',
> > > > -            'size':             'size',
> > > > +  'data': { '*file':            'BlockdevRef',
> > > > +            '*size':            'size',
> > > > +            '*header':          'BlockdevRef'
> > > >              '*preallocation':   'PreallocMode' } }
> > > >
> > > >  ##
> > > >
> > > > It ends up giving basicallly the same workflow as you outline,
> > > > without needing the new block driver
> > > >
> > >
> > > How about the design and usage, could it be simpler? Any advice? :)
> > >
> > >
> > > As you can see below, the Gluks format block layer driver's design is
> > > quite simple.
> > >
> > >          virtio-blk/vhost-user-blk...(front-end device)
> > >               ^
> > >               |
> > >              Gluks   (format-like disk node)
> > >           /         \
> > >        file       header (blockdev reference)
> > >         /             \
> > >      file            file (protocol node)
> > >        |               |
> > >    disk data       Luks data
> >
> > What I suggested above ends up with the exact same block driver
> > graph, unless I'm missing something.
> >
> 
> I could overlook something or fail to adequately convey the goal of the
> patchset. :(
> 
> Indeed, utilizing the same block driver might be effective if our only goal
> is to divide the header volume, giving us an additional way to use Luks.
> 
> While supporting encryption for any disk format that QEMU is capable of
> supporting is another feature of this patchset. This implies that we might
> link the Luks header to other blockdev references, which might alter how
> the Luks are used and make them incompatible with it. It's not
> user-friendly in my opinion, and I'm not aware of a more elegant solution.

That existing LUKS driver can already be used in combination with
any QEMU block driver, and in the case of disk formats, can be used
either above or below the format, depending on whether you want to
encrypt just the image payload, or the payload and metadata ie.

We can do

  luks -> qcow2 -> file    (qcow2 header and qcow2 payload protected)

or

  qcow2 -> luks -> file    (only qcow2 payload protected)

And in the qcow2 case, we also have support for LUKS integrated natively
into the qcow2 format, which is similar to the 2nd case, with the
benefit that we're explicit that the image requires encryption.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|