Luiz Capitulino <lcapitul...@redhat.com> writes:
> On Fri, 10 Aug 2012 18:35:26 +0200
> Markus Armbruster <arm...@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitul...@redhat.com> writes:
>>
>> > On Fri, 10 Aug 2012 09:56:11 +0200
>> > Markus Armbruster <arm...@redhat.com> wrote:
>> >
>> >> Revisited this one on review of v2, replying here for context.
>> >>
>> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
>> >>
>> >> > On Thu, 02 Aug 2012 13:35:54 +0200
>> >> > Markus Armbruster <arm...@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitul...@redhat.com> writes:
>> >> >>
>> >> >> > Signed-off-by: Luiz Capitulino <lcapitul...@redhat.com>
>> >> >> > ---
>> >> >> > 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.
>
> That's arguable. But I like the bools slightly better because they allow
> clients to do a true/false check vs. having to check against an enum value.
>
> Again, that's arguable.
>
>> >> >> 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...
>
> Ah, sorry, I read "sounds like true means the key is valid even for an
> invalid key". I've renamed it to encryption_key_missing, should be better
> (although I could also do encryption_key_is_missing).
>
>> >> 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.
>
> Why not? I'd expect that changing AES_set_decrypt_key() to fail for an
> invalid key wouldn't affect images, am I wrong?
AES_set_decrypt_key() and AES_set_encrypt_key() accept any key with 128,
192 or 256 bits. Decrypting with an incorrect key simply produces
garbage. That's what ciphers do.
>> You could even call it a feature that makes it (marginally) harder to
>> brute-force keys (I don't buy that argument myself).
>
> I don't, either.
