diff --git a/CHANGELOG.md b/CHANGELOG.md index 92e1b23cfde..2ebb6072239 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### libcontainer API * `configs.CommandHook` struct has changed, Command is now a pointer. Also, `configs.NewCommandHook` now accepts a `*Command`. (#4325) + * The `Process` struct has `User` string field replaced with numeric + `UID` and `GID` fields, and `AdditionalGroups` changed its type from + `[]string` to `[]int`. Essentially, resolution of user and group + names to IDs is no longer performed by libcontainer, so if a libcontainer + user previously relied on this feature, now they have to convert names to + IDs before calling libcontainer; it is recommended to use Go package + github.com/moby/sys/user for that. (#3999) ## [1.2.0] - 2024-10-22 diff --git a/libcontainer/container_linux.go b/libcontainer/container_linux.go index 2de3382f6a0..26f523a90da 100644 --- a/libcontainer/container_linux.go +++ b/libcontainer/container_linux.go @@ -302,6 +302,13 @@ func (c *Container) start(process *Process) (retErr error) { if c.config.Cgroups.Resources.SkipDevices { return errors.New("can't start container with SkipDevices set") } + + if c.config.RootlessEUID && len(process.AdditionalGroups) > 0 { + // We cannot set any additional groups in a rootless container + // and thus we bail if the user asked us to do so. + return errors.New("cannot set any additional groups in a rootless container") + } + if process.Init { if c.initProcessStartTime != 0 { return errors.New("container already has init process") @@ -686,7 +693,8 @@ func (c *Container) newInitConfig(process *Process) *initConfig { Config: c.config, Args: process.Args, Env: process.Env, - User: process.User, + UID: process.UID, + GID: process.GID, AdditionalGroups: process.AdditionalGroups, Cwd: process.Cwd, Capabilities: process.Capabilities, diff --git a/libcontainer/env.go b/libcontainer/env.go index d205cb65014..fe1abd500b0 100644 --- a/libcontainer/env.go +++ b/libcontainer/env.go @@ -6,6 +6,9 @@ import ( "os" "slices" "strings" + + "github.com/moby/sys/user" + "github.com/sirupsen/logrus" ) // prepareEnv processes a list of environment variables, preparing it @@ -13,13 +16,13 @@ import ( // - validates each variable is in the NAME=VALUE format and // contains no \0 (nil) bytes; // - removes any duplicates (keeping only the last value for each key) -// - sets PATH for the current process, if found in the list. +// - sets PATH for the current process, if found in the list; +// - adds HOME to returned environment, if not found in the list. // -// It returns the deduplicated environment, a flag telling whether HOME -// is present in the input, and an error. -func prepareEnv(env []string) ([]string, bool, error) { +// Returns the prepared environment. +func prepareEnv(env []string, uid int) ([]string, error) { if env == nil { - return nil, false, nil + return nil, nil } // Deduplication code based on dedupEnv from Go 1.22 os/exec. @@ -31,10 +34,10 @@ func prepareEnv(env []string) ([]string, bool, error) { kv := env[n-1] i := strings.IndexByte(kv, '=') if i == -1 { - return nil, false, errors.New("invalid environment variable: missing '='") + return nil, errors.New("invalid environment variable: missing '='") } if i == 0 { - return nil, false, errors.New("invalid environment variable: name cannot be empty") + return nil, errors.New("invalid environment variable: name cannot be empty") } key := kv[:i] if saw[key] { // Duplicate. @@ -42,12 +45,12 @@ func prepareEnv(env []string) ([]string, bool, error) { } saw[key] = true if strings.IndexByte(kv, 0) >= 0 { - return nil, false, fmt.Errorf("invalid environment variable %q: contains nul byte (\\x00)", key) + return nil, fmt.Errorf("invalid environment variable %q: contains nul byte (\\x00)", key) } if key == "PATH" { // Needs to be set as it is used for binary lookup. if err := os.Setenv("PATH", kv[i+1:]); err != nil { - return nil, false, err + return nil, err } } out = append(out, kv) @@ -55,5 +58,31 @@ func prepareEnv(env []string) ([]string, bool, error) { // Restore the original order. slices.Reverse(out) - return out, saw["HOME"], nil + // If HOME is not found in env, get it from container's /etc/passwd and add. + if !saw["HOME"] { + home, err := getUserHome(uid) + if err != nil { + // For backward compatibility, don't return an error, but merely log it. + logrus.WithError(err).Debugf("HOME not set in process.env, and getting UID %d homedir failed", uid) + } + + out = append(out, "HOME="+home) + } + + return out, nil +} + +func getUserHome(uid int) (string, error) { + const defaultHome = "/" // Default value, return this with any error. + + u, err := user.LookupUid(uid) + if err != nil { + // ErrNoPasswdEntries is kinda expected as any UID can be specified. + if errors.Is(err, user.ErrNoPasswdEntries) { + err = nil + } + return defaultHome, err + } + + return u.Home, nil } diff --git a/libcontainer/env_test.go b/libcontainer/env_test.go index 72d63b4af23..c917471670a 100644 --- a/libcontainer/env_test.go +++ b/libcontainer/env_test.go @@ -1,25 +1,37 @@ package libcontainer import ( + "os/user" "slices" + "strconv" "testing" ) -func TestPrepareEnvDedup(t *testing.T) { +func TestPrepareEnv(t *testing.T) { + u, err := user.Current() + if err != nil { + t.Fatal(err) + } + home := "HOME=" + u.HomeDir + uid, err := strconv.Atoi(u.Uid) + if err != nil { + t.Fatal(err) + } + tests := []struct { env, wantEnv []string }{ { env: []string{}, - wantEnv: []string{}, + wantEnv: []string{home}, }, { - env: []string{"HOME=/root", "FOO=bar"}, - wantEnv: []string{"HOME=/root", "FOO=bar"}, + env: []string{"HOME=/whoo", "FOO=bar"}, + wantEnv: []string{"HOME=/whoo", "FOO=bar"}, }, { env: []string{"A=a", "A=b", "A=c"}, - wantEnv: []string{"A=c"}, + wantEnv: []string{"A=c", home}, }, { env: []string{"TERM=vt100", "HOME=/home/one", "HOME=/home/two", "TERM=xterm", "HOME=/home/three", "FOO=bar"}, @@ -28,7 +40,7 @@ func TestPrepareEnvDedup(t *testing.T) { } for _, tc := range tests { - env, _, err := prepareEnv(tc.env) + env, err := prepareEnv(tc.env, uid) if err != nil { t.Error(err) continue diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index af62c54e5df..cff21e1bc66 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -14,7 +14,6 @@ import ( "syscall" "github.com/containerd/console" - "github.com/moby/sys/user" "github.com/opencontainers/runtime-spec/specs-go" "github.com/sirupsen/logrus" "github.com/vishvananda/netlink" @@ -57,8 +56,9 @@ type initConfig struct { ProcessLabel string `json:"process_label"` AppArmorProfile string `json:"apparmor_profile"` NoNewPrivileges bool `json:"no_new_privileges"` - User string `json:"user"` - AdditionalGroups []string `json:"additional_groups"` + UID int `json:"uid"` + GID int `json:"gid"` + AdditionalGroups []int `json:"additional_groups"` Config *configs.Config `json:"config"` Networks []*network `json:"network"` PassedFilesCount int `json:"passed_files_count"` @@ -208,7 +208,7 @@ func startInitialization() (retErr error) { } func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSocket, pidfdSocket, fifoFile, logPipe *os.File) error { - env, homeSet, err := prepareEnv(config.Env) + env, err := prepareEnv(config.Env, config.UID) if err != nil { return err } @@ -226,7 +226,6 @@ func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSock pidfdSocket: pidfdSocket, config: config, logPipe: logPipe, - addHome: !homeSet, } return i.Init() case initStandard: @@ -238,7 +237,6 @@ func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSock config: config, fifoFile: fifoFile, logPipe: logPipe, - addHome: !homeSet, } return i.Init() } @@ -274,7 +272,7 @@ func verifyCwd() error { // finalizeNamespace drops the caps, sets the correct user // and working dir, and closes any leaked file descriptors // before executing the command inside the namespace. -func finalizeNamespace(config *initConfig, addHome bool) error { +func finalizeNamespace(config *initConfig) error { // Ensure that all unwanted fds we may have accidentally // inherited are marked close-on-exec so they stay out of the // container @@ -320,7 +318,7 @@ func finalizeNamespace(config *initConfig, addHome bool) error { if err := system.SetKeepCaps(); err != nil { return fmt.Errorf("unable to set keep caps: %w", err) } - if err := setupUser(config, addHome); err != nil { + if err := setupUser(config); err != nil { return fmt.Errorf("unable to setup user: %w", err) } // Change working directory AFTER the user has been set up, if we haven't done it yet. @@ -438,52 +436,11 @@ func syncParentSeccomp(pipe *syncSocket, seccompFd int) error { return readSync(pipe, procSeccompDone) } -// setupUser changes the groups, gid, and uid for the user inside the container, -// and appends user's HOME to config.Env if addHome is true. -func setupUser(config *initConfig, addHome bool) error { - // Set up defaults. - defaultExecUser := user.ExecUser{ - Uid: 0, - Gid: 0, - Home: "/", - } - - passwdPath, err := user.GetPasswdPath() - if err != nil { - return err - } - - groupPath, err := user.GetGroupPath() - if err != nil { - return err - } - - execUser, err := user.GetExecUserPath(config.User, &defaultExecUser, passwdPath, groupPath) - if err != nil { - return err - } - - var addGroups []int - if len(config.AdditionalGroups) > 0 { - addGroups, err = user.GetAdditionalGroupsPath(config.AdditionalGroups, groupPath) - if err != nil { - return err - } - } - - if config.RootlessEUID { - // We cannot set any additional groups in a rootless container and thus - // we bail if the user asked us to do so. TODO: We currently can't do - // this check earlier, but if libcontainer.Process.User was typesafe - // this might work. - if len(addGroups) > 0 { - return errors.New("cannot set any additional groups in a rootless container") - } - } - +// setupUser changes the groups, gid, and uid for the user inside the container. +func setupUser(config *initConfig) error { // Before we change to the container's user make sure that the processes // STDIO is correctly owned by the user that we are switching to. - if err := fixStdioPermissions(execUser); err != nil { + if err := fixStdioPermissions(config.UID); err != nil { return err } @@ -502,36 +459,30 @@ func setupUser(config *initConfig, addHome bool) error { allowSupGroups := !config.RootlessEUID && string(bytes.TrimSpace(setgroups)) != "deny" if allowSupGroups { - suppGroups := append(execUser.Sgids, addGroups...) - if err := unix.Setgroups(suppGroups); err != nil { + if err := unix.Setgroups(config.AdditionalGroups); err != nil { return &os.SyscallError{Syscall: "setgroups", Err: err} } } - if err := unix.Setgid(execUser.Gid); err != nil { + if err := unix.Setgid(config.GID); err != nil { if err == unix.EINVAL { - return fmt.Errorf("cannot setgid to unmapped gid %d in user namespace", execUser.Gid) + return fmt.Errorf("cannot setgid to unmapped gid %d in user namespace", config.GID) } return err } - if err := unix.Setuid(execUser.Uid); err != nil { + if err := unix.Setuid(config.UID); err != nil { if err == unix.EINVAL { - return fmt.Errorf("cannot setuid to unmapped uid %d in user namespace", execUser.Uid) + return fmt.Errorf("cannot setuid to unmapped uid %d in user namespace", config.UID) } return err } - - // If we didn't get HOME already, set it based on the user's HOME. - if addHome { - config.Env = append(config.Env, "HOME="+execUser.Home) - } return nil } -// fixStdioPermissions fixes the permissions of PID 1's STDIO within the container to the specified user. +// fixStdioPermissions fixes the permissions of PID 1's STDIO within the container to the specified uid. // The ownership needs to match because it is created outside of the container and needs to be // localized. -func fixStdioPermissions(u *user.ExecUser) error { +func fixStdioPermissions(uid int) error { var null unix.Stat_t if err := unix.Stat("/dev/null", &null); err != nil { return &os.PathError{Op: "stat", Path: "/dev/null", Err: err} @@ -544,7 +495,7 @@ func fixStdioPermissions(u *user.ExecUser) error { // Skip chown if uid is already the one we want or any of the STDIO descriptors // were redirected to /dev/null. - if int(s.Uid) == u.Uid || s.Rdev == null.Rdev { + if int(s.Uid) == uid || s.Rdev == null.Rdev { continue } @@ -554,7 +505,7 @@ func fixStdioPermissions(u *user.ExecUser) error { // that users expect to be able to actually use their console. Without // this code, you couldn't effectively run as a non-root user inside a // container and also have a console set up. - if err := file.Chown(u.Uid, int(s.Gid)); err != nil { + if err := file.Chown(uid, int(s.Gid)); err != nil { // If we've hit an EINVAL then s.Gid isn't mapped in the user // namespace. If we've hit an EPERM then the inode's current owner // is not mapped in our user namespace (in particular, diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index c9f37ada4c7..c9a7cbc56f3 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -399,7 +399,7 @@ func TestAdditionalGroups(t *testing.T) { Env: standardEnvironment, Stdin: nil, Stdout: &stdout, - AdditionalGroups: []string{"plugdev", "audio"}, + AdditionalGroups: []int{3333, 99999}, Init: true, } err = container.Run(&pconfig) @@ -410,13 +410,11 @@ func TestAdditionalGroups(t *testing.T) { outputGroups := stdout.String() - // Check that the groups output has the groups that we specified - if !strings.Contains(outputGroups, "audio") { - t.Fatalf("Listed groups do not contain the audio group as expected: %v", outputGroups) - } - - if !strings.Contains(outputGroups, "plugdev") { - t.Fatalf("Listed groups do not contain the plugdev group as expected: %v", outputGroups) + // Check that the groups output has the groups that we specified. + for _, gid := range pconfig.AdditionalGroups { + if !strings.Contains(outputGroups, strconv.Itoa(gid)) { + t.Errorf("Listed groups do not contain gid %d as expected: %v", gid, outputGroups) + } } } diff --git a/libcontainer/integration/execin_test.go b/libcontainer/integration/execin_test.go index a81d11edb71..aa7c01548b6 100644 --- a/libcontainer/integration/execin_test.go +++ b/libcontainer/integration/execin_test.go @@ -162,7 +162,7 @@ func TestExecInAdditionalGroups(t *testing.T) { Env: standardEnvironment, Stdin: nil, Stdout: &stdout, - AdditionalGroups: []string{"plugdev", "audio"}, + AdditionalGroups: []int{4444, 87654}, } err = container.Run(&pconfig) ok(t, err) @@ -175,13 +175,11 @@ func TestExecInAdditionalGroups(t *testing.T) { outputGroups := stdout.String() - // Check that the groups output has the groups that we specified - if !strings.Contains(outputGroups, "audio") { - t.Fatalf("Listed groups do not contain the audio group as expected: %v", outputGroups) - } - - if !strings.Contains(outputGroups, "plugdev") { - t.Fatalf("Listed groups do not contain the plugdev group as expected: %v", outputGroups) + // Check that the groups output has the groups that we specified. + for _, gid := range pconfig.AdditionalGroups { + if !strings.Contains(outputGroups, strconv.Itoa(gid)) { + t.Errorf("Listed groups do not contain gid %d as expected: %v", gid, outputGroups) + } } } diff --git a/libcontainer/process.go b/libcontainer/process.go index 114b3f2b6cb..09162be9a40 100644 --- a/libcontainer/process.go +++ b/libcontainer/process.go @@ -26,13 +26,13 @@ type Process struct { // Env specifies the environment variables for the process. Env []string - // User will set the uid and gid of the executing process running inside the container + // UID and GID of the executing process running inside the container // local to the container's user and group configuration. - User string + UID, GID int // AdditionalGroups specifies the gids that should be added to supplementary groups // in addition to those that the user belongs to. - AdditionalGroups []string + AdditionalGroups []int // Cwd will change the processes current working directory inside the container's rootfs. Cwd string diff --git a/libcontainer/setns_init_linux.go b/libcontainer/setns_init_linux.go index b42b3be1a89..d1885b3fdda 100644 --- a/libcontainer/setns_init_linux.go +++ b/libcontainer/setns_init_linux.go @@ -25,7 +25,6 @@ type linuxSetnsInit struct { pidfdSocket *os.File config *initConfig logPipe *os.File - addHome bool } func (l *linuxSetnsInit) getSessionRingName() string { @@ -102,7 +101,7 @@ func (l *linuxSetnsInit) Init() error { return err } } - if err := finalizeNamespace(l.config, l.addHome); err != nil { + if err := finalizeNamespace(l.config); err != nil { return err } if err := apparmor.ApplyProfile(l.config.AppArmorProfile); err != nil { diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index 510f9483baa..9517820bcad 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -27,7 +27,6 @@ type linuxStandardInit struct { fifoFile *os.File logPipe *os.File config *initConfig - addHome bool } func (l *linuxStandardInit) getSessionRingParams() (string, uint32, uint32) { @@ -187,7 +186,7 @@ func (l *linuxStandardInit) Init() error { return err } } - if err := finalizeNamespace(l.config, l.addHome); err != nil { + if err := finalizeNamespace(l.config); err != nil { return err } // finalizeNamespace can change user/group which clears the parent death diff --git a/list.go b/list.go index 615f6271cb2..997cd88173b 100644 --- a/list.go +++ b/list.go @@ -5,11 +5,12 @@ import ( "errors" "fmt" "os" + "os/user" + "strconv" "syscall" "text/tabwriter" "time" - "github.com/moby/sys/user" "github.com/opencontainers/runc/libcontainer" "github.com/opencontainers/runc/libcontainer/utils" "github.com/urfave/cli" @@ -136,9 +137,10 @@ func getContainers(context *cli.Context) ([]containerState, error) { } // This cast is safe on Linux. uid := st.Sys().(*syscall.Stat_t).Uid - owner, err := user.LookupUid(int(uid)) - if err != nil { - owner.Name = fmt.Sprintf("#%d", uid) + owner := "#" + strconv.Itoa(int(uid)) + u, err := user.LookupId(owner[1:]) + if err == nil { + owner = u.Username } container, err := libcontainer.Load(root, item.Name()) @@ -170,7 +172,7 @@ func getContainers(context *cli.Context) ([]containerState, error) { Rootfs: state.BaseState.Config.Rootfs, Created: state.BaseState.Created, Annotations: annotations, - Owner: owner.Name, + Owner: owner, }) } return s, nil diff --git a/tests/integration/exec.bats b/tests/integration/exec.bats index a92a237809d..8e96f3dddff 100644 --- a/tests/integration/exec.bats +++ b/tests/integration/exec.bats @@ -126,6 +126,18 @@ function teardown() { runc exec --user 1000:1000 test_busybox id [ "$status" -eq 0 ] [[ "${output}" == "uid=1000 gid=1000"* ]] + + # Check the default value of HOME ("/") is set even in case when + # - HOME is not set in process.Env, and + # - there is no entry in container's /etc/passwd for a given UID. + # + # NOTE this is not a standard runtime feature, but rather + # a historical de facto behavior we're afraid to change. + + # shellcheck disable=SC2016 + runc exec --user 1000 test_busybox sh -u -c 'echo $HOME' + [ "$status" -eq 0 ] + [[ "$output" = "/" ]] } # https://github.com/opencontainers/runc/issues/3674. diff --git a/utils_linux.go b/utils_linux.go index eef78ea3845..95ba88af036 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -47,10 +47,10 @@ func getDefaultImagePath() string { // spec and stdio from the current process. func newProcess(p specs.Process) (*libcontainer.Process, error) { lp := &libcontainer.Process{ - Args: p.Args, - Env: p.Env, - // TODO: fix libcontainer's API to better support uid/gid in a typesafe way. - User: fmt.Sprintf("%d:%d", p.User.UID, p.User.GID), + Args: p.Args, + Env: p.Env, + UID: int(p.User.UID), + GID: int(p.User.GID), Cwd: p.Cwd, Label: p.SelinuxLabel, NoNewPrivileges: &p.NoNewPrivileges, @@ -72,8 +72,11 @@ func newProcess(p specs.Process) (*libcontainer.Process, error) { lp.Capabilities.Permitted = p.Capabilities.Permitted lp.Capabilities.Ambient = p.Capabilities.Ambient } - for _, gid := range p.User.AdditionalGids { - lp.AdditionalGroups = append(lp.AdditionalGroups, strconv.FormatUint(uint64(gid), 10)) + if l := len(p.User.AdditionalGids); l > 0 { + lp.AdditionalGroups = make([]int, l) + for i, g := range p.User.AdditionalGids { + lp.AdditionalGroups[i] = int(g) + } } for _, rlimit := range p.Rlimits { rl, err := createLibContainerRlimit(rlimit)