fix: dicht twee cross-tenant isolatielekken in OPI#70
Conversation
|
Drie commits toegevoegd vanuit een diepe review (commits Wat de PR oorspronkelijk dichtte (✅ ongewijzigd)
Onze toevoegingen1. GAT 1 —
|
|
Vervolg op de eerdere augmentatie-commits: na user-pushback heb ik commit Waarom verwijderen ipv defensief patchenUitputtende verificatie: geen enkele productiecode submit een
Plus: Wat de cleanup-commit doet
Resultaat: één authoritative project-creation pad — Consistent met de lijn van PR #72 ( Pre-existing test-hang op deze branch (niet door deze cleanup)Tijdens de bredere test-sweep hangt
Conclusie: de hang is geïntroduceerd door één van PR #70's eigen commits ( Audit-statusVoor deze PR zijn nu vier commits van mij toegevoegd:
Future-doc |
|
Vijfde commit toegevoegd ( Wat het probleem was
Root cause
if not KubectlConnector.isConnected:
self._retry_task = asyncio.create_task(self._connection_retry())De broer-test ( Op een dev-machine met werkende kubectl-context fired De fixTwee regels, gekopieerd uit de zuster-test: connector = KubectlConnector()
if connector._retry_task:
connector._retry_task.cancel()
connector._retry_task = NoneDe test blijft kubectl-env-onafhankelijk (de Geverifieerd: Bredere observatie (geen actie op deze PR)
Samenvatting van wat er nu op PR #70 staatAlle review-bevindingen geadresseerd: VULN 1+2 (origineel), GAT 1 (verwijderd), GAT 3 (helper-pin), plus deze tangentiële test-fix. |
VULN 1 - deployment.namespace niet vastgepind op projectnaam:
get_deployments nam de namespace ongecontroleerd over uit het
project-YAML. Een tenant kon namespace: <ander-project> zetten
(met cluster == CLUSTER_MANAGER om het enige filter te passeren)
waardoor OPI de namespace van een ander project labelde en
beheerde, plus ArgoCD AppProject/Application richting die namespace
genereerde. get_deployments dwingt nu af dat de namespace gelijk is
aan de projectnaam: ontbreekt hij dan wordt de projectnaam ingevuld
(de bestaande, legitieme default), wijkt hij af dan faalt het project
met een duidelijke foutmelding. Bewust fail-closed, geen stille
herschrijving die de intentie zou maskeren.
VULN 2 - project-overname via de wizard:
de create-flow schreef projects/{naam}.yaml zonder bestaans- of
eigenaarscontrole, waardoor het projectbestand van een andere tenant
stil werd overschreven en de eigenaar bij herladen wisselde. De
create-paden controleren nu eerst of het bestand al bestaat en
weigeren in dat geval (process_project_yaml_background krijgt een
is_new_project-vlag; alleen de wizard-create geeft True door). De
update/edit-flows blijven ongewijzigd een bestaand bestand
herschrijven.
Beide lekken waren pre-existing en staan los van andere PR's.
Tests: tests/test_tenant_isolation_namespace.py - red/green voor
beide fixes, inclusief de legitieme default- en update-gevallen.
De namespace-pin uit de vorige commit zat alleen in ProjectManager.get_deployments. Het git-monitor pad (check_and_create_namespaces in git_monitor.py) leest het projectbestand rechtstreeks en ging niet door dat chokepoint, waardoor een tenant die een projectbestand direct naar git commit nog steeds een namespace van een andere tenant kon laten aanmaken en labelen. De pin-logica staat nu in een gedeelde enforce_namespace_pin() in project_manager.py, aangeroepen vanuit zowel get_deployments als het git-monitor pad. Het git-monitor pad faalt gesloten: een afwijkende namespace wordt geweigerd en gelogd zonder iets aan te maken, zonder de polling-loop te laten crashen. Regressietest toegevoegd die bewijst dat het git-monitor pad nu ook gepind is (foreign namespace geweigerd voordat kubectl wordt geraakt; afwezige namespace valt terug op de projectnaam).
… task)
handle_create_project is registered for TaskType.CREATE_PROJECT in
server.py:122 and worker_main.py:58. It writes projects/{project_name}.yaml
via create_or_update_file without checking whether the file already exists
- same vulnerability that PR 70 fixed in simple_background but not in this
parallel async-task path.
Today no caller in the codebase submits TaskType.CREATE_PROJECT (grep over
opi/api/, opi/web/, opi/core/ confirms only test fixtures use the string),
so not exploitable now. But the handler is wired as a task type the
worker will accept, so a future endpoint added without thinking about the
existence check would silently overwrite victim projects. Defense in
depth.
The check mirrors simple_background.process_project_background exactly:
file_exists before create_or_update_file, fail-closed with a Dutch error
message and a failed task result. No functional difference for the
already-correct case (file does not exist).
PR 70 enforces the namespace pin in two places: - ProjectManager.get_deployments (API/wizard path) - git_monitor.check_and_create_namespaces (git-monitor path) But the same project YAML is read raw via extract_deployment_namespace from nine other callsites that do not go through get_deployments: - backup_router.py:665, 963, 1311 (3 endpoints) - restore_router.py:1167, 1589 (2 endpoints) - backup_tasks.py:87, 597, 755 (3 task handlers) - router_detail_edit.py:1340 (1 endpoint) A tenant who edits his own project YAML to set namespace=victim-project and then calls one of those endpoints with his own API key gets OPI to operate against the victim namespace - the exact cross-tenant primitive that PR 70 closes on the other paths but leaves open here. extract_deployment_namespace now enforces the same invariant as enforce_namespace_pin internally, without mutating project_data: - declared namespace matching project name -> return project name (no-op) - missing namespace -> default to project name - declared namespace mismatching project name -> raise ValueError with the same Dutch error message shape as enforce_namespace_pin Callers that depend on the helper's old contract (returning the raw declared value) would be the issue, but all nine callsites use the result for kubectl operations against the deployment's namespace - which under the pin invariant is always the project name. None benefit from getting an attacker-controlled value. Followup: callers do not catch ValueError; FastAPI converts it to a 500 Internal Server Error instead of a 400 Bad Request. Tracked in features/futures/tenant-isolation-followups.md - body is generic so no info-leak, just suboptimal UX.
…pens Seven additional tests pinning the augmentations from the previous two commits: - TestExtractDeploymentNamespacePinned (5 tests): declared-matching -> project name, missing -> project name, mismatched -> ValueError, unknown deployment -> None, multi-deployment scope-isolation. - TestHandleCreateProjectExistenceCheck (1 test): create_project task for an already-existing project name fails fast and does not call create_or_update_file. features/futures/tenant-isolation-followups.md parks the three items this PR review surfaced but did not address in code: 1. GAT 2 - TOCTOU on concurrent wizard-create with three possible designs. 2. Refactor: reuse git.check_overwrite_project_file instead of inline duplication in three places. 3. UX: ValueError from the new pin enforcement currently surfaces as 500 instead of 400. Plus status-table mapping all review findings against this PR.
…onnected Tangential to PR70's tenant-isolation scope but necessary for reliable pre-push CI: when kubectl is unavailable in the test environment, KubectlConnector.__init__ (opi/connectors/kubectl.py:77-78) spawns a background retry task. unittest.IsolatedAsyncioTestCase's teardown waits for all async tasks to be cancelled cleanly before exiting; the retry coroutine doesn't terminate, so _cancel_all_tasks hangs indefinitely. The sibling test test_stream_deployment_logs_success already cancels the retry task (lines 458-461). The not-connected test forgot to. This was masked when kubectl IS available on the developer machine (no retry task spawned, no hang) but reliably bites on machines without kubectl. Pattern copied verbatim from the sibling test. Two-line fix. The test remains kubectl-environment-independent (patch.object(KubectlConnector, 'isConnected', False) forces the no-connection path regardless of real kubectl status), so no CI marker needed. Verified: tests/test_logs_websocket_router.py::TestStreamDeploymentLogs runs cleanly in 20.91s with --timeout=20 (both tests pass).
uittenbroekrobbert
force-pushed
the
fix/tenant-isolation-namespace-ownership
branch
from
2794a40 to
9949629
Compare
Update — rebased op main + review-aanpassingenTijdens review zijn een paar dingen aangepast voordat het naar main kan. Rebase + conflict-resolutie
De existence-check die commit Conflict in
|
handle_create_project's file_exists check (PR #70) fired on every dispatch of the create_project task, but four callers use it: - router_wizard.py:1944 (real create -- check must fire) - router_detail_edit.py:493 (modal edit -- legitimate overwrite) - router_subdomain_admin.py:360 (subdomain admin) - router.py:498 (component delete) The three update flows hit the check and got 'project bestaat al' on every edit. Fix: payload['is_new_project'] flag, default False, only the wizard create sets True. Existence check + commit message ('Create' vs 'Update') key off it. database_manager: 'Creating database resources' -> 'Database klaarmaken' as a Dutch + neutral label that fits both create and update. Full create/update label split is tracked in issue #112.
… flag PR #71 made handle_create_project's existence check conditional on payload['is_new_project'] == True (so edit/update flows that reuse the create_project task aren't blocked). The existing test from PR #70 still ran without the flag and expected the block to fire -- now silently passes through. Add the flag, and add a complementary test that an edit-flow payload (no flag) DOES overwrite an existing file.
2 participants
Samenvatting
Twee bevestigde, pre-existing cross-tenant isolatielekken in OPI. Beide
staan los van de overige security-PR's en raken elkaars code niet.
VULN 1 - deployment.namespace niet vastgepind op de projectnaam
ProjectManager.get_deploymentsnam denamespaceongecontroleerd overuit het project-YAML. Het project-YAML is door de tenant te beheren. Door
namespace: <ander-project>te zetten (metcluster == CLUSTER_MANAGERom het enige bestaande filter te passeren) liet een tenant OPI de
namespace van een ander project labelen en beheren, en ArgoCD
AppProject/Application-resources richting die slachtoffer-namespacegenereren.
get_deploymentsis het enige knooppunt waar alle consumers(
check_and_create_namespaces,argo_manager) doorheen gaan.Fix:
get_deploymentsdwingt nu af dat de namespace gelijk is aan deprojectnaam. Ontbreekt de namespace, dan wordt de projectnaam ingevuld
(de bestaande, legitieme default die alle voorbeeldprojecten en fixtures
al gebruiken). Wijkt hij af, dan faalt het project met een duidelijke
Nederlandstalige foutmelding. Bewust fail-closed: geen stille
herschrijving die de werkelijke intentie zou maskeren.
VULN 2 - project-overname via de wizard
De create-flow schreef
projects/{naam}.yamlzonder bestaans- ofeigenaarscontrole. Een tenant kon de wizard indienen met de projectnaam
van een andere tenant, waarna diens projectbestand stil werd
overschreven en de eigenaar bij de eerstvolgende herladen wisselde.
Fix: de create-paden controleren nu eerst of het bestand al bestaat en
weigeren bij conflict.
process_project_yaml_backgroundkrijgt eenis_new_project-vlag; alleen de wizard-create (_start_project_creation)geeft
Truedoor. De update/edit-flows (router_detail_edit,router.pycomponentverwijdering) blijven met de defaultFalseeenbestaand bestand herschrijven, zodat legitiem bewerken ongewijzigd werkt.
Wijzigingen
opi/manager/project_manager.py: namespace-pinning inget_deploymentsopi/core/simple_background.py: bestaanscontrole op beide create-padenis_new_project-parameteropi/web/router_wizard.py: wizard-create geeftis_new_project=Truedoortests/test_tenant_isolation_namespace.py: regressietestsTestplan
uv run pytest tests/test_tenant_isolation_namespace.py -q- 7 passeduv run ruff check .+ruff format+pyrightschoon