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

[Michael Šimáček]

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.