8000 cli/config: do not discard permission errors when loading config-file · docker/cli@604c531 · GitHub
[go: up one dir, main page]
Skip to content

Commit 604c531

Browse files
thaJeztahthaJeztah
committed
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>
1 parent 4a2115b commit 604c531

File tree

2 files changed

+52
-17
lines changed

2 files changed

+52
-17
lines changed

‎cli/config/config.go‎

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ func Path(p ...string) (string, error) {
7575
}
7676

7777
// LoadFromReader is a convenience function that creates a ConfigFile object from
78-
// a reader
78+
// a reader. It returns an error if configData is malformed.
7979
func LoadFromReader(configData io.Reader) (*configfile.ConfigFile, error) {
8080
configFile := configfile.ConfigFile{
8181
AuthConfigs: make(map[string]types.AuthConfig),
@@ -88,6 +88,10 @@ func LoadFromReader(configData io.Reader) (*configfile.ConfigFile, error) {
8888
// If no directory is given, it uses the default [Dir]. A [configfile.ConfigFile]
8989
// is returned containing the contents of the configuration file, or a default
9090
// struct if no configfile exists in the given location.
91+
//
92+
// Load returns an error if a configuration file exists in the given location,
93+
// but cannot be read, or is malformed. Consumers must handle errors to prevent
94+
// overwriting an existing configuration file.
9195
func Load(configDir string) (*configfile.ConfigFile, error) {
9296
if configDir == "" {
9397
configDir = Dir()
@@ -102,29 +106,36 @@ func load(configDir string) (*configfile.ConfigFile, error) {
102106
file, err := os.Open(filename)
103107
if err != nil {
104108
if os.IsNotExist(err) {
105-
//
106-
// if file is there but we can't stat it for any reason other
107-
// than it doesn't exist then stop
109+
// It is OK for no configuration file to be present, in which
110+
// case we return a default struct.
108111
return configFile, nil
109112
}
110-
// if file is there but we can't stat it for any reason other
111-
// than it doesn't exist then stop
112-
return configFile, nil
113+
// Any other error happening when failing to read the file must be returned.
114+
return configFile, errors.Wrap(err, "loading config file")
113115
}
114116
defer file.Close()
115117
err = configFile.LoadFromReader(file)
116118
if err != nil {
117-
err = errors.Wrap(err, filename)
119+
err = errors.Wrapf(err, "loading config file: %s: ", filename)
118120
}
119121
return configFile, err
120122
}
121123

122124
// LoadDefaultConfigFile attempts to load the default config file and returns
123-
// an initialized ConfigFile struct if none is found.
125+
// an initialized ConfigFile struct if none is found or when failing to load
126+
// the configuration file. If no credentials-store is set in the configuration
127+
// file, it attempts to discover the default store to use for the current platform.
128+
//
129+
// Important: LoadDefaultConfigFile prints a warning to stderr when failing to
130+
// load the configuration file, but otherwise ignores errors. Consumers should
131+
// consider using [Load] (and [credentials.DetectDefaultStore]) to detect errors
132+
// when updating the configuration file, to prevent discarding a (malformed)
133+
// configuration file.
124134
func LoadDefaultConfigFile(stderr io.Writer) *configfile.ConfigFile {
125135
configFile, err := load(Dir())
126136
if err != nil {
127-
_, _ = fmt.Fprintf(stderr, "WARNING: Error loading config file: %v\n", err)
137+
// FIXME(thaJeztah): we should not proceed here to prevent overwriting existing (but malformed) config files; see https://github.com/docker/cli/issues/5075
138+
_, _ = fmt.Fprintln(stderr, "WARNING: Error", err)
128139
}
129140
if !configFile.ContainsAuth() {
130141
configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore)

‎cli/config/config_test.go‎

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,16 @@ func TestLoadDanglingSymlink(t *testing.T) {
6969
assert.Equal(t, fi.Mode()&os.ModeSymlink, os.ModeSymlink, "expected %v to be a symlink", cfgFile)
7070
}
7171

72+
func TestLoadNoPermissions(t *testing.T) {
73+
cfgDir := t.TempDir()
74+
cfgFile := filepath.Join(cfgDir, ConfigFileName)
75+
err := os.WriteFile(cfgFile, []byte(`{}`), os.FileMode(0o000))
76+
assert.NilError(t, err)
77+
78+
_, err = Load(cfgDir)
79+
assert.ErrorIs(t, err, os.ErrPermission)
80+
}
81+
7282
func TestSaveFileToDirs(t *testing.T) {
7383
tmpHome := filepath.Join(t.TempDir(), ".docker")
7484
config, err := Load(tmpHome)
@@ -345,14 +355,28 @@ func TestLoadDefaultConfigFile(t *testing.T) {
345355
err := os.WriteFile(filename, content, 0o644)
346356
assert.NilError(t, err)
347357

348-
configFile := LoadDefaultConfigFile(buffer)
349-
credStore := credentials.DetectDefaultStore("")
350-
expected := configfile.New(filename)
351-
expected.CredentialsStore = credStore
352-
expected.PsFormat = "format"
358+
t.Run("success", func(t *testing.T) {
359+
configFile := LoadDefaultConfigFile(buffer)
360+
credStore := credentials.DetectDefaultStore("")
361+
expected := configfile.New(filename)
362+
expected.CredentialsStore = credStore
363+
expected.PsFormat = "format"
353364

354-
assert.Check(t, is.DeepEqual(expected, configFile))
355-
assert.Check(t, is.Equal(buffer.String(), ""))
3 5C9C 65+
assert.Check(t, is.DeepEqual(expected, configFile))
366+
assert.Check(t, is.Equal(buffer.String(), ""))
367+
})
368+
369+
t.Run("permission error", func(t *testing.T) {
370+
err = os.Chmod(filename, 0o000)
371+
assert.NilError(t, err)
372+
373+
buffer.Reset()
374+
_ = LoadDefaultConfigFile(buffer)
375+
warnings := buffer.String()
376+
377+
assert.Check(t, is.Contains(warnings, "WARNING:"))
378+
assert.Check(t, is.Contains(warnings, os.ErrPermission.Error()))
379+
})
356380
}
357381

358382
func TestConfigPath(t *testing.T) {

0 commit comments

Comments
 (0)
0