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

Permission denied on /tmp/git #325

Closed
akloeckner opened this issue Jan 17, 2021 · 12 comments · Fixed by #329
Closed

Permission denied on /tmp/git #325

akloeckner opened this issue Jan 17, 2021 · 12 comments · Fixed by #329

Comments

@akloeckner
Copy link

akloeckner commented Jan 17, 2021

When using git-sync in the recommended way, by bind-mounting the destination folder onto /tmp/git, I get the following error:

E0116 23:29:01.393979      11 main.go:455]  "msg"="too many failures, aborting" "error"="Run(git clone --no-checkout -b master ssh://[...].git /tmp/git): exit status 1: { stdout: \"\", stderr: \"Cloning into '/tmp/git'...\\n/tmp/git/.git: Permission denied\\n\" }"  "failCount"=0

This is the case for an empty bind-mount and also if I use a volume.

I have read #245 (comment) and the workaround there also works for me: Mount the volume onto /tmp directly.

So, I wondered why this is...

Loading the fresh image with only a sleep command lets me inspect the initial contents of tmp:

$ ls -la
total 8
drwxrwxrwt 1 root root 4096 Jan  5 17:18 .
drwxr-xr-x 1 root root 4096 Jan 16 23:48 ..

As we can see, /tmp is world-writeable, so the git-sync user can create his own git directory here and work with it. This is why mounting a new volume onto /tmp works as expected:

$ ls -la /tmp
total 16
drwxrwxrwt 4 root     root  4096 Jan 16 23:45 .
drwxr-xr-x 1 root     root  4096 Jan 16 23:45 ..
drwx------ 2 git-sync 65533 4096 Jan 16 23:45 .ssh
drwxr-xr-x 4 git-sync 65533 4096 Jan 16 23:45 git

CAVEAT: The checkedout folder structure will be VOLUME/git/SYMLINK in this case. If I want to get rid of the git folder inbetween, e.g. for mounting the volume into an nginxcontainer, I end up with a SYMLINK inside /tmp. Since /tmp has the t flag set, I can not follow this link as root user, such as when I'm in a standard nginx container...

If I mount an empty volume onto /tmp/git, it will not be writeable by git-sync:

$ ls -la /tmp
total 12
drwxrwxrwt 1 root root 4096 Jan 16 23:53 .
drwxr-xr-x 1 root root 4096 Jan 16 23:53 ..
drwxr-xr-x 2 root root 4096 Jan 16 23:52 git

Apparently, the /tmp/git folder is created as root by docker itself, when mounting the volume.

Suggested improvement:

This is why I would suggest to create the git folder from the docs already in the image and make it be owned by git-sync. This would also address the caveat mentioned above, because the new git folder would ideally not have the t flag enabled...

Note: This will not fix the cause you lined out in #245. If we bind-mount a non-existing folder from the host system (even onto /tmp as in the workaround), the permission will still be denied by the host system. But as you said, there is nothing we can do about that.

What do you think?

@thockin
Copy link
Member

thockin commented Jan 17, 2021 via email

@akloeckner
Copy link
Author

Directly in docker

@thockin
Copy link
Member

thockin commented Jan 21, 2021

Can you show me a docker commandline to illustrate your point? As long as the "outside" dir exists before you bind mount it, the perms will not be changed.

I don't use docker (directly) for anything productionable, so I am not sure what exactly you are doing. I know docker's mkdir-as-root bindmount behavior has bitten many people.

@akloeckner
Copy link
Author

Sure, here's the three cases from above as pure docker oneliners...

Note that I use empty volumes instead of bind mounts, because I am trying to avoid fiddling with the host file system as much as possible. Not sure what will happen with bind mounts. (Probably, the cases will be more diverse, since we have to take into account the host permissions on the bind mount source, as you pointed out.) Nevertheless, git-sync should work with volumes, right?

Pure image: git-sync user can write to /tmp and thus also /tmp/git:

# docker run --entrypoint="" k8s.gcr.io/git-sync/git-sync:v3.2.2 ls -la /tmp
total 8
drwxrwxrwt 1 root root 4096 Jan  5 17:18 .
drwxr-xr-x 1 root root 4096 Jan 21 16:24 ..

Mount volume on /tmp: git-sync user can write to /tmp and thus also /tmp/git:

# docker run --entrypoint="" -v emptyvol1:/tmp k8s.gcr.io/git-sync/git-sync:v3.2.2 ls -la /tmp
total 8
drwxrwxrwt 2 root root 4096 Jan  5 17:18 .
drwxr-xr-x 1 root root 4096 Jan 21 16:25 ..

Mount volume on /tmp/git: git-sync user canNOT write to /tmp/git:

# docker run --entrypoint="" -v emptyvol2:/tmp/git k8s.gcr.io/git-sync/git-sync:v3.2.2 ls -la /tmp
total 12
drwxrwxrwt 1 root root 4096 Jan 21 16:26 .
drwxr-xr-x 1 root root 4096 Jan 21 16:26 ..
drwxr-xr-x 2 root root 4096 Jan 21 16:26 git

@thockin
Copy link
Member

thockin commented Jan 21, 2021 via email

@cpuguy83
Copy link

It's also important to remember that Docker volumes, when mounted/filled for the first time, will take the ownership of the directory being mounted to.

So, if /tmp/git exists in the container already, if it is owned by git-sync the volume will also be owned by git-sync

@thockin
Copy link
Member

thockin commented Jan 21, 2021

Thanks. I think that's OP's point (and I did not know that was a feature), but it is in conflict with a different goal (not defaulting the root flag). But I think I have a compromise answer. Doing some tests and will post back.

@thockin
Copy link
Member

thockin commented Jan 21, 2021

The problem with mounting a volume on /tmp is that it makes that volume world-writable in the host, which is probably not a great idea.

@akloeckner
Copy link
Author

akloeckner commented Jan 21, 2021

Thanks for looking into this!

I am planning to make --root be required and not defaulted

As a side note on that: actually, I wouldn't see a reason to be able to set this directory at all. A user obviously will have to use some mount anyways. That mount will always have to be related to the root setting. So, why not force the user to use one exact root directory of your choice? It would make configuration somewhat simpler. (But I'm a total newbie on Docker, so I might miss something blatantly obvious, here...)

The problem with mounting a volume on /tmp is that it makes that volume world-writable

And it sets the “sticky bit“ on the volume, such that the root user will not be allowed to follow the symlink anymore! See https://stackoverflow.com/a/26497532

@thockin
Copy link
Member

thockin commented Jan 21, 2021

I don't exclusively run this in a docker image. E.g. for testing I will run it directly, and it is super valuable to be able to run it against temporary directories.

What I am exploring now is how to make the binary non-opinionated, but the container image define a default root dir. I think I know how to do that, but am exploring all the combinations of knobs and documenting them a bit.

Ideally, when you run the container image you mount your volume at a specific place and are done. You should not need to mess with the flag.

I'm not super enamored with /tmp/git for this, but maybe that's the v3 compatible answer and v4 (which has other breaking changes) can reconsider.

@thockin
Copy link
Member

thockin commented Jan 21, 2021

Take a look at #329 - see if it does what you need.

@akloeckner
Copy link
Author

Sounds like it should. I am not sure how to build and test this. But it looks like you know exactly what you're doing, so I'm confident it's the right thing. 👍 I also like the detailed documentation!

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 a pull request may close this issue.

3 participants