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

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