On 11/18/2013 01:54 PM, Tomas Babej wrote: > On 11/15/2013 03:36 PM, Rob Crittenden wrote: >> Tomas Babej wrote: >>> On 11/15/2013 02:46 PM, Ana Krivokapic wrote: >>>> On 11/13/2013 02:57 PM, Tomas Babej wrote: >>>>> On 09/27/2013 10:14 AM, Martin Kosek wrote: >>>>>> On 09/26/2013 04:46 PM, Jan Cholasta wrote: >>>>>>> On 26.9.2013 12:59, Tomas Babej wrote: >>>>>>>> On 09/26/2013 12:54 PM, Jan Cholasta wrote: >>>>>>>>> On 24.9.2013 18:14, Nalin Dahyabhai wrote: >>>>>>>>>> On Tue, Sep 24, 2013 at 01:30:10PM +0200, Jan Cholasta wrote: >>>>>>>>>>> We discussed this with Tomáš off-line and it turns out that >>>>>>>>>>> ipa-client-install fails if the CA cert is not added to >>>>>>>>>>> /etc/pki/nssdb. >>>>>>>>>>> >>>>>>>>>>> However, according to p11-kit docs it should work: >>>>>>>>>>> <http://p11-glue.freedesktop.org/doc/p11-kit/trust-nss.html>. I >>>>>>>>>>> wonder what needs to be done to make it work in IPA... >>>>>>>>>> >>>>>>>>>> On my system, there's no symlink to libnssckbi.so (or the right >>>>>>>>>> location >>>>>>>>>> in the link farm under /etc/alternatives) in /etc/pki/nssdb, so >>>>>>>>>> that >>>>>>>>>> database isn't going to automatically pull in the list of >>>>>>>>>> trusted CAs >>>>>>>>>> that p11-kit maintains. >>>>>>>>>> >>>>>>>>>> Whether the database under /etc/pki/nssdb should automatically >>>>>>>>>> include >>>>>>>>>> the usual set of trust anchors is probably a different >>>>>>>>>> conversation. >>>>>>>>> >>>>>>>>> Thanks for the info. >>>>>>>>> >>>>>>>>> Tomáš, the patch is fine then. I have one more nitpick though: >>>>>>>>> why did >>>>>>>>> you change "the default NSS database" to "the NSS database"? The >>>>>>>>> database in /etc/pki/nssdb *is* the default NSS database, so please >>>>>>>>> change it back. Also I think "systemwide CA trust database" is >>>>>>>>> better >>>>>>>>> than "systemwide CA store". >>>>>>>>> >>>>>>>>> Honza >>>>>>>>> >>>>>>>> I fixed the descriptions. Updated patch attached. >>>>>>>> >>>>>>>> Tomas >>>>>>>> >>>>>>> >>>>>>> Thanks. >>>>>>> >>>>>>> There's one more thing: we should probably check if >>>>>>> /usr/bin/update-ca-trust >>>>>>> exists before using it, for the sake of cross-distro compatibility. >>>>>>> >>>>>> >>>>>> Right. I am also thinking if this functionality should not be >>>>>> somehow integrated into the platform files so that it can be >>>>>> overriden in platforms that do not have the systemwide storage. >>>>>> >>>>>> Martin >>>>> >>>>> Updated patch attached, requires my patch 130. >>>>> >>>>> >>>>> >>>>> _______________________________________________ >>>>> Freeipa-devel mailing list >>>>> Freeipa-devel@redhat.com >>>>> https://www.redhat.com/mailman/listinfo/freeipa-devel >>>> >>>> The patch works fine; a couple of nitpicks: >>>> >>>> 1) The import of root_logger in services.py.in is unused. >>>> >>>> 2) In ipa-client-install, you log the return values of functions >>>> insert_ca_cert_into_systemwide_ca_store() and >>>> remove_ca_cert_from_systemwide_ca_store(). But these functions do not >>>> return any values, so you will always be logging `None`. >>>> >>> Thanks for the review, >>> >>> I removed the code (it was meant for debugging purposes only). >>> >>> Updated patch attached. >> >> Adding the CA to the NSS cert database is considered a fatal error. Should >> adding it to the global trust database be fatal as well? >> >> I don't know the answer, but if we want to do this at some point should these >> functions return True/False to denote success/failure? >> >> rob > > I don't think it should be considered fatal, at least not now. > > I updated the patch to return the success/failure status, even though, this > could be done when it will be required. But doesn't hurt anything either, at > least other platform files will develop systemwide CA store functions with > this approach in mind. > > Updated patch attached. >
Looks good, ACK. -- Regards, Ana Krivokapic Associate Software Engineer FreeIPA team Red Hat Inc. _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
