From 46578a2359714e32d61af4efaeea86b7ff770359 Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 13 May 2014 21:39:59 +0300 Subject: [PATCH 1/4] integcli: resolve full path to docker binary Setting dockerBinary to the full path of the Docker binary is a good idea and this is now done in the test code. Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- integration-cli/docker_test_vars.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/integration-cli/docker_test_vars.go b/integration-cli/docker_test_vars.go index f8bd5c116b..78e0685a9d 100644 --- a/integration-cli/docker_test_vars.go +++ b/integration-cli/docker_test_vars.go @@ -1,7 +1,9 @@ package main import ( + "fmt" "os" + "os/exec" ) // the docker binary to use @@ -18,6 +20,15 @@ var workingDirectory string func init() { if dockerBin := os.Getenv("DOCKER_BINARY"); dockerBin != "" { dockerBinary = dockerBin + } else { + whichCmd := exec.Command("which", "docker") + out, _, err := runCommandWithOutput(whichCmd) + if err == nil { + dockerBinary = stripTrailingCharacters(out) + } else { + fmt.Printf("ERROR: couldn't resolve full path to the Docker binary") + os.Exit(1) + } } if registryImage := os.Getenv("REGISTRY_IMAGE"); registryImage != "" { registryImageName = registryImage From 1dedcd0d376f57abae5cadd38116c1aca2821330 Mon Sep 17 00:00:00 2001 From: unclejack Date: Sat, 10 May 2014 10:58:45 +0300 Subject: [PATCH 2/4] add ValidateContextDirectory to utils/utils.go This commit adds a function which can be used to ensure all contents of a directory can be accessed. This function doesn't follow symlinks to check if they're pointing to files which exist. Such symlinks can be useful later. Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- utils/utils.go | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/utils/utils.go b/utils/utils.go index 191c85206e..c3ae4e569d 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -1067,3 +1067,40 @@ func TreeSize(dir string) (size int64, err error) { }) return } + +// ValidateContextDirectory checks if all the contents of the directory +// can be read and returns an error if some files can't be read +// symlinks which point to non-existing files don't trigger an error +func ValidateContextDirectory(srcPath string) error { + var finalError error + + filepath.Walk(filepath.Join(srcPath, "."), func(filePath string, f os.FileInfo, err error) error { + // skip this directory/file if it's not in the path, it won't get added to the context + _, err = filepath.Rel(srcPath, filePath) + if err != nil && os.IsPermission(err) { + return nil + } + + if _, err := os.Stat(filePath); err != nil && os.IsPermission(err) { + finalError = fmt.Errorf("can't stat '%s'", filePath) + return err + } + // skip checking if symlinks point to non-existing files, such symlinks can be useful + lstat, _ := os.Lstat(filePath) + if lstat.Mode()&os.ModeSymlink == os.ModeSymlink { + return err + } + + if !f.IsDir() { + currentFile, err := os.Open(filePath) + if err != nil && os.IsPermission(err) { + finalError = fmt.Errorf("no permission to read from '%s'", filePath) + return err + } else { + currentFile.Close() + } + } + return nil + }) + return finalError +} From 33d5b38d62f48edcadbe239595e242300f3ecd27 Mon Sep 17 00:00:00 2001 From: unclejack Date: Sat, 10 May 2014 11:03:53 +0300 Subject: [PATCH 3/4] use ValidateContextDirectory to validate context This commit makes the Docker cli client use ValidateContextDirectory before attempting to create a tarball out of the context. This ensures we avoid errors such as "unexpected EOF" during the upload of the context. This check is done before uploading any data and can save time and bandwidth for remote Docker daemons. Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- api/client/commands.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/api/client/commands.go b/api/client/commands.go index 502c1be69e..f7eca7d358 100644 --- a/api/client/commands.go +++ b/api/client/commands.go @@ -161,6 +161,9 @@ func (cli *DockerCli) CmdBuild(args ...string) error { if _, err = os.Stat(filename); os.IsNotExist(err) { return fmt.Errorf("no Dockerfile found in %s", cmd.Arg(0)) } + if err = utils.ValidateContextDirectory(root); err != nil { + return fmt.Errorf("Error checking context is accessible: '%s'. Please check permissions and try again.", err) + } context, err = archive.Tar(root, archive.Uncompressed) } var body io.Reader From f5b1afae74a62ba9a4c89f3372dd6e5e5cb89dbf Mon Sep 17 00:00:00 2001 From: unclejack Date: Tue, 13 May 2014 21:49:12 +0300 Subject: [PATCH 4/4] add test for issue #5270 Docker-DCO-1.1-Signed-off-by: Cristian Staretu (github: unclejack) --- .../inaccessibledirectory/Dockerfile | 2 + .../directoryWeCantStat/bar | 1 + .../inaccessiblefile/Dockerfile | 2 + .../inaccessiblefile/fileWithoutReadAccess | 1 + .../linksdirectory/Dockerfile | 2 + .../linksdirectory/g | 1 + integration-cli/docker_cli_build_test.go | 88 +++++++++++++++++++ 7 files changed, 97 insertions(+) create mode 100644 integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/Dockerfile create mode 100644 integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/directoryWeCantStat/bar create mode 100644 integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/Dockerfile create mode 100644 integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/fileWithoutReadAccess create mode 100644 integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/Dockerfile create mode 120000 integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/g diff --git a/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/Dockerfile b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/Dockerfile new file mode 100644 index 0000000000..0964b8e87c --- /dev/null +++ b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/Dockerfile @@ -0,0 +1,2 @@ +FROM busybox +ADD . /foo/ diff --git a/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/directoryWeCantStat/bar b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/directoryWeCantStat/bar new file mode 100644 index 0000000000..257cc5642c --- /dev/null +++ b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/directoryWeCantStat/bar @@ -0,0 +1 @@ +foo diff --git a/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/Dockerfile b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/Dockerfile new file mode 100644 index 0000000000..0964b8e87c --- /dev/null +++ b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/Dockerfile @@ -0,0 +1,2 @@ +FROM busybox +ADD . /foo/ diff --git a/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/fileWithoutReadAccess b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/fileWithoutReadAccess new file mode 100644 index 0000000000..b25f9a2a19 --- /dev/null +++ b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/fileWithoutReadAccess @@ -0,0 +1 @@ +should make `docker build` throw an error diff --git a/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/Dockerfile b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/Dockerfile new file mode 100644 index 0000000000..0964b8e87c --- /dev/null +++ b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/Dockerfile @@ -0,0 +1,2 @@ +FROM busybox +ADD . /foo/ diff --git a/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/g b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/g new file mode 120000 index 0000000000..5fc3f33923 --- /dev/null +++ b/integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/g @@ -0,0 +1 @@ +../../../../../../../../../../../../../../../../../../../azA \ No newline at end of file diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index cd3e06d60c..c4d6f12925 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -2,8 +2,10 @@ package main import ( "fmt" + "os" "os/exec" "path/filepath" + "strings" "testing" ) @@ -119,6 +121,92 @@ func TestAddWholeDirToRoot(t *testing.T) { logDone("build - add whole directory to root") } +// Issue #5270 - ensure we throw a better error than "unexpected EOF" +// when we can't access files in the context. +func TestBuildWithInaccessibleFilesInContext(t *testing.T) { + buildDirectory := filepath.Join(workingDirectory, "build_tests", "TestBuildWithInaccessibleFilesInContext") + addUserCmd := exec.Command("adduser", "unprivilegeduser") + out, _, err := runCommandWithOutput(addUserCmd) + errorOut(err, t, fmt.Sprintf("failed to add user: %v %v", out, err)) + + { + // This is used to ensure we detect inaccessible files early during build in the cli client + pathToInaccessibleFileBuildDirectory := filepath.Join(buildDirectory, "inaccessiblefile") + pathToFileWithoutReadAccess := filepath.Join(pathToInaccessibleFileBuildDirectory, "fileWithoutReadAccess") + + err = os.Chown(pathToFileWithoutReadAccess, 0, 0) + errorOut(err, t, fmt.Sprintf("failed to chown file to root: %s", err)) + err = os.Chmod(pathToFileWithoutReadAccess, 0700) + errorOut(err, t, fmt.Sprintf("failed to chmod file to 700: %s", err)) + + buildCommandStatement := fmt.Sprintf("%s build -t inaccessiblefiles .", dockerBinary) + buildCmd := exec.Command("su", "unprivilegeduser", "-c", buildCommandStatement) + buildCmd.Dir = pathToInaccessibleFileBuildDirectory + out, exitCode, err := runCommandWithOutput(buildCmd) + if err == nil || exitCode == 0 { + t.Fatalf("build should have failed: %s %s", err, out) + } + + // check if we've detected the failure before we started building + if !strings.Contains(out, "no permission to read from ") { + t.Fatalf("output should've contained the string: no permission to read from ") + } + + if !strings.Contains(out, "Error checking context is accessible") { + t.Fatalf("output should've contained the string: Error checking context is accessible") + } + } + { + // This is used to ensure we detect inaccessible directories early during build in the cli client + pathToInaccessibleDirectoryBuildDirectory := filepath.Join(buildDirectory, "inaccessibledirectory") + pathToDirectoryWithoutReadAccess := filepath.Join(pathToInaccessibleDirectoryBuildDirectory, "directoryWeCantStat") + pathToFileInDirectoryWithoutReadAccess := filepath.Join(pathToDirectoryWithoutReadAccess, "bar") + + err = os.Chown(pathToDirectoryWithoutReadAccess, 0, 0) + errorOut(err, t, fmt.Sprintf("failed to chown directory to root: %s", err)) + err = os.Chmod(pathToDirectoryWithoutReadAccess, 0444) + errorOut(err, t, fmt.Sprintf("failed to chmod directory to 755: %s", err)) + err = os.Chmod(pathToFileInDirectoryWithoutReadAccess, 0700) + errorOut(err, t, fmt.Sprintf("failed to chmod file to 444: %s", err)) + + buildCommandStatement := fmt.Sprintf("%s build -t inaccessiblefiles .", dockerBinary) + buildCmd := exec.Command("su", "unprivilegeduser", "-c", buildCommandStatement) + buildCmd.Dir = pathToInaccessibleDirectoryBuildDirectory + out, exitCode, err := runCommandWithOutput(buildCmd) + if err == nil || exitCode == 0 { + t.Fatalf("build should have failed: %s %s", err, out) + } + + // check if we've detected the failure before we started building + if !strings.Contains(out, "can't stat") { + t.Fatalf("output should've contained the string: can't access %s", out) + } + + if !strings.Contains(out, "Error checking context is accessible") { + t.Fatalf("output should've contained the string: Error checking context is accessible") + } + + } + { + // This is used to ensure we don't follow links when checking if everything in the context is accessible + // This test doesn't require that we run commands as an unprivileged user + pathToDirectoryWhichContainsLinks := filepath.Join(buildDirectory, "linksdirectory") + + buildCmd := exec.Command(dockerBinary, "build", "-t", "testlinksok", ".") + buildCmd.Dir = pathToDirectoryWhichContainsLinks + out, exitCode, err := runCommandWithOutput(buildCmd) + if err != nil || exitCode != 0 { + t.Fatalf("build should have worked: %s %s", err, out) + } + + deleteImages("testlinksok") + + } + deleteImages("inaccessiblefiles") + logDone("build - ADD from context with inaccessible files must fail") + logDone("build - ADD from context with accessible links must work") +} + // TODO: TestCaching // TODO: TestADDCacheInvalidation