From d03cf061e864670fcc1f392ae64c1506b53769cd Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Tue, 18 Aug 2020 12:48:41 -0700 Subject: [PATCH 1/2] Update kyaml to specify user for function --- kyaml/fn/runtime/container/container.go | 11 ++++++++-- kyaml/fn/runtime/container/container_test.go | 20 +++++++++++++++++++ kyaml/fn/runtime/runtimeutil/functiontypes.go | 3 +++ kyaml/runfn/runfn.go | 9 +++++++++ 4 files changed, 41 insertions(+), 2 deletions(-) diff --git a/kyaml/fn/runtime/container/container.go b/kyaml/fn/runtime/container/container.go index 209aba93d..223290a3d 100644 --- a/kyaml/fn/runtime/container/container.go +++ b/kyaml/fn/runtime/container/container.go @@ -133,6 +133,9 @@ type Filter struct { // StorageMounts is a list of storage options that the container will have mounted. StorageMounts []runtimeutil.StorageMount `yaml:"mounts,omitempty"` + // User username used to run the application in container, + User string + Exec runtimeexec.Filter } @@ -174,14 +177,18 @@ func (c *Filter) getCommand() (string, []string) { if c.Network != "" { network = c.Network } - + // run as nobody by default + user := c.User + if user == "" { + user = "nobody" + } args := []string{"run", "--rm", // delete the container afterward "-i", "-a", "STDIN", "-a", "STDOUT", "-a", "STDERR", // attach stdin, stdout, stderr "--network", network, // added security options - "--user", "nobody", // run as nobody + "--user", user, "--security-opt=no-new-privileges", // don't allow the user to escalate privileges // note: don't make fs readonly because things like heredoc rely on writing tmp files } diff --git a/kyaml/fn/runtime/container/container_test.go b/kyaml/fn/runtime/container/container_test.go index fdf037fab..fd2d99bc3 100644 --- a/kyaml/fn/runtime/container/container_test.go +++ b/kyaml/fn/runtime/container/container_test.go @@ -88,6 +88,26 @@ metadata: }, }, }, + { + name: "root user", + functionConfig: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: foo +`, + expectedArgs: []string{ + "run", + "--rm", + "-i", "-a", "STDIN", "-a", "STDOUT", "-a", "STDERR", + "--network", "none", + "--user", "root", + "--security-opt=no-new-privileges", + }, + instance: Filter{ + Image: "example.com:version", + User: "root", + }, + }, } for i := range tests { diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 29bb9be73..038bdb80e 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -52,6 +52,9 @@ type ContainerSpec struct { // Mounts are the storage or directories to mount into the container StorageMounts []StorageMount `json:"mounts,omitempty" yaml:"mounts,omitempty"` + + // User is the username/uid that application runs as in continer + User string `json:"user,omitempty" yaml:"user,omitempty"` } // ContainerNetwork diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index 5c3b99fb7..5351d82f4 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -87,6 +87,9 @@ type RunFns struct { // this is a variable so it can be mocked in tests functionFilterProvider func( filter runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter, error) + + // User username used to run the application in container, + User string } // Execute runs the command @@ -380,11 +383,17 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter atomic.AddUint32(&r.resultsCount, 1) } if !r.DisableContainers && spec.Container.Image != "" { + // command line username has higher priority + user := spec.Container.User + if r.User != "" { + user = r.User + } // TODO: Add a test for this behavior cf := &container.Filter{ Image: spec.Container.Image, Network: spec.Network, StorageMounts: r.StorageMounts, + User: user, } cf.Exec.FunctionConfig = api cf.Exec.GlobalScope = r.GlobalScope From 451c5c32c9c83e643708d91a97df11aef67d171d Mon Sep 17 00:00:00 2001 From: Donny Xia Date: Fri, 21 Aug 2020 12:05:11 -0700 Subject: [PATCH 2/2] code review --- kyaml/fn/runtime/container/container.go | 24 ++--------- kyaml/fn/runtime/container/container_test.go | 42 +++++++++++++------ kyaml/fn/runtime/runtimeutil/functiontypes.go | 25 ++++++++--- kyaml/runfn/runfn.go | 28 ++++++++----- kyaml/runfn/runfn_test.go | 4 +- 5 files changed, 73 insertions(+), 50 deletions(-) diff --git a/kyaml/fn/runtime/container/container.go b/kyaml/fn/runtime/container/container.go index 223290a3d..9f34f1512 100644 --- a/kyaml/fn/runtime/container/container.go +++ b/kyaml/fn/runtime/container/container.go @@ -123,18 +123,7 @@ import ( //    ├── deployment_foo.yaml //    └── service_bar.yaml type Filter struct { - - // Image is the container image to use to create a container. - Image string `yaml:"image,omitempty"` - - // Network is the container network to use. - Network string `yaml:"network,omitempty"` - - // StorageMounts is a list of storage options that the container will have mounted. - StorageMounts []runtimeutil.StorageMount `yaml:"mounts,omitempty"` - - // User username used to run the application in container, - User string + runtimeutil.ContainerSpec `json:",inline" yaml:",inline"` Exec runtimeexec.Filter } @@ -174,13 +163,8 @@ func (c *Filter) getCommand() (string, []string) { // was run from the cli. network := "none" - if c.Network != "" { - network = c.Network - } - // run as nobody by default - user := c.User - if user == "" { - user = "nobody" + if c.Network.Name != "" { + network = c.Network.Name } args := []string{"run", "--rm", // delete the container afterward @@ -188,7 +172,7 @@ func (c *Filter) getCommand() (string, []string) { "--network", network, // added security options - "--user", user, + "--user", c.User.String(), "--security-opt=no-new-privileges", // don't allow the user to escalate privileges // note: don't make fs readonly because things like heredoc rely on writing tmp files } diff --git a/kyaml/fn/runtime/container/container_test.go b/kyaml/fn/runtime/container/container_test.go index fd2d99bc3..bd6f0e6b3 100644 --- a/kyaml/fn/runtime/container/container_test.go +++ b/kyaml/fn/runtime/container/container_test.go @@ -38,7 +38,12 @@ metadata: "--user", "nobody", "--security-opt=no-new-privileges", }, - instance: Filter{Image: "example.com:version"}, + instance: Filter{ + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + User: "nobody", + }, + }, }, { @@ -56,7 +61,15 @@ metadata: "--user", "nobody", "--security-opt=no-new-privileges", }, - instance: Filter{Image: "example.com:version", Network: "test-1"}, + instance: Filter{ + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + Network: runtimeutil.ContainerNetwork{ + Name: "test-1", + }, + User: "nobody", + }, + }, }, { @@ -79,12 +92,15 @@ metadata: "--mount", fmt.Sprintf("type=%s,source=%s,target=%s,readonly", "tmpfs", "", "/local/"), }, instance: Filter{ - Image: "example.com:version", - StorageMounts: []runtimeutil.StorageMount{ - {MountType: "bind", Src: "/mount/path", DstPath: "/local/"}, - {MountType: "bind", Src: "/mount/pathrw", DstPath: "/localrw/", ReadWriteMode: true}, - {MountType: "volume", Src: "myvol", DstPath: "/local/"}, - {MountType: "tmpfs", Src: "", DstPath: "/local/"}, + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + StorageMounts: []runtimeutil.StorageMount{ + {MountType: "bind", Src: "/mount/path", DstPath: "/local/"}, + {MountType: "bind", Src: "/mount/pathrw", DstPath: "/localrw/", ReadWriteMode: true}, + {MountType: "volume", Src: "myvol", DstPath: "/local/"}, + {MountType: "tmpfs", Src: "", DstPath: "/local/"}, + }, + User: "nobody", }, }, }, @@ -104,8 +120,10 @@ metadata: "--security-opt=no-new-privileges", }, instance: Filter{ - Image: "example.com:version", - User: "root", + ContainerSpec: runtimeutil.ContainerSpec{ + Image: "example.com:version", + User: "root", + }, }, }, } @@ -203,7 +221,7 @@ metadata: } } func TestFilter_String(t *testing.T) { - instance := Filter{Image: "foo"} + instance := Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "foo"}} if !assert.Equal(t, "foo", instance.String()) { t.FailNow() } @@ -234,7 +252,7 @@ func TestFilter_ExitCode(t *testing.T) { func TestIgnoreEnv(t *testing.T) { os.Setenv(tmpDirEnvKey, "") - fltr := Filter{Image: "example.com:version"} + fltr := Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "example.com:version"}} _, args := fltr.getCommand() for _, arg := range args { if arg == tmpDirEnvKey { diff --git a/kyaml/fn/runtime/runtimeutil/functiontypes.go b/kyaml/fn/runtime/runtimeutil/functiontypes.go index 038bdb80e..7eb1c7056 100644 --- a/kyaml/fn/runtime/runtimeutil/functiontypes.go +++ b/kyaml/fn/runtime/runtimeutil/functiontypes.go @@ -18,11 +18,23 @@ const ( var functionAnnotationKeys = []string{FunctionAnnotationKey, oldFunctionAnnotationKey} +// ContainerUser is a type for username/uid used in container +type ContainerUser string + +func (u *ContainerUser) String() string { + return string(*u) +} + +func (u *ContainerUser) IsEmpty() bool { + return string(*u) == "" +} + +const ( + UserNobody ContainerUser = "nobody" +) + // FunctionSpec defines a spec for running a function type FunctionSpec struct { - // Network is the name of the network to use from a container - Network string `json:"network,omitempty" yaml:"network,omitempty"` - DeferFailure bool `json:"deferFailure,omitempty" yaml:"deferFailure,omitempty"` // Container is the spec for running a function as a container @@ -54,13 +66,16 @@ type ContainerSpec struct { StorageMounts []StorageMount `json:"mounts,omitempty" yaml:"mounts,omitempty"` // User is the username/uid that application runs as in continer - User string `json:"user,omitempty" yaml:"user,omitempty"` + User ContainerUser `json:"user,omitempty" yaml:"user,omitempty"` } // ContainerNetwork type ContainerNetwork struct { // Required specifies that function requires a network Required bool `json:"required,omitempty" yaml:"required,omitempty"` + + // Name is the name of the network to use from a container + Name string `json:"name,omitempty" yaml:"name,omitempty"` } // StarlarkSpec defines how to run a function as a starlark program @@ -113,7 +128,7 @@ func GetFunctionSpec(n *yaml.RNode) *FunctionSpec { } if fn := getFunctionSpecFromAnnotation(n, meta); fn != nil { - fn.Network = "" + fn.Container.Network.Name = "" fn.StorageMounts = []StorageMount{} return fn } diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index 5351d82f4..c757967e2 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -89,7 +89,7 @@ type RunFns struct { filter runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter, error) // User username used to run the application in container, - User string + User runtimeutil.ContainerUser } // Execute runs the command @@ -280,8 +280,17 @@ func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) ( // TODO(eddiezane): Provide error info about which function needs the network return fltrs, errors.Errorf("network required but not enabled with --network") } - spec.Network = r.NetworkName + spec.Container.Network.Name = r.NetworkName } + // command line username has higher priority + if r.User != "" { + spec.Container.User = r.User + } + // default user is nobody + if spec.Container.User.IsEmpty() { + spec.Container.User = runtimeutil.UserNobody + } + c, err := r.functionFilterProvider(*spec, api) if err != nil { return nil, err @@ -383,17 +392,14 @@ func (r *RunFns) ffp(spec runtimeutil.FunctionSpec, api *yaml.RNode) (kio.Filter atomic.AddUint32(&r.resultsCount, 1) } if !r.DisableContainers && spec.Container.Image != "" { - // command line username has higher priority - user := spec.Container.User - if r.User != "" { - user = r.User - } // TODO: Add a test for this behavior cf := &container.Filter{ - Image: spec.Container.Image, - Network: spec.Network, - StorageMounts: r.StorageMounts, - User: user, + ContainerSpec: runtimeutil.ContainerSpec{ + Image: spec.Container.Image, + Network: spec.Container.Network, + StorageMounts: r.StorageMounts, + User: spec.Container.User, + }, } cf.Exec.FunctionConfig = api cf.Exec.GlobalScope = r.GlobalScope diff --git a/kyaml/runfn/runfn_test.go b/kyaml/runfn/runfn_test.go index fd9237209..8599609fc 100644 --- a/kyaml/runfn/runfn_test.go +++ b/kyaml/runfn/runfn_test.go @@ -60,7 +60,7 @@ kind: return } filter, _ := instance.functionFilterProvider(spec, api) - cf := &container.Filter{Image: "example.com:version"} + cf := &container.Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "example.com:version"}} cf.Exec.FunctionConfig = api assert.Equal(t, cf, filter) } @@ -90,7 +90,7 @@ kind: return } filter, _ := instance.functionFilterProvider(spec, api) - cf := &container.Filter{Image: "example.com:version"} + cf := &container.Filter{ContainerSpec: runtimeutil.ContainerSpec{Image: "example.com:version"}} cf.Exec.FunctionConfig = api cf.Exec.GlobalScope = true assert.Equal(t, cf, filter)