8000 [carry 39605] daemon/listeners: use chgrp binary for docker.sock by thaJeztah · Pull Request #42879 · moby/moby · GitHub
[go: up one dir, main page]
Skip to content

[carry 39605] daemon/listeners: use chgrp binary for docker.sock #42879

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[carry 39605] daemon/listeners: use chgrp binary for docker.sock #42879

wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Member

carries #39605
closes #39605

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 relyingon systemd creating the docker.sock file

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.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added this to the 21.xx milestone Sep 24, 2021
@thaJeztah
Copy link
Member Author

@kolyshkin @tonistiigi

may need some changes in packaging, per #39605 (comment)

We need to add this to the list of requirements for running Docker if we are shelling out.

@kolyshkin @thaJeztah
daemon/listeners: use chgrp binary for docker.sock
09de87e 
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>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the carry_39605_chgrp_docker_sock branch from 2891a46 to 09de87e Compare September 24, 2021 13:15
@tianon
Copy link
Member
tianon commented Sep 24, 2021

What if we shell out conditionally instead? It makes the code a little more complicated, but avoids requiring chgrp explicitly (I bet users with things like LDAP will already have it).

We could also consider shelling out to getent group instead, which would give a similar end result (although imo still worth considering making conditional).

@thaJeztah
Copy link
Member Author

We could also consider shelling out to getent group instead, which would give a similar end result (although imo still worth considering making conditional).

Do you know how to use getent group to get just the group ID? Trying to read GETENT(1), which describes;

group  When no key is provided, use setgrent(3), getgrent(3),
       and endgrent(3) to enumerate the group database.  When
       one or more key arguments are provided, pass each
       numeric key to getgrgid(3) and each nonnumeric key to
       getgrnam(3) and display the result.

@thaJeztah
Copy link
Member Author

Ah, never mind, I guess I was reading key wrong; and key (in this case) means "group name(s) or id(s)" to look up, and not "field" in the result;

getent group systemd-journal docker
systemd-journal:x:101:
docker:x:999:sebastiaan

@thaJeztah
Copy link
Member Author

I'm actually wondering if this PR is still needed; #38126 changed the lookupGID() function to use idtools.LookupGroup(name), which uses runc's group lookup function (parsing /etc/groups), and falls back to using getent group;

moby/pkg/idtools/idtools_unix.go

Lines 153 to 161 in 686be57

func LookupGroup(name string) (user.Group, error) {
// first try a local system files lookup using existing capabilities
group, err := user.LookupGroup(name)
if err == nil {
return group, nil
}
// local files lookup failed; attempt to call `getent` to query configured group dbs
return getentGroup(name)
}

moby/pkg/idtools/idtools_unix.go

Lines 175 to 188 in 686be57

func getentGroup(name string) (user.Group, error) {
reader, err := callGetent("group", name)
if err != nil {
return user.Group{}, err
}
groups, err := user.ParseGroup(reader)
if err != nil {
return user.Group{}, err
}
if len(groups) == 0 {
return user.Group{}, fmt.Errorf("getent failed to find groups entry for %q", name)
}
return groups[0], nil
}

@thaJeztah
Copy link
Member Author

Perhaps we no longer need this patch, or am I missing a reason we'd still need it? @kolyshkin @tianon

@tianon
Copy link
Member
tianon commented Sep 27, 2021

Aha, yeah, IMO this is a non-issue now 😀

@thaJeztah
Copy link
Member Author

👍 let's close this one

@thaJeztah thaJeztah mentioned this pull request Sep 27, 2021
Closed
@thaJeztah thaJeztah closed this Sep 27, 2021
@thaJeztah thaJeztah removed this from the 21.xx milestone Sep 27, 2021
@thaJeztah thaJeztah deleted the carry_39605_chgrp_docker_sock branch September 27, 2021 17:29
@kolyshkin
Copy link
Contributor

I am very sorry that I somehow managed to totally miss the fact that idtools do fallback to calling getent binary. 😞

@thaJeztah
Copy link
Member Author

I am very sorry that I somehow managed to totally miss the fact that idtools do fallback to calling getent binary. 😞

No worries, I forgot about the other change as well; I came on this carry, because I was cleaning up some of my branches, and then found #39605 (comment), and thought: "oh! I never pushed that branch" 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/daemon Core Engine status/2-code-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thaJeztah @tianon @kolyshkin
0