run: flag to include the docker socket

[stevvooe]

Adds a flag, --edit-docker-socket that can be used to start a container with the correctly configured parameters to ensure that accessing the docker socket will work with out the fiddly flags.

There are a few problems with this approach:

1. We need a reliably way to clean up the configuration file. This currently is put into a tmp file then bind mounted. There is probably a better way to do this such as copying in the file.
2. We need a way to resolve the correct socket outside the container. If a different socket is used or a address and port, this will attempt to bind mount a nonexistent socket.

Either way, this is good start and resolves a long standing issue.

To try it out, you can do the following:

❯ ./docker run --rm -it --use-docker-socket docker:cli
/ # docker pull redis
Using default tag: latest
latest: Pulling from library/redis
Digest: sha256:93a8d83b707d0d6a1b9186edecca2e37f83722ae0e398aee4eea0ff17c2fad0e
Status: Image is up to date for redis:latest
docker.io/library/redis:latest

This PR is somewhat incomplete and needs the following:

• Address credentials leak
• Address socket address location
• Implement unit and e2e tests
• Update man pages and docs

- Adds a new flag `--use-docker-socket` to enable access to docker from inside a container

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 7.31707% with 76 lines in your changes missing coverage. Please review.

Project coverage is 58.75%. Comparing base (77a8a8c) to head (97eb9f4).
Report is 9 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5858      +/-   ##
==========================================
- Coverage   59.30%   58.75%   -0.56%     
==========================================
  Files         353      350       -3     
  Lines       29694    29763      +69     
==========================================
- Hits        17609    17486     -123     
- Misses      11104    11293     +189     
- Partials      981      984       +3     

[stevvooe]

Alright, I just validated an approach to the credential leak issue using copy into the container. Let's validate the cli ux and then we can add that implementation if we want to go forward with the change.

[thaJeztah]

This is a tricky one; bind-mounting the docker socket by setting these options from the CLI will break in many scenarios. The current approach in this PR uses the default location (/var/run/docker.sock), which would work with a default config for a Linux daemon, but won't work for a Windows daemon (which currently uses a named pipe as default). A similar case would happen if the daemon is local, but running rootless, in which case the socket would be in XDG_RUNTIME_DIR (if set). In a non-default setting, the daemon can be configured to use a socket on a different location (or no socket at all if the daemon is configured to only list on TCP). The tricky bit here is that when connecting with a remote daemon, the CLI won't have the information where the socket is located.

For a daemon running locally (same machine), the CLI could have slightly more information, and look at the docker host configuration in the docker context used;

docker context inspect | jq .[].Endpoints.docker.Host
"unix:///var/run/docker.sock"

In general, these could be somewhat tricky even with the good location, as there's a potential race condition where the container is started before the API server (and thus socket) is up, in which case using the -v shorthand (which implicitly has "auto-create host path" enabled) can result in /var/run/docker.sock being created as a directory if the path is missing (which could prevent the daemon from finishing startup).

I recall some older proposals to have an option on the daemon side to grant a container access to the API; for those, the daemon would handle setting up a socket and mounting it into the container. Those were a really long time ago, and IIRC the initial implementation was to do so automatically for any container with --privileged, but there was discussion to have a separate option for this (because forwarding the socket into a --privileged container could interfere with DIND cases, which would have their own socket inside the container from the daemon running in it);

• Add unix socket for daemon access for privileged containers  moby/moby#25347
• Automatically add docker socket with --privileged moby/moby#9135
• Enhancement: exposing Docker API listening socket(s) moby/moby#33784

I think something along those lines (i.e., having an option to forward the socket, and have the daemon set this up) could solve most of the issues mentioned earlier; the daemon would be able to set up the socket for the container, regardless how it was configured, which could even mean a dedicated socket for the container (which could potentially be expanded to more granular / limited access to the socket, instead of access to all endpoints).

Slightly related (forwarding a socket from the client to the daemon to allow ssh-agent-like constructs for forwarding into the container); this could possibly be tying in to the "forward authentication" part;

• [WIP] Add forwarding local (client-side) sockets to container moby/moby#19667

[thaJeztah]

For the credentials; potentially we could use some env-var, although env-vars may not be ideal as they will leak both into the container's config (docker container inspect), and any process inside the container. Forwarding "all credentials" is what's currently used by the classic builder, which encodes them in a X-Registry-Config header that contains all credentials for all registries the CLI is aware of; https://github.com/moby/moby/blob/0de01f700d9040f70d71d16fff2ea4aafc4af36b/api/server/router/build/build_routes.go#L335-L348

Possibly we can make the CLI aware of an env-var along those lines; IIRC, BuildKit requests auth from the CLI when building, so not 100% sure if that would work in that combination, and likely all CLI plugins would have to be updated to be aware of this approach.

[laurazard]

Tangential, but regarding:

... reliably way to clean up the configuration file. This currently is put into a tmp file then bind mounted. There is probably a better way to do this such as copying in the file.

Ultimately, this should be unnecessary and we should make the necessary changes to stop the "credentials in ~/.docker/config.json" case so common. i.e., stop storing credentials together with CLI configurations (in a non-breaking way, and keep accepting those), either by just storing them elsewhere or using a "plaintext credential-helper", which would also solve a number of other issues.

[stevvooe]

@laurazard Yes but unfortunately, there is no way to call the credential helps from inside the container. We could have an ssh-agent like call api for it but that would require an interactive client to that to work correctly.

@thaJeztah It sounds like I should at least use a mount rather that volume notation to avoid the accidental file creation.

[laurazard]

Right, that's definitely true. I think there's two separate issues that folks frequently run into that we should keep in mind:

• wanting to easily expose the docker socket inside of a container, to use, without sharing all credentials
  • this is annoying because other necessary configurations live in the same config.json file
• wanting to share some/all credentials for use inside a container
  • this commonly leads users to use the plaintext store instead of a credential helper, since these are easier to share with a container

It'd be awesome if whatever solution we came up with could account for/solve both cases (and splitting out creds – even if just to another file – from config.json would already help).

[stevvooe]

@laurazard Looking at compose, they actually use the same technique that I've employed here: https://github.com/docker/compose/blob/145bb8466dffd18d7828ca0bc0725b1059506098/pkg/compose/secrets.go#L31. The secrets are copied over to /run/secrets. It looks like that is a tmpfs and the copy is done after start. This PR is a little problematic in that they are copied into the container's read-write layer. My notes on that are in https://github.com/docker/cli/pull/5858/files#diff-50fe347eb4bdafebc190aa42b2caa34b089a538cd444b8a151a763372e93c0cfR269. Basically, I'll have to start the container to employ this correctly, so we won't have it on create, which is probably ok.

Here's my proposal:

• I'll move the this over to the /run/secrets tmpfs mount to ensure the secrets don't leak to the overlay filesystem. The copy will be done post container start but before we hand terminal the success off to the user.
• I'll add some logic to better resolve the socket location and move to mounts to better control how it appears in the container.
• We can mark this flag as experimental. (no clue to do this, direction needed)
• I'll rally the troops a bit here to see if we can get a session-callback proposal in place or at least first-class support for a secret mounts, similar to what's happening buildkit.

I know this isn't ideal but this has been a long-lived pain point. The abstraction of the --use-docker-socket flag is high-level enough to where we can improve the credential access over time without breaking users, especially if we maintain it as an experimental feature.

I hope this is reasonable and can allow us to move forward.