[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

Dne 5.6.2015 v 21:50 Endi Sukma Dewata napsal(a):
> 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.

I don't agree - ideally the vault plugin should do the check at runtime, 
because it should work without httpd restart when KRA is installed on 
other replica. The KRA installer needs to be fixed in order to support 
this. I will provide a patch.

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

I have fixed the commands to inherit from Local.

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

Fixed.

>
> 4. There's a PEP8 error.

Fixed.

>
> 5. The VERSION needs to be updated.

Fixed.

>
> Assuming the above issues are addressed, ACK.

OK, pushed to master: df1bd39a43f30138cf55e0e7720fa3dec1d912e0

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

OK, no problem.

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

The patch needs a rebase and version bumb ("VERSION" line at the top of 
ipa-pki-proxy.conf).

-- 
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: 26525 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150608/8935a892/attachment.bin>