From 2c347b4b77be90dbfa424e2f275fbe9525decb65 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Dec 12 2022 11:44:12 +0000 Subject: [PATCH 1/9] Create new session when old session was timeout Fixes: https://pagure.io/koji/issue/3394 --- diff --git a/koji/__init__.py b/koji/__init__.py index e1422d9..db92f49 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2406,6 +2406,9 @@ def grab_session_options(options): 'upload_blocksize', 'no_ssl_verify', 'serverca', + 'keytab', + 'principal', + 'ccache', ) # cert is omitted for now if isinstance(options, dict): @@ -2442,6 +2445,8 @@ class ClientSession(object): self.rsession = None self.new_session() self.opts.setdefault('timeout', DEFAULT_REQUEST_TIMEOUT) + self.exclusive = False + self.hostip = None @property def multicall(self): @@ -2473,13 +2478,16 @@ class ClientSession(object): self.callnum = None # do we need to do anything else here? self.authtype = None + self.session_key = None else: self.logged_in = True self.callnum = 0 + self.session_key = sinfo['session-key'] self.sinfo = sinfo def login(self, opts=None): - sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], opts) + sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], + self.opts['session_key'], opts) if not sinfo: return False self.setSession(sinfo) @@ -2492,7 +2500,7 @@ class ClientSession(object): return type(self)(self.baseurl, self.opts, sinfo) def gssapi_login(self, principal=None, keytab=None, ccache=None, - proxyuser=None, proxyauthtype=None): + proxyuser=None, proxyauthtype=None, session_key=None): if not reqgssapi: raise PythonImportError( "Please install python-requests-gssapi to use GSSAPI." @@ -2537,7 +2545,7 @@ class ClientSession(object): # will fail with a handshake failure, which is retried by default. # For this case we're now using retry=False and test errors for # this exact usecase. - kwargs = {'proxyuser': proxyuser} + kwargs = {'proxyuser': proxyuser, 'session_key': session_key} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype for tries in range(self.opts.get('max_retries', 30)): @@ -2585,7 +2593,8 @@ class ClientSession(object): self.authtype = AUTHTYPES['GSSAPI'] return True - def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None): + def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None, + session_key=None): cert = cert or self.opts.get('cert') serverca = serverca or self.opts.get('serverca') if cert is None: @@ -2614,7 +2623,7 @@ class ClientSession(object): self.opts['serverca'] = serverca e_str = None try: - kwargs = {'proxyuser': proxyuser} + kwargs = {'proxyuser': proxyuser, 'session_key': session_key} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype sinfo = self._callMethod('sslLogin', [], kwargs) @@ -2831,6 +2840,32 @@ class ClientSession(object): result = result[0] return result + def _renew_session(self): + session_key = self.session_key + self.setSession(None) + if self.authtype == 'SSL' or \ + (self.opts.get('cert') and os.path.isfile(self.opts['cert'])): + self.ssl_login(cert=self.opts['cert'], + serverca=self.opts['serverca'], + session_key=session_key) + elif self.authtype == 'NORMAL' or self.opts.get('user'): + self.login(user=self.opts['user'], password=self.opts['password'], + session_key=session_key) + elif self.authtype in ['KERBEROS', 'GSSAPI'] or \ + self.opts.get('krb_principal'): + authtype = self.authtype or AUTHTYPES['GSSAPI'] + principal = self.opts.get('principal') + keytab = self.opts.get('keytab') + ccache = self.opts.get('ccache') + if authtype == 'KERBEROS': + self.krb_login(principal=principal, keytab=keytab, + ccache=ccache, session_key=session_key) + elif authtype == 'GSSAPI': + self.gssapi_login(self, principal=principal, keytab=keytab, + ccache=ccache, session_key=session_key) + if self.exclusive: + self.exclusiveSession() + def _callMethod(self, name, args, kwargs=None, retry=True): """Make a call to the hub with retries and other niceties""" @@ -2869,7 +2904,16 @@ class ClientSession(object): # server correctly reporting an outage tries = 0 continue - raise err + elif isinstance(err, AuthExpired): + if self.logged_in: + self._renew_session() + return self._callMethod(name, args, kwargs, retry) + else: + raise AuthError("Session ID %s is unlogged and expired." % + self.sinfo['session-id']) + else: + raise err + except (SystemExit, KeyboardInterrupt): # (depending on the python version, these may or may not be subclasses of # Exception) @@ -3181,6 +3225,11 @@ class ClientSession(object): result = self.callMethod('downloadTaskOutput', taskID, fileName, **dlopts) return base64.b64decode(result) + def exclusiveSession(self, force=False): + """Make this session exclusive""" + self._callMethod('exclusiveSession', {'force': force}) + self.exclusive = True + class MultiCallHack(object): """Workaround of a terribly overloaded namespace diff --git a/koji/auth.py b/koji/auth.py index c9a4aee..5486f05 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -278,7 +278,7 @@ class Session(object): if result['status'] != koji.USER_STATUS['NORMAL']: raise koji.AuthError('logins by %s are not allowed' % result['name']) - def login(self, user, password, opts=None): + def login(self, user, password, session_key=None, opts=None): """create a login session""" if opts is None: opts = {} @@ -299,7 +299,8 @@ class Session(object): self.checkLoginAllowed(user_id) # create session and return - sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL']) + sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], + session_key=session_key) context.cnx.commit() return sinfo @@ -324,7 +325,7 @@ class Session(object): return (local_ip, local_port, remote_ip, remote_port) - def sslLogin(self, proxyuser=None, proxyauthtype=None): + def sslLogin(self, proxyuser=None, proxyauthtype=None, session_key=None): """Login into brew via SSL. proxyuser name can be specified and if it is allowed in the configuration file then connection is allowed to login as @@ -410,7 +411,7 @@ class Session(object): hostip = self.get_remote_ip() - sinfo = self.createSession(user_id, hostip, authtype) + sinfo = self.createSession(user_id, hostip, authtype, session_key=session_key) return sinfo def makeExclusive(self, force=False): @@ -489,12 +490,22 @@ class Session(object): update.execute() context.cnx.commit() - def createSession(self, user_id, hostip, authtype, master=None): + def createSession(self, user_id, hostip, authtype, master=None, session_key=None): """Create a new session for the given user. Return a map containing the session-id and session-key. If master is specified, create a subsession """ + if session_key: + query = QueryProcessor(tables=['sessions'], columns=['master'], + clauses=['key=%(session_key)d'], + values={'session_key': session_key}) + row = query.executeOne(strict=False) + if not row: + raise koji.GenericError("Don't allow to renew subsession, " + "subsession doesn't exist.") + master = row['master'] + # generate a random key alnum = string.ascii_letters + string.digits key = "%s-%s" % (user_id, diff --git a/tests/test_lib/test_gssapi.py b/tests/test_lib/test_gssapi.py index 6249221..1c7d918 100644 --- a/tests/test_lib/test_gssapi.py +++ b/tests/test_lib/test_gssapi.py @@ -27,7 +27,7 @@ class TestGSSAPI(unittest.TestCase): old_environ = dict(**os.environ) self.session.gssapi_login() self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None}, retry=False) + 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) @mock.patch('koji.reqgssapi.HTTPKerberosAuth') @@ -47,7 +47,7 @@ class TestGSSAPI(unittest.TestCase): koji.reqgssapi.__version__ = accepted_version rv = self.session.gssapi_login(principal, keytab, ccache) self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None}, retry=False) + 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) self.assertTrue(rv) self.session._callMethod.reset_mock() @@ -84,7 +84,7 @@ class TestGSSAPI(unittest.TestCase): with self.assertRaises(koji.GSSAPIAuthError): self.session.gssapi_login() self.session._callMethod.assert_called_with( - 'sslLogin', [], {'proxyuser': None}, retry=False) + 'sslLogin', [], {'proxyuser': None, 'session_key': None}, retry=False) self.assertEqual(old_environ, dict(**os.environ)) def test_gssapi_login_http(self): From 4f1bf08455698ccf7431a7cb9f0de5567376eb9d Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Dec 12 2022 11:44:14 +0000 Subject: [PATCH 2/9] store original auth method --- diff --git a/koji/__init__.py b/koji/__init__.py index db92f49..9f2e65e 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2406,9 +2406,6 @@ def grab_session_options(options): 'upload_blocksize', 'no_ssl_verify', 'serverca', - 'keytab', - 'principal', - 'ccache', ) # cert is omitted for now if isinstance(options, dict): @@ -2425,7 +2422,13 @@ def grab_session_options(options): class ClientSession(object): - def __init__(self, baseurl, opts=None, sinfo=None): + def __init__(self, baseurl, opts=None, sinfo=None, auth_method=None): + """ + :param baseurl str: hub url + :param dict opts: dictionary with content varying according to authentication method + :param dict sinfo: session info returned by login method + :param dict auth_method: method for reauthentication, shouldn't be ever set manually + """ assert baseurl, "baseurl argument must not be empty" if opts is None: opts = {} @@ -2446,7 +2449,7 @@ class ClientSession(object): self.new_session() self.opts.setdefault('timeout', DEFAULT_REQUEST_TIMEOUT) self.exclusive = False - self.hostip = None + self.auth_method = auth_method @property def multicall(self): @@ -2485,9 +2488,18 @@ class ClientSession(object): self.session_key = sinfo['session-key'] self.sinfo = sinfo - def login(self, opts=None): + def login(self, opts=None, session_key=None): + """ + Username/password based login method + + :param dict opts: dict used by hub "login" call, currently can + contain only "host_ip" key. + :returns bool True: success or raises exception + """ + # store calling parameters + self.auth_method = {'method': 'login', 'kwargs': {'opts': opts}} sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], - self.opts['session_key'], opts) + opts=opts, session_key=session_key) if not sinfo: return False self.setSession(sinfo) @@ -2497,14 +2509,33 @@ class ClientSession(object): def subsession(self): "Create a subsession" sinfo = self.callMethod('subsession') - return type(self)(self.baseurl, self.opts, sinfo) + return type(self)(self.baseurl, opts=self.opts, sinfo=sinfo, auth_method=self.auth_method) def gssapi_login(self, principal=None, keytab=None, ccache=None, proxyuser=None, proxyauthtype=None, session_key=None): + """ + GSSAPI/Kerberos login method + + :param str principal: Kerberos principal + :param str keytab: path to keytab file + :param str ccache: path to ccache file/dir + :param str proxyuser: name of proxied user (e.g. forwarding by web ui) + :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) + :param str session_key: used for session renewal + :returns bool True: success or raises exception + """ if not reqgssapi: raise PythonImportError( "Please install python-requests-gssapi to use GSSAPI." ) + # store calling parameters + self.auth_method = { + 'method': 'gssapi_login', + 'kwargs': { + 'principal': principal, 'keytab': keytab, 'ccache': ccache, 'proxyuser': proxyuser, + 'proxyauthtype': proxyauthtype, 'session_key': session_key + } + } # force https old_baseurl = self.baseurl uri = six.moves.urllib.parse.urlsplit(self.baseurl) @@ -2595,6 +2626,26 @@ class ClientSession(object): def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None, session_key=None): + """ + SSL cert based login + + :param str cert: path to SSL certificate + :param str ca: deprecated, not used anymore + :param str serverca: path for CA public cert, otherwise system-wide CAs are used + :param str proxyuser: name of proxied user (e.g. forwarding by web ui) + :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) + :param str session_key: used for session renewal + :returns bool: success + """ + # store calling parameters + self.logger.error("ssl_login---------------") + self.auth_method = { + 'method': 'ssl_login', + 'kwargs': { + 'cert': cert, 'ca': ca, 'serverca': serverca, 'proxyuser': proxyuser, + 'proxyauthtype': proxyauthtype, 'session_key': session_key, + } + } cert = cert or self.opts.get('cert') serverca = serverca or self.opts.get('serverca') if cert is None: @@ -2841,28 +2892,15 @@ class ClientSession(object): return result def _renew_session(self): - session_key = self.session_key + if not hasattr(self, 'auth_method'): + raise GenericError("Missing info for reauthentication") + # will be deleted by setSession + auth_method = self.auth_method['method'] + args = self.auth_method.get('args', []) + kwargs = self.auth_method.get('kwargs', {}) + kwargs['session_key'] = self.session_key self.setSession(None) - if self.authtype == 'SSL' or \ - (self.opts.get('cert') and os.path.isfile(self.opts['cert'])): - self.ssl_login(cert=self.opts['cert'], - serverca=self.opts['serverca'], - session_key=session_key) - elif self.authtype == 'NORMAL' or self.opts.get('user'): - self.login(user=self.opts['user'], password=self.opts['password'], - session_key=session_key) - elif self.authtype in ['KERBEROS', 'GSSAPI'] or \ - self.opts.get('krb_principal'): - authtype = self.authtype or AUTHTYPES['GSSAPI'] - principal = self.opts.get('principal') - keytab = self.opts.get('keytab') - ccache = self.opts.get('ccache') - if authtype == 'KERBEROS': - self.krb_login(principal=principal, keytab=keytab, - ccache=ccache, session_key=session_key) - elif authtype == 'GSSAPI': - self.gssapi_login(self, principal=principal, keytab=keytab, - ccache=ccache, session_key=session_key) + auth_method(*args, **kwargs) if self.exclusive: self.exclusiveSession() diff --git a/koji/auth.py b/koji/auth.py index 5486f05..412d03a 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -278,7 +278,7 @@ class Session(object): if result['status'] != koji.USER_STATUS['NORMAL']: raise koji.AuthError('logins by %s are not allowed' % result['name']) - def login(self, user, password, session_key=None, opts=None): + def login(self, user, password, opts=None, session_key=None): """create a login session""" if opts is None: opts = {} @@ -497,14 +497,14 @@ class Session(object): If master is specified, create a subsession """ if session_key: + if master: + raise koji.GenericError("Can't call createSession with both master + session_key.") query = QueryProcessor(tables=['sessions'], columns=['master'], clauses=['key=%(session_key)d'], values={'session_key': session_key}) - row = query.executeOne(strict=False) - if not row: - raise koji.GenericError("Don't allow to renew subsession, " - "subsession doesn't exist.") - master = row['master'] + master = query.singleValue(strict=False) + if not master: + raise koji.GenericError("Don't allow to renew non-existent subsession") # generate a random key alnum = string.ascii_letters + string.digits From 06d98adf074730a6c65065f719ea6f7b16f7b883 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Dec 12 2022 11:44:14 +0000 Subject: [PATCH 3/9] Fix call auth_method --- diff --git a/koji/__init__.py b/koji/__init__.py index 9f2e65e..816050a 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2895,7 +2895,7 @@ class ClientSession(object): if not hasattr(self, 'auth_method'): raise GenericError("Missing info for reauthentication") # will be deleted by setSession - auth_method = self.auth_method['method'] + auth_method = getattr(self, self.auth_method['method']) args = self.auth_method.get('args', []) kwargs = self.auth_method.get('kwargs', {}) kwargs['session_key'] = self.session_key diff --git a/koji/auth.py b/koji/auth.py index 412d03a..649c25e 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -502,9 +502,10 @@ class Session(object): query = QueryProcessor(tables=['sessions'], columns=['master'], clauses=['key=%(session_key)d'], values={'session_key': session_key}) - master = query.singleValue(strict=False) - if not master: + row = query.executeOne(strict=False) + if not row: raise koji.GenericError("Don't allow to renew non-existent subsession") + master = row['master'] # generate a random key alnum = string.ascii_letters + string.digits From fd96b4073ec2f48c8c15ef213139364afe3769a1 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Dec 12 2022 11:44:51 +0000 Subject: [PATCH 4/9] Add closed column to session table and use it in session --- diff --git a/docs/schema-update-1.31-1.32.sql b/docs/schema-update-1.31-1.32.sql index 2c25886..e8b242f 100644 --- a/docs/schema-update-1.31-1.32.sql +++ b/docs/schema-update-1.31-1.32.sql @@ -2,8 +2,9 @@ -- from version 1.31 to 1.32 BEGIN; - -- fix duplicate extension in archivetypes UPDATE archivetypes SET extensions = 'vhdx.gz vhdx.xz' WHERE name = 'vhdx-compressed'; + -- for tag if session is closed or not + ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT 'false'; COMMIT; diff --git a/docs/schema.sql b/docs/schema.sql index 39b7893..74fde66 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -119,6 +119,7 @@ CREATE TABLE sessions ( start_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), update_time TIMESTAMPTZ NOT NULL DEFAULT NOW(), exclusive BOOLEAN CHECK (exclusive), + closed BOOLEAN NOT NULL DEFAULT FALSE, CONSTRAINT no_exclusive_subsessions CHECK ( master IS NULL OR "exclusive" IS NULL), CONSTRAINT exclusive_expired_sane CHECK ( diff --git a/koji/auth.py b/koji/auth.py index 649c25e..e42fe62 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -471,7 +471,8 @@ class Session(object): ses_id = session_id else: ses_id = self.id - update = UpdateProcessor('sessions', data={'expired': True, 'exclusive': None}, + update = UpdateProcessor('sessions', + data={'expired': True, 'exclusive': None, 'closed': True}, clauses=['id = %(id)i OR master = %(id)i'], values={'id': ses_id}) update.execute() @@ -500,11 +501,11 @@ class Session(object): if master: raise koji.GenericError("Can't call createSession with both master + session_key.") query = QueryProcessor(tables=['sessions'], columns=['master'], - clauses=['key=%(session_key)d'], + clauses=['key=%(session_key)d', 'closed=FALSE'], values={'session_key': session_key}) row = query.executeOne(strict=False) if not row: - raise koji.GenericError("Don't allow to renew non-existent subsession") + raise koji.GenericError("Don't allow to renew non-existent or logged out session") master = row['master'] # generate a random key From e02d0d1d580b4b7fc3a2449d59253462229c52f8 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Dec 12 2022 11:44:55 +0000 Subject: [PATCH 5/9] Add decorator for renew expired session --- diff --git a/koji/__init__.py b/koji/__init__.py index 816050a..a326a5c 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2686,6 +2686,7 @@ class ClientSession(object): sinfo = None finally: self.opts = old_opts + if not sinfo: err = 'unable to obtain a session' if e_str: @@ -2892,6 +2893,7 @@ class ClientSession(object): return result def _renew_session(self): + """Renew expirated session or subsession.""" if not hasattr(self, 'auth_method'): raise GenericError("Missing info for reauthentication") # will be deleted by setSession @@ -2904,9 +2906,19 @@ class ClientSession(object): if self.exclusive: self.exclusiveSession() + def renew_expired_session(func): + """Decorator to renew expirated session or subsession.""" + def _renew_expired_session(*args, **kwargs): + try: + return func(*args, **kwargs) + except AuthExpired: + args[0]._renew_session() + return func(*args, **kwargs) + return _renew_expired_session + + @renew_expired_session def _callMethod(self, name, args, kwargs=None, retry=True): """Make a call to the hub with retries and other niceties""" - if self.multicall: if kwargs is None: kwargs = {} @@ -2942,13 +2954,6 @@ class ClientSession(object): # server correctly reporting an outage tries = 0 continue - elif isinstance(err, AuthExpired): - if self.logged_in: - self._renew_session() - return self._callMethod(name, args, kwargs, retry) - else: - raise AuthError("Session ID %s is unlogged and expired." % - self.sinfo['session-id']) else: raise err From 08c15e96d08c0af8e2ab67e3030d6c1a3de7b829 Mon Sep 17 00:00:00 2001 From: Jana Cupova Date: Dec 12 2022 11:44:55 +0000 Subject: [PATCH 6/9] Fix unit tests --- diff --git a/tests/test_lib/test_auth.py b/tests/test_lib/test_auth.py index 3568107..963d068 100644 --- a/tests/test_lib/test_auth.py +++ b/tests/test_lib/test_auth.py @@ -434,7 +434,7 @@ class TestAuthSession(unittest.TestCase): self.assertEqual(update.table, 'sessions') self.assertEqual(update.values, {'id': 123, 'id': 123}) self.assertEqual(update.clauses, ['id = %(id)i OR master = %(id)i']) - self.assertEqual(update.data, {'expired': True, 'exclusive': None}) + self.assertEqual(update.data, {'closed': True, 'expired': True, 'exclusive': None}) self.assertEqual(update.rawdata, {}) def test_logoutChild_not_logged(self): @@ -668,15 +668,6 @@ class TestAuthSession(unittest.TestCase): self.assertEqual(query.clauses, ['active = TRUE', 'user_id=%(user_id)s']) self.assertEqual(query.columns, ['name']) - def test_logout_not_logged(self): - s, cntext = self.get_session() - - # not logged - s.logged_in = False - with self.assertRaises(koji.AuthError) as ex: - s.logout() - self.assertEqual("Not logged in", str(ex.exception)) - @mock.patch('koji.auth.context') def test_logout_logged_not_owner(self, context): s, cntext = self.get_session() From c60eb7cc0c72c268f69c75357b39dd057d9abe99 Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Dec 12 2022 11:45:40 +0000 Subject: [PATCH 7/9] remove passing session-id --- diff --git a/docs/schema-update-1.31-1.32.sql b/docs/schema-update-1.31-1.32.sql index e8b242f..55a45e4 100644 --- a/docs/schema-update-1.31-1.32.sql +++ b/docs/schema-update-1.31-1.32.sql @@ -6,5 +6,6 @@ BEGIN; UPDATE archivetypes SET extensions = 'vhdx.gz vhdx.xz' WHERE name = 'vhdx-compressed'; -- for tag if session is closed or not - ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT 'false'; + ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT FALSE; + ALTER TABLE sessions ADD CONSTRAINT no_closed_exclusive CHECK (closed IS FALSE OR "exclusive" IS NULL); COMMIT; diff --git a/koji/__init__.py b/koji/__init__.py index a326a5c..546efd7 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2481,14 +2481,12 @@ class ClientSession(object): self.callnum = None # do we need to do anything else here? self.authtype = None - self.session_key = None else: self.logged_in = True self.callnum = 0 - self.session_key = sinfo['session-key'] self.sinfo = sinfo - def login(self, opts=None, session_key=None): + def login(self, opts=None, renew=False): """ Username/password based login method @@ -2498,8 +2496,8 @@ class ClientSession(object): """ # store calling parameters self.auth_method = {'method': 'login', 'kwargs': {'opts': opts}} - sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], - opts=opts, session_key=session_key) + sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], opts=opts, + renew=renew) if not sinfo: return False self.setSession(sinfo) @@ -2512,7 +2510,7 @@ class ClientSession(object): return type(self)(self.baseurl, opts=self.opts, sinfo=sinfo, auth_method=self.auth_method) def gssapi_login(self, principal=None, keytab=None, ccache=None, - proxyuser=None, proxyauthtype=None, session_key=None): + proxyuser=None, proxyauthtype=None, renew=False): """ GSSAPI/Kerberos login method @@ -2521,7 +2519,6 @@ class ClientSession(object): :param str ccache: path to ccache file/dir :param str proxyuser: name of proxied user (e.g. forwarding by web ui) :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) - :param str session_key: used for session renewal :returns bool True: success or raises exception """ if not reqgssapi: @@ -2533,7 +2530,7 @@ class ClientSession(object): 'method': 'gssapi_login', 'kwargs': { 'principal': principal, 'keytab': keytab, 'ccache': ccache, 'proxyuser': proxyuser, - 'proxyauthtype': proxyauthtype, 'session_key': session_key + 'proxyauthtype': proxyauthtype } } # force https @@ -2576,7 +2573,7 @@ class ClientSession(object): # will fail with a handshake failure, which is retried by default. # For this case we're now using retry=False and test errors for # this exact usecase. - kwargs = {'proxyuser': proxyuser, 'session_key': session_key} + kwargs = {'proxyuser': proxyuser, 'renew': renew} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype for tries in range(self.opts.get('max_retries', 30)): @@ -2625,7 +2622,7 @@ class ClientSession(object): return True def ssl_login(self, cert=None, ca=None, serverca=None, proxyuser=None, proxyauthtype=None, - session_key=None): + renew=False): """ SSL cert based login @@ -2634,16 +2631,14 @@ class ClientSession(object): :param str serverca: path for CA public cert, otherwise system-wide CAs are used :param str proxyuser: name of proxied user (e.g. forwarding by web ui) :param int proxyauthtype: AUTHTYPE used by proxied user (can be different from ours) - :param str session_key: used for session renewal :returns bool: success """ # store calling parameters - self.logger.error("ssl_login---------------") self.auth_method = { 'method': 'ssl_login', 'kwargs': { - 'cert': cert, 'ca': ca, 'serverca': serverca, 'proxyuser': proxyuser, - 'proxyauthtype': proxyauthtype, 'session_key': session_key, + 'cert': cert, 'ca': ca, 'serverca': serverca, + 'proxyuser': proxyuser, 'proxyauthtype': proxyauthtype, } } cert = cert or self.opts.get('cert') @@ -2674,7 +2669,7 @@ class ClientSession(object): self.opts['serverca'] = serverca e_str = None try: - kwargs = {'proxyuser': proxyuser, 'session_key': session_key} + kwargs = {'proxyuser': proxyuser, 'renew': renew} if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype sinfo = self._callMethod('sslLogin', [], kwargs) @@ -2766,24 +2761,28 @@ class ClientSession(object): return self._prepUpload(*args, **kwargs) args = encode_args(*args, **kwargs) headers = [] - if self.logged_in: + + sinfo = None + if getattr(self, 'sinfo') is not None: + # session renewal (not logged in, but have session data) + # makes sense only for new method/server sinfo = self.sinfo.copy() sinfo['callnum'] = self.callnum self.callnum += 1 - 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': + headers += [ + ('Koji-Session-Id', str(sinfo['session-id'])), + ('Koji-Session-Key', str(sinfo['session-key'])), + ('Koji-Session-Callnum', str(sinfo['callnum'])), + ] + + if self.logged_in and not self.sinfo.get('header-auth'): + # old server + handler = "%s?%s" % (self.baseurl, six.moves.urllib.parse.urlencode(sinfo)) + elif name in 'sslLogin': handler = self.baseurl + '/ssllogin' else: handler = self.baseurl + request = dumps(args, name, allow_none=1) if six.PY3: # For python2, dumps() without encoding specified means return a str @@ -2896,12 +2895,11 @@ class ClientSession(object): """Renew expirated session or subsession.""" if not hasattr(self, 'auth_method'): raise GenericError("Missing info for reauthentication") - # will be deleted by setSession auth_method = getattr(self, self.auth_method['method']) args = self.auth_method.get('args', []) kwargs = self.auth_method.get('kwargs', {}) - kwargs['session_key'] = self.session_key - self.setSession(None) + kwargs['renew'] = True + self.logged_in = False auth_method(*args, **kwargs) if self.exclusive: self.exclusiveSession() diff --git a/koji/auth.py b/koji/auth.py index e42fe62..55758ee 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -56,6 +56,8 @@ RetryWhitelist = [ 'repoProblem', ] +AUTH_METHODS = ['login', 'sslLogin'] + logger = logging.getLogger('koji.auth') @@ -82,8 +84,8 @@ class Session(object): 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'] + self.id = int(environ['HTTP_KOJI_SESSION_ID']) + self.key = environ['HTTP_KOJI_SESSION_KEY'] try: callnum = int(environ['HTTP_KOJI_CALLNUM']) except KeyError: @@ -96,8 +98,8 @@ class Session(object): return args = urllib.parse.parse_qs(args, strict_parsing=True) try: - id = int(args['session-id'][0]) - key = args['session-key'][0] + self.id = int(args['session-id'][0]) + self.key = args['session-key'][0] except KeyError as field: raise koji.AuthError('%s not specified in session args' % field) try: @@ -119,23 +121,26 @@ class Session(object): query = QueryProcessor(tables=['sessions'], columns=columns, aliases=aliases, clauses=['id = %(id)i', 'key = %(key)s', 'hostip = %(hostip)s'], - values={'id': id, 'key': key, 'hostip': hostip}, + values={'id': self.id, 'key': self.key, 'hostip': hostip}, opts={'rowlock': True}) session_data = query.executeOne(strict=False) if not session_data: query = QueryProcessor(tables=['sessions'], columns=['key', 'hostip'], - clauses=['id = %(id)i'], values={'id': id}) + clauses=['id = %(id)i'], values={'id': self.id}) row = query.executeOne(strict=False) if row: - if key != row['key']: - logger.warning("Session ID %s is not related to session key %s.", id, key) + if self.key != row['key']: + logger.warning("Session ID %s is not related to session key %s.", + self.id, self.key) elif hostip != row['hostip']: - logger.warning("Session ID %s is not related to host IP %s.", id, hostip) + logger.warning("Session ID %s is not related to host IP %s.", self.id, hostip) raise koji.AuthError('Invalid session or bad credentials') # check for expiration if session_data['expired']: - raise koji.AuthExpired('session "%i" has expired' % id) + if getattr(context, 'method') not in AUTH_METHODS: + raise koji.AuthExpired(f'session "{self.id}" has expired') + # check for callnum sanity if callnum is not None: try: @@ -145,8 +150,7 @@ class Session(object): lastcall = session_data['callnum'] if lastcall is not None: if lastcall > callnum: - raise koji.SequenceError("%d > %d (session %d)" - % (lastcall, callnum, id)) + raise koji.SequenceError(f"{lastcall} > {callnum} (session {self.id})") elif lastcall == callnum: # Some explanation: # This function is one of the few that performs its own commit. @@ -159,8 +163,11 @@ class Session(object): method = getattr(context, 'method', 'UNKNOWN') if method not in RetryWhitelist: raise koji.RetryError( - "unable to retry call %d (method %s) for session %d" - % (callnum, method, id)) + f"unable to retry call {callnum} " + f"(method {method}) for session {self.id}") + + if session_data['expired']: + return # read user data # historical note: @@ -200,19 +207,17 @@ class Session(object): # update timestamp update = UpdateProcessor('sessions', rawdata={'update_time': 'NOW()'}, - clauses=['id = %(id)i'], values={'id': id}) + clauses=['id = %(id)i'], values={'id': self.id}) update.execute() context.cnx.commit() # update callnum (this is deliberately after the commit) # see earlier note near RetryError if callnum is not None: update = UpdateProcessor('sessions', rawdata={'callnum': callnum}, - clauses=['id = %(id)i'], values={'id': id}) + clauses=['id = %(id)i'], values={'id': self.id}) update.execute() # record the login data - self.id = id - self.key = key self.hostip = hostip self.callnum = callnum self.user_id = session_data['user_id'] @@ -325,7 +330,7 @@ class Session(object): return (local_ip, local_port, remote_ip, remote_port) - def sslLogin(self, proxyuser=None, proxyauthtype=None, session_key=None): + def sslLogin(self, proxyuser=None, proxyauthtype=None, renew=False): """Login into brew via SSL. proxyuser name can be specified and if it is allowed in the configuration file then connection is allowed to login as @@ -411,7 +416,7 @@ class Session(object): hostip = self.get_remote_ip() - sinfo = self.createSession(user_id, hostip, authtype, session_key=session_key) + sinfo = self.createSession(user_id, hostip, authtype, renew=renew) return sinfo def makeExclusive(self, force=False): @@ -491,37 +496,48 @@ class Session(object): update.execute() context.cnx.commit() - def createSession(self, user_id, hostip, authtype, master=None, session_key=None): + def createSession(self, user_id, hostip, authtype, master=None, renew=False): """Create a new session for the given user. Return a map containing the session-id and session-key. If master is specified, create a subsession """ - if session_key: - if master: - raise koji.GenericError("Can't call createSession with both master + session_key.") - query = QueryProcessor(tables=['sessions'], columns=['master'], - clauses=['key=%(session_key)d', 'closed=FALSE'], - values={'session_key': session_key}) - row = query.executeOne(strict=False) - if not row: - raise koji.GenericError("Don't allow to renew non-existent or logged out session") - master = row['master'] - # generate a random key alnum = string.ascii_letters + string.digits key = "%s-%s" % (user_id, ''.join([random.choice(alnum) for x in range(1, 20)])) # use sha? sha.new(phrase).hexdigest() - # get a session id - session_id = nextval('sessions_id_seq') - - # add session id to database - insert = InsertProcessor('sessions', - data={'id': session_id, 'user_id': user_id, 'key': key, - 'hostip': hostip, 'authtype': authtype, 'master': master}) - insert.execute() + if renew and self.id is not None: + # just update key + session_id = self.id + self.key = key + if self.master: + # check if master session died meanwhile + query = QueryProcessor(tables=['sessions'], + clauses=['id = %(master_id)d', + 'expired IS FALSE', + 'closed IS FALSE'], + values={'master_id': self.master}, + opts={'countOnly': True}) + if query.executeOne() == 0: + return None + + update = UpdateProcessor('sessions', + clauses=['id=%(id)i'], + rawdata={'update_time': 'NOW()'}, + data={'key': self.key, 'expired': False}, + values={'id': self.id}) + update.execute() + else: + # get a session id + session_id = nextval('sessions_id_seq') + # add session id to database + insert = InsertProcessor('sessions', + data={'id': session_id, 'user_id': user_id, 'key': key, + 'hostip': hostip, 'authtype': authtype, + 'master': master}) + insert.execute() context.cnx.commit() # return session info @@ -538,8 +554,7 @@ class Session(object): master = self.master if master is None: master = self.id - return self.createSession(self.user_id, self.hostip, self.authtype, - master=master) + return self.createSession(self.user_id, self.hostip, self.authtype, master=master) def getPerms(self): if not self.logged_in: From 35be62cdf542d366042cf77be83eb8cf0409f66e Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Dec 12 2022 11:45:42 +0000 Subject: [PATCH 8/9] renew exclusive status as part of login --- diff --git a/koji/__init__.py b/koji/__init__.py index 546efd7..297abea 100644 --- a/koji/__init__.py +++ b/koji/__init__.py @@ -2496,8 +2496,11 @@ class ClientSession(object): """ # store calling parameters self.auth_method = {'method': 'login', 'kwargs': {'opts': opts}} - sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], opts=opts, - renew=renew) + kwargs = {'opts': opts} + if renew: + kwargs['renew'] = True + kwargs['exclusive'] = self.exclusive + sinfo = self.callMethod('login', self.opts['user'], self.opts['password'], **kwargs) if not sinfo: return False self.setSession(sinfo) @@ -2573,7 +2576,10 @@ class ClientSession(object): # will fail with a handshake failure, which is retried by default. # For this case we're now using retry=False and test errors for # this exact usecase. - kwargs = {'proxyuser': proxyuser, 'renew': renew} + kwargs = {'proxyuser': proxyuser} + if renew: + kwargs['renew'] = True + kwargs['exclusive'] = self.exclusive if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype for tries in range(self.opts.get('max_retries', 30)): @@ -2669,7 +2675,10 @@ class ClientSession(object): self.opts['serverca'] = serverca e_str = None try: - kwargs = {'proxyuser': proxyuser, 'renew': renew} + kwargs = {'proxyuser': proxyuser} + if renew: + kwargs['renew'] = True + kwargs['exclusive'] = self.exclusive if proxyauthtype is not None: kwargs['proxyauthtype'] = proxyauthtype sinfo = self._callMethod('sslLogin', [], kwargs) @@ -2901,8 +2910,6 @@ class ClientSession(object): kwargs['renew'] = True self.logged_in = False auth_method(*args, **kwargs) - if self.exclusive: - self.exclusiveSession() def renew_expired_session(func): """Decorator to renew expirated session or subsession.""" diff --git a/koji/auth.py b/koji/auth.py index 55758ee..d15a6be 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -120,7 +120,8 @@ class Session(object): columns, aliases = zip(*fields) query = QueryProcessor(tables=['sessions'], columns=columns, aliases=aliases, - clauses=['id = %(id)i', 'key = %(key)s', 'hostip = %(hostip)s'], + clauses=['id = %(id)i', 'key = %(key)s', 'hostip = %(hostip)s', + 'closed IS FALSE'], values={'id': self.id, 'key': self.key, 'hostip': hostip}, opts={'rowlock': True}) session_data = query.executeOne(strict=False) @@ -146,7 +147,7 @@ class Session(object): try: callnum = int(callnum) except (ValueError, TypeError): - raise koji.AuthError("Invalid callnum: %r" % callnum) + raise koji.AuthError(f"Invalid callnum: {callnum!r}") lastcall = session_data['callnum'] if lastcall is not None: if lastcall > callnum: @@ -283,7 +284,7 @@ class Session(object): if result['status'] != koji.USER_STATUS['NORMAL']: raise koji.AuthError('logins by %s are not allowed' % result['name']) - def login(self, user, password, opts=None, session_key=None): + def login(self, user, password, opts=None, renew=False, exclusive=False): """create a login session""" if opts is None: opts = {} @@ -305,7 +306,9 @@ class Session(object): # create session and return sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], - session_key=session_key) + renew=renew) + if sinfo and exclusive and not self.exclusive: + self.makeExclusive() context.cnx.commit() return sinfo @@ -330,7 +333,7 @@ class Session(object): return (local_ip, local_port, remote_ip, remote_port) - def sslLogin(self, proxyuser=None, proxyauthtype=None, renew=False): + def sslLogin(self, proxyuser=None, proxyauthtype=None, renew=False, exclusive=None): """Login into brew via SSL. proxyuser name can be specified and if it is allowed in the configuration file then connection is allowed to login as @@ -417,6 +420,8 @@ class Session(object): hostip = self.get_remote_ip() sinfo = self.createSession(user_id, hostip, authtype, renew=renew) + if sinfo and exclusive and not self.exclusive: + self.makeExclusive() return sinfo def makeExclusive(self, force=False): From a56c10585ff401405a51a411ae27539408a6b96c Mon Sep 17 00:00:00 2001 From: Tomas Kopecek Date: Dec 12 2022 11:49:11 +0000 Subject: [PATCH 9/9] retain expired session exclusivity --- diff --git a/docs/schema-update-1.31-1.32.sql b/docs/schema-update-1.31-1.32.sql index 55a45e4..c676333 100644 --- a/docs/schema-update-1.31-1.32.sql +++ b/docs/schema-update-1.31-1.32.sql @@ -8,4 +8,5 @@ BEGIN; -- for tag if session is closed or not ALTER TABLE sessions ADD COLUMN closed BOOLEAN NOT NULL DEFAULT FALSE; ALTER TABLE sessions ADD CONSTRAINT no_closed_exclusive CHECK (closed IS FALSE OR "exclusive" IS NULL); + ALTER TABLE sessions DROP CONSTRAINT exclusive_expired_sane; COMMIT; diff --git a/docs/schema.sql b/docs/schema.sql index 74fde66..9de4e14 100644 --- a/docs/schema.sql +++ b/docs/schema.sql @@ -122,8 +122,8 @@ CREATE TABLE sessions ( closed BOOLEAN NOT NULL DEFAULT FALSE, CONSTRAINT no_exclusive_subsessions CHECK ( master IS NULL OR "exclusive" IS NULL), - CONSTRAINT exclusive_expired_sane CHECK ( - expired IS FALSE OR "exclusive" IS NULL), + CONSTRAINT no_closed_exclusive CHECK ( + closed IS FALSE OR "exclusive" IS NULL), UNIQUE (user_id,exclusive) ) WITHOUT OIDS; CREATE INDEX sessions_master ON sessions(master); diff --git a/koji/auth.py b/koji/auth.py index d15a6be..ba3dea8 100644 --- a/koji/auth.py +++ b/koji/auth.py @@ -190,7 +190,7 @@ class Session(object): # see if an exclusive session exists query = QueryProcessor(tables=['sessions'], columns=['id'], clauses=['user_id=%(user_id)s', 'exclusive = TRUE', - 'expired = FALSE'], + 'closed = FALSE'], values=session_data) excl_id = query.singleValue(strict=False) @@ -305,8 +305,7 @@ class Session(object): self.checkLoginAllowed(user_id) # create session and return - sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], - renew=renew) + sinfo = self.createSession(user_id, hostip, koji.AUTHTYPES['NORMAL'], renew=renew) if sinfo and exclusive and not self.exclusive: self.makeExclusive() context.cnx.commit() @@ -437,9 +436,9 @@ class Session(object): query = QueryProcessor(tables=['users'], columns=['id'], clauses=['id=%(user_id)s'], values={'user_id': user_id}, opts={'rowlock': True}) query.execute() - # check that no other sessions for this user are exclusive + # check that no other sessions for this user are exclusive (including expired) query = QueryProcessor(tables=['sessions'], columns=['id'], - clauses=['user_id=%(user_id)s', 'expired = FALSE', + clauses=['user_id=%(user_id)s', 'closed = FALSE', 'exclusive = TRUE'], values={'user_id': user_id}, opts={'rowlock': True}) excl_id = query.singleValue(strict=False) @@ -466,7 +465,7 @@ class Session(object): context.cnx.commit() def logout(self, session_id=None): - """expire a login session""" + """close a login session""" if not self.logged_in: # XXX raise an error? raise koji.AuthError("Not logged in") @@ -491,11 +490,12 @@ class Session(object): self.logged_in = False def logoutChild(self, session_id): - """expire a subsession""" + """close a subsession""" if not self.logged_in: # XXX raise an error? raise koji.AuthError("Not logged in") - update = UpdateProcessor('sessions', data={'expired': True, 'exclusive': None}, + update = UpdateProcessor('sessions', + data={'expired': True, 'exclusive': None, 'closed': True}, clauses=['id = %(session_id)i', 'master = %(master)i'], values={'session_id': session_id, 'master': self.id}) update.execute() @@ -518,11 +518,9 @@ class Session(object): session_id = self.id self.key = key if self.master: - # check if master session died meanwhile + # check if master session died meanwhile (expired is ok) query = QueryProcessor(tables=['sessions'], - clauses=['id = %(master_id)d', - 'expired IS FALSE', - 'closed IS FALSE'], + clauses=['id = %(master_id)d', 'closed IS FALSE'], values={'master_id': self.master}, opts={'countOnly': True}) if query.executeOne() == 0: