[Freeipa-devel] [PATCH] Password vault

Fri Jun 5 12:13:20 UTC 2015

Dne 3.6.2015 v 14:17 Jan Cholasta napsal(a):
> 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.
>

Attached is a patch including the requested changes.

I have also changed vault_config to vaultconfig_show, for consistency 
with {,dns}config_show (it also makes the transport certificate 
retrieval code in vault_{archive,retrieve} simpler).

I have noticed that triple-length DES is used for the session key. 
Wouldn't AES be better?

         # generate session key
         mechanism = nss.CKM_DES3_CBC_PAD

BTW, ipa-kra-install is broken with pki-core-10.2.4-1, but it works with 
pki-core-10.2.1-3.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-vault-archive-and-vault-retrieve-commands.patch
Type: text/x-patch
Size: 26262 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150605/c1ab1853/attachment.bin>