cli/config: do not discard permission errors when loading config-file

When attempting to load a config-file that exists, but is not accessible for
the current user, we should not discard the error.

This patch makes sure that the error is returned by Load(), but does not yet
change LoadDefaultConfigFile, as this requires a change in signature.

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Author: thaJeztah

cli/config/config.go

@@ -75,7 +75,7 @@ func Path(p ...string) (string, error) {
 }
 
 // LoadFromReader is a convenience function that creates a ConfigFile object from
-// a reader
+// a reader. It returns an error if configData is malformed.
 func LoadFromReader(configData io.Reader) (*configfile.ConfigFile, error) {
 	configFile := configfile.ConfigFile{
 		AuthConfigs: make(map[string]types.AuthConfig),
@@ -88,6 +88,10 @@ func LoadFromReader(configData io.Reader) (*configfile.ConfigFile, error) {
 // If no directory is given, it uses the default [Dir]. A [*configfile.ConfigFile]
 // is returned containing the contents of the configuration file, or a default
 // struct if no configfile exists in the given location.
+//
+// Load returns an error if a configuration file exists in the given location,
+// but cannot be read, or is malformed. Consumers must handle errors to prevent
+// overwriting an existing configuration file.
 func Load(configDir string) (*configfile.ConfigFile, error) {
 	if configDir == "" {
 		configDir = Dir()
@@ -102,29 +106,36 @@ func load(configDir string) (*configfile.ConfigFile, error) {
 	file, err := os.Open(filename)
 	if err != nil {
 		if os.IsNotExist(err) {
-			//
-			// if file is there but we can't stat it for any reason other
-			// than it doesn't exist then stop
+			// It is OK for no configuration file to be present, in which
+			// case we return a default struct.
 			return configFile, nil
 		}
-		// if file is there but we can't stat it for any reason other
-		// than it doesn't exist then stop
-		return configFile, nil
+		// Any other error happening when failing to read the file must be returned.
+		return configFile, errors.Wrap(err, "loading config file")
 	}
 	defer file.Close()
 	err = configFile.LoadFromReader(file)
 	if err != nil {
-		err = errors.Wrap(err, filename)
+		err = errors.Wrapf(err, "loading config file: %s: ", filename)
 	}
 	return configFile, err
 }
 
 // LoadDefaultConfigFile attempts to load the default config file and returns
-// an initialized ConfigFile struct if none is found.
+// an initialized ConfigFile struct if none is found or when failing to load
+// the configuration file. If no credentials-store is set in the configuration
+// file, it attempts to discover the default store to use for the current platform.
+//
+// Important: LoadDefaultConfigFile prints a warning to stderr when failing to
+// load the configuration file, but otherwise ignores errors. Consumers should
+// consider using [Load] (and [credentials.DetectDefaultStore]) to detect errors
+// when updating the configuration file, to prevent discarding a (malformed)
+// configuration file.
 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)
+		// FIXME(thaJeztah): we should not proceed here to prevent overwriting existing (but malformed) config files; see https://github.com/docker/cli/issues/5075
+		_, _ = fmt.Fprintln(stderr, "WARNING: Error", err)
 	}
 	if !configFile.ContainsAuth() {
 		configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore)

cli/config/config_test.go

@@ -5,13 +5,15 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
 	"github.com/docker/cli/cli/config/configfile"
 	"github.com/docker/cli/cli/config/credentials"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
+	"gotest.tools/v3/skip"
 )
 
 func setupConfigDir(t *testing.T) string {
@@ -69,6 +71,19 @@ func TestLoadDanglingSymlink(t *testing.T) {
 	assert.Equal(t, fi.Mode()&os.ModeSymlink, os.ModeSymlink, "expected %v to be a symlink", cfgFile)
 }
 
+func TestLoadNoPermissions(t *testing.T) {
+	if runtime.GOOS != "windows" {
+		skip.If(t, os.Getuid() == 0, "cannot test permission denied when running as root")
+	}
+	cfgDir := t.TempDir()
+	cfgFile := filepath.Join(cfgDir, ConfigFileName)
+	err := os.WriteFile(cfgFile, []byte(`{}`), os.FileMode(0o000))
+	assert.NilError(t, err)
+
+	_, err = Load(cfgDir)
+	assert.ErrorIs(t, err, os.ErrPermission)
+}
+
 func TestSaveFileToDirs(t *testing.T) {
 	tmpHome := filepath.Join(t.TempDir(), ".docker")
 	config, err := Load(tmpHome)
@@ -345,14 +360,31 @@ func TestLoadDefaultConfigFile(t *testing.T) {
 	err := os.WriteFile(filename, content, 0o644)
 	assert.NilError(t, err)
 
-	configFile := LoadDefaultConfigFile(buffer)
-	credStore := credentials.DetectDefaultStore("")
-	expected := configfile.New(filename)
-	expected.CredentialsStore = credStore
-	expected.PsFormat = "format"
+	t.Run("success", func(t *testing.T) {
+		configFile := LoadDefaultConfigFile(buffer)
+		credStore := credentials.DetectDefaultStore("")
+		expected := configfile.New(filename)
+		expected.CredentialsStore = credStore
+		expected.PsFormat = "format"
 
-	assert.Check(t, is.DeepEqual(expected, configFile))
-	assert.Check(t, is.Equal(buffer.String(), ""))
+		assert.Check(t, is.DeepEqual(expected, configFile))
+		assert.Check(t, is.Equal(buffer.String(), ""))
+	})
+
+	t.Run("permission error", func(t *testing.T) {
+		if runtime.GOOS != "windows" {
+			skip.If(t, os.Getuid() == 0, "cannot test permission denied when running as root")
+		}
+		err = os.Chmod(filename, 0o000)
+		assert.NilError(t, err)
+
+		buffer.Reset()
+		_ = LoadDefaultConfigFile(buffer)
+		warnings := buffer.String()
+
+		assert.Check(t, is.Contains(warnings, "WARNING:"))
+		assert.Check(t, is.Contains(warnings, os.ErrPermission.Error()))
+	})
 }
 
 func TestConfigPath(t *testing.T) {