Merge pull request #5077 from thaJeztah/config_error_handling

cli/config: improve handling of errors

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),
@@ -84,8 +84,14 @@ func LoadFromReader(configData io.Reader) (*configfile.ConfigFile, error) {
 	return &configFile, err
 }
 
-// Load reads the configuration files in the given directory, and sets up
-// the auth config information and returns values.
+// Load reads the configuration file ([ConfigFileName]) from the given directory.
+// 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()
@@ -100,29 +106,37 @@ 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.
+// a reference to the ConfigFile struct. If none is found or when failing to load
+// the configuration file, it initializes a default ConfigFile struct. 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,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 {
@@ -48,6 +50,40 @@ func TestMissingFile(t *testing.T) {
 	saveConfigAndValidateNewFormat(t, config, tmpHome)
 }
 
+// TestLoadDanglingSymlink verifies that we gracefully handle dangling symlinks.
+//
+// TODO(thaJeztah): consider whether we want dangling symlinks to be an error condition instead.
+func TestLoadDanglingSymlink(t *testing.T) {
+	cfgDir := t.TempDir()
+	cfgFile := filepath.Join(cfgDir, ConfigFileName)
+	err := os.Symlink(filepath.Join(cfgDir, "no-such-file"), cfgFile)
+	assert.NilError(t, err)
+
+	config, err := Load(cfgDir)
+	assert.NilError(t, err)
+
+	// Now save it and make sure it shows up in new form
+	saveConfigAndValidateNewFormat(t, config, cfgDir)
+
+	// Make sure we kept the symlink.
+	fi, err := os.Lstat(cfgFile)
+	assert.NilError(t, err)
+	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)
@@ -324,13 +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.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) {