[Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Markus Armbruster]

RFC because the series only covers open [PATCH 1], but not create.
Also missing: make qemu-img print a warning when it creates an
encrypted image.  Finally, some of the material in the cover letter
should be worked into the commit messages.

We've steered users away from QCOW/QCOW2 encryption for a while,
because it's a flawed design (commit 136cd19 Describe flaws in
qcow/qcow2 encryption in the docs).

In addition to flawed crypto, we have comically bad usability, and
plain old bugs.  Let me show you.

= Example images =

I'm going to use a raw image as backing file, and two QCOW2 images,
one encrypted, and one not:

    $ qemu-img create -f raw backing.img 4m
    Formatting 'backing.img', fmt=raw size=4194304
    $ qemu-img create -f qcow2 -o encryption,backing_file=backing.img,backing_fmt=raw geheim.qcow2 4m
    Formatting 'geheim.qcow2', fmt=qcow2 size=4194304 backing_file='backing.img' backing_fmt='raw' encryption=on cluster_size=65536 lazy_refcounts=off
    $ qemu-img create -f qcow2 -o backing_file=backing.img,backing_fmt=raw normal.qcow2 4m
    Formatting 'normal.qcow2', fmt=qcow2 size=4194304 backing_file='backing.img' backing_fmt='raw' encryption=off cluster_size=65536 lazy_refcounts=off

= Usability issues =

== Confusing startup ==

When no image is encrypted, and you don't give -S, QEMU starts the
guest immediately:

    $ qemu-system-x86_64 -nodefaults -display none -monitor stdio normal.qcow2
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) info status
    VM status: running

As soon as there's an encrypted image in play, the guest isn't started
with no notification whatsoever:

    $ qemu-system-x86_64 -nodefaults -display none -monitor stdio geheim.qcow2 
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) info status
    VM status: paused (prelaunch)

If the user figured out that he needs to type "cont" to enter his
keys, the confusion enters the next level: "cont" asks for at most
*one* key.  If more are needed, it then silently does nothing.  The
user has to type "cont" once per encrypted image:

    $ qemu-system-x86_64 -nodefaults -display none -monitor stdio -drive if=none,file=geheim.qcow2 -drive if=none,file=geheim.qcow2 
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) info status
    VM status: paused (prelaunch)
    (qemu) c
    none0 (geheim.qcow2) is encrypted.
    Password: ******
    (qemu) info status
    VM status: paused (prelaunch)
    (qemu) c
    none1 (geheim.qcow2) is encrypted.
    Password: ******
    (qemu) info status
    VM status: running

== Incorrect passwords not caught ==

All existing encryption schemes accept give you the GIGO treatment:
garbage password in, garbage data out.  Guests usually refuse to mount
garbage, but other usage is prone to data loss.

== Need to stop the guest to add an encrypted image ==

    $ qemu-system-x86_64 -nodefaults -display none -monitor stdio 
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) info status
    VM status: running
    (qemu) drive_add "" if=none,file=geheim.qcow2
    Guest must be stopped for opening of encrypted image
    (qemu) stop
    (qemu) drive_add "" if=none,file=geheim.qcow2
    OK

= Bugs =

== Use without key is not always caught ==

Encrypted images can be in an intermediate state "opened, but no key".
The weird startup behavior and the need to stop the guest are there to
ensure the guest isn't exposed to that state.  But other things still
are!

* drive_backup

    $ qemu-system-x86_64 -nodefaults -display none -monitor stdio geheim.qcow2 
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) drive_backup -f ide0-hd0 out.img raw
    Formatting 'out.img', fmt=raw size=4194304

  I guess this writes encrypted data to raw image out.img.  Good luck
  with figuring out how to decrypt that again.

* commit

    $ qemu-system-x86_64 -nodefaults -display none -monitor stdio geheim.qcow2 
    QEMU 2.2.50 monitor - type 'help' for more information
    (qemu) commit ide0-hd0

  I guess this writes encrypted data into the unencrypted raw backing
  image, effectively destroying it.

== QMP device_add of usb-storage fails when it shouldn't ==

When the image is encrypted, device_add creates the device, defers
actually attaching it to when the key becomes available, then fails.
This is wrong.  device_add must either create the device and succeed,
or do nothing and fail.

    $ qemu-system-x86_64 -nodefaults -display none -usb -qmp stdio -drive if=none,id=foo,file=geheim.qcow2
    {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 2}, "package": ""}, "capabilities": []}}
    { "execute": "qmp_capabilities" }
    {"return": {}}
    { "execute": "device_add", "arguments": { "driver": "usb-storage", "id": "bar", "drive": "foo" } }
    {"error": {"class": "DeviceEncrypted", "desc": "'foo' (geheim.qcow2) is encrypted"}}
    {"execute":"device_del","arguments": { "id": "bar" } }
    {"timestamp": {"seconds": 1426003440, "microseconds": 237181}, "event": "DEVICE_DELETED", "data": {"path": "/machine/peripheral/bar/bar.0/legacy[0]"}}
    {"timestamp": {"seconds": 1426003440, "microseconds": 238231}, "event": "DEVICE_DELETED", "data": {"device": "bar", "path": "/machine/peripheral/bar"}}
    {"return": {}}

This stuff is worse than useless, it's a trap for users.

If people become sufficiently interested in encrypted images to
contribute a cryptographically sane implementation for QCOW2 (or
whatever other format), then rewriting the necessary support around it
from scratch will likely be easier and yield better results than
fixing up the existing mess.

Let's drop the mess and move on.  Keep qemu-img convert working, of
course, to let users rescue their data.

Markus Armbruster (2):
  block: Limit opening of encrypted images to qemu-img
  block: Drop code supporting encryption outside qemu-img

 block.c                   | 30 --------------------
 block/qcow.c              |  5 ++++
 block/qcow2.c             |  5 ++++
 blockdev.c                | 43 +---------------------------
 hmp-commands.hx           | 14 ---------
 hmp.c                     | 41 ---------------------------
 hmp.h                     |  1 -
 hw/usb/dev-storage.c      | 26 -----------------
 include/block/block.h     |  3 +-
 include/monitor/monitor.h |  7 -----
 monitor.c                 | 72 -----------------------------------------------
 qapi-schema.json          | 13 ++-------
 qapi/block-core.json      | 42 ++-------------------------
 qapi/common.json          |  5 +---
 qemu-img.c                |  1 +
 qmp-commands.hx           | 26 -----------------
 qmp.c                     |  8 ------
 17 files changed, 18 insertions(+), 324 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH RFC 1/2] block: Limit opening of encrypted images to qemu-img

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block/qcow.c          | 5 +++++
 block/qcow2.c         | 5 +++++
 include/block/block.h | 3 +--
 qemu-img.c            | 1 +
 4 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index 0558969..f54fc86 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -155,6 +155,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->crypt_method_header = header.crypt_method;
     if (s->crypt_method_header) {
+        if (!(flags & BDRV_O_CRYPT_OK)) {
+            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
+            ret = -EINVAL;
+            goto fail;
+        }
         bs->encrypted = 1;
     }
     s->cluster_bits = header.cluster_bits;
diff --git a/block/qcow2.c b/block/qcow2.c
index 50e0a94..b12c67a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -693,6 +693,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     }
     s->crypt_method_header = header.crypt_method;
     if (s->crypt_method_header) {
+        if (!(flags & BDRV_O_CRYPT_OK)) {
+            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
+            ret = -EINVAL;
+            goto fail;
+        }
         bs->encrypted = 1;
     }
 
diff --git a/include/block/block.h b/include/block/block.h
index 649c269..76b6d3c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -76,6 +76,7 @@ typedef enum {
 #define BDRV_O_PROTOCOL    0x8000  /* if no block driver is explicitly given:
                                       select an appropriate protocol driver,
                                       ignoring the format layer */
+#define BDRV_O_CRYPT_OK    0x10000 /* open even when image is encrypted */
 
 #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
 
@@ -382,8 +383,6 @@ BlockDriverState *bdrv_next(BlockDriverState *bs);
 int bdrv_is_encrypted(BlockDriverState *bs);
 int bdrv_key_required(BlockDriverState *bs);
 int bdrv_set_key(BlockDriverState *bs, const char *key);
-void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp);
-int bdrv_query_missing_keys(void);
 void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
                          void *opaque);
 const char *bdrv_get_node_name(const BlockDriverState *bs);
diff --git a/qemu-img.c b/qemu-img.c
index 7ac7f56..bcd7d3f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -302,6 +302,7 @@ static BlockBackend *img_open(const char *id, const char *filename,
         qdict_put(options, "driver", qstring_from_str(fmt));
     }
 
+    flags |= BDRV_O_CRYPT_OK;
     blk = blk_new_open(id, filename, NULL, options, flags, &local_err);
     if (!blk) {
         error_report("Could not open '%s': %s", filename,
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 1/2] block: Limit opening of encrypted images to qemu-img

[Daniel P. Berrange]

On Tue, Mar 10, 2015 at 06:26:39PM +0100, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/qcow.c          | 5 +++++
>  block/qcow2.c         | 5 +++++
>  include/block/block.h | 3 +--
>  qemu-img.c            | 1 +
>  4 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 0558969..f54fc86 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -155,6 +155,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>      s->crypt_method_header = header.crypt_method;
>      if (s->crypt_method_header) {
> +        if (!(flags & BDRV_O_CRYPT_OK)) {
> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>          bs->encrypted = 1;
>      }
>      s->cluster_bits = header.cluster_bits;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 50e0a94..b12c67a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -693,6 +693,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>      s->crypt_method_header = header.crypt_method;
>      if (s->crypt_method_header) {
> +        if (!(flags & BDRV_O_CRYPT_OK)) {
> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>          bs->encrypted = 1;
>      }
>  
> diff --git a/include/block/block.h b/include/block/block.h
> index 649c269..76b6d3c 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -76,6 +76,7 @@ typedef enum {
>  #define BDRV_O_PROTOCOL    0x8000  /* if no block driver is explicitly given:
>                                        select an appropriate protocol driver,
>                                        ignoring the format layer */
> +#define BDRV_O_CRYPT_OK    0x10000 /* open even when image is encrypted */
>  
>  #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>  
> @@ -382,8 +383,6 @@ BlockDriverState *bdrv_next(BlockDriverState *bs);
>  int bdrv_is_encrypted(BlockDriverState *bs);
>  int bdrv_key_required(BlockDriverState *bs);
>  int bdrv_set_key(BlockDriverState *bs, const char *key);
> -void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp);

Nitpick, this method isn't deleted until your next patch, so this belongs
there.

> -int bdrv_query_missing_keys(void);

Fun, a method which doesn't even exist :-)

>  void bdrv_iterate_format(void (*it)(void *opaque, const char *name),
>                           void *opaque);
>  const char *bdrv_get_node_name(const BlockDriverState *bs);
> diff --git a/qemu-img.c b/qemu-img.c
> index 7ac7f56..bcd7d3f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -302,6 +302,7 @@ static BlockBackend *img_open(const char *id, const char *filename,
>          qdict_put(options, "driver", qstring_from_str(fmt));
>      }
>  
> +    flags |= BDRV_O_CRYPT_OK;
>      blk = blk_new_open(id, filename, NULL, options, flags, &local_err);
>      if (!blk) {
>          error_report("Could not open '%s': %s", filename,

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH RFC 1/2] block: Limit opening of encrypted images to qemu-img

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Mar 10, 2015 at 06:26:39PM +0100, Markus Armbruster wrote:
[...]
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 649c269..76b6d3c 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -76,6 +76,7 @@ typedef enum {
>>  #define BDRV_O_PROTOCOL    0x8000  /* if no block driver is explicitly given:
>>                                        select an appropriate protocol driver,
>>                                        ignoring the format layer */
>> +#define BDRV_O_CRYPT_OK    0x10000 /* open even when image is encrypted */
>>  
>>  #define BDRV_O_CACHE_MASK  (BDRV_O_NOCACHE | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH)
>>  
>> @@ -382,8 +383,6 @@ BlockDriverState *bdrv_next(BlockDriverState *bs);
>>  int bdrv_is_encrypted(BlockDriverState *bs);
>>  int bdrv_key_required(BlockDriverState *bs);
>>  int bdrv_set_key(BlockDriverState *bs, const char *key);
>> -void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp);
>
> Nitpick, this method isn't deleted until your next patch, so this belongs
> there.

Will fix, thanks!

>> -int bdrv_query_missing_keys(void);
>
> Fun, a method which doesn't even exist :-)

Extra fun: it never existed, yet this line is six years old :)

[..]

Re: [Qemu-devel] [PATCH RFC 1/2] block: Limit opening of encrypted images to qemu-img

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1432 bytes --]

On 03/10/2015 11:26 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/qcow.c          | 5 +++++
>  block/qcow2.c         | 5 +++++
>  include/block/block.h | 3 +--
>  qemu-img.c            | 1 +
>  4 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 0558969..f54fc86 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -155,6 +155,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>      s->crypt_method_header = header.crypt_method;
>      if (s->crypt_method_header) {
> +        if (!(flags & BDRV_O_CRYPT_OK)) {
> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>          bs->encrypted = 1;

I think this message will make it nicely through to libvirt, if libvirt
still tries to use encryption (although I didn't actually test it, as
qcow2 encryption is so broken that I've never actually tried using it).
 More importantly, patch 2/2 does something that can be observed via
'query-commands' introspection, so that newer libvirt can be made smart
enough to not even attempt to use encrypted images.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 1/2] block: Limit opening of encrypted images to qemu-img

[Kevin Wolf]

Am 10.03.2015 um 18:26 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block/qcow.c          | 5 +++++
>  block/qcow2.c         | 5 +++++
>  include/block/block.h | 3 +--
>  qemu-img.c            | 1 +
>  4 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index 0558969..f54fc86 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -155,6 +155,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>      s->crypt_method_header = header.crypt_method;
>      if (s->crypt_method_header) {
> +        if (!(flags & BDRV_O_CRYPT_OK)) {
> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>          bs->encrypted = 1;
>      }
>      s->cluster_bits = header.cluster_bits;
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 50e0a94..b12c67a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -693,6 +693,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>      }
>      s->crypt_method_header = header.crypt_method;
>      if (s->crypt_method_header) {
> +        if (!(flags & BDRV_O_CRYPT_OK)) {
> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
> +            ret = -EINVAL;
> +            goto fail;
> +        }
>          bs->encrypted = 1;
>      }

Can we do this in bdrv_open_common() instead of duplicating the code in
two drivers?

Kevin

Re: [Qemu-devel] [PATCH RFC 1/2] block: Limit opening of encrypted images to qemu-img

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 10.03.2015 um 18:26 hat Markus Armbruster geschrieben:
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  block/qcow.c          | 5 +++++
>>  block/qcow2.c         | 5 +++++
>>  include/block/block.h | 3 +--
>>  qemu-img.c            | 1 +
>>  4 files changed, 12 insertions(+), 2 deletions(-)
>> 
>> diff --git a/block/qcow.c b/block/qcow.c
>> index 0558969..f54fc86 100644
>> --- a/block/qcow.c
>> +++ b/block/qcow.c
>> @@ -155,6 +155,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
>>      }
>>      s->crypt_method_header = header.crypt_method;
>>      if (s->crypt_method_header) {
>> +        if (!(flags & BDRV_O_CRYPT_OK)) {
>> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>>          bs->encrypted = 1;
>>      }
>>      s->cluster_bits = header.cluster_bits;
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 50e0a94..b12c67a 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -693,6 +693,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
>>      }
>>      s->crypt_method_header = header.crypt_method;
>>      if (s->crypt_method_header) {
>> +        if (!(flags & BDRV_O_CRYPT_OK)) {
>> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
>> +            ret = -EINVAL;
>> +            goto fail;
>> +        }
>>          bs->encrypted = 1;
>>      }
>
> Can we do this in bdrv_open_common() instead of duplicating the code in
> two drivers?

I looked for a better place to do this, but couldn't find one that
*obviously* dominates the bdrv_open() methods, so I put it into the
block drivers.

Can you explain why bdrv_open_common() cannot be bypassed?

Re: [Qemu-devel] [PATCH RFC 1/2] block: Limit opening of encrypted images to qemu-img

[Kevin Wolf]

Am 11.03.2015 um 12:59 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 10.03.2015 um 18:26 hat Markus Armbruster geschrieben:
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>  block/qcow.c          | 5 +++++
> >>  block/qcow2.c         | 5 +++++
> >>  include/block/block.h | 3 +--
> >>  qemu-img.c            | 1 +
> >>  4 files changed, 12 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/block/qcow.c b/block/qcow.c
> >> index 0558969..f54fc86 100644
> >> --- a/block/qcow.c
> >> +++ b/block/qcow.c
> >> @@ -155,6 +155,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
> >>      }
> >>      s->crypt_method_header = header.crypt_method;
> >>      if (s->crypt_method_header) {
> >> +        if (!(flags & BDRV_O_CRYPT_OK)) {
> >> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
> >> +            ret = -EINVAL;
> >> +            goto fail;
> >> +        }
> >>          bs->encrypted = 1;
> >>      }
> >>      s->cluster_bits = header.cluster_bits;
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index 50e0a94..b12c67a 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -693,6 +693,11 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
> >>      }
> >>      s->crypt_method_header = header.crypt_method;
> >>      if (s->crypt_method_header) {
> >> +        if (!(flags & BDRV_O_CRYPT_OK)) {
> >> +            error_setg(errp, "image is encrypted, use qemu-img to decrypt it");
> >> +            ret = -EINVAL;
> >> +            goto fail;
> >> +        }
> >>          bs->encrypted = 1;
> >>      }
> >
> > Can we do this in bdrv_open_common() instead of duplicating the code in
> > two drivers?
> 
> I looked for a better place to do this, but couldn't find one that
> *obviously* dominates the bdrv_open() methods, so I put it into the
> block drivers.
> 
> Can you explain why bdrv_open_common() cannot be bypassed?

Because it's the only place in qemu that calls drv->bdrv_open() and
drv->bdrv_file_open(). (Except for bdrv_snapshot_goto(), which does a
close/open on an already opened image.)

Everything else is just a - potentially indirect - caller of
bdrv_open_common().

Kevin

[Qemu-devel] [PATCH RFC 2/2] block: Drop code supporting encryption outside qemu-img

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 block.c                   | 30 --------------------
 blockdev.c                | 43 +---------------------------
 hmp-commands.hx           | 14 ---------
 hmp.c                     | 41 ---------------------------
 hmp.h                     |  1 -
 hw/usb/dev-storage.c      | 26 -----------------
 include/monitor/monitor.h |  7 -----
 monitor.c                 | 72 -----------------------------------------------
 qapi-schema.json          | 13 ++-------
 qapi/block-core.json      | 42 ++-------------------------
 qapi/common.json          |  5 +---
 qmp-commands.hx           | 26 -----------------
 qmp.c                     |  8 ------
 13 files changed, 6 insertions(+), 322 deletions(-)

diff --git a/block.c b/block.c
index 28ea19a..e519ac7 100644
--- a/block.c
+++ b/block.c
@@ -3708,36 +3708,6 @@ int bdrv_set_key(BlockDriverState *bs, const char *key)
     return ret;
 }
 
-/*
- * Provide an encryption key for @bs.
- * If @key is non-null:
- *     If @bs is not encrypted, fail.
- *     Else if the key is invalid, fail.
- *     Else set @bs's key to @key, replacing the existing key, if any.
- * If @key is null:
- *     If @bs is encrypted and still lacks a key, fail.
- *     Else do nothing.
- * On failure, store an error object through @errp if non-null.
- */
-void bdrv_add_key(BlockDriverState *bs, const char *key, Error **errp)
-{
-    if (key) {
-        if (!bdrv_is_encrypted(bs)) {
-            error_setg(errp, "Device '%s' is not encrypted",
-                      bdrv_get_device_name(bs));
-        } else if (bdrv_set_key(bs, key) < 0) {
-            error_set(errp, QERR_INVALID_PASSWORD);
-        }
-    } else {
-        if (bdrv_key_required(bs)) {
-            error_set(errp, ERROR_CLASS_DEVICE_ENCRYPTED,
-                      "'%s' (%s) is encrypted",
-                      bdrv_get_device_name(bs),
-                      bdrv_get_encrypted_filename(bs));
-        }
-    }
-}
-
 const char *bdrv_get_format_name(BlockDriverState *bs)
 {
     return bs->drv ? bs->drv->format_name : NULL;
diff --git a/blockdev.c b/blockdev.c
index b9c1c0c..103cc67 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1774,47 +1774,6 @@ void qmp_eject(const char *device, bool has_force, bool force, Error **errp)
     eject_device(blk, force, errp);
 }
 
-void qmp_block_passwd(bool has_device, const char *device,
-                      bool has_node_name, const char *node_name,
-                      const char *password, Error **errp)
-{
-    Error *local_err = NULL;
-    BlockDriverState *bs;
-    AioContext *aio_context;
-
-    bs = bdrv_lookup_bs(has_device ? device : NULL,
-                        has_node_name ? node_name : NULL,
-                        &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    aio_context = bdrv_get_aio_context(bs);
-    aio_context_acquire(aio_context);
-
-    bdrv_add_key(bs, password, errp);
-
-    aio_context_release(aio_context);
-}
-
-/* Assumes AioContext is held */
-static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
-                                    int bdrv_flags, BlockDriver *drv,
-                                    const char *password, Error **errp)
-{
-    Error *local_err = NULL;
-    int ret;
-
-    ret = bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, &local_err);
-    if (ret < 0) {
-        error_propagate(errp, local_err);
-        return;
-    }
-
-    bdrv_add_key(bs, password, errp);
-}
-
 void qmp_change_blockdev(const char *device, const char *filename,
                          const char *format, Error **errp)
 {
@@ -1852,7 +1811,7 @@ void qmp_change_blockdev(const char *device, const char *filename,
     bdrv_flags = bdrv_is_read_only(bs) ? 0 : BDRV_O_RDWR;
     bdrv_flags |= bdrv_is_snapshot(bs) ? BDRV_O_SNAPSHOT : 0;
 
-    qmp_bdrv_open_encrypted(bs, filename, bdrv_flags, drv, NULL, errp);
+    bdrv_open(&bs, filename, NULL, NULL, bdrv_flags, drv, errp);
 
 out:
     aio_context_release(aio_context);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 1cf0081..058dee2 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1508,20 +1508,6 @@ used by another monitor command.
 ETEXI
 
     {
-        .name       = "block_passwd",
-        .args_type  = "device:B,password:s",
-        .params     = "block_passwd device password",
-        .help       = "set the password of encrypted block devices",
-        .mhandler.cmd = hmp_block_passwd,
-    },
-
-STEXI
-@item block_passwd @var{device} @var{password}
-@findex block_passwd
-Set the encrypted device @var{device} password to @var{password}
-ETEXI
-
-    {
         .name       = "block_set_io_throttle",
         .args_type  = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l",
         .params     = "device bps bps_rd bps_wr iops iops_rd iops_wr",
diff --git a/hmp.c b/hmp.c
index 71c28bc..c2a167b 100644
--- a/hmp.c
+++ b/hmp.c
@@ -918,37 +918,12 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)
     g_free(data);
 }
 
-static void hmp_cont_cb(void *opaque, int err)
-{
-    if (!err) {
-        qmp_cont(NULL);
-    }
-}
-
-static bool key_is_missing(const BlockInfo *bdev)
-{
-    return (bdev->inserted && bdev->inserted->encryption_key_missing);
-}
-
 void hmp_cont(Monitor *mon, const QDict *qdict)
 {
-    BlockInfoList *bdev_list, *bdev;
     Error *err = NULL;
 
-    bdev_list = qmp_query_block(NULL);
-    for (bdev = bdev_list; bdev; bdev = bdev->next) {
-        if (key_is_missing(bdev->value)) {
-            monitor_read_block_device_key(mon, bdev->value->device,
-                                          hmp_cont_cb, NULL);
-            goto out;
-        }
-    }
-
     qmp_cont(&err);
     hmp_handle_error(mon, &err);
-
-out:
-    qapi_free_BlockInfoList(bdev_list);
 }
 
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
@@ -974,16 +949,6 @@ void hmp_set_link(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &err);
 }
 
-void hmp_block_passwd(Monitor *mon, const QDict *qdict)
-{
-    const char *device = qdict_get_str(qdict, "device");
-    const char *password = qdict_get_str(qdict, "password");
-    Error *err = NULL;
-
-    qmp_block_passwd(true, device, false, NULL, password, &err);
-    hmp_handle_error(mon, &err);
-}
-
 void hmp_balloon(Monitor *mon, const QDict *qdict)
 {
     int64_t value = qdict_get_int(qdict, "value");
@@ -1228,12 +1193,6 @@ void hmp_change(Monitor *mon, const QDict *qdict)
     }
 
     qmp_change(device, target, !!arg, arg, &err);
-    if (err &&
-        error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
-        error_free(err);
-        monitor_read_block_device_key(mon, device, NULL, NULL);
-        return;
-    }
     hmp_handle_error(mon, &err);
 }
 
diff --git a/hmp.h b/hmp.h
index 81177b2..004eabf 100644
--- a/hmp.h
+++ b/hmp.h
@@ -51,7 +51,6 @@ void hmp_cont(Monitor *mon, const QDict *qdict);
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict);
 void hmp_nmi(Monitor *mon, const QDict *qdict);
 void hmp_set_link(Monitor *mon, const QDict *qdict);
-void hmp_block_passwd(Monitor *mon, const QDict *qdict);
 void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_block_resize(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 65d9aa6..641a69d 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -549,22 +549,6 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p)
     }
 }
 
-static void usb_msd_password_cb(void *opaque, int err)
-{
-    MSDState *s = opaque;
-    Error *local_err = NULL;
-
-    if (!err) {
-        usb_device_attach(&s->dev, &local_err);
-    }
-
-    if (local_err) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        qdev_unplug(&s->dev.qdev, NULL);
-    }
-}
-
 static void *usb_msd_load_request(QEMUFile *f, SCSIRequest *req)
 {
     MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent);
@@ -637,16 +621,6 @@ static void usb_msd_realize_storage(USBDevice *dev, Error **errp)
     }
     usb_msd_handle_reset(dev);
     s->scsi_dev = scsi_dev;
-
-    if (bdrv_key_required(blk_bs(blk))) {
-        if (cur_mon) {
-            monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
-                                        usb_msd_password_cb, s);
-            s->dev.auto_attach = 0;
-        } else {
-            autostart = 0;
-        }
-    }
 }
 
 static void usb_msd_realize_bot(USBDevice *dev, Error **errp)
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index 1c06bed..2104a49 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -26,13 +26,6 @@ void monitor_init(CharDriverState *chr, int flags);
 int monitor_suspend(Monitor *mon);
 void monitor_resume(Monitor *mon);
 
-int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-                                BlockCompletionFunc *completion_cb,
-                                void *opaque);
-int monitor_read_block_device_key(Monitor *mon, const char *device,
-                                  BlockCompletionFunc *completion_cb,
-                                  void *opaque);
-
 int monitor_get_fd(Monitor *mon, const char *fdname, Error **errp);
 int monitor_fd_param(Monitor *mon, const char *fdname, Error **errp);
 
diff --git a/monitor.c b/monitor.c
index c86a89e..7523c75 100644
--- a/monitor.c
+++ b/monitor.c
@@ -206,8 +206,6 @@ struct Monitor {
     ReadLineState *rs;
     MonitorControl *mc;
     CPUState *mon_cpu;
-    BlockCompletionFunc *password_completion_cb;
-    void *password_opaque;
     mon_cmd_t *cmd_table;
     QError *error;
     QLIST_HEAD(,mon_fd_t) fds;
@@ -5350,81 +5348,11 @@ void monitor_init(CharDriverState *chr, int flags)
         default_mon = mon;
 }
 
-static void bdrv_password_cb(void *opaque, const char *password,
-                             void *readline_opaque)
-{
-    Monitor *mon = opaque;
-    BlockDriverState *bs = readline_opaque;
-    int ret = 0;
-    Error *local_err = NULL;
-
-    bdrv_add_key(bs, password, &local_err);
-    if (local_err) {
-        monitor_printf(mon, "%s\n", error_get_pretty(local_err));
-        error_free(local_err);
-        ret = -EPERM;
-    }
-    if (mon->password_completion_cb)
-        mon->password_completion_cb(mon->password_opaque, ret);
-
-    monitor_read_command(mon, 1);
-}
-
 ReadLineState *monitor_get_rs(Monitor *mon)
 {
     return mon->rs;
 }
 
-int monitor_read_bdrv_key_start(Monitor *mon, BlockDriverState *bs,
-                                BlockCompletionFunc *completion_cb,
-                                void *opaque)
-{
-    Error *local_err = NULL;
-    int err;
-
-    bdrv_add_key(bs, NULL, &local_err);
-    if (!local_err) {
-        if (completion_cb)
-            completion_cb(opaque, 0);
-        return 0;
-    }
-
-    /* Need a key for @bs */
-
-    if (monitor_ctrl_mode(mon)) {
-        qerror_report_err(local_err);
-        return -1;
-    }
-
-    monitor_printf(mon, "%s (%s) is encrypted.\n", bdrv_get_device_name(bs),
-                   bdrv_get_encrypted_filename(bs));
-
-    mon->password_completion_cb = completion_cb;
-    mon->password_opaque = opaque;
-
-    err = monitor_read_password(mon, bdrv_password_cb, bs);
-
-    if (err && completion_cb)
-        completion_cb(opaque, err);
-
-    return err;
-}
-
-int monitor_read_block_device_key(Monitor *mon, const char *device,
-                                  BlockCompletionFunc *completion_cb,
-                                  void *opaque)
-{
-    BlockDriverState *bs;
-
-    bs = bdrv_find(device);
-    if (!bs) {
-        monitor_printf(mon, "Device not found %s\n", device);
-        return -1;
-    }
-
-    return monitor_read_bdrv_key_start(mon, bs, completion_cb, opaque);
-}
-
 QemuOptsList qemu_mon_opts = {
     .name = "mon",
     .implied_opt_name = "chardev",
diff --git a/qapi-schema.json b/qapi-schema.json
index e16f8eb..cb7ba49 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1234,10 +1234,6 @@
 #
 # Since:  0.14.0
 #
-# Returns:  If successful, nothing
-#           If QEMU was started with an encrypted block device and a key has
-#              not yet been set, DeviceEncrypted.
-#
 # Notes:  This command will succeed if the guest is currently running.  It
 #         will also succeed if the guest is in the "inmigrate" state; in
 #         this case, the effect of the command is to make sure the guest
@@ -1385,8 +1381,8 @@
 #        o This command is stateless, this means that commands that depend
 #          on state information (such as getfd) might not work
 #
-#       o Commands that prompt the user for data (eg. 'cont' when the block
-#         device is encrypted) don't currently work
+#        o Commands that prompt the user for data (eg. 'change vnc
+#          password') don't currently work
 ##
 { 'command': 'human-monitor-command',
   'data': {'command-line': 'str', '*cpu-index': 'int'},
@@ -1642,11 +1638,6 @@
 #
 # Returns: Nothing on success.
 #          If @device is not a valid block device, DeviceNotFound
-#          If the new block device is encrypted, DeviceEncrypted.  Note that
-#          if this error is returned, the device has been opened successfully
-#          and an additional call to @block_passwd is required to set the
-#          device's password.  The behavior of reads and writes to the block
-#          device between when these calls are executed is undefined.
 #
 # Notes:  It is strongly recommended that this interface is not used especially
 #         for changing block devices.
diff --git a/qapi/block-core.json b/qapi/block-core.json
index a3fdaf0..9a5aa37 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -220,10 +220,9 @@
 #
 # @backing_file_depth: number of files in the backing file chain (since: 1.2)
 #
-# @encrypted: true if the backing device is encrypted
+# @encrypted: for backward compatibility, always false
 #
-# @encryption_key_missing: true if the backing device is encrypted but an
-#                          valid encryption key is missing
+# @encryption_key_missing: for backward compatibility, always false
 #
 # @detect_zeroes: detect and optimize zero writes (Since 2.1)
 #
@@ -573,43 +572,6 @@
 { 'command': 'query-block-jobs', 'returns': ['BlockJobInfo'] }
 
 ##
-# @block_passwd:
-#
-# This command sets the password of a block device that has not been open
-# with a password and requires one.
-#
-# The two cases where this can happen are a block device is created through
-# QEMU's initial command line or a block device is changed through the legacy
-# @change interface.
-#
-# In the event that the block device is created through the initial command
-# line, the VM will start in the stopped state regardless of whether '-S' is
-# used.  The intention is for a management tool to query the block devices to
-# determine which ones are encrypted, set the passwords with this command, and
-# then start the guest with the @cont command.
-#
-# Either @device or @node-name must be set but not both.
-#
-# @device: #optional the name of the block backend device to set the password on
-#
-# @node-name: #optional graph node name to set the password on (Since 2.0)
-#
-# @password: the password to use for the device
-#
-# Returns: nothing on success
-#          If @device is not a valid block device, DeviceNotFound
-#          If @device is not encrypted, DeviceNotEncrypted
-#
-# Notes:  Not all block formats support encryption and some that do are not
-#         able to validate that a password is correct.  Disk corruption may
-#         occur if an invalid password is specified.
-#
-# Since: 0.14.0
-##
-{ 'command': 'block_passwd', 'data': {'*device': 'str',
-                                      '*node-name': 'str', 'password': 'str'} }
-
-##
 # @block_resize
 #
 # Resize a block image while a guest is running.
diff --git a/qapi/common.json b/qapi/common.json
index 63ef3b4..84b5cb4 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -12,9 +12,6 @@
 #
 # @CommandNotFound: the requested command has not been found
 #
-# @DeviceEncrypted: the requested operation can't be fulfilled because the
-#                   selected device is encrypted
-#
 # @DeviceNotActive: a device has failed to be become active
 #
 # @DeviceNotFound: the requested device has not been found
@@ -25,7 +22,7 @@
 # Since: 1.2
 ##
 { 'enum': 'ErrorClass',
-  'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted',
+  'data': [ 'GenericError', 'CommandNotFound',
             'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a85d847..0d202ea 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1677,32 +1677,6 @@ Note: The list of fd sets is shared by all monitor connections.
 EQMP
 
     {
-        .name       = "block_passwd",
-        .args_type  = "device:s?,node-name:s?,password:s",
-        .mhandler.cmd_new = qmp_marshal_input_block_passwd,
-    },
-
-SQMP
-block_passwd
-------------
-
-Set the password of encrypted block devices.
-
-Arguments:
-
-- "device": device name (json-string)
-- "node-name": name in the block driver state graph (json-string)
-- "password": password (json-string)
-
-Example:
-
--> { "execute": "block_passwd", "arguments": { "device": "ide0-hd0",
-                                               "password": "12345" } }
-<- { "return": {} }
-
-EQMP
-
-    {
         .name       = "block_set_io_throttle",
         .args_type  = "device:B,bps:l,bps_rd:l,bps_wr:l,iops:l,iops_rd:l,iops_wr:l,bps_max:l?,bps_rd_max:l?,bps_wr_max:l?,iops_max:l?,iops_rd_max:l?,iops_wr_max:l?,iops_size:l?",
         .mhandler.cmd_new = qmp_marshal_input_block_set_io_throttle,
diff --git a/qmp.c b/qmp.c
index d701cff..8ec9b03 100644
--- a/qmp.c
+++ b/qmp.c
@@ -160,7 +160,6 @@ SpiceInfo *qmp_query_spice(Error **errp)
 
 void qmp_cont(Error **errp)
 {
-    Error *local_err = NULL;
     BlockDriverState *bs;
 
     if (runstate_needs_reset()) {
@@ -173,13 +172,6 @@ void qmp_cont(Error **errp)
     for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
         bdrv_iostatus_reset(bs);
     }
-    for (bs = bdrv_next(NULL); bs; bs = bdrv_next(bs)) {
-        bdrv_add_key(bs, NULL, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            return;
-        }
-    }
 
     if (runstate_check(RUN_STATE_INMIGRATE)) {
         autostart = 1;
-- 
1.9.3

Re: [Qemu-devel] [PATCH RFC 2/2] block: Drop code supporting encryption outside qemu-img

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 3273 bytes --]

On 03/10/2015 11:26 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  block.c                   | 30 --------------------
>  blockdev.c                | 43 +---------------------------
>  hmp-commands.hx           | 14 ---------
>  hmp.c                     | 41 ---------------------------
>  hmp.h                     |  1 -
>  hw/usb/dev-storage.c      | 26 -----------------
>  include/monitor/monitor.h |  7 -----
>  monitor.c                 | 72 -----------------------------------------------
>  qapi-schema.json          | 13 ++-------
>  qapi/block-core.json      | 42 ++-------------------------
>  qapi/common.json          |  5 +---
>  qmp-commands.hx           | 26 -----------------
>  qmp.c                     |  8 ------
>  13 files changed, 6 insertions(+), 322 deletions(-)
> 

> +++ b/qapi/block-core.json

>  ##
> -# @block_passwd:
> -#
> -# This command sets the password of a block device that has not been open
> -# with a password and requires one.
> -#
> -# The two cases where this can happen are a block device is created through
> -# QEMU's initial command line or a block device is changed through the legacy
> -# @change interface.
> -#
> -# In the event that the block device is created through the initial command
> -# line, the VM will start in the stopped state regardless of whether '-S' is
> -# used.  The intention is for a management tool to query the block devices to
> -# determine which ones are encrypted, set the passwords with this command, and
> -# then start the guest with the @cont command.
> -#
> -# Either @device or @node-name must be set but not both.
> -#
> -# @device: #optional the name of the block backend device to set the password on
> -#
> -# @node-name: #optional graph node name to set the password on (Since 2.0)
> -#
> -# @password: the password to use for the device
> -#
> -# Returns: nothing on success
> -#          If @device is not a valid block device, DeviceNotFound
> -#          If @device is not encrypted, DeviceNotEncrypted
> -#
> -# Notes:  Not all block formats support encryption and some that do are not
> -#         able to validate that a password is correct.  Disk corruption may
> -#         occur if an invalid password is specified.
> -#
> -# Since: 0.14.0
> -##
> -{ 'command': 'block_passwd', 'data': {'*device': 'str',
> -                                      '*node-name': 'str', 'password': 'str'} }

Good - removing this command means 'query-commands' will have an easy
probe for whether qemu is in the window of time where old broken
encryption could even be attempted, or when a newer (hopefully!) qemu
can support sane LUKS encryption, so that libvirt can issue sane errors
to the user telling them that their qemu cannot support encryption.

I agree with the decision of removing the existing crufty interface so
that any future additions can add in a working design from the get-go,
rather than trying to retrofit fixes for all of the confusing aspects
that you pointed out.  As such, I could live with:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Daniel P. Berrange]

On Tue, Mar 10, 2015 at 06:26:38PM +0100, Markus Armbruster wrote:
> RFC because the series only covers open [PATCH 1], but not create.
> Also missing: make qemu-img print a warning when it creates an
> encrypted image.  Finally, some of the material in the cover letter
> should be worked into the commit messages.
> 
> We've steered users away from QCOW/QCOW2 encryption for a while,
> because it's a flawed design (commit 136cd19 Describe flaws in
> qcow/qcow2 encryption in the docs).
> 
> In addition to flawed crypto, we have comically bad usability, and
> plain old bugs.  Let me show you.


> = Usability issues =

> == Confusing startup ==
> == Incorrect passwords not caught ==
> == Need to stop the guest to add an encrypted image ==
> == Use without key is not always caught ==
> == QMP device_add of usb-storage fails when it shouldn't ==

So there's really two separate root cuase problems we're facing
here. One of the usability issues is inherant artifact of the
qcow design. The other 4 issues are all due to the badly designed
block driver / monitor key management approach.

> If people become sufficiently interested in encrypted images to
> contribute a cryptographically sane implementation for QCOW2 (or
> whatever other format), then rewriting the necessary support around it
> from scratch will likely be easier and yield better results than
> fixing up the existing mess.
> 
> Let's drop the mess and move on.  Keep qemu-img convert working, of
> course, to let users rescue their data.

Once I've got through the current work i'm doing on TLS support
for migrate/nbd/chardev/etc, my intention is to work on adding
support for the LUKS format to QEMU. We really need this natively
in OpenStack since we're increasingly using the QEMU native client
for nbd, iscsi, nfs, etc but at the same time don't want to sacrifice
encryption which we currently do via LUKS. It is probably a good
4-6 months though before I get on to working on this.

I agree with all your points about the usability being fubar. This
clearly needs to be fixed for encryption support to be viable in
QEMU, regardless of the actual encryption format used.

I guess my question is whether it is worth trying to fix the blockdev
integration part of things now, or to rip it out now and reimplement
it from scratch later ?  I think I probably agree with killing it
now, since it might actually make doing a sensible impl easier later
on.

And lets assume we do eventually have a fixed blockdev layer and a
sane LUKS encryption driver, would we still want to kill off qcow2
encryption ?  Given the way subpar encryption is being actively
attacked by everyone & their dog, I think mandatory retirement of
qcow2 encryption is a good idea sooner rather than later.

My only concern here is whether we've given users enough prior
warning. While we added that doc change a year ago, what are the
odds that anyone has actually read those docs & noticed the warning.
Should we have one major release where we log a deprecation warning
on stderr, informing users of an explicit timeframe for its removal,
before we actually use the big hammer of disabling it permanently ?



FWIW, I could see an improved interaction scheme working as follows

First, introduce a new monitor command for setting named passwords,

    add_key mykey1 SECRETDATA

Now, extend the blockdev_add so that you can provide key names
by adding

    'keyname': 'mykey1'

as a parameter in the json args.

If an attempt is made to add a blockdev without having provided a
key, the attempt should just fail. This avoids all the insanity
around delayed opening of files, as well as avoiding need to stop
the guest to add devices.

For cold plug, have a command line arg '--add-keys prompt' to
indicate the user should be prompted on TTY to enter keys, which
is good for interactive usage. For managed usage we could allow
'--add-keys fd=FDNUM' and just read keys from the file descriptor.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Tue, Mar 10, 2015 at 06:26:38PM +0100, Markus Armbruster wrote:
>> RFC because the series only covers open [PATCH 1], but not create.
>> Also missing: make qemu-img print a warning when it creates an
>> encrypted image.  Finally, some of the material in the cover letter
>> should be worked into the commit messages.
>> 
>> We've steered users away from QCOW/QCOW2 encryption for a while,
>> because it's a flawed design (commit 136cd19 Describe flaws in
>> qcow/qcow2 encryption in the docs).
>> 
>> In addition to flawed crypto, we have comically bad usability, and
>> plain old bugs.  Let me show you.
>
>
>> = Usability issues =
>
>> == Confusing startup ==
>> == Incorrect passwords not caught ==
>> == Need to stop the guest to add an encrypted image ==
>> == Use without key is not always caught ==
>> == QMP device_add of usb-storage fails when it shouldn't ==
>
> So there's really two separate root cuase problems we're facing
> here. One of the usability issues is inherant artifact of the
> qcow design. The other 4 issues are all due to the badly designed
> block driver / monitor key management approach.

Yup.  The latter is fixable, but not worth fixing as long as the
underlying crypto isn't worth using.

>> If people become sufficiently interested in encrypted images to
>> contribute a cryptographically sane implementation for QCOW2 (or
>> whatever other format), then rewriting the necessary support around it
>> from scratch will likely be easier and yield better results than
>> fixing up the existing mess.
>> 
>> Let's drop the mess and move on.  Keep qemu-img convert working, of
>> course, to let users rescue their data.
>
> Once I've got through the current work i'm doing on TLS support
> for migrate/nbd/chardev/etc, my intention is to work on adding
> support for the LUKS format to QEMU. We really need this natively
> in OpenStack since we're increasingly using the QEMU native client
> for nbd, iscsi, nfs, etc but at the same time don't want to sacrifice
> encryption which we currently do via LUKS. It is probably a good
> 4-6 months though before I get on to working on this.

Great!

> I agree with all your points about the usability being fubar. This
> clearly needs to be fixed for encryption support to be viable in
> QEMU, regardless of the actual encryption format used.
>
> I guess my question is whether it is worth trying to fix the blockdev
> integration part of things now, or to rip it out now and reimplement
> it from scratch later ?  I think I probably agree with killing it
> now, since it might actually make doing a sensible impl easier later
> on.

In your shoes, I'd certainly prefer to drop the current mess and start
over.  Not just because I think it'll make the work easier, but also
because I'd want to break out of the back-compat strait-jacket, to be
free to create the best user interface I can.

> And lets assume we do eventually have a fixed blockdev layer and a
> sane LUKS encryption driver, would we still want to kill off qcow2
> encryption ?  Given the way subpar encryption is being actively
> attacked by everyone & their dog, I think mandatory retirement of
> qcow2 encryption is a good idea sooner rather than later.

I strongly believe retiring it is a public service.

Except for qemu-img.  I'm prepared to keep it there for a long time,
maybe forever.

> My only concern here is whether we've given users enough prior
> warning. While we added that doc change a year ago, what are the
> odds that anyone has actually read those docs & noticed the warning.
> Should we have one major release where we log a deprecation warning
> on stderr, informing users of an explicit timeframe for its removal,
> before we actually use the big hammer of disabling it permanently ?

I'm fine with that.  In fact, I could agree to pretty much any
deprecation schedule, as long as we start it *now*.

> FWIW, I could see an improved interaction scheme working as follows
>
> First, introduce a new monitor command for setting named passwords,
>
>     add_key mykey1 SECRETDATA
>
> Now, extend the blockdev_add so that you can provide key names
> by adding
>
>     'keyname': 'mykey1'
>
> as a parameter in the json args.

Can you explain why that's better than sticking 'key': SECRETDATA right
into blockdev-add's arguments?

> If an attempt is made to add a blockdev without having provided a
> key, the attempt should just fail. This avoids all the insanity
> around delayed opening of files, as well as avoiding need to stop
> the guest to add devices.

Yes, we need to exterminate the awkward and dangerous intermediate state
"image opened, but lacks decryption key".  This series avoids it (except
in qemu-img, but there it's safely confined to img_open()).  Future work
should not bring it back.

> For cold plug, have a command line arg '--add-keys prompt' to
> indicate the user should be prompted on TTY to enter keys, which
> is good for interactive usage. For managed usage we could allow
> '--add-keys fd=FDNUM' and just read keys from the file descriptor.

I guess sugared command line like "-hda geheim.qcow2" should simply
prompt for the key right in the tty.  None of this nonsense with not
starting the guest, and having "cont" prompt in the monitor.

Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Daniel P. Berrange]

On Wed, Mar 11, 2015 at 09:55:16AM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > On Tue, Mar 10, 2015 at 06:26:38PM +0100, Markus Armbruster wrote:
> >> RFC because the series only covers open [PATCH 1], but not create.
> >> Also missing: make qemu-img print a warning when it creates an
> >> encrypted image.  Finally, some of the material in the cover letter
> >> should be worked into the commit messages.
> >> 
> >> We've steered users away from QCOW/QCOW2 encryption for a while,
> >> because it's a flawed design (commit 136cd19 Describe flaws in
> >> qcow/qcow2 encryption in the docs).
> >> 
> >> In addition to flawed crypto, we have comically bad usability, and
> >> plain old bugs.  Let me show you.
> >
> >
> >> = Usability issues =
> >
> >> == Confusing startup ==
> >> == Incorrect passwords not caught ==
> >> == Need to stop the guest to add an encrypted image ==
> >> == Use without key is not always caught ==
> >> == QMP device_add of usb-storage fails when it shouldn't ==
> >
> > So there's really two separate root cuase problems we're facing
> > here. One of the usability issues is inherant artifact of the
> > qcow design. The other 4 issues are all due to the badly designed
> > block driver / monitor key management approach.
> 
> Yup.  The latter is fixable, but not worth fixing as long as the
> underlying crypto isn't worth using.
> 
> >> If people become sufficiently interested in encrypted images to
> >> contribute a cryptographically sane implementation for QCOW2 (or
> >> whatever other format), then rewriting the necessary support around it
> >> from scratch will likely be easier and yield better results than
> >> fixing up the existing mess.
> >> 
> >> Let's drop the mess and move on.  Keep qemu-img convert working, of
> >> course, to let users rescue their data.
> >
> > Once I've got through the current work i'm doing on TLS support
> > for migrate/nbd/chardev/etc, my intention is to work on adding
> > support for the LUKS format to QEMU. We really need this natively
> > in OpenStack since we're increasingly using the QEMU native client
> > for nbd, iscsi, nfs, etc but at the same time don't want to sacrifice
> > encryption which we currently do via LUKS. It is probably a good
> > 4-6 months though before I get on to working on this.
> 
> Great!
> 
> > I agree with all your points about the usability being fubar. This
> > clearly needs to be fixed for encryption support to be viable in
> > QEMU, regardless of the actual encryption format used.
> >
> > I guess my question is whether it is worth trying to fix the blockdev
> > integration part of things now, or to rip it out now and reimplement
> > it from scratch later ?  I think I probably agree with killing it
> > now, since it might actually make doing a sensible impl easier later
> > on.
> 
> In your shoes, I'd certainly prefer to drop the current mess and start
> over.  Not just because I think it'll make the work easier, but also
> because I'd want to break out of the back-compat strait-jacket, to be
> free to create the best user interface I can.
> 
> > And lets assume we do eventually have a fixed blockdev layer and a
> > sane LUKS encryption driver, would we still want to kill off qcow2
> > encryption ?  Given the way subpar encryption is being actively
> > attacked by everyone & their dog, I think mandatory retirement of
> > qcow2 encryption is a good idea sooner rather than later.
> 
> I strongly believe retiring it is a public service.
> 
> Except for qemu-img.  I'm prepared to keep it there for a long time,
> maybe forever.
> 
> > My only concern here is whether we've given users enough prior
> > warning. While we added that doc change a year ago, what are the
> > odds that anyone has actually read those docs & noticed the warning.
> > Should we have one major release where we log a deprecation warning
> > on stderr, informing users of an explicit timeframe for its removal,
> > before we actually use the big hammer of disabling it permanently ?
> 
> I'm fine with that.  In fact, I could agree to pretty much any
> deprecation schedule, as long as we start it *now*.

IIUC, the 2.3.0 major branch is targetted for end of this month,
so we could put in code that issues a deprecation warning on
stderr for 2.3.0, and then delete all this stuff when GIT master
opens for 2.4.0 development ?

> 
> > FWIW, I could see an improved interaction scheme working as follows
> >
> > First, introduce a new monitor command for setting named passwords,
> >
> >     add_key mykey1 SECRETDATA
> >
> > Now, extend the blockdev_add so that you can provide key names
> > by adding
> >
> >     'keyname': 'mykey1'
> >
> > as a parameter in the json args.
> 
> Can you explain why that's better than sticking 'key': SECRETDATA right
> into blockdev-add's arguments?

Just have a small preference to keep passwords separated from the
rest of the data, so when logging the stuff for debug purposes we
don't compromise people's passwords quite so readily. It is more
straightforward for us to mask out the passwords if we can just
match on the command name, and not have to try to grok the specific
field in a large set of args.  Also in terms of cold startup, it
is not desirable to have the password directly included in the
args to -drive or equiv, as that's visible in process listings.

> > If an attempt is made to add a blockdev without having provided a
> > key, the attempt should just fail. This avoids all the insanity
> > around delayed opening of files, as well as avoiding need to stop
> > the guest to add devices.
> 
> Yes, we need to exterminate the awkward and dangerous intermediate state
> "image opened, but lacks decryption key".  This series avoids it (except
> in qemu-img, but there it's safely confined to img_open()).  Future work
> should not bring it back.
> 
> > For cold plug, have a command line arg '--add-keys prompt' to
> > indicate the user should be prompted on TTY to enter keys, which
> > is good for interactive usage. For managed usage we could allow
> > '--add-keys fd=FDNUM' and just read keys from the file descriptor.
> 
> I guess sugared command line like "-hda geheim.qcow2" should simply
> prompt for the key right in the tty.  None of this nonsense with not
> starting the guest, and having "cont" prompt in the monitor.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Kevin Wolf]

Am 11.03.2015 um 10:59 hat Daniel P. Berrange geschrieben:
> On Wed, Mar 11, 2015 at 09:55:16AM +0100, Markus Armbruster wrote:
> > "Daniel P. Berrange" <berrange@redhat.com> writes:
> > > FWIW, I could see an improved interaction scheme working as follows
> > >
> > > First, introduce a new monitor command for setting named passwords,
> > >
> > >     add_key mykey1 SECRETDATA
> > >
> > > Now, extend the blockdev_add so that you can provide key names
> > > by adding
> > >
> > >     'keyname': 'mykey1'
> > >
> > > as a parameter in the json args.
> > 
> > Can you explain why that's better than sticking 'key': SECRETDATA right
> > into blockdev-add's arguments?
> 
> Just have a small preference to keep passwords separated from the
> rest of the data, so when logging the stuff for debug purposes we
> don't compromise people's passwords quite so readily.

Indeed, it would be very easy for a password to end up in error
messages, or in json: "filenames" that might be used in query-block
replies or in a backing file path. BDS options should be considered
more or less public.

> It is more
> straightforward for us to mask out the passwords if we can just
> match on the command name, and not have to try to grok the specific
> field in a large set of args.  Also in terms of cold startup, it
> is not desirable to have the password directly included in the
> args to -drive or equiv, as that's visible in process listings.

Right, that too.

Kevin

Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Wed, Mar 11, 2015 at 09:55:16AM +0100, Markus Armbruster wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
[...]
>> > My only concern here is whether we've given users enough prior
>> > warning. While we added that doc change a year ago, what are the
>> > odds that anyone has actually read those docs & noticed the warning.
>> > Should we have one major release where we log a deprecation warning
>> > on stderr, informing users of an explicit timeframe for its removal,
>> > before we actually use the big hammer of disabling it permanently ?
>> 
>> I'm fine with that.  In fact, I could agree to pretty much any
>> deprecation schedule, as long as we start it *now*.
>
> IIUC, the 2.3.0 major branch is targetted for end of this month,
> so we could put in code that issues a deprecation warning on
> stderr for 2.3.0, and then delete all this stuff when GIT master
> opens for 2.4.0 development ?

Let's do that.  Kevin, any objections?

>> > FWIW, I could see an improved interaction scheme working as follows
>> >
>> > First, introduce a new monitor command for setting named passwords,
>> >
>> >     add_key mykey1 SECRETDATA
>> >
>> > Now, extend the blockdev_add so that you can provide key names
>> > by adding
>> >
>> >     'keyname': 'mykey1'
>> >
>> > as a parameter in the json args.
>> 
>> Can you explain why that's better than sticking 'key': SECRETDATA right
>> into blockdev-add's arguments?
>
> Just have a small preference to keep passwords separated from the
> rest of the data, so when logging the stuff for debug purposes we
> don't compromise people's passwords quite so readily. It is more
> straightforward for us to mask out the passwords if we can just
> match on the command name, and not have to try to grok the specific
> field in a large set of args.

Makes sense.

>                                Also in terms of cold startup, it
> is not desirable to have the password directly included in the
> args to -drive or equiv, as that's visible in process listings.

The obvious command line use should prompt for the key.

A reasonably safe non-interactive way to start with an encrypted image
would be nice, but I haven't considered details.

[...]

Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Paolo Bonzini]

On 10/03/2015 19:13, Daniel P. Berrange wrote:
> FWIW, I could see an improved interaction scheme working as follows
> 
> First, introduce a new monitor command for setting named passwords,
> 
>     add_key mykey1 SECRETDATA

Or reuse object_add:

    object_add secret,id=mykey1,secret=SECRETDATA

> Now, extend the blockdev_add so that you can provide key names
> by adding
> 
>     'keyname': 'mykey1'
> 
> as a parameter in the json args.

You can also add a command line option:

   -secret id=mykey1,secret=SECRETDATA

or possibly:

   -object secret,id=mykey1,secret=SECRETDATA

> For cold plug, have a command line arg '--add-keys prompt' to
> indicate the user should be prompted on TTY to enter keys,

This can even be the default if you have a human monitor open.
(Downside: the default human monitor, accessible with Ctrl-Alt-2, is not
easily discovered; same for Ctrl-A c for -nographic).

> For managed usage we could allow
> '--add-keys fd=FDNUM' and just read keys from the file descriptor.

For managed usage, options can also be passed via -readconfig like

   [object "mykey1"]
   type=secret
   secret=SECRETDATA

Paolo

Re: [Qemu-devel] [PATCH RFC 0/2] Limit support for encrypted images to qemu-img

[Kevin Wolf]

Am 12.03.2015 um 17:58 hat Paolo Bonzini geschrieben:
> > For cold plug, have a command line arg '--add-keys prompt' to
> > indicate the user should be prompted on TTY to enter keys,
> 
> This can even be the default if you have a human monitor open.
> (Downside: the default human monitor, accessible with Ctrl-Alt-2, is not
> easily discovered; same for Ctrl-A c for -nographic).

In some ancient version this actually worked as expected: When you
started a VM with an encrypted image, the HMP monitor was active, and
after providing the password, it switched to the graphical output.

> > For managed usage we could allow
> > '--add-keys fd=FDNUM' and just read keys from the file descriptor.
> 
> For managed usage, options can also be passed via -readconfig like
> 
>    [object "mykey1"]
>    type=secret
>    secret=SECRETDATA

Hopefully not using a real file, but /dev/fdset/something.

Kevin