From 426fb2fd81e95d36d1628b539d8cf2cf60ad0e59 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 May 2024 14:19:09 +0200 Subject: [PATCH 1/4] cli/config: Load(), LoadDefaultConfigFile(): improve GoDoc Signed-off-by: Sebastiaan van Stijn --- cli/config/config.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/cli/config/config.go b/cli/config/config.go index 952f6e71f4..9d318d85da 100644 --- a/cli/config/config.go +++ b/cli/config/config.go @@ -84,8 +84,10 @@ 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. func Load(configDir string) (*configfile.ConfigFile, error) { if configDir == "" { configDir = Dir() @@ -118,7 +120,16 @@ func load(configDir string) (*configfile.ConfigFile, error) { } // 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 { From de91207b870896a814661324804482ef0eef449a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 May 2024 15:53:57 +0200 Subject: [PATCH 2/4] cli/config: add test for dangling symlink for config-file This may need further discussion, but we currently handle dangling symlinks gracefully, so let's add a test for this, and verify that we don't replace symlinks with a file. Signed-off-by: Sebastiaan van Stijn --- cli/config/config_test.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/cli/config/config_test.go b/cli/config/config_test.go index b513a21522..45f2a66a69 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -48,6 +48,27 @@ 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 TestSaveFileToDirs(t *testing.T) { tmpHome := filepath.Join(t.TempDir(), ".docker") config, err := Load(tmpHome) From c80adf4e87e1034c3bba977c707a33dbed94ba32 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 May 2024 16:05:32 +0200 Subject: [PATCH 3/4] cli/config: TestLoadDefaultConfigFile: check that no warnings are printed Loading the config should print no warnings on a successful load. Signed-off-by: Sebastiaan van Stijn --- cli/config/config_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cli/config/config_test.go b/cli/config/config_test.go index 45f2a66a69..ceb96f4d51 100644 --- a/cli/config/config_test.go +++ b/cli/config/config_test.go @@ -352,6 +352,7 @@ func TestLoadDefaultConfigFile(t *testing.T) { expected.PsFormat = "format" assert.Check(t, is.DeepEqual(expected, configFile)) + assert.Check(t, is.Equal(buffer.String(), "")) } func TestConfigPath(t *testing.T) { From dc75e9ed6cd6d443bc5567c7360db70072d1eedc Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 16 May 2024 16:16:20 +0200 Subject: [PATCH 4/4] 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) {