[Freeipa-devel] [PATCH 0002] Port from python-krbV to python-gssapi

Thu Aug 13 07:38:25 UTC 2015

On 2015-08-03 09:25, Jan Cholasta wrote:
> Dne 31.7.2015 v 20:20 Simo Sorce napsal(a):
>> On Fri, 2015-07-31 at 16:41 +0200, Michael Šimáček wrote:
>>> On 2015-07-31 07:52, Jan Cholasta wrote:
>>>> Hi Michael,
>>>>
>>>> Dne 29.7.2015 v 10:09 Michael Šimáček napsal(a):
>>>>> Hi,
>>>>>
>>>>> this is the first attempt to port FreeIPA from deprecated
>>>>> python3-incompatible python-krbV library to python-gssapi. The patch
>>>>> depends on python-kerberos->python-gssapi patch [1] to apply cleanly,
>>>>> but the overlap is small, so I think it can be at least partially
>>>>> reviewed without it.
>>>>>
>>>>> Comments:
>>>>> I removed Backend.krb and KRB5_CCache classes as they were wrappers
>>>>> around krbV classes. I added few utility functions to krb_utils module
>>>>> that perform part of its functionality (no need for classes, because
>>>>> gssapi acquire calls don't pass any context objects, they wouldn't
>>>>> have
>>>>> any state).
>>>>>
>>>>> I merged the two different kinit_keytab functions.
>>>>>
>>>>> GSSAPI doesn't provide any method (that I'm aware of) to get default
>>>>> ccache name. In most cases this is not needed as we can simply not
>>>>> pass
>>>>> any name and it will use the default. The ldap plugin had to be
>>>>> adjusted
>>>>> for this - the connect method now takes new use_gssapi argument, which
>>>>> can turn on gssapi support without the need to supply explicit ccache
>>>>> name. The only place where the ccache name is really needed is the
>>>>> test
>>>>> server, where I use system klist command to obtain it.
>>>>
>>>> I would prefer if the semantics were the same as in IPAdmin, i.e.
>>>> GSSAPI
>>>> is used by default if bind password is not specified, see
>>>> IPAdmin.do_bind() in ipapython.ipaldap.
>>>
>>> Just to clarify, the current flow in ldap module is:
>>> if ccache: # I added "or use_gssapi" here in this patch
>>>       gssapi_bind
>>> elif autobind:
>>>       external_bind
>>> else:
>>>       simple_bind
>>
>> I had to make this change as well for my replica promotion code, and
>> incidentally used the same indicator "use_gssapi".
>>
>>> and you would like it to be changed into:
>>> if bind_pw:
>>>       simple_bind
>>> elif autobind:
>>>       external_bind
>>> else:
>>>       gssapi_bind
>>>
>>> Is that correct?
>
> Actually this is what IPAdmin does:
>
>      def do_bind(self, dm_password="", autobind=AUTOBIND_AUTO,
> timeout=DEFAULT_TIMEOUT):
>          if dm_password:
>              self.do_simple_bind(bindpw=dm_password, timeout=timeout)
>              return
>          if autobind != AUTOBIND_DISABLED and os.getegid() == 0 and
> self.ldapi:
>              try:
>                  # autobind
>                  pw_name = pwd.getpwuid(os.geteuid()).pw_name
>                  self.do_external_bind(pw_name, timeout=timeout)
>                  return
>              except errors.NotFound, e:
>                  if autobind == AUTOBIND_ENABLED:
>                      # autobind was required and failed, raise
>                      # exception that it failed
>                      raise
>
>          #fall back
>          self.do_sasl_gssapi_bind(timeout=timeout)
>
>>
>> I think this is what Jan wanted, but I am wondering if it is the right
>> thing to do. In ipa we have basically 2 possible default approaches.
>> One is to use GSSAPI, and one is to use LDAPI with external bind.
>>
>> The latter makes sense mostly only when running as root, so I am
>> wondering, should the default change depending on whether we are root
>> and we are connecting to the local LDAP server ?
>>
>> If this is a sensible option it means we have to preserver use_gssapi as
>> we may need to force use of gssapi in some case even when we are root
>> and connectiong to the local server (for example to test that the local
>> ccache can successfully be used).
>>
>> Jan,
>> what do you think ?
>
> I think GSSAPI should be the default and EXTERNAL should be opt-in, like
> in IPAdmin, see above.
>
>>
>>>>
>>>>>
>>>>> It's also not possible to directly get default realm name, what I
>>>>> do is
>>>>> importing nonexistent name, cannonicalizing it and extracting the
>>>>> realm
>>>>> from it. Which should work but is ugly. It would be better if we could
>>>>> modify the places that use it to not need it at all, but it's mostly
>>>>> used in ldap code and I don't understand that part of FreeIPA.
>>>>> Alternative would be parsing /etc/krb.conf.
>>>>
>>>> You should use api.env.realm where possible. I think this should be
>>>> most
>>>> of the places where default realm is currently used, if not all of
>>>> them.
>>>
>>> That would be great if all the usages could be replaced. How can I
>>> determine where api.env.realm can be used? In particular, I'm unsure
>>> about ipapython/config.py/__discover_config and
>>> ipaserver/plugins/join.py.
>
> I would just remove the code from __discover_config. It is used to get
> realm name in case it is not configured in /etc/ipa/default.conf, but it
> is called only from ipa-compat-manage and ipa-nis-manage, which can be
> run only on IPA server, and IPA server won't work if realm is not
> configured.
>
> As for join.py, you can just return api.env.realm in get_realm().
>
>>
>> try:
>>     realm = api.env.realm
>> except:
>>     realm = dirty gssapi trick ?
>
> Please don't, you should always be able to choose the correct one
> instead of guessing.
>

Attaching new revision of the patch. Changes from the previous:
- ldap2's connect now chooses the bind type same way as in ipaldap
- get_default_realm usages replaced by api.env.realm
- fixed missing third kinit attempt in trust-fetch-domains
- removed rewrapping gssapi errors to ccache errors in krb_utils
- updated some parts of exception handling

Michael
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-msimacek-0002-3-Port-from-python-krbV-to-python-gssapi.patch
Type: text/x-patch
Size: 69772 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150813/508995e6/attachment.bin>