From f486dcbbfcd3014357032f1de9c3f1f9da35c30e Mon Sep 17 00:00:00 2001 From: Julian Cuni Date: Sat, 4 Jul 2026 13:41:32 +0200 Subject: [PATCH] docs(wiki): vision-service hardening backlog + boot-migration data-seed note Two unrelated leftover wiki edits from earlier sessions: - NEW concepts/vision-service-hardening.md: the prioritised to-do list from the 2026-07-02 code + security reviews of apps/vision/ (DoS gaps, unauthenticated/ operator-writable model weights, 0.0.0.0 default bind). Cross-linked from opencv-anpr-service.md ("consult before touching this service"). - container-deployment.md: note that a boot-time migration can be a DATA SEED (e.g. an RBAC permission granted to the operator role via INSERT OR IGNORE), and that a built-in-role grant does not auto-apply to a custom role. Claude-Session: https://claude.ai/code/session_01Xcm6ikLgGoCxxHrxtjkk5V --- wiki/concepts/vision-service-hardening.md | 169 ++++++++++++++++++++++ wiki/decisions/container-deployment.md | 7 +- wiki/entities/opencv-anpr-service.md | 5 + 3 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 wiki/concepts/vision-service-hardening.md diff --git a/wiki/concepts/vision-service-hardening.md b/wiki/concepts/vision-service-hardening.md new file mode 100644 index 0000000..66281c1 --- /dev/null +++ b/wiki/concepts/vision-service-hardening.md @@ -0,0 +1,169 @@ +--- +type: concept +tags: [parking, vision, anpr, security, hardening, tech-debt] +sources: [] +updated: 2026-07-02 +status: open +--- + +# Vision Service — Hardening & Fix Backlog + +Tracked findings against the [[opencv-anpr-service]] (`apps/vision/`, the Python/FastAPI ANPR +microservice — see [[vision-service-packaging]]). Raised by two code reviews on 2026-07-02: a +general pass (bottlenecks / bugs / best-practice) and a security-focused pass. **Nothing here is +fixed yet** — this page is the to-do list; tick items off (and note the commit) as they land. + +Framing that shapes the priorities below, both from [[threat-model]]: + +- **A forged image cannot open a barrier by itself.** The Node server pushes snapshot bytes to + `/analyze`, then **re-gates** the result server-side (`VISION_ENTRY_MIN_CONFIDENCE` = 0.85, + stricter than the service's own advisory 0.5 floor), with debounce and a mid-poll + "already-transacted" abort (`apps/server/src/anpr-entry.ts`). So the real exposure is + **availability (DoS)** and **network/weight placement**, not decision forgery. See + [[lane-presence-and-anpr-entry]], [[fail-state-safety]] (recognition is advisory; the host falls + back to the ticket path when vision is down/unsure). +- **The primary adversary is the local booth operator**, with filesystem/USB access to the + appliance — which is what makes the model-weight and config-placement items real, not theoretical. + +## Priority 1 — DoS (remotely triggerable, zero appliance access) + +1. **Request body buffered *before* the size cap** — `apps/vision/vision_service/app.py:69`. + `image = await request.body()` concatenates the whole stream into memory before + `len(image) > MAX_IMAGE_BYTES` (12 MB) runs. A chunked POST with no `Content-Length` streams + unbounded bytes → RAM exhaustion → OOM-kill **before** the 413 is ever returned. Killing the + recognizer forces permanent ticket-fallback (which may be the operator's goal — suppress plate + evidence). **Fix:** reject on `Content-Length` up front and cap while draining + `request.stream()`; pass `--limit-max-request-body-size` to uvicorn. + +2. **Pixel-bomb: `cv2.imdecode` has no decoded-dimension bound** — + `apps/vision/vision_service/recognizer.py:143`. A crafted JPEG *under* the 12 MB byte cap can + declare enormous dimensions (e.g. 30000×30000) and decode to a multi-GB BGR ndarray. Combined + with item 3 (inference on the event loop), one request both spikes memory and stalls the whole + service. **Fix:** check `frame.shape` against a max pixel budget immediately after decode; reject + oversize frames with a 422. + +## Priority 2 — Bottleneck (also a DoS amplifier) + +3. **CPU-bound inference runs synchronously on the asyncio event loop** — + `apps/vision/vision_service/app.py:84` (calls `rec.analyze`, which does `cv2.imdecode` + ONNX + YOLO+OCR). uvicorn runs a single event loop; while inference runs (hundreds of ms on the i5-8500, + CPU-only), **nothing else is served — including `GET /health`**. Back-to-back entry/exit + snapshots serialize, and health probes time out, making a live service look down. When the Node + client's 1500 ms `AbortController` gives up, Python keeps burning CPU on the abandoned request. + Flagged independently by ~half the review angles — the one real bottleneck. **Fix:** + `await run_in_threadpool(rec.analyze, image)` (or make the `Recognizer` protocol async), plus an + `asyncio.Semaphore(1–2)` to bound concurrent inferences. This is a property the `Recognizer` + mechanism should own, not a per-call patch. + +## Priority 2 — Security: network placement & weight integrity + +4. **Model weights are unauthenticated *and* operator-writable → persistent recognition-poisoning.** + `fast-alpr`'s `ALPR()` downloads ONNX weights + config over plain `urllib` with **no + checksum/signature** (verified in `.venv`: `open_image_models/detection/core/hub.py`, + `fast_plate_ocr/inference/hub.py`), and a cache-hit is treated as trust (present → skip, never + re-verify). The `vision` user (uid 999) owns **both** the `~/.cache` weights and the runtime + process, so any same-user write primitive overwrites weights in place — no privesc — surviving + container **restarts** (cache is in the writable layer, not re-verified at startup). Under + [[threat-model]], a local operator overwriting a cached `.onnx` is a persistent, targeted fraud + primitive: a detector tuned to never see a specific plate, or an OCR model that reproducibly + substitutes a character. **Fix:** pin + verify a weights hash, mount the weights **read-only**, + and assert a known hash at startup. Relates to [[disk-os-hardening]] (physical-tamper chain), + [[reconciliation]] (the real backstop). + +5. **Service binds `0.0.0.0` by default at every layer** — `settings.py:18`, `Dockerfile:51` + (`ENV VISION_HOST=0.0.0.0`), `Dockerfile:57` (uvicorn `--host 0.0.0.0`). The `.env.example` + documents `127.0.0.1`-only intent, but loopback-only exposure then depends entirely on the + compose `ports:` prefix being right every time. The app provides **zero** defense-in-depth: + `/analyze` and `/health` have no auth, so reachability is the only control. **Fix:** make + `127.0.0.1` the default at all three layers; require explicit opt-in to widen. Relates to + [[network-isolation]], [[trust-boundary]]. + +6. **Dev compose publishes vision on all interfaces** — `docker-compose.dev.yml:29` maps + `"8089:8089"` (binds 0.0.0.0), unlike `docker-compose.prod.yml:91`'s `"127.0.0.1:8089:8089"`. + Anyone on the same LAN as a dev/staging box — or a field deploy that reuses the dev file — can + hit the unauthenticated endpoint: fingerprint the model via `/health`, run the DoS above, or + probe confidence behaviour. **Fix:** bind `127.0.0.1:8089:8089` to match prod. See + [[container-deployment]]. + +7. **No container resource limits or hardening.** The `vision` service has no + `mem_limit`/`cpus`/`pids_limit` and no `security_opt: [no-new-privileges:true]` / + `cap_drop: [ALL]` / `read_only` in any compose file. Since prod runs the server on the **host + network** doing safety-critical device I/O, an unbounded vision container (via items 1–2) can + starve the host of RAM/CPU. **Fix:** add `mem_limit`, `pids_limit`, `cap_drop: [ALL]`, + `no-new-privileges`. (Already good: runs non-root as uid 999; `.env` is **not** baked into any + image layer — `COPY`s are explicit, root `.dockerignore`/`.gitignore` cover `.env`.) See + [[disk-os-hardening]], [[container-deployment]]. + +## Priority 3 — Correctness, robustness, hygiene + +8. **`env_file=".env"` resolves against the process CWD** — `settings.py:16`. Launched from anywhere + but `apps/vision/` (a systemd unit, or a run from the monorepo root), the `.env` isn't found and + pydantic-settings raises **no error** — the service silently boots `recognizer="stub"`, `/health` + reports `ready: true`, and every scan returns `plate: null`. Production ANPR silently does + nothing while looking healthy. **Fix:** anchor the path, e.g. + `env_file=Path(__file__).parent.parent / ".env"`. + +9. **`/health` returns HTTP 200 even when `ready` is false**, and the Docker HEALTHCHECK + (`Dockerfile:55`) only checks `status==200` — `app.py:54`. If the fast-alpr weights fail to load + (e.g. the best-effort build pre-warm was skipped and the appliance is air-gapped), the container + stays "healthy" to Docker forever: no restart, no infra alert; only the in-app device monitor + notices. **Fix:** return 503 from `/health` when not ready, or make the HEALTHCHECK parse the + `ready` field. + +10. **Build pre-warm swallows *all* failures** — `Dockerfile:46` + (`... || echo "skipped (no network)"`). It can't distinguish "no network at build, expected" + from "download corrupted/tampered/interrupted". A skipped pre-warm silently converts the + air-gapped appliance into one that fetches weights from github.com on the **first real + `/analyze`** — an unreviewed runtime network dependency contradicting [[offline-first]] (and a + first-scan DoS if egress is truly blocked). **Fix:** fail the *prod* build on pre-warm failure + (or assert weights present at startup) rather than degrade to a lazy fetch. + +11. **Raw exception strings reflected into HTTP responses** — `app.py:88` (`analysis failed: {exc}`) + and the 503 loader path at `app.py:81`. Echoes native cv2/onnxruntime error text (absolute + paths, library versions) to any caller. **Fix:** log detail server-side; return a generic + message to the client. (Modest severity — the primary adversary already has local access.) + +12. **`min_confidence` has no bounds validation** — `settings.py:33`, unlike `PlateResult.confidence` + (`ge=0, le=1`). `VISION_MIN_CONFIDENCE=50` (someone thinking in percent) silently flags every + read as `low_confidence`, with no startup error or health symptom. **Fix:** + `float = Field(0.5, ge=0.0, le=1.0)`. + +13. **Node client discards the structured error detail** — `apps/server/src/vision-client.ts:187`. + `#post` throws `vision /analyze → HTTP 422` without reading the body, so + `"could not decode image bytes"` (corrupt camera frame — actionable) is indistinguishable in the + logs from a deploy problem. **Fix:** read `detail` from the JSON body before throwing. + (Server-side file, just outside `apps/vision/`, but it's the consumer of this contract.) + +14. **`build_recognizer` docstring is wrong** — `recognizer.py:169`. It says "falls back to the stub + if the real one can't load," but the function returns the **not-ready** `FastAlprRecognizer` + (better behaviour — `/analyze` 503s instead of silently returning no-plate). **Fix:** correct + the docstring so nobody "fixes" the code to match it. + +15. **Model-name env vars are undefended in-repo (latent)** — `settings.py:28–29`. + `detector_model` / `ocr_model` are plain `str`; the allowlist that makes them safe (rejects + unknown names) lives **entirely** in the third-party libs, not here. **Not a bug today** (no + SSRF/traversal), but a future recognizer swap (the pluggable design invites one) could turn an + operator-settable env var into a URL/path primitive. **Fix:** constrain to a `Literal`/enum or + validate explicitly. + +## Lower priority / noted (not scheduled) + +- **Base image tag-, not digest-, pinned** — `Dockerfile:9` + (`ghcr.io/astral-sh/uv:python3.12-bookworm-slim`); `apt-get` without package pinning + (`Dockerfile:16–18`). `uv sync --frozen` locks the Python deps, so this is OS-layer drift only — + low priority hardening. +- **CLI reads image files unbounded** — `cli.py:55` (`read_bytes()`, no size cap). Dev-only tool, + local invocation; mirror the HTTP size discipline for consistency. +- **Architecture note (server, not vision):** `anpr-entry.ts` auto-opens the barrier on a + high-confidence plate match with **no second factor**, so a printed duplicate of a known + subscriber's plate is a physical-spoofing bypass (mitigated only by the signed event log, once a + hardware signer lands — see [[hardware-signer-options]], [[append-only-event-chain]]). A conscious + design sign-off, not a vision-service bug — flagged so it isn't an accident. See + [[lane-presence-and-anpr-entry]], [[plate-reconciliation]]. + +## Clean (checked, no action) + +Content-type is **not** trusted for parsing (cv2 sniffs bytes, ignores the header). The service +already runs non-root (uid 999), loads models once at startup (not per request), and `.env` is not +committed or baked into an image layer. The `getattr`-based fast-alpr result mapping, the typed +`app.state` accessors, and the lifespan factory all have stated rationales and are fine as-is. diff --git a/wiki/decisions/container-deployment.md b/wiki/decisions/container-deployment.md index 1d0f625..31dba36 100644 --- a/wiki/decisions/container-deployment.md +++ b/wiki/decisions/container-deployment.md @@ -92,7 +92,12 @@ prod); `ENV=dev` switches to the dev override. - **Migrations at boot, not at build.** The DB lives on a mounted volume (`/data`), so the entrypoint runs them against the live file via a **drizzle-kit-free** runtime migrator (`packages/db/scripts/migrate-runtime.mjs`, using `drizzle-orm/.../migrator` — drizzle-kit is a - devDep, pruned from the prod bundle). Idempotent: a restart re-applies nothing. + devDep, pruned from the prod bundle). Idempotent: a restart re-applies nothing. **A migration is + not only schema** — it can also be a **data seed** (e.g. a new RBAC permission granted to the + `operator` role via `INSERT OR IGNORE`, so a new operator capability reaches the booth on the next + deploy). The whole `@parking/db` package ships in the bundle (no `files` allowlist), so every + `drizzle/*.sql` is present in the image. NB a permission seeded to the built-in `operator` role does + NOT auto-apply to a **custom** role — an admin toggles it in Setup → Roles. - **JWT_SECRET** must be a real value at deploy — `auth.ts` rejects anything `<32` chars or matching `change.?me|insecure|dev-only`, so the dev compose default is a benign 32-char string, not a "dev-only…" placeholder (which would crash boot). diff --git a/wiki/entities/opencv-anpr-service.md b/wiki/entities/opencv-anpr-service.md index 4185845..065de3e 100644 --- a/wiki/entities/opencv-anpr-service.md +++ b/wiki/entities/opencv-anpr-service.md @@ -230,6 +230,11 @@ service's `/health` each tick and shows a **"Vision" chip** in the booth footer > vision service must reach that VLAN to pull snapshots — but its own `/analyze` should bind > **localhost** (Node is the only caller). Keep the AGPL/heavy stack contained to this process. +> **Hardening / fix backlog** ([[vision-service-hardening]]): the 2026-07-02 code + security reviews +> logged a prioritised to-do list against `apps/vision/` — DoS gaps (body-cap-after-buffering, +> pixel-bomb, inference on the async event loop), unauthenticated + operator-writable model weights, +> and `0.0.0.0`-by-default binding lead it. **Consult it before touching this service.** + ## Open - **Recognizer choice** — **fast-alpr (MIT, YOLOv9+CCT on ONNX) is the baseline, AL-benchmarked**: the