diff --git a/forail/main/migrations/0211_rls_scoped_null_org.py b/forail/main/migrations/0211_rls_scoped_null_org.py new file mode 100644 index 0000000..3f5f329 --- /dev/null +++ b/forail/main/migrations/0211_rls_scoped_null_org.py @@ -0,0 +1,53 @@ +"""Multi-Tenancy v2: stop treating every NULL organization as globally shared. + +Codex M6 — the policies emitted ``OR organization_id IS NULL`` for every table, +so a row saved without an organization was readable by every tenant. Whether a +NULL organization means "shared with the platform" is now decided per table by +``RLS_GLOBAL_NULL_ORG_TABLES``; the rest are scoped strictly. + +This changes visibility for existing rows: on a tenant-scoped request, rows with +no organization in a strictly-scoped table stop being returned. They are not +deleted and remain visible to superusers and to any request without a tenant +context. If a resource "disappears" for a tenant after this migration, the fix +is to assign its organization -- not to add its table to the global list. + +Idempotent: each policy is dropped (IF EXISTS) then recreated, for both direct +and indirect tables, so this converges on fresh and existing databases. +""" + +from django.db import migrations + +from forail.main.tenancy.helpers import ( + RLS_TABLES_DIRECT, + RLS_TABLES_INDIRECT, + build_rls_policy_sql, + build_rls_policy_sql_indirect, +) + + +def _rebuild_sql(): + statements = [] + for table, org_col in RLS_TABLES_DIRECT: + create, drop = build_rls_policy_sql(table, org_col) + statements.append(f'ALTER TABLE {table} ENABLE ROW LEVEL SECURITY;') + statements.append(f'ALTER TABLE {table} FORCE ROW LEVEL SECURITY;') + statements.append(drop) + statements.append(create) + for table, fk_col, parent_table, parent_org_col in RLS_TABLES_INDIRECT: + create, drop = build_rls_policy_sql_indirect(table, fk_col, parent_table, parent_org_col) + statements.append(f'ALTER TABLE {table} ENABLE ROW LEVEL SECURITY;') + statements.append(f'ALTER TABLE {table} FORCE ROW LEVEL SECURITY;') + statements.append(drop) + statements.append(create) + return '\n'.join(statements) + + +class Migration(migrations.Migration): + + dependencies = [ + ('main', '0210_rls_nullif_cast'), + ] + + operations = [ + migrations.RunSQL(sql=_rebuild_sql(), reverse_sql=migrations.RunSQL.noop), + ] diff --git a/forail/main/observability/bootstrap.py b/forail/main/observability/bootstrap.py index 96f5a32..a068d4b 100644 --- a/forail/main/observability/bootstrap.py +++ b/forail/main/observability/bootstrap.py @@ -23,9 +23,22 @@ 'OTEL_TRACES_SAMPLER', 'OTEL_TRACES_SAMPLER_ARG', 'TENANCY_ENABLED', + # Codex M2: TENANCY_ENABLED synced but the controls that actually enforce + # isolation did not, so a deployment could set the one flag it was offered + # and get tenancy features with no row-level security behind them. Turning + # tenancy on through the environment now reaches the whole set. + 'TENANCY_RLS_ENABLED', + 'TENANCY_STRICT_ISOLATION_ENABLED', + 'TENANCY_RATE_LIMITING_ENABLED', ) -_BOOL_KEYS = ('OTEL_ENABLED', 'TENANCY_ENABLED') +_BOOL_KEYS = ( + 'OTEL_ENABLED', + 'TENANCY_ENABLED', + 'TENANCY_RLS_ENABLED', + 'TENANCY_STRICT_ISOLATION_ENABLED', + 'TENANCY_RATE_LIMITING_ENABLED', +) def _coerce_env_value(key, raw): diff --git a/forail/main/tenancy/helpers.py b/forail/main/tenancy/helpers.py index 6175475..c9dadfe 100644 --- a/forail/main/tenancy/helpers.py +++ b/forail/main/tenancy/helpers.py @@ -232,12 +232,11 @@ def tenant_queue_name(org_id): # that references Organization.id — directly ('organization_id') for most # tables or via a subquery for indirect relationships. # -# Tables with nullable organization_id are included; the RLS policy handles -# NULLs by treating them as "visible to everyone" (no tenant scope). This is -# intentional for AWX's genuinely global, org-less resources (a credential or -# label created with organization=NULL is shared platform-wide). Do NOT add a -# table here whose rows are always tenant-owned unless it forbids NULL org, or -# those NULL rows would leak across every tenant (see needtofix M5). +# Tables with nullable organization_id are included. Whether a NULL organization +# means "shared with the whole platform" or "nobody set it" is decided per table +# by RLS_GLOBAL_NULL_ORG_TABLES below -- it used to be assumed to mean the first +# for every table, which is what made a single missed assignment enough to turn +# a tenant-owned row into cross-tenant data (see needtofix M5, Codex M6). # # Coverage note (needtofix M4): several models that *look* org-scoped # (Project, WorkflowJobTemplate, Schedule, workflow nodes, job events) do not @@ -287,12 +286,51 @@ def tenant_queue_name(org_id): ] +# Tables where `organization_id IS NULL` is a deliberate sharing or ownership +# mechanism inherited from AWX, and the row really is meant to be visible +# outside any one tenant: +# +# main_credential NULL org = a user-owned (personal) credential +# main_oauth2application NULL org = a user-owned application +# main_executionenvironment NULL org = a globally shared execution environment +# main_unifiedjobtemplate system job templates carry no organization +# main_unifiedjob ...and neither do their runs +# +# Everything else is scoped strictly: a NULL organization there is an unset +# field, not a shared resource, and the row stays invisible to a tenant-scoped +# request. Hidden is the safe direction for an ambiguous row; visible-to-all is +# not. An unscoped request (superuser, or no tenant context) still sees it, so +# nothing becomes unreachable -- and every Forail-authored table is in this +# group, which is where an org is assigned by our own code rather than by AWX's +# long-standing conventions. +# +# Adding a table here is a decision that its NULL rows may be read by every +# tenant. Do not add one to silence a "resource disappeared" report; assign the +# organization instead. +RLS_GLOBAL_NULL_ORG_TABLES = frozenset( + { + 'main_credential', + 'main_oauth2application', + 'main_executionenvironment', + 'main_unifiedjobtemplate', + 'main_unifiedjob', + } +) + + +def null_org_is_global(table): + """Whether rows of ``table`` with no organization are visible to every tenant.""" + return table in RLS_GLOBAL_NULL_ORG_TABLES + + def build_rls_policy_sql(table, org_column='organization_id'): """Return (create_sql, drop_sql) for a permissive RLS policy. The policy allows the row when: 1. ``organization_id`` matches ``forail.current_tenant_id``, OR - 2. ``organization_id`` IS NULL (global/shared resources), OR + 2. ``organization_id`` IS NULL **and** this table is listed in + ``RLS_GLOBAL_NULL_ORG_TABLES`` (a NULL org is a sharing mechanism there, + not an unset field), OR 3. The session variable is empty / unset (no tenant context — backwards compatible for non-tenant requests and superusers). """ @@ -303,12 +341,13 @@ def build_rls_policy_sql(table, org_column='organization_id'): # before the cast is evaluated. NULLIF collapses '' to NULL, which casts # cleanly and is caught by the IS NULL branch (→ all rows visible). tenant = "NULLIF(current_setting('forail.current_tenant_id', true), '')" + null_branch = f'OR {org_column} IS NULL ' if null_org_is_global(table) else '' create = ( f'CREATE POLICY {policy_name} ON {table} ' f'AS PERMISSIVE FOR ALL ' f'USING (' f'{org_column} = {tenant}::int ' - f'OR {org_column} IS NULL ' + f'{null_branch}' f'OR {tenant} IS NULL' f');' ) @@ -319,10 +358,17 @@ def build_rls_policy_sql(table, org_column='organization_id'): def build_rls_policy_sql_indirect(table, fk_column, parent_table, parent_org_column): """Return (create_sql, drop_sql) for an indirect RLS policy. - Uses a subquery to resolve the organization from a parent table. + Uses a subquery to resolve the organization from a parent table. Whether a + NULL organization on the *parent* is global is decided by the parent's entry + in ``RLS_GLOBAL_NULL_ORG_TABLES``, since that is the row that carries it. + + ``{fk_column} IS NULL`` stays unconditionally: a row with no parent at all + has no organization to compare against, and hiding it would make orphans + unreachable rather than merely unscoped. """ policy_name = f'tenant_isolation_{table}' tenant = "NULLIF(current_setting('forail.current_tenant_id', true), '')" + parent_null_branch = f'OR {parent_org_column} IS NULL' if null_org_is_global(parent_table) else '' create = ( f'CREATE POLICY {policy_name} ON {table} ' f'AS PERMISSIVE FOR ALL ' @@ -330,7 +376,7 @@ def build_rls_policy_sql_indirect(table, fk_column, parent_table, parent_org_col f'{fk_column} IN (' f'SELECT id FROM {parent_table} WHERE ' f'{parent_org_column} = {tenant}::int ' - f'OR {parent_org_column} IS NULL' + f'{parent_null_branch}' f') ' f'OR {fk_column} IS NULL ' f'OR {tenant} IS NULL' diff --git a/forail/main/tenancy/isolation.py b/forail/main/tenancy/isolation.py index ea2dd4f..f1608ef 100644 --- a/forail/main/tenancy/isolation.py +++ b/forail/main/tenancy/isolation.py @@ -73,9 +73,25 @@ def __call__(self, request): try: tenant_org = self._resolve_tenant_org(request) except Exception: - # Resolving the user's tenant org failed. A user we cannot place - # into a tenant is treated as unscoped (superuser / non-tenant), - # which is the pre-existing contract for a None result. + # Fail CLOSED, for the same reason set_tenant_id does below. + # + # This used to swallow the error and continue with tenant_org=None, + # which __call__ cannot tell apart from "superuser / not a tenant + # user" -- so a transient database error during the org lookup let + # the request run with NO RLS scope, and RLS reads an unset tenant + # id as "all rows". A failed lookup is not evidence that the user + # has no tenant; it is evidence that we do not know. + # + # Only meaningful when tenancy and RLS are on: with either off, + # _resolve_tenant_org returns before it touches the database, so an + # exception here cannot come from the lookup and a single-tenant + # install is not turned into a 500. + if getattr(settings, 'TENANCY_ENABLED', False) and getattr(settings, 'TENANCY_RLS_ENABLED', False): + logger.exception('TenantIsolationMiddleware: tenant org resolution failed — failing closed') + return JsonResponse( + {'detail': 'Tenant isolation could not be established.'}, + status=500, + ) logger.debug('TenantIsolationMiddleware: tenant org resolution failed', exc_info=True) tenant_org = None @@ -214,14 +230,14 @@ def _resolve_tenant_org(request): if getattr(user, 'is_superuser', False): return None - try: - orgs = list( - user.organizations.filter(is_tenant_root=True) - .only('pk', 'is_tenant_root', 'tenant_isolation_strict')[:1] - ) - except Exception: - logger.debug('_resolve_tenant_org: org lookup failed', exc_info=True) - return None + # Deliberately not wrapped in try/except. A database error here must + # reach __call__, which fails the request closed; returning None would + # make "the lookup broke" indistinguishable from "this user has no + # tenant", and the second answer runs the request unscoped. + orgs = list( + user.organizations.filter(is_tenant_root=True) + .only('pk', 'is_tenant_root', 'tenant_isolation_strict')[:1] + ) return orgs[0] if orgs else None diff --git a/tests_standalone/test_observability.py b/tests_standalone/test_observability.py index 9d37f42..b7f54c6 100644 --- a/tests_standalone/test_observability.py +++ b/tests_standalone/test_observability.py @@ -22,6 +22,7 @@ def _load(mod_name, rel_path): helpers = _load('obs_helpers', 'forail/main/observability/helpers.py') +bootstrap = _load('obs_bootstrap', 'forail/main/observability/bootstrap.py') parse_resource_attributes = helpers.parse_resource_attributes parse_endpoint = helpers.parse_endpoint @@ -152,3 +153,35 @@ def test_should_recheck_is_inverse(self): if __name__ == '__main__': unittest.main() + + +class TestEnvSyncKeys(unittest.TestCase): + """Which settings a deployment can drive from the environment. + + Codex M2: the chart offered one tenancy switch, and only that one was + mirrored into the Setting registry -- so an operator who turned tenancy on + got the tenancy features with no row-level security behind them, because + TENANCY_RLS_ENABLED stayed at its False default with no way to reach it. + """ + + TENANCY_KEYS = ( + 'TENANCY_ENABLED', + 'TENANCY_RLS_ENABLED', + 'TENANCY_STRICT_ISOLATION_ENABLED', + 'TENANCY_RATE_LIMITING_ENABLED', + ) + + def test_every_tenancy_control_is_syncable(self): + for key in self.TENANCY_KEYS: + self.assertIn(key, bootstrap._ENV_SYNC_KEYS, f'{key} cannot be set from the environment') + + def test_every_tenancy_control_is_parsed_as_a_boolean(self): + # Without this the Setting row stores the string "false", which is + # truthy everywhere it is read. + for key in self.TENANCY_KEYS: + self.assertIn(key, bootstrap._BOOL_KEYS) + self.assertIs(bootstrap._coerce_env_value(key, 'false'), False) + self.assertIs(bootstrap._coerce_env_value(key, 'true'), True) + + def test_non_bool_keys_stay_strings(self): + self.assertEqual(bootstrap._coerce_env_value('OTEL_SERVICE_NAME', 'forail'), 'forail') diff --git a/tests_standalone/test_tenancy.py b/tests_standalone/test_tenancy.py index 2943e03..0f1cb97 100644 --- a/tests_standalone/test_tenancy.py +++ b/tests_standalone/test_tenancy.py @@ -197,6 +197,8 @@ def test_non_dict(self): build_rls_policy_sql_indirect = helpers.build_rls_policy_sql_indirect RLS_TABLES_DIRECT = helpers.RLS_TABLES_DIRECT RLS_TABLES_INDIRECT = helpers.RLS_TABLES_INDIRECT +RLS_GLOBAL_NULL_ORG_TABLES = helpers.RLS_GLOBAL_NULL_ORG_TABLES +null_org_is_global = helpers.null_org_is_global class TestBuildRlsPolicySql(unittest.TestCase): @@ -221,8 +223,46 @@ def test_create_contains_bypass_clauses(self): create, _ = build_rls_policy_sql('main_inventory', 'organization_id') # Bypass when session var is unset/empty — NULLIF('') collapses to NULL. self.assertIn("NULLIF(current_setting('forail.current_tenant_id', true), '') IS NULL", create) - # Bypass when org column is NULL (shared resources) - self.assertIn('organization_id IS NULL', create) + + def test_null_org_bypass_only_for_globally_shared_tables(self): + # Codex M6: this branch used to be emitted for every table, so one row + # saved without an organization was readable by every tenant. + shared, _ = build_rls_policy_sql('main_credential', 'organization_id') + scoped, _ = build_rls_policy_sql('main_scanner', 'organization_id') + self.assertIn('OR organization_id IS NULL', shared) + self.assertNotIn('OR organization_id IS NULL', scoped) + + def test_every_forail_table_is_scoped_strictly(self): + # The Forail-authored tables assign the organization in our own code, + # so a NULL there is an unset field rather than AWX's sharing mechanism. + forail_tables = [ + 'main_eventrule', 'main_outboundwebhook', 'main_eventlog', + 'main_hostfactsnapshot', 'main_driftdetection', 'main_driftalertrule', + 'main_driftalert', 'main_policy', 'main_policydecision', 'main_scanner', + 'main_scanresult', 'main_servicecatalogitem', 'main_auditevent', + ] + for table in forail_tables: + self.assertFalse(null_org_is_global(table), f'{table} must not expose NULL-org rows') + create, _ = build_rls_policy_sql(table, 'organization_id') + self.assertNotIn('OR organization_id IS NULL', create) + + def test_global_tables_are_a_closed_list(self): + # Every entry is a deliberate decision that NULL-org rows may be read by + # any tenant; the assertion is here so growing the list is not quiet. + self.assertEqual( + RLS_GLOBAL_NULL_ORG_TABLES, + frozenset({ + 'main_credential', + 'main_oauth2application', + 'main_executionenvironment', + 'main_unifiedjobtemplate', + 'main_unifiedjob', + }), + ) + + def test_every_global_table_is_actually_under_rls(self): + covered = {table for table, _ in RLS_TABLES_DIRECT} + self.assertTrue(RLS_GLOBAL_NULL_ORG_TABLES.issubset(covered)) def test_create_is_permissive(self): create, _ = build_rls_policy_sql('main_inventory', 'organization_id') @@ -265,6 +305,19 @@ def test_create_contains_bypass(self): # needtofix L6: NULLIF-guarded empty/unset sentinel (was `= ''`). self.assertIn("NULLIF(current_setting('forail.current_tenant_id', true), '') IS NULL", create) + def test_parent_null_org_follows_the_parent_table(self): + # A host is visible through its inventory, so whether a NULL org is + # global is the inventory's question, not the host's. main_inventory is + # not globally shared, so hosts of an org-less inventory stay hidden + # from a scoped tenant. + create, _ = build_rls_policy_sql_indirect( + 'main_host', 'inventory_id', 'main_inventory', 'organization_id' + ) + self.assertNotIn('OR organization_id IS NULL)', create) + # An orphan row keeps its bypass: no parent means no organization to + # compare, and hiding it would make it unreachable rather than unscoped. + self.assertIn('OR inventory_id IS NULL', create) + def test_drop_contains_policy_name(self): _, drop = build_rls_policy_sql_indirect( 'main_host', 'inventory_id', 'main_inventory', 'organization_id' diff --git a/tests_standalone/test_tenant_isolation.py b/tests_standalone/test_tenant_isolation.py new file mode 100644 index 0000000..47c29fc --- /dev/null +++ b/tests_standalone/test_tenant_isolation.py @@ -0,0 +1,182 @@ +"""Standalone tests for the tenant isolation middleware. + +Loads forail/main/tenancy/isolation.py directly with Django and the RLS module +stubbed -- no database, no settings module. The point of interest is what the +middleware does when it *cannot* determine a request's tenant: RLS reads an +unset tenant id as "every row", so the difference between "this user has no +tenant" and "the lookup failed" is the difference between a scoped request and +a global one. +""" + +import importlib.util +import os +import sys +import types +import unittest +from unittest.mock import MagicMock + + +def _load(mod_name, rel_path): + path = os.path.abspath(os.path.join(os.path.dirname(__file__), '..', rel_path)) + spec = importlib.util.spec_from_file_location(mod_name, path) + mod = importlib.util.module_from_spec(spec) + sys.modules[mod_name] = mod + spec.loader.exec_module(mod) + return mod + + +class _Settings: + """Stands in for django.conf.settings; only the flags read here matter.""" + + def __init__(self, **flags): + self.__dict__.update(flags) + + +class _JsonResponse: + """Records what the middleware answered instead of running the view.""" + + def __init__(self, payload, status=200): + self.payload = payload + self.status_code = status + + +settings_holder = _Settings() + +django = types.ModuleType('django') +django_conf = types.ModuleType('django.conf') +django_conf.settings = settings_holder +django_http = types.ModuleType('django.http') +django_http.JsonResponse = _JsonResponse +sys.modules.setdefault('django', django) +sys.modules['django.conf'] = django_conf +sys.modules['django.http'] = django_http + +# The RLS module talks to Postgres; the middleware only needs the two calls. +set_tenant_id = MagicMock() +clear_tenant_id = MagicMock() +rls = types.ModuleType('forail.main.tenancy.rls') +rls.set_tenant_id = set_tenant_id +rls.clear_tenant_id = clear_tenant_id + +for pkg in ('forail', 'forail.main', 'forail.main.tenancy'): + sys.modules.setdefault(pkg, types.ModuleType(pkg)) +sys.modules['forail.main.tenancy.rls'] = rls +_load('forail.main.tenancy.helpers', 'forail/main/tenancy/helpers.py') + +isolation = _load('forail.main.tenancy.isolation', 'forail/main/tenancy/isolation.py') +TenantIsolationMiddleware = isolation.TenantIsolationMiddleware + + +def tenant_user(is_superuser=False): + user = MagicMock() + user.is_authenticated = True + user.is_superuser = is_superuser + return user + + +def request_for(user): + request = MagicMock() + request.user = user + request.path = '/api/v2/job_templates/' + return request + + +class TenantResolutionFailure(unittest.TestCase): + """H3: a failed lookup must not read as 'this user has no tenant'.""" + + def setUp(self): + set_tenant_id.reset_mock() + clear_tenant_id.reset_mock() + settings_holder.__dict__.clear() + settings_holder.TENANCY_ENABLED = True + settings_holder.TENANCY_RLS_ENABLED = True + self.view = MagicMock(return_value='view-response') + self.mw = TenantIsolationMiddleware(self.view) + + def _fail_resolution(self): + self.mw._resolve_tenant_org = MagicMock(side_effect=RuntimeError('database is down')) + + def test_failed_lookup_does_not_run_the_view(self): + self._fail_resolution() + response = self.mw(request_for(tenant_user())) + self.assertEqual(response.status_code, 500) + self.view.assert_not_called() + + def test_failed_lookup_never_installs_a_scope(self): + self._fail_resolution() + self.mw(request_for(tenant_user())) + set_tenant_id.assert_not_called() + + def test_failed_lookup_is_ignored_when_tenancy_is_off(self): + # A single-tenant install has no scope to lose, and must not start + # answering 500 because of this gate. + settings_holder.TENANCY_ENABLED = False + self._fail_resolution() + self.assertEqual(self.mw(request_for(tenant_user())), 'view-response') + + def test_failed_lookup_is_ignored_when_rls_is_off(self): + settings_holder.TENANCY_RLS_ENABLED = False + self._fail_resolution() + self.assertEqual(self.mw(request_for(tenant_user())), 'view-response') + + def test_resolver_lets_a_database_error_out(self): + # The middleware can only fail closed if the lookup stops swallowing. + user = tenant_user() + user.organizations.filter.side_effect = RuntimeError('database is down') + request = request_for(user) + with self.assertRaises(RuntimeError): + TenantIsolationMiddleware._resolve_tenant_org(request) + + +class TenantResolutionSuccess(unittest.TestCase): + """The paths that must keep working unchanged.""" + + def setUp(self): + set_tenant_id.reset_mock() + clear_tenant_id.reset_mock() + settings_holder.__dict__.clear() + settings_holder.TENANCY_ENABLED = True + settings_holder.TENANCY_RLS_ENABLED = True + self.view = MagicMock(return_value='view-response') + self.mw = TenantIsolationMiddleware(self.view) + + def test_resolved_org_scopes_the_request(self): + org = MagicMock(pk=42) + self.mw._resolve_tenant_org = MagicMock(return_value=org) + request = request_for(tenant_user()) + self.assertEqual(self.mw(request), 'view-response') + set_tenant_id.assert_called_once_with(42) + clear_tenant_id.assert_called_once() + self.assertIs(request._tenant_org, org) + + def test_no_tenant_org_runs_unscoped(self): + # A superuser or a non-tenant user legitimately has no scope. + self.mw._resolve_tenant_org = MagicMock(return_value=None) + self.assertEqual(self.mw(request_for(tenant_user(is_superuser=True))), 'view-response') + set_tenant_id.assert_not_called() + + def test_set_tenant_id_failure_still_fails_closed(self): + # Pre-existing behaviour, kept under test alongside the new path. + self.mw._resolve_tenant_org = MagicMock(return_value=MagicMock(pk=7)) + set_tenant_id.side_effect = RuntimeError('cannot set session variable') + try: + response = self.mw(request_for(tenant_user())) + finally: + set_tenant_id.side_effect = None + self.assertEqual(response.status_code, 500) + self.view.assert_not_called() + + def test_superuser_resolver_returns_none_without_a_query(self): + user = tenant_user(is_superuser=True) + self.assertIsNone(TenantIsolationMiddleware._resolve_tenant_org(request_for(user))) + user.organizations.filter.assert_not_called() + + def test_resolver_returns_none_when_tenancy_is_off(self): + settings_holder.TENANCY_ENABLED = False + user = tenant_user() + self.assertIsNone(TenantIsolationMiddleware._resolve_tenant_org(request_for(user))) + user.organizations.filter.assert_not_called() + + +if __name__ == '__main__': + unittest.main()