From bb1ca345636bb576f113d878d6b042b4082bea70 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Jun 22 2022 09:21:10 +0000 Subject: [PATCH 1/3] Expire refreshable tokens in a configurable amount of time Signed-off-by: Aurélien Bompard --- diff --git a/ipsilon/providers/openidc/store.py b/ipsilon/providers/openidc/store.py index ae9518b..e7873e2 100644 --- a/ipsilon/providers/openidc/store.py +++ b/ipsilon/providers/openidc/store.py @@ -27,9 +27,10 @@ class OpenIDCStaticStore(Store): class OpenIDCStore(Store): - def __init__(self, database_url, static_store): + def __init__(self, database_url, static_store, token_lifetime): Store.__init__(self, database_url=database_url) self.static_store = static_store + self.token_lifetime = token_lifetime def registerDynamicClient(self, client): data = {} @@ -216,14 +217,15 @@ class OpenIDCStore(Store): token_security_check = generate_random_secure_string() refresh_security_check = generate_random_secure_string(128) - expires_in = 3600 - # TODO: Figure out values for this - refreshable_until = None + expires_in = self.token_lifetime['access'] token['security_check'] = token_security_check token['refresh_security_check'] = refresh_security_check token['expires_at'] = int(time.time()) + expires_in - token['refreshable_until'] = refreshable_until + token['refreshable_until'] = ( + None if self.token_lifetime['refresh'] is None else + int(time.time()) + self.token_lifetime['refresh'] + ) self.update_token(token) @@ -240,7 +242,7 @@ class OpenIDCStore(Store): userinfocode): token_security_check = generate_random_secure_string() - expires_in = 3600 + expires_in = self.token_lifetime['access'] token = { 'type': 'Bearer', @@ -256,8 +258,10 @@ class OpenIDCStore(Store): if issue_refresh: token['refreshable'] = True - # TODO: Figure out time for this - token['refreshable_until'] = None + token['refreshable_until'] = ( + None if self.token_lifetime['refresh'] is None else + int(time.time()) + self.token_lifetime['refresh'] + ) token['refresh_security_check'] = \ generate_random_secure_string(128) diff --git a/ipsilon/providers/openidcp.py b/ipsilon/providers/openidcp.py index aee776b..7d3ddb3 100644 --- a/ipsilon/providers/openidcp.py +++ b/ipsilon/providers/openidcp.py @@ -88,6 +88,14 @@ Provides OpenID Connect authentication infrastructure. """ 'default allowed attributes', 'Defines a list of allowed attributes, applied after mapping', ['*']), + pconfig.Integer( + 'access token lifetime', + 'The access tokens will expire after this number of seconds', + 3600), + pconfig.Integer( + 'refresh token lifetime', + 'The refresh tokens will expire after this number of seconds', + 3600 * 24 * 365), # 1 year ) @property @@ -175,10 +183,18 @@ Provides OpenID Connect authentication infrastructure. """ for key in loaded_keys['keys']: self.keyset.add(JWK(**key)) + token_lifetime = { + "access": self.get_config_value("access token lifetime"), + "refresh": self.get_config_value("refresh token lifetime"), + } static_store = OpenIDCStaticStore( - self.get_config_value('static database url')) - self.datastore = OpenIDCStore(self.get_config_value('database url'), - static_store) + self.get_config_value('static database url'), + ) + self.datastore = OpenIDCStore( + self.get_config_value('database url'), + static_store, + token_lifetime=token_lifetime + ) def openid_connect_issuer_wf_rel(self, resource): link = { diff --git a/tests/testcleanup.py b/tests/testcleanup.py index 3ef02f5..ce92b5c 100755 --- a/tests/testcleanup.py +++ b/tests/testcleanup.py @@ -113,6 +113,8 @@ if __name__ == '__main__': sess.add_server(idpname, 'https://127.0.0.10:45080', user, 'ipsilon') sess.add_server(sp1name, 'https://127.0.0.11:45081') + db_url_base = f"sqlite:///{os.environ['TESTDIR']}lib/idp1" + with TC.case('Verify logged out state'): page = sess.fetch_page(idpname, 'https://127.0.0.10:45080/idp1/') page.expected_value('//div[@id="content"]/p/a/text()', 'Log In') @@ -161,15 +163,23 @@ if __name__ == '__main__': with TC.case('Checking that refreshable OpenIDC tokens are not expired'): - static_db_path = os.path.join(os.environ['TESTDIR'], 'lib/idp1/openidc.static.sqlite') - db_path = os.path.join(os.environ['TESTDIR'], 'lib/idp1/openidc.sqlite') - static_store = OpenIDCStaticStore(database_url=f"sqlite:///{static_db_path}") + static_store = OpenIDCStaticStore(database_url=f"{db_url_base}/openidc.static.sqlite") store = OpenIDCStore( - database_url=f"sqlite:///{db_path}", static_store=static_store + database_url=f"{db_url_base}/openidc.sqlite", + static_store=static_store, + token_lifetime={"access": 3600, "refresh": None}, ) + # Remove existing tokens and userinfo + for token_id in store.get_unique_data("token"): + store.del_unique_data("token", token_id) + for ui_id in store.get_unique_data("userinfo"): + store.del_unique_data("userinfo", ui_id) + + # Prepare userinfo userinfocode = store.storeUserInfo({"name": "dummy"}) + # Create tokens token_refreshable = store.issueToken( client_id="client-id", username="username", scope=["openid"], issue_refresh=True, userinfocode=userinfocode @@ -182,7 +192,7 @@ if __name__ == '__main__': assert len(store.get_unique_data("token")) == 2 - conn = sqlite3.connect(db_path) + conn = sqlite3.connect(f"{os.environ['TESTDIR']}lib/idp1/openidc.sqlite") cur = conn.cursor() expired_ts = int(time.time()) - 1 @@ -195,11 +205,7 @@ if __name__ == '__main__': conn.commit() conn.close() - try: - cleanup_count = store._cleanupExpiredTokens() - except Exception as e: - print(e) - raise + cleanup_count = store._cleanupExpiredTokens() if cleanup_count != 1: raise Exception( @@ -215,3 +221,33 @@ if __name__ == '__main__': userinfo = store.get_unique_data("userinfo") if len(userinfo) != 1: raise Exception("The userinfo data has been cleaned up") + + + with TC.case('Checking that access tokens expire in the right amount of time'): + static_store = OpenIDCStaticStore(database_url=f"{db_url_base}/openidc.static.sqlite") + store = OpenIDCStore( + database_url=f"{db_url_base}/openidc.sqlite", + static_store=static_store, + token_lifetime={"access": 1, "refresh": 1}, + ) + token = store.issueToken( + client_id="client-id", username="username", scope=["openid"], + issue_refresh=False, userinfocode=userinfocode + ) + + time.sleep(3) + cleanup_count = store._cleanupExpiredTokens() + + assert cleanup_count == 1, f"{cleanup_count} tokens were cleaned up" + assert token["token_id"] not in store.get_unique_data("token").keys() + + with TC.case("Checking that refresh tokens expire in the right amount of time"): + token = store.issueToken( + client_id="client-id", username="username", scope=["openid"], + issue_refresh=True, userinfocode=userinfocode + ) + time.sleep(3) + cleanup_count = store._cleanupExpiredTokens() + + assert cleanup_count == 1, f"{cleanup_count} tokens were cleaned up" + assert token["token_id"] not in store.get_unique_data("token").keys() From 81f6b3960374ffe28d84454b44f6b4c4adb86b30 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Jun 22 2022 09:21:10 +0000 Subject: [PATCH 2/3] Make the tests outputs more readable Signed-off-by: Aurélien Bompard --- diff --git a/tests/helpers/common.py b/tests/helpers/common.py index 87d62e4..afbe09c 100755 --- a/tests/helpers/common.py +++ b/tests/helpers/common.py @@ -486,8 +486,9 @@ basicConstraints = CA:false""" % {'certdir': os.path.join(self.testdir, return self.run_and_collect([self.pycmd, exe], env) def run_and_collect(self, cmd, env): - p = subprocess.Popen(cmd, env=env, - stdout=subprocess.PIPE, stderr=self.stderr) + p = subprocess.Popen( + cmd, env=env, universal_newlines=True, + stdout=subprocess.PIPE, stderr=self.stderr) results = [] for line in p.stdout: line = line[:-1] # Strip newline diff --git a/tests/helpers/control.py b/tests/helpers/control.py index 001e279..dd2b104 100644 --- a/tests/helpers/control.py +++ b/tests/helpers/control.py @@ -3,6 +3,7 @@ from __future__ import print_function import sys +from traceback import print_exception class TC(object): @@ -37,6 +38,7 @@ class TC(object): if exc is None and not self.should_fail: TC.output_method(TC.prefix + 'done') elif not self.should_fail: + print_exception(exc) TC.output_method(TC.prefix + 'fail:' + repr(exc)) sys.exit(1) elif not exc: @@ -80,8 +82,8 @@ class TC(object): case done: ('done',) case fail: ('fail', 'some error') """ - if line.startswith(TC.prefix.encode('utf-8')): - return tuple(line[len(TC.prefix):].split(b':')) + if line.startswith(TC.prefix): + return tuple(line[len(TC.prefix):].split(':')) else: return None From e8b89638d4670da713f52baa83d027a6ac1b0b1b Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Jun 22 2022 09:21:10 +0000 Subject: [PATCH 3/3] Make the Integer config class actually return an integer Signed-off-by: Aurélien Bompard --- diff --git a/ipsilon/util/config.py b/ipsilon/util/config.py index bbc171a..1707daf 100644 --- a/ipsilon/util/config.py +++ b/ipsilon/util/config.py @@ -195,6 +195,15 @@ class Integer(String): if value: self._assigned_value = int(value) + def get_value(self, default=True): + value = super().get_value(default) + return int(value) if value is not None else None + + def export_value(self): + if self._assigned_value is None: + return None + return int(self._assigned_value) + class Image(Option): """