From a38ef3734df44b9baee47843ae36fd24ff38ffb7 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 13 Oct 2026 13:18:15 +0100 Subject: [PATCH] Keep NAS agent update copies executable Avoid rewriting an already-replaced QNAP executable through an aliased persistent path. Stage split QNAP and Unraid copies in owned same-directory temporaries, set execute permissions before atomic replacement, and clean every failed stage without changing saved state or update trust. Add authenticated HTTPS and real child-exec regressions plus persistence failure and recovery controls. Native appliance and old-installed-updater recovery remain separate. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 22 ++ internal/agentupdate/coverage_test.go | 143 +++++++++++- internal/agentupdate/update.go | 46 +++- internal/agentupdate/update_http_test.go | 203 ++++++++++++++++++ 4 files changed, 405 insertions(+), 9 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 09c777a1be..5266071948 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -7647,6 +7647,28 @@ that waits for the persistent data volume before launching the stored wrapper, and `internal/agentupdate/update.go` keeps the persisted QNAP binary copy in sync on self-update so reboot does not roll the runtime back to an older binary. +The persistent replacement must also remain executable. If the verified runtime +and persistent path identify the same file (including directory or executable +symlink aliases), the updater must not copy over its already-installed binary. +Separate QNAP/Unraid copies use an exclusively created temporary in the +destination directory, closed and explicitly made `0755` before atomic rename. +They must not reuse or follow a pre-existing fixed `.tmp` path. Read, creation, +write, close, chmod or rename failure leaves the previous persistent binary +unchanged, closes/removes the owned temporary and logs the persistence failure. +Persistence remains best-effort after live replacement; a runnable new runtime +does not establish that the boot copy was updated. Saved identity, credentials, +transport trust, mandatory checksum/signature, downloaded-binary self-test and +main replacement/backup recovery remain unchanged. + +`TestNASAuthenticatedUpdateKeepsExecutableCopies` exercises the real trusted +HTTPS/checksum/test-signature/self-test path with inert changed executables in +unified, aliased and split QNAP layouts, split Unraid and an ordinary Linux +control. Actual child execution checks both runtime and boot copies, preserved +saved state and temporary cleanup. `TestPersistentBinaryCopyFailuresRetainExecutable` +requires every persistence fault phase to retain an executable old copy and +clean its own temporary; unowned legacy temporaries and other hardlinks survive. +These Linux source controls do not establish native appliance, flash mount, +boot/watchdog, old-installed-updater recovery or release availability. That appliance lifecycle must remain bounded by available storage. Before downloading or replacing an agent, `scripts/install.sh` checks the effective temporary and installation filesystems for enough free space, coalesces the diff --git a/internal/agentupdate/coverage_test.go b/internal/agentupdate/coverage_test.go index 593c3f8d08..0ec8da460c 100644 --- a/internal/agentupdate/coverage_test.go +++ b/internal/agentupdate/coverage_test.go @@ -19,6 +19,7 @@ import ( "time" "github.com/rcourtman/pulse-go-rewrite/internal/securityutil" + "github.com/rs/zerolog" ) type roundTripperFunc func(*http.Request) (*http.Response, error) @@ -1418,6 +1419,7 @@ func TestPerformUpdateUnraidPaths(t *testing.T) { origWrite := writeFileFn origRename := renameFn origRestart := restartProcessFn + origCreateTemp := createTempFn t.Cleanup(func() { unraidVersionPath = origUnraid unraidPersistentPathFn = origPersist @@ -1425,6 +1427,7 @@ func TestPerformUpdateUnraidPaths(t *testing.T) { writeFileFn = origWrite renameFn = origRename restartProcessFn = origRestart + createTempFn = origCreateTemp }) tmpDir := t.TempDir() @@ -1453,10 +1456,17 @@ func TestPerformUpdateUnraidPaths(t *testing.T) { t.Run("WriteError", func(t *testing.T) { readFileFn = func(string) ([]byte, error) { return []byte("new"), nil } - writeFileFn = func(string, []byte, os.FileMode) error { return errors.New("write fail") } + createTempFn = func(dir, pattern string) (*os.File, error) { + file, err := os.CreateTemp(dir, pattern) + if err == nil && strings.Contains(pattern, "-persist-") { + err = file.Close() // Make the real persistent write fail. + } + return file, err + } if err := u.performUpdateWithExecPath(context.Background(), execPath); err != nil { t.Fatalf("expected update success, got %v", err) } + createTempFn = origCreateTemp }) t.Run("RenameError", func(t *testing.T) { @@ -1504,6 +1514,7 @@ func TestPerformUpdateQNAPPaths(t *testing.T) { origWrite := writeFileFn origRename := renameFn origRestart := restartProcessFn + origCreateTemp := createTempFn t.Cleanup(func() { qnapGetcfgPath = origQnapGetcfg qnapQpkgConfPath = origQnapQpkgConf @@ -1512,6 +1523,7 @@ func TestPerformUpdateQNAPPaths(t *testing.T) { writeFileFn = origWrite renameFn = origRename restartProcessFn = origRestart + createTempFn = origCreateTemp }) tmpDir := t.TempDir() @@ -1532,11 +1544,18 @@ func TestPerformUpdateQNAPPaths(t *testing.T) { t.Run("WriteError", func(t *testing.T) { readFileFn = func(string) ([]byte, error) { return []byte("new"), nil } - writeFileFn = func(string, []byte, os.FileMode) error { return errors.New("write fail") } + createTempFn = func(dir, pattern string) (*os.File, error) { + file, err := os.CreateTemp(dir, pattern) + if err == nil && strings.Contains(pattern, "-persist-") { + err = file.Close() + } + return file, err + } renameFn = os.Rename if err := u.performUpdateWithExecPath(context.Background(), execPath); err != nil { t.Fatalf("expected update success, got %v", err) } + createTempFn = origCreateTemp }) t.Run("RenameError", func(t *testing.T) { @@ -1570,6 +1589,126 @@ func TestPerformUpdateQNAPPaths(t *testing.T) { }) } +func TestPersistentBinaryCopyFailuresRetainExecutable(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("NAS executable persistence controls require Linux") + } + oldBinary := buildNASUpdateExecutable(t, "6.4.5") + newBinary := buildNASUpdateExecutable(t, "6.5.0") + for _, phase := range []string{"read", "create", "write", "close", "chmod", "rename", "success", "legacy-temp-file", "legacy-temp-symlink", "same-file", "hardlink"} { + t.Run(phase, func(t *testing.T) { + dir := t.TempDir() + execPath := filepath.Join(dir, "runtime") + persistPath := filepath.Join(dir, "pulse-agent") + if err := os.WriteFile(execPath, newBinary, 0755); err != nil { + t.Fatal(err) + } + if phase == "same-file" { + persistPath = execPath + } else if err := os.WriteFile(persistPath, oldBinary, 0755); err != nil { + t.Fatal(err) + } + if phase == "hardlink" { + if err := os.Link(persistPath, persistPath+".old-link"); err != nil { + t.Fatal(err) + } + } + const foreign = "unrelated recovery bytes" + if phase == "legacy-temp-file" { + if err := os.WriteFile(persistPath+".tmp", []byte(foreign), 0644); err != nil { + t.Fatal(err) + } + } + if phase == "legacy-temp-symlink" { + if err := os.WriteFile(filepath.Join(dir, "foreign"), []byte(foreign), 0600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(dir, "foreign"), persistPath+".tmp"); err != nil { + t.Fatal(err) + } + } + origRead, origCreate, origClose, origChmod, origRename := readFileFn, createTempFn, closeFileFn, chmodFn, renameFn + t.Cleanup(func() { + readFileFn, createTempFn, closeFileFn, chmodFn, renameFn = origRead, origCreate, origClose, origChmod, origRename + }) + var temp *os.File + createTempFn = func(dir, pattern string) (*os.File, error) { + if phase == "create" { + return nil, errors.New("fixture create refused") + } + file, err := os.CreateTemp(dir, pattern) + temp = file + if err == nil && phase == "write" { + err = file.Close() + } + return file, err + } + if phase == "read" { + readFileFn = func(string) ([]byte, error) { return nil, errors.New("fixture read refused") } + } + if phase == "close" { + closeFileFn = func(file *os.File) error { + if err := file.Close(); err != nil { + t.Fatal(err) + } + return errors.New("fixture close refused") + } + } + if phase == "chmod" { + chmodFn = func(string, os.FileMode) error { return errors.New("fixture chmod refused") } + } + if phase == "rename" { + renameFn = func(string, string) error { return errors.New("fixture rename refused") } + } + var logs bytes.Buffer + logger := zerolog.New(&logs) + u := New(Config{Logger: &logger}) + u.syncPersistentBinaryCopy(execPath, persistPath, "qnap") + failure := phase == "read" || phase == "create" || phase == "write" || phase == "close" || phase == "chmod" || phase == "rename" + wantBytes, wantVersion := newBinary, "6.5.0" + if failure { + wantBytes, wantVersion = oldBinary, "6.4.5" + if !strings.Contains(logs.String(), "failed to ") || !strings.Contains(logs.String(), "persistent") { + t.Fatalf("persistence failure was not logged: %s", logs.String()) + } + } + got, err := os.ReadFile(persistPath) + if err != nil || checksum(got) != checksum(wantBytes) { + t.Fatalf("persistent recovery bytes changed: %v", err) + } + info, err := os.Stat(persistPath) + if err != nil || info.Mode().Perm() != 0755 { + t.Fatalf("persistent binary lost execute mode: %v", err) + } + runNASUpdateExecutable(t, persistPath, wantVersion) + runNASUpdateExecutable(t, execPath, "6.5.0") + if temp != nil { + if _, err := temp.Stat(); err == nil { + t.Fatal("persistent temporary descriptor was not closed") + } + if _, err := os.Stat(temp.Name()); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("owned persistent temporary was not cleaned: %v", err) + } + } else if phase == "same-file" && strings.Contains(logs.String(), "updated persistent binary") { + t.Fatal("already replaced executable was copied again") + } + if phase == "legacy-temp-file" || phase == "legacy-temp-symlink" { + got, err := os.ReadFile(persistPath + ".tmp") + if err != nil || string(got) != foreign { + t.Fatalf("unowned legacy temporary was changed: %v", err) + } + } + if phase == "hardlink" { + got, err := os.ReadFile(persistPath + ".old-link") + if err != nil || checksum(got) != checksum(oldBinary) { + t.Fatalf("atomic replace mutated another hardlink: %v", err) + } + runNASUpdateExecutable(t, persistPath+".old-link", "6.4.5") + } + }) + } +} + type errorReader struct { sent bool } diff --git a/internal/agentupdate/update.go b/internal/agentupdate/update.go index dc301eec0c..4a36d15e2e 100644 --- a/internal/agentupdate/update.go +++ b/internal/agentupdate/update.go @@ -1058,7 +1058,7 @@ func (u *Updater) performUpdateWithExecPathForVersion(ctx context.Context, execP if isUnraid() { persistPath := unraidPersistentPathFn(agentName) if _, err := os.Stat(persistPath); err == nil { - u.syncPersistentBinaryCopy(execPath, persistPath, "unraid") + u.syncPersistentBinaryCopy(realExecPath, persistPath, "unraid") } } @@ -1066,7 +1066,7 @@ func (u *Updater) performUpdateWithExecPathForVersion(ctx context.Context, execP persistPath := qnapPersistentPathFn(u.cfg.StateDir, agentName) if persistPath != "" { if _, err := os.Stat(filepath.Dir(persistPath)); err == nil { - u.syncPersistentBinaryCopy(execPath, persistPath, "qnap") + u.syncPersistentBinaryCopy(realExecPath, persistPath, "qnap") } } } @@ -1187,6 +1187,15 @@ func (u *Updater) performPrivilegedUpdate(ctx context.Context, execPath string, } func (u *Updater) syncPersistentBinaryCopy(execPath, persistPath, platform string) { + // QNAP normally runs directly from its persistent data volume. The main + // replacement has already installed the verified, executable binary there. + // Compare file identity, not path spelling, to include symlinked mounts. + if execInfo, err := os.Stat(execPath); err == nil { + if persistInfo, err := os.Stat(persistPath); err == nil && os.SameFile(execInfo, persistInfo) { + return + } + } + u.logger.Debug().Str("path", persistPath).Str("platform", platform).Msg("updating persistent binary") newBinary, err := readFileFn(execPath) @@ -1195,16 +1204,39 @@ func (u *Updater) syncPersistentBinaryCopy(execPath, persistPath, platform strin return } - tmpPersist := persistPath + ".tmp" - if err := writeFileFn(tmpPersist, newBinary, 0644); err != nil { + // Use an owned, exclusive temporary in the destination directory. A fixed + // .tmp path can retain old permissions or follow a leftover symlink. + tmpFile, err := createTempFn(filepath.Dir(persistPath), filepath.Base(persistPath)+"-persist-*.tmp") + if err != nil { + u.logger.Warn().Err(err).Str("path", persistPath).Str("platform", platform).Msg("failed to create temporary persistent binary") + return + } + tmpPersist := tmpFile.Name() + defer func() { + if removeErr := os.Remove(tmpPersist); removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) { + u.logger.Warn().Err(removeErr).Str("path", tmpPersist).Str("platform", platform).Msg("failed to remove temporary persistent binary") + } + }() + + if _, err := tmpFile.Write(newBinary); err != nil { + if closeErr := closeFileFn(tmpFile); closeErr != nil { + err = errors.Join(err, closeErr) + } u.logger.Warn().Err(err).Str("path", tmpPersist).Str("platform", platform).Msg("failed to write persistent binary") return } + if err := closeFileFn(tmpFile); err != nil { + u.logger.Warn().Err(err).Str("path", tmpPersist).Str("platform", platform).Msg("failed to close persistent binary") + return + } + // WriteFile's mode does not repair an existing file, and a restrictive + // umask can remove execute bits. Establish them before the atomic rename. + if err := chmodFn(tmpPersist, 0755); err != nil { + u.logger.Warn().Err(err).Str("path", tmpPersist).Str("platform", platform).Msg("failed to chmod persistent binary") + return + } if err := renameFn(tmpPersist, persistPath); err != nil { u.logger.Warn().Err(err).Str("path", persistPath).Str("platform", platform).Msg("failed to replace persistent binary") - if removeErr := os.Remove(tmpPersist); removeErr != nil && !errors.Is(removeErr, os.ErrNotExist) { - u.logger.Warn().Err(removeErr).Str("path", tmpPersist).Str("platform", platform).Msg("failed to remove temporary persistent binary") - } return } diff --git a/internal/agentupdate/update_http_test.go b/internal/agentupdate/update_http_test.go index e8674cd68a..edb9ff1f00 100644 --- a/internal/agentupdate/update_http_test.go +++ b/internal/agentupdate/update_http_test.go @@ -6,11 +6,14 @@ import ( "crypto/rand" "encoding/base64" "encoding/json" + "encoding/pem" "errors" "net/http" "net/http/httptest" "os" + "os/exec" "path/filepath" + "runtime" "strings" "sync/atomic" "testing" @@ -19,6 +22,206 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/updatesignature" ) +// These executables are inert, not Pulse agents. They let the actual signed +// HTTPS updater, self-test and child exec paths check Unix execute permissions +// without replacing the test process or touching an appliance/service. +func buildNASUpdateExecutable(t *testing.T, version string) []byte { + t.Helper() + dir := t.TempDir() + source := filepath.Join(dir, "main.go") + const program = `package main +import ("fmt"; "os") +var version string +func main() { + if len(os.Args) > 1 && os.Args[1] == "--self-test" { + if len(os.Args) != 4 || os.Args[2] != "--token-file" { os.Exit(10) } + info, err := os.Stat(os.Args[3]); if err != nil || info.Mode().Perm() != 0600 { os.Exit(11) } + token, err := os.ReadFile(os.Args[3]); if err != nil || string(token) != "fixture-token" { os.Exit(12) } + } + fmt.Println(version) +} +` + if err := os.WriteFile(source, []byte(program), 0600); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), time.Minute) + defer cancel() + binary := filepath.Join(dir, "fixture-agent") + cmd := exec.CommandContext(ctx, "go", "build", "-trimpath", "-ldflags=-X main.version="+version, "-o", binary, source) + cmd.Env = append(os.Environ(), "CGO_ENABLED=0", "GOPROXY=off", "GOWORK=off") + if output, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("build inert executable: %v\n%s", err, output) + } + data, err := os.ReadFile(binary) + if err != nil { + t.Fatal(err) + } + return data +} + +func runNASUpdateExecutable(t *testing.T, path, wantVersion string) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + output, err := exec.CommandContext(ctx, path).CombinedOutput() + if err != nil || strings.TrimSpace(string(output)) != wantVersion { + t.Fatalf("execute inert binary: error=%v, output=%q, want %q", err, output, wantVersion) + } +} + +func TestNASAuthenticatedUpdateKeepsExecutableCopies(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("QNAP/Unraid executable-layout controls require Linux") + } + oldBinary := buildNASUpdateExecutable(t, "6.4.5") + newBinary := buildNASUpdateExecutable(t, "6.5.0") + key := configureTrustedUpdateSigningKey(t) + signature := signedUpdateHeader(t, newBinary, key) + + for _, layout := range []string{"qnap-unified", "qnap-directory-alias", "qnap-executable-alias", "qnap-persistent-alias", "qnap-split", "unraid-split", "ordinary"} { + t.Run(layout, func(t *testing.T) { + dir := t.TempDir() + stateDir := filepath.Join(dir, "state") + runtimeDir := filepath.Join(dir, "runtime") + for _, path := range []string{stateDir, runtimeDir} { + if err := os.Mkdir(path, 0700); err != nil { + t.Fatal(err) + } + } + execPath := filepath.Join(runtimeDir, "pulse-agent") + persistPath := filepath.Join(stateDir, "pulse-agent") + if layout == "qnap-unified" || layout == "qnap-directory-alias" || layout == "qnap-executable-alias" { + execPath = persistPath + } + for _, path := range []string{execPath, persistPath} { + if err := os.WriteFile(path, oldBinary, 0755); err != nil { + t.Fatal(err) + } + } + if layout == "qnap-directory-alias" { + alias := filepath.Join(dir, "state-alias") + if err := os.Symlink(stateDir, alias); err != nil { + t.Fatal(err) + } + persistPath = filepath.Join(alias, "pulse-agent") + } + if layout == "qnap-executable-alias" { + alias := filepath.Join(runtimeDir, "pulse-agent-link") + if err := os.Symlink(execPath, alias); err != nil { + t.Fatal(err) + } + execPath = alias + } + if layout == "qnap-persistent-alias" { + if err := os.Remove(persistPath); err != nil { + t.Fatal(err) + } + if err := os.Symlink(execPath, persistPath); err != nil { + t.Fatal(err) + } + } + state := map[string]string{"agent-id": "fixture-identity\n", "token": "saved-fixture-token\n", "agent.env": "PULSE_URL=https://fixture.invalid\n"} + for name, value := range state { + if err := os.WriteFile(filepath.Join(stateDir, name), []byte(value), 0600); err != nil { + t.Fatal(err) + } + } + + origUnraid, origGetcfg, origQpkg := unraidVersionPath, qnapGetcfgPath, qnapQpkgConfPath + origUnraidPersist, origQnapPersist, origRestart := unraidPersistentPathFn, qnapPersistentPathFn, restartProcessFn + t.Cleanup(func() { + unraidVersionPath, qnapGetcfgPath, qnapQpkgConfPath = origUnraid, origGetcfg, origQpkg + unraidPersistentPathFn, qnapPersistentPathFn, restartProcessFn = origUnraidPersist, origQnapPersist, origRestart + }) + unraidVersionPath, qnapGetcfgPath, qnapQpkgConfPath = filepath.Join(dir, "unraid-version"), filepath.Join(dir, "getcfg"), filepath.Join(dir, "qpkg.conf") + marker := "" + if strings.HasPrefix(layout, "qnap-") { + marker = qnapGetcfgPath + } else if layout == "unraid-split" { + marker = unraidVersionPath + } + if marker != "" { + if err := os.WriteFile(marker, []byte("fixture"), 0600); err != nil { + t.Fatal(err) + } + } + unraidPersistentPathFn = func(string) string { return persistPath } + qnapPersistentPathFn = func(string, string) string { return persistPath } + restarts := 0 + restartProcessFn = func(path string) error { + restarts++ + if path != execPath { + t.Errorf("restart changed executable path") + } + runNASUpdateExecutable(t, path, "6.5.0") + return nil + } + + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/download/pulse-agent" || r.URL.Query().Get("serverVersion") != "6.5.0" || r.Header.Get(apiTokenHeader) != "fixture-token" { + t.Error("missing download identity or authentication") + http.Error(w, "fixture refused", http.StatusForbidden) + return + } + w.Header().Set(checksumSHA256Header, checksum(newBinary)) + w.Header().Set(signatureHeader, signature) + _, _ = w.Write(newBinary) + })) + defer server.Close() + caPath := filepath.Join(dir, "fixture-ca.pem") + if err := os.WriteFile(caPath, pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: server.Certificate().Raw}), 0600); err != nil { + t.Fatal(err) + } + u := New(Config{PulseURL: server.URL, CACertPath: caPath, APIToken: "fixture-token", AgentName: "pulse-agent", CurrentVersion: "6.4.5", StateDir: stateDir}) + // Keep the real downloaded-binary self-test, checksum, signature and TLS. + if err := u.performUpdateWithExecPathForVersion(context.Background(), execPath, "6.5.0"); err != nil { + t.Fatal(err) + } + if restarts != 1 { + t.Fatalf("restart calls = %d, want 1", restarts) + } + for _, path := range []string{execPath, persistPath} { + wantBytes, wantVersion := newBinary, "6.5.0" + if layout == "ordinary" && path == persistPath { + wantBytes, wantVersion = oldBinary, "6.4.5" + } + data, err := os.ReadFile(path) + if err != nil || checksum(data) != checksum(wantBytes) { + t.Fatalf("binary bytes changed unexpectedly: %v", err) + } + info, err := os.Stat(path) + if err != nil || info.Mode().Perm() != 0755 { + t.Fatalf("binary is not 0755: %v", err) + } + runNASUpdateExecutable(t, path, wantVersion) + } + for name, value := range state { + path := filepath.Join(stateDir, name) + data, err := os.ReadFile(path) + if err != nil || string(data) != value { + t.Fatalf("saved %s changed: %v", name, err) + } + info, err := os.Stat(path) + if err != nil || info.Mode().Perm() != 0600 { + t.Fatalf("saved %s permissions changed: %v", name, err) + } + } + for _, path := range []string{stateDir, runtimeDir} { + entries, err := os.ReadDir(path) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.HasSuffix(entry.Name(), ".tmp") || strings.HasSuffix(entry.Name(), ".backup") { + t.Fatalf("update left a temporary/backup: %s", entry.Name()) + } + } + } + t.Logf("%s: authenticated replacement, real self-test, runtime/persistent exec and saved-state checks passed", layout) + }) + } +} + const testPEMCertificate = `-----BEGIN CERTIFICATE----- MIICuDCCAaACCQDptFpSdDdFNjANBgkqhkiG9w0BAQsFADAeMRwwGgYDVQQDDBNw dWxzZS10ZXN0LWNhLmxvY2FsMB4XDTI2MDMxNDA4NTgyN1oXDTI3MDMxNDA4NTgy