[Freeipa-devel] [PATCH] Password vault

[Martin Kosek]

On 06/02/2015 02:00 AM, Endi Sukma Dewata wrote:
> Please take a look at the updated patch.
>
> On 5/27/2015 12:39 AM, Jan Cholasta wrote:
>>>>>>>> 21) vault_archive is not a retrieve operation, it should be based on
>>>>>>>> LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it
>>>>>>>> does
>>>>>>>> not do anything with LDAP. The same applies to vault_retrieve.
>>>>>>>
>>>>>>> The vault_archive does not actually modify the LDAP entry because it
>>>>>>> stores the data in KRA. It is actually an LDAPRetrieve operation
>>>>>>> because
>>>>>>> it needs to get the vault info before it can perform the archival
>>>>>>> operation. Same thing with vault_retrieve.
>>>>>>
>>>>>> It is not a LDAPRetrieve operation, because it has different
>>>>>> semantics.
>>>>>> Please use Command as base class and either use ldap2 for direct
>>>>>> LDAP or
>>>>>> call vault_show instead of hacking around LDAPRetrieve.
>>>>>
>>>>> It's been changed to inherit from LDAPQuery instead.
>>>>
>>>> NACK, it's not a LDAPQuery operation, because it has different
>>>> semantics. There is more to a command than executing code, so you should
>>>> use a correct base class.
>>>
>>> Changed to inherit from Command as requested. Now these commands no
>>> longer have a direct access to the vault object (self.obj) although they
>>> are accessing vault objects like other vault commands. Also now the
>>> vault name argument has to be added explicitly on each command.
>>
>> You can inherit from crud.Retrieve and crud.Update to get self.obj and
>> the argument back.
>
> I tried this:
>
>    class vault_retrieve(Command, crud.Retrieve):
>
> and it gave me an error:
>
>    TypeError: Error when calling the metaclass bases
>        Cannot create a consistent method resolution
>    order (MRO) for bases Retrieve, Command
>
> I'm sticking with the original code since it works fine although not ideal. I'm
> not a Python expert, so if you know how to fix this properly please feel free
> to post a patch on top of this.
>
>> If KRA is not installed, vault-archive and vault-retrieve fail with
>> internal error.
>
> Added a code to check KRA installation in all vault commands. If you know a way
> not to load the vault plugin if the KRA is not installed please let me know,
> that's probably even better. Not sure how that will work on the client side
> though.
>
>> The commands still behave differently based on whether they were called
>> from API which was initialized with in_server set to True or False.
>
> That is unfortunately a restriction imposed by the framework. In order to
> guarantee the security, the vault is designed to have separate client and
> server code. The client code encrypts the secret, the server code forwards the
> encrypted secret to KRA. To archive a secret into a vault properly, you are
> supposed to call the client code. If you're calling the server code directly,
> you are responsible to do your own encryption (i.e. generating session key,
> nonce, and vault data).
>
> If another plugin wants to use vault, it should implement a client code which
> calls the vault client code to perform the archival from the client side.
>
> What is the use case for calling the vault API from the server side anyway?
> Wouldn't that defeat the purpose of having a vault? If a secret exists on the
> server side in an unencrypted form doesn't it mean the secret may already have
> been compromised?
>
>> There is no point in exposing the session_key, nonce and vault_data
>> options in CLI when their value is always overwritten in forward().
>
> I agree there is no need to expose them in CLI, but in this framework the API
> also defines the CLI. If there's a way to keep them in the server API but not
> expose them in the CLI please let me know. Or, if there's a way to define
> completely separate server API (without a matching client CLI) and client CLI
> (without a matching server API) that will work too.
>
>> Will this always succeed?
>>
>> +        # deactivate vault record in KRA
>> +        response = kra_client.keys.list_keys(
>> +            client_key_id, pki.key.KeyClient.KEY_STATUS_ACTIVE)
>
> Yes. If there's no active keys it will return an empty collection.
>
>> +        for key_info in response.key_infos:
>> +            kra_client.keys.modify_key_status(
>> +                key_info.get_key_id(),
>> +                pki.key.KeyClient.KEY_STATUS_INACTIVE)
>
> This loop will do nothing given an empty collection.
>
>> If not, we might get into an inconsistent state, where the vault is
>> deleted in LDAP but still active in KRA. (I'm not sure if this is
>> actually a problem or not.)
>
> That can only happen if the server crashes after deleting the vault but before
> deactivating the key. Regardless, it will not be a problem because the key is
> identified by vault ID/path so it will not conflict with other vaults, and it
> will get overwritten if the same vault is recreated again.
>

Hi Endi,

Quickly skimming through your patches raised couple questions on my side:

1) Will it be possible to also store plain text password via Vault? It talks 
about taking in the binary data or the text file, but will it also work with 
plain user secrets (passwords)? I am talking about use like this:

# ipa vault-archive <name> --user mkosek --data Secret123


2) Didn't we discuss a dependency of IPA/Vault on python-cryptography in the 
past? I rather see use of python-nss for cryptography...

3) You do a lot of actions in the forward() method (as planned in
https://www.freeipa.org/page/V4/Password_Vault#Archival). But how do you 
envision that this is consumed by the Web UI? It does not have access to the 
forward() method. Would it need to also include some crypto library?

4) In the vault-archive forward method, you use "pki" module. However, this 
module will be only available on FreeIPA PKI-powered servers and not on FreeIPA 
clients - so this will not work unless freeipa-client gets a dependency on 
pki-base - which is definitely not something we want...

Thanks,
Martin