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

login
register
mail settings
Submitter Luiz Capitulino
Date Aug. 2, 2012, 1:02 a.m.
Message ID <1343869374-23417-12-git-send-email-lcapitulino@redhat.com>
Download mbox | patch
Permalink /patch/174637/
State New
Headers show

Comments

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

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: