[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Jan Cholasta
jcholast at redhat.com
Wed Sep 3 14:05:47 UTC 2014
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.
--
Jan Cholasta
More information about the Freeipa-devel
mailing list