From 0fa06245761133a463b12a1d9a2d595295a947b9 Mon Sep 17 00:00:00 2001 From: Piotr Kazmierczak <470696+pkazmierczak@users.noreply.github.com> Date: Fri, 16 May 2025 15:02:45 +0200 Subject: [PATCH] exec: Fix incorrect `HOME` and `USER` env variables for tasks that have `user` set (#25859) Co-authored-by: Tim Gross --- .changelog/25859.txt | 3 + client/testutil/driver_compatible.go | 10 ++++ drivers/shared/executor/executor_basic.go | 2 - drivers/shared/executor/executor_linux.go | 9 +++ .../shared/executor/executor_linux_test.go | 34 +++++++++++ .../executor/executor_universal_linux.go | 49 ---------------- drivers/shared/executor/executor_unix.go | 56 +++++++++++++++++++ 7 files changed, 112 insertions(+), 51 deletions(-) create mode 100644 .changelog/25859.txt diff --git a/.changelog/25859.txt b/.changelog/25859.txt new file mode 100644 index 000000000..21437da00 --- /dev/null +++ b/.changelog/25859.txt @@ -0,0 +1,3 @@ +```release-note:bug +exec: Adjust USER and HOME env vars when user value is set +``` diff --git a/client/testutil/driver_compatible.go b/client/testutil/driver_compatible.go index 56563198e..f1ef1b8c2 100644 --- a/client/testutil/driver_compatible.go +++ b/client/testutil/driver_compatible.go @@ -62,6 +62,16 @@ func RequireLinux(t *testing.T) { } } +// RequireCILinux skips tests unless: +// - running on Linux +// - running on GHA +func RequireCILinux(t *testing.T) { + u, _ := user.Current() + if runtime.GOOS != "linux" || u.Username != "runner" { + t.Skip("Test requires Linux and Github CI runner") + } +} + // RequireNotWindows skips tests whenever: // - running on Windows func RequireNotWindows(t *testing.T) { diff --git a/drivers/shared/executor/executor_basic.go b/drivers/shared/executor/executor_basic.go index 72f1e21dd..d40967933 100644 --- a/drivers/shared/executor/executor_basic.go +++ b/drivers/shared/executor/executor_basic.go @@ -35,8 +35,6 @@ func withNetworkIsolation(f func() error, _ *drivers.NetworkIsolationSpec) error return f() } -func setCmdUser(*exec.Cmd, string) error { return nil } - func (e *UniversalExecutor) ListProcesses() set.Collection[int] { return procstats.ListByPid(e.childCmd.Process.Pid) } diff --git a/drivers/shared/executor/executor_linux.go b/drivers/shared/executor/executor_linux.go index d727f8555..ed2fc6b44 100644 --- a/drivers/shared/executor/executor_linux.go +++ b/drivers/shared/executor/executor_linux.go @@ -30,6 +30,7 @@ import ( cstructs "github.com/hashicorp/nomad/client/structs" "github.com/hashicorp/nomad/drivers/shared/capabilities" "github.com/hashicorp/nomad/drivers/shared/executor/procstats" + "github.com/hashicorp/nomad/helper/users" "github.com/hashicorp/nomad/helper/uuid" "github.com/hashicorp/nomad/nomad/structs" "github.com/hashicorp/nomad/plugins/drivers" @@ -230,6 +231,14 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro if command.User != "" { process.User = command.User + + // Override HOME and USER environment variables + u, err := users.Lookup(command.User) + if err != nil { + return nil, err + } + process.Env = append(process.Env, fmt.Sprintf("USER=%s", u.Username)) + process.Env = append(process.Env, fmt.Sprintf("HOME=%s", u.HomeDir)) } l.userProc = process diff --git a/drivers/shared/executor/executor_linux_test.go b/drivers/shared/executor/executor_linux_test.go index aabbd4a86..8e997670b 100644 --- a/drivers/shared/executor/executor_linux_test.go +++ b/drivers/shared/executor/executor_linux_test.go @@ -830,6 +830,40 @@ func TestExecutor_WorkDir(t *testing.T) { must.Eq(t, output, workDir) } +// TestExecutor_UserEnv tests that the USER environment variable is set +// correctly if user is set. We're not testing HOME because that could get +// tricky on GHA runners. +func TestExecutor_UserEnv(t *testing.T) { + t.Parallel() + testutil.RequireCILinux(t) + testutil.ExecCompatible(t) + + testExecCmd := testExecutorCommandWithChroot(t) + execCmd, allocDir := testExecCmd.command, testExecCmd.allocDir + execCmd.Cmd = "/bin/bash" + execCmd.Args = []string{"-c", "echo $USER"} + execCmd.User = "runner" + execCmd.ResourceLimits = true + defer allocDir.Destroy() + + executor := NewExecutorWithIsolation(testlog.HCLogger(t), compute) + defer executor.Shutdown("SIGKILL", 0) + + ps, err := executor.Launch(execCmd) + must.NoError(t, err) + must.NonZero(t, ps.Pid) + + state, err := executor.Wait(context.Background()) + must.NoError(t, err) + must.Zero(t, state.ExitCode) + + _, ok := executor.(*LibcontainerExecutor) + must.True(t, ok) + + output := strings.TrimSpace(testExecCmd.stdout.String()) + must.Eq(t, output, "runner") +} + func TestExecCommand_getCgroupOr_off(t *testing.T) { ci.Parallel(t) diff --git a/drivers/shared/executor/executor_universal_linux.go b/drivers/shared/executor/executor_universal_linux.go index 53d042a6e..9fdb0d7cd 100644 --- a/drivers/shared/executor/executor_universal_linux.go +++ b/drivers/shared/executor/executor_universal_linux.go @@ -16,7 +16,6 @@ import ( "github.com/hashicorp/nomad/client/lib/cgroupslib" "github.com/hashicorp/nomad/client/lib/nsutil" "github.com/hashicorp/nomad/drivers/shared/executor/procstats" - "github.com/hashicorp/nomad/helper/users" "github.com/hashicorp/nomad/plugins/drivers" "github.com/opencontainers/runc/libcontainer/cgroups" "golang.org/x/sys/unix" @@ -28,54 +27,6 @@ const ( memoryNoLimit = -1 ) -// setCmdUser takes a user id as a string and looks up the user, and sets the command -// to execute as that user. -func setCmdUser(cmd *exec.Cmd, userid string) error { - u, err := users.Lookup(userid) - if err != nil { - return fmt.Errorf("failed to identify user %v: %v", userid, err) - } - - // Get the groups the user is a part of - gidStrings, err := u.GroupIds() - if err != nil { - return fmt.Errorf("unable to lookup user's group membership: %v", err) - } - - gids := make([]uint32, len(gidStrings)) - for _, gidString := range gidStrings { - u, err := strconv.ParseUint(gidString, 10, 32) - if err != nil { - return fmt.Errorf("unable to convert user's group to uint32 %s: %v", gidString, err) - } - - gids = append(gids, uint32(u)) - } - - // Convert the uid and gid - uid, err := strconv.ParseUint(u.Uid, 10, 32) - if err != nil { - return fmt.Errorf("unable to convert userid to uint32: %s", err) - } - gid, err := strconv.ParseUint(u.Gid, 10, 32) - if err != nil { - return fmt.Errorf("unable to convert groupid to uint32: %s", err) - } - - // Set the command to run as that user and group. - if cmd.SysProcAttr == nil { - cmd.SysProcAttr = &syscall.SysProcAttr{} - } - if cmd.SysProcAttr.Credential == nil { - cmd.SysProcAttr.Credential = &syscall.Credential{} - } - cmd.SysProcAttr.Credential.Uid = uint32(uid) - cmd.SysProcAttr.Credential.Gid = uint32(gid) - cmd.SysProcAttr.Credential.Groups = gids - - return nil -} - // setSubCmdCgroup sets the cgroup for non-Task child processes of the // executor.Executor (since in cg2 it lives outside the task's cgroup) func (e *UniversalExecutor) setSubCmdCgroup(cmd *exec.Cmd, cgroup string) (func(), error) { diff --git a/drivers/shared/executor/executor_unix.go b/drivers/shared/executor/executor_unix.go index 0164d8c51..c241a3bc4 100644 --- a/drivers/shared/executor/executor_unix.go +++ b/drivers/shared/executor/executor_unix.go @@ -8,7 +8,11 @@ package executor import ( "fmt" "os" + "os/exec" + "strconv" "syscall" + + "github.com/hashicorp/nomad/helper/users" ) // configure new process group for child process @@ -51,3 +55,55 @@ func (e *UniversalExecutor) shutdownProcess(sig os.Signal, proc *os.Process) err return nil } + +// setCmdUser takes a user id as a string and looks up the user, and sets the command +// to execute as that user. +func setCmdUser(cmd *exec.Cmd, userid string) error { + u, err := users.Lookup(userid) + if err != nil { + return fmt.Errorf("failed to identify user %v: %v", userid, err) + } + + // Get the groups the user is a part of + gidStrings, err := u.GroupIds() + if err != nil { + return fmt.Errorf("unable to lookup user's group membership: %v", err) + } + + gids := make([]uint32, len(gidStrings)) + for _, gidString := range gidStrings { + u, err := strconv.ParseUint(gidString, 10, 32) + if err != nil { + return fmt.Errorf("unable to convert user's group to uint32 %s: %v", gidString, err) + } + + gids = append(gids, uint32(u)) + } + + // Convert the uid and gid + uid, err := strconv.ParseUint(u.Uid, 10, 32) + if err != nil { + return fmt.Errorf("unable to convert userid to uint32: %w", err) + } + gid, err := strconv.ParseUint(u.Gid, 10, 32) + if err != nil { + return fmt.Errorf("unable to convert groupid to uint32: %s", err) + } + + // Set the command to run as that user and group. + if cmd.SysProcAttr == nil { + cmd.SysProcAttr = &syscall.SysProcAttr{} + } + if cmd.SysProcAttr.Credential == nil { + cmd.SysProcAttr.Credential = &syscall.Credential{} + } + cmd.SysProcAttr.Credential.Uid = uint32(uid) + cmd.SysProcAttr.Credential.Gid = uint32(gid) + cmd.SysProcAttr.Credential.Groups = gids + + // Override HOME and USER environment variables + cmd.Env = append(cmd.Env, fmt.Sprintf("USER=%s", u.Username)) + cmd.Env = append(cmd.Env, fmt.Sprintf("HOME=%s", u.HomeDir)) + + return nil +}