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 }