Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docker: Automatically fix permissions #3744

Merged
merged 1 commit into from
Mar 18, 2017
Merged

Conversation

kpcyrd
Copy link
Contributor

@kpcyrd kpcyrd commented Mar 3, 2017

This patch is delaying the point where permissions are dropped into the start_ipfs script. This way, instead of exiting on permission issues, we can fix them on our own inside the script, then drop privileges and continue doing ipfs specific stuff with the correct user.

I've removed the chmod 0777 step from the readme since it's not needed anymore.

License: MIT
Signed-off-by: kpcyrd git@rxv.cc

This patch is delaying the point where permissions are dropped into the `start_ipfs` script. This way, instead of exiting on permission issues, we can fix them on our own inside the script, then drop privileges and continue doing ipfs specific stuff with the correct user.

I've removed the `chmod 0777` step from the readme since it's not needed anymore.

License: MIT
Signed-off-by: kpcyrd <git@rxv.cc>
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

Interesting. I think i get it. will need review from @lgierth and @Kubuxu for sure

@Kubuxu
Copy link
Member

Kubuxu commented Mar 5, 2017

I am not sure this is the best solution. It will cause the outside directory to be owned by unknown users with random UDI (due to user namespaces).

I am not sure if there is better solution for that.


EDIT: scratch that, I thought Docker used user namespaces by default (LXC does).


EDIT2:

Then it creates possible security hole. We hardcode UID 1000 as our UID. This means that the volume will be chown'ed to that UID which will be also UID 1000 outside of container but this UID might be some user that shouldn't have access to the data.

@whyrusleeping
Copy link
Member

@Kubuxu hrm... shouldnt it fail if the user running the docker commands doesnt have access to the data?

If not then i can use docker to access any files on a system that i have docker rights on

@Kubuxu
Copy link
Member

Kubuxu commented Mar 5, 2017

No, they have to be passed as volume and as by default docker container runs as root, anything that is passed as volume is accessible from docker.

In this case the situation is: the docker container start as root with volume passed, the volume is owned by Alice, the container runs chown with UID 1000 (user ipfs UID in container). Outside of the container UID 1000 is assigned to Eve, now Eve has access to all the data.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 5, 2017

This is why user namespace was introduced into docker but AFAIK it is not default.

@Kubuxu
Copy link
Member

Kubuxu commented Mar 6, 2017

IMO we should have env variable that allows to pass GID when starting the container. This way user can chown the to be volume to given group and then we can use that group.

@lgierth can you look at the discussion here.

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Mar 6, 2017

Then it creates possible security hole. We hardcode UID 1000 as our UID. This means that the volume will be chown'ed to that UID which will be also UID 1000 outside of container but this UID might be some user that shouldn't have access to the data.

I'm not sure about the security hole. docker is an extremely powerful command and you have to be very careful who's allowed to use it (by default, nobody except root is allowed to do that). Everybody who is able to use it is effectively local root :)

docker run -it -v /:/media debian

gives you a root shell with the host filesystem mounted at /media, and you can chown everything you like.

It's also how mongodb and mysql do it:
https://github.com/docker-library/mongo/blob/4a81205a13fefc418355248f750551e4f7c62361/3.4/Dockerfile
https://github.com/docker-library/mysql/blob/eeb0c33dfcad3db46a0dfb24c352d2a1601c7667/5.7/docker-entrypoint.sh

I'm not using docker internal volumes, I'm using ipfs with bindmounts (-v /var/lib/ipfs:/data/ipfs) for persistent data, and if that folder doesn't exist, it's created and owned by uid=0, so I either have to create a user uid=1000 and run chown or I have to type a python oneliner that chowns the folder to a dangling uid.

If you use docker internal volumes, the permissions are fine, currently with bindmounts it looks like this:

$ docker run -d --name go-ipfs -v /does/not/exist:/data/ipfs go-ipfs
3bc38c6e79914560876638e08904c178ff2a501a56576d6ac953f1497c63f135
$ 
$ ls -la /does/not/exist/
total 8
drwxr-xr-x 2 root root 4096 Mar  6 16:53 .
drwxr-xr-x 3 root root 4096 Mar  6 16:53 ..
$ 

That's also the reason why the readme suggested chmod'ing the folder to 777, which is probably worse.

If you're worried that you have a legitimate uid=1000 who is then able to access the data (which is currently the case as well), you have to create a folder that is owned by root:root with permissions 0700, create an ipfs folder inside that folder and bind mount it into the docker container.

The container has permissions to access it, but a local uid=1000 can't access it's parent directory.

@whyrusleeping
Copy link
Member

@lgierth thoughts here?

@kpcyrd
Copy link
Contributor Author

kpcyrd commented Mar 14, 2017

Besides the discussion about security, is there anything else that needs attention?

@whyrusleeping whyrusleeping assigned ghost and Kubuxu Mar 16, 2017
@ghost
Copy link

ghost commented Mar 16, 2017

I think this LGTM, but I'll test-deploy it just to make sure. Thanks for getting us rid of the USER/VOLUME hack :)

@ghost
Copy link

ghost commented Mar 17, 2017

Works nicely! 👍

The infra scripts first broke on:

> docker run -i -v "$repo:/data/ipfs" --entrypoint /bin/sh "ipfs:$ref" -c 'ipfs init --bits 2048'`

but then I noticed I could now shorten that to:

docker run -i -v "$repo:/data/ipfs" "ipfs:$ref" init --bits 2048

Thanks!

@ghost
Copy link

ghost commented Mar 17, 2017

but then I noticed I could now shorten that to:

Ah nevermind :D

@ghost
Copy link

ghost commented Mar 17, 2017

Anyhow, I added -u 1000 to the init call and it works fine. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants