Skip to content

Commit 57211bb

Browse files
authored
Merge pull request #239 from fluxcd/safe-rel-path
2 parents 1eb52ae + 29a051c commit 57211bb

File tree

4 files changed

+121
-73
lines changed

4 files changed

+121
-73
lines changed

controllers/helmchart_controller.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,11 @@ import (
2222
"io/ioutil"
2323
"net/url"
2424
"os"
25-
"path"
2625
"regexp"
2726
"strings"
2827
"time"
2928

29+
securejoin "github.com/cyphar/filepath-securejoin"
3030
"github.com/fluxcd/pkg/apis/meta"
3131
"github.com/go-logr/logr"
3232
helmchart "helm.sh/helm/v3/pkg/chart"
@@ -453,7 +453,10 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
453453
f.Close()
454454

455455
// Load the chart
456-
chartPath := path.Join(tmpDir, chart.Spec.Chart)
456+
chartPath, err := securejoin.SecureJoin(tmpDir, chart.Spec.Chart)
457+
if err != nil {
458+
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
459+
}
457460
chartFileInfo, err := os.Stat(chartPath)
458461
if err != nil {
459462
err = fmt.Errorf("chart location read error: %w", err)
@@ -512,7 +515,7 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
512515
if strings.HasPrefix(dep.Repository, "file://") {
513516
dwr = append(dwr, &helm.DependencyWithRepository{
514517
Dependency: dep,
515-
Repo: nil,
518+
Repository: nil,
516519
})
517520
continue
518521
}
@@ -574,18 +577,19 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context,
574577

575578
dwr = append(dwr, &helm.DependencyWithRepository{
576579
Dependency: dep,
577-
Repo: chartRepo,
580+
Repository: chartRepo,
578581
})
579582
}
580583

581584
// Construct dependencies for chart if any
582585
if len(dwr) > 0 {
583586
dm := &helm.DependencyManager{
587+
WorkingDir: tmpDir,
588+
ChartPath: chart.Spec.Chart,
584589
Chart: helmChart,
585-
ChartPath: chartPath,
586590
Dependencies: dwr,
587591
}
588-
err = dm.Build()
592+
err = dm.Build(ctx)
589593
if err != nil {
590594
return sourcev1.HelmChartNotReady(chart, sourcev1.StorageOperationFailedReason, err.Error()), err
591595
}

go.mod

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ replace github.com/fluxcd/source-controller/api => ./api
77
require (
88
github.com/Masterminds/semver/v3 v3.1.0
99
github.com/blang/semver/v4 v4.0.0
10+
github.com/cyphar/filepath-securejoin v0.2.2
1011
github.com/fluxcd/pkg/apis/meta v0.5.0
1112
github.com/fluxcd/pkg/gittestserver v0.1.0
1213
github.com/fluxcd/pkg/helmtestserver v0.1.0

internal/helm/dependency_manager.go

Lines changed: 77 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -19,123 +19,145 @@ package helm
1919
import (
2020
"context"
2121
"fmt"
22+
"net/url"
2223
"os"
23-
"path"
2424
"path/filepath"
2525
"strings"
2626

2727
"github.com/Masterminds/semver/v3"
28+
securejoin "github.com/cyphar/filepath-securejoin"
2829
"golang.org/x/sync/errgroup"
2930
helmchart "helm.sh/helm/v3/pkg/chart"
3031
"helm.sh/helm/v3/pkg/chart/loader"
3132
)
3233

33-
// DependencyWithRepository is a container for a dependency and its respective
34-
// repository
34+
// DependencyWithRepository is a container for a Helm chart dependency
35+
// and its respective repository.
3536
type DependencyWithRepository struct {
37+
// Dependency holds the reference to a chart.Chart dependency.
3638
Dependency *helmchart.Dependency
37-
Repo *ChartRepository
39+
// Repository is the ChartRepository the dependency should be
40+
// available at and can be downloaded from. If there is none,
41+
// a local ('file://') dependency is assumed.
42+
Repository *ChartRepository
3843
}
3944

40-
// DependencyManager manages dependencies for helm charts
45+
// DependencyManager manages dependencies for a Helm chart.
4146
type DependencyManager struct {
42-
Chart *helmchart.Chart
43-
ChartPath string
47+
// WorkingDir is the chroot path for dependency manager operations,
48+
// Dependencies that hold a local (relative) path reference are not
49+
// allowed to traverse outside this directory.
50+
WorkingDir string
51+
// ChartPath is the path of the Chart relative to the WorkingDir,
52+
// the combination of the WorkingDir and ChartPath is used to
53+
// determine the absolute path of a local dependency.
54+
ChartPath string
55+
// Chart holds the loaded chart.Chart from the ChartPath.
56+
Chart *helmchart.Chart
57+
// Dependencies contains a list of dependencies, and the respective
58+
// repository the dependency can be found at.
4459
Dependencies []*DependencyWithRepository
4560
}
4661

47-
// Build compiles and builds the chart dependencies
48-
func (dm *DependencyManager) Build() error {
49-
if dm.Dependencies == nil {
62+
// Build compiles and builds the dependencies of the Chart.
63+
func (dm *DependencyManager) Build(ctx context.Context) error {
64+
if len(dm.Dependencies) == 0 {
5065
return nil
5166
}
5267

53-
ctx := context.Background()
5468
errs, ctx := errgroup.WithContext(ctx)
55-
5669
for _, item := range dm.Dependencies {
57-
dep := item.Dependency
58-
chartRepo := item.Repo
5970
errs.Go(func() error {
60-
var (
61-
ch *helmchart.Chart
62-
err error
63-
)
64-
if strings.HasPrefix(dep.Repository, "file://") {
65-
ch, err = chartForLocalDependency(dep, dm.ChartPath)
66-
} else {
67-
ch, err = chartForRemoteDependency(dep, chartRepo)
71+
select {
72+
case <-ctx.Done():
73+
return ctx.Err()
74+
default:
6875
}
69-
if err != nil {
70-
return err
76+
77+
var err error
78+
switch item.Repository {
79+
case nil:
80+
err = dm.addLocalDependency(item)
81+
default:
82+
err = dm.addRemoteDependency(item)
7183
}
72-
dm.Chart.AddDependency(ch)
73-
return nil
84+
return err
7485
})
7586
}
7687

7788
return errs.Wait()
7889
}
7990

80-
func chartForLocalDependency(dep *helmchart.Dependency, cp string) (*helmchart.Chart, error) {
81-
origPath, err := filepath.Abs(path.Join(cp, strings.TrimPrefix(dep.Repository, "file://")))
91+
func (dm *DependencyManager) addLocalDependency(dpr *DependencyWithRepository) error {
92+
sLocalChartPath, err := dm.secureLocalChartPath(dpr)
8293
if err != nil {
83-
return nil, err
94+
return err
8495
}
8596

86-
if _, err := os.Stat(origPath); os.IsNotExist(err) {
87-
err := fmt.Errorf("chart path %s not found: %w", origPath, err)
88-
return nil, err
89-
} else if err != nil {
90-
return nil, err
97+
if _, err := os.Stat(sLocalChartPath); err != nil {
98+
if os.IsNotExist(err) {
99+
return fmt.Errorf("no chart found at '%s' (reference '%s') for dependency '%s'",
100+
strings.TrimPrefix(sLocalChartPath, dm.WorkingDir), dpr.Dependency.Repository, dpr.Dependency.Name)
101+
}
102+
return err
91103
}
92104

93-
ch, err := loader.Load(origPath)
105+
ch, err := loader.Load(sLocalChartPath)
94106
if err != nil {
95-
return nil, err
107+
return err
96108
}
97109

98-
constraint, err := semver.NewConstraint(dep.Version)
110+
constraint, err := semver.NewConstraint(dpr.Dependency.Version)
99111
if err != nil {
100-
err := fmt.Errorf("dependency %s has an invalid version/constraint format: %w", dep.Name, err)
101-
return nil, err
112+
err := fmt.Errorf("dependency '%s' has an invalid version/constraint format: %w", dpr.Dependency.Name, err)
113+
return err
102114
}
103115

104116
v, err := semver.NewVersion(ch.Metadata.Version)
105117
if err != nil {
106-
return nil, err
118+
return err
107119
}
108120

109121
if !constraint.Check(v) {
110-
err = fmt.Errorf("can't get a valid version for dependency %s", dep.Name)
111-
return nil, err
122+
err = fmt.Errorf("can't get a valid version for dependency '%s'", dpr.Dependency.Name)
123+
return err
112124
}
113125

114-
return ch, nil
126+
dm.Chart.AddDependency(ch)
127+
return nil
115128
}
116129

117-
func chartForRemoteDependency(dep *helmchart.Dependency, chartrepo *ChartRepository) (*helmchart.Chart, error) {
118-
if chartrepo == nil {
119-
err := fmt.Errorf("chartrepo should not be nil")
120-
return nil, err
130+
func (dm *DependencyManager) addRemoteDependency(dpr *DependencyWithRepository) error {
131+
if dpr.Repository == nil {
132+
return fmt.Errorf("no ChartRepository given for '%s' dependency", dpr.Dependency.Name)
121133
}
122134

123-
// Lookup the chart version in the chart repository index
124-
chartVer, err := chartrepo.Get(dep.Name, dep.Version)
135+
chartVer, err := dpr.Repository.Get(dpr.Dependency.Name, dpr.Dependency.Version)
125136
if err != nil {
126-
return nil, err
137+
return err
127138
}
128139

129-
// Download chart
130-
res, err := chartrepo.DownloadChart(chartVer)
140+
res, err := dpr.Repository.DownloadChart(chartVer)
131141
if err != nil {
132-
return nil, err
142+
return err
133143
}
134144

135145
ch, err := loader.LoadArchive(res)
136146
if err != nil {
137-
return nil, err
147+
return err
138148
}
139149

140-
return ch, nil
150+
dm.Chart.AddDependency(ch)
151+
return nil
152+
}
153+
154+
func (dm *DependencyManager) secureLocalChartPath(dep *DependencyWithRepository) (string, error) {
155+
localUrl, err := url.Parse(dep.Dependency.Repository)
156+
if err != nil {
157+
return "", fmt.Errorf("failed to parse alleged local chart reference: %w", err)
158+
}
159+
if localUrl.Scheme != "file" {
160+
return "", fmt.Errorf("'%s' is not a local chart reference", dep.Dependency.Repository)
161+
}
162+
return securejoin.SecureJoin(dm.WorkingDir, filepath.Join(dm.ChartPath, localUrl.Host, localUrl.Path))
141163
}

internal/helm/dependency_manager_test.go

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ limitations under the License.
1717
package helm
1818

1919
import (
20+
"context"
2021
"fmt"
2122
"io/ioutil"
2223
"strings"
@@ -43,7 +44,7 @@ func TestBuild_WithEmptyDependencies(t *testing.T) {
4344
dm := DependencyManager{
4445
Dependencies: nil,
4546
}
46-
if err := dm.Build(); err != nil {
47+
if err := dm.Build(context.TODO()); err != nil {
4748
t.Errorf("Build() should return nil")
4849
}
4950
}
@@ -72,6 +73,15 @@ func TestBuild_WithLocalChart(t *testing.T) {
7273
Repository: chartLocalRepository,
7374
},
7475
},
76+
{
77+
name: "allowed traversing path",
78+
dep: helmchart.Dependency{
79+
Name: chartName,
80+
Alias: "aliased",
81+
Version: chartVersion,
82+
Repository: "file://../../../testdata/charts/helmchartwithdeps/../helmchart",
83+
},
84+
},
7585
{
7686
name: "invalid path",
7787
dep: helmchart.Dependency{
@@ -80,7 +90,17 @@ func TestBuild_WithLocalChart(t *testing.T) {
8090
Repository: "file://../invalid",
8191
},
8292
wantErr: true,
83-
errMsg: "no such file or directory",
93+
errMsg: "no chart found at",
94+
},
95+
{
96+
name: "illegal traversing path",
97+
dep: helmchart.Dependency{
98+
Name: chartName,
99+
Version: chartVersion,
100+
Repository: "file://../../../../../controllers/testdata/charts/helmchart",
101+
},
102+
wantErr: true,
103+
errMsg: "no chart found at",
84104
},
85105
{
86106
name: "invalid version constraint format",
@@ -108,17 +128,18 @@ func TestBuild_WithLocalChart(t *testing.T) {
108128
t.Run(tt.name, func(t *testing.T) {
109129
c := chartFixture
110130
dm := DependencyManager{
111-
Chart: &c,
112-
ChartPath: "testdata/charts/helmchart",
131+
WorkingDir: "./",
132+
ChartPath: "testdata/charts/helmchart",
133+
Chart: &c,
113134
Dependencies: []*DependencyWithRepository{
114135
{
115136
Dependency: &tt.dep,
116-
Repo: nil,
137+
Repository: nil,
117138
},
118139
},
119140
}
120141

121-
err := dm.Build()
142+
err := dm.Build(context.TODO())
122143
deps := dm.Chart.Dependencies()
123144

124145
if (err != nil) && tt.wantErr {
@@ -130,7 +151,7 @@ func TestBuild_WithLocalChart(t *testing.T) {
130151
}
131152
return
132153
} else if err != nil {
133-
t.Errorf("Build() expected to return error")
154+
t.Errorf("Build() not expected to return an error: %s", err)
134155
return
135156
}
136157

@@ -166,12 +187,12 @@ func TestBuild_WithRemoteChart(t *testing.T) {
166187
Dependencies: []*DependencyWithRepository{
167188
{
168189
Dependency: &remoteDepFixture,
169-
Repo: cr,
190+
Repository: cr,
170191
},
171192
},
172193
}
173194

174-
if err := dm.Build(); err != nil {
195+
if err := dm.Build(context.TODO()); err != nil {
175196
t.Errorf("Build() expected to not return error: %s", err)
176197
}
177198

@@ -187,10 +208,10 @@ func TestBuild_WithRemoteChart(t *testing.T) {
187208
}
188209

189210
// When repo is not set
190-
dm.Dependencies[0].Repo = nil
191-
if err := dm.Build(); err == nil {
211+
dm.Dependencies[0].Repository = nil
212+
if err := dm.Build(context.TODO()); err == nil {
192213
t.Errorf("Build() expected to return error")
193-
} else if !strings.Contains(err.Error(), "chartrepo should not be nil") {
214+
} else if !strings.Contains(err.Error(), "is not a local chart reference") {
194215
t.Errorf("Build() expected to return different error, got: %s", err)
195216
}
196217
}

0 commit comments

Comments
 (0)