Skip to content

Commit

Permalink
Merge pull request #80436 from M00nF1sh/master
Browse files Browse the repository at this point in the history 
Refactors to kubectl CP command
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot committed Jul 22, 2019
2 parents f31d786 + 9ba6fe8 commit 08f9f2b
Show file tree
Hide file tree
Showing 3 changed files with 103 additions and 88 deletions.
1 change: 1 addition & 0 deletions pkg/kubectl/cmd/cp/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ go_test(
"//staging/src/k8s.io/cli-runtime/pkg/genericclioptions:go_default_library",
"//staging/src/k8s.io/client-go/rest/fake:go_default_library",
"//staging/src/k8s.io/kubectl/pkg/scheme:go_default_library",
"//vendor/github.com/stretchr/testify/assert:go_default_library",
"//vendor/github.com/stretchr/testify/require:go_default_library",
],
)
Expand Down
37 changes: 16 additions & 21 deletions pkg/kubectl/cmd/cp/cp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}

Expand All @@ -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
}
Expand All @@ -494,23 +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(base, link)
}

// isDestRelative returns true if dest is pointing outside the base directory,
// false otherwise.
func isDestRelative(base, dest string) bool {
fullPath := dest
if !filepath.IsAbs(dest) {
fullPath = filepath.Join(base, dest)
}
relative, err := filepath.Rel(base, fullPath)
relative, err := filepath.Rel(base, dest)
if err != nil {
return false
}
Expand Down
153 changes: 86 additions & 67 deletions pkg/kubectl/cmd/cp/cp_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"k8s.io/api/core/v1"
Expand Down Expand Up @@ -200,12 +201,12 @@ func TestIsDestRelative(t *testing.T) {
}{
{
base: "/dir",
dest: "../link",
dest: "/dir/../link",
relative: false,
},
{
base: "/dir",
dest: "../../link",
dest: "/dir/../../link",
relative: false,
},
{
Expand All @@ -215,32 +216,27 @@ func TestIsDestRelative(t *testing.T) {
},
{
base: "/dir",
dest: "link",
dest: "/dir/link",
relative: true,
},
{
base: "/dir",
dest: "int/file/link",
dest: "/dir/int/../link",
relative: true,
},
{
base: "/dir",
dest: "int/../link",
base: "dir",
dest: "dir/link",
relative: true,
},
{
base: "/dir",
dest: "/dir/link",
base: "dir",
dest: "dir/int/../link",
relative: true,
},
{
base: "/dir",
dest: "/dir/int/../link",
relative: true,
},
{
base: "/dir",
dest: "/dir/../../link",
base: "dir",
dest: "dir/../../link",
relative: false,
},
}
Expand Down Expand Up @@ -750,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
Expand All @@ -761,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: "",
Expand All @@ -792,9 +786,11 @@ func TestUntar(t *testing.T) {
}
return expected + suffix
}
mkBacktickExpectation := func(expected, suffix string) string {
dir, _ := filepath.Split(filepath.Clean(expected))
if len(strings.Split(dir, string(os.PathSeparator))) <= 1 {
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
Expand All @@ -807,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.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...)

Expand All @@ -826,32 +832,23 @@ 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.
files = append(files,
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: "",
})

Expand Down Expand Up @@ -891,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 {
Expand All @@ -916,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.Rel(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) {
Expand All @@ -937,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
}

0 comments on commit 08f9f2b

Please sign in to comment.