refactors to kubernetes cp command

pkg/kubectl/cmd/cp/BUILD

@@ -32,6 +32,7 @@ go_test(
         "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
         "//staging/src/k8s.io/cli-runtime/pkg/genericclioptions:go_default_library",
         "//staging/src/k8s.io/client-go/rest/fake:go_default_library",
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
         "//vendor/github.com/stretchr/testify/require:go_default_library",
     ],
 )

pkg/kubectl/cmd/cp/cp.go

@@ -436,9 +436,14 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error {
 
 		// basic file information
 		mode := header.FileInfo().Mode()
-		destFileName := path.Join(destDir, header.Name[len(prefix):])
-		baseName := path.Dir(destFileName)
+		destFileName := filepath.Join(destDir, header.Name[len(prefix):])
 
+		if !isDestRelative(destDir, destFileName) {
+			fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is outside target destination, skipping\n", destFileName)
+			continue
+		}
+
+		baseName := filepath.Dir(destFileName)
 		if err := os.MkdirAll(baseName, 0755); err != nil {
 			return err
 		}
@@ -452,15 +457,14 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error {
 		// We need to ensure that the destination file is always within boundries
 		// of the destination directory. This prevents any kind of path traversal
 		// from within tar archive.
-		dir, file := filepath.Split(destFileName)
-		evaledPath, err := filepath.EvalSymlinks(dir)
+		evaledPath, err := filepath.EvalSymlinks(baseName)
 		if err != nil {
 			return err
 		}
 		// For scrutiny we verify both the actual destination as well as we follow
 		// all the links that might lead outside of the destination directory.
-		if !isDestRelative(destDir, destFileName) || !isDestRelative(destDir, filepath.Join(evaledPath, file)) {
-			fmt.Fprintf(o.IOStreams.ErrOut, "warning: link %q is pointing to %q which is outside target destination, skipping\n", destFileName, header.Linkname)
+		if !isDestRelative(destDir, filepath.Join(evaledPath, filepath.Base(destFileName))) {
+			fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is outside target destination, skipping\n", destFileName)
 			continue
 		}
 
@@ -469,7 +473,11 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error {
 			// We need to ensure that the link destination is always within boundries
 			// of the destination directory. This prevents any kind of path traversal
 			// from within tar archive.
-			if !isDestRelative(destDir, linkJoin(destFileName, linkname)) {
+			linkTarget := linkname
+			if !filepath.IsAbs(linkname) {
+				linkTarget = filepath.Join(evaledPath, linkname)
+			}
+			if !isDestRelative(destDir, linkTarget) {
 				fmt.Fprintf(o.IOStreams.ErrOut, "warning: link %q is pointing to %q which is outside target destination, skipping\n", destFileName, header.Linkname)
 				continue
 			}
@@ -494,20 +502,10 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error {
 	return nil
 }
 
-// linkJoin joins base and link to get the final path to be created.
-// It will consider whether link is an absolute path or not when returning result.
-func linkJoin(base, link string) string {
-	if filepath.IsAbs(link) {
-		return link
-	}
-	return filepath.Join(filepath.Dir(base), link)
-}
-
 // isDestRelative returns true if dest is pointing outside the base directory,
 // false otherwise.
 func isDestRelative(base, dest string) bool {
-	fullPath := dest
-	relative, err := filepath.Rel(base, fullPath)
+	relative, err := filepath.Rel(base, dest)
 	if err != nil {
 		return false
 	}

pkg/kubectl/cmd/cp/cp_test.go

@@ -30,6 +30,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"k8s.io/api/core/v1"
@@ -745,9 +746,7 @@ func TestUntar(t *testing.T) {
 	defer os.RemoveAll(testdir)
 	t.Logf("Test base: %s", testdir)
 
-	const (
-		dest = "base"
-	)
+	basedir := filepath.Join(testdir, "base")
 
 	type file struct {
 		path       string
@@ -756,26 +755,26 @@ func TestUntar(t *testing.T) {
 	}
 	files := []file{{
 		// Absolute file within dest
-		path:     filepath.Join(testdir, dest, "abs"),
-		expected: filepath.Join(testdir, dest, testdir, dest, "abs"),
+		path:     filepath.Join(basedir, "abs"),
+		expected: filepath.Join(basedir, basedir, "abs"),
 	}, { // Absolute file outside dest
 		path:     filepath.Join(testdir, "abs-out"),
-		expected: filepath.Join(testdir, dest, testdir, "abs-out"),
+		expected: filepath.Join(basedir, testdir, "abs-out"),
 	}, { // Absolute nested file within dest
-		path:     filepath.Join(testdir, dest, "nested/nest-abs"),
-		expected: filepath.Join(testdir, dest, testdir, dest, "nested/nest-abs"),
+		path:     filepath.Join(basedir, "nested/nest-abs"),
+		expected: filepath.Join(basedir, basedir, "nested/nest-abs"),
 	}, { // Absolute nested file outside dest
-		path:     filepath.Join(testdir, dest, "nested/../../nest-abs-out"),
-		expected: filepath.Join(testdir, dest, testdir, "nest-abs-out"),
+		path:     filepath.Join(basedir, "nested/../../nest-abs-out"),
+		expected: filepath.Join(basedir, testdir, "nest-abs-out"),
 	}, { // Relative file inside dest
 		path:     "relative",
-		expected: filepath.Join(testdir, dest, "relative"),
+		expected: filepath.Join(basedir, "relative"),
 	}, { // Relative file outside dest
 		path:     "../unrelative",
 		expected: "",
 	}, { // Nested relative file inside dest
 		path:     "nested/nest-rel",
-		expected: filepath.Join(testdir, dest, "nested/nest-rel"),
+		expected: filepath.Join(basedir, "nested/nest-rel"),
 	}, { // Nested relative file outside dest
 		path:     "nested/../../nest-unrelative",
 		expected: "",
@@ -787,10 +786,11 @@ func TestUntar(t *testing.T) {
 		}
 		return expected + suffix
 	}
-	mkBacktickExpectation := func(path, expected, suffix string) string {
-		linkTarget := filepath.Join(backtick(path), "link-target")
-		baseDir := filepath.Join(testdir, dest)
-		if !isDestRelative(baseDir, linkJoin(filepath.Join(baseDir, path), linkTarget)) {
+	mkBacklinkExpectation := func(expected, suffix string) string {
+		// "resolve" the back link relative to the expectation
+		targetDir := filepath.Dir(filepath.Dir(expected))
+		// If the "resolved" target is not nested in basedir, it is escaping.
+		if !filepath.HasPrefix(targetDir, basedir) {
 			return ""
 		}
 		return expected + suffix
@@ -803,17 +803,27 @@ func TestUntar(t *testing.T) {
 			expected:   mkExpectation(f.expected, "-innerlink"),
 		}, file{
 			path:       f.path + "-innerlink-abs",
-			linkTarget: filepath.Join(testdir, dest, "link-target"),
+			linkTarget: filepath.Join(basedir, "link-target"),
 			expected:   mkExpectation(f.expected, "-innerlink-abs"),
 		}, file{
-			path:       f.path + "-outerlink",
-			linkTarget: filepath.Join(backtick(f.path), "link-target"),
-			expected:   mkBacktickExpectation(f.path, f.expected, "-outerlink"),
+			path:       f.path + "-backlink",
+			linkTarget: filepath.Join("..", "link-target"),
+			expected:   mkBacklinkExpectation(f.expected, "-backlink"),
 		}, file{
 			path:       f.path + "-outerlink-abs",
 			linkTarget: filepath.Join(testdir, "link-target"),
 			expected:   "",
 		})
+
+		if f.expected != "" {
+			// outerlink is the number of backticks to escape to testdir
+			outerlink, _ := filepath.Rel(f.expected, testdir)
+			links = append(links, file{
+				path:       f.path + "outerlink",
+				linkTarget: filepath.Join(outerlink, "link-target"),
+				expected:   "",
+			})
+		}
 	}
 	files = append(files, links...)
 
@@ -822,11 +832,11 @@ func TestUntar(t *testing.T) {
 		file{
 			path:       "nested/again/back-link",
 			linkTarget: "../../nested",
-			expected:   filepath.Join(testdir, dest, "nested/again/back-link"),
+			expected:   filepath.Join(basedir, "nested/again/back-link"),
 		},
 		file{
 			path:     "nested/again/back-link/../../../back-link-file",
-			expected: filepath.Join(testdir, dest, "back-link-file"),
+			expected: filepath.Join(basedir, "back-link-file"),
 		})
 
 	// Test chaining back-tick symlinks.
@@ -834,20 +844,11 @@ func TestUntar(t *testing.T) {
 		file{
 			path:       "nested/back-link-first",
 			linkTarget: "../",
-			expected:   filepath.Join(testdir, dest, "nested/back-link-first"),
+			expected:   filepath.Join(basedir, "nested/back-link-first"),
 		},
 		file{
 			path:       "nested/back-link-first/back-link-second",
 			linkTarget: "../",
-			expected:   filepath.Join(testdir, dest, "back-link-second"),
-		},
-		file{
-			// This case is chaining together symlinks that step back, so that
-			// if you just look at the target relative to the path it appears
-			// inside the destination directory, but if you actually follow each
-			// step of the path you end up outside the destination directory.
-			path:       "nested/back-link-first/back-link-second/back-link-term",
-			linkTarget: "",
 			expected:   "",
 		})
 
@@ -887,9 +888,11 @@ func TestUntar(t *testing.T) {
 	}
 	tw.Close()
 
-	opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard())
+	// Capture warnings to stderr for debugging.
+	output := (*testWriter)(t)
+	opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output})
 
-	require.NoError(t, opts.untarAll(buf, filepath.Join(testdir, dest), ""))
+	require.NoError(t, opts.untarAll(buf, filepath.Join(basedir), ""))
 
 	filepath.Walk(testdir, func(path string, info os.FileInfo, err error) error {
 		if err != nil {
@@ -912,10 +915,36 @@ func TestUntar(t *testing.T) {
 	}
 }
 
-// backtick returns a path to one directory up from the target
-func backtick(target string) string {
-	rel := filepath.Join(filepath.Dir(target), "../")
-	return rel
+func TestUntar_SingleFile(t *testing.T) {
+	testdir, err := ioutil.TempDir("", "test-untar")
+	require.NoError(t, err)
+	defer os.RemoveAll(testdir)
+
+	dest := filepath.Join(testdir, "target")
+
+	buf := &bytes.Buffer{}
+	tw := tar.NewWriter(buf)
+
+	const (
+		srcName = "source"
+		content = "file contents"
+	)
+	hdr := &tar.Header{
+		Name: srcName,
+		Mode: 0666,
+		Size: int64(len(content)),
+	}
+	require.NoError(t, tw.WriteHeader(hdr))
+	_, err = tw.Write([]byte(content))
+	require.NoError(t, err)
+	tw.Close()
+
+	// Capture warnings to stderr for debugging.
+	output := (*testWriter)(t)
+	opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output})
+
+	require.NoError(t, opts.untarAll(buf, filepath.Join(dest), srcName))
+	cmpFileData(t, dest, content)
 }
 
 func createTmpFile(t *testing.T, filepath, data string) {
@@ -933,20 +962,14 @@ func createTmpFile(t *testing.T, filepath, data string) {
 }
 
 func cmpFileData(t *testing.T, filePath, data string) {
-	f, err := os.Open(filePath)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
+	actual, err := ioutil.ReadFile(filePath)
+	require.NoError(t, err)
+	assert.EqualValues(t, data, actual)
+}
 
-	defer f.Close()
-	buff := &bytes.Buffer{}
-	if _, err := io.Copy(buff, f); err != nil {
-		t.Fatal(err)
-	}
-	if err := f.Close(); err != nil {
-		t.Fatal(err)
-	}
-	if data != string(buff.Bytes()) {
-		t.Fatalf("expected: %s, saw: %s", data, string(buff.Bytes()))
-	}
+type testWriter testing.T
+
+func (t *testWriter) Write(p []byte) (n int, err error) {
+	t.Logf(string(p))
+	return len(p), nil
 }