From c0a7873e2c557a89e619a286846db4b29a6b57ca Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 11 Dec 2020 13:24:04 -0500 Subject: [PATCH 1/4] copier: Put: skip entries with zero-length names When extracting archives, if we encounter an entry with a zero-length name, skip it, instead of attempting to Join the entry's name with the name of the parent directory where it should be placed (which just yields the parent directory's name again), and attempting to create that directory again, possibly as a file. Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/copier/copier.go b/copier/copier.go index 84b636202a4..d288cbe7af5 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -1360,6 +1360,11 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM tr := tar.NewReader(bulkReader) hdr, err := tr.Next() for err == nil { + if len(hdr.Name) == 0 { + // no name -> ignore the entry + hdr, err = tr.Next() + continue + } // figure out who should own this new item if idMappings != nil && !idMappings.Empty() { containerPair := idtools.IDPair{UID: hdr.Uid, GID: hdr.Gid} From 26b50dc1eb1f855858f83aeb5449de74b6e0acf0 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Fri, 11 Dec 2020 13:22:39 -0500 Subject: [PATCH 2/4] copier: handle replacing directories with not-directories Improve handling of cases where extracting an archive requires us to replace a directory with something that is not a directory, or vice- versa: * when replacing a directory with something that isn't a directory, remove the directory even if it has contents * don't fail when replacing something that isn't a directory with a directory Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 14 +++++++-- tests/conformance/conformance_test.go | 27 +++++++++++++----- .../testdata/add/dir-not-dir/Dockerfile | 3 ++ .../testdata/add/dir-not-dir/test.tar | Bin 0 -> 5632 bytes .../testdata/add/not-dir-dir/Dockerfile | 4 +++ .../testdata/add/not-dir-dir/test.tar | Bin 0 -> 10240 bytes .../add/populated-dir-not-dir/Dockerfile | 5 ++++ .../add/populated-dir-not-dir/test.tar | Bin 0 -> 10240 bytes 8 files changed, 43 insertions(+), 10 deletions(-) create mode 100644 tests/conformance/testdata/add/dir-not-dir/Dockerfile create mode 100644 tests/conformance/testdata/add/dir-not-dir/test.tar create mode 100644 tests/conformance/testdata/add/not-dir-dir/Dockerfile create mode 100644 tests/conformance/testdata/add/not-dir-dir/test.tar create mode 100644 tests/conformance/testdata/add/populated-dir-not-dir/Dockerfile create mode 100644 tests/conformance/testdata/add/populated-dir-not-dir/test.tar diff --git a/copier/copier.go b/copier/copier.go index d288cbe7af5..bf2d8854c7e 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -1311,8 +1311,8 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM createFile := func(path string, tr *tar.Reader) (int64, error) { f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|os.O_EXCL, 0600) if err != nil && os.IsExist(err) { - if err = os.Remove(path); err != nil { - return 0, errors.Wrapf(err, "copier: put: error removing file to be overwritten %q", path) + if err = os.RemoveAll(path); err != nil { + return 0, errors.Wrapf(err, "copier: put: error removing item to be overwritten %q", path) } f, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|os.O_EXCL, 0600) } @@ -1445,7 +1445,15 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM } case tar.TypeDir: if err = os.Mkdir(path, 0700); err != nil && os.IsExist(err) { - err = nil + var st os.FileInfo + if st, err = os.Stat(path); err == nil && !st.IsDir() { + // it's not a directory, so remove it and mkdir + if err = os.Remove(path); err == nil { + err = os.Mkdir(path, 0700) + } + } + // either we removed it and retried, or it was a directory, + // in which case we want to just add the new stuff under it } // make a note of the directory's times. we // might create items under it, which will diff --git a/tests/conformance/conformance_test.go b/tests/conformance/conformance_test.go index 5238c8ec08f..266b301dd94 100644 --- a/tests/conformance/conformance_test.go +++ b/tests/conformance/conformance_test.go @@ -928,8 +928,8 @@ func applyLayerToFSTree(t *testing.T, layer *Layer, root *FSEntry) { delete(dirEntry.Children, strings.TrimPrefix(base, ".wh.")) continue } - // if the item already exists, just make sure we're not trying - // to turn a directory into a non-directory or vice-versa + // if the item already exists, make sure we don't get confused + // by replacing a directory with a non-directory or vice-versa if child, ok := dirEntry.Children[base]; ok { if child.Children != nil { // it's a directory @@ -939,8 +939,7 @@ func applyLayerToFSTree(t *testing.T, layer *Layer, root *FSEntry) { child.FSHeader = entry continue } - // oh no - require.Failf(t, "layer diff error", "layer diff %q includes entry for directory %q, but it already exists and is not a directory", layer.UncompressedDigest, entry.Name) + // nope, a directory no longer } else { // it's not a directory if entry.Typeflag != tar.TypeDir { @@ -949,11 +948,10 @@ func applyLayerToFSTree(t *testing.T, layer *Layer, root *FSEntry) { dirEntry.Children[base].FSHeader = entry continue } - // oh no - require.Failf(t, "layer diff error", "layer diff %q includes entry for non-directory %q, but it already exists and is a directory", layer.UncompressedDigest, entry.Name) + // well, it's a directory now } } - // the item doesn't already exist, so we need to add it + // the item doesn't already exist, or it needs to be replaced, so we need to add it var children map[string]*FSEntry if entry.Typeflag == tar.TypeDir { // only directory entries can hold items @@ -2647,6 +2645,21 @@ var internalTestCases = []testCase{ fsSkip: []string{"(dir):sub:mtime"}, }, + { + name: "add-dir-not-dir", + contextDir: "add/dir-not-dir", + }, + + { + name: "add-not-dir-dir", + contextDir: "add/not-dir-dir", + }, + + { + name: "add-populated-dir-not-dir", + contextDir: "add/populated-dir-not-dir", + }, + { name: "dockerignore-allowlist-subdir-nofile-dir", contextDir: "dockerignore/allowlist/subdir-nofile", diff --git a/tests/conformance/testdata/add/dir-not-dir/Dockerfile b/tests/conformance/testdata/add/dir-not-dir/Dockerfile new file mode 100644 index 00000000000..d64852ff030 --- /dev/null +++ b/tests/conformance/testdata/add/dir-not-dir/Dockerfile @@ -0,0 +1,3 @@ +FROM busybox +RUN mkdir fileone +ADD test.tar / diff --git a/tests/conformance/testdata/add/dir-not-dir/test.tar b/tests/conformance/testdata/add/dir-not-dir/test.tar new file mode 100644 index 0000000000000000000000000000000000000000..a453463fbdc421135c7b322727cfcbe62084d7fc GIT binary patch literal 5632 zcmeH{-wJ{-6vp=`=M6gh=dOc{gs=;Rt|JP9P${F=&jmB=LepGSoH5wg^xO7*`;9UG z1vz4%6d)oun%mYE*WUYFnm`#{RZ)$bKpF<$h|fB~aBV#9@+#Gnhxqu$KaT{L2nl=JU)8_O&hD z{NoeD4kbl4c literal 0 HcmV?d00001 diff --git a/tests/conformance/testdata/add/not-dir-dir/Dockerfile b/tests/conformance/testdata/add/not-dir-dir/Dockerfile new file mode 100644 index 00000000000..5cf8016fca5 --- /dev/null +++ b/tests/conformance/testdata/add/not-dir-dir/Dockerfile @@ -0,0 +1,4 @@ +FROM busybox +RUN touch /new-directory +ADD test.tar / +RUN ls -ld /new-directory diff --git a/tests/conformance/testdata/add/not-dir-dir/test.tar b/tests/conformance/testdata/add/not-dir-dir/test.tar new file mode 100644 index 0000000000000000000000000000000000000000..80c397220f4341fda62ec95b91d3078887bd9001 GIT binary patch literal 10240 zcmeIuK?=e!5QX76N-iKK({$!MMY||S3TZ{Wy%iBkH(h1-zs*aS5QeXeugiU^<87Vm z`zlQmx48GRRrX}R*8J^%Bz11tRXd9klbWf7zR5O+PoK*=)DWZ$kEtAH`|H_!|CTcX z2q1s}0tg_000IagfB*srAb dirone/onefile.txt +RUN echo two > dirone/twofile.txt +ADD test.tar / diff --git a/tests/conformance/testdata/add/populated-dir-not-dir/test.tar b/tests/conformance/testdata/add/populated-dir-not-dir/test.tar new file mode 100644 index 0000000000000000000000000000000000000000..4cd067cedaae4e5e6e640c8581c32c099fc8d15f GIT binary patch literal 10240 zcmeIuOA5j;6a~;MB}+(RNY+!(K|v~1-M^_it`j>ucaTT8utG&Lz6~Yt6_}V zbNkHl-fz{GkWanUK9 Date: Mon, 14 Dec 2020 17:21:16 -0500 Subject: [PATCH 3/4] copier: add PutOptions.NoOverwriteDirNonDir, Get/PutOptions.Rename Add a flag for controlling overwriting-directories-with-non-directories behavior in PutOptions, and fields for controlling name mappings to both GetOptions and PutOptions. Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 120 +++++++++++++++---- copier/copier_test.go | 265 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 361 insertions(+), 24 deletions(-) diff --git a/copier/copier.go b/copier/copier.go index bf2d8854c7e..39d6521828d 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -10,6 +10,7 @@ import ( "net" "os" "os/user" + "path" "path/filepath" "strconv" "strings" @@ -229,18 +230,19 @@ func Stat(root string, directory string, options StatOptions, globs []string) ([ // GetOptions controls parts of Get()'s behavior. type GetOptions struct { - UIDMap, GIDMap []idtools.IDMap // map from hostIDs to containerIDs in the output archive - Excludes []string // contents to pretend don't exist, using the OS-specific path separator - ExpandArchives bool // extract the contents of named items that are archives - ChownDirs *idtools.IDPair // set ownership on directories. no effect on archives being extracted - ChmodDirs *os.FileMode // set permissions on directories. no effect on archives being extracted - ChownFiles *idtools.IDPair // set ownership of files. no effect on archives being extracted - ChmodFiles *os.FileMode // set permissions on files. no effect on archives being extracted - StripSetuidBit bool // strip the setuid bit off of items being copied. no effect on archives being extracted - StripSetgidBit bool // strip the setgid bit off of items being copied. no effect on archives being extracted - StripStickyBit bool // strip the sticky bit off of items being copied. no effect on archives being extracted - StripXattrs bool // don't record extended attributes of items being copied. no effect on archives being extracted - KeepDirectoryNames bool // don't strip the top directory's basename from the paths of items in subdirectories + UIDMap, GIDMap []idtools.IDMap // map from hostIDs to containerIDs in the output archive + Excludes []string // contents to pretend don't exist, using the OS-specific path separator + ExpandArchives bool // extract the contents of named items that are archives + ChownDirs *idtools.IDPair // set ownership on directories. no effect on archives being extracted + ChmodDirs *os.FileMode // set permissions on directories. no effect on archives being extracted + ChownFiles *idtools.IDPair // set ownership of files. no effect on archives being extracted + ChmodFiles *os.FileMode // set permissions on files. no effect on archives being extracted + StripSetuidBit bool // strip the setuid bit off of items being copied. no effect on archives being extracted + StripSetgidBit bool // strip the setgid bit off of items being copied. no effect on archives being extracted + StripStickyBit bool // strip the sticky bit off of items being copied. no effect on archives being extracted + StripXattrs bool // don't record extended attributes of items being copied. no effect on archives being extracted + KeepDirectoryNames bool // don't strip the top directory's basename from the paths of items in subdirectories + Rename map[string]string // rename items with the specified names, or under the specified names } // Get produces an archive containing items that match the specified glob @@ -278,15 +280,17 @@ func Get(root string, directory string, options GetOptions, globs []string, bulk // PutOptions controls parts of Put()'s behavior. type PutOptions struct { - UIDMap, GIDMap []idtools.IDMap // map from containerIDs to hostIDs when writing contents to disk - DefaultDirOwner *idtools.IDPair // set ownership of implicitly-created directories, default is ChownDirs, or 0:0 if ChownDirs not set - DefaultDirMode *os.FileMode // set permissions on implicitly-created directories, default is ChmodDirs, or 0755 if ChmodDirs not set - ChownDirs *idtools.IDPair // set ownership of newly-created directories - ChmodDirs *os.FileMode // set permissions on newly-created directories - ChownFiles *idtools.IDPair // set ownership of newly-created files - ChmodFiles *os.FileMode // set permissions on newly-created files - StripXattrs bool // don't bother trying to set extended attributes of items being copied - IgnoreXattrErrors bool // ignore any errors encountered when attempting to set extended attributes + UIDMap, GIDMap []idtools.IDMap // map from containerIDs to hostIDs when writing contents to disk + DefaultDirOwner *idtools.IDPair // set ownership of implicitly-created directories, default is ChownDirs, or 0:0 if ChownDirs not set + DefaultDirMode *os.FileMode // set permissions on implicitly-created directories, default is ChmodDirs, or 0755 if ChmodDirs not set + ChownDirs *idtools.IDPair // set ownership of newly-created directories + ChmodDirs *os.FileMode // set permissions on newly-created directories + ChownFiles *idtools.IDPair // set ownership of newly-created files + ChmodFiles *os.FileMode // set permissions on newly-created files + StripXattrs bool // don't bother trying to set extended attributes of items being copied + IgnoreXattrErrors bool // ignore any errors encountered when attempting to set extended attributes + NoOverwriteDirNonDir bool // instead of quietly overwriting directories with non-directories, return an error + Rename map[string]string // rename items with the specified names, or under the specified names } // Put extracts an archive from the bulkReader at the specified directory. @@ -1118,6 +1122,34 @@ func copierHandlerGet(bulkWriter io.Writer, req request, pm *fileutils.PatternMa return &response{Stat: statResponse.Stat, Get: getResponse{}}, cb, nil } +func handleRename(rename map[string]string, name string) string { + if rename == nil { + return name + } + // header names always use '/', so use path instead of filepath to manipulate it + if directMapping, ok := rename[name]; ok { + return directMapping + } + prefix, remainder := path.Split(name) + for prefix != "" { + if mappedPrefix, ok := rename[prefix]; ok { + return path.Join(mappedPrefix, remainder) + } + if prefix[len(prefix)-1] == '/' { + if mappedPrefix, ok := rename[prefix[:len(prefix)-1]]; ok { + return path.Join(mappedPrefix, remainder) + } + } + newPrefix, middlePart := path.Split(prefix) + if newPrefix == prefix { + return name + } + prefix = newPrefix + remainder = path.Join(middlePart, remainder) + } + return name +} + func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath string, options GetOptions, tw *tar.Writer, hardlinkChecker *util.HardlinkChecker, idMappings *idtools.IDMappings) error { // build the header using the name provided hdr, err := tar.FileInfoHeader(srcfi, symlinkTarget) @@ -1127,6 +1159,9 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str if name != "" { hdr.Name = filepath.ToSlash(name) } + if options.Rename != nil { + hdr.Name = handleRename(options.Rename, hdr.Name) + } if options.StripSetuidBit { hdr.Mode &^= cISUID } @@ -1164,6 +1199,9 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str tr := tar.NewReader(rc) hdr, err := tr.Next() for err == nil { + if options.Rename != nil { + hdr.Name = handleRename(options.Rename, hdr.Name) + } if err = tw.WriteHeader(hdr); err != nil { return errors.Wrapf(err, "error writing tar header from %q to pipe", contentPath) } @@ -1311,6 +1349,11 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM createFile := func(path string, tr *tar.Reader) (int64, error) { f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC|os.O_EXCL, 0600) if err != nil && os.IsExist(err) { + if req.PutOptions.NoOverwriteDirNonDir { + if st, err2 := os.Lstat(path); err2 == nil && st.IsDir() { + return 0, errors.Wrapf(err, "copier: put: error creating file at %q", path) + } + } if err = os.RemoveAll(path); err != nil { return 0, errors.Wrapf(err, "copier: put: error removing item to be overwritten %q", path) } @@ -1365,6 +1408,9 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM hdr, err = tr.Next() continue } + if req.PutOptions.Rename != nil { + hdr.Name = handleRename(req.PutOptions.Rename, hdr.Name) + } // figure out who should own this new item if idMappings != nil && !idMappings.Empty() { containerPair := idtools.IDPair{UID: hdr.Uid, GID: hdr.Gid} @@ -1417,28 +1463,55 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM } case tar.TypeLink: var linkTarget string + if req.PutOptions.Rename != nil { + hdr.Linkname = handleRename(req.PutOptions.Rename, hdr.Linkname) + } if linkTarget, err = resolvePath(targetDirectory, filepath.Join(req.Root, filepath.FromSlash(hdr.Linkname)), nil); err != nil { return errors.Errorf("error resolving hardlink target path %q under root %q", hdr.Linkname, req.Root) } if err = os.Link(linkTarget, path); err != nil && os.IsExist(err) { + if req.PutOptions.NoOverwriteDirNonDir { + if st, err := os.Lstat(path); err == nil && st.IsDir() { + break + } + } if err = os.Remove(path); err == nil { err = os.Link(linkTarget, path) } } case tar.TypeSymlink: + // if req.PutOptions.Rename != nil { + // todo: the general solution requires resolving to an absolute path, handling + // renaming, and then possibly converting back to a relative symlink + // } if err = os.Symlink(filepath.FromSlash(hdr.Linkname), filepath.FromSlash(path)); err != nil && os.IsExist(err) { + if req.PutOptions.NoOverwriteDirNonDir { + if st, err := os.Lstat(path); err == nil && st.IsDir() { + break + } + } if err = os.Remove(path); err == nil { err = os.Symlink(filepath.FromSlash(hdr.Linkname), filepath.FromSlash(path)) } } case tar.TypeChar: if err = mknod(path, chrMode(0600), int(mkdev(devMajor, devMinor))); err != nil && os.IsExist(err) { + if req.PutOptions.NoOverwriteDirNonDir { + if st, err := os.Lstat(path); err == nil && st.IsDir() { + break + } + } if err = os.Remove(path); err == nil { err = mknod(path, chrMode(0600), int(mkdev(devMajor, devMinor))) } } case tar.TypeBlock: if err = mknod(path, blkMode(0600), int(mkdev(devMajor, devMinor))); err != nil && os.IsExist(err) { + if req.PutOptions.NoOverwriteDirNonDir { + if st, err := os.Lstat(path); err == nil && st.IsDir() { + break + } + } if err = os.Remove(path); err == nil { err = mknod(path, blkMode(0600), int(mkdev(devMajor, devMinor))) } @@ -1466,6 +1539,11 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM }) case tar.TypeFifo: if err = mkfifo(path, 0600); err != nil && os.IsExist(err) { + if req.PutOptions.NoOverwriteDirNonDir { + if st, err := os.Lstat(path); err == nil && st.IsDir() { + break + } + } if err = os.Remove(path); err == nil { err = mkfifo(path, 0600) } diff --git a/copier/copier_test.go b/copier/copier_test.go index ac1f2fb9812..a2da223def1 100644 --- a/copier/copier_test.go +++ b/copier/copier_test.go @@ -196,6 +196,11 @@ var ( excludes []string expectedGetErrors []expectedError subdirContents map[string][]string + renames []struct { + name string + renames map[string]string + expected []string + } }{ { name: "regular", @@ -244,6 +249,156 @@ var ( {inSubdir: true, name: "subdir-a/file-b", err: syscall.ENOENT}, {inSubdir: true, name: "subdir-a/file-c", err: syscall.ENOENT}, }, + renames: []struct { + name string + renames map[string]string + expected []string + }{ + { + name: "no-match-dir", + renames: map[string]string{"subdir-z": "subdir-y"}, + expected: []string{ + "file-0", + "file-a", + "file-b", + "file-c", + "file-u", + "file-g", + "file-t", + "link-0", + "link-a", + "link-b", + "hlink-0", + "hlink-a", + "hlink-b", + "subdir-a", + "subdir-a/file-n", + "subdir-a/file-o", + "subdir-a/file-a", + "subdir-a/file-b", + "subdir-a/file-c", + "subdir-b", + "subdir-b/file-n", + "subdir-b/file-o", + "subdir-c", + "subdir-c/file-n", + "subdir-c/file-o", + "subdir-d", + "subdir-d/hlink-0", + "subdir-d/hlink-a", + "subdir-d/hlink-b", + "archive-a", + }, + }, + { + name: "no-match-file", + renames: map[string]string{"file-n": "file-z"}, + expected: []string{ + "file-0", + "file-a", + "file-b", + "file-c", + "file-u", + "file-g", + "file-t", + "link-0", + "link-a", + "link-b", + "hlink-0", + "hlink-a", + "hlink-b", + "subdir-a", + "subdir-a/file-n", + "subdir-a/file-o", + "subdir-a/file-a", + "subdir-a/file-b", + "subdir-a/file-c", + "subdir-b", + "subdir-b/file-n", + "subdir-b/file-o", + "subdir-c", + "subdir-c/file-n", + "subdir-c/file-o", + "subdir-d", + "subdir-d/hlink-0", + "subdir-d/hlink-a", + "subdir-d/hlink-b", + "archive-a", + }, + }, + { + name: "directory", + renames: map[string]string{"subdir-a": "subdir-z"}, + expected: []string{ + "file-0", + "file-a", + "file-b", + "file-c", + "file-u", + "file-g", + "file-t", + "link-0", + "link-a", + "link-b", + "hlink-0", + "hlink-a", + "hlink-b", + "subdir-z", + "subdir-z/file-n", + "subdir-z/file-o", + "subdir-z/file-a", + "subdir-z/file-b", + "subdir-z/file-c", + "subdir-b", + "subdir-b/file-n", + "subdir-b/file-o", + "subdir-c", + "subdir-c/file-n", + "subdir-c/file-o", + "subdir-d", + "subdir-d/hlink-0", + "subdir-d/hlink-a", + "subdir-d/hlink-b", + "archive-a", + }, + }, + { + name: "file-in-directory", + renames: map[string]string{"subdir-a/file-n": "subdir-a/file-z"}, + expected: []string{ + "file-0", + "file-a", + "file-b", + "file-c", + "file-u", + "file-g", + "file-t", + "link-0", + "link-a", + "link-b", + "hlink-0", + "hlink-a", + "hlink-b", + "subdir-a", + "subdir-a/file-z", + "subdir-a/file-o", + "subdir-a/file-a", + "subdir-a/file-b", + "subdir-a/file-c", + "subdir-b", + "subdir-b/file-n", + "subdir-b/file-o", + "subdir-c", + "subdir-c/file-n", + "subdir-c/file-o", + "subdir-d", + "subdir-d/hlink-0", + "subdir-d/hlink-a", + "subdir-d/hlink-b", + "archive-a", + }, + }, + }, }, { name: "devices", @@ -271,9 +426,12 @@ func TestPutChroot(t *testing.T) { } func testPut(t *testing.T) { - for _, topdir := range []string{"", ".", "top"} { - for i := range testArchives { - t.Run(fmt.Sprintf("topdir=%s,archive=%s", topdir, testArchives[i].name), func(t *testing.T) { + uidMap := []idtools.IDMap{{HostID: os.Getuid(), ContainerID: 0, Size: 1}} + gidMap := []idtools.IDMap{{HostID: os.Getgid(), ContainerID: 0, Size: 1}} + + for i := range testArchives { + for _, topdir := range []string{"", ".", "top"} { + t.Run(fmt.Sprintf("archive=%s,topdir=%s", testArchives[i].name, topdir), func(t *testing.T) { if uid != 0 && testArchives[i].rootOnly { t.Skipf("test archive %q can only be tested with root privileges, skipping", testArchives[i].name) } @@ -333,7 +491,69 @@ func testPut(t *testing.T) { } }) } + + for _, renames := range testArchives[i].renames { + t.Run(fmt.Sprintf("archive=%s,rename=%s", testArchives[i].name, renames.name), func(t *testing.T) { + if uid != 0 && testArchives[i].rootOnly { + t.Skipf("test archive %q can only be tested with root privileges, skipping", testArchives[i].name) + } + + tmp, err := ioutil.TempDir("", "copier-test-") + require.Nil(t, err, "error creating temporary directory") + defer os.RemoveAll(tmp) + + archive := makeArchive(testArchives[i].headers, testArchives[i].contents) + err = Put(tmp, tmp, PutOptions{UIDMap: uidMap, GIDMap: gidMap, Rename: renames.renames}, archive) + require.Nil(t, err, "error extracting archive %q to directory %q", testArchives[i].name, tmp) + + var found []string + err = filepath.Walk(tmp, func(path string, info os.FileInfo, err error) error { + if info == nil || err != nil { + return err + } + rel, err := filepath.Rel(tmp, path) + if err != nil { + return err + } + if rel == "." { + return nil + } + found = append(found, rel) + return nil + }) + require.Nil(t, err, "error walking context directory for archive %q under %q", testArchives[i].name, tmp) + sort.Strings(found) + + expected := renames.expected + sort.Strings(expected) + assert.Equal(t, expected, found, "renaming did not work as expected") + }) + } } + + for _, overwrite := range []bool{false, true} { + for _, typeFlag := range []byte{tar.TypeReg, tar.TypeLink, tar.TypeSymlink, tar.TypeChar, tar.TypeBlock, tar.TypeFifo} { + t.Run(fmt.Sprintf("overwrite=%v,type=%c", overwrite, typeFlag), func(t *testing.T) { + archive := makeArchiveSlice([]tar.Header{ + {Name: "target", Typeflag: tar.TypeReg, Size: 123, Mode: 0755, ModTime: testDate}, + {Name: "test", Typeflag: tar.TypeDir, Size: 0, Mode: 0755, ModTime: testDate}, + {Name: "test", Typeflag: typeFlag, Size: 0, Mode: 0755, Linkname: "target", ModTime: testDate}, + }) + tmp, err := ioutil.TempDir("", "copier-test-") + require.Nil(t, err, "error creating temporary directory") + defer os.RemoveAll(tmp) + err = Put(tmp, tmp, PutOptions{UIDMap: uidMap, GIDMap: gidMap, NoOverwriteDirNonDir: !overwrite}, bytes.NewReader(archive)) + if overwrite { + if unwrapError(err) != syscall.EPERM { + assert.Nilf(t, err, "expected to overwrite directory with type %c: %v", typeFlag, err) + } + } else { + assert.NotNilf(t, err, "expected an error trying to overwrite directory with type %c", typeFlag) + } + }) + } + } + } func isExpectedError(err error, inSubdir bool, name string, expectedErrors []expectedError) bool { @@ -609,6 +829,7 @@ func testGetMultiple(t *testing.T) { stripStickyBit bool stripXattrs bool keepDirectoryNames bool + renames map[string]string } var getTestArchives = []struct { name string @@ -1001,6 +1222,43 @@ func testGetMultiple(t *testing.T) { "file-o", }, }, + { + name: "wildcard-with-rename", + pattern: "*-a", + keepDirectoryNames: false, + renames: map[string]string{"file-a": "renamed"}, + items: []string{ + "renamed", // from file-a + "link-a", + "archive-a", + "non-archive-a", + "something-a", + "file-n", // from subdir-a + "file-o", // from subdir-a + "renamed", // from subdir-a/file-a -> file-a -> renamed + "file-b", // from subdir-a + "file-c", // from subdir-a + }, + }, + { + name: "wildcard-with-rename-keep", + pattern: "*-a", + keepDirectoryNames: true, + renames: map[string]string{"subdir-a": "subdir-b"}, + items: []string{ + "file-a", + "link-a", + "archive-a", + "non-archive-a", + "something-a", + "subdir-b", + "subdir-b/file-n", + "subdir-b/file-o", + "subdir-b/file-a", + "subdir-b/file-b", + "subdir-b/file-c", + }, + }, }, }, } @@ -1035,6 +1293,7 @@ func testGetMultiple(t *testing.T) { StripStickyBit: testCase.stripStickyBit, StripXattrs: testCase.stripXattrs, KeepDirectoryNames: testCase.keepDirectoryNames, + Rename: testCase.renames, } t.Run(fmt.Sprintf("topdir=%s,archive=%s,case=%s,pattern=%s", topdir, testArchive.name, testCase.name, testCase.pattern), func(t *testing.T) { From 9b3e7b1f02c13241c853e295814827372abd8df8 Mon Sep 17 00:00:00 2001 From: Nalin Dahyabhai Date: Wed, 16 Dec 2020 10:26:59 -0500 Subject: [PATCH 4/4] copier: don't assume we can chroot() on Unixy systems Fall back to non-chroot behavior on Unixy systems when we're not started as UID 0. Break the unit tests that exercise chroot functionality into a separate file so that we don't even try to run those cases on non-Unix OSes. Signed-off-by: Nalin Dahyabhai --- copier/copier.go | 54 +++++++++++++++++++----------------- copier/copier_test.go | 35 ----------------------- copier/copier_unix_test.go | 57 ++++++++++++++++++++++++++++++++++++++ copier/syscall_unix.go | 2 +- 4 files changed, 86 insertions(+), 62 deletions(-) create mode 100644 copier/copier_unix_test.go diff --git a/copier/copier.go b/copier/copier.go index 39d6521828d..ef0e4778d7f 100644 --- a/copier/copier.go +++ b/copier/copier.go @@ -203,11 +203,11 @@ type StatOptions struct { // If root and directory are both not specified, the current root directory is // used, and relative names in the globs list are treated as being relative to // the current working directory. -// If root is specified and the current OS supports it, the stat() is performed -// in a chrooted context. If the directory is specified as an absolute path, -// it should either be the root directory or a subdirectory of the root -// directory. Otherwise, the directory is treated as a path relative to the -// root directory. +// If root is specified and the current OS supports it, and the calling process +// has the necessary privileges, the stat() is performed in a chrooted context. +// If the directory is specified as an absolute path, it should either be the +// root directory or a subdirectory of the root directory. Otherwise, the +// directory is treated as a path relative to the root directory. // Relative names in the glob list are treated as being relative to the // directory. func Stat(root string, directory string, options StatOptions, globs []string) ([]*StatsForGlob, error) { @@ -250,11 +250,11 @@ type GetOptions struct { // If root and directory are both not specified, the current root directory is // used, and relative names in the globs list are treated as being relative to // the current working directory. -// If root is specified and the current OS supports it, the contents are read -// in a chrooted context. If the directory is specified as an absolute path, -// it should either be the root directory or a subdirectory of the root -// directory. Otherwise, the directory is treated as a path relative to the -// root directory. +// If root is specified and the current OS supports it, and the calling process +// has the necessary privileges, the contents are read in a chrooted context. +// If the directory is specified as an absolute path, it should either be the +// root directory or a subdirectory of the root directory. Otherwise, the +// directory is treated as a path relative to the root directory. // Relative names in the glob list are treated as being relative to the // directory. func Get(root string, directory string, options GetOptions, globs []string, bulkWriter io.Writer) error { @@ -296,11 +296,12 @@ type PutOptions struct { // Put extracts an archive from the bulkReader at the specified directory. // If root and directory are both not specified, the current root directory is // used. -// If root is specified and the current OS supports it, the contents are written -// in a chrooted context. If the directory is specified as an absolute path, -// it should either be the root directory or a subdirectory of the root -// directory. Otherwise, the directory is treated as a path relative to the -// root directory. +// If root is specified and the current OS supports it, and the calling process +// has the necessary privileges, the contents are written in a chrooted +// context. If the directory is specified as an absolute path, it should +// either be the root directory or a subdirectory of the root directory. +// Otherwise, the directory is treated as a path relative to the root +// directory. func Put(root string, directory string, options PutOptions, bulkReader io.Reader) error { req := request{ Request: requestPut, @@ -329,11 +330,12 @@ type MkdirOptions struct { // need to be created will be given the specified ownership and permissions. // If root and directory are both not specified, the current root directory is // used. -// If root is specified and the current OS supports it, the directory is -// created in a chrooted context. If the directory is specified as an absolute -// path, it should either be the root directory or a subdirectory of the root -// directory. Otherwise, the directory is treated as a path relative to the -// root directory. +// If root is specified and the current OS supports it, and the calling process +// has the necessary privileges, the directory is created in a chrooted +// context. If the directory is specified as an absolute path, it should +// either be the root directory or a subdirectory of the root directory. +// Otherwise, the directory is treated as a path relative to the root +// directory. func Mkdir(root string, directory string, options MkdirOptions) error { req := request{ Request: requestMkdir, @@ -551,13 +553,13 @@ func copierWithSubprocess(bulkReader io.Reader, bulkWriter io.Writer, req reques return nil, errors.Wrap(err, step) } if err = encoder.Encode(req); err != nil { - return killAndReturn(err, "error encoding request") + return killAndReturn(err, "error encoding request for copier subprocess") } if err = decoder.Decode(&resp); err != nil { - return killAndReturn(err, "error decoding response") + return killAndReturn(err, "error decoding response from copier subprocess") } if err = encoder.Encode(&request{Request: requestQuit}); err != nil { - return killAndReturn(err, "error encoding request") + return killAndReturn(err, "error encoding request for copier subprocess") } stdinWrite.Close() stdinWrite = nil @@ -630,7 +632,7 @@ func copierMain() { // Read a request. req := new(request) if err := decoder.Decode(req); err != nil { - fmt.Fprintf(os.Stderr, "error decoding request: %v", err) + fmt.Fprintf(os.Stderr, "error decoding request from copier parent process: %v", err) os.Exit(1) } if req.Request == requestQuit { @@ -721,12 +723,12 @@ func copierMain() { } resp, cb, err := copierHandler(bulkReader, bulkWriter, *req) if err != nil { - fmt.Fprintf(os.Stderr, "error handling request %#v: %v", *req, err) + fmt.Fprintf(os.Stderr, "error handling request %#v from copier parent process: %v", *req, err) os.Exit(1) } // Encode the response. if err := encoder.Encode(resp); err != nil { - fmt.Fprintf(os.Stderr, "error encoding response %#v: %v", *req, err) + fmt.Fprintf(os.Stderr, "error encoding response %#v for copier parent process: %v", *req, err) os.Exit(1) } // If there's bulk data to transfer, run the callback to either diff --git a/copier/copier_test.go b/copier/copier_test.go index a2da223def1..2660c23a45e 100644 --- a/copier/copier_test.go +++ b/copier/copier_test.go @@ -418,13 +418,6 @@ func TestPutNoChroot(t *testing.T) { canChroot = couldChroot } -func TestPutChroot(t *testing.T) { - if uid != 0 { - t.Skipf("chroot() requires root privileges, skipping") - } - testPut(t) -} - func testPut(t *testing.T) { uidMap := []idtools.IDMap{{HostID: os.Getuid(), ContainerID: 0, Size: 1}} gidMap := []idtools.IDMap{{HostID: os.Getgid(), ContainerID: 0, Size: 1}} @@ -582,13 +575,6 @@ func TestStatNoChroot(t *testing.T) { canChroot = couldChroot } -func TestStatChroot(t *testing.T) { - if uid != 0 { - t.Skipf("chroot() requires root privileges, skipping") - } - testStat(t) -} - func testStat(t *testing.T) { for _, absolute := range []bool{false, true} { for _, topdir := range []string{"", ".", "top"} { @@ -676,13 +662,6 @@ func TestGetSingleNoChroot(t *testing.T) { canChroot = couldChroot } -func TestGetSingleChroot(t *testing.T) { - if uid != 0 { - t.Skipf("chroot() requires root privileges, skipping") - } - testGetSingle(t) -} - func testGetSingle(t *testing.T) { for _, absolute := range []bool{false, true} { for _, topdir := range []string{"", ".", "top"} { @@ -810,13 +789,6 @@ func TestGetMultipleNoChroot(t *testing.T) { canChroot = couldChroot } -func TestGetMultipleChroot(t *testing.T) { - if uid != 0 { - t.Skipf("chroot() requires root privileges, skipping") - } - testGetMultiple(t) -} - func testGetMultiple(t *testing.T) { type getTestArchiveCase struct { name string @@ -1345,13 +1317,6 @@ func TestMkdirNoChroot(t *testing.T) { canChroot = couldChroot } -func TestMkdirChroot(t *testing.T) { - if uid != 0 { - t.Skipf("chroot() requires root privileges, skipping") - } - testMkdir(t) -} - func testMkdir(t *testing.T) { type testCase struct { name string diff --git a/copier/copier_unix_test.go b/copier/copier_unix_test.go new file mode 100644 index 00000000000..a68b95964b1 --- /dev/null +++ b/copier/copier_unix_test.go @@ -0,0 +1,57 @@ +// +build !windows + +package copier + +import ( + "testing" +) + +func TestPutChroot(t *testing.T) { + if uid != 0 { + t.Skipf("chroot() requires root privileges, skipping") + } + couldChroot := canChroot + canChroot = true + testPut(t) + canChroot = couldChroot +} + +func TestStatChroot(t *testing.T) { + if uid != 0 { + t.Skipf("chroot() requires root privileges, skipping") + } + couldChroot := canChroot + canChroot = true + testStat(t) + canChroot = couldChroot +} + +func TestGetSingleChroot(t *testing.T) { + if uid != 0 { + t.Skipf("chroot() requires root privileges, skipping") + } + couldChroot := canChroot + canChroot = true + testGetSingle(t) + canChroot = couldChroot +} + +func TestGetMultipleChroot(t *testing.T) { + if uid != 0 { + t.Skipf("chroot() requires root privileges, skipping") + } + couldChroot := canChroot + canChroot = true + testGetMultiple(t) + canChroot = couldChroot +} + +func TestMkdirChroot(t *testing.T) { + if uid != 0 { + t.Skipf("chroot() requires root privileges, skipping") + } + couldChroot := canChroot + canChroot = true + testMkdir(t) + canChroot = couldChroot +} diff --git a/copier/syscall_unix.go b/copier/syscall_unix.go index 55f2f368a93..2c2806d0a30 100644 --- a/copier/syscall_unix.go +++ b/copier/syscall_unix.go @@ -10,7 +10,7 @@ import ( "golang.org/x/sys/unix" ) -var canChroot = true +var canChroot = os.Getuid() == 0 func chroot(root string) (bool, error) { if canChroot {