From 2fe3de16befda10563cd78aedd5df5751e3805b5 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 12 Apr 2017 12:31:00 -0700 Subject: [PATCH 1/4] Chown files when copying into chroot Fixes #2552 Not needed when hardlinking. Only adds Linux support but other OS's may be easy. --- client/allocdir/alloc_dir.go | 12 ++++++++++-- client/allocdir/fs_darwin.go | 5 +++++ client/allocdir/fs_freebsd.go | 5 +++++ client/allocdir/fs_linux.go | 8 ++++++++ client/allocdir/fs_solaris.go | 5 +++++ client/allocdir/fs_unix.go | 10 +++++----- client/allocdir/fs_windows.go | 9 +++++++-- client/allocdir/task_dir.go | 6 ++++-- 8 files changed, 49 insertions(+), 11 deletions(-) diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go index bbdb6431798..8c95d0c7509 100644 --- a/client/allocdir/alloc_dir.go +++ b/client/allocdir/alloc_dir.go @@ -385,7 +385,9 @@ func getFileWatcher(path string) watch.FileWatcher { return watch.NewPollingFileWatcher(path) } -func fileCopy(src, dst string, perm os.FileMode) error { +// fileCopy from src to dst setting the permissions and owner (if uid & gid are +// both greater than 0) +func fileCopy(src, dst string, uid, gid int, perm os.FileMode) error { // Do a simple copy. srcFile, err := os.Open(src) if err != nil { @@ -400,7 +402,13 @@ func fileCopy(src, dst string, perm os.FileMode) error { defer dstFile.Close() if _, err := io.Copy(dstFile, srcFile); err != nil { - return fmt.Errorf("Couldn't copy %v to %v: %v", src, dst, err) + return fmt.Errorf("Couldn't copy %q to %q: %v", src, dst, err) + } + + if uid >= 0 && gid >= 0 { + if err := dstFile.Chown(uid, gid); err != nil { + return fmt.Errorf("Couldn't copy %q to %q: %v", src, dst, err) + } } return nil diff --git a/client/allocdir/fs_darwin.go b/client/allocdir/fs_darwin.go index fe29791c460..7cd5caf23b4 100644 --- a/client/allocdir/fs_darwin.go +++ b/client/allocdir/fs_darwin.go @@ -24,3 +24,8 @@ func createSecretDir(dir string) error { func removeSecretDir(dir string) error { return os.RemoveAll(dir) } + +// getOwner isn't implemented for Darwin +func getOwner(os.FileInfo) (int, int) { + return -1, -1 +} diff --git a/client/allocdir/fs_freebsd.go b/client/allocdir/fs_freebsd.go index fe29791c460..0d34925a7af 100644 --- a/client/allocdir/fs_freebsd.go +++ b/client/allocdir/fs_freebsd.go @@ -24,3 +24,8 @@ func createSecretDir(dir string) error { func removeSecretDir(dir string) error { return os.RemoveAll(dir) } + +// getOwner isn't implemented for FreeBSD +func getOwner(os.FileInfo) (int, int) { + return -1, -1 +} diff --git a/client/allocdir/fs_linux.go b/client/allocdir/fs_linux.go index 7a76c5dd1a0..c6b079f867a 100644 --- a/client/allocdir/fs_linux.go +++ b/client/allocdir/fs_linux.go @@ -88,3 +88,11 @@ func removeSecretDir(dir string) error { } return os.RemoveAll(dir) } + +func getOwner(fi os.FileInfo) (int, int) { + stat, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return -1, -1 + } + return int(stat.Uid), int(stat.Gid) +} diff --git a/client/allocdir/fs_solaris.go b/client/allocdir/fs_solaris.go index 088b58f5b3f..83359b1b3aa 100644 --- a/client/allocdir/fs_solaris.go +++ b/client/allocdir/fs_solaris.go @@ -25,3 +25,8 @@ func createSecretDir(dir string) error { func removeSecretDir(dir string) error { return os.RemoveAll(dir) } + +// getOwner isn't implemented for Solaris +func getOwner(os.FileInfo) (int, int) { + return -1, -1 +} diff --git a/client/allocdir/fs_unix.go b/client/allocdir/fs_unix.go index 648fce1adb6..7e3068b3bc9 100644 --- a/client/allocdir/fs_unix.go +++ b/client/allocdir/fs_unix.go @@ -82,7 +82,7 @@ func getGid(u *user.User) (int, error) { // linkOrCopy attempts to hardlink dst to src and fallsback to copying if the // hardlink fails. -func linkOrCopy(src, dst string, perm os.FileMode) error { +func linkOrCopy(src, dst string, uid, gid int, perm os.FileMode) error { // Avoid link/copy if the file already exists in the chroot // TODO 0.6 clean this up. This was needed because chroot creation fails // when a process restarts. @@ -90,9 +90,9 @@ func linkOrCopy(src, dst string, perm os.FileMode) error { return nil } // Attempt to hardlink. - if err := os.Link(src, dst); err == nil { - return nil - } + //if err := os.Link(src, dst); err == nil { + // return nil + //} - return fileCopy(src, dst, perm) + return fileCopy(src, dst, uid, gid, perm) } diff --git a/client/allocdir/fs_windows.go b/client/allocdir/fs_windows.go index de0fcec898c..f39f2cd72f3 100644 --- a/client/allocdir/fs_windows.go +++ b/client/allocdir/fs_windows.go @@ -21,8 +21,8 @@ var ( ) // linkOrCopy is always copies dst to src on Windows. -func linkOrCopy(src, dst string, perm os.FileMode) error { - return fileCopy(src, dst, perm) +func linkOrCopy(src, dst string, uid, gid int, perm os.FileMode) error { + return fileCopy(src, dst, uid, gid, perm) } // The windows version does nothing currently. @@ -70,3 +70,8 @@ func MountSpecialDirs(taskDir string) error { func unmountSpecialDirs(taskDir string) error { return nil } + +// getOwner doesn't work on Windows as Windows doesn't use int user IDs +func getOwner(os.FileInfo) (int, int) { + return -1, -1 +} diff --git a/client/allocdir/task_dir.go b/client/allocdir/task_dir.go index 4f2817e625c..b4376057213 100644 --- a/client/allocdir/task_dir.go +++ b/client/allocdir/task_dir.go @@ -163,7 +163,8 @@ func (t *TaskDir) embedDirs(entries map[string]string) error { // Copy the file. taskEntry := filepath.Join(t.Dir, dest) - if err := linkOrCopy(source, taskEntry, s.Mode().Perm()); err != nil { + uid, gid := getOwner(s) + if err := linkOrCopy(source, taskEntry, uid, gid, s.Mode().Perm()); err != nil { return err } @@ -217,7 +218,8 @@ func (t *TaskDir) embedDirs(entries map[string]string) error { continue } - if err := linkOrCopy(hostEntry, taskEntry, entry.Mode().Perm()); err != nil { + uid, gid := getOwner(entry) + if err := linkOrCopy(hostEntry, taskEntry, uid, gid, entry.Mode().Perm()); err != nil { return err } } From 1cffc7310ee300d2eefa5dbcf236cbec9a9f072d Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 12 Apr 2017 13:58:50 -0700 Subject: [PATCH 2/4] Don't disable hardlinking! --- client/allocdir/fs_unix.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client/allocdir/fs_unix.go b/client/allocdir/fs_unix.go index 7e3068b3bc9..e3672452d83 100644 --- a/client/allocdir/fs_unix.go +++ b/client/allocdir/fs_unix.go @@ -90,9 +90,9 @@ func linkOrCopy(src, dst string, uid, gid int, perm os.FileMode) error { return nil } // Attempt to hardlink. - //if err := os.Link(src, dst); err == nil { - // return nil - //} + if err := os.Link(src, dst); err == nil { + return nil + } return fileCopy(src, dst, uid, gid, perm) } From 17471bf7c0742e982106f0e63808a6c2c54ec4ab Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Mon, 17 Apr 2017 12:29:34 -0700 Subject: [PATCH 3/4] Set ownership on directories in chroot Also support getOwner on all Unixes as they all have `Stat_t.{U,G}id` --- client/allocdir/alloc_dir.go | 34 +++++++++++++++++++++++++++------- client/allocdir/fs_darwin.go | 5 ----- client/allocdir/fs_freebsd.go | 5 ----- client/allocdir/fs_linux.go | 8 -------- client/allocdir/fs_solaris.go | 5 ----- client/allocdir/fs_unix.go | 9 +++++++++ client/allocdir/fs_windows.go | 2 +- 7 files changed, 37 insertions(+), 31 deletions(-) diff --git a/client/allocdir/alloc_dir.go b/client/allocdir/alloc_dir.go index 8c95d0c7509..dd665926112 100644 --- a/client/allocdir/alloc_dir.go +++ b/client/allocdir/alloc_dir.go @@ -17,6 +17,12 @@ import ( "github.com/hpcloud/tail/watch" ) +const ( + // idUnsupported is what the uid/gid will be set to on platforms (eg + // Windows) that don't support integer ownership identifiers. + idUnsupported = -1 +) + var ( // The name of the directory that is shared across tasks in a task group. SharedAllocName = "alloc" @@ -405,7 +411,7 @@ func fileCopy(src, dst string, uid, gid int, perm os.FileMode) error { return fmt.Errorf("Couldn't copy %q to %q: %v", src, dst, err) } - if uid >= 0 && gid >= 0 { + if uid != idUnsupported && gid != idUnsupported { if err := dstFile.Chown(uid, gid); err != nil { return fmt.Errorf("Couldn't copy %q to %q: %v", src, dst, err) } @@ -456,6 +462,12 @@ func createDir(basePath, relPath string) error { if err := os.MkdirAll(destDir, fi.Perm); err != nil { return err } + + if fi.Uid != idUnsupported && fi.Gid != idUnsupported { + if err := os.Chown(destDir, fi.Uid, fi.Gid); err != nil { + return err + } + } } return nil } @@ -464,6 +476,10 @@ func createDir(basePath, relPath string) error { type fileInfo struct { Name string Perm os.FileMode + + // Uid and Gid are unsupported on Windows + Uid int + Gid int } // splitPath stats each subdirectory of a path. The first element of the array @@ -471,17 +487,19 @@ type fileInfo struct { // path. func splitPath(path string) ([]fileInfo, error) { var mode os.FileMode - i, err := os.Stat(path) + fi, err := os.Stat(path) // If the path is not present in the host then we respond with the most // flexible permission. + uid, gid := idUnsupported, idUnsupported if err != nil { mode = os.ModePerm } else { - mode = i.Mode() + uid, gid = getOwner(fi) + mode = fi.Mode() } var dirs []fileInfo - dirs = append(dirs, fileInfo{Name: path, Perm: mode}) + dirs = append(dirs, fileInfo{Name: path, Perm: mode, Uid: uid, Gid: gid}) currentDir := path for { dir := filepath.Dir(filepath.Clean(currentDir)) @@ -491,13 +509,15 @@ func splitPath(path string) ([]fileInfo, error) { // We try to find the permission of the file in the host. If the path is not // present in the host then we respond with the most flexible permission. - i, err = os.Stat(dir) + uid, gid := idUnsupported, idUnsupported + fi, err := os.Stat(dir) if err != nil { mode = os.ModePerm } else { - mode = i.Mode() + uid, gid = getOwner(fi) + mode = fi.Mode() } - dirs = append(dirs, fileInfo{Name: dir, Perm: mode}) + dirs = append(dirs, fileInfo{Name: dir, Perm: mode, Uid: uid, Gid: gid}) currentDir = dir } return dirs, nil diff --git a/client/allocdir/fs_darwin.go b/client/allocdir/fs_darwin.go index 7cd5caf23b4..fe29791c460 100644 --- a/client/allocdir/fs_darwin.go +++ b/client/allocdir/fs_darwin.go @@ -24,8 +24,3 @@ func createSecretDir(dir string) error { func removeSecretDir(dir string) error { return os.RemoveAll(dir) } - -// getOwner isn't implemented for Darwin -func getOwner(os.FileInfo) (int, int) { - return -1, -1 -} diff --git a/client/allocdir/fs_freebsd.go b/client/allocdir/fs_freebsd.go index 0d34925a7af..fe29791c460 100644 --- a/client/allocdir/fs_freebsd.go +++ b/client/allocdir/fs_freebsd.go @@ -24,8 +24,3 @@ func createSecretDir(dir string) error { func removeSecretDir(dir string) error { return os.RemoveAll(dir) } - -// getOwner isn't implemented for FreeBSD -func getOwner(os.FileInfo) (int, int) { - return -1, -1 -} diff --git a/client/allocdir/fs_linux.go b/client/allocdir/fs_linux.go index c6b079f867a..7a76c5dd1a0 100644 --- a/client/allocdir/fs_linux.go +++ b/client/allocdir/fs_linux.go @@ -88,11 +88,3 @@ func removeSecretDir(dir string) error { } return os.RemoveAll(dir) } - -func getOwner(fi os.FileInfo) (int, int) { - stat, ok := fi.Sys().(*syscall.Stat_t) - if !ok { - return -1, -1 - } - return int(stat.Uid), int(stat.Gid) -} diff --git a/client/allocdir/fs_solaris.go b/client/allocdir/fs_solaris.go index 83359b1b3aa..088b58f5b3f 100644 --- a/client/allocdir/fs_solaris.go +++ b/client/allocdir/fs_solaris.go @@ -25,8 +25,3 @@ func createSecretDir(dir string) error { func removeSecretDir(dir string) error { return os.RemoveAll(dir) } - -// getOwner isn't implemented for Solaris -func getOwner(os.FileInfo) (int, int) { - return -1, -1 -} diff --git a/client/allocdir/fs_unix.go b/client/allocdir/fs_unix.go index e3672452d83..77447a32f7f 100644 --- a/client/allocdir/fs_unix.go +++ b/client/allocdir/fs_unix.go @@ -8,6 +8,7 @@ import ( "os/user" "path/filepath" "strconv" + "syscall" "golang.org/x/sys/unix" ) @@ -96,3 +97,11 @@ func linkOrCopy(src, dst string, uid, gid int, perm os.FileMode) error { return fileCopy(src, dst, uid, gid, perm) } + +func getOwner(fi os.FileInfo) (int, int) { + stat, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return -1, -1 + } + return int(stat.Uid), int(stat.Gid) +} diff --git a/client/allocdir/fs_windows.go b/client/allocdir/fs_windows.go index f39f2cd72f3..3667c1953bb 100644 --- a/client/allocdir/fs_windows.go +++ b/client/allocdir/fs_windows.go @@ -73,5 +73,5 @@ func unmountSpecialDirs(taskDir string) error { // getOwner doesn't work on Windows as Windows doesn't use int user IDs func getOwner(os.FileInfo) (int, int) { - return -1, -1 + return idUnsupported, idUnsupported } From 75c3863533e2f550203d139d8d752a8dbeb96080 Mon Sep 17 00:00:00 2001 From: Michael Schurter Date: Wed, 19 Apr 2017 12:37:15 -0700 Subject: [PATCH 4/4] Add #2552 / #2553 to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index de933e8a8fa..cba97c6ee28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ BUG FIXES: * client/artifact: Honor netrc [GH-2524] * client/artifact: Handle tars where file in directory is listed before directory [GH-2524] + * driver/exec: Properly set file/dir ownership in chroots [GH-2552] * server: Reject non-TLS clients when TLS enabled [GH-2525] * server: Fix a panic in plan evaluation with partial failures and all_at_once set [GH-2544]