From 3eb314231358c4206d51a818df98ffb18136e7a4 Mon Sep 17 00:00:00 2001 From: Afonso Oliveira Date: Wed, 16 Sep 2026 22:55:37 +0100 Subject: [PATCH] Recognize legacy sudo grants for any account name The legacy command wrote the caller's unvalidated name into both the sudoers filename and the rule. Cleanup applied the current lower-case account pattern to that suffix, so an exact legacy grant for an account such as Alice was classified as administrator policy, left active, and the machine-wide migration marker was written anyway. Match a legacy grant by its exact filename and rule relationship instead of the account policy, and cover it in the lifecycle suite through both the unit cleanup and the real migration runner. The account pattern itself stays lower-case: sudoers reads an upper-case word such as ALICE as a User_Alias reference, and ALL as every user, so such names must never reach the generated rule. Pin that with a test. Co-Authored-By: Claude Fable 5.1 --- bin/omarchy-sudo-passwordless | 31 +++++++++++-------- docs/passwordless-sudo.md | 2 +- test/shell.d/nopasswd-sudo-expiry-test.sh | 7 +++-- .../passwordless-grant-lifecycle-test.sh | 12 +++++-- 4 files changed, 34 insertions(+), 18 deletions(-) diff --git a/bin/omarchy-sudo-passwordless b/bin/omarchy-sudo-passwordless index 91695c69..3b0af89d 100755 --- a/bin/omarchy-sudo-passwordless +++ b/bin/omarchy-sudo-passwordless @@ -56,9 +56,10 @@ resolve_account() { entry=$(/usr/bin/getent passwd "$((10#$uid))") || return 1 IFS=: read -r ACCOUNT_NAME _ ACCOUNT_UID _ _ _ _ <<<"$entry" [[ $ACCOUNT_UID == "$((10#$uid))" ]] || return 1 - # Sudoers names and the legacy filename both have metacharacters. Omarchy - # accounts use this portable subset; refusing anything else is safer than - # attempting to quote privileged policy syntax. + # Sudoers has metacharacters, and it reads an upper-case word such as ALICE + # as an alias reference rather than a user. Accounts use this portable + # lower-case subset; refusing anything else is safer than attempting to + # quote privileged policy syntax. valid_account_name "$ACCOUNT_NAME" || return 1 ACCOUNT_UID=$((10#$uid)) } @@ -109,6 +110,8 @@ read_grant() { valid_account_name "$GRANT_NAME" || return 2 } +# Returns 0 for a rule this command generated, 1 for anything else under the +# owned prefix (preserved as administrator policy), and 2 when unreadable. classify_generated_rule() { local file=$1 suffix contents name @@ -116,18 +119,20 @@ classify_generated_rule() { contents=$(/usr/bin/cat -- "$file") || return 2 suffix=${file##*/99-omarchy-nopasswd-} - if [[ $suffix =~ ^[0-9]+$ ]]; then - name=${contents%' ALL=(ALL) NOPASSWD: ALL'} - if valid_account_name "$name" && [[ $contents == "$name ALL=(ALL) NOPASSWD: ALL" ]]; then - return 0 - fi - name=${contents%%' ALL=(ALL) NOTAFTER='*} - valid_account_name "$name" && [[ $contents =~ ^[a-z_][a-z0-9_-]*\$?\ ALL=\(ALL\)\ NOTAFTER=[0-9]{14}Z\ NOPASSWD:\ ALL$ ]] - elif valid_account_name "$suffix" && [[ $contents == "$suffix ALL=(ALL) NOPASSWD: ALL" ]]; then + # The legacy command wrote the caller's unvalidated name into both the + # filename and the rule. That exact relationship is its fingerprint, so an + # account the current policy would reject still has its old grant removed. + if [[ $contents == "$suffix ALL=(ALL) NOPASSWD: ALL" ]]; then return 0 - else - return 1 fi + + [[ $suffix =~ ^[0-9]+$ ]] || return 1 + name=${contents%' ALL=(ALL) NOPASSWD: ALL'} + if valid_account_name "$name" && [[ $contents == "$name ALL=(ALL) NOPASSWD: ALL" ]]; then + return 0 + fi + name=${contents%%' ALL=(ALL) NOTAFTER='*} + valid_account_name "$name" && [[ $contents =~ ^[a-z_][a-z0-9_-]*\$?\ ALL=\(ALL\)\ NOTAFTER=[0-9]{14}Z\ NOPASSWD:\ ALL$ ]] } cleanup_uid_locked() { diff --git a/docs/passwordless-sudo.md b/docs/passwordless-sudo.md index 39355504..1a846995 100644 --- a/docs/passwordless-sudo.md +++ b/docs/passwordless-sudo.md @@ -10,7 +10,7 @@ An internal status result is `0` for an active, validated grant and `3` for conf Calendar timers clean up expired files; their liveness does not define authorization. Callbacks read the current rule and remove it only when expired. Earlier callbacks cannot shorten a renewed grant, so no timer identity needs to be persisted. Old UID-only and token-bearing callbacks remain accepted. Pending callbacks after renewal or manual disable are harmless and expire within the maximum 24-hour grant window. Boot-time tmpfiles cleanup removes the reserved generated filename namespace before users log in; routine non-boot tmpfiles maintenance leaves live grants alone. -Legacy cleanup uses a root-owned machine marker under `/var/lib/omarchy/migrations/`, written only after successful cleanup under the grant lock. Later accounts can finish their migration queues without sudo and without revoking grants created after the repair. Old grant state files are no longer consulted; generated legacy policy is removed conservatively and administrator-modified policy is preserved by the migration. +Legacy cleanup uses a root-owned machine marker under `/var/lib/omarchy/migrations/`, written only after successful cleanup under the grant lock. Later accounts can finish their migration queues without sudo and without revoking grants created after the repair. Old grant state files are no longer consulted. A legacy grant is recognized by its exact filename and rule relationship, since the old command wrote the caller's unvalidated name into both, so accounts outside the current name policy are still cleaned up; administrator-modified policy under the owned prefix is preserved by the migration. ## Package ownership diff --git a/test/shell.d/nopasswd-sudo-expiry-test.sh b/test/shell.d/nopasswd-sudo-expiry-test.sh index 173ea1b0..d6b1a505 100755 --- a/test/shell.d/nopasswd-sudo-expiry-test.sh +++ b/test/shell.d/nopasswd-sudo-expiry-test.sh @@ -9,7 +9,8 @@ source "$SHELL_TEST_DIR/fixtures/passwordless-sudo-test.sh" for minutes in 1 15 1440 00015; do valid_minutes "$minutes" || exit 1; done for minutes in 0 1441 -1 1m '' 18446744073709551617; do ! valid_minutes "$minutes" || exit 1; done for name in audituser 'buildbot$'; do valid_account_name "$name" || exit 1; done - for name in 'a$b' '$' aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; do ! valid_account_name "$name" || exit 1; done + # Upper-case words are sudoers alias references, so ALICE must never publish. + for name in 'a$b' '$' 'a b' 'a#b' Alice ALICE ALL aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa; do ! valid_account_name "$name" || exit 1; done ! valid_uid 18446744073709551617 ) pass "duration and account validation retains bounded inputs and trailing-dollar usernames" @@ -159,5 +160,7 @@ cp "$ROOT/default/libalpm/hooks/05-omarchy-passwordless-revoke.hook" "$test_tmp/ /usr/sbin/visudo -cf "$(rule_file 1000)" >/dev/null cleanup_uid_locked 1000 [[ ! -e $(rule_file 1000) ]] + TEST_ACCOUNT=ALICE assert_status 1 enable_locked 1000 15 + [[ ! -e $(rule_file 1000) ]] ) -pass "trailing-dollar accounts publish and revoke valid native policy" +pass "trailing-dollar accounts publish valid native policy and alias-shaped names never publish" diff --git a/test/shell.d/passwordless-grant-lifecycle-test.sh b/test/shell.d/passwordless-grant-lifecycle-test.sh index a0be9d4f..91a0ba89 100644 --- a/test/shell.d/passwordless-grant-lifecycle-test.sh +++ b/test/shell.d/passwordless-grant-lifecycle-test.sh @@ -8,14 +8,20 @@ source "$SHELL_TEST_DIR/fixtures/passwordless-sudo-test.sh" source "$library" printf 'deleteduser ALL=(ALL) NOPASSWD: ALL\n' >"$(rule_file 1000)" printf 'buildbot$ ALL=(ALL) NOPASSWD: ALL\n' >"$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-buildbot$" + # The legacy command never validated the account name, so a manual or NSS + # account outside the current policy still has its exact old grant removed. + printf 'Alice ALL=(ALL) NOPASSWD: ALL\n' >"$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-Alice" printf 'admin ALL=(ALL) NOPASSWD: /usr/bin/true\n' >"$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-custom" + printf 'Alice ALL=(ALL) NOPASSWD: ALL\n' >"$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-handwritten" TEST_DELETE_FAIL=1 assert_status 1 cleanup_all_locked [[ -e $(rule_file 1000) ]] cleanup_all_locked - [[ ! -e $(rule_file 1000) && -e $test_tmp/etc/sudoers.d/99-omarchy-nopasswd-custom ]] + [[ ! -e $(rule_file 1000) && ! -e $test_tmp/etc/sudoers.d/99-omarchy-nopasswd-Alice ]] + [[ -e $test_tmp/etc/sudoers.d/99-omarchy-nopasswd-custom && -e $test_tmp/etc/sudoers.d/99-omarchy-nopasswd-handwritten ]] ! compgen -G "$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-buildbot*" + rm "$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-custom" "$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-handwritten" ) -pass "legacy cleanup removes generated orphan rules and preserves custom policy" +pass "legacy cleanup removes generated orphan rules for any account and preserves custom policy" # Run the actual migration queue for separate temporary homes. Sudo only calls # the mapped helper and can be refused without requesting host authorization. @@ -31,10 +37,12 @@ marker="$test_tmp/var/lib/omarchy/migrations/1788163635" ( source "$library" printf 'audituser ALL=(ALL) NOPASSWD: ALL\n' >"$(rule_file 1000)" + printf 'Alice ALL=(ALL) NOPASSWD: ALL\n' >"$test_tmp/etc/sudoers.d/99-omarchy-nopasswd-Alice" TEST_DELETE_FAIL=1 assert_status 1 run_migrations first [[ ! -e $marker && ! -e $test_tmp/first/1788163636.sh ]] run_migrations first [[ -f $marker && -f $test_tmp/first/1788163636.sh ]] + [[ ! -e $test_tmp/etc/sudoers.d/99-omarchy-nopasswd-Alice ]] enable_locked 1000 15 cp "$(rule_file 1000)" "$test_tmp/renewed" : >"$test_tmp/commands"