[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Martin Kosek
mkosek at redhat.com
Thu Sep 4 11:40:09 UTC 2014
On 09/04/2014 01:19 PM, Jan Cholasta wrote:
> 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.
>
Thanks! But as I just found out, neither patch applies to ipa-4-1 branch. And
as the merge is not straightforward, I would leave the backport rather either
to Jan or David.
Martin
More information about the Freeipa-devel
mailing list