[Freeipa-devel] [PATCH] 0008 Use certmonger D-Bus API instead of messing with its files.
Martin Basti
mbasti at redhat.com
Tue Sep 9 14:25:03 UTC 2014
On 05/09/14 10:59, Martin Kosek wrote:
> 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
>
I found a problem during upgrade
https://fedorahosted.org/freeipa/ticket/4529
--
Martin Basti
More information about the Freeipa-devel
mailing list