[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Jan Cholasta
jcholast at redhat.com
Thu Sep 4 13:09:19 UTC 2014
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.
--
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0008-8-ipa41-Use-certmonger-D-Bus-API-instead-of-messing-with-its.patch
Type: text/x-patch
Size: 34160 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140904/eb8858d7/attachment.bin>
More information about the Freeipa-devel
mailing list