Re: [Qemu-block] [PATCH 02/10] qcrypto-luks: extend the create options for upcoming encryption key management

[Maxim Levitsky]

On Fri, 2019-09-06 at 14:49 +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 30, 2019 at 11:56:00PM +0300, Maxim Levitsky wrote:
> > Now you can specify which slot to put the encryption key to
> > Plus add 'active' option which will let  user erase the key secret
> > instead of adding it.
> > Check that it is true for creation
> > 
> > Signed-off-by: Maxim Levitsky <address@hidden>
> > ---
> >  block/crypto.c             |  2 ++
> >  block/crypto.h             | 16 +++++++++++
> >  block/qcow2.c              |  2 ++
> >  crypto/block-luks.c        | 26 +++++++++++++++---
> >  qapi/crypto.json           | 19 ++++++++++++++
> >  tests/qemu-iotests/082.out | 54 ++++++++++++++++++++++++++++++++++++++
> >  6 files changed, 115 insertions(+), 4 deletions(-)
> > 
> > diff --git a/block/crypto.c b/block/crypto.c
> > index 6e822c6e50..a6a3e1f1d8 100644
> > --- a/block/crypto.c
> > +++ b/block/crypto.c
> > @@ -144,6 +144,8 @@ static QemuOptsList block_crypto_create_opts_luks = {
> >          BLOCK_CRYPTO_OPT_DEF_LUKS_IVGEN_HASH_ALG(""),
> >          BLOCK_CRYPTO_OPT_DEF_LUKS_HASH_ALG(""),
> >          BLOCK_CRYPTO_OPT_DEF_LUKS_ITER_TIME(""),
> > +        BLOCK_CRYPTO_OPT_DEF_LUKS_SLOT(""),
> > +        BLOCK_CRYPTO_OPT_DEF_LUKS_ACTIVE(""),
> >          { /* end of list */ }
> >      },
> >  };
> > diff --git a/block/crypto.h b/block/crypto.h
> > index b935695e79..05cc43d9bc 100644
> > --- a/block/crypto.h
> > +++ b/block/crypto.h
> > @@ -35,12 +35,14 @@
> >          "ID of the secret that provides the AES encryption key")
> >  
> >  #define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"
> > +#define BLOCK_CRYPTO_OPT_LUKS_SLOT "slot"
> >  #define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"
> >  #define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
> >  #define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg"
> >  #define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg"
> >  #define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"
> >  #define BLOCK_CRYPTO_OPT_LUKS_ITER_TIME "iter-time"
> > +#define BLOCK_CRYPTO_OPT_LUKS_ACTIVE "active"
> >  
> >  #define BLOCK_CRYPTO_OPT_DEF_LUKS_KEY_SECRET(prefix)                    \
> >      BLOCK_CRYPTO_OPT_DEF_KEY_SECRET(prefix,                             \
> > @@ -88,6 +90,20 @@
> >          .help = "Time to spend in PBKDF in milliseconds",     \
> >      }
> >  
> > +#define BLOCK_CRYPTO_OPT_DEF_LUKS_SLOT(prefix)           \
> > +    {                                                         \
> > +        .name = prefix BLOCK_CRYPTO_OPT_LUKS_SLOT,       \
> > +        .type = QEMU_OPT_NUMBER,                              \
> > +        .help = "Controls the slot where the secret is added/erased",     \
> > +    }
> > +
> > +#define BLOCK_CRYPTO_OPT_DEF_LUKS_ACTIVE(prefix)           \
> > +    {                                                         \
> > +        .name = prefix BLOCK_CRYPTO_OPT_LUKS_ACTIVE,       \
> > +        .type = QEMU_OPT_BOOL,                              \
> > +        .help = "Controls if the added secret is added or erased",     \
> > +    }
> 
> Do we actually need the "active" property for initial
> creation. I think its only needed for amend, so perhaps
> we shuold not register this at all ?

Sadly we kind of do, since both amend and create use the same option list 
currently.
I tried to duplicate it, and it is possible, but then you end up
with significant code duplication in qcow2 with its huge create option list.

I am now thinking that we could have had , 'create only' option list, 'amend 
only' option list,
and 'common' option list.
What do you think?


[...]

Best regards,
        Maxim Levitsky