run: flag to include the docker socket

[stevvooe]

Adds a flag, --use-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 13.97849% with 80 lines in your changes missing coverage. Please review.

Project coverage is 59.28%. Comparing base (23eadcd) to head (8799c1e).
Report is 4 commits behind head on master.

❌ Your patch status has failed because the patch coverage (13.97%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5858      +/-   ##
==========================================
- Coverage   59.43%   59.28%   -0.16%     
==========================================
  Files         358      358              
  Lines       29762    29847      +85     
==========================================
+ Hits        17690    17695       +5     
- Misses      11107    11185      +78     
- Partials      965      967       +2     

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[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.

[stevvooe]

A few fixes that have been put into place:

1. The secrets are now in /run/secrets to follow the compose convention. We should probably use tmpfs there but compose doesn't do this today so we can leave it be. Ideally, we'd have /run be tmpfs and just have that mount at create time so we can have the convention of dumping secrects or whatever in there.
2. The socket address now resolves from the context.
   I'll be updating tests once this CI runs and fails.

I'll need guidance on marking the flag as experimental.

[Benehiko]

I've created a POC to instead share credentials over a unix socket which we could bind mount into a container. I don't know if that's the correct approach or if it's something that would be better than what is inside this PR. #5948

[stevvooe]

@Benehiko We do have discussions around the best way to allow secrets access from within a container but we don't want to block this addition too much longer. We can always change the implementation to use such a system in the future.

Let's keep the discussion on your PR going and work from there.

[cpuguy83]

Seems like this should be a feature on the engine rather than client side.
Maybe something like --mount type=api-socket.
Not sure how well this would work on the Windows side, though.

[stevvooe]

Seems like this should be a feature on the engine rather than client side.
For some implementations, client participation is required.

I like the idea of a mount long term but we'd have to teach every proxy about it. Plumbing for /var/run/docker.sock mostly already exists.

[stevvooe]

@cpuguy83 Do you want me to change the flag to the more general --use-api-socket?

[cpuguy83]

Either way for the flag is fine for me.

[Benehiko]

To me everything except the credentials part looks alright. I know we have already discussed on the PR a bit about it. If this is critical to get in now then I would be okay with merging as is, but we would end up having credential sync issues:

• On credential expiration
• Re-authenticating on the Host would completely invalidate any credentials in the container
• Currently CLI doesn't really use the OAuth Access and Refresh tokens - except for fetching a PAT. If this were to change, token refreshes would also invalidate container credentials.

[stevvooe]

@Benehiko Indeed, those are the current limitations we have here. I hope not to leave it in this condition. Thanks for your review!

I'll added some tests before the end of this week.