PR#1187: Add ctx option to ClientSession.krb_login() - koji

koji

#1187 Add ctx option to ClientSession.krb_login()

Merged 5 years ago by mikem. Opened 5 years ago by mprahl.

Unknown source krb_login-thread-safe into master

Don't force the Kerberos cache to be stored as a file when using the ccache kwarg on krb_login

mprahl • 5 years ago

5687f7b

Add a `ctx` kwarg to ClientSession.krb_login

mprahl • 5 years ago

4b6c920

koji/__init__.py

file modified

+6 -4

		`@@ -2107,7 +2107,7 @@`
		`sinfo = self.callMethod('subsession')`
		`return type(self)(self.baseurl, self.opts, sinfo)`

		`- def krb_login(self, principal=None, keytab=None, ccache=None, proxyuser=None):`
		`+ def krb_login(self, principal=None, keytab=None, ccache=None, proxyuser=None, ctx=None):`
		`"""Log in using Kerberos. If principal is not None and keytab is`
		`not None, then get credentials for the given principal from the given keytab.`
		`If both are None, authenticate using existing local credentials (as obtained`
		`@@ -2115,7 +2115,8 @@`
		`not specified, the default ccache will be used. If proxyuser is specified,`
		`log in the given user instead of the user associated with the Kerberos`
		`principal. The principal must be in the "ProxyPrincipals" list on`
		`- the server side."""`
		`+ the server side. ctx is the Kerberos context to use, and should be unique`
		`+ per thread. If ctx is not specified, the default context is used."""`

		`try:`
		`# Silently try GSSAPI first`
		`@@ -2134,10 +2135,11 @@`
		`"Please install python-krbV to use kerberos."`
		`)`

		`- ctx = krbV.default_context()`
		`+ if not ctx:`
		`+ ctx = krbV.default_context()`

		`if ccache != None:`
		`- ccache = krbV.CCache(name='FILE:' + ccache, context=ctx)`
		`+ ccache = krbV.CCache(name=ccache, context=ctx)`
		`else:`
		`ccache = ctx.default_ccache()`

mprahl commented 5 years ago

The first commit adds a ctx kwarg to krb_login. Before this change, koji.ClientSession.krb_login always used the default context. This can be an issue when a multi-threaded application shares this context and the Kerberos cache is stored in the thread keyring.

In this scenario, the first thread to run krb_login will succeed while all others will get a "Permission denied" error. By adding the ctx kwarg, a thread can establish a context and tell krb_login to use it instead of the default context.

The second commit makes it so that you can pass in a non-file ccache location to krb_login. This is useful because a multi-threaded application may choose to store the Kerberos cache in the thread keyring [1] to avoid Kerberos cache corruption. Since krb_login prepends the passed in ccache with FILE:, the application must resort to setting the KRB_CCACHE environment variable. This is annoying and unnecessary because ccache defaults to FILE anyways if no Kerberos cache type is specified in the value for ccache [2].

1 - http://man7.org/linux/man-pages/man7/thread-keyring.7.html
2 - https://web.mit.edu/kerberos/krb5-1.12/doc/basic/ccache_def.html#ccache-types

These issues were encountered in the Module Build Service (MBS), and this is currently hot-fixed in our product deployment to resolve our issue.

mikeb commented 5 years ago

+1 Looks good!

mprahl commented 5 years ago

@mikem, @tkopecek, or @breilly could you please review?

ngompa commented 5 years ago

:thumbsup:

mcurlej commented 5 years ago

@mikeb @mikem @tkopecek does this needs anything else before it can be merged?

tkopecek commented 5 years ago

+1 @mikem Is there some reason to have FILE hardcoded?

mikem commented 5 years ago

Is there some reason to have FILE hardcoded?

@mikeb do you recall?

mikeb commented 5 years ago

Is there some reason to have FILE hardcoded?

@mikeb do you recall?

@tkopecek @mikem Yes, me being pedantic. I hadn't considered the possibility of using a different ccache type. The Kerberos docs say that FILE: is the default if no type is specified, so this change is completely backward-compatible.

https://web.mit.edu/kerberos/krb5-latest/doc/basic/ccache_def.html

mikem commented 5 years ago

Confirmed, works for me.

$ kdestroy
$ kinit -c /tmp/FOOBAR mikem
...
>>> session.krb_login(ccache='/tmp/FOOBAR')

mikem commented 5 years ago

I've edited the title because I think "thread-safe" is a bit far. This change makes it possible to use krb_login in a multi-threaded environment, but requires the client to do part of the work and still defaults to the old behavior.

mikem commented 5 years ago

Commit f04346e fixes this pull-request