diff --git a/README.md b/README.md index e58ec0e..72aa0dc 100644 --- a/README.md +++ b/README.md @@ -22,3 +22,39 @@ To run all features appropriate for a given context, use component groups: * `--run-agent` * implies `--run-vdagent --run-rpc` * example usage: [`tart-guest-agent.plist`](https://github.com/cirruslabs/macos-image-templates/blob/main/data/tart-guest-agent.plist) + +## Wrapping RPC commands + +An image administrator can configure a fixed command prefix with repeated +`--exec-wrapper` flags. The guest agent appends the requested executable and its +arguments without shell interpolation. The prefix applies to every Exec RPC, +including interactive, PTY, detached, and user-override commands; clients cannot +disable it. With no prefix, execution is unchanged. + +For example, a managed image can add an environment variable to every command: + +```sh +tart-guest-agent --run-agent \ + --exec-wrapper=/usr/bin/env \ + --exec-wrapper=-- \ + --exec-wrapper=MANAGED_IMAGE=example +``` + +The first argument must be an absolute path to a regular file that the guest +agent's effective user can execute. Invalid configuration stops startup. A wrapper +that cannot start never falls back to running the requested command directly. +Attached commands return the wrapper's +exit status; detached commands retain their existing process-start acknowledgment. +Use a wrapper that replaces itself with the command so signals and exit handling +retain their usual behavior. + +The wrapper receives the command name unchanged and handles its executable +lookup. Include its end-of-options marker in the prefix when its interface +requires one. + +The wrapper runs with the command's requested environment, working directory, +and user. A requested user must also be able to execute the wrapper. Keep its +executable, configuration, and launch settings under the image administrator's +control, and choose a wrapper whose behavior remains correct +under those overrides. Only commands started through the guest agent use this +prefix. diff --git a/internal/command/root.go b/internal/command/root.go index 9559b4a..c1e8281 100644 --- a/internal/command/root.go +++ b/internal/command/root.go @@ -26,6 +26,7 @@ import ( var resizeDisk bool var runVdagent bool var runRPC bool +var execWrapper []string var runDaemon bool var runAgent bool @@ -56,6 +57,8 @@ func NewRootCommand() *cobra.Command { cmd.Flags().BoolVar(&runVdagent, "run-vdagent", false, "run vdagent") cmd.Flags().BoolVar(&runRPC, "run-rpc", false, "run RPC service (currently required "+ "to support \"tart exec\" functionality)") + cmd.Flags().StringArrayVar(&execWrapper, "exec-wrapper", nil, + "argv prefix for every RPC command; repeat for each argument (first must be an absolute executable path)") // Component groups cmd.Flags().BoolVar(&runDaemon, "run-daemon", false, "identical to running the agent"+ @@ -90,6 +93,13 @@ func run(cmd *cobra.Command, args []string) error { zap.S().Infof("running on Tart %s, proceeding...", version.String()) } + if len(execWrapper) > 0 && !runRPC { + return errors.New("--exec-wrapper requires --run-rpc or --run-agent") + } + if err := rpc.ValidateExecWrapper(execWrapper); err != nil { + return err + } + // Perform disk resizing if resizeDisk { zap.S().Info("attempting to resize disk...") @@ -188,7 +198,7 @@ func runRPCOnce(ctx context.Context) error { } defer listener.Close() - rpcServer, err := rpc.New(listener) + rpcServer, err := rpc.New(listener, execWrapper...) if err != nil { zap.S().Errorf("failed to initialize RPC server: %v", err) diff --git a/internal/command/root_test.go b/internal/command/root_test.go new file mode 100644 index 0000000..25ded6d --- /dev/null +++ b/internal/command/root_test.go @@ -0,0 +1,30 @@ +package command_test + +import ( + "testing" + + "github.com/cirruslabs/tart-guest-agent/internal/command" + "github.com/stretchr/testify/require" +) + +func TestExecWrapperFlagPreservesArguments(t *testing.T) { + cmd := command.NewRootCommand() + require.NoError(t, cmd.ParseFlags([]string{ + "--run-rpc", + "--exec-wrapper=/usr/bin/env", + "--exec-wrapper=--", + "--exec-wrapper=NAME=value,with spaces", + "--exec-wrapper=", + "--exec-wrapper=literal $HOME; $(false)", + })) + argv, err := cmd.Flags().GetStringArray("exec-wrapper") + require.NoError(t, err) + require.Equal(t, []string{ + "/usr/bin/env", "--", "NAME=value,with spaces", "", "literal $HOME; $(false)", + }, argv) + + defaultCommand := command.NewRootCommand() + argv, err = defaultCommand.Flags().GetStringArray("exec-wrapper") + require.NoError(t, err) + require.Empty(t, argv) +} diff --git a/internal/rpc/exec.go b/internal/rpc/exec.go index 1a59ff5..259b8dd 100644 --- a/internal/rpc/exec.go +++ b/internal/rpc/exec.go @@ -62,8 +62,8 @@ func (rpc *RPC) Exec(stream grpc.BidiStreamingServer[v1.ExecRequest, v1.ExecResp execCtx = context.Background() } - cmd := exec.CommandContext(execCtx, firstExecRequestCommand.Command.Name, - firstExecRequestCommand.Command.Args...) + cmd := rpc.execCommand(execCtx, firstExecRequestCommand.Command.GetName(), + firstExecRequestCommand.Command.GetArgs()) cmd.SysProcAttr = &syscall.SysProcAttr{} diff --git a/internal/rpc/exec_grpc_test.go b/internal/rpc/exec_grpc_test.go index 301dcc5..ac3fffa 100644 --- a/internal/rpc/exec_grpc_test.go +++ b/internal/rpc/exec_grpc_test.go @@ -25,9 +25,24 @@ import ( ) func TestExecGRPCBackgroundProcessLifetime(t *testing.T) { + testExecGRPCBackgroundProcessLifetime(t) +} + +func TestExecWrapperGRPCBackgroundProcessLifetime(t *testing.T) { + receipt := filepath.Join(t.TempDir(), "wrapper-ran") + wrapper := writeExecWrapper(t, `printf wrapped > "$1" +shift +exec "$@" +`) + testExecGRPCBackgroundProcessLifetime(t, wrapper, receipt) + require.FileExists(t, receipt) +} + +func testExecGRPCBackgroundProcessLifetime(t *testing.T, execWrapper ...string) { + t.Helper() for _, mode := range []string{"normal exit", "cancel after exit", "cancel while running"} { t.Run(mode, func(t *testing.T) { - client := v1.NewAgentClient(newExecGRPCTestConn(t)) + client := v1.NewAgentClient(newExecGRPCTestConn(t, execWrapper...)) ctx, cancel := context.WithTimeout(t.Context(), execTestTimeout) defer cancel() @@ -93,9 +108,9 @@ while [ ! -f "$RELEASE_FILE" ]; do sleep 0.01; done`}, } } -func newExecGRPCTestConn(t *testing.T) *grpc.ClientConn { +func newExecGRPCTestConn(t *testing.T, execWrapper ...string) *grpc.ClientConn { listener := bufconn.Listen(1024 * 1024) - agent, err := New(listener) + agent, err := New(listener, execWrapper...) require.NoError(t, err) serveResult := make(chan error, 1) diff --git a/internal/rpc/exec_test.go b/internal/rpc/exec_test.go index 877ac8a..43bf8ad 100644 --- a/internal/rpc/exec_test.go +++ b/internal/rpc/exec_test.go @@ -290,6 +290,18 @@ func startExecTest( configure ...func(*execTestStream), ) (*RPC, *execTestStream, <-chan error) { t.Helper() + rpc, err := New(nil) + require.NoError(t, err) + return startExecTestWithRPC(t, rpc, command, configure...) +} + +func startExecTestWithRPC( + t *testing.T, + rpc *RPC, + command *v1.ExecRequest_Command, + configure ...func(*execTestStream), +) (*RPC, *execTestStream, <-chan error) { + t.Helper() ctx, cancel := context.WithCancel(context.Background()) t.Cleanup(cancel) @@ -297,8 +309,6 @@ func startExecTest( for _, configureStream := range configure { configureStream(stream) } - rpc, err := New(nil) - require.NoError(t, err) result := make(chan error, 1) go func() { result <- rpc.Exec(stream) diff --git a/internal/rpc/exec_wrapper.go b/internal/rpc/exec_wrapper.go new file mode 100644 index 0000000..970436a --- /dev/null +++ b/internal/rpc/exec_wrapper.go @@ -0,0 +1,60 @@ +package rpc + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + + "golang.org/x/sys/unix" +) + +// ValidateExecWrapper checks the administrator-configured argv prefix. +// The wrapper itself is responsible for performing its work and executing the +// appended command. No request can change this prefix. +func ValidateExecWrapper(argv []string) error { + if len(argv) == 0 { + return nil + } + + if !filepath.IsAbs(argv[0]) { + return errors.New("exec wrapper executable must be an absolute path") + } + + for _, arg := range argv { + if strings.ContainsRune(arg, '\x00') { + return errors.New("exec wrapper arguments must not contain NUL") + } + } + + info, err := os.Stat(argv[0]) + if err != nil { + return fmt.Errorf("invalid exec wrapper executable: %w", err) + } + + if !info.Mode().IsRegular() { + return errors.New("exec wrapper must name an executable file") + } + if err := unix.Faccessat(unix.AT_FDCWD, argv[0], unix.X_OK, unix.AT_EACCESS); err != nil { + return fmt.Errorf("exec wrapper is not executable by the guest agent: %w", err) + } + + return nil +} + +func (rpc *RPC) execCommand(ctx context.Context, name string, args []string) *exec.Cmd { + if len(rpc.execWrapper) == 0 { + return exec.CommandContext(ctx, name, args...) // #nosec G204 -- Executing RPC commands is this service's purpose. + } + + wrappedArgs := make([]string, 0, len(rpc.execWrapper)+len(args)) + wrappedArgs = append(wrappedArgs, rpc.execWrapper[1:]...) + wrappedArgs = append(wrappedArgs, name) + wrappedArgs = append(wrappedArgs, args...) + + // #nosec G204 -- Fixed, validated administrator prefix. + return exec.CommandContext(ctx, rpc.execWrapper[0], wrappedArgs...) +} diff --git a/internal/rpc/exec_wrapper_test.go b/internal/rpc/exec_wrapper_test.go new file mode 100644 index 0000000..b37001a --- /dev/null +++ b/internal/rpc/exec_wrapper_test.go @@ -0,0 +1,181 @@ +//nolint:testpackage +package rpc + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "syscall" + "testing" + "time" + + "github.com/cirruslabs/tart-guest-agent/pkg/v1" + "github.com/stretchr/testify/require" +) + +func TestValidateExecWrapper(t *testing.T) { + notExecutable := filepath.Join(t.TempDir(), "not-executable") + require.NoError(t, os.WriteFile(notExecutable, nil, 0o600)) + + for _, argv := range [][]string{ + {""}, {"sh"}, {"/missing/tart-exec-wrapper"}, {t.TempDir()}, + {notExecutable}, {execTestShell, "bad\x00argument"}, + } { + require.Error(t, ValidateExecWrapper(argv), "%q", argv) + _, err := New(nil, argv...) + require.Error(t, err) + } + + require.NoError(t, ValidateExecWrapper(nil)) + require.NoError(t, ValidateExecWrapper([]string{execTestShell, "", "literal,$value"})) +} + +func TestValidateExecWrapperRequiresApplicableExecutePermission(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root can execute a file with any execute bit set") + } + + path := filepath.Join(t.TempDir(), "other-executable") + contents, err := os.ReadFile("/usr/bin/true") + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, contents, 0o600)) // #nosec G703 -- Destination is inside t.TempDir. + require.NoError(t, os.Chmod(path, 0o001)) // #nosec G302 -- Tests inapplicable execute permission. + // The current user owns this file, so its other-execute bit does not apply. + // #nosec G204 -- Path is an owned permission-test fixture. + require.ErrorIs(t, exec.CommandContext(t.Context(), path).Run(), os.ErrPermission) + require.ErrorIs(t, ValidateExecWrapper([]string{path}), os.ErrPermission) + _, err = New(nil, path) + require.ErrorIs(t, err, os.ErrPermission) +} + +func TestExecWrapperPreservesArgumentsAndOverrides(t *testing.T) { + workdir := t.TempDir() + canonicalWorkdir, err := filepath.EvalSymlinks(workdir) + require.NoError(t, err) + wrapper := writeExecWrapper(t, `printf '%s\n' "$1" +shift +exec "$@" +`) + prefix := []string{wrapper, "fixed,argument $HOME; $(false)"} + rpc, err := New(nil, prefix...) + require.NoError(t, err) + prefix[0] = "/missing/changed-after-startup" + prefix[1] = "changed-after-startup" + + _, stream, result := startExecTestWithRPC(t, rpc, &v1.ExecRequest_Command{ + Name: execTestShell, + Args: []string{"-c", `printf '%s\n' "$PWD" "$WRAPPER_TEST_VALUE" "$1"; id -u`, "test", "literal; $(false)"}, + Env: map[string]string{"WRAPPER_TEST_VALUE": "value with spaces"}, + Workdir: workdir, + User: strconv.Itoa(os.Getuid()), + }) + output, exitCode := collectExecWrapperResult(t, stream) + require.Zero(t, exitCode) + require.Equal(t, fmt.Sprintf("fixed,argument $HOME; $(false)\n%s\nvalue with spaces\nliteral; $(false)\n%d\n", + canonicalWorkdir, os.Getuid()), output) + require.NoError(t, receiveExecResult(t, result)) +} + +func TestExecWrapperDeniesEveryExecutionMode(t *testing.T) { + const detachedMode = "detached" + for _, mode := range []string{"normal", "interactive", "pty", detachedMode} { + t.Run(mode, func(t *testing.T) { + directory := t.TempDir() + receipt := filepath.Join(directory, "wrapper-ran") + target := filepath.Join(directory, "target-ran") + wrapper := writeExecWrapper(t, `printf denied > "$WRAPPER_RECEIPT" +exit 86 +`) + rpc, err := New(nil, wrapper) + require.NoError(t, err) + _, stream, result := startExecTestWithRPC(t, rpc, &v1.ExecRequest_Command{ + Name: execTestShell, + Args: []string{"-c", `printf unrestricted > "$TARGET_RECEIPT"`}, + Env: map[string]string{"WRAPPER_RECEIPT": receipt, "TARGET_RECEIPT": target}, + Interactive: mode == "interactive", + Tty: mode == "pty", + Detach: mode == detachedMode, + }) + _, exitCode := collectExecWrapperResult(t, stream) + if mode == detachedMode { + require.Zero(t, exitCode) // Detached success acknowledges process creation. + } else { + require.EqualValues(t, 86, exitCode) + } + require.NoError(t, receiveExecResult(t, result)) + require.Eventually(t, func() bool { _, err := os.Stat(receipt); return err == nil }, + execTestTimeout, 10*time.Millisecond) + _, err = os.Stat(target) + require.ErrorIs(t, err, os.ErrNotExist) + }) + } +} + +func TestExecWrapperDisappearingDoesNotRunTarget(t *testing.T) { + wrapper := writeExecWrapper(t, "exec \"$@\"\n") + rpc, err := New(nil, wrapper) + require.NoError(t, err) + require.NoError(t, os.Remove(wrapper)) + + _, stream, result := startExecTestWithRPC(t, rpc, &v1.ExecRequest_Command{Name: "/usr/bin/true"}) + response := receiveExecResponse(t, stream) + require.Nil(t, response.GetStarted()) + require.EqualValues(t, execRuntimeFailureExitCode, response.GetExit().GetCode()) + require.NoError(t, receiveExecResult(t, result)) +} + +func TestExecWrapperSignalsProcessGroup(t *testing.T) { + for _, tty := range []bool{false, true} { + t.Run(fmt.Sprintf("tty=%t", tty), func(t *testing.T) { + wrapper := writeExecWrapper(t, "exec \"$@\"\n") + rpc, err := New(nil, wrapper) + require.NoError(t, err) + _, stream, result := startExecTestWithRPC(t, rpc, &v1.ExecRequest_Command{ + Name: execTestShell, + Args: []string{"-c", "sleep 30 & printf ready; wait"}, + Tty: tty, + }) + started := receiveExecResponse(t, stream).GetStarted() + require.NotNil(t, started) + require.Equal(t, []byte("ready"), receiveExecResponse(t, stream).GetStandardOutput().GetData()) + + _, err = rpc.Signal(context.Background(), &v1.SignalRequest{ + ExecId: started.GetExecId(), + Signal: v1.SignalRequest_SIGNAL_SIGTERM, + }) + require.NoError(t, err) + response := receiveExecResponse(t, stream) + require.EqualValues(t, signalExitCodeOffset+syscall.SIGTERM, response.GetExit().GetCode()) + require.NoError(t, receiveExecResult(t, result)) + }) + } +} + +func writeExecWrapper(t *testing.T, body string) string { + path := filepath.Join(t.TempDir(), "wrapper") + require.NoError(t, os.WriteFile(path, []byte("#!/bin/sh\n"+body), 0o600)) + require.NoError(t, os.Chmod(path, 0o700)) // #nosec G302 -- This fixture must be executable. + return path +} + +func collectExecWrapperResult(t *testing.T, stream *execTestStream) (string, int32) { + require.NotNil(t, receiveExecResponse(t, stream).GetStarted()) + var output strings.Builder + for { + response := receiveExecResponse(t, stream) + switch value := response.GetType().(type) { + case *v1.ExecResponse_StandardOutput: + output.Write(value.StandardOutput.GetData()) + case *v1.ExecResponse_StandardError: + output.Write(value.StandardError.GetData()) + case *v1.ExecResponse_Exit_: + return output.String(), value.Exit.GetCode() + default: + t.Fatalf("unexpected exec response %T", value) + } + } +} diff --git a/internal/rpc/rpc.go b/internal/rpc/rpc.go index 198804d..2e9c729 100644 --- a/internal/rpc/rpc.go +++ b/internal/rpc/rpc.go @@ -4,6 +4,7 @@ import ( "context" "net" "os" + "slices" "github.com/cirruslabs/tart-guest-agent/pkg/v1" "github.com/puzpuzpuz/xsync/v4" @@ -13,16 +14,22 @@ import ( type RPC struct { v1.UnimplementedAgentServer - grpcServer *grpc.Server - listener net.Listener - execs *xsync.Map[string, *os.Process] + grpcServer *grpc.Server + listener net.Listener + execs *xsync.Map[string, *os.Process] + execWrapper []string } -func New(listener net.Listener) (*RPC, error) { +func New(listener net.Listener, execWrapper ...string) (*RPC, error) { + if err := ValidateExecWrapper(execWrapper); err != nil { + return nil, err + } + rpc := &RPC{ - grpcServer: grpc.NewServer(), - listener: listener, - execs: xsync.NewMap[string, *os.Process](), + grpcServer: grpc.NewServer(), + listener: listener, + execs: xsync.NewMap[string, *os.Process](), + execWrapper: slices.Clone(execWrapper), } v1.RegisterAgentServer(rpc.grpcServer, rpc)