[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.

Tue Sep 2 11:56:48 UTC 2014

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?

>>
>>
>> 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__().

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

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.

-- 
Jan Cholasta