Re: [PATCH 06/13] block/crypto: implement the encryption key management

["Daniel P. Berrangé" <berrange@redhat.com>]

On Tue, Jan 14, 2020 at 09:33:43PM +0200, Maxim Levitsky wrote:
> This implements the encryption key management using the generic code in
> qcrypto layer and exposes it to the user via qemu-img
> 
> This code adds another 'write_func' because the initialization
> write_func works directly on the underlying file, and amend
> works on instance of luks device.
> 
> This commit also adds a 'hack/workaround' I and Kevin Wolf (thanks)
> made to make the driver both support write sharing (to avoid breaking the users),
> and be safe against concurrent  metadata update (the keyslots)
> 
> Eventually the write sharing for luks driver will be deprecated
> and removed together with this hack.
> 
> The hack is that we ask (as a format driver) for BLK_PERM_CONSISTENT_READ
> and then when we want to update the keys, we unshare that permission.
> So if someone else has the image open, even readonly, encryption
> key update will fail gracefully.
> 
> Also thanks to Daniel Berrange for the idea of
> unsharing read, rather that write permission which allows
> to avoid cases when the other user had opened the image read-only.
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/crypto.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++--
>  block/crypto.h |  31 ++++++++++++
>  2 files changed, 158 insertions(+), 3 deletions(-)
> 

> @@ -148,6 +167,22 @@ static QemuOptsList block_crypto_create_opts_luks = {
>  };
>  
>  
> +static QemuOptsList block_crypto_amend_opts_luks = {
> +    .name = "crypto",
> +    .head = QTAILQ_HEAD_INITIALIZER(block_crypto_create_opts_luks.head),
> +    .desc = {
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.0."),
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.1."),
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.2."),
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.3."),
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.4."),
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.5."),
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.6."),
> +        BLOCK_CRYPTO_OPT_DEF_LUKS_KEYSLOT_UPDATE("keys.7."),

I'd probably suggest  "key.0" or "keyslot.0" as a name.

> +        { /* end of list */ }
> +    },
> +};
> +


> @@ -661,6 +696,95 @@ block_crypto_get_specific_info_luks(BlockDriverState *bs, Error **errp)
>      return spec_info;
>  }
>  
> +static int
> +block_crypto_amend_options(BlockDriverState *bs,
> +                           QemuOpts *opts,
> +                           BlockDriverAmendStatusCB *status_cb,
> +                           void *cb_opaque,
> +                           bool force,
> +                           Error **errp)

This method should have a "_luks" suffix since...

> +{
> +    BlockCrypto *crypto = bs->opaque;
> +    QDict *cryptoopts = NULL;
> +    QCryptoBlockAmendOptions *amend_options = NULL;
> +    int ret;
> +
> +    assert(crypto);
> +    assert(crypto->block);
> +    crypto->updating_keys = true;
> +
> +    ret = bdrv_child_refresh_perms(bs, bs->file, errp);
> +    if (ret < 0) {
> +        goto cleanup;
> +    }
> +
> +    cryptoopts = qemu_opts_to_qdict(opts, NULL);
> +    qdict_put_str(cryptoopts, "format", "luks");

...it is hardcoded here to assume luks.

> +    amend_options = block_crypto_amend_opts_init(cryptoopts, errp);
> +    if (!amend_options) {
> +        ret = -EINVAL;
> +        goto cleanup;
> +    }
> +
> +    ret = qcrypto_block_amend_options(crypto->block,
> +                                      block_crypto_read_func,
> +                                      block_crypto_write_func,
> +                                      bs,
> +                                      amend_options,
> +                                      force,
> +                                      errp);
> +cleanup:
> +    crypto->updating_keys = false;
> +    bdrv_child_refresh_perms(bs, bs->file, errp);
> +    qapi_free_QCryptoBlockAmendOptions(amend_options);
> +    qobject_unref(cryptoopts);
> +    return ret;
> +}

With the minor changes above

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|