From 14010c88b4517cd34ec89d2cad799916964f6808 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 17 Dec 2019 09:36:25 +0100 Subject: [PATCH 1/2] config: preserve ownership and permissions on configfile When running `docker login` or `docker logout`, the CLI updates the configuration file by creating a temporary file, to replace the old one (if exists). When using `sudo`, this caused the file to be created as `root`, making it inaccessible to the current user. This patch updates the CLI to fetch permissions and ownership of the existing configuration file, and applies those permissions to the new file, so that it has the same permissions as the existing file (if any). Currently, only done for "Unix-y" systems (Mac, Linux), but can be implemented for Windows in future if there's a need. Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 22a291f703b2511daaa8e0d325ab48523e817558) Signed-off-by: Brian Goff --- cli/config/configfile/file.go | 3 +++ cli/config/configfile/file_unix.go | 35 +++++++++++++++++++++++++++ cli/config/configfile/file_windows.go | 5 ++++ 3 files changed, 43 insertions(+) create mode 100644 cli/config/configfile/file_unix.go create mode 100644 cli/config/configfile/file_windows.go diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index 388a5d54d6..a4e97a5caa 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -196,6 +196,9 @@ func (configFile *ConfigFile) Save() error { os.Remove(temp.Name()) return err } + // Try copying the current config file (if any) ownership and permissions + copyFilePermissions(configFile.Filename, temp.Name()) + return os.Rename(temp.Name(), configFile.Filename) } diff --git a/cli/config/configfile/file_unix.go b/cli/config/configfile/file_unix.go new file mode 100644 index 0000000000..3ca65c6140 --- /dev/null +++ b/cli/config/configfile/file_unix.go @@ -0,0 +1,35 @@ +// +build !windows + +package configfile + +import ( + "os" + "syscall" +) + +// copyFilePermissions copies file ownership and permissions from "src" to "dst", +// ignoring any error during the process. +func copyFilePermissions(src, dst string) { + var ( + mode os.FileMode = 0600 + uid, gid int + ) + + fi, err := os.Stat(src) + if err != nil { + return + } + if fi.Mode().IsRegular() { + mode = fi.Mode() + } + if err := os.Chmod(dst, mode); err != nil { + return + } + + uid = int(fi.Sys().(*syscall.Stat_t).Uid) + gid = int(fi.Sys().(*syscall.Stat_t).Gid) + + if uid > 0 && gid > 0 { + _ = os.Chown(dst, uid, gid) + } +} diff --git a/cli/config/configfile/file_windows.go b/cli/config/configfile/file_windows.go new file mode 100644 index 0000000000..42fffc39ad --- /dev/null +++ b/cli/config/configfile/file_windows.go @@ -0,0 +1,5 @@ +package configfile + +func copyFilePermissions(src, dst string) { + // TODO implement for Windows +} From aaf1170520727772a5cbfa3982192055e17ff537 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Fri, 19 Jun 2020 11:14:14 -0700 Subject: [PATCH 2/2] Handle errors on close in config file write. I'm not sure if this fixes anything, however I have seen some weird behavior on Windows where temp config files are left around and there doesn't seem to be any errors reported. Signed-off-by: Brian Goff (cherry picked from commit d02173090ff15ffb3ec4a5427ba35dafeb7fa92b) Signed-off-by: Brian Goff --- cli/config/configfile/file.go | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/cli/config/configfile/file.go b/cli/config/configfile/file.go index a4e97a5caa..d28fa69ad6 100644 --- a/cli/config/configfile/file.go +++ b/cli/config/configfile/file.go @@ -13,6 +13,7 @@ import ( "github.com/docker/cli/cli/config/credentials" "github.com/docker/cli/cli/config/types" "github.com/pkg/errors" + "github.com/sirupsen/logrus" ) const ( @@ -177,7 +178,7 @@ func (configFile *ConfigFile) SaveToWriter(writer io.Writer) error { } // Save encodes and writes out all the authorization information -func (configFile *ConfigFile) Save() error { +func (configFile *ConfigFile) Save() (retErr error) { if configFile.Filename == "" { return errors.Errorf("Can't save config with empty filename") } @@ -190,15 +191,26 @@ func (configFile *ConfigFile) Save() error { if err != nil { return err } + defer func() { + temp.Close() + if retErr != nil { + if err := os.Remove(temp.Name()); err != nil { + logrus.WithError(err).WithField("file", temp.Name()).Debug("Error cleaning up temp file") + } + } + }() + err = configFile.SaveToWriter(temp) - temp.Close() if err != nil { - os.Remove(temp.Name()) return err } + + if err := temp.Close(); err != nil { + return errors.Wrap(err, "error closing temp file") + } + // Try copying the current config file (if any) ownership and permissions copyFilePermissions(configFile.Filename, temp.Name()) - return os.Rename(temp.Name(), configFile.Filename) }