docker / docker/cli

docker should fail if config file is invalid

Open
#5,075 0 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

area/authentication kind/bug version/26.1
Dominant language
Go
Stars
6.1k
Forks
2.2k
Avg merge
1d 15h
Merged PRs (30d)
43

Description

Description

In situations where the config-file is invalid, we ignore the error, and proceed, which can lead to the config file being overwritten for an "empty" file (only some defaults included)

Reproduce
docker run -it --rm -v /var/run/docker.sock:/var/run/docker.sock docker:26.1.2-cli sh
mkdir -p ~/.docker
echo '{"imagesFormat":"some special format I have carefully hand-crafted, artisinally"' > ~/.docker/config.json

docker logout
WARNING: Error loading config file: /root/.docker/config.json: unexpected EOF
Removing login credentials for https://index.docker.io/v1/

cat ~/.docker/config.json
{
	"auths": {}
}
Expected behavior

The problematic area here is that any error returned from loading the config-file is printed as a warning; https://github.com/docker/cli/blob/61fe22f21a1a618d73cba67fec498f3f9d3b1422/cli/config/config.go#L125

A result of that is that if the file fails to load (malformed file, or other reason), load returns an error AND an empty configFile struct; https://github.com/docker/cli/blob/61fe22f21a1a618d73cba67fec498f3f9d3b1422/cli/config/config.go#L117

In this case, we notify the user (warning), but continue the regular flow, which may involve "updating the file" (login / logout, perhaps docker context use)

I was curious though about the "warning" instead of "error"; wondering if it was intended to ignore "not found" errors only, but I don't think that's the case; I think this is mostly because code moved around too much. First time this became a warning looks to be from https://github.com/moby/moby/commit/18c9b6c6455f116ae59cde8544413b3d7d294a5e

Which unlined some of the code; before that, the separate function returned an error, but was not handled. Going back further; it looks like this commit started to ignore errors https://github.com/moby/moby/commit/3bae188b8dc51911a44ea1c7b5681f9f07f9d3af
And https://github.com/moby/moby/commit/18962d0ff33cfa6ba2976aa459c766acfd23c1bf looks to settle that by fully ignoring.

So Tl;DR; I don't think there's a good reason / motivation for ignoring the error other than "convenience" (prefer happy path) but changing will mean that we need to change the signature of some functions to return.

docker version
Client:
 Version:           26.1.2
 API version:       1.44 (downgraded from 1.45)
 Go version:        go1.21.10
 Git commit:        211e74b
 Built:             Wed May  8 13:59:48 2024
 OS/Arch:           linux/arm64
 Context:           default
docker info
not relevant
Additional Info

No response

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in cli/config/config.go at the referenced load and warning paths, then trace how docker login, logout, and context use handle the returned error. Reproduce the malformed ~/.docker/config.json case and inspect the relevant callers; done means invalid configuration is not treated as an empty file or overwritten during these flows.

Written by the indexing model from the issue text.

Assessment

Tech stack
docker, go
Domain
cli
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.