Re: [Qemu-block] [PATCH v1 11/15] qcow2: convert QCow2 to use QCryptoBlock for encryption

[Max Reitz]

On 03.01.2017 19:27, Daniel P. Berrange wrote:
> This converts the qcow2 driver to make use of the QCryptoBlock
> APIs for encrypting image content, using the legacyy QCow2 AES
> scheme.
> 
> With this change it is now required to use the QCryptoSecret
> object for providing passwords, instead of the current block
> password APIs / interactive prompting.
> 
>   $QEMU \
>     -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
>     -drive file=/home/berrange/encrypted.qcow2,aes-key-secret=sec0
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  block/qcow2-cluster.c      |  47 +----------
>  block/qcow2.c              | 190 
> +++++++++++++++++++++++++++++----------------
>  block/qcow2.h              |   5 +-
>  qapi/block-core.json       |   7 +-
>  tests/qemu-iotests/049     |   2 +-
>  tests/qemu-iotests/049.out |   4 +-
>  tests/qemu-iotests/082.out |  27 +++++++
>  tests/qemu-iotests/087     |  28 ++++++-
>  tests/qemu-iotests/087.out |   6 +-
>  tests/qemu-iotests/134     |  18 +++--
>  tests/qemu-iotests/134.out |  10 +--
>  tests/qemu-iotests/158     |  19 +++--
>  tests/qemu-iotests/158.out |  14 +---
>  13 files changed, 219 insertions(+), 158 deletions(-)

[...]

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3c14c86..5c9e196 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -1122,6 +1144,24 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          goto fail;
>      }
>  
> +    if (s->crypt_method_header == QCOW_CRYPT_AES) {
> +        unsigned int cflags = 0;
> +        if (flags & BDRV_O_NO_IO) {
> +            cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> +        }
> +        /* XXX how do we pass the same crypto opts down to the

I think a TODO instead of an XXX would have been sufficient, but it's
your call.

> +         * backing file by default, so we don't have to manually
> +         * provide the same key-secret property against the full
> +         * backing chain
> +         */
> +        s->crypto = qcrypto_block_open(s->crypto_opts, NULL, NULL,
> +                                       cflags, errp);
> +        if (!s->crypto) {
> +            ret = -EINVAL;
> +            goto fail;
> +        }

[...]

> @@ -2022,6 +2027,44 @@ static int qcow2_change_backing_file(BlockDriverState 
> *bs,
>      return qcow2_update_header(bs);
>  }
>  
> +
> +static int qcow2_change_encryption(BlockDriverState *bs, QemuOpts *opts,
> +                                   Error **errp)

I think this name is not quite appropriate, since this doesn't change
the format of the file if it is already encrypted (and it will not
encrypt any existing data).

Maybe set_up instead of change?

(qcow2_change_backing_file()'s name is good because it will actually
work if there already is a different backing file.)

> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    QCryptoBlockCreateOptions *cryptoopts = NULL;
> +    QCryptoBlock *crypto = NULL;
> +    int ret = -EINVAL;

[...]

> diff --git a/block/qcow2.h b/block/qcow2.h
> index 033d8c0..f4cb171 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -25,7 +25,7 @@
>  #ifndef BLOCK_QCOW2_H
>  #define BLOCK_QCOW2_H
>  
> -#include "crypto/cipher.h"
> +#include "crypto/block.h"
>  #include "qemu/coroutine.h"
>  
>  //#define DEBUG_ALLOC
> @@ -256,7 +256,8 @@ typedef struct BDRVQcow2State {
>  
>      CoMutex lock;
>  
> -    QCryptoCipher *cipher; /* current cipher, NULL if no key yet */
> +    QCryptoBlockOpenOptions *crypto_opts; /* Disk encryption runtime options 
> */
> +    QCryptoBlock *crypto; /* Disk encryption format driver */
>      uint32_t crypt_method_header;
>      uint64_t snapshots_offset;
>      int snapshots_size;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index c2b70e8..2ca5674 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -1935,6 +1935,9 @@
>  # @cache-clean-interval:  #optional clean unused entries in the L2 and 
> refcount
>  #                         caches. The interval is in seconds. The default 
> value
>  #                         is 0 and it disables this feature (since 2.5)
> +# @aes-key-secret:        #optional the ID of a QCryptoSecret object 
> providing
> +#                         the AES decryption key (since 2.9) Mandatory except

Missing full stop after the closing parenthesis.

Also, it's mandatory only for encrypted images. I know it's obvious but
that's not what it says here.

> +#                         when doing a metadata-only probe of the image.
>  #
>  # Since: 1.7
>  ##
> @@ -1948,8 +1951,8 @@
>              '*cache-size': 'int',
>              '*l2-cache-size': 'int',
>              '*refcount-cache-size': 'int',
> -            '*cache-clean-interval': 'int' } }
> -
> +            '*cache-clean-interval': 'int',
> +            '*aes-key-secret': 'str' } }
>  
>  ##
>  # @BlockdevOptionsArchipelago:
> diff --git a/tests/qemu-iotests/049 b/tests/qemu-iotests/049
> index fff0760..7da4ac8 100755
> --- a/tests/qemu-iotests/049
> +++ b/tests/qemu-iotests/049
> @@ -106,7 +106,7 @@ test_qemu_img create -f $IMGFMT -o preallocation=1234 
> "$TEST_IMG" 64M
>  echo "== Check encryption option =="
>  echo
>  test_qemu_img create -f $IMGFMT -o encryption=off "$TEST_IMG" 64M
> -test_qemu_img create -f $IMGFMT -o encryption=on "$TEST_IMG" 64M
> +test_qemu_img create -f $IMGFMT --object secret,id=sec0,data=123456 -o 
> encryption=on,qcow-key-secret=sec0 "$TEST_IMG" 64M

s/qcow-key-secret/aes-key-secret/

>  
>  echo "== Check lazy_refcounts option (only with v3) =="
>  echo

[...]

> diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
> index 9de57dd..fe30383 100755
> --- a/tests/qemu-iotests/087
> +++ b/tests/qemu-iotests/087
> @@ -124,9 +124,18 @@ echo
>  echo === Encrypted image ===
>  echo
>  
> -_make_test_img -o encryption=on $size
> +_make_test_img --object secret,id=sec0,data=123456 -o 
> encryption=on,qcow-key-secret=sec0 $size
>  run_qemu -S <<EOF
>  { "execute": "qmp_capabilities" }
> +{ "execute": "object-add",
> +  "arguments": {
> +      "qom-type": "secret",
> +      "id": "sec0",
> +      "props": {
> +          "data": "123456"
> +      }
> +  }
> +}
>  { "execute": "blockdev-add",
>    "arguments": {
>        "driver": "$IMGFMT",
> @@ -134,7 +143,8 @@ run_qemu -S <<EOF
>        "file": {
>            "driver": "file",
>            "filename": "$TEST_IMG"
> -      }
> +      },
> +      "qcow-key-secret": "sec0"

Same here,

>      }
>    }
>  { "execute": "quit" }
> @@ -142,6 +152,15 @@ EOF
>  
>  run_qemu <<EOF
>  { "execute": "qmp_capabilities" }
> +{ "execute": "object-add",
> +  "arguments": {
> +      "qom-type": "secret",
> +      "id": "sec0",
> +      "props": {
> +          "data": "123456"
> +      }
> +  }
> +}
>  { "execute": "blockdev-add",
>    "arguments": {
>        "driver": "$IMGFMT",
> @@ -149,7 +168,8 @@ run_qemu <<EOF
>        "file": {
>            "driver": "file",
>            "filename": "$TEST_IMG"
> -      }
> +      },
> +      "qcow-key-secret": "sec0"

here,

>      }
>    }
>  { "execute": "quit" }
> @@ -159,7 +179,7 @@ echo
>  echo === Missing driver ===
>  echo
>  
> -_make_test_img -o encryption=on $size
> +_make_test_img --object secret,id=sec0,data=123456 -o 
> encryption=on,qcow-key-secret=sec0 $size

here,

>  run_qemu -S <<EOF
>  { "execute": "qmp_capabilities" }
>  { "execute": "blockdev-add",

[...]

> diff --git a/tests/qemu-iotests/134 b/tests/qemu-iotests/134
> index af618b8..c2458d8 100755
> --- a/tests/qemu-iotests/134
> +++ b/tests/qemu-iotests/134
> @@ -43,23 +43,31 @@ _supported_os Linux
>  
>  
>  size=128M
> -IMGOPTS="encryption=on" _make_test_img $size
> +
> +SECRET="secret,id=sec0,data=astrochicken"
> +SECRETALT="secret,id=sec0,data=platypus"
> +
> +_make_test_img --object $SECRET -o "encryption=on,qcow-key-secret=sec0" $size

here,

> +
> +IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,qcow-key-secret=sec0"

here,

> +
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT

[...]

> diff --git a/tests/qemu-iotests/158 b/tests/qemu-iotests/158
> index a6cdd6d..2d1c015 100755
> --- a/tests/qemu-iotests/158
> +++ b/tests/qemu-iotests/158
> @@ -44,34 +44,39 @@ _supported_os Linux
>  
>  size=128M
>  TEST_IMG_BASE=$TEST_IMG.base
> +SECRET="secret,id=sec0,data=astrochicken"
>  
>  TEST_IMG_SAVE=$TEST_IMG
>  TEST_IMG=$TEST_IMG_BASE
>  echo "== create base =="
> -IMGOPTS="encryption=on" _make_test_img $size
> +_make_test_img --object $SECRET -o "encryption=on,qcow-key-secret=sec0" $size

here,

>  TEST_IMG=$TEST_IMG_SAVE
>  
> +IMGSPECBASE="driver=$IMGFMT,file.filename=$TEST_IMG_BASE,qcow-key-secret=sec0"
> +IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,backing.driver=$IMGFMT,backing.file.filename=$TEST_IMG_BASE,backing.qcow-key-secret=sec0,qcow-key-secret=sec0"

and here.

Max

> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
> +

[...]

signature.asc
Description: OpenPGP digital signature