diff --git a/pkg/loader/fileloader.go b/pkg/loader/fileloader.go index 8b5c626fe..4fdf7da39 100644 --- a/pkg/loader/fileloader.go +++ b/pkg/loader/fileloader.go @@ -45,7 +45,8 @@ import ( // These paths refer to resources, patches, // data for ConfigMaps and Secrets, etc. // -// They must terminate in or below the root. +// The loadRestrictor may disallow certain paths +// or classes of paths. // // * bases (other kustomizations) // @@ -75,8 +76,8 @@ import ( // These restrictions assure that kustomizations // are self-contained and relocatable, and impose // some safety when relying on remote kustomizations, -// e.g. a ConfigMap generator specified to read -// from /etc/passwd will fail. +// e.g. a remotely loaded ConfigMap generator specified +// to read from /etc/passwd will fail. // type fileLoader struct { // Loader that spawned this loader. @@ -88,6 +89,9 @@ type fileLoader struct { // paths relative to this directory. root fs.ConfirmedDir + // Restricts behavior of Load function. + loadRestrictor LoadRestrictorFunc + // If this is non-nil, the files were // obtained from the given repository. repoSpec *git.RepoSpec @@ -105,13 +109,17 @@ type fileLoader struct { const CWD = "." // NewFileLoaderAtCwd returns a loader that loads from ".". +// A convenience for kustomize edit commands. func NewFileLoaderAtCwd(fSys fs.FileSystem) *fileLoader { - return newLoaderOrDie(fSys, CWD) + return newLoaderOrDie( + RestrictionRootOnly, fSys, CWD) } // NewFileLoaderAtRoot returns a loader that loads from "/". +// A convenience for tests. func NewFileLoaderAtRoot(fSys fs.FileSystem) *fileLoader { - return newLoaderOrDie(fSys, string(filepath.Separator)) + return newLoaderOrDie( + RestrictionRootOnly, fSys, string(filepath.Separator)) } // Root returns the absolute path that is prepended to any @@ -120,25 +128,28 @@ func (l *fileLoader) Root() string { return l.root.String() } -func newLoaderOrDie(fSys fs.FileSystem, path string) *fileLoader { +func newLoaderOrDie( + lr LoadRestrictorFunc, fSys fs.FileSystem, path string) *fileLoader { root, err := demandDirectoryRoot(fSys, path) if err != nil { log.Fatalf("unable to make loader at '%s'; %v", path, err) } return newLoaderAtConfirmedDir( - root, fSys, nil, git.ClonerUsingGitExec) + lr, root, fSys, nil, git.ClonerUsingGitExec) } // newLoaderAtConfirmedDir returns a new fileLoader with given root. func newLoaderAtConfirmedDir( + lr LoadRestrictorFunc, root fs.ConfirmedDir, fSys fs.FileSystem, referrer *fileLoader, cloner git.Cloner) *fileLoader { return &fileLoader{ - root: root, - referrer: referrer, - fSys: fSys, - cloner: cloner, - cleaner: func() error { return nil }, + loadRestrictor: lr, + root: root, + referrer: referrer, + fSys: fSys, + cloner: cloner, + cleaner: func() error { return nil }, } } @@ -190,7 +201,7 @@ func (l *fileLoader) New(path string) (ifc.Loader, error) { return nil, err } return newLoaderAtConfirmedDir( - root, l.fSys, l, l.cloner), nil + l.loadRestrictor, root, l.fSys, l, l.cloner), nil } // newLoaderAtGitClone returns a new Loader pinned to a temporary @@ -216,12 +227,14 @@ func newLoaderAtGitClone( repoSpec.AbsPath(), f) } return &fileLoader{ - root: root, - referrer: referrer, - repoSpec: repoSpec, - fSys: fSys, - cloner: cloner, - cleaner: repoSpec.Cleaner(fSys), + // Clones never allowed to escape root. + loadRestrictor: RestrictionRootOnly, + root: root, + referrer: referrer, + repoSpec: repoSpec, + fSys: fSys, + cloner: cloner, + cleaner: repoSpec.Cleaner(fSys), }, nil } @@ -288,29 +301,16 @@ func (l *fileLoader) errIfRepoCycle(newRepoSpec *git.RepoSpec) error { // Load returns the content of file at the given path, // else an error. Relative paths are taken relative -// relative to the root. +// to the root. func (l *fileLoader) Load(path string) ([]byte, error) { - if filepath.IsAbs(path) { - return nil, l.loadOutOfBounds(path) + if !filepath.IsAbs(path) { + path = l.root.Join(path) } - d, f, err := l.fSys.CleanedAbs(l.root.Join(path)) + path, err := l.loadRestrictor(l.fSys, l.root, path) if err != nil { return nil, err } - if f == "" { - return nil, fmt.Errorf( - "'%s' must be a file (got d='%s')", path, d) - } - if !d.HasPrefix(l.root) { - return nil, l.loadOutOfBounds(path) - } - return l.fSys.ReadFile(d.Join(f)) -} - -func (l *fileLoader) loadOutOfBounds(path string) error { - return fmt.Errorf( - "security; file '%s' is not in or below '%s'", - path, l.root) + return l.fSys.ReadFile(path) } // Cleanup runs the cleaner. diff --git a/pkg/loader/fileloader_test.go b/pkg/loader/fileloader_test.go index 68114f368..ed504de0d 100644 --- a/pkg/loader/fileloader_test.go +++ b/pkg/loader/fileloader_test.go @@ -25,11 +25,10 @@ import ( "strings" "testing" - "sigs.k8s.io/kustomize/pkg/git" - "sigs.k8s.io/kustomize/pkg/pgmconfig" - "sigs.k8s.io/kustomize/pkg/fs" + "sigs.k8s.io/kustomize/pkg/git" "sigs.k8s.io/kustomize/pkg/ifc" + "sigs.k8s.io/kustomize/pkg/pgmconfig" ) type testData struct { @@ -207,46 +206,47 @@ func TestLoaderMisc(t *testing.T) { } } -func TestRestrictedLoadingInRealLoader(t *testing.T) { - // Create a structure like this - // - // /tmp/kustomize-test-SZwCZYjySj - // ├── base - // │ ├── okayData - // │ ├── symLinkToOkayData -> okayData - // │ └── symLinkToForbiddenData -> ../forbiddenData - // └── forbiddenData - // +const ( + contentOk = "hi there, i'm OK data" + contentExteriorData = "i am data from outside the root" +) + +// Create a structure like this +// +// /tmp/kustomize-test-random +// ├── base +// │ ├── okayData +// │ ├── symLinkToOkayData -> okayData +// │ └── symLinkToExteriorData -> ../exteriorData +// └── exteriorData +// +func commonSetupForLoaderRestrictionTest() (string, fs.FileSystem, error) { dir, err := ioutil.TempDir("", "kustomize-test-") if err != nil { - t.Fatalf("unexpected error: %v", err) + return "", nil, err } - defer os.RemoveAll(dir) - fSys := fs.MakeRealFS() fSys.Mkdir(filepath.Join(dir, "base")) - contentOk := "hi there, i'm OK data" fSys.WriteFile( filepath.Join(dir, "base", "okayData"), []byte(contentOk)) - contentForbidden := "don't be looking at me" fSys.WriteFile( - filepath.Join(dir, "forbiddenData"), []byte(contentForbidden)) + filepath.Join(dir, "exteriorData"), []byte(contentExteriorData)) os.Symlink( filepath.Join(dir, "base", "okayData"), filepath.Join(dir, "base", "symLinkToOkayData")) os.Symlink( - filepath.Join(dir, "forbiddenData"), - filepath.Join(dir, "base", "symLinkToForbiddenData")) + filepath.Join(dir, "exteriorData"), + filepath.Join(dir, "base", "symLinkToExteriorData")) + return dir, fSys, nil +} - var l ifc.Loader - - l = newLoaderOrDie(fSys, dir) - - // Sanity checks - loading from perspective of "dir". - // Everything works, including reading from a subdirectory. +// Make sure everything works when loading files +// in or below the loader root. +func doSanityChecksAndDropIntoBase( + t *testing.T, l ifc.Loader) ifc.Loader { data, err := l.Load(path.Join("base", "okayData")) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -254,11 +254,11 @@ func TestRestrictedLoadingInRealLoader(t *testing.T) { if string(data) != contentOk { t.Fatalf("unexpected content: %v", data) } - data, err = l.Load("forbiddenData") + data, err = l.Load("exteriorData") if err != nil { t.Fatalf("unexpected error: %v", err) } - if string(data) != contentForbidden { + if string(data) != contentExteriorData { t.Fatalf("unexpected content: %v", data) } @@ -285,9 +285,24 @@ func TestRestrictedLoadingInRealLoader(t *testing.T) { if string(data) != contentOk { t.Fatalf("unexpected content: %v", data) } + return l +} - // Reading symlink to forbiddenData fails. - _, err = l.Load("symLinkToForbiddenData") +func TestRestrictionRootInRealLoader(t *testing.T) { + dir, fSys, err := commonSetupForLoaderRestrictionTest() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer os.RemoveAll(dir) + + var l ifc.Loader + + l = newLoaderOrDie(RestrictionRootOnly, fSys, dir) + + l = doSanityChecksAndDropIntoBase(t, l) + + // Reading symlink to exteriorData fails. + _, err = l.Load("symLinkToExteriorData") if err == nil { t.Fatalf("expected error") } @@ -297,7 +312,7 @@ func TestRestrictedLoadingInRealLoader(t *testing.T) { // Attempt to read "up" fails, though earlier we were // able to read this file when root was "..". - _, err = l.Load("../forbiddenData") + _, err = l.Load("../exteriorData") if err == nil { t.Fatalf("expected error") } @@ -306,6 +321,32 @@ func TestRestrictedLoadingInRealLoader(t *testing.T) { } } +func TestRestrictionNoneInRealLoader(t *testing.T) { + dir, fSys, err := commonSetupForLoaderRestrictionTest() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + defer os.RemoveAll(dir) + + var l ifc.Loader + + l = newLoaderOrDie(RestrictionNone, fSys, dir) + + l = doSanityChecksAndDropIntoBase(t, l) + + // Reading symlink to exteriorData works. + _, err = l.Load("symLinkToExteriorData") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Attempt to read "up" works. + _, err = l.Load("../exteriorData") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + func splitOnNthSlash(v string, n int) (string, string) { left := "" for i := 0; i < n; i++ { @@ -393,7 +434,7 @@ func TestLoaderDisallowsLocalBaseFromRemoteOverlay(t *testing.T) { // Establish that a local overlay can navigate // to the local bases. - l1 = newLoaderOrDie(fSys, cloneRoot+"/foo/overlay") + l1 = newLoaderOrDie(RestrictionRootOnly, fSys, cloneRoot+"/foo/overlay") if l1.Root() != cloneRoot+"/foo/overlay" { t.Fatalf("unexpected root %s", l1.Root()) } @@ -468,7 +509,7 @@ func TestLocalLoaderReferencingGitBase(t *testing.T) { t.Fatalf("unexpected err: %v\n", err) } l1 := newLoaderAtConfirmedDir( - root, fSys, nil, + RestrictionRootOnly, root, fSys, nil, git.DoNothingCloner(fs.ConfirmedDir(cloneRoot))) if l1.Root() != topDir { t.Fatalf("unexpected root %s", l1.Root()) diff --git a/pkg/loader/loader.go b/pkg/loader/loader.go index 53de6553a..d930323ea 100644 --- a/pkg/loader/loader.go +++ b/pkg/loader/loader.go @@ -23,17 +23,24 @@ import ( "sigs.k8s.io/kustomize/pkg/ifc" ) -// NewLoader returns a Loader. -func NewLoader(path string, fSys fs.FileSystem) (ifc.Loader, error) { - repoSpec, err := git.NewRepoSpecFromUrl(path) +// NewLoader returns a Loader pointed at the given target. +// If the target is remote, the loader will be restricted +// to the root and below only. If the target is local, the +// loader will have no restrictions. If the local target +// attempts to transitively load remote bases, they will all +// be root-only restricted. +func NewLoader( + target string, fSys fs.FileSystem) (ifc.Loader, error) { + repoSpec, err := git.NewRepoSpecFromUrl(target) if err == nil { + // The target qualifies as a remote git target. return newLoaderAtGitClone( repoSpec, fSys, nil, git.ClonerUsingGitExec) } - root, err := demandDirectoryRoot(fSys, path) + root, err := demandDirectoryRoot(fSys, target) if err != nil { return nil, err } return newLoaderAtConfirmedDir( - root, fSys, nil, git.ClonerUsingGitExec), nil + RestrictionNone, root, fSys, nil, git.ClonerUsingGitExec), nil } diff --git a/pkg/loader/loadrestrictions.go b/pkg/loader/loadrestrictions.go new file mode 100644 index 000000000..135cd8afc --- /dev/null +++ b/pkg/loader/loadrestrictions.go @@ -0,0 +1,48 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loader + +import ( + "fmt" + + "sigs.k8s.io/kustomize/pkg/fs" +) + +type LoadRestrictorFunc func( + fs.FileSystem, fs.ConfirmedDir, string) (string, error) + +func RestrictionRootOnly( + fSys fs.FileSystem, root fs.ConfirmedDir, path string) (string, error) { + d, f, err := fSys.CleanedAbs(path) + if err != nil { + return "", err + } + if f == "" { + return "", fmt.Errorf("'%s' must be a file", path) + } + if !d.HasPrefix(root) { + return "", fmt.Errorf( + "security; file '%s' is not in or below '%s'", + path, root) + } + return d.Join(f), nil +} + +func RestrictionNone( + _ fs.FileSystem, _ fs.ConfirmedDir, path string) (string, error) { + return path, nil +} diff --git a/pkg/loader/loadrestrictions_test.go b/pkg/loader/loadrestrictions_test.go new file mode 100644 index 000000000..d76f5cb56 --- /dev/null +++ b/pkg/loader/loadrestrictions_test.go @@ -0,0 +1,74 @@ +/* +Copyright 2018 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package loader + +import ( + "strings" + "testing" + + "sigs.k8s.io/kustomize/pkg/fs" +) + +func TestRestrictionNone(t *testing.T) { + fSys := fs.MakeFakeFS() + root := fs.ConfirmedDir("irrelevant") + path := "whatever" + p, err := RestrictionNone(fSys, root, path) + if err != nil { + t.Fatal(err) + } + if p != path { + t.Fatalf("expected '%s', got '%s'", path, p) + } +} + +func TestRestrictionRootOnly(t *testing.T) { + fSys := fs.MakeFakeFS() + root := fs.ConfirmedDir("/tmp/foo") + + path := "/tmp/foo/whatever/beans" + p, err := RestrictionRootOnly(fSys, root, path) + if err != nil { + t.Fatal(err) + } + if p != path { + t.Fatalf("expected '%s', got '%s'", path, p) + } + + // Legal. + path = "/tmp/foo/whatever/../../foo/whatever" + p, err = RestrictionRootOnly(fSys, root, path) + if err != nil { + t.Fatal(err) + } + path = "/tmp/foo/whatever" + if p != path { + t.Fatalf("expected '%s', got '%s'", path, p) + } + + // Illegal. + path = "/tmp/illegal" + _, err = RestrictionRootOnly(fSys, root, path) + if err == nil { + t.Fatal("should have an error") + } + if !strings.Contains( + err.Error(), + "file '/tmp/illegal' is not in or below '/tmp/foo'") { + t.Fatalf("unexpected err: %s", err) + } +} diff --git a/pkg/target/baseandoverlaysmall_test.go b/pkg/target/baseandoverlaysmall_test.go index d71114655..c9c1a004a 100644 --- a/pkg/target/baseandoverlaysmall_test.go +++ b/pkg/target/baseandoverlaysmall_test.go @@ -17,7 +17,6 @@ limitations under the License. package target_test import ( - "strings" "testing" ) @@ -182,7 +181,7 @@ spec: `) } -func TestSharedPatchDisAllowed(t *testing.T) { +func TestSharedPatchAllowed(t *testing.T) { th := NewKustTestHarness(t, "/app/overlay") writeSmallBase(th) th.writeK("/app/overlay", ` @@ -201,15 +200,50 @@ metadata: spec: replicas: 1000 `) - _, err := th.makeKustTarget().MakeCustomizedResMap() - if err == nil { - t.Fatalf("expected error") - } - if !strings.Contains( - err.Error(), - "security; file '../shared/deployment-patch.yaml' is not in or below '/app/overlay'") { - t.Fatalf("unexpected error: %s", err) + m, err := th.makeKustTarget().MakeCustomizedResMap() + if err != nil { + t.Fatalf("Err: %v", err) } + th.assertActualEqualsExpected(m, ` +apiVersion: v1 +kind: Service +metadata: + labels: + app: myApp + env: prod + name: a-myService +spec: + ports: + - port: 7002 + selector: + app: myApp + backend: bungie + env: prod +--- +apiVersion: apps/v1 +kind: Deployment +metadata: + labels: + app: myApp + env: prod + name: a-myDeployment +spec: + replicas: 1000 + selector: + matchLabels: + app: myApp + env: prod + template: + metadata: + labels: + app: myApp + backend: awesome + env: prod + spec: + containers: + - image: whatever + name: whatever +`) } func TestSmallOverlayJSONPatch(t *testing.T) {