Re: [Qemu-block] [PATCH v1 00/15] Convert QCow[2] to QCryptoBlock & add LUKS support

[Max Reitz]

On 25.01.2017 17:29, Daniel P. Berrange wrote:
> On Wed, Jan 25, 2017 at 04:58:32PM +0100, Max Reitz wrote:
>> On 03.01.2017 19:27, Daniel P. Berrange wrote:
>>> This series is a continuation of previous work to support LUKS in
>>> QEMU. The existing merged code supports LUKS as a standalone
>>> driver which can be layered over/under any other QEMU block device
>>> driver. This works well when using LUKS over protocol drivers (file,
>>> rbd, iscsi, etc, etc), but has some downsides when combined with
>>> format drivers like qcow2.
>>
>> When trying out whether compressed images are actually encrypted (which
>> they are not, as I wrote in my last reply to patch 12), I noticed that
>> the user interface still has some flaws:
> 
> The original code explicitly forbids this combination
> 
>  "qemu-img: Compression and encryption not supported at the same time"
> 
> but I guess we lost the error check due to changing to use
> encryption-format as the option name

Yes, not supporting it is completely fine, but qemu-img should refuse
that combination then (or at least warn about it).

>> One is that you actually can't convert to encrypted images any more, or
>> if you can, it doesn't seem obvious to me:
>>
>> $ ./qemu-img convert -O qcow2 --object secret,id=sec0,data=12345 \
>>     -o encryption-format=luks,luks-key-secret=sec0 \
>>     foo.qcow2 bar.qcow2
>> qemu-img: Could not open 'bar.qcow2': Parameter 'key-secret' is required
>> for cipher
>>
>> The issue is that you have to specify the key secret as a runtime
>> parameter in addition to the creation option. Not only is that a bit
>> cumbersome, but it's also impossible because --image-opts doesn't work
>> for the output image.
> 
> Yeah, this is a problem I've not figured out a solutiuon for yet - it
> also affects the previously merged bare luks format code.
> 
> Somehow qemu-img needs to know which create options are also required
> to be passed when opening the newly created image.
> 
> Perhaps the BlockDriver struct needs a new callback like
> 
>   bdrv_create_opts_to_runtime_opts(QemuOpts *copts, QemuOpts *ropts);

Yeah, it's tough. For the moment, I'd be fine with qemu-img convert
working at all, though, even if that means having to specify the secret
twice. But I don't know if fixing the --image-opts issue is any easier
-- maybe we can allow image-opts syntax for the targets when -n is
given? Then you'd have to call qemu-img create and qemu-img convert
separately (or call qemu-img convert twice, once without -n (which
successfully creates the image but then fails when trying to open it)
and once without...), but at least there'd be a way to make it work.

I have to admit that I personally wouldn't mind a hack in qemu-img
convert like "copy every option ending in 'key-secret' to the runtime
opts" too much. But I don't know how much that might infuriate some
other people.

It's ugly, yes, but it would work perfectly well for now. I don't think
it would hurt us in the future. If other parameters appear that we have
to copy over, we can still implement the well-engineered solution.

>> The second flaw is also visible above: The parameter is called
>> "luks-key-secret" here, not just "key-secret". The error message should
>> reflect that.
> 
> This is hard to fix. The "luks-key-secret" parameter refers to the parameter
> at the block driver level. The error message though is coming from the crypto
> layer whose parameter genuinely is called "key-secret". Fixing it would
> require the block layer to pre-emptively check parmaeters, duplicating what
> the crypto layer does later, which I didn't really like the idea of.

I know that it is hard to fix, but as it stands, the error message
confuses more than it helps. In my opinion, just saying "Key required"
or "key secret must be given" would be better than implying the code
actually knew how the option is called exactly.

Max

signature.asc
Description: OpenPGP digital signature