PR#1648: support multiple realms by kerberos auth - koji

		`@@ -6131,6 +6131,7 @@`
		`'krbservice': 'host',`
		`'krb_rdns': True,`
		`'krb_canon_host': False,`
		`+ 'krb_server_realm': None,`
		`'server': None,`
		`'user': None,`
		`'password': None,`

cli/koji.conf

file modified

		`@@ -28,6 +28,9 @@`
		`;enable to lookup dns canonical hostname for krb auth`
		`;krb_canon_host = no`

		`+ ;The realm of server principal. Using client's realm if not set`
		`+ ;krb_server_realm = EXAMPLE.COM`
		`+`

		`;configuration for SSL authentication`

cli/koji_cli/commands.py

file modified

+1 -1

		`@@ -7332,7 +7332,7 @@`
		`elif authtype == koji.AUTHTYPE_GSSAPI:`
		`print("Authenticated via GSSAPI")`
		`elif authtype == koji.AUTHTYPE_KERB:`
		`- print("Authenticated via Kerberos principal %s" % u["krb_principal"])`
		`+ print("Authenticated via Kerberos principal %s" % session.krb_principal)`
		`elif authtype == koji.AUTHTYPE_SSL:`
		`print("Authenticated via client certificate %s" % options.cert)`

docs/schema-upgrade-1.18-1.19.sql

file modified

+12

		`@@ -65,4 +65,16 @@`
		`-- add kernel-image and imitramfs`
		`insert into archivetypes (name, description, extensions) values ('kernel-image', 'Kernel BZ2 Image', 'vmlinuz vmlinuz.gz vmlinuz.xz');`
		`insert into archivetypes (name, description, extensions) values ('initramfs', 'Compressed Initramfs Image', 'img');`
		`+`
		`+ -- schema update for https://pagure.io/koji/issue/1629`
		`+ CREATE TABLE user_krb_principals (`
		`+ user_id INTEGER NOT NULL REFERENCES users(id),`
		`+ krb_principal VARCHAR(255) NOT NULL UNIQUE,`
		`+ PRIMARY KEY (user_id, krb_principal)`
		`+ ) WITHOUT OIDS;`
		`+`
		`+ INSERT INTO user_krb_principals ( SELECT id, krb_principal FROM users WHERE users.krb_principal IS NOT NULL);`
		`+`
		`+ ALTER TABLE users DROP COLUMN krb_principal;`
		`+`
		`COMMIT;`

docs/schema.sql

file modified

+7 -2

		`@@ -38,8 +38,13 @@`
		`name VARCHAR(255) UNIQUE NOT NULL,`
		`password VARCHAR(255),`
		`status INTEGER NOT NULL,`
		`- usertype INTEGER NOT NULL,`
		`- krb_principal VARCHAR(255) UNIQUE`
		`+ usertype INTEGER NOT NULL`
		`+ ) WITHOUT OIDS;`
		`+`
		`+ CREATE TABLE user_krb_principals (`
		`+ user_id INTEGER NOT NULL REFERENCES users(id),`
		`+ krb_principal VARCHAR(255) NOT NULL UNIQUE,`
		`+ PRIMARY KEY (user_id, krb_principal)`
		`) WITHOUT OIDS;`

		`CREATE TABLE permissions (`

hub/hub.conf

file modified

		`@@ -22,6 +22,9 @@`
		`# ProxyPrincipals = koji/kojiweb@EXAMPLE.COM`
		`## format string for host principals (%s = hostname)`
		`# HostPrincipalFormat = compile/%s@EXAMPLE.COM`
		`+ ## Allowed Kerberos Realms separated by ','.`
		`+ ## Default value "*" indicates any Realm is allowed`
		`+ # AllowedKrbRealms = *`

		`## end Kerberos auth configuration`

hub/kojihub.py

file modified

+156 -26

		`@@ -3663,40 +3663,104 @@`
		`return repos`


		`- def get_user(userInfo=None, strict=False):`
		`+ def get_user(userInfo=None, strict=False, krb_princs=False):`
		`"""Return information about a user.`

		`- userInfo may be either a str (Kerberos principal or name) or an int (user id).`
		`-`
		`- A map will be returned with the following keys:`
		`- id: user id`
		`- name: user name`
		`- status: user status (int), may be null`
		`- usertype: user type (int), 0 person, 1 for host, may be null`
		`- krb_principal: the user's Kerberos principal`
		`+ :param userInfo: either a str (Kerberos principal or name) or an int (user id)`
		`+ :param strict: whether raising Error when no user found`
		`+ :param krb_princs: whether show krb_principals in result`
		`+ :return: a dict as user's information:`
		`+ id: user id`
		`+ name: user name`
		`+ status: user status (int), may be null`
		`+ usertype: user type (int), 0 person, 1 for host, may be null`
		`+ krb_principals: the user's Kerberos principals (list)`
		`"""`
		`if userInfo is None:`
		`userInfo = context.session.user_id`
		`if userInfo is None:`
		`# not logged in`
		`raise koji.GenericError("No user provided")`
		`- fields = ['id', 'name', 'status', 'usertype', 'krb_principal']`
		`- #fields, aliases = zip(*fields.items())`
		`- data = {'info' : userInfo}`
		`+ fields = ['id', 'name', 'status', 'usertype']`
		`+ data = {'info': userInfo}`
		`if isinstance(userInfo, six.integer_types):`
		`clauses = ['id = %(info)i']`
		`elif isinstance(userInfo, str):`
		`clauses = ['krb_principal = %(info)s OR name = %(info)s']`
		`else:`
		`- raise koji.GenericError('invalid type for userInfo: %s' % type(userInfo))`
		`+ raise koji.GenericError('invalid type for userInfo: %s'`
		`+ % type(userInfo))`
		`query = QueryProcessor(tables=['users'], columns=fields,`
		`+ joins=['LEFT JOIN user_krb_principals'`
		`+ ' ON users.id = user_krb_principals.user_id'],`
		`clauses=clauses, values=data)`
		`user = query.executeOne()`
		`if not user and strict:`
		`raise koji.GenericError("No such user: %r" % userInfo)`
		`+ if user and krb_princs:`
		`+ user['krb_principals'] = list_user_krb_principals(user['id'])`
		`return user`


		`+ def list_user_krb_principals(user_info=None):`
		`+ """Return kerberos principal list of a user.`
		`+`
		`+ :param user_info: either a str (username) or an int (user id)`
		`+ :return: user's kerberos principals (list)`
		`+ """`
		`+ if user_info is None:`
		`+ user_info = context.session.user_id`
		`+ if user_info is None:`
		`+ # not logged in`
		`+ raise koji.GenericError("No user provided")`
		`+ fields = ['krb_principal']`
		`+ data = {'info': user_info}`
		`+ if isinstance(user_info, six.integer_types):`
		`+ joins = []`
		`+ clauses = ['user_id = %(info)i']`
		`+ elif isinstance(user_info, str):`
		`+ joins = ['users ON users.id = user_krb_principals.user_id']`
		`+ clauses = ['name = %(info)s']`
		`+ else:`
		`+ raise koji.GenericError('invalid type for user_info: %s'`
		`+ % type(user_info))`
		`+ query = QueryProcessor(tables=['user_krb_principals'],`
		`+ columns=fields, joins=joins,`
		`+ clauses=clauses, values=data,`
		`+ transform=lambda row: row['krb_principal'])`
		`+ return query.execute() or []`
		`+`
		`+`
		`+ def get_user_by_krb_principal(krb_principal, strict=False, krb_princs=False):`
		`+ """get information about a user by kerberos principal.`
		`+`
		`+ :param krb_principal: full user kerberos principals`
		`+ :param strict: whether raising Error when no user found`
		`+ :param krb_princs: whether show krb_principals in result`
		`+ :return: a dict as user's information:`
		`+ id: user id`
		`+ name: user name`
		`+ status: user status (int), may be null`
		`+ usertype: user type (int), 0 person, 1 for host, may be null`
		`+ krb_principals: the user's Kerberos principals (list)`
		`+ """`
		`+ if krb_principal is None:`
		`+ raise koji.GenericError("No kerberos principal provided")`
		`+ if not isinstance(krb_principal, str):`
		`+ raise koji.GenericError("invalid type for krb_principal: %s"`
		`+ % type(krb_principal))`
		`+ fields = ['user_id']`
		`+ data = {'krb_principal': krb_principal}`
		`+ clauses = ['krb_principal = %(krb_principal)s']`
		`+ query = QueryProcessor(tables=['users_krb_principals'], columns=fields,`
		`+ clauses=clauses, values=data)`
		`+ princ_item = query.executeOne()`
		`+ if not princ_item and strict:`
		`+ raise koji.GenericError("Cannot find user with kerberos principal: %s"`
		`+ % krb_principal)`
		`+ return get_user(data, strict=strict, krb_princ=krb_princs)`
		`+`
		`+`
		`def find_build_id(X, strict=False):`
		`if isinstance(X, six.integer_types):`
		`return X`
		`@@ -7936,10 +8000,13 @@`
		`if not ginfo or ginfo['usertype'] != koji.USERTYPES['GROUP']:`
		`raise koji.GenericError("Not a group: %s" % group)`
		`group_id = ginfo['id']`
		`- fields = ('id', 'name', 'usertype', 'krb_principal')`
		`+ fields = ('id', 'name', 'usertype', 'array_remove(array_agg(krb_principal)'`
		`+ ', NULL) AS krb_principals')`
		`q = """SELECT %s FROM user_groups`
		`- JOIN users ON user_id = users.id`
		`- WHERE active = TRUE AND group_id = %%(group_id)i""" % ','.join(fields)`
		`+ JOIN users ON user_groups.user_id = users.id`
		`+ LEFT JOIN user_krb_principals ON users.id = user_krb_principals.user_id`
		`+ WHERE active = TRUE AND group_id = %%(group_id)i`
		`+ GROUP BY users.id""" % ','.join(fields)`
		`return _multiRow(q, locals(), fields)`

		`def set_user_status(user, status):`
		`@@ -8243,13 +8310,16 @@`
		`asList: if True, return results as a list of lists, where each list contains the`
		`column values in query order, rather than the usual list of maps`
		`rowlock: if True, use "FOR UPDATE" to lock the queried rows`
		`+ group: a column or alias name to use in the 'GROUP BY' clause`
		`+ (controlled by enable_group)`
		`+ - enable_group: if True, opts.group will be enabled`
		`"""`

		`iterchunksize = 1000`

		`def __init__(self, columns=None, aliases=None, tables=None,`
		`joins=None, clauses=None, values=None, transform=None,`
		`- opts=None):`
		`+ opts=None, enable_group=False):`
		`self.columns = columns`
		`self.aliases = aliases`
		`if columns and aliases:`
		`@@ -8282,6 +8352,7 @@`
		`self.opts = opts`
		`else:`
		`self.opts = {}`
		`+ self.enable_group = enable_group`

		`def countOnly(self, count):`
		`self.opts['countOnly'] = count`
		`@@ -8293,6 +8364,7 @@`
		`FROM %(table_str)s`
		`%(join_str)s`
		`%(clause_str)s`
		`+ %(group_str)s`
		`%(order_str)s`
		`%(offset_str)s`
		`%(limit_str)s`
		`@@ -8314,6 +8386,10 @@`
		`clause_str = self._seqtostr(self.clauses, sep=')\n AND (')`
		`if clause_str:`
		`clause_str = ' WHERE (' + clause_str + ')'`
		`+ if self.enable_group:`
		`+ group_str = self._group()`
		`+ else:`
		`+ group_str = ''`
		`order_str = self._order()`
		`offset_str = self._optstr('offset')`
		`limit_str = self._optstr('limit')`
		`@@ -8379,6 +8455,17 @@`
		`else:`
		`return ''`

		`+ def _group(self):`
		`+ group_opt = self.opts.get('group')`
		`+ if group_opt:`
		`+ group_exprs = []`
		`+ for group in group_opt.split(','):`
		`+ if group:`
		`+ group_exprs.append(group)`
		`+ return 'GROUP BY ' + ', '.join(group_exprs)`
		`+ else:`
		`+ return ''`
		`+`
		`def _optstr(self, optname):`
		`optval = self.opts.get(optname)`
		`if optval:`
		`@@ -8480,9 +8567,11 @@`
		`offset`
		`limit`

		`- Note: asList is supported by QueryProcessor but not by this method.`
		`+ Note:`
		`+ - asList is supported by QueryProcessor but not by this method.`
		`We don't know the original query order, and so don't have a way to`
		`return a useful list. asList should be handled by the caller.`
		`+ - group is supported by QueryProcessor but not by this method as well.`
		`"""`
		`if queryOpts is None:`
		`queryOpts = {}`
		`@@ -10885,10 +10974,34 @@`
		`context.session.assertPerm('admin')`
		`if get_user(username):`
		`raise koji.GenericError('user already exists: %s' % username)`
		`- if krb_principal and get_user(krb_principal):`
		`- raise koji.GenericError('user with this Kerberos principal already exists: %s' % krb_principal)`
		`+ if krb_principal and get_user_by_krb_principal(krb_principal):`
		`+ raise koji.GenericError(`
		`+ 'user with this Kerberos principal already exists: %s'`
		`+ % krb_principal)`
		`+`
		`+ return context.session.createUser(username, status=status,`
		`+ krb_principal=krb_principal)`

		`- return context.session.createUser(username, status=status, krb_principal=krb_principal)`
		`+ def addUserKrbPrincipal(self, user, krb_principal):`
		`+ """Add a Kerberos principal for user"""`
		`+ context.session.assertPerm('admin')`
		`+ userinfo = get_user(user, strict=True)`
		`+ if not krb_principal:`
		`+ raise koji.GenericError('krb_principal must be specified')`
		`+ if get_user_by_krb_principal(krb_principal):`
		`+ raise koji.GenericError(`
		`+ 'user with this Kerberos principal already exists: %s'`
		`+ % krb_principal)`
		`+ return context.session.setKrbPrincipal(userinfo['name'], krb_principal)`
		`+`
		`+ def removeUserKrbPrincipal(self, user, krb_principal):`
		`+ """remove a Kerberos principal for user"""`
		`+ context.session.assertPerm('admin')`
		`+ userinfo = get_user(user, strict=True)`
		`+ if not krb_principal:`
		`+ raise koji.GenericError('krb_principal must be specified')`
		`+ return context.session.removeKrbPrincipal(userinfo['name'],`
		`+ krb_principal)`

		`def enableUser(self, username):`
		`"""Enable logins by the specified user"""`
		`@@ -10922,16 +11035,28 @@`
		`- name`
		`- status`
		`- usertype`
		`- - krb_principal`
		`+ - krb_principals`

		`If no users of the specified`
		`type exist, return an empty list."""`
		`- fields = ('id', 'name', 'status', 'usertype', 'krb_principal')`
		`+ fields = ('id', 'name', 'status', 'usertype',`
		`+ 'array_remove(array_agg(krb_principal), NULL)')`
		`+ aliases = ('id', 'name', 'status', 'usertype', 'krb_principals')`
		`+ joins = ('LEFT JOIN user_krb_principals'`
		`+ ' ON users.id = user_krb_principals.user_id',)`
		`clauses = ['usertype = %(userType)i']`
		`if prefix:`
		`clauses.append("name ilike %(prefix)s \|\| '%%'")`
		`- query = QueryProcessor(columns=fields, tables=('users',), clauses=clauses,`
		`- values=locals(), opts=queryOpts)`
		`+ if queryOpts is None:`
		`+ queryOpts = {}`
		`+ if not queryOpts.get('group'):`
		`+ queryOpts['group'] = 'users.id'`
		`+ else:`
		`+ raise koji.GenericError('queryOpts.group is not available for this API')`
		`+ query = QueryProcessor(columns=fields, aliases=aliases,`
		`+ tables=('users',), joins=joins, clauses=clauses,`
		`+ values=locals(), opts=queryOpts,`
		`+ enable_group=True)`
		`return query.execute()`

		`def getBuildConfig(self, tag, event=None):`
		`@@ -11476,8 +11601,13 @@`
		`in the same format as getUser(), plus the authtype. If there is no`
		`currently logged-in user, return None."""`
		`if context.session.logged_in:`
		`- me = self.getUser(context.session.user_id)`
		`+ me = self.getUser(context.session.user_id, krb_princs=True)`
		`me['authtype'] = context.session.authtype`
		`+ # backward compatible for cli moshimoshi, but it's not real`
		`+ if me.get('krb_principals'):`
		`+ me['krb_principal'] = me['krb_principals'][0]`
		`+ else:`
		`+ me['krb_principal'] = None`
		`return me`
		`else:`
		`return None`

hub/kojixmlrpc.py

file modified

		`@@ -421,6 +421,7 @@`
		`['AuthKeytab', 'string', None],`
		`['ProxyPrincipals', 'string', ''],`
		`['HostPrincipalFormat', 'string', None],`
		`+ ['AllowedKrbRealms', 'string', '*'],`

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

koji/__init__.py

file modified

+6 -1

		`@@ -1709,6 +1709,7 @@`
		`'krbservice': 'host',`
		`'krb_rdns': True,`
		`'krb_canon_host': False,`
		`+ 'krb_server_realm': None,`
		`'principal': None,`
		`'keytab': None,`
		`'cert': None,`
		`@@ -2124,6 +2125,7 @@`
		`'upload_blocksize',`
		`'krb_rdns',`
		`'krb_canon_host',`
		`+ 'krb_server_realm',`
		`'no_ssl_verify',`
		`'serverca',`
		`)`
		`@@ -2293,6 +2295,7 @@`
		`return False`
		`self.setSession(sinfo)`

		`+ self.krb_principal = cprinc.name`
		`self.authtype = AUTHTYPE_KERB`
		`return True`

		`@@ -2302,7 +2305,9 @@`

		`host = six.moves.urllib.parse.urlparse(self.baseurl).hostname`
		`servername = self._fix_krb_host(host)`
		`- realm = cprinc.realm`
		`+ realm = self.opts.get('krb_server_realm')`
		`+ if not realm:`
		`+ realm = cprinc.realm`
		`service = self.opts.get('krbservice', 'host')`

		`ret = '%s/%s@%s' % (service, servername, realm)`

koji/auth.py

file modified

+103 -33

		`@@ -23,6 +23,7 @@`
		`import socket`
		`import string`
		`import random`
		`+ import re`
		`import base64`
		`try:`
		`import krbV`
		`@@ -339,16 +340,22 @@`
		`'Kerberos principal %s is not authorized to log in other users' % cprinc.name)`
		`else:`
		`login_principal = cprinc.name`
		`- user_id = self.getUserIdFromKerberos(login_principal)`
		`- if not user_id:`
		`+`
		`+ if '@' in login_principal:`
		`+ user_id = self.getUserIdFromKerberos(login_principal)`
		`+ else:`
		`+ # backward compatible`
		`+ # it's only possible when proxyuser is username, but we shouldn't`
		`+ # allow this.`
		`user_id = self.getUserId(login_principal)`
		`- if not user_id:`
		`- # Only do autocreate if we also couldn't find by username AND the proxyuser`
		`- # looks like a krb5 principal`
		`- if context.opts.get('LoginCreatesUser') and '@' in login_principal:`
		`- user_id = self.createUserFromKerberos(login_principal)`
		`- else:`
		`- raise koji.AuthError('Unknown Kerberos principal: %s' % login_principal)`
		`+`
		`+ if not user_id:`
		`+ # Only do autocreate if we also couldn't find by username AND the proxyuser`
		`+ # looks like a krb5 principal`
		`+ if context.opts.get('LoginCreatesUser') and '@' in login_principal:`
		`+ user_id = self.createUserFromKerberos(login_principal)`
		`+ else:`
		`+ raise koji.AuthError('Unknown Kerberos principal: %s' % login_principal)`

		`self.checkLoginAllowed(user_id)`

		`@@ -514,6 +521,7 @@`
		`c.execute(q, {})`
		`(session_id,) = c.fetchone()`

		`+`
		`#add session id to database`
		`q = """`
		`INSERT INTO sessions (id, user_id, key, hostip, authtype, master)`
		`@@ -602,8 +610,12 @@`
		`def getUserIdFromKerberos(self, krb_principal):`
		`"""Return the user ID associated with a particular Kerberos principal.`
		`If no user with the given princpal if found, return None."""`
		`+ self.checkKrbPrincipal(krb_principal)`
		`c = context.cnx.cursor()`
		`- q = """SELECT id FROM users WHERE krb_principal = %(krb_principal)s"""`
		`+ q = """SELECT id FROM users`
		`+ JOIN user_krb_principals`
		`+ ON users.id = user_krb_principals.user_id`
		`+ WHERE krb_principal = %(krb_principal)s"""`
		`c.execute(q, locals())`
		`r = c.fetchone()`
		`c.close()`
		`@@ -612,7 +624,8 @@`
		`else:`
		`return None`

		`- def createUser(self, name, usertype=None, status=None, krb_principal=None):`
		`+ def createUser(self, name, usertype=None, status=None, krb_principal=None,`
		`+ krb_princ_check=True):`
		`"""`
		`Create a new user, using the provided values.`
		`Return the user_id of the newly-created user.`
		`@@ -620,41 +633,75 @@`
		`if not name:`
		`raise koji.GenericError('a user must have a non-empty name')`

		`- if usertype == None:`
		`+ if usertype is None:`
		`usertype = koji.USERTYPES['NORMAL']`
		`elif not koji.USERTYPES.get(usertype):`
		`raise koji.GenericError('invalid user type: %s' % usertype)`

		`- if status == None:`
		`+ if status is None:`
		`status = koji.USER_STATUS['NORMAL']`
		`elif not koji.USER_STATUS.get(status):`
		`raise koji.GenericError('invalid status: %s' % status)`

		`+ # check if krb_principal is allowed`
		`+ if krb_princ_check:`
		`+ self.checkKrbPrincipal(krb_principal)`
		`+`
		`cursor = context.cnx.cursor()`
		`select = """SELECT nextval('users_id_seq')"""`
		`cursor.execute(select, locals())`
		`user_id = cursor.fetchone()[0]`

		`- insert = """INSERT INTO users (id, name, usertype, status, krb_principal)`
		`- VALUES (%(user_id)i, %(name)s, %(usertype)i, %(status)i, %(krb_principal)s)"""`
		`+ insert = """INSERT INTO users (id, name, usertype, status)`
		`+ VALUES (%(user_id)i, %(name)s, %(usertype)i, %(status)i"""`
		`cursor.execute(insert, locals())`
		`+ if krb_principal:`
		`+ insert = """INSERT INTO user_krb_principals (user_id, krb_principal)`
		`+ VALUES (%(user_id)i, %(krb_principal)s)"""`
		`+ cursor.execute(insert, locals())`
		`context.cnx.commit()`

		`return user_id`

		`- def setKrbPrincipal(self, name, krb_principal):`
		`- usertype = koji.USERTYPES['NORMAL']`
		`- status = koji.USER_STATUS['NORMAL']`
		`- update = """UPDATE users SET krb_principal = %(krb_principal)s WHERE name = %(name)s AND usertype = %(usertype)i AND status = %(status)i RETURNING users.id"""`
		`+ def setKrbPrincipal(self, name, krb_principal, krb_princ_check=True):`
		`+ if krb_princ_check:`
		`+ self.checkKrbPrincipal(krb_principal)`
		`+ select = """SELECT id FROM users WHERE name = %(name)s"""`
		`+ cursor = context.cnx.cursor()`
		`+ cursor.execute(select, locals())`
		`+ r = cursor.fetchone()`
		`+ if not r:`
		`+ context.cnx.rollback()`
		`+ raise koji.AuthError('No such user: %s' % name)`
		`+ else:`
		`+ user_id = r[0]`
		`+ insert = """INSERT INTO user_krb_principals (user_id, krb_principal)`
		`+ VALUES (%(user_id)i, %(krb_principal)s)"""`
		`+ cursor.execute(insert, locals())`
		`+ context.cnx.commit()`
		`+ return user_id`
		`+`
		`+ def removeKrbPrincipal(self, name, krb_principal):`
		`+ select = """SELECT id FROM users`
		`+ JOIN user_krb_principals`
		`+ WHERE name = %(name)s`
		`+ AND krb_principal = %(krb_principal)s"""`
		`cursor = context.cnx.cursor()`
		`- cursor.execute(update, locals())`
		`- r = cursor.fetchall()`
		`- if len(r) != 1:`
		`+ cursor.execute(select, locals())`
		`+ r = cursor.fetchone()`
		`+ if not r:`
		`context.cnx.rollback()`
		`- raise koji.AuthError('could not automatically associate Kerberos Principal with existing user %s' % name)`
		`+ raise koji.AuthError(`
		`+ 'could not automatically remove Kerberos Principal:'`
		`+ ' %(krb_principal)s with user %(name)s' % locals())`
		`else:`
		`- context.cnx.commit()`
		`- return r[0][0]`
		`+ user_id = r[0]`
		`+ delete = """DELETE FROM user_krb_principals`
		`+ WHERE user_id = (%(user_id)i`
		`+ AND krb_principal = %(krb_principal)s"""`
		`+ cursor.execute(delete, locals())`
		`+ context.cnx.commit()`
		`+ return user_id`

		`def createUserFromKerberos(self, krb_principal):`
		`"""Create a new user, based on the Kerberos principal. Their`
		`@@ -667,17 +714,40 @@`

		`# check if user already exists`
		`c = context.cnx.cursor()`
		`- q = """SELECT krb_principal FROM users`
		`- WHERE name = %(user_name)s"""`
		`+ q = """SELECT id, krb_principal FROM users`
		`+ LEFT JOIN user_krb_principals`
		`+ ON users.id = user_krb_principals.user_id`
		`+ WHERE name = %(user_name)s"""`
		`c.execute(q, locals())`
		`- r = c.fetchone()`
		`+ r = c.fetchall()`
		`if not r:`
		`- return self.createUser(user_name, krb_principal=krb_principal)`
		`+ return self.createUser(user_name, krb_principal=krb_principal,`
		`+ krb_princ_check=False)`
		`else:`
		`- existing_user_krb = r[0]`
		`- if existing_user_krb is not None:`
		`- raise koji.AuthError('user %s already associated with other Kerberos principal: %s' % (user_name, existing_user_krb))`
		`- return self.setKrbPrincipal(user_name, krb_principal)`
		`+ existing_user_krb_princs = [row[1] for row in r]`
		`+ if krb_principal in existing_user_krb_princs:`
		`+ # do not set Kerberos principal if it already exists`
		`+ return r[0][0]`
		`+ return self.setKrbPrincipal(user_name, krb_principal,`
		`+ krb_princ_check=False)`
		`+`
		`+ def checkKrbPrincipal(self, krb_principal):`
		`+ """Check if the Kerberos principal is allowed"""`
		`+ if krb_principal is None:`
		`+ return`
		`+ allowed_realms = context.opts.get('AllowedKrbRealms', '*')`
		`+ if allowed_realms == '*':`
		`+ return`
		`+ allowed_realms = re.split(r'\s,\s', allowed_realms)`
		`+ atidx = krb_principal.find('@')`
		`+ if atidx == -1 or atidx == len(krb_principal) - 1:`
		`+ raise koji.AuthError(`
		`+ 'invalid Kerberos principal: %s' % krb_principal)`
		`+ realm = krb_principal[atidx + 1:]`
		`+ if realm not in allowed_realms:`
		`+ raise koji.AuthError(`
		`+ "Kerberos principal's realm: %s is not allowed" % realm)`
		`+`

		`def get_user_groups(user_id):`
		`"""Get user groups`

tests/test_cli/test_hello.py

file modified

+1 -1

		`@@ -52,10 +52,10 @@`
		`'krb_principal': '%s@localhost' % self.progname}`
		`cert = '/etc/pki/user.cert'`
		`options = mock.MagicMock()`
		`-`
		`# Mock out the xmlrpc server`
		`session = mock.MagicMock(baseurl=self.huburl, authtype=None)`
		`session.getLoggedInUser.return_value = None`
		`+ session.krb_principal = user['krb_principal']`
		`print_unicode_mock.return_value = "Hello"`

		`expect = """Usage: %s moshimoshi [options]`

tests/test_hub/test_query_processor.py

file modified

+13 -3

		`@@ -26,8 +26,10 @@`
		`'order': 'other',`
		`'offset': 10,`
		`'limit': 3,`
		`+ 'group': 'awesome.aha'`
		`#'rowlock': True,`
		`},`
		`+ enable_group=True`
		`)`
		`self.original_chunksize = kojihub.QueryProcessor.iterchunksize`
		`kojihub.QueryProcessor.iterchunksize = 2`
		`@@ -62,7 +64,15 @@`
		`def test_complex_as_string(self):`
		`proc = kojihub.QueryProcessor(**self.complex_arguments)`
		`actual = " ".join([token for token in str(proc).split() if token])`
		`- expected = "SELECT something FROM awesome JOIN morestuff ORDER BY something OFFSET 10 LIMIT 3"`
		`+ expected = "SELECT something FROM awesome JOIN morestuff" \`
		`+ " GROUP BY awesome.aha ORDER BY something OFFSET 10 LIMIT 3"`
		`+ self.assertEqual(actual, expected)`
		`+ args2 = self.complex_arguments.copy()`
		`+ args2['enable_group'] = False`
		`+ proc = kojihub.QueryProcessor(**args2)`
		`+ actual = " ".join([token for token in str(proc).split() if token])`
		`+ expected = "SELECT something FROM awesome JOIN morestuff" \`
		`+ " ORDER BY something OFFSET 10 LIMIT 3"`
		`self.assertEqual(actual, expected)`

		`@mock.patch('kojihub.context')`
		`@@ -71,7 +81,7 @@`
		`context.cnx.cursor.return_value = cursor`
		`proc = kojihub.QueryProcessor(**self.simple_arguments)`
		`proc.execute()`
		`- cursor.execute.assert_called_once_with('\nSELECT something\n FROM awesome\n\n\n \n\n \n', {})`
		`+ cursor.execute.assert_called_once_with('\nSELECT something\n FROM awesome\n\n\n \n \n\n \n', {})`

		`@mock.patch('kojihub.context')`
		`def test_simple_count_with_execution(self, context):`
		`@@ -82,7 +92,7 @@`
		`args['opts'] = {'countOnly': True}`
		`proc = kojihub.QueryProcessor(**args)`
		`results = proc.execute()`
		`- cursor.execute.assert_called_once_with('\nSELECT count(*)\n FROM awesome\n\n\n \n\n \n', {})`
		`+ cursor.execute.assert_called_once_with('\nSELECT count(*)\n FROM awesome\n\n\n \n \n\n \n', {})`
		`self.assertEqual(results, 'some count')`

		`@mock.patch('kojihub.context')`

tests/test_lib/test_auth.py

file modified

+50 -2

		`@@ -209,16 +209,64 @@`
		`self.assertEqual(cm.exception.args[0],`
		`'Unknown Kerberos principal:'`
		`' proxyuser@realm.com')`
		`+ # case: create user by kerberos`
		`context.opts['LoginCreatesUser'] = True`
		`context.cnx.cursor.return_value. \`
		`fetchone.side_effect = [None,`
		`- None,`
		`- None,`
		`(1,),`
		`('name', 'type',`
		`koji.USER_STATUS['NORMAL']),`
		`('session-id',)]`
		`+ context.cnx.cursor.return_value.fetchall.return_value = None`
		`s.krbLogin('krb_req', 'proxyuser@realm.com')`
		`+ self.assertEqual(context.cnx.cursor.return_value.execute.`
		`+ call_count, 14)`
		`+ # case: create user by username, proxyuser is username`
		`+ context.cnx.cursor.return_value. \`
		`+ fetchone.side_effect = [None]`
		`+ context.cnx.cursor.return_value.fetchall.return_value = None`
		`+ with self.assertRaises(koji.AuthError) as cm:`
		`+ s.krbLogin('krb_req', 'proxyuser')`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ 'Unknown Kerberos principal: proxyuser')`
		`+ # case: create user by kerberos - set krb princ`
		`+ context.opts['LoginCreatesUser'] = True`
		`+ context.cnx.cursor.return_value. \`
		`+ fetchone.side_effect = [None,`
		`+ (1,),`
		`+ ('name', 'type',`
		`+ koji.USER_STATUS['NORMAL']),`
		`+ ('session-id',)]`
		`+ context.cnx.cursor.return_value.fetchall. \`
		`+ return_value = [('proxyuser', 'proxyuser@otherrealm.com')]`
		`+ s.krbLogin('krb_req', 'proxyuser@realm.com')`
		`+ self.assertEqual(context.cnx.cursor.return_value.execute.`
		`+ call_count, 22)`
		`+`
		`+ @mock.patch('koji.auth.context')`
		`+ def test_checkKrbPrincipal(self, context):`
		`+ s, cntext, cursor = self.get_session()`
		`+ self.assertIsNone(s.checkKrbPrincipal(None))`
		`+ context.opts = {'AllowedKrbRealms': '*'}`
		`+ self.assertIsNone(s.checkKrbPrincipal('any'))`
		`+ context.opts = {'AllowedKrbRealms': 'example.com'}`
		`+ with self.assertRaises(koji.AuthError) as cm:`
		`+ s.checkKrbPrincipal('any')`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ 'invalid Kerberos principal: any')`
		`+ with self.assertRaises(koji.AuthError) as cm:`
		`+ s.checkKrbPrincipal('any@')`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ 'invalid Kerberos principal: any@')`
		`+ with self.assertRaises(koji.AuthError) as cm:`
		`+ s.checkKrbPrincipal('any@bannedrealm')`
		`+ self.assertEqual(cm.exception.args[0],`
		`+ "Kerberos principal's realm:"`
		`+ " bannedrealm is not allowed")`
		`+ self.assertIsNone(s.checkKrbPrincipal('user@example.com'))`
		`+ context.opts = {'AllowedKrbRealms': 'example.com,example.net'`
		`+ ' , example.org'}`
		`+ self.assertIsNone(s.checkKrbPrincipal('user@example.net'))`

		`# functions outside Session object`

util/koji-gc

file modified

		`@@ -48,6 +48,8 @@`
		`help=_("get reverse dns FQDN for krb target"))`
		`parser.add_option("--krb-canon-host", action="store_true", default=False,`
		`help=_("get canonical hostname for krb target"))`
		`+ parser.add_option("--krb-server-realm",`
		`+ help=_("the realm of server Kerberos principal"))`
		`parser.add_option("--runas", metavar="USER",`
		`help=_("run as the specified user (requires special privileges)"))`
		`parser.add_option("--user", help=_("specify user"))`
		`@@ -131,6 +133,7 @@`
		`['krbservice', None, 'string'],`
		`['krb_rdns', None, 'boolean'],`
		`['krb_canon_host', None, 'boolean'],`
		`+ ['krb_server_realm', None, 'string'],`
		`['runas', None, 'string'],`
		`['user', None, 'string'],`
		`['password', None, 'string'],`

util/koji-gc.conf

file modified

		`@@ -19,6 +19,9 @@`
		`# The service name of the principal being used by the hub`
		`#krbservice = host`

		`+ ## The realm of server principal. Using client's realm if not set`
		`+ # krb_server_realm = EXAMPLE.COM`
		`+`
		`# The domain name that will be appended to Koji usernames`
		`# when creating email notifications`
		`#email_domain = fedoraproject.org`

util/koji-shadow

file modified

		`@@ -85,6 +85,8 @@`
		`help=_("get reverse dns FQDN for krb target"))`
		`parser.add_option("--krb-canon-host", action="store_true", default=False,`
		`help=_("get canonical hostname for krb target"))`
		`+ parser.add_option("--krb-server-realm",`
		`+ help=_("the realm of server Kerberos principal"))`
		`parser.add_option("--noauth", action="store_true", default=False,`
		`help=_("do not authenticate"))`
		`parser.add_option("-n", "--test", action="store_true", default=False,`

util/koji-shadow.conf

file modified

		`@@ -5,3 +5,6 @@`
		`server=http://localhost/kojihub/`
		`krbservice=host`
		`remote=https://koji.fedoraproject.org/kojihub`
		`+`
		`+ ## The realm of server principal. Using client's realm if not set`
		`+ # krb_server_realm = EXAMPLE.COM`

util/kojira

file modified

		`@@ -924,6 +924,7 @@`
		`'krbservice': 'host',`
		`'krb_rdns': True,`
		`'krb_canon_host': False,`
		`+ 'krb_server_realm': None,`
		`'retry_interval': 60,`
		`'max_retries': 120,`
		`'offline_retry': True,`

util/kojira.conf

file modified

		`@@ -32,6 +32,9 @@`
		`;the service name of the principal being used by the hub`
		`;krbservice = host`

		`+ ;The realm of server principal. Using client's realm if not set`
		`+ ;krb_server_realm = EXAMPLE.COM`
		`+`
		`;configuration for SSL authentication`

		`;client certificate`

vm/kojivmd

file modified

		`@@ -128,6 +128,7 @@`
		`'krbservice': 'host',`
		`'krb_rdns': True,`
		`'krb_canon_host': False,`
		`+ 'krb_server_realm': None,`
		`'server': None,`
		`'user': None,`
		`'password': None,`

vm/kojivmd.conf

file modified

		`@@ -45,6 +45,9 @@`
		`;the service name of the principal being used by the hub`
		`;krbservice = host`

		`+ ;The realm of server principal. Using client's realm if not set`
		`+ ;krb_server_realm = EXAMPLE.COM`
		`+`
		`;configuration for SSL authentication`

		`;client certificate`

www/conf/web.conf

file modified

		`@@ -12,6 +12,8 @@`
		`# WebCCache = /var/tmp/kojiweb.ccache`
		`# The service name of the principal being used by the hub`
		`# KrbService = host`
		`+ ## The realm of server principal. Using client's realm if not set`
		`+ # KrbServerRealm = EXAMPLE.COM`

		`# SSL authentication options`
		`# WebCert = /etc/kojiweb/kojiweb.crt`

www/kojiweb/index.py

file modified

		`@@ -170,6 +170,7 @@`
		`s_opts = {'krbservice': opts['KrbService'],`
		`'krb_rdns': opts['KrbRDNS'],`
		`'krb_canon_host': opts['KrbCanonHost'],`
		`+ 'krb_server_realm': opts['KrbServerRealm']`
		`}`
		`session = koji.ClientSession(opts['KojiHubURL'], opts=s_opts)`

www/kojiweb/wsgi_publisher.py

file modified

		`@@ -79,6 +79,7 @@`
		`['KrbService', 'string', 'host'],`
		`['KrbRDNS', 'boolean', True],`
		`['KrbCanonHost', 'boolean', False],`
		`+ ['KrbServerRealm', 'string', None],`

		`['WebCert', 'string', None],`
		`['KojiHubCA', 'string', '/etc/kojiweb/kojihubca.crt'],`

julian8628 commented 4 years ago

a new table user_krb_principals instead of one-on-one mapping users.krb_principal
all APIs related to user or krb principals are changed
now, userinfo of getUser won't contain krb_principal. Instead, it will contain a new list krb_principals having all available princs if krb_princs=True
a new hub option: AllowedKrbRealms to indicate which realms are allowed
a new client option krb_server_realm to allow krbV login to set server realm, which was the same as client princ realm before. This support all clients - cli, builder, web(KrbServerRealm), vmbuilder, gc, kojira, shadow
QueryProcessor has a new queryOpt group, which is used to generate GROUP BY section. By default, this feature is disabled by arg: enable_group=False
with PR #1419, GSSAPI will natively support multiple realms as well, auth_to_local mapping in /etc/krb5.conf is not necessary anymore, we could just set GssapiLocalName Off in httpd.conf

related: #1419
fixes: #1629

breilly commented 4 years ago

Would it make sense to be able to set the server realm in hub.conf or something similar?

julian8628 commented 4 years ago

Would it make sense to be able to set the server realm in hub.conf or something similar?

Do you mean let hub decide which server realm should be used in krbV way?
I think that, client doesn't know the server realm, unless having a new API to return it before aunthenticating. It looks a little complex and maybe introduce some extra time consuming. (client has to call this API, no matter if server realm is the same as the client one)

breilly commented 4 years ago

Makes sense, LGTM as is.