PR#3760: Add renewal session timeout - koji

koji

#3760 Add renewal session timeout

Merged a year ago by tkopecek. Opened a year ago by jcupova.

jcupova/koji issue-3596 into master

Drop default Null for renew_time in sql

Jana Cupova • a year ago

805fb2f

Add renew_ts column and check with renew_ts

Jana Cupova • a year ago

13166bb

Add renewal session timeout

Jana Cupova • a year ago

b1f614b

docs/schema-upgrade-1.32-1.33.sql

file added

		`@@ -0,0 +1,7 @@`
		`+ -- upgrade script to migrate the Koji database schema`
		`+ -- from version 1.32 to 1.33`
		`+`
		`+ BEGIN;`
		`+ ALTER TABLE sessions ADD COLUMN renew_time TIMESTAMPTZ;`
		`+ COMMIT;`
		`+`

docs/schema.sql

file modified

		`@@ -120,6 +120,7 @@`
		`update_time TIMESTAMPTZ NOT NULL DEFAULT NOW(),`
		`exclusive BOOLEAN CHECK (exclusive),`
		`closed BOOLEAN NOT NULL DEFAULT FALSE,`
		`+ renew_time TIMESTAMPTZ,`
		`CONSTRAINT no_exclusive_subsessions CHECK (`
		`master IS NULL OR "exclusive" IS NULL),`
		`CONSTRAINT no_closed_exclusive CHECK (`

docs/source/hub_conf.rst

file modified

+9 -1

		`@@ -112,6 +112,14 @@`

		`Whether or not to automatically create a new user from valid ssl or gssapi credentials.`

		`+ SessionRenewalTimeout`
		`+ Type: integer`
		`+`
		+ Default: ``1440``
		`+`
		`+ The number of minutes before sessions are required to re-authenticate.`
		`+ Set to 0 for no timeout.`
		`+`
		`GSSAPI authentication options`
		`^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^`

		`@@ -553,4 +561,4 @@`

		Default: ``md5 sha256``

		- Set RPM default checksums type. Default value is set upt to ``md5 sha256``.
		+ Set RPM default checksums type. Default value is set up to ``md5 sha256``.

kojihub/app/hub.conf

file modified

		`@@ -144,3 +144,6 @@`

		`## Determines default checksums`
		`# RPMDefaultChecksums = md5 sha256`
		`+`
		`+ # The number of minutes before sessions are required to re-authenticate. Set to 0 for no timeout.`
		`+ # SessionRenewalTimeout = 1440`

kojihub/auth.py

file modified

+20 -3

		`@@ -26,6 +26,7 @@`
		`import re`
		`import socket`
		`import string`
		`+ import time`

		`import six`
		`from six.moves import range, urllib`
		`@@ -116,7 +117,8 @@`
		`fields = (('authtype', 'authtype'), ('callnum', 'callnum'), ('exclusive', 'exclusive'),`
		`('expired', 'expired'), ('master', 'master'), ('start_time', 'start_time'),`
		`('update_time', 'update_time'), ("date_part('epoch', start_time)", 'start_ts'),`
		`- ("date_part('epoch', update_time)", 'update_ts'), ('user_id', 'user_id'))`
		`+ ("date_part('epoch', update_time)", 'update_ts'), ('user_id', 'user_id'),`
		`+ ('renew_time', 'renew_time'), ("date_part('epoch', renew_time)", 'renew_ts'))`
		`columns, aliases = zip(*fields)`

		`query = QueryProcessor(tables=['sessions'], columns=columns, aliases=aliases,`
		`@@ -137,7 +139,22 @@`
		`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 not session_data['expired'] and context.opts['SessionRenewalTimeout'] != 0:`
		`+ if session_data['renew_ts']:`
		`+ renewal_cutoff = (session_data['renew_ts'] +`
		`+ context.opts['SessionRenewalTimeout'] * 60)`
		`+ else:`
		`+ renewal_cutoff = (session_data['start_ts'] +`
		`+ context.opts['SessionRenewalTimeout'] * 60)`
		`+ if time.time() > renewal_cutoff:`
		`+ session_data['expired'] = True`
		`+ update = UpdateProcessor('sessions',`
		`+ data={'expired': True},`
		`+ clauses=['id = %(id)s OR master = %(id)s'],`
		`+ values={'id': self.id})`
		`+ update.execute()`
		`+ context.cnx.commit()`
		`+`
		`if session_data['expired']:`
		`if getattr(context, 'method') not in AUTH_METHODS:`
		`raise koji.AuthExpired('session "%s" has expired' % self.id)`
		`@@ -523,7 +540,7 @@`

		`update = UpdateProcessor('sessions',`
		`clauses=['id=%(id)i'],`
		`- rawdata={'update_time': 'NOW()'},`
		`+ rawdata={'update_time': 'NOW()', 'renew_time': 'NOW()'},`
		`data={'key': self.key, 'expired': False},`
		`values={'id': self.id})`
		`update.execute()`

kojihub/kojixmlrpc.py

file modified

+3 -1

		`@@ -499,7 +499,9 @@`
		`['RegexNameInternal', 'string', r'^[A-Za-z0-9/_.+-]+$'],`
		`['RegexUserName', 'string', r'^[A-Za-z0-9/_.@-]+$'],`

		`- ['RPMDefaultChecksums', 'string', 'md5 sha256']`
		`+ ['RPMDefaultChecksums', 'string', 'md5 sha256'],`
		`+`
		`+ ['SessionRenewalTimeout', 'integer', 1440],`
		`]`
		`opts = {}`
		`for name, dtype, default in cfgmap:`

tests/test_hub/test_get_session_info.py

file modified

+4 -4

		`@@ -41,8 +41,8 @@`
		`self.assertEqual(query.clauses, ['expired is FALSE', 'user_id = %(user_id)i'])`
		`self.assertEqual(query.joins, None)`
		`self.assertEqual(query.columns, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`- "date_part('epoch', start_time)", 'update_time',`
		`- 'user_id'])`
		`+ "date_part('epoch', start_time)",`
		`+ 'update_time', 'user_id'])`
		`self.assertEqual(query.aliases, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`'start_time', 'update_time', 'user_id'])`

		`@@ -71,8 +71,8 @@`
		`self.assertEqual(query.clauses, ['expired is FALSE', 'user_id = %(user_id)i'])`
		`self.assertEqual(query.joins, None)`
		`self.assertEqual(query.columns, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`- "date_part('epoch', start_time)", 'update_time',`
		`- 'user_id'])`
		`+ "date_part('epoch', start_time)",`
		`+ 'update_time', 'user_id'])`
		`self.assertEqual(query.aliases, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`'start_time', 'update_time', 'user_id'])`

tests/test_lib/test_auth.py

file modified

+63 -4

		`@@ -49,6 +49,7 @@`
		`self.context.opts = {`
		`'CheckClientIP': True,`
		`'DisableURLSessions': False,`
		`+ 'SessionRenewalTimeout': 0,`
		`}`
		`with self.assertRaises(koji.GenericError) as cm:`
		`kojihub.auth.Session()`
		`@@ -62,6 +63,7 @@`
		`self.context.opts = {`
		`'CheckClientIP': True,`
		`'DisableURLSessions': False,`
		`+ 'SessionRenewalTimeout': 0,`
		`}`
		`self.context.environ = {`
		`'QUERY_STRING': 'session-id=123&session-key=xyz&callnum=345',`
		`@@ -88,6 +90,7 @@`
		`self.context.opts = {`
		`'CheckClientIP': True,`
		`'DisableURLSessions': True,`
		`+ 'SessionRenewalTimeout': 0,`
		`}`
		`self.context.environ = {`
		`'HTTP_KOJI_SESSION_ID': '123',`
		`@@ -113,6 +116,60 @@`
		`def test_session_old(self):`
		`self.get_session_old()`

		`+ def test_renewal_timeout(self):`
		`+ """Simple kojihub.auth.Session instance"""`
		`+ self.context.opts = {`
		`+ 'CheckClientIP': True,`
		`+ 'DisableURLSessions': False,`
		`+ 'SessionRenewalTimeout': 1440,`
		`+ }`
		`+ self.context.environ = {`
		`+ 'QUERY_STRING': 'session-id=123&session-key=xyz&callnum=345',`
		`+ 'REMOTE_ADDR': 'remote-addr',`
		`+ }`
		`+`
		`+ self.query_executeOne.side_effect = [`
		`+ {'authtype': 2, 'callnum': 1, "start_ts": 1666599426.227002,`
		`+ "update_ts": 1666599426.254308, 'exclusive': None,`
		`+ 'expired': False, 'master': None,`
		`+ 'start_time': datetime.datetime(2022, 10, 24, 8, 17, 6, 227002,`
		`+ tzinfo=datetime.timezone.utc),`
		`+ 'update_time': datetime.datetime(2022, 10, 24, 8, 17, 6, 254308,`
		`+ tzinfo=datetime.timezone.utc),`
		`+ 'renew_ts': None,`
		`+ 'user_id': 1},`
		`+ {'name': 'kojiadmin', 'status': 0, 'usertype': 0}]`
		`+ with self.assertRaises(koji.GenericError) as cm:`
		`+ kojihub.auth.Session()`
		`+ # no args in request/environment`
		`+ self.assertEqual(cm.exception.args[0], 'session "123" has expired')`
		`+`
		`+ self.assertEqual(len(self.updates), 1)`
		`+ self.assertEqual(len(self.queries), 1)`
		`+`
		`+ update = self.updates[0]`
		`+`
		`+ self.assertEqual(update.table, 'sessions')`
		`+ self.assertEqual(update.values['id'], 123)`
		`+ self.assertEqual(update.clauses, ['id = %(id)s OR master = %(id)s'])`
		`+ self.assertEqual(update.data, {'expired': True})`
		`+ self.assertEqual(update.rawdata, {})`
		`+`
		`+ query = self.queries[0]`
		`+ self.assertEqual(query.tables, ['sessions'])`
		`+ self.assertEqual(query.joins, None)`
		`+ self.assertEqual(query.clauses, ['closed IS FALSE', 'hostip = %(hostip)s', 'id = %(id)i',`
		`+ 'key = %(key)s'])`
		`+ self.assertEqual(query.columns, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`+ 'renew_time', "date_part('epoch', renew_time)",`
		`+ 'start_time', "date_part('epoch', start_time)",`
		`+ 'update_time', "date_part('epoch', update_time)",`
		`+ 'user_id'])`
		`+ self.assertEqual(query.aliases, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`+ 'renew_time', 'renew_ts', 'start_time', 'start_ts',`
		`+ 'update_time', 'update_ts', 'user_id'])`
		`+ self.assertEqual(query.values, {'id': 123, 'key': 'xyz', 'hostip': 'remote-addr'})`
		`+`
		`def test_basic_instance(self):`
		`"""auth.Session instance"""`
		`s, cntext = self.get_session()`
		`@@ -141,12 +198,13 @@`
		`self.assertEqual(query.clauses, ['closed IS FALSE', 'hostip = %(hostip)s', 'id = %(id)i',`
		`'key = %(key)s'])`
		`self.assertEqual(query.columns, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`+ 'renew_time', "date_part('epoch', renew_time)",`
		`'start_time', "date_part('epoch', start_time)",`
		`'update_time', "date_part('epoch', update_time)",`
		`'user_id'])`
		`self.assertEqual(query.aliases, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`- 'start_time', 'start_ts', 'update_time', 'update_ts',`
		`- 'user_id'])`
		`+ 'renew_time', 'renew_ts', 'start_time', 'start_ts',`
		`+ 'update_time', 'update_ts', 'user_id'])`
		`self.assertEqual(query.values, {'id': 123, 'key': 'xyz', 'hostip': 'remote-addr'})`

		`query = self.queries[1]`
		`@@ -192,12 +250,13 @@`
		`self.assertEqual(query.clauses, ['closed IS FALSE', 'hostip = %(hostip)s', 'id = %(id)i',`
		`'key = %(key)s'])`
		`self.assertEqual(query.columns, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`+ 'renew_time', "date_part('epoch', renew_time)",`
		`'start_time', "date_part('epoch', start_time)",`
		`'update_time', "date_part('epoch', update_time)",`
		`'user_id'])`
		`self.assertEqual(query.aliases, ['authtype', 'callnum', 'exclusive', 'expired', 'master',`
		`- 'start_time', 'start_ts', 'update_time', 'update_ts',`
		`- 'user_id'])`
		`+ 'renew_time', 'renew_ts', 'start_time', 'start_ts',`
		`+ 'update_time', 'update_ts', 'user_id'])`
		`self.assertEqual(query.values, {'id': 123, 'key': 'xyz', 'hostip': 'remote-addr'})`

		`query = self.queries[1]`

jcupova commented a year ago

Fixes: https://pagure.io/koji/issue/3596

rebased onto 89545a9e38fff81b653f29471237ead80025651d

a year ago

mikem commented a year ago

We should probably avoid the timeout calculation in the case where the session is already expired.

It should be possible to disable the timeout, e.g. by setting it to zero.

Including a comma after the last config item will make future diffs prettier.

New config option needs to be documented.

Since the term for what we're implementing is "renewal timeout", it might be better to keep that term intact in our options and variables, e.g. SessionRenewalTimeout

mikem commented a year ago

So maybe something like this?

        if not session_data['expired']:
            renewal_cutoff = (session_data['start_ts'] +
                                context.opts['SessionRenewalTimeout'] * 60)
            if time.time() > renewal_cutoff:
                session_data['expired'] = True
                update = UpdateProcessor('sessions',
                                         data={'expired': True},
                                         clauses=['id = %(id)s OR master = %(id)s'],
                                         values={'id': self.id})
                update.execute()
                context.cnx.commit()

Note that while we have a lot of old code that uses paramstyle codes other than "s", and db.py has code to compensate, it is best to use "%(param)s" regardless of the value type.

1 new commit added

Fix review comments

a year ago

jcupova commented a year ago

@mikem all fixed and I add check when options is set up to 0 for disable it.
@tkopecek are you ok with current option name?

mikem commented a year ago

The renewal docs can go under General authentication options. I don't think it it needs its own heading. Also, the grammar could be improved.

Perhaps, "The number of minutes before sessions are required to re-authenticate. Set to 0 for no timeout."

1 new commit added

Update docs

a year ago

jcupova commented a year ago

@mikem I moved renewal docs and updated it.

tkopecek commented a year ago

@tkopecek are you ok with current option name?

yep, I agree

mikem commented a year ago

works for me

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

a year ago

rebased onto b1f614b

a year ago

jcupova commented a year ago

@mikem there is a problem when we are checking start_ts. It returns fault with expiration, when kojid is trying the relogin. I spoke about it with Tomas and his idea is that we can add 'last_expired_ts' column to session table and check this timestamp instead of start_ts. What do you think? Do we want to add new column to session table? Or do you have any other idea?

tkopecek commented a year ago

few notes:
- start_ts + timeout is constant time, so after first expiration it will always expire
- modification of start_ts via relogin is not suitable as we would lose information for absolute expiration.
- adding new column is extending busy sessions table, but impact shouldn't be that big

mikem commented a year ago

Yeah, pretty much the only way is to add a new field. I recommend naming it renew_time.

I'm wondering if it makes sense to start it out NULL. This makes sense to me since at the start it has yet to be renewed. OTOH, it does make our check slightly more complex (I think
cutoff = (renew_ts or start_ts) + timeout).

I don't think it would be terrible to just update start_time since the renew call must provide full credentials, but it would be nice to keep the data anyway.

I suppose it might also be interesting to record a renewal_count, but probably not worth bothering at the moment.

Edited a year ago by mikem

1 new commit added

Add renew_ts column and check with renew_ts

a year ago

2 new commits added

Add renew_ts column and check with renew_ts
Add renewal session timeout

a year ago

mikem commented a year ago

Probably worth adding the new field to getSessionInfo

There's no need to specify a field as "DEFAULT NULL". All fields default to null if there is no other default specified.

Overall looks good!

jcupova commented a year ago

@mikem I spoked about add this new field to getSessionInfo, but our final reason for this was not add it to the getSessionInfo, because this is not anything what we want to show there. But if you have any idea, why we want to add it there, ok, I can add it.

mikem commented a year ago

getSessionInfo can only be used by:

calls from the session itself
admins

I don't see why we wouldn't treat it differently from start_time, which we expose. That said, it's fine to leave it out of this PR

1 new commit added

Add renew time and ts to getSessionInfo

a year ago

3 new commits added

Add renew time and ts to getSessionInfo
Add renew_ts column and check with renew_ts
Add renewal session timeout

a year ago

mikem commented a year ago

When I brought up getSessionInfo, I hadn't quite realized how inconsistent the api was. I am hesitant to try to solve that here. You've rightly filed this as a separate issue in #3793

Given that, I think the best thing to do for this PR is to leave the getSessionInfo code alone for now, and only add the renew_ts field to the query in auth.py, which is basically where we were before the last commit (apart from the schema fix).

3 new commits added