From dc75e9ed6cd6d443bc5567c7360db70072d1eedc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 May 2024 16:16:20 +0200 Subject: [PATCH] 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 --- cli/config/config.go | 21 ++++++++++-------- cli/config/config_test.go | 46 +++++++++++++++++++++++++++++++++------ 2 files changed, 51 insertions(+), 16 deletions(-) diff --git a/cli/config/config.go b/cli/config/config.go index 9d318d85da..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), @@ -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) { - // - // 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 } @@ -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) diff --git a/cli/config/config_test.go b/cli/config/config_test.go index ceb96f4d51..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 { @@ -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) {