Skip to content

Use idtools.LookupGroup instead of parsing /etc/group file for docker.sock ownership #38126

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 12, 2018
Merged

Use idtools.LookupGroup instead of parsing /etc/group file for docker.sock ownership #38126

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Use idtools.LookupGroup instead of parsing /etc/group file for docker… 
….sock ownership

Signed-off-by: James Watkins-Harvey <jwatkins@progi-media.com>
  • Loading branch information
@mjameswh
mjameswh committed Nov 29, 2018
commit a2e384682b8d2b2997ba8e9dd9762baab7ebc2f0
18 changes: 4 additions & 14 deletions daemon/listeners/group_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,25 +6,15 @@ import (
"fmt"
"strconv"

"github.com/opencontainers/runc/libcontainer/user"
"github.com/pkg/errors"
Copy link
Member

Choose a reason for hiding this comment

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

Ah.. this broke master in combination with #38316 fix upcoming

Copy link
Member

Choose a reason for hiding this comment

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

Fix #38360 👍

"github.com/docker/docker/pkg/idtools"
)

const defaultSocketGroup = "docker"

func lookupGID(name string) (int, error) {
groupFile, err := user.GetGroupPath()
if err != nil {
return -1, errors.Wrap(err, "error looking up groups")
}
groups, err := user.ParseGroupFileFilter(groupFile, func(g user.Group) bool {
return g.Name == name || strconv.Itoa(g.Gid) == name
})
if err != nil {
return -1, errors.Wrapf(err, "error parsing groups for %s", name)
}
if len(groups) > 0 {
return groups[0].Gid, nil
group, err := idtools.LookupGroup(name)
if err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

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

return user.Group{}, fmt.Errorf("getent failed to find groups entry for %q", strings.Split(args, " ")[1])
🤔 ideally we should have a "IsNotFound"-like error, or perhaps no error at all for that)

Copy link
Contributor Author

@mjameswh mjameswh Nov 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

logrus.Warnf("could not change group %s to %s: %v", addr, defaultSocketGroup, err)

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return group.Gid, nil
}
gid, err := strconv.Atoi(name)
if err == nil {
Expand Down