diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 98a8f4c..813253f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,5 +33,11 @@ jobs: - name: Run the checks run: make check + # make check runs as the runner user, so the tests that need root skip. + # They cover "sudo fly update": the owner of the binary, and a link that + # a hostile owner of the binary folder puts in place of the new file. + - name: Run the tests that need root + run: sudo env "PATH=$PATH" "GOCACHE=$(go env GOCACHE)" "GOMODCACHE=$(go env GOMODCACHE)" go test -count=1 -run 'TestReplaceBinary' ./internal/release/ + - name: Build the release archives run: make release diff --git a/cmd/version.go b/cmd/version.go index bef2ca2..9a28f90 100644 --- a/cmd/version.go +++ b/cmd/version.go @@ -1,12 +1,16 @@ package cmd import ( + "context" "errors" "fmt" + "io" "os" "github.com/flywp/server-cli/internal/release" + "github.com/flywp/server-cli/internal/service" "github.com/flywp/server-cli/internal/version" + "github.com/mattn/go-isatty" "github.com/spf13/cobra" ) @@ -25,6 +29,9 @@ var versionCmd = &cobra.Command{ var updateCmd = &cobra.Command{ Use: "update", Short: "Update fly-cli to the latest version", + Long: `Update fly-cli to the latest release. When fly already runs the latest +release, the command does nothing. On a server with the monitoring agent, the +command also restarts the agent, so that the agent runs the new binary.`, RunE: func(cmd *cobra.Command, args []string) error { if os.Geteuid() != 0 { return errors.New("the update command must be run as root, please run 'sudo fly update'") @@ -41,17 +48,17 @@ var updateCmd = &cobra.Command{ fmt.Printf("This is not a release build (version %s). Latest release: %s\n", version.Version, latest) case !update.Available: fmt.Println("You are already running the latest version.") - return nil + return restartStaleAgent(cmd.Context()) default: fmt.Printf("New version available: %s\n", latest) } if !yesFlag { - fmt.Printf("Do you want to install %s? (y/n): ", latest) - var response string - // An empty or unreadable answer cancels the update. - _, _ = fmt.Scanln(&response) - if response != "y" && response != "Y" { + ok, err := confirm(os.Stdin, os.Stdout, latest) + if err != nil { + return err + } + if !ok { fmt.Println("Update cancelled.") return nil } @@ -61,12 +68,61 @@ var updateCmd = &cobra.Command{ if err := release.SelfUpdate(cmd.Context(), update.Release); err != nil { return fmt.Errorf("updating: %w", err) } - fmt.Printf("Updated to %s.\n", latest) - return nil + + return restartAgent(cmd.Context()) }, } +// confirm asks whether to install latest. Without a terminal nobody can +// answer, so it returns an error: a script must not read "cancelled" as done. +func confirm(in *os.File, out io.Writer, latest string) (bool, error) { + if !isatty.IsTerminal(in.Fd()) && !isatty.IsCygwinTerminal(in.Fd()) { + return false, errors.New("there is no terminal to confirm the update: run 'sudo fly update --yes'") + } + + _, _ = fmt.Fprintf(out, "Do you want to install %s? (y/n): ", latest) + var response string + // An empty or unreadable answer cancels the update. + _, _ = fmt.Fscanln(in, &response) + + return response == "y" || response == "Y", nil +} + +// restartAgent restarts the monitoring agent, if the server has it, so that it +// runs the new binary. +func restartAgent(ctx context.Context) error { + if !service.Installed() { + return nil + } + + fmt.Println("Restarting the monitoring agent...") + if err := service.Restart(ctx); err != nil { + return fmt.Errorf("the update is installed, but the monitoring agent did not restart: %w", err) + } + + return nil +} + +// restartStaleAgent restarts the monitoring agent when it still runs a binary +// that an earlier update replaced. +func restartStaleAgent(ctx context.Context) error { + if !service.Installed() { + return nil + } + + stale, err := service.Stale(ctx) + if err != nil { + return fmt.Errorf("checking the monitoring agent: %w", err) + } + if !stale { + return nil + } + + fmt.Println("The monitoring agent runs an older binary.") + return restartAgent(ctx) +} + func init() { updateCmd.Flags().BoolVarP(&yesFlag, "yes", "y", false, "Automatically answer yes to update confirmation") rootCmd.AddCommand(versionCmd) diff --git a/cmd/version_test.go b/cmd/version_test.go new file mode 100644 index 0000000..09b29ca --- /dev/null +++ b/cmd/version_test.go @@ -0,0 +1,117 @@ +package cmd + +import ( + "context" + "io" + "os" + "path/filepath" + "strconv" + "strings" + "testing" + + "github.com/flywp/server-cli/internal/service" +) + +func TestConfirmNeedsATerminal(t *testing.T) { + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + defer func() { _ = r.Close() }() + _, _ = w.WriteString("y\n") + _ = w.Close() + + // A script that pipes "y" still needs --yes: the pipe is not a terminal. + ok, err := confirm(r, io.Discard, "v0.2.0") + if ok || err == nil || !strings.Contains(err.Error(), "--yes") { + t.Errorf("confirm() = %v, %v; want an error that tells to use --yes", ok, err) + } +} + +// fakeAgentService makes the agent unit exist and puts a fake systemctl in +// PATH. It returns the log of the systemctl calls. +func fakeAgentService(t *testing.T, installed bool) string { + return fakeAgentServiceExit(t, installed, 0) +} + +// fakeAgentServiceExit is fakeAgentService with a systemctl that exits with +// exit. +func fakeAgentServiceExit(t *testing.T, installed bool, exit int) string { + t.Helper() + + unit := filepath.Join(t.TempDir(), "fly-agent.service") + if installed { + if err := os.WriteFile(unit, []byte("[Unit]\n"), 0o644); err != nil { + t.Fatal(err) + } + } + old := service.UnitPath + service.UnitPath = unit + t.Cleanup(func() { service.UnitPath = old }) + + dir := t.TempDir() + log := filepath.Join(t.TempDir(), "calls") + script := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> " + log + "\n[ \"$1\" = show ] && echo 0\n[ " + strconv.Itoa(exit) + " -ne 0 ] && echo 'Failed to connect to bus' >&2\nexit " + strconv.Itoa(exit) + "\n" + if err := os.WriteFile(filepath.Join(dir, "systemctl"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + return log +} + +func systemctlCalls(t *testing.T, log string) string { + t.Helper() + data, err := os.ReadFile(log) + if err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + return strings.TrimSpace(string(data)) +} + +func TestRestartAgentAfterAnUpdate(t *testing.T) { + log := fakeAgentService(t, true) + if err := restartAgent(context.Background()); err != nil { + t.Fatal(err) + } + if got := systemctlCalls(t, log); got != "try-restart fly-agent" { + t.Errorf("systemctl calls = %q, want try-restart fly-agent", got) + } +} + +func TestNoRestartWithoutTheAgent(t *testing.T) { + log := fakeAgentService(t, false) + if err := restartAgent(context.Background()); err != nil { + t.Fatal(err) + } + if err := restartStaleAgent(context.Background()); err != nil { + t.Fatal(err) + } + if got := systemctlCalls(t, log); got != "" { + t.Errorf("systemctl calls = %q, want none on a server without the agent", got) + } +} + +func TestNoRestartWhenTheAgentDoesNotRun(t *testing.T) { + // The fake systemctl shows MainPID 0: the agent does not run, so systemd + // starts the binary on the disk. + log := fakeAgentService(t, true) + if err := restartStaleAgent(context.Background()); err != nil { + t.Fatal(err) + } + if got := systemctlCalls(t, log); got != "show --property=MainPID --value fly-agent" { + t.Errorf("systemctl calls = %q, want only the check", got) + } +} + +func TestASystemctlFailureShowsItsMessage(t *testing.T) { + fakeAgentServiceExit(t, true, 1) + + for name, f := range map[string]func(context.Context) error{"restartAgent": restartAgent, "restartStaleAgent": restartStaleAgent} { + var stderr strings.Builder + code := exitCode(f(context.Background()), &stderr) + if code != 1 || !strings.Contains(stderr.String(), "Failed to connect to bus") { + t.Errorf("%s: exit %d, stderr %q; want exit 1 and the systemctl message", name, code, stderr.String()) + } + } +} diff --git a/go.mod b/go.mod index cd772e0..b8b9ef0 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ toolchain go1.27.1 require ( github.com/fatih/color v1.19.0 + github.com/mattn/go-isatty v0.0.24 github.com/oklog/ulid/v2 v2.1.2 github.com/spf13/cobra v1.10.2 golang.org/x/mod v0.41.0 @@ -16,6 +17,5 @@ require ( require ( github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/mattn/go-colorable v0.1.15 // indirect - github.com/mattn/go-isatty v0.0.24 // indirect github.com/spf13/pflag v1.0.10 // indirect ) diff --git a/internal/release/release.go b/internal/release/release.go index 18b4192..e962ad9 100644 --- a/internal/release/release.go +++ b/internal/release/release.go @@ -16,6 +16,7 @@ import ( "path/filepath" "regexp" "runtime" + "syscall" "time" "github.com/flywp/server-cli/internal/version" @@ -226,6 +227,10 @@ func writeBinary(exe string, r io.Reader) (err error) { _ = tmp.Close() return fmt.Errorf("making binary executable: %w", err) } + if err = keepOwner(tmp, exe); err != nil { + _ = tmp.Close() + return fmt.Errorf("keeping the owner of the binary: %w", err) + } // Put the binary on the disk before the rename: after a power loss, a // renamed but empty binary would not start. if err = tmp.Sync(); err != nil { @@ -247,3 +252,21 @@ func writeBinary(exe string, r io.Reader) (err error) { return nil } + +// keepOwner gives the open file f the owner of exe. "sudo fly update" then +// keeps the binary of the agent with the server user, not with root. Only +// root can give a file to a different user; for other users the owner is +// already correct. It changes the open file, not a path: a path in a +// directory of an other user can be replaced by a link to a root file. +func keepOwner(f *os.File, exe string) error { + info, err := os.Stat(exe) + if err != nil { + return nil + } + st, ok := info.Sys().(*syscall.Stat_t) + if !ok || os.Geteuid() != 0 { + return nil + } + + return f.Chown(int(st.Uid), int(st.Gid)) +} diff --git a/internal/release/release_test.go b/internal/release/release_test.go index b9d8711..a967c2a 100644 --- a/internal/release/release_test.go +++ b/internal/release/release_test.go @@ -5,11 +5,13 @@ import ( "bytes" "compress/gzip" "context" + "io" "net/http" "net/http/httptest" "os" "path/filepath" "strings" + "syscall" "testing" ) @@ -232,3 +234,102 @@ func assertOnlyFile(t *testing.T, dir, name string) { t.Errorf("directory contains %q, want only %q", names, name) } } + +func TestReplaceBinaryKeepsTheOwner(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("only root can give a file to a different user") + } + + dir := t.TempDir() + exe := filepath.Join(dir, "fly") + if err := os.WriteFile(exe, []byte("old"), 0o755); err != nil { + t.Fatal(err) + } + // The binary of the agent belongs to the server user, for example 1000. + if err := os.Chown(exe, 1000, 1000); err != nil { + t.Fatal(err) + } + + if err := replaceBinary(exe, archive(t, map[string]string{"fly-linux-amd64": "new"}), "fly-linux-amd64"); err != nil { + t.Fatal(err) + } + + info, err := os.Stat(exe) + if err != nil { + t.Fatal(err) + } + if st := info.Sys().(*syscall.Stat_t); st.Uid != 1000 || st.Gid != 1000 { + t.Errorf("owner = %d:%d, want 1000:1000", st.Uid, st.Gid) + } +} + +// swapReader is an archive that, halfway, puts a link to victim in place of +// the temporary file in dir, like a hostile owner of dir. +type swapReader struct { + t *testing.T + r io.Reader + dir string + victim string + swapped bool +} + +func (s *swapReader) Read(p []byte) (int, error) { + if !s.swapped { + s.swapped = true + matches, _ := filepath.Glob(filepath.Join(s.dir, ".fly-update-*")) + for _, m := range matches { + if err := os.Rename(m, m+".moved"); err != nil { + s.t.Fatal(err) + } + if err := os.Symlink(s.victim, m); err != nil { + s.t.Fatal(err) + } + } + } + return s.r.Read(p) +} + +func TestReplaceBinaryDoesNotFollowALinkToAnOtherFile(t *testing.T) { + if os.Geteuid() != 0 { + t.Skip("the attack needs root to write the binary") + } + + dir := t.TempDir() + exe := filepath.Join(dir, "fly") + if err := os.WriteFile(exe, []byte("old"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Chown(exe, 1000, 1000); err != nil { + t.Fatal(err) + } + // A root file, for example /etc/shadow. + victim := filepath.Join(t.TempDir(), "shadow") + if err := os.WriteFile(victim, []byte("secret"), 0o640); err != nil { + t.Fatal(err) + } + + gz := &bytes.Buffer{} + tw := tar.NewWriter(gz) + _ = tw.WriteHeader(&tar.Header{Name: "fly-linux-amd64", Mode: 0o755, Size: 3, Typeflag: tar.TypeReg}) + _, _ = tw.Write([]byte("new")) + _ = tw.Close() + var zipped bytes.Buffer + zw := gzip.NewWriter(&zipped) + _, _ = zw.Write(gz.Bytes()) + _ = zw.Close() + + tr := tar.NewReader(func() io.Reader { r, _ := gzip.NewReader(&zipped); return r }()) + if _, err := tr.Next(); err != nil { + t.Fatal(err) + } + _ = writeBinary(exe, &swapReader{t: t, r: tr, dir: dir, victim: victim}) + + info, err := os.Stat(victim) + if err != nil { + t.Fatal(err) + } + st := info.Sys().(*syscall.Stat_t) + if info.Mode().Perm() != 0o640 || st.Uid != 0 { + t.Errorf("victim = %v owned by %d, want 0640 owned by root: root followed the link", info.Mode().Perm(), st.Uid) + } +} diff --git a/internal/service/service.go b/internal/service/service.go new file mode 100644 index 0000000..22f8b59 --- /dev/null +++ b/internal/service/service.go @@ -0,0 +1,85 @@ +// Package service controls the systemd service of the monitoring agent, +// fly-agent.service, for "fly update". +package service + +import ( + "context" + "errors" + "fmt" + "io/fs" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "time" +) + +const unit = "fly-agent" + +// UnitPath is the unit file that the FlyWP installer writes. procRoot is the +// proc file system. Tests replace them. +var ( + UnitPath = "/etc/systemd/system/fly-agent.service" + procRoot = "/proc" +) + +// Installed reports whether the server has the monitoring agent. +func Installed() bool { + _, err := os.Stat(UnitPath) + return err == nil +} + +// Stale reports whether the agent runs a binary that was replaced: after a +// rename over the binary, the kernel shows the old one as "(deleted)". +func Stale(ctx context.Context) (bool, error) { + out, err := systemctl(ctx, "show", "--property=MainPID", "--value", unit) + if err != nil { + return false, err + } + + pid, err := strconv.Atoi(strings.TrimSpace(out)) + if err != nil { + return false, fmt.Errorf("reading the process id of %s: %q", unit, out) + } + if pid == 0 { + // The agent does not run: systemd starts the binary on the disk. + return false, nil + } + + exe, err := os.Readlink(filepath.Join(procRoot, strconv.Itoa(pid), "exe")) + if errors.Is(err, fs.ErrNotExist) { + // The process ended after systemctl showed it, for example for its + // own update or restart. systemd starts the binary on the disk. + return false, nil + } + if err != nil { + return false, fmt.Errorf("reading the binary of the agent: %w", err) + } + + return strings.HasSuffix(exe, " (deleted)"), nil +} + +// Restart restarts the agent if it runs, so that it runs the binary on the +// disk. An agent that an administrator stopped stays stopped. +func Restart(ctx context.Context) error { + _, err := systemctl(ctx, "try-restart", unit) + return err +} + +// systemctlTimeout is longer than the default stop timeout of systemd (90 s). +const systemctlTimeout = 2 * time.Minute + +func systemctl(ctx context.Context, args ...string) (string, error) { + ctx, cancel := context.WithTimeout(ctx, systemctlTimeout) + defer cancel() + + out, err := exec.CommandContext(ctx, "systemctl", args...).CombinedOutput() + if err != nil { + // %v, not %w: an *exec.ExitError would tell fly that the child + // already showed its error, and the output here would be lost. + return "", fmt.Errorf("systemctl %s: %v: %s", strings.Join(args, " "), err, strings.TrimSpace(string(out))) + } + + return string(out), nil +} diff --git a/internal/service/service_test.go b/internal/service/service_test.go new file mode 100644 index 0000000..7b22e6d --- /dev/null +++ b/internal/service/service_test.go @@ -0,0 +1,141 @@ +package service + +import ( + "context" + "os" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// fakeSystemctl puts a systemctl command first in PATH. It logs its arguments +// and prints mainPID for "show". +func fakeSystemctl(t *testing.T, mainPID string, exit int) (log string) { + t.Helper() + + dir := t.TempDir() + log = filepath.Join(t.TempDir(), "calls") + script := "#!/bin/sh\nprintf '%s\\n' \"$*\" >> " + log + "\n" + + "[ \"$1\" = show ] && echo " + mainPID + "\n" + + "exit " + strconv.Itoa(exit) + "\n" + if err := os.WriteFile(filepath.Join(dir, "systemctl"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + return log +} + +// fakeProc makes a proc directory in which process 4242 runs exe. +func fakeProc(t *testing.T, exe string) { + t.Helper() + + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "4242"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(exe, filepath.Join(dir, "4242", "exe")); err != nil { + t.Fatal(err) + } + + old := procRoot + procRoot = dir + t.Cleanup(func() { procRoot = old }) +} + +func calls(t *testing.T, log string) string { + t.Helper() + data, err := os.ReadFile(log) + if err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + return strings.TrimSpace(string(data)) +} + +func TestInstalled(t *testing.T) { + old := UnitPath + t.Cleanup(func() { UnitPath = old }) + + UnitPath = filepath.Join(t.TempDir(), "fly-agent.service") + if Installed() { + t.Error("Installed() = true without the unit file") + } + if err := os.WriteFile(UnitPath, []byte("[Unit]\n"), 0o644); err != nil { + t.Fatal(err) + } + if !Installed() { + t.Error("Installed() = false with the unit file") + } +} + +func TestStale(t *testing.T) { + tests := []struct { + name, mainPID, exe string + want bool + }{ + {"replaced binary", "4242", "/home/fly/.fly/bin/fly (deleted)", true}, + {"current binary", "4242", "/home/fly/.fly/bin/fly", false}, + {"agent not running", "0", "", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + log := fakeSystemctl(t, tt.mainPID, 0) + if tt.exe != "" { + fakeProc(t, tt.exe) + } + + got, err := Stale(context.Background()) + if err != nil { + t.Fatal(err) + } + if got != tt.want { + t.Errorf("Stale() = %v, want %v", got, tt.want) + } + if c := calls(t, log); c != "show --property=MainPID --value fly-agent" { + t.Errorf("systemctl calls = %q", c) + } + }) + } +} + +func TestStaleWhenTheProcessJustEnded(t *testing.T) { + fakeSystemctl(t, "4242", 0) + // No /proc/4242: the process ended after systemctl showed it. + old := procRoot + procRoot = t.TempDir() + t.Cleanup(func() { procRoot = old }) + + stale, err := Stale(context.Background()) + if err != nil || stale { + t.Errorf("Stale() = %v, %v; want false without an error", stale, err) + } +} + +func TestStaleErrors(t *testing.T) { + fakeSystemctl(t, "not-a-number", 0) + if _, err := Stale(context.Background()); err == nil { + t.Error("Stale() = nil error, want an error for a bad process id") + } + + fakeSystemctl(t, "4242", 1) + if _, err := Stale(context.Background()); err == nil || !strings.Contains(err.Error(), "systemctl show") { + t.Errorf("Stale() error = %v, want the systemctl error", err) + } +} + +func TestRestart(t *testing.T) { + log := fakeSystemctl(t, "0", 0) + if err := Restart(context.Background()); err != nil { + t.Fatal(err) + } + if c := calls(t, log); c != "try-restart fly-agent" { + t.Errorf("systemctl calls = %q, want try-restart fly-agent: a stopped agent stays stopped", c) + } + + fakeSystemctl(t, "0", 1) + if err := Restart(context.Background()); err == nil { + t.Error("Restart() = nil error, want the systemctl error") + } +}