Merge pull request #5782 from unclejack/fix_5270

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

integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/Dockerfile

@@ -0,0 +1,2 @@
+FROM busybox
+ADD . /foo/

integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessibledirectory/directoryWeCantStat/bar

@@ -0,0 +1 @@
+foo

integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/Dockerfile

@@ -0,0 +1,2 @@
+FROM busybox
+ADD . /foo/

integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/inaccessiblefile/fileWithoutReadAccess

@@ -0,0 +1 @@
+should make `docker build` throw an error

integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/Dockerfile

@@ -0,0 +1,2 @@
+FROM busybox
+ADD . /foo/

integration-cli/build_tests/TestBuildWithInaccessibleFilesInContext/linksdirectory/g

@@ -0,0 +1 @@
+../../../../../../../../../../../../../../../../../../../azA

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

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

utils/utils.go

@@ -1051,3 +1051,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
+}