From 6a62c94cf8dad62728c7b940a1683156b5269d74 Mon Sep 17 00:00:00 2001 From: librelad Date: Sat, 29 Aug 2026 05:07:12 +0100 Subject: [PATCH] Adoption: a new config file inherits its siblings' mode, not 0640 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third occurrence of the same defect, in a third directory. config-adopt gave a file the destination did not already have 0640 — "these can hold secrets, so default closed" — but the config tree is 0755 and the backup account reads all of it. One 0640 file makes restic report permission denied, write an INCOMPLETE snapshot and exit 3, so the entire system-config backup is reported failed, and a first-run restore has nothing to restore from. A new file now inherits from a sibling in the same directory, matching whatever that tree's convention is rather than having a mode picked for it. Since this has now happened three times for three unrelated reasons, the test asserts the class rather than the instances: no file anywhere under configs/ may be unreadable by the backup account. Mutation-checked — chmod 0640 on any one config makes it fail. Also repaired the files the earlier buggy adoption runs clamped on this install, and fixed lp-storage-step-test's hardcoded step index, which the new Start step had shifted. Co-Authored-By: Claude Opus 5 --- docs/roadmap/first-run-restore.md | 19 ++++++++++++++++--- scripts/dev/lp-restore-adopt-test | 24 ++++++++++++++++++++++-- scripts/system/libreportal-ownership | 18 ++++++++++++++---- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/docs/roadmap/first-run-restore.md b/docs/roadmap/first-run-restore.md index fc700ef..fd7465b 100644 --- a/docs/roadmap/first-run-restore.md +++ b/docs/roadmap/first-run-restore.md @@ -461,9 +461,22 @@ to read. It also clamped every parent directory it passed through, closing The fix is a principle rather than a special case: **a restore replaces the content of a config file and nothing else.** The live install already knows who -is allowed to read each one. Adoption now preserves the destination's existing -ownership and mode, defaults closed only for a file that did not exist, and -never re-permissions a directory it merely passes through. +is allowed to read each one. Adoption preserves the destination's existing +ownership and mode and never re-permissions a directory it merely passes +through. + +For a file this install did not have, the first attempt picked `0640` — "these +can hold secrets, so default closed". That is the storage-location bug again, +one directory over: the config tree is `0755`, the backup account reads all of +it, and a single `0640` file makes restic write an INCOMPLETE snapshot and +report the whole run as failed. A new file now inherits from a sibling in the +same directory, so it matches whatever the tree's convention is rather than +having a mode chosen for it. + +That is three occurrences of the same defect in three directories for three +different reasons, so the test now asserts the *class*: **no file anywhere +under `configs/` may be unreadable by the backup account.** That one line would +have caught all three. ## 3.12 — The WebUI branch diff --git a/scripts/dev/lp-restore-adopt-test b/scripts/dev/lp-restore-adopt-test index 5cdcbd1..5fd6eba 100755 --- a/scripts/dev/lp-restore-adopt-test +++ b/scripts/dev/lp-restore-adopt-test @@ -148,11 +148,16 @@ if [[ -x "$H" ]]; then else echo " SKIP no webui_logins to test against" fi - # A file that does not exist yet gets the closed default. + # A file this install did not have inherits from its siblings rather than + # getting a mode of its own. It was 0640, which is unreadable by the backup + # account — the same defect that made storage location configs break every + # system snapshot, reintroduced one directory over. mkdir -p "$stage/general"; echo "n=1" > "$stage/general/lp_test_new_file" "$H" config-adopt "$stage" "general/lp_test_new_file" >/dev/null 2>&1 if [[ -f "$CONFIGS/general/lp_test_new_file" ]]; then - chk "a new file defaults closed" "$(stat -c '%a' "$CONFIGS/general/lp_test_new_file")" "640" + sib=$(find "$CONFIGS/general" -maxdepth 1 -type f ! -name lp_test_new_file -print -quit) + chk "a new file matches its siblings" \ + "$(stat -c '%a' "$CONFIGS/general/lp_test_new_file")" "$(stat -c '%a' "$sib")" rm -f "$CONFIGS/general/lp_test_new_file" else echo " FAIL a new file was not created"; fail=1 @@ -172,6 +177,21 @@ for d in general network security webui backup; do else echo " FAIL configs/$d is $m — the container user cannot traverse it"; fail=1; fi done +echo "every adopted config stays readable by the backup" +# The assertion that would have caught all of this at once. A config the backup +# account cannot read makes restic report "permission denied", write an +# INCOMPLETE snapshot and exit 3 — so the whole system backup is reported as +# failed, and a first-run restore has nothing to restore from. It has happened +# three times now, in three different directories, each time for a different +# reason. +BACKUP_USER=$(stat -c '%G' "$CONFIGS/backup/locations" 2>/dev/null || echo dockerinstall) +unreadable=0 +while IFS= read -r c; do + sudo -u "$BACKUP_USER" head -c 1 "$c" >/dev/null 2>&1 || { + echo " FAIL $BACKUP_USER cannot read $c"; unreadable=$((unreadable+1)); } +done < <(find "$CONFIGS" -type f -name '*' ! -path '*/.*' 2>/dev/null) +chk "no config is unreadable by $BACKUP_USER" "$unreadable" "0" + echo "the domain reader" # Config values carry a trailing comment column, and updateConfigOption writes # an empty value as a literal "". Both had to be stripped: without the first diff --git a/scripts/system/libreportal-ownership b/scripts/system/libreportal-ownership index 462fd6e..5729b25 100644 --- a/scripts/system/libreportal-ownership +++ b/scripts/system/libreportal-ownership @@ -581,10 +581,20 @@ config_adopt() { chown "$had_owner" -- "$real_dst" || return 1 chmod "$had_mode" -- "$real_dst" || return 1 else - # New file: these can carry repository passwords and login hashes, so - # the default is closed. - chown "$MANAGER:$MANAGER" -- "$real_dst" || return 1 - chmod 0640 -- "$real_dst" || return 1 + # A file this install did not have. Match a sibling in the same + # directory rather than picking a mode: the config tree has its own + # convention, and a file that deviates from it is the bug that keeps + # recurring here — 0640 in a tree the backup reads means restic writes + # an INCOMPLETE snapshot and calls the whole run a failure, which is + # how storage location configs broke every system backup. + local sibling sib_owner="" sib_mode="" + sibling=$(find "${real_dst%/*}" -maxdepth 1 -type f ! -name "${real_dst##*/}" -print -quit 2>/dev/null) + if [[ -n "$sibling" ]]; then + sib_owner="$(stat -c '%U:%G' -- "$sibling" 2>/dev/null)" + sib_mode="$(stat -c '%a' -- "$sibling" 2>/dev/null)" + fi + chown "${sib_owner:-$MANAGER:$MANAGER}" -- "$real_dst" || return 1 + chmod "${sib_mode:-0644}" -- "$real_dst" || return 1 fi return 0 }