Improve / harden handling of CLI configuration files

[thaJeztah]

I noticed this "WARNING" line when reading the code;

cli/cli/config/config.go

Lines 117 to 126 in 48bd4c6

	func LoadDefaultConfigFile(stderr io.Writer) *configfile.ConfigFile {
	configFile, err := Load(Dir())
	if err != nil {
	fmt.Fprintf(stderr, "WARNING: Error loading config file: %v\n", err)
	}
	if !configFile.ContainsAuth() {
	configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore)
	}
	return configFile
	}

From that code, I think we're handling configuration files incorrectly, so I did some testing, and wrote down what I propose to be the correct behavior for these situations

Prepare the test;

mkdir config-example && cd config-example
mkdir ./non-existing-config

mkdir -p ./not-a-file-config/config.json

touch ./not-a-directory-config

mkdir ./invalid-config && echo "Invalid JSON" > ./invalid-config/config.json
mkdir ./empty-config && touch ./empty-config/config.json
mkdir ./whitespace-config && echo '' > ./whitespace-config/config.json

mkdir ./no-access-config && echo "{}" > ./no-access-config/config.json
chmod 600 ./no-access-config/config.json
sudo chown 0:0 ./no-access-config/config.json

1. missing configuration file

Specifying a non-existing config.json silently ignores the missing file;

docker --config=./non-existing-config version

Client: Docker Engine - Community
 Version:           18.09.1
 API version:       1.39
...

Proposed behavior

We should

• fail if a user explicitly specifies a configuration file to use, and the file is missing.
• if no configuration file is specified; ignore the missing file (as having a configuration file is optional)

2. Invalid configuration file

Using an invalid config.json will print a warning, but continue anyway;

docker --config=./invalid-config version

WARNING: Error loading config file: invalid-config/config.json: invalid character 'I' looking for beginning of value
Client: Docker Engine - Community
 Version:           18.09.1
 API version:       1.39
...

Reporting a "successful" exit-code;

echo $?
0

Same for non-accessible, non-file configurations;

docker --config=./not-a-file-config/ version

WARNING: Error loading config file: not-a-file-config/config.json: read not-a-file-config/config.json: is a directory
Client: Docker Engine - Community

docker --config=./no-access-config/config.json

WARNING: Error loading config file: no-access-config/config.json: open no-access-config/config.json: permission denied

docker --config=./not-a-directory-config version
WARNING: Error loading config file: not-a-directory-config/config.json: stat not-a-directory-config/config.json: not a directory

Proposed behavior

We should always fail if the configuration file is invalid. The current behavior only warns about the missing file, then continues with the default configuration.

3. Don't fallback to old file if --config is set

If a non-existing config-path is set, the problem is silenty ignored, and the CLI falls back to using the old (~/.dockercfg) configuration file:

echo '{}' > ~/.dockercfg
docker --config=./non-existing-config version

Client: Docker Engine - Community
 Version:           18.09.1
...

Proposed behavior

We should only fallback to the old configuration file if no --config path is specified by the user.

If a --config path is set, and that file is not found, we must produce an error because the specified file could not be loaded.

4. Fail on invalid legacy configuration file;

echo 'Invalid JSON' > ~/.dockercfg

docker version

WARNING: Error loading config file: /Users/sebastiaan/.docker/config.json: Invalid Auth config file

Proposed behavior

Same as for the default configuration file: even though the ~/.dockercfg file is optional; if a file is present, it must be valid, otherwise make it a hard fail.

5. (bug) Wrong path is printed when falling back to an invalid ~/.dockercfg

mv ~/.docker/config.json ~/.docker/config.json-temp

echo 'Invalid JSON' > ~/.dockercfg

docker version
WARNING: Error loading config file: /Users/sebastiaan/.docker/config.json: Invalid Auth config file

docker --config=./non-existing-config version
WARNING: Error loading config file: non-existing-config/config.json: Invalid Auth config file


# restore your config.json ;-)
mv ~/.docker/config.json-temp ~/.docker/config.json

Proposed behavior

The warning/error should show the actual path of the invalid file; the current behavior is confusing, because the user will be looking for a ~/.docker/config.json, but won't find one

6. (tbd) Ignore empty configuration file?

Not sure what's best here; ignore empty files, or fail? If we decide to fail, the error message could be improved (e.g. "the file is empty" instead of EOF).

docker --config=./empty-config version
WARNING: Error loading config file: empty-config/config.json: EOF


docker --config=./whitespace-config version
WARNING: Error loading config file: whitespace-config/config.json: EOF

[thaJeztah]

@ijc @simonferquel @vdemeester

[ijc]

All seems reasonable to me.

[simonferquel]

SGTM as well.