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

[Daniel P . Berrangé]

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.

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