Skip to content

security(docker): opt-in non-root Go services with safe volume ownership (#339) - #404

Merged
dborup merged 12 commits into
masterfrom
codex/issue-339-nonroot-containers
Oct 13, 2026
Merged

dborup merged 12 commits into
masterfrom
codex/issue-339-nonroot-containers

Conversation

@dborup-agent

@dborup-agent dborup-agent commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Relates to #339

Summary

Opt-in, default unchanged: CORESCOPE_RUN_AS_NONROOT=1 plus CORESCOPE_UID / CORESCOPE_GID runs only corescope-server and corescope-ingestor as an unprivileged user. Unset, empty, 0 or false keeps today's start on any volume that never used the mode: the helper is not executed and supervisord gets the same config files as before. Any other value, missing or unsafe IDs, or a file in the data directory that the services could have written stops the container with an [nonroot] ERROR: line before anything is sourced, linked or changed.

Review round 2 changed the security model; see "Security boundary" below.

Plan (milestones)

  1. Tests first: Go tests, a no-Docker helper unit test, a CI-run start-path test with supervisord stubbed, and a local full-image Docker test. All red on the previous implementation.
  2. Implementation: the helper, the entrypoint hooks, the Dockerfile wiring, an explicit geo-filter refusal, the admin CLI guard, docs, and CI steps.
  3. Owner-run staging rehearsal (checklist below). Production is decided separately. Nothing here touches any deployment.

Customizer (AGENTS rule 8): no new thresholds or display values. The three variables are deployment settings, so there is nothing to expose in the customizer.

Security boundary

At every start, root sources /app/data/.env and links /app/data/config.json and /app/data/theme.json. The services must therefore never be able to write or replace those files:

  • The services never own the data directory. It keeps its owner. It gets the services' group, group write (SQLite creates -wal/-shm/-journal there) and the sticky bit, so the services cannot remove, rename or replace entries they do not own. A start with CORESCOPE_UID equal to the data directory's owner is refused.
  • Check before use. nonroot.sh preflight runs before .env is sourced and before config.json/theme.json are linked. It refuses the start if any of those files is a symlink, a hard link or not a regular file, is owned by the services' user or by anyone but root or the data directory's owner, or is writable for the services' group or for everyone. A read-only mount (config.json:ro) is accepted whoever owns it. A file the services create later under one of those names is therefore refused, not used. For this check the settings are read from .env as data, so they must be plain KEY=VALUE lines.
  • The check outlives the setting. A root-owned /app/data/.corescope-nonroot file (UID:GID) keeps the check active after the setting is removed, until the rollback clears it. A volume that never used the mode never runs it.
  • Off start after on. With the kernel's fs.protected_regular=2, a common default, root cannot open files it does not own in a sticky, group-writable directory. The first start with the setting off therefore removes group write and the sticky bit again, after the check, and logs it. Rollback step 1 alone keeps restoring service.
  • IDs. The docs and the compose example ask for a dedicated ID (e.g. 10001:10001), not the host owner's.

How it works

docker/entrypoint-go.sh keeps its old path. When the mode is requested (environment or .env) or the volume has the marker, it first runs nonroot.sh preflight. When the setting is on, it then runs nonroot.sh setup <selected conf> before supervisord. setup:

  • validates the settings. CORESCOPE_UID must not be 0 and must not be an existing image account. CORESCOPE_GID must be ≥ 100 and must not be the mosquitto/caddy group.
  • repeats the check with the IDs that were actually sourced, and creates a corescope passwd/group entry.
  • secures the data directory (group, group write, sticky bit). Its owner stays unchanged.
  • changes ownership only where it differs, never recursively, on meshcore.db, ping_scores_history.db and their -wal/-shm/-journal, client-rx-delete.salt, client-rx-delete-audit.jsonl and the four queue directories themselves. Symlinks, hard links and special files are skipped with a warning. A unit test keeps the list in sync with the Go constants.
  • sets aside queue requests (request-*.json, cmd-*.json) that the services did not write: they are renamed to *.held, keep their owner, are reported, and are never run.
  • writes the marker, checks that the services can use the data dir and read config.json, removes a stale root-owned ingestor stats file in /tmp, and writes /run/corescope/supervisord.conf: the selected config plus user=corescope under the two Go programs.

supervisord, the entrypoint and Caddy stay root. Mosquitto still runs as mosquitto and still has PUID/PGID stripped.

Startup cost: a few stat calls, plus chown only for wrong owners. That is a few milliseconds and does not depend on database size. The log reports [nonroot] ownership: N of M path(s) …, which is 0 of M on every later start.

Host-side effect: on a bind mount, the data directory gets group CORESCOPE_GID and mode 1775 (or similar). The listed files and any file the services create are owned by the numeric IDs, and the marker file appears. Your own files keep their owner and mode. They must be readable for the services and not writable for their group.

Geo-filter save

In this mode the server never writes config.json, whether it is mounted :ro or sits writable in the data directory. SaveGeoFilter refuses before touching the filesystem, so the result does not depend on directory ownership or symlink resolution (#422). The response is 409 geo filter not saved: config.json is read-only for the server (…), and the filter is not applied. Other failures keep the existing 500.

Admin commands

Run them as the services' user (docker exec -u UID:GID …). As root, admin delete-client-rx refuses when the database belongs to another user, and writes nothing.

Not changed

  • docker/entrypoint.sh and docker/supervisord.conf belong to the deprecated Node image.
  • docker-compose.yml, docker-compose.no-mosquitto.yml, both staging compose files and the four docker/supervisord-go*.conf files are unchanged. docker-compose.example.yml only gains commented lines.

Migration and rollback (full text in docs/deployment.md, "Running the Go services without root (opt-in)")

From the default mode:

  1. Back up with sh scripts/backup-sqlite.sh <container> /backups, and keep a copy or snapshot of the whole data directory.
  2. Record owners and modes: docker run --rm --pull=never --network none --volumes-from <container> --entrypoint /usr/local/lib/corescope/nonroot.sh "$IMAGE" owners > owners.txt.
  3. Make config.json, theme.json and .env readable but not group-writable for the chosen group.
  4. Add the three variables (plain lines if in .env), choosing a dedicated ID. Restart, then check the [nonroot] log lines and ps.

From the earlier draft of this PR (eb49596f): that build gave the data directory itself to CORESCOPE_UID. With the new image, such a volume is refused at start (… owns the data directory), and so is any root-read file owned by the services' user. Stop the container, check .env, config.json and theme.json for changes you did not make, then run set-owner (or chown the directory back to its previous owner on the host). After that, follow the steps above with a dedicated ID.

Roll back:

  1. Remove the variables and restart. Everything runs as root. The first such start takes back group write and the sticky bit (logged), and the check keeps running until step 2.
  2. With the container stopped, run … nonroot.sh … restore-owners < owners.txt. This restores every recorded owner and mode exactly, gives listed paths created later to root, and removes the marker. Without a record, set-owner 0:0 is the coarse fallback. Both refuse while a root-read file fails the check.
  3. If data looks wrong, restore the backup.

Staging rehearsal checklist

  • Note the deployed image/commit. Take a backup with sh scripts/backup-sqlite.sh corescope-staging-go <backup-dir>, and a full copy or snapshot of the data directory (DBs + WAL, salt, audit log, queue dirs, config.json, theme.json, .env).
  • Record owners and modes with nonroot.sh owners (see the docs) and keep the file next to the backup.
  • Choose a dedicated CORESCOPE_UID/CORESCOPE_GID (e.g. 10001:10001). Do not use the host owner of the staging data dir, and use a group that no host user belongs to.
  • Make config.json, theme.json and .env readable for that group but not writable for it.
  • Make sure no tooling or operator runs ingestor admin commands into the container as root during the rehearsal (use docker exec -u UID:GID).
  • Set rollback triggers and a time box before you start: any [nonroot] ERROR, permission denied/readonly/SQLITE_BUSY in the logs, /api/healthz failing, or ingest stalling for more than a few minutes. Note that the backup helper stops the container briefly.
  • Add CORESCOPE_RUN_AS_NONROOT=1, CORESCOPE_UID=…, CORESCOPE_GID=… to <data-dir>/.env as plain lines, then docker restart corescope-staging-go.
  • Logs show [nonroot] /app/data: group … sticky bit, ownership: N of M … and … run as UID:GID; Mosquitto and Caddy unchanged, with no [nonroot] ERROR.
  • docker exec corescope-staging-go ps -o pid,user,group,args: both Go services show corescope; supervisord and Caddy show root.
  • /api/healthz is OK, packets keep arriving, /api/stats counts rise, and pprof ports still answer.
  • Customizer → geo filter save shows the 409 message, and the filter is not applied.
  • docker restart again: the log shows ownership: 0 of M.
  • Watch the logs for some hours: docker logs corescope-staging-go 2>&1 | grep -iE 'permission denied|readonly|SQLITE_BUSY|\[nonroot\]'.
  • Rollback drill: remove the three lines and restart (everything root; the log shows [nonroot] the mode is off: …). Stop the container, run restore-owners with the recorded file, start it again, and compare the owners with the record.
  • Decide on production separately.

Tests

  • bash scripts/test-nonroot-helper.sh (no Docker, no root, CI): config rendering for all four supervisor variants, setting/ID validation, the path list against the Go constants, Dockerfile and CI wiring, docs.
  • bash scripts/test-nonroot-entrypoint.sh (CI, Docker, no Go build): the real entrypoint and helper as root in the runtime image's pinned Alpine base, with supervisord stubbed. It covers:
    • the off-mode start for unset/empty/0/false × the four DISABLE_* combinations, plus the generated config in on mode;
    • the data-directory model, and 0 of N with an unchanged ctime on the second start;
    • the services' user being unable to modify, replace, remove or re-mode the root-read files (host-owned and root-owned originals);
    • the start check for each kind of tampering, also after the setting is removed;
    • held queue requests;
    • exact owners/restore-owners round trips, refused bad records, and refusal of the rollback commands while a root-read file is untrusted.
  • scripts/test-docker-nonroot.sh <image> [baseline-image] (local, full image, --network none, labelled volumes, self-cleaning):
    • off mode identical to the baseline image across 16 combinations;
    • a fresh volume;
    • a fixture DB with root-owned WAL/SHM;
    • a read-only and an in-volume config (409, file unchanged);
    • toggles and the record/restore rollback;
    • invalid settings;
    • as the services' user in a running container: the root-read files cannot be changed, and a .env created later stops the next start without being sourced;
    • the root admin command refused.
  • Go: geofilter_save_readonly_339_test.go (409 for errnos, plus refusal in non-root mode with every path writable and a symlinked config) and admin_delete_client_rx_owner_339_test.go.

Performance: no hot path is touched. The helper runs once per container start and is O(listed paths + queue entries).

🤖 Generated with Claude Code

dborup and others added 2 commits October 13, 2026 09:05
…r save error (#339)

- scripts/test-nonroot-helper.sh: no-Docker unit test for the opt-in
  helper (supervisor config rendering for all four variants, setting and
  ID validation, the writable-path list against the Go constants, image
  wiring and docs).
- scripts/test-docker-nonroot.sh: local-only runtime test against a built
  image (off-mode start identical to a baseline image, fresh volume,
  root-owned DB with WAL/SHM, read-only config mount, toggle off/on/off,
  supervisor restarts, invalid settings, rollback).
- cmd/server: a geo-filter save that cannot write config.json must answer
  409 with a message naming the arrangement and must not apply the filter.

All three fail on master: there is no helper and the save answers 500
"failed to save config".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ration (#339)

CORESCOPE_RUN_AS_NONROOT=1 with CORESCOPE_UID/CORESCOPE_GID runs
corescope-server and corescope-ingestor as that user. Unset, empty, 0 or
false keep the historical start: the entrypoint never calls the helper and
supervisord gets the same config files as before.

When on, docker/nonroot.sh (run by the entrypoint as root, before any
service) validates the IDs, creates a `corescope` account, gives only the
services' own paths in /app/data to it (the directory itself, both
databases with -wal/-shm, the client-rx salt/audit files and the four flat
queue directories) where the owner differs, never recursively and never
through a symlink or onto a hard link, checks that config.json stays
readable, drops a root-owned stats file left in /tmp by a root run, and
writes /run/corescope/supervisord.conf: the selected config plus
`user=corescope` for the two Go programs. supervisord, Caddy and the
entrypoint stay root; Mosquitto keeps its PUID/PGID stripping.
`nonroot.sh set-owner UID:GID` is the documented rollback.

config.json is read-only for the services in this mode. A geo-filter save
that hits a permission or read-only-filesystem error on create/rename now
answers 409 with a message naming the arrangement (ErrConfigReadOnly);
other failures keep the 500.

Docs: docs/deployment.md (settings, what changes, host-side effect,
startup cost, enable steps with a backup first, rollback) and commented
settings in docker-compose.example.yml. CI runs the helper unit test and
ShellCheck.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stale root-owned stats file left by the off run already reports
tx_inserted=1, so expecting 1 after the in-place switch could pass
without a rewrite. Publish two packets and expect 2; with the stale-file
cleanup removed from the helper, both stats checks now fail.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent1 PR#404 #339 — head eb49596

Status: Draft. Opt-in non-root mode is implemented; with it off, the start is unchanged. Everything local is green (Go, test-all.sh, helper unit 81/81, Docker 213/213 with 16 off-mode snapshots identical to the baseline image, #332 E2E, browser check), and all 30 mutants are killed. CI: Go Build & Test passes. Playwright E2E fails in the unrelated #315 test (passes 6/6 locally, including on the merge tree; not rerun because it is not #271). No staging or production was touched.

Evidence tags: [T] run by me (test, live run or mutant), [A] analysis or code reading, [K] known from CI/GitHub or a pre-existing condition.

Base: origin/master 5cceea08. Three regular commits, all authored dborup <kontakt@meshview.dk>, with no rebase, amend or force-push:

  • b4dfe95c: tests, red on master;
  • dacac90e: implementation and docs;
  • eb49596f: a stronger Docker check.

The title, body and commits contain no closing keywords. [T]

Setting: CORESCOPE_RUN_AS_NONROOT=1|true with CORESCOPE_UID/CORESCOPE_GID. Unset, empty, 0 or false mean off; any other value fails fast. [A]

Requirements

# Requirement Change Test Red before Mutants (all killed)
1 Opt-in; off is byte-for-byte equivalent in effect entrypoint-go.sh only calls the helper for values other than unset, empty, 0 or false. The four supervisord-go*.conf files are unchanged (git diff empty), and the prod/staging compose files are unchanged. [A] Docker case 0 snapshots the start through a stubbed supervisord: argv, uid, environment, passwd/group checksums, /run, /tmp, and a stat of every path under /app except public/. There are 16 off combinations (unset, empty, 0, false × the four DISABLE_* variants), each diffed against the baseline master image: identical. The only new path is the helper file. The data volume is unchanged, and "off again" logs no [nonroot] line. [T] This is a guard: it is green on master by construction. [T] D1: unset treated as on → 51 checks fail [T]
2 Both Go services run as the configured UID:GID through per-program user= docker/nonroot.sh renders /run/corescope/supervisord.conf = the selected config plus user=corescope under exactly the two Go programs, and creates the account. [A] Unit test, all four supervisor variants (the output minus the two lines is byte-identical to the input). Docker cases 1–4: ingestor and server at 10001:10001. [T] Base image: both at 0:0 (51/197 checks red) [T] U2 ingestor only (unit); D8 server stays root: 11 checks [T]
3 Mosquitto and Caddy unchanged, including PUID/PGID No user= for mosquitto or caddy; the broker command is untouched. [A] Unit test: no user= in the mosquitto/caddy sections. Docker: mosquitto 101:102, caddy 0:0, supervisord 0:0; with PUID/PGID=1000 the broker environment still has no PUID. [T] Guard, green on master [T] U1/U1b user= on every program (unit); D7 user= on caddy: 7 checks [T]
4 Targeted ownership migration, only when wrong, no blanket recursive chown fix_owner handles each listed path itself: the data dir; meshcore.db/ping_scores_history.db with -wal/-shm; the client-rx salt and audit log; and the four flat queue dirs with their direct entries. It changes ownership only on a differing owner, and skips symlinks, hard links and special files with a warning. [A] Docker case 2 copies e2e-fixture.db and leaves root-owned WAL/SHM from a SIGKILLed root run. Every listed path is migrated. config.json, theme.json, notes/, backup-old.db, a planted symlink and a hard link are all untouched. The WAL content survives. Case 1: the second start logs ownership: 0 of N. The unit test checks the list against the Go constants. [T] Base: not migrated (7 checks) [T] D2 chown -R: 12; D3 unconditional chown: 1; D4 follow symlink: 2; D9 drop -shm: 10; U4/U4b list drift (unit) [T]
5 Startup cost and host-side effect documented docs/deployment.md, "Running the Go services without root (opt-in)", and commented lines in docker-compose.example.yml. [A] The unit test checks that both files document the three variables, and that the section names the backup helper, Rollback, set-owner and the host effect. [T] Red on master: 53/61 unit checks [T] U7 variable name, U8 Rollback heading (unit) [T]
6 A read-only config mount keeps working Config ownership is never changed; readability is checked. [A] Docker case 3: config.json:ro bind mount in on mode. The services are ready, a packet is stored and read, and the host file and its owner are unchanged. [T] Base: services root [T] D2 recursive chown fails on the ro mount → "ready with a read-only config" fails [T]
7 Geo-filter save fails clearly on read-only arrangements ErrConfigReadOnly for permission or EROFS errors on create/rename → 409 geo filter not saved: config.json is read-only for the server (…). The filter is not applied; other errors keep the 500. [A] geofilter_save_readonly_339_test.go: EACCES, EPERM and EROFS at create and rename, a real 0555 directory, ENOSPC → 500, unreadable config → 500. Docker case 3: 409, not applied, host file unchanged. Browser: the customizer shows the message (screenshot taken locally). [T] Master: 500 failed to save config (2 test funcs, 6 subtests) [T] G1 never wrap, G1b drop EROFS, G2 status 500, G3 classify read error, G4 rename unclassified, G5 apply on failure [T]
8 Unsupported arrangements and invalid settings fail clearly validate runs before any change; an unreadable config.json stops the start. [A] Unit: 20 validation cases. Docker case 5: yes, missing UID, UID 0, UID 101 (mosquitto), GID 102 → exit 1, [nonroot] ERROR:, volume unchanged. Case 3: an unreadable config stops with a reason. [T] Base: starts anyway (13 checks) [T] D6 drop the config check: 2; D10 any value enables: 3; U3/U3b/U3c/U5/U6 (unit) [T]
9 Toggle off → on → off A stale root-owned stats file in /tmp is removed at an on-start. [A] Docker case 4: off (root) → on (10001) → off (root; ownership stays and the off start runs no ownership step). Then an in-place switch through /app/data/.env + docker restart, and back. A packet is stored and read in every phase. [T] Base: on phases root (6 checks) [T] D5 no stats cleanup: 2 checks (after eb49596f) [T]
10 Ingestor writes and server reads under non-root — Docker cases 1–4: mosquitto_pub → ingestor tx_inserted → server /api/stats totalTransmissions. [T] Base: processes root [T] D9: "non-root ingestor writes the migrated DB" fails [T]
11 Supervisor restart — Docker case 1: SIGTERM to both Go programs → supervisor respawns them, still 10001:10001, packet 2 stored. docker restart → still non-root, 0 of N changes, packet 3 read. [T] Base: root [T] D8: fails after both restarts; D3: second start [T]
12 Migration and rollback instructions, backup first Docs: scripts/backup-sqlite.sh first, record owners, enable; rollback = remove the variables, optionally nonroot.sh set-owner 0:0 while stopped, then restore the backup. [A] Docker case 4 rollback: set-owner 0:0 → ownership: 5 of 7 path(s) … changed to 0:0; the listed paths are root again and the config is untouched. [T] Unit docs check red on master [T] D11 no-op set-owner: 3 checks [T]
13 Staging rehearsal checklist Added to the PR description. [A] — — —

Process UIDs (ps inside the container, final run on the image built from this head) [T]:

# on, migrated root-owned DB (case 2)
    1 root     root     {supervisord} /usr/bin/python3 /usr/bin/supervisord -c /run/corescope/supervisord.conf
   66 root     root     /usr/sbin/caddy run --config /etc/caddy/Caddyfile
   67 corescop corescop /app/corescope-ingestor -config /app/config.json
   68 corescop corescop /app/corescope-server -config-dir /app -db /app/data/meshcore.db -public /app/public -port 3000
   69 mosquitt mosquitt /usr/sbin/mosquitto -c /etc/mosquitto/mosquitto.conf
   supervisord 0:0 · corescope-ingestor 10001:10001 · corescope-server 10001:10001 · mosquitto 101:102 · caddy 0:0
# off again (case 4)
    1 root     root     {supervisord} /usr/bin/python3 /usr/bin/supervisord -c /etc/supervisor/conf.d/supervisord.conf
   15 root     root     /app/corescope-ingestor -config /app/config.json
   16 root     root     /app/corescope-server -config-dir /app -db /app/data/meshcore.db -public /app/public -port 3000
   17 mosquitt mosquitt /usr/sbin/mosquitto -c /etc/mosquitto/mosquitto.conf

Invariants

Check Result
cmd/server stays read-only Only the geo-filter error mapping changed; no DB write. readonly_invariant_test.go passes in the full suite. [T]
No new map[string]interface{} git diff origin/master adds 0 occurrences of interface{}. [T]
No hardcoded colours No public/ or CSS change. [A] CI "Lint CSS variables" passes. [K]
scripts/check-xss-sinks.sh --diff origin/master no public/**/*.{js,html} changes to scan, exit 0. [T]
Fork guards github.repository == 'Kpa-clawbot/CoreScope': 9 in deploy.yml, 1 in release-fast-path.yml, unchanged. [T]

Local runs [T]

  • cd cmd/server && go test ./...: ok. An earlier run, made while 3 mutant containers loaded the host, failed once in a _1352 paths test (503 index loading, a timing issue). A rerun on an idle host and CI were green.
  • cd cmd/ingestor && go test ./...: ok (215 s).
  • sh test-all.sh: 237 files passed.
  • node test-frontend-helpers.js: 713 passed.
  • bash scripts/test-nonroot-helper.sh: 81/0. The same under busybox sh and dash. ShellCheck is clean for all three scripts.
  • scripts/test-docker-nonroot.sh corescope-339:head corescope-339:base: 213/213. Base image: 51/197 red, all on-mode expectations. The run leaves no containers or volumes behind.
  • Affected E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, corescope-migrate, the three seed files): test-issue-332-no-carto-tiles-e2e.js 31/0. The server was stopped through its pid.
  • Browser check (Playwright, local container, on mode, read-only config): the customizer's geo-filter save got PUT 409 and showed the message.

CI per job

Run 38379532833 on eb49596f; CI tests the PR merge ref against current master 4a5ec5e2.

Job Result
Go Build & Test success. It includes the new step "Non-root container helper unit tests + ShellCheck (issue #339)", Go server/ingestor tests, test-all.sh, the XSS --diff gate and the CSS-variable lint. [K]
Playwright E2E Tests failure, in test-issue-315-estimated-positions-e2e.js: phase "enabled" timed out after 10 s waiting for the seeded node row tr[data-key="3152…"]. Phase "default" and all earlier E2E files passed. [K]
Release Artifacts, Build & Publish Docker Image, Deploy Staging, Publish Badges skipped (fork guards / dependencies) [K]

About the E2E failure: this PR does not touch public/, the nodes API or that test. The only server change is the geo-filter PUT error mapping. I ran #315 exactly as CI does (pinned Leaflet 1.9.4, own default/on/off servers), and it passed 3/3 on this branch and 3/3 on the merge tree with current master [T]. Master and other branches pass this job [K]. It is not #271, so I did not rerun the job: the rerun rule covers only #271. A rerun is the owner's call.

The earlier run 38374081568 on dacac90e had Go Build & Test success; its E2E was cancelled when eb49596f superseded it. [K]

Remaining issues

  1. Pre-existing, off mode, unchanged here [T][K]: with a read-only config mount, the off-mode geo-filter save returns 200, but it only replaces the /app/config.json symlink with a regular file in the container layer. The host file is not updated, and the entrypoint's ln -sf discards the change at the next start. The same replacement also happens with a writable data dir. Fixing it changes default-mode behaviour, so I kept it out of this opt-in PR and suggest a separate issue.
  2. Pre-existing [T]: on a fresh volume, corescope-server exits once or twice until the ingestor has created and migrated the DB, and supervisor restarts it. This is identical on the master image.
  3. The Docker runtime test is local-only, like test-docker-mosquitto-compat.sh. CI runs the unit test and ShellCheck, not the container test. [A]
  4. In non-root mode, ingestor admin commands must run with docker exec -u UID:GID … (documented). A docker cp restore creates root-owned files; the next non-root start fixes the listed ones. Paths outside /app/data (a custom dbPath, CORESCOPE_INGESTOR_STATS, hashRegionsPath) are not migrated (documented). [A]
  5. A read-only container root filesystem is not supported in either mode: the entrypoint writes /app/config.json and the helper writes /etc/passwd and /run. [A]
  6. Staging rehearsal and the production decision remain with the owner.

@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent3 PR#404 — head eb49596

Dom: REQUEST CHANGES

This is an independent, read-only review. Evidence tags: [T] run by me (a test, a local container run or a mutant), [A] analysis or code reading, [K] known from CI or GitHub.

  • Head eb49596f was unchanged before and after the review (git ls-remote). [T]
  • The merged tree is git merge-tree --write-tree origin/master eb49596f on current master 8f40525f. It merges cleanly (tree f0c91350). [T]
  • I built images from master and from the merged tree and ran them only on local Docker. No staging or production system was touched, and no server API key was used. [T]

Summary:

Findings

# Severity Area Finding Required change / acceptance criterion
1 High (blocking) docker/nonroot.sh (ownership of /app/data), docker/entrypoint-go.sh (root-side handling of files in /app/data), docs/deployment.md (recommended IDs, "config.json is read-only") The ownership migration gives the non-root processes write access to files that root later trusts. The services own /app/data. The root entrypoint consumes files in /app/data at every start. With the IDs the docs and the staging checklist recommend (the host owner of the data directory), those files are also owned by the service user. A compromised server or ingestor can therefore change what root acts on at the next start and regain root. For the same reason, "config.json is read-only for the services" holds only with a read-only config mount. [T] (confirmed locally on the merged-tree image, both with the recommended IDs and with root-owned files) Root-consumed files in /app/data must not be writable or replaceable by CORESCOPE_UID in on-mode. Otherwise the start must refuse with a clear [nonroot] ERROR. The docs and the staging checklist must not recommend an ID setup that breaks this. Acceptance: a test, red before and green after, shows that in on-mode a process running as CORESCOPE_UID can neither modify nor replace any file that the root entrypoint reads, and that the next start fails closed if such a file has been tampered with. The docs' security claims must match what that test proves. The technical details were handed to the owner separately.
2 Medium cmd/server/config.go SaveGeoFilter + open PR #422 The 409 in on-mode currently depends on an incidental detail. The temp file is created in /app, which is root-owned, because /app/config.json is a symlink. I applied #422's config.go change, which resolves symlinks, onto this PR. On-mode then returned 200, and the non-root server replaced a root-owned config.json in /app/data, which became 10001:10001. Without #422 the same setup returns 409 and the file is unchanged. [T] It has the same root cause as #1. Coordinate with #422. The "not saved, not applied" property in on-mode must hold even when the save goes to the symlink target, and a test that fails if config.json becomes replaceable must pin it.
3 Low–Medium docker/nonroot.sh queue migration, ingestor admin CLI An admin command run as root in on-mode, for example the non-dry-run admin delete-client-rx through the default docker exec, creates a root-owned queue dir and a 0600 request. The CLI exits 2 with queued — ingestor not running?, and the ingestor logs permission denied every 15 s. At the next container start, the helper hands the request to the service user and the ingestor executes the delete then. The operator was told it failed. [T] It is documented that admin commands need -u, but this failure mode is surprising and can be destructive. Do not migrate foreign-owned request-* files. Instead, report them and leave them alone, or move them aside. Alternatively, the CLI could refuse to run as the wrong user in on-mode. Acceptance: a request written by root in on-mode is never executed later without an explicit operator step.
4 Low Test coverage in CI Two behaviour-critical properties are enforced only by the local-only Docker test, so CI would not catch a regression in either. See the mutants M2 and M5 below: both pass scripts/test-nonroot-helper.sh 81/81. Add CI-run checks for the entrypoint's off values and for "chown only when the owner differs", using stubs.
5 Low (docs) docs/deployment.md Turning it on (step 2) and Rollback (step 2) The docs say to note the owners "so you can restore them exactly". However, step 2 records only the data dir and *.db* (the named-volume variant records only /app/data), and set-owner applies a single UID:GID to every listed path. Mixed original owners therefore cannot be restored exactly. That case is common: after root runs on a bind mount I saw a dir owned 1000:1000 alongside root-owned queue dirs and ping_scores_history.db. [T] Record every path that nonroot.sh paths covers (incl. queue dirs and salt/audit). Document how to restore mixed owners, or drop "exactly".

Answers to the review points

Setting off (default). [T] I started master 8f40525f and the merged tree on bind dirs with identical content (config, theme, .env with PUID/PGID, fixture DB, nested user files). The merged tree ran with the setting unset, =0 (with UID/GID set), =false and empty. For all four I compared, against master, the ps user/group of every program, the /proc/*/status IDs, supervisord's argv (/etc/supervisor/conf.d/supervisord.conf), the md5 of /etc/passwd and /etc/group, the /run and /tmp listings, the stat of /app (excluding public), and the stat of every file in the data dir after one ingested packet. The only difference is the new /usr/local/lib/corescope/nonroot.sh (plus log timestamps). Startup log lines are the same. The one-off corescope-server restart before the DB exists also happens on master. A shutdown-time WAL checkpoint error: disk I/O error and an ON CONFLICT insert warning on the unmigrated fixture also occur on master. [T]

Setting on. [T] I used UID/GID 1000:1000 on a bind mount, a read-only config.json mount and PUID/PGID=1000 in .env:

    1 root     root     {supervisord} /usr/bin/python3 /usr/bin/supervisord -c /run/corescope/supervisord.conf
   63 root     root     /usr/sbin/caddy run --config /etc/caddy/Caddyfile
   64 corescop corescop /app/corescope-ingestor -config /app/config.json
   65 corescop corescop /app/corescope-server -config-dir /app -db /app/data/meshcore.db -public /app/public -port 3000
   66 mosquitt mosquitt /usr/sbin/mosquitto -c /etc/mosquitto/mosquitto.conf
supervisord=0:0 corescope-ingestor=1000:1000 corescope-server=1000:1000 mosquitto=101:102 caddy=0:0
  • Ingestor writes and server reads: mosquitto_pub raised the ingestor's tx_inserted, and /api/stats totalTransmissions followed (499 → 504 across 5 packets and 2 restarts). [T]
  • Mosquitto stays 101:102 despite PUID/PGID=1000, and Caddy and supervisord stay root. [T]
  • With DISABLE_CADDY, DISABLE_MOSQUITTO or both, the generated config is used, the Go services run non-root, and the disabled programs are absent. A GID of 100 reuses users. [T]
  • A supervisor respawn (SIGTERM to both Go programs) and docker restart both keep 1000:1000. [T]

Ownership migration. [T]

  • Root-owned DB with WAL/SHM. A master root run on a root-owned fixture DB, then SIGKILL, left root-owned -wal/-shm. The first on start logged ownership: 11 of 11, and only the listed paths changed. notes/sub/f, a stray backup-old.db, the config mount and the second-level file prune-requests/nested/inner kept their owners, so the migration is not recursive. The WAL data survived, and the ingestor wrote to the migrated DB.
  • Second start. It logged 0 of 9, and the ctime of untouched listed paths (e.g. ping_scores_history.db, prune-requests) was unchanged, so nothing is chowned without need.
  • Read-only config mount. It works, the host file and its owner are unchanged, and an unreadable config fails fast with the documented message.
  • Geo-filter save. It returns 409 {"error":"geo filter not saved: config.json is read-only for the server (read-only mount, or CORESCOPE_RUN_AS_NONROOT is on); edit geo_filter in config.json on the host and restart"}, and the filter is not applied (GET still shows polygon:null). See finding 2 for how that interacts with fix(config): persist geo-filter saves through config symlinks #422.
  • Invalid settings. yes, TRUE, a missing UID, UID 0, UID 101, GID 0 and GID 102 each exit 1 with a [nonroot] ERROR: line, and the volume is unchanged.

Toggle on/off/on and rollback, following the docs verbatim (named volume). [T]

  1. backup-sqlite.sh worked.
  2. Step 2's named-volume stat command ran.
  3. I enabled the mode via .env and restarted: services 1000:1000, 7 of 7 changed.
  4. Rollback step 1: removing the variables and restarting brought everything back to root, and a packet was stored and read.
  5. Rollback step 2: I ran the docker run --rm --pull=never --network none --volumes-from … set-owner 0:0 block verbatim with the container stopped. It logged 5 of 7 … changed to 0:0, and every owner in the volume was identical to before the first enable.
  6. Enabling again worked (7 of 7, packets flow).

The steps are correct for a uniformly owned volume. See finding 5 for mixed owners.

CI. [K]

Staging checklist. It covers logs, ps, health, ingest, the 0 of M restart, log watching and a rollback drill. It is not sufficient yet for a rehearsal with a snapshot and rollback:

  • (a) Its ID choice ("host owner of the staging data dir") is exactly the arrangement in finding 1.
  • (b) The backup covers only the two DBs and the WAL. Take a full copy or snapshot of the data dir first, including the salt, the audit log, the queue dirs, config.json and .env.
  • (c) Record the owners of every listed path (finding 5).
  • (d) Add a check that no tooling or operator runs admin commands into the container as root during the rehearsal (finding 3).
  • (e) Verify the geo-filter 409 in the customizer.
  • (f) Set explicit rollback triggers and a time box. Note that the backup helper stops the container briefly.

Invariants.

  • cmd/server stays read-only: the only change is error mapping, and the full server suite incl. readonly_invariant_test.go passes. [T]
  • 0 new map[string]interface{}. [T]
  • No public//CSS changes, and scripts/check-xss-sinks.sh --diff origin/master exits 0 ("no public/**/*.{js,html} changes to scan"). [T]
  • Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, the same as master. [T]
  • No closing keywords in the title, body or commits. [T]
  • All three commits are authored and committed by dborup <kontakt@meshview.dk>. [T]

Acceptance criteria of #339 vs. tests.

  • Geo-filter / read-only config: geofilter_save_readonly_339_test.go is red on master (2 test funcs, 5 subtests) and green after. [T]
  • Render, validation, path list and docs: scripts/test-nonroot-helper.sh is 8 passed / 53 failed on master and 81/0 after. [T]
  • Runtime properties (non-root processes, migration, toggle, rollback): covered only by the local-only scripts/test-docker-nonroot.sh. [T]
  • "Run as non-root in all supported Go container variants" is met only as opt-in, which is the scope chosen here. [A]
  • Finding 1 has no test yet.

Tests and mutants

All on the merged tree [T]:

  • cd cmd/server && go test ./...: ok. cd cmd/ingestor && go test ./...: ok (242 s).
  • sh test-all.sh: 241 passed, 0 failed. node test-frontend-helpers.js: 713 passed.
  • bash scripts/test-nonroot-helper.sh: 81/0. ShellCheck is clean for all three scripts.
  • scripts/test-docker-nonroot.sh cs404:merged cs404:master: 213/213, no containers left behind.
  • E2E against a local Go server on e2e-fixture.db, prepared as in CI: freshen, Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486 seed SQL, corescope-migrate, seeds 2073, 199 and 245.
    • test-issue-332-no-carto-tiles-e2e.js: 31/0
    • test-xss-333-pages-e2e.js: 11/0
    • test-issue-315-estimated-positions-e2e.js: 3/3 runs PASS
    • test-e2e-playwright.js stops fail-fast at test 7 ("Version info lives on Perf dashboard", #navStats 10 s timeout). The same thing happens against a master server built the same way, so it is a local environment issue. That suite passed in CI attempt 2.
    • The servers were stopped by port.

Own mutants [T]:

Mutant Change Result
M2 entrypoint-go.sh: 0 no longer counts as off Survives the CI unit test (81/81). Killed by my Docker check: =0 → exit 1, [nonroot] ERROR
M3 configReadOnlyErr: %w → %v for ErrConfigReadOnly Killed: 2 funcs / 5 subtests of the #339 Go tests fail
M4 drop ping_scores_history.db-wal from DB_FILES Killed by the unit test (2 checks)
M5 fix_owner without the owner-differs short-circuit Survives the CI unit test. Killed by my Docker check: the second start logs 7 of 7 instead of 0 of 7
M6 GID minimum 100 → 1 Killed by the unit test (1 check)

Not verified

  • No staging or production run (by design), and no rehearsal of the staging checklist itself.
  • The full test-e2e-playwright.js locally (see above), Playwright coverage collection, and arm64 images.
  • The fix for finding 1 does not exist yet, so the proposed acceptance test has not been run.
  • The behaviour with a custom dbPath, hashRegionsPath or CORESCOPE_INGESTOR_STATS outside /app/data (documented as not migrated). [A]

dborup and others added 7 commits October 13, 2026 13:04
…n write (#339)

Review round 2 of the opt-in non-root mode. All red on the previous head:

- scripts/test-nonroot-entrypoint.sh (new, meant for CI): the real
  entrypoint and helper as root in the runtime image's pinned Alpine base
  with supervisord stubbed. Pins the unchanged off-mode start for
  unset/empty/0/false x DISABLE_*, the data dir keeping its owner (group
  write + sticky bit only), "0 of N" on a second start, that the services'
  user cannot modify, replace or remove .env/config.json/theme.json
  (host-owned and root-owned originals), that a start refuses such files
  when the services could have written them (also after the setting is
  removed), that root-written queue requests are set aside, and exact
  owner/mode restore from an owners record.
- scripts/test-nonroot-helper.sh: -journal files in the list, docs and
  compose example must not suggest the host owner's IDs, CI wiring.
- scripts/test-docker-nonroot.sh (local): new data dir expectations, a held
  root request, 409 with an in-volume writable config, record/restore
  rollback, and a case run as the services' user in a real container.
- Go: geo-filter save refused in non-root mode even when every path is
  writable (no longer depends on /app being root-owned, cf. #422); the
  delete-client-rx CLI refuses root next to a database owned by another
  user. Adds the clientRxGeteuid seam the test needs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…#339)

Review round 2 of the opt-in non-root mode.

Finding 1: the services no longer own the data directory. It keeps its
owner and gets the services' group with group write and the sticky bit, so
SQLite can still create its -wal/-shm/-journal files but the services can
neither remove nor replace entries they do not own. Before .env is sourced
and config.json/theme.json are linked, `nonroot.sh preflight` refuses the
start ([nonroot] ERROR, nothing sourced, linked or changed) if one of them
is a symlink, hard link or non-regular file, owned by the services' user,
writable for their group or everyone, or if that user owns the data dir.
The settings are read from .env as data for this check (plain lines only).
A root-owned marker with the IDs keeps the check active after the setting
is removed, until a rollback. Docs and the compose example now ask for a
dedicated UID instead of the host owner's.

Finding 2: SaveGeoFilter refuses explicitly in this mode, before touching
the filesystem, so the 409 no longer depends on /app being root-owned (and
holds with #422's symlink resolution).

Finding 3: delete-client-rx refuses to run as root next to a database owned
by another user, and writes nothing. At start, queue requests the services
did not write are renamed to *.held, reported and never run; queue entries
are no longer chowned.

Finding 4: scripts/test-nonroot-entrypoint.sh runs in CI (Go Build & Test),
plus ShellCheck of the entrypoint.

Finding 5: `nonroot.sh owners` records owner and mode of the data dir and
every listed path; `restore-owners` applies such a record exactly (paths
created while on go to root) and accepts nothing outside the list.
`set-owner` stays as the coarse fallback. Both refuse while a file root
uses fails the check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… rule (#339)

Red on 2173593:

- With the kernel's fs.protected_regular=2 (a common default), root cannot
  open files it does not own in a sticky, group-writable directory. After
  the setting is turned off, the root ingestor then fails with "attempt to
  write a readonly database" until the rollback's second step. Pin that the
  first off start takes back group write and the sticky bit, says so, and
  that the next on start sets them again. Full-image test: case 4 off again
  is ready and logs it once.
- The data dir is writable for the services' group, so a root-read file
  owned by anyone but root or the data dir's owner is refused, while a
  read-only config mount is accepted whoever owns it.
- The bad-record checks each carry a valid data dir line, so every bad line
  is refused for itself (a surviving mutant showed the old records were all
  refused for the missing data dir line only).
- Full-image test: the last ownership line of case 1 is "0 of N" (the data
  dir is no longer counted); deterministic post-rollback log check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… by root or the data dir owner (#339)

- preflight: when the setting is off but the volume still carries the
  marker, take back group write and the sticky bit from the data dir after
  the check passes, and log it. Otherwise fs.protected_regular=2 stops root
  from opening the services' databases, and rollback step 1 alone would no
  longer restore service. restore-owners still restores the recorded mode.
- check_trust: a root-read file must be owned by root or by the data dir's
  owner (never by a service UID), or be a read-only mount. Group write on
  the data dir lets every member of CORESCOPE_GID create files there.
  Its IDs come only from $check_ids (a loop variable clobbered the caller's
  `ids`).
- Docs: the owner rule, a dedicated group without host members, the off
  start, and -u for tools such as sqlite3.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Case 4 rollback: skip recorded -wal/-shm files that SQLite removed on a
  clean stop (restore-owners skips them too).
- Case 6: model a bind mount, where the host user owns the data dir and
  theme.json; a host-owned theme.json in a root-owned named volume is now
  refused by design.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ces (#339)

The owner rule already refuses such a file; the explicit check exists to
say which ID owns it. A mutant that dropped it survived, so the message is
now asserted.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent3 PR#404 runde-2 — head 3b614bc

Review feedback addressed (commit 3b614bca)

Evidence tags: [T] run by me (test, container run or mutant), [A] analysis or code reading, [K] known from CI or GitHub.

The default (setting off) is unchanged. On a volume that never used the mode, all 16 off-mode snapshots are identical to the master image. No staging or production system was touched, and no API key was used. The PR stays draft.

  1. Finding 1 (blocking): root no longer uses files the services can write or replace. I chose fix option 2, plus a stricter owner rule. [A][T]
    • Data directory. The services never own /app/data. It keeps its owner and only gets the services' group, group write (SQLite needs it for -wal/-shm/-journal) and the sticky bit, so the services cannot remove, rename or replace entries they do not own. A start with CORESCOPE_UID equal to the directory's owner is refused.

    • Start check. nonroot.sh preflight runs before the entrypoint sources .env or links config.json/theme.json. It stops the start with [nonroot] ERROR (nothing sourced, linked or changed) when one of those files:

      • is a symlink, a hard link or not a regular file;
      • is owned by the services' user, or by anyone but root or the data directory's owner;
      • or is writable for the services' group or for everyone.

      A read-only mount (config.json:ro) is accepted whoever owns it. The settings are read from .env as data for this check, so they must be plain lines.

    • The check outlives the setting. A root-owned marker (/app/data/.corescope-nonroot, UID:GID) keeps the check running after the setting is removed, until the rollback removes it. A volume that never used the mode never runs it.

    • Off start after on. With fs.protected_regular=2, a common distro default, root cannot open the services' files in a sticky, group-writable directory. I found this when the first full-image run failed. The first off start therefore takes back group write and the sticky bit (logged), so rollback step 1 alone still restores service.

    • Creating a missing name. The services can still create a file under a missing name, because the directory has to stay writable for SQLite. Such a file is never used: the next start refuses it, and the tests prove both. I did not use placeholder files, because an empty config.json/theme.json would change behaviour. [A][T]

    • Docs. The docs, the compose example and the staging checklist now ask for a dedicated UID/GID (e.g. 10001:10001, a group with no host members), not the host owner's. The security claims are corrected to what the tests prove.

    • Migration. From the default mode: "Turning it on" in docs/deployment.md. From the earlier draft eb49596f: a short subsection in the docs and in the PR description. The new image refuses such a volume until the operator has checked the root-read files and run set-owner (or chowned the directory back).

  2. Finding 2: the 409 is explicit. In non-root mode, SaveGeoFilter returns ErrConfigReadOnly before it touches the filesystem, so the result no longer depends on /app being root-owned. [A]
    • A Go test proves it with every path writable and a symlinked config in a writable data dir. A full-image test proves it for an in-volume, non-:ro config (root-owned and host-owned): 409, filter not applied, and the file's checksum, inode, owner and mode unchanged. That test was run both on head and on head with fix(config): persist geo-filter saves through config symlinks #422's config.go change applied, and passed both times. [T]
  3. Finding 3: root admin commands. [A][T]
    • admin delete-client-rx now refuses to run as root next to a database owned by another user (refusing to run as root …, exit 1) and writes nothing.
    • At start, queue requests (request-*.json, cmd-*.json) that the services did not write are renamed to *.held, keep their owner and are reported. They are never run. Queue entries are no longer chowned.
    • The full-image test confirms it in a real container: root is refused, while the same command as -u UID:GID runs.
  4. Finding 4: the behaviour runs in CI. The new scripts/test-nonroot-entrypoint.sh runs in Go Build & Test. [T][K]
    • It runs the real entrypoint and helper as root in the runtime image's pinned Alpine base, with supervisord stubbed and no Go build.
    • It covers the off-mode start (unset/empty/0/false × the four DISABLE_* variants), "chown only when different" (0 of N and unchanged ctime), the start check, the access attempts as the services' user, held requests, and rollback.
    • ShellCheck now covers the entrypoint too. M2 and M5 are killed (see the mutant table).
  5. Finding 5: exact rollback. [A][T]
    • nonroot.sh owners records owner and mode of the data dir and of every listed path, including queue dirs, their entries, the salt and the audit log.
    • restore-owners applies such a record exactly, and gives listed paths created later to root. It refuses any record line outside the list and any record without the data-dir line, as a whole.
    • set-owner stays as the coarse fallback. Both refuse while a root-read file fails the check.
    • The docs now use the record. The test restores mixed owners and modes, e.g. a 1000:1000 775 dir next to root-owned queue dirs.

I disagree with no finding.

Commits (all dborup <kontakt@meshview.dk>, regular push eb49596f..3b614bca, no rebase/amend/force)

Commit Content
cadd4d02 Merge origin/master 779f2b93 (clean)
759a1d0a tests, red on the previous implementation
21735936 fix for findings 1–5, docs, CI
2d637607 tests: off start after on, stricter owner rule, a stronger record test (red on 21735936)
17fc8221 fix: off start, owner rule
e34e23bc full-image test fixtures for the owner rule
3b614bca message assertion (kills an equivalent mutant)

Tests, red before and green after [T]

Test Before After (3b614bca)
scripts/test-nonroot-entrypoint.sh (CI) 118 of 285 failed on the previous implementation; 7 of 296 failed on 21735936 297/297
scripts/test-nonroot-helper.sh (CI) 11 failed 92/0, also under dash and busybox sh
Go geofilter_save_readonly_339_test.go non-root mode: 500, temp file created ok
Go admin_delete_client_rx_owner_339_test.go exit 2, request queued ok
scripts/test-docker-nonroot.sh on the old-implementation image 27 of 238 failed, including ".env not sourced by root" —
scripts/test-docker-nonroot.sh head master — 260/260; 16/16 off snapshots identical to the master image
scripts/test-docker-nonroot.sh head+#422 — 244/244

Other local runs on the final head [T]:

  • cd cmd/server && go test ./...: ok.
  • cd cmd/ingestor && go test ./...: ok (198 s).
  • sh test-all.sh: 242/0.
  • node test-frontend-helpers.js: 713/0.
  • ShellCheck is clean for all four scripts and the entrypoint (-e SC1091, a pre-existing note).
  • E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486 seed SQL, corescope-migrate, seeds 2073/199/245); the server was stopped by its pid:
    • test-issue-332-no-carto-tiles-e2e.js: 31/0
    • test-xss-333-pages-e2e.js: 11/0
    • test-e2e-playwright.js: 132/135 (3 skipped by the suite)

Invariants [T]:

  • 0 new interface{}.
  • check-xss-sinks.sh --diff: nothing to scan.
  • Fork guards: 9 + 1, unchanged.
  • No closing keywords.
  • cmd/server stays read-only, and readonly_invariant_test.go passes.

Mutants: one or more per finding, all killed [T]

Mutant Killed by
F1a: entrypoint never runs the start check CI stub test, 35 checks
F1b: no sticky bit 25
F1c: no explicit "owned by the services' user" check 1 (the message); behaviourally equivalent through the owner rule
F1d: no marker 17
F1e: data dir handed to the services (old behaviour) 48
F1f: settings in .env not read before sourcing 8
F1g: off start keeps group write/sticky 2
F1h: no root-or-dir-owner rule 5
F1i: read-only mount not accepted 2
F2: no explicit refusal in non-root mode Go, 3 (sub)tests
F3a: CLI without the root check Go, 1 test
F3b: foreign requests not held 10
F4/M2: 0 no longer counts as off 16
F4/M5: chown without the owner-differs short-circuit 2
F5a: restore ignores the recorded mode 2
F5b: record accepts any path 9
F5c: rollback without the check 4
F5d: record misses queue entries 1

F5b and F1c survived at first. F5b showed that every bad test record also lacked the data-dir line; each bad line is now tested on its own. For F1c, the message is now asserted.

CI per job [K]

Run 38413530337 on 3b614bca, conclusion success, first attempt with no rerun:

Job Result
Go Build & Test success. It includes "Non-root container helper unit tests + ShellCheck" (92/0) and the new "Non-root container start path, supervisord stubbed" (297/297).
Playwright E2E Tests success
Validate Docker Image success
Build & Publish Docker Image success
Release Artifacts, Deploy Staging, Publish Badges skipped (fork guards)

Remaining notes

  • Host users in CORESCOPE_GID can create files in the data directory. The docs ask for a group with no host members, and the owner rule refuses such files for root's use. Image group 100 (users) is still accepted by validation and documented as usually shared. [A]
  • The check reads owner, group and mode bits. It does not evaluate POSIX ACLs on those three files. [A]
  • A volume from the earlier draft whose setting sits in .env in a non-plain form, and is removed at the same time as the upgrade, is not checked automatically. The docs describe a manual check for draft volumes. [A]
  • On hosts with fs.protected_regular=2, root tools inside the container in on mode (e.g. sqlite3) need -u, as documented. backup-sqlite.sh uses docker cp and is unaffected. [A]
  • The full-image Docker test stays local-only. CI covers the start path with the stubbed test. [A]
  • Staging rehearsal and the production decision remain with the owner. The checklist in the PR description is updated: full data-dir snapshot, owners record, dedicated IDs, no root admin commands, 409 check, rollback triggers and time box.

@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent2 PR#404 — head 3b614bc

Dom: APPROVE med nits

Independent, read-only re-review after round 2, checked against the review "CS-pve-agent3 PR#404" (findings 1–5) and the report "CS-pve-agent3 PR#404 runde-2". Evidence tags: [T] run by me (test, local container run or mutant), [A] analysis or code reading, [K] known from CI or GitHub.

  • Head 3b614bca was unchanged before and after the review (git ls-remote). [T]
  • Merged tree: git merge-tree --write-tree origin/master 3b614bca on current master d34b228a, which includes the merged fix(config): persist geo-filter saves through config symlinks #422. It merges cleanly (tree f95c36b2). [T]
  • I tested on git archive copies of head and of the merged tree. I built images from the merged tree and from master and ran them only on local Docker with --network none, on scratch volumes. No staging or production system was touched. No server API key was used: the geo-filter checks used a throwaway local key in a scratch config.json. [T]

Summary: all five findings are resolved. The security model is sound: root no longer uses anything the services can write or replace, and the start fails closed when that is no longer true. The default path is unchanged. Two non-blocking nits remain (N1, N2 below).

Findings

# Severity Area Status / finding
1 High → resolved docker/nonroot.sh, docker/entrypoint-go.sh Resolved. In on-mode, as the services' user inside a running container, I could not modify, replace, rename away, re-mode or hard-link the root-read files, the data directory or the marker. A root-read file planted under a missing name is refused at the next start, before root uses it. This also holds after the setting is removed. [T] Details below.
2 Medium → resolved cmd/server/config.go, routes.go Resolved on the merged tree, i.e. with #422 actually merged. In on-mode the response is 409 and the filter is not applied, for both a host-owned and a root-owned in-volume config.json that is not mounted :ro. The file's inode, owner, mode and checksum are unchanged, and no temp file is left behind. In off-mode the same request still returns 200 and saves. [T]
3 Low–Medium → resolved cmd/ingestor/admin_delete_client_rx.go, nonroot.sh hold_requests Resolved. Root admin delete-client-rx exits 1 with refusing to run as root … and creates no queue directory or request. A root-written request-*.json is renamed to *.held, keeps root ownership and is never run. The same command as -u 10001:10001 completes. [T]
4 Low → resolved CI Resolved. Both new script tests run in the Go Build & Test job (helper 92/0, stubbed entrypoint 297/297). M2 and M5 are now covered there, see the mutant table. [K][T]
5 Low → resolved docs/deployment.md, owners / restore-owners Resolved. Following the docs, owners → enable → 0 of N restart → rollback step 1 → restore-owners restored a mixed-owner volume exactly, as described below. [T]
N1 Nit (tests) scripts/test-nonroot-entrypoint.sh The entrypoint's .env trigger for the start check (grep -q CORESCOPE_RUN_AS_NONROOT /app/data/.env) is not covered by any test, CI or local. Removing it (my mutant MD) leaves all 297 stub checks and the helper's 92 checks green. Every gate_case first does an enabled start, which writes the marker, so the check runs anyway. The mutant is not equivalent. With a marker-less volume, the setting only in .env, and a .env that is writable for the services' group, the real code refuses the start without sourcing .env. Under MD, root sources .env first, and only setup's second check refuses afterwards. [T] This is the path that protects the earlier draft's volumes, which have no marker. Suggest one gate case on a volume without a marker. The code itself is correct.
N2 Nit (UX, follow-up) cmd/server/routes.go handleConfigGeoFilter In on-mode, GET /api/config/geo-filter still reports "writeEnabled":true (it only looks at the API key). The customizer therefore offers a save that is certain to get the 409. The 409 itself is clear and documented, so this is fine for an opt-in. It could follow configWritesDisabled() later. [T][A]

Answers to the review points

Finding 1: own attack attempts. [T] Setup: merged-tree image, bind dir owned by the host user, config.json/theme.json/.env 644, IDs 10001:10001 (setting in .env, and in a second variant with -e and no .env). The services ran with Uid: 10001 and CapEff: 0, the image has no setuid binaries, and root's account is locked. Run as 10001:10001 in the running container (docker exec -u), for each of .env, config.json and theme.json:

  • Writing, removing, replacing (from inside and from outside the directory), renaming away and chmod all fail. So do deleting the marker, hard-linking a root-read file, renaming the data directory and changing its mode.
  • I tried to make root's ownership step act on a root-read file through a database-side name that the services can replace. The next start left the target file's owner unchanged and logged the warning for skipped symlinks.
  • A missing root-read file can be created, because the directory has to stay group-writable for SQLite. I tested a services-created .env containing a side effect, a services-owned theme.json, a symlinked theme.json, and a root-owned but group-writable theme.json. The next start refused each with a clear [nonroot] ERROR, and nothing was sourced. The planted .env was also refused, and not sourced, after the setting was removed (marker).
  • A root-owned 644 theme.json is accepted. CORESCOPE_UID equal to the data directory's owner is refused.

Result: 38/38 attack and control checks as expected. One extra check failed only because busybox ps truncates corescope to corescop, which is not a defect. I also reviewed what else root touches at start [A]: Caddy uses /data/caddy and /etc/caddy; supervisord uses no control socket; queue requests carry no file paths; and the /tmp stats file is handled. The services do not run during preflight/setup (a container restart kills them), so no check-then-use race applies at start.

Finding 2. See the table. The Go tests are also red on master (3 test functions fail) and green on the merged tree (6 #339 tests). [T]

Finding 3. See the table. One note: the services' user delete-client-rx took about 50 s on a fresh volume, because the 15 s ticker only starts after ingestor startup. That is how master already behaves and is not caused by this PR. [T]

Finding 4. [K] Run 38413530337 on 3b614bca, attempt 1, conclusion success. Per job: Go Build & Test, Playwright E2E, Validate Docker Image and Build & Publish Docker Image succeeded. Release Artifacts, Deploy Staging and Publish Badges were skipped (fork guards). In the Go job log, test-nonroot-helper reports 92 passed, 0 failed, and test-nonroot-entrypoint.sh reports 297 ok lines of 1..297. CI tested the merge with master 4c7a1ca5, before #422; my merged-tree runs cover #422. No flaky #256/#267 failures were involved.

Finding 5. [T] I used a named volume first run by the master image as root (root-owned DB, WAL and SHM, root-owned queue dirs with entries), then gave it mixed owners: data dir 1000:1000 775, ping_scores_history.db 1000:1000 664, prune-requests 0:0 750, and an unrelated notes/f 1234:1234. Steps, as in the docs:

  1. owners.
  2. Enable via .env: 7 of 7 changed, dir 1000:10001 1775, services run as corescope.
  3. Restart: 0 of 7.
  4. Rollback step 1: everything root, the the mode is off: … line, and no permission denied/readonly/SQLITE_BUSY.
  5. Stop, then restore-owners < record: 8 of 10 … restored.

A stat snapshot of the data dir, every listed path, the queue dirs, the root-read files and notes/f is identical before and after, and the marker is gone. The services ran as root again after the restart.

Default unchanged. [T] With the setting unset, 0, false or empty, under all four DISABLE_* combinations, the merged-tree image starts exactly like master. This is the author's test-docker-nonroot.sh with master as the baseline, see below. In off-mode, geo-filter save still returns 200. [A] On a volume without the marker, and with no mention of the variable in .env, the helper is never executed.

Invariants

  • cmd/server stays read-only. The only server changes are the early refusal in SaveGeoFilter and the error mapping, and the server suite including readonly_invariant_test.go passes. [T][A]
  • 0 new map[string]interface{} in the diff. [T]
  • No public/ or CSS changes, so no colours. scripts/check-xss-sinks.sh --diff origin/master exits 0: "no public/**/*.{js,html} changes to scan". [T]
  • Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, the same as master. [T]
  • No closing keywords in the title, body or commits. [T]
  • All 10 commits are authored and committed by dborup <kontakt@meshview.dk>. [T]
  • No behaviour change beyond the purpose. Off-mode has the same behaviour and the same supervisor configs, and docker-compose.example.yml only gains comments. [T][A]

Acceptance criteria of #339 vs. tests (red before → green after)

Criterion Test Before After
Non-root Go services in all Go variants (opt-in) stub entrypoint test (CI) + full-image test (local) stub test on the eb49596f implementation: 127 of 297 not ok 297/297; full image 260/260
UID/GID behaviour for bind mounts / named volumes / IDs same + helper unit test helper unit test 11 failed on the previous implementation [K] 92/0 [T]
Root-owned DB/WAL/SHM, config, logs, writable paths full-image cases 1–2, stub ownership checks — green [T]
No recursive chown; startup cost and host effect documented stub 0 of N + ctime, M5 — green [T]; docs [A]
Mosquitto/Caddy and PUID/PGID unchanged full-image case 1/3 — green [T]
Read-only config and geo-filter save Go geofilter_save_readonly_339_test.go + full-image case 3 + my own runs 3 test funcs fail on master green [T]
Migration/rollback docs; backups full-image case 4 + stub owners/restore-owners + my mixed-owner run — green [T]
Finding 3 (admin CLI) Go admin_delete_client_rx_owner_339_test.go red on master (does not compile: clientRxGeteuid undefined) green [T]

Tests and mutants

All on the merged tree [T]:

  • cd cmd/server && go test ./...: ok. On the first run, TestHandleNodePaths_SortCountTiebreaker_1145 and …FallbackPreconfirmed_1352 failed with 503 index loading while a Docker build ran in parallel. Both passed -count=3 on the merged tree and on master, and a full rerun is ok. They are unrelated to this PR, which touches no path or store code.
  • cd cmd/ingestor && go test ./...: ok (233 s).
  • sh test-all.sh: 242 passed, 0 failed. node test-frontend-helpers.js: 713 passed, 0 failed.
  • bash scripts/test-nonroot-helper.sh: 92/0. bash scripts/test-nonroot-entrypoint.sh: 297/297.
  • scripts/test-docker-nonroot.sh <merged> <master>: 260/260, 16/16 off-mode snapshots identical to the master image, no containers left behind
  • My own scripts: attack test 38/38 as expected, geo-filter on/off, admin CLI/held requests, mixed-owner rollback (see above).
  • E2E against a local Go server on e2e-fixture.db, prepared as in CI: freshen, seed SQL, corescope-migrate, seeds 2073, 199 and 245. The PR has no E2E of its own and changes no frontend, so I ran the geo-filter/customizer-related ones:
    • test-issue-332-no-carto-tiles-e2e.js: 31/0
    • test-xss-333-e2e.js: 8/0
    • test-issue-1329-map-controls-accordion-e2e.js: 5/5
    • test-issue-1648-m5-icons-e2e.js: 11/0
    • test-e2e-playwright.js stops fail-fast at test 7 ("Version info lives on Perf dashboard", #navStats 10 s timeout). The same thing happens against a master server built and seeded the same way, so it is environmental. That suite passed in CI.
    • Both servers were stopped by the pid found from their port.

Own mutants [T]:

Mutant Change Result
MA check_trust: drop the hard-link check Killed by the CI stub test (5 checks)
MB check_trust: drop the world-writable check Killed by the CI stub test (4)
MC off start / set-owner: remove only the sticky bit, keep group write Killed by the CI stub test (3)
ME hold_requests: ignore cmd-*.json Killed by the CI stub test (3)
MD entrypoint: drop the .env trigger for preflight Survives all CI tests. Not equivalent: my targeted container run shows root sourcing an untrusted .env before refusing → N1
MG Go configWritesDisabled: accept only "1", not "true" Killed: TestPutGeoFilterNonRootModeRefusesWritableConfig339
MH Go clientRxCheckRequester: invert the owner condition Killed: TestDeleteClientRxAsRootRefusesForeignOwnedDB339

Not verified

  • No staging or production run (by design), and no rehearsal of the staging checklist.
  • arm64 images, Playwright coverage collection, and the full test-e2e-playwright.js locally (see above).
  • POSIX ACLs on the root-read files, and a host where fs.protected_regular or fs.protected_hardlinks is 0. My host has 2 and 1. With protected_hardlinks=0, the usable() check for more than one hard link still applies. [A]
  • A real earlier-draft (eb49596f) volume migrated to this head. I only analysed that path and reproduced its trigger with mutant MD.
  • A custom dbPath, hashRegionsPath or CORESCOPE_INGESTOR_STATS outside /app/data (documented as not migrated). [A]
  • Residual and inherent to rollback step 1 [A]: until the rollback is finished, the root services open a database and queue entries that the non-root services could have written. That is today's default trust model, and the docs say to check data and restore the backup if anything looks wrong.

dborup and others added 2 commits October 13, 2026 17:22
Resolve the one conflict in docker-compose.example.yml by keeping both
sides: #431's forwarded in-process settings (MQTT_BROKER, MQTT_TOPIC,
MQTT_SOURCE_STRICT, ENABLE_PPROF, PPROF_*) stay active, and #339's
commented opt-in non-root lines follow them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…339)

Re-review nit N1: every gate case first did an enabled start, which writes
the marker, so dropping the entrypoint's "CORESCOPE_RUN_AS_NONROOT is in
.env" trigger left all checks green. Two cases now start a marker-less
volume whose .env alone asks for the mode: a .env writable for the
services' group, and an earlier-draft (eb49596) volume whose data dir and
.env the services own. Both must stop before .env is sourced or
config.json is linked.

Red on the reviewer's mutant MD (4 checks: config linked, .env sourced),
green on the real entrypoint (309/309).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Rapport — CS-pve-agent3 PR#404 master-sync — head cddc854

Review feedback addressed (commit cddc8541)

Evidence tags: [T] run by me (test, container run or mutant), [A] analysis or code reading, [K] known from CI or GitHub.

This round syncs the branch with master after #431 and #430 and handles the two nits from the re-review "CS-pve-agent2 PR#404" (APPROVE with nits). No staging or production system was touched. No server API key was used: the geo-filter and attack runs used a throwaway key in a scratch config.json. The PR stays draft.

  1. Master sync: merge conflict with ops(runtime): make bundled MQTT and pprof exposure explicit settings (#372) #431 resolved, both sides kept. [T][A]
  2. Nit N1 (tests): the .env trigger of the start check is now covered. [T]
    • scripts/test-nonroot-entrypoint.sh (CI) gains two gate cases on a volume without the marker, where only /app/data/.env asks for the mode:
      • a .env that is writable for the services' group;
      • an earlier-draft (eb49596f) volume, whose data directory and .env the services own.
    • Both must stop before .env is sourced or config.json is linked. The case first asserts that no marker exists, so the earlier "enable first" shortcut cannot hide the trigger again. gate_case was split into gate_refused + gate_case for this (no behaviour change for the existing cases).
    • Red before: this is a coverage gap, so the real code is already correct and the test is green on it. It is red on the reviewer's mutant MD (the grep -q CORESCOPE_RUN_AS_NONROOT /app/data/.env trigger removed): 4 of 309 checks fail. In each case config.json gets linked and the .env payload runs as root before setup refuses. Green after: 309/309 on the real entrypoint.
  3. Nit N2 (UX): not changed in this round, listed under remaining issues. [A][T] I agree with the observation (GET /api/config/geo-filter reports writeEnabled: true in on-mode). However, a one-line && !configWritesDisabled() would be wrong:
    • In the customizer, writeEnabled also shows the Prune historical nodes section (customize-v2.js _initGeoFilterTab).
    • Prune goes through the ingestor's prune-requests queue, which the services own in on-mode, so it still works there.
    • Hiding it would break a working feature.
    • A correct fix needs a separate field (e.g. configWritable, ideally with that handler's map[string]interface{} replaced by a named struct), a customizer change that hides only Save/Remove, and an E2E. That is not trivial and goes beyond a master sync.

I disagree with no finding.

Commits (all dborup <kontakt@meshview.dk>, regular push 3b614bca..cddc8541, no rebase/amend/force)

Commit Content
3b46c1e6 Merge origin/master dd712bc9 (conflict in docker-compose.example.yml, both sides kept)
cddc8541 test: N1, gate cases on a marker-less volume (red on mutant MD)

Tests on the merged head [T]

Suite Result
cd cmd/server && go test ./... ok (38 s), including compose_bind_372_test.go, pprof_bind_wiring_test.go, config_geofilter_symlink_407_test.go, geofilter_save_readonly_339_test.go, readonly_invariant_test.go
cd cmd/ingestor && go test ./... ok (382 s), including broker_mode_test.go, pprof_bind_wiring_test.go, admin_delete_client_rx_owner_339_test.go
internal/* modules all ok, including pprofbind
sh test-all.sh 243 passed, 0 failed (includes test-issue-372-broker-check.js, 24/0)
node test-frontend-helpers.js 713 passed, 0 failed
bash scripts/test-nonroot-helper.sh 92/0
bash scripts/test-nonroot-entrypoint.sh (stubbed supervisord, CI) 309/309 (297 before + 12 new)
ShellCheck (-e SC1091): entrypoint, nonroot.sh and the three test scripts clean
scripts/test-docker-nonroot.sh cs404:merged cs404:master (full image, both modes, local) 260/260; 16/16 off-mode snapshots identical to the master dd712bc9 image; no containers or volumes left behind

E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, the Kpa-clawbot#1486 seed SQL, corescope-migrate, seeds 2073/199/245) on port 13581. The server was stopped by its pid. [T]

  • test-issue-332-no-carto-tiles-e2e.js: 31/0
  • test-xss-333-pages-e2e.js: 11/0
  • test-issue-406-packets-live-channel-e2e.js (new from master): 21 checks passed
  • test-e2e-playwright.js stops fail-fast at test 7, "Version info lives on Perf dashboard" (#navStats 10 s timeout). This happens with plain and instrumented public/, and the same way against a master dd712bc9 server built and seeded the same way (port 13582). It is environmental, as noted in the earlier rounds, and CI runs that suite (see below).

Attack test from finding 1, repeated on the merged image [T]

This is a separate script, not one of the PR's tests. It uses a scratch bind dir owned by the host user, --network none, IDs 10001:10001 (setting in /app/data/.env) and #431's forwarded settings as compose renders them. The result is 42/42 as expected:

  • Services' user. Server and ingestor run as uid 10001.
  • Changing root-read files. As 10001:10001 in the running container, every one of these fails for .env, config.json and theme.json: append, truncate, chmod, remove, rename away, replace by rename, replace by symlink, hard link. So do chmod of the data dir and removing or chgrp-ing the marker. The three files are byte-, inode-, owner- and mode-identical afterwards.
  • Planting under a missing name. A theme.json the services create under a missing name is possible, because the dir must stay writable for SQLite. The next start is refused with [nonroot] ERROR: /app/data/theme.json is owned by 10001 ….
  • After the setting is removed. With the setting gone from env and .env, a services-owned .env containing a payload is refused because of the marker, and the payload never ran as root.

#431 behaviour with this PR [T]

  • Default unchanged. docker compose config of docker-compose.example.yml, with an empty .env and with a .env that sets MQTT_BROKER/ENABLE_PPROF/PPROF_HOST/PPROF_PORT_SERVER, is identical between master and the merged head (apart from the scratch path). All other compose files are byte-identical to master. The image's off-mode start is identical to the master image in all 16 combinations (above).
  • .env settings reach the container, also in on-mode. The forwarded settings reach the non-root processes: PPROF_PORT_SERVER=7060 and PPROF_PORT_INGESTOR=7061 appear in their /proc/<pid>/environ. Both pprof listeners answer on 7060/7061 as uid 10001, and nothing listens on the default 6060.
  • Bind addresses and broker check. PROD_MQTT_BIND/PPROF_BIND live only in compose files that this PR does not change. manage.sh broker-check is unchanged, and its test passes 24/0.

Mutants this round (one per item, all killed) [T]

Mutant Change Killed by
MD (N1) entrypoint: drop the .env trigger for preflight CI stub test, 4 checks (config linked, .env sourced as root)
R1 (sync) conflict resolved to #339's side only Go TestComposeFilesPassEveryProcessKnobToTheContainer
R2 (sync) conflict resolved to master's side only scripts/test-nonroot-helper.sh, 3 checks

N2 has no mutant because it was not changed.

CI per job [K]

Run 38440287010 on cddc8541 (pull_request, so CI tests the merge with current master), attempt 1, conclusion failure:

Job Result
Go Build & Test success. "Non-root container helper unit tests + ShellCheck": 92 passed, 0 failed. "Non-root container start path, supervisord stubbed": 1..309, RESULT: PASS (309 checks), including the new no-marker cases 245–256. test-all.sh: 243/0. Frontend helpers: 713/0. Server and ingestor suites: ok.
Validate Docker Image success
Playwright E2E Tests failure, in exactly one check: test-issue-1692-packets-init-parallel-e2e.js, "first table row appears < 3000ms despite 4000ms /api/observers stub" (waitForSelector 10 s timeout). Every earlier E2E file passed, including test-e2e-playwright.js 151/154 (3 skipped by the suite) and the new test-issue-406-packets-live-channel-e2e.js.
Build & Publish Docker Image failure at its gate step "Require tests and image validation", because Playwright failed. Build and push were skipped.
Release Artifacts, Deploy Staging, Publish Badges & Summary skipped

About the E2E failure:

Invariants [T]

  • 0 added interface{} lines in the whole PR diff against master dd712bc9.
  • scripts/check-xss-sinks.sh --diff origin/master exits 0: "no public/**/*.{js,html} changes to scan".
  • Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, the same as master.
  • No closing keywords in the commits.
  • cmd/server stays read-only, and readonly_invariant_test.go passes.

Remaining issues

  1. N2 (follow-up): in on-mode, GET /api/config/geo-filter reports writeEnabled: true, so the customizer offers a save that always gets the clear 409. A fix needs a separate field, because writeEnabled also gates the prune section, which works in on-mode. It also needs a customizer change and an E2E (see item 3 above). I suggest a separate issue. [A]
  2. Not done here: an operator who writes CORESCOPE_RUN_AS_NONROOT=… into the compose project's .env changes nothing in the container, because none of the compose files forwards those three settings. That is by design (the prod and staging compose files stay unchanged), and the docs name only -e, compose environment: and /app/data/.env. ops(runtime): make bundled MQTT and pprof exposure explicit settings (#372) #431's docs make the general "only forwarded settings reach the container" point. [A]
  3. Unchanged from round 2:
    • Host users in CORESCOPE_GID can create files in the data dir. Such files are refused for root's use.
    • POSIX ACLs on the three root-read files are not evaluated.
    • The full-image Docker test stays local-only.
    • The staging rehearsal and the production decision remain with the owner. [A]
  4. Local environment: test-e2e-playwright.js test 7 times out locally on both head and master (see above). [T]

Not verified

  • No staging or production run (by design), and no rehearsal of the staging checklist.
  • arm64 images and Playwright coverage collection.
  • A real earlier-draft (eb49596f) volume migrated to this head. The new no-marker gate case reproduces its trigger in CI.

@dborup-agent

Copy link
Copy Markdown
Collaborator Author

Review — CS-pve-agent2 PR#404 — head cddc854

Dom: APPROVE

Short independent re-review of the master sync (3b614bca → cddc8541). Evidence tags: [T] run by me, [A] analysis or code reading, [K] known from CI or GitHub.

  • Head cddc8541 was unchanged before and after the review (git ls-remote). [T]
  • Merged tree: git merge-tree --write-tree origin/master cddc8541 on master a20c611e merges cleanly. It changes the same 15 files as the PR against dd712bc9, and nothing under public/. [T]
  • I worked on git archive copies only. Images were built locally and run on local Docker with --network none and scratch bind dirs. No staging or production system was touched, no server API key was used (the geo-filter/attack runs used a throwaway key in a scratch config.json), and nothing was written upstream. [T]

Findings

# Severity Area Finding
– – – No blocking findings and no new nits.
N2 Nit (already tracked) GET /api/config/geo-filter Unchanged from the previous round: in on-mode writeEnabled is still true. The author's reason for not touching it in a sync (the same flag gates the prune section, which works in on-mode) holds up against the code, and the follow-up is correctly left to a separate issue. [A]

Answers to the review points

1. Is the conflict resolution against #431 correct? Yes. [T][A]

2. Attack test from finding 1, repeated on the merged image. [T]
Setup: image built from the merged tree, scratch bind dir owned by the host user, config.json/theme.json/.env 644, IDs 10001:10001 with the setting in .env, plus #431's forwarded ENABLE_PPROF/PPROF_PORT_SERVER=7060/PPROF_PORT_INGESTOR=7061.

  • Server and ingestor ran as uid 10001 with CapEff: 0. supervisord ran as root.
  • The forwarded pprof settings reached the non-root server process (/proc/<pid>/environ), and the pprof endpoints answered on 7060 and 7061 as uid 10001. Nothing answered on 6060.
  • As 10001:10001 in the running container, for each of .env, config.json and theme.json, append, truncate, chmod, remove, rename away, replace by rename, replace by symlink and hard link all failed. So did chmod of the data dir and removing or chgrp-ing the marker. 27 of 27 attempts were blocked. Inode, owner, mode and sha256 of the three files were identical afterwards.
  • A theme.json created by the services under a missing name: the next start exited 1 with [nonroot] ERROR: /app/data/theme.json is owned by 10001, the services' user ….
  • With the setting removed from env and .env, a services-created .env containing a payload was refused because of the marker (exit 1, [nonroot] ERROR: /app/data/.env is owned by 10001 …), and the payload never ran as root.

3. Default unchanged. [T] scripts/test-docker-nonroot.sh <merged> <master image from a20c611e> passed 260/260. That includes 16/16 off-mode snapshots (unset/empty/0/false × the four DISABLE_* combinations) identical to the master image. I did not re-run the compose-render comparison.

4. CI. [K] Run 38440287010 on cddc8541, attempt 2 (rerun of the failed job): Go Build & Test, Validate Docker Image, Playwright E2E and Build & Publish Docker Image all succeeded. Release Artifacts, Deploy Staging and Publish Badges were skipped (fork guards). The attempt-1 failure was test-issue-1692 only (#439, fixed on master in #440). Locally, on the merged tree (which contains #440), that test passes 2/2 — see below. The known flaky #256 and #267 did not appear.

Invariants

  • cmd/server stays read-only: readonly_invariant_test.go passes in the server suite below. [T]
  • 0 added map[string]interface{} in the diff against master. [T]
  • No public/ or CSS changes, so no hardcoded colours. scripts/check-xss-sinks.sh --diff origin/master exits 0. [T]
  • Fork guards: 9 in deploy.yml and 1 in release-fast-path.yml, same as master. [T]
  • No closing keywords in the title, body or any of the 12 PR commits. [T]
  • All 12 PR commits are authored and committed by dborup <kontakt@meshview.dk>. [T]
  • No behaviour change beyond the purpose. The example compose only gains comments, and off-mode behaves like master (see 3). [T][A]

Acceptance criteria of #339

Unchanged since the last approved head (table in the earlier review). The only test change since then is the N1 coverage. I re-checked that it is red on a mutant and green on the real code:

Item Test Red before Green after
N1: .env trigger of the start check on a volume without the marker test-nonroot-entrypoint.sh, gate cases 245–256 MD (drop the .env trigger): 4 checks fail (config linked, .env sourced as root) [T] 309/309 on the real entrypoint [T]

Tests and mutants (merged tree on a20c611e) [T]

  • cd cmd/server && go test ./...: ok.
  • cd cmd/ingestor && go test ./...: ok.
  • sh test-all.sh: 244 passed, 0 failed. node test-frontend-helpers.js: 713 passed, 0 failed.
  • bash scripts/test-nonroot-helper.sh: 92/0. bash scripts/test-nonroot-entrypoint.sh: 309/309. scripts/test-docker-nonroot.sh: 260/260.
  • E2E against a local Go server on e2e-fixture.db, prepared as in CI (freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486 seed SQL, corescope-migrate, seeds 2073, 199 and 245), port 13581, stopped by the pid found from the port:
    • test-issue-1692-packets-init-parallel-e2e.js: 2/0
    • test-issue-332-no-carto-tiles-e2e.js: 31/0
    • test-xss-333-pages-e2e.js: 11/0
    • test-issue-406-packets-live-channel-e2e.js: 21 checks passed
Mutant Change Result
MD entrypoint: drop the .env trigger for preflight Killed by the new gate cases (4 checks)
MX entrypoint: drop the marker trigger for preflight Killed (11 checks, e.g. service-created .env after the setting is removed, marker not written by root, off-after-on)
R1 example compose: drop #431's PPROF_PORT_* pass-through Killed by TestComposeFilesPassEveryProcessKnobToTheContainer
R2 example compose: drop #339's commented lines Killed by test-nonroot-helper.sh (3 checks)

Not verified

  • No staging or production run (by design), and no rehearsal of the staging checklist.
  • The full test-e2e-playwright.js locally (it passed in the CI rerun), and the other E2E files of the CI list.
  • arm64 images, Playwright coverage collection, POSIX ACLs on the root-read files, and hosts with fs.protected_regular or fs.protected_hardlinks set to 0.
  • A real volume from the earlier draft (eb49596f); only its trigger is covered, by the new no-marker gate case.
  • docker compose config comparison of the example file against master (the example now carries ops(runtime): make bundled MQTT and pprof exposure explicit settings (#372) #431's lines, but I only read them).

@dborup
dborup marked this pull request as ready for review October 13, 2026 19:39
@dborup
dborup merged commit e67d36f into master Oct 13, 2026
12 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants