Chown files when copying into chroot

[schmichael]

Fixes #2552

Not needed when hardlinking. Only adds Linux support but other Unixes may
be easy.

[jshuping]

Thanks for addressing this so quickly, today was my first day reading go so please bear with me!

[jshuping]

Do we want to say || here in case the perms are like root:bob?

[schmichael]

I want to make sure they're both both valid IDs. Even if it's root:bob that's uid=0, gid>0 which will pass this conditional.

[jshuping]

Were you maybe just doing this for testing / can we keep hardlinking?

[schmichael]

Wow! Huge goof on my part. Thanks for the review! Fixing.

[dadgar]

Is syscall.Stat_t not implemented on the other platforms we chroot in? We shouldn't leave the bug open on other platforms we chroot in

[sean-]

https://godoc.org/golang.org/x/sys/unix#Stat_t

[jshuping]

Not needed when hardlinking.

We tried a build with the existing patch, but even when hardlinking files, the code is creating the directories to contain these hardlinks, without honoring the ownership (you can't hardlink directories).

I think we need to check the ownership of the source directory somehwere around here: https://github.com/hashicorp/nomad/blob/master/client/allocdir/task_dir.go#L176

[jcua]

With this change, this allows ownership of directories to be preserved.

diff --git a/client/allocdir/task_dir.go b/client/allocdir/task_dir.go
index d7e5fcf..e58d21a 100644
--- a/client/allocdir/task_dir.go
+++ b/client/allocdir/task_dir.go
@@ -155,6 +155,7 @@ func (t *TaskDir) embedDirs(entries map[string]string) error {
                        continue
                }

+               uid, gid := getOwner(s)
                // Embedding a single file
                if !s.IsDir() {
                        if err := createDir(t.Dir, filepath.Dir(dest)); err != nil {
@@ -163,7 +164,6 @@ func (t *TaskDir) embedDirs(entries map[string]string) error {

                        // Copy the file.
                        taskEntry := filepath.Join(t.Dir, dest)
-                       uid, gid := getOwner(s)
                        if err := linkOrCopy(source, taskEntry, uid, gid, s.Mode().Perm()); err != nil {
                                return err
                        }
@@ -178,6 +178,12 @@ func (t *TaskDir) embedDirs(entries map[string]string) error {
                        return fmt.Errorf("Couldn't create destination directory %v: %v", destDir, err)
                }

+               if uid >= 0 && gid >= 0 {
+                       if err := os.Chown(destDir, uid, gid); err != nil {
+                               return fmt.Errorf("Couldn't change owner of directory %v: %v", destDir, err)
+                       }
+               }
+
                // Enumerate the files in source.
                dirEntries, err := ioutil.ReadDir(source)
                if err != nil {

[sean-]

This looks good except for the use of the syscall package. Instead, please find compatible syscalls from the golang.org/x/sys/unix package, which should address nearly all of the portability concerns. If you want to merge this with syscall instead, let me know and I'll follow up with a PR to use the aforementioned unix package instead.

[schmichael]

@dadgar Fixed! A quick peek reveals all Unixes we support have Stat_t.{U,G}id fields, so we can have a single getOwner implementation in fs_unix.go.

@jshuping Great catch. Fixed!

@sean- The only place we use syscall is to type assert the return value of FileInfo.Sys(). To switch to sys/unix I think we'd need to change a lot of our cross-platform os.Foo(). I hate our mess of fs_<OS>.go helper files, so any PRs to clean it up are welcome! Just noticed x/sys/unix.Stat_t is a type alias for syscall.Stat_t so sticking with the stdlib copy.

[dadgar]

LGTM

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.