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

Mon Aug 24 16:23:22 UTC 2015


On 2015-08-24 17:49, Simo Sorce wrote:
> On Mon, 2015-08-24 at 17:18 +0200, Michael Šimáček wrote:
>> On 2015-08-24 14:50, Jan Cholasta wrote:
>>> On 23.8.2015 23:27, Michael Šimáček wrote:
>>>>
>>>>
>>>> On 2015-08-21 15:52, Michael Šimáček wrote:
>>>>>
>>>>>
>>>>> On 2015-08-20 20:42, Robbie Harwood wrote:
>>>>>> Michael Šimáček <msimacek at redhat.com> writes:
>>>>>>
>>>>>>> On 2015-08-20 12:32, Michael Šimáček wrote:
>>>>>>>
>>>>>>>>>>> Michael Šimáček <msimacek at redhat.com> writes:
>>>>>>>>>>>
>>>>>>>>>>>> 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
>>>>>>>>
>>>>>>>> Rebased on top of current master.
>>>>>>>
>>>>>>> One of the commits reintroduced krbV dependency that I didn't notice.
>>>>>>> Attaching updated revision. Only changes against previous revision are
>>>>>>> in files daemons/dnssec/ipa-dnskeysync-replica and
>>>>>>> daemons/dnssec/ipa-ods-exporter.
>>>>>>
>>>>>> This is much better, thanks!  I've got some comments inline.
>>>>>>
>>>>>>> +except gssapi.exceptions.GSSError:
>>>>>>>        # If there was failure on using keytab, assume it is stale and
>>>>>>> retrieve again
>>>>>>>        retrieve_keytab(api, ccache_name, oneway_keytab_name,
>>>>>>> oneway_principal)
>>>>>>
>>>>>> This code still bothers me a bit, but I think fixing it is probably
>>>>>> beyond the scope of a python-gssapi port.
>>>>>
>>>>> The code catches all GSSAPI exceptions and retries to do the same thing
>>>>> with different keytab. So if there was a problem unrelated to keytab,
>>>>> the same exception will be raised again. Nothing will be ignored
>>>>> silently.
>>>>>
>>>>>>
>>>>>>> +    try:
>>>>>>> +        creds = get_credentials(name=name, ccache_name=ccache_name)
>>>>>>> +        # property access would raise exception if expired
>>>>>>> +        if creds.lifetime > 0:
>>>>>>> +            return creds
>>>>>>> +    except gssapi.exceptions.ExpiredCredentialsError:
>>>>>>> +        return None
>>>>>>
>>>>>> Per rfc2744, lifetime is unsigned.  It's not immediately clear what
>>>>>> will
>>>>>> happen when `creds.lifetime == 0`; perhaps an explicit `return Nune` in
>>>>>> that case?
>>>>>
>>>>> I think the check is probably redundant, gssapi raises exception upon
>>>>> inquiring expired credentials. In trust-fetch-domains I just access the
>>>>> lifetime in try-except without using the value, so I could do the same
>>>>> here. It would be nice if gssapi provided some 'is_valid' or
>>>>> 'is_expired' method, so I wouldn't need to rely on side-effects of
>>>>> property access, which is hard to read and confuses pylint.
>>>>>
>>>>>>
>>>>>>>            # Setup LDAP connection
>>>>>>>            try:
>>>>>>> -            ctx = krbV.default_context()
>>>>>>> -            ccache = ctx.default_ccache()
>>>>>>> -            api.Backend.ldap2.connect(ccache)
>>>>>>> +            api.Backend.ldap2.connect()
>>>>>>>                cls.ldap = api.Backend.ldap2
>>>>>>> -        except krbV.Krb5Error as e:
>>>>>>> +        except gssapi.exceptions.GSSError:
>>>>>>>                sys.exit("Must have Kerberos credentials to migrate
>>>>>>> Winsync users.")
>>>>>>
>>>>>> Can you log the error here?  The other places GSSError is being caught
>>>>>> are doing a great job of either filtering-and-raising or
>>>>>> logging-and-exiting, so thanks for fixing those.
>>>>>
>>>>> Yes, I'll update it in next revision of the patch.
>>>>>
>>>>>>
>>>>>>> +# Ugly hack for test purposes only. GSSAPI has no way to get default
>>>>>>> ccache
>>>>>>> +# name, but we don't need it outside test server
>>>>>>> +def get_default_ccache_name():
>>>>>>> +    try:
>>>>>>> +        out = check_output(['klist'])
>>>>>>> +    except CalledProcessError:
>>>>>>> +        raise RuntimeError("Default ccache not found. Did you
>>>>>>> kinit?")
>>>>>>> +    match = re.match(r'^Ticket cache:\s*(\S+)', out)
>>>>>>> +    if not match:
>>>>>>> +        raise RuntimeError("Cannot obtain ccache name")
>>>>>>> +    return match.group(1)
>>>>>>
>>>>>> Yup, this is still ugly.  Ah well, it's only test code.
>>>>>>
>>>>>
>>>>> I was trying to modify the code to not need the variable and just use
>>>>> the default, but it is used for manipulating it as a file - in
>>>>> production it is always defined by mod_auth_gssapi. So I'd keep this
>>>>> as is.
>>>>>
>>>>
>>>>
>>>> Next revision of the patch. Changes from previous rev: printing
>>>> exception in ipa_winsync_migrate and more thorough dealing with
>>>> credentials expiration in krb_utils and trust-fetch-domains.
>>>
>>> 1) There is a merge conflict in freeipa.spec.in, please rebase the patch
>>> on top of current master.
>>>
>>
>> Done.
>>
>>>
>>> 2) pylint fails with:
>>>
>>> ************* Module ipa-ods-exporter
>>> daemons/dnssec/ipa-ods-exporter:23: [E0611(no-name-in-module), ] No name
>>> 'GSSError' in module 'gssapi')
>>> ************* Module ipa-dnskeysync-replica
>>> daemons/dnssec/ipa-dnskeysync-replica:15: [E0611(no-name-in-module), ]
>>> No name 'GSSError' in module 'gssapi')
>>>
>>> Both failures are caused by "from gssapi import GSSError" statement,
>>> which should read "from gssapi.exceptions import GSSError".
>>>
>>
>> Fixed.
>>
>>>
>>> 3) ipa-adtrust-install fails with:
>>>
>>> admin password:
>>>
>>> Unrecognized error during check of admin rights:
>>> admin at abc.idm.lab.eng.brq.redhat.com: user not found
>>>
>>> Apparently there is a "user-show admin at abc.idm.lab.eng.brq.redhat.com"
>>> call where a "user-show admin" call should be.
>>>
>>
>> Fixed. python-gssapi has a display_as method that could pull the name
>> from it, but it doesn't work in current version, therefore using
>> partition to split on '@'
>>
>>>
>>> 4) ipa-client-automount fails with:
>>>
>>> Failed to obtain host TGT: Major (851968): Unspecified GSS failure.
>>> Minor code may provide more information, Minor (2529639111): Bad format
>>> in credentials cache
>>>
>>
>> gssapi apparently refuses to accept empty file as ccache. In order to
>> not create a race condition / security hole by deleting and recreating
>> the temp file, I changed it to use per-process kernel keyring (I believe
>> non-persistent keyring should be available on all supported platforms).
>
> Ypu can create a temp directory in tmp where only the user has access
> to, then you can have a file within it. This way you can still use a
> file w/o having to create it, w/o race condition, but with compatibility
> to file ccaches in case the kernel/krb5 libs are not built with keyring
> support.
>

Done. Attaching new revision that uses mkdtemp.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-msimacek-0002-9-Port-from-python-krbV-to-python-gssapi.patch
Type: text/x-patch
Size: 72269 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150824/abc485b5/attachment.bin>