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

daemon/listeners: use chgrp binary for docker.sock #39605

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

Closed
wants to merge 2 commits into from
Closed

daemon/listeners: use chgrp binary for docker.sock #39605

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a2a162
daemon/listeners: use chgrp binary for docker.sock
kolyshkin Jul 24, 2019
b576b70
daemon/listeners: use pkg/errors 8000
kolyshkin Jul 25, 2019
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
24 changes: 0 additions & 24 deletions daemon/listeners/group_unix.go
View file
Open in desktop

This file was deleted.

41 changes: 26 additions & 15 deletions daemon/listeners/listeners_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,22 @@
package listeners // import "github.com/docker/docker/daemon/listeners"

import (
"bytes"
"crypto/tls"
"fmt"
"net"
"os"
"os/exec"
"strconv"

"github.com/coreos/go-systemd/activation"
"github.com/docker/docker/pkg/homedir"
"github.com/docker/go-connections/sockets"
"github.com/pkg/errors"
"github.com/sirupsen/logrus"
)

const defaultSocketGroup = "docker"

// Init creates new listeners for the server.
// TODO: Clean up the fact that socketGroup and tlsConfig aren't always used.
func Init(proto, addr, socketGroup string, tlsConfig *tls.Config) ([]net.Listener, error) {
Expand All @@ -32,27 +36,34 @@ func Init(proto, addr, socketGroup string, tlsConfig *tls.Config) ([]net.Listene
}
ls = append(ls, l)
case "unix":
gid, err := lookupGID(socketGroup)
gid := os.Getgid()
l, err := sockets.NewUnixSocket(addr, gid)
Comment on lines +39 to +40
Copy link
Member

Choose a reason for hiding this comment

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

nit: could simplify to

Suggested change
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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if err != nil {
if socketGroup != "" {
return nil, errors.Wrapf(err, "can't create unix socket %s", addr)
}
if socketGroup != "" {
out, err := exec.Command("chgrp", socketGroup, addr).CombinedOutput()
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could do this only in the fallback if lookup with go implementation fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tonistiigi can you please elaborate why do you want to have both implementations? In case chgrp binary is not available, or as an optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but I think a POSIX system lacking chgrp is something extremely weird, and an exit with an error seems like a proper thing to do in such case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @tonistiigi. Having the Go implementation as a default when Docker isn't statically linked does seem to be a good idea. Docker already relies on some binaries being present on the systems. It doesn't seem to be a good idea to introduce new requirements. The packages would have to be adjusted as well. POSIX systems may indeed have chgrp. Containers and other such stripped down environments may not.

if err != nil {
msg := err.Error()
if len(out) > 0 {
msg = string(bytes.TrimSpace(out))
}
err = errors.Errorf("can't change group of unix socket %s: %s", addr, msg)
if socketGroup != defaultSocketGroup {
return nil, err
}
logrus.Warnf("could not change group %s to %s: %v", addr, defaultSocketGroup, err)
// "docker" group does not exist? Don't fail, just warn
logrus.Warn(err)
}
gid = os.Getgid()
}
l, err := sockets.NewUnixSocket(addr, gid)
if err != nil {
return nil, fmt.Errorf("can't create unix socket %s: %v", addr, err)
}

if _, err := homedir.StickRuntimeDirContents([]string{addr}); err != nil {
// StickRuntimeDirContents returns nil error if XDG_RUNTIME_DIR is just unset
logrus.WithError(err).Warnf("cannot set sticky bit on socket %s under XDG_RUNTIME_DIR", addr)
}
ls = append(ls, l)
default:
return nil, fmt.Errorf("invalid protocol format: %q", proto)
return nil, errors.Errorf("invalid protocol format: %q", proto)
}

return ls, nil
Expand All @@ -76,7 +87,7 @@ func listenFD(addr string, tlsConfig *tls.Config) ([]net.Listener, error) {
}

if len(listeners) == 0 {
return nil, fmt.Errorf("no sockets found via socket activation: make sure the service was started by systemd")
return nil, errors.New("no sockets found via socket activation: make sure the service was started by systemd")
}

// default to all fds just like unix:// and tcp://
Expand All @@ -86,21 +97,21 @@ func listenFD(addr string, tlsConfig *tls.Config) ([]net.Listener, error) {

fdNum, err := strconv.Atoi(addr)
if err != nil {
return nil, fmt.Errorf("failed to parse systemd fd address: should be a number: %v", addr)
return nil, errors.Errorf("failed to parse systemd fd address: should be a number: %v", addr)
}
fdOffset := fdNum - 3
if len(listeners) < fdOffset+1 {
return nil, fmt.Errorf("too few socket activated files passed in by systemd")
return nil, errors.New("too few socket activated files passed in by systemd")
}
if listeners[fdOffset] == nil {
return nil, fmt.Errorf("failed to listen on systemd activated file: fd %d", fdOffset+3)
return nil, errors.Errorf("failed to listen on systemd activated file: fd %d", fdOffset+3)
}
for i, ls := range listeners {
if i == fdOffset || ls == nil {
continue
}
if err := ls.Close(); err != nil {
return nil, fmt.Errorf("failed to close systemd activated file: fd %d: %v", fdOffset+3, err)
return nil, errors.Wrapf(err, "failed to close systemd activated file: fd %d", fdOffset+3)
}
}
return []net.Listener{listeners[fdOffset]}, nil
Expand Down
0