From 4f926df7cf43a627f7b16d730dea6d9a8802be91 Mon Sep 17 00:00:00 2001 From: Phillip Wittrock Date: Mon, 6 Apr 2020 16:09:01 -0700 Subject: [PATCH] Fix path in runfn - Calculate path relative to the functionConfig file - Do not allow absolute paths or traversal to parent directories --- kyaml/kio/filters/functiontypes.go | 10 +--- kyaml/runfn/runfn.go | 34 +++++++++--- kyaml/runfn/runfn_test.go | 84 +++++++++++++++++++++++++++--- 3 files changed, 104 insertions(+), 24 deletions(-) diff --git a/kyaml/kio/filters/functiontypes.go b/kyaml/kio/filters/functiontypes.go index db5a8adb3..5145adf46 100644 --- a/kyaml/kio/filters/functiontypes.go +++ b/kyaml/kio/filters/functiontypes.go @@ -4,7 +4,6 @@ package filters import ( - "sigs.k8s.io/kustomize/kyaml/kio/kioutil" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -17,9 +16,6 @@ var functionAnnotationKeys = []string{FunctionAnnotationKey, oldFunctionAnnotati // FunctionSpec defines a spec for running a function type FunctionSpec struct { - // Path defines the path for scoped functions - Path string `json:"path,omitempty" yaml:"path,omitempty"` - // Network is the name of the network to use from a container Network string `json:"network,omitempty" yaml:"network,omitempty"` @@ -85,20 +81,16 @@ func GetFunctionSpec(n *yaml.RNode) *FunctionSpec { return nil } - // path to the function, this will be mounted into the container - path := meta.Annotations[kioutil.PathAnnotation] if fn := getFunctionSpecFromAnnotation(n, meta); fn != nil { fn.Network = "" fn.StorageMounts = []StorageMount{} - fn.Path = path return fn } // legacy function specification for backwards compatibility container := meta.Annotations["config.kubernetes.io/container"] if container != "" { - return &FunctionSpec{ - Path: path, Container: ContainerSpec{Image: container}} + return &FunctionSpec{Container: ContainerSpec{Image: container}} } return nil } diff --git a/kyaml/runfn/runfn.go b/kyaml/runfn/runfn.go index 19d348486..0b3adfa0f 100644 --- a/kyaml/runfn/runfn.go +++ b/kyaml/runfn/runfn.go @@ -67,7 +67,7 @@ type RunFns struct { // functionFilterProvider provides a filter to perform the function. // this is a variable so it can be mocked in tests functionFilterProvider func( - filter filters.FunctionSpec, api *yaml.RNode) kio.Filter + filter filters.FunctionSpec, api *yaml.RNode) (kio.Filter, error) } // Execute runs the command @@ -215,7 +215,10 @@ func (r RunFns) getFunctionFilters(global bool, fns ...*yaml.RNode) ( } spec.Network = r.NetworkName } - c := r.functionFilterProvider(*spec, api) + c, err := r.functionFilterProvider(*spec, api) + if err != nil { + return nil, err + } if c == nil { continue } @@ -299,7 +302,7 @@ func (r *RunFns) init() { } // ffp provides function filters -func (r *RunFns) ffp(spec filters.FunctionSpec, api *yaml.RNode) kio.Filter { +func (r *RunFns) ffp(spec filters.FunctionSpec, api *yaml.RNode) (kio.Filter, error) { if !r.DisableContainers && spec.Container.Image != "" { return &filters.ContainerFilter{ Image: spec.Container.Image, @@ -307,14 +310,31 @@ func (r *RunFns) ffp(spec filters.FunctionSpec, api *yaml.RNode) kio.Filter { Network: spec.Network, StorageMounts: r.StorageMounts, GlobalScope: r.GlobalScope, - } + }, nil } if r.EnableStarlark && spec.Starlark.Path != "" { + // the script path is relative to the function config file + m, err := api.GetMeta() + if err != nil { + return nil, errors.Wrap(err) + } + p := m.Annotations[kioutil.PathAnnotation] + spec.Starlark.Path = path.Clean(spec.Starlark.Path) + if path.IsAbs(spec.Starlark.Path) { + return nil, errors.Errorf( + "absolute function path %s not allowed", spec.Starlark.Path) + } + if strings.HasPrefix(spec.Starlark.Path, "..") { + return nil, errors.Errorf( + "function path %s not allowed to start with ../", spec.Starlark.Path) + } + p = path.Join(r.Path, path.Dir(p), spec.Starlark.Path) + return &starlark.Filter{ Name: spec.Starlark.Name, - Path: spec.Starlark.Path, + Path: p, FunctionConfig: api, - } + }, nil } - return nil + return nil, nil } diff --git a/kyaml/runfn/runfn_test.go b/kyaml/runfn/runfn_test.go index 236f5610c..b0e5cd92f 100644 --- a/kyaml/runfn/runfn_test.go +++ b/kyaml/runfn/runfn_test.go @@ -55,7 +55,7 @@ kind: if !assert.NoError(t, err) { return } - filter := instance.functionFilterProvider(spec, api) + filter, _ := instance.functionFilterProvider(spec, api) assert.Equal(t, &filters.ContainerFilter{Image: "example.com:version", Config: api}, filter) } @@ -83,7 +83,7 @@ kind: if !assert.NoError(t, err) { return } - filter := instance.functionFilterProvider(spec, api) + filter, _ := instance.functionFilterProvider(spec, api) assert.Equal(t, &filters.ContainerFilter{ Image: "example.com:version", Config: api, GlobalScope: true}, filter) } @@ -196,6 +196,13 @@ func TestRunFns_getFilters(t *testing.T) { in []f // images to be run in a specific order out []string + + // images to be run in a specific order -- computed from directory path + outFn func(string) []string + + // expected Error + error string + // name of the test name string // value to set for NoFunctionsFromInput @@ -486,7 +493,54 @@ metadata: }, }, enableStarlark: true, - out: []string{"name: path: a/b/c url: program:"}, + outFn: func(path string) []string { + return []string{ + fmt.Sprintf("name: path: %s/foo/a/b/c url: program:", path)} + }, + }, + + // Test + // + // + {name: "starlark-function-absolute", + in: []f{ + { + path: filepath.Join("foo", "bar.yaml"), + value: ` +apiVersion: example.com/v1alpha1 +kind: ExampleFunction +metadata: + annotations: + config.kubernetes.io/function: | + starlark: + path: /a/b/c +`, + }, + }, + enableStarlark: true, + error: "absolute function path /a/b/c not allowed", + }, + + // Test + // + // + {name: "starlark-function-escape-parent", + in: []f{ + { + path: filepath.Join("foo", "bar.yaml"), + value: ` +apiVersion: example.com/v1alpha1 +kind: ExampleFunction +metadata: + annotations: + config.kubernetes.io/function: | + starlark: + path: ../a/b/c +`, + }, + }, + enableStarlark: true, + error: "function path ../a/b/c not allowed to start with ../", }, {name: "starlark-function-disabled", @@ -569,6 +623,14 @@ metadata: // get the filters which would be run var results []string _, fltrs, _, err := r.getNodesAndFilters() + + if tt.error != "" { + if !assert.EqualError(t, err, tt.error) { + t.FailNow() + } + return + } + if !assert.NoError(t, err) { t.FailNow() } @@ -577,8 +639,14 @@ metadata: } // compare the actual ordering to the expected ordering - if !assert.Equal(t, tt.out, results) { - t.FailNow() + if tt.outFn != nil { + if !assert.Equal(t, tt.outFn(d), results) { + t.FailNow() + } + } else { + if !assert.Equal(t, tt.out, results) { + t.FailNow() + } } }) } @@ -748,8 +816,8 @@ func setupTest(t *testing.T) string { // getFilterProvider fakes the creation of a filter, replacing the ContainerFiler with // a filter to s/kind: Deployment/kind: StatefulSet/g. // this can be used to simulate running a filter. -func getFilterProvider(t *testing.T) func(filters.FunctionSpec, *yaml.RNode) kio.Filter { - return func(f filters.FunctionSpec, node *yaml.RNode) kio.Filter { +func getFilterProvider(t *testing.T) func(filters.FunctionSpec, *yaml.RNode) (kio.Filter, error) { + return func(f filters.FunctionSpec, node *yaml.RNode) (kio.Filter, error) { // parse the filter from the input filter := yaml.YFilter{} b := &bytes.Buffer{} @@ -765,6 +833,6 @@ func getFilterProvider(t *testing.T) func(filters.FunctionSpec, *yaml.RNode) kio return filters.Modifier{ Filters: []yaml.YFilter{{Filter: yaml.Lookup("kind")}, filter}, - } + }, nil } }