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

2023-12-05 Thread Daniel P . Berrangé
On Tue, Dec 05, 2023 at 09:51:21AM +0800, Yong Huang wrote:
> Let me make a conclusion about our discussion, if i
> misunderstand something,
> point that out please:
> 
> 1. To achieve the generic encryption,  extending the pre-existing LUKS QEMU
>block driver is suggested in practice.

yes

> 
> 2. Introducing a optional field called "header-file" that store the LUKS
> header
>independently, and once "header-file" was specified, we should use it to
>encrypt/decrypt the blockdev node referenced by the "file" field.

Yes.

> 
> The blockdev tree is like:
>  virtio-blk/vhost-user-blk...(frontend device)
>^
> |
> LUKS
>  / \
>  file   header-file
>  / \
> data protocol nodeLUKS header protocol node


That is one possible blockdev tree, but there are many possible
variations.

> 3. The usage of the generic LUKS is like:
> 
> Take the qcow2 as an example:
> 
> 1. add a protocol blockdev node of data disk
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
>   "arguments":{"node-name": "libvirt-1-storage", "driver": "file",
>   "filename": "/path/to/test_disk.qcow2"}}'
> 
> 2. add a protocol blockdev node of Luks header as above.
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
>   "arguments":{"node-name": "libvirt-2-storage", "driver": "file",
>   "filename": "/path/to/cipher.gluks" }}'
> 
> 3. add the secret for decrypting the cipher stored in Gluks header as
>above
> $ virsh qemu-monitor-command vm '{"execute":"object-add",
>   "arguments":{"qom-type": "secret", "id":
>   "libvirt-2-storage-secret0", "data": "abc123"}}'
> 
> 4. add the qcow2-drived blockdev format node:
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
>   "arguments":{"node-name": "libvirt-1-format", "driver": "qcow2",
>   "file": "libvirt-1-storage"}}'
> 
> 5. add the luks-drived blockdev to connect the qcow2 disk with Luks
>header by specifying the field "header-file"
> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
>   "arguments":{"node-name": "libvirt-2-format", "driver": "luks",
>   "file": "libvirt-1-format", "header-file": "libvirt-2-storage",
>   "key-secret": "libvirt-2-format-secret0"}}'
> 
> 6. add the device finally
> $ virsh qemu-monitor-command vm '{"execute":"device_add",
>   "arguments": {"num-queues": "1", "driver": "virtio-blk-pci", "scsi":
>   "off", "drive": "libvirt-2-format", "id": "virtio-disk2"}}'

Yes, that looks like a valid sequence of steps.

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




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

2023-12-05 Thread Kevin Wolf
Am 04.12.2023 um 18:43 hat Daniel P. Berrangé geschrieben:
> 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é 
> > 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é 
> > > > 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)
> > > > / \
> > > >  filefile (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
> > 

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

2023-12-04 Thread Yong Huang
On Tue, Dec 5, 2023 at 1:44 AM Daniel P. Berrangé 
wrote:

> 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é 
> > 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é <
> berra...@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)
> > > > / \
> > > >  filefile (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 

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

2023-12-04 Thread 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é 
> 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é 
> > > 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)
> > > / \
> > >  filefile (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 

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

2023-12-04 Thread Yong Huang
On Tue, Dec 5, 2023 at 12:51 AM Daniel P. Berrangé 
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é 
> > 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)
> > / \
> >  filefile (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.



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

-- 
Best regards


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

2023-12-04 Thread 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é 
> 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)
> / \
>  filefile (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 :|




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

2023-12-04 Thread Yong Huang
On Tue, Dec 5, 2023 at 12:24 AM Daniel P. Berrangé 
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)
/ \
 filefile (protocol node)
   |   |
   disk data   Luks data


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

-- 
Best regards


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

2023-12-04 Thread Yong Huang
On Tue, Dec 5, 2023 at 12:24 AM Daniel P. Berrangé 
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.
>

Indeed, this definitely makes things simple. The next version would do that
!

>
> 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
>
Yes, most of the logic reuse the pre-existing Luks driver.


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

-- 
Best regards


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

2023-12-04 Thread Daniel P . Berrangé
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

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