From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44682) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0une-0000Bm-3A for qemu-devel@nongnu.org; Mon, 13 Aug 2012 09:35:22 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1T0unZ-0000lw-FV for qemu-devel@nongnu.org; Mon, 13 Aug 2012 09:35:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25649) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1T0unZ-0000ln-70 for qemu-devel@nongnu.org; Mon, 13 Aug 2012 09:35:13 -0400 Date: Mon, 13 Aug 2012 10:35:48 -0300 From: Luiz Capitulino Message-ID: <20120813103548.0768d209@doriath.home> In-Reply-To: <87pq6xlvn9.fsf@blackfin.pond.sub.org> References: <1343869374-23417-1-git-send-email-lcapitulino@redhat.com> <1343869374-23417-12-git-send-email-lcapitulino@redhat.com> <87r4rpr0f9.fsf@blackfin.pond.sub.org> <20120802105403.608761ea@doriath.home> <87pq6zch9g.fsf@blackfin.pond.sub.org> <20120810103300.68fe975b@doriath.home> <87boiivh69.fsf@blackfin.pond.sub.org> <20120810140016.0fb5d3eb@doriath.home> <87zk62r7j1.fsf@blackfin.pond.sub.org> <20120810145059.7a40fb35@doriath.home> <87pq6xlvn9.fsf@blackfin.pond.sub.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/34] qmp: query-block: add 'valid_encryption_key' field List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, aliguori@us.ibm.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com, pbonzini@redhat.com, eblake@redhat.com On Sat, 11 Aug 2012 09:45:14 +0200 Markus Armbruster wrote: > Luiz Capitulino writes: > > > On Fri, 10 Aug 2012 19:17:22 +0200 > > Markus Armbruster wrote: > > > >> Luiz Capitulino writes: > >> > >> > On Fri, 10 Aug 2012 18:35:26 +0200 > >> > Markus Armbruster wrote: > >> > > >> >> Luiz Capitulino writes: > >> >> > >> >> > On Fri, 10 Aug 2012 09:56:11 +0200 > >> >> > Markus Armbruster wrote: > >> >> > > >> >> >> Revisited this one on review of v2, replying here for context. > >> >> >> > >> >> >> Luiz Capitulino writes: > >> >> >> > >> >> >> > On Thu, 02 Aug 2012 13:35:54 +0200 > >> >> >> > Markus Armbruster wrote: > >> >> >> > > >> >> >> >> Luiz Capitulino writes: > >> >> >> >> > >> >> >> >> > Signed-off-by: Luiz Capitulino > >> >> >> >> > --- > >> >> >> >> > 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 :) 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.