Fix possible path traversal attack when supporting Helm values.yaml

[simster7]

Fixes #2447 by addressing #2020 (comment).

Currently looking for other possible path traversal attacks.

[simster7]

As part of this change, all values stemming from Helm.Values will now have to be saved in a temp file within the application path.

[alexec]

clever

[alexec]

Looks good so far. Need to find out why e2e test failed

[codecov]

Codecov Report

Merging #2452 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2452   +/-   ##
======================================
  Coverage    39.2%   39.2%           
======================================
  Files         108     108           
  Lines       15862   15862           
======================================
  Hits         6218    6218           
  Misses       8864    8864           
  Partials      780     780

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87cb498...2622a64. Read the comment docs.

[alexec]

@simster7 you haven't removed the 'not ready for review' label, so I'll assume you don't want a review yet.

[simster7]

@alexec I don't have permissions to add or remove labels haha, but this should be ready for review!

[simster7]

@alexec Added the extra tests!

[alexec]

Conditional approval. You must fix the lint error!

[jannfis]

@alexec @simster7 Hm, I think this PR went a little over the goal. The behaviour of this change now forbids usage of values files outside the Chart folder, but should it not rather forbid usage of values files outside the repository root? It broke a few of our Helm applications that have a structure in repo like after we updated our integration environment to 1.3 today:

chart/Chart.yaml
env/env1/values.yaml
env/env2/values.yaml

Was this the intention?

[simster7]

Correct, the directory used as "root" for Helm apps is the directory the Chart.yaml is located:

argo-cd/reposerver/repository/repository.go

Lines 159 to 164 in 09808b5

	chartPath, closer, err := helmClient.ExtractChart(source.Chart, version)
	if err != nil {
	return err
	}
	defer util.Close(closer)
	return operation(chartPath, revision)

I think it would be up to discussion to determine if we should support files/directories outside of the Chart.yaml directory, yet still under the repo.

One one hand, supporting the repo as a whole may allow an attacker to use a values.yaml file that is unintended for the current deployment – but still present in the repo – for malicious purposes. On the other hand, as a Kustomize user (which has a similar strict directory enforcer) I am not familiar with Helm repo's standard patterns of organization. If the set up that you described is standard or even popular enough, then one could argue that we would be forced to support it, while losing the benefits of stricter security. (I can't think of another simple way to enforce against the use of unintended files other than directory structure).

An easy and flexible solution to this could be to introduce a flag to allow the user to disable the directory enforcer.

[adamjohnson01]

A lot of our deployments use shared values outside of the root but in the same repo. I think a flag to enable this would be good.

[jannfis]

Yes, a switch would be fine. Regardless of its setting, it should prevent jumping out the repo root, tho.

[simster7]

Will work on a PR.