Adoption: a new config file inherits its siblings' mode, not 0640

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 <noreply@anthropic.com>
This commit is contained in:
librelad 2026-08-29 05:07:12 +01:00
parent a361e38562
commit 6a62c94cf8
3 changed files with 52 additions and 9 deletions

View File

@ -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 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 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 is allowed to read each one. Adoption preserves the destination's existing
ownership and mode, defaults closed only for a file that did not exist, and ownership and mode and never re-permissions a directory it merely passes
never re-permissions a directory it merely passes through. 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 ## 3.12 — The WebUI branch

View File

@ -148,11 +148,16 @@ if [[ -x "$H" ]]; then
else else
echo " SKIP no webui_logins to test against" echo " SKIP no webui_logins to test against"
fi 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" 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 "$H" config-adopt "$stage" "general/lp_test_new_file" >/dev/null 2>&1
if [[ -f "$CONFIGS/general/lp_test_new_file" ]]; then 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" rm -f "$CONFIGS/general/lp_test_new_file"
else else
echo " FAIL a new file was not created"; fail=1 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 else echo " FAIL configs/$d is $m — the container user cannot traverse it"; fail=1; fi
done 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" echo "the domain reader"
# Config values carry a trailing comment column, and updateConfigOption writes # Config values carry a trailing comment column, and updateConfigOption writes
# an empty value as a literal "". Both had to be stripped: without the first # an empty value as a literal "". Both had to be stripped: without the first

View File

@ -581,10 +581,20 @@ config_adopt() {
chown "$had_owner" -- "$real_dst" || return 1 chown "$had_owner" -- "$real_dst" || return 1
chmod "$had_mode" -- "$real_dst" || return 1 chmod "$had_mode" -- "$real_dst" || return 1
else else
# New file: these can carry repository passwords and login hashes, so # A file this install did not have. Match a sibling in the same
# the default is closed. # directory rather than picking a mode: the config tree has its own
chown "$MANAGER:$MANAGER" -- "$real_dst" || return 1 # convention, and a file that deviates from it is the bug that keeps
chmod 0640 -- "$real_dst" || return 1 # 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 fi
return 0 return 0
} }