From d900cafee890d120f878f12aaf61dea678be961c Mon Sep 17 00:00:00 2001 From: Andrei Onel Date: Thu, 13 Aug 2026 22:26:31 +0100 Subject: [PATCH 1/3] Let container logs reach journald, so the Logs tab has something to show (#414) The per-app Logs tab was empty on every real box, for every app, since it shipped: it opened the stream and waited forever. host-agent-real tails an app by running `journalctl CONTAINER_NAME= -f`, and CONTAINER_NAME is a field only Docker's journald log driver sets. LOGGING.md calls that driver switch "the single biggest configuration decision", but no image ever wrote /etc/docker/daemon.json, so both real profiles ran Docker's json-file default and the match returned nothing. Two blind spots hid it, both the same mistake: the inner loop's fake host-agent reads `docker logs -f`, and the cloud boot proof greps the brain through `docker logs`. Both work on any driver, so nothing exercised the query the feature depends on. Ship the driver on both images, disable journald's per-unit rate limit for dockerd (every container now shares docker.service's one bucket, so a chatty container would silently starve the rest), and assert in the boot proof that `journalctl CONTAINER_NAME=malmo-brain` returns lines -- not via docker logs, since reading it that way is why this shipped. The journal is still volatile; persistence is the next entry. --- dev/cloud/cloud-assertions.sh | 23 +++++++++++ dev/cloud/mkosi.conf | 12 ++++++ dev/cloud/mkosi.extra/etc/docker/daemon.json | 3 ++ .../docker.service.d/10-malmo-logging.conf | 18 +++++++++ dev/test-qemu/bootstrap.sh | 31 ++++++++++++++- docs/progress/README.md | 4 +- .../container-logs-journald-driver.md | 38 +++++++++++++++++++ docs/specs/LOGGING.md | 2 +- 8 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 dev/cloud/mkosi.extra/etc/docker/daemon.json create mode 100644 dev/cloud/mkosi.extra/etc/systemd/system/docker.service.d/10-malmo-logging.conf create mode 100644 docs/progress/container-logs-journald-driver.md diff --git a/dev/cloud/cloud-assertions.sh b/dev/cloud/cloud-assertions.sh index 5e4d0a92..a950acd5 100755 --- a/dev/cloud/cloud-assertions.sh +++ b/dev/cloud/cloud-assertions.sh @@ -247,6 +247,29 @@ for c in $want; do grep -qw "$c" <<<"$running" || fail "control-plane container '$c' not running after 120s (have: $running)" done +# --- 5b. container stdout is readable through journald by CONTAINER_NAME — the +# EXACT query host-agent-real's per-app log tail runs +# (internal/hostagent/journalsource: `journalctl CONTAINER_NAME=`). +# This is deliberately not a `docker logs` read: `docker logs` works on every log +# driver, which is precisely why the driver being wrong went unnoticed and every +# app's Logs tab hung on "Waiting for log output…" on a real box. Assert the +# driver, then assert the query it exists to serve actually returns lines. +log_driver="$(docker info --format '{{.LoggingDriver}}' 2>/dev/null || true)" +[ "$log_driver" = journald ] || \ + fail "docker log driver is '$log_driver' (want journald) — the per-app Logs tab reads journalctl CONTAINER_NAME=, which only the journald driver populates" +# malmo-brain is the safe probe: it is up by now (step 5) and always writes +# startup milestones to stdout. Poll — journald ingest can lag container start +# by a beat under a loaded TCG boot, same race wait_brain_log documents. +brain_journal="" +for _i in $(seq 1 60); do + brain_journal="$(journalctl CONTAINER_NAME=malmo-brain -b --no-pager -n 5 -o cat 2>/dev/null || true)" + [ -n "$brain_journal" ] && break + sleep 1 +done +[ -n "$brain_journal" ] || \ + fail "journalctl CONTAINER_NAME=malmo-brain returned nothing after 60s — container stdout is not reaching journald, so the per-app Logs tab will hang for every app" +echo "cloud-assertions: container logs readable via journalctl CONTAINER_NAME= (driver=journald)" + # --- 6. proxy boundary: the brain reaches Docker only through the socket-proxy, # never the raw socket (CONTROL_PLANE.md # Docker socket exposure). brain_sock="$(docker inspect malmo-brain --format '{{range .Mounts}}{{println .Source}}{{end}}' 2>/dev/null | grep -c 'docker.sock' || true)" diff --git a/dev/cloud/mkosi.conf b/dev/cloud/mkosi.conf index ba1ea751..c4e37aee 100644 --- a/dev/cloud/mkosi.conf +++ b/dev/cloud/mkosi.conf @@ -193,6 +193,18 @@ Packages= # the brain reads at startup (#202/C1a, internal/profile.Read) to select the # hosted seams (e.g. skip mDNS publish). A static one-line file committed under # mkosi.extra/. +# +# Also carries the container-logging wiring, which is load-bearing and easy to +# miss because JSON takes no comments. /etc/docker/daemon.json sets Docker's +# daemon-wide log driver to journald (LOGGING.md # Docker daemon uses the +# `journald` log driver). host-agent-real's per-app log tail runs +# `journalctl CONTAINER_NAME=` (internal/hostagent/journalsource), +# and CONTAINER_NAME is a field ONLY the journald driver sets — under Docker's +# default json-file driver that match returns nothing and the dashboard's Logs +# tab hangs on "Waiting for log output…" forever, for every app. The paired +# docker.service.d/10-malmo-logging.conf drop-in lifts journald's per-unit rate +# limit for dockerd, because that routing puts every container on docker.service's +# single budget (LOGGING.md # Tuning). ExtraTrees=mkosi.extra # The first-boot runtime wiring (#242): the slim host-agent + its units, the baked # control-plane image bundle + loader, the control-plane compose, the PAM stack, diff --git a/dev/cloud/mkosi.extra/etc/docker/daemon.json b/dev/cloud/mkosi.extra/etc/docker/daemon.json new file mode 100644 index 00000000..fe3a9bc5 --- /dev/null +++ b/dev/cloud/mkosi.extra/etc/docker/daemon.json @@ -0,0 +1,3 @@ +{ + "log-driver": "journald" +} diff --git a/dev/cloud/mkosi.extra/etc/systemd/system/docker.service.d/10-malmo-logging.conf b/dev/cloud/mkosi.extra/etc/systemd/system/docker.service.d/10-malmo-logging.conf new file mode 100644 index 00000000..d825b624 --- /dev/null +++ b/dev/cloud/mkosi.extra/etc/systemd/system/docker.service.d/10-malmo-logging.conf @@ -0,0 +1,18 @@ +# Docker is the deliberate exception to journald's per-unit rate limit +# (LOGGING.md # Tuning). /etc/docker/daemon.json routes every container's +# stdout through dockerd, and journald enforces its limit against +# _SYSTEMD_UNIT — so all containers on the box share ONE bucket attributed to +# docker.service. Under the default 10000-per-30s, a single chatty container +# makes journald silently drop messages from every other container and from +# dockerd itself, which surfaces as a per-app Logs tab that looks healthy while +# missing lines. +# +# Disabling the limit here removes that cross-container starvation without +# loosening it for real system services (sshd brute-force spam still caps). +# The journal's size cap is then the sole backpressure for container output — +# a misbehaving container ages useful history out faster than rate-limiting +# would, which LOGGING.md accepts on the grounds that silent drops are the +# worse debugging experience. +[Service] +LogRateLimitIntervalSec=0 +LogRateLimitBurst=0 diff --git a/dev/test-qemu/bootstrap.sh b/dev/test-qemu/bootstrap.sh index df010dec..5e5a6426 100755 --- a/dev/test-qemu/bootstrap.sh +++ b/dev/test-qemu/bootstrap.sh @@ -21,7 +21,7 @@ TEST_DIR="${REPO_ROOT}/dev/test-qemu" WORK="${REPO_ROOT}/.dev/qemu" EXTRA="${TEST_DIR}/mkosi.extra" CANARY="${WORK}/.malmo-medium-ready" -CANARY_VERSION="v26" # bump when mkosi.conf changes require a clean rebuild +CANARY_VERSION="v27" # bump when mkosi.conf changes require a clean rebuild PASSPHRASE_FILE="${TEST_DIR}/mkosi.passphrase" # LUKS recovery key (slice 0023); gitignored IMAGE_OUT="${WORK}/malmo-medium.raw" SSH_KEY="${WORK}/ssh-key" @@ -208,8 +208,37 @@ mkdir -p "$EXTRA/etc/systemd/system" \ "$EXTRA/etc/ssh/sshd_config.d" \ "$EXTRA/etc/pam.d" \ "$EXTRA/etc/malmo/secrets" \ + "$EXTRA/etc/docker" \ + "$EXTRA/etc/systemd/system/docker.service.d" \ "$EXTRA/usr/local/bin" +# Container logging. Docker's daemon-wide log driver must be journald +# (LOGGING.md # Docker daemon uses the `journald` log driver): host-agent-real's +# per-app log tail runs `journalctl CONTAINER_NAME=` +# (internal/hostagent/journalsource), and CONTAINER_NAME is set only by that +# driver. Under Docker's json-file default the match returns nothing and the +# dashboard's Logs tab waits forever, for every app. Kept byte-identical to the +# hosted lane's committed dev/cloud/mkosi.extra/etc/docker/daemon.json — both +# real profiles run the same host-agent binary against the same expectation. +cat > "$EXTRA/etc/docker/daemon.json" <<'EOF' +{ + "log-driver": "journald" +} +EOF + +# Docker is the deliberate exception to journald's per-unit rate limit +# (LOGGING.md # Tuning). journald enforces the limit against _SYSTEMD_UNIT, so +# with the driver above every container on the box shares ONE bucket attributed +# to docker.service; under the 10000-per-30s default a single chatty container +# silently starves every other container's lines. Disabled here rather than +# globally, so real system services keep their cap. Byte-identical to the +# hosted lane's dev/cloud/mkosi.extra/.../10-malmo-logging.conf. +cat > "$EXTRA/etc/systemd/system/docker.service.d/10-malmo-logging.conf" <<'EOF' +[Service] +LogRateLimitIntervalSec=0 +LogRateLimitBurst=0 +EOF + # Recovery keyfile baked at the production path STORAGE.md specifies # (/etc/malmo/secrets/luks-recovery.key, mode 0400, root-owned). The # first-boot enrollment service reads it via systemd-cryptenroll diff --git a/docs/progress/README.md b/docs/progress/README.md index c64ed28d..a34bc585 100644 --- a/docs/progress/README.md +++ b/docs/progress/README.md @@ -20,7 +20,8 @@ The implementation slice queue, ordered. Each item links back to the progress en This is the **maintainer's critical-path** queue. Work carved off for **parallel contributors** lives in [GitHub Issues](https://github.com/malmoos/malmo/issues) (some items there are pulled from these "what's next" follow-ups). The two are kept from overlapping on purpose. See [`../dev/contributing.md`](../dev/contributing.md) for the contributor loop. -1. **GPU + device capacity enforcement.** `install-permissions-enforcement.md` deferred `gpu` enforcement and device-existence validation (both need a host hardware-introspection endpoint). A 422 from the brain will surface correctly in the UI via the existing `dialogError` path ([install-consent-ui.md](install-consent-ui.md)) once the host endpoint lands. See `NEXT.md` # GPU. +1. **Persistent journal.** [container-logs-journald-driver.md](container-logs-journald-driver.md) made container logs reach journald, but neither image creates `/var/log/journal` or sets `Storage=persistent`, so the journal is volatile and app-log scrollback is lost on every reboot — `LOGGING.md` # Per-app logs promises "scrollback up to journald's cap". Ship the journald drop-in from `LOGGING.md` # Tuning (`Storage=persistent`, `SystemMaxUse=1G`, `RuntimeMaxUse=128M`) on both images, sized against the root partition that grows at first boot. Also restores the size-based backpressure # Tuning assumes now that docker's per-unit rate limit is off. +2. **GPU + device capacity enforcement.** `install-permissions-enforcement.md` deferred `gpu` enforcement and device-existence validation (both need a host hardware-introspection endpoint). A 422 from the brain will surface correctly in the UI via the existing `dialogError` path ([install-consent-ui.md](install-consent-ui.md)) once the host endpoint lands. See `NEXT.md` # GPU. ## Entry template @@ -238,3 +239,4 @@ Oldest first; append new entries to the bottom. | [update-target-per-box.md](update-target-per-box.md): **Closes #404.** A provisioned box can now be pointed at its own update target. `host-agent.service` imports two optional systemd credentials on the same path `malmo.seed` uses: `malmo.update_target_url` and `malmo.update_window`. Precedence is credential > env var > built-in default, so a box provisioned with neither behaves exactly as before, while one carrying a credential is pinned to it. **An unusable target URL is refused and the loop does not start** rather than falling back to the fleet endpoint, so a box pinned to a candidate cannot quietly join `stable`; an unusable *window* still falls back with a warning, because a wrong hour cannot send a box to a wrong version. Which source won is logged at startup, at **warn** when it is not the fleet default. None of the resolution is build-tagged, because CI vets and tests untagged, so only the call site keeps `//go:build hosted`. Also writes down the **downgrade property** (`UPDATES.md` # 8.4): the loop has no notion of "forward", so a box newer than its target rolls back to it | done | | [update-target-from-seed.md](update-target-from-seed.md) — **Closes #407.** #404's per-box update target shipped as two `ImportCredential=` lines and **never reached a real box**: a hosted box gets its per-box facts as metadata user-data, which does not feed systemd's credential store, so the mechanism worked in the QEMU lane and was inert everywhere else. The target URL moves to the **provisioning seed** — the only production channel for a per-box fact, and write-once, which fits a fact fixed for the life of the box ("which control plane does this box belong to"). Precedence becomes seed > env var > compiled default; `from` logs `seed` where it logged `credential`. host-agent reads the seed **itself**, not through the brain's `profile.ReadSeed`, which hard-errors on a missing `box_id` — a box seeded only for updates, and a box with no seed at all, must both work. **Three seed states, decided on purpose:** absent falls through and is not an error (the appliance path), a readable seed is used when the field is set, and a **malformed** one is refused with the loop not starting, because bytes we cannot parse might have pinned this box and reading `stable` instead is the one outcome pinning exists to prevent. **The credential path is deleted, not kept alongside** — two mechanisms for one setting, one of which silently does nothing in production, is worse than either. The **window does not move to the seed and will not**: it has to be changeable while a box runs and user-data cannot be rewritten, so it waits for the control plane's answer. The cloud lane's `update` boot now carries the URL in its **seed** rather than an `Environment=` drop-in, and asserts `from=seed` before the apply — without that, the apply passes on a box that read its target from anywhere at all, which is exactly how the credential version stayed green. **Gaps:** the boot-proof edit is unexercised locally (needs root + KVM, CI-only); nothing cloud-side sets the field yet, so this ships inert; an empty field is treated as absent, not as a refusal | done | | [update-target-per-box-and-window.md](update-target-per-box-and-window.md) — **Closes #408.** A hosted box now says **which box is asking**: `GET ?box_id=`, with the id read off the same seed pass as the target URL (#407) rather than a second file read. That is what makes a per-box answer possible at all — `UPDATES.md` # 8.1 specified a target per `box_id` from the start, and until now the answer was the same for every box. **A box with no identity sends no parameter at all**, not an empty one: an appliance box and an unseeded hosted box ask byte-for-byte what they asked before. The answer also gains an optional `window` field, so **when** to update joins **what** to run as a fact the control plane owns and can change while a box runs — the home the seed could never be, because user-data is write-once. Precedence is **answer > `MALMO_UPDATE_WINDOW` > built-in default**, and an answer with **no** window field means "no opinion", never "use the default", or a silent field would outrank an operator's variable. An unusable window **warns and falls back to the box's own setting** (not to the default), keeping #404's asymmetry: a wrong hour only applies at the wrong time, a wrong target sends the box to the wrong version. The window is resolved per tick, deduped with memory separate from the target lines, and logged with `from=answer` — a fourth value for that field. The `update` boot now inverts its window setup — `MALMO_UPDATE_WINDOW=04:00-04:01` against a whole-day window in the answer — so the apply can only pass if the answer won, and asserts the `box_id` and `from=answer` before it. **The identity is deliberately weak and written down, not fixed:** a bare `box_id` on an unauthenticated endpoint can be read and claimed by anyone who knows it, accepted only because the ask is a **read**, with the trade-off in `UPDATES.md` # 8.1 and the real credential a new `NEXT.md` item. Nothing that mutates state may be built on it. **Gaps:** the boot-proof edit is unexercised locally (CI-only lane); nothing control-plane-side reads the parameter or sends a window yet, so it ships inert; the box id is not validated | done | +| [container-logs-journald-driver.md](container-logs-journald-driver.md) — The dashboard's per-app **Logs tab was empty on every real box**, hosted and appliance, for every app, since the tail shipped: it opened the stream and sat on "Waiting for log output…" forever. Not a code bug — a missing image file. `LOGGING.md` calls the Docker log-driver switch "the single biggest configuration decision" and host-agent-real is built straight on it (`internal/hostagent/journalsource` runs `journalctl CONTAINER_NAME= -f`, and **`CONTAINER_NAME` is a field only the journald driver sets**), but **no image ever wrote `/etc/docker/daemon.json`**, so both real profiles ran Docker's `json-file` default, the match returned nothing, and `journalctl -f` blocked on a stream that could never produce a line. **Two blind spots hid it, and both are the same mistake:** the inner loop's fake host-agent reads `docker logs -f` (`cmd/host-agent/dockerlogsource.go`) and the cloud boot proof greps the brain through `docker logs` — both work on *any* driver, so nothing ever exercised the query the feature depends on; `cloud-assertions.sh` even documents the daemon as being on json-file. Ships `daemon.json` on the hosted image (committed, so the boot-proof image inherits it via `Include=..`) and byte-identically into the appliance lane's generated tree, with `CANARY_VERSION` bumped so the medium lane rebuilds. Rate limiting follows the spec rather than the obvious fix: journald counts against `_SYSTEMD_UNIT`, so routing every container through dockerd puts them all in **one** bucket where a single chatty container silently starves the rest — disabled **per-unit** on `docker.service` (`LOGGING.md` # Tuning), not raised globally, so sshd brute-force spam still caps. New boot-proof assertion 5b asserts the driver **and** that `journalctl CONTAINER_NAME=malmo-brain` returns lines, deliberately not through `docker logs`, since reading it that way is exactly why this shipped. **Gaps:** the journal is still **volatile** — neither image sets `Storage=persistent` or creates `/var/log/journal`, so scrollback dies on reboot and `SystemMaxUse`, which # Tuning assumes is the backpressure once docker's rate limit is off, is unset; the appliance change is unbooted (medium lane is local-only, needs root + KVM); the tab still follows `main_service` only, so sidecar output stays unreachable | done | diff --git a/docs/progress/container-logs-journald-driver.md b/docs/progress/container-logs-journald-driver.md new file mode 100644 index 00000000..8d42a8fa --- /dev/null +++ b/docs/progress/container-logs-journald-driver.md @@ -0,0 +1,38 @@ +# Container logs reach journald — Docker's log driver on both real images + +- **Status:** done +- **Date:** 2026-08-13 +- **Specs touched:** docs/specs/LOGGING.md + +## What was done + +The dashboard's per-app Logs tab was empty on every real box — hosted and appliance, every app, since the tail shipped. It opened the stream and sat on "Waiting for log output…" forever. This makes it work. + +The cause was a missing image file, not a code bug. `LOGGING.md` # Docker daemon uses the `journald` log driver calls the driver switch "the single biggest configuration decision", and host-agent-real is built directly on it: `internal/hostagent/journalsource` runs `journalctl CONTAINER_NAME= -f -o json`, and `CONTAINER_NAME` is a field only Docker's journald driver sets. But no image ever wrote `/etc/docker/daemon.json`, so both real profiles ran on Docker's `json-file` default, the match returned nothing, and `journalctl -f` blocked on a stream that could never produce a line. + +Shipped: + +- **`dev/cloud/mkosi.extra/etc/docker/daemon.json`** — `{"log-driver": "journald"}` for the hosted image. Committed, so the boot-proof test image picks it up too (`dev/cloud/test/mkosi.conf` does `Include=..`, inheriting `ExtraTrees=mkosi.extra`). +- **`dev/cloud/mkosi.extra/etc/systemd/system/docker.service.d/10-malmo-logging.conf`** — `LogRateLimitIntervalSec=0` / `LogRateLimitBurst=0`, exactly as `LOGGING.md` # Tuning specifies. journald enforces its rate limit against `_SYSTEMD_UNIT`, so once every container's stdout flows through dockerd they all share one bucket attributed to `docker.service`; under the 10000-per-30s default one chatty container silently starves every other container's lines. Disabled per-unit rather than globally, so real system services keep their cap. +- **`dev/test-qemu/bootstrap.sh`** — stages both files into the appliance lane's generated `mkosi.extra/`, byte-identical to the hosted lane. `CANARY_VERSION` bumped v26 → v27 so the medium lane rebuilds rather than booting a cached image without them. +- **`dev/cloud/cloud-assertions.sh`** — new assertion 5b in the boot proof: `docker info` reports `journald`, and `journalctl CONTAINER_NAME=malmo-brain` returns lines. Polled, because journald ingest can lag container start under a loaded TCG boot (the same race `wait_brain_log` already documents). +- **`dev/cloud/mkosi.conf`** — the comment block over `ExtraTrees=mkosi.extra` now explains the logging wiring. JSON takes no comments and `daemon.json` is two lines with no hint of what depends on it, so the "why" needed a home a reader will actually reach. +- **`docs/specs/LOGGING.md`** — the drop-in's filename synced to the `10-` prefix the rest of the tree uses. + +## How it maps to the specs + +Realizes `LOGGING.md` # Docker daemon uses the `journald` log driver and the Docker half of # Tuning, both of which were written but never built. Makes # Per-app logs true on a real box for the first time: "Source: brain → host-agent → `journalctl CONTAINER_NAME= --follow`" was accurate about the code path and wrong about whether it returned anything. + +## Known gaps & deviations + +- **The journal is still volatile, so scrollback dies on reboot.** `LOGGING.md` # Journal lives on the OS drive specifies a persistent journal at `/var/log/journal/` with `Storage=persistent`, `SystemMaxUse=1G`, `RuntimeMaxUse=128M`. Neither image sets any of it and neither creates `/var/log/journal`, so Debian's `Storage=auto` lands on volatile — the journal lives in `/run` under journald's default cap (10% of RAM) and is lost on every reboot. Live tail and the 100-line backfill work; `# Per-app logs`' promise of "scrollback up to journald's cap" does not, and today's cap is "until you reboot". Deliberately left out of this change: it is a disk-sizing decision on a root partition that grows at first boot, and it would have held up a fix for a fully broken feature. Named as the next item below. +- **Backpressure is weaker than the spec assumes.** `# Tuning` reasons that with docker's rate limit off, `SystemMaxUse=1G` becomes "the sole backpressure for container output". `SystemMaxUse` is not set yet, so the real ceiling is journald's default runtime cap. Bounded and self-rotating, so this is not a disk-fill risk, but a chatty container ages useful history out faster than the spec intends until persistence lands. +- **Not verified on a booted appliance image.** The medium lane (swtpm + LUKS, `dev/test-qemu`) is local-only and needs root plus `/dev/kvm`. The appliance change is a byte-identical copy of the hosted one and the canary is bumped, but it has not been booted. The hosted lane is covered by the boot proof. +- **Only the per-app tail is fixed.** The System logs view and the diagnostic bundle's journal export are not built yet. Both were designed against the same assumption and would have had the same hole; they will now get container output for free when they land. +- **The Logs tab has no empty-state signal.** `LOGGING.md` # Apps are expected to log to stdout specifies a runtime hint — "No logs received. This app may be logging to a file." — after a container produces zero entries over a window. Not built. Had it existed, this bug would have announced itself on every app card instead of looking like a slow stream. + +## What's next + +1. **Persistent journal** — create `/var/log/journal` and ship the journald drop-in from `LOGGING.md` # Tuning (`Storage=persistent`, `SystemMaxUse=1G`, `RuntimeMaxUse=128M`) on both images, sized against the grown root. Closes the scrollback gap and restores the backpressure the disabled docker rate limit assumes. +2. **The zero-entries empty state** on the app card, per `# Apps are expected to log to stdout`. The cheap signal that would have caught this. +3. **Sidecar logs are unreachable.** The tail follows `main_service` only (`internal/lifecycle/lifecycle.go:1533`; `APP_LIFECYCLE.md` pins `container_name` on the main service by design), so a one-shot bootstrap container's output has no UI path. This is what sent `uptimepage` users looking for an owner sign-in link that the Logs tab can never show — worth a decision on whether the tab should follow the whole compose project. diff --git a/docs/specs/LOGGING.md b/docs/specs/LOGGING.md index c2636736..7ce36be0 100644 --- a/docs/specs/LOGGING.md +++ b/docs/specs/LOGGING.md @@ -95,7 +95,7 @@ RateLimitBurst=10000 - **128 MB volatile cap** for the pre-`/var/log/journal/`-ready early-boot window. - **Rate limit: 10000 messages / 30s per systemd unit.** Generous burst for system services. Caps `sshd` brute-force spam, runaway daemons, anything attributed to a real systemd unit. -Docker is the deliberate exception, at `/etc/systemd/system/docker.service.d/malmo-logging.conf`: +Docker is the deliberate exception, at `/etc/systemd/system/docker.service.d/10-malmo-logging.conf`: ``` [Service] From 3de771919f5dd0f63cc91ce691c6e44992509641 Mon Sep 17 00:00:00 2001 From: Andrei Onel Date: Fri, 14 Aug 2026 16:52:04 +0100 Subject: [PATCH 2/3] Let an owner-only app keep one door open for its own API (#415) (#416) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Let an owner-only app keep one door open for its own API (#415) Hosted exposure was whole-app and binary, so an app that pairs a token-authed API with a session-authed UI had to go fully public for its SDK to work — which dropped the box login in front of the UI too. A manifest may now declare `access.public_paths`. A restricted app's route becomes one Caddy subroute: the declared paths proxy straight to the app, everything else stays gated. One route and one @id, so the insert-at-0 ordering against the catch-all is unchanged, and the proxy handler is built once and used on both branches so the per-cookie strip cannot land on one and not the other. The identity headers are now scrubbed on every hosted app route, not only where the gate runs. The gate does not run on a public path, so nothing there would overwrite a caller-supplied X-Malmo-User; the same hole already existed for a fully public app. The dashboard names the open paths instead of claiming a bare "Only me", reading them from the instance's own manifest copy so the label cannot drift from the route. The hosted lane's access boot carries the proof through real Caddy, including a bypass table that must stay gated. * Carry the declared paths on the exposure echo too, not just the detail GET The access label is exposure plus declared public paths together, so the toggle's echo carrying only the exposure would render "Only me" for an app with open paths until the next refetch. Both responses now go through one withPublicPaths helper. * Fix the bypass table's wrong expectation, and the glob that hid its failure Two things the first cloud run found. //v1/ never reaches the gate: Caddy collapses the duplicate slash and 301s to the normalized path before matching, so the app is not reached and the expectation was wrong, not the probe. The table now asserts the claim that matters — an undeclared path never reaches the app upstream anonymously — and allows the merge-redirect shape for that entry. Worse, that failure was reported green. run-cloud-tests.sh matched the guest verdict with a *PASS* glob and the reason read 'PATH GATE BYPASS', which contains PASS, so a red access boot printed 'boot access OK'. The verdict is now matched exactly. Any failure text containing bypass or password would have done this to any boot in this lane. --- api/openapi.json | 9 ++ api/openapi.yaml | 6 + dev/cloud/cloud-assertions.sh | 93 +++++++++++++ dev/cloud/run-cloud-tests.sh | 15 ++- dev/cloud/test/catalog/whoami/manifest.yml | 8 ++ docs/architecture.md | 4 +- docs/dev/hosted-boot-proof.md | 2 +- docs/progress/README.md | 1 + docs/progress/path-scoped-app-exposure.md | 57 ++++++++ docs/specs/APP_MANIFEST.md | 20 +++ docs/specs/DASHBOARD.md | 2 +- docs/specs/ENVIRONMENT.md | 4 + docs/specs/UPDATES.md | 3 + internal/api/api.go | 24 ++++ internal/api/appexposure.go | 7 +- internal/api/appexposure_test.go | 74 +++++++++++ internal/caddy/caddy.go | 76 ++++++++++- internal/caddy/caddy_test.go | 123 ++++++++++++++++++ internal/lifecycle/exposure_test.go | 114 ++++++++++++++++ internal/lifecycle/lifecycle.go | 29 ++++- internal/manifest/manifest.go | 105 +++++++++++++++ internal/manifest/manifest_test.go | 105 +++++++++++++++ web-ui/src/generated/openapi.ts | 1 + .../settings/InstalledAppDetailSection.vue | 18 ++- 24 files changed, 878 insertions(+), 22 deletions(-) create mode 100644 docs/progress/path-scoped-app-exposure.md diff --git a/api/openapi.json b/api/openapi.json index e59b36cb..64b9661e 100644 --- a/api/openapi.json +++ b/api/openapi.json @@ -1367,6 +1367,15 @@ "owner_username": { "type": "string" }, + "public_paths": { + "items": { + "type": "string" + }, + "type": [ + "array", + "null" + ] + }, "scope": { "type": "string" }, diff --git a/api/openapi.yaml b/api/openapi.yaml index 676a6cfa..6163eccf 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -962,6 +962,12 @@ components: type: string owner_username: type: string + public_paths: + items: + type: string + type: + - array + - "null" scope: type: string slug: diff --git a/dev/cloud/cloud-assertions.sh b/dev/cloud/cloud-assertions.sh index a950acd5..305b7400 100755 --- a/dev/cloud/cloud-assertions.sh +++ b/dev/cloud/cloud-assertions.sh @@ -352,6 +352,19 @@ full_get() { # PATH HOST [COOKIE] -> full response cat <&3 exec 3>&- 3<&- } +# Like full_get, plus one arbitrary extra request header. The path-scoped +# exposure probes (#415) need to send a FORGED X-Malmo-User and see what the app +# upstream received, which no cookie-only helper can do. +full_get_hdr() { # PATH HOST HEADER-LINE [COOKIE] -> full response + exec 3<>/dev/tcp/127.0.0.1/80 || return 1 + if [ -n "${4:-}" ]; then + printf 'GET %s HTTP/1.0\r\nHost: %s\r\n%s\r\nCookie: %s\r\nConnection: close\r\n\r\n' "$1" "$2" "$3" "$4" >&3 + else + printf 'GET %s HTTP/1.0\r\nHost: %s\r\n%s\r\nConnection: close\r\n\r\n' "$1" "$2" "$3" >&3 + fi + cat <&3 + exec 3>&- 3<&- +} full_send() { # METHOD PATH HOST COOKIE JSON -> full response local len; len="$(printf '%s' "$5" | wc -c | tr -d ' ')" exec 3<>/dev/tcp/127.0.0.1/80 || return 1 @@ -692,6 +705,86 @@ access) || fail "access: restricted-app 302 Location is not the box login: $(grep -i '^Location:' <<<"$n_resp" | tr -d '\r')" echo "cloud-assertions: restricted app gates an unauthenticated request (302 → box login)" + # 3b. PATH-SCOPED EXPOSURE (#415). The app is still restricted, and its manifest + # declares access.public_paths ["/v1", "/v1/*"]. The claim: those paths + # answer anonymously (an external SDK can post to the API) while every other + # path keeps the box login in front of it. This is the half a unit test + # cannot prove — Caddy matches a normalized path, the app sees the original + # URI, and that gap is where this bug class lives. + for p in /v1 /v1/traces "/v1/traces?x=1"; do + pp_resp="$(full_get "$p" "$app_host" 2>/dev/null || true)" + grep -q ' 200' <<<"$(status_of "$pp_resp")" && grep -qi 'Hostname:' <<<"$pp_resp" \ + || fail "access: declared public path $p did not reach the app anonymously: status='$(status_of "$pp_resp")'" + done + echo "cloud-assertions: declared public paths answer with no session (the token-authed API works while the UI stays gated)" + + # 3c. THE FORGERY GUARD, and the reason the scrub is unconditional. The gate does + # not run on a public path, so nothing there would overwrite a caller-supplied + # X-Malmo-User. If the app got a brain-vouched header on one path and a forged + # one on another it could not tell them apart, and "malmo says this is the + # owner" would become "anyone on the internet says so". + fg_resp="$(full_get_hdr /v1/traces "$app_host" 'X-Malmo-User: attacker' 2>/dev/null || true)" + grep -qi 'Hostname:' <<<"$fg_resp" || fail "access: forged-header probe did not reach the app on a public path" + grep -qiE '^X-Malmo-User:' <<<"$fg_resp" \ + && fail "access: IDENTITY FORGERY — a client-supplied X-Malmo-User survived to the app upstream on a public path: $(grep -i '^X-Malmo-User:' <<<"$fg_resp" | tr -d '\r')" + # Same forgery on the GATED path, with the owner's cookie: the app must receive + # the brain's value, never the caller's. + fg2_resp="$(full_get_hdr / "$app_host" 'X-Malmo-User: attacker' "$fa_cookie" 2>/dev/null || true)" + grep -qi 'Hostname:' <<<"$fg2_resp" || fail "access: forged-header probe did not reach the app on the gated path" + grep -qiE '^X-Malmo-User: *attacker' <<<"$fg2_resp" \ + && fail "access: IDENTITY FORGERY — a client-supplied X-Malmo-User survived the gate: $(grep -i '^X-Malmo-User:' <<<"$fg2_resp" | tr -d '\r')" + grep -qiE '^X-Malmo-User:' <<<"$fg2_resp" \ + || fail "access: the gated path lost the vouched X-Malmo-User entirely (the scrub is deleting the brain's own value)" + echo "cloud-assertions: identity headers scrubbed on both branches (forged X-Malmo-User never reaches the app; the vouched one still does)" + + # 3d. The #335 per-cookie strip holds on the public branch too — it is the same + # proxy handler on both sides of the subroute, and this proves it. + pc_resp="$(full_get /v1/traces "$app_host" "${fa_cookie}; probe=leakcheck" 2>/dev/null || true)" + grep -qi 'Hostname:' <<<"$pc_resp" || fail "access: public-path cookie probe did not reach the app" + grep -qiE '^Cookie:.*malmo_forward_auth=' <<<"$pc_resp" \ + && fail "access: COOKIE LEAK (public path) — the app upstream received malmo_forward_auth on a declared public path" + grep -qiE '^Cookie:.*probe=leakcheck' <<<"$pc_resp" \ + || fail "access: public path lost the app's own cookie — the strip is removing more than malmo_forward_auth" + echo "cloud-assertions: public paths strip only malmo_forward_auth (same proxy handler as the gated branch)" + + # 3e. THE BYPASS TABLE — the point of running this through real Caddy. Every + # entry is a request that must NOT be treated as a public path. The requests + # are written raw onto the socket (no client-side normalization), so what + # Caddy matches is exactly what is asserted here: + # /v1extra "/v1/*" must not behave like a bare "/v1*" prefix, or + # a sibling route would be exposed by accident; + # /v1/../ and //v1/ path traversal and slash-merging: Caddy matches the + # cleaned path, so these resolve to the app root; + # /v1/%2e%2e/ the encoded form of the same, the case where a matcher + # and an app can disagree about what the path is; + # /V1x, /admin plain non-matches, the control. + # A 302 to the box login is the pass condition: the gate ran. + # The claim asserted for every entry is the one that matters: the app is + # NOT reached without a session. How the box says no differs by entry: + # gate — the forward_auth gate ran and 302'd to the box login; + # merge — Caddy collapses the duplicate slash and 301's to the + # normalized path BEFORE matching, so the app is never reached + # and the redirect target is then judged on its own merits + # (`/v1/` is genuinely public, `//admin` normalizes to a gated + # `/admin`). This was the one real correction the first CI run + # produced: the probe is safe, the expectation was wrong. + for probe in "/v1extra|gate" "/v1/../|gate" "//v1/|merge" "/v1/%2e%2e/|gate" "/V1x|gate" "/admin|gate"; do + bad="${probe%|*}"; want="${probe#*|}" + bp_resp="$(full_get "$bad" "$app_host" 2>/dev/null || true)" + bp_status="$(status_of "$bp_resp")" + grep -qi 'Hostname:' <<<"$bp_resp" \ + && fail "access: UNGATED PATH — '$bad' reached the app upstream with no session; it is not a declared public path" + case "$want:$bp_status" in + gate:*" 302"*) ;; + merge:*" 301"*) + grep -qiE '^Location:.*//v1/' <<<"$bp_resp" \ + && fail "access: UNGATED PATH — '$bad' redirected without collapsing the duplicate slash: $(grep -i '^Location:' <<<"$bp_resp" | tr -d '\r')" + ;; + *) fail "access: UNGATED PATH — '$bad' answered '$bp_status', wanted $want; an undeclared path must never be served anonymously" ;; + esac + done + echo "cloud-assertions: undeclared paths stay closed (prefix footgun, traversal, encoded traversal, double slash, case variant)" + # 4. flip to PUBLIC via the exposure toggle (owner session; the endpoint is # hosted-only + owner-or-admin). Resolve the instance id from the running # container's malmo.instance_id label (whoami is FROM-scratch — no shell to diff --git a/dev/cloud/run-cloud-tests.sh b/dev/cloud/run-cloud-tests.sh index bdb4178b..5c77a7dc 100755 --- a/dev/cloud/run-cloud-tests.sh +++ b/dev/cloud/run-cloud-tests.sh @@ -327,8 +327,16 @@ run_boot() { return 1 fi echo "phase=${phase} verdict: ${v}" + # Match the verdict EXACTLY, not as a substring. The guest emits either + # "MALMO_CLOUD_ASSERTIONS: PASS" or "MALMO_CLOUD_ASSERTIONS: FAIL: ", + # and the old `*PASS*` glob read any failure whose REASON happened to contain + # the letters "pass" as a pass. That is not hypothetical: a new assertion + # failing with "PATH GATE BYPASS — ..." turned a genuinely red access boot + # into a green CI run, printing "boot access OK" under a FAIL verdict it had + # just echoed (#415). Any word like bypass/passphrase/password in a failure + # message re-opens it, so anchor on the verdict word itself. case "$v" in - *PASS*) + "MALMO_CLOUD_ASSERTIONS: PASS"*) # Print what the guest proved, not only that it passed. A PASS used to # discard every `cloud-assertions:` line (dump_serial runs on failure # only), so a scenario that silently stopped asserting — a section @@ -419,7 +427,10 @@ fi # SSO → installs whoami air-gapped → proves the restricted gate (302 without a # session, proxied-through WITH the owner's forward-auth cookie), the public toggle # (reachable with no session), and the Cookie-strip invariant (the app upstream never -# receives the cookie) in both modes. +# receives the cookie) in both modes. The same app declares access.public_paths, so +# the boot also proves the path-scoped carve-out (#415) through real Caddy: declared +# paths answer anonymously, forged identity headers never reach the app, and the +# path-matcher bypass table stays gated. if should_run access; then [ -n "$GO" ] && [ -x "$GO" ] || { echo "access boot needs go to mint the owner assertion; none found (\$GO='${GO:-}')" >&2 diff --git a/dev/cloud/test/catalog/whoami/manifest.yml b/dev/cloud/test/catalog/whoami/manifest.yml index 6547c597..8ccacbb2 100644 --- a/dev/cloud/test/catalog/whoami/manifest.yml +++ b/dev/cloud/test/catalog/whoami/manifest.yml @@ -28,6 +28,14 @@ permissions: internet: false lan: false +# Path-scoped exposure (#415). The lane needs an app that declares public paths +# to prove the carve-out, and whoami is the right one: it echoes the request, so +# the assertions can see exactly what the app upstream received on each branch. +# The exact entry and the prefix entry are both declared, because "/v1/*" does +# NOT match "/v1" itself in Caddy and a real API app needs both. +access: + public_paths: ["/v1", "/v1/*"] + images: traefik/whoami:v1.10.3: digest: sha256:43a68d10b9dfcfc3ffbfe4dd42100dc9aeaf29b3a5636c856337a5940f1b4f1c diff --git a/docs/architecture.md b/docs/architecture.md index ffa5e207..6b3d65b2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -79,12 +79,12 @@ are stated below. | Package | Owns | Imported by | |---|---|---| | `api` | HTTP handlers (huma), auth middleware, request/response shapes. The only package that knows about HTTP. | `cmd/brain` | -| `lifecycle` | The install transaction: door-1 (catalog) and door-2 (paste-a-compose), digest pinning, reconcile pass, health-wait, Caddy timing, uninstall. Owns the **one central route builder** (`buildRouteConfig`) that resolves each app's `caddy.RouteConfig` from profile + per-instance `exposure` (hosted owner-only default, `SetExposure` toggle, #306). Defines `DockerDriver` consumer-side. | `api`, `cmd/brain` | +| `lifecycle` | The install transaction: door-1 (catalog) and door-2 (paste-a-compose), digest pinning, reconcile pass, health-wait, Caddy timing, uninstall. Owns the **one central route builder** (`buildRouteConfig`) that resolves each app's `caddy.RouteConfig` from profile + per-instance `exposure` (hosted owner-only default, `SetExposure` toggle, #306) + the manifest's `access.public_paths` carve-out and the always-on identity-header scrub (#415). Defines `DockerDriver` consumer-side. | `api`, `cmd/brain` | | `store` | SQLite schema + queries. Sole persistence boundary. `ErrNotFound` is the only typed error. | `api`, `lifecycle`, `auth`, `audit`, `cmd/brain` | | `catalog` | Door-1 source behind a fixed six-method facade. Production (every profile) uses the control-plane thin client (`NewRemote`, `MALMO_CATALOG_URL`): fetches `GET /catalog/sync`, integrity-digest-verifies, last-good on-disk cache, proxies+caches assets. The snapshot also carries the store's authored landing page (a spotlight app + category groups, authored in `home.yml`) and the authored category vocabulary (id + display `label`, in authored order) verbatim; `Home()` projects both filtered to the box's environment, mirroring the control plane's own projection (`docs/specs/APP_STORE.md` # Landing page, # Category labels). Category display text is always the authored label, never derived from the id. The disk reader (`New`) is retained only as a test constructor; no catalog is baked into the image. | `lifecycle`, `api`, `cmd/brain` | | `manifest` | `manifest.yml` schema (parse + validate), and the synthesizer that wraps a pasted compose into a door-2 manifest. | `catalog`, `lifecycle`, `api` | | `admission` | The single compose admission policy applied to both doors (image pinning rules, forbidden constructs, etc.). | `lifecycle` | -| `caddy` | Client for Caddy's admin API. Site-block JSON generation lives here (per-app route via `AddRoute(RouteConfig)` — optional hosted `forward_auth` gate + a strip of the single `RouteConfig.StripCookieName` cookie from the `Cookie` header, never the whole header, #306/#335), plus the hosted wildcard-TLS automation policy (`EnsureWildcardTLS`: ACME DNS-01 via the `acmedns` provider for `*..malmo.network`). Profile-agnostic: the strip/gate policy is resolved by the caller. | `lifecycle`, `cmd/brain` | +| `caddy` | Client for Caddy's admin API. Site-block JSON generation lives here (per-app route via `AddRoute(RouteConfig)` — optional hosted `forward_auth` gate, a strip of the single `RouteConfig.StripCookieName` cookie from the `Cookie` header, never the whole header, #306/#335, an unconditional `RouteConfig.ScrubHeaders` delete, and a `subroute` that carves `RouteConfig.PublicPaths` out of the gate, #415), plus the hosted wildcard-TLS automation policy (`EnsureWildcardTLS`: ACME DNS-01 via the `acmedns` provider for `*..malmo.network`). Profile-agnostic: the strip/gate policy is resolved by the caller. | `lifecycle`, `cmd/brain` | | `profile` | The environment-profile marker (`appliance`\|`hosted`) + the first-boot seed reader, and the hosted URL-shape helpers (`HostedAppHost`/`HostedAppURL`/`HostedDashboardHost`/`CertSubjects` — the single place `..malmo.network` is named). Leaf package. | `api`, `lifecycle`, `cmd/brain` | | `hostclient` | Brain-side client for `host-agent`. Mirrors the routes in `protocol`. | `lifecycle`, `api`, `auth`, `cmd/brain` | | `protocol` | Wire types shared with `cmd/host-agent`. Source of truth for the host protocol. | `hostclient`, `cmd/host-agent` | diff --git a/docs/dev/hosted-boot-proof.md b/docs/dev/hosted-boot-proof.md index f9eb52dc..1e535dde 100644 --- a/docs/dev/hosted-boot-proof.md +++ b/docs/dev/hosted-boot-proof.md @@ -25,7 +25,7 @@ Net: a provisioned box logs both milestones, binds `:443`, and serves every ` env var > compiled default; `from` logs `seed` where it logged `credential`. host-agent reads the seed **itself**, not through the brain's `profile.ReadSeed`, which hard-errors on a missing `box_id` — a box seeded only for updates, and a box with no seed at all, must both work. **Three seed states, decided on purpose:** absent falls through and is not an error (the appliance path), a readable seed is used when the field is set, and a **malformed** one is refused with the loop not starting, because bytes we cannot parse might have pinned this box and reading `stable` instead is the one outcome pinning exists to prevent. **The credential path is deleted, not kept alongside** — two mechanisms for one setting, one of which silently does nothing in production, is worse than either. The **window does not move to the seed and will not**: it has to be changeable while a box runs and user-data cannot be rewritten, so it waits for the control plane's answer. The cloud lane's `update` boot now carries the URL in its **seed** rather than an `Environment=` drop-in, and asserts `from=seed` before the apply — without that, the apply passes on a box that read its target from anywhere at all, which is exactly how the credential version stayed green. **Gaps:** the boot-proof edit is unexercised locally (needs root + KVM, CI-only); nothing cloud-side sets the field yet, so this ships inert; an empty field is treated as absent, not as a refusal | done | | [update-target-per-box-and-window.md](update-target-per-box-and-window.md) — **Closes #408.** A hosted box now says **which box is asking**: `GET ?box_id=`, with the id read off the same seed pass as the target URL (#407) rather than a second file read. That is what makes a per-box answer possible at all — `UPDATES.md` # 8.1 specified a target per `box_id` from the start, and until now the answer was the same for every box. **A box with no identity sends no parameter at all**, not an empty one: an appliance box and an unseeded hosted box ask byte-for-byte what they asked before. The answer also gains an optional `window` field, so **when** to update joins **what** to run as a fact the control plane owns and can change while a box runs — the home the seed could never be, because user-data is write-once. Precedence is **answer > `MALMO_UPDATE_WINDOW` > built-in default**, and an answer with **no** window field means "no opinion", never "use the default", or a silent field would outrank an operator's variable. An unusable window **warns and falls back to the box's own setting** (not to the default), keeping #404's asymmetry: a wrong hour only applies at the wrong time, a wrong target sends the box to the wrong version. The window is resolved per tick, deduped with memory separate from the target lines, and logged with `from=answer` — a fourth value for that field. The `update` boot now inverts its window setup — `MALMO_UPDATE_WINDOW=04:00-04:01` against a whole-day window in the answer — so the apply can only pass if the answer won, and asserts the `box_id` and `from=answer` before it. **The identity is deliberately weak and written down, not fixed:** a bare `box_id` on an unauthenticated endpoint can be read and claimed by anyone who knows it, accepted only because the ask is a **read**, with the trade-off in `UPDATES.md` # 8.1 and the real credential a new `NEXT.md` item. Nothing that mutates state may be built on it. **Gaps:** the boot-proof edit is unexercised locally (CI-only lane); nothing control-plane-side reads the parameter or sends a window yet, so it ships inert; the box id is not validated | done | | [container-logs-journald-driver.md](container-logs-journald-driver.md) — The dashboard's per-app **Logs tab was empty on every real box**, hosted and appliance, for every app, since the tail shipped: it opened the stream and sat on "Waiting for log output…" forever. Not a code bug — a missing image file. `LOGGING.md` calls the Docker log-driver switch "the single biggest configuration decision" and host-agent-real is built straight on it (`internal/hostagent/journalsource` runs `journalctl CONTAINER_NAME= -f`, and **`CONTAINER_NAME` is a field only the journald driver sets**), but **no image ever wrote `/etc/docker/daemon.json`**, so both real profiles ran Docker's `json-file` default, the match returned nothing, and `journalctl -f` blocked on a stream that could never produce a line. **Two blind spots hid it, and both are the same mistake:** the inner loop's fake host-agent reads `docker logs -f` (`cmd/host-agent/dockerlogsource.go`) and the cloud boot proof greps the brain through `docker logs` — both work on *any* driver, so nothing ever exercised the query the feature depends on; `cloud-assertions.sh` even documents the daemon as being on json-file. Ships `daemon.json` on the hosted image (committed, so the boot-proof image inherits it via `Include=..`) and byte-identically into the appliance lane's generated tree, with `CANARY_VERSION` bumped so the medium lane rebuilds. Rate limiting follows the spec rather than the obvious fix: journald counts against `_SYSTEMD_UNIT`, so routing every container through dockerd puts them all in **one** bucket where a single chatty container silently starves the rest — disabled **per-unit** on `docker.service` (`LOGGING.md` # Tuning), not raised globally, so sshd brute-force spam still caps. New boot-proof assertion 5b asserts the driver **and** that `journalctl CONTAINER_NAME=malmo-brain` returns lines, deliberately not through `docker logs`, since reading it that way is exactly why this shipped. **Gaps:** the journal is still **volatile** — neither image sets `Storage=persistent` or creates `/var/log/journal`, so scrollback dies on reboot and `SystemMaxUse`, which # Tuning assumes is the backpressure once docker's rate limit is off, is unset; the appliance change is unbooted (medium lane is local-only, needs root + KVM); the tab still follows `main_service` only, so sidecar output stays unreachable | done | +| [path-scoped-app-exposure.md](path-scoped-app-exposure.md) — **Closes #415.** Hosted exposure was whole-app and binary, which has no answer for an app that pairs a **token-authed API** with a **session-authed UI**: letting an external SDK reach the API meant making the app public, which dropped the box login in front of the UI too. Two catalog apps already hit that wall, and one of them (Laminar) signs in **any email with no password** on its self-hosted UI, so "go public to ingest" read as "anyone can sign in as anyone". A manifest may now declare `access.public_paths`, and a `restricted` app's route becomes **one** Caddy `subroute` — declared paths straight to the app, everything else gated exactly as before — so `upsertRoute`'s insert-at-0 and the catch-all order are untouched, and the proxy handler is built once and used on both branches so the #335 per-cookie strip cannot land on one and not the other. **The scrub grew past the issue's shape and that is the load-bearing part:** the identity headers are now deleted at the head of **every** hosted app route, not just where the gate runs, because the gate does not run on a public path — the app would otherwise get a brain-vouched `X-Malmo-User` on one path and a client-forged one on another with no way to tell them apart — and the same hole already existed for a fully public app, where an app that learned to trust the header while restricted kept trusting it the moment the owner flipped the toggle. Validation refuses `/`, `/*`, `/**` (a manifest must not void the owner's toggle), a bare-suffix wildcard (`/v1*` also matches `/v1admin`), and `%`/`?`/`#`/`\`/`//`/`..` (the proxy matches a cleaned decoded path while the app sees the original URI, so those mean two different things on the two sides) — but the spec now says plainly that **catalog review is the trust boundary**, not this validator. The dashboard names the open paths instead of claiming a bare "Only me", reading them from the **instance's** manifest copy so the label cannot drift from the route, and carrying them on **both** responses that report an app's exposure (the detail `GET` and the toggle's echo) rather than only the first — the pair is what the label is built from. The hosted lane's `access` boot carries the proof through real Caddy, including a bypass table (prefix footgun, traversal, encoded traversal, double slash, case variant) that must stay gated. **The first CI run paid for itself twice:** `//v1/` never reaches the gate — Caddy collapses the duplicate slash and 301's to the normalized path before matching, so the probe was safe and the expectation was wrong (the table now asserts the claim that matters: an undeclared path never reaches the app upstream anonymously) — and that failure came back **green**, because the lane matched its verdict with a `*PASS*` glob and the reason read "PATH GATE **BYPASS**". The harness now anchors on the exact verdict string; any failure text containing *bypass*/*password* would have silently greened any boot in this lane, so that one was latent well beyond this PR. **Gaps:** no catalog manifest declares the field yet, so it ships inert; the `UPDATES.md` clause making a widened `public_paths` prompt the owner is specified, not built; anonymous public-path traffic is invisible to Activity and has no rate limit in front of it | done | diff --git a/docs/progress/path-scoped-app-exposure.md b/docs/progress/path-scoped-app-exposure.md new file mode 100644 index 00000000..32d40953 --- /dev/null +++ b/docs/progress/path-scoped-app-exposure.md @@ -0,0 +1,57 @@ +# Path-scoped app exposure: a token-authed API stays open while the UI keeps the box login + +- **Status:** done - unit-tested locally; the real-Caddy proof is the hosted lane's `access` boot, which is CI-only (see Known gaps) +- **Date:** 2026-08-14 +- **Specs touched:** `docs/specs/APP_MANIFEST.md` (# E2, new), `docs/specs/ENVIRONMENT.md` (# Per-app owner-only access), `docs/specs/DASHBOARD.md` (# Settings, the Access control), `docs/specs/UPDATES.md` (# Auto-apply unless permissions expand), `docs/architecture.md` + +Closes #415. Follow-up to the closed epic #304: it builds on #306's central route builder and #305's verify endpoint, both of which stay as they were. + +## The problem + +Hosted exposure is whole-app and binary. An app is `restricted` (the box login in front of everything) or `public` (no login at all). That has no answer for the app that pairs a **token-authed API** with a **session-authed UI**, which is the normal shape for developer tooling: + +- **Langfuse** ships today with the limitation `external-sdk-needs-public-app`: an application outside the box cannot send traces while the app is owner-only, because the box login sits in front of the tracing API as well as the UI. +- **Laminar** hits the same wall and worse. Its self-hosted frontend signs in **any email with no password**, so "make it public so the SDK can ingest" reads as "let anyone on the internet sign in as anyone". Its whole `/v1/*` surface is already project-API-key authed and needs no session at all. + +## What was done + +**One optional manifest field** (`internal/manifest`), with the validation carrying most of the weight: + +```yaml +access: + public_paths: ["/v1", "/v1/*"] +``` + +Two shapes and nothing else: an exact path, or a path plus everything under it. `/`, `/*` and `/**` are refused — they would make the whole app public while the dashboard still offers the owner an access toggle for it, so a manifest would be overriding a decision that is not the author's. `/v1*` is refused too, because it also matches `/v1admin`: an author reaching for "everything under /v1" would open a sibling path they never read. `%`, `?`, `#`, `\`, `//` and `..` are refused because Caddy matches a cleaned, decoded path while the app sees the original URI, so a declaration containing any of them means two different things on the two sides of the proxy. At most 16 entries, 128 characters each, de-duplicated case-insensitively (Caddy's path matcher is case-insensitive, so `/v1/*` opens `/V1/x` whether the author meant it or not). + +**The validation is not the trust boundary, and the spec now says so.** These paths ship in the catalog, which malmo curates; a manifest that wanted to be hostile already chooses the app's images. The rules catch an author's mistake. Writing that down matters more than the rules, because the next person to read them would otherwise mistake them for a security control and relax them. + +**One route, one `@id`, two branches** (`internal/caddy`). When a gated route has public paths, the flat `[forwardAuth, proxy]` becomes a `subroute`: the declared paths proxy straight through, and a matcher-less second route gates everything else exactly as before. `upsertRoute`'s remove-then-`PUT`-at-index-0 and the catch-all's evaluation order are untouched, because it is still one route. The proxy handler is **built once and used on both branches**, so the #335 per-cookie strip cannot end up on one and not the other. + +**The identity scrub is now unconditional, and this is the part that was not in the issue's shape.** `X-Malmo-User` / `X-Malmo-User-Id` are deleted at the head of **every hosted app route**, before any gate runs. The gate keeps its own delete-before-set on the allow branch. The reason the outer scrub has to exist: + +- The gate does not run on a public path, so nothing there would overwrite a caller-supplied header. The app would receive a brain-vouched header on one path and a forged one on another, with no way to tell them apart. +- The same hole already existed for a **fully public app**, and this closes it in the same line of code. The old argument — "a public app never receives vouched headers at all" — stops holding the moment an app can be both: an app that learns to trust `X-Malmo-User` while restricted keeps trusting it the second the owner flips it to Public. + +**The route builder takes the manifest** (`internal/lifecycle`). `buildRouteConfig(inst, man, host, upstream)`; all four call sites already had `man` in scope. The gate still fires only on `hosted && restricted`, so the appliance route is byte-for-byte what it always was — no scrub, no strip, no gate — even for a manifest that declares public paths. + +**The dashboard says what is actually true.** `GET /api/v1/apps/{id}` and the exposure `PUT`'s echo both carry the declared paths — through one `withPublicPaths` helper, because these are the two responses that carry an app's exposure and the access label is built from the pair; a response with the exposure but not the paths says "Only me" about an app that is partly open. The Access control names those paths instead of claiming a bare "Only you can open it". The paths are read from the **instance's own manifest copy** — the same file the route was built from — not from the catalog, so the label cannot drift from the route when the catalog moves on. + +**The proof runs through real Caddy.** The hosted lane's `access` boot already installs an app and drives both exposure modes end to end; its test-catalog whoami now declares `public_paths`, and the boot asserts the parts a unit test cannot reach: the declared paths answer anonymously, a forged `X-Malmo-User` never reaches the app on either branch while the vouched one still does, `malmo_forward_auth` is stripped on the public branch too, and a **bypass table** never reaches the app anonymously — `/v1extra` (the prefix footgun), `/v1/../`, `//v1/`, `/v1/%2e%2e/`, `/V1x`, `/admin`. The requests are written raw onto the socket, so no client-side normalization softens them. The traversal result is the one worth knowing: `/v1/../` is **gated**, which is the box confirming that Caddy matches a cleaned path rather than the raw one. + +## How it maps to the specs + +`APP_MANIFEST.md` gains # E2 for the field and its rules. `ENVIRONMENT.md` # Per-app owner-only access gains the subroute shape and the unconditional scrub. `DASHBOARD.md` records that the label must name the paths. `UPDATES.md` gains one line in the permission-expansion diff: a new or widened `public_paths` entry prompts the instance owner, because it exposes more of the app to anonymous callers even though it sits outside the `permissions:` block. That line is **specified, not built** — the permission diff itself is unimplemented, and this entry does not implement it. + +## Known gaps & deviations + +- **The bypass table cost two corrections on its first CI run, and both are worth reading.** First: `//v1/` does not reach the gate at all — Caddy collapses the duplicate slash and answers `301` to the normalized path *before* matching. The app is never reached, so the probe was safe and the **expectation** was wrong; the table now asserts the claim that actually matters (an undeclared path never reaches the app upstream anonymously) and allows the merge-redirect shape for that one entry. Second, and worse: that failure was reported as a **green** CI run. The lane matched its verdict with a `*PASS*` glob, and the failure reason read "PATH GATE **BYPASS**" — so a genuinely red access boot printed `boot access OK`. The harness now anchors on the exact verdict string. Any assertion whose failure text contained *bypass*, *passphrase* or *password* would have done the same thing to any boot in this lane, so this was latent well beyond this PR. +- **Nothing in the catalog declares the field yet.** Langfuse's `status.yml` limitation and Laminar's screening note both still describe the wall as permanent; both are accurate until a catalog manifest actually declares its ingestion paths, which is a `malmoos/store` change, not this one. +- **The 16-entry cap is not a boundary.** An author with enough entries can still cover an app. The cap keeps the list reviewable; catalog review is what stops the case it cannot. +- **No audit trail on the public branch.** Anonymous requests bypass the brain entirely, so Activity never sees them. Inherent to the design, now stated rather than assumed. + +## What's next + +- Declare the field on Langfuse in `malmoos/store` and move its `external-sdk-needs-public-app` limitation off `by-design`; revisit Laminar's sign-in limitation when it is imported. +- Build the permission-expansion diff (`UPDATES.md`), which now has a `public_paths` clause waiting for it. +- Decide whether the box should rate-limit an anonymous public path. Today an owner-only app can carry an unauthenticated internet-facing surface with no budget in front of it, and the only thing between it and abuse is the app's own token check. diff --git a/docs/specs/APP_MANIFEST.md b/docs/specs/APP_MANIFEST.md index e77d4f1e..6196e3c5 100644 --- a/docs/specs/APP_MANIFEST.md +++ b/docs/specs/APP_MANIFEST.md @@ -390,6 +390,26 @@ Apps that don't support external libraries fall back to `storage.app_managed_use **No `cap_add` for store (Tier-3) apps.** The brain's override drops ALL capabilities and adds none. Apps that genuinely need Linux capabilities (VPN clients, FUSE mounts, raw sockets) belong in Tier 2 — OS integrations curated by malmo with a separate install path. See `SERVICE_PROVISIONING.md`. If a Tier-3 compose declares `cap_add`, the brain refuses to install it. +### E2. Access — path-scoped exceptions to the box login + +Hosted boxes put a **box login in front of each app** by default, and the owner flips a per-app toggle between "Only me" and "Public" (`ENVIRONMENT.md` # Per-app owner-only access). That is a whole-app decision, and some apps do not fit it: a developer tool often pairs a **token-authed API** with a **session-authed UI**, so letting an external SDK reach the API means making the whole app public, which drops the box login in front of the UI too. Langfuse hit this wall first; Laminar hit it worse, because its self-hosted UI signs in any email with no password. + +The manifest may therefore declare paths that stay open even while the app is owner-only: + +```yaml +access: + public_paths: ["/v1", "/v1/*"] # served anonymously; the rest keeps the box login +``` + +- **It is an exception, not a second exposure switch.** The app stays "Only me". Only the listed paths skip the gate, and only on hosted — an appliance app is public anyway, so the field changes nothing there. +- **Two shapes, nothing else:** an exact path (`/v1`) or a path and everything under it (`/v1/*`). `/`, `/*` and `/**` are rejected outright: they would make the whole app public while the dashboard still offers the owner an access toggle for it. A mid-path or bare-suffix wildcard is rejected too, because `/v1*` also matches `/v1admin` — an author reaching for "everything under /v1" would open a sibling path they never read. At most 16 entries, 128 characters each. +- **Declare the plain, decoded path.** `%`, `?`, `#`, `\`, `//` and `..` are rejected. The proxy matches a cleaned, decoded path while the app sees the original URI, so a declaration containing any of those means two different things on the two sides. +- **Matching is case-insensitive**, so `/v1/*` also opens `/V1/x`. Entries that differ only by case are duplicates. +- **`/v1/*` does not match `/v1` itself.** Declare both when the API answers on the bare prefix. +- **The author does not get identity from a public path.** malmo's vouched identity headers are stripped from every inbound request on every hosted app route, so a caller can never forge them — see `ENVIRONMENT.md` # Per-app owner-only access. +- **The trust boundary is catalog review**, not this validation. These paths ship in the catalog, which malmo curates; the rules above catch an author's mistake, and a manifest that wanted to be hostile already chooses the app's images. A reviewer should ask one question: is every declared path authenticated by the app itself? +- **The dashboard says so.** An app that declares public paths shows them on its access control instead of a bare "Only you can open it" (`DASHBOARD.md` # Settings). + ### F. Lifecycle hooks — deferred from MVP The `hooks:` block is **not part of v1.** Apps already run their own migrations on container start; the brain's **pre-update snapshot** (`UPDATES.md` # Pre-update snapshot) is the v1 safety net for migrations that go wrong. diff --git a/docs/specs/DASHBOARD.md b/docs/specs/DASHBOARD.md index 60dfa35b..0ccaec1c 100644 --- a/docs/specs/DASHBOARD.md +++ b/docs/specs/DASHBOARD.md @@ -202,7 +202,7 @@ A floating bottom dock with exactly four destinations: **Activity (audit log) and Users live *under Settings* as gated routes**, not as top-level dock items — administrative surface, not daily-use. Role gating per `AUTH.md`: **Users is admin-only.** **Activity is open to every signed-in user but scoped server-side** — a member sees only events where they are the actor or target, an admin sees the full box-wide feed (`LOGGING.md` # Visibility rules; the brain enforces the split, the UI renders whatever it returns). Admins additionally see the system/storage/network panels. (Activity's all-user visibility was settled by issue #11 and `LOGGING.md`; this supersedes an earlier "admin-surface" framing — see `DECISIONS.md` 2026-06-05.) -Settings is itself a **left-nav shell**: a sidebar of sections on the left (collapsing to a horizontal tab strip on narrow screens) and the active section's content filling the rest. Each section is its own nested route under `/settings`, so sections deep-link and the avatar-menu links land directly on them; `/settings` redirects to the Account section. The section set as built: **Account** (the signed-in user's identity + self-service password change), **Notifications** (per-category bell mutes), **Installed apps** (a list of installed instances; each row opens a per-app detail page at `/settings/apps/` — logo, name, description, the **Stop service** / **Start service** control, **Uninstall**, an **Access** toggle on hosted boxes (**Only me** / **Public** — the per-app access mode of `ENVIRONMENT.md` # Per-app owner-only access, #306/#307; hidden on the appliance, which has no public app subdomains, and switching re-writes the app's Caddy route via `PUT /api/v1/apps//exposure`), an **outgoing-email** picker for mail-capable apps, a **Setup secrets** reveal for apps that declare an owner-visible secret (`APP_MANIFEST.md` # D2 — the per-instance bootstrap credential, masked until revealed, with a best-effort copy that degrades to select-on-screen since `.local` is HTTP-only), a **Settings** editor for apps that declare `config:` (`APP_MANIFEST.md` # D4 — the same fields the install form collected; non-secret values shown and editable, secret values shown as "set" with a **replace** affordance rather than revealed, and saving rewrites the override and restarts the app), and the app's logs. Control authorization mirrors install/uninstall: admins for any app, the owner for their own personal app — the same gate guards the secret reveal and the config editor), **Activity**, **Users** (admin-only; the nav item is hidden from members and the section also redirects), and **About** (product identity; grows to show version/box-name once the brain exposes them). The admin **Storage / Network / System** panels are reserved sections, not yet built. The sidebar visuals are implementation-time UX; the section set and the role gating are the spec. +Settings is itself a **left-nav shell**: a sidebar of sections on the left (collapsing to a horizontal tab strip on narrow screens) and the active section's content filling the rest. Each section is its own nested route under `/settings`, so sections deep-link and the avatar-menu links land directly on them; `/settings` redirects to the Account section. The section set as built: **Account** (the signed-in user's identity + self-service password change), **Notifications** (per-category bell mutes), **Installed apps** (a list of installed instances; each row opens a per-app detail page at `/settings/apps/` — logo, name, description, the **Stop service** / **Start service** control, **Uninstall**, an **Access** toggle on hosted boxes (**Only me** / **Public** — the per-app access mode of `ENVIRONMENT.md` # Per-app owner-only access, #306/#307; hidden on the appliance, which has no public app subdomains, and switching re-writes the app's Caddy route via `PUT /api/v1/apps//exposure`). An app whose manifest declares `access.public_paths` (`APP_MANIFEST.md` # E2) must say so on that control instead of a bare "Only you can open it": the toggle still reads **Only me**, and the line under it names the paths that stay open to anyone, because a label that hides them would claim a narrower app than the box is serving. The paths are read from the instance's own manifest copy — the same file the route was built from — so the label cannot drift from the route (#415), an **outgoing-email** picker for mail-capable apps, a **Setup secrets** reveal for apps that declare an owner-visible secret (`APP_MANIFEST.md` # D2 — the per-instance bootstrap credential, masked until revealed, with a best-effort copy that degrades to select-on-screen since `.local` is HTTP-only), a **Settings** editor for apps that declare `config:` (`APP_MANIFEST.md` # D4 — the same fields the install form collected; non-secret values shown and editable, secret values shown as "set" with a **replace** affordance rather than revealed, and saving rewrites the override and restarts the app), and the app's logs. Control authorization mirrors install/uninstall: admins for any app, the owner for their own personal app — the same gate guards the secret reveal and the config editor), **Activity**, **Users** (admin-only; the nav item is hidden from members and the section also redirects), and **About** (product identity; grows to show version/box-name once the brain exposes them). The admin **Storage / Network / System** panels are reserved sections, not yet built. The sidebar visuals are implementation-time UX; the section set and the role gating are the spec. **Global cross-surface search** (one box that spans apps + files) is deferred. At v1 app counts the home grid is scannable; that search earns its place when a household's app + file corpus outgrows the eye. Reserved, not built. The **Store** is the exception, and a scoped one: as the catalog corpus grows it carries its own in-page browse filters — a page-wide search over name + short description, and a row of category pills (the union of the catalog's own `categories`, "All" first; `APP_STORE.md` # Catalog schema, "Browse UI groups by category"). These filter the browse grid in place and don't reach beyond the Store. diff --git a/docs/specs/ENVIRONMENT.md b/docs/specs/ENVIRONMENT.md index 310f548f..296d98f8 100644 --- a/docs/specs/ENVIRONMENT.md +++ b/docs/specs/ENVIRONMENT.md @@ -140,6 +140,10 @@ The "require a malmo session in front" half of that refinement is being built as The brain side of this — the verify endpoint the box Caddy calls (`GET /_malmo/forward-auth/verify`), and minting/clearing the forward-auth cookie on every hosted session-establishment path (login, the portal-to-box SSO exchange, logout) — is built (#305). Identity scope is **owner-only** in v1: only the box owner's session validates; box users the owner may later create are an additive step. All of it is behind the `hosted` profile gate — a `Domain=.malmo.network` cookie is meaningless on the appliance's `.local` / mesh names, so the appliance login path and its same-origin Tier-2 auth model are byte-for-byte unchanged. +**Path-scoped exposure (#415).** Whole-app exposure does not fit an app that pairs a **token-authed API** with a **session-authed UI**: making the API reachable by an external SDK meant making the app public, which dropped the box login in front of the UI too. A manifest may therefore declare `access.public_paths` (`APP_MANIFEST.md` # E2) — a short list of paths a `restricted` app still serves anonymously. The route becomes one Caddy `subroute` with two branches (still one route, one `@id`, so the insert-at-0 ordering against the catch-all is unchanged): the declared paths proxy straight to the app, everything else is gated exactly as before. Both branches share the same proxy handler, so the `malmo_forward_auth` strip applies identically. The list comes from the catalog, never from the user, and `/`, `/*` and `/**` are rejected so a manifest cannot void the owner's toggle; the dashboard names the declared paths rather than claiming a plain "Only me". + +**Identity headers are scrubbed on every hosted app route, in every exposure.** `X-Malmo-User` / `X-Malmo-User-Id` are deleted from each inbound request before any gate runs, so a caller can never supply them. The forward_auth gate also deletes-before-sets on its allow branch, and that stays — but the gate cannot cover what it does not run on: a public app, or a declared public path. Without the outer scrub an app would get a brain-vouched header on one path and a client-forged one on another, with no way to tell them apart, and an app that learned to trust the header while restricted would keep trusting it the moment the owner flipped it to Public. + The **box-side enforcement is now built too (#306)**: a per-instance exposure state (`restricted` owner-only / `public`) in the brain's SQLite, reconciled through **one central Caddy route builder** (the safety boundary) that, on hosted, strips `malmo_forward_auth` from the `Cookie` header of every app route (#335) and wraps a `restricted` app in the `forward_auth` gate at the verify endpoint (a 2xx injects the identity headers and proxies on; a 401 redirects to the box login). The `PUT /api/v1/apps/{id}/exposure` endpoint (owner-or-admin, hosted-only, audited) re-applies the route on a toggle. **The hosted default for new installs is flipped to `restricted`** (owner-only); the appliance is untouched — always `public`, never wrapped, byte-for-byte the same plain reverse_proxy. What remains of epic #304 is the dashboard Only-me / Public toggle (#307) and the end-to-end lane proving both modes plus the cookie-leak probe of the strip invariant (#308). ## Storage (hosted) diff --git a/docs/specs/UPDATES.md b/docs/specs/UPDATES.md index facc7c97..e1846fbe 100644 --- a/docs/specs/UPDATES.md +++ b/docs/specs/UPDATES.md @@ -161,8 +161,11 @@ The trigger for prompting is **permission expansion**, not version bumps. Concre - New permission key (e.g., `devices` newly present) → prompt. - Widened value (`internet: false → true`, new entry in `folders`, new entry in `devices`, `gpu: false → true`, mode upgrade `read → write` on an existing folder, etc.) → prompt. +- A new or widened `access.public_paths` entry (`APP_MANIFEST.md` # E2) → prompt. It exposes more of the app to anonymous callers, which is the same kind of trust event as a new folder grant, even though it sits outside the `permissions:` block. A narrower or unchanged list → no prompt. - Same or narrower permissions → auto-apply, no prompt. +The `public_paths` rule is **specified, not yet built** — the permission diff itself is unimplemented (no app-update transaction reads it yet), so this says what the diff must cover when it lands. + This means a Photos `1.4 → 2.0` bump that doesn't touch permissions auto-applies. A Photos `1.4 → 1.5` bump that adds `devices: [/dev/dri]` for hardware-accelerated thumbnails prompts. Reasoning: diff --git a/internal/api/api.go b/internal/api/api.go index bf742697..0d3ada3b 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -346,6 +346,13 @@ type InstanceDTO struct { // rebind picker (SERVICE_PROVISIONING.md # BYO outgoing mail). MailSupported bool `json:"mail_supported,omitempty"` MailProviderID string `json:"mail_provider_id,omitempty"` + // PublicPaths are the request paths the app's manifest declares as anonymous + // even while Exposure is "restricted" (#415). Detail-page enrichment, like the + // mail fields. It is read from the instance's own manifest copy — the same + // file the route builder reads — so the dashboard label cannot claim a + // narrower app than Caddy is actually serving. Empty ⇒ the access toggle means + // exactly what it says. + PublicPaths []string `json:"public_paths,omitempty"` } func (s *Server) toDTO(i store.Instance, ownerUsername string, e *catalog.Entry) InstanceDTO { @@ -378,6 +385,22 @@ func (s *Server) toDTO(i store.Instance, ownerUsername string, e *catalog.Entry) return dto } +// withPublicPaths fills in the app's manifest-declared anonymous paths (#415). +// Every response that carries an app's exposure must carry these too: the +// dashboard's access label is built from the pair, and a response with the +// exposure but not the paths says "Only me" about an app that is partly open. +// +// The source is the INSTANCE's manifest copy, not the catalog's. That is the +// file the route builder read, so the label cannot claim a narrower app than +// Caddy is serving even after the catalog moves on. A missing or unreadable copy +// leaves the field empty rather than failing the request — the app's own detail +// page must still render. +func (s *Server) withPublicPaths(dto *InstanceDTO) { + if man, err := s.life.InstanceManifest(dto.ID); err == nil { + dto.PublicPaths = man.Access.PublicPaths + } +} + // --- handlers ------------------------------------------------------------ func (s *Server) listCatalog(ctx context.Context, _ *struct{}) (*struct { @@ -566,6 +589,7 @@ func (s *Server) getApp(ctx context.Context, in *struct { catEntry = &e } dto := s.toDTO(i, owner.Username, catEntry) + s.withPublicPaths(&dto) // Mail enrichment for the rebind picker. The manifest comes from the // catalog, so a withdrawn app simply hides the picker (the binding itself // keeps working — lifecycle reads the instance dir's own manifest copy). diff --git a/internal/api/appexposure.go b/internal/api/appexposure.go index c915403c..cc5e410b 100644 --- a/internal/api/appexposure.go +++ b/internal/api/appexposure.go @@ -68,5 +68,10 @@ func (s *Server) setAppExposure(ctx context.Context, in *struct { if e, err := s.catalog.Entry(inst.ManifestID); err == nil { catEntry = &e } - return &struct{ Body InstanceDTO }{Body: s.toDTO(inst, owner.Username, catEntry)}, nil + dto := s.toDTO(inst, owner.Username, catEntry) + // The access label is exposure + declared public paths together, so a toggle + // response that carried only the exposure would let the dashboard render + // "Only me" for an app with open paths until the next refetch. + s.withPublicPaths(&dto) + return &struct{ Body InstanceDTO }{Body: dto}, nil } diff --git a/internal/api/appexposure_test.go b/internal/api/appexposure_test.go index 89e54cde..bdcfe84a 100644 --- a/internal/api/appexposure_test.go +++ b/internal/api/appexposure_test.go @@ -136,3 +136,77 @@ func TestSetAppExposure_FailureAudits(t *testing.T) { t.Error("expected an app.exposure.set failure audit") } } + +// The detail response carries the manifest's declared public paths, so the +// dashboard can say that part of an "Only me" app is open (#415). It reads the +// INSTANCE's manifest copy — the same file the route builder reads — so the +// label cannot claim a narrower app than Caddy is serving. +func TestGetApp_CarriesDeclaredPublicPaths(t *testing.T) { + s, id, instDir := hostedExposureServer(t, "u_owner", store.ScopePersonal, "running") + man := ` +id: cfgapp +manifest_version: 1 +name: Cfg App +version: "1.0" +compose_file: compose.yml +main_service: app +main_port: 8080 +access: + public_paths: ["/v1/*"] +` + if err := os.WriteFile(filepath.Join(instDir, "manifest.yml"), []byte(man), 0o644); err != nil { + t.Fatalf("write manifest: %v", err) + } + out, err := s.getApp(adminCtx("u_admin"), &struct { + ID string `path:"id"` + }{ID: id}) + if err != nil { + t.Fatalf("get app: %v", err) + } + if got := out.Body.PublicPaths; len(got) != 1 || got[0] != "/v1/*" { + t.Fatalf("public_paths = %v, want the manifest's declaration", got) + } +} + +// An app that declares nothing carries nothing, so the toggle keeps meaning +// exactly what it says for almost every app. +func TestGetApp_NoPublicPathsWhenUndeclared(t *testing.T) { + s, id, _ := hostedExposureServer(t, "u_owner", store.ScopePersonal, "running") + out, err := s.getApp(adminCtx("u_admin"), &struct { + ID string `path:"id"` + }{ID: id}) + if err != nil { + t.Fatalf("get app: %v", err) + } + if len(out.Body.PublicPaths) != 0 { + t.Fatalf("public_paths = %v, want none", out.Body.PublicPaths) + } +} + +// The exposure toggle echoes the app so the dashboard can render without a +// refetch. That echo must carry the declared public paths too: exposure alone +// would render "Only me" for an app that is partly open. +func TestSetAppExposure_EchoCarriesPublicPaths(t *testing.T) { + s, id, instDir := hostedExposureServer(t, "u_owner", store.ScopePersonal, "stopped") + man := ` +id: cfgapp +manifest_version: 1 +name: Cfg App +version: "1.0" +compose_file: compose.yml +main_service: app +main_port: 8080 +access: + public_paths: ["/v1/*"] +` + if err := os.WriteFile(filepath.Join(instDir, "manifest.yml"), []byte(man), 0o644); err != nil { + t.Fatalf("write manifest: %v", err) + } + out, err := putExposure(t, s, adminCtx("u_admin"), id, store.ExposureRestricted) + if err != nil { + t.Fatalf("set exposure: %v", err) + } + if got := out.Body.PublicPaths; len(got) != 1 || got[0] != "/v1/*" { + t.Fatalf("exposure echo public_paths = %v, want the manifest's declaration", got) + } +} diff --git a/internal/caddy/caddy.go b/internal/caddy/caddy.go index 16191a04..e93ea5ca 100644 --- a/internal/caddy/caddy.go +++ b/internal/caddy/caddy.go @@ -63,6 +63,23 @@ type RouteConfig struct { // subrequest to the brain verify endpoint (hosted restricted apps). nil is a // public app: the plain reverse_proxy with no gate. ForwardAuth *ForwardAuthConfig + // PublicPaths, when non-empty alongside a non-nil ForwardAuth, carves those + // request paths out of the gate: they proxy straight to the app with no box + // login, while every other path stays gated (#415). It is how an app that + // pairs a token-authed API with a session-authed UI keeps the box login in + // front of the UI. Ignored when ForwardAuth is nil — a public app has no gate + // to carve out of. + PublicPaths []string + // ScrubHeaders lists request headers deleted from EVERY request this route + // proxies, before any gate runs. It carries malmo's vouched-identity headers + // so a caller can never supply them: the app must be able to read + // X-Malmo-User as "the brain said so", on every path and in every exposure. + // + // The forward_auth gate does its own delete-before-set of the same headers on + // the allow branch, and that stays. This is the outer guarantee the gate + // cannot give, because the gate does not run on a public app or on a public + // path. + ScrubHeaders []string } // ForwardAuthConfig parameterises the forward_auth gate placed in front of a @@ -77,6 +94,19 @@ type ForwardAuthConfig struct { // AddRoute registers Host(cfg.Host) -> the app's reverse_proxy(cfg.Upstream), // optionally wrapped in a forward_auth gate and/or a Cookie strip per cfg. The // upstream is the real container alias, added once the app is healthy. +// +// The emitted handler chain, in order: +// +// 1. the identity-header scrub (cfg.ScrubHeaders), first so it covers every +// later branch, gated or not; +// 2. either the flat [forwardAuth?, proxy], or — when cfg carves public paths +// out of a gate — a subroute whose first route proxies those paths straight +// through and whose second route gates everything else. +// +// The proxy handler is built once and used on both branches of the subroute, so +// the Cookie strip (#335) applies identically to gated and public paths. A route +// with no scrub, no gate and no public paths is byte-for-byte the plain +// reverse_proxy the appliance has always emitted. func (c *Client) AddRoute(ctx context.Context, cfg RouteConfig) error { proxy := map[string]any{ "handler": "reverse_proxy", @@ -87,14 +117,52 @@ func (c *Client) AddRoute(ctx context.Context, cfg RouteConfig) error { "request": map[string]any{"replace": stripCookieReplacements(cfg.StripCookieName)}, } } - handle := make([]any, 0, 2) - if cfg.ForwardAuth != nil { - handle = append(handle, forwardAuthHandler(*cfg.ForwardAuth)) + handle := make([]any, 0, 3) + if len(cfg.ScrubHeaders) > 0 { + handle = append(handle, scrubHeadersHandler(cfg.ScrubHeaders)) + } + switch { + case cfg.ForwardAuth != nil && len(cfg.PublicPaths) > 0: + handle = append(handle, map[string]any{ + "handler": "subroute", + "routes": []any{ + // Declared public paths: no gate, straight to the app. Its token + // auth is the app's own business. + map[string]any{ + "match": []any{map[string]any{"path": cfg.PublicPaths}}, + "handle": []any{proxy}, + }, + // No matcher ⇒ every other path, gated exactly as a fully + // restricted app is. + map[string]any{ + "handle": []any{forwardAuthHandler(*cfg.ForwardAuth), proxy}, + }, + }, + }) + case cfg.ForwardAuth != nil: + handle = append(handle, forwardAuthHandler(*cfg.ForwardAuth), proxy) + default: + handle = append(handle, proxy) } - handle = append(handle, proxy) return c.upsertRoute(ctx, cfg.InstanceID, cfg.Host, handle) } +// scrubHeadersHandler renders a Caddy headers handler that deletes the named +// request headers. Placed at the head of an app route it is the one guarantee +// forward_auth cannot give on its own: forward_auth scrubs only on the branch +// where it runs, and it does not run on a public app or on a public path, so +// without this a caller could hand an app a forged X-Malmo-User there. +func scrubHeadersHandler(names []string) map[string]any { + del := make([]any, 0, len(names)) + for _, n := range names { + del = append(del, n) + } + return map[string]any{ + "handler": "headers", + "request": map[string]any{"delete": del}, + } +} + // stripCookieReplacements renders the Caddy headers.request.replace body that // removes exactly one named cookie from the inbound Cookie header, leaving every // other cookie byte-for-byte intact. Two ordered passes, both load-bearing: diff --git a/internal/caddy/caddy_test.go b/internal/caddy/caddy_test.go index 2f6be9d4..8c399129 100644 --- a/internal/caddy/caddy_test.go +++ b/internal/caddy/caddy_test.go @@ -351,6 +351,129 @@ func TestAddRoute_ForwardAuthGate(t *testing.T) { assertCookieStripped(t, app) } +// Path-scoped exposure (#415): a restricted app that declares public paths must +// emit ONE route whose subroute proxies those paths straight through and gates +// everything else. One route and one @id, so the insert-at-0 ordering against +// the catch-all is unchanged. +func TestAddRoute_PublicPathsCarveOutOfTheGate(t *testing.T) { + handle := routeHandle(t, RouteConfig{ + InstanceID: "i1", Host: "h", Upstream: "app:80", + StripCookieName: "malmo_forward_auth", + PublicPaths: []string{"/v1", "/v1/*"}, + ScrubHeaders: []string{"X-Malmo-User", "X-Malmo-User-Id"}, + ForwardAuth: &ForwardAuthConfig{ + Upstream: "malmo-brain:8080", VerifyPath: "/_malmo/forward-auth/verify", + CopyHeaders: []string{"X-Malmo-User", "X-Malmo-User-Id"}, + LoginURL: "https://cindy-fox.malmo.network/", + }, + }) + if len(handle) != 2 { + t.Fatalf("want [scrub, subroute], got %d handlers", len(handle)) + } + sub := handle[1].(map[string]any) + if sub["handler"] != "subroute" { + t.Fatalf("second handler = %v, want subroute", sub["handler"]) + } + routes := sub["routes"].([]any) + if len(routes) != 2 { + t.Fatalf("want 2 subroutes (public paths, then the gated rest), got %d", len(routes)) + } + + // Subroute 0: the declared paths, verbatim, straight to the app with no gate. + pub := routes[0].(map[string]any) + match := pub["match"].([]any)[0].(map[string]any)["path"].([]any) + if len(match) != 2 || match[0] != "/v1" || match[1] != "/v1/*" { + t.Errorf("public matcher = %v, want the declared paths verbatim", match) + } + pubHandle := pub["handle"].([]any) + if len(pubHandle) != 1 { + t.Fatalf("public branch = %d handlers, want the app proxy alone (no gate)", len(pubHandle)) + } + pubProxy := pubHandle[0].(map[string]any) + if pubProxy["handler"] != "reverse_proxy" { + t.Errorf("public branch handler = %v, want reverse_proxy", pubProxy["handler"]) + } + // The #335 invariant holds on the public branch too: it is the same proxy. + assertCookieStripped(t, pubProxy) + + // Subroute 1: no matcher ⇒ everything else, gated exactly as before. + rest := routes[1].(map[string]any) + if _, ok := rest["match"]; ok { + t.Error("the gated branch must have no matcher (catch-all for every other path)") + } + restHandle := rest["handle"].([]any) + if len(restHandle) != 2 { + t.Fatalf("gated branch = %d handlers, want [forward_auth, proxy]", len(restHandle)) + } + if rw := restHandle[0].(map[string]any)["rewrite"].(map[string]any); rw["uri"] != "/_malmo/forward-auth/verify" { + t.Errorf("gated branch is not the forward_auth gate: %v", restHandle[0]) + } + assertCookieStripped(t, restHandle[1].(map[string]any)) +} + +// The identity scrub is the outer guarantee forward_auth cannot give: it must be +// the FIRST handler, so it covers the public branch (where no gate runs), the +// gated branch, and the verify subrequest itself. A caller-forged X-Malmo-User +// must never reach an app on any path (#415). +func TestAddRoute_ScrubsIdentityHeadersFirst(t *testing.T) { + for _, tc := range []struct { + name string + cfg RouteConfig + }{ + {"public app, no gate", RouteConfig{ + InstanceID: "i1", Host: "h", Upstream: "app:80", + StripCookieName: "malmo_forward_auth", + ScrubHeaders: []string{"X-Malmo-User", "X-Malmo-User-Id"}, + }}, + {"restricted app", RouteConfig{ + InstanceID: "i1", Host: "h", Upstream: "app:80", + StripCookieName: "malmo_forward_auth", + ScrubHeaders: []string{"X-Malmo-User", "X-Malmo-User-Id"}, + ForwardAuth: &ForwardAuthConfig{Upstream: "b:8080", VerifyPath: "/v", LoginURL: "https://l/"}, + }}, + {"restricted app with public paths", RouteConfig{ + InstanceID: "i1", Host: "h", Upstream: "app:80", + StripCookieName: "malmo_forward_auth", + ScrubHeaders: []string{"X-Malmo-User", "X-Malmo-User-Id"}, + PublicPaths: []string{"/v1/*"}, + ForwardAuth: &ForwardAuthConfig{Upstream: "b:8080", VerifyPath: "/v", LoginURL: "https://l/"}, + }}, + } { + t.Run(tc.name, func(t *testing.T) { + handle := routeHandle(t, tc.cfg) + first := handle[0].(map[string]any) + if first["handler"] != "headers" { + t.Fatalf("first handler = %v, want the identity scrub", first["handler"]) + } + del := first["request"].(map[string]any)["delete"].([]any) + for _, want := range []string{"X-Malmo-User", "X-Malmo-User-Id"} { + found := false + for _, d := range del { + if d == want { + found = true + } + } + if !found { + t.Errorf("%s is not scrubbed, so a caller can forge it", want) + } + } + }) + } +} + +// PublicPaths without a gate is meaningless — a public app has nothing to carve +// out of — and must not turn the route into a subroute. +func TestAddRoute_PublicPathsIgnoredWithoutGate(t *testing.T) { + handle := routeHandle(t, RouteConfig{ + InstanceID: "i1", Host: "h", Upstream: "app:80", + StripCookieName: "malmo_forward_auth", + PublicPaths: []string{"/v1/*"}, + }) + if len(handle) != 1 || handle[0].(map[string]any)["handler"] != "reverse_proxy" { + t.Fatalf("public app route must stay the plain proxy, got %v", handle) + } +} + // applyEmittedCookieStrip runs the route's own emitted Cookie replacements over // a real Cookie header, exactly as Caddy does: each search_regexp is compiled // with Go's regexp and applied with ReplaceAllString, in the emitted order. diff --git a/internal/lifecycle/exposure_test.go b/internal/lifecycle/exposure_test.go index 85ea343f..a32411be 100644 --- a/internal/lifecycle/exposure_test.go +++ b/internal/lifecycle/exposure_test.go @@ -185,3 +185,117 @@ func TestReconcile_Hosted_ReassertKeepsGate(t *testing.T) { t.Fatalf("a restart must not drop the gate or strip: %+v", cfg) } } + +// --- path-scoped exposure (#415) ------------------------------------------ + +// installWhoamiWithPublicPaths installs the same test app with an +// `access.public_paths` block, so the route builder has a manifest that declares +// one. +func installWhoamiWithPublicPaths(t *testing.T, e *testEnv, paths string) store.Instance { + t.Helper() + man := whoamiManifest(testDigest) + "access:\n public_paths: " + paths + "\n" + e.writeCatalogApp(t, "whoami", whoamiCompose, man) + e.docker.digests[testImage] = testDigest + inst, err := e.m.Install(context.Background(), "whoami", Owner{UserID: "u_admin", Username: "admin"}, store.ScopeHousehold, nil, "", nil, nil) + if err != nil { + t.Fatalf("install: %v", err) + } + return inst +} + +// A restricted hosted app carries its manifest's public paths into the route, so +// the token-authed API answers anonymously while the UI keeps the box login. +func TestInstall_Hosted_RestrictedCarriesPublicPaths(t *testing.T) { + e := newTestEnv(t) + e.m.SetEnvironment(profile.Hosted, "cindy-fox") + inst := installWhoamiWithPublicPaths(t, e, `["/v1/*"]`) + + cfg := e.caddy.config(inst.ID) + if cfg.ForwardAuth == nil { + t.Fatal("the app must still be gated — public paths are an exception, not an off switch") + } + if len(cfg.PublicPaths) != 1 || cfg.PublicPaths[0] != "/v1/*" { + t.Fatalf("PublicPaths = %v, want the manifest's declaration", cfg.PublicPaths) + } +} + +// Every hosted app route scrubs the vouched identity headers, in every exposure. +// The gate scrubs only where it runs, and it does not run on a public app or on +// a public path — so without this a caller could forge X-Malmo-User there. +func TestHosted_AllExposuresScrubIdentityHeaders(t *testing.T) { + e := newTestEnv(t) + e.m.SetEnvironment(profile.Hosted, "cindy-fox") + inst := installWhoamiWithPublicPaths(t, e, `["/v1/*"]`) + + assertScrubbed := func(what string) { + t.Helper() + cfg := e.caddy.config(inst.ID) + if len(cfg.ScrubHeaders) == 0 { + t.Fatalf("%s: route scrubs no identity headers", what) + } + for _, want := range []string{"X-Malmo-User", "X-Malmo-User-Id"} { + found := false + for _, h := range cfg.ScrubHeaders { + if h == want { + found = true + } + } + if !found { + t.Errorf("%s: %s is not scrubbed, so a caller can forge it", what, want) + } + } + } + assertScrubbed("restricted with public paths") + + if err := e.m.SetExposure(context.Background(), inst.ID, store.ExposurePublic); err != nil { + t.Fatalf("set public: %v", err) + } + assertScrubbed("public") +} + +// A public app has no gate, so its manifest's public paths are meaningless and +// must not reach the route: nothing to carve out of. +func TestSetExposure_PublicDropsPublicPaths(t *testing.T) { + e := newTestEnv(t) + e.m.SetEnvironment(profile.Hosted, "cindy-fox") + inst := installWhoamiWithPublicPaths(t, e, `["/v1/*"]`) + if err := e.m.SetExposure(context.Background(), inst.ID, store.ExposurePublic); err != nil { + t.Fatalf("set public: %v", err) + } + cfg := e.caddy.config(inst.ID) + if cfg.ForwardAuth != nil { + t.Fatal("a public app must not be gated") + } + if len(cfg.PublicPaths) != 0 { + t.Fatalf("PublicPaths = %v, want none on a public app", cfg.PublicPaths) + } +} + +// The appliance route stays exactly what it was: no scrub, no strip, no gate, no +// public paths — even for a manifest that declares them. +func TestInstall_Appliance_IgnoresPublicPaths(t *testing.T) { + e := newTestEnv(t) + inst := installWhoamiWithPublicPaths(t, e, `["/v1/*"]`) + cfg := e.caddy.config(inst.ID) + if cfg.StripCookieName != "" || cfg.ForwardAuth != nil || len(cfg.PublicPaths) != 0 || len(cfg.ScrubHeaders) != 0 { + t.Fatalf("appliance route must stay the plain reverse_proxy, got %+v", cfg) + } +} + +// A restart rebuilds the route from the instance's own manifest copy, so the +// carve-out survives a reboot instead of quietly closing (or quietly widening). +func TestReconcile_Hosted_ReassertKeepsPublicPaths(t *testing.T) { + e := newTestEnv(t) + e.m.SetEnvironment(profile.Hosted, "cindy-fox") + inst := installWhoamiWithPublicPaths(t, e, `["/v1/*"]`) + + e.docker.psManaged = map[string]bool{} + e.caddy.calls = nil + if err := e.m.Reconcile(context.Background()); err != nil { + t.Fatalf("reconcile: %v", err) + } + cfg := e.caddy.config(inst.ID) + if cfg.ForwardAuth == nil || len(cfg.PublicPaths) != 1 || cfg.PublicPaths[0] != "/v1/*" { + t.Fatalf("a restart must rebuild the same carve-out, got %+v", cfg) + } +} diff --git a/internal/lifecycle/lifecycle.go b/internal/lifecycle/lifecycle.go index 9a711730..e63322aa 100644 --- a/internal/lifecycle/lifecycle.go +++ b/internal/lifecycle/lifecycle.go @@ -262,23 +262,40 @@ func (m *Manager) defaultExposure() string { // // Only that one cookie is stripped. The app's own cookies must reach it, or a // third-party app with a cookie login cannot authenticate at all (#335). -func (m *Manager) buildRouteConfig(inst store.Instance, host, upstream string) caddy.RouteConfig { +// +// Two things follow from the manifest rather than the instance row (#415): +// the identity headers are scrubbed from every hosted app route, and a +// restricted app's declared `access.public_paths` are carved out of its gate. +func (m *Manager) buildRouteConfig(inst store.Instance, man *manifest.Manifest, host, upstream string) caddy.RouteConfig { cfg := caddy.RouteConfig{InstanceID: inst.ID, Host: host, Upstream: upstream} if !m.hosted() { return cfg } cfg.StripCookieName = auth.ForwardAuthCookieName + // Scrubbed on EVERY hosted route, not only where the gate runs. A restricted + // app that learns to trust X-Malmo-User keeps trusting it after the owner + // flips it to Public, or on one of its own public paths — and there the gate + // is not there to overwrite a forged header (#415). + cfg.ScrubHeaders = identityHeaders if inst.Exposure == store.ExposureRestricted { cfg.ForwardAuth = &caddy.ForwardAuthConfig{ Upstream: m.brainUpstream, VerifyPath: profile.ForwardAuthVerifyPath, - CopyHeaders: []string{"X-Malmo-User", "X-Malmo-User-Id"}, + CopyHeaders: identityHeaders, LoginURL: "https://" + profile.HostedDashboardHost(m.boxID) + "/", } + if man != nil { + cfg.PublicPaths = man.Access.PublicPaths + } } return cfg } +// identityHeaders are the request headers the brain vouches for on an allowed +// forward-auth request, and the same set scrubbed from every inbound request so +// a caller can never forge them (ENVIRONMENT.md # Per-app owner-only access). +var identityHeaders = []string{"X-Malmo-User", "X-Malmo-User-Id"} + // lockInstance acquires the per-instance lock (creating it on first use) and // returns the unlock func. Callers `defer unlock()`. See instLocks. func (m *Manager) lockInstance(id string) func() { @@ -825,7 +842,7 @@ func (m *Manager) install(ctx context.Context, man *manifest.Manifest, composeBy // 12. Flip the Caddy upstream from splash to the real container. step("flipping_route") upstream := fmt.Sprintf("malmo-%s-%s:%d", id, man.MainService, man.MainPort) - if err := m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, host, upstream)); err != nil { + if err := m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, man, host, upstream)); err != nil { slog.Warn("caddy upstream flip failed (continuing)", "instance_id", id, "host", host, "upstream", upstream, "err", err) } @@ -1060,7 +1077,7 @@ func (m *Manager) Start(ctx context.Context, id string) error { // Healthy — flip the splash to the real container. upstream := fmt.Sprintf("malmo-%s-%s:%d", id, man.MainService, man.MainPort) - if err := m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, host, upstream)); err != nil { + if err := m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, man, host, upstream)); err != nil { slog.Warn("start: caddy upstream flip failed (continuing)", "instance_id", id, "host", host, "upstream", upstream, "err", err) } @@ -1099,7 +1116,7 @@ func (m *Manager) SetExposure(ctx context.Context, instanceID, exposure string) } host, _ := m.publishHost(ctx, inst) upstream := fmt.Sprintf("malmo-%s-%s:%d", inst.ID, man.MainService, man.MainPort) - return m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, host, upstream)) + return m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, man, host, upstream)) } // startFailed parks a start that came up but never went healthy in the same @@ -1449,7 +1466,7 @@ func (m *Manager) reassertRouting(ctx context.Context, inst store.Instance) bool } host, avahiOK := m.publishHost(ctx, inst) upstream := fmt.Sprintf("malmo-%s-%s:%d", inst.ID, man.MainService, man.MainPort) - if err := m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, host, upstream)); err != nil { + if err := m.caddy.AddRoute(ctx, m.buildRouteConfig(inst, man, host, upstream)); err != nil { slog.Warn("reconcile: caddy route", "instance_id", inst.ID, "host", host, "upstream", upstream, "err", err) } diff --git a/internal/manifest/manifest.go b/internal/manifest/manifest.go index 5b4d318f..e31cda47 100644 --- a/internal/manifest/manifest.go +++ b/internal/manifest/manifest.go @@ -137,6 +137,30 @@ type Manifest struct { // this schema. Nor is either one a limitation: a limitation is a broken // feature, and paying for something is not a defect. ExternalCosts []ExternalCost `yaml:"external_costs,omitempty"` + + // Access declares path-scoped exceptions to the box's own login gate + // (APP_MANIFEST.md # E2, ENVIRONMENT.md # Per-app owner-only access). Absent ⇒ + // the whole app follows the instance's exposure, which is the normal case. + Access Access `yaml:"access,omitempty"` +} + +// Access is the app's declared access shape. Today it holds one field, and it is +// a block rather than a bare list so later access-related declarations have a +// home that does not re-open the top level. +type Access struct { + // PublicPaths lists request paths an owner-only ("restricted") app still + // serves to anonymous callers. It exists for the app that pairs a + // token-authed API with a session-authed UI: without it, letting an external + // SDK reach the API means making the whole app public, which drops the box + // login in front of the UI too (#415). + // + // It is a narrow exception, not a second exposure switch. The paths are + // carried by the catalog, so catalog review is the trust boundary; the + // validation below is a guard against an author mistake, not against a + // hostile manifest (a hostile manifest already chooses the app's images). + // The gate is hosted-only: an appliance app is always public, so the field + // changes nothing there. + PublicPaths []string `yaml:"public_paths,omitempty" json:"public_paths,omitempty"` } // ExternalCost is one third-party charge the app depends on (APP_MANIFEST.md # @@ -558,6 +582,16 @@ var secretName = regexp.MustCompile(`^[a-z][a-z0-9_]*$`) // `API-KEY` rejected. var configEnvName = regexp.MustCompile(`^[A-Z_][A-Z0-9_]*$`) +// MaxPublicPaths caps how many path-scoped exceptions one app may declare +// (APP_MANIFEST.md # E2). The cap is not a security boundary — an author with +// enough entries can still cover an app — it keeps the list reviewable, and a +// manifest that needs more than this is really asking to be a public app. +const MaxPublicPaths = 16 + +// maxPublicPathLen caps one declared path. Long enough for any real API prefix, +// short enough that a path can be read at a glance in review. +const maxPublicPathLen = 128 + // reservedConfigEnv is the loader/runtime denylist a config `app_env` may never // take (APP_MANIFEST.md # D4 # Reserved names): names that reach process or // platform internals. The MALMO_ prefix is rejected separately (the brain's @@ -648,9 +682,80 @@ func (m *Manifest) validate() error { if err := m.validateMail(); err != nil { return err } + if err := m.validateAccess(); err != nil { + return err + } return ValidatePermissions(&m.Permissions) } +// validateAccess checks the declared public paths (APP_MANIFEST.md # E2). Two +// shapes are allowed and nothing else: +// +// /v1 an exact path +// /v1/* that path and everything under it +// +// Everything else is rejected, and each rule pays for itself: +// +// - A path must start with "/" — a relative pattern matches nothing in Caddy, +// so it would silently leave the API gated. +// - "/" and "/*" are rejected outright. Either one exposes the whole app while +// the dashboard still offers an access toggle for it, so the manifest would +// be overriding a decision that belongs to the box owner. +// - The wildcard may only appear as a trailing "/*". The obvious-looking +// "/v1*" also matches "/v1admin", so an author reaching for "everything +// under /v1" would open a sibling path they never read. +// - "%", "?", "#", "\", "//" and ".." are rejected. Caddy matches a cleaned, +// decoded path while the app sees the original URI, so a declaration +// containing any of these means two different things on the two sides of the +// proxy. Authors declare the plain, decoded prefix and let Caddy normalize. +// +// Matching in Caddy is case-insensitive, so "/v1/*" also opens "/V1/x"; entries +// are therefore de-duplicated case-insensitively and the spec says so. Absent ⇒ +// no-op. +func (m *Manifest) validateAccess() error { + paths := m.Access.PublicPaths + if len(paths) == 0 { + return nil + } + if len(paths) > MaxPublicPaths { + return fmt.Errorf("access: %d public_paths declared, at most %d allowed", len(paths), MaxPublicPaths) + } + seen := make(map[string]bool, len(paths)) + for _, p := range paths { + switch { + case p == "" || !strings.HasPrefix(p, "/"): + return fmt.Errorf("access: public_paths entry %q must start with %q", p, "/") + case p == "/" || p == "/*" || p == "/**": + return fmt.Errorf("access: public_paths entry %q would make the whole app public; use the app's access toggle instead", p) + case len(p) > maxPublicPathLen: + return fmt.Errorf("access: public_paths entry %q is longer than %d characters", p, maxPublicPathLen) + } + for _, bad := range []string{"%", "?", "#", `\`, "//", ".."} { + if strings.Contains(p, bad) { + return fmt.Errorf("access: public_paths entry %q may not contain %q (declare the plain decoded path)", p, bad) + } + } + if strings.ContainsAny(p, " \t") { + return fmt.Errorf("access: public_paths entry %q may not contain whitespace", p) + } + // The only wildcard allowed is a trailing "/*". Trimming it must leave a + // star-free literal path, which rejects "/v1*", "/*/v1" and "/v1/*/x". + literal := strings.TrimSuffix(p, "/*") + if strings.Contains(literal, "*") { + return fmt.Errorf("access: public_paths entry %q may only use a trailing %q wildcard (%q matches %q too)", p, "/*", "/v1*", "/v1admin") + } + if literal == "" { + return fmt.Errorf("access: public_paths entry %q would make the whole app public; use the app's access toggle instead", p) + } + key := strings.ToLower(p) + if seen[key] { + return fmt.Errorf("access: duplicate public_paths entry %q (matching is case-insensitive)", p) + } + seen[key] = true + } + return nil +} + // validateSecrets checks declared secret names and normalizes the byte length // in place (APP_MANIFEST.md # secrets). Names must be snake_case and unique; a // requested length below MinSecretBytes is raised to it, and an omitted length diff --git a/internal/manifest/manifest_test.go b/internal/manifest/manifest_test.go index 8a9c53af..1bef0d06 100644 --- a/internal/manifest/manifest_test.go +++ b/internal/manifest/manifest_test.go @@ -1,6 +1,7 @@ package manifest import ( + "fmt" "reflect" "strings" "testing" @@ -1116,3 +1117,107 @@ external_costs: t.Fatalf("parse: %v (a %d-rune estimate is exactly at the cap)", err, EstimateMaxChars) } } + +// --- access.public_paths (#415) ------------------------------------------- + +func accessManifest(paths string) []byte { + return []byte(` +id: langfuse +manifest_version: 1 +name: Langfuse +version: "3.0" +compose_file: compose.yml +main_service: web +main_port: 3000 +access: + public_paths: ` + paths + ` +`) +} + +func TestParseAccessPublicPaths(t *testing.T) { + m, err := Parse(accessManifest(`["/v1", "/api/public/*"]`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + if want := []string{"/v1", "/api/public/*"}; !reflect.DeepEqual(m.Access.PublicPaths, want) { + t.Fatalf("public_paths = %v, want %v", m.Access.PublicPaths, want) + } +} + +func TestParseAccessAbsentIsEmpty(t *testing.T) { + m, err := Parse([]byte(` +id: whoami +manifest_version: 1 +name: Whoami +version: "1.10" +compose_file: compose.yml +main_service: whoami +main_port: 80 +`)) + if err != nil { + t.Fatalf("parse: %v", err) + } + if len(m.Access.PublicPaths) != 0 { + t.Fatalf("public_paths = %v, want empty for a manifest with no access block", m.Access.PublicPaths) + } +} + +// The validation is what stops an author from writing a path that opens more of +// the app than they meant to, and what stops a manifest from silently voiding +// the box owner's access toggle. Each case names the mistake it catches. +func TestParseRejectsBadPublicPaths(t *testing.T) { + cases := map[string]string{ + "whole app via slash": `["/"]`, + "whole app via star": `["/*"]`, + "whole app via double star": `["/**"]`, + "whole app via bare star": `["*"]`, + "relative path matches none": `["v1/*"]`, + "empty entry": `[""]`, + // "/v1*" also matches "/v1admin" — the classic prefix footgun. + "wildcard not on a segment": `["/v1*"]`, + "wildcard mid path": `["/v1/*/traces"]`, + "leading wildcard": `["/*/v1"]`, + // Caddy matches a decoded, cleaned path; the app sees the original URI. + // A declaration containing any of these means two different things on the + // two sides of the proxy. + "percent encoding": `["/v1/%2e%2e/*"]`, + "traversal": `["/v1/../admin/*"]`, + "double slash": `["//v1/*"]`, + "query string": `["/v1?public=1"]`, + "fragment": `["/v1#x"]`, + "backslash": `["/v1\\admin/*"]`, + "whitespace": `["/v1 /*"]`, + // Matching is case-insensitive, so these two are the same rule. + "case-insensitive duplicate": `["/v1/*", "/V1/*"]`, + "exact duplicate": `["/v1/*", "/v1/*"]`, + } + for label, paths := range cases { + t.Run(label, func(t *testing.T) { + if _, err := Parse(accessManifest(paths)); err == nil { + t.Errorf("Parse accepted %s (%s), want an error", paths, label) + } + }) + } +} + +func TestParseRejectsTooManyPublicPaths(t *testing.T) { + var b strings.Builder + b.WriteString("[") + for i := 0; i <= MaxPublicPaths; i++ { + if i > 0 { + b.WriteString(", ") + } + fmt.Fprintf(&b, `"/p%d/*"`, i) + } + b.WriteString("]") + if _, err := Parse(accessManifest(b.String())); err == nil { + t.Fatalf("Parse accepted %d public paths, want at most %d", MaxPublicPaths+1, MaxPublicPaths) + } +} + +func TestParseRejectsOverlongPublicPath(t *testing.T) { + long := "/" + strings.Repeat("a", maxPublicPathLen) + "/*" + if _, err := Parse(accessManifest(`["` + long + `"]`)); err == nil { + t.Fatalf("Parse accepted a %d-character public path, want an error", len(long)) + } +} diff --git a/web-ui/src/generated/openapi.ts b/web-ui/src/generated/openapi.ts index 1c8eec86..d73de422 100644 --- a/web-ui/src/generated/openapi.ts +++ b/web-ui/src/generated/openapi.ts @@ -1314,6 +1314,7 @@ export interface components { name: string; owner_user_id: string; owner_username: string; + public_paths?: string[] | null; scope: string; slug: string; state: string; diff --git a/web-ui/src/views/settings/InstalledAppDetailSection.vue b/web-ui/src/views/settings/InstalledAppDetailSection.vue index 4a360a93..18478453 100644 --- a/web-ui/src/views/settings/InstalledAppDetailSection.vue +++ b/web-ui/src/views/settings/InstalledAppDetailSection.vue @@ -115,6 +115,18 @@ const setExposure = useMutation({ mutationFn: (next: Exposure) => api.put(`/apps/${id.value}/exposure`, { exposure: next }), onSettled: invalidate, }); +// Paths the app's manifest keeps open to anyone even while the app is "Only me" +// (#415 — an app whose API is token-authed and whose UI is session-authed). The +// label must say so: "Only me" would otherwise claim a narrower app than the box +// is really serving. Empty for almost every app. +const publicPaths = computed(() => app.value?.public_paths ?? []); +const accessSummary = computed(() => { + if (exposure.value === "public") return "Anyone with the link can open it."; + if (publicPaths.value.length > 0) { + return `Only you can open the app — visitors sign in to your box first. These paths stay open to anyone, so tools outside your box can reach them: ${publicPaths.value.join(", ")}`; + } + return "Only you can open it — visitors sign in to your box first."; +}); // ── Outgoing email (SERVICE_PROVISIONING.md # BYO outgoing mail) ───────────── // Shown only for mail-capable apps (mail_supported comes from GET /apps/{id}). @@ -363,11 +375,7 @@ const saveConfig = useMutation({
Who can open this app
-
- {{ exposure === "public" - ? "Anyone with the link can open it." - : "Only you can open it — visitors sign in to your box first." }} -
+
{{ accessSummary }}