search

sourcegraph / sourcegraph-public-snapshot

Code AI platform with Code Search & Cody
https://sourcegraph.com
Other
10.12k stars 1.29k forks source link

security: sourcegraph/symbols container runs as `root` #13237

Open slimsag opened 4 years ago

slimsag commented 4 years ago

All of our Docker images inherit from sourcegraph/alpine which sets up a non-root sourcegraph user. However, a review by myself has shown this is not the case for the sourcegraph/symbols container. To confirm this I used the following:

$ docker run --entrypoint=sh -it sourcegraph/frontend:3.19.1 -c 'whoami'
sourcegraph

$ docker run --entrypoint=sh -it sourcegraph/symbols:3.19.1 -c 'whoami'
root

This is a non-critical security issue.

slimsag commented 4 years ago

Figuring out how to migrate this in all deployments will be tough/complex/tricky.

keegancsmith commented 2 years ago

This is still the case, we just noticed this on https://github.com/sourcegraph/sourcegraph/pull/38830/f

@chrismwendt if I am not mistaken, there is no good reason this should be root user unlike all our other containers. Can we make the user sourcegraph?

chrismwendt commented 2 years ago

There's only 1 snag to making the user sourcegraph: ctags must be able to write to /tmp, or whatever TMPDIR is set to.

We've already run into that problem in our Helm chart where the user was set to sourcegraph and ctags failed to process symbols, so I set TMPDIR to a writable location: https://github.com/sourcegraph/deploy-sourcegraph-helm/pull/132

As long as we bear in mind that requirement and do a test run in each deployment environment, I think we can switch to the user sourcegraph without any other problems.

keegancsmith commented 2 years ago

In searcher I made TMPDIR a subdir of CACHE_DIR. That should reslove this more generally?

Edit: here is the PR I did that https://github.com/sourcegraph/sourcegraph/pull/36160

chrismwendt commented 2 years ago

Other than potential conflicts between diskcache and ctags using the same dir, I think that'll work.

AFAICT diskcache won't interfere with ctags since diskcache only creates/deletes .zip files:

https://github.com/sourcegraph/sourcegraph/blob/fa6179a985869a147fd84abc9630d25822d3fe36/internal/diskcache/cache.go#L358-L360

ctags appears to write a small number of files at the root then deletes them immediately after. Those files will get included in diskcache's size calculation, but they should be pretty small and not skew the calculation much. If that's a concern, we could tell the symbols service to assign diskcache a subdir like CACHE_DIR/symbols instead.

keegancsmith commented 2 years ago

FYI I don't directly set TMPDIR to CACHE_DIR, I make it a subdir that looks something like .tmp

Edit: coffee hasn't kicked in yet, I see what you mean. I agree we should make diskcache use a subdir.