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

[Maxim Levitsky]

On Mon, 2019-10-07 at 09:49 +0200, Markus Armbruster wrote:
> Quick QAPI schema review only.
> 
> Maxim Levitsky <address@hidden> writes:
> 
> > 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 active=true it when creating.
> > 
> > Signed-off-by: Maxim Levitsky <address@hidden>
> 
> [...]
> > diff --git a/qapi/crypto.json b/qapi/crypto.json
> > index b2a4cff683..9b83a70634 100644
> > --- a/qapi/crypto.json
> > +++ b/qapi/crypto.json
> > @@ -190,6 +190,20 @@
> 
>    ##
>    # @QCryptoBlockCreateOptionsLUKS:
>    #
>    # The options that apply to LUKS encryption format initialization
>    #
>    # @cipher-alg: the cipher algorithm for data encryption
>    #              Currently defaults to 'aes-256'.
>    # @cipher-mode: the cipher mode for data encryption
>    #               Currently defaults to 'xts'
>    # @ivgen-alg: the initialization vector generator
>    #             Currently defaults to 'plain64'
>    # @ivgen-hash-alg: the initialization vector generator hash
> >  #                  Currently defaults to 'sha256'
> >  # @hash-alg: the master key hash algorithm
> >  #            Currently defaults to 'sha256'
> > +#
> > +# @active: Should the new secret be added (true) or erased (false)
> > +#          (amend only, since 4.2)
> 
> Is "active" established terminology?  I wouldn't have guessed its
> meaning from its name...

Yea, this is one of the warts of the interface that I wanted to discuss with 
everyone
basically the result of using same API for creation and amend.

blockdev-amend, similiar to qemu-img amend will allow the user to change the 
format driver
settings, and will use the same BlockdevCreateOptions for that, similiar to how 
qemu-img amend works.

For creation of course the 'active' parameter is redundant, and it is forced to 
true.
For amend it allows to add or erase a new key.
We couldn't really think about any better name, so I kind of decided just to 
document
this and leave it like that.

> 
> As far as I can see, QCryptoBlockCreateOptionsLUKS is used just for
> blockdev-create with options.driver \in { luks, qcow, qcow2 }:
> 
>    { 'command': 'blockdev-create',
>      'data': { ...
>                'options': 'BlockdevCreateOptions' } }
> 
>    { 'union': 'BlockdevCreateOptions',
>      ...
>      'data': {
>          ...
>          'luks':           'BlockdevCreateOptionsLUKS',
>          ...
>          'qcow':           'BlockdevCreateOptionsQcow',
>          'qcow2':          'BlockdevCreateOptionsQcow2',
>          ... } }
> 
> With luks:
> 
>    { 'struct': 'BlockdevCreateOptionsLUKS',
>      'base': 'QCryptoBlockCreateOptionsLUKS',
>      ... }
> 
> With qcow and qcow2:
> 
>     { 'struct': 'BlockdevCreateOptionsQcow',
>       'data': { ...
>                 '*encrypt':         'QCryptoBlockCreateOptions' } }
>     { 'struct': 'BlockdevCreateOptionsQcow2',
>       'data': { ...
>                 '*encrypt':         'QCryptoBlockCreateOptions',
>                 ... } }
> 
>     { 'union': 'QCryptoBlockCreateOptions',
>       'base': 'QCryptoBlockOptionsBase',
>       'discriminator': 'format',
>       'data': { ...
>                 'luks': 'QCryptoBlockCreateOptionsLUKS' } }
> 
> I think I understand why we want blockdev-create to be able to specify a
> new secret.
> 
> Why do we want it to be able to delete an existing secret?  How would
> that even work?  Color me confused...

The BlockdevCreateOptions will now be used in
both creation and amend of a block device. Of course the deletion
of an existing secret doesn't make sense on creation time, and a check
is present to disallow the user to do that.

At the same time, the size and 'file' arguments are made optional,
so that during amend you could change the block device without
specifying those.


> 
> > +#
> > +# @slot: The slot in which to put/erase the secret
> > +#        if not given, will select first free slot for secret addtion
> > +#        and erase all matching keyslots for erase. except last one
> > +#        (optional, since 4.2)
> 
> Excuse my possibly ignorant question: what exactly is a "matching
> keyslot"?
Not ignorant at all, I dropped a word here.
I meant to say that it will erase all the keyslots which match the given
secret, except last one. The 'active' is what decides if to add to to remove
a secret.


> 
> > +#
> > +# @unlock-secret: The secret to use to unlock the image
> > +#        If not given, will use the secret that was used
> > +#        when opening the image.
> > +#        (optional, for amend only, since 4.2)
> 
> More ignorance: what is "amend"?  No mention of it in qapi/*json...
Not ignorant at all again! This interface will be added in the next patch,
and all the changes in this patch (other that specifying the keyslot,
which can be in theory useful anyway) are for it.


> 
> > +#
> >  # @iter-time: number of milliseconds to spend in
> >  #             PBKDF passphrase processing. Currently defaults
> >  #             to 2000. (since 2.8)
> > @@ -201,7 +215,12 @@
> >              '*cipher-mode': 'QCryptoCipherMode',
> >              '*ivgen-alg': 'QCryptoIVGenAlgorithm',
> >              '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> > +
> >              '*hash-alg': 'QCryptoHashAlgorithm',
> > +            '*active' : 'bool',
> > +            '*slot': 'int',
> > +            '*unlock-secret': 'str',
> > +
> >              '*iter-time': 'int'}}
> >  
> >  
> 
> [...]


Best regards,
        Maxim Levitsky