From 56fe9cf1ff4284975aa69f74ccdf3bfad9ff5fd8 Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Mon, 11 Jul 2022 11:45:32 -0400 Subject: [PATCH 1/4] Enforce a limit on API keys per Principal. --- tiled/_tests/test_authentication.py | 6 ++++++ tiled/server/authentication.py | 20 ++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/tiled/_tests/test_authentication.py b/tiled/_tests/test_authentication.py index af0ffc81e..465b55785 100644 --- a/tiled/_tests/test_authentication.py +++ b/tiled/_tests/test_authentication.py @@ -11,6 +11,7 @@ from ..client import from_config from ..client.context import CannotRefreshAuthentication from ..client.utils import ClientError +from ..server.authentication import API_KEY_LIMIT arr = ArrayAdapter.from_array(numpy.ones((5, 5))) @@ -328,3 +329,8 @@ def test_api_keys(enter_password, config): time.sleep(2) with fail_with_status_code(401): from_config(config, api_key=user_key_info["secret"]) + + # Hit API key limit. + with fail_with_status_code(400): + for i in range(1 + API_KEY_LIMIT): + user_client.context.create_api_key(note=f"key {i}") diff --git a/tiled/server/authentication.py b/tiled/server/authentication.py index bb86a2982..d49430982 100644 --- a/tiled/server/authentication.py +++ b/tiled/server/authentication.py @@ -47,6 +47,13 @@ ALGORITHM = "HS256" UNIT_SECOND = timedelta(seconds=1) +# Max API keys allowed to Principal. +# This is here for at least two reasons: +# 1. Ensure that the route which lists API keys, which is not paginated, returns in +# a reasonable time. +# 2. Avoid unintentional or intentional abuse. +API_KEY_LIMIT = 100 + def utcnow(): "UTC now with second resolution" @@ -444,6 +451,19 @@ def generate_apikey(db, principal, apikey_params, request): # plus 4 more for extra safety since we store the first eight HEX chars. secret = secrets.token_bytes(4 + 32) hashed_secret = hashlib.sha256(secret).digest() + keys_count = ( + db.query(orm.APIKey) + .join(orm.Principal) + .filter(orm.Principal.id == principal.id) + .count() + ) + if keys_count >= API_KEY_LIMIT: + raise HTTPException( + 400, + f"This Principal already has {keys_count} API keys which is greater " + f"than or equal to the maximum number allowed, {API_KEY_LIMIT}. " + "Some API keys must be deleted before creating new ones.", + ) new_key = orm.APIKey( principal_id=principal.id, expiration_time=expiration_time, From d492102772352748f1b8ecbf7bf500315de19e6b Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Mon, 11 Jul 2022 12:11:34 -0400 Subject: [PATCH 2/4] Enforce a limit on Sessions per Principal. --- tiled/_tests/test_authentication.py | 39 ++++++++++++++++++++++++++--- tiled/server/authentication.py | 18 +++++++++++-- 2 files changed, 51 insertions(+), 6 deletions(-) diff --git a/tiled/_tests/test_authentication.py b/tiled/_tests/test_authentication.py index 465b55785..280029940 100644 --- a/tiled/_tests/test_authentication.py +++ b/tiled/_tests/test_authentication.py @@ -11,7 +11,7 @@ from ..client import from_config from ..client.context import CannotRefreshAuthentication from ..client.utils import ClientError -from ..server.authentication import API_KEY_LIMIT +from ..server import authentication arr = ArrayAdapter.from_array(numpy.ones((5, 5))) @@ -330,7 +330,38 @@ def test_api_keys(enter_password, config): with fail_with_status_code(401): from_config(config, api_key=user_key_info["secret"]) - # Hit API key limit. - with fail_with_status_code(400): - for i in range(1 + API_KEY_LIMIT): + +def test_api_key_limit(enter_password, config): + # Decrease the limit so this test runs faster. + original_limit = authentication.API_KEY_LIMIT + authentication.API_KEY_LIMIT = 10 + try: + with enter_password("secret2"): + user_client = from_config(config, username="bob", token_cache={}) + + for i in range(authentication.API_KEY_LIMIT): user_client.context.create_api_key(note=f"key {i}") + # Hit API key limit. + with fail_with_status_code(400): + user_client.context.create_api_key(note="one key too many") + finally: + authentication.API_KEY_LIMIT = original_limit + + +def test_session_limit(enter_password, config): + # Decrease the limit so this test runs faster. + original_limit = authentication.SESSION_LIMIT + authentication.SESSION_LIMIT = 10 + try: + with enter_password("secret2"): + from_config(config, username="bob", token_cache={}) + + with enter_password("secret1"): + for _ in range(authentication.SESSION_LIMIT): + from_config(config, username="alice", token_cache={}) + # Hit Session limit. + with fail_with_status_code(400): + with enter_password("secret1"): + from_config(config, username="alice", token_cache={}) + finally: + authentication.SESSION_LIMIT = original_limit diff --git a/tiled/server/authentication.py b/tiled/server/authentication.py index d49430982..ee4718f6e 100644 --- a/tiled/server/authentication.py +++ b/tiled/server/authentication.py @@ -49,10 +49,11 @@ # Max API keys allowed to Principal. # This is here for at least two reasons: -# 1. Ensure that the route which lists API keys, which is not paginated, returns in -# a reasonable time. +# 1. Ensure that the routes which list API keys and sessions, which are +# not paginated, returns in a reasonable time. # 2. Avoid unintentional or intentional abuse. API_KEY_LIMIT = 100 +SESSION_LIMIT = 200 def utcnow(): @@ -343,6 +344,19 @@ def create_session(settings, identity_provider, id): else: identity.latest_login = now principal = identity.principal + session_count = ( + db.query(orm.Session) + .join(orm.Principal) + .filter(orm.Principal.id == principal.id) + .count() + ) + if session_count >= SESSION_LIMIT: + raise HTTPException( + 400, + f"This Principal already has {session_count} sessions which is greater " + f"than or equal to the maximum number allowed, {SESSION_LIMIT}. " + "Some Sessions must be closed before creating new ones.", + ) session = orm.Session( principal_id=principal.id, expiration_time=utcnow() + settings.session_max_age, From 724a5aedbb184b9a5b66d5dcef9af16fbdf6f3a9 Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Mon, 11 Jul 2022 12:12:26 -0400 Subject: [PATCH 3/4] update comment --- tiled/server/authentication.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tiled/server/authentication.py b/tiled/server/authentication.py index ee4718f6e..56d4d7953 100644 --- a/tiled/server/authentication.py +++ b/tiled/server/authentication.py @@ -47,7 +47,7 @@ ALGORITHM = "HS256" UNIT_SECOND = timedelta(seconds=1) -# Max API keys allowed to Principal. +# Max API keys and Sessions allowed to Principal. # This is here for at least two reasons: # 1. Ensure that the routes which list API keys and sessions, which are # not paginated, returns in a reasonable time. From 744f450c6266052979de776cfa628959f400b336 Mon Sep 17 00:00:00 2001 From: Dan Allan Date: Mon, 11 Jul 2022 12:19:19 -0400 Subject: [PATCH 4/4] Simplify test. --- tiled/_tests/test_authentication.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tiled/_tests/test_authentication.py b/tiled/_tests/test_authentication.py index 280029940..a67e3e4c2 100644 --- a/tiled/_tests/test_authentication.py +++ b/tiled/_tests/test_authentication.py @@ -353,15 +353,11 @@ def test_session_limit(enter_password, config): original_limit = authentication.SESSION_LIMIT authentication.SESSION_LIMIT = 10 try: - with enter_password("secret2"): - from_config(config, username="bob", token_cache={}) - with enter_password("secret1"): for _ in range(authentication.SESSION_LIMIT): from_config(config, username="alice", token_cache={}) - # Hit Session limit. - with fail_with_status_code(400): - with enter_password("secret1"): + # Hit Session limit. + with fail_with_status_code(400): from_config(config, username="alice", token_cache={}) finally: authentication.SESSION_LIMIT = original_limit