Merge pull request #663 from fluxcd/helm-safe-dir-loader

Author: hiddeco

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

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
 }
 

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

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{}

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}
 	}

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))
 

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())
 }