[11/34] qmp: query-block: add 'valid_encryption_key' field

Submitted by Luiz Capitulino on Aug. 2, 2012, 1:02 a.m.

Details

Message ID 1343869374-23417-12-git-send-email-lcapitulino@redhat.com
State New
Headers show

Commit Message

Luiz Capitulino Aug. 2, 2012, 1:02 a.m.
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
---
 block.c          | 1 +
 qapi-schema.json | 7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Markus Armbruster Aug. 2, 2012, 11:35 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> Signed-off-by: Luiz Capitulino <lcapitulino@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?

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?

valid_encryption_key is a bit long for my taste.  Yours may be
different.
Luiz Capitulino Aug. 2, 2012, 1:54 p.m.
On Thu, 02 Aug 2012 13:35:54 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > Signed-off-by: Luiz Capitulino <lcapitulino@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.

> 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 :)
Markus Armbruster Aug. 10, 2012, 7:56 a.m.
Revisited this one on review of v2, replying here for context.

Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 02 Aug 2012 13:35:54 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > Signed-off-by: Luiz Capitulino <lcapitulino@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.

>> 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.

got_crypt_key?  Also avoids "valid".  Good, because current encrypted
formats don't actually validate the key; they happily accept any key.
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.
Luiz Capitulino Aug. 10, 2012, 1:33 p.m.
On Fri, 10 Aug 2012 09:56:11 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Revisited this one on review of v2, replying here for context.
> 
> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Thu, 02 Aug 2012 13:35:54 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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).

> >> 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.

> 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.

> 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.
Markus Armbruster Aug. 10, 2012, 4:35 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 10 Aug 2012 09:56:11 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Revisited this one on review of v2, replying here for context.
>> 
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Thu, 02 Aug 2012 13:35:54 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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.

>> >> 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.
Luiz Capitulino Aug. 10, 2012, 5 p.m.
On Fri, 10 Aug 2012 18:35:26 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 10 Aug 2012 09:56:11 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Revisited this one on review of v2, replying here for context.
> >> 
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Thu, 02 Aug 2012 13:35:54 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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?

> 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.
Markus Armbruster Aug. 10, 2012, 5:17 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 10 Aug 2012 18:35:26 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 10 Aug 2012 09:56:11 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Revisited this one on review of v2, replying here for context.
>> >> 
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Thu, 02 Aug 2012 13:35:54 +0200
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> 
>> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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.
Luiz Capitulino Aug. 10, 2012, 5:50 p.m.
On Fri, 10 Aug 2012 19:17:22 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 10 Aug 2012 18:35:26 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Fri, 10 Aug 2012 09:56:11 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Revisited this one on review of v2, replying here for context.
> >> >> 
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Thu, 02 Aug 2012 13:35:54 +0200
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> 
> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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.

(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?

Even if that's the case, any possible fix should be done in the block layer.
Markus Armbruster Aug. 11, 2012, 7:45 a.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Fri, 10 Aug 2012 19:17:22 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 10 Aug 2012 18:35:26 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Fri, 10 Aug 2012 09:56:11 +0200
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Revisited this one on review of v2, replying here for context.
>> >> >> 
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> 
>> >> >> > On Thu, 02 Aug 2012 13:35:54 +0200
>> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >> >
>> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> >> 
>> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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.
>
> (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.
Luiz Capitulino Aug. 13, 2012, 1:35 p.m.
On Sat, 11 Aug 2012 09:45:14 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Fri, 10 Aug 2012 19:17:22 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Fri, 10 Aug 2012 18:35:26 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Fri, 10 Aug 2012 09:56:11 +0200
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> Revisited this one on review of v2, replying here for context.
> >> >> >> 
> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> 
> >> >> >> > On Thu, 02 Aug 2012 13:35:54 +0200
> >> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >> >
> >> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> >> 
> >> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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.
> >
> > (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.
Markus Armbruster Aug. 13, 2012, 1:50 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Sat, 11 Aug 2012 09:45:14 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> 
>> > On Fri, 10 Aug 2012 19:17:22 +0200
>> > Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> 
>> >> > On Fri, 10 Aug 2012 18:35:26 +0200
>> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >
>> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> 
>> >> >> > On Fri, 10 Aug 2012 09:56:11 +0200
>> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >> >
>> >> >> >> Revisited this one on review of v2, replying here for context.
>> >> >> >> 
>> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> >> 
>> >> >> >> > On Thu, 02 Aug 2012 13:35:54 +0200
>> >> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
>> >> >> >> >
>> >> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
>> >> >> >> >> 
>> >> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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.
>> >
>> > (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 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.
Luiz Capitulino Aug. 13, 2012, 2:02 p.m.
On Mon, 13 Aug 2012 15:50:13 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Luiz Capitulino <lcapitulino@redhat.com> writes:
> 
> > On Sat, 11 Aug 2012 09:45:14 +0200
> > Markus Armbruster <armbru@redhat.com> wrote:
> >
> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> 
> >> > On Fri, 10 Aug 2012 19:17:22 +0200
> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >
> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> 
> >> >> > On Fri, 10 Aug 2012 18:35:26 +0200
> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >
> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> 
> >> >> >> > On Fri, 10 Aug 2012 09:56:11 +0200
> >> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >> >
> >> >> >> >> Revisited this one on review of v2, replying here for context.
> >> >> >> >> 
> >> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> >> 
> >> >> >> >> > On Thu, 02 Aug 2012 13:35:54 +0200
> >> >> >> >> > Markus Armbruster <armbru@redhat.com> wrote:
> >> >> >> >> >
> >> >> >> >> >> Luiz Capitulino <lcapitulino@redhat.com> writes:
> >> >> >> >> >> 
> >> >> >> >> >> > Signed-off-by: Luiz Capitulino <lcapitulino@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.
> >> >
> >> > (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.

Patch hide | download patch | download mbox

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: