Re: [PATCH v4 4/7] block: Support detached LUKS header creation using blockdev-create

["Daniel P. Berrangé" <berrange@redhat.com>]

On Mon, Feb 19, 2024 at 02:57:13PM +0000, Daniel P. Berrangé wrote:
> On Mon, Feb 19, 2024 at 03:49:30PM +0100, Markus Armbruster wrote:
> > One more thing...
> > 
> > Markus Armbruster <armbru@redhat.com> writes:
> > 
> > > yong.huang@smartx.com writes:
> > >
> > >> From: Hyman Huang <yong.huang@smartx.com>
> > >>
> > >> Firstly, enable the ability to choose the block device containing
> > >> a detachable LUKS header by adding the 'header' parameter to
> > >> BlockdevCreateOptionsLUKS.
> > >>
> > >> Secondly, when formatting the LUKS volume with a detachable header,
> > >> truncate the payload volume to length without a header size.
> > >>
> > >> Using the qmp blockdev command, create the LUKS volume with a
> > >> detachable header as follows:
> > >>
> > >> 1. add the secret to lock/unlock the cipher stored in the
> > >>    detached LUKS header
> > >> $ virsh qemu-monitor-command vm '{"execute":"object-add",
> > >>> "arguments":{"qom-type": "secret", "id": "sec0", "data": "foo"}}'
> > >>
> > >> 2. create a header img with 0 size
> > >> $ virsh qemu-monitor-command vm '{"execute":"blockdev-create",
> > >>> "arguments":{"job-id":"job0", "options":{"driver":"file",
> > >>> "filename":"/path/to/detached_luks_header.img", "size":0 }}}'
> > >>
> > >> 3. add protocol blockdev node for header
> > >> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > >>> "arguments": {"driver":"file", "filename":
> > >>> "/path/to/detached_luks_header.img", "node-name":
> > >>> "detached-luks-header-storage"}}'
> > >>
> > >> 4. create a payload img with 0 size
> > >> $ virsh qemu-monitor-command vm '{"execute":"blockdev-create",
> > >>> "arguments":{"job-id":"job1", "options":{"driver":"file",
> > >>> "filename":"/path/to/detached_luks_payload_raw.img", "size":0}}}'
> > >>
> > >> 5. add protocol blockdev node for payload
> > >> $ virsh qemu-monitor-command vm '{"execute":"blockdev-add",
> > >>> "arguments": {"driver":"file", "filename":
> > >>> "/path/to/detached_luks_payload_raw.img", "node-name":
> > >>> "luks-payload-raw-storage"}}'
> > >>
> > >> 6. do the formatting with 128M size
> > >> $ virsh qemu-monitor-command c81_node1 '{"execute":"blockdev-create",
> > >>> "arguments":{"job-id":"job2", "options":{"driver":"luks", "header":
> > >>> "detached-luks-header-storage", "file":"luks-payload-raw-storage",
> > >>> "size":134217728, "preallocation":"full", "key-secret":"sec0" }}}'
> > >>
> > >> Signed-off-by: Hyman Huang <yong.huang@smartx.com>
> > >> ---
> > >
> > > [...]
> > >
> > >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> > >> index 69a88d613d..eab15d7dd9 100644
> > >> --- a/qapi/block-core.json
> > >> +++ b/qapi/block-core.json
> > >> @@ -4960,6 +4960,8 @@
> > >>  # @file: Node to create the image format on, mandatory except when
> > >>  #        'preallocation' is not requested
> > >>  #
> > >> +# @header: Block device holding a detached LUKS header. (since 9.0)
> > >> +#
> > >
> > > Behavior when @header is present vs. behavior when it's absent?
> > 
> > The next patch adds a member to QCryptoBlockCreateOptionsLUKS, with a
> > similar description, but a different name:
> > 
> >     # @detached-header: create a detached LUKS header. (since 9.0)
> > 
> > Should we name the one added here @detached-header, too?
> 
> Yikes, that's a mistake. When I reviewed this I was somehow under the
> illusion that QCryptoBlockCreateOptionsLUKS was internal use only, for
> the block driver impl to interact with the crypto LUKS impl.
> 
> In fact, as the diff context below shows, QCryptoBlockCreateOptionsLUKS
> is a base struct for BlockdevCreateOptionsLUKS. So in effect we have
> one struct with two fields expressing similar concept.
> 
> TL;DR: the @detached-header field needs to go, as that's supposed to
> be internal only. The mgmt app should only care about 'header' in the
> BlockdevCreateOptionsLUKS struct.
> 
> FYI, this whole series is already merged last week. So this will need
> a fixup. I'll look into it now.

In fact the '@detached-header' added in the next patch is redundant
in the QAPI, as it is never set. So this QAPI field can be deleted.

We do have a 'detached-header' QemuOpts setting which is simply a
bool flag, as opposed to a full blockdev ref, since qemu-img create
doesn't work in terms of the QAPI BlockdevCreate schema.



> 
> > 
> > >>  # @size: Size of the virtual disk in bytes
> > >>  #
> > >>  # @preallocation: Preallocation mode for the new image (since: 4.2)
> > >> @@ -4970,6 +4972,7 @@
> > >>  { 'struct': 'BlockdevCreateOptionsLUKS',
> > >>    'base': 'QCryptoBlockCreateOptionsLUKS',
> > >>    'data': { '*file':            'BlockdevRef',
> > >> +            '*header':          'BlockdevRef',
> > >>              'size':             'size',
> > >>              '*preallocation':   'PreallocMode' } }
> 
> 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 :|
> 
> 

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