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

[Michael Šimáček]

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.

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