From bfdcb1bc4c8c1b2fdfded6db207fde38d66e26ed Mon Sep 17 00:00:00 2001 From: Yuming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 1/11] update schema with new table - user_krb_principals --- diff --git a/docs/schema-upgrade-1.18-1.19.sql b/docs/schema-upgrade-1.18-1.19.sql index 093df76..6562e09 100644 --- a/docs/schema-upgrade-1.18-1.19.sql +++ b/docs/schema-upgrade-1.18-1.19.sql @@ -65,4 +65,16 @@ insert into archivetypes (name, description, extensions) values ('vmdk-compresse -- 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; diff --git a/docs/schema.sql b/docs/schema.sql index b34daa5..c5c9dad 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -38,8 +38,13 @@ CREATE TABLE users ( 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 ( From f2cc908ca97ad745a2115bea949af4a8957dd308 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 2/11] change users and krb_principal operations --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 721835b..b56aeef 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3663,40 +3663,105 @@ def get_external_repo_list(tag_info, event=None): 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: + if user and krb_princs: + user['krb_principals'] = list_user_krb_principals(user['id']) + elif strict: raise koji.GenericError("No such user: %r" % userInfo) 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'], + opts={'asList': True}) + 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 +8001,13 @@ def get_group_members(group): 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 +8311,16 @@ class QueryProcessor(object): 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 +8353,7 @@ class QueryProcessor(object): self.opts = opts else: self.opts = {} + self.enable_group = enable_group def countOnly(self, count): self.opts['countOnly'] = count @@ -8293,6 +8365,7 @@ SELECT %(col_str)s FROM %(table_str)s %(join_str)s %(clause_str)s + %(group_str)s %(order_str)s %(offset_str)s %(limit_str)s @@ -8314,6 +8387,10 @@ SELECT %(col_str)s 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 +8456,17 @@ SELECT %(col_str)s 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 +8568,11 @@ def _applyQueryOpts(results, queryOpts): 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,7 +10975,7 @@ class RootExports(object): context.session.assertPerm('admin') if get_user(username): raise koji.GenericError('user already exists: %s' % username) - if krb_principal and get_user(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) @@ -10922,16 +11012,28 @@ class RootExports(object): - 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): diff --git a/koji/auth.py b/koji/auth.py index 560019b..3ceadc8 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -603,7 +603,10 @@ class Session(object): """Return the user ID associated with a particular Kerberos principal. If no user with the given princpal if found, return None.""" c = context.cnx.cursor() - q = """SELECT id FROM users WHERE krb_principal = %(krb_principal)s""" + q = """SELECT id FROM users + JOIN users_krb_principals + ON users.id = users_krb_principals.user_id + WHERE krb_principal = %(krb_principal)s""" c.execute(q, locals()) r = c.fetchone() c.close() @@ -620,12 +623,12 @@ class Session(object): 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) @@ -635,26 +638,54 @@ class Session(object): 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""" + select = """SELECT id FROM users WHERE name = %(name)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('No such user: %s' % name) else: - context.cnx.commit() - return r[0][0] + 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(select, locals()) + r = cursor.fetchone() + if not r: + context.cnx.rollback() + raise koji.AuthError( + 'could not automatically remove Kerberos Principal:' + ' %(krb_principal)s with user %(name)s' % locals()) + else: + 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,16 +698,19 @@ class Session(object): # 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) 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)) + 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) def get_user_groups(user_id): From 27bb3aef5c509acfb7551f05d0862a47174f26fc Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 3/11] hub: add AllowedKrbRealms option and (add|remove)UserKrbPrincipal API --- diff --git a/hub/hub.conf b/hub/hub.conf index 16b6192..452d023 100644 --- a/hub/hub.conf +++ b/hub/hub.conf @@ -22,6 +22,9 @@ KojiDir = /mnt/koji # 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 diff --git a/hub/kojihub.py b/hub/kojihub.py index b56aeef..5fbb61f 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -10976,9 +10976,33 @@ class RootExports(object): if get_user(username): raise koji.GenericError('user already exists: %s' % username) if krb_principal and get_user_by_krb_principal(krb_principal): - raise koji.GenericError('user with this Kerberos principal already exists: %s' % 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) + + 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) - return context.session.createUser(username, status=status, krb_principal=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""" diff --git a/hub/kojixmlrpc.py b/hub/kojixmlrpc.py index 18b11ec..9f2dd3c 100644 --- a/hub/kojixmlrpc.py +++ b/hub/kojixmlrpc.py @@ -421,6 +421,7 @@ def load_config(environ): ['AuthKeytab', 'string', None], ['ProxyPrincipals', 'string', ''], ['HostPrincipalFormat', 'string', None], + ['AllowedKrbRealms', 'string', '*'], ['DNUsernameComponent', 'string', 'CN'], ['ProxyDNs', 'string', ''], diff --git a/koji/auth.py b/koji/auth.py index 3ceadc8..c003dfd 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -23,6 +23,7 @@ from __future__ import absolute_import import socket import string import random +import re import base64 try: import krbV @@ -339,16 +340,22 @@ class Session(object): '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) @@ -602,6 +609,7 @@ class Session(object): 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 JOIN users_krb_principals @@ -615,7 +623,8 @@ class Session(object): 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. @@ -633,6 +642,10 @@ class Session(object): 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()) @@ -649,7 +662,9 @@ class Session(object): return user_id - def setKrbPrincipal(self, name, krb_principal): + 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()) @@ -705,13 +720,33 @@ class Session(object): c.execute(q, locals()) 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_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) + 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('\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 From 4df721a853bd334d0721a92951e7f4e0518045cc Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 4/11] fix table name typo --- diff --git a/koji/auth.py b/koji/auth.py index c003dfd..dae34ce 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -612,8 +612,8 @@ class Session(object): self.checkKrbPrincipal(krb_principal) c = context.cnx.cursor() q = """SELECT id FROM users - JOIN users_krb_principals - ON users.id = users_krb_principals.user_id + 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() From 8cee4e66aa3c97bffff5e2dd0e9062968c784a24 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 5/11] cli: fix moshimoshi for krbLogin --- diff --git a/cli/koji_cli/commands.py b/cli/koji_cli/commands.py index 43b6f6a..849a6d8 100644 --- a/cli/koji_cli/commands.py +++ b/cli/koji_cli/commands.py @@ -7332,7 +7332,7 @@ def handle_moshimoshi(options, session, args): 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) diff --git a/koji/__init__.py b/koji/__init__.py index 2e9399d..b8da258 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2293,6 +2293,7 @@ class ClientSession(object): return False self.setSession(sinfo) + self.krb_principal = cprinc.name self.authtype = AUTHTYPE_KERB return True From 5f7cb173270e8d7e3943f58b559463cc33a495e4 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 6/11] hub: backward compatible for cli moshimoshi command --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 5fbb61f..046e6f8 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -11602,8 +11602,13 @@ class RootExports(object): 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 From e7a91f3d79fa128e40459cff4db6bfebc787d7b5 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 7/11] hub: remove asList in queryOpts of list_user_krb_principals --- diff --git a/hub/kojihub.py b/hub/kojihub.py index 046e6f8..fd72218 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3727,8 +3727,7 @@ def list_user_krb_principals(user_info=None): query = QueryProcessor(tables=['user_krb_principals'], columns=fields, joins=joins, clauses=clauses, values=data, - transform=lambda row: row['krb_principal'], - opts={'asList': True}) + transform=lambda row: row['krb_principal']) return query.execute() or [] From a17bb68d8f24fd0d39104ed94b6b38b793a502ed Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 8/11] add an option to set server realm for all clients --- diff --git a/builder/kojid b/builder/kojid index 8a308d0..08d0a99 100755 --- a/builder/kojid +++ b/builder/kojid @@ -6131,6 +6131,7 @@ def get_options(): 'krbservice': 'host', 'krb_rdns': True, 'krb_canon_host': False, + 'krb_server_realm': None, 'server': None, 'user': None, 'password': None, diff --git a/cli/koji.conf b/cli/koji.conf index 0a27810..96e7bf4 100644 --- a/cli/koji.conf +++ b/cli/koji.conf @@ -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 diff --git a/koji/__init__.py b/koji/__init__.py index b8da258..c591247 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -1709,6 +1709,7 @@ def read_config(profile_name, user_config=None): 'krbservice': 'host', 'krb_rdns': True, 'krb_canon_host': False, + 'krb_server_realm': None, 'principal': None, 'keytab': None, 'cert': None, @@ -2124,6 +2125,7 @@ def grab_session_options(options): 'upload_blocksize', 'krb_rdns', 'krb_canon_host', + 'krb_server_realm', 'no_ssl_verify', 'serverca', ) @@ -2303,7 +2305,9 @@ class ClientSession(object): 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) diff --git a/util/koji-gc b/util/koji-gc index 02a29f1..29e250a 100755 --- a/util/koji-gc +++ b/util/koji-gc @@ -48,6 +48,8 @@ def get_options(): 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 @@ def get_options(): ['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'], diff --git a/util/koji-gc.conf b/util/koji-gc.conf index dea8880..a657c20 100644 --- a/util/koji-gc.conf +++ b/util/koji-gc.conf @@ -19,6 +19,9 @@ weburl = https://koji.fedoraproject.org/koji # 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 diff --git a/util/koji-shadow b/util/koji-shadow index c0924fc..c603c50 100755 --- a/util/koji-shadow +++ b/util/koji-shadow @@ -85,6 +85,8 @@ def get_options(): 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, diff --git a/util/koji-shadow.conf b/util/koji-shadow.conf index d31080c..daf3bac 100644 --- a/util/koji-shadow.conf +++ b/util/koji-shadow.conf @@ -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 diff --git a/util/kojira b/util/kojira index 69fbf8c..cb85eef 100755 --- a/util/kojira +++ b/util/kojira @@ -924,6 +924,7 @@ def get_options(): 'krbservice': 'host', 'krb_rdns': True, 'krb_canon_host': False, + 'krb_server_realm': None, 'retry_interval': 60, 'max_retries': 120, 'offline_retry': True, diff --git a/util/kojira.conf b/util/kojira.conf index 6cf509e..bf5527e 100644 --- a/util/kojira.conf +++ b/util/kojira.conf @@ -32,6 +32,9 @@ with_src=no ;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 diff --git a/vm/kojivmd b/vm/kojivmd index 6594ca8..f9f3daa 100755 --- a/vm/kojivmd +++ b/vm/kojivmd @@ -128,6 +128,7 @@ def get_options(): 'krbservice': 'host', 'krb_rdns': True, 'krb_canon_host': False, + 'krb_server_realm': None, 'server': None, 'user': None, 'password': None, diff --git a/vm/kojivmd.conf b/vm/kojivmd.conf index 2b431b2..802b19a 100644 --- a/vm/kojivmd.conf +++ b/vm/kojivmd.conf @@ -45,6 +45,9 @@ from_addr=Koji Build System ;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 diff --git a/www/conf/web.conf b/www/conf/web.conf index db73a1e..cd176b0 100644 --- a/www/conf/web.conf +++ b/www/conf/web.conf @@ -12,6 +12,8 @@ KojiFilesURL = http://server.example.com/kojifiles # 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 diff --git a/www/kojiweb/index.py b/www/kojiweb/index.py index ebee2ea..e5ec5d8 100644 --- a/www/kojiweb/index.py +++ b/www/kojiweb/index.py @@ -170,6 +170,7 @@ def _getServer(environ): 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) diff --git a/www/kojiweb/wsgi_publisher.py b/www/kojiweb/wsgi_publisher.py index abb3424..7c1782f 100644 --- a/www/kojiweb/wsgi_publisher.py +++ b/www/kojiweb/wsgi_publisher.py @@ -79,6 +79,7 @@ class Dispatcher(object): ['KrbService', 'string', 'host'], ['KrbRDNS', 'boolean', True], ['KrbCanonHost', 'boolean', False], + ['KrbServerRealm', 'string', None], ['WebCert', 'string', None], ['KojiHubCA', 'string', '/etc/kojiweb/kojihubca.crt'], From 9b0f0e6ccd46f6a30a9572767af005024d87a2d4 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 9/11] hub: fix condition mistake in get_user --- diff --git a/hub/kojihub.py b/hub/kojihub.py index fd72218..2e7fff1 100644 --- a/hub/kojihub.py +++ b/hub/kojihub.py @@ -3695,10 +3695,10 @@ def get_user(userInfo=None, strict=False, krb_princs=False): ' 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']) - elif strict: - raise koji.GenericError("No such user: %r" % userInfo) return user From ad5905cac506a17d34f97961ed9d6e68bd94eaa0 Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 10/11] fix error msg and regex sytax --- diff --git a/koji/auth.py b/koji/auth.py index dae34ce..d281319 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -521,6 +521,7 @@ class Session(object): c.execute(q, {}) (session_id,) = c.fetchone() + #add session id to database q = """ INSERT INTO sessions (id, user_id, key, hostip, authtype, master) @@ -737,7 +738,7 @@ class Session(object): allowed_realms = context.opts.get('AllowedKrbRealms', '*') if allowed_realms == '*': return - allowed_realms = re.split('\s*,\s*', allowed_realms) + 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( @@ -745,7 +746,7 @@ class Session(object): realm = krb_principal[atidx + 1:] if realm not in allowed_realms: raise koji.AuthError( - "Kerberos principal's realm : %s is not allowed" % realm) + "Kerberos principal's realm: %s is not allowed" % realm) def get_user_groups(user_id): From c1b42adad73b10494c8d0d680738a671c84d4daf Mon Sep 17 00:00:00 2001 From: Yu Ming Zhu Date: Oct 10 2019 12:44:41 +0000 Subject: [PATCH 11/11] fix unittests --- diff --git a/tests/test_cli/test_hello.py b/tests/test_cli/test_hello.py index f9cbf19..a5638a1 100644 --- a/tests/test_cli/test_hello.py +++ b/tests/test_cli/test_hello.py @@ -52,10 +52,10 @@ class TestHello(utils.CliTestCase): '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] diff --git a/tests/test_hub/test_query_processor.py b/tests/test_hub/test_query_processor.py index b807d21..64eb5bb 100644 --- a/tests/test_hub/test_query_processor.py +++ b/tests/test_hub/test_query_processor.py @@ -26,8 +26,10 @@ class TestQueryProcessor(unittest.TestCase): '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 @@ class TestQueryProcessor(unittest.TestCase): 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 @@ class TestQueryProcessor(unittest.TestCase): 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 @@ class TestQueryProcessor(unittest.TestCase): 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') diff --git a/tests/test_lib/test_auth.py b/tests/test_lib/test_auth.py index 0791017..7aa27d4 100644 --- a/tests/test_lib/test_auth.py +++ b/tests/test_lib/test_auth.py @@ -209,16 +209,64 @@ class TestAuthSession(unittest.TestCase): 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