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

[Daniel P . Berrangé]

On Fri, Sep 06, 2019 at 04:57:22PM +0300, Maxim Levitsky wrote:
> 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.

Ah, I see now.

> 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?

I'm not too fussed - either way is fine with me.


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 :|