Re: [PATCH 02/13] qcrypto-luks: implement encryption key management

[Maxim Levitsky <mlevitsk@redhat.com>]

On Tue, 2020-01-21 at 08:54 +0100, Markus Armbruster wrote:

<trimmed>

> > +##
> > +# @LUKSKeyslotUpdate:
> > +#
> > +# @keyslot:         If specified, will update only keyslot with this index
> > +#
> > +# @old-secret:      If specified, will only update keyslots that
> > +#                   can be opened with password which is contained in
> > +#                   QCryptoSecret with @old-secret ID
> > +#
> > +#                   If neither @keyslot nor @old-secret is specified,
> > +#                   first empty keyslot is selected for the update
> > +#
> > +# @new-secret:      The ID of a QCryptoSecret object providing a new decryption
> > +#                   key to place in all matching keyslots.
> > +#                   null/empty string erases all matching keyslots
> 
> I hate making the empty string do something completely different than a
> non-empty string.
> 
> What about making @new-secret optional, and have absent @new-secret
> erase?

I don't remember already why I and Keven Wolf decided to do this this way, but I think that you are right here.
I don't mind personally to do this this way.
empty string though is my addition, since its not possible to pass null on command line.

> 
> > +#                                                                  unless
> > +#                   last valid keyslot is erased.
> 
> Leaves me to wonder what happens when I try.  If I read your code
> correctly, it's an error.  Suggest "You cannot erase the last valid
> keyslot".
> 
> Not documented here: "Refusing to overwrite active slot".

In my current implementation, if all slots are selected for erase,
I just refuse the erase operation. In former versions of my patches,
I would instead erase all but the last one.
IMHO, its unlikely that more that one slot would have the same password,
thus anyway correct usage for replacing the password would be first add
a new slot, then erase all slots that match the old password.
If all slots are active and have the same password, then user still can
use 'force' option to overwrite one of them.

> 
> > +#
> > +# @iter-time:       number of milliseconds to spend in
> > +#                   PBKDF passphrase processing
> 
> Default?
2000, as in QCryptoBlockCreateOptionsLUKS. I forgot to copy this here.

> 
> > +# Since: 5.0
> > +##
> > +{ 'struct': 'LUKSKeyslotUpdate',
> > +  'data': {
> > +           '*keyslot': 'int',
> > +           '*old-secret': 'str',
> > +           'new-secret' : 'StrOrNull',
> > +           '*iter-time' : 'int' } }
> > +
> > +
> > +##
> > +# @QCryptoBlockAmendOptionsLUKS:
> > +#
> > +# The options that can be changed on existing luks encrypted device
> > +# @keys:           list of keyslot updates to perform
> > +#                  (updates are performed in order)
> > +# @unlock-secret:  use this secret to retrieve the current master key
> > +#                  if not given will use the same secret as one
> 
> "as the one"?
Yea, this is wrong wording, I'll drop those words. Thanks.

> 
> > +#                  that was used to open the image
> > +#
> > +# Since: 5.0
> > +##
> > +{ 'struct': 'QCryptoBlockAmendOptionsLUKS',
> > +  'data' : {
> > +            'keys': ['LUKSKeyslotUpdate'],
> > +             '*unlock-secret' : 'str' } }
> > +
> >   
> >   
> >   ##
> > @@ -324,4 +372,4 @@
> >     'base': 'QCryptoBlockOptionsBase',
> >     'discriminator': 'format',
> >     'data': {
> > -            } }
> > +          'luks': 'QCryptoBlockAmendOptionsLUKS' } }


Thanks for review,
	Best regards,
		Maxim Levitsky