On 2015-07-29 19:20, Robbie Harwood wrote:
Michael Šimáček <msima...@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

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to