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.

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

--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to