daemon/listeners: use chgrp binary for docker.sock #39605
Conversation
In case the daemon is compiled statically, it is not using glibc's uid/gid to name resolving functions (because of `osusergo` build tag), and so users who (1) use LDAP, NIS, or similar stuff and (2) not relying on systemd creating the docker.sock file will suffer from the issue described in docker/for-linux#186. Commit a2e3846 fixed that, but not for the static build case. So, instead of relying on golang os/user doing a lookup, let's just call chgrp binary and have it do a group name to gid conversion. PS Commit messages from c7c2cd4 and 169c013 helped me to figure out why the code is the way it is. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
We need to add this to the list of requirements for running Docker if we are shelling out. |
|
Please sign your commits following these rules: $ git clone -b "chgrp-docker-sock" git@github.com:kolyshkin/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842359065288
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
Co-Authored-By: Sebastiaan van Stijn <github@gone.nl> Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
a5b2354 to
b576b70
Compare
|
Second patch is optional and can be removed |
@justincormack where is this list? |
| return nil, errors.Wrapf(err, "can't create unix socket %s", addr) | ||
| } | if socketGroup != "" { | |
| out, err := exec.Command("chgrp", socketGroup, addr).CombinedOutput() |
There was a problem hiding this comment.
maybe we could do this only in the fallback if lookup with go implementation fails.
There was a problem hiding this comment.
@tonistiigi can you please elaborate why do you want to have both implementations? In case chgrp binary is not available, or as an optimization?
There was a problem hiding this comment.
I was asking to distinguish between the two cases because if you want an optimization (skip exec'ing chgrp if possible) I think in some cases lookupGID() might return some wrong result (say on a system with NIS or somesuch enabled), and in this case we'll end up with a wrong socket group, thereas chgrp should always be right.
If, on the other hand, you are concerned that chgrp binary might not be available, when we can amend the current code by falling back to lookupGID.
|
@kolyshkin could you have a look at @tonistiigi's comment? |
|
ping @kolyshkin |
|
Is this still on plan? |
There was a problem hiding this comment.
@kolyshkin this needs a rebase now, because I opened the second commit as a separate PR to make this diff smaller; if it helps, I did a rebase of your branch and pushed it here; https://github.com/moby/moby/compare/master...thaJeztah:carry_39605_chgrp_docker_sock?expand=1
| gid := os.Getgid() | ||
| l, err := sockets.NewUnixSocket(addr, gid) |
There was a problem hiding this comment.
nit: could simplify to
| gid := os.Getgid() | |
| l, err := sockets.NewUnixSocket(addr, gid) | |
| l, err := sockets.NewUnixSocket(addr, os.Getgid()) |
However, with the current code, it looks like we'll be creating the socket to have the root group, not the docker group, correct? (I expect the process to be running as root:root, and not docker to be the primary group), and then do a chown after, so we'll always be chown-ing twice
Note that more recent versions of the go-connections package have a NewUnixSocketWithOpts, which could possibly avoid that; https://github.com/docker/go-connections/blob/09f47925228a97335b2462178c79669712efe44e/sockets/unix_socket.go#L81
With that one, we could pass a custom functional argument that does the chown
There was a problem hiding this comment.
I think the file UID doesn't matter much. What I mean is, whether it's root or docker, it's fine either way.
What matters is GID.
this LGTM; can you please carry this? |
|
closing this one per #42879 (comment) (should probably be fixed by #38126) |
In case the daemon is compiled statically, it is not using glibc's
uid/gid to name resolving functions (because of
osusergobuild tag),and so users who
will suffer from the issue described in docker/for-linux#186.
Commit a2e3846 (PR #38126) fixed that, but not for the static build case.
So, instead of relying on golang os/user doing a lookup, let's just call
chgrp binary and have it do a group name to gid conversion.
PS Commit messages from c7c2cd4 and 169c013 helped me to
figure out why the code is the way it is.