[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
David Kupka
dkupka at redhat.com
Wed Sep 3 14:25:21 UTC 2014
On 09/03/2014 04:05 PM, Jan Cholasta wrote:
> Dne 3.9.2014 v 12:37 David Kupka napsal(a):
>> On 09/02/2014 01:56 PM, Jan Cholasta wrote:
>>> Dne 29.8.2014 v 14:34 David Kupka napsal(a):
>>>> Hope, I've addressed all the issues (except 9 and 11, inline). Let's go
>>>> for another round :-)
>>>>
>>>> On 08/27/2014 11:05 AM, Jan Cholasta wrote:
>>>>> Hi,
>>>>>
>>>>> Dne 25.8.2014 v 15:39 David Kupka napsal(a):
>>>>>> On 08/19/2014 05:44 PM, Rob Crittenden wrote:
>>>>>>> David Kupka wrote:
>>>>>>>> On 08/19/2014 09:58 AM, Martin Kosek wrote:
>>>>>>>>> On 08/19/2014 09:05 AM, David Kupka wrote:
>>>>>>>>>> FreeIPA will use certmonger D-Bus API as discussed in this thread
>>>>>>>>>> https://www.redhat.com/archives/freeipa-devel/2014-July/msg00304.html
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> This change should prevent hard-to-reproduce bugs like
>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4280
>>>>>>>>>
>>>>>>>>> Thanks for this effort, the updated certmonger module looks much
>>>>>>>>> better! This
>>>>>>>>> will help us get rid of the non-standard communication with
>>>>>>>>> certmonger.
>>>>>>>>>
>>>>>>>>> Just couple initial comments from me by reading the code:
>>>>>>>>>
>>>>>>>>> 1) Testing needs fixed version of certmonger, right? This needs
>>>>>>>>> to be
>>>>>>>>> spelled
>>>>>>>>> out right with the patch.
>>>>>>>> Yes, certmonger 0.75.13 and above should be fine according ticket
>>>>>>>> https://fedorahosted.org/certmonger/ticket/36. Added to patch
>>>>>>>> description.
>>>>>>>
>>>>>>> You should update the spec to set the minimum version as well.
>>>>>> Sure, thanks.
>>>>>>>
>>>>>>>>>
>>>>>>>>> 2) Description text in patches is cheap, do not be afraid to
>>>>>>>>> use it
>>>>>>>>> and
>>>>>>>>> describe what you did and why. Link to the ticket is missing in
>>>>>>>>> the
>>>>>>>>> description
>>>>>>>>> as well:
>>>>>>>> Ok, increased verbosity a bit :-)
>>>>>>>>>
>>>>>>>>>> Subject: [PATCH] Use certmonger D-Bus API instead of messing with
>>>>>>>>>> its
>>>>>>>>>> files.
>>>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 3) get_request_id API:
>>>>>>>>>
>>>>>>>>>> criteria = (
>>>>>>>>>> - ('cert_storage_location',
>>>>>>>>>> dogtag_constants.ALIAS_DIR,
>>>>>>>>>> - certmonger.NPATH),
>>>>>>>>>> - ('cert_nickname', nickname, None),
>>>>>>>>>> + ('cert_storage_location',
>>>>>>>>>> dogtag_constants.ALIAS_DIR),
>>>>>>>>>> + ('cert_nickname', nickname),
>>>>>>>>>> )
>>>>>>>>>> request_id = certmonger.get_request_id(criteria)
>>>>>>>>>
>>>>>>>>> Do we want to continue using the "criteria" object or should we
>>>>>>>>> rather
>>>>>>>>> switch
>>>>>>>>> to normal function options? I.e. rather using
>>>>>>>>>
>>>>>>>>> request_id = certmonger.get_request_id(cert_nickname=nickname,
>>>>>>>>> cert_storage_location=dogtag_constants.ALIAS_DIR)
>>>>>>>>>
>>>>>>>>> ? It would look more consistent with other calls. I am just
>>>>>>>>> asking,
>>>>>>>>> not insisting.
>>>>>>>> I've no preference here. It seems to be a very small change. Has
>>>>>>>> anyone
>>>>>>>> a reason to do it one way and not the other?
>>>>>>>
>>>>>>> I think I used this criteria thing to avoid having a bazillion
>>>>>>> optional
>>>>>>> parameters and for future-proofing. I think at this point the
>>>>>>> list is
>>>>>>> probably pretty stable, so I'd base it on whether you care about
>>>>>>> having
>>>>>>> a whole ton of optional parameters or not (it has the advantage of
>>>>>>> self-documenting itself).
>>>>>>>
>>>>>> The list is probably stable but also really excessive. I don't
>>>>>> think it
>>>>>> would help to have more than dozen optional parameters. So I
>>>>>> prefer to
>>>>>> leave as-is and change it in future if it is wanted.
>>>>>>>>>
>>>>>>>>> 3) Starting function:
>>>>>>>>>
>>>>>>>>>> + try:
>>>>>>>>>> + ipautil.run([paths.SYSTEMCTL, 'start', 'certmonger'],
>>>>>>>>>> skip_output=True)
>>>>>>>>>> + except Exception, e:
>>>>>>>>>> + root_logger.error('Failed to start certmonger: %s' % e)
>>>>>>>>>> + raise e
>>>>>>>>>
>>>>>>>>> I see 2 issues related to this code:
>>>>>>>>> a) Do not call SYSTEMCTL directly. To be platform independent,
>>>>>>>>> rather use
>>>>>>>>> services.knownservices.messagebus.start() that is overridable by
>>>>>>>>> someone else
>>>>>>>>> porting to non-systemd platforms.
>>>>>>>> Is there anything that can't be done using
>>>>>>>> ipalib/ipapython/ipaplatform?
>>>>>>>
>>>>>>> It can't make coffee (yet).
>>>>>>>
>>>>>>>>> b) In this case, do not use "raise e", but just "raise" to keep
>>>>>>>>> the
>>>>>>>>> exception
>>>>>>>>> stack trace intact for better debugging.
>>>>>>>> Every day there's something new to learn about python or FreeIPA.
>>>>>>>>>
>>>>>>>>> Both a) and b) should be fixed in other occasions and places.
>>>>>>>> I found only one occurence of a) issue. Is there some hidden or are
>>>>>>>> you
>>>>>>>> talking about the whole FreeIPA project?
>>>>>>>>>
>>>>>>>>> 4) Feel free to add yourself to Authors section of this module.
>>>>>>>>> You
>>>>>>>>> refactored
>>>>>>>>> it greatly to earn it :-)
>>>>>>>> Done.
>>>>>>>
>>>>>>> You already import dbus, why also separately import DBusException?
>>>>>>>
>>>>>> Removed, thanks for noticing.
>>>>>>> rob
>>>>>>>
>>>>>
>>>>> 1) The patch needs to be rebased.
>>>
>>> I didn't notice the patch is targeted for 4.0. Can you please provide
>>> patches for both ipa-4-0 and ipa-4-1/master?
>>>
>>
>> Attached, 0008-5 works on master/ipa-4-1 and 0008-5-ipa40 works on
>> ipa-4-0.
>
> There is a little bug in ipa-upgradeconfig in the 4.0 version of the
> patch. This is wrong:
>
> for request in requests:
> nss_dir, nickname, ca_name, pre_command, post_command, profile
> = request
> criteria = {
> 'cert-database': nss_dir,
> 'cert-nickname': nickname,
> 'ca-name': ca_name,
> 'template-profile': profile,
> }
>
> It should be:
>
> for nss_dir, nickname, ca_name, pre_command, post_command in
> requests:
> criteria = {
> 'cert-database': nss_dir,
> 'cert-nickname': nickname,
> 'ca-name': ca_name,
> }
>
>>
>>>>>
>>>>>
>>>>> 2) Please try to follow PEP8, at least in certmonger.py.
>>>>>
>>>>>
>>>>> 3) In certificate_renewal_update() in ipa-upgradeconfig you removed
>>>>> ca_name from criteria.
>>>>>
>>>>>
>>>>> 4) IMO renaming nickname to cert_nickname in dogtag_start_tracking()
>>>>> and
>>>>> stop_tracking() is unnecessary. We can keep calling request nicknames
>>>>> "request_id" and certificate nicknames "nickname" in our APIs.
>>>>>
>>>>>
>>>>> 5) I think it would be better to add a docstring to
>>>>> _cm_dbus_object.__init__() instead of doing this:
>>>>>
>>>>> # object is accesible over this DBus bus instance
>>>>> bus = None
>>>>> # DBus object path
>>>>> path = None
>>>>> # the actual DBus object
>>>>> obj = None
>>>>> # object interface name
>>>>> obj_dbus_if = None
>>>>> # object parent interface name
>>>>> parent_dbus_if = None
>>>>> # object inteface
>>>>> obj_if = None
>>>>> # property interface
>>>>> prop_if = None
>>>
>>> You removed the comments, but left the attributes there. You should
>>> remove them as well, they are not necessary, as you set them all in
>>> __init__().
>>>
>>
>> Removed.
>>
>>>>>
>>>>>
>>>>> 6) In _start_certmonger(), please check if certmonger is already
>>>>> running
>>>>> before starting it, what applies to systemd might not apply to other
>>>>> init systems.
>>>>>
>>>>>
>>>>> 7) Do we ever need to connect to certmonger on the session bus? If
>>>>> not,
>>>>> there is no need to support it in _connect_to_certmonger().
>>>>>
>>>>>
>>>>> 8) You should not ignore other criteria when 'nickname' is
>>>>> specified in
>>>>> _get_requests(). Use find_request_by_nickname only if 'nickname' is
>>>>> the
>>>>> only criterion, otherwise filter the result of get_requests, including
>>>>> 'nickname'.
>>>>>
>>>>>
>>>>> 9) IMO you can indeed remove request_cert().
>>>>
>>>> Not sure, it is used in self-test although I doubt anyone ever ran it.
>>>>
>>>>>
>>>>>
>>>>> 10) You forgot to port modify() and resubmit().
>
> You no longer check if the profile argument is set in modify() - either
> re-introduce the check, or remove the default value of the argument to
> make it mandatory.
>
>>>>>
>>>>>
>>>>> 11) As for get_pin(), it should be moved to ipapython.dogtag,
>>>>> because it
>>>>> is Dogtag related (you don't need to do it in this patch).
>>>>>
>>>>
>>>> This patch is ugly enough. I will create a separate one for this.
>>>
>>> OK, no need to include the TODO comment though.
>>>
>>>>
>>>>>
>>>>> I haven't done functional testing yet, expect more comments later.
>>>>>
>>>>> Honza
>>>>>
>>>>
>>>
>>> 12) ipa-client-install still uses ipa-getcert instead of certmonger API
>>> to request the host certificate, but since we plan on stopping doing
>>> that (https://fedorahosted.org/freeipa/ticket/4449), I guess you don't
>>> have to do anything about it.
>>
>> Ok. I will leave it on you since you are owner of this ticket.
>>
>>>
>>>
>>> 13) You require dict for criteria in get_request_id, but you pass tuples
>>> to it in ipa-upgradeconfig, ipa-cacert-manage and ipa-certupdate, which
>>> makes them fail.
>>
>> Good point, fixed.
>
> I forgot about ipaserver.install.plugins.ca_renewal_master (sorry), it
> should be fixed as well.
>
I need to check my patches more carefully, thank for the patience.
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-6-ipa40-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 29435 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140903/2988d590/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-6-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 32216 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140903/2988d590/attachment-0001.bin>
More information about the Freeipa-devel
mailing list