From ae8c004dc18c59fec9cd67759a5e0087300e872d Mon Sep 17 00:00:00 2001 From: Phil Estes Date: Wed, 14 Oct 2015 14:35:48 -0400 Subject: [PATCH] Correct build-time directory creation with user namespaced daemon This fixes errors in ownership on directory creation during build that can cause inaccessible files depending on the paths in the Dockerfile and non-existing directories in the starting image. Add tests for the mkdir variants in pkg/idtools Docker-DCO-1.1-Signed-off-by: Phil Estes (github: estesp) --- daemon/daemonbuilder/builder.go | 4 +- pkg/chrootarchive/archive.go | 9 +- pkg/idtools/idtools.go | 11 +- pkg/idtools/idtools_unix.go | 60 ++++++ pkg/idtools/idtools_unix_test.go | 243 ++++++++++++++++++++++++ pkg/idtools/idtools_windows.go | 18 ++ pkg/idtools/usergroupadd_linux.go | 20 -- pkg/idtools/usergroupadd_unsupported.go | 16 +- 8 files changed, 340 insertions(+), 41 deletions(-) create mode 100644 pkg/idtools/idtools_unix.go create mode 100644 pkg/idtools/idtools_unix_test.go create mode 100644 pkg/idtools/idtools_windows.go diff --git a/daemon/daemonbuilder/builder.go b/daemon/daemonbuilder/builder.go index d75e9ec8aa..b8ff702119 100644 --- a/daemon/daemonbuilder/builder.go +++ b/daemon/daemonbuilder/builder.go @@ -17,10 +17,10 @@ import ( "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/httputils" + "github.com/docker/docker/pkg/idtools" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/progressreader" - "github.com/docker/docker/pkg/system" "github.com/docker/docker/pkg/urlutil" "github.com/docker/docker/registry" "github.com/docker/docker/runconfig" @@ -180,7 +180,7 @@ func (d Docker) Copy(c *daemon.Container, destPath string, src builder.FileInfo, destPath = filepath.Join(destPath, filepath.Base(srcPath)) } - if err := system.MkdirAll(filepath.Dir(destPath), 0755); err != nil { + if err := idtools.MkdirAllNewAs(filepath.Dir(destPath), 0755, rootUID, rootGID); err != nil { return err } if err := d.Archiver.CopyFileWithTar(srcPath, destPath); err != nil { diff --git a/pkg/chrootarchive/archive.go b/pkg/chrootarchive/archive.go index 8e8e15977f..a7814f5b90 100644 --- a/pkg/chrootarchive/archive.go +++ b/pkg/chrootarchive/archive.go @@ -8,7 +8,7 @@ import ( "path/filepath" "github.com/docker/docker/pkg/archive" - "github.com/docker/docker/pkg/system" + "github.com/docker/docker/pkg/idtools" ) var chrootArchiver = &archive.Archiver{Untar: Untar} @@ -41,9 +41,14 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions options.ExcludePatterns = []string{} } + rootUID, rootGID, err := idtools.GetRootUIDGID(options.UIDMaps, options.GIDMaps) + if err != nil { + return err + } + dest = filepath.Clean(dest) if _, err := os.Stat(dest); os.IsNotExist(err) { - if err := system.MkdirAll(dest, 0777); err != nil { + if err := idtools.MkdirAllNewAs(dest, 0755, rootUID, rootGID); err != nil { return err } } diff --git a/pkg/idtools/idtools.go b/pkg/idtools/idtools.go index f6671403e3..a1301ee976 100644 --- a/pkg/idtools/idtools.go +++ b/pkg/idtools/idtools.go @@ -38,13 +38,20 @@ const ( // ownership to the requested uid/gid. If the directory already exists, this // function will still change ownership to the requested uid/gid pair. func MkdirAllAs(path string, mode os.FileMode, ownerUID, ownerGID int) error { - return mkdirAs(path, mode, ownerUID, ownerGID, true) + return mkdirAs(path, mode, ownerUID, ownerGID, true, true) +} + +// MkdirAllNewAs creates a directory (include any along the path) and then modifies +// ownership ONLY of newly created directories to the requested uid/gid. If the +// directories along the path exist, no change of ownership will be performed +func MkdirAllNewAs(path string, mode os.FileMode, ownerUID, ownerGID int) error { + return mkdirAs(path, mode, ownerUID, ownerGID, true, false) } // MkdirAs creates a directory and then modifies ownership to the requested uid/gid. // If the directory already exists, this function still changes ownership func MkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int) error { - return mkdirAs(path, mode, ownerUID, ownerGID, false) + return mkdirAs(path, mode, ownerUID, ownerGID, false, true) } // GetRootUIDGID retrieves the remapped root uid/gid pair from the set of maps. diff --git a/pkg/idtools/idtools_unix.go b/pkg/idtools/idtools_unix.go new file mode 100644 index 0000000000..b57d6ef125 --- /dev/null +++ b/pkg/idtools/idtools_unix.go @@ -0,0 +1,60 @@ +// +build !windows + +package idtools + +import ( + "os" + "path/filepath" + + "github.com/docker/docker/pkg/system" +) + +func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chownExisting bool) error { + // make an array containing the original path asked for, plus (for mkAll == true) + // all path components leading up to the complete path that don't exist before we MkdirAll + // so that we can chown all of them properly at the end. If chownExisting is false, we won't + // chown the full directory path if it exists + var paths []string + if _, err := os.Stat(path); err != nil && os.IsNotExist(err) { + paths = []string{path} + } else if err == nil && chownExisting { + if err := os.Chown(path, ownerUID, ownerGID); err != nil { + return err + } + // short-circuit--we were called with an existing directory and chown was requested + return nil + } else if err == nil { + // nothing to do; directory path fully exists already and chown was NOT requested + return nil + } + + if mkAll { + // walk back to "/" looking for directories which do not exist + // and add them to the paths array for chown after creation + dirPath := path + for { + dirPath = filepath.Dir(dirPath) + if dirPath == "/" { + break + } + if _, err := os.Stat(dirPath); err != nil && os.IsNotExist(err) { + paths = append(paths, dirPath) + } + } + if err := system.MkdirAll(path, mode); err != nil && !os.IsExist(err) { + return err + } + } else { + if err := os.Mkdir(path, mode); err != nil && !os.IsExist(err) { + return err + } + } + // even if it existed, we will chown the requested path + any subpaths that + // didn't exist when we called MkdirAll + for _, pathComponent := range paths { + if err := os.Chown(pathComponent, ownerUID, ownerGID); err != nil { + return err + } + } + return nil +} diff --git a/pkg/idtools/idtools_unix_test.go b/pkg/idtools/idtools_unix_test.go new file mode 100644 index 0000000000..55b338c96e --- /dev/null +++ b/pkg/idtools/idtools_unix_test.go @@ -0,0 +1,243 @@ +// +build !windows + +package idtools + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "syscall" + "testing" +) + +type node struct { + uid int + gid int +} + +func TestMkdirAllAs(t *testing.T) { + dirName, err := ioutil.TempDir("", "mkdirall") + if err != nil { + t.Fatalf("Couldn't create temp dir: %v", err) + } + defer os.RemoveAll(dirName) + + testTree := map[string]node{ + "usr": {0, 0}, + "usr/bin": {0, 0}, + "lib": {33, 33}, + "lib/x86_64": {45, 45}, + "lib/x86_64/share": {1, 1}, + } + + if err := buildTree(dirName, testTree); err != nil { + t.Fatal(err) + } + + // test adding a directory to a pre-existing dir; only the new dir is owned by the uid/gid + if err := MkdirAllAs(filepath.Join(dirName, "usr", "share"), 0755, 99, 99); err != nil { + t.Fatal(err) + } + testTree["usr/share"] = node{99, 99} + verifyTree, err := readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } + + // test 2-deep new directories--both should be owned by the uid/gid pair + if err := MkdirAllAs(filepath.Join(dirName, "lib", "some", "other"), 0755, 101, 101); err != nil { + t.Fatal(err) + } + testTree["lib/some"] = node{101, 101} + testTree["lib/some/other"] = node{101, 101} + verifyTree, err = readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } + + // test a directory that already exists; should be chowned, but nothing else + if err := MkdirAllAs(filepath.Join(dirName, "usr"), 0755, 102, 102); err != nil { + t.Fatal(err) + } + testTree["usr"] = node{102, 102} + verifyTree, err = readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } +} + +func TestMkdirAllNewAs(t *testing.T) { + + dirName, err := ioutil.TempDir("", "mkdirnew") + if err != nil { + t.Fatalf("Couldn't create temp dir: %v", err) + } + defer os.RemoveAll(dirName) + + testTree := map[string]node{ + "usr": {0, 0}, + "usr/bin": {0, 0}, + "lib": {33, 33}, + "lib/x86_64": {45, 45}, + "lib/x86_64/share": {1, 1}, + } + + if err := buildTree(dirName, testTree); err != nil { + t.Fatal(err) + } + + // test adding a directory to a pre-existing dir; only the new dir is owned by the uid/gid + if err := MkdirAllNewAs(filepath.Join(dirName, "usr", "share"), 0755, 99, 99); err != nil { + t.Fatal(err) + } + testTree["usr/share"] = node{99, 99} + verifyTree, err := readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } + + // test 2-deep new directories--both should be owned by the uid/gid pair + if err := MkdirAllNewAs(filepath.Join(dirName, "lib", "some", "other"), 0755, 101, 101); err != nil { + t.Fatal(err) + } + testTree["lib/some"] = node{101, 101} + testTree["lib/some/other"] = node{101, 101} + verifyTree, err = readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } + + // test a directory that already exists; should NOT be chowned + if err := MkdirAllNewAs(filepath.Join(dirName, "usr"), 0755, 102, 102); err != nil { + t.Fatal(err) + } + verifyTree, err = readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } +} + +func TestMkdirAs(t *testing.T) { + + dirName, err := ioutil.TempDir("", "mkdir") + if err != nil { + t.Fatalf("Couldn't create temp dir: %v", err) + } + defer os.RemoveAll(dirName) + + testTree := map[string]node{ + "usr": {0, 0}, + } + if err := buildTree(dirName, testTree); err != nil { + t.Fatal(err) + } + + // test a directory that already exists; should just chown to the requested uid/gid + if err := MkdirAs(filepath.Join(dirName, "usr"), 0755, 99, 99); err != nil { + t.Fatal(err) + } + testTree["usr"] = node{99, 99} + verifyTree, err := readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } + + // create a subdir under a dir which doesn't exist--should fail + if err := MkdirAs(filepath.Join(dirName, "usr", "bin", "subdir"), 0755, 102, 102); err == nil { + t.Fatalf("Trying to create a directory with Mkdir where the parent doesn't exist should have failed") + } + + // create a subdir under an existing dir; should only change the ownership of the new subdir + if err := MkdirAs(filepath.Join(dirName, "usr", "bin"), 0755, 102, 102); err != nil { + t.Fatal(err) + } + testTree["usr/bin"] = node{102, 102} + verifyTree, err = readTree(dirName, "") + if err != nil { + t.Fatal(err) + } + if err := compareTrees(testTree, verifyTree); err != nil { + t.Fatal(err) + } +} + +func buildTree(base string, tree map[string]node) error { + for path, node := range tree { + fullPath := filepath.Join(base, path) + if err := os.MkdirAll(fullPath, 0755); err != nil { + return fmt.Errorf("Couldn't create path: %s; error: %v", fullPath, err) + } + if err := os.Chown(fullPath, node.uid, node.gid); err != nil { + return fmt.Errorf("Couldn't chown path: %s; error: %v", fullPath, err) + } + } + return nil +} + +func readTree(base, root string) (map[string]node, error) { + tree := make(map[string]node) + + dirInfos, err := ioutil.ReadDir(base) + if err != nil { + return nil, fmt.Errorf("Couldn't read directory entries for %q: %v", base, err) + } + + for _, info := range dirInfos { + s := &syscall.Stat_t{} + if err := syscall.Stat(filepath.Join(base, info.Name()), s); err != nil { + return nil, fmt.Errorf("Can't stat file %q: %v", filepath.Join(base, info.Name()), err) + } + tree[filepath.Join(root, info.Name())] = node{int(s.Uid), int(s.Gid)} + if info.IsDir() { + // read the subdirectory + subtree, err := readTree(filepath.Join(base, info.Name()), filepath.Join(root, info.Name())) + if err != nil { + return nil, err + } + for path, nodeinfo := range subtree { + tree[path] = nodeinfo + } + } + } + return tree, nil +} + +func compareTrees(left, right map[string]node) error { + if len(left) != len(right) { + return fmt.Errorf("Trees aren't the same size") + } + for path, nodeLeft := range left { + if nodeRight, ok := right[path]; ok { + if nodeRight.uid != nodeLeft.uid || nodeRight.gid != nodeLeft.gid { + // mismatch + return fmt.Errorf("mismatched ownership for %q: expected: %d:%d, got: %d:%d", path, + nodeLeft.uid, nodeLeft.gid, nodeRight.uid, nodeRight.gid) + } + continue + } + return fmt.Errorf("right tree didn't contain path %q", path) + } + return nil +} diff --git a/pkg/idtools/idtools_windows.go b/pkg/idtools/idtools_windows.go new file mode 100644 index 0000000000..c9e3c937cd --- /dev/null +++ b/pkg/idtools/idtools_windows.go @@ -0,0 +1,18 @@ +// +build windows + +package idtools + +import ( + "os" + + "github.com/docker/docker/pkg/system" +) + +// Platforms such as Windows do not support the UID/GID concept. So make this +// just a wrapper around system.MkdirAll. +func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll, chownExisting bool) error { + if err := system.MkdirAll(path, mode); err != nil && !os.IsExist(err) { + return err + } + return nil +} diff --git a/pkg/idtools/usergroupadd_linux.go b/pkg/idtools/usergroupadd_linux.go index c65fc33687..c1eedff104 100644 --- a/pkg/idtools/usergroupadd_linux.go +++ b/pkg/idtools/usergroupadd_linux.go @@ -2,13 +2,10 @@ package idtools import ( "fmt" - "os" "os/exec" "path/filepath" "strings" "syscall" - - "github.com/docker/docker/pkg/system" ) // add a user and/or group to Linux /etc/passwd, /etc/group using standard @@ -156,20 +153,3 @@ func findUnused(file string, id int) (int, error) { } } } - -func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll bool) error { - if mkAll { - if err := system.MkdirAll(path, mode); err != nil && !os.IsExist(err) { - return err - } - } else { - if err := os.Mkdir(path, mode); err != nil && !os.IsExist(err) { - return err - } - } - // even if it existed, we will chown to change ownership as requested - if err := os.Chown(path, ownerUID, ownerGID); err != nil { - return err - } - return nil -} diff --git a/pkg/idtools/usergroupadd_unsupported.go b/pkg/idtools/usergroupadd_unsupported.go index 2ec21fd675..d98b354cbd 100644 --- a/pkg/idtools/usergroupadd_unsupported.go +++ b/pkg/idtools/usergroupadd_unsupported.go @@ -2,12 +2,7 @@ package idtools -import ( - "fmt" - "os" - - "github.com/docker/docker/pkg/system" -) +import "fmt" // AddNamespaceRangesUser takes a name and finds an unused uid, gid pair // and calls the appropriate helper function to add the group and then @@ -15,12 +10,3 @@ import ( func AddNamespaceRangesUser(name string) (int, int, error) { return -1, -1, fmt.Errorf("No support for adding users or groups on this OS") } - -// Platforms such as Windows do not support the UID/GID concept. So make this -// just a wrapper around system.MkdirAll. -func mkdirAs(path string, mode os.FileMode, ownerUID, ownerGID int, mkAll bool) error { - if err := system.MkdirAll(path, mode); err != nil && !os.IsExist(err) { - return err - } - return nil -}