Unique repo path and permissions #8517

alexmt · 2022-02-15T22:25:22Z

Signed-off-by: Alexander Matyushentsev AMatyushentsev@gmail.com

PR implements two changes:

Git and Helm repositories are cloned/fetched into random/non-predicable local directory
Repo server remove read permissions from Git repos when not using it for manifest generation

codecov · 2022-02-15T22:56:08Z

Codecov Report

Merging #8517 (74001dd) into master (ac47a42) will decrease coverage by 0.01%.
The diff coverage is 50.52%.

@@            Coverage Diff             @@
##           master    #8517      +/-   ##
==========================================
- Coverage   42.60%   42.59%   -0.02%     
==========================================
  Files         176      183       +7     
  Lines       22941    23102     +161     
==========================================
+ Hits         9774     9840      +66     
- Misses      11770    11853      +83     
- Partials     1397     1409      +12

Impacted Files	Coverage Δ
cmd/argocd/commands/app.go	`0.53% <0.00%> (ø)`
pkg/apis/application/v1alpha1/repository_types.go	`60.52% <0.00%> (ø)`
util/git/creds.go	`9.80% <4.34%> (-0.20%)`	⬇️
reposerver/askpass/server.go	`35.29% <35.29%> (ø)`
util/helm/client.go	`45.14% <55.55%> (-3.37%)`	⬇️
reposerver/repository/repository.go	`58.49% <55.93%> (-0.18%)`	⬇️
util/io/paths.go	`68.75% <68.75%> (ø)`
util/grpc/sanitizer.go	`85.00% <85.00%> (ø)`
reposerver/repository/lock.go	`100.00% <100.00%> (ø)`
util/git/client.go	`44.74% <100.00%> (ø)`
... and 7 more

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 764b7a6...74001dd. Read the comment docs.

crenshaw-dev

LGTM!

crenshaw-dev

Should we go ahead an use the UUIDv4 path generator for this line, too?

argo-cd/reposerver/repository/repository.go

Line 758 in d7fbc91

file, err := ioutil.TempFile("", "values-*.yaml")

alexmt · 2022-02-22T19:59:48Z

@crenshaw-dev request about file, err := ioutil.TempFile("", "values-*.yaml") already implemented in another PR

crenshaw-dev

One small comment.

crenshaw-dev · 2022-02-22T21:22:36Z

util/helm/client.go

+		return "", err
+	}
+	tempDir := path.Join(os.TempDir(), newUUID.String())
+	if err := os.Mkdir(tempDir, 0755); err != nil {


Do we need 755? Or just 700?

util/grpc/sanitizer.go

jessesuen · 2022-02-22T21:55:04Z

util/grpc/sanitizer.go

+	for k, v := range s.replacements {
+		val = strings.Replace(val, k, v, -1)
+	}
+	return val


Rather than maintain the map everytime we clone a repo, I feel we could have a regexp replace this

func NewSanitizer(root string) { return &sanitizer{ re: regexp.MustCompile(`(` + root + `/.*?)/`) // non-greedy until next slash after root } } func (s *sanitizer) Replace(val string) string { return s.re.ReplaceAllString(val, ".") }

To avoid missing relative paths, we could add a reasonably-unique prefix to the temp dir name and use a regex like this:

no-log-[0-9A-F]{8}-[0-9A-F]{4}-4[0-9A-F]{3}-[89AB][0-9A-F]{3}-[0-9A-F]{12}

(The end is just a regex for UUIDv4.)

Another benefit of a keyword-based replace would be that random paths outside the repo dir (like temporary values.yaml files' paths) could be sanitized as well.

empDir := path.Join(os.TempDir(), fmt.Sprintf("no-log-%s", newUUID.String()))

Both approaches also help us completely avoid an attack where someone uses the sanitizer to leak valid paths by forcing errors containing lots of UUIDs. I think that attack is super impractical, but if we can solve two problems at once, great.

New "sanitizer" is created and deleted every request. So this map will get 2~3 entries max.

Using the gprc interceptor and context was the least intrusive way to introduce sanitization. I really did not want to introduce a lot of copy-paste code. So it forced me to make sanitizer pretty generic . I think regex based sanitization will be useful. Implemented option suggested by @jessesuen because it is less coupled with how we generate unique paths .

jessesuen

I see that we sanitize the gRPC responses, but does this change sanitize the log outputs as well? Or do we not care?

…cation Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

…in active use Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

…tory; change repo directory permissions Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

util/helm/client.go

reposerver/repository/repository.go

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

jessesuen

This looks a lot simpler than before. Thanks!

Unique repo path and permissions (#8517) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

Unique repo path and permissions (argoproj#8517) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com> Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>

alexmt requested review from leoluz, crenshaw-dev, jannfis and jessesuen February 15, 2022 22:25

alexmt added this to the v2.3 milestone Feb 15, 2022

alexmt force-pushed the unique-repo-path-and-permissions branch 2 times, most recently from 958041d to 81f7df8 Compare February 16, 2022 17:30

crenshaw-dev approved these changes Feb 16, 2022

View reviewed changes

alexmt force-pushed the unique-repo-path-and-permissions branch from 81f7df8 to 7cecf78 Compare February 18, 2022 17:27

crenshaw-dev reviewed Feb 18, 2022

View reviewed changes

crenshaw-dev requested changes Feb 22, 2022

View reviewed changes

jessesuen reviewed Feb 22, 2022

View reviewed changes

util/grpc/sanitizer.go Outdated Show resolved Hide resolved

jessesuen reviewed Feb 22, 2022

View reviewed changes

alexmt added 4 commits February 22, 2022 18:15

refactor: git repos and helm charts should be downloaded to random lo…

f13024a

…cation Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

refactor: remove read/write permissions from cloned repo when it not …

a00ea3f

…in active use Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

support restoring URLs from file system; sanitize error messages

ca19d8f

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

apply reviewer notes: add/remove list permissions to repos root direc…

ab59ccd

…tory; change repo directory permissions Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt force-pushed the unique-repo-path-and-permissions branch from 7cecf78 to ec2e2c4 Compare February 23, 2022 02:30

more reviewer notes

278b27b

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt force-pushed the unique-repo-path-and-permissions branch from ec2e2c4 to 278b27b Compare February 23, 2022 18:43

leoluz reviewed Feb 24, 2022

View reviewed changes

util/helm/client.go Show resolved Hide resolved

leoluz reviewed Feb 24, 2022

View reviewed changes

reposerver/repository/repository.go Show resolved Hide resolved

alexmt force-pushed the unique-repo-path-and-permissions branch from 8687c23 to 4a1f628 Compare February 24, 2022 18:22

support sanitizing error messages using regex

74001dd

Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

alexmt force-pushed the unique-repo-path-and-permissions branch from 4a1f628 to 74001dd Compare February 24, 2022 18:31

jessesuen approved these changes Feb 24, 2022

View reviewed changes

alexmt merged commit c7da148 into argoproj:master Feb 24, 2022

alexmt deleted the unique-repo-path-and-permissions branch February 24, 2022 19:45

alexmt added a commit that referenced this pull request Feb 25, 2022

Unique repo path and permissions (#8517)

196dab9

Unique repo path and permissions (#8517) Signed-off-by: Alexander Matyushentsev <AMatyushentsev@gmail.com>

crenshaw-dev mentioned this pull request Mar 22, 2022

repo-server duplicates git packfiles, filling up disk #8845

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unique repo path and permissions #8517

Unique repo path and permissions #8517

alexmt commented Feb 15, 2022

codecov bot commented Feb 15, 2022 •

edited

crenshaw-dev left a comment

crenshaw-dev left a comment

alexmt commented Feb 22, 2022

crenshaw-dev left a comment

crenshaw-dev Feb 22, 2022

jessesuen Feb 22, 2022 •

edited

crenshaw-dev Feb 22, 2022 •

edited

crenshaw-dev Feb 22, 2022

crenshaw-dev Feb 22, 2022 •

edited

alexmt Feb 24, 2022

jessesuen left a comment

jessesuen left a comment

Unique repo path and permissions #8517

Unique repo path and permissions #8517

Conversation

alexmt commented Feb 15, 2022

codecov bot commented Feb 15, 2022 • edited

Codecov Report

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev left a comment

Choose a reason for hiding this comment

alexmt commented Feb 22, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev Feb 22, 2022

Choose a reason for hiding this comment

jessesuen Feb 22, 2022 • edited

Choose a reason for hiding this comment

crenshaw-dev Feb 22, 2022 • edited

Choose a reason for hiding this comment

crenshaw-dev Feb 22, 2022

Choose a reason for hiding this comment

crenshaw-dev Feb 22, 2022 • edited

Choose a reason for hiding this comment

alexmt Feb 24, 2022

Choose a reason for hiding this comment

jessesuen left a comment

Choose a reason for hiding this comment

jessesuen left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 15, 2022 •

edited

jessesuen Feb 22, 2022 •

edited

crenshaw-dev Feb 22, 2022 •

edited

crenshaw-dev Feb 22, 2022 •

edited