[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

Dne 2.6.2015 v 02:00 Endi Sukma Dewata napsal(a):
> 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.

The class hierarchy is as follows:

    frontend.Command
        frontend.Method
            crud.PKQuery
                crud.Retrieve
                cdur.Update

So removing Command from the list of base classes should fix it.

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

I see this has been already resolved in the other thread.

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

I understand why the code has to be separated, what I don't understand 
is why it is in fact *not* separated and crammed into a single command, 
making weird and undefined behavior possible.

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

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.

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

As I suggested above, you can split the commands into separate client 
and server commands. The client command should inherit from 
frontend.Local so that it is always executed locally and the server 
command should have a "NO_CLI = True" attribute so that it is not 
available in the CLI.

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

OK.

-- 
Jan Cholasta