[Freeipa-devel] [PATCH] Password vault

Wed Jul 1 06:53:37 UTC 2015

Dne 25.6.2015 v 19:01 Endi Sukma Dewata napsal(a):
> 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

What changed is that I now know there is also escrow public key, which I 
didn't know six months ago.

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

For example for ipaPublicKey searches - if ipaVaultPublicKey and 
ipaEscrowPublicKey both inherit from ipaPublicKey, then an ipaPublicKey 
search will look in both ipaVaultPublicKey and ipaEscrowPublicKey. This 
is not something we actually need right now, but once the schema is 
done, it can't be fixed and I don't think we should prevent this, 
especially since we can get it for free. BTW even the core LDAP schema 
does this, see for example how the cn attribute inherits from the more 
general name attribute: <https://tools.ietf.org/html/rfc4519#section-2.3>.

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

Right.

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

Yes.

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

Well, it would be better if there was no Flag class at all and flags 
were handled by CLI exclusively, because parameter classes should 
reflect the data type (bool) and not the presentation (flag).

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

It shows CLI options, see how the API object is initialized in makeapi.

>
>>> * 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?

Yes, basically, but I'm also saying that you are not limited to doing 
LDAP plugins only.

You can abuse the callbacks to do anything, including data retrieval 
from other sources, but it doesn't make it right, as it only leads to 
code duplication, inconsistencies and weird bugs. I have seen too much 
of this, hence my reluctance to do it again.

>
>>> * 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?

Because it's object-verb and not object-verbofsomeotherobject. (Also I 
already acknowledged the vaultdata idea is ugly.)

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

Aligning new code is exactly what I'm aiming to do and why I want people 
to look at their APIs from an object oriented perspective rather than 
just dumb RPC, because that's the direction the framework is heading.

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

Well, there's so many hacks in every corner of IPA that many people 
don't see them as hacks. PKQuery is not that bad, though it doesn't 
implement as many bits as would be useful in this case.

-- 
Jan Cholasta