diff --git a/cli/cmd/bastion_watchdog.go b/cli/cmd/bastion_watchdog.go new file mode 100644 index 00000000..82d4db00 --- /dev/null +++ b/cli/cmd/bastion_watchdog.go @@ -0,0 +1,48 @@ +package cmd + +import ( + "fmt" + "os" + "syscall" + + "github.com/dreadnode/dreadgoad/internal/azure" + "github.com/spf13/cobra" +) + +const bastionParentLifetimeFD = 3 + +// bastionWatchdogCmd is an internal subprocess entry point used by the Azure +// provision tunnel. It stays hidden because its inherited file descriptor is +// meaningful only when the command is launched by StartProvisionTunnel. +var bastionWatchdogCmd = &cobra.Command{ + Use: "__bastion-watchdog [args...]", + Hidden: true, + DisableFlagParsing: true, + Args: cobra.MinimumNArgs(1), + RunE: func(_ *cobra.Command, args []string) error { + parentLifetime, err := openBastionParentLifetime(bastionParentLifetimeFD) + if err != nil { + return err + } + // ExtraFiles starts at fd 3. Keep the descriptor private to this + // watchdog; the supervised az process must not inherit it. + syscall.CloseOnExec(bastionParentLifetimeFD) + return azure.RunBastionWatchdog(parentLifetime, args) + }, +} + +func openBastionParentLifetime(fd uintptr) (*os.File, error) { + parentLifetime := os.NewFile(fd, "dreadgoad-parent-lifetime") + if parentLifetime == nil { + return nil, fmt.Errorf("open parent lifetime descriptor %d", fd) + } + if _, err := parentLifetime.Stat(); err != nil { + _ = parentLifetime.Close() + return nil, fmt.Errorf("validate parent lifetime descriptor %d: %w", fd, err) + } + return parentLifetime, nil +} + +func init() { + rootCmd.AddCommand(bastionWatchdogCmd) +} diff --git a/cli/cmd/bastion_watchdog_test.go b/cli/cmd/bastion_watchdog_test.go new file mode 100644 index 00000000..d15eec9e --- /dev/null +++ b/cli/cmd/bastion_watchdog_test.go @@ -0,0 +1,29 @@ +package cmd + +import ( + "os" + "strings" + "testing" +) + +func TestOpenBastionParentLifetimeRejectsClosedDescriptor(t *testing.T) { + file, err := os.CreateTemp(t.TempDir(), "closed-descriptor") + if err != nil { + t.Fatalf("create temporary file: %v", err) + } + fd := file.Fd() + if err := file.Close(); err != nil { + t.Fatalf("close temporary file: %v", err) + } + + parentLifetime, err := openBastionParentLifetime(fd) + if err == nil { + if parentLifetime != nil { + _ = parentLifetime.Close() + } + t.Fatal("openBastionParentLifetime() error = nil, want invalid descriptor error") + } + if !strings.Contains(err.Error(), "validate parent lifetime descriptor") { + t.Fatalf("openBastionParentLifetime() error = %q, want validation error", err) + } +} diff --git a/cli/internal/azure/provision_tunnel.go b/cli/internal/azure/provision_tunnel.go index 7bcd50ef..4e756b7d 100644 --- a/cli/internal/azure/provision_tunnel.go +++ b/cli/internal/azure/provision_tunnel.go @@ -3,6 +3,7 @@ package azure import ( "context" "fmt" + "io" "net" "os" "os/exec" @@ -21,10 +22,22 @@ import ( // controller sits in the same VNet as the GOAD VMs, so SOCKS-routed WinRM // traffic reaches private 5985 listeners that the laptop can't touch directly. type ProvisionTunnel struct { - socks *ludus.SOCKSTunnel - bastionCmd *exec.Cmd - localPort int - closeOnce sync.Once + socks *ludus.SOCKSTunnel + bastionProcess *bastionTunnelProcess + localPort int + closeOnce sync.Once +} + +// bastionTunnelProcess is the parent-side handle for the watchdog subprocess. +// The write end is deliberately held only by dreadgoad: normal cleanup closes +// it explicitly, while abrupt process death makes the kernel close it. Either +// event wakes the watchdog and causes it to reap the complete az process group. +type bastionTunnelProcess struct { + cmd *exec.Cmd + parentWrite *os.File + closeOnce sync.Once + done chan struct{} + waitErr error } // ProxyURL returns the SOCKS5 proxy URL Ansible's psrp connection plugin @@ -40,59 +53,151 @@ func (t *ProvisionTunnel) SOCKSAddr() string { // Close terminates the SOCKS5 listener, the underlying SSH connection to the // controller, and the spawned `az network bastion tunnel` subprocess tree. // -// Teardown runs exactly once even if Close is called concurrently. That is not -// cosmetic: killBastionTunnel reaps via cmd.Wait, and two Wait calls racing on -// one exec.Cmd is a data race the detector flags. Callers reach Close through -// several paths (winrmRunner.close, the deferred Drain in `validate`, the -// deferred socksTunnel.Close in `provision`), so the guarantee lives here -// rather than depending on every caller staying serialized. +// Teardown runs exactly once even if Close is called concurrently. Callers +// reach Close through several paths (winrmRunner.close, the deferred Drain in +// `validate`, the deferred socksTunnel.Close in `provision`), so pipe closure +// and the wait for watchdog exit are serialized here rather than depending on +// every caller staying ordered. func (t *ProvisionTunnel) Close() { t.closeOnce.Do(func() { if t.socks != nil { t.socks.Close() } - killBastionTunnel(t.bastionCmd) + killBastionTunnel(t.bastionProcess) }) } -// killBastionTunnel reaps the whole `az network bastion tunnel` process tree. -// The `az` entry point is a shell wrapper that *spawns* a `python -m azure.cli` -// child, so killing only cmd.Process (the wrapper) leaves that child running — -// it reparents to init/launchd and the Bastion tunnel leaks. We start the -// command in its own process group (Setpgid) and signal the whole group here. -// killGracePeriod is how long the tunnel gets to honor SIGTERM before the -// group is SIGKILLed. +// killGracePeriod is how long the tunnel process group gets to honor SIGTERM +// before the watchdog escalates to SIGKILL. const killGracePeriod = 500 * time.Millisecond -func killBastionTunnel(cmd *exec.Cmd) { - if cmd == nil || cmd.Process == nil { +func (p *bastionTunnelProcess) signalParentExit() { + if p == nil { return } - pid := cmd.Process.Pid + p.closeOnce.Do(func() { + if p.parentWrite != nil { + _ = p.parentWrite.Close() + } + }) +} + +// killBastionTunnel tells the watchdog to terminate the az process group and +// waits for the watchdog to reap it. Closing parentWrite is the same event the +// watchdog observes automatically if dreadgoad is killed without running its +// deferred cleanup. +func killBastionTunnel(p *bastionTunnelProcess) { + if p == nil { + return + } + p.signalParentExit() + if p.done != nil { + <-p.done + } else if p.cmd != nil && p.cmd.Process != nil { + _ = p.cmd.Wait() + } +} + +// RunBastionWatchdog supervises command until it exits or parentLifetime is +// closed. It runs in a separate process so it survives a SIGKILL of dreadgoad. +// The command gets its own process group, allowing the watchdog to terminate +// both the az shell wrapper and the Python child without signalling itself. +// +// This is exported only for the hidden __bastion-watchdog CLI command. +func RunBastionWatchdog(parentLifetime *os.File, command []string) error { + if parentLifetime == nil { + return fmt.Errorf("parent lifetime pipe is unavailable") + } + defer func() { + _ = parentLifetime.Close() + }() + if len(command) == 0 { + return fmt.Errorf("watchdog command is required") + } + + // The tunnel must not inherit the liveness descriptor. Only the watchdog + // should own its read end; otherwise a descendant could keep it open after + // the watchdog exits and obscure the ownership contract. + syscall.CloseOnExec(int(parentLifetime.Fd())) + + cmd := exec.Command(command[0], command[1:]...) + cmd.Stdin = os.Stdin + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := cmd.Start(); err != nil { + return fmt.Errorf("start watched bastion tunnel: %w", err) + } + pgid, err := syscall.Getpgid(cmd.Process.Pid) + if err != nil || pgid != cmd.Process.Pid { + _ = cmd.Process.Kill() + _ = cmd.Wait() + return fmt.Errorf("establish bastion tunnel process group: pgid=%d pid=%d err=%v", pgid, cmd.Process.Pid, err) + } - // Reap in the background so the wrapper leaves zombie state the moment it - // dies. This is what makes the polling below work at all: an unreaped - // zombie still answers kill(pid, 0), so waiting on the group without - // concurrently reaping would never observe the exit. reaped := make(chan struct{}) + var waitErr error go func() { - _ = cmd.Wait() + waitErr = cmd.Wait() close(reaped) }() + parentGone := make(chan struct{}, 1) + go func() { + _, _ = io.Copy(io.Discard, parentLifetime) + parentGone <- struct{}{} + }() + + // The az launcher can exit while a descendant continues the tunnel in the + // same process group. Keep the watchdog alive until the entire group exits, + // not merely until cmd.Wait reports that the group leader is gone. + poll := time.NewTicker(25 * time.Millisecond) + defer poll.Stop() + commandDone := (<-chan struct{})(reaped) + leaderExited := false + for { + select { + case <-commandDone: + leaderExited = true + commandDone = nil + if !processGroupAlive(pgid) { + return waitErr + } + case <-parentGone: + terminateKnownProcessGroup(pgid, reaped) + return nil + case <-poll.C: + if leaderExited && !processGroupAlive(pgid) { + return waitErr + } + } + } +} + +// terminateProcessGroup reaps a command and every descendant that retained its +// process group. The az entry point is a shell wrapper that spawns Python, so a +// single-process kill is insufficient. +func terminateProcessGroup(cmd *exec.Cmd, reaped <-chan struct{}) { + if cmd == nil || cmd.Process == nil { + return + } + pid := cmd.Process.Pid // pgid == pid proves Setpgid took effect. Without that check, a cmd started - // without SysProcAttr.Setpgid reports *our* group, and the negative-pid kill - // below would take down dreadgoad itself along with its foreground group. + // without SysProcAttr.Setpgid reports the watchdog's group, and the + // negative-pid kill below would terminate the watchdog itself. pgid, err := syscall.Getpgid(pid) if err != nil || pgid != pid { _ = cmd.Process.Kill() <-reaped return } + terminateKnownProcessGroup(pgid, reaped) +} +func terminateKnownProcessGroup(pgid int, reaped <-chan struct{}) { // Negative pid targets the entire process group (wrapper + python). _ = syscall.Kill(-pgid, syscall.SIGTERM) - if !awaitGroupExit(pgid, reaped, killGracePeriod) { + if !awaitGroupExit(pgid, killGracePeriod) { _ = syscall.Kill(-pgid, syscall.SIGKILL) } <-reaped @@ -102,16 +207,11 @@ func killBastionTunnel(cmd *exec.Cmd) { // Returns true if the group went away on its own, letting the caller skip the // SIGKILL escalation — a tunnel that honors SIGTERM promptly costs a few // milliseconds here instead of the full grace period. -func awaitGroupExit(pgid int, reaped <-chan struct{}, timeout time.Duration) bool { +func awaitGroupExit(pgid int, timeout time.Duration) bool { deadline := time.Now().Add(timeout) for { - select { - case <-reaped: - // Wrapper is reaped, so a surviving group means real stragglers. - if syscall.Kill(-pgid, 0) == syscall.ESRCH { - return true - } - default: + if !processGroupAlive(pgid) { + return true } if !time.Now().Before(deadline) { return false @@ -120,6 +220,62 @@ func awaitGroupExit(pgid int, reaped <-chan struct{}, timeout time.Duration) boo } } +func processGroupAlive(pgid int) bool { + return syscall.Kill(-pgid, 0) != syscall.ESRCH +} + +func startBastionTunnelProcess(ctx context.Context, command []string) (*bastionTunnelProcess, error) { + if len(command) == 0 { + return nil, fmt.Errorf("watchdog command is required") + } + + parentRead, parentWrite, err := os.Pipe() + if err != nil { + return nil, fmt.Errorf("create watchdog pipe: %w", err) + } + + executable, err := os.Executable() + if err != nil { + _ = parentRead.Close() + _ = parentWrite.Close() + return nil, fmt.Errorf("locate dreadgoad executable: %w", err) + } + + args := append([]string{"__bastion-watchdog"}, command...) + cmd := exec.CommandContext(ctx, executable, args...) + cmd.Stdout = os.Stderr + cmd.Stderr = os.Stderr + cmd.ExtraFiles = []*os.File{parentRead} + // The watchdog must survive the console killing dreadgoad's process group + // long enough to observe pipe EOF and reap its own az child group. + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + + process := &bastionTunnelProcess{ + cmd: cmd, + parentWrite: parentWrite, + done: make(chan struct{}), + } + // Context cancellation follows the same graceful watchdog path as Close. + // WaitDelay is only a last-resort guard against a broken watchdog. + cmd.Cancel = func() error { + process.signalParentExit() + return nil + } + cmd.WaitDelay = 2 * time.Second + + if err := cmd.Start(); err != nil { + _ = parentRead.Close() + process.signalParentExit() + return nil, fmt.Errorf("start bastion watchdog: %w", err) + } + _ = parentRead.Close() + go func() { + process.waitErr = cmd.Wait() + close(process.done) + }() + return process, nil +} + // StartProvisionTunnel discovers the in-VNet controller, opens a Bastion port // tunnel to it, then layers a Go SOCKS5 listener on top whose dials are routed // via SSH through the controller. Caller MUST Close() to release resources. @@ -147,35 +303,20 @@ func StartProvisionTunnel(ctx context.Context, c *Client, env string) (*Provisio return nil, fmt.Errorf("controller ephemeral key not found at expected path; was 'infra apply' run?") } - // exec.CommandContext so a cancelled ctx (Ctrl+C / SIGTERM propagated to a - // signal-aware root context) kills the tunnel even if Close() is skipped. - cmd := exec.CommandContext(ctx, "az", "network", "bastion", "tunnel", + process, err := startBastionTunnelProcess(ctx, []string{ + "az", "network", "bastion", "tunnel", "--name", bastion.Name, "--resource-group", bastion.ResourceGroup, "--target-resource-id", controller.ID, "--resource-port", "22", - "--port", strconv.Itoa(localPort)) - cmd.Stdout = os.Stderr - cmd.Stderr = os.Stderr - // Own process group so Close() (and ctx-cancel) can reap the wrapper *and* - // its python child as one unit — see killBastionTunnel. - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - // Kill the whole group on ctx-cancel, not just the wrapper process. Same - // pgid == pid guard as killBastionTunnel: never negative-kill a group we - // haven't confirmed belongs to the child. - cmd.Cancel = func() error { - pid := cmd.Process.Pid - if pgid, err := syscall.Getpgid(pid); err == nil && pgid == pid { - return syscall.Kill(-pgid, syscall.SIGKILL) - } - return cmd.Process.Kill() - } - if err := cmd.Start(); err != nil { - return nil, fmt.Errorf("start bastion tunnel: %w", err) + "--port", strconv.Itoa(localPort), + }) + if err != nil { + return nil, err } - if err := waitForLocalPort(ctx, localPort, 60*time.Second); err != nil { - killBastionTunnel(cmd) + if err := waitForLocalPort(ctx, process, localPort, 60*time.Second); err != nil { + killBastionTunnel(process) return nil, fmt.Errorf("bastion tunnel never came up on :%d: %w", localPort, err) } @@ -189,11 +330,11 @@ func StartProvisionTunnel(ctx context.Context, c *Client, env string) (*Provisio } socks, err := ludus.StartSOCKSTunnel(sshCfg) if err != nil { - killBastionTunnel(cmd) + killBastionTunnel(process) return nil, fmt.Errorf("start SOCKS5 over controller: %w", err) } - return &ProvisionTunnel{socks: socks, bastionCmd: cmd, localPort: localPort}, nil + return &ProvisionTunnel{socks: socks, bastionProcess: process, localPort: localPort}, nil } // findControllerInstance locates the Ansible controller VM (Role=AnsibleController @@ -242,13 +383,18 @@ func pickFreePort() (int, error) { return port, nil } -func waitForLocalPort(ctx context.Context, port int, timeout time.Duration) error { +func waitForLocalPort(ctx context.Context, process *bastionTunnelProcess, port int, timeout time.Duration) error { deadline := time.Now().Add(timeout) addr := fmt.Sprintf("127.0.0.1:%d", port) for time.Now().Before(deadline) { select { case <-ctx.Done(): return ctx.Err() + case <-process.done: + if process.waitErr != nil { + return fmt.Errorf("bastion watchdog exited before tunnel became ready: %w", process.waitErr) + } + return fmt.Errorf("bastion watchdog exited before tunnel became ready") default: } conn, err := net.DialTimeout("tcp", addr, 2*time.Second) diff --git a/cli/internal/azure/provision_tunnel_test.go b/cli/internal/azure/provision_tunnel_test.go index ab2ef021..e1357d30 100644 --- a/cli/internal/azure/provision_tunnel_test.go +++ b/cli/internal/azure/provision_tunnel_test.go @@ -4,8 +4,11 @@ package azure import ( "bufio" + "context" "os" "os/exec" + "path/filepath" + "runtime" "strconv" "strings" "sync" @@ -14,19 +17,31 @@ import ( "time" ) +func TestStartBastionTunnelProcessRequiresCommand(t *testing.T) { + process, err := startBastionTunnelProcess(context.Background(), nil) + if err == nil { + if process != nil { + killBastionTunnel(process) + } + t.Fatal("startBastionTunnelProcess() error = nil, want missing command error") + } + if !strings.Contains(err.Error(), "watchdog command is required") { + t.Fatalf("startBastionTunnelProcess() error = %q, want missing command error", err) + } +} + // processAlive reports whether pid still exists (signal 0 probes without // delivering). A reaped process yields ESRCH. func processAlive(pid int) bool { return syscall.Kill(pid, 0) == nil } -// TestKillBastionTunnelReapsChildTree is the regression guard for the tunnel +// TestTerminateProcessGroupReapsChildTree is the regression guard for the tunnel // leak: the real `az network bastion tunnel` is a shell wrapper that spawns a -// python child, so killing only the wrapper leaves the child (and its tunnel) +// Python child, so killing only the wrapper leaves the child (and its tunnel) // running. We reproduce that topology with `sh` (wrapper) spawning a -// backgrounded `sleep` (child), then assert killBastionTunnel reaps BOTH by -// signalling the whole process group. -func TestKillBastionTunnelReapsChildTree(t *testing.T) { +// backgrounded `sleep` (child), then assert terminateProcessGroup reaps BOTH. +func TestTerminateProcessGroupReapsChildTree(t *testing.T) { // sh backgrounds a long sleep (the "python child"), prints its PID, then // waits — mirroring a wrapper that outlives nothing of its own but holds a // child that must die with it. @@ -57,7 +72,12 @@ func TestKillBastionTunnelReapsChildTree(t *testing.T) { t.Fatalf("precondition failed: child %d not alive after start", childPID) } - killBastionTunnel(cmd) + reaped := make(chan struct{}) + go func() { + _ = cmd.Wait() + close(reaped) + }() + terminateProcessGroup(cmd, reaped) // The grandchild reparents to init and is reaped shortly after SIGKILL. deadline := time.Now().Add(5 * time.Second) @@ -67,29 +87,40 @@ func TestKillBastionTunnelReapsChildTree(t *testing.T) { } time.Sleep(50 * time.Millisecond) } - t.Fatalf("child process %d survived killBastionTunnel — tunnel would leak", childPID) + t.Fatalf("child process %d survived terminateProcessGroup — tunnel would leak", childPID) } -// TestKillBastionTunnelNilSafe guards the early-error paths that may call -// Close() before the command was started. -func TestKillBastionTunnelNilSafe(t *testing.T) { - killBastionTunnel(nil) - killBastionTunnel(&exec.Cmd{}) // Process == nil +// TestTerminateProcessGroupNilSafe guards early-error paths before start. +func TestTerminateProcessGroupNilSafe(t *testing.T) { + terminateProcessGroup(nil, nil) + terminateProcessGroup(&exec.Cmd{}, nil) // Process == nil } -// TestProvisionTunnelCloseIsRaceFree pins the closeOnce guard. killBastionTunnel -// reaps with cmd.Wait, and two Wait calls on one exec.Cmd is a data race — so -// concurrent Close must collapse to a single teardown. Run under -race; without -// closeOnce the detector reports a write/write race inside os/exec.(*Cmd).Wait. +// TestProvisionTunnelCloseIsRaceFree pins the closeOnce guards. Concurrent +// Close calls must collapse to one pipe close and one wait for watchdog exit. func TestProvisionTunnelCloseIsRaceFree(t *testing.T) { - cmd := exec.Command("sleep", "120") - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + parentRead, parentWrite, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + cmd := exec.Command("sh", "-c", "cat <&3") + cmd.ExtraFiles = []*os.File{parentRead} if err := cmd.Start(); err != nil { t.Fatalf("start: %v", err) } + _ = parentRead.Close() // socks stays nil: this exercises the subprocess half, which is where the // race lives. - tunnel := &ProvisionTunnel{bastionCmd: cmd} + process := &bastionTunnelProcess{ + cmd: cmd, + parentWrite: parentWrite, + done: make(chan struct{}), + } + go func() { + process.waitErr = cmd.Wait() + close(process.done) + }() + tunnel := &ProvisionTunnel{bastionProcess: process} var wg sync.WaitGroup for range 4 { @@ -106,6 +137,200 @@ func TestProvisionTunnelCloseIsRaceFree(t *testing.T) { } } +const ( + parentDeathRoleEnv = "DREADGOAD_TEST_PARENT_DEATH_ROLE" + parentDeathProcessFile = "DREADGOAD_TEST_PARENT_DEATH_PROCESS_FILE" +) + +func runParentDeathWatchdogProcess() { + parentLifetime := os.NewFile(3, "test-parent-lifetime") + err := RunBastionWatchdog(parentLifetime, []string{ + "sh", "-c", + `sleep 120 & printf '%s %s\n' "$$" "$!" > "$DREADGOAD_TEST_PARENT_DEATH_PROCESS_FILE"; wait`, + }) + if err != nil { + os.Exit(4) + } + os.Exit(0) +} + +func runParentDeathParentProcess() { + parentRead, parentWrite, err := os.Pipe() + if err != nil { + os.Exit(5) + } + executable, err := os.Executable() + if err != nil { + os.Exit(6) + } + watchdog := exec.Command(executable, "-test.run=^TestBastionWatchdogReapsOnParentDeath$") + watchdog.Env = append(os.Environ(), parentDeathRoleEnv+"=watchdog") + watchdog.ExtraFiles = []*os.File{parentRead} + watchdog.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + if err := watchdog.Start(); err != nil { + os.Exit(7) + } + _ = parentRead.Close() + watchdogFile := os.Getenv(parentDeathProcessFile) + ".watchdog" + if err := os.WriteFile(watchdogFile, []byte(strconv.Itoa(watchdog.Process.Pid)), 0o600); err != nil { + os.Exit(8) + } + for { + // Keep the writer live until the outer test kills this process. Its + // kernel-driven close is the event under test. + runtime.KeepAlive(parentWrite) + time.Sleep(time.Second) + } +} + +// TestBastionWatchdogReapsOnParentDeath exercises the failure mode that +// in-process cleanup cannot cover. The outer test starts a simulated dreadgoad +// parent, which starts the watchdog, which starts a shell wrapper and child. +// SIGKILLing the simulated parent closes the liveness pipe; the independently +// running watchdog must then terminate and reap the complete command group. +func TestBastionWatchdogReapsOnParentDeath(t *testing.T) { + role := os.Getenv(parentDeathRoleEnv) + if role == "watchdog" { + runParentDeathWatchdogProcess() + } + + if role == "parent" { + runParentDeathParentProcess() + } + + tempDir := t.TempDir() + processFile := filepath.Join(tempDir, "processes") + executable, err := os.Executable() + if err != nil { + t.Fatalf("locate test executable: %v", err) + } + parent := exec.Command(executable, "-test.run=^TestBastionWatchdogReapsOnParentDeath$") + parent.Env = append(os.Environ(), + parentDeathRoleEnv+"=parent", + parentDeathProcessFile+"="+processFile, + ) + if err := parent.Start(); err != nil { + t.Fatalf("start simulated parent: %v", err) + } + defer func() { + if parent.ProcessState == nil { + _ = parent.Process.Kill() + _ = parent.Wait() + } + }() + + commandPIDs := waitForPIDFile(t, processFile, 2) + watchdogPIDs := waitForPIDFile(t, processFile+".watchdog", 1) + allChildren := make([]int, 0, len(watchdogPIDs)+len(commandPIDs)) + allChildren = append(allChildren, watchdogPIDs...) + allChildren = append(allChildren, commandPIDs...) + for _, pid := range allChildren { + if !processAlive(pid) { + t.Fatalf("precondition failed: process %d not alive", pid) + } + } + + if err := parent.Process.Kill(); err != nil { + t.Fatalf("SIGKILL simulated parent: %v", err) + } + _ = parent.Wait() + + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + alive := false + for _, pid := range allChildren { + alive = alive || processAlive(pid) + } + if !alive { + return + } + time.Sleep(50 * time.Millisecond) + } + t.Fatalf("watchdog or tunnel processes survived parent death: %v", allChildren) +} + +// TestBastionWatchdogTracksGroupAfterLeaderExit mirrors the observed orphan +// topology: the process-group leader is gone but a descendant still owns the +// tunnel. The watchdog must continue supervising the group and reap that +// descendant when the parent-lifetime pipe closes. +func TestBastionWatchdogTracksGroupAfterLeaderExit(t *testing.T) { + processFile := filepath.Join(t.TempDir(), "child-pid") + t.Setenv(parentDeathProcessFile, processFile) + parentRead, parentWrite, err := os.Pipe() + if err != nil { + t.Fatalf("pipe: %v", err) + } + defer func() { + _ = parentWrite.Close() + }() + + done := make(chan error, 1) + go func() { + done <- RunBastionWatchdog(parentRead, []string{ + "sh", "-c", + `sleep 120 & printf '%s\n' "$!" > "$DREADGOAD_TEST_PARENT_DEATH_PROCESS_FILE"; exit 0`, + }) + }() + + childPID := waitForPIDFile(t, processFile, 1)[0] + childPGID, err := syscall.Getpgid(childPID) + if err != nil { + t.Fatalf("get descendant process group: %v", err) + } + defer func() { + _ = syscall.Kill(-childPGID, syscall.SIGKILL) + }() + time.Sleep(100 * time.Millisecond) // let the short-lived group leader exit + select { + case err := <-done: + t.Fatalf("watchdog exited with descendant %d still alive: %v", childPID, err) + default: + } + if !processAlive(childPID) { + t.Fatalf("precondition failed: descendant %d exited early", childPID) + } + + if err := parentWrite.Close(); err != nil { + t.Fatalf("close parent lifetime: %v", err) + } + select { + case err := <-done: + if err != nil { + t.Fatalf("watchdog cleanup: %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("watchdog did not exit after parent lifetime closed") + } + if processAlive(childPID) { + t.Fatalf("descendant %d survived watchdog cleanup", childPID) + } +} + +func waitForPIDFile(t *testing.T, path string, count int) []int { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for time.Now().Before(deadline) { + data, err := os.ReadFile(path) + if err == nil { + fields := strings.Fields(string(data)) + if len(fields) == count { + pids := make([]int, 0, count) + for _, field := range fields { + pid, err := strconv.Atoi(field) + if err != nil { + t.Fatalf("parse pid %q from %s: %v", field, path, err) + } + pids = append(pids, pid) + } + return pids + } + } + time.Sleep(20 * time.Millisecond) + } + t.Fatalf("timed out waiting for %d pids in %s", count, path) + return nil +} + // pgidGuardEnv re-enters this test binary as a subprocess for the guard check // below. A regression there SIGKILLs the caller's whole process group, so the // dangerous half runs isolated in its own group rather than taking down @@ -115,12 +340,12 @@ const pgidGuardEnv = "DREADGOAD_TEST_PGID_GUARD_CHILD" // pgidGuardOK is the exit code the child reports when it survived the kill. const pgidGuardOK = 7 -// TestKillBastionTunnelSpareOwnProcessGroup pins the `pgid == pid` guard in -// killBastionTunnel. Given a command started WITHOUT SysProcAttr.Setpgid, +// TestTerminateProcessGroupSparesOwnProcessGroup pins the `pgid == pid` guard +// in terminateProcessGroup. Given a command started WITHOUT Setpgid, // syscall.Getpgid returns the *caller's* group — so an unguarded // kill(-pgid, SIGKILL) would take down dreadgoad itself. The guard must detect // that and fall back to killing only the single process. -func TestKillBastionTunnelSpareOwnProcessGroup(t *testing.T) { +func TestTerminateProcessGroupSparesOwnProcessGroup(t *testing.T) { if os.Getenv(pgidGuardEnv) == "1" { // Detach into our own process group so a regression's group-kill is // contained to this subprocess. @@ -133,7 +358,12 @@ func TestKillBastionTunnelSpareOwnProcessGroup(t *testing.T) { if err := victim.Start(); err != nil { os.Exit(4) } - killBastionTunnel(victim) + reaped := make(chan struct{}) + go func() { + _ = victim.Wait() + close(reaped) + }() + terminateProcessGroup(victim, reaped) // Still executing => the guard held and we did not signal our own group. os.Exit(pgidGuardOK) } @@ -142,7 +372,7 @@ func TestKillBastionTunnelSpareOwnProcessGroup(t *testing.T) { if err != nil { t.Fatalf("locate test binary: %v", err) } - cmd := exec.Command(exe, "-test.run=TestKillBastionTunnelSpareOwnProcessGroup") + cmd := exec.Command(exe, "-test.run=TestTerminateProcessGroupSparesOwnProcessGroup") cmd.Env = append(os.Environ(), pgidGuardEnv+"=1") err = cmd.Run() @@ -151,7 +381,7 @@ func TestKillBastionTunnelSpareOwnProcessGroup(t *testing.T) { return // guard held } if code == -1 { - t.Fatalf("subprocess was killed by a signal (%v) — killBastionTunnel "+ + t.Fatalf("subprocess was killed by a signal (%v) — terminateProcessGroup "+ "signalled its own process group; the pgid == pid guard is missing", cmd.ProcessState) } t.Fatalf("subprocess exited %d (err=%v), want %d", code, err, pgidGuardOK)