From ad597b352cb89996a29043dc48c19e4f0af365d5 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Thu, 7 Apr 2022 15:27:34 +0200 Subject: [PATCH 1/7] helm: copy internal ignore and sympath modules We require these to be able to mimic Helm's own directory loader, and surprisingly (for `ignore` at least), these are not public. Signed-off-by: Hidde Beydals --- internal/helm/chart/loader/ignore/doc.go | 67 +++++ internal/helm/chart/loader/ignore/rules.go | 228 ++++++++++++++++++ .../helm/chart/loader/ignore/rules_test.go | 155 ++++++++++++ .../chart/loader/ignore/testdata/.helmignore | 3 + .../helm/chart/loader/ignore/testdata/.joonix | 0 .../helm/chart/loader/ignore/testdata/a.txt | 0 .../chart/loader/ignore/testdata/cargo/a.txt | 0 .../chart/loader/ignore/testdata/cargo/b.txt | 0 .../chart/loader/ignore/testdata/cargo/c.txt | 0 .../chart/loader/ignore/testdata/helm.txt | 0 .../chart/loader/ignore/testdata/mast/a.txt | 0 .../chart/loader/ignore/testdata/mast/b.txt | 0 .../chart/loader/ignore/testdata/mast/c.txt | 0 .../chart/loader/ignore/testdata/rudder.txt | 0 .../loader/ignore/testdata/templates/.dotfile | 0 .../chart/loader/ignore/testdata/tiller.txt | 0 internal/helm/chart/loader/sympath/walk.go | 119 +++++++++ .../helm/chart/loader/sympath/walk_test.go | 151 ++++++++++++ 18 files changed, 723 insertions(+) create mode 100644 internal/helm/chart/loader/ignore/doc.go create mode 100644 internal/helm/chart/loader/ignore/rules.go create mode 100644 internal/helm/chart/loader/ignore/rules_test.go create mode 100644 internal/helm/chart/loader/ignore/testdata/.helmignore create mode 100644 internal/helm/chart/loader/ignore/testdata/.joonix create mode 100644 internal/helm/chart/loader/ignore/testdata/a.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/cargo/a.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/cargo/b.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/cargo/c.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/helm.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/mast/a.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/mast/b.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/mast/c.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/rudder.txt create mode 100644 internal/helm/chart/loader/ignore/testdata/templates/.dotfile create mode 100644 internal/helm/chart/loader/ignore/testdata/tiller.txt create mode 100644 internal/helm/chart/loader/sympath/walk.go create mode 100644 internal/helm/chart/loader/sympath/walk_test.go diff --git a/internal/helm/chart/loader/ignore/doc.go b/internal/helm/chart/loader/ignore/doc.go new file mode 100644 index 000000000..4ca25c989 --- /dev/null +++ b/internal/helm/chart/loader/ignore/doc.go @@ -0,0 +1,67 @@ +/* +Copyright The Helm 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 ignore provides tools for writing ignore files (a la .gitignore). + +This provides both an ignore parser and a file-aware processor. + +The format of ignore files closely follows, but does not exactly match, the +format for .gitignore files (https://git-scm.com/docs/gitignore). + +The formatting rules are as follows: + + - Parsing is line-by-line + - Empty lines are ignored + - Lines the begin with # (comments) will be ignored + - Leading and trailing spaces are always ignored + - Inline comments are NOT supported ('foo* # Any foo' does not contain a comment) + - There is no support for multi-line patterns + - Shell glob patterns are supported. See Go's "path/filepath".Match + - If a pattern begins with a leading !, the match will be negated. + - If a pattern begins with a leading /, only paths relatively rooted will match. + - If the pattern ends with a trailing /, only directories will match + - If a pattern contains no slashes, file basenames are tested (not paths) + - The pattern sequence "**", while legal in a glob, will cause an error here + (to indicate incompatibility with .gitignore). + +Example: + + # Match any file named foo.txt + foo.txt + + # Match any text file + *.txt + + # Match only directories named mydir + mydir/ + + # Match only text files in the top-level directory + /*.txt + + # Match only the file foo.txt in the top-level directory + /foo.txt + + # Match any file named ab.txt, ac.txt, or ad.txt + a[b-d].txt + +Notable differences from .gitignore: + - The '**' syntax is not supported. + - The globbing library is Go's 'filepath.Match', not fnmatch(3) + - Trailing spaces are always ignored (there is no supported escape sequence) + - The evaluation of escape sequences has not been tested for compatibility + - There is no support for '\!' as a special leading sequence. +*/ +package ignore diff --git a/internal/helm/chart/loader/ignore/rules.go b/internal/helm/chart/loader/ignore/rules.go new file mode 100644 index 000000000..a80923baf --- /dev/null +++ b/internal/helm/chart/loader/ignore/rules.go @@ -0,0 +1,228 @@ +/* +Copyright The Helm 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 ignore + +import ( + "bufio" + "bytes" + "io" + "log" + "os" + "path/filepath" + "strings" + + "github.com/pkg/errors" +) + +// HelmIgnore default name of an ignorefile. +const HelmIgnore = ".helmignore" + +// Rules is a collection of path matching rules. +// +// Parse() and ParseFile() will construct and populate new Rules. +// Empty() will create an immutable empty ruleset. +type Rules struct { + patterns []*pattern +} + +// Empty builds an empty ruleset. +func Empty() *Rules { + return &Rules{patterns: []*pattern{}} +} + +// AddDefaults adds default ignore patterns. +// +// Ignore all dotfiles in "templates/" +func (r *Rules) AddDefaults() { + r.parseRule(`templates/.?*`) +} + +// ParseFile parses a helmignore file and returns the *Rules. +func ParseFile(file string) (*Rules, error) { + f, err := os.Open(file) + if err != nil { + return nil, err + } + defer f.Close() + return Parse(f) +} + +// Parse parses a rules file +func Parse(file io.Reader) (*Rules, error) { + r := &Rules{patterns: []*pattern{}} + + s := bufio.NewScanner(file) + currentLine := 0 + utf8bom := []byte{0xEF, 0xBB, 0xBF} + for s.Scan() { + scannedBytes := s.Bytes() + // We trim UTF8 BOM + if currentLine == 0 { + scannedBytes = bytes.TrimPrefix(scannedBytes, utf8bom) + } + line := string(scannedBytes) + currentLine++ + + if err := r.parseRule(line); err != nil { + return r, err + } + } + return r, s.Err() +} + +// Ignore evaluates the file at the given path, and returns true if it should be ignored. +// +// Ignore evaluates path against the rules in order. Evaluation stops when a match +// is found. Matching a negative rule will stop evaluation. +func (r *Rules) Ignore(path string, fi os.FileInfo) bool { + // Don't match on empty dirs. + if path == "" { + return false + } + + // Disallow ignoring the current working directory. + // See issue: + // 1776 (New York City) Hamilton: "Pardon me, are you Aaron Burr, sir?" + if path == "." || path == "./" { + return false + } + for _, p := range r.patterns { + if p.match == nil { + log.Printf("ignore: no matcher supplied for %q", p.raw) + return false + } + + // For negative rules, we need to capture and return non-matches, + // and continue for matches. + if p.negate { + if p.mustDir && !fi.IsDir() { + return true + } + if !p.match(path, fi) { + return true + } + continue + } + + // If the rule is looking for directories, and this is not a directory, + // skip it. + if p.mustDir && !fi.IsDir() { + continue + } + if p.match(path, fi) { + return true + } + } + return false +} + +// parseRule parses a rule string and creates a pattern, which is then stored in the Rules object. +func (r *Rules) parseRule(rule string) error { + rule = strings.TrimSpace(rule) + + // Ignore blank lines + if rule == "" { + return nil + } + // Comment + if strings.HasPrefix(rule, "#") { + return nil + } + + // Fail any rules that contain ** + if strings.Contains(rule, "**") { + return errors.New("double-star (**) syntax is not supported") + } + + // Fail any patterns that can't compile. A non-empty string must be + // given to Match() to avoid optimization that skips rule evaluation. + if _, err := filepath.Match(rule, "abc"); err != nil { + return err + } + + p := &pattern{raw: rule} + + // Negation is handled at a higher level, so strip the leading ! from the + // string. + if strings.HasPrefix(rule, "!") { + p.negate = true + rule = rule[1:] + } + + // Directory verification is handled by a higher level, so the trailing / + // is removed from the rule. That way, a directory named "foo" matches, + // even if the supplied string does not contain a literal slash character. + if strings.HasSuffix(rule, "/") { + p.mustDir = true + rule = strings.TrimSuffix(rule, "/") + } + + if strings.HasPrefix(rule, "/") { + // Require path matches the root path. + p.match = func(n string, fi os.FileInfo) bool { + rule = strings.TrimPrefix(rule, "/") + ok, err := filepath.Match(rule, n) + if err != nil { + log.Printf("Failed to compile %q: %s", rule, err) + return false + } + return ok + } + } else if strings.Contains(rule, "/") { + // require structural match. + p.match = func(n string, fi os.FileInfo) bool { + ok, err := filepath.Match(rule, n) + if err != nil { + log.Printf("Failed to compile %q: %s", rule, err) + return false + } + return ok + } + } else { + p.match = func(n string, fi os.FileInfo) bool { + // When there is no slash in the pattern, we evaluate ONLY the + // filename. + n = filepath.Base(n) + ok, err := filepath.Match(rule, n) + if err != nil { + log.Printf("Failed to compile %q: %s", rule, err) + return false + } + return ok + } + } + + r.patterns = append(r.patterns, p) + return nil +} + +// matcher is a function capable of computing a match. +// +// It returns true if the rule matches. +type matcher func(name string, fi os.FileInfo) bool + +// pattern describes a pattern to be matched in a rule set. +type pattern struct { + // raw is the unparsed string, with nothing stripped. + raw string + // match is the matcher function. + match matcher + // negate indicates that the rule's outcome should be negated. + negate bool + // mustDir indicates that the matched file must be a directory. + mustDir bool +} diff --git a/internal/helm/chart/loader/ignore/rules_test.go b/internal/helm/chart/loader/ignore/rules_test.go new file mode 100644 index 000000000..9581cf09f --- /dev/null +++ b/internal/helm/chart/loader/ignore/rules_test.go @@ -0,0 +1,155 @@ +/* +Copyright The Helm 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 ignore + +import ( + "bytes" + "os" + "path/filepath" + "testing" +) + +var testdata = "./testdata" + +func TestParse(t *testing.T) { + rules := `#ignore + + #ignore +foo +bar/* +baz/bar/foo.txt + +one/more +` + r, err := parseString(rules) + if err != nil { + t.Fatalf("Error parsing rules: %s", err) + } + + if len(r.patterns) != 4 { + t.Errorf("Expected 4 rules, got %d", len(r.patterns)) + } + + expects := []string{"foo", "bar/*", "baz/bar/foo.txt", "one/more"} + for i, p := range r.patterns { + if p.raw != expects[i] { + t.Errorf("Expected %q, got %q", expects[i], p.raw) + } + if p.match == nil { + t.Errorf("Expected %s to have a matcher function.", p.raw) + } + } +} + +func TestParseFail(t *testing.T) { + shouldFail := []string{"foo/**/bar", "[z-"} + for _, fail := range shouldFail { + _, err := parseString(fail) + if err == nil { + t.Errorf("Rule %q should have failed", fail) + } + } +} + +func TestParseFile(t *testing.T) { + f := filepath.Join(testdata, HelmIgnore) + if _, err := os.Stat(f); err != nil { + t.Fatalf("Fixture %s missing: %s", f, err) + } + + r, err := ParseFile(f) + if err != nil { + t.Fatalf("Failed to parse rules file: %s", err) + } + + if len(r.patterns) != 3 { + t.Errorf("Expected 3 patterns, got %d", len(r.patterns)) + } +} + +func TestIgnore(t *testing.T) { + // Test table: Given pattern and name, Ignore should return expect. + tests := []struct { + pattern string + name string + expect bool + }{ + // Glob tests + {`helm.txt`, "helm.txt", true}, + {`helm.*`, "helm.txt", true}, + {`helm.*`, "rudder.txt", false}, + {`*.txt`, "tiller.txt", true}, + {`*.txt`, "cargo/a.txt", true}, + {`cargo/*.txt`, "cargo/a.txt", true}, + {`cargo/*.*`, "cargo/a.txt", true}, + {`cargo/*.txt`, "mast/a.txt", false}, + {`ru[c-e]?er.txt`, "rudder.txt", true}, + {`templates/.?*`, "templates/.dotfile", true}, + // "." should never get ignored. https://github.com/helm/helm/issues/1776 + {`.*`, ".", false}, + {`.*`, "./", false}, + {`.*`, ".joonix", true}, + {`.*`, "helm.txt", false}, + {`.*`, "", false}, + + // Directory tests + {`cargo/`, "cargo", true}, + {`cargo/`, "cargo/", true}, + {`cargo/`, "mast/", false}, + {`helm.txt/`, "helm.txt", false}, + + // Negation tests + {`!helm.txt`, "helm.txt", false}, + {`!helm.txt`, "tiller.txt", true}, + {`!*.txt`, "cargo", true}, + {`!cargo/`, "mast/", true}, + + // Absolute path tests + {`/a.txt`, "a.txt", true}, + {`/a.txt`, "cargo/a.txt", false}, + {`/cargo/a.txt`, "cargo/a.txt", true}, + } + + for _, test := range tests { + r, err := parseString(test.pattern) + if err != nil { + t.Fatalf("Failed to parse: %s", err) + } + fi, err := os.Stat(filepath.Join(testdata, test.name)) + if err != nil { + t.Fatalf("Fixture missing: %s", err) + } + + if r.Ignore(test.name, fi) != test.expect { + t.Errorf("Expected %q to be %v for pattern %q", test.name, test.expect, test.pattern) + } + } +} + +func TestAddDefaults(t *testing.T) { + r := Rules{} + r.AddDefaults() + + if len(r.patterns) != 1 { + t.Errorf("Expected 1 default patterns, got %d", len(r.patterns)) + } +} + +func parseString(str string) (*Rules, error) { + b := bytes.NewBuffer([]byte(str)) + return Parse(b) +} diff --git a/internal/helm/chart/loader/ignore/testdata/.helmignore b/internal/helm/chart/loader/ignore/testdata/.helmignore new file mode 100644 index 000000000..b2693bae7 --- /dev/null +++ b/internal/helm/chart/loader/ignore/testdata/.helmignore @@ -0,0 +1,3 @@ +mast/a.txt +.DS_Store +.git diff --git a/internal/helm/chart/loader/ignore/testdata/.joonix b/internal/helm/chart/loader/ignore/testdata/.joonix new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/a.txt b/internal/helm/chart/loader/ignore/testdata/a.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/cargo/a.txt b/internal/helm/chart/loader/ignore/testdata/cargo/a.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/cargo/b.txt b/internal/helm/chart/loader/ignore/testdata/cargo/b.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/cargo/c.txt b/internal/helm/chart/loader/ignore/testdata/cargo/c.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/helm.txt b/internal/helm/chart/loader/ignore/testdata/helm.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/mast/a.txt b/internal/helm/chart/loader/ignore/testdata/mast/a.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/mast/b.txt b/internal/helm/chart/loader/ignore/testdata/mast/b.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/mast/c.txt b/internal/helm/chart/loader/ignore/testdata/mast/c.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/rudder.txt b/internal/helm/chart/loader/ignore/testdata/rudder.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/templates/.dotfile b/internal/helm/chart/loader/ignore/testdata/templates/.dotfile new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/ignore/testdata/tiller.txt b/internal/helm/chart/loader/ignore/testdata/tiller.txt new file mode 100644 index 000000000..e69de29bb diff --git a/internal/helm/chart/loader/sympath/walk.go b/internal/helm/chart/loader/sympath/walk.go new file mode 100644 index 000000000..752526fe9 --- /dev/null +++ b/internal/helm/chart/loader/sympath/walk.go @@ -0,0 +1,119 @@ +/* +Copyright (c) for portions of walk.go are held by The Go Authors, 2009 and are +provided under the BSD license. + +https://github.com/golang/go/blob/master/LICENSE + +Copyright The Helm 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 sympath + +import ( + "log" + "os" + "path/filepath" + "sort" + + "github.com/pkg/errors" +) + +// Walk walks the file tree rooted at root, calling walkFn for each file or directory +// in the tree, including root. All errors that arise visiting files and directories +// are filtered by walkFn. The files are walked in lexical order, which makes the +// output deterministic but means that for very large directories Walk can be +// inefficient. Walk follows symbolic links. +func Walk(root string, walkFn filepath.WalkFunc) error { + info, err := os.Lstat(root) + if err != nil { + err = walkFn(root, nil, err) + } else { + err = symwalk(root, info, walkFn) + } + if err == filepath.SkipDir { + return nil + } + return err +} + +// readDirNames reads the directory named by dirname and returns +// a sorted list of directory entries. +func readDirNames(dirname string) ([]string, error) { + f, err := os.Open(dirname) + if err != nil { + return nil, err + } + names, err := f.Readdirnames(-1) + f.Close() + if err != nil { + return nil, err + } + sort.Strings(names) + return names, nil +} + +// symwalk recursively descends path, calling walkFn. +func symwalk(path string, info os.FileInfo, walkFn filepath.WalkFunc) error { + // Recursively walk symlinked directories. + if IsSymlink(info) { + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + return errors.Wrapf(err, "error evaluating symlink %s", path) + } + log.Printf("found symbolic link in path: %s resolves to %s", path, resolved) + if info, err = os.Lstat(resolved); err != nil { + return err + } + if err := symwalk(path, info, walkFn); err != nil && err != filepath.SkipDir { + return err + } + return nil + } + + if err := walkFn(path, info, nil); err != nil { + return err + } + + if !info.IsDir() { + return nil + } + + names, err := readDirNames(path) + if err != nil { + return walkFn(path, info, err) + } + + for _, name := range names { + filename := filepath.Join(path, name) + fileInfo, err := os.Lstat(filename) + if err != nil { + if err := walkFn(filename, fileInfo, err); err != nil && err != filepath.SkipDir { + return err + } + } else { + err = symwalk(filename, fileInfo, walkFn) + if err != nil { + if (!fileInfo.IsDir() && !IsSymlink(fileInfo)) || err != filepath.SkipDir { + return err + } + } + } + } + return nil +} + +// IsSymlink is used to determine if the fileinfo is a symbolic link. +func IsSymlink(fi os.FileInfo) bool { + return fi.Mode()&os.ModeSymlink != 0 +} diff --git a/internal/helm/chart/loader/sympath/walk_test.go b/internal/helm/chart/loader/sympath/walk_test.go new file mode 100644 index 000000000..25f737134 --- /dev/null +++ b/internal/helm/chart/loader/sympath/walk_test.go @@ -0,0 +1,151 @@ +/* +Copyright (c) for portions of walk_test.go are held by The Go Authors, 2009 and are +provided under the BSD license. + +https://github.com/golang/go/blob/master/LICENSE + +Copyright The Helm 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 sympath + +import ( + "os" + "path/filepath" + "testing" +) + +type Node struct { + name string + entries []*Node // nil if the entry is a file + marks int + expectedMarks int + symLinkedTo string +} + +var tree = &Node{ + "testdata", + []*Node{ + {"a", nil, 0, 1, ""}, + {"b", []*Node{}, 0, 1, ""}, + {"c", nil, 0, 2, ""}, + {"d", nil, 0, 0, "c"}, + { + "e", + []*Node{ + {"x", nil, 0, 1, ""}, + {"y", []*Node{}, 0, 1, ""}, + { + "z", + []*Node{ + {"u", nil, 0, 1, ""}, + {"v", nil, 0, 1, ""}, + {"w", nil, 0, 1, ""}, + }, + 0, + 1, + "", + }, + }, + 0, + 1, + "", + }, + }, + 0, + 1, + "", +} + +func walkTree(n *Node, path string, f func(path string, n *Node)) { + f(path, n) + for _, e := range n.entries { + walkTree(e, filepath.Join(path, e.name), f) + } +} + +func makeTree(t *testing.T) { + walkTree(tree, tree.name, func(path string, n *Node) { + if n.entries == nil { + if n.symLinkedTo != "" { + if err := os.Symlink(n.symLinkedTo, path); err != nil { + t.Fatalf("makeTree: %v", err) + } + } else { + fd, err := os.Create(path) + if err != nil { + t.Fatalf("makeTree: %v", err) + return + } + fd.Close() + } + } else { + if err := os.Mkdir(path, 0770); err != nil { + t.Fatalf("makeTree: %v", err) + } + } + }) +} + +func checkMarks(t *testing.T, report bool) { + walkTree(tree, tree.name, func(path string, n *Node) { + if n.marks != n.expectedMarks && report { + t.Errorf("node %s mark = %d; expected %d", path, n.marks, n.expectedMarks) + } + n.marks = 0 + }) +} + +// Assumes that each node name is unique. Good enough for a test. +// If clear is true, any incoming error is cleared before return. The errors +// are always accumulated, though. +func mark(info os.FileInfo, err error, errors *[]error, clear bool) error { + if err != nil { + *errors = append(*errors, err) + if clear { + return nil + } + return err + } + name := info.Name() + walkTree(tree, tree.name, func(path string, n *Node) { + if n.name == name { + n.marks++ + } + }) + return nil +} + +func TestWalk(t *testing.T) { + makeTree(t) + errors := make([]error, 0, 10) + clear := true + markFn := func(path string, info os.FileInfo, err error) error { + return mark(info, err, &errors, clear) + } + // Expect no errors. + err := Walk(tree.name, markFn) + if err != nil { + t.Fatalf("no error expected, found: %s", err) + } + if len(errors) != 0 { + t.Fatalf("unexpected errors: %s", errors) + } + checkMarks(t, true) + + // cleanup + if err := os.RemoveAll(tree.name); err != nil { + t.Errorf("removeTree: %v", err) + } +} From 25f54ee80e509826d2dea4d702ec46f623099929 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 8 Apr 2022 00:17:28 +0200 Subject: [PATCH 2/7] sympath: provide abs path after eval symlink This can be used to detect traversion outside of a certain path scope while walking. Signed-off-by: Hidde Beydals --- internal/helm/chart/loader/sympath/walk.go | 32 +++++++----- .../helm/chart/loader/sympath/walk_test.go | 49 +++++++++++-------- 2 files changed, 48 insertions(+), 33 deletions(-) diff --git a/internal/helm/chart/loader/sympath/walk.go b/internal/helm/chart/loader/sympath/walk.go index 752526fe9..af0e1a153 100644 --- a/internal/helm/chart/loader/sympath/walk.go +++ b/internal/helm/chart/loader/sympath/walk.go @@ -5,6 +5,7 @@ provided under the BSD license. https://github.com/golang/go/blob/master/LICENSE Copyright The Helm Authors. +Copyright The Flux 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 @@ -21,7 +22,7 @@ limitations under the License. package sympath import ( - "log" + "io/fs" "os" "path/filepath" "sort" @@ -29,17 +30,21 @@ import ( "github.com/pkg/errors" ) +// AbsWalkFunc functions like filepath.WalkFunc but provides the absolute path +// of fs.FileInfo when path is a symlink. +type AbsWalkFunc func(path, absPath string, info fs.FileInfo, err error) error + // Walk walks the file tree rooted at root, calling walkFn for each file or directory // in the tree, including root. All errors that arise visiting files and directories // are filtered by walkFn. The files are walked in lexical order, which makes the // output deterministic but means that for very large directories Walk can be // inefficient. Walk follows symbolic links. -func Walk(root string, walkFn filepath.WalkFunc) error { +func Walk(root string, walkFn AbsWalkFunc) error { info, err := os.Lstat(root) if err != nil { - err = walkFn(root, nil, err) + err = walkFn(root, root, nil, err) } else { - err = symwalk(root, info, walkFn) + err = symwalk(root, root, info, walkFn) } if err == filepath.SkipDir { return nil @@ -63,25 +68,25 @@ func readDirNames(dirname string) ([]string, error) { return names, nil } -// symwalk recursively descends path, calling walkFn. -func symwalk(path string, info os.FileInfo, walkFn filepath.WalkFunc) error { +// symwalk recursively descends path, calling AbsWalkFunc. +func symwalk(path, absPath string, info os.FileInfo, walkFn AbsWalkFunc) error { // Recursively walk symlinked directories. if IsSymlink(info) { resolved, err := filepath.EvalSymlinks(path) if err != nil { return errors.Wrapf(err, "error evaluating symlink %s", path) } - log.Printf("found symbolic link in path: %s resolves to %s", path, resolved) if info, err = os.Lstat(resolved); err != nil { return err } - if err := symwalk(path, info, walkFn); err != nil && err != filepath.SkipDir { + // NB: pass-on resolved as absolute path + if err := symwalk(path, resolved, info, walkFn); err != nil && err != filepath.SkipDir { return err } return nil } - if err := walkFn(path, info, nil); err != nil { + if err := walkFn(path, absPath, info, nil); err != nil { return err } @@ -91,19 +96,20 @@ func symwalk(path string, info os.FileInfo, walkFn filepath.WalkFunc) error { names, err := readDirNames(path) if err != nil { - return walkFn(path, info, err) + return walkFn(path, absPath, info, err) } for _, name := range names { filename := filepath.Join(path, name) + // NB: possibly absPath != path separately + absFilename := filepath.Join(absPath, name) fileInfo, err := os.Lstat(filename) if err != nil { - if err := walkFn(filename, fileInfo, err); err != nil && err != filepath.SkipDir { + if err := walkFn(filename, absFilename, fileInfo, err); err != nil && err != filepath.SkipDir { return err } } else { - err = symwalk(filename, fileInfo, walkFn) - if err != nil { + if err = symwalk(filename, absFilename, fileInfo, walkFn); err != nil { if (!fileInfo.IsDir() && !IsSymlink(fileInfo)) || err != filepath.SkipDir { return err } diff --git a/internal/helm/chart/loader/sympath/walk_test.go b/internal/helm/chart/loader/sympath/walk_test.go index 25f737134..50740f34e 100644 --- a/internal/helm/chart/loader/sympath/walk_test.go +++ b/internal/helm/chart/loader/sympath/walk_test.go @@ -27,45 +27,47 @@ import ( ) type Node struct { - name string - entries []*Node // nil if the entry is a file - marks int - expectedMarks int - symLinkedTo string + name string + entries []*Node // nil if the entry is a file + marks int + expectedMarks int + symLinkedTo string + absPath string + expectedAbsPath string } var tree = &Node{ "testdata", []*Node{ - {"a", nil, 0, 1, ""}, - {"b", []*Node{}, 0, 1, ""}, - {"c", nil, 0, 2, ""}, - {"d", nil, 0, 0, "c"}, + {"a", nil, 0, 1, "", "", "testdata/a"}, + {"b", []*Node{}, 0, 1, "", "", "testdata/b"}, + {"c", nil, 0, 2, "", "", "testdata/c"}, + {"d", nil, 0, 0, "c", "", "testdata/c"}, { "e", []*Node{ - {"x", nil, 0, 1, ""}, - {"y", []*Node{}, 0, 1, ""}, + {"x", nil, 0, 1, "", "", "testdata/e/x"}, + {"y", []*Node{}, 0, 1, "", "", "testdata/e/y"}, { "z", []*Node{ - {"u", nil, 0, 1, ""}, - {"v", nil, 0, 1, ""}, - {"w", nil, 0, 1, ""}, + {"u", nil, 0, 1, "", "", "testdata/e/z/u"}, + {"v", nil, 0, 1, "", "", "testdata/e/z/v"}, + {"w", nil, 0, 1, "", "", "testdata/e/z/w"}, }, 0, 1, - "", + "", "", "testdata/e/z", }, }, 0, 1, - "", + "", "", "testdata/e", }, }, 0, 1, - "", + "", "", "testdata", } func walkTree(n *Node, path string, f func(path string, n *Node)) { @@ -103,6 +105,9 @@ func checkMarks(t *testing.T, report bool) { if n.marks != n.expectedMarks && report { t.Errorf("node %s mark = %d; expected %d", path, n.marks, n.expectedMarks) } + if n.absPath != n.expectedAbsPath && report { + t.Errorf("node %s absPath = %s; expected %s", path, n.absPath, n.expectedAbsPath) + } n.marks = 0 }) } @@ -110,7 +115,7 @@ func checkMarks(t *testing.T, report bool) { // Assumes that each node name is unique. Good enough for a test. // If clear is true, any incoming error is cleared before return. The errors // are always accumulated, though. -func mark(info os.FileInfo, err error, errors *[]error, clear bool) error { +func mark(absPath string, info os.FileInfo, err error, errors *[]error, clear bool) error { if err != nil { *errors = append(*errors, err) if clear { @@ -120,8 +125,12 @@ func mark(info os.FileInfo, err error, errors *[]error, clear bool) error { } name := info.Name() walkTree(tree, tree.name, func(path string, n *Node) { + if n.symLinkedTo == name { + n.absPath = absPath + } if n.name == name { n.marks++ + n.absPath = absPath } }) return nil @@ -131,8 +140,8 @@ func TestWalk(t *testing.T) { makeTree(t) errors := make([]error, 0, 10) clear := true - markFn := func(path string, info os.FileInfo, err error) error { - return mark(info, err, &errors, clear) + markFn := func(path, absPath string, info os.FileInfo, err error) error { + return mark(absPath, info, err, &errors, clear) } // Expect no errors. err := Walk(tree.name, markFn) From 5ae30cb4aabe833185058bc681171975d6f763d1 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 8 Apr 2022 11:37:33 +0200 Subject: [PATCH 3/7] helm: drop github.com/pkg/errors Signed-off-by: Hidde Beydals --- internal/helm/chart/loader/ignore/rules.go | 3 +-- internal/helm/chart/loader/sympath/walk.go | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/internal/helm/chart/loader/ignore/rules.go b/internal/helm/chart/loader/ignore/rules.go index a80923baf..d8054b44d 100644 --- a/internal/helm/chart/loader/ignore/rules.go +++ b/internal/helm/chart/loader/ignore/rules.go @@ -19,13 +19,12 @@ package ignore import ( "bufio" "bytes" + "errors" "io" "log" "os" "path/filepath" "strings" - - "github.com/pkg/errors" ) // HelmIgnore default name of an ignorefile. diff --git a/internal/helm/chart/loader/sympath/walk.go b/internal/helm/chart/loader/sympath/walk.go index af0e1a153..a9763c56a 100644 --- a/internal/helm/chart/loader/sympath/walk.go +++ b/internal/helm/chart/loader/sympath/walk.go @@ -22,12 +22,11 @@ limitations under the License. package sympath import ( + "fmt" "io/fs" "os" "path/filepath" "sort" - - "github.com/pkg/errors" ) // AbsWalkFunc functions like filepath.WalkFunc but provides the absolute path @@ -74,7 +73,7 @@ func symwalk(path, absPath string, info os.FileInfo, walkFn AbsWalkFunc) error { if IsSymlink(info) { resolved, err := filepath.EvalSymlinks(path) if err != nil { - return errors.Wrapf(err, "error evaluating symlink %s", path) + return fmt.Errorf("error evaluating symlink %s: %w", path, err) } if info, err = os.Lstat(resolved); err != nil { return err From 6fc066b1b60b70bc78d202316b6036813c362713 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 8 Apr 2022 11:33:52 +0200 Subject: [PATCH 4/7] helm: introduce customized chart loaders This introduces our own `secureloader` package, with a directory loader that's capable of following symlinks while validating they stay within a certain root boundary. Signed-off-by: Hidde Beydals --- internal/helm/chart/secureloader/directory.go | 208 ++++++++++++++++++ .../helm/chart/secureloader/directory_test.go | 82 +++++++ internal/helm/chart/secureloader/file.go | 47 ++++ .../{loader => secureloader}/ignore/doc.go | 0 .../{loader => secureloader}/ignore/rules.go | 0 .../ignore/rules_test.go | 0 .../ignore/testdata/.helmignore | 0 .../ignore/testdata/.joonix | 0 .../ignore/testdata/a.txt | 0 .../ignore/testdata/cargo/a.txt | 0 .../ignore/testdata/cargo/b.txt | 0 .../ignore/testdata/cargo/c.txt | 0 .../ignore/testdata/helm.txt | 0 .../ignore/testdata/mast/a.txt | 0 .../ignore/testdata/mast/b.txt | 0 .../ignore/testdata/mast/c.txt | 0 .../ignore/testdata/rudder.txt | 0 .../ignore/testdata/templates/.dotfile | 0 .../ignore/testdata/tiller.txt | 0 internal/helm/chart/secureloader/loader.go | 76 +++++++ .../helm/chart/secureloader/loader_test.go | 54 +++++ .../{loader => secureloader}/sympath/walk.go | 0 .../sympath/walk_test.go | 0 23 files changed, 467 insertions(+) create mode 100644 internal/helm/chart/secureloader/directory.go create mode 100644 internal/helm/chart/secureloader/directory_test.go create mode 100644 internal/helm/chart/secureloader/file.go rename internal/helm/chart/{loader => secureloader}/ignore/doc.go (100%) rename internal/helm/chart/{loader => secureloader}/ignore/rules.go (100%) rename internal/helm/chart/{loader => secureloader}/ignore/rules_test.go (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/.helmignore (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/.joonix (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/a.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/cargo/a.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/cargo/b.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/cargo/c.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/helm.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/mast/a.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/mast/b.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/mast/c.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/rudder.txt (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/templates/.dotfile (100%) rename internal/helm/chart/{loader => secureloader}/ignore/testdata/tiller.txt (100%) create mode 100644 internal/helm/chart/secureloader/loader.go create mode 100644 internal/helm/chart/secureloader/loader_test.go rename internal/helm/chart/{loader => secureloader}/sympath/walk.go (100%) rename internal/helm/chart/{loader => secureloader}/sympath/walk_test.go (100%) diff --git a/internal/helm/chart/secureloader/directory.go b/internal/helm/chart/secureloader/directory.go new file mode 100644 index 000000000..6b342a68e --- /dev/null +++ b/internal/helm/chart/secureloader/directory.go @@ -0,0 +1,208 @@ +/* +Copyright The Helm Authors. +Copyright 2022 The Flux 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. + +This file has been derived from +https://github.com/helm/helm/blob/v3.8.1/pkg/chart/loader/directory.go. + +It has been modified to not blindly accept any resolved symlink path, but +instead check it against the configured root before allowing it to be included. +It also allows for capping the size of any file loaded into the chart. +*/ + +package secureloader + +import ( + "bytes" + "fmt" + "os" + "path/filepath" + "strings" + + securejoin "github.com/cyphar/filepath-securejoin" + "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chart/loader" + + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/ignore" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/sympath" +) + +var ( + // DefaultMaxFileSize is the default maximum file size of any chart file + // loaded. + DefaultMaxFileSize = 16 << 20 // 16MiB + + utf8bom = []byte{0xEF, 0xBB, 0xBF} +) + +// SecureDirLoader securely loads a chart from a directory while resolving +// symlinks without including files outside root. +type SecureDirLoader struct { + root string + dir string + maxSize int +} + +// NewSecureDirLoader returns a new SecureDirLoader, configured to the scope of the +// root and provided dir. Max size configures the maximum size a file must not +// exceed to be loaded. If 0 it defaults to defaultMaxFileSize, it can be +// disabled using a negative integer. +func NewSecureDirLoader(root string, dir string, maxSize int) SecureDirLoader { + if maxSize == 0 { + maxSize = DefaultMaxFileSize + } + return SecureDirLoader{ + root: root, + dir: dir, + maxSize: maxSize, + } +} + +// Load loads and returns the chart.Chart, or an error. +func (l SecureDirLoader) Load() (*chart.Chart, error) { + return SecureLoadDir(l.root, l.dir, l.maxSize) +} + +// SecureLoadDir securely loads from a directory, without going outside root. +func SecureLoadDir(root, dir string, maxSize int) (*chart.Chart, error) { + root, err := filepath.Abs(root) + if err != nil { + return nil, err + } + + topDir, err := filepath.Abs(dir) + if err != nil { + return nil, err + } + + // Confirm topDir is actually relative to root + if _, err = isSecureSymlinkPath(root, topDir); err != nil { + return nil, fmt.Errorf("cannot load chart from dir: %w", err) + } + + // Just used for errors + c := &chart.Chart{} + + // Get the absolute location of the .helmignore file + relDirPath, err := filepath.Rel(root, topDir) + if err != nil { + // We are not expected to be returning this error, as the above call to + // isSecureSymlinkPath already does the same. However, especially + // because we are dealing with security aspects here, we check it + // anyway in case this assumption changes. + return nil, err + } + iFile, err := securejoin.SecureJoin(root, filepath.Join(relDirPath, ignore.HelmIgnore)) + + // Load the .helmignore rules + rules := ignore.Empty() + if _, err = os.Stat(iFile); err == nil { + r, err := ignore.ParseFile(iFile) + if err != nil { + return c, err + } + rules = r + } + rules.AddDefaults() + + var files []*loader.BufferedFile + topDir += string(filepath.Separator) + + walk := func(name, absoluteName string, fi os.FileInfo, err error) error { + n := strings.TrimPrefix(name, topDir) + if n == "" { + // No need to process top level. Avoid bug with helmignore .* matching + // empty names. See issue 1779. + return nil + } + + // Normalize to / since it will also work on Windows + n = filepath.ToSlash(n) + + if err != nil { + return err + } + if fi.IsDir() { + // Directory-based ignore rules should involve skipping the entire + // contents of that directory. + if rules.Ignore(n, fi) { + return filepath.SkipDir + } + // Check after excluding ignores to provide the user with an option + // to opt-out from including certain paths. + if _, err := isSecureSymlinkPath(root, absoluteName); err != nil { + return fmt.Errorf("cannot load '%s' directory: %w", n, err) + } + return nil + } + + // If a .helmignore file matches, skip this file. + if rules.Ignore(n, fi) { + return nil + } + + // Check after excluding ignores to provide the user with an option + // to opt-out from including certain paths. + if _, err := isSecureSymlinkPath(root, absoluteName); err != nil { + return fmt.Errorf("cannot load '%s' file: %w", n, err) + } + + // Irregular files include devices, sockets, and other uses of files that + // are not regular files. In Go they have a file mode type bit set. + // See https://golang.org/pkg/os/#FileMode for examples. + if !fi.Mode().IsRegular() { + return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", n) + } + + if fileSize := fi.Size(); maxSize > 0 && fileSize > int64(maxSize) { + return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, maxSize) + } + + data, err := os.ReadFile(name) + if err != nil { + return fmt.Errorf("error reading %s: %w", n, err) + } + data = bytes.TrimPrefix(data, utf8bom) + + files = append(files, &loader.BufferedFile{Name: n, Data: data}) + return nil + } + if err = sympath.Walk(topDir, walk); err != nil { + return c, err + } + return loader.LoadFiles(files) +} + +// isSecureSymlinkPath attempts to make the given absolute path relative to +// root and securely joins this with root. If the result equals absolute path, +// it is safe to use. +func isSecureSymlinkPath(root, absPath string) (bool, error) { + root, absPath = filepath.Clean(root), filepath.Clean(absPath) + if root == "/" { + return true, nil + } + unsafePath, err := filepath.Rel(root, absPath) + if err != nil { + return false, fmt.Errorf("cannot calculate path relative to root for resolved symlink") + } + safePath, err := securejoin.SecureJoin(root, unsafePath) + if err != nil { + return false, fmt.Errorf("cannot securely join root with resolved relative symlink path") + } + if safePath != absPath { + return false, fmt.Errorf("symlink traverses outside root boundary: relative path to root %s", unsafePath) + } + return true, nil +} diff --git a/internal/helm/chart/secureloader/directory_test.go b/internal/helm/chart/secureloader/directory_test.go new file mode 100644 index 000000000..e2031062a --- /dev/null +++ b/internal/helm/chart/secureloader/directory_test.go @@ -0,0 +1,82 @@ +/* +Copyright 2022 The Flux 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 secureloader + +import ( + "testing" + + . "github.com/onsi/gomega" +) + +func Test_isSecureSymlinkPath(t *testing.T) { + tests := []struct { + name string + root string + absPath string + safe bool + wantErr string + }{ + { + name: "absolute path in root", + root: "/", + absPath: "/bar/", + safe: true, + }, + + { + name: "abs path not relative to root", + root: "/working/dir", + absPath: "/working/in/another/dir", + safe: false, + wantErr: "symlink traverses outside root boundary", + }, + { + name: "abs path relative to root", + root: "/working/dir/", + absPath: "/working/dir/path", + safe: true, + }, + { + name: "illegal abs path", + root: "/working/dir", + absPath: "/working/dir/../but/not/really", + safe: false, + wantErr: "symlink traverses outside root boundary", + }, + { + name: "illegal root", + root: "working/dir/", + absPath: "/working/dir", + safe: false, + wantErr: "cannot calculate path relative to root for resolved symlink", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + got, err := isSecureSymlinkPath(tt.root, tt.absPath) + g.Expect(got).To(Equal(tt.safe)) + if tt.wantErr != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErr)) + return + } + g.Expect(err).ToNot(HaveOccurred()) + }) + } +} diff --git a/internal/helm/chart/secureloader/file.go b/internal/helm/chart/secureloader/file.go new file mode 100644 index 000000000..ce42e4ed2 --- /dev/null +++ b/internal/helm/chart/secureloader/file.go @@ -0,0 +1,47 @@ +/* +Copyright The Helm Authors. +Copyright 2022 The Flux 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 secureloader + +import ( + "io" + + "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chart/loader" +) + +// FileLoader is equal to Helm's. +// Redeclared to avoid having to deal with multiple package imports, +// possibly resulting in using the non-secure directory loader. +type FileLoader = loader.FileLoader + +// LoadFile loads from an archive file. +func LoadFile(name string) (*chart.Chart, error) { + return loader.LoadFile(name) +} + +// LoadArchiveFiles reads in files out of an archive into memory. This function +// performs important path security checks and should always be used before +// expanding a tarball +func LoadArchiveFiles(in io.Reader) ([]*loader.BufferedFile, error) { + return loader.LoadArchiveFiles(in) +} + +// LoadArchive loads from a reader containing a compressed tar archive. +func LoadArchive(in io.Reader) (*chart.Chart, error) { + return loader.LoadArchive(in) +} diff --git a/internal/helm/chart/loader/ignore/doc.go b/internal/helm/chart/secureloader/ignore/doc.go similarity index 100% rename from internal/helm/chart/loader/ignore/doc.go rename to internal/helm/chart/secureloader/ignore/doc.go diff --git a/internal/helm/chart/loader/ignore/rules.go b/internal/helm/chart/secureloader/ignore/rules.go similarity index 100% rename from internal/helm/chart/loader/ignore/rules.go rename to internal/helm/chart/secureloader/ignore/rules.go diff --git a/internal/helm/chart/loader/ignore/rules_test.go b/internal/helm/chart/secureloader/ignore/rules_test.go similarity index 100% rename from internal/helm/chart/loader/ignore/rules_test.go rename to internal/helm/chart/secureloader/ignore/rules_test.go diff --git a/internal/helm/chart/loader/ignore/testdata/.helmignore b/internal/helm/chart/secureloader/ignore/testdata/.helmignore similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/.helmignore rename to internal/helm/chart/secureloader/ignore/testdata/.helmignore diff --git a/internal/helm/chart/loader/ignore/testdata/.joonix b/internal/helm/chart/secureloader/ignore/testdata/.joonix similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/.joonix rename to internal/helm/chart/secureloader/ignore/testdata/.joonix diff --git a/internal/helm/chart/loader/ignore/testdata/a.txt b/internal/helm/chart/secureloader/ignore/testdata/a.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/a.txt rename to internal/helm/chart/secureloader/ignore/testdata/a.txt diff --git a/internal/helm/chart/loader/ignore/testdata/cargo/a.txt b/internal/helm/chart/secureloader/ignore/testdata/cargo/a.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/cargo/a.txt rename to internal/helm/chart/secureloader/ignore/testdata/cargo/a.txt diff --git a/internal/helm/chart/loader/ignore/testdata/cargo/b.txt b/internal/helm/chart/secureloader/ignore/testdata/cargo/b.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/cargo/b.txt rename to internal/helm/chart/secureloader/ignore/testdata/cargo/b.txt diff --git a/internal/helm/chart/loader/ignore/testdata/cargo/c.txt b/internal/helm/chart/secureloader/ignore/testdata/cargo/c.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/cargo/c.txt rename to internal/helm/chart/secureloader/ignore/testdata/cargo/c.txt diff --git a/internal/helm/chart/loader/ignore/testdata/helm.txt b/internal/helm/chart/secureloader/ignore/testdata/helm.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/helm.txt rename to internal/helm/chart/secureloader/ignore/testdata/helm.txt diff --git a/internal/helm/chart/loader/ignore/testdata/mast/a.txt b/internal/helm/chart/secureloader/ignore/testdata/mast/a.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/mast/a.txt rename to internal/helm/chart/secureloader/ignore/testdata/mast/a.txt diff --git a/internal/helm/chart/loader/ignore/testdata/mast/b.txt b/internal/helm/chart/secureloader/ignore/testdata/mast/b.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/mast/b.txt rename to internal/helm/chart/secureloader/ignore/testdata/mast/b.txt diff --git a/internal/helm/chart/loader/ignore/testdata/mast/c.txt b/internal/helm/chart/secureloader/ignore/testdata/mast/c.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/mast/c.txt rename to internal/helm/chart/secureloader/ignore/testdata/mast/c.txt diff --git a/internal/helm/chart/loader/ignore/testdata/rudder.txt b/internal/helm/chart/secureloader/ignore/testdata/rudder.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/rudder.txt rename to internal/helm/chart/secureloader/ignore/testdata/rudder.txt diff --git a/internal/helm/chart/loader/ignore/testdata/templates/.dotfile b/internal/helm/chart/secureloader/ignore/testdata/templates/.dotfile similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/templates/.dotfile rename to internal/helm/chart/secureloader/ignore/testdata/templates/.dotfile diff --git a/internal/helm/chart/loader/ignore/testdata/tiller.txt b/internal/helm/chart/secureloader/ignore/testdata/tiller.txt similarity index 100% rename from internal/helm/chart/loader/ignore/testdata/tiller.txt rename to internal/helm/chart/secureloader/ignore/testdata/tiller.txt diff --git a/internal/helm/chart/secureloader/loader.go b/internal/helm/chart/secureloader/loader.go new file mode 100644 index 000000000..86ff2cf6d --- /dev/null +++ b/internal/helm/chart/secureloader/loader.go @@ -0,0 +1,76 @@ +/* +Copyright The Helm Authors. +Copyright 2022 The Flux 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 secureloader + +import ( + "errors" + "io/fs" + "os" + "path/filepath" + + securejoin "github.com/cyphar/filepath-securejoin" + "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chart/loader" +) + +// Loader returns a new loader.ChartLoader appropriate for the given chart +// name. That being, SecureDirLoader when name is a directory, and +// FileLoader when it's a file. +// Name can be an absolute or relative path, but always has to be inside +// root. +func Loader(root, name string) (loader.ChartLoader, error) { + root, name = filepath.Clean(root), filepath.Clean(name) + relName := name + if filepath.IsAbs(relName) { + var err error + if relName, err = filepath.Rel(root, name); err != nil { + return nil, err + } + } + secureName, err := securejoin.SecureJoin(root, relName) + if err != nil { + return nil, err + } + fi, err := os.Lstat(secureName) + if err != nil { + if pathErr := new(fs.PathError); errors.As(err, &pathErr) { + return nil, &fs.PathError{Op: pathErr.Op, Path: name, Err: pathErr.Err} + } + return nil, err + } + if fi.IsDir() { + return NewSecureDirLoader(root, secureName, 0), nil + } + return FileLoader(secureName), nil +} + +// Load takes a string root and name, tries to resolve it to a file or directory, +// and then loads it securely without traversing outside of root. +// +// This is the preferred way to load a chart. It will discover the chart encoding +// and hand off to the appropriate chart reader. +// +// If a .helmignore file is present, the directory loader will skip loading any files +// matching it. But .helmignore is not evaluated when reading out of an archive. +func Load(root, name string) (*chart.Chart, error) { + l, err := Loader(root, name) + if err != nil { + return nil, err + } + return l.Load() +} diff --git a/internal/helm/chart/secureloader/loader_test.go b/internal/helm/chart/secureloader/loader_test.go new file mode 100644 index 000000000..d0b69a846 --- /dev/null +++ b/internal/helm/chart/secureloader/loader_test.go @@ -0,0 +1,54 @@ +/* +Copyright 2022 The Flux 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 secureloader + +import ( + "io/fs" + "os" + "path/filepath" + "testing" + + . "github.com/onsi/gomega" + "helm.sh/helm/v3/pkg/chart/loader" +) + +func TestLoader(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + fakeChart := filepath.Join(tmpDir, "fake.tgz") + g.Expect(os.WriteFile(fakeChart, []byte(""), 0o644)).To(Succeed()) + + got, err := Loader(tmpDir, fakeChart) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(loader.FileLoader(fakeChart))) + + fakeChartPath := filepath.Join(tmpDir, "fake") + g.Expect(os.Mkdir(fakeChartPath, 0o700)).To(Succeed()) + got, err = Loader(tmpDir, "fake") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, dir: fakeChartPath, maxSize: DefaultMaxFileSize})) + + symlinkRoot := filepath.Join(tmpDir, "symlink") + g.Expect(os.Mkdir(symlinkRoot, 0o700)).To(Succeed()) + symlinkPath := filepath.Join(symlinkRoot, "fake.tgz") + g.Expect(os.Symlink(fakeChart, symlinkPath)) + got, err = Loader(symlinkRoot, symlinkPath) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(BeAssignableToTypeOf(&fs.PathError{})) + g.Expect(got).To(BeNil()) +} diff --git a/internal/helm/chart/loader/sympath/walk.go b/internal/helm/chart/secureloader/sympath/walk.go similarity index 100% rename from internal/helm/chart/loader/sympath/walk.go rename to internal/helm/chart/secureloader/sympath/walk.go diff --git a/internal/helm/chart/loader/sympath/walk_test.go b/internal/helm/chart/secureloader/sympath/walk_test.go similarity index 100% rename from internal/helm/chart/loader/sympath/walk_test.go rename to internal/helm/chart/secureloader/sympath/walk_test.go From b9063d7362595fb01ea2905810daa06443b4b0df Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Sat, 9 Apr 2022 02:25:38 +0200 Subject: [PATCH 5/7] helm: add more test coverage for secureloader Signed-off-by: Hidde Beydals --- internal/helm/chart/secureloader/directory.go | 216 ++++++----- .../helm/chart/secureloader/directory_test.go | 347 +++++++++++++++++- internal/helm/chart/secureloader/loader.go | 15 +- .../helm/chart/secureloader/loader_test.go | 66 +++- 4 files changed, 536 insertions(+), 108 deletions(-) diff --git a/internal/helm/chart/secureloader/directory.go b/internal/helm/chart/secureloader/directory.go index 6b342a68e..1c4b5f4b9 100644 --- a/internal/helm/chart/secureloader/directory.go +++ b/internal/helm/chart/secureloader/directory.go @@ -26,7 +26,9 @@ package secureloader import ( "bytes" + "errors" "fmt" + "io/fs" "os" "path/filepath" "strings" @@ -51,158 +53,204 @@ var ( // symlinks without including files outside root. type SecureDirLoader struct { root string - dir string + path string maxSize int } // NewSecureDirLoader returns a new SecureDirLoader, configured to the scope of the // root and provided dir. Max size configures the maximum size a file must not -// exceed to be loaded. If 0 it defaults to defaultMaxFileSize, it can be +// exceed to be loaded. If 0 it defaults to DefaultMaxFileSize, it can be // disabled using a negative integer. -func NewSecureDirLoader(root string, dir string, maxSize int) SecureDirLoader { +func NewSecureDirLoader(root string, path string, maxSize int) SecureDirLoader { if maxSize == 0 { maxSize = DefaultMaxFileSize } return SecureDirLoader{ root: root, - dir: dir, + path: path, maxSize: maxSize, } } // Load loads and returns the chart.Chart, or an error. func (l SecureDirLoader) Load() (*chart.Chart, error) { - return SecureLoadDir(l.root, l.dir, l.maxSize) + return SecureLoadDir(l.root, l.path, l.maxSize) } -// SecureLoadDir securely loads from a directory, without going outside root. -func SecureLoadDir(root, dir string, maxSize int) (*chart.Chart, error) { +// SecureLoadDir securely loads a chart from the path relative to root, without +// traversing outside root. When maxSize >= 0, files are not allowed to exceed +// this size, or an error is returned. +func SecureLoadDir(root, path string, maxSize int) (*chart.Chart, error) { root, err := filepath.Abs(root) if err != nil { return nil, err } - topDir, err := filepath.Abs(dir) + // Ensure path is relative + if filepath.IsAbs(path) { + relChartPath, err := filepath.Rel(root, path) + if err != nil { + return nil, err + } + path = relChartPath + } + + // Resolve secure absolute path + absChartName, err := securejoin.SecureJoin(root, path) if err != nil { return nil, err } - // Confirm topDir is actually relative to root - if _, err = isSecureSymlinkPath(root, topDir); err != nil { - return nil, fmt.Errorf("cannot load chart from dir: %w", err) + // Load ignore rules + rules, err := secureLoadIgnoreRules(root, path) + if err != nil { + return nil, fmt.Errorf("cannot load ignore rules for chart: %w", err) } - // Just used for errors - c := &chart.Chart{} + // Lets go for a walk... + fileWalker := newSecureFileWalker(root, absChartName, maxSize, rules) + if err = sympath.Walk(fileWalker.absChartPath, fileWalker.walk); err != nil { + return nil, fmt.Errorf("failed to load files from %s: %w", strings.TrimPrefix(fileWalker.absChartPath, fileWalker.root), err) + } - // Get the absolute location of the .helmignore file - relDirPath, err := filepath.Rel(root, topDir) + loaded, err := loader.LoadFiles(fileWalker.files) if err != nil { - // We are not expected to be returning this error, as the above call to - // isSecureSymlinkPath already does the same. However, especially - // because we are dealing with security aspects here, we check it - // anyway in case this assumption changes. - return nil, err + return nil, fmt.Errorf("failed to load chart from %s: %w", strings.TrimPrefix(fileWalker.absChartPath, fileWalker.root), err) } - iFile, err := securejoin.SecureJoin(root, filepath.Join(relDirPath, ignore.HelmIgnore)) + return loaded, nil +} - // Load the .helmignore rules +// secureLoadIgnoreRules attempts to load the ignore.HelmIgnore file from the +// chart path relative to root. If the file is a symbolic link, it is evaluated +// with the given root treated as root of the filesystem. +// If the ignore file does not exist, or points to a location outside of root, +// default ignore.Rules are returned. Any error other than fs.ErrNotExist is +// returned. +func secureLoadIgnoreRules(root, chartPath string) (*ignore.Rules, error) { rules := ignore.Empty() - if _, err = os.Stat(iFile); err == nil { - r, err := ignore.ParseFile(iFile) - if err != nil { - return c, err + + iFile, err := securejoin.SecureJoin(root, filepath.Join(chartPath, ignore.HelmIgnore)) + if err != nil { + return nil, err + } + _, err = os.Stat(iFile) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + return nil, err + } + if err == nil { + if rules, err = ignore.ParseFile(iFile); err != nil { + return nil, err } - rules = r } + rules.AddDefaults() + return rules, nil +} - var files []*loader.BufferedFile - topDir += string(filepath.Separator) +// secureFileWalker does the actual walking over the directory, any file loaded +// by walk is appended to files. +type secureFileWalker struct { + root string + absChartPath string + maxSize int + rules *ignore.Rules + files []*loader.BufferedFile +} - walk := func(name, absoluteName string, fi os.FileInfo, err error) error { - n := strings.TrimPrefix(name, topDir) - if n == "" { - // No need to process top level. Avoid bug with helmignore .* matching - // empty names. See issue 1779. - return nil - } +func newSecureFileWalker(root, absChartPath string, maxSize int, rules *ignore.Rules) *secureFileWalker { + absChartPath = filepath.Clean(absChartPath) + string(filepath.Separator) + return &secureFileWalker{ + root: root, + absChartPath: absChartPath, + maxSize: maxSize, + rules: rules, + files: make([]*loader.BufferedFile, 0), + } +} - // Normalize to / since it will also work on Windows - n = filepath.ToSlash(n) +func (w *secureFileWalker) walk(name, absName string, fi os.FileInfo, err error) error { + n := strings.TrimPrefix(name, w.absChartPath) + if n == "" { + // No need to process top level. Avoid bug with helmignore .* matching + // empty names. See issue 1779. + return nil + } - if err != nil { - return err - } - if fi.IsDir() { - // Directory-based ignore rules should involve skipping the entire - // contents of that directory. - if rules.Ignore(n, fi) { - return filepath.SkipDir - } - // Check after excluding ignores to provide the user with an option - // to opt-out from including certain paths. - if _, err := isSecureSymlinkPath(root, absoluteName); err != nil { - return fmt.Errorf("cannot load '%s' directory: %w", n, err) - } - return nil - } + if err != nil { + return err + } - // If a .helmignore file matches, skip this file. - if rules.Ignore(n, fi) { - return nil - } + // Normalize to / since it will also work on Windows + n = filepath.ToSlash(n) + if fi.IsDir() { + // Directory-based ignore rules should involve skipping the entire + // contents of that directory. + if w.rules.Ignore(n, fi) { + return filepath.SkipDir + } // Check after excluding ignores to provide the user with an option // to opt-out from including certain paths. - if _, err := isSecureSymlinkPath(root, absoluteName); err != nil { - return fmt.Errorf("cannot load '%s' file: %w", n, err) + if _, err := isSecureAbsolutePath(w.root, absName); err != nil { + return fmt.Errorf("cannot load '%s' directory: %w", n, err) } + return nil + } - // Irregular files include devices, sockets, and other uses of files that - // are not regular files. In Go they have a file mode type bit set. - // See https://golang.org/pkg/os/#FileMode for examples. - if !fi.Mode().IsRegular() { - return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", n) - } + // If a .helmignore file matches, skip this file. + if w.rules.Ignore(n, fi) { + return nil + } - if fileSize := fi.Size(); maxSize > 0 && fileSize > int64(maxSize) { - return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, maxSize) - } + // Check after excluding ignores to provide the user with an option + // to opt-out from including certain paths. + if _, err := isSecureAbsolutePath(w.root, absName); err != nil { + return fmt.Errorf("cannot load '%s' file: %w", n, err) + } - data, err := os.ReadFile(name) - if err != nil { - return fmt.Errorf("error reading %s: %w", n, err) - } - data = bytes.TrimPrefix(data, utf8bom) + // Irregular files include devices, sockets, and other uses of files that + // are not regular files. In Go they have a file mode type bit set. + // See https://golang.org/pkg/os/#FileMode for examples. + if !fi.Mode().IsRegular() { + return fmt.Errorf("cannot load irregular file %s as it has file mode type bits set", n) + } - files = append(files, &loader.BufferedFile{Name: n, Data: data}) - return nil + // Confirm size it not outside boundaries + if fileSize := fi.Size(); w.maxSize > 0 && fileSize > int64(w.maxSize) { + return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, w.maxSize) } - if err = sympath.Walk(topDir, walk); err != nil { - return c, err + + data, err := os.ReadFile(absName) + if err != nil { + if pathErr := new(fs.PathError); errors.As(err, &pathErr) { + err = &fs.PathError{Op: pathErr.Op, Path: strings.TrimPrefix(absName, w.root), Err: pathErr.Err} + } + return fmt.Errorf("error reading %s: %w", n, err) } - return loader.LoadFiles(files) + data = bytes.TrimPrefix(data, utf8bom) + + w.files = append(w.files, &loader.BufferedFile{Name: n, Data: data}) + return nil } -// isSecureSymlinkPath attempts to make the given absolute path relative to +// isSecureAbsolutePath attempts to make the given absolute path relative to // root and securely joins this with root. If the result equals absolute path, // it is safe to use. -func isSecureSymlinkPath(root, absPath string) (bool, error) { +func isSecureAbsolutePath(root, absPath string) (bool, error) { root, absPath = filepath.Clean(root), filepath.Clean(absPath) if root == "/" { return true, nil } unsafePath, err := filepath.Rel(root, absPath) if err != nil { - return false, fmt.Errorf("cannot calculate path relative to root for resolved symlink") + return false, fmt.Errorf("cannot calculate path relative to root for absolute path") } safePath, err := securejoin.SecureJoin(root, unsafePath) if err != nil { - return false, fmt.Errorf("cannot securely join root with resolved relative symlink path") + return false, fmt.Errorf("cannot securely join root with resolved relative path") } if safePath != absPath { - return false, fmt.Errorf("symlink traverses outside root boundary: relative path to root %s", unsafePath) + return false, fmt.Errorf("absolute path traverses outside root boundary: relative path to root %s", unsafePath) } return true, nil } diff --git a/internal/helm/chart/secureloader/directory_test.go b/internal/helm/chart/secureloader/directory_test.go index e2031062a..063b559c5 100644 --- a/internal/helm/chart/secureloader/directory_test.go +++ b/internal/helm/chart/secureloader/directory_test.go @@ -17,12 +17,349 @@ limitations under the License. package secureloader import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" "testing" + "testing/fstest" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/ignore" . "github.com/onsi/gomega" + "helm.sh/helm/v3/pkg/chart" + "sigs.k8s.io/yaml" ) -func Test_isSecureSymlinkPath(t *testing.T) { +func TestSecureDirLoader_Load(t *testing.T) { + metadata := chart.Metadata{ + Name: "test", + APIVersion: "v2", + Version: "1.0", + Type: "application", + } + + t.Run("chart", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + + got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(m.Name)) + }) + + t.Run("chart with absolute path", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + + got, err := (NewSecureDirLoader(tmpDir, tmpDir, DefaultMaxFileSize)).Load() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(m.Name)) + }) + + t.Run("chart with illegal path", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + + root := filepath.Join(tmpDir, "root") + g.Expect(os.Mkdir(root, 0o700)).To(Succeed()) + + got, err := (NewSecureDirLoader(root, "../", DefaultMaxFileSize)).Load() + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing")) + g.Expect(got).To(BeNil()) + + got, err = (NewSecureDirLoader(root, tmpDir, DefaultMaxFileSize)).Load() + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing")) + g.Expect(got).To(BeNil()) + }) + + t.Run("chart with .helmignore", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + m := metadata + b, err := yaml.Marshal(&m) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "file.txt"), []byte("not included"), 0o644)).To(Succeed()) + + got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load() + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(m.Name)) + g.Expect(got.Raw).To(HaveLen(2)) + }) +} + +func Test_secureLoadIgnoreRules(t *testing.T) { + t.Run("defaults", func(t *testing.T) { + g := NewWithT(t) + + r, err := secureLoadIgnoreRules("/workdir", "") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeFalse()) + g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue()) + }) + + t.Run("with "+ignore.HelmIgnore, func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed()) + + r, err := secureLoadIgnoreRules(tmpDir, "") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeTrue()) + g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue()) + g.Expect(r.Ignore("other.txt", nil)).To(BeFalse()) + }) + + t.Run("with chart path and "+ignore.HelmIgnore, func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + chartPath := "./sub/chart" + g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, chartPath, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed()) + + r, err := secureLoadIgnoreRules(tmpDir, chartPath) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeTrue()) + }) + + t.Run("with relative "+ignore.HelmIgnore+" symlink", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + chartPath := "sub/chart" + g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "symlink"), []byte("file.txt"), 0o644)).To(Succeed()) + g.Expect(os.Symlink("../../symlink", filepath.Join(tmpDir, chartPath, ignore.HelmIgnore))) + + r, err := secureLoadIgnoreRules(tmpDir, chartPath) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("file.txt", nil)).To(BeTrue()) + }) + + t.Run("with illegal "+ignore.HelmIgnore+" symlink", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + chartPath := "/sub/chart" + g.Expect(os.MkdirAll(filepath.Join(tmpDir, chartPath), 0o700)).To(Succeed()) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "symlink"), []byte("file.txt"), 0o644)).To(Succeed()) + g.Expect(os.Symlink("../../symlink", filepath.Join(tmpDir, chartPath, ignore.HelmIgnore))) + + r, err := secureLoadIgnoreRules(filepath.Join(tmpDir, chartPath), "") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(r.Ignore("templates/.dotfile", nil)).To(BeTrue()) + g.Expect(r.Ignore("file.txt", nil)).To(BeFalse()) + }) + + t.Run("with "+ignore.HelmIgnore+" parsing error", func(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("**"), 0o644)).To(Succeed()) + + _, err := secureLoadIgnoreRules(tmpDir, "") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("syntax is not supported")) + }) +} + +func Test_secureFileWalker_walk(t *testing.T) { + g := NewWithT(t) + + const ( + root = "/fake/root" + chartPath = "/fake/root/dir" + ) + + fakeDirName := "fake-dir" + fakeFileName := "fake-file" + fakeDeviceFileName := "fake-device" + fakeFS := fstest.MapFS{ + fakeDirName: &fstest.MapFile{Mode: fs.ModeDir}, + fakeFileName: &fstest.MapFile{Data: []byte("a couple bytes")}, + fakeDeviceFileName: &fstest.MapFile{Mode: fs.ModeDevice}, + } + + // Safe to further re-use this for other paths + fakeDirInfo, err := fakeFS.Stat(fakeDirName) + g.Expect(err).ToNot(HaveOccurred()) + fakeFileInfo, err := fakeFS.Stat(fakeFileName) + g.Expect(err).ToNot(HaveOccurred()) + fakeDeviceInfo, err := fakeFS.Stat(fakeDeviceFileName) + g.Expect(err).ToNot(HaveOccurred()) + + t.Run("given name equals top dir", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(chartPath+"/", chartPath, nil, nil)).To(BeNil()) + }) + + t.Run("given error is returned", func(t *testing.T) { + g := NewWithT(t) + + err := errors.New("error argument") + got := (&secureFileWalker{}).walk("name", "/name", nil, err) + g.Expect(got).To(HaveOccurred()) + g.Expect(got).To(Equal(err)) + }) + + t.Run("ignore rule matches dir", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir)) + }) + + t.Run("absolute path match ignored", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, "symlink"), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil()) + }) + + t.Run("ignore rule not applicable to dir", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil()) + }) + + t.Run("absolute path outside root", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-dir' directory: absolute path traverses outside root boundary")) + }) + + t.Run("dir ignore rules before secure path check", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir)) + }) + + t.Run("ignore rule matches file", func(t *testing.T) { + g := NewWithT(t) + + rules, err := ignore.Parse(strings.NewReader(fakeFileName)) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + g.Expect(w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join(w.absChartPath, fakeFileName), fakeFileInfo, nil)).To(BeNil()) + }) + + t.Run("file path outside root", func(t *testing.T) { + g := NewWithT(t) + + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join("/fake/another/root/", fakeFileName), fakeFileInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-file' file: absolute path traverses outside root boundary")) + }) + + t.Run("irregular file", func(t *testing.T) { + w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(fakeDeviceFileName, filepath.Join(w.absChartPath), fakeDeviceInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("cannot load irregular file fake-device as it has file mode type bits set")) + }) + + t.Run("file exceeds max size", func(t *testing.T) { + w := newSecureFileWalker(root, chartPath, 5, ignore.Empty()) + err := w.walk(fakeFileName, filepath.Join(w.absChartPath), fakeFileInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(Equal(fmt.Sprintf("cannot load file fake-file as file size (%d) exceeds limit (%d)", fakeFileInfo.Size(), w.maxSize))) + }) + + t.Run("file is appended", func(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + + fileName := "append-file" + fileData := []byte("append-file-data") + absFilePath := filepath.Join(tmpDir, fileName) + g.Expect(os.WriteFile(absFilePath, fileData, 0o644)).To(Succeed()) + fileInfo, err := os.Lstat(absFilePath) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed()) + g.Expect(w.files).To(HaveLen(1)) + g.Expect(w.files[0].Name).To(Equal(fileName)) + g.Expect(w.files[0].Data).To(Equal(fileData)) + }) + + t.Run("utf8bom is removed from file data", func(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + + fileName := "append-file" + fileData := []byte("append-file-data") + fileDataWithBom := append(utf8bom, fileData...) + absFilePath := filepath.Join(tmpDir, fileName) + g.Expect(os.WriteFile(absFilePath, fileDataWithBom, 0o644)).To(Succeed()) + fileInfo, err := os.Lstat(absFilePath) + g.Expect(err).ToNot(HaveOccurred()) + + w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed()) + g.Expect(w.files).To(HaveLen(1)) + g.Expect(w.files[0].Name).To(Equal(fileName)) + g.Expect(w.files[0].Data).To(Equal(fileData)) + }) + + t.Run("file does not exist", func(t *testing.T) { + g := NewWithT(t) + tmpDir := t.TempDir() + + w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + err := w.walk(filepath.Join(w.absChartPath, "invalid"), filepath.Join(w.absChartPath, "invalid"), fakeFileInfo, nil) + g.Expect(err).To(HaveOccurred()) + g.Expect(errors.Is(err, fs.ErrNotExist)).To(BeTrue()) + g.Expect(err.Error()).To(ContainSubstring("error reading invalid: open /invalid: no such file or directory")) + }) +} + +func Test_isSecureAbsolutePath(t *testing.T) { tests := []struct { name string root string @@ -42,7 +379,7 @@ func Test_isSecureSymlinkPath(t *testing.T) { root: "/working/dir", absPath: "/working/in/another/dir", safe: false, - wantErr: "symlink traverses outside root boundary", + wantErr: "absolute path traverses outside root boundary", }, { name: "abs path relative to root", @@ -55,21 +392,21 @@ func Test_isSecureSymlinkPath(t *testing.T) { root: "/working/dir", absPath: "/working/dir/../but/not/really", safe: false, - wantErr: "symlink traverses outside root boundary", + wantErr: "absolute path traverses outside root boundary", }, { name: "illegal root", root: "working/dir/", absPath: "/working/dir", safe: false, - wantErr: "cannot calculate path relative to root for resolved symlink", + wantErr: "cannot calculate path relative to root for absolute path", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - got, err := isSecureSymlinkPath(tt.root, tt.absPath) + got, err := isSecureAbsolutePath(tt.root, tt.absPath) g.Expect(got).To(Equal(tt.safe)) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) diff --git a/internal/helm/chart/secureloader/loader.go b/internal/helm/chart/secureloader/loader.go index 86ff2cf6d..25bce34bb 100644 --- a/internal/helm/chart/secureloader/loader.go +++ b/internal/helm/chart/secureloader/loader.go @@ -22,6 +22,7 @@ import ( "io/fs" "os" "path/filepath" + "strings" securejoin "github.com/cyphar/filepath-securejoin" "helm.sh/helm/v3/pkg/chart" @@ -34,14 +35,19 @@ import ( // Name can be an absolute or relative path, but always has to be inside // root. func Loader(root, name string) (loader.ChartLoader, error) { - root, name = filepath.Clean(root), filepath.Clean(name) - relName := name + root, err := filepath.Abs(root) + if err != nil { + return nil, err + } + + relName := filepath.Clean(name) if filepath.IsAbs(relName) { var err error if relName, err = filepath.Rel(root, name); err != nil { return nil, err } } + secureName, err := securejoin.SecureJoin(root, relName) if err != nil { return nil, err @@ -49,12 +55,13 @@ func Loader(root, name string) (loader.ChartLoader, error) { fi, err := os.Lstat(secureName) if err != nil { if pathErr := new(fs.PathError); errors.As(err, &pathErr) { - return nil, &fs.PathError{Op: pathErr.Op, Path: name, Err: pathErr.Err} + return nil, &fs.PathError{Op: pathErr.Op, Path: strings.TrimPrefix(secureName, root), Err: pathErr.Err} } return nil, err } + if fi.IsDir() { - return NewSecureDirLoader(root, secureName, 0), nil + return NewSecureDirLoader(root, relName, DefaultMaxFileSize), nil } return FileLoader(secureName), nil } diff --git a/internal/helm/chart/secureloader/loader_test.go b/internal/helm/chart/secureloader/loader_test.go index d0b69a846..d5032de67 100644 --- a/internal/helm/chart/secureloader/loader_test.go +++ b/internal/helm/chart/secureloader/loader_test.go @@ -23,7 +23,9 @@ import ( "testing" . "github.com/onsi/gomega" + "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" + "sigs.k8s.io/yaml" ) func TestLoader(t *testing.T) { @@ -33,22 +35,56 @@ func TestLoader(t *testing.T) { fakeChart := filepath.Join(tmpDir, "fake.tgz") g.Expect(os.WriteFile(fakeChart, []byte(""), 0o644)).To(Succeed()) - got, err := Loader(tmpDir, fakeChart) + t.Run("file loader", func(t *testing.T) { + g := NewWithT(t) + + got, err := Loader(tmpDir, fakeChart) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(loader.FileLoader(fakeChart))) + }) + + t.Run("dir loader", func(t *testing.T) { + g := NewWithT(t) + + fakeChartPath := filepath.Join(tmpDir, "fake") + g.Expect(os.Mkdir(fakeChartPath, 0o700)).To(Succeed()) + + got, err := Loader(tmpDir, "fake") + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, path: "fake", maxSize: DefaultMaxFileSize})) + }) + + t.Run("illegal path", func(t *testing.T) { + g := NewWithT(t) + + symlinkRoot := filepath.Join(tmpDir, "symlink") + g.Expect(os.Mkdir(symlinkRoot, 0o700)).To(Succeed()) + symlinkPath := filepath.Join(symlinkRoot, "fake.tgz") + g.Expect(os.Symlink(fakeChart, symlinkPath)) + + got, err := Loader(symlinkRoot, symlinkPath) + g.Expect(err).To(HaveOccurred()) + g.Expect(err).To(BeAssignableToTypeOf(&fs.PathError{})) + g.Expect(got).To(BeNil()) + }) +} + +func TestLoad(t *testing.T) { + g := NewWithT(t) + + tmpDir := t.TempDir() + metadata := chart.Metadata{ + Name: "test", + APIVersion: "v2", + Version: "1.0", + Type: "application", + } + b, err := yaml.Marshal(&metadata) g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(loader.FileLoader(fakeChart))) + g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) - fakeChartPath := filepath.Join(tmpDir, "fake") - g.Expect(os.Mkdir(fakeChartPath, 0o700)).To(Succeed()) - got, err = Loader(tmpDir, "fake") + got, err := Load(tmpDir, "") g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, dir: fakeChartPath, maxSize: DefaultMaxFileSize})) - - symlinkRoot := filepath.Join(tmpDir, "symlink") - g.Expect(os.Mkdir(symlinkRoot, 0o700)).To(Succeed()) - symlinkPath := filepath.Join(symlinkRoot, "fake.tgz") - g.Expect(os.Symlink(fakeChart, symlinkPath)) - got, err = Loader(symlinkRoot, symlinkPath) - g.Expect(err).To(HaveOccurred()) - g.Expect(err).To(BeAssignableToTypeOf(&fs.PathError{})) - g.Expect(got).To(BeNil()) + g.Expect(got).ToNot(BeNil()) + g.Expect(got.Name()).To(Equal(metadata.Name)) } From e85ea781e24a53a8b65a203c3455ed5714d3babd Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 8 Apr 2022 11:34:56 +0200 Subject: [PATCH 6/7] helm: switch to our own chart loader package This includes some rewiring of tests, and slight changes in how we work with the local chart reference. `Path` is expected to be relative to `WorkDir`, and both fields are now mandatory. Signed-off-by: Hidde Beydals --- controllers/helmchart_controller.go | 15 +------ internal/helm/chart/builder.go | 11 ++++- internal/helm/chart/builder_local.go | 24 +++++++---- internal/helm/chart/builder_local_test.go | 41 +++++++++++++------ internal/helm/chart/builder_remote.go | 4 +- internal/helm/chart/builder_remote_test.go | 4 +- internal/helm/chart/builder_test.go | 26 ++++++++---- internal/helm/chart/dependency_manager.go | 14 +++---- .../helm/chart/dependency_manager_test.go | 16 +++++--- 9 files changed, 93 insertions(+), 62 deletions(-) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index ab64d2dca..2b4b498e7 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -28,7 +28,6 @@ import ( "strings" "time" - securejoin "github.com/cyphar/filepath-securejoin" helmgetter "helm.sh/helm/v3/pkg/getter" helmrepo "helm.sh/helm/v3/pkg/repo" corev1 "k8s.io/api/core/v1" @@ -609,18 +608,6 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj } } - // Calculate (secure) absolute chart path - chartPath, err := securejoin.SecureJoin(sourceDir, obj.Spec.Chart) - if err != nil { - e := &serror.Stalling{ - Err: fmt.Errorf("path calculation for chart '%s' failed: %w", obj.Spec.Chart, err), - Reason: "IllegalPath", - } - conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error()) - // We are unable to recover from this change without a change in generation - return sreconcile.ResultEmpty, e - } - // Setup dependency manager dm := chart.NewDependencyManager( chart.WithRepositoryCallback(r.namespacedChartRepositoryCallback(ctx, obj.GetNamespace())), @@ -673,7 +660,7 @@ func (r *HelmChartReconciler) buildFromTarballArtifact(ctx context.Context, obj cb := chart.NewLocalBuilder(dm) build, err := cb.Build(ctx, chart.LocalReference{ WorkDir: sourceDir, - Path: chartPath, + Path: obj.Spec.Chart, }, util.TempPathForObj("", ".tgz", obj), opts) if err != nil { return sreconcile.ResultEmpty, err diff --git a/internal/helm/chart/builder.go b/internal/helm/chart/builder.go index e3ce2207d..36486c9bf 100644 --- a/internal/helm/chart/builder.go +++ b/internal/helm/chart/builder.go @@ -43,16 +43,25 @@ type LocalReference struct { // WorkDir used as chroot during build operations. // File references are not allowed to traverse outside it. WorkDir string - // Path of the chart on the local filesystem. + // Path of the chart on the local filesystem relative to WorkDir. Path string } // Validate returns an error if the LocalReference does not have // a Path set. func (r LocalReference) Validate() error { + if r.WorkDir == "" { + return fmt.Errorf("no work dir set for local chart reference") + } if r.Path == "" { return fmt.Errorf("no path set for local chart reference") } + if !filepath.IsAbs(r.WorkDir) { + return fmt.Errorf("local chart reference work dir is expected to be absolute") + } + if filepath.IsAbs(r.Path) { + return fmt.Errorf("local chart reference path is expected to be relative") + } return nil } diff --git a/internal/helm/chart/builder_local.go b/internal/helm/chart/builder_local.go index 2710e41a9..0e0b20c28 100644 --- a/internal/helm/chart/builder_local.go +++ b/internal/helm/chart/builder_local.go @@ -24,10 +24,11 @@ import ( "github.com/Masterminds/semver/v3" securejoin "github.com/cyphar/filepath-securejoin" - "helm.sh/helm/v3/pkg/chart/loader" "sigs.k8s.io/yaml" "github.com/fluxcd/pkg/runtime/transform" + + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" ) type localChartBuilder struct { @@ -75,7 +76,11 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // Load the chart metadata from the LocalReference to ensure it points // to a chart - curMeta, err := LoadChartMetadata(localRef.Path) + securePath, err := securejoin.SecureJoin(localRef.WorkDir, localRef.Path) + if err != nil { + return nil, &BuildError{Reason: ErrChartReference, Err: err} + } + curMeta, err := LoadChartMetadata(securePath) if err != nil { return nil, &BuildError{Reason: ErrChartReference, Err: err} } @@ -101,7 +106,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, result.Version = ver.String() } - isChartDir := pathIsDir(localRef.Path) + isChartDir := pathIsDir(securePath) requiresPackaging := isChartDir || opts.VersionMetadata != "" || len(opts.GetValuesFiles()) != 0 // If all the following is true, we do not need to package the chart: @@ -127,7 +132,7 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // If the chart at the path is already packaged and no custom values files // options are set, we can copy the chart without making modifications if !requiresPackaging { - if err = copyFileToPath(localRef.Path, p); err != nil { + if err = copyFileToPath(securePath, p); err != nil { return result, &BuildError{Reason: ErrChartPull, Err: err} } result.Path = p @@ -145,15 +150,16 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, // At this point we are certain we need to load the chart; // either to package it because it originates from a directory, // or because we have merged values and need to repackage - chart, err := loader.Load(localRef.Path) + loadedChart, err := secureloader.Load(localRef.WorkDir, localRef.Path) if err != nil { return result, &BuildError{Reason: ErrChartPackage, Err: err} } + // Set earlier resolved version (with metadata) - chart.Metadata.Version = result.Version + loadedChart.Metadata.Version = result.Version // Overwrite default values with merged values, if any - if ok, err = OverwriteChartDefaultValues(chart, mergedValues); ok || err != nil { + if ok, err = OverwriteChartDefaultValues(loadedChart, mergedValues); ok || err != nil { if err != nil { return result, &BuildError{Reason: ErrValuesFilesMerge, Err: err} } @@ -166,13 +172,13 @@ func (b *localChartBuilder) Build(ctx context.Context, ref Reference, p string, err = fmt.Errorf("local chart builder requires dependency manager for unpackaged charts") return result, &BuildError{Reason: ErrDependencyBuild, Err: err} } - if result.ResolvedDependencies, err = b.dm.Build(ctx, ref, chart); err != nil { + if result.ResolvedDependencies, err = b.dm.Build(ctx, ref, loadedChart); err != nil { return result, &BuildError{Reason: ErrDependencyBuild, Err: err} } } // Package the chart - if err = packageToPath(chart, p); err != nil { + if err = packageToPath(loadedChart, p); err != nil { return result, &BuildError{Reason: ErrChartPackage, Err: err} } result.Path = p diff --git a/internal/helm/chart/builder_local_test.go b/internal/helm/chart/builder_local_test.go index cff5f180f..10f681fd3 100644 --- a/internal/helm/chart/builder_local_test.go +++ b/internal/helm/chart/builder_local_test.go @@ -26,10 +26,10 @@ import ( . "github.com/onsi/gomega" "github.com/otiai10/copy" helmchart "helm.sh/helm/v3/pkg/chart" - "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/repo" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" "github.com/fluxcd/source-controller/internal/helm/repository" ) @@ -86,31 +86,31 @@ func TestLocalBuilder_Build(t *testing.T) { }, { name: "invalid local reference - no file", - reference: LocalReference{Path: "/tmp/non-existent-path.xyz"}, + reference: LocalReference{WorkDir: "/tmp", Path: "non-existent-path.xyz"}, wantErr: "no such file or directory", }, { name: "invalid version metadata", - reference: LocalReference{Path: "./../testdata/charts/helmchart"}, + reference: LocalReference{Path: "../testdata/charts/helmchart"}, buildOpts: BuildOptions{VersionMetadata: "^"}, wantErr: "Invalid Metadata string", }, { name: "with version metadata", - reference: LocalReference{Path: "./../testdata/charts/helmchart"}, + reference: LocalReference{Path: "../testdata/charts/helmchart"}, buildOpts: BuildOptions{VersionMetadata: "foo"}, wantVersion: "0.1.0+foo", wantPackaged: true, }, { name: "already packaged chart", - reference: LocalReference{Path: "./../testdata/charts/helmchart-0.1.0.tgz"}, + reference: LocalReference{Path: "../testdata/charts/helmchart-0.1.0.tgz"}, wantVersion: "0.1.0", wantPackaged: false, }, { name: "default values", - reference: LocalReference{Path: "./../testdata/charts/helmchart"}, + reference: LocalReference{Path: "../testdata/charts/helmchart"}, wantValues: chartutil.Values{ "replicaCount": float64(1), }, @@ -119,7 +119,7 @@ func TestLocalBuilder_Build(t *testing.T) { }, { name: "with values files", - reference: LocalReference{Path: "./../testdata/charts/helmchart"}, + reference: LocalReference{Path: "../testdata/charts/helmchart"}, buildOpts: BuildOptions{ ValuesFiles: []string{"custom-values1.yaml", "custom-values2.yaml"}, }, @@ -145,7 +145,7 @@ fullnameOverride: "full-foo-name-override"`), }, { name: "chart with dependencies", - reference: LocalReference{Path: "./../testdata/charts/helmchartwithdeps"}, + reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps"}, repositories: map[string]*repository.ChartRepository{ "https://grafana.github.io/helm-charts/": mockRepo(), }, @@ -164,11 +164,11 @@ fullnameOverride: "full-foo-name-override"`), }, { name: "v1 chart with dependencies", - reference: LocalReference{Path: "./../testdata/charts/helmchartwithdeps-v1"}, + reference: LocalReference{Path: "../testdata/charts/helmchartwithdeps-v1"}, repositories: map[string]*repository.ChartRepository{ "https://grafana.github.io/helm-charts/": mockRepo(), }, - dependentChartPaths: []string{"./../testdata/charts/helmchart-v1"}, + dependentChartPaths: []string{"../testdata/charts/helmchart-v1"}, wantVersion: "0.3.0", wantPackaged: true, }, @@ -184,13 +184,23 @@ fullnameOverride: "full-foo-name-override"`), // Only if the reference is a LocalReference, set the WorkDir. localRef, ok := tt.reference.(LocalReference) if ok { + // If the source chart path is valid, copy it into the workdir + // and update the localRef.Path with the copied local chart + // path. + if localRef.Path != "" { + _, err := os.Lstat(localRef.Path) + if err == nil { + helmchartDir := filepath.Join(workDir, "testdata", "charts", filepath.Base(localRef.Path)) + g.Expect(copy.Copy(localRef.Path, helmchartDir)).ToNot(HaveOccurred()) + } + } localRef.WorkDir = workDir tt.reference = localRef } // Write value file in the base dir. for _, f := range tt.valuesFiles { - vPath := filepath.Join(workDir, f.Name) + vPath := filepath.Join(localRef.WorkDir, f.Name) g.Expect(os.WriteFile(vPath, f.Data, 0644)).ToNot(HaveOccurred()) } @@ -223,7 +233,7 @@ fullnameOverride: "full-foo-name-override"`), g.Expect(cb.Path).ToNot(BeEmpty(), "empty Build.Path") // Load the resulting chart and verify the values. - resultChart, err := loader.Load(cb.Path) + resultChart, err := secureloader.LoadFile(cb.Path) g.Expect(err).ToNot(HaveOccurred()) g.Expect(resultChart.Metadata.Version).To(Equal(tt.wantVersion)) @@ -241,7 +251,7 @@ func TestLocalBuilder_Build_CachedChart(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer os.RemoveAll(workDir) - reference := LocalReference{Path: "./../testdata/charts/helmchart"} + testChartPath := "./../testdata/charts/helmchart" dm := NewDependencyManager() b := NewLocalBuilder(dm) @@ -250,6 +260,11 @@ func TestLocalBuilder_Build_CachedChart(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer os.RemoveAll(tmpDir) + // Copy the source chart into the workdir. + g.Expect(copy.Copy(testChartPath, filepath.Join(workDir, "testdata", "charts", filepath.Base("helmchart")))).ToNot(HaveOccurred()) + + reference := LocalReference{WorkDir: workDir, Path: testChartPath} + // Build first time. targetPath := filepath.Join(tmpDir, "chart1.tgz") buildOpts := BuildOptions{} diff --git a/internal/helm/chart/builder_remote.go b/internal/helm/chart/builder_remote.go index b3594cefb..e6543dfef 100644 --- a/internal/helm/chart/builder_remote.go +++ b/internal/helm/chart/builder_remote.go @@ -25,13 +25,13 @@ import ( "github.com/Masterminds/semver/v3" helmchart "helm.sh/helm/v3/pkg/chart" - "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" "sigs.k8s.io/yaml" "github.com/fluxcd/pkg/runtime/transform" "github.com/fluxcd/source-controller/internal/fs" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" "github.com/fluxcd/source-controller/internal/helm/repository" ) @@ -145,7 +145,7 @@ func (b *remoteChartBuilder) Build(_ context.Context, ref Reference, p string, o // Load the chart and merge chart values var chart *helmchart.Chart - if chart, err = loader.LoadArchive(res); err != nil { + if chart, err = secureloader.LoadArchive(res); err != nil { err = fmt.Errorf("failed to load downloaded chart: %w", err) return result, &BuildError{Reason: ErrChartPackage, Err: err} } diff --git a/internal/helm/chart/builder_remote_test.go b/internal/helm/chart/builder_remote_test.go index 015b1bdac..604aa6006 100644 --- a/internal/helm/chart/builder_remote_test.go +++ b/internal/helm/chart/builder_remote_test.go @@ -27,10 +27,10 @@ import ( . "github.com/onsi/gomega" helmchart "helm.sh/helm/v3/pkg/chart" - "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" helmgetter "helm.sh/helm/v3/pkg/getter" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" "github.com/fluxcd/source-controller/internal/helm/repository" ) @@ -186,7 +186,7 @@ entries: g.Expect(cb.Path).ToNot(BeEmpty(), "empty Build.Path") // Load the resulting chart and verify the values. - resultChart, err := loader.Load(cb.Path) + resultChart, err := secureloader.LoadFile(cb.Path) g.Expect(err).ToNot(HaveOccurred()) g.Expect(resultChart.Metadata.Version).To(Equal(tt.wantVersion)) diff --git a/internal/helm/chart/builder_test.go b/internal/helm/chart/builder_test.go index 4d0812298..0671cea17 100644 --- a/internal/helm/chart/builder_test.go +++ b/internal/helm/chart/builder_test.go @@ -24,8 +24,9 @@ import ( "testing" . "github.com/onsi/gomega" - "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" + + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" ) func TestLocalReference_Validate(t *testing.T) { @@ -35,18 +36,29 @@ func TestLocalReference_Validate(t *testing.T) { wantErr string }{ { - name: "ref with path", - ref: LocalReference{Path: "/a/path"}, + name: "ref with path and work dir", + ref: LocalReference{WorkDir: "/workdir/", Path: "./a/path"}, }, { - name: "ref with path and work dir", - ref: LocalReference{Path: "/a/path", WorkDir: "/with/a/workdir"}, + name: "ref without work dir", + ref: LocalReference{Path: "/a/path"}, + wantErr: "no work dir set for local chart reference", + }, + { + name: "ref with relative work dir", + ref: LocalReference{WorkDir: "../a/path", Path: "foo"}, + wantErr: "local chart reference work dir is expected to be absolute", }, { name: "ref without path", ref: LocalReference{WorkDir: "/just/a/workdir"}, wantErr: "no path set for local chart reference", }, + { + name: "ref with an absolute path", + ref: LocalReference{WorkDir: "/a/path", Path: "/foo"}, + wantErr: "local chart reference path is expected to be relative", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -210,7 +222,7 @@ func TestChartBuildResult_String(t *testing.T) { func Test_packageToPath(t *testing.T) { g := NewWithT(t) - chart, err := loader.Load("../testdata/charts/helmchart-0.1.0.tgz") + chart, err := secureloader.LoadFile("../testdata/charts/helmchart-0.1.0.tgz") g.Expect(err).ToNot(HaveOccurred()) g.Expect(chart).ToNot(BeNil()) @@ -219,7 +231,7 @@ func Test_packageToPath(t *testing.T) { err = packageToPath(chart, out) g.Expect(err).ToNot(HaveOccurred()) g.Expect(out).To(BeARegularFile()) - _, err = loader.Load(out) + _, err = secureloader.LoadFile(out) g.Expect(err).ToNot(HaveOccurred()) } diff --git a/internal/helm/chart/dependency_manager.go b/internal/helm/chart/dependency_manager.go index 1a053e623..246159cfa 100644 --- a/internal/helm/chart/dependency_manager.go +++ b/internal/helm/chart/dependency_manager.go @@ -30,8 +30,8 @@ import ( "golang.org/x/sync/errgroup" "golang.org/x/sync/semaphore" helmchart "helm.sh/helm/v3/pkg/chart" - "helm.sh/helm/v3/pkg/chart/loader" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" "github.com/fluxcd/source-controller/internal/helm/repository" ) @@ -191,7 +191,7 @@ func (dm *DependencyManager) addLocalDependency(ref LocalReference, c *chartWith if _, err := os.Stat(sLocalChartPath); err != nil { if os.IsNotExist(err) { - return fmt.Errorf("no chart found at '%s' (reference '%s')", sLocalChartPath, dep.Repository) + return fmt.Errorf("no chart found at '%s' (reference '%s')", strings.TrimPrefix(sLocalChartPath, ref.WorkDir), dep.Repository) } return err } @@ -202,7 +202,7 @@ func (dm *DependencyManager) addLocalDependency(ref LocalReference, c *chartWith return err } - ch, err := loader.Load(sLocalChartPath) + ch, err := secureloader.Load(ref.WorkDir, sLocalChartPath) if err != nil { return fmt.Errorf("failed to load chart from '%s' (reference '%s'): %w", strings.TrimPrefix(sLocalChartPath, ref.WorkDir), dep.Repository, err) @@ -245,7 +245,7 @@ func (dm *DependencyManager) addRemoteDependency(chart *chartWithLock, dep *helm if err != nil { return fmt.Errorf("chart download of version '%s' failed: %w", ver.Version, err) } - ch, err := loader.LoadArchive(res) + ch, err := secureloader.LoadArchive(res) if err != nil { return fmt.Errorf("failed to load downloaded archive of version '%s': %w", ver.Version, err) } @@ -290,11 +290,7 @@ func (dm *DependencyManager) secureLocalChartPath(ref LocalReference, dep *helmc if localUrl.Scheme != "" && localUrl.Scheme != "file" { return "", fmt.Errorf("'%s' is not a local chart reference", dep.Repository) } - relPath, err := filepath.Rel(ref.WorkDir, ref.Path) - if err != nil { - relPath = ref.Path - } - return securejoin.SecureJoin(ref.WorkDir, filepath.Join(relPath, localUrl.Host, localUrl.Path)) + return securejoin.SecureJoin(ref.WorkDir, filepath.Join(ref.Path, localUrl.Host, localUrl.Path)) } // collectMissing returns a map with dependencies from reqs that are missing diff --git a/internal/helm/chart/dependency_manager_test.go b/internal/helm/chart/dependency_manager_test.go index 04c0fc46e..d3e5ee173 100644 --- a/internal/helm/chart/dependency_manager_test.go +++ b/internal/helm/chart/dependency_manager_test.go @@ -28,10 +28,10 @@ import ( . "github.com/onsi/gomega" helmchart "helm.sh/helm/v3/pkg/chart" - "helm.sh/helm/v3/pkg/chart/loader" helmgetter "helm.sh/helm/v3/pkg/getter" "helm.sh/helm/v3/pkg/repo" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader" "github.com/fluxcd/source-controller/internal/helm/repository" ) @@ -166,14 +166,16 @@ func TestDependencyManager_Build(t *testing.T) { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - chart, err := loader.Load(filepath.Join(tt.baseDir, tt.path)) + chart, err := secureloader.Load(tt.baseDir, tt.path) g.Expect(err).ToNot(HaveOccurred()) dm := NewDependencyManager( WithRepositories(tt.repositories), WithRepositoryCallback(tt.getChartRepositoryCallback), ) - got, err := dm.Build(context.TODO(), LocalReference{WorkDir: tt.baseDir, Path: tt.path}, chart) + absBaseDir, err := filepath.Abs(tt.baseDir) + g.Expect(err).ToNot(HaveOccurred()) + got, err := dm.Build(context.TODO(), LocalReference{WorkDir: absBaseDir, Path: tt.path}, chart) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) @@ -262,7 +264,7 @@ func TestDependencyManager_addLocalDependency(t *testing.T) { Version: chartVersion, Repository: "file://../../../absolutely/invalid", }, - wantErr: "no chart found at '../testdata/charts/absolutely/invalid'", + wantErr: "no chart found at '/absolutely/invalid'", }, { name: "invalid chart archive", @@ -289,7 +291,11 @@ func TestDependencyManager_addLocalDependency(t *testing.T) { dm := NewDependencyManager() chart := &helmchart.Chart{} - err := dm.addLocalDependency(LocalReference{WorkDir: "../testdata/charts", Path: "helmchartwithdeps"}, + + absWorkDir, err := filepath.Abs("../testdata/charts") + g.Expect(err).ToNot(HaveOccurred()) + + err = dm.addLocalDependency(LocalReference{WorkDir: absWorkDir, Path: "helmchartwithdeps"}, &chartWithLock{Chart: chart}, tt.dep) if tt.wantErr != "" { g.Expect(err).To(HaveOccurred()) From 9a17fd53e7cb794acc8f25ddbf385111ddf63467 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Mon, 11 Apr 2022 10:12:47 +0200 Subject: [PATCH 7/7] helm: attach loader to helm.MaxChartFileSize Signed-off-by: Hidde Beydals --- internal/helm/chart/secureloader/directory.go | 21 +++++----- .../helm/chart/secureloader/directory_test.go | 38 ++++++++++--------- internal/helm/chart/secureloader/loader.go | 4 +- .../helm/chart/secureloader/loader_test.go | 4 +- 4 files changed, 35 insertions(+), 32 deletions(-) diff --git a/internal/helm/chart/secureloader/directory.go b/internal/helm/chart/secureloader/directory.go index 1c4b5f4b9..90285758b 100644 --- a/internal/helm/chart/secureloader/directory.go +++ b/internal/helm/chart/secureloader/directory.go @@ -37,15 +37,12 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" + "github.com/fluxcd/source-controller/internal/helm" "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/ignore" "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/sympath" ) var ( - // DefaultMaxFileSize is the default maximum file size of any chart file - // loaded. - DefaultMaxFileSize = 16 << 20 // 16MiB - utf8bom = []byte{0xEF, 0xBB, 0xBF} ) @@ -54,16 +51,16 @@ var ( type SecureDirLoader struct { root string path string - maxSize int + maxSize int64 } // NewSecureDirLoader returns a new SecureDirLoader, configured to the scope of the // root and provided dir. Max size configures the maximum size a file must not -// exceed to be loaded. If 0 it defaults to DefaultMaxFileSize, it can be +// exceed to be loaded. If 0 it defaults to helm.MaxChartFileSize, it can be // disabled using a negative integer. -func NewSecureDirLoader(root string, path string, maxSize int) SecureDirLoader { +func NewSecureDirLoader(root string, path string, maxSize int64) SecureDirLoader { if maxSize == 0 { - maxSize = DefaultMaxFileSize + maxSize = helm.MaxChartFileSize } return SecureDirLoader{ root: root, @@ -80,7 +77,7 @@ func (l SecureDirLoader) Load() (*chart.Chart, error) { // SecureLoadDir securely loads a chart from the path relative to root, without // traversing outside root. When maxSize >= 0, files are not allowed to exceed // this size, or an error is returned. -func SecureLoadDir(root, path string, maxSize int) (*chart.Chart, error) { +func SecureLoadDir(root, path string, maxSize int64) (*chart.Chart, error) { root, err := filepath.Abs(root) if err != nil { return nil, err @@ -152,12 +149,12 @@ func secureLoadIgnoreRules(root, chartPath string) (*ignore.Rules, error) { type secureFileWalker struct { root string absChartPath string - maxSize int + maxSize int64 rules *ignore.Rules files []*loader.BufferedFile } -func newSecureFileWalker(root, absChartPath string, maxSize int, rules *ignore.Rules) *secureFileWalker { +func newSecureFileWalker(root, absChartPath string, maxSize int64, rules *ignore.Rules) *secureFileWalker { absChartPath = filepath.Clean(absChartPath) + string(filepath.Separator) return &secureFileWalker{ root: root, @@ -216,7 +213,7 @@ func (w *secureFileWalker) walk(name, absName string, fi os.FileInfo, err error) } // Confirm size it not outside boundaries - if fileSize := fi.Size(); w.maxSize > 0 && fileSize > int64(w.maxSize) { + if fileSize := fi.Size(); w.maxSize > 0 && fileSize > w.maxSize { return fmt.Errorf("cannot load file %s as file size (%d) exceeds limit (%d)", n, fileSize, w.maxSize) } diff --git a/internal/helm/chart/secureloader/directory_test.go b/internal/helm/chart/secureloader/directory_test.go index 063b559c5..a0594fd74 100644 --- a/internal/helm/chart/secureloader/directory_test.go +++ b/internal/helm/chart/secureloader/directory_test.go @@ -26,10 +26,12 @@ import ( "testing" "testing/fstest" - "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/ignore" . "github.com/onsi/gomega" "helm.sh/helm/v3/pkg/chart" "sigs.k8s.io/yaml" + + "github.com/fluxcd/source-controller/internal/helm" + "github.com/fluxcd/source-controller/internal/helm/chart/secureloader/ignore" ) func TestSecureDirLoader_Load(t *testing.T) { @@ -49,7 +51,7 @@ func TestSecureDirLoader_Load(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) - got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load() + got, err := (NewSecureDirLoader(tmpDir, "", helm.MaxChartFileSize)).Load() g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).ToNot(BeNil()) g.Expect(got.Name()).To(Equal(m.Name)) @@ -64,7 +66,7 @@ func TestSecureDirLoader_Load(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) g.Expect(os.WriteFile(filepath.Join(tmpDir, "Chart.yaml"), b, 0o644)).To(Succeed()) - got, err := (NewSecureDirLoader(tmpDir, tmpDir, DefaultMaxFileSize)).Load() + got, err := (NewSecureDirLoader(tmpDir, tmpDir, helm.MaxChartFileSize)).Load() g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).ToNot(BeNil()) g.Expect(got.Name()).To(Equal(m.Name)) @@ -83,12 +85,12 @@ func TestSecureDirLoader_Load(t *testing.T) { root := filepath.Join(tmpDir, "root") g.Expect(os.Mkdir(root, 0o700)).To(Succeed()) - got, err := (NewSecureDirLoader(root, "../", DefaultMaxFileSize)).Load() + got, err := (NewSecureDirLoader(root, "../", helm.MaxChartFileSize)).Load() g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing")) g.Expect(got).To(BeNil()) - got, err = (NewSecureDirLoader(root, tmpDir, DefaultMaxFileSize)).Load() + got, err = (NewSecureDirLoader(root, tmpDir, helm.MaxChartFileSize)).Load() g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("failed to load chart from /: Chart.yaml file is missing")) g.Expect(got).To(BeNil()) @@ -105,7 +107,7 @@ func TestSecureDirLoader_Load(t *testing.T) { g.Expect(os.WriteFile(filepath.Join(tmpDir, ignore.HelmIgnore), []byte("file.txt"), 0o644)).To(Succeed()) g.Expect(os.WriteFile(filepath.Join(tmpDir, "file.txt"), []byte("not included"), 0o644)).To(Succeed()) - got, err := (NewSecureDirLoader(tmpDir, "", DefaultMaxFileSize)).Load() + got, err := (NewSecureDirLoader(tmpDir, "", helm.MaxChartFileSize)).Load() g.Expect(err).ToNot(HaveOccurred()) g.Expect(got).ToNot(BeNil()) g.Expect(got.Name()).To(Equal(m.Name)) @@ -218,7 +220,7 @@ func Test_secureFileWalker_walk(t *testing.T) { t.Run("given name equals top dir", func(t *testing.T) { g := NewWithT(t) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, ignore.Empty()) g.Expect(w.walk(chartPath+"/", chartPath, nil, nil)).To(BeNil()) }) @@ -237,7 +239,7 @@ func Test_secureFileWalker_walk(t *testing.T) { rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) g.Expect(err).ToNot(HaveOccurred()) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, rules) g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir)) }) @@ -247,21 +249,21 @@ func Test_secureFileWalker_walk(t *testing.T) { rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) g.Expect(err).ToNot(HaveOccurred()) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, rules) g.Expect(w.walk(filepath.Join(w.absChartPath, "symlink"), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil()) }) t.Run("ignore rule not applicable to dir", func(t *testing.T) { g := NewWithT(t) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, ignore.Empty()) g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join(w.absChartPath, fakeDirName), fakeDirInfo, nil)).To(BeNil()) }) t.Run("absolute path outside root", func(t *testing.T) { g := NewWithT(t) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, ignore.Empty()) err := w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-dir' directory: absolute path traverses outside root boundary")) @@ -273,7 +275,7 @@ func Test_secureFileWalker_walk(t *testing.T) { rules, err := ignore.Parse(strings.NewReader(fakeDirName + "/")) g.Expect(err).ToNot(HaveOccurred()) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, rules) g.Expect(w.walk(filepath.Join(w.absChartPath, fakeDirName), filepath.Join("/fake/another/root/", fakeDirName), fakeDirInfo, nil)).To(Equal(fs.SkipDir)) }) @@ -283,21 +285,21 @@ func Test_secureFileWalker_walk(t *testing.T) { rules, err := ignore.Parse(strings.NewReader(fakeFileName)) g.Expect(err).ToNot(HaveOccurred()) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, rules) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, rules) g.Expect(w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join(w.absChartPath, fakeFileName), fakeFileInfo, nil)).To(BeNil()) }) t.Run("file path outside root", func(t *testing.T) { g := NewWithT(t) - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, ignore.Empty()) err := w.walk(filepath.Join(w.absChartPath, fakeFileName), filepath.Join("/fake/another/root/", fakeFileName), fakeFileInfo, nil) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("cannot load 'fake-file' file: absolute path traverses outside root boundary")) }) t.Run("irregular file", func(t *testing.T) { - w := newSecureFileWalker(root, chartPath, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(root, chartPath, helm.MaxChartFileSize, ignore.Empty()) err := w.walk(fakeDeviceFileName, filepath.Join(w.absChartPath), fakeDeviceInfo, nil) g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring("cannot load irregular file fake-device as it has file mode type bits set")) @@ -321,7 +323,7 @@ func Test_secureFileWalker_walk(t *testing.T) { fileInfo, err := os.Lstat(absFilePath) g.Expect(err).ToNot(HaveOccurred()) - w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(tmpDir, tmpDir, helm.MaxChartFileSize, ignore.Empty()) g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed()) g.Expect(w.files).To(HaveLen(1)) g.Expect(w.files[0].Name).To(Equal(fileName)) @@ -340,7 +342,7 @@ func Test_secureFileWalker_walk(t *testing.T) { fileInfo, err := os.Lstat(absFilePath) g.Expect(err).ToNot(HaveOccurred()) - w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(tmpDir, tmpDir, helm.MaxChartFileSize, ignore.Empty()) g.Expect(w.walk(fileName, absFilePath, fileInfo, nil)).To(Succeed()) g.Expect(w.files).To(HaveLen(1)) g.Expect(w.files[0].Name).To(Equal(fileName)) @@ -351,7 +353,7 @@ func Test_secureFileWalker_walk(t *testing.T) { g := NewWithT(t) tmpDir := t.TempDir() - w := newSecureFileWalker(tmpDir, tmpDir, DefaultMaxFileSize, ignore.Empty()) + w := newSecureFileWalker(tmpDir, tmpDir, helm.MaxChartFileSize, ignore.Empty()) err := w.walk(filepath.Join(w.absChartPath, "invalid"), filepath.Join(w.absChartPath, "invalid"), fakeFileInfo, nil) g.Expect(err).To(HaveOccurred()) g.Expect(errors.Is(err, fs.ErrNotExist)).To(BeTrue()) diff --git a/internal/helm/chart/secureloader/loader.go b/internal/helm/chart/secureloader/loader.go index 25bce34bb..e17adc314 100644 --- a/internal/helm/chart/secureloader/loader.go +++ b/internal/helm/chart/secureloader/loader.go @@ -27,6 +27,8 @@ import ( securejoin "github.com/cyphar/filepath-securejoin" "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" + + "github.com/fluxcd/source-controller/internal/helm" ) // Loader returns a new loader.ChartLoader appropriate for the given chart @@ -61,7 +63,7 @@ func Loader(root, name string) (loader.ChartLoader, error) { } if fi.IsDir() { - return NewSecureDirLoader(root, relName, DefaultMaxFileSize), nil + return NewSecureDirLoader(root, relName, helm.MaxChartFileSize), nil } return FileLoader(secureName), nil } diff --git a/internal/helm/chart/secureloader/loader_test.go b/internal/helm/chart/secureloader/loader_test.go index d5032de67..374948cdb 100644 --- a/internal/helm/chart/secureloader/loader_test.go +++ b/internal/helm/chart/secureloader/loader_test.go @@ -26,6 +26,8 @@ import ( "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" "sigs.k8s.io/yaml" + + "github.com/fluxcd/source-controller/internal/helm" ) func TestLoader(t *testing.T) { @@ -51,7 +53,7 @@ func TestLoader(t *testing.T) { got, err := Loader(tmpDir, "fake") g.Expect(err).ToNot(HaveOccurred()) - g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, path: "fake", maxSize: DefaultMaxFileSize})) + g.Expect(got).To(Equal(SecureDirLoader{root: tmpDir, path: "fake", maxSize: helm.MaxChartFileSize})) }) t.Run("illegal path", func(t *testing.T) {