From 743db9e15b7c7bf7ce9ce3b9e8699162bb4c217e Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Tue, 10 Oct 2023 20:53:28 +0000 Subject: [PATCH 1/3] fix: properly handle shebangs by writing files See https://github.com/coder/coder/pull/10134#discussion_r1351841004 --- agent/agent.go | 6 +++-- agent/agentscripts/agentscripts.go | 3 ++- agent/agentssh/agentssh.go | 42 ++++++++++++++++++++++-------- agent/agentssh/agentssh_test.go | 7 +++-- 4 files changed, 42 insertions(+), 16 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 109b4ad90c1ec..09689e241263f 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -292,11 +292,12 @@ func (a *agent) collectMetadata(ctx context.Context, md codersdk.WorkspaceAgentM // if it can guarantee the clocks are synchronized. CollectedAt: now, } - cmdPty, err := a.sshServer.CreateCommand(ctx, md.Script, nil) + cmdPty, cleanup, err := a.sshServer.CreateCommand(ctx, md.Script, nil) if err != nil { result.Error = fmt.Sprintf("create cmd: %+v", err) return result } + defer cleanup() cmd := cmdPty.AsExec() cmd.Stdout = &out @@ -1069,7 +1070,7 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, logger slog.Logger, m }() // Empty command will default to the users shell! - cmd, err := a.sshServer.CreateCommand(ctx, msg.Command, nil) + cmd, cleanup, err := a.sshServer.CreateCommand(ctx, msg.Command, nil) if err != nil { a.metrics.reconnectingPTYErrors.WithLabelValues("create_command").Add(1) return xerrors.Errorf("create command: %w", err) @@ -1082,6 +1083,7 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, logger slog.Logger, m if err = a.trackConnGoroutine(func() { rpty.Wait() + cleanup() a.reconnectingPTYs.Delete(msg.ID) }); err != nil { rpty.Close(err) diff --git a/agent/agentscripts/agentscripts.go b/agent/agentscripts/agentscripts.go index 98a6901ebbbc4..8b6166cfb548b 100644 --- a/agent/agentscripts/agentscripts.go +++ b/agent/agentscripts/agentscripts.go @@ -171,10 +171,11 @@ func (r *Runner) run(ctx context.Context, script codersdk.WorkspaceAgentScript) cmdCtx, ctxCancel = context.WithTimeout(ctx, script.Timeout) defer ctxCancel() } - cmdPty, err := r.SSHServer.CreateCommand(cmdCtx, script.Script, nil) + cmdPty, cleanup, err := r.SSHServer.CreateCommand(cmdCtx, script.Script, nil) if err != nil { return xerrors.Errorf("%s script: create command: %w", logPath, err) } + defer cleanup() cmd = cmdPty.AsExec() cmd.SysProcAttr = cmdSysProcAttr() cmd.WaitDelay = 10 * time.Second diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index 19831c0d7caa8..b5ae8467024d0 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -5,6 +5,7 @@ import ( "context" "crypto/rand" "crypto/rsa" + "crypto/sha256" "errors" "fmt" "io" @@ -274,7 +275,7 @@ func (s *Server) sessionStart(session ssh.Session, extraEnv []string) (retErr er magicTypeLabel := magicTypeMetricLabel(magicType) sshPty, windowSize, isPty := session.Pty() - cmd, err := s.CreateCommand(ctx, session.RawCommand(), env) + cmd, cleanup, err := s.CreateCommand(ctx, session.RawCommand(), env) if err != nil { ptyLabel := "no" if isPty { @@ -283,6 +284,7 @@ func (s *Server) sessionStart(session ssh.Session, extraEnv []string) (retErr er s.metrics.sessionErrors.WithLabelValues(magicTypeLabel, ptyLabel, "create_command").Add(1) return err } + defer cleanup() if ssh.AgentRequested(session) { l, err := ssh.NewAgentListener() @@ -493,21 +495,21 @@ func (s *Server) sftpHandler(session ssh.Session) { // CreateCommand processes raw command input with OpenSSH-like behavior. // If the script provided is empty, it will default to the users shell. // This injects environment variables specified by the user at launch too. -func (s *Server) CreateCommand(ctx context.Context, script string, env []string) (*pty.Cmd, error) { +func (s *Server) CreateCommand(ctx context.Context, script string, env []string) (*pty.Cmd, func(), error) { currentUser, err := user.Current() if err != nil { - return nil, xerrors.Errorf("get current user: %w", err) + return nil, nil, xerrors.Errorf("get current user: %w", err) } username := currentUser.Username shell, err := usershell.Get(username) if err != nil { - return nil, xerrors.Errorf("get user shell: %w", err) + return nil, nil, xerrors.Errorf("get user shell: %w", err) } manifest := s.Manifest.Load() if manifest == nil { - return nil, xerrors.Errorf("no metadata was provided") + return nil, nil, xerrors.Errorf("no metadata was provided") } // OpenSSH executes all commands with the users current shell. @@ -518,7 +520,9 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string) } name := shell args := []string{caller, script} - + cleanup := func() { + // Default to noop. This only applies for scripts. + } // A preceding space is generally not idiomatic for a shebang, // but in Terraform it's quite standard to use < 1 { @@ -539,7 +543,23 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string) } else { args = []string{} } - args = append(args, caller, script) + scriptSha := sha256.Sum256([]byte(script)) + tempFile, err := os.CreateTemp("", fmt.Sprintf("coder-script-%x", scriptSha)) + if err != nil { + return nil, nil, xerrors.Errorf("create temp file: %w", err) + } + cleanup = func() { + _ = os.Remove(tempFile.Name()) + } + _, err = tempFile.WriteString(script) + if err != nil { + return nil, nil, xerrors.Errorf("write temp file: %w", err) + } + err = tempFile.Close() + if err != nil { + return nil, nil, xerrors.Errorf("close temp file: %w", err) + } + args = append(args, tempFile.Name()) } // gliderlabs/ssh returns a command slice of zero @@ -563,14 +583,14 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string) // Default to user home if a directory is not set. homedir, err := userHomeDir() if err != nil { - return nil, xerrors.Errorf("get home dir: %w", err) + return nil, nil, xerrors.Errorf("get home dir: %w", err) } cmd.Dir = homedir } cmd.Env = append(os.Environ(), env...) executablePath, err := os.Executable() if err != nil { - return nil, xerrors.Errorf("getting os executable: %w", err) + return nil, nil, xerrors.Errorf("getting os executable: %w", err) } // Set environment variables reliable detection of being inside a // Coder workspace. @@ -615,7 +635,7 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string) cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", envKey, value)) } - return cmd, nil + return cmd, cleanup, nil } func (s *Server) Serve(l net.Listener) (retErr error) { diff --git a/agent/agentssh/agentssh_test.go b/agent/agentssh/agentssh_test.go index b72da96e4ce43..5020af42fab22 100644 --- a/agent/agentssh/agentssh_test.go +++ b/agent/agentssh/agentssh_test.go @@ -90,19 +90,22 @@ func TestNewServer_ExecuteShebang(t *testing.T) { t.Run("Basic", func(t *testing.T) { t.Parallel() - cmd, err := s.CreateCommand(ctx, `#!/bin/bash + cmd, cleanup, err := s.CreateCommand(ctx, `#!/bin/bash echo test`, nil) require.NoError(t, err) + t.Cleanup(cleanup) output, err := cmd.AsExec().CombinedOutput() require.NoError(t, err) require.Equal(t, "test\n", string(output)) }) t.Run("Args", func(t *testing.T) { t.Parallel() - cmd, err := s.CreateCommand(ctx, `#!/usr/bin/env bash + cmd, cleanup, err := s.CreateCommand(ctx, `#!/usr/bin/env bash echo test`, nil) require.NoError(t, err) + t.Cleanup(cleanup) output, err := cmd.AsExec().CombinedOutput() + t.Log(string(output)) require.NoError(t, err) require.Equal(t, "test\n", string(output)) }) From 883ccc754f5eacf17b19118def7ca05e146031cf Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 19 Oct 2023 11:29:27 -0500 Subject: [PATCH 2/3] Update agent/agent.go Co-authored-by: Mathias Fredriksson --- agent/agent.go | 1 + 1 file changed, 1 insertion(+) diff --git a/agent/agent.go b/agent/agent.go index 09689e241263f..89c719894e313 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -1087,6 +1087,7 @@ func (a *agent) handleReconnectingPTY(ctx context.Context, logger slog.Logger, m a.reconnectingPTYs.Delete(msg.ID) }); err != nil { rpty.Close(err) + cleanup() return xerrors.Errorf("start routine: %w", err) } From 8f0257cd38f9fa54a599ae667dd83078cd18d676 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Thu, 19 Oct 2023 11:29:49 -0500 Subject: [PATCH 3/3] Update agent/agentssh/agentssh.go Co-authored-by: Mathias Fredriksson --- agent/agentssh/agentssh.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/agent/agentssh/agentssh.go b/agent/agentssh/agentssh.go index b5ae8467024d0..9a53808b1cdcc 100644 --- a/agent/agentssh/agentssh.go +++ b/agent/agentssh/agentssh.go @@ -544,7 +544,7 @@ func (s *Server) CreateCommand(ctx context.Context, script string, env []string) args = []string{} } scriptSha := sha256.Sum256([]byte(script)) - tempFile, err := os.CreateTemp("", fmt.Sprintf("coder-script-%x", scriptSha)) + tempFile, err := os.CreateTemp("", fmt.Sprintf("coder-script-%x.*", scriptSha)) if err != nil { return nil, nil, xerrors.Errorf("create temp file: %w", err) }