From 7f64188f1752fc9fc75c07e479509353ca963ad8 Mon Sep 17 00:00:00 2001 From: Anda Xu Date: Fri, 29 Jun 2018 16:00:24 -0700 Subject: [PATCH] add missing supplementary group IDs Signed-off-by: Anda Xu --- executor/containerdexecutor/executor.go | 7 +-- executor/oci/user.go | 71 ++++++++++++++++--------- executor/runcexecutor/executor.go | 4 +- frontend/dockerfile/dockerfile_test.go | 49 +++++++++++++++++ 4 files changed, 101 insertions(+), 30 deletions(-) diff --git a/executor/containerdexecutor/executor.go b/executor/containerdexecutor/executor.go index 05921b0e..09538dba 100644 --- a/executor/containerdexecutor/executor.go +++ b/executor/containerdexecutor/executor.go @@ -55,14 +55,15 @@ func (w containerdExecutor) Exec(ctx context.Context, meta executor.Meta, root c } defer mountable.Release() - uid, gid, err := oci.ParseUser(meta.User) + var sgids []uint32 + uid, gid, err := oci.ParseUIDGID(meta.User) if err != nil { lm := snapshot.LocalMounterWithMounts(rootMounts) rootfsPath, err := lm.Mount() if err != nil { return err } - uid, gid, err = oci.GetUser(ctx, rootfsPath, meta.User) + uid, gid, sgids, err = oci.GetUser(ctx, rootfsPath, meta.User) if err != nil { lm.Unmount() return err @@ -70,7 +71,7 @@ func (w containerdExecutor) Exec(ctx context.Context, meta executor.Meta, root c lm.Unmount() } - opts := []containerdoci.SpecOpts{containerdoci.WithUIDGID(uid, gid)} + opts := []containerdoci.SpecOpts{oci.WithUIDGID(uid, gid, sgids)} if meta.ReadonlyRootFS { opts = append(opts, containerdoci.WithRootFSReadonly()) } diff --git a/executor/oci/user.go b/executor/oci/user.go index ce755f18..ac5dbebd 100644 --- a/executor/oci/user.go +++ b/executor/oci/user.go @@ -2,27 +2,31 @@ package oci import ( "context" + "errors" "os" "strconv" "strings" + "github.com/containerd/containerd/containers" + containerdoci "github.com/containerd/containerd/oci" "github.com/containerd/continuity/fs" "github.com/opencontainers/runc/libcontainer/user" + "github.com/opencontainers/runtime-spec/specs-go" ) -func GetUser(ctx context.Context, root, username string) (uint32, uint32, error) { +func GetUser(ctx context.Context, root, username string) (uint32, uint32, []uint32, error) { // fast path from uid/gid - if uid, gid, err := ParseUser(username); err == nil { - return uid, gid, nil + if uid, gid, err := ParseUIDGID(username); err == nil { + return uid, gid, nil, nil } passwdPath, err := user.GetPasswdPath() if err != nil { - return 0, 0, err + return 0, 0, nil, err } groupPath, err := user.GetGroupPath() if err != nil { - return 0, 0, err + return 0, 0, nil, err } passwdFile, err := openUserFile(root, passwdPath) if err == nil { @@ -35,33 +39,29 @@ func GetUser(ctx context.Context, root, username string) (uint32, uint32, error) execUser, err := user.GetExecUser(username, nil, passwdFile, groupFile) if err != nil { - return 0, 0, err + return 0, 0, nil, err } - - return uint32(execUser.Uid), uint32(execUser.Gid), nil + var sgids []uint32 + for _, g := range execUser.Sgids { + sgids = append(sgids, uint32(g)) + } + return uint32(execUser.Uid), uint32(execUser.Gid), sgids, nil } -func ParseUser(str string) (uid uint32, gid uint32, err error) { +// ParseUIDGID takes the fast path to parse UID and GID if and only if they are both provided +func ParseUIDGID(str string) (uid uint32, gid uint32, err error) { if str == "" { return 0, 0, nil } parts := strings.SplitN(str, ":", 2) - for i, v := range parts { - switch i { - case 0: - uid, err = parseUID(v) - if err != nil { - return 0, 0, err - } - if len(parts) == 1 { - gid = uid - } - case 1: - gid, err = parseUID(v) - if err != nil { - return 0, 0, err - } - } + if len(parts) == 1 { + return 0, 0, errors.New("groups ID is not provided") + } + if uid, err = parseUID(parts[0]); err != nil { + return 0, 0, err + } + if gid, err = parseUID(parts[1]); err != nil { + return 0, 0, err } return } @@ -84,3 +84,24 @@ func parseUID(str string) (uint32, error) { } return uint32(uid), nil } + +// WithUIDGID allows the UID and GID for the Process to be set +// FIXME: This is a temporeray fix for the missing supplementary GIDs from containerd +// once the PR in containerd is merged we should remove this function. +func WithUIDGID(uid, gid uint32, sgids []uint32) containerdoci.SpecOpts { + return func(_ context.Context, _ containerdoci.Client, _ *containers.Container, s *containerdoci.Spec) error { + setProcess(s) + s.Process.User.UID = uid + s.Process.User.GID = gid + s.Process.User.AdditionalGids = sgids + return nil + } +} + +// setProcess sets Process to empty if unset +// FIXME: Same on this one. Need to be removed after containerd fix merged +func setProcess(s *containerdoci.Spec) { + if s.Process == nil { + s.Process = &specs.Process{} + } +} diff --git a/executor/runcexecutor/executor.go b/executor/runcexecutor/executor.go index edffb5bf..f0dfc18d 100644 --- a/executor/runcexecutor/executor.go +++ b/executor/runcexecutor/executor.go @@ -133,7 +133,7 @@ func (w *runcExecutor) Exec(ctx context.Context, meta executor.Meta, root cache. } defer mount.Unmount(rootFSPath, 0) - uid, gid, err := oci.GetUser(ctx, rootFSPath, meta.User) + uid, gid, sgids, err := oci.GetUser(ctx, rootFSPath, meta.User) if err != nil { return err } @@ -143,7 +143,7 @@ func (w *runcExecutor) Exec(ctx context.Context, meta executor.Meta, root cache. return err } defer f.Close() - opts := []containerdoci.SpecOpts{containerdoci.WithUIDGID(uid, gid)} + opts := []containerdoci.SpecOpts{oci.WithUIDGID(uid, gid, sgids)} if system.SeccompSupported() { opts = append(opts, seccomp.WithDefaultProfile()) } diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 271eb39d..05f0fd23 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -1121,6 +1121,55 @@ func testUser(t *testing.T, sb integration.Sandbox) { FROM busybox AS base RUN mkdir -m 0777 /out RUN id -un > /out/rootuser + +# Make sure our defaults work +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)" = '0:0/root:root' ] + +# TODO decide if "args.user = strconv.Itoa(syscall.Getuid())" is acceptable behavior for changeUser in sysvinit instead of "return nil" when "USER" isn't specified (so that we get the proper group list even if that is the empty list, even in the default case of not supplying an explicit USER to run as, which implies USER 0) +USER root +RUN [ "$(id -G):$(id -Gn)" = '0 10:root wheel' ] + +# Setup dockerio user and group +RUN echo 'dockerio:x:1001:1001::/bin:/bin/false' >> /etc/passwd && \ + echo 'dockerio:x:1001:' >> /etc/group + +# Make sure we can switch to our user and all the information is exactly as we expect it to be +USER dockerio +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/dockerio:dockerio/1001:dockerio' ] + +# Switch back to root and double check that worked exactly as we might expect it to +USER root +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '0:0/root:root/0 10:root wheel' ] && \ + # Add a "supplementary" group for our dockerio user + echo 'supplementary:x:1002:dockerio' >> /etc/group + +# ... and then go verify that we get it like we expect +USER dockerio +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/dockerio:dockerio/1001 1002:dockerio supplementary' ] +USER 1001 +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/dockerio:dockerio/1001 1002:dockerio supplementary' ] + +# super test the new "user:group" syntax +USER dockerio:dockerio +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/dockerio:dockerio/1001:dockerio' ] +USER 1001:dockerio +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/dockerio:dockerio/1001:dockerio' ] +USER dockerio:1001 +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/dockerio:dockerio/1001:dockerio' ] +USER 1001:1001 +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1001/dockerio:dockerio/1001:dockerio' ] +USER dockerio:supplementary +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1002/dockerio:supplementary/1002:supplementary' ] +USER dockerio:1002 +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1002/dockerio:supplementary/1002:supplementary' ] +USER 1001:supplementary +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1002/dockerio:supplementary/1002:supplementary' ] +USER 1001:1002 +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1001:1002/dockerio:supplementary/1002:supplementary' ] + +# make sure unknown uid/gid still works properly +USER 1042:1043 +RUN [ "$(id -u):$(id -g)/$(id -un):$(id -gn)/$(id -G):$(id -Gn)" = '1042:1043/1042:1043/1043:1043' ] USER daemon RUN id -un > /out/daemonuser FROM scratch