fix(netpol): vervang per-tenant allow-all NetworkPolicy door least-privilege baseline#76
Conversation
Blokker 1: File-collision bij multi-component deployments
Fix: één policy per namespace berekenen na alle componenten (union van Blokker 2: Shared PostgreSQL/Redis worden niet meegenomen
Idem voor shared Redis ( Fix: detecteer ook Blokker 3: SSO/Keycloak-egress volledig gebrokenGeen In Fix-opties:
Andere belangrijke punten
|
Follow-up: 1 van 3 blokkers deels aangepakt, 3 persisterenCommit `364b9cf1` adresseert Blokker 3 (Keycloak/Redis egress) deels — maar op ODCN nog steeds verkeerd. Plus Blokker 1 en 2 zijn niet aangeraakt. Verifieerd in worktree: Blokker 1 — file-collision multi-component (ONGEWIJZIGD)`project_manager.py:4536-4538`: `generate_network_policy_manifest_name("tenant-baseline")` retourneert `tenant-baseline-network-policy` zonder component-suffix (`utils/naming.py:1481`). Beide `generate_network_policy_name` en `generate_network_policy_manifest_name` zijn niet ge-parametriseerd op component. Per-component loop schrijft naar hetzelfde pad → laatste component wint. Fix-richting: bereken de union van `component_uses_*` flags vóór de component-loop, render één policy per namespace na de loop. Dat past ook beter bij de semantiek (`podSelector: {}` is namespace-breed, niet component-specifiek). Blokker 2 — shared PostgreSQL routing (ONGEWIJZIGD)`project_manager.py:4500-4503`: route gaat naar `get_infrastructure_namespace(cluster, project_name)` (= `rig-prd-{project}-infrastructure`) zodra `component_uses_postgresql` true is. Maar de flag pakt zowel `POSTGRESQL_DATABASE` (shared) als `NAMESPACE_POSTGRESQL_DATABASE` (dedicated). Shared Postgres op ODCN woont op `rig-prd-operations` (`cluster_config.py:97`: `rig-db-rw.rig-prd-operations.svc.cluster.local`), niet in de infrastructure-namespace van het project. Gevolg: een component met `service: postgresql-database` (shared) krijgt egress naar de verkeerde namespace en kan de DB niet bereiken onder default-deny. Fix-richting: splits de detectie. Voor `POSTGRESQL_DATABASE` (shared) → `get_namespace(cluster)` (= `rig-prd-operations`). Voor `NAMESPACE_POSTGRESQL_DATABASE` (dedicated) → `get_infrastructure_namespace(cluster, project_name)`. De twee zijn niet uitwisselbaar. Blokker 3 — Keycloak egress (DEELS, MAAR ALSNOG VERKEERD OP ODCN)`project_manager.py:4521-4522` voegt nu `get_cluster_namespace(cluster)` + `ingress-nginx` toe als `component_uses_sso`. Twee problemen:
`cluster_config` heeft `keycloak_discovery_url` (publieke FQDN) maar geen `keycloak_namespace` en geen `ingress_controller_namespace`. Beide zijn nodig om dit per cluster correct te configureren. Fix-richting: voeg toe aan `cluster_config.py`:
Plus helpers `get_keycloak_namespace(cluster)` en `get_ingress_controller_namespace(cluster)`. Gebruik die in plaats van de hardcoded `"ingress-nginx"` en `get_cluster_namespace`. Aanvullend — template default `ingress_namespace` ook verkeerd op ODCN`tenant-baseline-network-policy.yaml.jinja:20`: `{{ ingress_namespace | default("ingress-nginx") }}`. `project_manager.py:4530-4535` zet `ingress_namespace` niet als template-variabele, dus de default kicks in voor élke cluster — inclusief ODCN waar het `openshift-ingress` moet zijn. Gevolg: gepubliceerde webapps op ODCN raken hun ingress-traffic kwijt. Zelfde fix-richting als hierboven. Tests`tests/test_tenant_baseline_netpol.py` (9 tests) groen, maar dekt geen van bovenstaande scenarios. Tests bevestigen alleen template-shape + grep-niveau code-presence. Voor merge-blokkade is een testset nodig die:
Wat te doenMijn voorkeur: deze PR opsplitsen of zware revisie in plaats van augmentation-by-comment. De vier punten raken `tenant-baseline-network-policy.yaml.jinja`, `cluster_config.py`, `project_manager.py` en `naming.py`/`utils` — dat is een herontwerp van de policy-emissie. Bestaande projecten gaan stuk bij eerste reconciliation als deze in huidige vorm landt op ODCN. Als je liever hebt dat ik de augmentation push (zoals bij #75), zeg het en ik doe het in één commit met testdekking voor de drie blokkers. Geen merge in huidige vorm. |
Conclusie na overleg: niet mergen in huidige vormTwee overwegingen die deze PR opzij zetten: 1. Mismatch met de bestaande design-doc`features/restrictive-network-policies.md` beschrijft een per-deployment model: pods praten alleen met andere pods uit hun eigen deployment (via het bestaande `app: ` label) plus gedeelde infra. Deze PR doet per-namespace met `podSelector: {}` — een tenant-pod kan alle andere tenant-pods in dezelfde namespace bereiken, ook van losstaande deployments. Dat is een ander (lossere) scope dan de design-doc voorschrijft. De PR-omschrijving erkent dit zelf ("`features/restrictive-network-policies.md` beschrijft een breder herontwerp ... buiten de scope van deze fix"). Maar dat "bredere herontwerp" is exact het werkelijk gewenste model. Een tussenstap die het verkeerde scope-niveau pakt creëert later weer een migratie. 2. Internet-egress hoeft niet zo striktHet oorspronkelijke probleem (cross-tenant netwerktoegang tot gedeelde datastores) zit aan de ingress-kant van de datastores, en is in PR #75 opgelost (`namespaceSelector: {}` → expliciete allowlist op postgres/minio/vault). Tenants strak op egress naar het internet zetten is een ander probleem — niet noodzakelijk op dezelfde merge-as. Voorstel
Drie blokkers en het scope-mismatch maken deze PR niet de juiste landingsplaats. Geen kritiek op het werk — het probleem dat de PR adresseert is reëel — maar de definitieve fix verdient een implementatie die de design-doc volgt. |
Refactor gepusht (`ad8de54b`): per-deployment baseline + permissieve egressNa overleg is de aanpak veranderd: in plaats van een strikte default-deny per component die te veel gevallen miste, levert deze commit een permissieve baseline per deployment. Cross-tenant verkeer is dicht; internet egress (HTTP/HTTPS) blijft open. Stricter regelen volgt later via `features/restrictive-network-policies.md`. Wat is veranderdPer-deployment in plaats van per-component
Pod-selector via een nieuw `deployment` label
Per-cluster benoemde namespaces in plaats van hardcoded strings
Baseline-inhoud (permissief maar gescoped):
Pre-merge gate (verplicht)
Wat NIET in deze PR zit (bewust)
Tests: 17/17 groen via `pytest tests/test_tenant_baseline_netpol.py --noconftest`. Ruff/format/pyright pre-commit schoon. |
Follow-up issues aangemaakt
|
uittenbroekrobbert
force-pushed
the
fix/tenant-baseline-netpol
branch
from
f6d26d7 to
5566538
Compare
This doc belongs to a different scope (PR #76 era). Will be re-introduced in its proper PR if needed.
…ivilege baseline
OPI schreef in elke tenant-namespace een allow-all NetworkPolicy
(podSelector {}, ingress [{}], egress [{}], beide policyTypes). Dat hief
egress-containment platformbreed op: elke tenant-pod kon bij datastores van
andere tenants, de Kubernetes API, cloud-metadata en willekeurige
internetbestemmingen, en een latere default-deny werd door deze egress [{}]
teniet gedaan. Het was geen opt-in.
De allow-all template is verwijderd en vervangen door
tenant-baseline-network-policy.yaml.jinja: default-deny met expliciete,
gescopete uitzonderingen. Toegestaan: DNS-egress naar kube-dns, verkeer
binnen de eigen namespace, ingress vanaf de ingress-nginx controller (zodat
gepubliceerde webapps bereikbaar blijven), en egress naar exact de
datastore-namespaces die het project heeft aangevraagd (PostgreSQL in de
infrastructure-namespace van het project, MinIO alleen als minio-storage is
gevraagd). Beide aanroeppunten in project_manager.py (infrastructure-namespace
en per-component manifestlijst) emitteren nu de baseline.
Breder netwerk blijft mogelijk maar moet expliciet/opt-in via het projectbestand
(extra_egress_cidrs), nooit als default.
De default-deny baseline scopete egress alleen naar PostgreSQL en MinIO. Componenten die Redis of SSO (Keycloak) gebruiken kregen wel de bijbehorende secrets en env-vars, maar geen egress-regel naar de Redis-service of de Keycloak back-channel. Onder de default-deny baseline brak daardoor elke Redis- of SSO-afhankelijke tenant-app. Redis (gedeeld en namespace) resolvet via get_redis_server() naar rig-redis.<cluster-namespace>. SSO-componenten draaien de OIDC back-channel tegen Keycloak, dat via de publieke ingress-hostname loopt en dus ook door ingress-nginx hairpint. Beide egress-doelen worden nu toegevoegd, met deterministische deduplicatie. Test uitgebreid zodat deze regressie niet opnieuw kan optreden.
Lost de drie blokkers uit de eerdere review op door de baseline-emissie
te herontwerpen:
1. File-collision (per-component overschrijven): de baseline wordt nu
één keer per deployment gerendered, na de component-loop, met een
deployment-specifieke resource- en bestandsnaam
(`{deployment}-tenant-baseline-network-policy`). Twee componenten
in dezelfde namespace botsen niet meer.
2. Shared Postgres / Redis / Keycloak routing: de policy verwijst naar
per-cluster benoemde namespaces in plaats van per-component-gehakte
service-detectie. Nieuwe `cluster_config` velden +
`get_backup_namespace()` en `get_ingress_controller_namespace()`
helpers. Op odcn-production matchen die nu de realiteit:
`rig-prd-operations`, `rig-prd-backup`, `openshift-ingress`.
3. Per-cluster ingress controller: hardcoded `ingress-nginx` is weg,
loopt nu via `get_ingress_controller_namespace(cluster)`.
Selector op pod-niveau via een nieuw `deployment: <name>` label op
deployment.yaml.jinja, zodat meerdere deployments in dezelfde namespace
elk hun eigen policy hebben en helm/helmfile pods (zonder dit label)
buiten de policy vallen — daar geldt status quo (default-allow) tot een
aparte baseline volgt.
De baseline staat policy default-deny binnen het pod-vlak, maar laat
internet egress (HTTP/HTTPS) bewust open (m.u.v. het cloud-metadata IP).
Verdere internet-restrictie is een follow-up uit
features/restrictive-network-policies.md.
Tests volledig herschreven (17/17 groen, geen FastAPI-collectie nodig)
om de nieuwe shape, per-cluster overrides en het pod-label te dekken.
Cloud metadata IP is op ODCN niet bereikbaar (geverifieerd via curl vanuit pod: connection refused). De except-clausule blokkeerde niets en gaf valse suggestie dat we tegen iets reëel beschermden.
Per ODCN docs (docs/knowledge/odcn-ingress-controller.md): de aanbevolen NetworkPolicy match is pod-selector op ingresscontroller.operator.openshift.io/deployment-ingresscontroller naast namespaceSelector. Daarmee alleen de 'rig' router-pods, niet eventuele andere customer-routers in dezelfde namespace. cluster_config heeft nu ingress_controller_selector (dict met namespace + pod_labels). Sandbox/local laten pod_labels leeg (nginx geen OpenShift- specifieke labels). Plus: test_internet_egress_is_permissive — geen 169.254 exception meer (eerder verwijderd want IP is op ODCN niet bereikbaar).
Rebase op main introduceerde een dubbele dict-key — ruff F601 catchte het in CI.
uittenbroekrobbert
force-pushed
the
fix/tenant-baseline-netpol
branch
from
de57169 to
bf24be7
Compare
… 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.
- ContainerImageConverter.write retourneert None bij lege invoer, samen met remove_when_none=True op COMPONENT_IMAGE_EDITABLE wordt de key uit het YAML weggelaten i.p.v. als lege string geschreven. - Schema-pattern accepteert nu ook een lege string als vangnet voor bestaande YAMLs of code-paden die nog "image: " kunnen schrijven. Voorheen accepteerde de wizard een lege image, maar wees de schema-validator het project later af met "'' does not match ^[A-Za-z0-9]...".
…validatie De asyncio-pump die als vervanger voor de oude shell-pipeline werd geintroduceerd had subtiele race-bugs (WriteUnixTransport closed=True tijdens stream). Vervangen door een subprocess.Popen-keten (pg_dump | sed | psql) via OS-pipes, gewikkeld in asyncio.to_thread, zodat de event-loop niet blokkeert maar het OS de stream zelf afhandelt. Defense-in-depth aan de subprocess-grens: - _validate_in_cluster_host: alleen K8s service-namen of *.svc.cluster.local FQDNs worden geaccepteerd, externe hosts geweigerd. - _validate_pg_port: allow-list (op dit moment alleen 5432). - _validate_pg_password_safe: NUL/CR/LF in wachtwoord geweigerd om argv-/env-injectie via een wachtwoord te voorkomen. - Schema-naam wordt als identifier behandeld en geparseerd door pg_dump zelf; sed doet alleen de regel-geankerde 'CREATE SCHEMA ' -> 'CREATE SCHEMA IF NOT EXISTS ' rewrite. De obsolete _rewrite_create_schema-helper en bijbehorende test_pgdump_clone_rewrite.py vervallen; sed neemt de rewrite over.
Na de centrale CSRF-bescherming (#71) miste de Herverwerken-knop in het danger-confirm-modal de X-CSRF-Token header. Klikken faalde daardoor met een 403 op /projects/<naam>/refresh.
Zonder vaste SECRET_KEY mint Field(default_factory=generate_secret_key) per process een nieuwe random key, waardoor dev-sessies elke pod-restart verlopen. Vaste plaintext-key in de sandbox-overlay (niet-internet-facing dev cluster) houdt sessies stabiel tussen rebuilds. Raakt alleen sandboxed-local; odcn-production overlay is ongewijzigd.
2 participants
Probleem
OPI genereerde in elke tenant-namespace een allow-all NetworkPolicy
(
podSelector: {},ingress: [- {}],egress: [- {}], beide policyTypes).Dat was geen opt-in. Gevolg: elke tenant-pod had onbeperkte ingress en egress
en kon bij:
Bovendien werd elke later toegevoegde default-deny door deze
egress: [{}]volledig teniet gedaan. De policy werd op twee plaatsen onvoorwaardelijk
weggeschreven naar GitOps: de infrastructure-namespace en de per-component
manifestlijst in
project_manager.py.Wijziging
allow-all-network-policy.yaml.jinjais verwijderd en vervangen doortenant-baseline-network-policy.yaml.jinja. De baseline is default-deny metexpliciete, gescopete uitzonderingen.
Toegestaan:
kube-dnspods.ingress-nginxcontroller, zodat gepubliceerde webappsbereikbaar blijven.
het project (afgeleid uit
deployments), zodat de app zijn eigen PostgreSQLblijft bereiken.
aangevraagd. PostgreSQL via de
*-infrastructurenamespace van het project(alleen als de component
postgresql-database/namespace-postgresql-databasegebruikt), MinIO via de cluster-operations namespace (alleen als
minio-storageis gevraagd).Geweigerd: al het overige ingress- en egressverkeer. De ongerestricteerde
egress: [{}]is volledig weg, wat de kern van de fix is.Breder netwerk blijft mogelijk maar moet expliciet/opt-in via het
projectbestand (
extra_egress_cidrsin de template), nooit als default.Werking blijft intact
(egress naar de infrastructure-namespace + ingress in die namespace vanaf de
app-namespaces).
Tests
Nieuw:
tests/test_tenant_baseline_netpol.py(rood-groen, 9 tests). Rendert detemplate en assert dat er geen lege allow-all ingress/egress-regel in zit en dat
DNS + gescopete regels aanwezig zijn; assert dat
project_managerde baselineemit (niet de allow-all) op beide aanroeppunten en dat de oude template
verwijderd is. Geslaagd: 9/9.
ruffenpyrightschoon op de gewijzigdePython.
De volledige suite faalt bij collection door een bestaande, los van deze
wijziging staande breuk (Python 3.14.0b4 + pydantic 2.12.5
import fastapi).De nieuwe test importeert bewust geen FastAPI en draait geisoleerd via
pytest tests/test_tenant_baseline_netpol.py --noconftest.Residu
features/restrictive-network-policies.mdbeschrijft een brederherontwerp (permissive/restrictive modi, aparte default-deny + deployment
policies) en verwijst nog naar de oude template. Dat is planningsdocumentatie,
geen code, en buiten de scope van deze fix. De kern (verwijderen van de
ongerestricteerde
egress: [{}]en het vervangen door een gescopete baseline)is hier afgerond.
Bevinding is pre-existing en onafhankelijk van andere openstaande fixes.