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 <github@gone.nl>

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,19 +106,17 @@ func load(configDir string) (*configfile.ConfigFile, error) {
 	file, err := os.Open(filename)
 	if err != nil {
 		if os.IsNotExist(err) {
-			//
+			// It is OK for no configuration file to be present, in which
-			// if file is there but we can't stat it for any reason other
+			// case we return a default struct.
-			// than it doesn't exist then stop
 			return configFile, nil
 		}
-		// if file is there but we can't stat it for any reason other
+		// Any other error happening when failing to read the file must be returned.
-		// than it doesn't exist then stop
+		return configFile, errors.Wrap(err, "loading config file")
-		return configFile, nil
 	}
 	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
 }
@@ -133,7 +135,8 @@ func load(configDir string) (*configfile.ConfigFile, error) {
 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,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 
@@ -12,6 +13,7 @@ import (
 	"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)
+	t.Run("success", func(t *testing.T) {
-	credStore := credentials.DetectDefaultStore("")
+		configFile := LoadDefaultConfigFile(buffer)
-	expected := configfile.New(filename)
+		credStore := credentials.DetectDefaultStore("")
-	expected.CredentialsStore = credStore
+		expected := configfile.New(filename)
-	expected.PsFormat = "format"
+		expected.CredentialsStore = credStore
+		expected.PsFormat = "format"
 
-	assert.Check(t, is.DeepEqual(expected, configFile))
+		assert.Check(t, is.DeepEqual(expected, configFile))
-	assert.Check(t, is.Equal(buffer.String(), ""))
+		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) {