docs(roadmap): tighten the storage-locations design
Three refinements that make the plan smaller rather than bigger, plus an honest accounting of what stays awkward. The .libreportal-storage marker now does three jobs instead of one: admission, mount detection and migrate provenance. Marker-lives-on-the- drive replaces the findmnt/REQUIRE_MOUNT/fs_uuid guard family with a single file read, and it covers cases those miss — a LUKS volume that was never unlocked, a dropped network mount. It also unblocks adopting a drive that already holds app data, which strict emptiness forbade; that relaxation is free, because planting a marker requires write access you'd have to already have. appDir becomes the one place availability is checked, so an absent drive fails centrally instead of needing a guard at 200 callers. And the sweep verifies itself: retire containers_dir to a sentinel path so survivors fail loudly on something that cannot exist. Pinning infra apps becomes CFG_<APP>_STORAGE_PINNED, mirroring MULTI_INSTANCE, rather than a hardcoded list in the engine. New §9 records the four things that don't get a clean answer: the TOCTOU window and the parent-writability restriction that closes it, two registries that will look like one, a 90-file diff that only process makes safe, and large moves simply being slow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
472a63466b
commit
8ef04ef5ad
@ -18,7 +18,7 @@ Nextcloud's 4 TB of photos go on the spinning disk. Vaultwarden and the control
|
|||||||
- ❌ A general volume manager. We don't format, partition, mount, or write `/etc/fstab`. The drive must already be mounted; we validate and use it.
|
- ❌ A general volume manager. We don't format, partition, mount, or write `/etc/fstab`. The drive must already be mounted; we validate and use it.
|
||||||
- ❌ Striping/tiering/RAID-alikes. One app's data lives on exactly one location. No splitting an app across two.
|
- ❌ Striping/tiering/RAID-alikes. One app's data lives on exactly one location. No splitting an app across two.
|
||||||
- ❌ Per-*volume* placement inside an app (`./data` here, `./db` there). Location granularity is the app directory. Revisit only if a real need shows up.
|
- ❌ Per-*volume* placement inside an app (`./data` here, `./db` there). Location granularity is the app directory. Revisit only if a real need shows up.
|
||||||
- ❌ Making backup locations and storage locations the same registry. They overlap on disk but differ in trust, lifecycle and ownership (see §8.1).
|
- ❌ Making backup locations and storage locations the same registry. They overlap on disk but differ in trust, lifecycle and ownership (see §9, §11.2).
|
||||||
|
|
||||||
## 2. What already works in our favour
|
## 2. What already works in our favour
|
||||||
|
|
||||||
@ -47,14 +47,31 @@ The manager (`libreportal`) runs with a **scoped sudoers allowlist**: it may run
|
|||||||
- absolute, and `realpath -e` returns the input unchanged (no symlink component, no `..`)
|
- absolute, and `realpath -e` returns the input unchanged (no symlink component, no `..`)
|
||||||
- not in the protected set (`/ /etc /usr /bin /sbin /lib* /boot /proc /sys /dev /run /var /tmp /root /home`), and not inside any user's home unless the install was made with `--allow-home`
|
- not in the protected set (`/ /etc /usr /bin /sbin /lib* /boot /proc /sys /dev /run /var /tmp /root /home`), and not inside any user's home unless the install was made with `--allow-home`
|
||||||
- does not nest — in either direction — with the system/containers/backups roots or any already-registered location
|
- does not nest — in either direction — with the system/containers/backups roots or any already-registered location
|
||||||
- **is an empty directory** (tolerating only `lost+found`, or an existing `.libreportal-storage` marker whose install-id matches)
|
- **is an empty directory** (tolerating only `lost+found`) **or already carries a `.libreportal-storage` marker** — the adopt case, §3.1
|
||||||
- is not the mountpoint of a filesystem holding someone else's data — implied by the emptiness rule, which is the point
|
|
||||||
|
|
||||||
The emptiness rule is what makes this safe: **root only ever chowns a directory that contains nothing**, so acceptance can't hand away anything that already existed. Everything created underneath afterwards is ours by construction.
|
The emptiness rule is what makes this safe: **root only ever chowns a directory that contains nothing**, so acceptance can't hand away anything that already existed. Everything created underneath afterwards is ours by construction.
|
||||||
3. On acceptance the helper writes a root-owned `.libreportal-storage` marker (id + install id + created-at), `chown`s the root to the container owner, `chmod 0751`, and appends the record. `remove` refuses while any app dir still lives there.
|
3. On acceptance the helper writes a root-owned `.libreportal-storage` marker (location id + install id + created-at), `chown`s the root to the container owner, `chmod 0751`, and appends the record. `remove` refuses while any app dir still lives there.
|
||||||
|
|
||||||
Consequence worth stating plainly: **`storage add` is a genuine privilege boundary crossing, not a config edit.** The WebUI can drive it (through the task system → CLI → helper, same as every other mutating action), but the *helper*, not the WebUI, is the gate.
|
Consequence worth stating plainly: **`storage add` is a genuine privilege boundary crossing, not a config edit.** The WebUI can drive it (through the task system → CLI → helper, same as every other mutating action), but the *helper*, not the WebUI, is the gate.
|
||||||
|
|
||||||
|
### 3.1 — Why "empty **or** marked" is still safe (and why the marker earns its keep)
|
||||||
|
|
||||||
|
Requiring strict emptiness would break the most valuable case there is: plugging in a drive that **already holds LibrePortal app data** from another install and adopting it. So the rule relaxes to *empty or carrying our marker* — and that relaxation costs nothing, because:
|
||||||
|
|
||||||
|
> writing the marker into a directory requires already being able to write that directory.
|
||||||
|
|
||||||
|
The manager can only plant a marker somewhere it can already write, and chowning a directory it already controls grants it nothing. There is no path where the marker gets the manager access it didn't have. (The fixed-path helpers can't be tricked into writing one elsewhere — none of them takes a caller-supplied destination.)
|
||||||
|
|
||||||
|
That single file then does **three** jobs, which is the main reason to like it:
|
||||||
|
|
||||||
|
| Job | How |
|
||||||
|
|---|---|
|
||||||
|
| Admission | "empty or marked" — §3 |
|
||||||
|
| **Mount detection** | the marker lives *on the drive*. Not mounted ⇒ bare mountpoint ⇒ no marker ⇒ location unavailable. No `findmnt`, no `fs_uuid` bookkeeping, and it works identically for USB disks, network mounts, and LUKS volumes that haven't been unlocked (§8.1) |
|
||||||
|
| Provenance on migrate | it carries the install id and location id the snapshot's manifest names, so "is this the same `bigdisk` the app came from?" is a file read (§7) |
|
||||||
|
|
||||||
|
**Residual wrinkle, not solved by elegance:** validate-then-`chown` is a TOCTOU window, and bash is a poor language for race-free path handling. The practical closure is to additionally require that the **parent** directory is not manager-writable — true for `/mnt`, `/srv`, `/media`, false for a path inside the manager's home. That's a real restriction on where locations may live, not a free lunch, and it should be stated in the docs rather than hidden.
|
||||||
|
|
||||||
## 4. The resolution layer — one function, ~200 call sites
|
## 4. The resolution layer — one function, ~200 call sites
|
||||||
|
|
||||||
The mechanical bulk of the work. Three new primitives in `scripts/source/paths.sh` (and mirrored inline in `init.sh`, per the existing keep-in-sync note):
|
The mechanical bulk of the work. Three new primitives in `scripts/source/paths.sh` (and mirrored inline in `init.sh`, per the existing keep-in-sync note):
|
||||||
@ -66,6 +83,8 @@ pathIsContainerData <p> # is this path under ANY container root?
|
|||||||
```
|
```
|
||||||
|
|
||||||
- **`appDir`** builds a `slug → dir` map once per process by scanning each enabled root for `*/<slug>.config`, memoises it in an associative array, and falls back to the primary root for a slug that doesn't exist yet (fresh install). Discovery-first, config-second: if the map and `CFG_<APP>_STORAGE` disagree, **the directory on disk wins** and we warn. That's what makes the system self-healing after a hand-move or a half-finished migration.
|
- **`appDir`** builds a `slug → dir` map once per process by scanning each enabled root for `*/<slug>.config`, memoises it in an associative array, and falls back to the primary root for a slug that doesn't exist yet (fresh install). Discovery-first, config-second: if the map and `CFG_<APP>_STORAGE` disagree, **the directory on disk wins** and we warn. That's what makes the system self-healing after a hand-move or a half-finished migration.
|
||||||
|
|
||||||
|
It is also **the single place the availability check belongs**. Every one of the ~200 sites has to call it, so a location whose drive is missing fails *once*, centrally, instead of needing a guard sprinkled at each caller. `appDir` returns non-zero **and** prints an unusable sentinel path, so the many callers that won't check `$?` still fail loudly on a path that cannot exist, rather than writing into a bare mountpoint. See §8.1.
|
||||||
- **`pathIsContainerData`** replaces the `[[ "$p" == "$containers_dir"* ]]` idiom that decides manager-vs-container-user elevation. It appears in ~10 files (`create_folder.sh`, `create_touch.sh`, `copy_file.sh`, `copy_files.sh`, `copy_folder.sh`, `copy_folders.sh`, `move_file.sh`, `runCfgOp`, `tags_manager_update.sh`, `webui_atomic_write.sh`). **Every one is a silent-corruption bug if missed** — a file under an unrecognised root gets written as the manager, lands with the wrong owner, and the container fails to read it at a moment far removed from the cause.
|
- **`pathIsContainerData`** replaces the `[[ "$p" == "$containers_dir"* ]]` idiom that decides manager-vs-container-user elevation. It appears in ~10 files (`create_folder.sh`, `create_touch.sh`, `copy_file.sh`, `copy_files.sh`, `copy_folder.sh`, `copy_folders.sh`, `move_file.sh`, `runCfgOp`, `tags_manager_update.sh`, `webui_atomic_write.sh`). **Every one is a silent-corruption bug if missed** — a file under an unrecognised root gets written as the manager, lands with the wrong owner, and the container fails to read it at a moment far removed from the cause.
|
||||||
|
|
||||||
Scale of the sweep (measured, not estimated):
|
Scale of the sweep (measured, not estimated):
|
||||||
@ -80,6 +99,8 @@ Scale of the sweep (measured, not estimated):
|
|||||||
|
|
||||||
`sourceScanFiles app_configs` must also loop the roots. Keep the existing `-maxdepth 3` + prune rules exactly as they are — and remember `*.config` is a reserved extension anywhere under an app dir.
|
`sourceScanFiles app_configs` must also loop the roots. Keep the existing `-maxdepth 3` + prune rules exactly as they are — and remember `*.config` is a reserved extension anywhere under an app dir.
|
||||||
|
|
||||||
|
**Finding the sites we missed.** A 90-file mechanical diff is exactly where a silent regression hides, and review alone won't catch a `$containers_dir$app` that survived. Make the runtime find them instead: once the sweep lands, **stop defining `containers_dir` as a usable path** and point it at a sentinel (`/nonexistent-libreportal-unconverted/`). Legitimate primary-root users have moved to `primaryRoot`/`webuiDir` by then, so every survivor now fails immediately, loudly, and *harmlessly* — on a path that cannot exist — with a sentinel string that greps straight out of the logs. Silent wrongness becomes a stack trace. Then a `scripts/dev/` linter keeps the pattern from coming back, the way `lp-task-names` guards task titles.
|
||||||
|
|
||||||
## 5. Per-app configuration
|
## 5. Per-app configuration
|
||||||
|
|
||||||
One new key in every app template, defaulting to the primary location:
|
One new key in every app template, defaulting to the primary location:
|
||||||
@ -140,14 +161,20 @@ Also worth noting: an app's restic snapshot paths change when it moves. Snapshot
|
|||||||
|
|
||||||
**8.1 — An unplugged drive is the top data-integrity risk.** `crontab_boot_app_reconcile.sh` brings **every** installed app up at boot. If a location's drive isn't mounted yet — or at all — docker cheerfully creates the bind-mount directories on the bare mountpoint and the app boots **empty**. Plug the drive back in and there are now two divergent copies, with the good one hidden underneath the mount. Mitigation is mandatory, not optional:
|
**8.1 — An unplugged drive is the top data-integrity risk.** `crontab_boot_app_reconcile.sh` brings **every** installed app up at boot. If a location's drive isn't mounted yet — or at all — docker cheerfully creates the bind-mount directories on the bare mountpoint and the app boots **empty**. Plug the drive back in and there are now two divergent copies, with the good one hidden underneath the mount. Mitigation is mandatory, not optional:
|
||||||
|
|
||||||
- every location carries `REQUIRE_MOUNT` (default **true** for anything not on the root filesystem), reusing `backupLocationLocalGuard`'s `findmnt --target` check plus the recorded `dev`/`fs_uuid`
|
The mitigation is **one test, not a family of them**: `.libreportal-storage` lives on the drive, so *marker present* ⇒ mounted, and *marker absent where the registry says a location is* ⇒ not mounted, refuse. That subsumes `findmnt`, `REQUIRE_MOUNT` and `fs_uuid` comparison into a single file read, and it behaves correctly for cases those don't cover — a LUKS volume that hasn't been unlocked, an NFS/SMB mount that dropped, a mountpoint shadowed by a stale directory.
|
||||||
- **`dockerComposeUp` refuses** to start an app whose location fails the guard — this is the real gate, since it also covers the manual and task-triggered paths, not just boot
|
|
||||||
- boot reconcile skips those apps with a loud notice and surfaces a dashboard warning rather than failing silently
|
|
||||||
- add a `.libreportal-storage` marker check: marker missing where a location is registered ⇒ almost certainly an unmounted drive, refuse
|
|
||||||
|
|
||||||
**8.2 — Missing an elevation-test site** (§4) produces wrong-owner files that fail much later. Mitigate by making `containers_dir` itself the tripwire: once `appDir` lands, add a `scripts/dev/` linter that fails on any new `$containers_dir$app`-shaped concatenation, the same way `lp-task-names` guards task titles.
|
Where it's enforced:
|
||||||
|
|
||||||
**8.3 — Cross-app literal paths.** Traefik, Prometheus, Grafana, AdGuard, Gluetun, CrowdSec, Headscale are reached into by name from other apps' code. They're also small and infrastructural. **Pin them to the primary location** in phase 1 (`CFG_<APP>_STORAGE=default`, not user-changeable) and relax per-app later once the call sites go through `appDir`. The WebUI's own dir is pinned permanently.
|
- **`appDir` fails** on an unavailable location (§4) — the central gate, hit by every caller by construction
|
||||||
|
- **`dockerComposeUp` refuses** to start such an app — the gate that actually protects data, since it also covers manual and task-triggered starts, not just boot
|
||||||
|
- boot reconcile skips those apps with a loud notice and a dashboard warning rather than failing silently
|
||||||
|
- keep `fs_uuid` in the registry anyway, but as *diagnostics* ("this is a different disk than the one registered"), not as the liveness test
|
||||||
|
|
||||||
|
**8.2 — Missing an elevation-test site** (§4) produces wrong-owner files that fail much later. The sentinel-plus-linter approach in §4 is the mitigation: make the survivors fail loudly during the sweep, then make the pattern un-reintroducible.
|
||||||
|
|
||||||
|
**8.3 — Cross-app literal paths.** Traefik, Prometheus, Grafana, AdGuard, Gluetun, CrowdSec, Headscale are reached into by name from other apps' code. They're also small and infrastructural, so pinning them to the primary location costs a user nothing real.
|
||||||
|
|
||||||
|
Do it as a **declared property of the app, not a hardcoded list in the engine**: `CFG_<APP>_STORAGE_PINNED=true` in those templates, exactly mirroring how `CFG_<APP>_MULTI_INSTANCE` already gates instancing. The engine asks the config; the WebUI renders the field disabled with the reason; relaxing an app later is a one-line template change plus its call sites, with no core edit. The WebUI's own dir is pinned permanently and structurally (`webuiDir`), not by this flag.
|
||||||
|
|
||||||
**8.4 — Filesystem capability.** exFAT/NTFS/vfat can't hold POSIX ownership, so a rootless app dir on one is broken from the start — worse than for backups, where it's only a warning. For storage locations this must be a **refusal**, not a notice. Also check `noexec`/`nosuid` mount flags (some apps execute out of their data dir) and that the filesystem supports the sub-UID range rootless docker maps into.
|
**8.4 — Filesystem capability.** exFAT/NTFS/vfat can't hold POSIX ownership, so a rootless app dir on one is broken from the start — worse than for backups, where it's only a warning. For storage locations this must be a **refusal**, not a notice. Also check `noexec`/`nosuid` mount flags (some apps execute out of their data dir) and that the filesystem supports the sub-UID range rootless docker maps into.
|
||||||
|
|
||||||
@ -157,7 +184,16 @@ Also worth noting: an app's restic snapshot paths change when it moves. Snapshot
|
|||||||
|
|
||||||
**8.7 — Footprint version.** New root helper + changed baked helpers ⇒ **bump `footprint_version`** (currently 5), or existing installs update the manager-owned code and keep root helpers that don't know about the registry.
|
**8.7 — Footprint version.** New root helper + changed baked helpers ⇒ **bump `footprint_version`** (currently 5), or existing installs update the manager-owned code and keep root helpers that don't know about the registry.
|
||||||
|
|
||||||
## 9. Phasing
|
## 9. What doesn't get a clean answer
|
||||||
|
|
||||||
|
Most of §8 collapses into a small number of good primitives — the marker file, `appDir`, the emptiness rule, a declared pin flag. Four things don't, and pretending otherwise would set us up to be surprised later.
|
||||||
|
|
||||||
|
- **The TOCTOU window in `libreportal-storage add`.** Validate-then-`chown` can't be made atomic in bash. The closure is a *restriction* — the parent directory must not be manager-writable — which rules out locations inside the manager's home and has to be documented as a rule users will occasionally hit and dislike.
|
||||||
|
- **Two registries that look like one.** Storage locations and backup locations will sit next to each other in the UI, both pointing at drives, with different lifecycles and different trust. No amount of engineering fixes that; it's a naming and information-architecture problem (§11.1) and the honest options are a genuinely different word or a single combined "Disks" page that owns both roles.
|
||||||
|
- **The sweep is big no matter how good the target API is.** ~90 files. Elegance makes the *result* better and the pattern un-reintroducible; it does not make the diff safe. Only the sentinel, the linter, and a full install → backup → restore → migrate soak on a real box do that — which is why phase 0 ships alone.
|
||||||
|
- **Moving a large app is just slow.** Cross-filesystem `cp -a` of a multi-terabyte Nextcloud is hours, needs root for sub-UID data, and the app is down throughout. There is no clever version. The deliverable is honesty about it: size estimate and duration warning up front, real progress in the task row, resumable on interruption, and never a delete of the source until the copy verifies.
|
||||||
|
|
||||||
|
## 10. Phasing
|
||||||
|
|
||||||
Each phase is independently shippable and independently verifiable. Phase 0 carries almost all of the regression risk and none of the user-visible value — land and soak it alone.
|
Each phase is independently shippable and independently verifiable. Phase 0 carries almost all of the regression risk and none of the user-visible value — land and soak it alone.
|
||||||
|
|
||||||
@ -170,7 +206,7 @@ Each phase is independently shippable and independently verifiable. Phase 0 carr
|
|||||||
| **4** | Manifest `storage` block, staged restore + path rewrite (fixes §7's latent bug), the "unknown location" prompt in migrate. | Yes |
|
| **4** | Manifest `storage` block, staged restore + path rewrite (fixes §7's latent bug), the "unknown location" prompt in migrate. | Yes |
|
||||||
| **5** | WebUI **Storage** page mirroring *Backup → Locations*: add/remove, mount state, free space, apps per location, drag-to-move. | Yes |
|
| **5** | WebUI **Storage** page mirroring *Backup → Locations*: add/remove, mount state, free space, apps per location, drag-to-move. | Yes |
|
||||||
|
|
||||||
## 10. Open questions
|
## 11. Open questions
|
||||||
|
|
||||||
1. **Naming.** "Storage location" vs "backup location" in the same UI — is that confusing enough to want a different word (*volume*? *disk*? *pool*?) for one of them?
|
1. **Naming.** "Storage location" vs "backup location" in the same UI — is that confusing enough to want a different word (*volume*? *disk*? *pool*?) for one of them?
|
||||||
2. **Can a storage location double as a backup location?** Same drive, different subdirectory, two registries. Recommend keeping them separate but having the picker offer known paths. Backing an app up onto the same drive its data lives on should at least warn.
|
2. **Can a storage location double as a backup location?** Same drive, different subdirectory, two registries. Recommend keeping them separate but having the picker offer known paths. Backing an app up onto the same drive its data lives on should at least warn.
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user