diff --git a/hack/.staticcheck_failures b/hack/.staticcheck_failures index 6d9f33f9d15..4e325233cb4 100644 --- a/hack/.staticcheck_failures +++ b/hack/.staticcheck_failures @@ -26,7 +26,6 @@ pkg/credentialprovider pkg/credentialprovider/aws pkg/credentialprovider/azure pkg/kubeapiserver/admission -pkg/kubectl/cmd/cp pkg/kubectl/cmd/get pkg/kubelet/apis/podresources pkg/kubelet/cm/devicemanager diff --git a/pkg/kubectl/cmd/cp/cp.go b/pkg/kubectl/cmd/cp/cp.go index 64ef5143248..1f43cbf6403 100644 --- a/pkg/kubectl/cmd/cp/cp.go +++ b/pkg/kubectl/cmd/cp/cp.go @@ -45,6 +45,15 @@ var ( # !!!Important Note!!! # Requires that the 'tar' binary is present in your container # image. If 'tar' is not present, 'kubectl cp' will fail. + # + # For advanced use cases, such as symlinks, wildcard expansion or + # file mode preservation consider using 'kubectl exec'. + + # Copy /tmp/foo local file to /tmp/bar in a remote pod in namespace + tar cf - /tmp/foo | kubectl exec -i -n -- tar xf - -C /tmp/bar + + # Copy /tmp/foo from a remote pod to /tmp/bar locally + kubectl exec -n -- tar cf - /tmp/foo | tar xf - -C /tmp/bar # Copy /tmp/foo_dir local directory to /tmp/bar_dir in a remote pod in the default namespace kubectl cp /tmp/foo_dir :/tmp/bar_dir @@ -71,8 +80,9 @@ type CopyOptions struct { Namespace string NoPreserve bool - ClientConfig *restclient.Config - Clientset kubernetes.Interface + ClientConfig *restclient.Config + Clientset kubernetes.Interface + ExecParentCmdName string genericclioptions.IOStreams } @@ -143,6 +153,10 @@ func extractFileSpec(arg string) (fileSpec, error) { // Complete completes all the required options func (o *CopyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error { + if cmd.Parent() != nil { + o.ExecParentCmdName = cmd.Parent().CommandPath() + } + var err error o.Namespace, _, err = f.ToRawKubeConfigLoader().Namespace() if err != nil { @@ -307,7 +321,7 @@ func (o *CopyOptions) copyFromPod(src, dest fileSpec) error { // remove extraneous path shortcuts - these could occur if a path contained extra "../" // and attempted to navigate beyond "/" in a remote filesystem prefix = stripPathShortcuts(prefix) - return o.untarAll(reader, dest.File, prefix) + return o.untarAll(src, reader, dest.File, prefix) } // stripPathShortcuts removes any leading or trailing "../" from a given path @@ -412,7 +426,8 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e return nil } -func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error { +func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix string) error { + symlinkWarningPrinted := false // TODO: use compression here? tarReader := tar.NewReader(reader) for { @@ -453,48 +468,25 @@ func (o *CopyOptions) untarAll(reader io.Reader, destDir, prefix string) error { continue } - // 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. - evaledPath, err := filepath.EvalSymlinks(baseName) + if mode&os.ModeSymlink != 0 { + if !symlinkWarningPrinted && len(o.ExecParentCmdName) > 0 { + fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is a symlink, skipping (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File) + symlinkWarningPrinted = true + continue + } + fmt.Fprintf(o.IOStreams.ErrOut, "warning: skipping symlink: %q -> %q\n", destFileName, header.Linkname) + continue + } + outFile, err := os.Create(destFileName) 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, filepath.Join(evaledPath, filepath.Base(destFileName))) { - fmt.Fprintf(o.IOStreams.ErrOut, "warning: file %q is outside target destination, skipping\n", destFileName) - continue + defer outFile.Close() + if _, err := io.Copy(outFile, tarReader); err != nil { + return err } - - if mode&os.ModeSymlink != 0 { - linkname := header.Linkname - // 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. - 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 - } - if err := os.Symlink(linkname, destFileName); err != nil { - return err - } - } else { - outFile, err := os.Create(destFileName) - if err != nil { - return err - } - defer outFile.Close() - if _, err := io.Copy(outFile, tarReader); err != nil { - return err - } - if err := outFile.Close(); err != nil { - return err - } + if err := outFile.Close(); err != nil { + return err } } diff --git a/pkg/kubectl/cmd/cp/cp_test.go b/pkg/kubectl/cmd/cp/cp_test.go index 5490a8939c9..eb75a0e18b8 100644 --- a/pkg/kubectl/cmd/cp/cp_test.go +++ b/pkg/kubectl/cmd/cp/cp_test.go @@ -302,11 +302,13 @@ func TestTarUntar(t *testing.T) { { name: "gakki", data: "tmp/gakki", + omitted: true, fileType: SymLink, }, { name: "relative_to_dest", data: path.Join(dir2, "foo"), + omitted: true, fileType: SymLink, }, { @@ -358,7 +360,7 @@ func TestTarUntar(t *testing.T) { } reader := bytes.NewBuffer(writer.Bytes()) - if err := opts.untarAll(reader, dir2, ""); err != nil { + if err := opts.untarAll(fileSpec{}, reader, dir2, ""); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -419,7 +421,7 @@ func TestTarUntarWrongPrefix(t *testing.T) { } reader := bytes.NewBuffer(writer.Bytes()) - err = opts.untarAll(reader, dir2, "verylongprefix-showing-the-tar-was-tempered-with") + err = opts.untarAll(fileSpec{}, reader, dir2, "verylongprefix-showing-the-tar-was-tempered-with") if err == nil || !strings.Contains(err.Error(), "tar contents corrupted") { t.Fatalf("unexpected error: %v", err) } @@ -534,7 +536,7 @@ func TestBadTar(t *testing.T) { } opts := NewCopyOptions(genericclioptions.NewTestIOStreamsDiscard()) - if err := opts.untarAll(&buf, dir, "/prefix"); err != nil { + if err := opts.untarAll(fileSpec{}, &buf, dir, "/prefix"); err != nil { t.Errorf("unexpected error: %v ", err) t.FailNow() } @@ -780,35 +782,20 @@ func TestUntar(t *testing.T) { expected: "", }} - mkExpectation := func(expected, suffix string) string { - if expected == "" { - return "" - } - return expected + suffix - } - 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 - } links := []file{} for _, f := range files { links = append(links, file{ path: f.path + "-innerlink", linkTarget: "link-target", - expected: mkExpectation(f.expected, "-innerlink"), + expected: "", }, file{ path: f.path + "-innerlink-abs", linkTarget: filepath.Join(basedir, "link-target"), - expected: mkExpectation(f.expected, "-innerlink-abs"), + expected: "", }, file{ path: f.path + "-backlink", linkTarget: filepath.Join("..", "link-target"), - expected: mkBacklinkExpectation(f.expected, "-backlink"), + expected: "", }, file{ path: f.path + "-outerlink-abs", linkTarget: filepath.Join(testdir, "link-target"), @@ -832,7 +819,7 @@ func TestUntar(t *testing.T) { file{ path: "nested/again/back-link", linkTarget: "../../nested", - expected: filepath.Join(basedir, "nested/again/back-link"), + expected: "", }, file{ path: "nested/again/back-link/../../../back-link-file", @@ -844,7 +831,7 @@ func TestUntar(t *testing.T) { file{ path: "nested/back-link-first", linkTarget: "../", - expected: filepath.Join(basedir, "nested/back-link-first"), + expected: "", }, file{ path: "nested/back-link-first/back-link-second", @@ -892,7 +879,7 @@ func TestUntar(t *testing.T) { output := (*testWriter)(t) opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output}) - require.NoError(t, opts.untarAll(buf, filepath.Join(basedir), "")) + require.NoError(t, opts.untarAll(fileSpec{}, buf, filepath.Join(basedir), "")) filepath.Walk(testdir, func(path string, info os.FileInfo, err error) error { if err != nil { @@ -943,7 +930,7 @@ func TestUntar_SingleFile(t *testing.T) { output := (*testWriter)(t) opts := NewCopyOptions(genericclioptions.IOStreams{In: &bytes.Buffer{}, Out: output, ErrOut: output}) - require.NoError(t, opts.untarAll(buf, filepath.Join(dest), srcName)) + require.NoError(t, opts.untarAll(fileSpec{}, buf, filepath.Join(dest), srcName)) cmpFileData(t, dest, content) }