[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:45:59 UTC 2014
Dne 3.9.2014 v 16:25 David Kupka napsal(a):
> 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.
>
And I need to do my reviews more carefully: in ca_renewal_master, the
"cert-storage" criterium should in fact be "cert-database".
ACK after you fix the above.
--
Jan Cholasta
More information about the Freeipa-devel
mailing list