From 29ff2800c3938e4750ab2139141d4ac97588f138 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 14 Jun 2019 01:37:30 +0000 Subject: [PATCH 1/7] Revert "Revert "Pass root to chroot to for chroot Untar"" This reverts commit 60013ba69b4fef09b4c5197b175e58bd775c3d33. Signed-off-by: Tibor Vass --- daemon/archive.go | 7 ++- pkg/chrootarchive/archive.go | 24 ++++++-- pkg/chrootarchive/archive_unix.go | 34 ++++++++++-- pkg/chrootarchive/archive_unix_test.go | 77 ++++++++++++++++++++++++++ pkg/chrootarchive/archive_windows.go | 2 +- 5 files changed, 132 insertions(+), 12 deletions(-) create mode 100644 pkg/chrootarchive/archive_unix_test.go diff --git a/daemon/archive.go b/daemon/archive.go index 9c7971b56ea3e..9f56ca750392d 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -31,11 +31,12 @@ type archiver interface { } // helper functions to extract or archive -func extractArchive(i interface{}, src io.Reader, dst string, opts *archive.TarOptions) error { +func extractArchive(i interface{}, src io.Reader, dst string, opts *archive.TarOptions, root string) error { if ea, ok := i.(extractor); ok { return ea.ExtractArchive(src, dst, opts) } - return chrootarchive.Untar(src, dst, opts) + + return chrootarchive.UntarWithRoot(src, dst, opts, root) } func archivePath(i interface{}, src string, opts *archive.TarOptions) (io.ReadCloser, error) { @@ -367,7 +368,7 @@ func (daemon *Daemon) containerExtractToDir(container *container.Container, path } } - if err := extractArchive(driver, content, resolvedPath, options); err != nil { + if err := extractArchive(driver, content, resolvedPath, options, container.BaseFS.Path()); err != nil { return err } diff --git a/pkg/chrootarchive/archive.go b/pkg/chrootarchive/archive.go index 2d9d662830b7b..7ebca3774c3dc 100644 --- a/pkg/chrootarchive/archive.go +++ b/pkg/chrootarchive/archive.go @@ -27,18 +27,34 @@ func NewArchiver(idMapping *idtools.IdentityMapping) *archive.Archiver { // The archive may be compressed with one of the following algorithms: // identity (uncompressed), gzip, bzip2, xz. func Untar(tarArchive io.Reader, dest string, options *archive.TarOptions) error { - return untarHandler(tarArchive, dest, options, true) + return untarHandler(tarArchive, dest, options, true, dest) +} + +// UntarWithRoot is the same as `Untar`, but allows you to pass in a root directory +// The root directory is the directory that will be chrooted to. +// `dest` must be a path within `root`, if it is not an error will be returned. +// +// `root` should set to a directory which is not controlled by any potentially +// malicious process. +// +// This should be used to prevent a potential attacker from manipulating `dest` +// such that it would provide access to files outside of `dest` through things +// like symlinks. Normally `ResolveSymlinksInScope` would handle this, however +// sanitizing symlinks in this manner is inherrently racey: +// ref: CVE-2018-15664 +func UntarWithRoot(tarArchive io.Reader, dest string, options *archive.TarOptions, root string) error { + return untarHandler(tarArchive, dest, options, true, root) } // UntarUncompressed reads a stream of bytes from `archive`, parses it as a tar archive, // and unpacks it into the directory at `dest`. // The archive must be an uncompressed stream. func UntarUncompressed(tarArchive io.Reader, dest string, options *archive.TarOptions) error { - return untarHandler(tarArchive, dest, options, false) + return untarHandler(tarArchive, dest, options, false, dest) } // Handler for teasing out the automatic decompression -func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool) error { +func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions, decompress bool, root string) error { if tarArchive == nil { return fmt.Errorf("Empty archive") } @@ -69,5 +85,5 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions r = decompressedArchive } - return invokeUnpack(r, dest, options) + return invokeUnpack(r, dest, options, root) } diff --git a/pkg/chrootarchive/archive_unix.go b/pkg/chrootarchive/archive_unix.go index 5df8afd662055..96f07c4bb4d68 100644 --- a/pkg/chrootarchive/archive_unix.go +++ b/pkg/chrootarchive/archive_unix.go @@ -10,6 +10,7 @@ import ( "io" "io/ioutil" "os" + "path/filepath" "runtime" "github.com/docker/docker/pkg/archive" @@ -30,11 +31,21 @@ func untar() { fatal(err) } - if err := chroot(flag.Arg(0)); err != nil { + dst := flag.Arg(0) + var root string + if len(flag.Args()) > 1 { + root = flag.Arg(1) + } + + if root == "" { + root = dst + } + + if err := chroot(root); err != nil { fatal(err) } - if err := archive.Unpack(os.Stdin, "/", options); err != nil { + if err := archive.Unpack(os.Stdin, dst, options); err != nil { fatal(err) } // fully consume stdin in case it is zero padded @@ -45,7 +56,7 @@ func untar() { os.Exit(0) } -func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions) error { +func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions, root string) error { // We can't pass a potentially large exclude list directly via cmd line // because we easily overrun the kernel's max argument/environment size @@ -57,7 +68,21 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T return fmt.Errorf("Untar pipe failure: %v", err) } - cmd := reexec.Command("docker-untar", dest) + if root != "" { + relDest, err := filepath.Rel(root, dest) + if err != nil { + return err + } + if relDest == "." { + relDest = "/" + } + if relDest[0] != '/' { + relDest = "/" + relDest + } + dest = relDest + } + + cmd := reexec.Command("docker-untar", dest, root) cmd.Stdin = decompressedArchive cmd.ExtraFiles = append(cmd.ExtraFiles, r) @@ -69,6 +94,7 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T w.Close() return fmt.Errorf("Untar error on re-exec cmd: %v", err) } + //write the options to the pipe for the untar exec to read if err := json.NewEncoder(w).Encode(options); err != nil { w.Close() diff --git a/pkg/chrootarchive/archive_unix_test.go b/pkg/chrootarchive/archive_unix_test.go new file mode 100644 index 0000000000000..d81c19048179f --- /dev/null +++ b/pkg/chrootarchive/archive_unix_test.go @@ -0,0 +1,77 @@ +// +build !windows + +package chrootarchive + +import ( + "bytes" + "io" + "io/ioutil" + "os" + "path/filepath" + "testing" + + "github.com/docker/docker/pkg/archive" + "golang.org/x/sys/unix" + "gotest.tools/assert" +) + +// Test for CVE-2018-15664 +// Assures that in the case where an "attacker" controlled path is a symlink to +// some path outside of a container's rootfs that we do not copy data to a +// container path that will actually overwrite data on the host +func TestUntarWithMaliciousSymlinks(t *testing.T) { + dir, err := ioutil.TempDir("", t.Name()) + assert.NilError(t, err) + defer os.RemoveAll(dir) + + root := filepath.Join(dir, "root") + + err = os.MkdirAll(root, 0755) + assert.NilError(t, err) + + // Add a file into a directory above root + // Ensure that we can't access this file while tarring. + err = ioutil.WriteFile(filepath.Join(dir, "host-file"), []byte("I am a host file"), 0644) + assert.NilError(t, err) + + // Create some data which which will be copied into the "container" root into + // the symlinked path. + // Before this change, the copy would overwrite the "host" content. + // With this change it should not. + data := filepath.Join(dir, "data") + err = os.MkdirAll(data, 0755) + assert.NilError(t, err) + err = ioutil.WriteFile(filepath.Join(data, "local-file"), []byte("pwn3d"), 0644) + assert.NilError(t, err) + + safe := filepath.Join(root, "safe") + err = unix.Symlink(dir, safe) + assert.NilError(t, err) + + rdr, err := archive.TarWithOptions(data, &archive.TarOptions{IncludeFiles: []string{"local-file"}, RebaseNames: map[string]string{"local-file": "host-file"}}) + assert.NilError(t, err) + + // Use tee to test both the good case and the bad case w/o recreating the archive + bufRdr := bytes.NewBuffer(nil) + tee := io.TeeReader(rdr, bufRdr) + + err = UntarWithRoot(tee, safe, nil, root) + assert.Assert(t, err != nil) + assert.ErrorContains(t, err, "open /safe/host-file: no such file or directory") + + // Make sure the "host" file is still in tact + // Before the fix the host file would be overwritten + hostData, err := ioutil.ReadFile(filepath.Join(dir, "host-file")) + assert.NilError(t, err) + assert.Equal(t, string(hostData), "I am a host file") + + // Now test by chrooting to an attacker controlled path + // This should succeed as is and overwrite a "host" file + // Note that this would be a mis-use of this function. + err = UntarWithRoot(bufRdr, safe, nil, safe) + assert.NilError(t, err) + + hostData, err = ioutil.ReadFile(filepath.Join(dir, "host-file")) + assert.NilError(t, err) + assert.Equal(t, string(hostData), "pwn3d") +} diff --git a/pkg/chrootarchive/archive_windows.go b/pkg/chrootarchive/archive_windows.go index f2973132a3916..bd5712c5c04cb 100644 --- a/pkg/chrootarchive/archive_windows.go +++ b/pkg/chrootarchive/archive_windows.go @@ -14,7 +14,7 @@ func chroot(path string) error { func invokeUnpack(decompressedArchive io.ReadCloser, dest string, - options *archive.TarOptions) error { + options *archive.TarOptions, root string) error { // Windows is different to Linux here because Windows does not support // chroot. Hence there is no point sandboxing a chrooted process to // do the unpack. We call inline instead within the daemon process. From 44023afb7d6031c8de6473207b06c2e470714f86 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 14 Jun 2019 01:37:32 +0000 Subject: [PATCH 2/7] Revert "Revert "Add chroot for tar packing operations"" This reverts commit 96df6d4d0b039d2889ddbe235af7293d49f4ee3f. Signed-off-by: Tibor Vass --- daemon/archive.go | 8 +-- daemon/export.go | 2 +- pkg/chrootarchive/archive.go | 8 +++ pkg/chrootarchive/archive_unix.go | 98 +++++++++++++++++++++++++- pkg/chrootarchive/archive_unix_test.go | 94 ++++++++++++++++++++++++ pkg/chrootarchive/archive_windows.go | 7 ++ pkg/chrootarchive/init_unix.go | 1 + 7 files changed, 211 insertions(+), 7 deletions(-) diff --git a/daemon/archive.go b/daemon/archive.go index 9f56ca750392d..109376b4b566e 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -39,11 +39,11 @@ func extractArchive(i interface{}, src io.Reader, dst string, opts *archive.TarO return chrootarchive.UntarWithRoot(src, dst, opts, root) } -func archivePath(i interface{}, src string, opts *archive.TarOptions) (io.ReadCloser, error) { +func archivePath(i interface{}, src string, opts *archive.TarOptions, root string) (io.ReadCloser, error) { if ap, ok := i.(archiver); ok { return ap.ArchivePath(src, opts) } - return archive.TarWithOptions(src, opts) + return chrootarchive.Tar(src, opts, root) } // ContainerCopy performs a deprecated operation of archiving the resource at @@ -239,7 +239,7 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path sourceDir, sourceBase := driver.Dir(resolvedPath), driver.Base(resolvedPath) opts := archive.TarResourceRebaseOpts(sourceBase, driver.Base(absPath)) - data, err := archivePath(driver, sourceDir, opts) + data, err := archivePath(driver, sourceDir, opts, container.BaseFS.Path()) if err != nil { return nil, nil, err } @@ -433,7 +433,7 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str archive, err := archivePath(driver, basePath, &archive.TarOptions{ Compression: archive.Uncompressed, IncludeFiles: filter, - }) + }, container.BaseFS.Path()) if err != nil { return nil, err } diff --git a/daemon/export.go b/daemon/export.go index 27bc35967d220..01593f4e8a4f4 100644 --- a/daemon/export.go +++ b/daemon/export.go @@ -70,7 +70,7 @@ func (daemon *Daemon) containerExport(container *container.Container) (arch io.R Compression: archive.Uncompressed, UIDMaps: daemon.idMapping.UIDs(), GIDMaps: daemon.idMapping.GIDs(), - }) + }, basefs.Path()) if err != nil { rwlayer.Unmount() return nil, err diff --git a/pkg/chrootarchive/archive.go b/pkg/chrootarchive/archive.go index 7ebca3774c3dc..6ff61e6a767af 100644 --- a/pkg/chrootarchive/archive.go +++ b/pkg/chrootarchive/archive.go @@ -87,3 +87,11 @@ func untarHandler(tarArchive io.Reader, dest string, options *archive.TarOptions return invokeUnpack(r, dest, options, root) } + +// Tar tars the requested path while chrooted to the specified root. +func Tar(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) { + if options == nil { + options = &archive.TarOptions{} + } + return invokePack(srcPath, options, root) +} diff --git a/pkg/chrootarchive/archive_unix.go b/pkg/chrootarchive/archive_unix.go index 96f07c4bb4d68..ea2879dc002f2 100644 --- a/pkg/chrootarchive/archive_unix.go +++ b/pkg/chrootarchive/archive_unix.go @@ -12,9 +12,11 @@ import ( "os" "path/filepath" "runtime" + "strings" "github.com/docker/docker/pkg/archive" "github.com/docker/docker/pkg/reexec" + "github.com/pkg/errors" ) // untar is the entry-point for docker-untar on re-exec. This is not used on @@ -24,7 +26,7 @@ func untar() { runtime.LockOSThread() flag.Parse() - var options *archive.TarOptions + var options archive.TarOptions //read the options from the pipe "ExtraFiles" if err := json.NewDecoder(os.NewFile(3, "options")).Decode(&options); err != nil { @@ -45,7 +47,7 @@ func untar() { fatal(err) } - if err := archive.Unpack(os.Stdin, dst, options); err != nil { + if err := archive.Unpack(os.Stdin, dst, &options); err != nil { fatal(err) } // fully consume stdin in case it is zero padded @@ -57,6 +59,9 @@ func untar() { } func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.TarOptions, root string) error { + if root == "" { + return errors.New("must specify a root to chroot to") + } // We can't pass a potentially large exclude list directly via cmd line // because we easily overrun the kernel's max argument/environment size @@ -112,3 +117,92 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T } return nil } + +func tar() { + runtime.LockOSThread() + flag.Parse() + + src := flag.Arg(0) + var root string + if len(flag.Args()) > 1 { + root = flag.Arg(1) + } + + if root == "" { + root = src + } + + if err := realChroot(root); err != nil { + fatal(err) + } + + var options archive.TarOptions + if err := json.NewDecoder(os.Stdin).Decode(&options); err != nil { + fatal(err) + } + + rdr, err := archive.TarWithOptions(src, &options) + if err != nil { + fatal(err) + } + defer rdr.Close() + + if _, err := io.Copy(os.Stdout, rdr); err != nil { + fatal(err) + } + + os.Exit(0) +} + +func invokePack(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) { + if root == "" { + return nil, errors.New("root path must not be empty") + } + + relSrc, err := filepath.Rel(root, srcPath) + if err != nil { + return nil, err + } + if relSrc == "." { + relSrc = "/" + } + if relSrc[0] != '/' { + relSrc = "/" + relSrc + } + + // make sure we didn't trim a trailing slash with the call to `Rel` + if strings.HasSuffix(srcPath, "/") && !strings.HasSuffix(relSrc, "/") { + relSrc += "/" + } + + cmd := reexec.Command("docker-tar", relSrc, root) + + errBuff := bytes.NewBuffer(nil) + cmd.Stderr = errBuff + + tarR, tarW := io.Pipe() + cmd.Stdout = tarW + + stdin, err := cmd.StdinPipe() + if err != nil { + return nil, errors.Wrap(err, "error getting options pipe for tar process") + } + + if err := cmd.Start(); err != nil { + return nil, errors.Wrap(err, "tar error on re-exec cmd") + } + + go func() { + err := cmd.Wait() + err = errors.Wrapf(err, "error processing tar file: %s", errBuff) + tarW.CloseWithError(err) + }() + + if err := json.NewEncoder(stdin).Encode(options); err != nil { + stdin.Close() + return nil, errors.Wrap(err, "tar json encode to pipe failed") + } + stdin.Close() + + return tarR, nil +} diff --git a/pkg/chrootarchive/archive_unix_test.go b/pkg/chrootarchive/archive_unix_test.go index d81c19048179f..f39a88ad38140 100644 --- a/pkg/chrootarchive/archive_unix_test.go +++ b/pkg/chrootarchive/archive_unix_test.go @@ -3,11 +3,14 @@ package chrootarchive import ( + gotar "archive/tar" "bytes" "io" "io/ioutil" "os" + "path" "path/filepath" + "strings" "testing" "github.com/docker/docker/pkg/archive" @@ -75,3 +78,94 @@ func TestUntarWithMaliciousSymlinks(t *testing.T) { assert.NilError(t, err) assert.Equal(t, string(hostData), "pwn3d") } + +// Test for CVE-2018-15664 +// Assures that in the case where an "attacker" controlled path is a symlink to +// some path outside of a container's rootfs that we do not unwittingly leak +// host data into the archive. +func TestTarWithMaliciousSymlinks(t *testing.T) { + dir, err := ioutil.TempDir("", t.Name()) + assert.NilError(t, err) + // defer os.RemoveAll(dir) + t.Log(dir) + + root := filepath.Join(dir, "root") + + err = os.MkdirAll(root, 0755) + assert.NilError(t, err) + + hostFileData := []byte("I am a host file") + + // Add a file into a directory above root + // Ensure that we can't access this file while tarring. + err = ioutil.WriteFile(filepath.Join(dir, "host-file"), hostFileData, 0644) + assert.NilError(t, err) + + safe := filepath.Join(root, "safe") + err = unix.Symlink(dir, safe) + assert.NilError(t, err) + + data := filepath.Join(dir, "data") + err = os.MkdirAll(data, 0755) + assert.NilError(t, err) + + type testCase struct { + p string + includes []string + } + + cases := []testCase{ + {p: safe, includes: []string{"host-file"}}, + {p: safe + "/", includes: []string{"host-file"}}, + {p: safe, includes: nil}, + {p: safe + "/", includes: nil}, + {p: root, includes: []string{"safe/host-file"}}, + {p: root, includes: []string{"/safe/host-file"}}, + {p: root, includes: nil}, + } + + maxBytes := len(hostFileData) + + for _, tc := range cases { + t.Run(path.Join(tc.p+"_"+strings.Join(tc.includes, "_")), func(t *testing.T) { + // Here if we use archive.TarWithOptions directly or change the "root" parameter + // to be the same as "safe", data from the host will be leaked into the archive + var opts *archive.TarOptions + if tc.includes != nil { + opts = &archive.TarOptions{ + IncludeFiles: tc.includes, + } + } + rdr, err := Tar(tc.p, opts, root) + assert.NilError(t, err) + defer rdr.Close() + + tr := gotar.NewReader(rdr) + assert.Assert(t, !isDataInTar(t, tr, hostFileData, int64(maxBytes)), "host data leaked to archive") + }) + } +} + +func isDataInTar(t *testing.T, tr *gotar.Reader, compare []byte, maxBytes int64) bool { + for { + h, err := tr.Next() + if err == io.EOF { + break + } + assert.NilError(t, err) + + if h.Size == 0 { + continue + } + assert.Assert(t, h.Size <= maxBytes, "%s: file size exceeds max expected size %d: %d", h.Name, maxBytes, h.Size) + + data := make([]byte, int(h.Size)) + _, err = io.ReadFull(tr, data) + assert.NilError(t, err) + if bytes.Contains(data, compare) { + return true + } + } + + return false +} diff --git a/pkg/chrootarchive/archive_windows.go b/pkg/chrootarchive/archive_windows.go index bd5712c5c04cb..de87113e95448 100644 --- a/pkg/chrootarchive/archive_windows.go +++ b/pkg/chrootarchive/archive_windows.go @@ -20,3 +20,10 @@ func invokeUnpack(decompressedArchive io.ReadCloser, // do the unpack. We call inline instead within the daemon process. return archive.Unpack(decompressedArchive, longpath.AddPrefix(dest), options) } + +func invokePack(srcPath string, options *archive.TarOptions, root string) (io.ReadCloser, error) { + // Windows is different to Linux here because Windows does not support + // chroot. Hence there is no point sandboxing a chrooted process to + // do the pack. We call inline instead within the daemon process. + return archive.TarWithOptions(srcPath, options) +} diff --git a/pkg/chrootarchive/init_unix.go b/pkg/chrootarchive/init_unix.go index a15e4bb83c40e..c24fea7d9c13a 100644 --- a/pkg/chrootarchive/init_unix.go +++ b/pkg/chrootarchive/init_unix.go @@ -14,6 +14,7 @@ import ( func init() { reexec.Register("docker-applyLayer", applyLayer) reexec.Register("docker-untar", untar) + reexec.Register("docker-tar", tar) } func fatal(err error) { From e3f83e7aa75864d13bed502469723ec6dc7c9b5a Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Mon, 10 Jun 2019 19:03:58 -0700 Subject: [PATCH 3/7] Add test for copying entire container rootfs CID=$(docker create alpine) docker cp $CID:/ out Signed-off-by: Brian Goff Signed-off-by: Tibor Vass (cherry picked from commit 6db9f1c3d6e9ad634554cacaf197a435efcf8833) Signed-off-by: Tibor Vass --- integration/container/copy_test.go | 82 ++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index 9c5c5ce297317..58355b0921cad 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -1,13 +1,20 @@ package container // import "github.com/docker/docker/integration/container" import ( + "archive/tar" "context" + "encoding/json" "fmt" + "io" + "io/ioutil" + "os" "testing" "github.com/docker/docker/api/types" "github.com/docker/docker/client" "github.com/docker/docker/integration/internal/container" + "github.com/docker/docker/internal/test/fakecontext" + "github.com/docker/docker/pkg/jsonmessage" "gotest.tools/assert" is "gotest.tools/assert/cmp" "gotest.tools/skip" @@ -64,3 +71,78 @@ func TestCopyToContainerPathIsNotDir(t *testing.T) { err := apiclient.CopyToContainer(ctx, cid, "/etc/passwd/", nil, types.CopyToContainerOptions{}) assert.Assert(t, is.ErrorContains(err, "not a directory")) } + +func TestCopyFromContainerRoot(t *testing.T) { + skip.If(t, testEnv.DaemonInfo.OSType == "windows") + defer setupTest(t)() + + ctx := context.Background() + apiClient := testEnv.APIClient() + + dir, err := ioutil.TempDir("", t.Name()) + assert.NilError(t, err) + defer os.RemoveAll(dir) + + buildCtx := fakecontext.New(t, dir, fakecontext.WithFile("foo", "hello"), fakecontext.WithFile("baz", "world"), fakecontext.WithDockerfile(` + FROM scratch + COPY foo /foo + COPY baz /bar/baz + CMD /fake + `)) + defer buildCtx.Close() + + resp, err := apiClient.ImageBuild(ctx, buildCtx.AsTarReader(t), types.ImageBuildOptions{}) + assert.NilError(t, err) + defer resp.Body.Close() + + var imageID string + err = jsonmessage.DisplayJSONMessagesStream(resp.Body, ioutil.Discard, 0, false, func(msg jsonmessage.JSONMessage) { + var r types.BuildResult + assert.NilError(t, json.Unmarshal(*msg.Aux, &r)) + imageID = r.ID + }) + assert.NilError(t, err) + assert.Assert(t, imageID != "") + + cid := container.Create(ctx, t, apiClient, container.WithImage(imageID)) + + rdr, _, err := apiClient.CopyFromContainer(ctx, cid, "/") + assert.NilError(t, err) + defer rdr.Close() + + tr := tar.NewReader(rdr) + expect := map[string]string{ + "/foo": "hello", + "/bar/baz": "world", + } + found := make(map[string]bool, 2) + var numFound int + for { + h, err := tr.Next() + if err == io.EOF { + break + } + assert.NilError(t, err) + + expected, exists := expect[h.Name] + if !exists { + // this archive will have extra stuff in it since we are copying from root + // and docker adds a bunch of stuff + continue + } + + numFound++ + found[h.Name] = true + + buf, err := ioutil.ReadAll(tr) + assert.NilError(t, err) + assert.Check(t, is.Equal(string(buf), expected)) + + if numFound == len(expect) { + break + } + } + + assert.Check(t, found["/foo"], "/foo file not found in archive") + assert.Check(t, found["/bar/baz"], "/bar/baz file not found in archive") +} From 9b97965f22816167609d53293740e50f635fcc25 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Thu, 13 Jun 2019 05:36:21 +0000 Subject: [PATCH 4/7] add more tests Signed-off-by: Tibor Vass (cherry picked from commit 02f1eb89a42b4a9e91a8c80c213f7dc3193c75b9) Signed-off-by: Tibor Vass --- integration/container/copy_test.go | 97 +++++++++++++++++------------- 1 file changed, 56 insertions(+), 41 deletions(-) diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index 58355b0921cad..218b5759215cc 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -72,7 +72,7 @@ func TestCopyToContainerPathIsNotDir(t *testing.T) { assert.Assert(t, is.ErrorContains(err, "not a directory")) } -func TestCopyFromContainerRoot(t *testing.T) { +func TestCopyFromContainer(t *testing.T) { skip.If(t, testEnv.DaemonInfo.OSType == "windows") defer setupTest(t)() @@ -84,9 +84,10 @@ func TestCopyFromContainerRoot(t *testing.T) { defer os.RemoveAll(dir) buildCtx := fakecontext.New(t, dir, fakecontext.WithFile("foo", "hello"), fakecontext.WithFile("baz", "world"), fakecontext.WithDockerfile(` - FROM scratch + FROM busybox COPY foo /foo - COPY baz /bar/baz + COPY baz /bar/quux/baz + RUN ln -s notexist /bar/notarget && ln -s quux/baz /bar/filesymlink && ln -s quux /bar/dirsymlink && ln -s / /bar/root CMD /fake `)) defer buildCtx.Close() @@ -106,43 +107,57 @@ func TestCopyFromContainerRoot(t *testing.T) { cid := container.Create(ctx, t, apiClient, container.WithImage(imageID)) - rdr, _, err := apiClient.CopyFromContainer(ctx, cid, "/") - assert.NilError(t, err) - defer rdr.Close() - - tr := tar.NewReader(rdr) - expect := map[string]string{ - "/foo": "hello", - "/bar/baz": "world", + for _, x := range []struct { + src string + expect map[string]string + }{ + {"/", map[string]string{"/": "", "/foo": "hello", "/bar/quux/baz": "world", "/bar/filesymlink": "", "/bar/dirsymlink": "", "/bar/notarget": ""}}, + {"/bar/root", map[string]string{"root": ""}}, + {"/bar/root/", map[string]string{"root/": "", "root/foo": "hello", "root/bar/quux/baz": "world", "root/bar/filesymlink": "", "root/bar/dirsymlink": "", "root/bar/notarget": ""}}, + + {"bar/quux", map[string]string{"quux/": "", "quux/baz": "world"}}, + {"bar/quux/", map[string]string{"quux/": "", "quux/baz": "world"}}, + {"bar/quux/baz", map[string]string{"baz": "world"}}, + + {"bar/filesymlink", map[string]string{"filesymlink": ""}}, + {"bar/dirsymlink", map[string]string{"dirsymlink": ""}}, + {"bar/dirsymlink/", map[string]string{"dirsymlink/": "", "dirsymlink/baz": "world"}}, + {"bar/notarget", map[string]string{"notarget": ""}}, + } { + t.Run(x.src, func(t *testing.T) { + rdr, _, err := apiClient.CopyFromContainer(ctx, cid, x.src) + assert.NilError(t, err) + defer rdr.Close() + + found := make(map[string]bool, len(x.expect)) + var numFound int + tr := tar.NewReader(rdr) + for numFound < len(x.expect) { + h, err := tr.Next() + if err == io.EOF { + break + } + assert.NilError(t, err) + + expected, exists := x.expect[h.Name] + if !exists { + // this archive will have extra stuff in it since we are copying from root + // and docker adds a bunch of stuff + continue + } + + numFound++ + found[h.Name] = true + + buf, err := ioutil.ReadAll(tr) + if err == nil { + assert.Check(t, is.Equal(string(buf), expected)) + } + } + + for f := range x.expect { + assert.Check(t, found[f], f+" not found in archive") + } + }) } - found := make(map[string]bool, 2) - var numFound int - for { - h, err := tr.Next() - if err == io.EOF { - break - } - assert.NilError(t, err) - - expected, exists := expect[h.Name] - if !exists { - // this archive will have extra stuff in it since we are copying from root - // and docker adds a bunch of stuff - continue - } - - numFound++ - found[h.Name] = true - - buf, err := ioutil.ReadAll(tr) - assert.NilError(t, err) - assert.Check(t, is.Equal(string(buf), expected)) - - if numFound == len(expect) { - break - } - } - - assert.Check(t, found["/foo"], "/foo file not found in archive") - assert.Check(t, found["/bar/baz"], "/bar/baz file not found in archive") } From a0063c534a9c1bcc42898a95c0253f41ecba2a4a Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 11 Jun 2019 01:55:38 +0000 Subject: [PATCH 5/7] daemon: fix docker cp when container source is / Before 7a7357da, archive.TarResourceRebase was being used to copy files and folders from the container. That function splits the source path into a dirname + basename pair to support copying a file: if you wanted to tar `dir/file` it would tar from `dir` the file `file` (as part of the IncludedFiles option). However, that path splitting logic was kept for folders as well, which resulted in weird inputs to archive.TarWithOptions: if you wanted to tar `dir1/dir2` it would tar from `dir1` the directory `dir2` (as part of IncludedFiles option). Although it was weird, it worked fine until we started chrooting into the container rootfs when doing a `docker cp` with container source set to `/` (cf 3029e765). The fix is to only do the path splitting logic if the source is a file. Unfortunately, 7a7357da added support for LCOW by duplicating some of this subtle logic. Ideally we would need to do more refactoring of the archive codebase to properly encapsulate these behaviors behind well- documented APIs. This fix does not do that. Instead, it fixes the issue inline. Signed-off-by: Tibor Vass (cherry picked from commit 171538c190ee3a1a8211946ab8fa78cdde54b47a) Signed-off-by: Tibor Vass --- daemon/archive.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/daemon/archive.go b/daemon/archive.go index 109376b4b566e..21339cfb68c35 100644 --- a/daemon/archive.go +++ b/daemon/archive.go @@ -236,7 +236,13 @@ func (daemon *Daemon) containerArchivePath(container *container.Container, path if driver.Base(resolvedPath) == "." { resolvedPath += string(driver.Separator()) + "." } - sourceDir, sourceBase := driver.Dir(resolvedPath), driver.Base(resolvedPath) + + sourceDir := resolvedPath + sourceBase := "." + + if stat.Mode&os.ModeDir == 0 { // not dir + sourceDir, sourceBase = driver.Split(resolvedPath) + } opts := archive.TarResourceRebaseOpts(sourceBase, driver.Base(absPath)) data, err := archivePath(driver, sourceDir, opts, container.BaseFS.Path()) @@ -426,9 +432,6 @@ func (daemon *Daemon) containerCopy(container *container.Container, resource str d, f := driver.Split(basePath) basePath = d filter = []string{f} - } else { - filter = []string{driver.Base(basePath)} - basePath = driver.Dir(basePath) } archive, err := archivePath(driver, basePath, &archive.TarOptions{ Compression: archive.Uncompressed, From 186afe3ce370cee096c6cdbe8b1685e7bad3e403 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Wed, 12 Jun 2019 23:01:04 +0000 Subject: [PATCH 6/7] pkg/archive: keep walkRoot clean if source is / Previously, getWalkRoot("/", "foo") would return "//foo" Now it returns "/foo" Signed-off-by: Tibor Vass (cherry picked from commit 7410f1a859063d4ed3d8fca44f27bdde4c2cb5a3) Signed-off-by: Tibor Vass --- pkg/archive/archive_unix.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/archive/archive_unix.go b/pkg/archive/archive_unix.go index 1eec912b7b0b6..d626336032ba8 100644 --- a/pkg/archive/archive_unix.go +++ b/pkg/archive/archive_unix.go @@ -7,6 +7,7 @@ import ( "errors" "os" "path/filepath" + "strings" "syscall" "github.com/docker/docker/pkg/idtools" @@ -26,7 +27,7 @@ func fixVolumePathPrefix(srcPath string) string { // can't use filepath.Join(srcPath,include) because this will clean away // a trailing "." or "/" which may be important. func getWalkRoot(srcPath string, include string) string { - return srcPath + string(filepath.Separator) + include + return strings.TrimSuffix(srcPath, string(filepath.Separator)) + string(filepath.Separator) + include } // CanonicalTarNameForPath returns platform-specific filepath From 8f4b96f19e64b96df9d8c43208cefb113715ccbf Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 14 Jun 2019 18:23:21 +0000 Subject: [PATCH 7/7] integration: have container.Create call compile For reference on why this is needed: https://github.com/docker/engine/pull/280#issuecomment-502056661 Signed-off-by: Tibor Vass --- integration/container/copy_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/container/copy_test.go b/integration/container/copy_test.go index 218b5759215cc..9020b802f3884 100644 --- a/integration/container/copy_test.go +++ b/integration/container/copy_test.go @@ -105,7 +105,7 @@ func TestCopyFromContainer(t *testing.T) { assert.NilError(t, err) assert.Assert(t, imageID != "") - cid := container.Create(ctx, t, apiClient, container.WithImage(imageID)) + cid := container.Create(t, ctx, apiClient, container.WithImage(imageID)) for _, x := range []struct { src string