fix: verwijder onbereikbare git-repo-aanmaakcode (latente RCE op git-host)#72
Conversation
|
LGTM — geverifieerd op de PR-branch. Geverifieerd
Eén kleine opvolg-observatie (geen blocker)Na merge zijn Pre-existing buiten scope
Bij eventueel toekomstig herintroducerenMocht remote-repo-aanmaken ooit opnieuw nodig zijn: |
|
Cleanup-commit toegevoegd ( Wat is bijgewerkt
Verificatie
Onafhankelijk audit van de originele PR-commit (5ffc43d)Vóór deze cleanup heb ik de dead-code claim onafhankelijk nagelopen:
Geen dynamic dispatch ( Verder zijn er geen andere ssh-string-interpolation-patronen in de codebase — dit was de enige instantie van het anti-pattern (subprocess_exec → ssh user@host "string-met-tenant-input"). Architectonisch contextDe keten lijkt een artefact van een vroeger ontwerp waarin OPI tenant-git-repos zelf aanmaakte via SSH naar een centraal git-server. Het huidige model gebruikt externe GitHub-repos voor tenant-app-code en in-cluster Forgejo (HTTP) voor sandboxed-local. Verwijderen is consistent met de actuele architectuur, niet alleen een security-fix. Pre-existing observatie (geen actie op deze PR)
Audit: cleanup-commit direct op de PR-branch gepushed. Pre-push pytest-hook groen. |
De SSH-key onder keys/git-server-key was van een oude lokale Docker-git- daemon opzet (host.docker.internal:2222). Productie + sandbox gebruiken HTTPS naar GitHub met PATs in operations-manager-env-secrets; geen enkele URL begint met git:// of ssh://. De key werd nergens gelezen. Verwijderd: - keys/git-server-key, keys/git-server-key.pub (tracked files) - /app/keys Secret-volume + mount in base/deployment.yaml De resterende cleanup (config.py env-vars, dood SSH-URL-pad in argo_manager/git_monitor) komt na merge van #72 in een aparte PR.
fix(security): hygiene cleanup van development keys, dummy env en local cluster-role De keys/git-server-key files in de repo waren ontwikkel-keys voor de oude lokale Docker-git daemon (host.docker.internal:2222). Geen productie-risico — productie GitOps gaat via HTTPS+PAT in een SOPS-Secret — maar wel overbodig in de repo en image-lagen. Tegelijk de local/sandbox cluster-role getrimd zodat de minimum-rechten netjes gedocumenteerd staan voor de operations-manager ServiceAccount. Wijzigingen: - Dockerfile: COPY van keys/git-server-key, .env en .env.production verwijderd (waren dev-files, niet productie-secrets) - keys/git-server-key, keys/git-server-key.pub uit repo verwijderd - /keys/ toegevoegd aan .gitignore (forward-protection) - base/deployment.yaml: ongebruikte git-server-key Secret-volume weg - local + sandboxed-local ClusterRole namespace-manager: cluster-brede secrets list/update verwijderd; create/get/patch behouden voor de bootstrap-flow - odcn-production overlay: _blueprint-cluster-role.yaml en _blueprint-cluster-binding.yaml als referentie van OPI's minimum-rechten (niet in kustomization; productie krijgt rechten via Capsule per-tenant RoleBindings naar de built-in admin ClusterRole) - config.py: DEBUG default → False, zodat productie zonder expliciete env-set niet per ongeluk in debug-mode draait Follow-ups op project ZAD: #90 (per-namespace Role+RoleBinding voor secrets), #91 (git-server-key runtime-pad opruimen na #72), #92 (image digest-pin), #93 (ODCN coördinatie cluster-role parity), #104 (KEYCLOAK_MASTER_OIDC naar prod ConfigMap).
…host)
GitConnector.create_repository, de module-helper create_git_repository
en ProjectManager.create_project_repository bouwden remote shell-commando's
als ongeescapete strings ("mkdir -p /srv/git/{repo_name}.git",
"git init -b main --bare ...") die als enkel argument aan
`ssh host "<cmd>"` werden meegegeven. De remote shell herinterpreteert
die string, en repo_name kwam ongefilterd uit de repo-URL in het
project-YAML (os.path.basename, geen stripping van shell-metatekens).
Dat is een latente kritieke remote command execution op de git-host.
De code was niet bereikbaar: create_project_repository had nergens
in de hele boom een aanroep, en create_git_repository werd verder
alleen gebruikt door het losse, niet door enige testrunner of Taskfile
aangeroepen hulpscript functional_tests/setup_test_infrastructure.py.
Dode code, maar wel een geladen wapen.
Verwijderd: de twee git.py-functies, de project_manager-methode plus
de nu ongebruikte import, de connectors-package re-export, het dode
setup-script en de bijbehorende README-sectie, en de tests die
uitsluitend deze verwijderde code testten (test_git_connector.py
volledig; in test_git_ssh.py alleen test_git_connector_create_repository).
Geen nog-gebruikte code aangeraakt. Gevalideerd met ruff en pyright;
geen resterende verwijzingen naar de verwijderde symbolen in de boom.
After the previous commit removed create_repository / create_git_repository / create_project_repository, these three settings have zero consumers in Python code. Verified with grep over operations-manager/, bootstrap/ and the .env files in all three overlays (odcn-production, sandboxed-local, local) -- no GIT_SERVER_HOST / GIT_SERVER_PORT / GIT_SERVER_USER references remain. GIT_SERVER_KEY_PATH stays: it is still read by opi/core/git_monitor.py:190 when the monitored project URL starts with ssh:// or git:// (the local cluster's docker-compose git-daemon flow). The block's stale 'Legacy / should be removed or fixed in the future' comment is replaced with an accurate description of what the remaining setting actually does. .env in operations-manager/python is the only .env that named these keys; removed those three lines too, kept GIT_SERVER_KEY_PATH.
PR #80 verwijderde al de SSH-key files (keys/git-server-key + .pub) en de volume mount in deployment.yaml. Hierna is de SSH-key-injectie code-pad functioneel dood — een open(/app/keys/git-server-key) zou nu crashen. Restanten: - keys/authorized_keys (alleen gemount door git-test-server) - git-test-server/ docker-compose + README (lokale SSH git-daemon) - functional_tests/ (consumeerde alleen die docker-git server) - GIT_SERVER_KEY_PATH + GIT_ARGO_APPLICATIONS_KEY in config.py - SSH-key file-read in argo_manager.py + git_monitor.py - GIT_ARGO_APPLICATIONS_KEY="" regels uit cluster overlays - GIT_*_KEY regels uit operations-manager/python/.env(.production) - functional_tests-referentie uit operations-manager/CLAUDE.md Per-project SSH-keys (AGE-encrypted via project file -> ArgoCD repo Secret) blijven werken via een ander code-pad; deze opruiming raakt alleen het 'globale SSH-key path uit settings' patroon dat niemand meer gebruikt.
uittenbroekrobbert
force-pushed
the
fix/remove-dead-git-rce
branch
from
5875854 to
48a309a
Compare
Update — cleanup uitgebreid (commit 48a309a)Na review en verificatie tegen PR #80 had al de SSH-key files ( Toegevoegd in de cleanup-commitVerwijderd:
Bewust behouden:
Verificatie
|
2 participants
Wat dit was
Drie functies vormden samen een ongebruikte keten om op afstand een bare git-repo aan te maken:
GitConnector.create_repository(statische methode inopi/connectors/git.py)create_git_repository(module-helper inopi/connectors/git.py, riep de statische methode aan)ProjectManager.create_project_repository(opi/manager/project_manager.py, riep de helper aan)Ze bouwden de remote commando's als ongeescapete strings (
mkdir -p /srv/git/{repo_name}.git,git init -b main --bare ...) en gaven die als één argument mee aanssh host "<cmd>". De shell op de git-host herinterpreteert die string.repo_namekwam ongefilterd uit de repo-URL in het project-YAML viaos.path.basename, zonder enige stripping van shell-metatekens. Een URL als.../$(...).gitof...;rm -rf ....gitzou op de git-host als shell uitgevoerd worden: een latente kritieke remote command execution.Waarom verwijderen het juiste is
create_project_repositoryhad nergens in de hele boom een aanroep (geverifieerd met een volledige grep overopi/,tests/,web/,api/,scripts/,functional_tests/, geen dynamische dispatch).create_git_repositorywerd verder alleen gebruikt doorfunctional_tests/setup_test_infrastructure.py, een los hulpscript met een__main__-entrypoint dat door geen enkele testrunner, Taskfile of CI wordt aangeroepen, alleen handmatig genoemd in een README. Geen bereikbare code dus.Een patch met shell-escaping zou dode code repareren die niemand aanroept. De keten weghalen elimineert het latente risico volledig en verkleint het aanvalsoppervlak, zonder dat er gedrag verandert. Wordt later opnieuw remote-repo-creatie nodig, dan hoort dat met
asyncio.create_subprocess_execen argv-lijsten (zonder shell-herinterpretatie) opnieuw te worden opgebouwd.Wat is verwijderd
opi/connectors/git.pyProjectManager.create_project_repositoryplus de daardoor ongebruikt geworden importopi/connectors/__init__.pyfunctional_tests/setup_test_infrastructure.pyen de bijbehorende README-sectietests/test_git_connector.py(volledig), en intests/test_git_ssh.pyalleentest_git_connector_create_repositoryNog gebruikte code (o.a.
create_or_update_file, de SSH-helpers,poll_for_changes,create_git_connector_from_repo_config) is niet aangeraakt.Pre-existing en onafhankelijk
Deze wijziging staat los van de andere openstaande fix-PR's en raakt geen code die zij aanpassen. Het betreft dode code die al in de codebase stond.
Validatie
uv run ruff check . --fix: alle checks geslaagduv run pyrightop de gewijzigde bestanden: 0 errors, 0 warnings (bevestigt geen resterende verwijzingen)create_project_repository,create_git_repository,.create_repository(,GitConnector.create_repositoryensetup_test_infrastructure: nul resultaten na de wijzigingimport fastapifalen); dat staat los van deze PR en is hier niet aangeraakt