| Message ID | 1343869374-23417-12-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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 :)
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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:
Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- block.c | 1 + qapi-schema.json | 7 +++++-- 2 files changed, 6 insertions(+), 2 deletions(-)