fix(agent): make package update checks work under the systemd sandbox (#2538)

The agent runs as the beszel user under ProtectSystem=strict, so package
managers that write temp, cache or log files failed with only
"exit status 1" in the log.

- Include the command's stderr in package update check errors.
- dnf5: point XDG_STATE_HOME and XDG_CACHE_HOME into the agent's data
  dir. They default to $HOME, which is /nonexistent for the beszel user.
  With -C, dnf5 still reads the system metadata cache.
- zypper: keep the solv cache in the data dir with --solv-cache-dir,
  since the agent user cannot rebuild it in /var/cache/zypp.
- install-agent.sh, Debian unit: add PrivateTmp=yes. dnf4 (Rocky, Alma)
  and zypper keep temporary files in /var/tmp, which ProtectSystem=strict
  makes read-only. The install script also adds it to existing units
  when re-run.

agent/package_updates.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"context"
 	"errors"
+	"fmt"
 	"log/slog"
 	"os"
 	"os/exec"
@@ -54,7 +55,8 @@ type packageUpdatesManager struct {
 
 // newPackageUpdatesManager returns nil if disabled or no supported package manager
 // is found. Agents running in a container are skipped because the container's
-// package database is not the host's. dataDir holds pacman's private sync databases.
+// package database is not the host's. dataDir holds the package managers' per-user
+// caches and state, which they cannot write to their usual locations as the agent user.
 func newPackageUpdatesManager(dataDir string) *packageUpdatesManager {
 	if runtime.GOOS != "linux" || runningInContainer() {
 		return nil
@@ -150,9 +152,9 @@ func detectPackageManager(dataDir string) (string, packageUpdatesCheck) {
 	case commandExists("apt-get") && exec.Command("dpkg-query", "-W", "apt").Run() == nil:
 		return "apt", checkApt
 	case commandExists("dnf"):
-		return "dnf", checkDnf
+		return "dnf", newDnfCheck(dataDir)
 	case commandExists("zypper"):
-		return "zypper", checkZypper
+		return "zypper", newZypperCheck(dataDir)
 	case commandExists("checkupdates"):
 		return "pacman", newPacmanCheck(dataDir)
 	case commandExists("apk"):
@@ -183,8 +185,14 @@ func runPackageCommandEnv(ctx context.Context, env []string, okCodes []int, name
 	// children can keep stdout open. WaitDelay stops Output from waiting on them.
 	cmd.WaitDelay = 10 * time.Second
 	out, err := cmd.Output()
-	if exitErr, ok := errors.AsType[*exec.ExitError](err); ok && slices.Contains(okCodes, exitErr.ExitCode()) {
-		return string(out), nil
+	if exitErr, ok := errors.AsType[*exec.ExitError](err); ok {
+		if slices.Contains(okCodes, exitErr.ExitCode()) {
+			return string(out), nil
+		}
+		// include stderr so a failure says why, not just "exit status 1"
+		if stderr := strings.TrimSpace(string(exitErr.Stderr)); stderr != "" {
+			err = fmt.Errorf("%s: %w: %s", name, err, stderr)
+		}
 	}
 	return string(out), err
 }
@@ -214,10 +222,26 @@ func checkApt(ctx context.Context) (packageUpdatesResult, error) {
 	}, nil
 }
 
+// newDnfCheck points dnf5's per-user state (log) and cache dirs into dataDir.
+// They default to $HOME/.local/state and $HOME/.cache, and the agent user's home
+// is usually /nonexistent. With -C, dnf5 still reads the system metadata cache.
+func newDnfCheck(dataDir string) packageUpdatesCheck {
+	var env []string
+	if dataDir != "" {
+		env = []string{
+			"XDG_STATE_HOME=" + filepath.Join(dataDir, "state"),
+			"XDG_CACHE_HOME=" + filepath.Join(dataDir, "cache"),
+		}
+	}
+	return func(ctx context.Context) (packageUpdatesResult, error) {
+		return checkDnf(ctx, env)
+	}
+}
+
 // checkDnf uses the system metadata cache only (-C), so it never downloads metadata.
 // check-update lists only available versions, so installed versions come from rpm.
-func checkDnf(ctx context.Context) (packageUpdatesResult, error) {
-	out, err := runPackageCommand(ctx, []int{100}, "dnf", "-q", "-C", "check-update")
+func checkDnf(ctx context.Context, env []string) (packageUpdatesResult, error) {
+	out, err := runPackageCommandEnv(ctx, env, []int{100}, "dnf", "-q", "-C", "check-update")
 	if err != nil {
 		return packageUpdatesResult{}, err
 	}
@@ -237,7 +261,7 @@ func checkDnf(ctx context.Context) (packageUpdatesResult, error) {
 		}
 	}
 
-	out, err = runPackageCommand(ctx, []int{100}, "dnf", "-q", "-C", "check-update", "--security")
+	out, err = runPackageCommandEnv(ctx, env, []int{100}, "dnf", "-q", "-C", "check-update", "--security")
 	if err == nil {
 		// --security lists the lowest version that fixes an advisory, which may be
 		// older than the version check-update offers, so match on name.arch only
@@ -261,20 +285,28 @@ func checkDnf(ctx context.Context) (packageUpdatesResult, error) {
 	return result, nil
 }
 
-// checkZypper lists package updates. Security updates come from patches, which
+// newZypperCheck lists package updates. Security updates come from patches, which
 // zypper does not map to packages here, so only the security count is known.
-func checkZypper(ctx context.Context) (packageUpdatesResult, error) {
-	out, err := runPackageCommand(ctx, nil, "zypper", "--no-refresh", "-q", "list-updates")
-	if err != nil {
-		return packageUpdatesResult{}, err
+// zypper rebuilds the solv cache from the raw metadata when it is missing or stale,
+// which the agent user cannot do in /var/cache/zypp, so it keeps its own in dataDir.
+func newZypperCheck(dataDir string) packageUpdatesCheck {
+	args := []string{"--no-refresh", "-q"}
+	if dataDir != "" {
+		args = append(args, "--solv-cache-dir", filepath.Join(dataDir, "zypp-solv"))
 	}
-	packages := parseZypperListUpdates(out)
-	result := packageUpdatesResult{packages: packages, counts: []uint16{uint16(len(packages))}}
-	out, err = runPackageCommand(ctx, nil, "zypper", "--no-refresh", "-q", "list-patches", "--category", "security")
-	if err == nil {
-		result.counts = append(result.counts, parseZypperTable(out))
+	return func(ctx context.Context) (packageUpdatesResult, error) {
+		out, err := runPackageCommand(ctx, nil, "zypper", slices.Concat(args, []string{"list-updates"})...)
+		if err != nil {
+			return packageUpdatesResult{}, err
+		}
+		packages := parseZypperListUpdates(out)
+		result := packageUpdatesResult{packages: packages, counts: []uint16{uint16(len(packages))}}
+		out, err = runPackageCommand(ctx, nil, "zypper", slices.Concat(args, []string{"list-patches", "--category", "security"})...)
+		if err == nil {
+			result.counts = append(result.counts, parseZypperTable(out))
+		}
+		return result, nil
 	}
-	return result, nil
 }
 
 // newPacmanCheck uses checkupdates (pacman-contrib), which syncs a private copy of

agent/package_updates_test.go

@@ -278,7 +278,7 @@ exit 100`,
 				"rpm": `cat "` + testDataPath(t, tt.installed) + `"
 exit 1`,
 			})
-			result, err := checkDnf(context.Background())
+			result, err := newDnfCheck(t.TempDir())(context.Background())
 			require.NoError(t, err)
 			assert.Equal(t, []uint16{uint16(tt.total), uint16(tt.securityCount)}, result.counts)
 			assert.True(t, result.securityKnown)
@@ -297,12 +297,36 @@ echo "bash.x86_64 5.1.8-9.el9 baseos"
 exit 100`,
 			"rpm": `echo "bash.x86_64 5.1.8-6.el9_1"`,
 		})
-		result, err := checkDnf(context.Background())
+		result, err := newDnfCheck(t.TempDir())(context.Background())
 		require.NoError(t, err)
 		assert.Equal(t, []uint16{1}, result.counts)
 		assert.False(t, result.securityKnown)
 		assert.Equal(t, []system.PackageUpdate{{Name: "bash", Current: "5.1.8-6.el9_1", Available: "5.1.8-9.el9"}}, result.packages)
 	})
+
+	t.Run("error includes stderr", func(t *testing.T) {
+		fakeCommands(t, map[string]string{
+			"dnf": `echo "Error: Cache-only enabled but no cache for 'baseos'" >&2
+exit 1`,
+		})
+		_, err := newDnfCheck(t.TempDir())(context.Background())
+		require.Error(t, err)
+		assert.Equal(t, "dnf: exit status 1: Error: Cache-only enabled but no cache for 'baseos'", err.Error())
+	})
+
+	t.Run("state and cache in data dir", func(t *testing.T) {
+		dataDir := t.TempDir()
+		envFile := filepath.Join(t.TempDir(), "env")
+		fakeCommands(t, map[string]string{
+			"dnf": `echo "$XDG_STATE_HOME $XDG_CACHE_HOME" >> "` + envFile + `"`,
+		})
+		_, err := newDnfCheck(dataDir)(context.Background())
+		require.NoError(t, err)
+		env, err := os.ReadFile(envFile)
+		require.NoError(t, err)
+		want := filepath.Join(dataDir, "state") + " " + filepath.Join(dataDir, "cache") + "\n"
+		assert.Equal(t, want+want, string(env))
+	})
 }
 
 func TestDetectPackageManager(t *testing.T) {
@@ -357,15 +381,25 @@ func TestParseZypperListUpdates(t *testing.T) {
 }
 
 func TestCheckZypper(t *testing.T) {
+	dataDir := t.TempDir()
+	argsFile := filepath.Join(t.TempDir(), "args")
 	fakeCommands(t, map[string]string{
-		"zypper": `case "$*" in *list-patches*) cat "` + testDataPath(t, "zypper_leap155_list_patches_security.txt") + `" ;; *) cat "` + testDataPath(t, "zypper_leap155_list_updates.txt") + `" ;; esac`,
+		"zypper": `echo "$*" >> "` + argsFile + `"
+case "$*" in *list-patches*) cat "` + testDataPath(t, "zypper_leap155_list_patches_security.txt") + `" ;; *) cat "` + testDataPath(t, "zypper_leap155_list_updates.txt") + `" ;; esac`,
 	})
-	result, err := checkZypper(context.Background())
+	result, err := newZypperCheck(dataDir)(context.Background())
 	require.NoError(t, err)
 	// security patches don't map to packages, so only the count is known
 	assert.Equal(t, []uint16{22, 4}, result.counts)
 	assert.False(t, result.securityKnown)
 	assert.Len(t, result.packages, 22)
+
+	// both commands use the agent's own solv cache
+	args, err := os.ReadFile(argsFile)
+	require.NoError(t, err)
+	solvDir := filepath.Join(dataDir, "zypp-solv")
+	assert.Equal(t, "--no-refresh -q --solv-cache-dir "+solvDir+" list-updates\n"+
+		"--no-refresh -q --solv-cache-dir "+solvDir+" list-patches --category security\n", string(args))
 }
 
 func TestParsePacmanCheckUpdates(t *testing.T) {

supplemental/debian/beszel-agent.service

@@ -20,6 +20,7 @@ ProtectHome=read-only
 ProtectHostname=yes
 ProtectKernelLogs=yes
 ProtectSystem=strict
+PrivateTmp=yes
 RemoveIPC=yes
 RestrictSUIDSGID=true
 

supplemental/scripts/install-agent.sh

@@ -1369,6 +1369,7 @@ ProtectHome=read-only
 ProtectHostname=yes
 ProtectKernelLogs=yes
 ProtectSystem=strict
+PrivateTmp=yes
 RemoveIPC=yes
 RestrictSUIDSGID=true
 
@@ -1396,6 +1397,13 @@ EOF
     sed -i '/^User=beszel$/a SupplementaryGroups=systemd-journal' /etc/systemd/system/beszel-agent.service
   fi
 
+  # ProtectSystem=strict makes /var/tmp read-only, but dnf4 and zypper run as a non-root
+  # user keep temporary files in /var/tmp, so package update checks fail without this.
+  if grep -q '^ProtectSystem=strict$' /etc/systemd/system/beszel-agent.service && ! grep -q '^PrivateTmp=' /etc/systemd/system/beszel-agent.service; then
+    echo "Giving the agent service a private writable /tmp and /var/tmp"
+    sed -i '/^ProtectSystem=strict$/a PrivateTmp=yes' /etc/systemd/system/beszel-agent.service
+  fi
+
   # Load and start the service
   printf "\nLoading and starting the agent service...\n"
   systemctl daemon-reload