[Freeipa-devel] [PATCH] Password vault

[Martin Kosek]

On 06/02/2015 11:22 PM, Alexander Bokovoy wrote:
> On Tue, 02 Jun 2015, Endi Sukma Dewata wrote:
>> Please take a look at the new patch.
>>
>> On 6/2/2015 10:05 AM, Martin Kosek wrote:
>>>>> 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...
>>>>
>>>> In my opinion it should be fine to require pki-base on the client because it
>>>> contains just the client library, unless you have other concerns? Any
>>>> objections to having pki-nss and pki-cryptography dependencies on the client?
>>>>
>>>> Even if we can change the client code not to depend on "pki" module, since in
>>>> this framework the client and server code are written in the same plugin, the
>>>> "import pki" still cannot be removed since it's still needed by the server
>>>> code, and I don't think conditional import is a good programming practice.
>>>
>>> I have major concerns here. Look at the different between installing
>>> "freeipa-client" and "freeipa-client + pki-base" on my F21:
>>>
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> $ sudo yum install freeipa-client
>>> ...
>>> Install  1 Package (+4 Dependent packages)
>>>
>>> Total download size: 2.6 M
>>> Installed size: 14 M
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> $ sudo yum install freeipa-client pki-base
>>> ...
>>> Install  2 Packages (+288 Dependent packages)
>>>
>>> Total download size: 160 M
>>> Installed size: 235 M
>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> This is obviously a no-go for client. The conditional import is smaller concern
>>> that big dependency growth on the client. We do them in trust plugin for
>>> example and it works fine (though I agree it is not ideal programming
>>> practice).
>>>
>>> IMO, we should limit new freeipa-client dependencies only to
>>> python-cryptography (or also python-nss in the worst case, in case
>>> python-cryptography is not enough) - there should be no pki dependencies at
>>> all, these should be only on the server side.
>>
>> OK. I opened a ticket to split the pki-base into separate Python and Java
>> packages:
>> https://fedorahosted.org/pki/ticket/1399
>>
>> For now in this patch I added conditional imports for pki.account and pki.key
>> which are needed to access KRA on the server side. I removed dependency on
>> pki.crypto on the client side and replaced it with direct python-nss code.
>>
>> On 6/2/2015 1:40 PM, Simo Sorce wrote:
>>> I have coded in python (jwcrypto)
>>> support for some key wrapping not yet available in python-cryptography
>>> and can lend you the code as needed.
>>
>> Thanks. I'll get back to you when it's time to add support for
>> python-cryptography in KRA:
>> https://fedorahosted.org/pki/ticket/1400
>>
>> On 6/2/2015 10:16 AM, Alexander Bokovoy wrote:
>>> Yes, please use conditional import here, it is perfectly valid use case
>>> for the client side.
>>
>> On 6/2/2015 1:40 PM, Simo Sorce wrote:
>>> conditional import is just fine
>>
>> The conditional imports that I've seen usually are used for importing
>> different versions of the same module, which I think is acceptable because
>> the dependency always exists. In the vault case we're selectively importing a
>> module depending on where the code runs. I think that is bad because it adds
>> complexity and it's easy to make mistakes. Any code that depends on that
>> module would have to be (a) guarded:
>>
>>  if pki_is_loaded:
>>      ... call pki ...
>>
>> or (b) used in a method that's only called if the module is loaded:
>>
>>  def do_something(self): # runs only on server
>>      ... call pki ...
>>
>> The (a) is similar to #ifdef's which should be avoidable using OOD, and in
>> (b) we may inadvertently call a wrong method indirectly. I think ideally the
>> client and server code should be in separate files (so they can be deployed
>> separately too), but the framework doesn't seem to allow that.
> This exactly the case we have to use here and we are using that in
> trusts case as well -- some code has to run on server only and shouldn't
> cause to install Samba related packages on the client. This is because
> IPA client is actually using the same IPA plugins that server uses, to
> have access to the API calls metadata and client-side callbacks are
> defined in the same place where server-side callbacks are. It is IPA
> framework design, so we have to use what we have.

This is planned to be changed BTW, when we start with the "Thin Client" concept
and have different code/plugins for FreeIPA server side and client side.

> In other words, it is not necessarily an evil under conditions we are
> dealing with.
>