[Freeipa-devel] [PATCH] Password vault

[Endi Sukma Dewata]

On 6/5/2015 7:13 AM, Jan Cholasta wrote:
>>>> 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 other thread was talking about removing the pki-base dependency on 
the client side, but the vault plugin is still loaded on both client and 
server regardless of KRA installation. Ideally the vault plugin should 
not even be loaded so you cannot even execute the commands.

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

I see the vault_archive and vault_retrieve now inherit from PKQuery, and 
there is a hack to execute the forward() even on the server side. A few 
things below:

1. Why didn't you use frontend.Local as you initially suggested? If 
there's a problem with frontend.Local please attach the ticket number in 
the code.

2. The forward() can be merged into run(). There is no need to keep the 
code in forward(). It would make more sense to have a run() method that 
runs both on client and server, rather than a forward() that is supposed 
to run on the client only but now forced to run on server too, 
semantically speaking.

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

3. The parameter description for nonce should be just 'Nonce' instead of 
'Nonce encrypted'.

4. There's a PEP8 error.

5. The VERSION needs to be updated.

Assuming the above issues are addressed, ACK.

> 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

That's the default used by the KRA's client library, and that's what the 
KRA has been tested with. We probably can change it to AES later. It 
shouldn't be blocking this patch.

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

There's a bug in IPA: https://bugzilla.redhat.com/show_bug.cgi?id=1228671

-- 
Endi S. Dewata