PR#3426: header-based sessions - koji

		`@@ -153,6 +153,17 @@`
		`option.`
		`The default value of False is recommended.`

		`+ DisableURLSessions`
		`+ Type: boolean`
		`+`
		+ Default: ``False``
		`+`
		+ If set to ``False``, it enables older clients to log in via session parameters
		`+ encoded in URL. New behaviour uses header-based parameteres. This default`
		+ will be changed in future to ``True`` effectively disabling older clients. It is
		+ encouraged to set it to ``True`` as soon as possible when no older clients are
		`+ using the hub. (Added in 1.30, will be removed in 1.34)`
		`+`
		`Enabling gssapi auth also requires settings in the httpd config.`

		`SSL client certificate auth configuration`

hub/hub.conf

file modified

		`@@ -35,6 +35,8 @@`
		`# AllowedKrbRealms = *`
		`## TODO: this option should be removed in future release`
		`# DisableGSSAPIProxyDNFallback = False`
		`+ ## TODO: this option should be turned True in 1.34`
		`+ # DisableURLSessions = False`

		`## end Kerberos auth configuration`

hub/kojixmlrpc.py

file modified

		`@@ -448,6 +448,8 @@`
		`['AllowedKrbRealms', 'string', '*'],`
		`# TODO: this option should be removed in future release`
		`['DisableGSSAPIProxyDNFallback', 'boolean', False],`
		`+ # TODO: this option should be turned True in 1.34`
		`+ ['DisableURLSessions', 'boolean', False],`

		`['DNUsernameComponent', 'string', 'CN'],`
		`['ProxyDNs', 'string', ''],`

koji/__init__.py

file modified

+30 -9

		`@@ -2700,11 +2700,21 @@`
		`if name == 'rawUpload':`
		`return self._prepUpload(args, *kwargs)`
		`args = encode_args(args, *kwargs)`
		`+ headers = []`
		`if self.logged_in:`
		`sinfo = self.sinfo.copy()`
		`sinfo['callnum'] = self.callnum`
		`self.callnum += 1`
		`- handler = "%s?%s" % (self.baseurl, six.moves.urllib.parse.urlencode(sinfo))`
		`+ if sinfo.get('header-auth'):`
		`+ handler = self.baseurl`
		`+ headers += [`
		`+ ('Koji-Session-Id', str(self.sinfo['session-id'])),`
		`+ ('Koji-Session-Key', str(self.sinfo['session-key'])),`
		`+ ('Koji-Session-Callnum', str(sinfo['callnum'])),`
		`+ ]`
		`+ else:`
		`+ # old server`
		`+ handler = "%s?%s" % (self.baseurl, six.moves.urllib.parse.urlencode(sinfo))`
		`elif name == 'sslLogin':`
		`handler = self.baseurl + '/ssllogin'`
		`else:`
		`@@ -2715,7 +2725,7 @@`
		`# encoded as UTF-8. For python3 it means "return a str with an appropriate`
		`# xml declaration for encoding as UTF-8".`
		`request = request.encode('utf-8')`
		`- headers = [`
		`+ headers += [`
		`# connection class handles Host`
		`('User-Agent', 'koji/1'),`
		`('Content-Type', 'text/xml'),`
		`@@ -3030,20 +3040,31 @@`
		`"""prep a rawUpload call"""`
		`if not self.logged_in:`
		`raise ActionNotAllowed("you must be logged in to upload")`
		`- args = self.sinfo.copy()`
		`- args['callnum'] = self.callnum`
		`- args['filename'] = name`
		`- args['filepath'] = path`
		`- args['fileverify'] = verify`
		`- args['offset'] = str(offset)`
		`+ sinfo = self.sinfo.copy()`
		`+ sinfo['callnum'] = self.callnum`
		`+ args = {`
		`+ 'filename': name,`
		`+ 'filepath': path,`
		`+ 'fileverify': verify,`
		`+ 'offset': str(offset),`
		`+ }`
		`if overwrite:`
		`args['overwrite'] = "1"`
		`if volume is not None:`
		`args['volume'] = volume`
		`size = len(chunk)`
		`self.callnum += 1`
		`+ headers = []`
		`+ if sinfo.get('header-auth'):`
		`+ headers += [`
		`+ ('Koji-Session-Id', str(self.sinfo['session-id'])),`
		`+ ('Koji-Session-Key', str(self.sinfo['session-key'])),`
		`+ ('Koji-Session-Callnum', str(sinfo['callnum'])),`
		`+ ]`
		`+ else:`
		`+ args.update(sinfo)`
		`handler = "%s?%s" % (self.baseurl, six.moves.urllib.parse.urlencode(args))`
		`- headers = [`
		`+ headers += [`
		`('User-Agent', 'koji/1'),`
		`("Content-Type", "application/octet-stream"),`
		`("Content-length", str(size)),`

koji/auth.py

file modified

+31 -16

		`@@ -29,7 +29,6 @@`

		`import six`
		`from six.moves import range, urllib`
		`-`
		`import koji`
		`from .context import context`
		`from .util import to_list`
		`@@ -79,24 +78,36 @@`
		`self._perms = None`
		`self._groups = None`
		`self._host_id = ''`
		`- # get session data from request`
		`- if args is None:`
		`- environ = getattr(context, 'environ', {})`
		`- args = environ.get('QUERY_STRING', '')`
		`+ environ = getattr(context, 'environ', {})`
		`+ args = environ.get('QUERY_STRING', '')`
		`+ # prefer new header-based sessions`
		`+ if 'HTTP_KOJI_SESSION_ID' in environ:`
		`+ id = int(environ['HTTP_KOJI_SESSION_ID'])`
		`+ key = environ['HTTP_KOJI_SESSION_KEY']`
		`+ try:`
		`+ callnum = int(environ['HTTP_KOJI_CALLNUM'])`
		`+ except KeyError:`
		`+ callnum = None`
		`+ elif not context.opts['DisableURLSessions'] and args is not None:`
		`+ # old deprecated method with session values in query string`
		`+ # Option will be turned off by default in future release and removed later`
		`if not args:`
		`- self.message = 'no session args'`
		`+ self.message = 'no session header or session args'`
		`return`
		`args = urllib.parse.parse_qs(args, strict_parsing=True)`
		`+ try:`
		`+ id = int(args['session-id'][0])`
		`+ key = args['session-key'][0]`
		`+ except KeyError as field:`
		`+ raise koji.AuthError('%s not specified in session args' % field)`
		`+ try:`
		`+ callnum = args['callnum'][0]`
		`+ except Exception:`
		`+ callnum = None`
		`+ else:`
		`+ self.message = 'no Koji-Session-* headers'`
		`+ return`
		`hostip = self.get_remote_ip(override=hostip)`
		`- try:`
		`- id = int(args['session-id'][0])`
		`- key = args['session-key'][0]`
		`- except KeyError as field:`
		`- raise koji.AuthError('%s not specified in session args' % field)`
		`- try:`
		`- callnum = args['callnum'][0]`
		`- except Exception:`
		`- callnum = None`
		`# lookup the session`
		`# sort for stability (unittests)`

		`@@ -489,7 +500,11 @@`
		`context.cnx.commit()`

		`# return session info`
		`- return {'session-id': session_id, 'session-key': key}`
		`+ return {`
		`+ 'session-id': session_id,`
		`+ 'session-key': key,`
		`+ 'header-auth': True, # signalize to client to use new session handling in 1.30`
		`+ }`

		`def subsession(self):`
		`"Create a subsession"`

tests/test_lib/test_auth.py

file modified

+47 -4

		`@@ -43,16 +43,27 @@`
		`# start with "assert"`
		`self.context.session.assertLogin = mock.MagicMock()`

		`- def test_instance(self):`
		`+ @mock.patch('koji.auth.context')`
		`+ def test_instance(self, context):`
		`"""Simple auth.Session instance"""`
		`- s = koji.auth.Session()`
		`+ context.opts = {`
		`+ 'CheckClientIP': True,`
		`+ 'DisableURLSessions': False,`
		`+ }`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ koji.auth.Session()`
		`# no args in request/environment`
		`- self.assertEqual(s.message, 'no session args')`
		`+ self.assertEqual(cm.exception.args[0], "'session-id' not specified in session args")`

		`@mock.patch('koji.auth.context')`
		`- def get_session(self, context):`
		`+ def get_session_old(self, context):`
		`"""auth.Session instance"""`
		`# base session from test_basic_instance`
		`+ # url-based auth - will be dropped in 1.34`
		`+ context.opts = {`
		`+ 'CheckClientIP': True,`
		`+ 'DisableURLSessions': False,`
		`+ }`
		`context.environ = {`
		`'QUERY_STRING': 'session-id=123&session-key=xyz&callnum=345',`
		`'REMOTE_ADDR': 'remote-addr',`
		`@@ -72,6 +83,38 @@`
		`s = koji.auth.Session()`
		`return s, context`

		`+ @mock.patch('koji.auth.context')`
		`+ def get_session(self, context):`
		`+ # base session from test_basic_instance`
		`+ # header-based auth`
		`+ context.opts = {`
		`+ 'CheckClientIP': True,`
		`+ 'DisableURLSessions': True,`
		`+ }`
		`+ context.environ = {`
		`+ 'HTTP_KOJI_SESSION_ID': '123',`
		`+ 'HTTP_KOJI_SESSION_KEY': 'xyz',`
		`+ 'HTTP_KOJI_CALLNUM': '345',`
		`+ 'REMOTE_ADDR': 'remote-addr',`
		`+ }`
		`+`
		`+ self.query_executeOne.side_effect = [`
		`+ {'authtype': 2, 'callnum': 1, "date_part('epoch', start_time)": 1666599426.227002,`
		`+ "date_part('epoch', update_time)": 1666599426.254308, 'exclusive': None,`
		`+ 'expired': False, 'master': None,`
		`+ 'start_time': datetime.datetime(2022, 10, 24, 8, 17, 6, 227002,`
		`+ tzinfo=datetime.timezone.utc),`
		`+ 'update_time': datetime.datetime(2022, 10, 24, 8, 17, 6, 254308,`
		`+ tzinfo=datetime.timezone.utc),`
		`+ 'user_id': 1},`
		`+ {'name': 'kojiadmin', 'status': 0, 'usertype': 0}]`
		`+ self.query_singleValue.return_value = 123`
		`+ s = koji.auth.Session()`
		`+ return s, context`
		`+`
		`+ def test_session_old(self):`
		`+ self.get_session_old()`
		`+`
		`def test_basic_instance(self):`
		`"""auth.Session instance"""`
		`s, cntext = self.get_session()`

tkopecek commented 2 years ago

/tests will be fixed when we agree on solution/

pretty please pagure-ci rebuild

2 years ago

mikem commented 2 years ago

I'll start with the big one. I'm not sure we should use cookies for this. Cookies are tied to browser behavior and the hub is not intended to be accessed with a browser. Also, while python-requests makes using cookies fairly easy for us, things might be more involved for non-python clients.

The key thing we need to do here is get this information out of the url. The simplest thing is probably to just put this data in a custom header of the client request.

mikem commented 2 years ago

Next big things are fallback behaviors.

In the current version of this PR, the hub no longer supports the url parameters at all (in fact the args parameter to the session class is unused). This means that version n-1 will be able effectively authenticate with the version where this lands.

I realize that we want to expunge these url parameters, but I think we should allow for backwards compatibility for at least a brief time. Best thing is probably to have a hub config option to allow it it and deprecate the behavior. We can remove it in a later version.

Secondly, while the client code does have fallback behavior, it ends up sending every call twice which is not ideal. The client should remember that the server needs the fallback.

mikem commented 2 years ago

Server could report in sinfo that the new behavior is expected. Client could easily cue its behavior off of that.

tkopecek commented 2 years ago

I'll start with the big one. I'm not sure we should use cookies for this. Cookies are tied to browser behavior and the hub is not intended to be accessed with a browser. Also, while python-requests makes using cookies fairly easy for us, things might be more involved for non-python clients.

The key thing we need to do here is get this information out of the url. The simplest thing is probably to just put this data in a custom header of the client request.

Yep, it looks like safer solution, on the other hand, cookies are pretty standard now even for xmlrpc. User-side cookie is de facto just a header - is there any important difference to handling custom header? With potential future things like automatic session reauth for expiring sessions we would probably end with cookie-like behaviour.

tkopecek commented 2 years ago

ping @mikem

mikem commented 2 years ago

on the other hand, cookies are pretty standard now even for xmlrpc.

I'm not sure that's true.

On the flip side, for REST apis it seems like JWT is an emerging standard (not that I'm suggesting we use those right now).

With potential future things like automatic session reauth for expiring sessions we would probably end with cookie-like behaviour.

Ultimately, cookies are solving a much larger problem than auth and bring with them a very complex history.

I think that while we may overlap a bit with some aspects of cookies, that overlap is not going to be perfect or complete.

rebased onto f0713af0a6f684bdf7a277cee10df3b070efca93

2 years ago

2 new commits added

use header-based auth
cookies-based sessions

2 years ago

ktdreyer commented 2 years ago

The RH Bugzilla authors recently had to solve a similar problem. They added an "Authorization" header, like this:

Authorization: Bearer <api-key-string>

I like this implementation because it's very simple for client authors. In particular, there's only one value to handle instead of Koji's two (session-id and session-key). Can we use this opportunity to start dropping session-id handling?

I wonder how easy it will be for non-Python XML-RPC clients to add HTTP headers. I did it for my txbugzilla library but I don't have experience with the other ecosystems like the java or go clients that perform a lot of authenticated calls.

tkopecek commented 2 years ago

token (as in BZ) is a single point of failure - this allows login without any other encrypted channel. So, e.g. in some (weird) cases when auth is going through http or via untrusted SSL CAs it would reveal the token in header.
we also encode callnum here to ensure that non-idempotent calls are not done twice. We would need to either decouple this from auth header or ensure this behaviour in some other way.
we can theoretically drop session-id with ensuring that session-key is really unique (this is not true now)
@mikem ?

ktdreyer commented 2 years ago

Re: token as a single point of failure - yes, this is the case for essentially all web applications. I think we should set SSLRequireSSL for the entire Koji application in Apache (see #2163).

Maybe the word "Bearer" in an Authorization header is simply too generic and too easy for developers to confuse with OpenID (it originally came from OpenID).

What if you renamed the header in this PR from "X-Session-Data" to "X-Koji-Session"? I think that would be clearer in logs and client libraries what this is.

mikem commented 2 years ago

I wonder how easy it will be for non-Python XML-RPC clients to add HTTP headers

Even in python, the native xmlrpc.client lib only added the headers option in 3.8. It would not be surprising if other implementations don't make this easy since headers aren't really part of the xmlrpc spec.

In Koji, we handle our connections with python-requests and only use the xmlrpc lib for encoding and decoding the request, so we've been able to control our headers for a while. Good thing too, since we'll need to backport this for py2.

Edited 2 years ago by mikem

mikem commented 2 years ago

we can theoretically drop session-id with ensuring that session-key is really unique (this is not true now)

Easiest way to ensure uniqueness is to embed the session-id. We already embed the userid, and it looks like a very simple change to include the session id.

mikem commented 2 years ago

I do like the idea of keeping this header simpler, so a single key with session-id embedded would be preferred. That should allow us to simplify these changes a bit (e.g. avoid importing email and simplify the header parsing in auth.py.

Might be better to invert the sense of the config option to AllowURLSessions. Also, it should not be grouped with the kerberos-specific options. The more general auth settings are a bit later.

tkopecek commented 2 years ago

@mikem So, you mean just passing 'X-Session-Id' via headers and leave callnum in query string?

mikem commented 2 years ago

So, you mean just passing 'X-Session-Id' via headers and leave callnum in query string?

Hmm, so I guess I'd say specify that in a separate header for callnum. It looks like passing callnum is technically optional, so if a 3rd party client leaves this out it should just skip the callnum check.

As far as header naming, it appears that using the "X- prefix is deprecated, so perhaps is it simpler to just prefix everything with Koji-, e.g.

Koji-Session-Id
Koji-Session-Callnum

Another aspect of your question is whether we want to support mixing the two when AllowURLSessions is set, and I think probably not. I'd like to encourage code for move forward and it would be nice to get those urls cleaner.

rebased onto 261880638845301676202815cc20b85850edf49a

2 years ago

mikem commented a year ago

Ok, I think this is about where it needs to be. Just a couple things.

we can drop the import email from kojixmlrpc now I think
I don't think kojixmlrpc needs to return the session info via headers. We're already returning that info via the login call
given 2, auth doesn't need to copy the session info into context
typo in auth.py. Should be: self.message = 'no session header or session args'