diff --git a/internal/cli/inherited_fds_darwin.go b/internal/cli/inherited_fds_darwin.go new file mode 100644 index 000000000..195c53ce0 --- /dev/null +++ b/internal/cli/inherited_fds_darwin.go @@ -0,0 +1,20 @@ +//go:build darwin + +package cli + +// sealInheritedDescriptors keeps every descriptor this process inherited out +// of the processes it starts, by marking each one close-on-exec. It is the +// Linux one's (inherited_fds_linux.go) for macOS, where there is no +// close_range: the process's own list of descriptors is /dev/fd, and each is +// marked as the Linux fallback marks /proc/self/fd's. +// +// /dev/fd is a filesystem macOS always mounts. Where it cannot be read this +// fails closed: no other source says which descriptors are open, and the +// descriptor limit is no bound, since a parent can lower it after opening one +// above it (Copilot on #812). +func sealInheritedDescriptors() error { + return sealListedDescriptors(devFD) +} + +// devFD is where macOS lists the descriptors a process holds. +const devFD = "/dev/fd" diff --git a/internal/cli/inherited_fds_darwin_test.go b/internal/cli/inherited_fds_darwin_test.go new file mode 100644 index 000000000..5eb728937 --- /dev/null +++ b/internal/cli/inherited_fds_darwin_test.go @@ -0,0 +1,76 @@ +//go:build darwin + +package cli + +import ( + "os" + "os/exec" + "strconv" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/unix" +) + +// The descriptor number is high on purpose: a child picks the lowest free one +// for its own files. +const inheritedFDDarwin = 200 + +// inheritDarwin puts a readable pipe at fd, the way an exec'd child receives +// one: open, with no close-on-exec flag of its own. +func inheritDarwin(t *testing.T, fd int) { + t.Helper() + reader, writer, err := os.Pipe() + require.NoError(t, err) + _, err = writer.WriteString("a-task-token\n") + require.NoError(t, err) + require.NoError(t, writer.Close()) + t.Cleanup(func() { _ = reader.Close() }) + + require.NoError(t, unix.Dup2(int(reader.Fd()), fd)) + t.Cleanup(func() { _ = unix.Close(fd) }) + flags, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + require.NoError(t, err) + require.Zero(t, flags&unix.FD_CLOEXEC, "an inherited descriptor arrives without it") +} + +func cloexec(t *testing.T, fd int) bool { + t.Helper() + flags, err := unix.FcntlInt(uintptr(fd), unix.F_GETFD, 0) + require.NoError(t, err) + return flags&unix.FD_CLOEXEC != 0 +} + +// On macOS an inherited descriptor does not reach the processes this one +// starts, and stays readable here. +func TestSealedDescriptorsDoNotReachChildrenOnMacOS(t *testing.T) { + inheritDarwin(t, inheritedFDDarwin) + + require.NoError(t, sealInheritedDescriptors()) + + assert.True(t, cloexec(t, inheritedFDDarwin)) + fd := strconv.Itoa(inheritedFDDarwin) + out, err := exec.CommandContext(t.Context(), "/bin/sh", "-c", + "if (: <&"+fd+") 2>/dev/null; then echo inherited; else echo sealed; fi").Output() //nolint:gosec // G204: the descriptor number is the test's own constant + require.NoError(t, err) + assert.Equal(t, "sealed\n", string(out), "the child inherited the descriptor") + + buffer := make([]byte, len("a-task-token\n")) + read, err := unix.Read(inheritedFDDarwin, buffer) + require.NoError(t, err) + assert.Equal(t, "a-task-token\n", string(buffer[:read]), "marked, not closed or consumed") +} + +// /dev/fd is how macOS lists a process's descriptors, and the walk seals what +// it names. +func TestTheDevFDWalkSealsWhatItNames(t *testing.T) { + inheritDarwin(t, inheritedFDDarwin) + require.NoError(t, sealListedDescriptors(devFD)) + assert.True(t, cloexec(t, inheritedFDDarwin)) +} + +// A listing that cannot be read is an error, never a quiet success. +func TestTheDevFDWalkRefusesAListingItCannotRead(t *testing.T) { + require.Error(t, sealListedDescriptors(t.TempDir()+"/missing")) +} diff --git a/internal/cli/inherited_fds_linux.go b/internal/cli/inherited_fds_linux.go index 4f0011c55..b4901485d 100644 --- a/internal/cli/inherited_fds_linux.go +++ b/internal/cli/inherited_fds_linux.go @@ -3,10 +3,7 @@ package cli import ( - "errors" - "fmt" "math" - "os" "golang.org/x/sys/unix" @@ -46,59 +43,3 @@ func sealInheritedDescriptors() error { // parameter of sealListedDescriptors only so a test can watch the walk refuse // a listing it cannot trust. const procSelfFD = "/proc/self/fd" - -func sealListedDescriptors(listingDir string) error { - dir, err := os.Open(listingDir) - if err != nil { - return fmt.Errorf("could not open %s to seal the descriptors this process inherited: %w", listingDir, err) - } - defer func() { _ = dir.Close() }() - names, err := dir.Readdirnames(-1) - if err != nil { - return fmt.Errorf("could not read %s to seal the descriptors this process inherited: %w", listingDir, err) - } - listing, err := sysfd.Of(dir.Fd()) - if err != nil { - return fmt.Errorf("could not seal the descriptors this process inherited: %w", err) - } - for _, name := range names { - fd, err := sysfd.Parse(name) - if err != nil { - return fmt.Errorf("could not seal the descriptors this process inherited: %s holds %q, which is not a descriptor", listingDir, name) - } - if fd < sysfd.FirstNonStandard || fd == listing { - continue - } - if err := sealDescriptor(fd); err != nil { - return err - } - } - return nil -} - -// sealDescriptor marks one descriptor close-on-exec. -// -// A descriptor that is no longer open is the one failure that is not a -// failure: the listing is a snapshot, and a number that has been closed since -// it was taken is already out of reach of every child. Anything else means -// the descriptor is open and still inheritable, which is the thing this -// refuses to let happen quietly. -func sealDescriptor(fd sysfd.Descriptor) error { - flags, err := unix.FcntlInt(fd.Uintptr(), unix.F_GETFD, 0) - if err != nil { - if errors.Is(err, unix.EBADF) { - return nil - } - return fmt.Errorf("could not read the flags of inherited descriptor %s: %w", fd, err) - } - if flags&unix.FD_CLOEXEC != 0 { - return nil - } - if _, err := unix.FcntlInt(fd.Uintptr(), unix.F_SETFD, flags|unix.FD_CLOEXEC); err != nil { - if errors.Is(err, unix.EBADF) { - return nil - } - return fmt.Errorf("could not keep inherited descriptor %s from the processes this one starts: %w", fd, err) - } - return nil -} diff --git a/internal/cli/inherited_fds_other.go b/internal/cli/inherited_fds_other.go index b488ba929..71d19b7c2 100644 --- a/internal/cli/inherited_fds_other.go +++ b/internal/cli/inherited_fds_other.go @@ -1,10 +1,10 @@ -//go:build !linux +//go:build !linux && !darwin package cli // sealInheritedDescriptors has nothing to seal where the handover it protects // cannot happen: a task token arrives on an inherited descriptor only on -// Linux, which is what internal/commands/mcp_token_linux.go accepts and -// internal/commands/mcp_token_other.go refuses. Elsewhere this keeps the +// Linux and macOS, which is what internal/commands/mcp_token_sealed.go accepts +// and internal/commands/mcp_token_other.go refuses. Elsewhere this keeps the // program building and starting the same way. func sealInheritedDescriptors() error { return nil } diff --git a/internal/cli/inherited_fds_seal.go b/internal/cli/inherited_fds_seal.go new file mode 100644 index 000000000..7ae75e1a0 --- /dev/null +++ b/internal/cli/inherited_fds_seal.go @@ -0,0 +1,72 @@ +//go:build linux || darwin + +package cli + +import ( + "errors" + "fmt" + "os" + + "golang.org/x/sys/unix" + + "github.com/basecamp/basecamp-cli/internal/sysfd" +) + +// sealListedDescriptors marks close-on-exec every descriptor listingDir names: +// /proc/self/fd on Linux, /dev/fd on macOS, each the process's own list of +// what it holds. +func sealListedDescriptors(listingDir string) error { + dir, err := os.Open(listingDir) + if err != nil { + return fmt.Errorf("could not open %s to seal the descriptors this process inherited: %w", listingDir, err) + } + defer func() { _ = dir.Close() }() + names, err := dir.Readdirnames(-1) + if err != nil { + return fmt.Errorf("could not read %s to seal the descriptors this process inherited: %w", listingDir, err) + } + listing, err := sysfd.Of(dir.Fd()) + if err != nil { + return fmt.Errorf("could not seal the descriptors this process inherited: %w", err) + } + for _, name := range names { + fd, err := sysfd.Parse(name) + if err != nil { + return fmt.Errorf("could not seal the descriptors this process inherited: %s holds %q, which is not a descriptor", listingDir, name) + } + if fd < sysfd.FirstNonStandard || fd == listing { + continue + } + if err := sealDescriptor(fd); err != nil { + return err + } + } + return nil +} + +// sealDescriptor marks one descriptor close-on-exec. +// +// A descriptor that is no longer open is the one failure that is not a +// failure: the listing is a snapshot, and a number that has been closed since +// it was taken is already out of reach of every child. Anything else means +// the descriptor is open and still inheritable, which is the thing this +// refuses to let happen quietly. +func sealDescriptor(fd sysfd.Descriptor) error { + flags, err := unix.FcntlInt(fd.Uintptr(), unix.F_GETFD, 0) + if err != nil { + if errors.Is(err, unix.EBADF) { + return nil + } + return fmt.Errorf("could not read the flags of inherited descriptor %s: %w", fd, err) + } + if flags&unix.FD_CLOEXEC != 0 { + return nil + } + if _, err := unix.FcntlInt(fd.Uintptr(), unix.F_SETFD, flags|unix.FD_CLOEXEC); err != nil { + if errors.Is(err, unix.EBADF) { + return nil + } + return fmt.Errorf("could not keep inherited descriptor %s from the processes this one starts: %w", fd, err) + } + return nil +} diff --git a/internal/cli/token_handover_platforms_test.go b/internal/cli/token_handover_platforms_test.go index 554d8cb22..d74afb793 100644 --- a/internal/cli/token_handover_platforms_test.go +++ b/internal/cli/token_handover_platforms_test.go @@ -15,7 +15,7 @@ import ( // asks about the code rather than about a build tag it repeats. const ( sealingDir = "." - sealingFile = "inherited_fds_linux.go" + sealingFile = "inherited_fds_seal.go" handoverDir = "../commands" refusalFile = "mcp_token_other.go" ) diff --git a/internal/commands/connect.go b/internal/commands/connect.go index c966f099d..8c147a019 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -54,7 +54,7 @@ go to stderr. SIGINT and SIGTERM cancel live workers with stop reason shutdown, settle them, and exit 130 and 143. --shadow admits and logs in an isolated state directory and dispatches nothing. --hold sets a durable hold: intake and admission run, nothing dispatches or posts, and earlier records -wait for review, until basecamp connect release. Linux only. +wait for review, until basecamp connect release. Linux and macOS only. basecamp connect status what it heard, holds and ran basecamp connect doctor what it needs to run diff --git a/internal/commands/connect_doctor.go b/internal/commands/connect_doctor.go index 3778cbb83..b8d4746ca 100644 --- a/internal/commands/connect_doctor.go +++ b/internal/commands/connect_doctor.go @@ -365,14 +365,12 @@ func preflightReason(err error) string { // connectUnsupportedOSCheck is the Platform check on a GOOS the connector // does not run on. It gives the constraint that actually applies // (connectSupportedOS, and #736): the task token reaches a worker's MCP -// server over an inherited descriptor, and Linux alone seals the descriptors -// a process passes on. Reading a process's start time is the half macOS has, -// so naming that here told a Mac reader the connector could run there and -// then refused it anyway. +// server over an inherited descriptor, and only Linux and macOS seal the +// descriptors a process passes on. func connectUnsupportedOSCheck(goos string) setup.Check { return setup.Check{Name: "Platform", Status: setup.StatusFail, - Message: fmt.Sprintf("The connector does not run on %s: %s", goos, connectLinuxOnlyReason), - Hint: "Run the connector on Linux; the rest of the CLI runs here."} + Message: fmt.Sprintf("The connector does not run on %s: %s", goos, connectSupportedOSReason), + Hint: "Run the connector on Linux or macOS; the rest of the CLI runs here."} } // driverChecks refuses what the run command refuses: doctor never calls a diff --git a/internal/commands/connect_operator_test.go b/internal/commands/connect_operator_test.go index 563c5d9a8..7d53bbaac 100644 --- a/internal/commands/connect_operator_test.go +++ b/internal/commands/connect_operator_test.go @@ -267,18 +267,16 @@ func TestConnectDoctorRefusesOnlyWhatTheRunCommandRefuses(t *testing.T) { // The check and the run command's refusal say the one reason, so a person // cannot be told two different things about the same platform. func TestConnectDoctorPlatformCheckNamesTheConstraintThatApplies(t *testing.T) { - c := connectUnsupportedOSCheck("darwin") + c := connectUnsupportedOSCheck("windows") assert.Equal(t, "Platform", c.Name) assert.Equal(t, setup.StatusFail, c.Status) - assert.Contains(t, c.Message, "darwin", "it names the platform it refuses") - assert.Contains(t, c.Message, connectLinuxOnlyReason, "it gives the reason the run command gives") - assert.NotContains(t, c.Message, "macOS", - "a refusal must not name a capability the platform it refuses actually has") + assert.Contains(t, c.Message, "windows", "it names the platform it refuses") + assert.Contains(t, c.Message, connectSupportedOSReason, "it gives the reason the run command gives") - err := connectUnsupportedOSError("darwin") + err := connectUnsupportedOSError("windows") require.Error(t, err) - assert.Contains(t, err.Error(), connectLinuxOnlyReason, "doctor and the run command give the one reason") - assert.NotContains(t, err.Error(), "macOS") + assert.Contains(t, err.Error(), connectSupportedOSReason, "doctor and the run command give the one reason") + assert.Contains(t, err.Error(), "Linux and macOS only, not windows") } // The acp driver runs a pinned adapter out of the connector's own npm diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index 63887a2b6..3755693fc 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -614,28 +614,26 @@ func connectConsumerNamespace(agentID int64, shadow bool) string { return name } -// connectSupportedOS is where the connector runs: Linux, and for now only -// Linux. +// connectSupportedOS is where the connector runs: Linux and macOS. // -// Two things have to hold, and macOS has only one of them. The driver must -// be able to read process start times, so a recorded worker group is never -// signaled after its pid was reused — macOS can. And the task token has to -// reach the worker's MCP server, which it does on an inherited descriptor: -// `connect worker-mcp` execs `basecamp mcp --connect-token-fd`, and that -// hand-over is accepted only where the descriptors this process inherited -// are sealed against everything it starts, which is Linux alone -// (mcp_token_linux.go, and #736, which gated it deliberately). On macOS -// every non-shadow dispatch would start a worker whose Basecamp tools fail -// at the handshake, so the connector says so here rather than at the far -// end of each task. +// Two things have to hold. The driver must be able to read process start +// times, so a recorded worker group is never signaled after its pid was +// reused. And the task token has to reach the worker's MCP server, which it +// does on an inherited descriptor: `connect worker-mcp` execs `basecamp mcp +// --connect-token-fd`, and that hand-over is accepted only where the +// descriptors this process inherited are sealed against everything it +// starts (mcp_token_sealed.go, internal/cli's inherited_fds_seal.go). Linux +// and macOS do both; anywhere else every non-shadow dispatch would start a +// worker whose Basecamp tools fail at the handshake, so the connector says so +// here rather than at the far end of each task. func connectSupportedOS(goos string) bool { - return goos == "linux" + return goos == "linux" || goos == "darwin" } -// connectLinuxOnlyReason is why, in one place: the run command's refusal and +// connectSupportedOSReason is why, in one place: the run command's refusal and // doctor's Platform check say the same thing, so a person who meets one and // then the other is not told two different stories about their machine. -const connectLinuxOnlyReason = "the task token reaches a worker's MCP server over an inherited descriptor, and Linux is the only platform that seals the descriptors a process inherits" +const connectSupportedOSReason = "the task token reaches a worker's MCP server over an inherited descriptor, and the CLI keeps inherited descriptors from the programs it starts only on Linux and macOS" // connectUnsupportedOSError is the refusal on a platform the connector does // not run on, given by the run command and by `service install`, which would @@ -645,7 +643,7 @@ const connectLinuxOnlyReason = "the task token reaches a worker's MCP server ove // refusal is prose, not a command to run, and a hint that begins like a // command is read as one — by a person, and by TestHintCommandsResolve. func connectUnsupportedOSError(goos string) error { - return output.ErrUsage(fmt.Sprintf("The connector runs on Linux only, not %s: %s", goos, connectLinuxOnlyReason)) + return output.ErrUsage(fmt.Sprintf("The connector runs on Linux and macOS only, not %s: %s", goos, connectSupportedOSReason)) } // connectServed is connect.json's served projects as they are now, not as diff --git a/internal/commands/connect_run_test.go b/internal/commands/connect_run_test.go index 37a058e9d..f7cf68ee0 100644 --- a/internal/commands/connect_run_test.go +++ b/internal/commands/connect_run_test.go @@ -50,10 +50,11 @@ func TestConnectStateLivesUnderXDGStateHome(t *testing.T) { // Copilot on #738: macOS passed this check and then failed every non-shadow // dispatch at the worker's MCP handshake, because the token hand-over onto // an inherited descriptor is accepted only where those descriptors are -// sealed — Linux (#736). -func TestConnectRunsOnLinuxOnly(t *testing.T) { +// sealed — Linux (#736), and macOS, which seals them through /dev/fd. +func TestConnectRunsOnLinuxAndMacOS(t *testing.T) { assert.True(t, connectSupportedOS("linux")) - for _, goos := range []string{"darwin", "freebsd", "openbsd", "windows"} { + assert.True(t, connectSupportedOS("darwin")) + for _, goos := range []string{"freebsd", "openbsd", "windows"} { assert.False(t, connectSupportedOS(goos), goos) } } diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go index 824940ff1..eca0e1bc8 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -32,9 +32,9 @@ import ( // OS. These commands write the unit that does the restarting, and take it // away again. // -// systemd only. The connector runs on Linux alone, for the reason the run -// command gives, so there is no second supervisor to write for: a launchd -// agent would supervise a process that refuses to start. +// systemd only, so Linux only. The connector also runs on macOS, where a +// launchd agent would be the supervisor; none is written yet, and the service +// stays hidden until one is. // connectServiceUnitPrefix begins every unit this writes. The profile name // completes it, so one machine can supervise several agents. @@ -189,6 +189,10 @@ func connectServiceProfile(app *appctx.App) (string, error) { if !connectSupportedOS(connectServiceGOOS) { return "", connectUnsupportedOSError(connectServiceGOOS) } + // The connector runs on macOS too; the unit this writes is systemd's. + if connectServiceGOOS != "linux" { + return "", output.ErrUsage(fmt.Sprintf("The connector's background service is a systemd unit, so it runs on Linux only, not %s. Start the connector yourself in the folder it should work in.", connectServiceGOOS)) + } name := app.Config.ActiveProfile if name == "" { return "", output.ErrUsageHint("The connector's service needs the agent's profile", "Pass -P/--profile , a profile set up with `basecamp connect setup`.") diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go index 0101189e6..d06e74333 100644 --- a/internal/commands/connect_service_test.go +++ b/internal/commands/connect_service_test.go @@ -334,23 +334,31 @@ func TestConnectServiceInstallRefusesWhenSystemdReadsAnotherFile(t *testing.T) { assert.Contains(t, err.Error(), "/etc/systemd/user/basecamp-connect-agent.service") } -// The platform gate is the run command's, so a Mac is refused before a unit -// is written for a connector that would not start on it. -func TestConnectServiceRefusesAPlatformTheConnectorDoesNotRunOn(t *testing.T) { - connectServiceHome(t) - writeConnectSetup(t, "agent") - prev := connectServiceGOOS - connectServiceGOOS = "darwin" - t.Cleanup(func() { connectServiceGOOS = prev }) - app, _ := connectServiceApp(t, "agent") - - _, err := runConnectServiceCmd(t, app, "install") - require.Error(t, err) - assert.Contains(t, err.Error(), connectLinuxOnlyReason) +// The platform gate is the run command's, so a platform the connector does +// not run on is refused before a unit is written for it; and the unit is +// systemd's, so a Mac, where the connector runs, is refused for that. +func TestConnectServiceRefusesAnInstallOffLinux(t *testing.T) { + for goos, want := range map[string]string{ + "windows": connectSupportedOSReason, + "darwin": "background service is a systemd unit, so it runs on Linux only, not darwin", + } { + t.Run(goos, func(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + prev := connectServiceGOOS + connectServiceGOOS = goos + t.Cleanup(func() { connectServiceGOOS = prev }) + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install") + require.Error(t, err) + assert.Contains(t, err.Error(), want) - path, err := connectServiceUnitPath("agent") - require.NoError(t, err) - assert.NoFileExists(t, path) + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + assert.NoFileExists(t, path) + }) + } } // When systemctl is missing, or fails in the running rather than in what it diff --git a/internal/commands/connect_setup_guided_test.go b/internal/commands/connect_setup_guided_test.go index 078fbd090..d2f54bbc6 100644 --- a/internal/commands/connect_setup_guided_test.go +++ b/internal/commands/connect_setup_guided_test.go @@ -271,11 +271,11 @@ func TestGuidedConnectSetupRefusesAnUnsupportedPlatformBeforeConnecting(t *testi ownedByTheOperator(s) s.agentProjects = projectsNamed(setupProject) guided(t, &scriptedPrompter{}) - connectServiceGOOS = "darwin" + connectServiceGOOS = "windows" out, err := runConnectSetupCmd(t, bareSetupApp(t, s, "agent")) require.Error(t, err, out) - assert.Contains(t, err.Error(), "Linux only, not darwin") + assert.Contains(t, err.Error(), "Linux and macOS only, not windows") assert.NotContains(t, out, "connect this computer") assertNotWritten(t, "agent") } diff --git a/internal/commands/connect_worker_mcp_other.go b/internal/commands/connect_worker_mcp_other.go index 0ca35b9e8..3ce0c0d73 100644 --- a/internal/commands/connect_worker_mcp_other.go +++ b/internal/commands/connect_worker_mcp_other.go @@ -5,5 +5,5 @@ package commands import "errors" func execWorkerMCP(string, string, string, string) error { - return errors.New("worker-mcp runs on Linux only") + return errors.New("worker-mcp runs on Linux and macOS only") } diff --git a/internal/commands/mcp_connect_test.go b/internal/commands/mcp_connect_sealed_test.go similarity index 99% rename from internal/commands/mcp_connect_test.go rename to internal/commands/mcp_connect_sealed_test.go index e8e328a84..141dbc6cd 100644 --- a/internal/commands/mcp_connect_test.go +++ b/internal/commands/mcp_connect_sealed_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || darwin package commands diff --git a/internal/commands/mcp_connect_token_linux_test.go b/internal/commands/mcp_connect_token_sealed_test.go similarity index 99% rename from internal/commands/mcp_connect_token_linux_test.go rename to internal/commands/mcp_connect_token_sealed_test.go index bee398b8d..f6e5b4caf 100644 --- a/internal/commands/mcp_connect_token_linux_test.go +++ b/internal/commands/mcp_connect_token_sealed_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || darwin package commands diff --git a/internal/commands/mcp_token_other.go b/internal/commands/mcp_token_other.go index 6a11677df..f14cfd2b1 100644 --- a/internal/commands/mcp_token_other.go +++ b/internal/commands/mcp_token_other.go @@ -1,4 +1,4 @@ -//go:build !linux +//go:build !linux && !darwin package commands @@ -8,14 +8,14 @@ import ( ) // readTaskToken is refused wherever the descriptor carrying the token cannot -// be kept from the processes this one starts. Only Linux seals the +// be kept from the processes this one starts. Only Linux and macOS seal the // descriptors this process inherited, in internal/cli's -// inherited_fds_linux.go, so only Linux may be handed a credential on one: +// inherited_fds_seal.go, so only they may be handed a credential on one: // accepting a token where nothing seals it would leave it inheritable through // every hook that runs before this command, which is the whole point of // handing it over on a descriptor instead of in a file or an environment. // -// The connector runs on Linux, so this refuses nothing that works today. +// The connector runs on Linux and macOS, so this refuses nothing that works today. func readTaskToken(sysfd.Descriptor) (string, error) { - return "", output.ErrUsage("--connect-state is only available on Linux, where an inherited descriptor can be kept from the processes this one starts") + return "", output.ErrUsage("--connect-token-fd is only available on Linux and macOS, where an inherited descriptor can be kept from the processes this one starts") } diff --git a/internal/commands/mcp_token_linux.go b/internal/commands/mcp_token_sealed.go similarity index 92% rename from internal/commands/mcp_token_linux.go rename to internal/commands/mcp_token_sealed.go index dbbbe30a3..fe5de3151 100644 --- a/internal/commands/mcp_token_linux.go +++ b/internal/commands/mcp_token_sealed.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux || darwin package commands @@ -22,10 +22,10 @@ import ( // exists at a path, in argv or in the environment; once read, the descriptor // is gone too, and nothing this process starts can inherit it. // -// This file is built for Linux alone, and shares that constraint with -// internal/cli's inherited_fds_linux.go on purpose: a credential may arrive -// on an inherited descriptor only where startup has already sealed every -// inherited descriptor against the children the pre-command hooks start. +// This file is built for Linux and macOS alone, and shares that constraint +// with internal/cli's inherited_fds_seal.go on purpose: a credential may +// arrive on an inherited descriptor only where startup has already sealed +// every inherited descriptor against the children the pre-command hooks start. // Everywhere else mcp_token_other.go refuses the handover, and // TestTheTokenIsOnlyReadWhereItIsSealed holds the two constraints together. // diff --git a/internal/connector/admission/policy.go b/internal/connector/admission/policy.go index 270f756c6..e63b0f975 100644 --- a/internal/connector/admission/policy.go +++ b/internal/connector/admission/policy.go @@ -176,7 +176,7 @@ func checkLegacyPathValue(raw json.RawMessage) error { if lossyJSONString(bytes.TrimSpace(raw)) { return errors.New(`the "path" of a connector that routed projects is not encoded the way it was written: it holds malformed UTF-8 or an unpaired surrogate escape, which encoding/json silently replaces with U+FFFD, so the value read back is not the value in the file`) } - // POSIX, not this host's rules. The connector runs on Linux only, so the + // POSIX, not this host's rules. The connector runs on Linux and macOS, so the // writer of this key wrote a POSIX path — and path.IsAbs answers for // that format on every platform, where filepath.IsAbs answers for // whatever the reader happens to be compiled for. This package builds diff --git a/internal/connector/driver/claude/claude_test.go b/internal/connector/driver/claude/claude_test.go index 1aa4e1e7a..995a92d90 100644 --- a/internal/connector/driver/claude/claude_test.go +++ b/internal/connector/driver/claude/claude_test.go @@ -112,13 +112,16 @@ func fakeClaude(scenario string) { } if scenario == "deaf" || scenario == "deaf-secret" { - // Reads nothing, ever: the pipe fills and a write blocks. - select {} + // Reads nothing, ever: the pipe fills and a write blocks. A sleep, + // not an empty select: with nothing else running, the runtime calls + // an empty select a deadlock and ends the process, which on macOS + // it does before the pipe fills. + time.Sleep(time.Hour) } if scenario == "badmode-eager" { // An init before any prompt, in a mode the policy did not ask for. emit(map[string]any{"type": "system", "subtype": "init", "session_id": sessionID, "permissionMode": "bypassPermissions", "mcp_servers": []any{}}) - select {} + time.Sleep(time.Hour) } in := bufio.NewScanner(os.Stdin) diff --git a/internal/connector/driver/worker_unix.go b/internal/connector/driver/worker_unix.go index 12bde4d01..0912ee7c4 100644 --- a/internal/connector/driver/worker_unix.go +++ b/internal/connector/driver/worker_unix.go @@ -3,7 +3,9 @@ package driver import ( + "errors" "os/exec" + "runtime" "sync" "syscall" "time" @@ -28,7 +30,21 @@ func signalGroup(pgid int, sig syscall.Signal) error { if pgid <= 1 || pgid == syscall.Getpgrp() { return syscall.EINVAL } - return syscall.Kill(-pgid, sig) + err := syscall.Kill(-pgid, sig) + // macOS only: Linux delivers a signal to a zombie-only group, so EPERM + // there is a group this process may not signal — and with /proc mounted + // hidepid, its listing cannot see that group's members to say otherwise. + if runtime.GOOS == "darwin" && errors.Is(err, syscall.EPERM) { + // macOS refuses any signal to a group whose only members are + // zombies, where Linux delivers it; nothing in such a group runs. + // When the kernel's own listing says so, it is the absent group it + // is everywhere else. A group that runs but may not be signaled + // (another user's) still lists its members, and keeps the refusal. + if running, listErr := groupRunning(pgid); listErr == nil && !running { + return syscall.ESRCH + } + } + return err } // probeGroup is a probe run as the leader of a group of its own. diff --git a/internal/connector/driver/zombie_darwin_test.go b/internal/connector/driver/zombie_darwin_test.go new file mode 100644 index 000000000..c31d6c5c5 --- /dev/null +++ b/internal/connector/driver/zombie_darwin_test.go @@ -0,0 +1,46 @@ +//go:build darwin + +package driver + +import ( + "os/exec" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// macOS refuses any signal to a process group whose only members are +// zombies. That refusal is the absent group it is on Linux: signalGroup +// answers ESRCH, so the group can be confirmed gone. +func TestAZombieOnlyGroupIsNoSuchGroupOnMacOS(t *testing.T) { + leader := exec.CommandContext(t.Context(), "/bin/sleep", "300") + leader.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + require.NoError(t, leader.Start()) + pgid := leader.Process.Pid + t.Cleanup(func() { _ = leader.Wait() }) + require.NoError(t, syscall.Kill(pgid, syscall.SIGKILL)) + // Dead, and not yet waited for: a zombie, alone in its group. + require.Eventually(t, func() bool { _, err := processStartTime(pgid); return err != nil }, 5*time.Second, 10*time.Millisecond) + require.ErrorIs(t, syscall.Kill(-pgid, 0), syscall.EPERM, "macOS refuses a zombie-only group") + + assert.ErrorIs(t, signalGroup(pgid, 0), syscall.ESRCH) + assert.NoError(t, groupGone(pgid)) +} + +// A group whose leader is a zombie but whose child still runs is not gone: +// the signal reaches the child. +func TestAGroupWithALiveMemberIsStillSignaledOnMacOS(t *testing.T) { + leader := exec.CommandContext(t.Context(), "/bin/sh", "-c", "/bin/sleep 300 & exit 0") + leader.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + require.NoError(t, leader.Start()) + pgid := leader.Process.Pid + t.Cleanup(func() { _ = signalGroup(pgid, syscall.SIGKILL); _ = leader.Wait() }) + require.Eventually(t, func() bool { _, err := processStartTime(pgid); return err != nil }, 5*time.Second, 10*time.Millisecond, "the leader has exited") + + err := signalGroup(pgid, 0) + assert.NoError(t, err, "the live child receives it") + assert.Error(t, groupGone(pgid), "a group with a live member is not gone") +} diff --git a/internal/connector/tokensocket.go b/internal/connector/tokensocket.go index dccbc38fd..05b2998c9 100644 --- a/internal/connector/tokensocket.go +++ b/internal/connector/tokensocket.go @@ -645,15 +645,26 @@ func (s *TokenSocket) handOne(window time.Duration) (Handoff, driver.Process) { } defer func() { _ = conn.Close() }() _ = conn.SetDeadline(deadline) - if !s.trusted(conn, deadline) { + peer, ok, gone := s.trusted(conn, deadline) + if !ok { + if gone { + // This user's process, closed before it could be named: nothing + // untrusted asked, and nothing was handed over. + return HandoffUndelivered, driver.Process{} + } return HandoffRefused, driver.Process{} } + // The peer named before the write, not asked again, and looked up before + // the write too: a server that reads its token and closes at once is the + // ordinary case, macOS no longer names a peer that has closed, and once + // it has gone its pid could belong to another process (Copilot on #812). + taker := s.takerOf(peer.PID) if _, err := conn.Write([]byte(s.token + "\n")); err != nil { // The peer was the worker's; the write is what failed. On a unix // socket a peer that has gone makes this EPIPE at once. return HandoffUndelivered, driver.Process{} } - return HandoffDelivered, s.takerOfConn(conn) + return HandoffDelivered, taker } // allowedGroup is the worker's process group, or 0 before it is named. @@ -667,12 +678,21 @@ func (s *TokenSocket) allowedGroup() int { } } +// errPeerGone is a peer that closed before the kernel would name its +// process. macOS answers LOCAL_PEERPID only while the peer is connected, and +// still names its user. +var errPeerGone = errors.New("connector: the peer closed before it could be identified") + // trusted reports whether the peer is this user's process in the worker's -// own process group. -func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) bool { +// own process group. gone is a peer of this user's that closed before its +// process could be named: not trusted, and not a refusal either. +func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) (peer PeerCredentials, ok, gone bool) { cred, err := s.peer(conn) + if errors.Is(err, errPeerGone) { + return cred, false, cred.UID == os.Getuid() + } if err != nil || cred.UID != os.Getuid() || cred.PID <= 0 { - return false + return cred, false, false } ctx, cancel := context.WithDeadline(context.Background(), deadline) defer cancel() @@ -681,15 +701,15 @@ func (s *TokenSocket) trusted(conn *net.UnixConn, deadline time.Time) bool { case want = <-s.group: s.group <- want case <-ctx.Done(): - return false + return cred, false, false } if want <= 1 { - return false + return cred, false, false } if got, err := s.groupOf(cred.PID); err == nil && got == want { - return true + return cred, true, false } - return s.descendsFrom(cred.PID, want) + return cred, s.descendsFrom(cred.PID, want), false } // maxAncestry bounds the walk up a peer's parents. @@ -710,16 +730,15 @@ func (s *TokenSocket) descendsFrom(pid, ancestor int) bool { return false } -// takerOfConn is the identity of the process the token just went to, so the +// takerOf is the identity of the process the token just went to, so the // release point can end it: it is outside the worker's process group whenever // the agent started it in one of its own. A restarted MCP server is a new // process, and the newest is the one holding the token. -func (s *TokenSocket) takerOfConn(conn *net.UnixConn) driver.Process { - cred, err := s.peer(conn) - if err != nil || cred.PID <= 0 { +func (s *TokenSocket) takerOf(pid int) driver.Process { + if pid <= 0 { return driver.Process{} } - taker, err := s.lookup(cred.PID) + taker, err := s.lookup(pid) if err != nil { return driver.Process{} } diff --git a/internal/connector/tokensocket_darwin.go b/internal/connector/tokensocket_darwin.go index c2e09369c..71d7cd38b 100644 --- a/internal/connector/tokensocket_darwin.go +++ b/internal/connector/tokensocket_darwin.go @@ -1,6 +1,7 @@ package connector import ( + "errors" "net" "golang.org/x/sys/unix" @@ -33,6 +34,11 @@ func peerCredentials(conn *net.UnixConn) (PeerCredentials, error) { if credOK != nil { return PeerCredentials{}, credOK } + if errors.Is(pidOK, unix.ENOTCONN) { + // The user is still answered; the process is not, once the peer has + // closed. + return PeerCredentials{UID: int(cred.Uid)}, errPeerGone + } if pidOK != nil { return PeerCredentials{}, pidOK } diff --git a/internal/connector/tokensocket_darwin_test.go b/internal/connector/tokensocket_darwin_test.go new file mode 100644 index 000000000..973606ff3 --- /dev/null +++ b/internal/connector/tokensocket_darwin_test.go @@ -0,0 +1,47 @@ +//go:build darwin + +package connector + +import ( + "context" + "errors" + "net" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// macOS names a peer's process only while it is connected, and still names +// its user. peerCredentials maps that to errPeerGone with the user, which is +// what keeps an early close undelivered rather than refused. +func TestMacOSNamesAClosedPeerAsGoneWithItsUser(t *testing.T) { + path := filepath.Join(tokenDir(t), "s") + ln, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + require.NoError(t, err) + defer ln.Close() + + dialer := net.Dialer{Timeout: 2 * time.Second} + client, err := dialer.DialContext(context.Background(), "unix", path) + require.NoError(t, err) + conn, err := ln.AcceptUnix() + require.NoError(t, err) + defer conn.Close() + + open, err := peerCredentials(conn) + require.NoError(t, err) + assert.Equal(t, os.Getpid(), open.PID) + assert.Equal(t, os.Getuid(), open.UID) + + require.NoError(t, client.Close()) + require.Eventually(t, func() bool { + _, err := peerCredentials(conn) + return errors.Is(err, errPeerGone) + }, 5*time.Second, 10*time.Millisecond) + gone, err := peerCredentials(conn) + require.ErrorIs(t, err, errPeerGone) + assert.Equal(t, os.Getuid(), gone.UID) +} diff --git a/internal/connector/tokensocket_test.go b/internal/connector/tokensocket_test.go index 531d29461..34ad4e60b 100644 --- a/internal/connector/tokensocket_test.go +++ b/internal/connector/tokensocket_test.go @@ -302,10 +302,13 @@ func TestTheShortSocketBaseIsChosenSoTheSocketFits(t *testing.T) { // an empty taker for a token that was in fact handed over. func TestAHandoffInFlightIsFinishedBeforeTheTakerIsRead(t *testing.T) { // The identity lookup is where the handoff is slowest; hold it there. - slow := make(chan struct{}) + // It runs before the write (Copilot on #812), so the peer is connected + // and waiting for its token. + reached, slow := make(chan struct{}), make(chan struct{}) s, err := serveTaskTokenWith(tokenDir(t), socketTestToken, 2*time.Second, peerCredentials, processGroupOf, parentProcessOf, func(pid int) (driver.Process, error) { + close(reached) <-slow return driver.LookupProcess(pid) }) @@ -318,7 +321,7 @@ func TestAHandoffInFlightIsFinishedBeforeTheTakerIsRead(t *testing.T) { token, _ := fetch(t, s.Path()) got <- token }() - require.Equal(t, socketTestToken, strings.TrimSpace(<-got), "the token is out before the taker is known") + <-reached _, ok := s.Taker() require.False(t, ok, "the fixture must have the handoff still deciding") @@ -330,6 +333,7 @@ func TestAHandoffInFlightIsFinishedBeforeTheTakerIsRead(t *testing.T) { require.True(t, ok, "and the process that took the token is known by then") assert.Equal(t, os.Getpid(), taker.PID) assert.Equal(t, HandoffDelivered, s.Result()) + assert.Equal(t, socketTestToken, strings.TrimSpace(<-got), "the handoff in flight still delivered") } // Opus r8: after a delivery the socket does not arm again while the process @@ -526,3 +530,63 @@ func TestAHandoffStillInFlightAtTheReleasePointHoldsTheAttempt(t *testing.T) { holder := settledTaker(s, slog.New(slog.DiscardHandler), "att", 50*time.Millisecond) assert.True(t, holder.Held(), "an attempt is not released around a handoff that is still deciding") } + +// macOS names a peer's process only while it is connected: a peer of this +// user's that closed first is not a refusal (nothing untrusted asked) and is +// handed nothing; the socket waits for the next start. Another user's is +// still refused. +func TestAPeerGoneBeforeItIsNamedIsUndeliveredNotRefused(t *testing.T) { + for uid, want := range map[int]Handoff{os.Getuid(): HandoffUndelivered, os.Getuid() + 1: HandoffRefused} { + gone := func(*net.UnixConn) (PeerCredentials, error) { return PeerCredentials{UID: uid}, errPeerGone } + s, err := serveTaskTokenWith(tokenDir(t), socketTestToken, 5*time.Second, gone, + processGroupOf, parentProcessOf, driver.LookupProcess) + require.NoError(t, err) + handoffs := make(chan Handoff, 4) + s.OnHandoff(func(h Handoff, _ driver.Process, _ bool) { handoffs <- h }) + s.AllowGroup(syscall.Getpgrp()) + + dialer := net.Dialer{Timeout: 2 * time.Second} + conn, err := dialer.DialContext(context.Background(), "unix", s.Path()) + require.NoError(t, err) + got, err := io.ReadAll(conn) + require.NoError(t, err) + _ = conn.Close() + assert.Empty(t, got, "no token for a peer that could not be named") + assert.Equal(t, want, <-handoffs, "uid %d", uid) + s.Close() + } +} + +// The process that took the token is the one the trust check named before +// the write. A server that reads its token and closes at once is the +// ordinary case, and macOS will not name a peer that has closed: asking a +// second time would lose the taker, and hold the attempt over a delivery that +// worked. +func TestTheTakerIsThePeerNamedBeforeTheWrite(t *testing.T) { + var calls atomic.Int32 + namedOnce := func(*net.UnixConn) (PeerCredentials, error) { + if calls.Add(1) > 1 { + return PeerCredentials{UID: os.Getuid()}, errPeerGone + } + return PeerCredentials{PID: os.Getpid(), UID: os.Getuid()}, nil + } + s, err := serveTaskTokenWith(tokenDir(t), socketTestToken, 5*time.Second, namedOnce, + processGroupOf, parentProcessOf, driver.LookupProcess) + require.NoError(t, err) + defer s.Close() + type handed struct { + h Handoff + taker driver.Process + } + handoffs := make(chan handed, 2) + s.OnHandoff(func(h Handoff, taker driver.Process, _ bool) { handoffs <- handed{h, taker} }) + s.AllowGroup(syscall.Getpgrp()) + + got, err := fetch(t, s.Path()) + require.NoError(t, err) + assert.Equal(t, socketTestToken, strings.TrimSpace(got)) + first := <-handoffs + assert.Equal(t, HandoffDelivered, first.h) + assert.Equal(t, os.Getpid(), first.taker.PID, "the taker is the peer the trust check named") + assert.Equal(t, int32(1), calls.Load(), "the peer is asked once") +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index c26edacb5..8e8338640 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -502,8 +502,8 @@ Pass the run's shape to the connector itself: `--project ` (repeatable) to hear only some projects, `--shadow` to admit and log without dispatching or posting anything, `--hold` to run with the durable hold set. -Linux only for now: on any other system the connector refuses to start, and -says so. If a person on a Mac asks, say that. +Linux and macOS only: on any other system the connector refuses to start, +and says so. Quote the profile, as everywhere else in this skill. diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 2b821002d..469c3c46a 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1477,9 +1477,9 @@ basecamp connect setup -P agent --worker codex # Run workers with Codex inst answer. Stdout is a wire of one JSON object per line (events seen, verdicts, dispatches — ids and states, never content) and the logs are on stderr, so read the lines rather than the log. SIGINT and SIGTERM cancel whatever workers are -running, settle them, and exit 130 and 143. It runs on Linux only: the task -token's hand-over onto a descriptor the next program inherits is sealed only -there. It refuses a second connector for the same agent, and takes `--project` +running, settle them, and exit 130 and 143. It runs on Linux and macOS: the +task token's hand-over onto a descriptor the next program inherits is sealed +only there. It refuses a second connector for the same agent, and takes `--project` (repeatable) to hear and dispatch only those projects. Run it under a supervisor rather than from a session you will close.