Make directory enforcer more lenient and add flag #2716

simster7 · 2019-11-13T21:54:19Z

Fixes: #2715 and updates change in #2452. ~~Adds a new argo-cd-cm flag (helm.directoryEnforcerLevel) to set Directory Enforcer level. Supported values are:~~

~~Strict: only allows access to files within the directory containing Chart.yaml~~
~~Repo (default): allows access to files within the entire repo~~

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
The title of the PR states what changed and the related issues number (used for the release note).
I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
Optional. My organization is added to the README.
I've signed the CLA and my build is green (troubleshooting builds).

util/security/path_traversal.go

util/security/path_traversal_test.go

util/settings/settings.go

simster7 · 2019-11-13T22:14:38Z

@jannfis @wecger @adamjohnson01 If possible, could you guys download and build this branch and see if the fix works as you would expect? The default behavior is to permit all files within a repo.

codecov · 2019-11-13T22:17:28Z

Codecov Report

Merging #2716 into master will increase coverage by 0.04%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #2716      +/-   ##
==========================================
+ Coverage   38.38%   38.42%   +0.04%     
==========================================
  Files         156      156              
  Lines       17292    17310      +18     
  Branches      203      203              
==========================================
+ Hits         6638     6652      +14     
- Misses       9831     9833       +2     
- Partials      823      825       +2

Impacted Files	Coverage Δ
util/settings/settings.go	`30.89% <100%> (+0.25%)`	⬆️
util/security/path_traversal.go	`81.48% <100%> (+10.89%)`	⬆️
util/helm/cmd.go	`40.56% <100%> (-2.42%)`	⬇️
reposerver/repository/repository.go	`59.88% <75%> (-0.08%)`	⬇️

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 257c276...80a5ac2. Read the comment docs.

util/security/path_traversal_test.go

simster7 · 2019-11-15T16:37:00Z

@jannfis @wecger @adamjohnson01 If possible, could you guys download and build this branch and see if the fix works as you would expect? The default behavior is to permit all files within a repo.

@jannfis @wecger @adamjohnson01 Hope you guys don't mind if I try you one more time :)

jannfis · 2019-11-15T17:07:13Z

Sorry for the late reply @simster7 and also thanks for the quick PR - unfortunately, We didn't find the time to build from that branch yet, as we're pretty busy with other topics. I'll see if I find time over the weekend to test it out and let you know any feedback.

simster7 · 2019-11-15T17:28:55Z

@alexec This could potentially need to be backported.

jannfis

Still not built & tested, but reviewed the change. I just have some minor comments, please check.

pkg/apis/application/v1alpha1/types.go

reposerver/repository/repository.go

util/settings/settings.go

alexec · 2019-11-15T18:42:59Z

IMHO for app where repo type is Git (as opposed to Helm) you should be able to read any file within the repo. No need for options.

simster7 · 2019-11-15T20:57:00Z

@alexec Sure, I can remove the option functionality. Since it's already implemented though, we could leave it in? Doesn't seem like it would hurt

alexec · 2019-11-15T22:38:38Z

I'd prefer to not have extra code to maintain TBH.

simster7 · 2019-11-16T01:52:02Z

@alexec Removed the enforcer level option

util/security/path_traversal.go

util/helm/cmd.go

util/settings/settings.go

util/helm/cmd_test.go

simster7 · 2019-11-26T21:04:00Z

reposerver/repository/repository.go

 		if appHelm.Values != "" {
-			file, err := ioutil.TempFile(appPath, "values-*.yaml")
+			file, err := ioutil.TempFile("", "values-*.yaml")


util/helm/cmd.go

reposerver/repository/repository.go

simster7 · 2019-11-26T23:03:23Z

@alexec Ready for another look

alexec

Looks good so far. Can we allow URLs please?

reposerver/repository/repository.go

reposerver/repository/repository_test.go

alexec · 2019-11-27T00:11:52Z

Please dismiss and re-request review when ready - so it appears in my notifications.

alexec

LGTM

wecger · 2019-11-27T14:30:49Z

built, deployed and tested this branch on our env. Using value files from outside the chart folder, but inside the repo worked fine for me :)

* Make directory enforcer more lenient and add flag * Fixes * Lint fixes * Lint fixes * Fixed test * Minor * Removed enforcer option * Move directory traversal check higher up * Go fmt * Allow URLs * Added test

simster7 added 2 commits November 13, 2019 13:25

Make directory enforcer more lenient and add flag

50943c6

Fixes

bd2d42e

golangcibot reviewed Nov 13, 2019

View reviewed changes

util/security/path_traversal.go Outdated Show resolved Hide resolved

util/security/path_traversal_test.go Outdated Show resolved Hide resolved

util/settings/settings.go Outdated Show resolved Hide resolved

util/settings/settings.go Outdated Show resolved Hide resolved

Lint fixes

9fd03cd

Lint fixes

237326f

golangcibot reviewed Nov 13, 2019

View reviewed changes

util/security/path_traversal_test.go Outdated Show resolved Hide resolved

simster7 marked this pull request as ready for review November 15, 2019 16:36

Fixed test

d7d245f

jannfis reviewed Nov 15, 2019

View reviewed changes

Minor

cec3316

Removed enforcer option

8486cc3

alexec reviewed Nov 18, 2019

View reviewed changes

util/security/path_traversal.go Show resolved Hide resolved

alexec self-assigned this Nov 21, 2019

alexec reviewed Nov 21, 2019

View reviewed changes

util/helm/cmd.go Outdated Show resolved Hide resolved

util/settings/settings.go Show resolved Hide resolved

alexec mentioned this pull request Nov 26, 2019

Regression: Can not supply value files as URLs #2762

Closed

3 tasks

simster7 added 2 commits November 26, 2019 12:12

Merge branch 'master' into dir-traversal-switch

2cf1da2

Move directory traversal check higher up

94e9e11

simster7 commented Nov 26, 2019

View reviewed changes

util/helm/cmd_test.go Outdated Show resolved Hide resolved

simster7 commented Nov 26, 2019

View reviewed changes

golangcibot reviewed Nov 26, 2019

View reviewed changes

util/helm/cmd.go Outdated Show resolved Hide resolved

reposerver/repository/repository.go Outdated Show resolved Hide resolved

Go fmt

f1c1d92

simster7 requested a review from alexec November 26, 2019 23:03

alexec requested changes Nov 26, 2019

View reviewed changes

reposerver/repository/repository.go Outdated Show resolved Hide resolved

reposerver/repository/repository_test.go Show resolved Hide resolved

simster7 added 2 commits November 26, 2019 15:46

Allow URLs

27f0899

Added test

80a5ac2

simster7 requested a review from alexec November 27, 2019 00:13

alexec approved these changes Nov 27, 2019

View reviewed changes

alexec added this to the v1.3 milestone Nov 27, 2019

alexec added S bug Something isn't working labels Nov 27, 2019

simster7 merged commit c8ae89f into argoproj:master Nov 27, 2019

moensch mentioned this pull request Dec 3, 2019

Error: index out of range when Chart.yaml is at root of git repo #2796

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make directory enforcer more lenient and add flag #2716

Make directory enforcer more lenient and add flag #2716

simster7 commented Nov 13, 2019 •

edited

simster7 commented Nov 13, 2019

codecov bot commented Nov 13, 2019 •

edited

simster7 commented Nov 15, 2019

jannfis commented Nov 15, 2019

simster7 commented Nov 15, 2019

jannfis left a comment

alexec commented Nov 15, 2019

simster7 commented Nov 15, 2019

alexec commented Nov 15, 2019

simster7 commented Nov 16, 2019

simster7 Nov 26, 2019

simster7 commented Nov 26, 2019

alexec left a comment

alexec commented Nov 27, 2019

alexec left a comment

wecger commented Nov 27, 2019

Make directory enforcer more lenient and add flag #2716

Make directory enforcer more lenient and add flag #2716

Conversation

simster7 commented Nov 13, 2019 • edited

simster7 commented Nov 13, 2019

codecov bot commented Nov 13, 2019 • edited

Codecov Report

simster7 commented Nov 15, 2019

jannfis commented Nov 15, 2019

simster7 commented Nov 15, 2019

jannfis left a comment

Choose a reason for hiding this comment

alexec commented Nov 15, 2019

simster7 commented Nov 15, 2019

alexec commented Nov 15, 2019

simster7 commented Nov 16, 2019

simster7 Nov 26, 2019

Choose a reason for hiding this comment

simster7 commented Nov 26, 2019

alexec left a comment

Choose a reason for hiding this comment

alexec commented Nov 27, 2019

alexec left a comment

Choose a reason for hiding this comment

wecger commented Nov 27, 2019

simster7 commented Nov 13, 2019 •

edited

codecov bot commented Nov 13, 2019 •

edited