From 54a4a44bae61e6df5b24899518bff18355d595f3 Mon Sep 17 00:00:00 2001 From: NguyenND Date: Thu, 20 Aug 2026 14:24:41 -0400 Subject: [PATCH] Aug 20 - Session tenant binding --- app/__init__.py | 19 +++- app/api/decorators.py | 14 +++ app/api/jwt_utils.py | 15 ++++ app/models/user.py | 19 +++- app/routes/auth.py | 26 ++++++ app/tenancy/__init__.py | 11 +++ app/tenancy/engine_cache.py | 53 ++++++++++-- app/tenancy/middleware.py | 15 ++++ app/tenancy/session_binding.py | 154 +++++++++++++++++++++++++++++++++ config.py | 12 ++- 10 files changed, 325 insertions(+), 13 deletions(-) create mode 100644 app/tenancy/session_binding.py diff --git a/app/__init__.py b/app/__init__.py index d33bb8c..f0db58c 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -18,8 +18,25 @@ login_manager = LoginManager() migrate = Migrate() mail = Mail() csrf = CSRFProtect() # initialized here; .init_app() called in create_app() +def _rate_limit_key(): + """MT-21: scope rate-limit buckets per tenant as well as per client IP. + + With a bare remote-address key, two tenants behind the same NAT egress + share every route's counter, so one tenant's traffic can lock another out + of (for example) /auth/login. Falls back to the plain address in + single-tenant mode and on tenant-exempt paths, leaving today's buckets + unchanged there. + """ + from flask import g, has_request_context + addr = get_remote_address() + if not has_request_context(): + return addr + tenant = getattr(g, 'tenant', None) + return f't{tenant.id}|{addr}' if tenant is not None else addr + + limiter = Limiter( - key_func = get_remote_address, + key_func = _rate_limit_key, default_limits = [], # no global limit — applied per-route only # Use Redis when REDIS_URL is set in the environment (production multi-worker). # Falls back to in-process memory for local development (single-worker only; diff --git a/app/api/decorators.py b/app/api/decorators.py index 16014d6..03f88ac 100644 --- a/app/api/decorators.py +++ b/app/api/decorators.py @@ -62,6 +62,20 @@ def jwt_required(f): if payload is None: return api_error('Access token is invalid or expired', 401) + # MT-21: a token signed for another tenant verifies fine here (shared + # SECRET_KEY), so check the tenant claim before 'sub' is resolved + # against the bound database. No-op in single-tenant mode. + from app.tenancy.session_binding import current_tenant_id + tenant_id = current_tenant_id() + if tenant_id is not None: + token_tid = payload.get('tid') + if token_tid != tenant_id: + logger.warning( + 'API tenant mismatch | token_tid=%s resolved=%s endpoint=%s', + token_tid, tenant_id, request.endpoint, + ) + return api_error('Access token is not valid for this workspace', 401) + user_id = int(payload.get('sub', 0)) user = db.session.get(User, user_id) diff --git a/app/api/jwt_utils.py b/app/api/jwt_utils.py index bde55b2..f029e34 100644 --- a/app/api/jwt_utils.py +++ b/app/api/jwt_utils.py @@ -10,6 +10,7 @@ claims needed to identify the caller: { "sub": "42", # user.id as string "role": "inspector", # user.role + "tid": 3, # issuing tenant id (multi-tenant mode only) "iat": 1710000000, # issued-at (UTC epoch) "exp": 1710003600, # expiry (UTC epoch, 60 min later) } @@ -33,6 +34,12 @@ def _secret(): return current_app.config['SECRET_KEY'] +def _current_tenant_id(): + """Resolved tenant id, or None in single-tenant / unbound contexts.""" + from app.tenancy.session_binding import current_tenant_id + return current_tenant_id() + + def generate_access_token(user, lifetime_minutes: int = ACCESS_TOKEN_LIFETIME_MINUTES) -> str: """ Create and sign a new access token for the given user. @@ -54,6 +61,14 @@ def generate_access_token(user, lifetime_minutes: int = ACCESS_TOKEN_LIFETIME_MI 'iat': now, 'exp': now + timedelta(minutes=lifetime_minutes), } + # MT-21: bind the token to the issuing tenant. Every tenant is signed with + # the same SECRET_KEY, so without this claim a token minted at one tenant + # host verifies at another and 'sub' resolves against whichever database + # the middleware bound. Omitted in single-tenant mode so token shape is + # unchanged there. + tid = _current_tenant_id() + if tid is not None: + payload['tid'] = tid return jwt.encode(payload, _secret(), algorithm='HS256') diff --git a/app/models/user.py b/app/models/user.py index 713e9f0..3045e98 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -25,7 +25,15 @@ ROLE_LABELS = { @login_manager.user_loader def load_user(user_id): from app import db - return db.session.get(User, int(user_id)) + from app.tenancy.session_binding import parse_user_id + # MT-21: the identity string is tenant-tagged in multi-tenant mode. A tag + # naming another tenant (a session or remember-me cookie replayed onto this + # host) resolves to None here rather than loading the same-numbered user out + # of whichever database happens to be bound. + uid = parse_user_id(user_id) + if uid is None: + return None + return db.session.get(User, uid) class User(UserMixin, db.Model): __tablename__ = 'users' @@ -125,6 +133,15 @@ class User(UserMixin, db.Model): def is_active(self): return self.active + # MT-21: Flask-Login derives BOTH the session '_user_id' and the + # remember-me cookie payload from get_id(), and feeds both back through + # load_user(). Tagging the tenant here is therefore the single seam that + # binds every persisted identity to the tenant that issued it. Returns a + # bare id (today's format) whenever no tenant is bound. + def get_id(self): + from app.tenancy.session_binding import tag_user_id + return tag_user_id(self.id) + def set_password(self, password): self.password_hash = generate_password_hash(password) diff --git a/app/routes/auth.py b/app/routes/auth.py index f5d6121..c50e44e 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -8,6 +8,7 @@ from app.utils import mfa import logging from app.utils.audit import log_action, ACTION_CREATE, ACTION_UPDATE, ACTION_DELETE, ACTION_LOGIN, ACTION_LOGOUT from app.tenancy.gates import quota_soft_check +from app.tenancy.session_binding import bind_session_tenant, SESSION_TENANT_KEY logger = logging.getLogger(__name__) @@ -40,6 +41,7 @@ def login(): # second-factor step. Password is verified; identity is NOT yet # established until the code is confirmed at /auth/mfa. if user.mfa_enabled and user.mfa_secret: + bind_session_tenant() # MT-21: tag before identity enters the session session['mfa_pending_user_id'] = user.id session['mfa_pending_remember'] = bool(form.remember_me.data) session['mfa_pending_next'] = safe_redirect_url(request.args.get('next')) @@ -47,6 +49,7 @@ def login(): return redirect(url_for('auth.mfa_challenge')) login_user(user, remember=form.remember_me.data) + bind_session_tenant() # MT-21 # Use validated next URL — never redirect blindly to request.args['next'] next_page = safe_redirect_url(request.args.get('next')) log_action(ACTION_LOGIN, 'User', user.id, user.username) @@ -106,6 +109,7 @@ def mfa_challenge(): next_page = session.pop('mfa_pending_next', None) session.pop('mfa_pending_user_id', None) login_user(user, remember=remember) + bind_session_tenant() # MT-21 log_action(ACTION_LOGIN, 'User', user.id, user.username, f'2fa via {via}') if via == 'recovery': remaining_n = len(user.mfa_recovery_codes or []) @@ -948,6 +952,23 @@ def impersonate_entry(): flask_session['impersonating_tenant_id'] = tenant_id flask_session['impersonating_superadmin_id'] = superadmin_id + # ── MT-21: re-tag the session for the impersonated tenant ──────────────── + # Identity in the session is tenant-tagged (User.get_id), and from the next + # request onward the middleware binds the impersonated tenant's database. + # Without re-tagging, load_user() would correctly reject the tag issued by + # the host tenant and the superadmin would land on a login page. + # + # This preserves the pre-existing impersonation semantics EXACTLY: the + # numeric user id carries over, so the superadmin is loaded as the + # same-numbered user in the target tenant's database. That behaviour is + # arbitrary and worth revisiting (see MULTI_TENANT_PLAN.md open items) — + # but changing it is a separate decision, not a security fix. + flask_session[SESSION_TENANT_KEY] = tenant_id + raw_uid = flask_session.get('_user_id') + if raw_uid is not None: + numeric_uid = str(raw_uid).rpartition(':')[2] + flask_session['_user_id'] = f'{tenant_id}:{numeric_uid}' + logger.info('AUTH | impersonate_start | sa=%s tenant=%s', superadmin_id, tenant_id) import os @@ -971,6 +992,11 @@ def impersonate_end(): import os flask_session.pop('impersonating_tenant_id', None) flask_session.pop('impersonating_superadmin_id', None) + # MT-21: the session identity is still tagged for the impersonated tenant. + # Drop it rather than carrying it back to the superadmin's own host, where + # the middleware would clear it on tenant mismatch anyway. + logout_user() + flask_session.clear() panel_url = f"https://admin.{os.environ.get('TENANT_BASE_DOMAIN', 'jqc.app')}" logger.info('AUTH | impersonate_end | redirecting to panel') return redirect(panel_url) \ No newline at end of file diff --git a/app/tenancy/__init__.py b/app/tenancy/__init__.py index b782888..d4db71b 100644 --- a/app/tenancy/__init__.py +++ b/app/tenancy/__init__.py @@ -12,6 +12,10 @@ from app.tenancy.middleware import init_tenancy from app.tenancy.context import TenantContext from app.tenancy.gates import feature_required, quota_soft_check from app.tenancy.quota import check_quota, check_feature +from app.tenancy.session_binding import ( + bind_session_tenant, enforce_session_tenant, current_tenant_id, + tag_user_id, parse_user_id, SESSION_TENANT_KEY, +) __all__ = [ 'RoutingSession', @@ -21,4 +25,11 @@ __all__ = [ 'quota_soft_check', 'check_quota', 'check_feature', + # MT-21 + 'bind_session_tenant', + 'enforce_session_tenant', + 'current_tenant_id', + 'tag_user_id', + 'parse_user_id', + 'SESSION_TENANT_KEY', ] diff --git a/app/tenancy/engine_cache.py b/app/tenancy/engine_cache.py index f6d0b19..c557d01 100644 --- a/app/tenancy/engine_cache.py +++ b/app/tenancy/engine_cache.py @@ -5,40 +5,75 @@ Process-local cache of per-tenant SQLAlchemy engines, keyed by tenant id. Each tenant has its own database, hence its own engine + connection pool. Engines are created lazily on first use and reused across requests. Total -backend connections ≈ workers × cached-tenants × pool_size, so pool sizing is -a real scaling lever (see MULTI_TENANT_PLAN.md §4); tune via config, or set a -small pool / switch to NullPool when the tenant count grows large. +backend connections ≈ workers × cached-tenants × (pool_size + max_overflow), +so the cache is BOUNDED (MT-21): it holds at most +``TENANT_ENGINE_CACHE_MAX`` engines and evicts the least-recently-used one +beyond that, disposing it. Without the bound, a process that has served N +tenants holds N pools forever, and the connection count grows without limit +until MySQL's ``max_connections`` (default 151) rejects new sessions. + +Worked example — 8 workers, pool_size 2 + max_overflow 3, cache cap 32: + 8 × 32 × 5 = 1280 worst case, vs. unbounded before this change. + +Eviction disposes the engine, which closes its *idle* pooled connections. +Connections already checked out by another thread stay valid and are closed +when returned, so eviction is safe under concurrency; the evicted tenant +simply rebuilds its engine on the next request. `invalidate(tenant_id)` drops a cached engine (e.g. after credential rotation or tenant suspension); the next request rebuilds it. """ import threading +from collections import OrderedDict from flask import current_app from sqlalchemy import create_engine -_engines = {} +_engines = OrderedDict() _lock = threading.Lock() +# Fallback cap used when no application context is available (CLI/cron paths +# that touch the cache outside a request). Mirrors the config default. +_DEFAULT_CACHE_MAX = 32 + + +def _cache_max(): + try: + return int(current_app.config.get('TENANT_ENGINE_CACHE_MAX', _DEFAULT_CACHE_MAX)) + except Exception: + return _DEFAULT_CACHE_MAX + def get_tenant_engine(tenant): """Return (building if needed) the cached engine for a TenantContext.""" - engine = _engines.get(tenant.id) - if engine is not None: - return engine + evicted = [] with _lock: engine = _engines.get(tenant.id) if engine is None: engine = create_engine( tenant.db_uri, pool_pre_ping=True, - pool_size=current_app.config.get('TENANT_ENGINE_POOL_SIZE', 5), - max_overflow=current_app.config.get('TENANT_ENGINE_MAX_OVERFLOW', 5), + pool_size=current_app.config.get('TENANT_ENGINE_POOL_SIZE', 2), + max_overflow=current_app.config.get('TENANT_ENGINE_MAX_OVERFLOW', 3), pool_recycle=current_app.config.get('TENANT_ENGINE_POOL_RECYCLE', 1800), future=True, ) _engines[tenant.id] = engine + # Bound the cache: drop least-recently-used engines beyond the cap. + cap = _cache_max() + while cap > 0 and len(_engines) > cap: + old_id, old_engine = _engines.popitem(last=False) + evicted.append((old_id, old_engine)) + else: + _engines.move_to_end(tenant.id) + + # Dispose outside the lock — dispose() can block on socket teardown. + for old_id, old_engine in evicted: + try: + old_engine.dispose() + except Exception: + pass return engine diff --git a/app/tenancy/middleware.py b/app/tenancy/middleware.py index 1d44ec1..115f215 100644 --- a/app/tenancy/middleware.py +++ b/app/tenancy/middleware.py @@ -139,6 +139,13 @@ def init_tenancy(app): ) g.tenant = ctx g.tenant_engine = get_tenant_engine(ctx) + # MT-21: impersonation deliberately rebinds the session + # to the impersonated tenant, so stamp it rather than + # clearing it. /auth/impersonate has already written the + # same marker; this keeps it correct if the superadmin's + # target changes mid-session. + from app.tenancy.session_binding import bind_session_tenant + bind_session_tenant() return # skip normal Host resolution except Exception: logger.warning('TENANCY | impersonation_failed | tenant_id=%s', imp_id) @@ -156,6 +163,14 @@ def init_tenancy(app): g.tenant = tenant g.tenant_engine = get_tenant_engine(tenant) + # MT-21: a session cookie signed for a different tenant validates fine + # here — same app, same SECRET_KEY — so drop it before any downstream + # code reads identity out of it. Covers pre-auth session state + # (mfa_pending_user_id); the tenant tag in User.get_id() covers the + # authenticated session and the remember-me cookie. + from app.tenancy.session_binding import enforce_session_tenant + enforce_session_tenant() + @app.before_request def _billing_gate(): """MT-8: Enforce subscription status. Inert when BILLING_ENABLED=False.""" diff --git a/app/tenancy/session_binding.py b/app/tenancy/session_binding.py new file mode 100644 index 0000000..a00add2 --- /dev/null +++ b/app/tenancy/session_binding.py @@ -0,0 +1,154 @@ +""" +app/tenancy/session_binding.py +------------------------------ +MT-21: bind a browser session to the tenant that issued it. + +Why this exists +--------------- +Every tenant is served by the same Flask application with the same +``SECRET_KEY``. Before this module, nothing in a signed cookie identified +*which* tenant it was issued by, so a cookie minted on one tenant host +validated perfectly on another: + + 1. A legitimate user of tenant A logs in at a.jqc.app. + 2. They copy their session cookie onto b.jqc.app (devtools / curl — + host-only cookie scoping stops the *browser* replaying it, but not a + person doing it by hand). + 3. The tenancy middleware resolves Host → tenant B and binds tenant B's + database. + 4. Flask-Login calls load_user('7') and gets **tenant B's user #7**. + +The signature was always valid, because it is the same key. The identity was +never checked against the tenant. Result: authentication as an arbitrary user +in any tenant whose hostname the attacker knows. + +Two layers close this, and both live here: + +``SESSION_TENANT_KEY`` + A ``_tenant_id`` marker written into the session at every point where the + session starts carrying identity (login, MFA challenge hand-off, + impersonation). ``enforce_session_tenant()`` clears the whole session when + that marker disagrees with the resolved tenant, so pre-authentication + session state (``mfa_pending_user_id`` and friends) cannot cross tenants + either. + +``tag_user_id`` / ``parse_user_id`` + Flask-Login derives BOTH the session ``_user_id`` and the "remember me" + cookie payload from ``User.get_id()``, and feeds both back through + ``user_loader``. Tagging the id there — ``":"`` — is + therefore a single seam that covers both cookies. Clearing the session + alone would not have been enough: a remember-me cookie repopulates the + session immediately afterwards. + +Single-tenant behaviour is unchanged. When ``MULTI_TENANT_ENABLED`` is False, +or no tenant is bound (cron, CLI, exempt paths), ids stay bare integers and +the enforcement is a no-op. +""" + +import logging + +from flask import current_app, g, session + +logger = logging.getLogger(__name__) + +SESSION_TENANT_KEY = '_tenant_id' + + +def current_tenant_id(): + """Resolved tenant id for this request, or None in single-tenant mode.""" + if not current_app.config.get('MULTI_TENANT_ENABLED', False): + return None + tenant = getattr(g, 'tenant', None) + return tenant.id if tenant is not None else None + + +def bind_session_tenant(): + """Stamp the current session with the tenant that issued it. + + Called at every point that puts identity into the session. No-op in + single-tenant mode so existing sessions keep working untouched. + """ + tid = current_tenant_id() + if tid is not None: + session[SESSION_TENANT_KEY] = tid + + +def enforce_session_tenant(): + """Clear the session if it was issued by a different tenant. + + Returns True when the session was cleared. Runs after tenant resolution, + from the tenancy middleware. + """ + tid = current_tenant_id() + if tid is None: + return False + + bound = session.get(SESSION_TENANT_KEY) + + if bound is None: + # An untagged session carrying identity predates this binding, or was + # lifted from somewhere else. Either way it cannot be trusted here. + if '_user_id' in session or 'mfa_pending_user_id' in session: + logger.warning('TENANCY | session_untagged_cleared | tenant=%s', tid) + session.clear() + return True + return False + + if bound != tid: + logger.warning('TENANCY | session_tenant_mismatch | bound=%s resolved=%s', + bound, tid) + session.clear() + return True + + return False + + +def tag_user_id(user_id): + """Render a Flask-Login identity string, tenant-tagged when applicable. + + Used by ``User.get_id()``. Feeds both the session ``_user_id`` and the + remember-me cookie. + """ + tid = current_tenant_id() + if tid is None: + return str(user_id) + return f'{tid}:{user_id}' + + +def parse_user_id(raw): + """Inverse of :func:`tag_user_id`, with the tenant check applied. + + Returns the integer user id, or None when the identity must be rejected — + a foreign tenant tag, an untagged id arriving in multi-tenant mode, or + anything unparseable. ``user_loader`` turns None into an anonymous user, + which sends the caller back to the login page. + """ + if raw is None: + return None + raw = str(raw) + tid = current_tenant_id() + + if ':' in raw: + tag, _, uid = raw.partition(':') + if tid is None: + # Tagged id replayed at a single-tenant / unbound context. + return None + try: + if int(tag) != tid: + logger.warning('TENANCY | user_id_tenant_mismatch | tag=%s resolved=%s', + tag, tid) + return None + except (TypeError, ValueError): + return None + else: + uid = raw + if tid is not None: + # Untagged id in multi-tenant mode: either a session predating + # MT-21 or one lifted from another host. Force re-authentication. + logger.warning('TENANCY | user_id_untagged_rejected | tenant=%s', tid) + return None + + try: + return int(uid) + except (TypeError, ValueError): + return None diff --git a/config.py b/config.py index bda33c7..b0fa95d 100644 --- a/config.py +++ b/config.py @@ -53,9 +53,17 @@ class Config: # by the provisioner to build . subdomains. TENANT_BASE_DOMAIN = os.environ.get('TENANT_BASE_DOMAIN', 'jqc.app') # Per-tenant SQLAlchemy engine pool tuning (see MULTI_TENANT_PLAN.md §4). - TENANT_ENGINE_POOL_SIZE = int(os.environ.get('TENANT_ENGINE_POOL_SIZE', 5)) - TENANT_ENGINE_MAX_OVERFLOW = int(os.environ.get('TENANT_ENGINE_MAX_OVERFLOW', 5)) + # MT-21: defaults lowered from 5/5 to 2/3. Backend connections scale as + # workers × cached-tenants × (pool_size + max_overflow); at the old + # defaults 8 workers × 32 tenants would reserve 2560 connections against a + # MySQL max_connections that defaults to 151. + TENANT_ENGINE_POOL_SIZE = int(os.environ.get('TENANT_ENGINE_POOL_SIZE', 2)) + TENANT_ENGINE_MAX_OVERFLOW = int(os.environ.get('TENANT_ENGINE_MAX_OVERFLOW', 3)) TENANT_ENGINE_POOL_RECYCLE = int(os.environ.get('TENANT_ENGINE_POOL_RECYCLE', 1800)) + # MT-21: hard cap on cached per-tenant engines per worker process. Beyond + # this the least-recently-used engine is disposed and rebuilt on demand. + # 0 disables the bound (previous behaviour — not recommended). + TENANT_ENGINE_CACHE_MAX = int(os.environ.get('TENANT_ENGINE_CACHE_MAX', 32)) # ── File uploads ──────────────────────────────────────────────────────── UPLOAD_FOLDER = os.path.join(basedir, 'app/static/uploads')