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

[Kevin Wolf]

Am 05.02.2020 um 11:03 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> writes:
> 
> > Am 05.02.2020 um 09:24 hat Markus Armbruster geschrieben:
> >> Daniel, Kevin, any comments or objections to the QAPI schema design
> >> sketch developed below?
> >> 
> >> For your convenience, here's the result again:
> >> 
> >>     { 'enum': 'LUKSKeyslotState',
> >>       'data': [ 'active', 'inactive' ] }
> >>     { 'struct': 'LUKSKeyslotActive',
> >>       'data': { 'secret': 'str',
> >>                 '*iter-time': 'int } }
> >>     { 'union': 'LUKSKeyslotAmend',
> >>       'base': { '*keyslot': 'int',
> >>                 'state': 'LUKSKeyslotState' }
> >>       'discriminator': 'state',
> >>       'data': { 'active': 'LUKSKeyslotActive' } }
> >
> > I think one of the requirements was that you can specify the keyslot not
> > only by using its number, but also by specifying the old secret.
> 
> Quoting myself:
> 
>   When we don't specify the slot#, then "new state active" selects an
>   inactive slot (chosen by the system, and "new state inactive selects
>   slots by secret (commonly just one slot).
> 
> This takes care of selecting (active) slots by old secret with "new
> state inactive".

"new secret inactive" can't select a slot by secret because 'secret'
doesn't even exist for inactive.

> I intentionally did not provide for selecting (active) slots by old
> secret with "new state active", because that's unsafe update in place.
> 
> We want to update secrets, of course.  But the safe way to do that is to
> put the new secret into a free slot, and if that succeeds, deactivate
> the old secret.  If deactivation fails, you're left with both old and
> new secret, which beats being left with no secret when update in place
> fails.

Right. I wonder if qemu-img wants support for that specifically
(possibly with allowing to enter the key interactively) rather than
requiring the user to call qemu-img amend twice.

> >                                                                  Trivial
> > extension, you just get another optional field that can be specified
> > instead of 'keyslot'.
> >
> > Resulting commands:
> >
> >     Adding a key:
> >     qemu-img amend -o 
> > encrypt.keys.0.state=active,encrypt.keys.0.secret=sec0 test.qcow2
> 
> This activates an inactive slot chosen by the sysem.
> 
> You can activate a specific keyslot N by throwing in
> encrypt.keys.0.keyslot=N.

Yes. The usual case is that you just want to add a new key somwhere.

> >     Deleting a key:
> >     qemu-img amend -o 
> > encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2
> 
> This deactivates keyslot#2.
> 
> You can deactivate slots holding a specific secret S by replacing
> encrypt.keys.0.keyslot=2 by encrypt.keys.0.secret=S.

Not with your definition above, but with the appropriate changes, this
makes sense.

> > Previous version (if this series is applied unchanged):
> >
> >     Adding a key:
> >     qemu-img amend -o encrypt.keys.0.new-secret=sec0 test.qcow2
> >
> >     Deleting a key:
> >     qemu-img amend -o encrypt.keys.0.new-secret=,encrypt.keys.0.keyslot=2 
> > test.qcow2
> >
> > Adding a key gets more complicated with your proposed interface because
> > state must be set explicitly now whereas before it was derived
> > automatically from the fact that if you give a key, only active makes
> > sense.
> 
> The explicitness could be viewed as an improvement :)

Not really. I mean, I really know to appreciate the advantages of
-blockdev where needed, but usually I don't want to type all that stuff
for the most common tasks. qemu-img amend is similar.

For deleting, I might actually agree that explicitness is an
improvement, but for creating it's just unnecessary verbosity.

> If you'd prefer implicit here: Max has patches for making union tags
> optional with a default.  They'd let you default active to true.

I guess this would improve the usability in this case.

Kevin