Skip to content

Commit

Permalink
helm: attach loader to helm.MaxChartFileSize
Browse files Browse the repository at this point in the history 
Signed-off-by: Hidde Beydals <[email protected]>
  • Loading branch information
@hiddeco
hiddeco committed Apr 11, 2022
1 parent e85ea78 commit 9a17fd5
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 32 deletions.
21 changes: 9 additions & 12 deletions internal/helm/chart/secureloader/directory.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}
)

Expand All @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
}

Expand Down
38 changes: 20 additions & 18 deletions internal/helm/chart/secureloader/directory_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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))
Expand All @@ -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))
Expand All @@ -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())
Expand All @@ -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))
Expand Down Expand Up @@ -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())
})

Expand All @@ -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))
})

Expand All @@ -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"))
Expand All @@ -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))
})

Expand All @@ -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"))
Expand All @@ -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))
Expand All @@ -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))
Expand All @@ -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())
Expand Down
4 changes: 3 additions & 1 deletion internal/helm/chart/secureloader/loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
4 changes: 3 additions & 1 deletion internal/helm/chart/secureloader/loader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down

0 comments on commit 9a17fd5

Please sign in to comment.