[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Martin Kosek
mkosek at redhat.com
Fri Sep 5 08:59:14 UTC 2014
On 09/04/2014 03:09 PM, Jan Cholasta wrote:
> Dne 4.9.2014 v 13:40 Martin Kosek napsal(a):
>> 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
>>
>
> Here.
>
Thanks, I just had to remove one pylint bug:
ipaserver/install/cainstance.py:324: [E0601(used-before-assignment),
stop_tracking_certificates] Using variable 'e' before assignment)
Pushed to master, ipa-4-1, ipa-4-0.
Martin
More information about the Freeipa-devel
mailing list