Re: [Qemu-devel] [PATCH 11/34] qmp: query-block: add 'valid_encryption_key' field

[Markus Armbruster]

Luiz Capitulino <address@hidden> writes:

> On Fri, 10 Aug 2012 09:56:11 +0200
> Markus Armbruster <address@hidden> wrote:
>
>> Revisited this one on review of v2, replying here for context.
>> 
>> Luiz Capitulino <address@hidden> writes:
>> 
>> > On Thu, 02 Aug 2012 13:35:54 +0200
>> > Markus Armbruster <address@hidden> wrote:
>> >
>> >> Luiz Capitulino <address@hidden> writes:
>> >> 
>> >> > Signed-off-by: Luiz Capitulino <address@hidden>
>> >> > ---
>> >> >  block.c          | 1 +
>> >> >  qapi-schema.json | 7 +++++--
>> >> >  2 files changed, 6 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/block.c b/block.c
>> >> > index b38940b..9c113b8 100644
>> >> > --- a/block.c
>> >> > +++ b/block.c
>> >> > @@ -2445,6 +2445,7 @@ BlockInfoList *qmp_query_block(Error **errp)
>> >> >              info->value->inserted->ro = bs->read_only;
>> >> >              info->value->inserted->drv = 
>> >> > g_strdup(bs->drv->format_name);
>> >> >              info->value->inserted->encrypted = bs->encrypted;
>> >> > +            info->value->inserted->valid_encryption_key = 
>> >> > bs->valid_key;
>> >> >              if (bs->backing_file[0]) {
>> >> >                  info->value->inserted->has_backing_file = true;
>> >> >                  info->value->inserted->backing_file = 
>> >> > g_strdup(bs->backing_file);
>> >> > diff --git a/qapi-schema.json b/qapi-schema.json
>> >> > index bc55ed2..1b2d7f5 100644
>> >> > --- a/qapi-schema.json
>> >> > +++ b/qapi-schema.json
>> >> > @@ -400,6 +400,8 @@
>> >> >  #
>> >> >  # @encrypted: true if the backing device is encrypted
>> >> >  #
>> >> > +# @valid_encryption_key: true if a valid encryption key has been set
>> >> > +#
>> >> >  # @bps: total throughput limit in bytes per second is specified
>> >> >  #
>> >> >  # @bps_rd: read throughput limit in bytes per second is specified
>> >> > @@ -419,8 +421,9 @@
>> >> >  { 'type': 'BlockDeviceInfo',
>> >> >    'data': { 'file': 'str', 'ro': 'bool', 'drv': 'str',
>> >> >              '*backing_file': 'str', 'encrypted': 'bool',
>> >> > -            'bps': 'int', 'bps_rd': 'int', 'bps_wr': 'int',
>> >> > -            'iops': 'int', 'iops_rd': 'int', 'iops_wr': 'int'} }
>> >> > +            'valid_encryption_key': 'bool', 'bps': 'int',
>> >> > +            'bps_rd': 'int', 'bps_wr': 'int', 'iops': 'int',
>> >> > +            'iops_rd': 'int', 'iops_wr': 'int'} }
>> >> >  
>> >> >  ##
>> >> >  # @BlockDeviceIoStatus:
>> >> 
>> >> BlockDeviceInfo is API, isn't it?
>> >
>> > Yes.
>> >
>> >> Note that bs->valid_key currently implies bs->encrypted.  bs->valid_key
>> >> && !bs->encrypted is impossible.  Should we make valid_encryption_key
>> >> only available when encrypted?
>> >
>> > I don't think so. It's a bool, so it's ok for it to be false when
>> > encrypted is false.
>> 
>> What bothers me is encrypted=false, valid_encryption_key=true.
>
> Disappearing keys is worse, IMHO (assuming that that situation is impossible
> in practice, of course).

It's fundamentally three states: unencrypted, encrypted-no-key,
encrypted-got-key.  I'm fine with mapping these onto two bools, it's how
the block layer does it.  You may want to consider a single enumeration
instead.

>> >> valid_encryption_key is a bit long for my taste.  Yours may be
>> >> different.
>> >
>> > We should choose more descriptive and self-documenting names for the
>> > protocol. Besides, I can't think of anything shorter that won't get
>> > cryptic.
>> >
>> > Suggestions are always welcome though :)
>> 
>> valid_encryption_key sounds like the value is the valid key.
>
> That's exactly what it is.

Err, isn't the value bool?  The key value is a string...

>> got_crypt_key?  Also avoids "valid".  Good, because current encrypted
>> formats don't actually validate the key; they happily accept any key.
>
> That's a block layer bug, not QMP's.
>
> QMP clients are going to be misguided by valid_encryption_key the same way
> they are with the block_passwd command or how we suffer from it internally
> when calling bdrv_set_key() (which also manifests itself in HMP).
>
> Fixing the bug where it is will automatically fix all its instances.

It's not fixable for existing image formats, and thus existing images.

You could even call it a feature that makes it (marginally) harder to
brute-force keys (I don't buy that argument myself).

>> GIGO.  In theory, you can trash a disk that way.  In practice, we can
>> hope the guest will refuse to touch the disk, because it can't recognize
>> partition table / filesystems.