[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Jan Cholasta
jcholast at redhat.com
Thu Sep 4 11:19:04 UTC 2014
Dne 4.9.2014 v 12:31 David Kupka napsal(a):
> On 09/03/2014 04:45 PM, Jan Cholasta wrote:
>> 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.
>>
> Fixed together with other issues discussed on IRC. Please review it one
> more time.
Thanks, ACK.
--
Jan Cholasta
More information about the Freeipa-devel
mailing list