Use idtools.LookupGroup instead of parsing /etc/group file for docker.sock ownership #38126
Conversation
| if len(groups) > 0 { | ||
| return groups[0].Gid, nil | ||
| group, err := idtools.LookupGroup(name) | ||
| if err == nil { |
There was a problem hiding this comment.
Wondering if we should log the error as a warning if we both fail to find a group, and converting the name to an integer also fails.
(It's a bit unfortunate that "group not found", and "an error occurred looking up the group" both return an error here;
moby/pkg/idtools/idtools_unix.go
Line 183 in b3e9f7b
There was a problem hiding this comment.
Regarding the specific case of daemon/listeners/group_unix.go, errors get intercepted and are logged if appropriate in listeners_linux.go :
moby/daemon/listeners/listeners_linux.go
Line 40 in b3e9f7b
This is the only call site to group_unix.lookupGID().
As for the more general matter of error handling in idtools_unix.go, I agree that some minor improvement could be possible, but I think the behaviour of returning an error status (whatever its exact semantic) is the proper way to go. After all, the fact that a named group doesn't exist is not necessarily an error in all use case, but callers definitely need a way to help the user identify why a group was not found, notably when using external sources... I also think that it would be desirable to move to idtools_unix.go the handling of "the group name is actually a numerical id" cases, as it appears (well, at least to me) to always be a desirable fallback...
Now, come to that, I have noticed that there are quite a few places in Docker's source code where passwd and group files are crawled using various ad-hoc strategies. If more efforts is to be put in idtools_unix.go, then maybe it would be wise to consider using those functions everywhere instead of having custom code there and there...
By the way, I tried to retain the original behaviour as much as possible, in the hope that it would make the fix more acceptable for inclusion, essentially because it is my first contribution here. Still, I can certainly put a little more work in it if you are willing to give me some hints on what is to be done.
There was a problem hiding this comment.
Ok, after some more checks, it turns out that there are almost no case left of ad-hoc parsing. I only found oci_linux.go and internals_linux.go, and I don't think that supporting external user sources for the second would be desirable.
|
ping @estesp PTAL 🤗 |
GordonTheTurtle
added
the
dco/no
|
Please sign your commits following these rules: $ git clone -b "fix-1715" git@github.com:mjameswh/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354377400
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. |
GordonTheTurtle
removed
the
dco/no
|
Can you please squash your commits into one? |
….sock ownership Signed-off-by: James Watkins-Harvey <jwatkins@progi-media.com>
Codecov Report
@@ Coverage Diff @@
## master #38126 +/- ##
=========================================
Coverage ? 36.13%
=========================================
Files ? 610
Lines ? 45216
Branches ? 0
=========================================
Hits ? 16337
Misses ? 26641
Partials ? 2238 |
Done. |
thaJeztah
added
status/4-merge
impact/changelog
and removed
status/2-code-review
labels
|
Thanks! |
| @@ -6,25 +6,15 @@ import ( | |||
| "fmt" | |||
| "strconv" | |||
|
|
|||
| "github.com/opencontainers/runc/libcontainer/user" | |||
| "github.com/pkg/errors" | |||
There was a problem hiding this comment.
Ah.. this broke master in combination with #38316 fix upcoming
This includes dropping "snappy-socket-group.patch" (thanks to moby/moby#38126, which will shell out to "getent group docker" if the "/etc/group" lookup fails, which should DTRT for extrausers).
fixes #1715
fixes docker/for-linux#186
What I did
Fix
dockerdsetting/var/run/docker.sock's group torootrather thandockerwhen/etc/nsswitch.confspecify external sources for groups (for example LDAP, NIS or SSS).How I did it
Used the existing
idtools.LookupGroupfunction instead of reading the/etc/groupfile directly.idtools.LookupGroupfirst try reading thegroupfile; if it fails, it makes a system call to thegetentprogram. This is an uncommon strategy in general, but ensure thatdockerddoesn't need to be linked againstlibnss, which depend on plugins and can't therefore be statically linked.How to verify
I can't unfortunately provide a viable test case since that would require the test environment to be configured to use an external identity source (LDAP, ActiveDirectory, etc). However, existing tests should at least provide the garante that the existing behavior (that is, the most common case of using the docker group defined in the
/etc/group) is indeed maintained, and the fact thatidtools.LookupGroupis already being used in docker indicates that it most probably provides the expected new behavior.Changelog
Properly set group on docker.sock when using external group databases (LDAP, NIS, SSS...)