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

[Maxim Levitsky]

On Tue, 2020-01-28 at 17:27 +0000, Daniel P. Berrangé wrote:
> 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 <address@hidden>
> > ---
> >  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.
To be honest, I don't like either of these. 

I don't like the 'keys'
array, because it is a bit misleading, as basically each 'key' is a command
that can add/erase an arbitrary keyslot.

I would call this 'command' or cmd at least.

Also note that the 'keys' here is passed as is to qmp parser so if I change it 
here, I will have probably
to update the qmp version as well and there the 'keys' name is more or less 
agreed upon.
Thoughts?


> 
> > +        { /* 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...

Oops, thanks!
> 
> > +{
> > +    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é <address@hidden>

Best regards,
        Thanks for the review,
                Maxim Levitsky


> 
> 
> Regards,
> Daniel