Re: [Qemu-devel] [PATCH 00/13] RFC: luks/encrypted qcow2 key management

[Maxim Levitsky <mlevitsk@redhat.com>]

On Thu, 2019-08-22 at 12:35 +0100, Daniel P. Berrangé wrote:
> On Wed, Aug 14, 2019 at 11:22:06PM +0300, Maxim Levitsky wrote:
> > Hi!
> > 
> > This patch series implements key management for luks based encryption
> > It supports both raw luks images and qcow2 encrypted images.
> > 
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1731898
> > 
> > There are still several issues that need to be figured out,
> > on which the feedback is very welcome, but other than that the code mostly works.
> > 
> > The main issues are:
> > 
> > 1. Instead of the proposed blockdev-update-encryption/blockdev-erase-encryption
> > interface, it is probably better to implement 'blockdev-amend-options' in qmp,
> > and use this both for offline and online key update (with some translation
> > layer to convert the qemu-img 'options' to qmp structures)
> > 
> > This interface already exists for offline qcow2 format options update/
> > 
> > This is an issue that was raised today on IRC with Kevin Wolf. Really thanks
> > for the idea!
> > 
> > We agreed that this new qmp interface should take the same options as
> > blockdev-create does, however since we want to be able to edit the encryption
> > slots separately, this implies that we sort of need to allow this on creation
> > time as well.
> > 
> > Also the BlockdevCreateOptions is a union, which is specialized by the driver name
> > which is great for creation, but for update, the driver name is already known,
> > and thus the user should not be forced to pass it again.
> > However qmp doesn't seem to support union type guessing based on actual fields
> > given (this might not be desired either), which complicates this somewhat.
> 
> Given this design question around the integration into blockdev, I'd
> suggest splitting the series into two parts.
> 
> One series should do all the work in crypto/ code to support adding
> and erasing key slots.
> 
> One series should focus on block/ layer QMP/qemu-img integration.
> 
> The block layer QAPI stuff shouldn't leak into the crypto/ code.
> 
> So this will let us get on with reviewing & unit testing the
> crypto code, while we discuss block layer design options in more
> detail.
> 
> Regards,
> Daniel


I think we need 3 series here.


1. All the re-factoring/preparation work I done in luks crypto driver, which can be merged
now, pending minor changes from the review.
I think that it at least doesn't make the code worse.

2. Common code for the block layer to support key management this way or another,
   can be even added with not a single driver implementing it.

1,2 don't depend on each other mostly.


3. Key management in LUKS, which needs both 1,2, but thankfully is mostly implemented,
and won't need to change much from the current implementation.


So I'll send 1 now, and I will star working on 2.

Last week we (I and Daniel) defined a draft of amend interface,
and if time permits we will work on that tomorrow to finalize the
interface.

Best regards,
	Maxim Levitsky