diff --git a/cli/config/config.go b/cli/config/config.go index 952f6e71f4..650f59e465 100644 --- a/cli/config/config.go +++ b/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) diff --git a/cli/config/config_test.go b/cli/config/config_test.go index b513a21522..e74a25863e 100644 --- a/cli/config/config_test.go +++ b/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) {