Re: QAPI schema for desired state of LUKS keyslots (was: [PATCH 02/13] qcrypto-luks: implement encryption key management)

[Maxim Levitsky]

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.
> 
> 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.
> 
> * @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.
> 
>     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.
> 
> Note that LUKSKeyslotAmend can specify only one desired state for
> commonly just one keyslot.  Rationale: this satisfies practical needs.
> An array of LUKSKeyslotAmend could specify desired state for all
> keyslots.  However, multiple array elements could then apply to the same
> slot.  We'd have to specify how to resolve such conflicts, and we'd have
> to code up conflict detection.  Not worth it.
> 
> Examples:
> 
> * Add a secret to some free keyslot:
> 
>   { "state": "active", "secret": "CIA/GRU/MI6" }
> 
> * Deactivate all keyslots holding a secret:
> 
>   { "state": "inactive", "old-secret": "CIA/GRU/MI6" }
> 
> * Add a secret to a specific keyslot:
> 
>   { "state": "active", "secret": "CIA/GRU/MI6", "keyslot": 0 }
> 
> * Deactivate a specific keyslot:
> 
>   { "state": "inactive", "keyslot": 0 }
> 
>   Possibly less dangerous:
> 
>   { "state": "inactive", "keyslot": 0, "old-secret": "CIA/GRU/MI6" }
> 
> Option: Make use of Max's patches to support optional union tag with
> default value to let us default @state to "active".  I doubt this makes
> much of a difference in QMP.  A human-friendly interface should probably
> be higher level anyway (Daniel pointed to cryptsetup).
> 
> Option: LUKSKeyslotInactive member @old-secret could also be named
> @secret.  I don't care.
> 
> Option: delete @keyslot.  It provides low-level slot access.
> Complicates the interface.  Fine if we need lov-level slot access.  Do
> we?
> 
> I apologize for the time it has taken me to write this.
> 
> Comments?

I tried today to implement this but I hit a very unpleasant roadblock:

Since QCrypto is generic (even though it only implements currently luks for 
raw/qcow2 usage,
and legacy qcow2 aes encryption), I still can't assume that this is always the 
case.
Thus I implemented the Qcrypto amend API in this way:

##
# @QCryptoBlockAmendOptions:
#
# The options that are available for all encryption formats
# when amending encryption settings
#
# Since: 5.0
##
{ 'union': 'QCryptoBlockAmendOptions',
  'base': 'QCryptoBlockOptionsBase',
  'discriminator': 'format',
  'data': {
          'luks': 'QCryptoBlockAmendOptionsLUKS' } }

However the QCryptoBlockAmendOptionsLUKS is a union too to be in line with the 
API proposal,
but that is not supported on QAPI level and after I and Markus talked about we 
are not sure
that it is worth it to implement this support only for this case.

So far I see the following solutions


1. Drop the QCryptoBlockAmendOptionsLUKS union for now.
This will bring the schema pretty much to be the same as my original proposal,
however the API will be the same thus once nested unions are implemented this 
union
can always be introduced again.

2. Drop the QCryptoBlockAmendOptions union. Strictly speaking this union is not 
needed
since it only has one member anyway, however this union is used both by qcow2 
QAPI scheme,
so that it doesn't hardcode an encryption format for amend just like it doesn't 
for creation,
(this can be hardcoded for now as well for now as long as we don't have more 
amendable encryption formats).
However I also use the QCryptoBlockAmendOptions in C code in QCrypto API thus 
it will be ugly to use the 
QCryptoBlockAmendOptionsLUKS instead.


3. Make QCryptoBlockAmendOptionsLUKS a struct and add to it a nested member 
with new union type 
(say QCryptoBlockAmendOptionsLUKS1) which will be exactly as 
QCryptoBlockAmendOptionsLUKS was.

This IMHO is even uglier since it changes the API (which we can't later fix) 
and adds both a dummy struct
field and a dummy struct name.

I personally vote 1.

Best regards,
        Maxim Levitsky