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

[Jan Cholasta]

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.


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".


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.


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


Honza

-- 
Jan Cholasta