Skip to content

docker: run in front-envoy container as root#12112

Closed
akonradi wants to merge 1 commit intoenvoyproxy:masterfrom
akonradi:docker-front-proxy
Closed

docker: run in front-envoy container as root#12112
akonradi wants to merge 1 commit intoenvoyproxy:masterfrom
akonradi:docker-front-proxy

Conversation

@akonradi
Copy link
Contributor

If run as the 'envoy' user, the proxy can't read the config yaml file and fails to start up. The not-starting-up appears to be a side-effect of #11323.

Also change the docker-compose version to 3.3 because none of the newer features are being used.

Signed-off-by: Alex Konradi akonradi@google.com

Commit Message: Run proxy as root in front-envoy container
Additional Description:
If run as the 'envoy' user, the proxy can't read the config yaml file and fails to start up.

Risk Level: low
Testing: ran docker-compose up and saw proxy and services were running
Docs Changes: none
Release Notes: none

@yosrym93

If run as the 'envoy' user, the proxy can't read the config yaml file
and fails to start up.

Also change the docker-compose version to 3.3 because none of the later
features are being used.

Signed-off-by: Alex Konradi <akonradi@google.com>
@dio
Copy link
Member

dio commented Jul 16, 2020

cc. @phlax

@phlax
Copy link
Member

phlax commented Jul 16, 2020

i think we dont want to run as root - rather ensure the envoy user can read the config file

i didnt test all the examples, but i can take a look and PR to fix this in this case. ill check the other examples at the same time to see if there is any other problems.

@akonradi
Copy link
Contributor Author

This is definitely a permissions issue at heart. I was able to work around it by giving world read permissions to front-proxy.yaml, but that isn't something that can be done directly with git.

@lizan
Copy link
Member

lizan commented Jul 18, 2020

@akonradi git does have permission for the repo and the file is setting right at 644.

git ls-files -s examples/front-proxy/front-envoy.yaml
100644 c266022e68067d2de62552f3d3bf784ebda33552 0       examples/front-proxy/front-envoy.yaml

It is more likely your environment specific issue, (perhaps umask being 027?) so that the git checked out file isn't world readable.

- "8443:8443"
- "8001:8001"
environment:
- ENVOY_UID=0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- ENVOY_UID=0
- ENVOY_UID

we can do this so it propagates from your local env, while the default is still non-root?

@phlax
Copy link
Member

phlax commented Jul 18, 2020

@lizan if the problem is with the environment rather than the example, then changing the example here won't help with other examples or running (with this problem) more generally

im wondering if we should add something to the running with docker docs along the lines of


The envoy user needs to have permission to access any required configuration files mounted into the container.

If you are running in an environment with a strict umask setting, you may need to provide envoy with access either by setting the uid or gid of the file, or by making the configuration file world readable.


@phlax
Copy link
Member

phlax commented Jul 19, 2020

also, if you want to give the process inside the container permission to file/s without changing permissions on the file/s, the better pattern is to make the envoy user have the host users uid, not roots uid

ie - good pattern

docker run -e ENVOY_UID=`id -u` ...other docker args... envoyproxy/envoy

this also works with docker-compose without the need for adding the empty env var in the compose file

this would also work with gid if the file has group permissions

@phlax
Copy link
Member

phlax commented Jul 21, 2020

I added a PR with some docs on configuration file permissions

@akonradi
Copy link
Contributor Author

Thanks, that's a good clarification for the docs. I still want to try to make the example work as-is, with just docker-compose. I don't know that running as root in the container is the right solution, but I think that targeting "git clone [...]; cd [...]; docker-compose [...]" as the steps required to play around is the goal we should be aiming for.

@phlax
Copy link
Member

phlax commented Jul 21, 2020

@akonradi its a pretty rare situation to have strict umask like that in a dev env (imhe)

the above fix with ENVOY_UID in the docker-compose file would still necessitate export ENVOY_UID=... of either host user or 0. I dont really see a way of it working out of the box without something setting the uid

personally i think using the -e flag is the most explicit - and is portable between docker/compose

...but in the vast majority of cases would not be necessary

@akonradi
Copy link
Contributor Author

I can't speak to the rarity of having world read permissions on dev files. I don't understand why running Envoy as root in the container for the purposes of the example is bad. The rationale in #11311 is that it's a better security practice to run as a non-root user, and I think that's true and that the ability to set the UID is a useful knob to have. That being said, the binary is running in a container and Docker prevents it binding new ports, or writing files to the host since the only attached volume is the config file.

@phlax
Copy link
Member

phlax commented Jul 21, 2020

I can't speak to the rarity of having world read permissions on dev files.

yep, i started thinking of some counter-examples. i think the point remains that most people would not face this issue

don't understand why running Envoy as root in the container for the purposes of the example is bad.

i think because its an example - and the example should exemplify good practice - running as root in containers is not good practice, i think.

this point has been underlined by the suggestions to restore the old behaviour - ie root in container - over using the host user perms (who has docker perms anyway so the point is a little moot) or setting file permissions correctly

the binary is running in a container and Docker prevents it binding new ports,

the main risk is that an admin mounts something that exposes the host security - eg /var/ or /var/run//docker.sock

zuercher pushed a commit that referenced this pull request Jul 21, 2020
Add further info on file permissions to Docker docs. See #12112

Risk Level: very low
Testing: n/a
Docs Changes: yes
Release Notes: n/a
Signed-off-by: Ryan Northey <ryan@synca.io>
@yosrym93
Copy link
Contributor

yosrym93 commented Jul 21, 2020

We could write a bash script to set ENVOY_UID variable to $(id -u) then build and run using docker compose. This way we can just add

environment:
      - ENVOY_UID

to docker-compose.yaml as @lizan suggested.

The script could also accept an option to add/remove the --pull option from docker build, in case they want to run the sandbox on an image they built from source (although they still have to manually modify the dockerfile then).

p.s I am a newbie, so I could be wrong.

@yosrym93
Copy link
Contributor

Another solution that comes to mind (again not sure if is good enough), we could just copy the config file and give ownership of the new file to the envoy user instead of mounting it.

In docker-compose.yaml -> remove the mounted volume.
In Docerkfile-frontenvoy -> add this line:
COPY --chown=envoy ./front-envoy.yaml /etc/front-envoy.yaml
before the CMD.

I tested this and it runs perfectly. Not sure if copying is a "best practice" here tho.

@phlax
Copy link
Member

phlax commented Jul 22, 2020

Another solution that comes to mind (again not sure if is good enough), we could just copy the config file and give ownership of the new file to the envoy user instead of mounting it.

I think this is a good idea if we want the examples to work out of the box without needing to add any env flags in bash or docker etc

Looking through the examples it seems like most of them would need updating in this way.

Im also noticing that in the existing recipes there are already some files that are copied in eg here:

https://github.com/envoyproxy/envoy/blob/master/examples/cors/backend/Dockerfile-service#L6

These may need their permissions changing too

My only niggling concern - which i dont think would break the examples as documented - is that if a user did set the ENVOY_UID to something other than the uid in the image (or root) would the file still be readable.

One possible solution is to make the file world readable within the container - ie chmod go+r /path/to/config instead of giving envoy ownership

@phlax
Copy link
Member

phlax commented Jul 22, 2020

i created a test repo to run through the commands in the docs against the recipes in the examples.

so far i have managed to reproduce @akonradi 's error - as can be seen here:

https://travis-ci.org/github/phlax/envoy-examples/builds/710765435#L838

i'll create a PR with the variant suggested to @yosrym93 's fix , and add the other examples to the test runner

@yosrym93
Copy link
Contributor

@phlax I tried making the file readable in the container by running chmod .. in the dockerfile but it could not find the file. Does mounting happen after the docker build?

@akonradi
Copy link
Contributor Author

akonradi commented Jul 22, 2020

@yosrym93 the file gets mounted as a volume when the container runs. We could bake it into the image like you suggested (the COPY directive) but then it's harder to test new changes since you'd need to rebuild the entire image every time you want to change the file.

@yosrym93
Copy link
Contributor

@akonradi These config files are meant to be static, and (I think) envoy only reads the config once when it runs. So if the user changes the config they have to re-run the container anyway. Rebuilding won't take anytime because everything is cached and this is the final layer, it'll just re-copy the file.

@akonradi
Copy link
Contributor Author

Yep, there's no watching of the config or anything after startup. That seems like a workable solution for this example if we're okay with having people run docker-compose up --build -d instead of docker-compose up -d.

@phlax
Copy link
Member

phlax commented Jul 22, 2020

@akonradi @yosrym93 i am going through and testing the examples with the fix suggested.

there are quite a few other failures - some eg trying to log to /var/log/access.log - but also other problems - eg:

front-envoy_1  | [2020-07-22 17:53:06.792][9][critical][main] [source/server/server.cc:103] error initializing configuration '/etc/front-envoy.yaml': type envoy.api.v2.route.CorsPolicy Using deprecated option 'envoy.api.v2.route.CorsPolicy.allow_origin' from file route_components.proto. This configuration will be removed from Envoy soon. Please see https://www.envoyproxy.io/docs/envoy/latest/version_history/version_history for details. If continued use of this field is absolutely necessary, see https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime#using-runtime-overrides-for-deprecated-features for how to apply a temporary and highly discouraged override.

its going to take some time to go through and test/fix each one, but i should have a PR with all examples working by tomorrow

for info the latest passing build is here

https://travis-ci.org/github/phlax/envoy-examples/builds/710840148

and failing here:

https://travis-ci.org/github/phlax/envoy-examples/builds/710826221

re rebuilding - i would agree - its trivial to rebuild.

@stale
Copy link

stale bot commented Jul 29, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 29, 2020
KBaichoo pushed a commit to KBaichoo/envoy that referenced this pull request Jul 30, 2020
Add further info on file permissions to Docker docs. See envoyproxy#12112

Risk Level: very low
Testing: n/a
Docs Changes: yes
Release Notes: n/a
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Kevin Baichoo <kbaichoo@google.com>
scheler pushed a commit to scheler/envoy that referenced this pull request Aug 4, 2020
Add further info on file permissions to Docker docs. See envoyproxy#12112

Risk Level: very low
Testing: n/a
Docs Changes: yes
Release Notes: n/a
Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: scheler <santosh.cheler@appdynamics.com>
@stale
Copy link

stale bot commented Aug 9, 2020

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot closed this Aug 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale stalebot believes this issue/PR has not been touched recently

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants