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

[Kevin Wolf <kwolf@redhat.com>]

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

    Deleting a key:
    qemu-img amend -o encrypt.keys.0.state=inactive,encrypt.keys.0.keyslot=2 test.qcow2

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.

Deleting stays more or less the same, you just change the state instead
of clearing the secret.

Kevin

> Markus Armbruster <armbru@redhat.com> writes:
> 
> [...]
> > A keyslot can be either inactive or active.
> >
> > Let's start low-level, i.e. we specify the slot by slot#:
> >
> >     state       new state   action
> >     inactive    inactive    nop
> >     inactive    active      put secret, iter-time, mark active
> >     active      inactive    mark inactive (effectively deletes secret)
> >     active      active      in general, error (unsafe update in place)
> >                             we can make it a nop when secret, iter-time
> >                                 remain unchanged
> >                             we can allow unsafe update with force: true
> >
> > As struct:
> >
> >     { 'struct': 'LUKSKeyslotUpdate',
> >       'data': { 'active': 'bool',       # could do enum instead
> >                 'keyslot': 'int',
> >                 '*secret': 'str',       # present if @active is true
> >                 '*iter-time': 'int' } } # absent if @active is false
> >
> > As union:
> >
> >     { 'enum': 'LUKSKeyslotState',
> >       'data': [ 'active', 'inactive' ] }
> >     { 'struct': 'LUKSKeyslotActive',
> >       'data': { 'secret': 'str',
> >                 '*iter-time': 'int } }
> >     { 'union': 'LUKSKeyslotAmend',
> >       'base': { 'state': 'LUKSKeyslotState' }   # must do enum
> >       'discriminator': 'state',
> >       'data': { 'active': 'LUKSKeyslotActive' } }
> >
> > 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).
> >
> > New state active:
> >
> >     state       new state   action
> >     inactive    active      put secret, iter-time, mark active
> >     active      active      N/A (system choses inactive slot)
> >
> > New state inactive, for each slot holding the specified secret:
> >
> >     state       new state   action
> >     inactive    inactive    N/A (inactive slot holds no secret)
> >     active      inactive    mark inactive (effectively deletes secret)
> >
> > As struct:
> >
> >     { 'struct': 'LUKSKeyslotUpdate',
> >       'data': { 'active': 'bool',       # could do enum instead
> >                 '*keyslot': 'int',
> >                 '*secret': 'str',       # present if @active is true
> >                 '*iter-time': 'int' } } # absent if @active is false
> >
> > As union:
> >
> >     { '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' } }
> >
> > Union looks more complicated because our union notation sucks[*].  I
> > like it anyway, because you don't have to explain when which optional
> > members aren't actually optional.
> >
> > Regardless of struct vs. union, this supports an active -> active
> > transition only with an explicit keyslot.  Feels fine to me.  If we want
> > to support it without keyslot as well, we need a way to specify both old
> > and new secret.  Do we?
> >
> >
> > [*] I hope to fix that one day.  It's not even hard.