docker-library / docker

Docker Official Image packaging for Docker
Apache License 2.0
1.09k stars 568 forks source link

(re)consider adding `docker` group to docker-in-docker (dind) image #459

Closed thaJeztah closed 6 months ago

thaJeztah commented 7 months ago

There's two older tickets related to this;

The docker daemon by default tries to set permissions of the unix-socket to be owned by the docker group. This group is used to allow accessing the socket as a non-root user, as described in the "Manage Docker as a non-root user" section in the documentation.

Currently, the docker images do not contain a docker group, which results in a warning being printed during startup of the daemon;

docker run -it --rm --privileged -v docker:/var/lib/docker docker:dind sh
/ # dockerd
INFO[2023-11-15T11:24:34.632589125Z] Starting up
WARN[2023-11-15T11:24:34.634358166Z] could not change group /var/run/docker.sock to docker: group docker not found
INFO[2023-11-15T11:24:34.635752416Z] containerd not running, starting managed containerd

This warning originates from the daemon, which attempts to use the default (docker) group as owner for the socket https://github.com/moby/moby/blob/93fffa299c2cb23055bb54c380a02d53c9a7d525/daemon/listeners/listeners_linux.go#L35-L49

case "unix":
    gid, err := lookupGID(socketGroup)
    if err != nil {
        if socketGroup != "" {
            if socketGroup != defaultSocketGroup {
                return nil, err
            }
            log.G(context.TODO()).Warnf("could not change group %s to %s: %v", addr, defaultSocketGroup, err)
        }
        gid = os.Getgid()
    }
    l, err := sockets.NewUnixSocket(addr, gid)
    if err != nil {
        return nil, errors.Wrapf(err, "can't create unix socket %s", addr)
    }

note: we should probably update the Manage Docker as a non-root user documentation a bit though as the instructions assume the docker group has to be created (we should mention how to check if it already exists).

I went looking what we do in other situations, i.e., when installing from our .deb and .rpm packages, and for those, we are creating the docker group as part of the installation; https://github.com/docker/docker-ce-packaging/blob/3479bfbc96ec430a808b8d22a503c12773463a66/deb/common/docker-ce.postinst#L5-L11

configure)
    if [ -z "$2" ]; then
        if ! getent group docker > /dev/null; then
            groupadd --system docker
        fi
    fi
    ;;

And, at runtime, that group is used as owner for the socket, as well as log-files produced;

With systemd

https://github.com/moby/moby/blob/93fffa299c2cb23055bb54c380a02d53c9a7d525/contrib/init/systemd/docker.socket#L4-L10

[Socket]
# If /var/run is not implemented as a symlink to /run, you may need to
# specify ListenStream=/var/run/docker.sock instead.
ListenStream=/run/docker.sock
SocketMode=0660
SocketUser=root
SocketGroup=docker

With openrc

https://github.com/moby/moby/blob/93fffa299c2cb23055bb54c380a02d53c9a7d525/contrib/init/openrc/docker.initd#L24-L26

start_pre() {
    checkpath -f -m 0644 -o root:docker "$DOCKER_LOGFILE"
}

sysvinit

https://github.com/moby/moby/blob/93fffa299c2cb23055bb54c380a02d53c9a7d525/contrib/init/sysvinit-debian/docker#L59

chgrp docker "$DOCKER_LOGFILE"

Consider adding a docker group

While users should still "think twice" before granting access to the unix-socket (as it is equivalent of root), and consider rootless instead if their situation allows it, I think it would make sense to add a docker group to the image, so that we're consistent with other ways the docker daemon is distributed / packaged.

Perhaps there's reasons why we didn't choose to though, but if that's the case (and we decide not to), we can use this ticket to document those reasons (and possibly extend the documentation on Docker Hub).

Alternatively, we could update the entrypoint script to explicitly set root as group with the -G / --group flag; doing so should suppress the warning.

  -G, --group string                            Group for the unix socket (default "docker")
docker run -it --rm --privileged -v docker:/var/lib/docker docker:dind sh
/ # dockerd --group=root
INFO[2023-11-15T11:24:03.549368180Z] Starting up
INFO[2023-11-15T11:24:03.556428722Z] containerd not running, starting managed containerd
thaJeztah commented 7 months ago

/cc @tianon @yosifkit - opening this for discussion, as I kept noticing that warning being printed, so wondered what the best option would be 😄

tianon commented 7 months ago

Hmm, yeah, I guess we should consider adding it.

We should probably give it a fixed GID, maybe 2375 to match Docker's port number while still being unlikely to overlap with anything users might be creating or using already?

thaJeztah commented 7 months ago

Yeah, that's a good question. I also recall some images specifically using 1000 or 1001, but those may have been for matching permissions on boot2docker?

tianon commented 6 months ago

Yeah, or just matching what they'd set it to historically (before they chose a specific number, so the relevant tool chose one for them).

yosifkit commented 6 months ago

Does the rootless user in the rootless image also need to be added to the docker group?

i.e., add it to the docker group after creating the user here: https://github.com/docker-library/docker/blob/d8b20e0d84b8bac8629e782e0fc779d537eab8d8/Dockerfile-dind-rootless.template#L13


Edit: we can still add the rootless user to the group if necessary, but I didn't want to hold up the PR.