diff --git a/CHANGELOG.md b/CHANGELOG.md index 1e0d64d..a309349 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,29 @@ and the project adheres to CalVer (`YYYY.MM.PATCH`). - Removed dead, unregistered `ForailOIDCAuth` backend to avoid implying a second active OIDC backend. OIDC is handled by `social_core`'s `OpenIdConnectAuth`, whose requests honor `SOCIAL_AUTH_OIDC_VERIFY_SSL` (verified). +- **Tenant isolation fails closed**: the RLS middleware now aborts a request + (HTTP 500) if it cannot install the tenant scope, instead of proceeding with + global row visibility. The strict-isolation gate resolves the target org with + the caller's RLS scope removed (so cross-tenant objects are actually visible) + and defaults to deny when a covered resource's org cannot be determined. +- **RLS coverage + robustness**: added a policy for `main_eventlog` (it carries + its own `organization_id`); RLS policies now cast the tenant GUC via + `NULLIF(current_setting(...), '')::int` so the empty "no scope" sentinel can't + raise. New migrations `0209`, `0210` (idempotent). +- **`import_from_awx` trust boundary**: superuser / system-role promotion from + the source is gated behind `--grant-superusers` (off by default, logged); + custom credential-type injectors are dropped for admin re-approval unless + `--trust-injectors` is given; secrets are read from `AWX_TOKEN` / `AWX_PASSWORD` + in preference to argv. +- **SSO account-takeover fix**: `associate_by_email` removed from the auth + pipeline — accounts associate by provider UID, not by matching email address. +- **Tenant provisioning** refuses to silently reuse an existing username (which + discarded the supplied password and cross-linked accounts) unless + `attach_existing_admin` is set. +- **IaC scanner path traversal**: a job template's `playbook` field can no + longer point the scanner outside the project checkout (absolute / `..`). +- **Rate limiter** logs Redis outages loudly and honours a new + `TENANCY_RATE_LIMIT_FAIL_CLOSED` setting (default open for availability). ### Fixed - `pytest.ini` pointed `DJANGO_SETTINGS_MODULE` at the pre-rename diff --git a/docs/12-configuration-reference.md b/docs/12-configuration-reference.md index 129771e..ddaacc6 100644 --- a/docs/12-configuration-reference.md +++ b/docs/12-configuration-reference.md @@ -109,6 +109,36 @@ Access via API: `/api/v2/settings/` | `LOG_AGGREGATOR_ENABLED` | `False` | Enable log forwarding | | `LOG_AGGREGATOR_LEVEL` | `WARNING` | Minimum level | +### Multi-tenancy & isolation (`/api/v2/settings/system/`) + +Every isolation control is **off by default** to preserve single-tenant +backwards compatibility. A multi-tenant deployment must enable at least +`TENANCY_ENABLED` + `TENANCY_RLS_ENABLED`, and `TENANCY_STRICT_ISOLATION_ENABLED` +for hard cross-tenant blocking (vs audit-only). + +| Setting | Default | Description | +| ----------------------------------- | ------- | ------------------------------------------------------------------ | +| `TENANCY_ENABLED` | `False` | Master switch for the multi-tenancy feature set | +| `TENANCY_RLS_ENABLED` | `False` | Set the per-request Postgres RLS tenant scope | +| `TENANCY_STRICT_ISOLATION_ENABLED` | `False` | Block cross-tenant API access (HTTP 403) instead of audit-only | +| `TENANCY_RATE_LIMITING_ENABLED` | `False` | Per-tenant token-bucket API rate limiting (needs Redis) | +| `TENANCY_RATE_LIMIT_FAIL_CLOSED` | `False` | Reject requests (503) if the rate-limiter Redis is down, vs allow | +| `TENANCY_DEDICATED_QUEUES_ENABLED` | `False` | Route tenant jobs to dedicated `tenant-{org_id}` Celery queues | + +> When RLS is on, tenant isolation **fails closed**: if the tenant scope cannot +> be installed for a request, the request is aborted rather than run with global +> row visibility. + +### `import_from_awx` security flags + +The AWX importer treats the source as untrusted by default: + +| Flag | Effect | +| -------------------- | -------------------------------------------------------------------------- | +| `--grant-superusers` | Honour `is_superuser` / system-role grants from the source (off by default) | +| `--trust-injectors` | Import custom credential-type injectors verbatim (off; else re-approve) | +| `AWX_TOKEN` / `AWX_PASSWORD` | Preferred over `--token` / `--password` (argv leaks via `ps`/`/proc`) | + --- ## Common Configuration Tasks diff --git a/docs/22-multi-tenancy.md b/docs/22-multi-tenancy.md index a976bf2..a83999a 100644 --- a/docs/22-multi-tenancy.md +++ b/docs/22-multi-tenancy.md @@ -210,6 +210,41 @@ All four live under the **System** category and show up in --- +## Security posture (v2 hardening) + +The v2 isolation stack is designed to **fail closed** and is off by default so +single-tenant installs are unaffected. A multi-tenant deployment enables at +least `TENANCY_ENABLED` + `TENANCY_RLS_ENABLED`, plus +`TENANCY_STRICT_ISOLATION_ENABLED` for hard cross-tenant blocking. + +- **RLS gate fails closed.** If the per-request Postgres tenant scope cannot be + installed, the request is aborted (HTTP 500) rather than run with global row + visibility (an unset scope makes RLS treat all rows as visible). +- **Strict gate sees cross-tenant objects.** The 403 gate resolves the target + resource's organization with the caller's RLS scope removed — otherwise a + genuinely cross-tenant object is invisible to the lookup and could never be + blocked. When a covered resource's org can't be determined, it defaults to + **deny**. +- **RLS coverage.** Every table with its own `organization_id` has a policy + (including `main_eventlog`); child tables of `main_unifiedjobtemplate` / + `main_unifiedjob` (projects, workflow templates, schedules, nodes, job events) + are covered indirectly through the parent via the multi-table-inheritance join. + Policies cast the tenant GUC as `NULLIF(current_setting(...), '')::int` so the + empty "no scope" sentinel can't raise. +- **Rate limiter** logs Redis outages loudly and can be made to fail closed with + `TENANCY_RATE_LIMIT_FAIL_CLOSED` (default open, favouring availability). +- **Provisioning** refuses to silently reuse an existing username (which would + discard the supplied password and cross-link accounts) unless + `attach_existing_admin` is set. + +| Key | Default | Notes | +| ---------------------------------- | ------- | --------------------------------------------------------- | +| `TENANCY_RLS_ENABLED` | `False` | Install the per-request RLS tenant scope | +| `TENANCY_STRICT_ISOLATION_ENABLED` | `False` | Block (403) cross-tenant access instead of audit-only | +| `TENANCY_RATE_LIMIT_FAIL_CLOSED` | `False` | Reject (503) when the rate-limiter Redis is down | + +--- + ## REST API All `/api/v2/tenants/*` endpoints require `is_superuser`. `/api/v2/branding/` diff --git a/docs/24-awx-import.md b/docs/24-awx-import.md index 31c6f2c..1b7f58b 100644 --- a/docs/24-awx-import.md +++ b/docs/24-awx-import.md @@ -17,12 +17,21 @@ forail-manage import_from_awx \ | Option | Description | | ----------------------- | ---------------------------------------------------------------------- | | `--url` | Base URL of the source AWX install (required). | -| `--token` | OAuth2 token for the source AWX API (preferred auth). | -| `--username/--password` | Basic auth, if no token. | +| `--token` | OAuth2 token for the source AWX API (preferred auth). Prefer `AWX_TOKEN` env. | +| `--username/--password` | Basic auth, if no token. Prefer `AWX_USERNAME` / `AWX_PASSWORD` env. | | `--insecure` | Skip source TLS certificate verification. | | `--dry-run` | Fetch and report what would change, then roll back without writing. | +| `--grant-superusers` | Honour `is_superuser` / system-role grants from the source (**off** by default). | +| `--trust-injectors` | Import custom credential-type injectors verbatim (**off**; else re-approve). | | `--resource ` | Limit to specific resource type(s); repeatable. Default: all. | +> **The source is treated as untrusted by default.** Superuser promotion and +> custom credential-type injectors (which render into env/extra-vars/files at +> job-execution time) are **not** applied unless you opt in with +> `--grant-superusers` / `--trust-injectors`. Pass secrets via `AWX_TOKEN` / +> `AWX_PASSWORD` environment variables rather than the command line, where they +> would be visible in `ps` / `/proc`. + Resource types (and import order): `organizations`, `users`, `teams`, `credential_types`, `credentials`, `projects`, `inventories`, `groups`, `hosts`, `inventory_sources`, `job_templates`, `workflow_job_templates`, diff --git a/forail/main/conf.py b/forail/main/conf.py index fe79680..809c4fd 100644 --- a/forail/main/conf.py +++ b/forail/main/conf.py @@ -1197,6 +1197,16 @@ def csrf_trusted_origins_validate(serializer, attrs): ) # --- Multi-Tenancy v2 settings ----------------------------------------------- +# +# needtofix M16: every tenant-isolation control below defaults to False, so a +# deployment gets NO cross-tenant enforcement until each flag is turned on. A +# multi-tenant install MUST enable, at minimum: +# TENANCY_ENABLED=True, TENANCY_RLS_ENABLED=True +# and, for hard cross-tenant blocking (vs audit-only): +# TENANCY_STRICT_ISOLATION_ENABLED=True (plus per-org tenant_isolation_strict) +# Rate limiting and dedicated queues are opt-in performance/abuse controls. +# The defaults stay False to preserve single-tenant backwards compatibility; +# the deployment docs call out the required enablement set for tenants. register( 'TENANCY_RLS_ENABLED', @@ -1241,6 +1251,21 @@ def csrf_trusted_origins_validate(serializer, attrs): category_slug='system', ) +register( + 'TENANCY_RATE_LIMIT_FAIL_CLOSED', + field_class=fields.BooleanField, + default=False, + label=_('Rate Limiter Fails Closed'), + help_text=_( + 'When True, tenant API requests are rejected with HTTP 503 if the ' + 'rate-limiter backend (Redis) is unavailable, instead of being allowed ' + 'through (fail-open). Defaults to False to keep the API available ' + 'during a Redis outage.' + ), + category=_('System'), + category_slug='system', +) + register( 'TENANCY_DEFAULT_API_RATE_LIMIT', field_class=fields.IntegerField, diff --git a/forail/main/management/commands/import_from_awx.py b/forail/main/management/commands/import_from_awx.py index f334788..4d8bd7a 100644 --- a/forail/main/management/commands/import_from_awx.py +++ b/forail/main/management/commands/import_from_awx.py @@ -36,6 +36,7 @@ import json import logging +import os import requests @@ -128,23 +129,56 @@ class Command(BaseCommand): def add_arguments(self, parser): parser.add_argument('--url', required=True, help='Base URL of the source AWX install, e.g. https://awx.example.com') - parser.add_argument('--token', help='OAuth2 token for the source AWX API (preferred).') - parser.add_argument('--username', help='Username for basic auth (if no token).') - parser.add_argument('--password', help='Password for basic auth (if no token).') + parser.add_argument('--token', help='OAuth2 token for the source AWX API (preferred). ' + 'Prefer the AWX_TOKEN env var — CLI args are visible in ps/proc.') + parser.add_argument('--username', help='Username for basic auth (if no token). Env: AWX_USERNAME.') + parser.add_argument('--password', help='Password for basic auth (if no token). ' + 'Prefer the AWX_PASSWORD env var — CLI args are visible in ps/proc.') parser.add_argument('--insecure', action='store_true', help='Do not verify the source TLS certificate.') parser.add_argument('--dry-run', action='store_true', help='Fetch and report what would change, then roll back without writing.') + parser.add_argument('--grant-superusers', action='store_true', + help='Honour is_superuser / system_administrator / system_auditor from the source. ' + 'OFF by default: superuser promotion from a remote source is a privilege-escalation ' + 'vector, so these grants are skipped and reported unless you opt in explicitly.') + parser.add_argument('--trust-injectors', action='store_true', + help='Import custom credential-type injectors verbatim. OFF by default: injector templates ' + 'render into env/extra-vars/files at job-execution time, so an untrusted source could ' + 'ship an injector that runs attacker code. Without this flag the type is imported ' + 'without its injectors and an admin must re-approve them.') parser.add_argument('--resource', action='append', choices=RESOURCE_ORDER, help='Limit to specific resource type(s); may be repeated. Default: all.') def handle(self, *args, **options): - if not options.get('token') and not options.get('username'): - raise CommandError('Provide --token, or --username/--password.') + # M3: prefer secrets from the environment; CLI args leak via ps/proc, + # shell history and process accounting. + token = options.get('token') or os.environ.get('AWX_TOKEN') + username = options.get('username') or os.environ.get('AWX_USERNAME') + password = options.get('password') or os.environ.get('AWX_PASSWORD') + if options.get('token') or options.get('password'): + self.stderr.write(self.style.WARNING( + 'Passing --token/--password on the command line is insecure (visible in ' + 'ps/proc and shell history). Prefer AWX_TOKEN / AWX_PASSWORD env vars.')) + + if not token and not username: + raise CommandError('Provide a token (--token or AWX_TOKEN), or a username ' + '(--username/AWX_USERNAME with --password/AWX_PASSWORD).') + + # L1: --insecure sends the token / basic-auth credentials over a + # connection with no certificate verification. + if options.get('insecure'): + self.stderr.write(self.style.WARNING( + '--insecure disables TLS verification; credentials are sent over an ' + 'unauthenticated channel. Use only against a trusted network.')) + + # M1/M2: carry the trust opt-ins on the context so importers can gate. + self.grant_superusers = options.get('grant_superusers', False) + self.trust_injectors = options.get('trust_injectors', False) client = AWXClient( options['url'], - token=options.get('token'), - username=options.get('username'), - password=options.get('password'), + token=token, + username=username, + password=password, verify=not options.get('insecure'), ) # Fail fast on connectivity / auth before opening a transaction. @@ -247,8 +281,19 @@ def _import_users(self, client, ctx): # not a superuser must not silently demote — and potentially lock you # out of — your own Forail bootstrap admin. System-role grants are # (re-)applied in _import_roles. + # + # M1: superuser is a remote-controlled privilege-escalation vector. + # A compromised/malicious source could flag arbitrary usernames as + # superusers. Gate it behind an explicit --grant-superusers opt-in + # and log every promotion loudly. if u.get('is_superuser'): - obj.is_superuser = True + if self.grant_superusers: + if not obj.is_superuser: + ctx.warn('GRANTING superuser to "%s" from source (--grant-superusers).' % u['username']) + obj.is_superuser = True + else: + ctx.warn('Skipped superuser grant for "%s" (source flagged it superuser; ' + 'pass --grant-superusers to honour it).' % u['username']) if created: obj.set_unusable_password() ctx.warn('User "%s" created without a password — set one (passwords are not exported by AWX).' % u['username']) @@ -278,7 +323,18 @@ def _import_credential_types(self, client, ctx): obj.description = ct.get('description', '') or '' obj.kind = ct.get('kind', obj.kind) obj.inputs = ct.get('inputs', {}) or {} - obj.injectors = ct.get('injectors', {}) or {} + # M2: a custom credential type's injectors are rendered into env + # vars / extra-vars / files at job-execution time. Importing them + # verbatim from an untrusted source is a post-migration RCE vector. + # Skip injectors unless the operator explicitly trusts the source; + # an admin re-approves the injector bodies afterwards. + source_injectors = ct.get('injectors', {}) or {} + if source_injectors and not self.trust_injectors: + obj.injectors = {} + ctx.warn('Credential type "%s": injectors NOT imported (re-approve manually, ' + 'or re-run with --trust-injectors).' % ct['name']) + else: + obj.injectors = source_injectors self._save(ctx, 'credential_type', obj, created) ctx.maps['credential_type'][ct['id']] = obj @@ -626,10 +682,18 @@ def _import_roles(self, client, ctx): user = ctx.maps['user'].get(u['id']) if user is None: continue - if role_field == 'system_administrator': - user.is_superuser = True - elif role_field == 'system_auditor': - user.is_system_auditor = True + # M1: gate remote system-role promotion behind opt-in. + if role_field in ('system_administrator', 'system_auditor'): + if not self.grant_superusers: + ctx.warn('Skipped %s grant for "%s" (pass --grant-superusers to honour it).' + % (role_field, user.username)) + continue + if role_field == 'system_administrator': + ctx.warn('GRANTING system_administrator (superuser) to "%s".' % user.username) + user.is_superuser = True + else: + ctx.warn('GRANTING system_auditor to "%s".' % user.username) + user.is_system_auditor = True else: continue if not ctx.dry_run: diff --git a/forail/main/migrations/0209_rls_eventlog.py b/forail/main/migrations/0209_rls_eventlog.py new file mode 100644 index 0000000..dca24ef --- /dev/null +++ b/forail/main/migrations/0209_rls_eventlog.py @@ -0,0 +1,45 @@ +"""Multi-Tenancy v2: extend RLS coverage to main_eventlog. + +needtofix M4 — ``EventLog`` carries its own ``organization_id`` but was +omitted from the original RLS policy set (0206). Add the policy here. + +Idempotent by construction: the CREATE is preceded by DROP POLICY IF EXISTS, +so this converges whether or not 0206 already created the policy (a fresh +install runs 0206 with main_eventlog already in RLS_TABLES_DIRECT). +""" + +from django.db import migrations + +from forail.main.tenancy.helpers import build_rls_policy_sql + + +_TABLE = 'main_eventlog' + + +def _forward_sql(): + create, drop = build_rls_policy_sql(_TABLE, 'organization_id') + return '\n'.join([ + f'ALTER TABLE {_TABLE} ENABLE ROW LEVEL SECURITY;', + f'ALTER TABLE {_TABLE} FORCE ROW LEVEL SECURITY;', + drop, # drop first so re-create is safe on fresh installs + create, + ]) + + +def _reverse_sql(): + _, drop = build_rls_policy_sql(_TABLE, 'organization_id') + return '\n'.join([ + drop, + f'ALTER TABLE {_TABLE} DISABLE ROW LEVEL SECURITY;', + ]) + + +class Migration(migrations.Migration): + + dependencies = [ + ('main', '0208_driftalertrule_audit_fields'), + ] + + operations = [ + migrations.RunSQL(sql=_forward_sql(), reverse_sql=_reverse_sql()), + ] diff --git a/forail/main/migrations/0210_rls_nullif_cast.py b/forail/main/migrations/0210_rls_nullif_cast.py new file mode 100644 index 0000000..ef9f200 --- /dev/null +++ b/forail/main/migrations/0210_rls_nullif_cast.py @@ -0,0 +1,52 @@ +"""Multi-Tenancy v2: recreate RLS policies with a NULLIF-guarded cast. + +needtofix L6 — the original policies cast the raw GUC as +``current_setting(...)::int``, which raises on the empty-string "no tenant +scope" sentinel; Postgres does not guarantee the ``= ''`` guard is evaluated +before the cast. build_rls_policy_sql now emits +``NULLIF(current_setting(...), '')::int`` instead. Recreate every policy so +existing installs pick up the robust form. + +Idempotent: each policy is dropped (IF EXISTS) then recreated, for both +direct and indirect tables, so this converges on fresh and existing DBs. +""" + +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', '0209_rls_eventlog'), + ] + + operations = [ + # Forward and reverse both rebuild from the current builder; the reverse + # is a no-op distinction here (policies are recreated either way), which + # is acceptable because the builder is the single source of truth. + migrations.RunSQL(sql=_rebuild_sql(), reverse_sql=_rebuild_sql()), + ] diff --git a/forail/main/models/tenancy.py b/forail/main/models/tenancy.py index 220023b..e23d6ce 100644 --- a/forail/main/models/tenancy.py +++ b/forail/main/models/tenancy.py @@ -141,6 +141,9 @@ class Meta: related_name='tenant_quota_events', ) message = models.TextField(blank=True, default='') + # migration 0205 declares this column NOT NULL (default ''); CreatedModifiedModel + # does not provide it, so it must be defined here or inserts fail. + description = models.TextField(blank=True, default='') def get_absolute_url(self, request=None): return reverse('api:tenant_quota_event_detail', kwargs={'pk': self.pk}, request=request) @@ -185,6 +188,10 @@ class Meta: resource_id = models.PositiveIntegerField(null=True, blank=True) request_path = models.CharField(max_length=1024, blank=True, default='') blocked = models.BooleanField(default=False) + # migration 0205 declares this column NOT NULL (default ''); CreatedModifiedModel + # does not provide it, so it must be defined here or inserts fail (which is + # exactly what the strict gate hit once it started blocking). + description = models.TextField(blank=True, default='') def get_absolute_url(self, request=None): return reverse('api:tenant_isolation_event_detail', kwargs={'pk': self.pk}, request=request) diff --git a/forail/main/scanning/runner.py b/forail/main/scanning/runner.py index 44562d2..d379e44 100644 --- a/forail/main/scanning/runner.py +++ b/forail/main/scanning/runner.py @@ -60,7 +60,17 @@ def _resolve_target(unified_job, resource_type, project_path, tool_name): playbook = getattr(unified_job, 'playbook', '') or '' if not playbook: return None - return os.path.join(project_path, playbook) + # M15: ``playbook`` is a user-editable JobTemplate field. An absolute + # value makes os.path.join discard project_path, and ``..`` segments + # escape the checkout — the scanner would then run against arbitrary + # host paths (/etc/…) and leak fragments into ScanResult output. + # Resolve and confirm the target stays inside project_path. + base = os.path.realpath(project_path) + target = os.path.realpath(os.path.join(base, playbook)) + if target != base and not target.startswith(base + os.sep): + logger.warning('scan target %r escapes project path %r — refusing', playbook, project_path) + return None + return target if resource_type == 'ad_hoc_command': if tool_name == 'ansible-lint': return None diff --git a/forail/main/scanning/tools/pip_audit.py b/forail/main/scanning/tools/pip_audit.py index e0ed3e0..23292a5 100644 --- a/forail/main/scanning/tools/pip_audit.py +++ b/forail/main/scanning/tools/pip_audit.py @@ -6,18 +6,29 @@ """ import json +import logging +import os from forail.main.scanning.types import NormalizedFinding TOOL_NAME = 'pip-audit' +logger = logging.getLogger('forail.main.scanning.tools.pip_audit') + def build_command(target_path, config): cmd = ['pip-audit', '--format', 'json'] requirements = target_path if config and isinstance(config, dict): - if config.get('requirements'): - requirements = config['requirements'] + override = config.get('requirements') + # L9: the admin-set requirements override is used as the -r path. Reject + # ../ traversal so it can't be pointed outside the checkout (admin-only, + # low impact, but no reason to leave the escape open). + if override: + if os.path.isabs(override) or '..' in override.replace('\\', '/').split('/'): + logger.warning('pip-audit: ignoring unsafe requirements override %r', override) + else: + requirements = override cmd.extend(['-r', requirements]) return cmd diff --git a/forail/main/tenancy/helpers.py b/forail/main/tenancy/helpers.py index 0811052..6175475 100644 --- a/forail/main/tenancy/helpers.py +++ b/forail/main/tenancy/helpers.py @@ -233,7 +233,19 @@ def tenant_queue_name(org_id): # 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). +# 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). +# +# Coverage note (needtofix M4): several models that *look* org-scoped +# (Project, WorkflowJobTemplate, Schedule, workflow nodes, job events) do not +# carry their own organization_id column — AWX 3.7 (migration 0109) moved it +# onto the shared parent main_unifiedjobtemplate / main_unifiedjob, both of +# which ARE covered below. Django multi-table inheritance always joins that +# parent when querying the child, so those rows are filtered by the parent +# policy. Only tables with their own organization_id column belong here. RLS_TABLES_DIRECT = [ # Core resources @@ -250,6 +262,7 @@ def tenant_queue_name(org_id): # EDA ('main_eventrule', 'organization_id'), ('main_outboundwebhook', 'organization_id'), + ('main_eventlog', 'organization_id'), # needtofix M4: has own org column, was uncovered # Drift detection ('main_hostfactsnapshot', 'organization_id'), ('main_driftdetection', 'organization_id'), @@ -284,14 +297,19 @@ def build_rls_policy_sql(table, org_column='organization_id'): compatible for non-tenant requests and superusers). """ policy_name = f'tenant_isolation_{table}' + # L6: wrap the GUC in NULLIF(...,'')::int rather than casting the raw + # setting. An empty string ('' — the "no tenant scope" sentinel) would + # raise on ::int; Postgres does not guarantee the '' guard short-circuits + # 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), '')" create = ( f'CREATE POLICY {policy_name} ON {table} ' f'AS PERMISSIVE FOR ALL ' f'USING (' - f'{org_column} = current_setting(\'forail.current_tenant_id\', true)::int ' + f'{org_column} = {tenant}::int ' f'OR {org_column} IS NULL ' - f'OR current_setting(\'forail.current_tenant_id\', true) IS NULL ' - f'OR current_setting(\'forail.current_tenant_id\', true) = \'\'' + f'OR {tenant} IS NULL' f');' ) drop = f'DROP POLICY IF EXISTS {policy_name} ON {table};' @@ -304,18 +322,18 @@ def build_rls_policy_sql_indirect(table, fk_column, parent_table, parent_org_col Uses a subquery to resolve the organization from a parent table. """ policy_name = f'tenant_isolation_{table}' + tenant = "NULLIF(current_setting('forail.current_tenant_id', true), '')" create = ( f'CREATE POLICY {policy_name} ON {table} ' f'AS PERMISSIVE FOR ALL ' f'USING (' f'{fk_column} IN (' f'SELECT id FROM {parent_table} WHERE ' - f'{parent_org_column} = current_setting(\'forail.current_tenant_id\', true)::int ' + f'{parent_org_column} = {tenant}::int ' f'OR {parent_org_column} IS NULL' f') ' f'OR {fk_column} IS NULL ' - f'OR current_setting(\'forail.current_tenant_id\', true) IS NULL ' - f'OR current_setting(\'forail.current_tenant_id\', true) = \'\'' + f'OR {tenant} IS NULL' f');' ) drop = f'DROP POLICY IF EXISTS {policy_name} ON {table};' diff --git a/forail/main/tenancy/isolation.py b/forail/main/tenancy/isolation.py index 8bc16f9..ea2dd4f 100644 --- a/forail/main/tenancy/isolation.py +++ b/forail/main/tenancy/isolation.py @@ -20,6 +20,7 @@ """ import logging +from contextlib import contextmanager from django.conf import settings from django.http import JsonResponse @@ -33,6 +34,29 @@ logger = logging.getLogger('forail.main.tenancy.isolation') +# Sentinel returned by target-org resolution: the request targets a covered +# (org-scoped) resource, but its organization could not be determined. The +# strict gate must default to DENY on this rather than fail open. +_DENY_UNRESOLVED = object() + + +@contextmanager +def _rls_unscoped(restore_org_id): + """Temporarily drop the caller's RLS tenant scope, then restore it. + + The strict gate has to *see* cross-tenant objects in order to block them; + running the target-org lookup under the caller's own RLS scope would hide + exactly the rows we need to detect (an empty/unset tenant id makes all + rows visible). We clear the scope for the lookup and reinstate it after. + """ + clear_tenant_id() + try: + yield + finally: + if restore_org_id is not None: + set_tenant_id(restore_org_id) + + class TenantIsolationMiddleware: """Set Postgres ``forail.current_tenant_id`` per request for RLS and enforce strict cross-tenant isolation when configured.""" @@ -48,12 +72,31 @@ def __call__(self, request): tenant_org = None try: tenant_org = self._resolve_tenant_org(request) - if tenant_org is not None: - set_tenant_id(tenant_org.pk) - # Stash on request for process_view (strict gate). - request._tenant_org = tenant_org except Exception: - logger.debug('TenantIsolationMiddleware: failed to set tenant id', exc_info=True) + # 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. + logger.debug('TenantIsolationMiddleware: tenant org resolution failed', exc_info=True) + tenant_org = None + + if tenant_org is not None: + try: + set_tenant_id(tenant_org.pk) + except Exception: + # Fail CLOSED. If we know the request belongs to a tenant but + # cannot install the RLS scope, running the request would give + # it GLOBAL visibility (RLS treats an unset tenant id as "all + # rows"). Abort rather than leak across tenants. + logger.exception( + 'TenantIsolationMiddleware: could not set tenant id for org=%s ' + '— failing closed', getattr(tenant_org, 'pk', None), + ) + return JsonResponse( + {'detail': 'Tenant isolation could not be established.'}, + status=500, + ) + # Stash on request for process_view (strict gate). + request._tenant_org = tenant_org try: response = self.get_response(request) @@ -93,15 +136,37 @@ def process_view(self, request, view_func, view_args, view_kwargs): if tenant_org is None: return None + global_strict = getattr(settings, 'TENANCY_STRICT_ISOLATION_ENABLED', False) + user_org_strict = getattr(tenant_org, 'tenant_isolation_strict', False) + # Resolve the organization of the target resource. target_org_id = self._resolve_target_org_id(request, view_func, view_kwargs) + + if target_org_id is _DENY_UNRESOLVED: + # A covered, org-scoped resource whose organization we could not + # resolve. Fail CLOSED when strict isolation is actually in force + # for this tenant; otherwise fall through to audit-only allow. + if user_org_strict and global_strict: + event = self._emit_isolation_event(request, tenant_org, None, True) + logger.warning( + 'tenant_isolation: BLOCKED (unresolved target org) user=%s org=%s path=%s', + getattr(user, 'pk', None), tenant_org.pk, getattr(request, 'path', ''), + ) + return JsonResponse( + { + 'detail': 'Cross-tenant access denied.', + 'isolation_event_id': getattr(event, 'pk', None), + }, + status=403, + ) + return None + if target_org_id is None: - # Cannot determine target org — allow (fail-open for audit). + # Not a covered resource (no org-scoped model / object not found) — + # nothing to enforce. return None is_cross_tenant = (int(target_org_id) != int(tenant_org.pk)) - user_org_strict = getattr(tenant_org, 'tenant_isolation_strict', False) - global_strict = getattr(settings, 'TENANCY_STRICT_ISOLATION_ENABLED', False) should_block, should_audit = make_isolation_decision( user_org_strict, global_strict, is_cross_tenant, @@ -171,7 +236,18 @@ def _resolve_target_org_id(request, view_func, view_kwargs): 2. If ``organization`` is in kwargs (sub-resource views), use it directly. - Returns an int org_id or None. + The object lookup is performed with the caller's RLS scope removed + (see ``_rls_unscoped``) so that a genuinely cross-tenant object is + visible here — otherwise it would resolve to ``None`` and the strict + gate could never fire. + + Returns: + - an ``int`` org id when the target org is known, + - ``None`` when the target is not an org-scoped resource, the + object does not exist, or the resource is global (NULL org), + - ``_DENY_UNRESOLVED`` when the target *is* a covered, org-scoped + resource but its organization could not be determined (the + strict gate must fail closed on this). """ # Strategy A: explicit org in URL kwargs (e.g. /organizations/{pk}/...). org_kwarg = view_kwargs.get('organization') @@ -194,25 +270,42 @@ def _resolve_target_org_id(request, view_func, view_kwargs): if model is None: return None + from forail.main.models import Host + is_host = model is Host or getattr(model, '__name__', '') == 'Host' + is_direct = hasattr(model, 'organization_id') or hasattr(model, 'organization') + if not is_direct and not is_host: + # Not an org-scoped model — nothing for the strict gate to enforce. + return None + + restore_org_id = getattr(getattr(request, '_tenant_org', None), 'pk', None) try: - # Direct organization_id on the model. - if hasattr(model, 'organization_id') or hasattr(model, 'organization'): - obj = model.objects.filter(pk=pk).values_list('organization_id', flat=True).first() - if obj is not None: - return int(obj) if obj else None - - # Indirect: Host → Inventory → Organization. - from forail.main.models import Host - if model is Host or (hasattr(model, '__name__') and model.__name__ == 'Host'): + with _rls_unscoped(restore_org_id): + if is_direct: + found = list( + model.objects.filter(pk=pk) + .values_list('organization_id', flat=True)[:1] + ) + if not found: + # Object does not exist even unscoped — let the view 404. + return None + org_id = found[0] + # NULL org → global/shared resource, not cross-tenant. + return int(org_id) if org_id else None + + # Indirect: Host → Inventory → Organization. from forail.main.models import Inventory inv_id = Host.objects.filter(pk=pk).values_list('inventory_id', flat=True).first() - if inv_id: - org_id = Inventory.objects.filter(pk=inv_id).values_list('organization_id', flat=True).first() - return int(org_id) if org_id else None + if inv_id is None: + return None + org_id = Inventory.objects.filter(pk=inv_id).values_list('organization_id', flat=True).first() + return int(org_id) if org_id else None except Exception: - logger.debug('_resolve_target_org_id: lookup failed', exc_info=True) - - return None + # Covered resource, but org resolution errored — fail CLOSED. + logger.warning( + '_resolve_target_org_id: lookup failed for covered model %s pk=%s — denying', + getattr(model, '__name__', model), pk, exc_info=True, + ) + return _DENY_UNRESOLVED @staticmethod def _emit_isolation_event(request, user_org, target_org_id, blocked): @@ -223,7 +316,7 @@ def _emit_isolation_event(request, user_org, target_org_id, blocked): event = TenantIsolationEvent.objects.create( user=user if getattr(user, 'is_authenticated', False) else None, user_organization=user_org, - accessed_organization_id=int(target_org_id), + accessed_organization_id=int(target_org_id) if target_org_id is not None else None, resource_type=_get_resource_type(request), resource_id=_get_resource_id(request), request_path=getattr(request, 'path', '')[:1024], diff --git a/forail/main/tenancy/provisioning.py b/forail/main/tenancy/provisioning.py index 20dfa48..059f95f 100644 --- a/forail/main/tenancy/provisioning.py +++ b/forail/main/tenancy/provisioning.py @@ -46,17 +46,30 @@ def provision_tenant(payload): tenant_contact_email=payload.get('contact_email', '') or '', ) - # Admin user. If the username already exists, reuse it rather than crash. + # Admin user. + # + # M6: refuse to silently reuse an existing username. Doing so both (a) + # discards the supplied password (get_or_create only set it on create) and + # (b) grants that pre-existing account — possibly another tenant's admin — + # membership in this new org. Require an explicit attach_existing_admin + # opt-in, and never accept a password we would throw away. admin_username = payload['admin_username'] admin_email = payload['admin_email'] admin_password = payload['admin_password'] - user, created = User.objects.get_or_create( - username=admin_username, - defaults={'email': admin_email, 'is_active': True}, - ) - if created: + attach_existing = bool(payload.get('attach_existing_admin', False)) + + existing = User.objects.filter(username=admin_username).first() + if existing is not None: + if not attach_existing: + raise ProvisioningError([ + f'admin_username "{admin_username}" already exists; refusing to attach an ' + f'existing account to a new tenant. Pass attach_existing_admin=true to ' + f'intentionally reuse it (the supplied admin_password is then ignored).' + ]) + user = existing + else: + user = User.objects.create(username=admin_username, email=admin_email, is_active=True) user.set_password(admin_password) - user.email = admin_email user.save() # Grant admin role on the new Organization. Best-effort — role API may vary. diff --git a/forail/main/tenancy/rate_limit.py b/forail/main/tenancy/rate_limit.py index bcc8dc9..ba0190d 100644 --- a/forail/main/tenancy/rate_limit.py +++ b/forail/main/tenancy/rate_limit.py @@ -80,12 +80,30 @@ def __call__(self, request): if max_tokens <= 0: return self.get_response(request) + # L7: the limiter's behaviour when Redis is unavailable is a conscious + # trade-off. It fails OPEN by default (an outage must not take the whole + # API down), but operators who prefer availability-of-isolation over + # availability-of-service can set TENANCY_RATE_LIMIT_FAIL_CLOSED=True to + # return 503 instead. Either way the outage is logged loudly, not + # swallowed at debug level. + fail_closed = getattr(settings, 'TENANCY_RATE_LIMIT_FAIL_CLOSED', False) + + def _on_redis_unavailable(reason): + if fail_closed: + logger.warning('rate_limit: Redis unavailable (%s) — failing CLOSED (503) org=%s', + reason, tenant_org.pk) + return JsonResponse( + {'detail': 'Rate limiter temporarily unavailable.'}, status=503, + ) + logger.warning('rate_limit: Redis unavailable (%s) — failing open, throttle bypassed org=%s', + reason, tenant_org.pk) + return self.get_response(request) + # Check the bucket. try: redis_client = _get_redis() if redis_client is None: - # Fail-open: if Redis is unavailable, allow the request. - return self.get_response(request) + return _on_redis_unavailable('no client') sha = _ensure_script(redis_client) key = f'tenant_ratelimit:{tenant_org.pk}' @@ -115,7 +133,7 @@ def __call__(self, request): return response except Exception: - # Fail-open: on any Redis error, allow the request. - logger.debug('rate_limit: Redis error, allowing request', exc_info=True) + logger.warning('rate_limit: Redis error', exc_info=True) + return _on_redis_unavailable('redis error') return self.get_response(request) diff --git a/forail/settings/defaults/social_auth.py b/forail/settings/defaults/social_auth.py index 1ca9c05..cce3e3f 100644 --- a/forail/settings/defaults/social_auth.py +++ b/forail/settings/defaults/social_auth.py @@ -14,13 +14,20 @@ ROLE_BYPASS_SUPERUSER_FLAGS = ['is_superuser'] ROLE_BYPASS_ACTION_FLAGS = {'view': 'is_system_auditor'} +# needtofix M7: 'social_core.pipeline.social_auth.associate_by_email' is +# intentionally NOT in this pipeline. Associating an SSO login to an existing +# local account purely by matching email address enables cross-IdP account +# takeover: if any configured IdP does not verify email ownership, a user who +# registers admin@company.com there can log in as the existing admin account. +# Accounts are associated by provider UID instead. Operators who trust every +# configured IdP to assert verified emails may re-add associate_by_email after +# get_username via a settings override. _SOCIAL_AUTH_PIPELINE_BASE = ( 'social_core.pipeline.social_auth.social_details', 'social_core.pipeline.social_auth.social_uid', 'social_core.pipeline.social_auth.auth_allowed', 'social_core.pipeline.social_auth.social_user', 'social_core.pipeline.user.get_username', - 'social_core.pipeline.social_auth.associate_by_email', 'social_core.pipeline.user.create_user', 'forail.sso.social_base_pipeline.check_user_found_or_created', 'social_core.pipeline.social_auth.associate_user', diff --git a/tests_standalone/test_tenancy.py b/tests_standalone/test_tenancy.py index ef936c6..2943e03 100644 --- a/tests_standalone/test_tenancy.py +++ b/tests_standalone/test_tenancy.py @@ -213,14 +213,14 @@ def test_create_contains_policy_name(self): def test_create_contains_org_column(self): create, _ = build_rls_policy_sql('main_inventory', 'organization_id') - self.assertIn('organization_id = current_setting', create) + # needtofix L6: the GUC is wrapped in NULLIF(...,'')::int so the empty + # "no scope" sentinel can't raise on the cast. + self.assertIn("organization_id = NULLIF(current_setting", create) def test_create_contains_bypass_clauses(self): create, _ = build_rls_policy_sql('main_inventory', 'organization_id') - # Bypass when session var is NULL - self.assertIn("IS NULL", create) - # Bypass when session var is empty string - self.assertIn("= ''", create) + # 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) @@ -262,7 +262,8 @@ def test_create_contains_bypass(self): 'main_host', 'inventory_id', 'main_inventory', 'organization_id' ) self.assertIn("IS NULL", create) - self.assertIn("= ''", create) + # needtofix L6: NULLIF-guarded empty/unset sentinel (was `= ''`). + self.assertIn("NULLIF(current_setting('forail.current_tenant_id', true), '') IS NULL", create) def test_drop_contains_policy_name(self): _, drop = build_rls_policy_sql_indirect(