qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Luiz Capitulino
Subject: Re: [Qemu-devel] [PATCH 11/34] qmp: query-block: add 'valid_encryption_key' field
Date: Mon, 13 Aug 2012 11:02:43 -0300

On Mon, 13 Aug 2012 15:50:13 +0200
Markus Armbruster <address@hidden> wrote:

> Luiz Capitulino <address@hidden> writes:
> 
> > On Sat, 11 Aug 2012 09:45:14 +0200
> > Markus Armbruster <address@hidden> wrote:
> >
> >> Luiz Capitulino <address@hidden> writes:
> >> 
> >> > On Fri, 10 Aug 2012 19:17:22 +0200
> >> > Markus Armbruster <address@hidden> wrote:
> >> >
> >> >> Luiz Capitulino <address@hidden> writes:
> >> >> 
> >> >> > On Fri, 10 Aug 2012 18:35:26 +0200
> >> >> > Markus Armbruster <address@hidden> wrote:
> >> >> >
> >> >> >> 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.
> >> >> >
> >> >> > 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.
> >> >
> >> > (That's not my area of expertise, so hope I won't embarass myself)
> >> >
> >> > But how is ssh or any other software using encryption capable of telling
> >> > you that you entered a wrong password? Do they check against known data?
> >> 
> >> SSH password authentication boils down to the remote's password
> >> authentication, with the communication channel secured against
> >> eavesdroppers.
> >> 
> >> More relevant: if you secure your private SSH key with a passphrase,
> >> it's stored encrypted.  I don't know how exactly SSH determines that a
> >> passphrase is correct.  A plausible guess is it encrypts (key,h(key)).
> >> Decrypt, split into key and checksum, compare h(key) to checksum.
> >> 
> >> > Even if that's the case, any possible fix should be done in the block 
> >> > layer.
> >> 
> >> It's not fixable there.  Which makes it a feature.
> >> 
> >> Best we could do is extend QCOW2 so that invalid keys can be rejected.
> >> Will work only with new QCOW2 driver and new images.
> >
> > It's fixable in the block layer then :)
> 
> I'd rather not debate the meaning of "fixable".  All I'm saying is that
> the case "we got a key, and it may or may not be the right one" won't go
> away, and thus calling the thing valid_encryption_key will remain
> misleading.

It's not called valid_encryption_key anymore, so there's no point discussing
this anymore (unless I'm missing something and there are other points to
be discussed).

> > It can be acceptable to have workarounds in QMP if a severe issue is found
> > with current images, but this bug exists for ages and nobody has complained
> > so far. So, I'd go for the Right fix.
> 
> We can't fix it for old images, only for new ones.

Yes, what I'm saying is: let's go for the right fix (for new images) and only
add workarounds (for existing images) if really required.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]