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

[Michael Šimáček]

On 2015-07-29 19:20, Robbie Harwood wrote:
> Michael Šimáček <msimacek at redhat.com> writes:
>
>> 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.
>
> This is sub-optimal, but not a huge deal if it's only in the test
> suite.
>
>> 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.
>
> Please try not to do this.  DEFINITELY do not parse krb.conf.
> Unfortunately, I do not know enough about the LDAP code to know why this
> is needed or to suggest an alternate solution.
>
>> Sorry for long patch, but I'm afraid it cannot be reasonably split.
>
> This is indeed really long and difficult to work through.  I have
> probably missed some things; apologies if they come through in a later
> round.
>
>> +try:
>> +    cred = kinit_keytab(principal, keytab_name, ccache_name)
>> +    # would raise exception if expired
>> +    cred.lifetime
>> +except gssapi.exceptions.ExpiredCredentialsError:
>> +    # delete stale ccache and try again
>> +    os.unlink(ccache_name)
>> +    cred = kinit_keytab(principal, keytab_name, ccache_name)
>
> See next comment.
>
>> -    # The keytab may have stale key material (from older trust-add run)
>> -    if not os.path.isfile(oneway_ccache_name):
>> -        oneway_ccache = kinit_keytab(oneway_principal, oneway_keytab_name, oneway_ccache_name)
>> -except krbV.Krb5Error as e:
>> +    try:
>> +        # The keytab may have stale key material (from older trust-add run)
>> +        cred = kinit_keytab(oneway_principal, oneway_keytab_name, oneway_ccache_name)
>> +        # would raise exception if expired
>> +        cred.lifetime
>> +    except gssapi.exceptions.ExpiredCredentialsError:
>> +        cred = kinit_keytab(oneway_principal, oneway_keytab_name, oneway_ccache_name)
>> +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)
>
> In general, it's bad practice to catch *all* possible GSS errors unless
> you intend to display their status and/or abort/raise.  If there's a
> specific state you want to cope with here, catch the exception related
> to it, not all of them.  Up above is a place where I think this is done
> right.

I haven't found any specific exception for keytab problems, what should 
I catch?
But there's a different error, there should be one more attempt to get 
the credentials there. I'll fix it in the next revision of the patch.

>
>> -        ctx = krbV.default_context()
>> -        ccache = ctx.default_ccache()
>> -        principal = ccache.principal()
>> -    except krbV.Krb5Error, e:
>> +        principal = krb_utils.get_principal()
>> +    except errors.CCacheError:
>>           sys.exit("Must have Kerberos credentials to setup AD trusts on server")
>
> Based on how GSSAPI error messages are being packed into CCache errors
> (the name of which is itself unfortunate...), it would be nice to give
> some hint of the problem here if it were GSSAPI; otherwise, to my eye,
> it looks like the GSSAPI status is being dropped.

Will do (or just not rewrapping might actually be better)


>
>> +def get_credentials(name=None, ccache_name=None):
>>       '''
>> -    Kerberos stores a TGT (Ticket Granting Ticket) and the service
>> -    tickets bound to it in a ccache (credentials cache). ccaches are
>> -    bound to a Kerberos user principal. This class opens a Kerberos
>> -    ccache and allows one to manipulate it. Most useful is the
>> -    extraction of ticket entries (cred's) in the ccache and the
>> -    ability to examine their attributes.
>> +    Obtains GSSAPI credentials with given principal name from ccache. When no
>> +    principal name specified, it retrieves the default one for given
>> +    credentials cache.
>> +
>> +    :parameters:
>> +      name
>> +        gssapi.Name object specifying principal or None for the default
>> +      ccache_name
>> +        string specifying Kerberos credentials cache name or None for the
>> +        default
>> +    :returns:
>> +      gssapi.Credentials object
>> +    ''''
>> +    store = None
>> +    if ccache_name:
>> +        store = {'ccache': ccache_name}
>> +    try:
>> +        return gssapi.Credentials(usage='initiate', name=name, store=store)
>> +    except gssapi.exceptions.GSSError as e:
>> +        if e.min_code == KRB5_FCC_NOFILE:
>> +            raise ValueError('"%s", ccache="%s"' % (e.message, ccache_name))
>> +        raise errors.CCacheError()
>
> This is another case where it stands out that the specific error from
> GSSAPI should probably be checked.

I will try to get rid of the rewrapping entirely, the wrapper doesn't 
add any value.

>
>> +    # FIXME this is a temporary workaround. We should find some nicer solution
>> +    name = gssapi.Name('notempty', gssapi.NameType.user)
>> +    can_name = unicode(name.canonicalize(gssapi.MechType.kerberos))
>> +    return can_name.partition('@')[2] or None
>
> As mentioned in my email to you, I do not think we can guarantee that
> the realm will actually be present after the '@' in all cases, so
> hopefully everything copes with a None here.  But if it copes with a
> None, why have this code?

krbV's get_default_realm can also return None under some circumstances, 
but I doubt the code using it actually copes with it. It would be ideal 
to get rid of the need to know the default realm from kerberos. I will 
ask in a new thread, so someone who understands the ldap part can 
comment on it. Or maybe ctypes would be the way to go as suggested in 
other part of this thread.


>
>> +# 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)
>>
>>
>>   class KRBCheater(object):
>>       def __init__(self, app):
>>           self.app = app
>>           self.url = app.url
>> -        self.ccname = api.Backend.krb.default_ccname()
>> +        self.ccname = get_default_ccache_name()
>>       def __call__(self, environ, start_response):
>>           environ['KRB5CCNAME'] = self.ccname
>
> If all you're doing is using this to set $KRB5CCNAME (as __call__
> suggests), that variable when unset is the default value and the
> song-and-dance to get the default ccache name isn't necessary.
>

In production, the variable is always set by mod_auth_gssapi. So in 
tests I try to mimic production environment. But I think you're right, 
the code could be adjusted to cope without it.

Thank you for your feedback, I'll post a next revision of the patch 
after we clarify how to proceed with the default realm.

--
Michael Simacek