[Freeipa-devel] [PATCH] Password vault

[Endi Sukma Dewata]

On 6/25/2015 12:35 AM, Jan Cholasta wrote:
>>>>> I think it would be better to use a new attribute type which inherits
>>>>> from ipaPublicKey (ipaVaultPublicKey?) rather than ipaPublicKey
>>>>> directly
>>>>> for assymetric vault public keys, so that assymetric public key and
>>>>> escrow public key are on the same level and you can still use
>>>>> ipaPublicKey to refer to either one:
>>>>>
>>>>>      ipaPublicKey
>>>>>          ipaVaultPublicKey
>>>>>          ipaEscrowPublicKey
>>>>>
>>>> OK. To be consistent the parameters need to be renamed too:
>>>> --vault-public-key and --vault-public-key-file.
>>>
>>> It doesn't need to, there is no requirement for CLI names to always
>>> match attribute names. (Also I don't insist on the name
>>> "ipaVaultPublicKey", feel free to change it if you want.)
>>
>> It's unchanged for now. In a previous discussion it was advised to reuse
>> the existing attribute type whenever possible.
>
> Well, in this discussion, it is not. Escrow public key should also reuse
> ipaPublicKey, but it can't if you use it for vault public key. By using
> ipaPublicKey subtypes you can distinguish between the two uses and still
> use ipaPublicKey to refer to either of them.

So what's changed? This is what you said when I posted the same patch 
six months ago:

>> In this patch I'm adding ipaVaultSalt and ipaVaultPublicKey attribute
>> types to store salt and public key for vault. Are there existing
>> attribute types that I can use instead? I see there's an ipaPublicKey,
>> should I use that and maybe add ipaSalt/ipaEncSalt? Thanks.
>
> yes, please re-use existing attributes where possible.
>
> Honza

Could you show me how to use ipaPublicKey to refer to ipaVaultPublicKey 
and ipaEscrowPublicKey? Under what situation would that be useful?

>>>   a) When the non-split vault_{archive,retrieve} was called from a
>>> server API with client-only options, it crashed. This is the broken API
>>> I was talking about.
>>
>> This is because in the current framework any API called on the server
>> side will be a server API, so you are not supposed to call it with
>> client options in the first place. Because of that limitation, the only
>> way to use client options is to use a separate API on the client side to
>> call the original API on the server side. The point is, client options
>> belong to client API, and server options belong to server API. In
>> vault_add the public key file name belongs to client API because it's
>> used to load a file on the client side. You should not add public key
>> file name option to the server API just because it can safely be ignored.
>
> I don't disagree, but file name options do not belong to the general
> client API either, as they are strictly CLI-specific.

To my understanding the current framework doesn't have a separate CLI 
class, so you don't have a choice but to put CLI-specific options in the 
client API class too. However, you do have a choice not to combine 
client API class and server API class because otherwise that will put 
CLI-specific options in the server API class too.

>>>>>>> 2. Since the vault_archive_internal inherits from Update, it accepts
>>>>>>> all non
>>>>>>> primary-key attributes automatically. This is incorrect since we
>>>>>>> don't want to
>>>>>>> update these parameters during archival. Can this behavior be
>>>>>>> overridden?
>>>>>
>>>>> Inherit from PKQuery instead (don't forget to add "has_output =
>>>>> output.standard_entry").
>>>>
>>>> Previously you didn't want to use LDAPQuery because of semantics
>>>> reasons. Is PKQuery fine semantically?
>>>
>>> It's not. Currently there is a set of commands which operate on the LDAP
>>> part of vault and another set of commands which operate on the KRA part
>>> of vault and we don't want the commands in one set to see attributes
>>> related to the other part of vault. If you insist on keeping both parts
>>> in a single object, you have to resort to hackery like using PKQuery,
>>> hence my suggestion to split the data part off to a separate object to
>>> avoid this.
>>
>> This because the framework was based on simplistic assumptions which
>> create unnecessary restrictions, for example:
>> * client API is just a proxy to server API (i.e. client and server
>> cannot do different things)
>
> They can do different things the same way vault_archive/vault_retrieve
> does that, the commands just can't be called the same (which is not
> necessarily a bad thing).

Of course different APIs can do different things, like vault_add calling 
vault_archive, or vault_archive calling vault_archive_internal. The 
point is right now the client portion of an API (i.e. the forward() 
method) cannot do anything other than forwarding the request to the 
server, so the API has to be split into different APIs:

  * vault_archive
  * vault_archive_internal

It would be nice to have formal separation between client and server 
APIs so it's clear they are different but still related without 
resorting to ugly names:

  * client.vault_archive
  * server.vault_archive

>> * CLI options will be identical to client and server API options (i.e.
>> no CLI-only, client-only, or server-only options)
>
> Actually, you can create CLI-only options (add include='cli' to the
> param's kwargs).

I need to look at this more closely. If I understand correctly in 
user_del there are two 'preserve' options, the Bool preserve is for 
client and server API, and the Flag preserve is for CLI. Wouldn't it be 
better if they are stored in separate lists (or maybe separate classes)? 
And it looks like you still need to delete the CLI options explicitly 
anyway.

Does the API.txt actually show the CLI options, the client API options, 
or the server API options? I only see the Flag preserve, not the Bool 
preserve.

>> * a plugin will only access one type of data (i.e. LDAP plugin can only
>> access LDAP data)
>
> This is not assumed anywhere in the framework, you can access whatever
> you want, but you can't expect baseldap to do everything for you.

Nobody is expecting baseldap to do KRA operations.

> As the
> name implies, it is LDAP specific, if you want something else, you have
> to implement it yourself.

In the previous patch vault_retrieve inherits from LDAPRetrieve so it 
can rely on baseldap to retrieve the vault entry, then on top of that it 
implements an additional KRA operations (without baseldap obviously). If 
that is not allowed, aren't you basically saying LDAP plugin can only 
access LDAP data?

>> * a command name will match the object name (i.e. must use vaultdata_mod
>> instead of a more intuitive vault_archive)
>
> I don't see how consistency is a bad thing, or how this could limit
> anyone doing things cleanly. I do agree that vaultdata_mod is ugly, but
> it's not the only way to achieve the same goal.

Look at it from user's perspective. If you create a vault using 
vault-add <vault name>, then archive data using vaultdata-mod <vault 
name>, how is this consistent?

>> We know that some use cases do not fit these assumptions. Rather than
>> compromising the use case, or looking at workarounds as hacks, I'd
>> suggest finding ideas to improve the framework itself to be more
>> accommodating.
>
> I would personally love to improve the framework (it's just retarded
> sometimes as you may have noticed), but it does not have high priority
> right now (not my decision).

We don't have to modify the current framework right now, but we can 
align new codes that don't fit the current framework to match the future 
framework. Although the future framework is not defined yet, some things 
are already clear, for example there should be separate client and 
server APIs. So if a command like vault_add has differing client and 
server options, regardless how insignificant it is, there's no reason to 
force it to be combined. The current framework doesn't prevent 
separation anyway.

> Keep in mind that workarounds which screw with the object model will
> always be considered hacks, even after the framework is made more
> accomodating.

Don't get this wrong. The framework will only be considered 
accommodating if it allows people to implement features without 
'hacking'. 'Hacking' itself is never a goal, it's the last resort to 
work around the framework's current limitations, just like how you ended 
up using PKQuery for vault_archive_internal.

-- 
Endi S. Dewata