[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

Dne 23.6.2015 v 05:27 Endi Sukma Dewata napsal(a):
> Please take a look at the new patch.
>
> On 6/17/2015 1:32 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.

>
>>>>>> 1. The vault_add was split into a client-side vault_add and
>>>>>> server-side
>>>>>> vault_add_internal since the parameters are different (i.e. public
>>>>>> key file and
>>>>>> future escrow-related params). Since vault_add inherits from Local
>>>>>> all
>>>>>> non-primary-key attributes have to be added explicitly.
>>>>
>>>> The split is not really necessary, since the only difference is the
>>>> public_key_file option, which exists only because of the lack of proper
>>>> file support in the framework. This is a different situation from
>>>> vault_{archive,retrieve}, which has two different sets of options on
>>>> client and server side. Escrow adds only ipaescrowpublickey and
>>>> escrow_public_key_file, right? If yes, we can safely keep the
>>>> command in
>>>> a single piece.
>>>
>>> We know the vault-add will have at least two client-only parameters:
>>> vault_public_key_file and escrow_public_key_file. Keeping these
>>> parameters on the server API would be wrong and confusing. If the API is
>>> called on the server side with vault_public_key_file the operation will
>>> fail. In the previous discussion you considered this as broken API:
>>>
>>>> Server API is used not only by the server itself, but also by
>>>> installers
>>>> for example. Anyway the point is that there *can't* be a broken API
>>>> like
>>>> this, you should at least raise an error if the command is called from
>>>> server API, although actually separating it into client and server
>>>> parts
>>>> would be preferable.
>>
>> You are comparing apples and oranges:
>
> Non-identical items are different by definition. Even between 2 apples
> there are differences, but it doesn't mean the distinction is important.
> The latest patch shows that the vault_add needs to be split, not just
> because of the options, but because of what they do differently on the
> client and server.
>
>>   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.

>
>>   b) The non-split vault_{archive,retrieve} had server-only options,
>> which were also accepted on client, but setting them had no effect.
>
> Similarly, in a combined vault_add the public key file name option will
> be accepted by the server, but it will be ignored. If something calls
> vault_add on the server side and provides a file name, the operation
> will crash too because the command expects the public key data to be
> provided via another option. Splitting the vault_add into client and
> server components avoids the potential problems.
>
>>   c) The CLI options to read param values from files should be generated
>> by the framework without having to specify dummy params. Once this is
>> implemented, the dummy params will go away. However, this will still
>> leave some client-only options in vault_{archive,retrieve}.
>
> I'm not sure how the options will look like when that's implemented, but
> regardless, the vault_add will still have client-only password option.
>
>> None of the above applies to vault_add - it does not have any
>> server-only options and the only client-only options it has are the
>> dummy options for file input, which are ignored on the server.
>
> Let's not get fixated with just the options. The vault_add will now
> archive a blank initial data as it was originally designed. The data can
> be used later to verify the vault password in subsequent archival
> operations. The vault_archive must be called by vault_add's client
> component since it takes a password and the password cannot be sent to
> the server.

OK, the password option and related stuff is a good reason to split the 
command.

>
>>> Also, originally the vault was designed like this: when you create a
>>> symmetric vault you're supposed to specify the password as well, similar
>>> to adding a public key when creating an asymmetric vault. When you
>>> archive, you're supposed to enter the same password for verification,
>>> not a new password. So it would look like this:
>>>
>>> $ ipa vault-add test --type symmetric
>>> New password: ********
>>> Verify password: ********
>>>
>>> $ ipa vault-archive test --in secret1.txt
>>> Password: ******** (same password)
>>>
>>> $ ipa vault-archive test --in secret2.txt
>>> Password: ******** (same password)
>>>
>>> In the original design the vault-add would also archive a blank data,
>>> which later could be used to verify the password during vault-archive by
>>> decrypting the existing data first. There's also a plan to add a
>>> mechanism to change the password after the ACL patch.
>>>
>>> In the current design the vault-add doesn't archive anything, so during
>>> vault-archive it cannot verify the password because there is nothing to
>>> decrypt. In other words you can specify different passwords on each
>>> archival, regardless of previous archivals:
>>>
>>> $ ipa vault-add test --type symmetric
>>>
>>> $ ipa vault-archive test --in secret1.txt
>>> New password: ********
>>> Verify password: ********
>>>
>>> $ ipa vault-archive test --in secret2.txt
>>> New password: ********
>>> Verify password: ********
>>>
>>> So basically here are the options:
>>>
>>> 1. Specify the crypto parameters once during vault creation, then
>>> reuse/verify the parameters on each archival & retrieval. You can change
>>> the parameters only with a special command.
>>>
>>> 2. Don't specify the crypto parameters during vault creation, but
>>> specify new parameters on each archival. For retrieval you'd have to
>>> use/verify the parameters specified in the last archival.
>>>
>>> I think the first one makes more sense and is easier to use. That also
>>> means the vault-add will have additional client-only parameters such as
>>> --password and --password-file.
>>
>> How come --password is client-side? When setting password for a user,
>> the password is sent to the server. If it's OK for users, why is it not
>> OK for vaults?
>
> Please see the sequence diagram in the vault design page. Vault password
> is used by the user to encrypt the secret before it's sent to the
> server. The server is not supposed to know the vault password. I'm not
> sure exactly how the user password is used, but I suppose the crypto
> operation is done on the server side.
>
>> Does the password need to be set in vault_add? Why not have a separate
>> command for setting the password, like what we have for users?
>
> No. Vault password is not stored on the server. It's only used to
> generate encryption key on the client side, and the password & key will
> be discarded immediately after each use. That's why you have to specify
> the password on each archival & retrieval.

OK, makes sense.

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

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

> * 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. As the 
name implies, it is LDAP specific, if you want something else, you have 
to implement it yourself.

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

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

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

>
>>> Why not use LDAPQuery since vault
>>> is an LDAPObject?
>>
>> Because you are retrieving data from KRA, not from LDAP.
>
> The vault archive and retrieve do actually retrieve the vault LDAP entry
> first, then perform the KRA archival/retrieval after that. Right now
> they use vault_show to do the LDAP retrieval, but in the old patch it
> was implemented as LDAPRetrieve. Regardless, they are retrieving both
> LDAP and KRA data.
>
>>> And to be consistent should vault_retrieve_internal
>>> inherit from the same class?
>>
>> It could, but it's not necessary.
>
> Changed for consistency.
>


-- 
Jan Cholasta