From 6fe87b822628eae1fcc8b72dd6880151ddc49156 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 16 Aug 2026 07:22:39 -0700 Subject: [PATCH 1/6] Delete a Conflicting Sudo Timestamp Drop-In on Confirm install-tools.sh --sudo-timestamp already detected another sudoers file setting the same timestamp option, but only ever warned. A host that already carried a hand-authored drop-in, or one left by a prior buggy run, ended up with two files asserting the same thing, forever. configure_sudo_timestamp now classifies each conflicting entry: - Scoped to this user, and the file sets nothing else: delete it on confirm, after the managed drop-in is proved in place. - Scoped to a different user, or unscoped: report only, never touched. - Scoped to this user but mixed with unrelated settings, or living in /etc/sudoers itself: die and ask for a manual visudo fix rather than guess which lines are safe to drop. A decline leaves the host unchanged, matching the existing confirm-gate pattern. The cleanup also runs when the managed file already has the right content, closing the case where a host that already ran the old script is left with both files despite nothing else needing to change. Verified with an isolated fixture harness (fake SUDOERS_FILE and search paths, stubbed visudo/install, no real root writes) covering: pure duplicate deleted, mixed-content file refused, steady-state no-op, correct-file-plus-stray-duplicate cleaned up, and a different user's entry left alone. Also dry-run tested against this host's real /etc/sudoers.d/pieter-global-timestamp. shellcheck (koalaman/shellcheck via Docker, per GOVERNANCE.md) is clean. --- host-setup/linux/install-tools.sh | 81 ++++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 18 deletions(-) diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index 57a940a..1c122fc 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -1096,6 +1096,15 @@ sudo_timestamp_report() { fi } +# Whether a drop-in holds nothing but this user's timestamp Defaults, aside from blank lines and comments. +# That purity is what makes deleting the whole file safe, since a mixed file would lose whatever else it sets, and picking lines back out of one is guessing, not reading. +sudo_timestamp_file_is_pure() { + local user="$1" file="$2" other + other=$("${SUDO[@]}" grep -vE "^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user}[[:space:]]+timestamp_(type|timeout)=" \ + "$file" 2> /dev/null) || other="" + [[ -z $other ]] +} + # Share one sudo credential cache across a user's terminals, rather than sudo's default of one per terminal. configure_sudo_timestamp() { local user staged @@ -1132,11 +1141,8 @@ configure_sudo_timestamp() { die "$switch_to does not parse this host's sudoers, so switching to it would lock every user out. This host is unchanged." fi - if "${SUDO[@]}" cmp -s "$staged" "$SUDOERS_FILE" 2> /dev/null; then - log "$SUDOERS_FILE already carries exactly this, leaving it alone" - sudo_timestamp_report "$user" - return 0 - fi + local own_current=false + "${SUDO[@]}" cmp -s "$staged" "$SUDOERS_FILE" 2> /dev/null && own_current=true # Another file setting either option is named rather than merged into, since which one wins is the order sudo reads them in and not something this can decide. local elsewhere @@ -1144,6 +1150,28 @@ configure_sudo_timestamp() { elsewhere=$("${SUDO[@]}" grep -rnsE '^[[:space:]]*Defaults.*timestamp_(type|timeout)' \ --exclude='*.*' --exclude='*~' --exclude="${SUDOERS_FILE##*/}" \ /etc/sudoers /etc/sudoers.d 2> /dev/null) || elsewhere="" + + # Only this user's own entry is ever a delete candidate; a different user's entry, or one with no user named at all, changes something beyond what this run was asked to change, so it is reported and left alone. + local -a delete_files=() unsafe_files=() + if [[ -n $elsewhere ]]; then + local candidate + while read -r candidate; do + [[ -n $candidate ]] || continue + # The main sudoers file is never auto-edited, and a drop-in that sets something else besides is never guessed at line by line, since either mistake risks removing a rule unrelated to this. + if [[ $candidate == "${SUDOERS_FILE%/*}"/* ]] && sudo_timestamp_file_is_pure "$user" "$candidate"; then + delete_files+=("$candidate") + else + unsafe_files+=("$candidate") + fi + done < <(grep -E "Defaults:${user}[[:space:]]+timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u) + fi + + if [[ $own_current == true && ${#delete_files[@]} -eq 0 ]]; then + log "$SUDOERS_FILE already carries exactly this, leaving it alone" + sudo_timestamp_report "$user" + return 0 + fi + if [[ -n $elsewhere ]]; then warn "A timestamp option is already set elsewhere, and the file sudo reads last wins:" local line @@ -1152,12 +1180,20 @@ configure_sudo_timestamp() { done <<< "$elsewhere" fi + if [[ ${#unsafe_files[@]} -gt 0 ]]; then + die "${unsafe_files[*]} sets $user's timestamp option but also carries something unrelated (or is the main sudoers file itself), so this will not guess which lines are safe to remove. Resolve it by hand with visudo, then run this again." + fi + + if [[ ${#delete_files[@]} -gt 0 ]]; then + info "Continuing deletes: ${delete_files[*]} (nothing in it but $user's timestamp Defaults), leaving $SUDOERS_FILE as the one place setting this" + fi + if [[ -n $switch_to ]]; then log "The sudo this host runs parses no timestamp_type, and $switch_to does" info "Switching the alternative changes which sudo implementation every user on this host runs" info "\"update-alternatives --auto sudo\" puts that back" fi - confirm "Change this host?" || die "Declined" + confirm "Change this host?" || die "Declined, leaving the host unchanged" if [[ -n $switch_to ]]; then run_root update-alternatives --set sudo "$switch_to" @@ -1165,17 +1201,26 @@ configure_sudo_timestamp() { info "The cached credential does not carry across the switch, so the next step may ask for a password" fi - # A drop-in whose name holds a dot is one sudo skips, so the content lands under such a name, is proved where it will be read, and only then is renamed over. - # Renaming is atomic, which a copy into place is not, and a half-written file in that directory locks every user out of sudo. - local pending="${SUDOERS_FILE%/*}/.${SUDOERS_FILE##*/}.pending" - run_root install -m 0440 -o root -g root "$staged" "$pending" - if [[ $DRY_RUN == false ]]; then - "${SUDO[@]}" visudo -cqf "$pending" > /dev/null 2>&1 || { - run_root rm -f "$pending" - die "The staged drop-in does not parse where sudo would read it, so this host is unchanged" - } + if [[ $own_current == false ]]; then + # A drop-in whose name holds a dot is one sudo skips, so the content lands under such a name, is proved where it will be read, and only then is renamed over. + # Renaming is atomic, which a copy into place is not, and a half-written file in that directory locks every user out of sudo. + local pending="${SUDOERS_FILE%/*}/.${SUDOERS_FILE##*/}.pending" + run_root install -m 0440 -o root -g root "$staged" "$pending" + if [[ $DRY_RUN == false ]]; then + "${SUDO[@]}" visudo -cqf "$pending" > /dev/null 2>&1 || { + run_root rm -f "$pending" + die "The staged drop-in does not parse where sudo would read it, so this host is unchanged" + } + fi + run_root mv "$pending" "$SUDOERS_FILE" fi - run_root mv "$pending" "$SUDOERS_FILE" + + # A superseded file is removed only once $SUDOERS_FILE is proved in place, so a failure above never leaves this user's cache unset. + local old + for old in "${delete_files[@]}"; do + run_root rm -f "$old" + [[ $DRY_RUN == true ]] || log "Removed $old, superseded by $SUDOERS_FILE" + done if [[ $DRY_RUN == true ]]; then sudo_timestamp_report "$user" @@ -1183,9 +1228,9 @@ configure_sudo_timestamp() { fi "${SUDO[@]}" visudo -cq > /dev/null 2>&1 || - die "sudoers stopped parsing once $SUDOERS_FILE landed. Remove that file from a root shell to restore sudo." + die "sudoers stopped parsing once this run's changes landed. Remove $SUDOERS_FILE from a root shell to restore sudo, and recreate ${delete_files[*]:-any file this run deleted} if it turns out to have been needed." - log "Wrote $SUDOERS_FILE" + [[ $own_current == false ]] && log "Wrote $SUDOERS_FILE" sudo_timestamp_report "$user" } From f9195fd2b0dac89a8e17c942dc555a005d490d1b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 16 Aug 2026 07:32:49 -0700 Subject: [PATCH 2/6] Escape the Username Before Embedding It in an ERE Copilot review on #764: sudo_timestamp_file_is_pure and the elsewhere classifier both interpolated $user directly into an ERE. A username can legally hold a regex metacharacter (a dot is common in a domain-joined account), and left unescaped it can match a different user's line as if it were this one's, in the exact deletion path this PR added. sudo_timestamp_user_re escapes each ERE metacharacter in pure bash, so "john.doe" and "john_doe" are no longer conflated. Verified with a new fixture scenario (a target user whose name collides via regex with an unrelated user's own file) plus a shellcheck-clean pass on the pure-bash rewrite (the first, sed-based version tripped SC2001/SC2016/SC1003). --- host-setup/linux/install-tools.sh | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index 1c122fc..3c495d8 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -1096,11 +1096,26 @@ sudo_timestamp_report() { fi } +# A username escaped for safe embedding in an ERE. +# A username can legally hold a regex metacharacter (a dot is common in a domain-joined account), and left unescaped it can match a different user's line as if it were this one's. +sudo_timestamp_user_re() { + local user="$1" out="" i c + for ((i = 0; i < ${#user}; i++)); do + c="${user:i:1}" + case "$c" in + '.' | '[' | $'\\' | '^' | '$' | '(' | ')' | '*' | '+' | '?' | '{' | '}' | '|') out+="\\$c" ;; + *) out+="$c" ;; + esac + done + printf '%s' "$out" +} + # Whether a drop-in holds nothing but this user's timestamp Defaults, aside from blank lines and comments. # That purity is what makes deleting the whole file safe, since a mixed file would lose whatever else it sets, and picking lines back out of one is guessing, not reading. sudo_timestamp_file_is_pure() { - local user="$1" file="$2" other - other=$("${SUDO[@]}" grep -vE "^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user}[[:space:]]+timestamp_(type|timeout)=" \ + local user="$1" file="$2" other user_re + user_re=$(sudo_timestamp_user_re "$user") + other=$("${SUDO[@]}" grep -vE "^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=" \ "$file" 2> /dev/null) || other="" [[ -z $other ]] } @@ -1154,7 +1169,8 @@ configure_sudo_timestamp() { # Only this user's own entry is ever a delete candidate; a different user's entry, or one with no user named at all, changes something beyond what this run was asked to change, so it is reported and left alone. local -a delete_files=() unsafe_files=() if [[ -n $elsewhere ]]; then - local candidate + local candidate user_re + user_re=$(sudo_timestamp_user_re "$user") while read -r candidate; do [[ -n $candidate ]] || continue # The main sudoers file is never auto-edited, and a drop-in that sets something else besides is never guessed at line by line, since either mistake risks removing a rule unrelated to this. @@ -1163,7 +1179,7 @@ configure_sudo_timestamp() { else unsafe_files+=("$candidate") fi - done < <(grep -E "Defaults:${user}[[:space:]]+timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u) + done < <(grep -E "Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u) fi if [[ $own_current == true && ${#delete_files[@]} -eq 0 ]]; then From 238bd3df059b1175b44dcc667e7582f033ba56d4 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 16 Aug 2026 07:40:33 -0700 Subject: [PATCH 3/6] Anchor the Purity Allowlist and Match Any Option Ordering Copilot review on #764, round 2: two real gaps in the classification path this PR added. sudo_timestamp_file_is_pure's allowlist was a prefix match, so a line like "Defaults:user timestamp_type=global,requiretty" passed as pure even though it also carries requiretty. The allowlist is now anchored at both ends and accepts only comma-separated timestamp_type/timeout assignments, nothing else. The same function silently treated a grep read error the same as "no disallowed line found" (pure), so an unreadable file could have been deleted sight unseen; it now reads grep's exit status directly and only exit 1 (every line matched, ergo pure) counts, never a fallback default. The elsewhere classifier required the timestamp option to be the first one after the username on its Defaults line, so "Defaults:user !authenticate, timestamp_type=global" evaded classification entirely and the file would not even reach the unsafe-file check. Widened to match the timestamp assignment anywhere after the required username-boundary whitespace, which is what keeps a user like "john" from matching inside "johnson". Verified with four new fixture scenarios: same-line unrelated option (now refused), reordered options (now recognized and refused), a username that is a literal prefix of another user's (still not conflated, boundary intact), and a simulated grep read error (now treated as not-pure rather than pure). --- host-setup/linux/install-tools.sh | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index 3c495d8..36ad24a 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -1112,12 +1112,14 @@ sudo_timestamp_user_re() { # Whether a drop-in holds nothing but this user's timestamp Defaults, aside from blank lines and comments. # That purity is what makes deleting the whole file safe, since a mixed file would lose whatever else it sets, and picking lines back out of one is guessing, not reading. +# The allowlist is anchored at both ends, so a line carrying a timestamp assignment plus anything else (a trailing ",requiretty") fails it rather than passing as a prefix match. sudo_timestamp_file_is_pure() { - local user="$1" file="$2" other user_re + local user="$1" file="$2" user_re allow status user_re=$(sudo_timestamp_user_re "$user") - other=$("${SUDO[@]}" grep -vE "^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=" \ - "$file" 2> /dev/null) || other="" - [[ -z $other ]] + allow="^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=[^,[:space:]]+(,[[:space:]]*timestamp_(type|timeout)=[^,[:space:]]+)*[[:space:]]*\$" + "${SUDO[@]}" grep -vE "$allow" "$file" > /dev/null 2>&1 && status=0 || status=$? + # Exit 1 means every line matched the allowlist (pure), 0 means one did not (impure), and anything else is a read failure this cannot tell apart from either, so it is never treated as pure. + [[ $status -eq 1 ]] } # Share one sudo credential cache across a user's terminals, rather than sudo's default of one per terminal. @@ -1179,7 +1181,7 @@ configure_sudo_timestamp() { else unsafe_files+=("$candidate") fi - done < <(grep -E "Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u) + done < <(grep -E "Defaults:${user_re}[[:space:]]+.*timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u) fi if [[ $own_current == true && ${#delete_files[@]} -eq 0 ]]; then From 758a964e0f21dd41028c3d1d7034ff939a82362b Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 16 Aug 2026 07:47:40 -0700 Subject: [PATCH 4/6] Fold Unsafe Files Into the Already-Correct Early Return Copilot review on #764, round 3: the early-return guard checked own_current and delete_files but not unsafe_files. A standing mixed-content conflict for this user, with the managed file already correct and no pure duplicate to clean up, made the function return success silently, the one outcome this PR exists to prevent. Requires unsafe_files to be empty too before returning early, so that case now falls through to the same warn-and-die path every other unsafe conflict takes. Verified with a new fixture scenario: managed file pre-seeded correct, a mixed-content file added for the same user, run again, must die rather than report "already carries exactly this." --- host-setup/linux/install-tools.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index 36ad24a..8fbb939 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -1184,7 +1184,8 @@ configure_sudo_timestamp() { done < <(grep -E "Defaults:${user_re}[[:space:]]+.*timestamp_(type|timeout)=" <<< "$elsewhere" | awk -F: '{print $1}' | sort -u) fi - if [[ $own_current == true && ${#delete_files[@]} -eq 0 ]]; then + # A standing unsafe file still reaches the die below even when this run's own file needs no change, since silently returning here would report success over a same-user conflict this cannot resolve on its own. + if [[ $own_current == true && ${#delete_files[@]} -eq 0 && ${#unsafe_files[@]} -eq 0 ]]; then log "$SUDOERS_FILE already carries exactly this, leaving it alone" sudo_timestamp_report "$user" return 0 From 035347844fc6f7e5aeab16408a25f3039442b7a8 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 16 Aug 2026 07:52:06 -0700 Subject: [PATCH 5/6] Correct the Purity Comment's Overclaim on Trailing Comments Copilot review on #764, round 4: sudo_timestamp_file_is_pure's leading comment said "aside from blank lines and comments," but the allowlist only accepts a full-line comment, not one trailing a Defaults line on the same line. Narrowed the wording to match what the regex actually does. The regex itself is unchanged: widening it to also accept a trailing "# ..." on an otherwise-safe Defaults line was suggested but declined in the PR thread, since every prior round's fix in this file moved the same direction (never guess, refuse rather than assume safe), and this would move the opposite way for an authoring style the one real drop-in this feature was built against does not use. --- host-setup/linux/install-tools.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index 8fbb939..af43232 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -1110,7 +1110,7 @@ sudo_timestamp_user_re() { printf '%s' "$out" } -# Whether a drop-in holds nothing but this user's timestamp Defaults, aside from blank lines and comments. +# Whether a drop-in holds nothing but this user's timestamp Defaults, aside from a blank line or a full-line comment. # That purity is what makes deleting the whole file safe, since a mixed file would lose whatever else it sets, and picking lines back out of one is guessing, not reading. # The allowlist is anchored at both ends, so a line carrying a timestamp assignment plus anything else (a trailing ",requiretty") fails it rather than passing as a prefix match. sudo_timestamp_file_is_pure() { From 38115d4c4f243ef8dffc019a4ab1a6e0b50b726c Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Sun, 16 Aug 2026 07:57:14 -0700 Subject: [PATCH 6/6] Drop the Redundant Backslash Before the Allowlist's End Anchor Copilot review on #764, round 5: raised \$ in the allow regex as matching a literal dollar rather than ERE's end-of-line anchor. Verified against the running shell rather than taken on faith: the allow string is double-quoted, and bash's own quote processing strips a backslash before $ there regardless, so grep was always receiving a plain $ (confirmed by diffing the stored value of the escaped and unescaped forms, byte for byte identical, plus every fixture scenario in this PR already exercising this exact regex and passing). The finding was a false positive, working from the source text rather than what bash hands to grep. Dropped the now-provably-redundant backslash anyway, since it is exactly what produced this reading once and would read the same way to the next person, and added a comment stating why a bare $ is correct here so it does not get "fixed" back. --- host-setup/linux/install-tools.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/host-setup/linux/install-tools.sh b/host-setup/linux/install-tools.sh index af43232..9043cb8 100755 --- a/host-setup/linux/install-tools.sh +++ b/host-setup/linux/install-tools.sh @@ -1116,7 +1116,8 @@ sudo_timestamp_user_re() { sudo_timestamp_file_is_pure() { local user="$1" file="$2" user_re allow status user_re=$(sudo_timestamp_user_re "$user") - allow="^[[:space:]]*(#.*)?\$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=[^,[:space:]]+(,[[:space:]]*timestamp_(type|timeout)=[^,[:space:]]+)*[[:space:]]*\$" + # $ here is bash's own end-of-string, and the double quotes strip a backslash before it, so grep receives a plain $ (ERE's end-of-line anchor), never a literal one. + allow="^[[:space:]]*(#.*)?$|^[[:space:]]*Defaults:${user_re}[[:space:]]+timestamp_(type|timeout)=[^,[:space:]]+(,[[:space:]]*timestamp_(type|timeout)=[^,[:space:]]+)*[[:space:]]*$" "${SUDO[@]}" grep -vE "$allow" "$file" > /dev/null 2>&1 && status=0 || status=$? # Exit 1 means every line matched the allowlist (pure), 0 means one did not (impure), and anything else is a read failure this cannot tell apart from either, so it is never treated as pure. [[ $status -eq 1 ]]