diff --git a/apps/sandboxio/cmd/oac-sandbox-io/main.go b/apps/sandboxio/cmd/oac-sandbox-io/main.go index 94666fd8a..bf3fb2f89 100644 --- a/apps/sandboxio/cmd/oac-sandbox-io/main.go +++ b/apps/sandboxio/cmd/oac-sandbox-io/main.go @@ -34,7 +34,7 @@ func main() { // Reap is the process's only wait. As a child subreaper it also reaps // the orphaned descendants of operations. if err := unix.Prctl(unix.PR_SET_CHILD_SUBREAPER, 1, 0, 0, 0); err != nil { - fail(&sandboxio.StartupError{Step: sandboxio.StepSubreaper, Err: err}) + fail(fmt.Errorf("become a child subreaper: %w", err)) } go processservice.Reap(context.Background()) diff --git a/apps/sandboxio/internal/processservice/launch.go b/apps/sandboxio/internal/processservice/launch.go index 951879f04..4e9af85d7 100644 --- a/apps/sandboxio/internal/processservice/launch.go +++ b/apps/sandboxio/internal/processservice/launch.go @@ -21,8 +21,8 @@ import ( // launch spawns the process and emits Started or StartFailed, then starts // the output readers. The leader's exit follows the output it left buffered; // see markLocked. -func (op *operation) launch(spec sp.ProcessSpec) { - l, f := op.spawn(spec) +func (op *operation) launch(req sp.StartRequest) { + l, f := op.spawn(req) op.mu.Lock() if f != nil { op.state, op.startFailure = sp.StateStartFailed, f @@ -59,9 +59,9 @@ type launched struct { streams []*stream } -// spawn starts the trampoline in a new session and waits until it has -// exec'd the target or reported why it could not. -func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { +// spawn starts the trampoline in a new session, hands it the request and +// waits until it has exec'd the target or reported why it could not. +func (op *operation) spawn(req sp.StartRequest) (l launched, f *sp.Failure) { ioFail := func(what string, err error) *sp.Failure { return sp.Fail(sp.CodeIO, sandboxwire.EffectNone, "%s: %v", what, err) } @@ -94,8 +94,8 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { if files[statusFD], statusR, f = pipe("status pipe", false); f != nil { return l, f } - if spec.PTY != nil { - master, tty, err := openPTY(*spec.PTY) + if req.Spec.PTY != nil { + master, tty, err := openPTY(*req.Spec.PTY) if err != nil { return l, ioFail("open terminal", err) } @@ -125,7 +125,7 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { Env: []string{trampolineEnv}, Files: files, // Setctty makes the child's descriptor 0, the terminal, its controlling terminal. - Sys: &syscall.SysProcAttr{Setsid: true, Setctty: spec.PTY != nil}, + Sys: &syscall.SysProcAttr{Setsid: true, Setctty: req.Spec.PTY != nil}, } reaping.RLock() p, err := os.StartProcess("/proc/self/exe", []string{trampolineArg0}, attr) @@ -157,7 +157,7 @@ func (op *operation) spawn(spec sp.ProcessSpec) (l launched, f *sp.Failure) { closeAll(child) child = nil - _, werr := launchW.Write(encodeLaunch(spec)) + _, werr := launchW.Write(sp.Encode(req)) launchW.Close() status, rerr := io.ReadAll(statusR) statusR.Close() diff --git a/apps/sandboxio/internal/processservice/service.go b/apps/sandboxio/internal/processservice/service.go index 7b69a8e35..9d9ea1a0d 100644 --- a/apps/sandboxio/internal/processservice/service.go +++ b/apps/sandboxio/internal/processservice/service.go @@ -187,7 +187,7 @@ func (s *Service) Start(_ context.Context, conn *sp.Conn, req sp.StartRequest) ( op.observeLocked(conn, 0) op.mu.Unlock() s.mu.Unlock() - op.launch(req.Spec) + op.launch(req) return sp.StartResponse{Disposition: sp.StartCreated}, nil } diff --git a/apps/sandboxio/internal/processservice/service_test.go b/apps/sandboxio/internal/processservice/service_test.go index 3ee75eca6..bed22bb22 100644 --- a/apps/sandboxio/internal/processservice/service_test.go +++ b/apps/sandboxio/internal/processservice/service_test.go @@ -642,7 +642,7 @@ func TestRevokeWhileStarting(t *testing.T) { h.svc.active.Add(1) h.svc.mu.Unlock() h.svc.AttachmentRevoked(h.att) - op.launch(spec) + op.launch(sp.StartRequest{OperationRef: sp.OperationRef{ServerInstanceID: h.svc.instance, OperationID: key.operation}, Spec: spec}) for deadline := time.Now().Add(10 * time.Second); op.inspect().State != sp.StateExited; time.Sleep(10 * time.Millisecond) { if time.Now().After(deadline) { t.Fatal("the launched operation was not cancelled") diff --git a/apps/sandboxio/internal/processservice/trampoline.go b/apps/sandboxio/internal/processservice/trampoline.go index ebf5194b0..5dd2a2005 100644 --- a/apps/sandboxio/internal/processservice/trampoline.go +++ b/apps/sandboxio/internal/processservice/trampoline.go @@ -3,9 +3,9 @@ package processservice import ( - "bytes" "encoding/binary" "errors" + "io" "os" "os/signal" "runtime" @@ -15,19 +15,20 @@ import ( "golang.org/x/sys/unix" - "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxprocess" - "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxwire" + sp "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxprocess" ) // os/exec cannot set a child's umask, so every launch re-executes the service -// binary as a trampoline. It reads the launch from an inherited descriptor, -// applies the umask and working directory, and execs the target. The marker -// is its whole environment and argv; it is not a command-line interface. +// binary as a trampoline. It reads the operation's Start request from an +// inherited descriptor, applies the umask and working directory, and execs the +// target. The marker is its whole environment and argv; it is not a +// command-line interface. const ( trampolineArg0 = "oac-process-trampoline" trampolineEnv = "OAC_PROCESS_TRAMPOLINE=1" - // Descriptors the trampoline inherits: the launch, and a close-on-exec - // status pipe that reaches EOF without data exactly when exec succeeds. + // Descriptors the trampoline inherits: the Start request, and a + // close-on-exec status pipe that reaches EOF without data exactly when + // exec succeeds. launchFD = 3 statusFD = 4 ) @@ -64,31 +65,22 @@ func errnoOf(err error) syscall.Errno { func trampoline() (uint16, error) { unix.CloseOnExec(statusFD) - var buf bytes.Buffer - chunk := make([]byte, 64<<10) - for { - n, err := unix.Read(launchFD, chunk) - if err == unix.EINTR { - continue - } - if err != nil { - return stageLaunch, err - } - if n == 0 { - break - } - buf.Write(chunk[:n]) + f := os.NewFile(launchFD, "launch") + b, err := io.ReadAll(f) + f.Close() + if err != nil { + return stageLaunch, err } - unix.Close(launchFD) - l, err := decodeLaunch(buf.Bytes()) + m, err := sp.Decode(sp.OpStart, b) if err != nil { return stageLaunch, syscall.EINVAL } + spec := m.(sp.StartRequest).Spec if err := closeInherited(); err != nil { return stageDescriptors, err } - unix.Umask(int(l.umask)) - if err := unix.Chdir(l.cwd); err != nil { + unix.Umask(int(spec.Umask)) + if err := unix.Chdir(string(spec.Cwd)); err != nil { return stageChdir, err } // The Go runtime keeps SIGHUP and SIGINT ignored when it starts with them @@ -96,7 +88,7 @@ func trampoline() (uint16, error) { signal.Notify(make(chan os.Signal, 1), syscall.SIGHUP, syscall.SIGINT) var none unix.Sigset_t unix.PthreadSigmask(unix.SIG_SETMASK, &none, nil) - return stageExec, execTarget(l) + return stageExec, execTarget(spec) } // closeInherited makes exec close every descriptor from 3 up, so the target @@ -116,21 +108,34 @@ func closeInherited() error { return nil } -// execTarget execs the executable, searching PATH for a name without '/' the -// way execvpe does. -func execTarget(l launch) error { - if strings.Contains(l.executable, "/") { - return unix.Exec(l.executable, l.argv, l.env) +// execTarget execs the spec with TERM added for a terminal, searching PATH +// for a name without '/' the way execvpe does. Decode guarantees PATH then. +func execTarget(spec sp.ProcessSpec) error { + argv := make([]string, len(spec.Argv)) + for i, a := range spec.Argv { + argv[i] = string(a) + } + var env []string + var path string + for _, v := range spec.Env { + env = append(env, string(v.Name)+"="+string(v.Value)) + if string(v.Name) == "PATH" { + path = string(v.Value) + } + } + if spec.PTY != nil { + env = append(env, "TERM="+string(spec.PTY.Term)) } - if !l.hasPath { - return unix.ENOENT + exe := string(spec.Executable) + if strings.Contains(exe, "/") { + return unix.Exec(exe, argv, env) } denied := false - for _, dir := range strings.Split(l.path, ":") { + for _, dir := range strings.Split(path, ":") { if dir == "" { dir = "." } - err := unix.Exec(dir+"/"+l.executable, l.argv, l.env) + err := unix.Exec(dir+"/"+exe, argv, env) switch err { case unix.EACCES: denied = true @@ -144,86 +149,3 @@ func execTarget(l launch) error { } return unix.ENOENT } - -// launch is what the trampoline needs from a ProcessSpec. -type launch struct { - executable string - argv []string - env []string - cwd string - umask uint32 - hasPath bool - path string -} - -func encodeLaunch(spec sandboxprocess.ProcessSpec) []byte { - var e sandboxwire.Encoder - e.Bytes(spec.Executable) - e.Count(len(spec.Argv)) - for _, a := range spec.Argv { - e.Bytes(a) - } - env := make([][]byte, 0, len(spec.Env)+1) - var path []byte - for _, v := range spec.Env { - env = append(env, bytes.Join([][]byte{v.Name, v.Value}, []byte("="))) - if string(v.Name) == "PATH" { - path = v.Value - } - } - if spec.PTY != nil { - env = append(env, append([]byte("TERM="), spec.PTY.Term...)) - } - e.Count(len(env)) - for _, v := range env { - e.Bytes(v) - } - e.Bytes(spec.Cwd) - e.U32(spec.Umask) - e.Present(path != nil) - if path != nil { - e.Bytes(path) - } - return e.Payload() -} - -func decodeLaunch(b []byte) (launch, error) { - d := sandboxwire.NewDecoder(b) - var l launch - var err error - str := func() string { - var v []byte - if err == nil { - v, err = d.Bytes() - } - return string(v) - } - list := func() []string { - var n int - if err == nil { - n, err = d.Count(sandboxwire.MaxPayload) - } - out := make([]string, n) - for i := range out { - out[i] = str() - } - return out - } - l.executable = str() - l.argv = list() - l.env = list() - l.cwd = str() - if err == nil { - l.umask, err = d.U32() - } - if err == nil { - l.hasPath, err = d.Present() - } - if l.hasPath { - l.path = str() - } - if err == nil { - err = d.Finish() - } - return l, err -} diff --git a/apps/sandboxio/internal/sandboxio/sandboxio.go b/apps/sandboxio/internal/sandboxio/sandboxio.go index 1615235e2..50ce77242 100644 --- a/apps/sandboxio/internal/sandboxio/sandboxio.go +++ b/apps/sandboxio/internal/sandboxio/sandboxio.go @@ -10,6 +10,7 @@ package sandboxio import ( "context" "crypto/tls" + "fmt" "log" "sync/atomic" "time" @@ -26,26 +27,6 @@ import ( "github.com/MiniMax-AI/OpenAgentCore/internal/sandboxwire" ) -// Step names the startup step a StartupError failed in. -type Step string - -const ( - StepSubreaper Step = "become a child subreaper" - StepBootstrap Step = "read the bootstrap file" - StepProcessService Step = "start the process service" - StepFileService Step = "start the file service" -) - -// StartupError is a failure before the service serves. Its message names the -// step and never includes the credential. -type StartupError struct { - Step Step - Err error -} - -func (e *StartupError) Error() string { return string(e.Step) + ": " + e.Err.Error() } -func (e *StartupError) Unwrap() error { return e.Err } - // shutdownMargin is how long Shutdown waits past the grace limit for killed // processes to be observed gone. const shutdownMargin = 5 * time.Second @@ -53,8 +34,9 @@ const shutdownMargin = 5 * time.Second // Run serves the sandbox the bootstrap file at bootstrapPath names, with the // export world rooted at "/", until ctx ends or the relay refuses the link // for good. The caller has made the process a child subreaper running -// processservice.Reap. Run returns nil when ctx ended, a *StartupError when -// it could not start, and otherwise the relay's *sandboxlink.Error. +// processservice.Reap. Run returns nil when ctx ended, an error naming the +// step and never the credential when it could not start, and otherwise the +// relay's *sandboxlink.Error. func Run(ctx context.Context, bootstrapPath string) error { return run(ctx, bootstrapPath, options{root: "/"}) } @@ -69,20 +51,20 @@ type options struct { func run(ctx context.Context, bootstrapPath string, opt options) error { raw, err := runtimefs.ReadPrivatePath(bootstrapPath, sandboxbootstrap.MaxBytes) if err != nil { - return &StartupError{StepBootstrap, err} + return fmt.Errorf("read the bootstrap file: %w", err) } in, err := sandboxbootstrap.Decode(raw) if err != nil { - return &StartupError{StepBootstrap, err} + return fmt.Errorf("read the bootstrap file: %w", err) } procCfg := processservice.DefaultConfig() procs, err := processservice.New(procCfg) if err != nil { - return &StartupError{StepProcessService, err} + return fmt.Errorf("start the process service: %w", err) } files, err := fileservice.New(opt.root) if err != nil { - return &StartupError{StepFileService, err} + return fmt.Errorf("start the file service: %w", err) } defer files.Close() fileServer := sandboxfs.NewServer(files) diff --git a/apps/sandboxio/internal/sandboxio/sandboxio_test.go b/apps/sandboxio/internal/sandboxio/sandboxio_test.go index 8a9843de5..4afafd42d 100644 --- a/apps/sandboxio/internal/sandboxio/sandboxio_test.go +++ b/apps/sandboxio/internal/sandboxio/sandboxio_test.go @@ -263,3 +263,22 @@ func TestRefusedCredentialEndsTheService(t *testing.T) { t.Fatalf("run: %v, want AuthenticationFailed without the credential", err) } } + +// A failure before serving names its step, and the message never carries the +// credential. +func TestStartupFailureNamesTheStep(t *testing.T) { + valid, _ := writeBootstrap(t, "wss://relay.invalid/link", "serve-credential") + invalid := filepath.Join(t.TempDir(), "bootstrap.json") + if err := os.WriteFile(invalid, []byte(`{"version":1,"credential":"serve-credential"}`), 0o600); err != nil { + t.Fatal(err) + } + for _, c := range []struct{ bootstrap, root, want string }{ + {invalid, t.TempDir(), "read the bootstrap file: "}, + {valid, filepath.Join(t.TempDir(), "missing"), "start the file service: "}, + } { + err := run(context.Background(), c.bootstrap, options{root: c.root}) + if err == nil || !strings.HasPrefix(err.Error(), c.want) || strings.Contains(err.Error(), "serve-credential") { + t.Errorf("run: %v, want %q without the credential", err, c.want) + } + } +}