Re: QAPI schema for desired state of LUKS keyslots

[Maxim Levitsky <mlevitsk@redhat.com>]

On Mon, 2020-02-17 at 07:45 +0100, Markus Armbruster wrote:
> Maxim Levitsky <mlevitsk@redhat.com> writes:
> 
> > On Sat, 2020-02-15 at 15:51 +0100, Markus Armbruster wrote:
> > > Review of this patch led to a lengthy QAPI schema design discussion.
> > > Let me try to condense it into a concrete proposal.
> > > 
> > > This is about the QAPI schema, and therefore about QMP.  The
> > > human-friendly interface is out of scope.  Not because it's not
> > > important (it clearly is!), only because we need to *focus* to have a
> > > chance at success.
> > 
> > 100% agree.
> > > 
> > > I'm going to include a few design options.  I'll mark them "Option:".
> > > 
> > > The proposed "amend" interface takes a specification of desired state,
> > > and figures out how to get from here to there by itself.  LUKS keyslots
> > > are one part of desired state.
> > > 
> > > We commonly have eight LUKS keyslots.  Each keyslot is either active or
> > > inactive.  An active keyslot holds a secret.
> > > 
> > > Goal: a QAPI type for specifying desired state of LUKS keyslots.
> > > 
> > > Proposal:
> > > 
> > >     { 'enum': 'LUKSKeyslotState',
> > >       'data': [ 'active', 'inactive' ] }
> > > 
> > >     { 'struct': 'LUKSKeyslotActive',
> > >       'data': { 'secret': 'str',
> > >                 '*iter-time': 'int } }
> > > 
> > >     { 'struct': 'LUKSKeyslotInactive',
> > >       'data': { '*old-secret': 'str' } }
> > > 
> > >     { 'union': 'LUKSKeyslotAmend',
> > >       'base': { '*keyslot': 'int',
> > >                 'state': 'LUKSKeyslotState' }
> > >       'discriminator': 'state',
> > >       'data': { 'active': 'LUKSKeyslotActive',
> > >                 'inactive': 'LUKSKeyslotInactive' } }
> > > 
> > > LUKSKeyslotAmend specifies desired state for a set of keyslots.
> > > 
> > > Four cases:
> > > 
> > > * @state is "active"
> > > 
> > >   Desired state is active holding the secret given by @secret.  Optional
> > >   @iter-time tweaks key stretching.
> > > 
> > >   The keyslot is chosen either by the user or by the system, as follows:
> > > 
> > >   - @keyslot absent
> > > 
> > >     One inactive keyslot chosen by the system.  If none exists, error.
> > > 
> > >   - @keyslot present
> > > 
> > >     The keyslot given by @keyslot.
> > > 
> > >     If it's already active holding @secret, no-op.  Rationale: the
> > >     current state is the desired state.
> > > 
> > >     If it's already active holding another secret, error.  Rationale:
> > >     update in place is unsafe.
> > > 
> > >     Option: delete the "already active holding @secret" case.  Feels
> > >     inelegant to me.  Okay if it makes things substantially simpler.
> > 
> > I didn't really understand this, since in state=active we shouldn't
> > delete anything. Looks OK otherwise.
> 
> Let me try to clarify.
> 
> Option: make the "already active holding @secret" case an error like the
> "already active holding another secret" case.  In longhand:
> 
>      - @keyslot present
> 
>        The keyslot given by @keyslot.
> 
>        If it's already active, error.
> 
> It feels inelegant to me, because it deviates from "specify desired
> state" paradigm: the specified desired state is fine, the way to get
> there from current state is obvious (do nothing), yet it's still an
> error.
Yep, although in theory we also specify that iteration count, which might not
match (and it will never exactly match since it is benchmark based), thus
if user specified it, we might err out, and otherwise indeed ignore this.
This is of course very minor issue.

> 
> > > * @state is "inactive"
> > > 
> > >   Desired state is inactive.
> > > 
> > >   Error if the current state has active keyslots, but the desired state
> > >   has none.
> > > 
> > >   The user choses the keyslot by number and/or by the secret it holds,
> > >   as follows:
> > > 
> > >   - @keyslot absent, @old-secret present
> > > 
> > >     All active keyslots holding @old-secret.  If none exists, error.
> > > 
> > >   - @keyslot present, @old-secret absent
> > > 
> > >     The keyslot given by @keyslot.
> > > 
> > >     If it's already inactive, no-op.  Rationale: the current state is
> > >     the desired state.
> > > 
> > >   - both @keyslot and @old-secret present
> > > 
> > >     The keyslot given by keyslot.
> > > 
> > >     If it's inactive or holds a secret other than @old-secret, error.
> > 
> > Yea, that would be very nice to have.
> > > 
> > >     Option: error regardless of @old-secret, if that makes things
> > >     simpler.
> > > 
> > >   - neither @keyslot not @old-secret present
> > > 
> > >     All keyslots.  Note that this will error out due to "desired state
> > >     has no active keyslots" unless the current state has none, either.
> > > 
> > >     Option: error out unconditionally.
> > 
> > Yep, that the best IMHO.
> 
> It's a matter of taste.
> 
> If we interpret "both absent" as "all keyslots", then all cases flow out
> of the following simple spec:
> 
>     0. Start with the set of all keyslots
> 
>     1. If @old-secret is present, interset it with the set of slots
>        holding that secret.
> 
>     2. If @keyslots is present, intersect it with the set of slots with
>        that slot number.
> 
> The order of steps 1 and 2 doesn't matter.
> 
> To error out unconditionally, we have to make "both absent" a special
> case.
Yes but to be honest it is few lines of code, and gets you a better
error message in the process. I don't have a strong opinion on this though.

> 
> A good way to resolve such matters of taste is to try writing doc
> comments for all proposals.  If you find you hate one of them much less,
> you have a winner :)

I am already out of this game for some time, I myself won't argue
over this interface (in addition to that that current interface is very good IMHO),
and I am just waiting for others to accept it.


Best regards and thanks for help,
	Maxim Levitsky

> 
> [...]