[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
David Kupka
dkupka at redhat.com
Thu Sep 4 10:31:50 UTC 2014
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.
--
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-8-ipa40-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 32526 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140904/0fe5e53f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-8-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 35087 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140904/0fe5e53f/attachment-0001.bin>
More information about the Freeipa-devel
mailing list