From 09c38a8d43cb7fcb128861d0a7f32acaee92115c Mon Sep 17 00:00:00 2001 From: unclejack Date: Wed, 5 Nov 2014 10:12:57 +0200 Subject: [PATCH 01/26] bump fpm to 1.3.2 Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 34b0460c0..d9c58a921 100644 --- a/Dockerfile +++ b/Dockerfile @@ -78,7 +78,7 @@ RUN cd /usr/local/go/src && bash -xc 'for platform in $DOCKER_CROSSPLATFORMS; do RUN go get code.google.com/p/go.tools/cmd/cover # TODO replace FPM with some very minimal debhelper stuff -RUN gem install --no-rdoc --no-ri fpm --version 1.0.2 +RUN gem install --no-rdoc --no-ri fpm --version 1.3.2 # Install man page generator RUN mkdir -p /go/src/github.com/cpuguy83 \ From fd9c2ae27dc08114ae1170503217da8d2cb778d6 Mon Sep 17 00:00:00 2001 From: Alexandr Morozov Date: Mon, 3 Nov 2014 10:50:16 -0800 Subject: [PATCH 02/26] Fix deadlock in ps exited filter Fixes #8909 Signed-off-by: Alexandr Morozov Conflicts: integration-cli/docker_cli_ps_test.go fixed merge issue caused by missing tests --- daemon/list.go | 2 +- integration-cli/docker_cli_ps_test.go | 78 +++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/daemon/list.go b/daemon/list.go index 35effa434..25360b679 100644 --- a/daemon/list.go +++ b/daemon/list.go @@ -93,7 +93,7 @@ func (daemon *Daemon) Containers(job *engine.Job) engine.Status { if len(filt_exited) > 0 && !container.Running { should_skip := true for _, code := range filt_exited { - if code == container.GetExitCode() { + if code == container.ExitCode { should_skip = false break } diff --git a/integration-cli/docker_cli_ps_test.go b/integration-cli/docker_cli_ps_test.go index 4bcf95bf7..f2a7b2ab4 100644 --- a/integration-cli/docker_cli_ps_test.go +++ b/integration-cli/docker_cli_ps_test.go @@ -282,3 +282,81 @@ func TestPsListContainersFilterStatus(t *testing.T) { logDone("ps - test ps filter status") } + +func TestPsListContainersFilterExited(t *testing.T) { + deleteAllContainers() + defer deleteAllContainers() + runCmd := exec.Command(dockerBinary, "run", "--name", "zero1", "busybox", "true") + out, _, err := runCommandWithOutput(runCmd) + if err != nil { + t.Fatal(out, err) + } + firstZero, err := getIDByName("zero1") + if err != nil { + t.Fatal(err) + } + + runCmd = exec.Command(dockerBinary, "run", "--name", "zero2", "busybox", "true") + out, _, err = runCommandWithOutput(runCmd) + if err != nil { + t.Fatal(out, err) + } + secondZero, err := getIDByName("zero2") + if err != nil { + t.Fatal(err) + } + + runCmd = exec.Command(dockerBinary, "run", "--name", "nonzero1", "busybox", "false") + out, _, err = runCommandWithOutput(runCmd) + if err == nil { + t.Fatal("Should fail.", out, err) + } + firstNonZero, err := getIDByName("nonzero1") + if err != nil { + t.Fatal(err) + } + + runCmd = exec.Command(dockerBinary, "run", "--name", "nonzero2", "busybox", "false") + out, _, err = runCommandWithOutput(runCmd) + if err == nil { + t.Fatal("Should fail.", out, err) + } + secondNonZero, err := getIDByName("nonzero2") + if err != nil { + t.Fatal(err) + } + + // filter containers by exited=0 + runCmd = exec.Command(dockerBinary, "ps", "-a", "-q", "--no-trunc", "--filter=exited=0") + out, _, err = runCommandWithOutput(runCmd) + if err != nil { + t.Fatal(out, err) + } + ids := strings.Split(strings.TrimSpace(out), "\n") + if len(ids) != 2 { + t.Fatalf("Should be 2 zero exited containerst got %d", len(ids)) + } + if ids[0] != secondZero { + t.Fatalf("First in list should be %q, got %q", secondZero, ids[0]) + } + if ids[1] != firstZero { + t.Fatalf("Second in list should be %q, got %q", firstZero, ids[1]) + } + + runCmd = exec.Command(dockerBinary, "ps", "-a", "-q", "--no-trunc", "--filter=exited=1") + out, _, err = runCommandWithOutput(runCmd) + if err != nil { + t.Fatal(out, err) + } + ids = strings.Split(strings.TrimSpace(out), "\n") + if len(ids) != 2 { + t.Fatalf("Should be 2 zero exited containerst got %d", len(ids)) + } + if ids[0] != secondNonZero { + t.Fatalf("First in list should be %q, got %q", secondNonZero, ids[0]) + } + if ids[1] != firstNonZero { + t.Fatalf("Second in list should be %q, got %q", firstNonZero, ids[1]) + } + logDone("ps - test ps filter exited") +} From 1d1b813d255512002b382b2d2db48984d9b03957 Mon Sep 17 00:00:00 2001 From: shuai-z Date: Sun, 26 Oct 2014 13:55:29 +0800 Subject: [PATCH 03/26] removed redundant Clean The doc (or src) says: The result is Cleaned. http://golang.org/pkg/path/filepath/#Join Signed-off-by: shuai-z --- pkg/symlink/fs.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/symlink/fs.go b/pkg/symlink/fs.go index da9c59067..d76173257 100644 --- a/pkg/symlink/fs.go +++ b/pkg/symlink/fs.go @@ -35,7 +35,6 @@ func FollowSymlinkInScope(link, root string) (string, error) { for _, p := range strings.Split(link, "/") { prev = filepath.Join(prev, p) - prev = filepath.Clean(prev) loopCounter := 0 for { @@ -72,7 +71,7 @@ func FollowSymlinkInScope(link, root string) (string, error) { } else { prev, _ = filepath.Abs(prev) - if prev = filepath.Clean(filepath.Join(filepath.Dir(prev), dest)); len(prev) < len(root) { + if prev = filepath.Join(filepath.Dir(prev), dest); len(prev) < len(root) { prev = filepath.Join(root, filepath.Base(dest)) } } From 662ca4114de579e53e670296615ceb49b8c32d75 Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 28 Oct 2014 23:18:45 +0200 Subject: [PATCH 04/26] pkg/symlink: avoid following out of scope Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- pkg/symlink/fs.go | 47 +++++++---- pkg/symlink/fs_test.go | 151 +++++++++++++++++++++++++++++++++--- pkg/symlink/testdata/fs/j/k | 1 + 3 files changed, 172 insertions(+), 27 deletions(-) create mode 120000 pkg/symlink/testdata/fs/j/k diff --git a/pkg/symlink/fs.go b/pkg/symlink/fs.go index d76173257..6ce99c6bd 100644 --- a/pkg/symlink/fs.go +++ b/pkg/symlink/fs.go @@ -12,6 +12,12 @@ const maxLoopCounter = 100 // FollowSymlink will follow an existing link and scope it to the root // path provided. +// The role of this function is to return an absolute path in the root +// or normalize to the root if the symlink leads to a path which is +// outside of the root. +// Errors encountered while attempting to follow the symlink in path +// will be reported. +// Normalizations to the root don't constitute errors. func FollowSymlinkInScope(link, root string) (string, error) { root, err := filepath.Abs(root) if err != nil { @@ -60,25 +66,36 @@ func FollowSymlinkInScope(link, root string) (string, error) { } return "", err } - if stat.Mode()&os.ModeSymlink == os.ModeSymlink { - dest, err := os.Readlink(prev) - if err != nil { - return "", err - } - if path.IsAbs(dest) { - prev = filepath.Join(root, dest) - } else { - prev, _ = filepath.Abs(prev) - - if prev = filepath.Join(filepath.Dir(prev), dest); len(prev) < len(root) { - prev = filepath.Join(root, filepath.Base(dest)) - } - } - } else { + // let's break if we're not dealing with a symlink + if stat.Mode()&os.ModeSymlink != os.ModeSymlink { break } + + // process the symlink + dest, err := os.Readlink(prev) + if err != nil { + return "", err + } + + if path.IsAbs(dest) { + prev = filepath.Join(root, dest) + } else { + prev, _ = filepath.Abs(prev) + + dir := filepath.Dir(prev) + prev = filepath.Join(dir, dest) + if dir == root && !strings.HasPrefix(prev, root) { + prev = root + } + if len(prev) < len(root) || (len(prev) == len(root) && prev != root) { + prev = filepath.Join(root, filepath.Base(dest)) + } + } } } + if prev == "/" { + prev = root + } return prev, nil } diff --git a/pkg/symlink/fs_test.go b/pkg/symlink/fs_test.go index d85fd6da7..0e2f948b6 100644 --- a/pkg/symlink/fs_test.go +++ b/pkg/symlink/fs_test.go @@ -46,6 +46,7 @@ func TestFollowSymLinkUnderLinkedDir(t *testing.T) { if err != nil { t.Fatal(err) } + defer os.RemoveAll(dir) os.Mkdir(filepath.Join(dir, "realdir"), 0700) os.Symlink("realdir", filepath.Join(dir, "linkdir")) @@ -97,25 +98,151 @@ func TestFollowSymLinkRelativeLink(t *testing.T) { } func TestFollowSymLinkRelativeLinkScope(t *testing.T) { - link := "testdata/fs/a/f" + // avoid letting symlink f lead us out of the "testdata" scope + // we don't normalize because symlink f is in scope and there is no + // information leak + { + link := "testdata/fs/a/f" - rewrite, err := FollowSymlinkInScope(link, "testdata") - if err != nil { - t.Fatal(err) + rewrite, err := FollowSymlinkInScope(link, "testdata") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/test"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } } - if expected := abs(t, "testdata/test"); expected != rewrite { - t.Fatalf("Expected %s got %s", expected, rewrite) + // avoid letting symlink f lead us out of the "testdata/fs" scope + // we don't normalize because symlink f is in scope and there is no + // information leak + { + link := "testdata/fs/a/f" + + rewrite, err := FollowSymlinkInScope(link, "testdata/fs") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/fs/test"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } } - link = "testdata/fs/b/h" + // avoid letting symlink g (pointed at by symlink h) take out of scope + // TODO: we should probably normalize to scope here because ../[....]/root + // is out of scope and we leak information + { + link := "testdata/fs/b/h" - rewrite, err = FollowSymlinkInScope(link, "testdata") - if err != nil { - t.Fatal(err) + rewrite, err := FollowSymlinkInScope(link, "testdata") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/root"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } } - if expected := abs(t, "testdata/root"); expected != rewrite { - t.Fatalf("Expected %s got %s", expected, rewrite) + // avoid letting allowing symlink e lead us to ../b + // normalize to the "testdata/fs/a" + { + link := "testdata/fs/a/e" + + rewrite, err := FollowSymlinkInScope(link, "testdata/fs/a") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/fs/a"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } + } + + // avoid letting symlink -> ../directory/file escape from scope + // normalize to "testdata/fs/j" + { + link := "testdata/fs/j/k" + + rewrite, err := FollowSymlinkInScope(link, "testdata/fs/j") + if err != nil { + t.Fatal(err) + } + + if expected := abs(t, "testdata/fs/j"); expected != rewrite { + t.Fatalf("Expected %s got %s", expected, rewrite) + } + } + + // make sure we don't allow escaping to / + // normalize to dir + { + dir, err := ioutil.TempDir("", "docker-fs-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + linkFile := filepath.Join(dir, "foo") + os.Mkdir(filepath.Join(dir, ""), 0700) + os.Symlink("/", linkFile) + + rewrite, err := FollowSymlinkInScope(linkFile, dir) + if err != nil { + t.Fatal(err) + } + + if rewrite != dir { + t.Fatalf("Expected %s got %s", dir, rewrite) + } + } + + // make sure we don't allow escaping to / + // normalize to dir + { + dir, err := ioutil.TempDir("", "docker-fs-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + linkFile := filepath.Join(dir, "foo") + os.Mkdir(filepath.Join(dir, ""), 0700) + os.Symlink("/../../", linkFile) + + rewrite, err := FollowSymlinkInScope(linkFile, dir) + if err != nil { + t.Fatal(err) + } + + if rewrite != dir { + t.Fatalf("Expected %s got %s", dir, rewrite) + } + } + + // make sure we stay in scope without leaking information + // this also checks for escaping to / + // normalize to dir + { + dir, err := ioutil.TempDir("", "docker-fs-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + linkFile := filepath.Join(dir, "foo") + os.Mkdir(filepath.Join(dir, ""), 0700) + os.Symlink("../../", linkFile) + + rewrite, err := FollowSymlinkInScope(linkFile, dir) + if err != nil { + t.Fatal(err) + } + + if rewrite != dir { + t.Fatalf("Expected %s got %s", dir, rewrite) + } } } diff --git a/pkg/symlink/testdata/fs/j/k b/pkg/symlink/testdata/fs/j/k new file mode 120000 index 000000000..f559e8fda --- /dev/null +++ b/pkg/symlink/testdata/fs/j/k @@ -0,0 +1 @@ +../i/a \ No newline at end of file From c9379eb3fbbc484c056f5a5e49d8d0b755a29c45 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 3 Nov 2014 22:57:18 +0000 Subject: [PATCH 05/26] Move security opts to HostConfig These settings need to be in the HostConfig so that they are not committed to an image and cannot introduce a security issue. We can safely move this field from the Config to the HostConfig without any regressions because these settings are consumed at container created and used to populate fields on the Container struct. Because of this, existing settings will be honored for containers already created on a daemon with custom security settings and prevent values being consumed via an Image. Signed-off-by: Michael Crosby --- daemon/daemon.go | 11 +++++------ daemon/daemon_unit_test.go | 2 +- daemon/start.go | 3 +++ runconfig/config.go | 2 -- runconfig/hostconfig.go | 2 ++ runconfig/parse.go | 2 +- 6 files changed, 12 insertions(+), 10 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index d069797ac..9f90643c4 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -528,10 +528,10 @@ func (daemon *Daemon) getEntrypointAndArgs(configEntrypoint, configCmd []string) return entrypoint, args } -func parseSecurityOpt(container *Container, config *runconfig.Config) error { +func parseSecurityOpt(container *Container, config *runconfig.HostConfig) error { var ( - label_opts []string - err error + labelOpts []string + err error ) for _, opt := range config.SecurityOpt { @@ -541,7 +541,7 @@ func parseSecurityOpt(container *Container, config *runconfig.Config) error { } switch con[0] { case "label": - label_opts = append(label_opts, con[1]) + labelOpts = append(labelOpts, con[1]) case "apparmor": container.AppArmorProfile = con[1] default: @@ -549,7 +549,7 @@ func parseSecurityOpt(container *Container, config *runconfig.Config) error { } } - container.ProcessLabel, container.MountLabel, err = label.InitLabels(label_opts) + container.ProcessLabel, container.MountLabel, err = label.InitLabels(labelOpts) return err } @@ -583,7 +583,6 @@ func (daemon *Daemon) newContainer(name string, config *runconfig.Config, img *i execCommands: newExecStore(), } container.root = daemon.containerRoot(container.ID) - err = parseSecurityOpt(container, config) return container, err } diff --git a/daemon/daemon_unit_test.go b/daemon/daemon_unit_test.go index f3b899ec8..fbc3302aa 100644 --- a/daemon/daemon_unit_test.go +++ b/daemon/daemon_unit_test.go @@ -8,7 +8,7 @@ import ( func TestParseSecurityOpt(t *testing.T) { container := &Container{} - config := &runconfig.Config{} + config := &runconfig.HostConfig{} // test apparmor config.SecurityOpt = []string{"apparmor:test_profile"} diff --git a/daemon/start.go b/daemon/start.go index f2c375ddc..f72407e3f 100644 --- a/daemon/start.go +++ b/daemon/start.go @@ -44,6 +44,9 @@ func (daemon *Daemon) ContainerStart(job *engine.Job) engine.Status { } func (daemon *Daemon) setHostConfig(container *Container, hostConfig *runconfig.HostConfig) error { + if err := parseSecurityOpt(container, hostConfig); err != nil { + return err + } // Validate the HostConfig binds. Make sure that: // the source exists for _, bind := range hostConfig.Binds { diff --git a/runconfig/config.go b/runconfig/config.go index 28e85de64..c00110bf7 100644 --- a/runconfig/config.go +++ b/runconfig/config.go @@ -32,7 +32,6 @@ type Config struct { Entrypoint []string NetworkDisabled bool OnBuild []string - SecurityOpt []string } func ContainerConfigFromJob(job *engine.Job) *Config { @@ -56,7 +55,6 @@ func ContainerConfigFromJob(job *engine.Job) *Config { } job.GetenvJson("ExposedPorts", &config.ExposedPorts) job.GetenvJson("Volumes", &config.Volumes) - config.SecurityOpt = job.GetenvList("SecurityOpt") if PortSpecs := job.GetenvList("PortSpecs"); PortSpecs != nil { config.PortSpecs = PortSpecs } diff --git a/runconfig/hostconfig.go b/runconfig/hostconfig.go index 5c4952203..ae75434d4 100644 --- a/runconfig/hostconfig.go +++ b/runconfig/hostconfig.go @@ -56,6 +56,7 @@ type HostConfig struct { CapAdd []string CapDrop []string RestartPolicy RestartPolicy + SecurityOpt []string } // This is used by the create command when you want to set both the @@ -90,6 +91,7 @@ func ContainerHostConfigFromJob(job *engine.Job) *HostConfig { job.GetenvJson("PortBindings", &hostConfig.PortBindings) job.GetenvJson("Devices", &hostConfig.Devices) job.GetenvJson("RestartPolicy", &hostConfig.RestartPolicy) + hostConfig.SecurityOpt = job.GetenvList("SecurityOpt") if Binds := job.GetenvList("Binds"); Binds != nil { hostConfig.Binds = Binds } diff --git a/runconfig/parse.go b/runconfig/parse.go index 42ec68898..43976f604 100644 --- a/runconfig/parse.go +++ b/runconfig/parse.go @@ -256,7 +256,6 @@ func Parse(cmd *flag.FlagSet, args []string, sysInfo *sysinfo.SysInfo) (*Config, Volumes: flVolumes.GetMap(), Entrypoint: entrypoint, WorkingDir: *flWorkingDir, - SecurityOpt: flSecurityOpt.GetAll(), } hostConfig := &HostConfig{ @@ -276,6 +275,7 @@ func Parse(cmd *flag.FlagSet, args []string, sysInfo *sysinfo.SysInfo) (*Config, CapAdd: flCapAdd.GetAll(), CapDrop: flCapDrop.GetAll(), RestartPolicy: restartPolicy, + SecurityOpt: flSecurityOpt.GetAll(), } if sysInfo != nil && flMemory > 0 && !sysInfo.SwapLimit { From 0573b17b24b66c26e8311d55dff4d81923030f75 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Mon, 3 Nov 2014 23:00:49 +0000 Subject: [PATCH 06/26] Add AppArmorProfile to container inspect json Signed-off-by: Michael Crosby --- daemon/inspect.go | 1 + 1 file changed, 1 insertion(+) diff --git a/daemon/inspect.go b/daemon/inspect.go index 396ca0227..cf2ed644d 100644 --- a/daemon/inspect.go +++ b/daemon/inspect.go @@ -47,6 +47,7 @@ func (daemon *Daemon) ContainerInspect(job *engine.Job) engine.Status { out.Set("ProcessLabel", container.ProcessLabel) out.SetJson("Volumes", container.Volumes) out.SetJson("VolumesRW", container.VolumesRW) + out.SetJson("AppArmorProfile", container.AppArmorProfile) if children, err := daemon.Children(container.Name); err == nil { for linkAlias, child := range children { From d51a02091ca5485ed42b336d3df8c6c4ab2d9a6b Mon Sep 17 00:00:00 2001 From: unclejack Date: Thu, 30 Oct 2014 14:48:30 +0200 Subject: [PATCH 07/26] pkg/reexec: move reexec code to a new package Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) Conflicts: integration/runtime_test.go fixed imports --- daemon/execdriver/lxc/init.go | 2 +- daemon/execdriver/native/exec.go | 2 +- daemon/execdriver/native/init.go | 2 +- daemon/networkdriver/portmapper/proxy.go | 2 +- docker/docker.go | 2 +- dockerinit/dockerinit.go | 2 +- integration/runtime_test.go | 2 +- pkg/reexec/MAINTAINERS | 1 + {reexec => pkg/reexec}/README.md | 0 {reexec => pkg/reexec}/reexec.go | 0 10 files changed, 8 insertions(+), 7 deletions(-) create mode 100644 pkg/reexec/MAINTAINERS rename {reexec => pkg/reexec}/README.md (100%) rename {reexec => pkg/reexec}/reexec.go (100%) diff --git a/daemon/execdriver/lxc/init.go b/daemon/execdriver/lxc/init.go index 2a91bbb5f..680f53e1a 100644 --- a/daemon/execdriver/lxc/init.go +++ b/daemon/execdriver/lxc/init.go @@ -13,7 +13,7 @@ import ( "strings" "syscall" - "github.com/docker/docker/reexec" + "github.com/docker/docker/pkg/reexec" "github.com/docker/libcontainer/netlink" ) diff --git a/daemon/execdriver/native/exec.go b/daemon/execdriver/native/exec.go index 0f7e5c07b..84ad09672 100644 --- a/daemon/execdriver/native/exec.go +++ b/daemon/execdriver/native/exec.go @@ -11,7 +11,7 @@ import ( "runtime" "github.com/docker/docker/daemon/execdriver" - "github.com/docker/docker/reexec" + "github.com/docker/docker/pkg/reexec" "github.com/docker/libcontainer" "github.com/docker/libcontainer/namespaces" ) diff --git a/daemon/execdriver/native/init.go b/daemon/execdriver/native/init.go index 7021eeb67..c1c988d93 100644 --- a/daemon/execdriver/native/init.go +++ b/daemon/execdriver/native/init.go @@ -10,7 +10,7 @@ import ( "path/filepath" "runtime" - "github.com/docker/docker/reexec" + "github.com/docker/docker/pkg/reexec" "github.com/docker/libcontainer" "github.com/docker/libcontainer/namespaces" "github.com/docker/libcontainer/syncpipe" diff --git a/daemon/networkdriver/portmapper/proxy.go b/daemon/networkdriver/portmapper/proxy.go index 341f0605e..af20469ed 100644 --- a/daemon/networkdriver/portmapper/proxy.go +++ b/daemon/networkdriver/portmapper/proxy.go @@ -14,7 +14,7 @@ import ( "time" "github.com/docker/docker/pkg/proxy" - "github.com/docker/docker/reexec" + "github.com/docker/docker/pkg/reexec" ) const userlandProxyCommandName = "docker-proxy" diff --git a/docker/docker.go b/docker/docker.go index f0cbb6f6a..12900ecb1 100644 --- a/docker/docker.go +++ b/docker/docker.go @@ -13,7 +13,7 @@ import ( "github.com/docker/docker/api/client" "github.com/docker/docker/dockerversion" flag "github.com/docker/docker/pkg/mflag" - "github.com/docker/docker/reexec" + "github.com/docker/docker/pkg/reexec" "github.com/docker/docker/utils" ) diff --git a/dockerinit/dockerinit.go b/dockerinit/dockerinit.go index c5bba782b..a6754b05a 100644 --- a/dockerinit/dockerinit.go +++ b/dockerinit/dockerinit.go @@ -3,7 +3,7 @@ package main import ( _ "github.com/docker/docker/daemon/execdriver/lxc" _ "github.com/docker/docker/daemon/execdriver/native" - "github.com/docker/docker/reexec" + "github.com/docker/docker/pkg/reexec" ) func main() { diff --git a/integration/runtime_test.go b/integration/runtime_test.go index 6720485e2..0a9a74cf9 100644 --- a/integration/runtime_test.go +++ b/integration/runtime_test.go @@ -22,7 +22,7 @@ import ( "github.com/docker/docker/nat" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/log" - "github.com/docker/docker/reexec" + "github.com/docker/docker/pkg/reexec" "github.com/docker/docker/runconfig" "github.com/docker/docker/utils" ) diff --git a/pkg/reexec/MAINTAINERS b/pkg/reexec/MAINTAINERS new file mode 100644 index 000000000..e48a0c7d4 --- /dev/null +++ b/pkg/reexec/MAINTAINERS @@ -0,0 +1 @@ +Michael Crosby (@crosbymichael) diff --git a/reexec/README.md b/pkg/reexec/README.md similarity index 100% rename from reexec/README.md rename to pkg/reexec/README.md diff --git a/reexec/reexec.go b/pkg/reexec/reexec.go similarity index 100% rename from reexec/reexec.go rename to pkg/reexec/reexec.go From 3ab5251f5620e7b8b052ee331f9ac1fbdbb725b1 Mon Sep 17 00:00:00 2001 From: Josh Hawn Date: Mon, 27 Oct 2014 11:38:22 -0700 Subject: [PATCH 08/26] Use archive.CopyWithTar in vfs.Create The vfs storage driver currently shells out to the `cp` binary on the host system to perform an 'archive' copy of the base image to a new directory. The archive option preserves the modified time of the files which are created but there was an issue where it was unable to preserve the modified time of copied symbolic links on some host systems with an outdated version of `cp`. This change no longer relies on the host system implementation and instead utilizes the `CopyWithTar` function found in `pkg/archive` which is used to copy from source to destination directory using a Tar archive, which should correctly preserve file attributes. Docker-DCO-1.1-Signed-off-by: Josh Hawn (github: jlhawn) --- daemon/graphdriver/vfs/driver.go | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) diff --git a/daemon/graphdriver/vfs/driver.go b/daemon/graphdriver/vfs/driver.go index a186060d0..1076eb38d 100644 --- a/daemon/graphdriver/vfs/driver.go +++ b/daemon/graphdriver/vfs/driver.go @@ -8,6 +8,7 @@ import ( "path" "github.com/docker/docker/daemon/graphdriver" + "github.com/docker/docker/pkg/archive" "github.com/docker/libcontainer/label" ) @@ -46,21 +47,6 @@ func isGNUcoreutils() bool { return false } -func copyDir(src, dst string) error { - argv := make([]string, 0, 4) - - if isGNUcoreutils() { - argv = append(argv, "-aT", "--reflink=auto", src, dst) - } else { - argv = append(argv, "-a", src+"/.", dst+"/.") - } - - if output, err := exec.Command("cp", argv...).CombinedOutput(); err != nil { - return fmt.Errorf("Error VFS copying directory: %s (%s)", err, output) - } - return nil -} - func (d *Driver) Create(id, parent string) error { dir := d.dir(id) if err := os.MkdirAll(path.Dir(dir), 0700); err != nil { @@ -80,7 +66,7 @@ func (d *Driver) Create(id, parent string) error { if err != nil { return fmt.Errorf("%s: %s", parent, err) } - if err := copyDir(parentDir, dir); err != nil { + if err := archive.CopyWithTar(parentDir, dir); err != nil { return err } return nil From 1d4a82365bfa215767fc4a4cc7e4d5d10dbcd0e4 Mon Sep 17 00:00:00 2001 From: unclejack Date: Thu, 6 Nov 2014 20:01:37 +0200 Subject: [PATCH 09/26] pkg/archive: add interface for Untar Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- pkg/archive/archive.go | 71 +++++++++++++++++++++++++++--------------- 1 file changed, 46 insertions(+), 25 deletions(-) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index 7d9103e10..cdac22840 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -35,10 +35,17 @@ type ( Compression Compression NoLchown bool } + + // Archiver allows the reuse of most utility functions of this package + // with a pluggable Untar function. + Archiver struct { + Untar func(io.Reader, string, *TarOptions) error + } ) var ( ErrNotImplemented = errors.New("Function not implemented") + defaultArchiver = &Archiver{Untar} ) const ( @@ -508,45 +515,47 @@ loop: return nil } -// TarUntar is a convenience function which calls Tar and Untar, with -// the output of one piped into the other. If either Tar or Untar fails, -// TarUntar aborts and returns the error. -func TarUntar(src string, dst string) error { +func (archiver *Archiver) TarUntar(src, dst string) error { log.Debugf("TarUntar(%s %s)", src, dst) archive, err := TarWithOptions(src, &TarOptions{Compression: Uncompressed}) if err != nil { return err } defer archive.Close() - return Untar(archive, dst, nil) + return archiver.Untar(archive, dst, nil) } -// UntarPath is a convenience function which looks for an archive -// at filesystem path `src`, and unpacks it at `dst`. -func UntarPath(src, dst string) error { +// TarUntar is a convenience function which calls Tar and Untar, with the output of one piped into the other. +// If either Tar or Untar fails, TarUntar aborts and returns the error. +func TarUntar(src, dst string) error { + return defaultArchiver.TarUntar(src, dst) +} + +func (archiver *Archiver) UntarPath(src, dst string) error { archive, err := os.Open(src) if err != nil { return err } defer archive.Close() - if err := Untar(archive, dst, nil); err != nil { + if err := archiver.Untar(archive, dst, nil); err != nil { return err } return nil } -// CopyWithTar creates a tar archive of filesystem path `src`, and -// unpacks it at filesystem path `dst`. -// The archive is streamed directly with fixed buffering and no -// intermediary disk IO. -// -func CopyWithTar(src, dst string) error { +// UntarPath is a convenience function which looks for an archive +// at filesystem path `src`, and unpacks it at `dst`. +func UntarPath(src, dst string) error { + return defaultArchiver.UntarPath(src, dst) +} + +func (archiver *Archiver) CopyWithTar(src, dst string) error { srcSt, err := os.Stat(src) if err != nil { return err } if !srcSt.IsDir() { - return CopyFileWithTar(src, dst) + return archiver.CopyFileWithTar(src, dst) } // Create dst, copy src's content into it log.Debugf("Creating dest directory: %s", dst) @@ -554,16 +563,18 @@ func CopyWithTar(src, dst string) error { return err } log.Debugf("Calling TarUntar(%s, %s)", src, dst) - return TarUntar(src, dst) + return archiver.TarUntar(src, dst) } -// CopyFileWithTar emulates the behavior of the 'cp' command-line -// for a single file. It copies a regular file from path `src` to -// path `dst`, and preserves all its metadata. -// -// If `dst` ends with a trailing slash '/', the final destination path -// will be `dst/base(src)`. -func CopyFileWithTar(src, dst string) (err error) { +// CopyWithTar creates a tar archive of filesystem path `src`, and +// unpacks it at filesystem path `dst`. +// The archive is streamed directly with fixed buffering and no +// intermediary disk IO. +func CopyWithTar(src, dst string) error { + return defaultArchiver.CopyWithTar(src, dst) +} + +func (archiver *Archiver) CopyFileWithTar(src, dst string) (err error) { log.Debugf("CopyFileWithTar(%s, %s)", src, dst) srcSt, err := os.Stat(src) if err != nil { @@ -611,7 +622,17 @@ func CopyFileWithTar(src, dst string) (err error) { err = er } }() - return Untar(r, filepath.Dir(dst), nil) + return archiver.Untar(r, filepath.Dir(dst), nil) +} + +// CopyFileWithTar emulates the behavior of the 'cp' command-line +// for a single file. It copies a regular file from path `src` to +// path `dst`, and preserves all its metadata. +// +// If `dst` ends with a trailing slash '/', the final destination path +// will be `dst/base(src)`. +func CopyFileWithTar(src, dst string) (err error) { + return defaultArchiver.CopyFileWithTar(src, dst) } // CmdStream executes a command, and returns its stdout as a stream. From 0357b26c1b5ecc3a4d0d1b2e7cf63ea8e5f70eba Mon Sep 17 00:00:00 2001 From: unclejack Date: Wed, 29 Oct 2014 21:06:51 +0200 Subject: [PATCH 10/26] add pkg/chrootarchive and use it on the daemon Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- builder/internals.go | 11 +++-- daemon/graphdriver/aufs/aufs.go | 3 +- daemon/graphdriver/fsdiff.go | 3 +- daemon/graphdriver/vfs/driver.go | 4 +- daemon/volumes.go | 4 +- pkg/chrootarchive/archive.go | 76 +++++++++++++++++++++++++++++++ pkg/chrootarchive/diff.go | 38 ++++++++++++++++ pkg/chrootarchive/init.go | 18 ++++++++ pkg/reexec/command_linux.go | 18 ++++++++ pkg/reexec/command_unsupported.go | 11 +++++ pkg/reexec/reexec.go | 3 -- 11 files changed, 176 insertions(+), 13 deletions(-) create mode 100644 pkg/chrootarchive/archive.go create mode 100644 pkg/chrootarchive/diff.go create mode 100644 pkg/chrootarchive/init.go create mode 100644 pkg/reexec/command_linux.go create mode 100644 pkg/reexec/command_unsupported.go diff --git a/builder/internals.go b/builder/internals.go index 5fd03f774..5d0d197a0 100644 --- a/builder/internals.go +++ b/builder/internals.go @@ -22,6 +22,7 @@ import ( "github.com/docker/docker/daemon" imagepkg "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/log" "github.com/docker/docker/pkg/parsers" "github.com/docker/docker/pkg/promise" @@ -46,7 +47,9 @@ func (b *Builder) readContext(context io.Reader) error { if b.context, err = tarsum.NewTarSum(decompressedStream, true, tarsum.Version0); err != nil { return err } - if err := archive.Untar(b.context, tmpdirPath, nil); err != nil { + + os.MkdirAll(tmpdirPath, 0700) + if err := chrootarchive.Untar(b.context, tmpdirPath, nil); err != nil { return err } @@ -620,7 +623,7 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec } // try to successfully untar the orig - if err := archive.UntarPath(origPath, tarDest); err == nil { + if err := chrootarchive.UntarPath(origPath, tarDest); err == nil { return nil } else if err != io.EOF { log.Debugf("Couldn't untar %s to %s: %s", origPath, tarDest, err) @@ -630,7 +633,7 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec if err := os.MkdirAll(path.Dir(destPath), 0755); err != nil { return err } - if err := archive.CopyWithTar(origPath, destPath); err != nil { + if err := chrootarchive.CopyWithTar(origPath, destPath); err != nil { return err } @@ -643,7 +646,7 @@ func (b *Builder) addContext(container *daemon.Container, orig, dest string, dec } func copyAsDirectory(source, destination string, destinationExists bool) error { - if err := archive.CopyWithTar(source, destination); err != nil { + if err := chrootarchive.CopyWithTar(source, destination); err != nil { return err } diff --git a/daemon/graphdriver/aufs/aufs.go b/daemon/graphdriver/aufs/aufs.go index 8e3ae0b18..bbaad08e3 100644 --- a/daemon/graphdriver/aufs/aufs.go +++ b/daemon/graphdriver/aufs/aufs.go @@ -32,6 +32,7 @@ import ( "github.com/docker/docker/daemon/graphdriver" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/log" mountpk "github.com/docker/docker/pkg/mount" "github.com/docker/docker/utils" @@ -304,7 +305,7 @@ func (a *Driver) Diff(id, parent string) (archive.Archive, error) { } func (a *Driver) applyDiff(id string, diff archive.ArchiveReader) error { - return archive.Untar(diff, path.Join(a.rootPath(), "diff", id), nil) + return chrootarchive.Untar(diff, path.Join(a.rootPath(), "diff", id), nil) } // DiffSize calculates the changes between the specified id diff --git a/daemon/graphdriver/fsdiff.go b/daemon/graphdriver/fsdiff.go index 5e9d32c1c..14e27d60c 100644 --- a/daemon/graphdriver/fsdiff.go +++ b/daemon/graphdriver/fsdiff.go @@ -5,6 +5,7 @@ import ( "time" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/ioutils" "github.com/docker/docker/pkg/log" "github.com/docker/docker/utils" @@ -120,7 +121,7 @@ func (gdw *naiveDiffDriver) ApplyDiff(id, parent string, diff archive.ArchiveRea start := time.Now().UTC() log.Debugf("Start untar layer") - if err = archive.ApplyLayer(layerFs, diff); err != nil { + if err = chrootarchive.ApplyLayer(layerFs, diff); err != nil { return } log.Debugf("Untar time: %vs", time.Now().UTC().Sub(start).Seconds()) diff --git a/daemon/graphdriver/vfs/driver.go b/daemon/graphdriver/vfs/driver.go index 1076eb38d..aa104500b 100644 --- a/daemon/graphdriver/vfs/driver.go +++ b/daemon/graphdriver/vfs/driver.go @@ -8,7 +8,7 @@ import ( "path" "github.com/docker/docker/daemon/graphdriver" - "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/libcontainer/label" ) @@ -66,7 +66,7 @@ func (d *Driver) Create(id, parent string) error { if err != nil { return fmt.Errorf("%s: %s", parent, err) } - if err := archive.CopyWithTar(parentDir, dir); err != nil { + if err := chrootarchive.CopyWithTar(parentDir, dir); err != nil { return err } return nil diff --git a/daemon/volumes.go b/daemon/volumes.go index b34d9678c..ec0ccd023 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -10,7 +10,7 @@ import ( "syscall" "github.com/docker/docker/daemon/execdriver" - "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/log" "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/volumes" @@ -302,7 +302,7 @@ func copyExistingContents(source, destination string) error { if len(srcList) == 0 { // If the source volume is empty copy files from the root into the volume - if err := archive.CopyWithTar(source, destination); err != nil { + if err := chrootarchive.CopyWithTar(source, destination); err != nil { return err } } diff --git a/pkg/chrootarchive/archive.go b/pkg/chrootarchive/archive.go new file mode 100644 index 000000000..f1df57ca5 --- /dev/null +++ b/pkg/chrootarchive/archive.go @@ -0,0 +1,76 @@ +package chrootarchive + +import ( + "flag" + "fmt" + "io" + "os" + "runtime" + "syscall" + + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" +) + +func untar() { + runtime.LockOSThread() + flag.Parse() + + if err := syscall.Chroot(flag.Arg(0)); err != nil { + fatal(err) + } + if err := syscall.Chdir("/"); err != nil { + fatal(err) + } + if err := archive.Untar(os.Stdin, "/", nil); err != nil { + fatal(err) + } + os.Exit(0) +} + +var ( + chrootArchiver = &archive.Archiver{Untar} +) + +func Untar(archive io.Reader, dest string, options *archive.TarOptions) error { + if _, err := os.Stat(dest); os.IsNotExist(err) { + if err := os.MkdirAll(dest, 0777); err != nil { + return err + } + } + cmd := reexec.Command("docker-untar", dest) + cmd.Stdin = archive + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("Untar %s %s", err, out) + } + return nil +} + +func TarUntar(src, dst string) error { + return chrootArchiver.TarUntar(src, dst) +} + +// CopyWithTar creates a tar archive of filesystem path `src`, and +// unpacks it at filesystem path `dst`. +// The archive is streamed directly with fixed buffering and no +// intermediary disk IO. +func CopyWithTar(src, dst string) error { + return chrootArchiver.CopyWithTar(src, dst) +} + +// CopyFileWithTar emulates the behavior of the 'cp' command-line +// for a single file. It copies a regular file from path `src` to +// path `dst`, and preserves all its metadata. +// +// If `dst` ends with a trailing slash '/', the final destination path +// will be `dst/base(src)`. +func CopyFileWithTar(src, dst string) (err error) { + return chrootArchiver.CopyFileWithTar(src, dst) +} + +// UntarPath is a convenience function which looks for an archive +// at filesystem path `src`, and unpacks it at `dst`. +func UntarPath(src, dst string) error { + return chrootArchiver.UntarPath(src, dst) +} diff --git a/pkg/chrootarchive/diff.go b/pkg/chrootarchive/diff.go new file mode 100644 index 000000000..2133200c6 --- /dev/null +++ b/pkg/chrootarchive/diff.go @@ -0,0 +1,38 @@ +package chrootarchive + +import ( + "flag" + "fmt" + "os" + "runtime" + "syscall" + + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" +) + +func applyLayer() { + runtime.LockOSThread() + flag.Parse() + + if err := syscall.Chroot(flag.Arg(0)); err != nil { + fatal(err) + } + if err := syscall.Chdir("/"); err != nil { + fatal(err) + } + if err := archive.ApplyLayer("/", os.Stdin); err != nil { + fatal(err) + } + os.Exit(0) +} + +func ApplyLayer(dest string, layer archive.ArchiveReader) error { + cmd := reexec.Command("docker-applyLayer", dest) + cmd.Stdin = layer + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("ApplyLayer %s %s", err, out) + } + return nil +} diff --git a/pkg/chrootarchive/init.go b/pkg/chrootarchive/init.go new file mode 100644 index 000000000..b548e9fe7 --- /dev/null +++ b/pkg/chrootarchive/init.go @@ -0,0 +1,18 @@ +package chrootarchive + +import ( + "fmt" + "os" + + "github.com/docker/docker/pkg/reexec" +) + +func init() { + reexec.Register("docker-untar", untar) + reexec.Register("docker-applyLayer", applyLayer) +} + +func fatal(err error) { + fmt.Fprint(os.Stderr, err) + os.Exit(1) +} diff --git a/pkg/reexec/command_linux.go b/pkg/reexec/command_linux.go new file mode 100644 index 000000000..8dc3f3a4a --- /dev/null +++ b/pkg/reexec/command_linux.go @@ -0,0 +1,18 @@ +// +build linux + +package reexec + +import ( + "os/exec" + "syscall" +) + +func Command(args ...string) *exec.Cmd { + return &exec.Cmd{ + Path: Self(), + Args: args, + SysProcAttr: &syscall.SysProcAttr{ + Pdeathsig: syscall.SIGTERM, + }, + } +} diff --git a/pkg/reexec/command_unsupported.go b/pkg/reexec/command_unsupported.go new file mode 100644 index 000000000..a579318e8 --- /dev/null +++ b/pkg/reexec/command_unsupported.go @@ -0,0 +1,11 @@ +// +build !linux + +package reexec + +import ( + "os/exec" +) + +func Command(args ...string) *exec.Cmd { + return nil +} diff --git a/pkg/reexec/reexec.go b/pkg/reexec/reexec.go index 136b905bd..774e71c76 100644 --- a/pkg/reexec/reexec.go +++ b/pkg/reexec/reexec.go @@ -27,19 +27,16 @@ func Init() bool { return true } - return false } // Self returns the path to the current processes binary func Self() string { name := os.Args[0] - if filepath.Base(name) == name { if lp, err := exec.LookPath(name); err == nil { name = lp } } - return name } From 3ac6394b8082d4700483d52fbfe54914be537d9e Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Sat, 8 Nov 2014 10:38:42 -0500 Subject: [PATCH 11/26] pkg/chrootarchive: pass TarOptions via CLI arg Signed-off-by: Tibor Vass --- builder/internals.go | 1 - graph/load.go | 3 ++- pkg/chrootarchive/archive.go | 18 ++++++++++++-- pkg/chrootarchive/archive_test.go | 39 +++++++++++++++++++++++++++++++ pkg/chrootarchive/init.go | 1 + 5 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 pkg/chrootarchive/archive_test.go diff --git a/builder/internals.go b/builder/internals.go index 5d0d197a0..fa8b9f703 100644 --- a/builder/internals.go +++ b/builder/internals.go @@ -48,7 +48,6 @@ func (b *Builder) readContext(context io.Reader) error { return err } - os.MkdirAll(tmpdirPath, 0700) if err := chrootarchive.Untar(b.context, tmpdirPath, nil); err != nil { return err } diff --git a/graph/load.go b/graph/load.go index 753f31d2c..fcbeef619 100644 --- a/graph/load.go +++ b/graph/load.go @@ -10,6 +10,7 @@ import ( "github.com/docker/docker/engine" "github.com/docker/docker/image" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/chrootarchive" "github.com/docker/docker/pkg/log" ) @@ -53,7 +54,7 @@ func (s *TagStore) CmdLoad(job *engine.Job) engine.Status { excludes[i] = k i++ } - if err := archive.Untar(repoFile, repoDir, &archive.TarOptions{Excludes: excludes}); err != nil { + if err := chrootarchive.Untar(repoFile, repoDir, &archive.TarOptions{Excludes: excludes}); err != nil { return job.Error(err) } diff --git a/pkg/chrootarchive/archive.go b/pkg/chrootarchive/archive.go index f1df57ca5..fc2bea2c4 100644 --- a/pkg/chrootarchive/archive.go +++ b/pkg/chrootarchive/archive.go @@ -1,11 +1,14 @@ package chrootarchive import ( + "bytes" + "encoding/json" "flag" "fmt" "io" "os" "runtime" + "strings" "syscall" "github.com/docker/docker/pkg/archive" @@ -22,7 +25,12 @@ func untar() { if err := syscall.Chdir("/"); err != nil { fatal(err) } - if err := archive.Untar(os.Stdin, "/", nil); err != nil { + options := new(archive.TarOptions) + dec := json.NewDecoder(strings.NewReader(flag.Arg(1))) + if err := dec.Decode(options); err != nil { + fatal(err) + } + if err := archive.Untar(os.Stdin, "/", options); err != nil { fatal(err) } os.Exit(0) @@ -33,12 +41,18 @@ var ( ) func Untar(archive io.Reader, dest string, options *archive.TarOptions) error { + var buf bytes.Buffer + enc := json.NewEncoder(&buf) + if err := enc.Encode(options); err != nil { + return fmt.Errorf("Untar json encode: %v", err) + } if _, err := os.Stat(dest); os.IsNotExist(err) { if err := os.MkdirAll(dest, 0777); err != nil { return err } } - cmd := reexec.Command("docker-untar", dest) + + cmd := reexec.Command("docker-untar", dest, buf.String()) cmd.Stdin = archive out, err := cmd.CombinedOutput() if err != nil { diff --git a/pkg/chrootarchive/archive_test.go b/pkg/chrootarchive/archive_test.go new file mode 100644 index 000000000..aeac44874 --- /dev/null +++ b/pkg/chrootarchive/archive_test.go @@ -0,0 +1,39 @@ +package chrootarchive + +import ( + "io/ioutil" + "os" + "path/filepath" + "testing" + + "github.com/docker/docker/pkg/archive" +) + +func TestChrootTarUntar(t *testing.T) { + tmpdir, err := ioutil.TempDir("", "docker-TestChrootTarUntar") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpdir) + src := filepath.Join(tmpdir, "src") + if err := os.MkdirAll(src, 0700); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(src, "toto"), []byte("hello toto"), 0644); err != nil { + t.Fatal(err) + } + if err := ioutil.WriteFile(filepath.Join(src, "lolo"), []byte("hello lolo"), 0644); err != nil { + t.Fatal(err) + } + stream, err := archive.Tar(src, archive.Uncompressed) + if err != nil { + t.Fatal(err) + } + dest := filepath.Join(tmpdir, "src") + if err := os.MkdirAll(dest, 0700); err != nil { + t.Fatal(err) + } + if err := Untar(stream, dest, &archive.TarOptions{Excludes: []string{"lolo"}}); err != nil { + t.Fatal(err) + } +} diff --git a/pkg/chrootarchive/init.go b/pkg/chrootarchive/init.go index b548e9fe7..f05698f65 100644 --- a/pkg/chrootarchive/init.go +++ b/pkg/chrootarchive/init.go @@ -10,6 +10,7 @@ import ( func init() { reexec.Register("docker-untar", untar) reexec.Register("docker-applyLayer", applyLayer) + reexec.Init() } func fatal(err error) { From 8d90b0faf8e289edc2b4c13c6f312995745f161b Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 11 Nov 2014 13:02:14 +0200 Subject: [PATCH 12/26] don't call reexec.Init from chrootarchive Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- daemon/graphdriver/aufs/aufs_test.go | 10 ++++++++-- daemon/graphdriver/vfs/vfs_test.go | 9 ++++++++- graph/pools_test.go | 10 +++++++++- pkg/chrootarchive/archive_test.go | 5 +++++ pkg/chrootarchive/init.go | 1 - 5 files changed, 30 insertions(+), 5 deletions(-) diff --git a/daemon/graphdriver/aufs/aufs_test.go b/daemon/graphdriver/aufs/aufs_test.go index cc5b3a203..0c940bc9e 100644 --- a/daemon/graphdriver/aufs/aufs_test.go +++ b/daemon/graphdriver/aufs/aufs_test.go @@ -4,18 +4,24 @@ import ( "crypto/sha256" "encoding/hex" "fmt" - "github.com/docker/docker/daemon/graphdriver" - "github.com/docker/docker/pkg/archive" "io/ioutil" "os" "path" "testing" + + "github.com/docker/docker/daemon/graphdriver" + "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" ) var ( tmp = path.Join(os.TempDir(), "aufs-tests", "aufs") ) +func init() { + reexec.Init() +} + func testInit(dir string, t *testing.T) graphdriver.Driver { d, err := Init(dir, nil) if err != nil { diff --git a/daemon/graphdriver/vfs/vfs_test.go b/daemon/graphdriver/vfs/vfs_test.go index eaf70f59d..1ee6ae4a9 100644 --- a/daemon/graphdriver/vfs/vfs_test.go +++ b/daemon/graphdriver/vfs/vfs_test.go @@ -1,10 +1,17 @@ package vfs import ( - "github.com/docker/docker/daemon/graphdriver/graphtest" "testing" + + "github.com/docker/docker/daemon/graphdriver/graphtest" + + "github.com/docker/docker/pkg/reexec" ) +func init() { + reexec.Init() +} + // This avoids creating a new driver for each test if all tests are run // Make sure to put new tests between TestVfsSetup and TestVfsTeardown func TestVfsSetup(t *testing.T) { diff --git a/graph/pools_test.go b/graph/pools_test.go index 785a4bd12..129a5e1fe 100644 --- a/graph/pools_test.go +++ b/graph/pools_test.go @@ -1,6 +1,14 @@ package graph -import "testing" +import ( + "testing" + + "github.com/docker/docker/pkg/reexec" +) + +func init() { + reexec.Init() +} func TestPools(t *testing.T) { s := &TagStore{ diff --git a/pkg/chrootarchive/archive_test.go b/pkg/chrootarchive/archive_test.go index aeac44874..69e18e319 100644 --- a/pkg/chrootarchive/archive_test.go +++ b/pkg/chrootarchive/archive_test.go @@ -7,8 +7,13 @@ import ( "testing" "github.com/docker/docker/pkg/archive" + "github.com/docker/docker/pkg/reexec" ) +func init() { + reexec.Init() +} + func TestChrootTarUntar(t *testing.T) { tmpdir, err := ioutil.TempDir("", "docker-TestChrootTarUntar") if err != nil { diff --git a/pkg/chrootarchive/init.go b/pkg/chrootarchive/init.go index f05698f65..b548e9fe7 100644 --- a/pkg/chrootarchive/init.go +++ b/pkg/chrootarchive/init.go @@ -10,7 +10,6 @@ import ( func init() { reexec.Register("docker-untar", untar) reexec.Register("docker-applyLayer", applyLayer) - reexec.Init() } func fatal(err error) { From 967f80f3cceb96f85a4795d42eeb7b84ae0ce24a Mon Sep 17 00:00:00 2001 From: Thomas Orozco Date: Thu, 23 Oct 2014 14:50:06 +0200 Subject: [PATCH 13/26] Fix: Failed Start breaks VolumesFrom Running parseVolumesFromSpec on all VolumesFrom specs before initialize any mounts endures that we don't leave container.Volumes in an inconsistent (partially initialized) if one of out mount groups is not available (e.g. the container we're trying to mount from does not exist). Keeping container.Volumes in a consistent state ensures that next time we Start() the container, it'll run prepareVolumes() again. The attached test demonstrates that when a container fails to start due to a missing container specified in VolumesFrom, it "remembers" a Volume that worked. Fixes: #8726 Signed-off-by: Thomas Orozco Conflicts: integration-cli/docker_cli_start_test.go cli integration test --- daemon/volumes.go | 9 ++++++-- integration-cli/docker_cli_start_test.go | 29 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/daemon/volumes.go b/daemon/volumes.go index ec0ccd023..f98baa1c7 100644 --- a/daemon/volumes.go +++ b/daemon/volumes.go @@ -191,15 +191,20 @@ func parseBindMountSpec(spec string) (string, string, bool, error) { func (container *Container) applyVolumesFrom() error { volumesFrom := container.hostConfig.VolumesFrom + mountGroups := make([]map[string]*Mount, 0, len(volumesFrom)) + for _, spec := range volumesFrom { - mounts, err := parseVolumesFromSpec(container.daemon, spec) + mountGroup, err := parseVolumesFromSpec(container.daemon, spec) if err != nil { return err } + mountGroups = append(mountGroups, mountGroup) + } + for _, mounts := range mountGroups { for _, mnt := range mounts { mnt.container = container - if err = mnt.initialize(); err != nil { + if err := mnt.initialize(); err != nil { return err } } diff --git a/integration-cli/docker_cli_start_test.go b/integration-cli/docker_cli_start_test.go index 18ad96aef..addc781ca 100644 --- a/integration-cli/docker_cli_start_test.go +++ b/integration-cli/docker_cli_start_test.go @@ -2,6 +2,7 @@ package main import ( "os/exec" + "strings" "testing" "time" ) @@ -36,3 +37,31 @@ func TestStartAttachReturnsOnError(t *testing.T) { logDone("start - error on start with attach exits") } + +// gh#8726: a failed Start() breaks --volumes-from on subsequent Start()'s +func TestStartVolumesFromFailsCleanly(t *testing.T) { + defer deleteAllContainers() + + // Create the first data volume + cmd(t, "run", "-d", "--name", "data_before", "-v", "/foo", "busybox") + + // Expect this to fail because the data test after contaienr doesn't exist yet + if _, err := runCommand(exec.Command(dockerBinary, "run", "-d", "--name", "consumer", "--volumes-from", "data_before", "--volumes-from", "data_after", "busybox")); err == nil { + t.Fatal("Expected error but got none") + } + + // Create the second data volume + cmd(t, "run", "-d", "--name", "data_after", "-v", "/bar", "busybox") + + // Now, all the volumes should be there + cmd(t, "start", "consumer") + + // Check that we have the volumes we want + out, _, _ := cmd(t, "inspect", "--format='{{ len .Volumes }}'", "consumer") + n_volumes := strings.Trim(out, " \r\n'") + if n_volumes != "2" { + t.Fatalf("Missing volumes: expected 2, got %s", n_volumes) + } + + logDone("start - missing containers in --volumes-from did not affect subsequent runs") +} From ea361c0476d03f44051289d8e3c128d699c76acc Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Mon, 20 Oct 2014 15:35:48 -0400 Subject: [PATCH 14/26] archive: add breakout tests Signed-off-by: Tibor Vass Conflicts: pkg/archive/archive.go fixed conflict which git couldn't fix with the added BreakoutError --- pkg/archive/archive.go | 5 + pkg/archive/archive_test.go | 192 +++++++++++++++++++++++++++++++++++- pkg/archive/diff_test.go | 191 +++++++++++++++++++++++++++++++++++ pkg/archive/utils_test.go | 166 +++++++++++++++++++++++++++++++ 4 files changed, 553 insertions(+), 1 deletion(-) create mode 100644 pkg/archive/diff_test.go create mode 100644 pkg/archive/utils_test.go diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index cdac22840..50ed8b72a 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -41,6 +41,11 @@ type ( Archiver struct { Untar func(io.Reader, string, *TarOptions) error } + + // breakoutError is used to differentiate errors related to breaking out + // When testing archive breakout in the unit tests, this error is expected + // in order for the test to pass. + breakoutError error ) var ( diff --git a/pkg/archive/archive_test.go b/pkg/archive/archive_test.go index b46f95322..76dab21fa 100644 --- a/pkg/archive/archive_test.go +++ b/pkg/archive/archive_test.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "path" + "path/filepath" "testing" "time" @@ -169,7 +170,12 @@ func TestTarWithOptions(t *testing.T) { // Failing prevents the archives from being uncompressed during ADD func TestTypeXGlobalHeaderDoesNotFail(t *testing.T) { hdr := tar.Header{Typeflag: tar.TypeXGlobalHeader} - err := createTarFile("pax_global_header", "some_dir", &hdr, nil, true) + tmpDir, err := ioutil.TempDir("", "docker-test-archive-pax-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + err = createTarFile(filepath.Join(tmpDir, "pax_global_header"), tmpDir, &hdr, nil, true) if err != nil { t.Fatal(err) } @@ -242,3 +248,187 @@ func BenchmarkTarUntar(b *testing.B) { os.RemoveAll(target) } } + +func TestUntarInvalidFilenames(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { + { + Name: "../victim/dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { + { + // Note the leading slash + Name: "/../victim/slash-dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("untar", "docker-TestUntarInvalidFilenames", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestUntarInvalidHardlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeLink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeLink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (hardlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try reading victim/hello (hardlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try removing victim directory (hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("untar", "docker-TestUntarInvalidHardlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestUntarInvalidSymlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeSymlink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeSymlink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try removing victim directory (symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("untar", "docker-TestUntarInvalidSymlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} diff --git a/pkg/archive/diff_test.go b/pkg/archive/diff_test.go new file mode 100644 index 000000000..758c4115d --- /dev/null +++ b/pkg/archive/diff_test.go @@ -0,0 +1,191 @@ +package archive + +import ( + "testing" + + "github.com/docker/docker/vendor/src/code.google.com/p/go/src/pkg/archive/tar" +) + +func TestApplyLayerInvalidFilenames(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { + { + Name: "../victim/dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { + { + // Note the leading slash + Name: "/../victim/slash-dotdot", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidFilenames", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestApplyLayerInvalidHardlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeLink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeLink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (hardlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try reading victim/hello (hardlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // Try removing victim directory (hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeLink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidHardlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} + +func TestApplyLayerInvalidSymlink(t *testing.T) { + for i, headers := range [][]*tar.Header{ + { // try reading victim/hello (../) + { + Name: "dotdot", + Typeflag: tar.TypeSymlink, + Linkname: "../victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (/../) + { + Name: "slash-dotdot", + Typeflag: tar.TypeSymlink, + // Note the leading slash + Linkname: "/../victim/hello", + Mode: 0644, + }, + }, + { // try writing victim/file + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim/file", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "symlink", + Typeflag: tar.TypeSymlink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try reading victim/hello (symlink, hardlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "hardlink", + Typeflag: tar.TypeLink, + Linkname: "loophole-victim/hello", + Mode: 0644, + }, + }, + { // try removing victim directory (symlink) + { + Name: "loophole-victim", + Typeflag: tar.TypeSymlink, + Linkname: "../victim", + Mode: 0755, + }, + { + Name: "loophole-victim", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, + } { + if err := testBreakout("applylayer", "docker-TestApplyLayerInvalidSymlink", headers); err != nil { + t.Fatalf("i=%d. %v", i, err) + } + } +} diff --git a/pkg/archive/utils_test.go b/pkg/archive/utils_test.go new file mode 100644 index 000000000..3624fe5af --- /dev/null +++ b/pkg/archive/utils_test.go @@ -0,0 +1,166 @@ +package archive + +import ( + "bytes" + "fmt" + "io" + "io/ioutil" + "os" + "path/filepath" + "time" + + "github.com/docker/docker/vendor/src/code.google.com/p/go/src/pkg/archive/tar" +) + +var testUntarFns = map[string]func(string, io.Reader) error{ + "untar": func(dest string, r io.Reader) error { + return Untar(r, dest, nil) + }, + "applylayer": func(dest string, r io.Reader) error { + return ApplyLayer(dest, ArchiveReader(r)) + }, +} + +// testBreakout is a helper function that, within the provided `tmpdir` directory, +// creates a `victim` folder with a generated `hello` file in it. +// `untar` extracts to a directory named `dest`, the tar file created from `headers`. +// +// Here are the tested scenarios: +// - removed `victim` folder (write) +// - removed files from `victim` folder (write) +// - new files in `victim` folder (write) +// - modified files in `victim` folder (write) +// - file in `dest` with same content as `victim/hello` (read) +// +// When using testBreakout make sure you cover one of the scenarios listed above. +func testBreakout(untarFn string, tmpdir string, headers []*tar.Header) error { + tmpdir, err := ioutil.TempDir("", tmpdir) + if err != nil { + return err + } + defer os.RemoveAll(tmpdir) + + dest := filepath.Join(tmpdir, "dest") + if err := os.Mkdir(dest, 0755); err != nil { + return err + } + + victim := filepath.Join(tmpdir, "victim") + if err := os.Mkdir(victim, 0755); err != nil { + return err + } + hello := filepath.Join(victim, "hello") + helloData, err := time.Now().MarshalText() + if err != nil { + return err + } + if err := ioutil.WriteFile(hello, helloData, 0644); err != nil { + return err + } + helloStat, err := os.Stat(hello) + if err != nil { + return err + } + + reader, writer := io.Pipe() + go func() { + t := tar.NewWriter(writer) + for _, hdr := range headers { + t.WriteHeader(hdr) + } + t.Close() + }() + + untar := testUntarFns[untarFn] + if untar == nil { + return fmt.Errorf("could not find untar function %q in testUntarFns", untarFn) + } + if err := untar(dest, reader); err != nil { + if _, ok := err.(breakoutError); !ok { + // If untar returns an error unrelated to an archive breakout, + // then consider this an unexpected error and abort. + return err + } + // Here, untar detected the breakout. + // Let's move on verifying that indeed there was no breakout. + fmt.Printf("breakoutError: %v\n", err) + } + + // Check victim folder + f, err := os.Open(victim) + if err != nil { + // codepath taken if victim folder was removed + return fmt.Errorf("archive breakout: error reading %q: %v", victim, err) + } + defer f.Close() + + // Check contents of victim folder + // + // We are only interested in getting 2 files from the victim folder, because if all is well + // we expect only one result, the `hello` file. If there is a second result, it cannot + // hold the same name `hello` and we assume that a new file got created in the victim folder. + // That is enough to detect an archive breakout. + names, err := f.Readdirnames(2) + if err != nil { + // codepath taken if victim is not a folder + return fmt.Errorf("archive breakout: error reading directory content of %q: %v", victim, err) + } + for _, name := range names { + if name != "hello" { + // codepath taken if new file was created in victim folder + return fmt.Errorf("archive breakout: new file %q", name) + } + } + + // Check victim/hello + f, err = os.Open(hello) + if err != nil { + // codepath taken if read permissions were removed + return fmt.Errorf("archive breakout: could not lstat %q: %v", hello, err) + } + defer f.Close() + b, err := ioutil.ReadAll(f) + if err != nil { + return err + } + fi, err := f.Stat() + if err != nil { + return err + } + if helloStat.IsDir() != fi.IsDir() || + // TODO: cannot check for fi.ModTime() change + helloStat.Mode() != fi.Mode() || + helloStat.Size() != fi.Size() || + !bytes.Equal(helloData, b) { + // codepath taken if hello has been modified + return fmt.Errorf("archive breakout: file %q has been modified. Contents: expected=%q, got=%q. FileInfo: expected=%#v, got=%#v.", hello, helloData, b, helloStat, fi) + } + + // Check that nothing in dest/ has the same content as victim/hello. + // Since victim/hello was generated with time.Now(), it is safe to assume + // that any file whose content matches exactly victim/hello, managed somehow + // to access victim/hello. + return filepath.Walk(dest, func(path string, info os.FileInfo, err error) error { + if info.IsDir() { + if err != nil { + // skip directory if error + return filepath.SkipDir + } + // enter directory + return nil + } + if err != nil { + // skip file if error + return nil + } + b, err := ioutil.ReadFile(path) + if err != nil { + // Houston, we have a problem. Aborting (space)walk. + return err + } + if bytes.Equal(helloData, b) { + return fmt.Errorf("archive breakout: file %q has been accessed via %q", hello, path) + } + return nil + }) +} From a111eea20cad697bb58ebfbf455ee1670c7cf82c Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Mon, 20 Oct 2014 15:36:28 -0400 Subject: [PATCH 15/26] archive: prevent breakout in Untar Signed-off-by: Tibor Vass --- pkg/archive/archive.go | 22 +++++++++++++++++++++- pkg/symlink/fs.go | 4 +++- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index 50ed8b72a..c5255013a 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -22,6 +22,7 @@ import ( "github.com/docker/docker/pkg/log" "github.com/docker/docker/pkg/pools" "github.com/docker/docker/pkg/promise" + "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" ) @@ -275,11 +276,23 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L } case tar.TypeLink: - if err := os.Link(filepath.Join(extractDir, hdr.Linkname), path); err != nil { + targetPath := filepath.Join(extractDir, hdr.Linkname) + // check for hardlink breakout + if !strings.HasPrefix(targetPath, extractDir) { + return breakoutError(fmt.Errorf("invalid hardlink %q -> %q", targetPath, hdr.Linkname)) + } + if err := os.Link(targetPath, path); err != nil { return err } case tar.TypeSymlink: + // check for symlink breakout + if _, err := symlink.FollowSymlinkInScope(filepath.Join(filepath.Dir(path), hdr.Linkname), extractDir); err != nil { + if _, ok := err.(symlink.ErrBreakout); ok { + return breakoutError(fmt.Errorf("invalid symlink %q -> %q", path, hdr.Linkname)) + } + return err + } if err := os.Symlink(hdr.Linkname, path); err != nil { return err } @@ -424,6 +437,8 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) // identity (uncompressed), gzip, bzip2, xz. // FIXME: specify behavior when target path exists vs. doesn't exist. func Untar(archive io.Reader, dest string, options *TarOptions) error { + dest = filepath.Clean(dest) + if options == nil { options = &TarOptions{} } @@ -461,6 +476,7 @@ loop: } // Normalize name, for safety and for a simple is-root check + // This keeps "../" as-is, but normalizes "/../" to "/" hdr.Name = filepath.Clean(hdr.Name) for _, exclude := range options.Excludes { @@ -481,7 +497,11 @@ loop: } } + // Prevent symlink breakout path := filepath.Join(dest, hdr.Name) + if !strings.HasPrefix(path, dest) { + return breakoutError(fmt.Errorf("%q is outside of %q", path, dest)) + } // If path exits we almost always just want to remove and replace it // The only exception is when it is a directory *and* the file from diff --git a/pkg/symlink/fs.go b/pkg/symlink/fs.go index 6ce99c6bd..09271ffc4 100644 --- a/pkg/symlink/fs.go +++ b/pkg/symlink/fs.go @@ -10,6 +10,8 @@ import ( const maxLoopCounter = 100 +type ErrBreakout error + // FollowSymlink will follow an existing link and scope it to the root // path provided. // The role of this function is to return an absolute path in the root @@ -34,7 +36,7 @@ func FollowSymlinkInScope(link, root string) (string, error) { } if !strings.HasPrefix(filepath.Dir(link), root) { - return "", fmt.Errorf("%s is not within %s", link, root) + return "", ErrBreakout(fmt.Errorf("%s is not within %s", link, root)) } prev := "/" From 04175d07634600f2a5cf43dd197eb2a926aba8b9 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 31 Oct 2014 13:18:39 -0400 Subject: [PATCH 16/26] archive: prevent breakout in ApplyLayer Signed-off-by: Tibor Vass --- pkg/archive/diff.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/archive/diff.go b/pkg/archive/diff.go index 215f62ec0..5ed1a1d90 100644 --- a/pkg/archive/diff.go +++ b/pkg/archive/diff.go @@ -24,6 +24,8 @@ func mkdev(major int64, minor int64) uint32 { // ApplyLayer parses a diff in the standard layer format from `layer`, and // applies it to the directory `dest`. func ApplyLayer(dest string, layer ArchiveReader) error { + dest = filepath.Clean(dest) + // We need to be able to set any perms oldmask := syscall.Umask(0) defer syscall.Umask(oldmask) @@ -93,6 +95,12 @@ func ApplyLayer(dest string, layer ArchiveReader) error { path := filepath.Join(dest, hdr.Name) base := filepath.Base(path) + + // Prevent symlink breakout + if !strings.HasPrefix(path, dest) { + return breakoutError(fmt.Errorf("%q is outside of %q", path, dest)) + } + if strings.HasPrefix(base, ".wh.") { originalBase := base[len(".wh."):] originalPath := filepath.Join(filepath.Dir(path), originalBase) From 1527979e87d361539c2791d4e89602aea3cce174 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 11 Nov 2014 17:37:44 -0500 Subject: [PATCH 17/26] registry: refactor registry.IsSecure calls into registry.NewEndpoint Signed-off-by: Tibor Vass Conflicts: registry/endpoint.go registry/endpoint_test.go registry/registry_test.go --- graph/pull.go | 4 +--- graph/push.go | 4 +--- registry/endpoint.go | 38 ++++++++++++++++++++++++-------------- registry/endpoint_test.go | 27 +++++++++++++++++++++++++++ registry/registry_test.go | 29 +++++++++++++++++++++++++++++ registry/service.go | 6 ++---- 6 files changed, 84 insertions(+), 24 deletions(-) create mode 100644 registry/endpoint_test.go diff --git a/graph/pull.go b/graph/pull.go index 897e0813a..c1db605b8 100644 --- a/graph/pull.go +++ b/graph/pull.go @@ -113,9 +113,7 @@ func (s *TagStore) CmdPull(job *engine.Job) engine.Status { return job.Error(err) } - secure := registry.IsSecure(hostname, s.insecureRegistries) - - endpoint, err := registry.NewEndpoint(hostname, secure) + endpoint, err := registry.NewEndpoint(hostname, s.insecureRegistries) if err != nil { return job.Error(err) } diff --git a/graph/push.go b/graph/push.go index 1d2bf7242..165b580f2 100644 --- a/graph/push.go +++ b/graph/push.go @@ -214,9 +214,7 @@ func (s *TagStore) CmdPush(job *engine.Job) engine.Status { return job.Error(err) } - secure := registry.IsSecure(hostname, s.insecureRegistries) - - endpoint, err := registry.NewEndpoint(hostname, secure) + endpoint, err := registry.NewEndpoint(hostname, s.insecureRegistries) if err != nil { return job.Error(err) } diff --git a/registry/endpoint.go b/registry/endpoint.go index 6dd4e1f60..2eac41ce3 100644 --- a/registry/endpoint.go +++ b/registry/endpoint.go @@ -33,21 +33,15 @@ func scanForApiVersion(hostname string) (string, APIVersion) { return hostname, DefaultAPIVersion } -func NewEndpoint(hostname string, secure bool) (*Endpoint, error) { - var ( - endpoint = Endpoint{secure: secure} - trimmedHostname string - err error - ) - if !strings.HasPrefix(hostname, "http") { - hostname = "https://" + hostname - } - trimmedHostname, endpoint.Version = scanForApiVersion(hostname) - endpoint.URL, err = url.Parse(trimmedHostname) +func NewEndpoint(hostname string, insecureRegistries []string) (*Endpoint, error) { + endpoint, err := newEndpoint(hostname) if err != nil { return nil, err } + secure := isSecure(endpoint.URL.Host, insecureRegistries) + endpoint.secure = secure + // Try HTTPS ping to registry endpoint.URL.Scheme = "https" if _, err := endpoint.Ping(); err != nil { @@ -65,12 +59,28 @@ func NewEndpoint(hostname string, secure bool) (*Endpoint, error) { endpoint.URL.Scheme = "http" _, err2 := endpoint.Ping() if err2 == nil { - return &endpoint, nil + return endpoint, nil } return nil, fmt.Errorf("Invalid registry endpoint %q. HTTPS attempt: %v. HTTP attempt: %v", endpoint, err, err2) } + return endpoint, nil +} +func newEndpoint(hostname string) (*Endpoint, error) { + var ( + endpoint = Endpoint{secure: true} + trimmedHostname string + err error + ) + if !strings.HasPrefix(hostname, "http") { + hostname = "https://" + hostname + } + trimmedHostname, endpoint.Version = scanForApiVersion(hostname) + endpoint.URL, err = url.Parse(trimmedHostname) + if err != nil { + return nil, err + } return &endpoint, nil } @@ -141,9 +151,9 @@ func (e Endpoint) Ping() (RegistryInfo, error) { return info, nil } -// IsSecure returns false if the provided hostname is part of the list of insecure registries. +// isSecure returns false if the provided hostname is part of the list of insecure registries. // Insecure registries accept HTTP and/or accept HTTPS with certificates from unknown CAs. -func IsSecure(hostname string, insecureRegistries []string) bool { +func isSecure(hostname string, insecureRegistries []string) bool { if hostname == IndexServerAddress() { return true } diff --git a/registry/endpoint_test.go b/registry/endpoint_test.go new file mode 100644 index 000000000..0ec1220d9 --- /dev/null +++ b/registry/endpoint_test.go @@ -0,0 +1,27 @@ +package registry + +import "testing" + +func TestEndpointParse(t *testing.T) { + testData := []struct { + str string + expected string + }{ + {IndexServerAddress(), IndexServerAddress()}, + {"http://0.0.0.0:5000", "http://0.0.0.0:5000/v1/"}, + {"0.0.0.0:5000", "https://0.0.0.0:5000/v1/"}, + } + for _, td := range testData { + e, err := newEndpoint(td.str) + if err != nil { + t.Errorf("%q: %s", td.str, err) + } + if e == nil { + t.Logf("something's fishy, endpoint for %q is nil", td.str) + continue + } + if e.String() != td.expected { + t.Errorf("expected %q, got %q", td.expected, e.String()) + } + } +} diff --git a/registry/registry_test.go b/registry/registry_test.go index c9a9fc81b..7e63ee92a 100644 --- a/registry/registry_test.go +++ b/registry/registry_test.go @@ -316,3 +316,32 @@ func TestAddRequiredHeadersToRedirectedRequests(t *testing.T) { } } } + +func TestIsSecure(t *testing.T) { + tests := []struct { + addr string + insecureRegistries []string + expected bool + }{ + {"example.com", []string{}, true}, + {"example.com", []string{"example.com"}, false}, + {"localhost", []string{"localhost:5000"}, false}, + {"localhost:5000", []string{"localhost:5000"}, false}, + {"localhost", []string{"example.com"}, false}, + {"127.0.0.1:5000", []string{"127.0.0.1:5000"}, false}, + {"localhost", []string{}, false}, + {"localhost:5000", []string{}, false}, + {"127.0.0.1", []string{}, false}, + {"localhost", []string{"example.com"}, false}, + {"127.0.0.1", []string{"example.com"}, false}, + {"example.com", []string{}, true}, + {"example.com", []string{"example.com"}, false}, + {"127.0.0.1", []string{"example.com"}, false}, + {"127.0.0.1:5000", []string{"example.com"}, false}, + } + for _, tt := range tests { + if sec := isSecure(tt.addr, tt.insecureRegistries); sec != tt.expected { + t.Errorf("isSecure failed for %q %v, expected %v got %v", tt.addr, tt.insecureRegistries, tt.expected, sec) + } + } +} diff --git a/registry/service.go b/registry/service.go index 7051d9343..53e8278b0 100644 --- a/registry/service.go +++ b/registry/service.go @@ -40,7 +40,7 @@ func (s *Service) Auth(job *engine.Job) engine.Status { job.GetenvJson("authConfig", authConfig) if addr := authConfig.ServerAddress; addr != "" && addr != IndexServerAddress() { - endpoint, err := NewEndpoint(addr, IsSecure(addr, s.insecureRegistries)) + endpoint, err := NewEndpoint(addr, s.insecureRegistries) if err != nil { return job.Error(err) } @@ -92,9 +92,7 @@ func (s *Service) Search(job *engine.Job) engine.Status { return job.Error(err) } - secure := IsSecure(hostname, s.insecureRegistries) - - endpoint, err := NewEndpoint(hostname, secure) + endpoint, err := NewEndpoint(hostname, s.insecureRegistries) if err != nil { return job.Error(err) } From 6152460c1e4c058676476314320b17e54de0c621 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 11 Nov 2014 20:08:59 -0500 Subject: [PATCH 18/26] Put mock registry address in insecureRegistries for unit tests Signed-off-by: Tibor Vass Conflicts: registry/registry_mock_test.go --- registry/registry_mock_test.go | 16 +++++++++++----- registry/registry_test.go | 4 ++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/registry/registry_mock_test.go b/registry/registry_mock_test.go index 379dc78f4..fc2b46b9b 100644 --- a/registry/registry_mock_test.go +++ b/registry/registry_mock_test.go @@ -19,8 +19,9 @@ import ( ) var ( - testHttpServer *httptest.Server - testLayers = map[string]map[string]string{ + testHTTPServer *httptest.Server + insecureRegistries []string + testLayers = map[string]map[string]string{ "77dbf71da1d00e3fbddc480176eac8994025630c6590d11cfc8fe1209c2a1d20": { "json": `{"id":"77dbf71da1d00e3fbddc480176eac8994025630c6590d11cfc8fe1209c2a1d20", "comment":"test base image","created":"2013-03-23T12:53:11.10432-07:00", @@ -99,7 +100,12 @@ func init() { // /v2/ r.HandleFunc("/v2/version", handlerGetPing).Methods("GET") - testHttpServer = httptest.NewServer(handlerAccessLog(r)) + testHTTPServer = httptest.NewServer(handlerAccessLog(r)) + URL, err := url.Parse(testHTTPServer.URL) + if err != nil { + panic(err) + } + insecureRegistries = []string{URL.Host} } func handlerAccessLog(handler http.Handler) http.Handler { @@ -111,7 +117,7 @@ func handlerAccessLog(handler http.Handler) http.Handler { } func makeURL(req string) string { - return testHttpServer.URL + req + return testHTTPServer.URL + req } func writeHeaders(w http.ResponseWriter) { @@ -301,7 +307,7 @@ func handlerUsers(w http.ResponseWriter, r *http.Request) { } func handlerImages(w http.ResponseWriter, r *http.Request) { - u, _ := url.Parse(testHttpServer.URL) + u, _ := url.Parse(testHTTPServer.URL) w.Header().Add("X-Docker-Endpoints", fmt.Sprintf("%s , %s ", u.Host, "test.example.com")) w.Header().Add("X-Docker-Token", fmt.Sprintf("FAKE-SESSION-%d", time.Now().UnixNano())) if r.Method == "PUT" { diff --git a/registry/registry_test.go b/registry/registry_test.go index 7e63ee92a..aaba3f0a3 100644 --- a/registry/registry_test.go +++ b/registry/registry_test.go @@ -18,7 +18,7 @@ var ( func spawnTestRegistrySession(t *testing.T) *Session { authConfig := &AuthConfig{} - endpoint, err := NewEndpoint(makeURL("/v1/"), false) + endpoint, err := NewEndpoint(makeURL("/v1/"), insecureRegistries) if err != nil { t.Fatal(err) } @@ -30,7 +30,7 @@ func spawnTestRegistrySession(t *testing.T) *Session { } func TestPingRegistryEndpoint(t *testing.T) { - ep, err := NewEndpoint(makeURL("/v1/"), false) + ep, err := NewEndpoint(makeURL("/v1/"), insecureRegistries) if err != nil { t.Fatal(err) } From eb3738347ade64c2b14995c30b400bc6aa48b84e Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Thu, 13 Nov 2014 06:56:36 -0800 Subject: [PATCH 19/26] registry: parse INDEXSERVERADDRESS into a URL for easier check in isSecure Signed-off-by: Tibor Vass --- registry/auth.go | 10 ++++++++++ registry/endpoint.go | 14 ++++++-------- registry/endpoint_test.go | 2 +- registry/registry_test.go | 1 + 4 files changed, 18 insertions(+), 9 deletions(-) diff --git a/registry/auth.go b/registry/auth.go index 7c0709a47..dad58c163 100644 --- a/registry/auth.go +++ b/registry/auth.go @@ -7,6 +7,7 @@ import ( "fmt" "io/ioutil" "net/http" + "net/url" "os" "path" "strings" @@ -27,8 +28,17 @@ const ( var ( ErrConfigFileMissing = errors.New("The Auth config file is missing") + IndexServerURL *url.URL ) +func init() { + url, err := url.Parse(INDEXSERVER) + if err != nil { + panic(err) + } + IndexServerURL = url +} + type AuthConfig struct { Username string `json:"username,omitempty"` Password string `json:"password,omitempty"` diff --git a/registry/endpoint.go b/registry/endpoint.go index 2eac41ce3..eb0e9a1fa 100644 --- a/registry/endpoint.go +++ b/registry/endpoint.go @@ -34,21 +34,18 @@ func scanForApiVersion(hostname string) (string, APIVersion) { } func NewEndpoint(hostname string, insecureRegistries []string) (*Endpoint, error) { - endpoint, err := newEndpoint(hostname) + endpoint, err := newEndpoint(hostname, insecureRegistries) if err != nil { return nil, err } - secure := isSecure(endpoint.URL.Host, insecureRegistries) - endpoint.secure = secure - // Try HTTPS ping to registry endpoint.URL.Scheme = "https" if _, err := endpoint.Ping(); err != nil { //TODO: triggering highland build can be done there without "failing" - if secure { + if endpoint.secure { // If registry is secure and HTTPS failed, show user the error and tell them about `--insecure-registry` // in case that's what they need. DO NOT accept unknown CA certificates, and DO NOT fallback to HTTP. return nil, fmt.Errorf("Invalid registry endpoint %s: %v. If this private registry supports only HTTP or HTTPS with an unknown CA certificate, please add `--insecure-registry %s` to the daemon's arguments. In the case of HTTPS, if you have access to the registry's CA certificate, no need for the flag; simply place the CA certificate at /etc/docker/certs.d/%s/ca.crt", endpoint, err, endpoint.URL.Host, endpoint.URL.Host) @@ -67,9 +64,9 @@ func NewEndpoint(hostname string, insecureRegistries []string) (*Endpoint, error return endpoint, nil } -func newEndpoint(hostname string) (*Endpoint, error) { +func newEndpoint(hostname string, insecureRegistries []string) (*Endpoint, error) { var ( - endpoint = Endpoint{secure: true} + endpoint = Endpoint{} trimmedHostname string err error ) @@ -81,6 +78,7 @@ func newEndpoint(hostname string) (*Endpoint, error) { if err != nil { return nil, err } + endpoint.secure = isSecure(endpoint.URL.Host, insecureRegistries) return &endpoint, nil } @@ -154,7 +152,7 @@ func (e Endpoint) Ping() (RegistryInfo, error) { // isSecure returns false if the provided hostname is part of the list of insecure registries. // Insecure registries accept HTTP and/or accept HTTPS with certificates from unknown CAs. func isSecure(hostname string, insecureRegistries []string) bool { - if hostname == IndexServerAddress() { + if hostname == IndexServerURL.Host { return true } diff --git a/registry/endpoint_test.go b/registry/endpoint_test.go index 0ec1220d9..54105ec17 100644 --- a/registry/endpoint_test.go +++ b/registry/endpoint_test.go @@ -12,7 +12,7 @@ func TestEndpointParse(t *testing.T) { {"0.0.0.0:5000", "https://0.0.0.0:5000/v1/"}, } for _, td := range testData { - e, err := newEndpoint(td.str) + e, err := newEndpoint(td.str, insecureRegistries) if err != nil { t.Errorf("%q: %s", td.str, err) } diff --git a/registry/registry_test.go b/registry/registry_test.go index aaba3f0a3..dbedefe05 100644 --- a/registry/registry_test.go +++ b/registry/registry_test.go @@ -323,6 +323,7 @@ func TestIsSecure(t *testing.T) { insecureRegistries []string expected bool }{ + {IndexServerURL.Host, nil, true}, {"example.com", []string{}, true}, {"example.com", []string{"example.com"}, false}, {"localhost", []string{"localhost:5000"}, false}, From 5e2d02ab73c06aec4ddeca6965dee3ef3cf6a04e Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Tue, 11 Nov 2014 16:31:15 -0500 Subject: [PATCH 20/26] Add the possibility of specifying a subnet for --insecure-registry Signed-off-by: Tibor Vass Conflicts: registry/endpoint.go --- daemon/config.go | 10 +++- docs/sources/reference/commandline/cli.md | 2 +- registry/endpoint.go | 61 ++++++++++++++++++++--- registry/registry_mock_test.go | 26 ++++++++++ registry/registry_test.go | 19 ++++--- 5 files changed, 103 insertions(+), 15 deletions(-) diff --git a/daemon/config.go b/daemon/config.go index 9e8d08e2a..e45e73b99 100644 --- a/daemon/config.go +++ b/daemon/config.go @@ -56,7 +56,7 @@ func (config *Config) InstallFlags() { flag.StringVar(&config.BridgeIP, []string{"#bip", "-bip"}, "", "Use this CIDR notation address for the network bridge's IP, not compatible with -b") flag.StringVar(&config.BridgeIface, []string{"b", "-bridge"}, "", "Attach containers to a pre-existing network bridge\nuse 'none' to disable container networking") flag.StringVar(&config.FixedCIDR, []string{"-fixed-cidr"}, "", "IPv4 subnet for fixed IPs (ex: 10.20.0.0/16)\nthis subnet must be nested in the bridge subnet (which is defined by -b or --bip)") - opts.ListVar(&config.InsecureRegistries, []string{"-insecure-registry"}, "Enable insecure communication with specified registries (no certificate verification for HTTPS and enable HTTP fallback)") + opts.ListVar(&config.InsecureRegistries, []string{"-insecure-registry"}, "Enable insecure communication with specified registries (no certificate verification for HTTPS and enable HTTP fallback) (e.g., localhost:5000 or 10.20.0.0/16)") flag.BoolVar(&config.InterContainerCommunication, []string{"#icc", "-icc"}, true, "Enable inter-container communication") flag.StringVar(&config.GraphDriver, []string{"s", "-storage-driver"}, "", "Force the Docker runtime to use a specific storage driver") flag.StringVar(&config.ExecDriver, []string{"e", "-exec-driver"}, "native", "Force the Docker runtime to use a specific exec driver") @@ -68,6 +68,14 @@ func (config *Config) InstallFlags() { opts.IPListVar(&config.Dns, []string{"#dns", "-dns"}, "Force Docker to use specific DNS servers") opts.DnsSearchListVar(&config.DnsSearch, []string{"-dns-search"}, "Force Docker to use specific DNS search domains") opts.MirrorListVar(&config.Mirrors, []string{"-registry-mirror"}, "Specify a preferred Docker registry mirror") + + // Localhost is by default considered as an insecure registry + // This is a stop-gap for people who are running a private registry on localhost (especially on Boot2docker). + // + // TODO: should we deprecate this once it is easier for people to set up a TLS registry or change + // daemon flags on boot2docker? + // If so, do not forget to check the TODO in TestIsSecure + config.InsecureRegistries = append(config.InsecureRegistries, "127.0.0.0/8") } func GetDefaultNetworkMtu() int { diff --git a/docs/sources/reference/commandline/cli.md b/docs/sources/reference/commandline/cli.md index a57b4cc28..728f95cee 100644 --- a/docs/sources/reference/commandline/cli.md +++ b/docs/sources/reference/commandline/cli.md @@ -70,7 +70,7 @@ expect an integer, and they can only be specified once. -g, --graph="/var/lib/docker" Path to use as the root of the Docker runtime -H, --host=[] The socket(s) to bind to in daemon mode or connect to in client mode, specified using one or more tcp://host:port, unix:///path/to/socket, fd://* or fd://socketfd. --icc=true Enable inter-container communication - --insecure-registry=[] Enable insecure communication with specified registries (no certificate verification for HTTPS and enable HTTP fallback) + --insecure-registry=[] Enable insecure communication with specified registries (no certificate verification for HTTPS and enable HTTP fallback) (ex: localhost:5000 or 10.20.0.0/16) --ip=0.0.0.0 Default IP address to use when binding container ports --ip-forward=true Enable net.ipv4.ip_forward --ip-masq=true Enable IP masquerading for bridge's IP range diff --git a/registry/endpoint.go b/registry/endpoint.go index eb0e9a1fa..d65fd7e8a 100644 --- a/registry/endpoint.go +++ b/registry/endpoint.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "io/ioutil" + "net" "net/http" "net/url" "strings" @@ -11,6 +12,9 @@ import ( "github.com/docker/docker/pkg/log" ) +// for mocking in unit tests +var lookupIP = net.LookupIP + // scans string for api version in the URL path. returns the trimmed hostname, if version found, string and API version. func scanForApiVersion(hostname string) (string, APIVersion) { var ( @@ -78,7 +82,10 @@ func newEndpoint(hostname string, insecureRegistries []string) (*Endpoint, error if err != nil { return nil, err } - endpoint.secure = isSecure(endpoint.URL.Host, insecureRegistries) + endpoint.secure, err = isSecure(endpoint.URL.Host, insecureRegistries) + if err != nil { + return nil, err + } return &endpoint, nil } @@ -151,16 +158,56 @@ func (e Endpoint) Ping() (RegistryInfo, error) { // isSecure returns false if the provided hostname is part of the list of insecure registries. // Insecure registries accept HTTP and/or accept HTTPS with certificates from unknown CAs. -func isSecure(hostname string, insecureRegistries []string) bool { +// +// The list of insecure registries can contain an element with CIDR notation to specify a whole subnet. +// If the subnet contains one of the IPs of the registry specified by hostname, the latter is considered +// insecure. +// +// hostname should be a URL.Host (`host:port` or `host`) +func isSecure(hostname string, insecureRegistries []string) (bool, error) { if hostname == IndexServerURL.Host { - return true + return true, nil } - for _, h := range insecureRegistries { - if hostname == h { - return false + host, _, err := net.SplitHostPort(hostname) + if err != nil { + // assume hostname is of the form `host` without the port and go on. + host = hostname + } + addrs, err := lookupIP(host) + if err != nil { + ip := net.ParseIP(host) + if ip == nil { + // if resolving `host` fails, error out, since host is to be net.Dial-ed anyway + return true, fmt.Errorf("issecure: could not resolve %q: %v", host, err) + } + addrs = []net.IP{ip} + } + if len(addrs) == 0 { + return true, fmt.Errorf("issecure: could not resolve %q", host) + } + + for _, addr := range addrs { + for _, r := range insecureRegistries { + // hostname matches insecure registry + if hostname == r { + return false, nil + } + + // now assume a CIDR was passed to --insecure-registry + _, ipnet, err := net.ParseCIDR(r) + if err != nil { + // if could not parse it as a CIDR, even after removing + // assume it's not a CIDR and go on with the next candidate + continue + } + + // check if the addr falls in the subnet + if ipnet.Contains(addr) { + return false, nil + } } } - return true + return true, nil } diff --git a/registry/registry_mock_test.go b/registry/registry_mock_test.go index fc2b46b9b..50724f0f9 100644 --- a/registry/registry_mock_test.go +++ b/registry/registry_mock_test.go @@ -2,9 +2,11 @@ package registry import ( "encoding/json" + "errors" "fmt" "io" "io/ioutil" + "net" "net/http" "net/http/httptest" "net/url" @@ -80,6 +82,11 @@ var ( "latest": "42d718c941f5c532ac049bf0b0ab53f0062f09a03afd4aa4a02c098e46032b9d", }, } + mockHosts = map[string][]net.IP{ + "": {net.ParseIP("0.0.0.0")}, + "localhost": {net.ParseIP("127.0.0.1"), net.ParseIP("::1")}, + "example.com": {net.ParseIP("42.42.42.42")}, + } ) func init() { @@ -106,6 +113,25 @@ func init() { panic(err) } insecureRegistries = []string{URL.Host} + + // override net.LookupIP + lookupIP = func(host string) ([]net.IP, error) { + if host == "127.0.0.1" { + // I believe in future Go versions this will fail, so let's fix it later + return net.LookupIP(host) + } + for h, addrs := range mockHosts { + if host == h { + return addrs, nil + } + for _, addr := range addrs { + if addr.String() == host { + return []net.IP{addr}, nil + } + } + } + return nil, errors.New("lookup: no such host") + } } func handlerAccessLog(handler http.Handler) http.Handler { diff --git a/registry/registry_test.go b/registry/registry_test.go index dbedefe05..1ffb44f31 100644 --- a/registry/registry_test.go +++ b/registry/registry_test.go @@ -330,19 +330,26 @@ func TestIsSecure(t *testing.T) { {"localhost:5000", []string{"localhost:5000"}, false}, {"localhost", []string{"example.com"}, false}, {"127.0.0.1:5000", []string{"127.0.0.1:5000"}, false}, - {"localhost", []string{}, false}, - {"localhost:5000", []string{}, false}, - {"127.0.0.1", []string{}, false}, + {"localhost", nil, false}, + {"localhost:5000", nil, false}, + {"127.0.0.1", nil, false}, {"localhost", []string{"example.com"}, false}, {"127.0.0.1", []string{"example.com"}, false}, - {"example.com", []string{}, true}, + {"example.com", nil, true}, {"example.com", []string{"example.com"}, false}, {"127.0.0.1", []string{"example.com"}, false}, {"127.0.0.1:5000", []string{"example.com"}, false}, + {"example.com:5000", []string{"42.42.0.0/16"}, false}, + {"example.com", []string{"42.42.0.0/16"}, false}, + {"example.com:5000", []string{"42.42.42.42/8"}, false}, + {"127.0.0.1:5000", []string{"127.0.0.0/8"}, false}, + {"42.42.42.42:5000", []string{"42.1.1.1/8"}, false}, } for _, tt := range tests { - if sec := isSecure(tt.addr, tt.insecureRegistries); sec != tt.expected { - t.Errorf("isSecure failed for %q %v, expected %v got %v", tt.addr, tt.insecureRegistries, tt.expected, sec) + // TODO: remove this once we remove localhost insecure by default + insecureRegistries := append(tt.insecureRegistries, "127.0.0.0/8") + if sec, err := isSecure(tt.addr, insecureRegistries); err != nil || sec != tt.expected { + t.Fatalf("isSecure failed for %q %v, expected %v got %v. Error: %v", tt.addr, insecureRegistries, tt.expected, sec, err) } } } From c650d17a266f3fec3c48d7e3c79afe13a6ec139a Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 14 Nov 2014 11:04:47 -0800 Subject: [PATCH 21/26] Rewrite documentation for insecure registries Signed-off-by: Tibor Vass Conflicts: docs/sources/reference/commandline/cli.md --- docs/sources/reference/commandline/cli.md | 46 ++++++++++++++++------- 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/docs/sources/reference/commandline/cli.md b/docs/sources/reference/commandline/cli.md index 728f95cee..6e6ccd69a 100644 --- a/docs/sources/reference/commandline/cli.md +++ b/docs/sources/reference/commandline/cli.md @@ -70,7 +70,7 @@ expect an integer, and they can only be specified once. -g, --graph="/var/lib/docker" Path to use as the root of the Docker runtime -H, --host=[] The socket(s) to bind to in daemon mode or connect to in client mode, specified using one or more tcp://host:port, unix:///path/to/socket, fd://* or fd://socketfd. --icc=true Enable inter-container communication - --insecure-registry=[] Enable insecure communication with specified registries (no certificate verification for HTTPS and enable HTTP fallback) (ex: localhost:5000 or 10.20.0.0/16) + --insecure-registry=[] Enable insecure communication with specified registries (disables certificate verification for HTTPS and enables HTTP fallback) (e.g., localhost:5000 or 10.20.0.0/16) --ip=0.0.0.0 Default IP address to use when binding container ports --ip-forward=true Enable net.ipv4.ip_forward --ip-masq=true Enable IP masquerading for bridge's IP range @@ -193,24 +193,44 @@ To set the DNS server for all Docker containers, use To set the DNS search domain for all Docker containers, use `docker -d --dns-search example.com`. +### Insecure registries + +Docker considers a private registry either secure or insecure. +In the rest of this section, *registry* is used for *private registry*, and `myregistry:5000` +is a placeholder example for a private registry. + +A secure registry uses TLS and a copy of its CA certificate is placed on the Docker host at +`/etc/docker/certs.d/myregistry:5000/ca.crt`. +An insecure registry is either not using TLS (i.e., listening on plain text HTTP), or is using +TLS with a CA certificate not known by the Docker daemon. The latter can happen when the +certificate was not found under `/etc/docker/certs.d/myregistry:5000/`, or if the certificate +verification failed (i.e., wrong CA). + +By default, Docker assumes all, but local (see local registries below), registries are secure. +Communicating with an insecure registry is not possible if Docker assumes that registry is secure. +In order to communicate with an insecure registry, the Docker daemon requires `--insecure-registry` +in one of the following two forms: + +* `--insecure-registry myregistry:5000` tells the Docker daemon that myregistry:5000 should be considered insecure. +* `--insecure-registry 10.1.0.0/16` tells the Docker daemon that all registries whose domain resolve to an IP address is part +of the subnet described by the CIDR syntax, should be considered insecure. + +The flag can be used multiple times to allow multiple registries to be marked as insecure. + +If an insecure registry is not marked as insecure, `docker pull`, `docker push`, and `docker search` +will result in an error message prompting the user to either secure or pass the `--insecure-registry` +flag to the Docker daemon as described above. + +Local registries, whose IP address falls in the 127.0.0.0/8 range, are automatically marked as insecure +as of Docker 1.3.2. It is not recommended to rely on this, as it may change in the future. + + ### Miscellaneous options IP masquerading uses address translation to allow containers without a public IP to talk to other machines on the Internet. This may interfere with some network topologies and can be disabled with --ip-masq=false. - -By default, Docker will assume all registries are secured via TLS with certificate verification -enabled. Prior versions of Docker used an auto fallback if a registry did not support TLS -(or if the TLS connection failed). This introduced the opportunity for Man In The Middle (MITM) -attacks, so as of Docker 1.3.1, the user must now specify the `--insecure-registry` daemon flag -for each insecure registry. An insecure registry is either not using TLS (i.e. plain text HTTP), -or is using TLS with a CA certificate not known by the Docker daemon (i.e. certification -verification disabled). For example, if there is a registry listening for HTTP at 127.0.0.1:5000, -as of Docker 1.3.1 you are required to specify `--insecure-registry 127.0.0.1:5000` when starting -the Docker daemon. - - Docker supports softlinks for the Docker data directory (`/var/lib/docker`) and for `/var/lib/docker/tmp`. The `DOCKER_TMPDIR` and the data directory can be set like this: From 87f59e3802a44aecdb8febfd34a9661e136ffcf4 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Thu, 23 Oct 2014 15:26:52 -0700 Subject: [PATCH 22/26] Skip V2 registry and immediately fallback to V1 when mirrors are provided Since V2 registry does not yet implement mirrors, when mirrors are given automatically fallback to V1 without checking V2 first. Signed-off-by: Derek McGowan (github: dmcgowan) --- graph/pull.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/graph/pull.go b/graph/pull.go index c1db605b8..05d5ec765 100644 --- a/graph/pull.go +++ b/graph/pull.go @@ -137,7 +137,7 @@ func (s *TagStore) CmdPull(job *engine.Job) engine.Status { mirrors = s.mirrors } - if isOfficial || endpoint.Version == registry.APIVersion2 { + if len(mirrors) == 0 && (isOfficial || endpoint.Version == registry.APIVersion2) { j := job.Eng.Job("trust_update_base") if err = j.Run(); err != nil { return job.Errorf("error updating trust base graph: %s", err) From 409f65bfd1bde5e45c68e2e1ecf68016ed91426b Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 18 Nov 2014 23:33:13 +0200 Subject: [PATCH 23/26] pkg/chrootarchive: provide TMPDIR for ApplyLayer Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- pkg/chrootarchive/diff.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/pkg/chrootarchive/diff.go b/pkg/chrootarchive/diff.go index 2133200c6..2653aefe9 100644 --- a/pkg/chrootarchive/diff.go +++ b/pkg/chrootarchive/diff.go @@ -3,6 +3,7 @@ package chrootarchive import ( "flag" "fmt" + "io/ioutil" "os" "runtime" "syscall" @@ -21,9 +22,16 @@ func applyLayer() { if err := syscall.Chdir("/"); err != nil { fatal(err) } - if err := archive.ApplyLayer("/", os.Stdin); err != nil { + tmpDir, err := ioutil.TempDir("/", "temp-docker-extract") + if err != nil { fatal(err) } + os.Setenv("TMPDIR", tmpDir) + if err := archive.ApplyLayer("/", os.Stdin); err != nil { + os.RemoveAll(tmpDir) + fatal(err) + } + os.RemoveAll(tmpDir) os.Exit(0) } From b256616589700f895845e3c5aa72e9b4511cd67d Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Wed, 19 Nov 2014 11:27:34 -0500 Subject: [PATCH 24/26] archive: do not call FollowSymlinkInScope in createTarFile Signed-off-by: Tibor Vass --- pkg/archive/archive.go | 15 ++++++++------- pkg/archive/archive_test.go | 14 ++++++++++++++ pkg/symlink/fs.go | 4 +--- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index c5255013a..155145f5d 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -22,7 +22,6 @@ import ( "github.com/docker/docker/pkg/log" "github.com/docker/docker/pkg/pools" "github.com/docker/docker/pkg/promise" - "github.com/docker/docker/pkg/symlink" "github.com/docker/docker/pkg/system" ) @@ -286,12 +285,14 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L } case tar.TypeSymlink: - // check for symlink breakout - if _, err := symlink.FollowSymlinkInScope(filepath.Join(filepath.Dir(path), hdr.Linkname), extractDir); err != nil { - if _, ok := err.(symlink.ErrBreakout); ok { - return breakoutError(fmt.Errorf("invalid symlink %q -> %q", path, hdr.Linkname)) - } - return err + // path -> hdr.Linkname = targetPath + // e.g. /extractDir/path/to/symlink -> ../2/file = /extractDir/path/2/file + targetPath := filepath.Join(filepath.Dir(path), hdr.Linkname) + + // the reason we don't need to check symlinks in the path (with FollowSymlinkInScope) is because + // that symlink would first have to be created, which would be caught earlier, at this very check: + if !strings.HasPrefix(targetPath, extractDir) { + return breakoutError(fmt.Errorf("invalid symlink %q -> %q", path, hdr.Linkname)) } if err := os.Symlink(hdr.Linkname, path); err != nil { return err diff --git a/pkg/archive/archive_test.go b/pkg/archive/archive_test.go index 76dab21fa..7c9db4443 100644 --- a/pkg/archive/archive_test.go +++ b/pkg/archive/archive_test.go @@ -426,6 +426,20 @@ func TestUntarInvalidSymlink(t *testing.T) { Mode: 0644, }, }, + { // try writing to victim/newdir/newfile with a symlink in the path + { + // this header needs to be before the next one, or else there is an error + Name: "dir/loophole", + Typeflag: tar.TypeSymlink, + Linkname: "../../victim", + Mode: 0755, + }, + { + Name: "dir/loophole/newdir/newfile", + Typeflag: tar.TypeReg, + Mode: 0644, + }, + }, } { if err := testBreakout("untar", "docker-TestUntarInvalidSymlink", headers); err != nil { t.Fatalf("i=%d. %v", i, err) diff --git a/pkg/symlink/fs.go b/pkg/symlink/fs.go index 09271ffc4..6ce99c6bd 100644 --- a/pkg/symlink/fs.go +++ b/pkg/symlink/fs.go @@ -10,8 +10,6 @@ import ( const maxLoopCounter = 100 -type ErrBreakout error - // FollowSymlink will follow an existing link and scope it to the root // path provided. // The role of this function is to return an absolute path in the root @@ -36,7 +34,7 @@ func FollowSymlinkInScope(link, root string) (string, error) { } if !strings.HasPrefix(filepath.Dir(link), root) { - return "", ErrBreakout(fmt.Errorf("%s is not within %s", link, root)) + return "", fmt.Errorf("%s is not within %s", link, root) } prev := "/" From 324953d74adf6803efd8796274a80ae0b0cbdbf8 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Mon, 24 Nov 2014 11:33:38 -0500 Subject: [PATCH 25/26] docs: Add 1.3.2 release notes Signed-off-by: Tibor Vass --- docs/sources/release-notes.md | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/docs/sources/release-notes.md b/docs/sources/release-notes.md index 6b2f0fd57..a19eb71b6 100644 --- a/docs/sources/release-notes.md +++ b/docs/sources/release-notes.md @@ -4,6 +4,35 @@ understanding, release #Release Notes +##Version 1.3.2 +(2014-11-24) + +This release fixes some bugs and addresses some security issues. We have also +made improvements to aspects of `docker run`. + +*Security fixes* + +Patches and changes were made to address CVE-2014-6407 and CVE-2014-6408. +Specifically, changes were made in order to: + +* Prevent host privilege escalation from an image extraction vulnerability (CVE-2014-6407). + +* Prevent container escalation from malicious security options applied to images (CVE-2014-6408). + +*Daemon fixes* + +The `--insecure-registry` flag of the `docker run` command has undergone +several refinements and additions. For details, please see the +[command-line reference](http://docs.docker.com/reference/commandline/cli/#run). + +* You can now specify a sub-net in order to set a range of registries which the Docker daemon will consider insecure. + +* By default, Docker now defines `localhost` as an insecure registry. + +* Registries can now be referenced using the Classless Inter-Domain Routing (CIDR) format. + +* When mirroring is enabled, the experimental registry v2 API is skipped. + ##Version 1.3.1 (2014-10-28) From 39fa2faad2f3d6fa5133de4eb740677202f53ef4 Mon Sep 17 00:00:00 2001 From: Tibor Vass Date: Fri, 14 Nov 2014 14:47:37 -0800 Subject: [PATCH 26/26] Bump to version v1.3.2 Signed-off-by: Tibor Vass --- CHANGELOG.md | 16 ++++++++++++++++ VERSION | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f958bbd48..2cddc6c90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## 1.3.2 (2014-11-20) + +#### Security +- Fix tar breakout vulnerability +* Extractions are now sandboxed chroot +- Security options are no longer committed to images + +#### Runtime +- Fix deadlock in `docker ps -f exited=1` +- Fix a bug when `--volumes-from` references a container that failed to start + +#### Registry ++ `--insecure-registry` now accepts CIDR notation such as 10.1.0.0/16 +* Private registries whose IPs fall in the 127.0.0.0/8 range do no need the `--insecure-registry` flag +- Skip the experimental registry v2 API when mirroring is enabled + ## 1.3.1 (2014-10-28) #### Security diff --git a/VERSION b/VERSION index 3a3cd8cc8..1892b9267 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.3.1 +1.3.2