From 44b0500abc90788a6fcb257a8ccf931793d2d6e3 Mon Sep 17 00:00:00 2001
From: "pulse-triage[bot]"
 <249995291+pulse-triage[bot]@users.noreply.github.com>
Date: Mon, 26 Oct 2026 20:52:58 +0000
Subject: [PATCH] Preserve recovery keys when local legacy cleanup cannot
 complete

Separate staging-file admission from grep no-match in both shipped local cleanup paths. Resolve shared targets, require intact metadata and return incomplete results without replacing unrelated keys or caller resources. Add actual filesystem failure and caller-path controls.

Change-source: pulse-maintainer
---
 .../subsystems/deployment-installability.md   |  34 ++-
 install.sh                                    |  38 +++-
 scripts/installtests/root_install_sh_test.go  |  35 +++
 .../uninstall_sensor_proxy_test.go            | 199 ++++++++++++++++++
 scripts/uninstall-sensor-proxy.sh             |  35 +--
 5 files changed, 313 insertions(+), 28 deletions(-)

diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md
index c1d504f8d7..8e552d205e 100644
--- a/docs/release-control/v6/internal/subsystems/deployment-installability.md
+++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md
@@ -769,12 +769,21 @@ must not silently select local-only cleanup. An explicit `--local-only` remains
 available on each node without cluster discovery or SSH. Local keys are cleaned
 once and duplicate remote addresses once, with strict SSH host verification intact.
 
-Local key replacement uses an adjacent temporary file and preserves the original
-on read/comparison/metadata/rename failure. A no-match grep result is distinct
-from a read failure; removing an all-managed file may legitimately produce an
-empty file. Only the two existing Pulse comment suffixes are eligible, and
-unrelated keys and file metadata remain intact. Conditional helper invocation
-must propagate failure rather than print a successful key-removal claim.
+Both the standalone helper and `install.sh`'s presence-gated local legacy cleanup
+resolve existing key paths to regular targets before replacement; dangling
+links, non-file targets and failed resolution are not confirmed clean. Shared
+links and peer access to their target remain intact. Local key replacement uses
+an adjacent temporary file and preserves the original on allocation, output-open,
+read, comparison, metadata or rename failure. Opening the output descriptor is
+a separate checked operation: shell redirection failure can return 1 but is not
+grep's valid no-match result. Removing an all-managed file may legitimately
+produce an empty file. Only the two existing Pulse comment suffixes are eligible;
+unrelated keys and file metadata remain intact. Already-clean targets are not
+replaced. Owned staging cleanup failure is nonzero, including after otherwise
+successful work; diagnostic wording must not promise unchanged original bytes
+in that case. Private subshell traps/descriptors must not replace the caller's.
+Conditional helper invocation must propagate failure, not successful removal.
+No cluster/API action is added to the root installer.
 
 Remote cleanup resolves the fixed `authorized_keys` pathname before replacing
 its target; it must never replace a cluster-shared symlink with a private copy.
@@ -814,6 +823,19 @@ inventory through the real cluster/remote helpers into main's incomplete result,
 without live SSH, Proxmox or customer operations. These checks do not establish
 native cluster-filesystem semantics or any original installed recovery.
 
+`TestUninstallSensorProxyLocalKeyWritersPreserveRecovery` applies real private
+filesystem controls to both shipped local entrypoints, including regular/shared
+targets, peer readback, dangling/non-file paths, every writer failure and an
+actual ordinary-user mode-0400 output-open denial. Root test invocations explicitly
+skip only that permission-specific case, not the other controls.
+`TestUninstallSensorProxyLocalKeyWriterKeepsCallerResources` proves the caller's
+EXIT trap and descriptor survive. `TestUninstallSensorProxyLocalKeyFailureReachesMain`
+and `TestRootInstallSensorProxyCleanupCannotHideKeyReadFailure` execute the actual
+caller paths with closed service/file/API doubles and a partial failed key read;
+they require preserved recovery keys, nonzero status and no completion claim.
+These source checks do not establish native privileged filesystem semantics,
+installed uninstall/recovery, credential revocation or reporter acceptance.
+
 ### Legacy sensor cleanup requires observed stopped targets
 
 The legacy sensor uninstaller must confirm each existing unit has stopped and
diff --git a/install.sh b/install.sh
index be25080d61..b1c1dc35f8 100644
--- a/install.sh
+++ b/install.sh
@@ -5754,17 +5754,37 @@ local_sensor_proxy_present() {
 remove_local_sensor_proxy_managed_keys() {
     local auth_file="$SENSOR_PROXY_AUTHORIZED_KEYS_PATH"
     local tmp_file=""
-    [[ -f "$auth_file" ]] || return 0
-    tmp_file=$(mktemp) || return 0
-    grep -v -E '# pulse-(managed|proxy)-key$' "$auth_file" >"$tmp_file" 2>/dev/null || true
-    if cmp -s "$tmp_file" "$auth_file"; then
-        rm -f "$tmp_file"
+    local status=0
+    if [[ ! -e "$auth_file" && ! -L "$auth_file" ]]; then
         return 0
     fi
-    chmod --reference="$auth_file" "$tmp_file" 2>/dev/null || chmod 600 "$tmp_file" 2>/dev/null || true
-    chown --reference="$auth_file" "$tmp_file" 2>/dev/null || true
-    if ! mv "$tmp_file" "$auth_file"; then
-        rm -f "$tmp_file"
+    # Proxmox can link this file into the shared cluster filesystem. Resolve
+    # before replacing it; broken/non-file targets are not confirmed absence.
+    if ! auth_file=$(readlink -f -- "$auth_file") || [[ ! -f "$auth_file" || -L "$auth_file" ]]; then
+        return 1
+    fi
+    # Keep traps/descriptors private and replacement on the same filesystem.
+    # A failed output open must never masquerade as grep's valid no-match (1).
+    if ! (
+        tmp_file=$(mktemp "${auth_file}.pulse-cleanup.XXXXXX") || exit 1
+        trap 'status=$?; rm -f -- "$tmp_file" || status=1; exit "$status"' EXIT
+        trap 'exit 1' HUP INT TERM
+        exec 3>"$tmp_file" || exit 1
+        grep -v -E '# pulse-(managed|proxy)-key$' "$auth_file" >&3 2>/dev/null || status=$?
+        exec 3>&- || exit 1
+        (( status <= 1 )) || exit 1
+        status=0
+        cmp -s "$tmp_file" "$auth_file" || status=$?
+        case "$status" in
+            0) ;;
+            1)
+                chmod --reference="$auth_file" "$tmp_file" &&
+                    chown --reference="$auth_file" "$tmp_file" &&
+                    mv -- "$tmp_file" "$auth_file" || exit 1
+                ;;
+            *) exit 1 ;;
+        esac
+    ); then
         return 1
     fi
     echo "Removed legacy pulse-sensor-proxy SSH key entries from $auth_file"
diff --git a/scripts/installtests/root_install_sh_test.go b/scripts/installtests/root_install_sh_test.go
index edbf487df7..feaf2218c7 100644
--- a/scripts/installtests/root_install_sh_test.go
+++ b/scripts/installtests/root_install_sh_test.go
@@ -2587,6 +2587,41 @@ fi
 	}
 }
 
+func TestRootInstallSensorProxyCleanupCannotHideKeyReadFailure(t *testing.T) {
+	dir := t.TempDir()
+	keys := filepath.Join(dir, "authorized_keys")
+	const original = "ssh-ed25519 AAAAold # pulse-managed-key\n" + sensorProxyRecoveryKeys
+	if err := os.WriteFile(keys, []byte(original), 0600); err != nil {
+		t.Fatal(err)
+	}
+	body := `source "$1"
+SENSOR_PROXY_AUTHORIZED_KEYS_PATH="$FIXTURE_DIR/authorized_keys"
+local_sensor_proxy_present() { :; }
+stop_pulse_unit_for_removal() { :; }
+# Filesystem/service/user removal outside the key fixture is never executed.
+rm() {
+    case "${!#}" in "$FIXTURE_DIR/authorized_keys.pulse-cleanup."*) command rm "$@";; *) :;; esac
+}
+grep() { printf 'partial key read\n'; return 2; }
+userdel() { echo unexpected-user-removal >>"$FIXTURE_DIR/actions"; }
+groupdel() { echo unexpected-group-removal >>"$FIXTURE_DIR/actions"; }
+if cleanup_local_sensor_proxy; then exit 0; else exit $?; fi
+`
+	out, code := runSensorProxySafetyShell(t, repoFile("install.sh"), dir, "grep", body)
+	if code == 0 || strings.Contains(out, "Removed legacy") {
+		t.Fatalf("installer cleanup hid failed key read: exit %d: %s", code, out)
+	}
+	if got, err := os.ReadFile(keys); err != nil || string(got) != original {
+		t.Fatalf("installer failure lost recovery keys: %q, %v", got, err)
+	}
+	if _, err := os.Stat(filepath.Join(dir, "actions")); !os.IsNotExist(err) {
+		t.Fatalf("service-user removal followed unconfirmed key cleanup: %v", err)
+	}
+	if leftovers, err := filepath.Glob(keys + ".pulse-cleanup.*"); err != nil || len(leftovers) != 0 {
+		t.Fatalf("failed installer cleanup left staging: %v, %v", leftovers, err)
+	}
+}
+
 func TestRootInstallResetBoundsEverySystemdObservationAndMutation(t *testing.T) {
 	for _, name := range []string{"reset_pulse", "read_pulse_reset_unit_state", "confirm_pulse_reset_unit_stopped"} {
 		body := extractRootInstallShellFunction(t, name)
diff --git a/scripts/installtests/uninstall_sensor_proxy_test.go b/scripts/installtests/uninstall_sensor_proxy_test.go
index f5a3ddfb4b..b07946304a 100644
--- a/scripts/installtests/uninstall_sensor_proxy_test.go
+++ b/scripts/installtests/uninstall_sensor_proxy_test.go
@@ -760,6 +760,7 @@ getent() {
     [[ "$1" == ahostsv4 ]] || return 90
     case "$2" in pve-local) echo '127.0.0.1 STREAM pve-local';; pve-remote) echo '192.0.2.11 STREAM pve-remote';; *) return 90;; esac
 }
+
 disable_legacy_units() { :; }
 cleanup_stale_sensor_proxy_mounts() { :; }
 remove_legacy_files() { echo files >>"$FIXTURE_DIR/actions"; }
@@ -778,3 +779,201 @@ if main; then exit 0; else exit $?; fi
 		t.Fatalf("failed remote replacement changed recovery keys: %q, %v", got, err)
 	}
 }
+
+// Both shipped entrypoints must preserve unrelated access keys. All filesystem
+// operations below are real private-user operations; only faults are injected.
+func TestUninstallSensorProxyLocalKeyWritersPreserveRecovery(t *testing.T) {
+	for _, entry := range []struct{ name, script, function string }{
+		{"standalone", repoFile("scripts", "uninstall-sensor-proxy.sh"), "cleanup_local_authorized_keys"},
+		{"installer", repoFile("install.sh"), "remove_local_sensor_proxy_managed_keys"},
+	} {
+		for _, kind := range []string{"regular", "shared_link"} {
+			for _, phase := range []string{"success", "unchanged", "all_managed", "missing", "directory", "readlink", "mktemp", "output_open", "grep", "cmp", "chmod", "chown", "mv", "cleanup"} {
+				t.Run(entry.name+"/"+kind+"/"+phase, func(t *testing.T) {
+					if phase == "output_open" && os.Geteuid() == 0 {
+						t.Skip("real mode-0400 write denial requires an ordinary user")
+					}
+					dir := t.TempDir()
+					auth := filepath.Join(dir, "local", "authorized_keys")
+					target := auth
+					if kind == "shared_link" {
+						target = filepath.Join(dir, "cluster keys", "authorized_keys")
+					}
+					for _, parent := range []string{filepath.Dir(auth), filepath.Dir(target), filepath.Join(dir, "peer")} {
+						if err := os.MkdirAll(parent, 0700); err != nil {
+							t.Fatal(err)
+						}
+					}
+					original := "ssh-ed25519 AAAAold pulse # pulse-managed-key\n" + sensorProxyRecoveryKeys + "ssh-ed25519 AAAAold2 pulse # pulse-proxy-key\n"
+					if phase == "unchanged" || phase == "cleanup" {
+						original = sensorProxyRecoveryKeys
+					} else if phase == "all_managed" {
+						original = "ssh-ed25519 AAAAold pulse # pulse-managed-key\n"
+					}
+					if phase == "directory" {
+						if err := os.Mkdir(target, 0700); err != nil {
+							t.Fatal(err)
+						}
+					} else if phase != "missing" {
+						if err := os.WriteFile(target, []byte(original), 0640); err != nil {
+							t.Fatal(err)
+						}
+					}
+					peer := filepath.Join(dir, "peer", "authorized_keys")
+					const linkTarget = "../cluster keys/authorized_keys"
+					if kind == "shared_link" {
+						for _, link := range []string{auth, peer} {
+							if err := os.Symlink(linkTarget, link); err != nil {
+								t.Fatal(err)
+							}
+						}
+					}
+					before, _ := os.Stat(target)
+					linkBefore, _ := os.Lstat(auth)
+					body := `source "$1"
+AUTHORIZED_KEYS_PATH="$FIXTURE_DIR/local/authorized_keys"
+SENSOR_PROXY_AUTHORIZED_KEYS_PATH="$AUTHORIZED_KEYS_PATH"
+readlink() { [[ "$SCENARIO" != readlink ]] || return 9; command readlink "$@"; }
+mktemp() {
+    [[ "$SCENARIO" != mktemp ]] || return 9
+    local path
+    path=$(command mktemp "$@") || return 1
+    [[ "$SCENARIO" != output_open ]] || command chmod 0400 "$path" || return 1
+    printf '%s\n' "$path"
+}
+grep() {
+    if [[ "$SCENARIO" == grep ]]; then printf 'partial read\n'; return 2; fi
+    command grep "$@"
+}
+cmp() { [[ "$SCENARIO" != cmp ]] || return 2; command cmp "$@"; }
+chmod() { case "$SCENARIO" in chmod|unchanged|cleanup) return 9;; esac; command chmod "$@"; }
+chown() { case "$SCENARIO" in chown|unchanged|cleanup) return 9;; esac; command chown "$@"; }
+mv() { case "$SCENARIO" in mv|unchanged|cleanup) return 9;; esac; command mv "$@"; }
+rm() { [[ "$SCENARIO" != cleanup ]] || return 9; command rm "$@"; }
+# Conditional invocation disables errexit in the complete helper call tree.
+if ` + entry.function + `; then exit 0; else exit $?; fi
+`
+					out, code := runSensorProxySafetyShell(t, entry.script, dir, phase, body)
+					success := phase == "success" || phase == "unchanged" || phase == "all_managed" || (phase == "missing" && kind == "regular")
+					if (code == 0) != success || (!success && (strings.Contains(out, "[ OK ]") || strings.Contains(out, "Removed legacy"))) {
+						t.Fatalf("local status mismatch: exit %d: %s", code, out)
+					}
+					if kind == "shared_link" {
+						afterLink, err := os.Lstat(auth)
+						if err != nil || !os.SameFile(linkBefore, afterLink) {
+							t.Fatalf("local cleanup replaced shared-key link: %v", err)
+						}
+						for _, link := range []string{auth, peer} {
+							if got, err := os.Readlink(link); err != nil || got != linkTarget {
+								t.Fatalf("shared-key relationship changed: %q, %v", got, err)
+							}
+						}
+					}
+					if phase == "missing" {
+						if _, err := os.Stat(target); !os.IsNotExist(err) {
+							t.Fatalf("missing key target created: %v", err)
+						}
+						return
+					}
+					after, err := os.Stat(target)
+					if err != nil {
+						t.Fatal(err)
+					}
+					if phase == "directory" {
+						if !after.IsDir() || !os.SameFile(before, after) {
+							t.Fatal("non-file target was replaced")
+						}
+						return
+					}
+					want := original
+					if phase == "success" {
+						want = sensorProxyRecoveryKeys
+					} else if phase == "all_managed" {
+						want = ""
+					}
+					for _, path := range []string{auth, target} {
+						if got, err := os.ReadFile(path); err != nil || string(got) != want {
+							t.Fatalf("local recovery keys corrupted: %q, %v; want %q", got, err, want)
+						}
+					}
+					if kind == "shared_link" {
+						if got, err := os.ReadFile(peer); err != nil || string(got) != want {
+							t.Fatalf("peer sees different shared keys: %q, %v", got, err)
+						}
+					}
+					if after.Mode().Perm() != before.Mode().Perm() {
+						t.Fatal("local key permissions changed")
+					}
+					if phase != "success" && phase != "all_managed" && !os.SameFile(before, after) {
+						t.Fatal("failed or unchanged key target was replaced")
+					}
+					leftovers, err := filepath.Glob(target + ".pulse-cleanup.*")
+					if err != nil || (phase != "cleanup" && len(leftovers) != 0) || (phase == "cleanup" && len(leftovers) != 1) {
+						t.Fatalf("unexpected staging cleanup result: %v, %v", leftovers, err)
+					}
+				})
+			}
+		}
+	}
+}
+
+func TestUninstallSensorProxyLocalKeyWriterKeepsCallerResources(t *testing.T) {
+	for _, entry := range []struct{ name, script, function string }{
+		{"standalone", repoFile("scripts", "uninstall-sensor-proxy.sh"), "cleanup_local_authorized_keys"},
+		{"installer", repoFile("install.sh"), "remove_local_sensor_proxy_managed_keys"},
+	} {
+		t.Run(entry.name, func(t *testing.T) {
+			dir := t.TempDir()
+			body := `source "$1"
+AUTHORIZED_KEYS_PATH="$FIXTURE_DIR/authorized_keys"
+SENSOR_PROXY_AUTHORIZED_KEYS_PATH="$AUTHORIZED_KEYS_PATH"
+printf 'ssh-ed25519 AAAAold # pulse-managed-key\n' >"$AUTHORIZED_KEYS_PATH"
+trap 'printf caller-exit >"$FIXTURE_DIR/trap"' EXIT
+exec 3>"$FIXTURE_DIR/descriptor"
+if ` + entry.function + `; then
+    printf caller-still-open >&3
+else
+    exit 1
+fi
+`
+			out, code := runSensorProxySafetyShell(t, entry.script, dir, "success", body)
+			if code != 0 {
+				t.Fatalf("caller did not survive cleanup: exit %d: %s", code, out)
+			}
+			for file, want := range map[string]string{"trap": "caller-exit", "descriptor": "caller-still-open"} {
+				if got, err := os.ReadFile(filepath.Join(dir, file)); err != nil || string(got) != want {
+					t.Fatalf("caller's %s changed: %q, %v", file, got, err)
+				}
+			}
+		})
+	}
+}
+
+func TestUninstallSensorProxyLocalKeyFailureReachesMain(t *testing.T) {
+	dir := t.TempDir()
+	keys := filepath.Join(dir, "authorized_keys")
+	const original = "ssh-ed25519 AAAAold # pulse-managed-key\n" + sensorProxyRecoveryKeys
+	if err := os.WriteFile(keys, []byte(original), 0600); err != nil {
+		t.Fatal(err)
+	}
+	body := `source "$1"
+AUTHORIZED_KEYS_PATH="$FIXTURE_DIR/authorized_keys"
+disable_legacy_units() { :; }
+cleanup_stale_sensor_proxy_mounts() { :; }
+remove_legacy_files() { echo files >>"$FIXTURE_DIR/actions"; }
+remove_proxmox_access() { :; }
+systemctl_if_available() { :; }
+grep() { printf 'partial key read\n'; return 2; }
+if main --local-only; then exit 0; else exit $?; fi
+`
+	out, code := runSensorProxySafetyShell(t, repoFile("scripts", "uninstall-sensor-proxy.sh"), dir, "grep", body)
+	if code == 0 || strings.Contains(out, "[ OK ]") || !strings.Contains(out, "cluster SSH key cleanup is incomplete") {
+		t.Fatalf("main hid local key failure: exit %d: %s", code, out)
+	}
+	if got, err := os.ReadFile(keys); err != nil || string(got) != original {
+		t.Fatalf("main lost recovery keys after read failure: %q, %v", got, err)
+	}
+	if got, err := os.ReadFile(filepath.Join(dir, "actions")); err != nil || string(got) != "files\n" {
+		t.Fatalf("local failure did not reach the final main result: %q, %v", got, err)
+	}
+}
diff --git a/scripts/uninstall-sensor-proxy.sh b/scripts/uninstall-sensor-proxy.sh
index 6cfdeecfa4..f220d333b4 100755
--- a/scripts/uninstall-sensor-proxy.sh
+++ b/scripts/uninstall-sensor-proxy.sh
@@ -135,43 +135,52 @@ disable_legacy_units() {
     return "$result"
 }
 
-remove_managed_keys_from_authorized_keys_file() {
+remove_managed_keys_from_authorized_keys_file() (
     local file="$1"
     local tmp_file=""
     local status=0
 
-    if [[ ! -f "$file" ]]; then
+    if [[ ! -e "$file" && ! -L "$file" ]]; then
         return 0
     fi
+    [[ -f "$file" && ! -L "$file" ]] || return 1
 
     # Keep replacement on the same filesystem. A failed read, metadata copy or
     # rename must leave the original keys intact, even under conditional calls.
+    # Isolate the descriptor and traps from the caller. Open separately: a
+    # failed shell redirection returns 1 too, but is not grep's valid no-match.
     tmp_file=$(mktemp "${file}.pulse-cleanup.XXXXXX") || return 1
-    grep -v -E '# pulse-(managed|proxy)-key$' "$file" >"$tmp_file" 2>/dev/null || status=$?
-    if (( status > 1 )); then
-        rm -f -- "$tmp_file"
-        return 1
-    fi
+    trap 'status=$?; rm -f -- "$tmp_file" || status=1; exit "$status"' EXIT
+    trap 'exit 1' HUP INT TERM
+    exec 3>"$tmp_file" || return 1
+    grep -v -E '# pulse-(managed|proxy)-key$' "$file" >&3 2>/dev/null || status=$?
+    exec 3>&- || return 1
+    (( status <= 1 )) || return 1
     status=0
     cmp -s "$tmp_file" "$file" || status=$?
     if (( status == 0 )); then
-        rm -f -- "$tmp_file" || return 1
         return 0
     fi
     if (( status > 1 )) ||
         ! chmod --reference="$file" "$tmp_file" ||
         ! chown --reference="$file" "$tmp_file" ||
         ! mv -- "$tmp_file" "$file"; then
-        rm -f -- "$tmp_file"
         return 1
     fi
-}
+)
 
 cleanup_local_authorized_keys() {
-    local auth_file=""
-    auth_file=$(resolve_path "$AUTHORIZED_KEYS_PATH") || return 1
+    local auth_file="$AUTHORIZED_KEYS_PATH"
+    # An absent file is an idempotent no-op, but a dangling/shared link or a
+    # non-file target is not confirmed clean. Never replace an unresolved link.
+    if [[ -e "$auth_file" || -L "$auth_file" ]]; then
+        if ! auth_file=$(readlink -f -- "$auth_file") || [[ ! -f "$auth_file" || -L "$auth_file" ]]; then
+            print_warn "Local legacy SSH key path is unresolved or not a regular file; cleanup is incomplete"
+            return 1
+        fi
+    fi
     if ! remove_managed_keys_from_authorized_keys_file "$auth_file"; then
-        print_warn "Local legacy SSH key cleanup failed; original keys were preserved"
+        print_warn "Local legacy SSH key cleanup is incomplete; check the key file and any retained staging locally"
         return 1
     fi
     print_success "Removed legacy Pulse SSH key entries from ${auth_file}"