Skip to content

Commit

Permalink
Use separate pathSpec for local and remote to properly handle cleanin…
Browse files Browse the repository at this point in the history 
…g paths
  • Loading branch information
@soltysh @enj
soltysh authored and enj committed Nov 8, 2021
1 parent bd146ab commit 24b725f
Show file tree
Hide file tree
Showing 3 changed files with 341 additions and 180 deletions.
159 changes: 53 additions & 106 deletions staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,8 @@ import (
"io"
"io/ioutil"
"os"
"path"
"path/filepath"
"strings"

"github.com/lithammer/dedent"
"github.com/spf13/cobra"

"k8s.io/cli-runtime/pkg/genericclioptions"
Expand Down Expand Up @@ -67,12 +64,6 @@ var (
# Copy /tmp/foo from a remote pod to /tmp/bar locally
kubectl cp <some-namespace>/<some-pod>:/tmp/foo /tmp/bar`))

cpUsageStr = dedent.Dedent(`
expected 'cp <file-spec-src> <file-spec-dest> [-c container]'.
<file-spec> is:
[namespace/]pod-name:/file/path for a remote file
/file/path for a local file`)
)

// CopyOptions have the data required to perform the copy operation
Expand Down Expand Up @@ -158,6 +149,7 @@ func NewCmdCp(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.C
},
Run: func(cmd *cobra.Command, args []string) {
cmdutil.CheckErr(o.Complete(f, cmd))
cmdutil.CheckErr(o.Validate(cmd, args))
cmdutil.CheckErr(o.Run(args))
},
}
Expand All @@ -167,41 +159,36 @@ func NewCmdCp(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.C
return cmd
}

type fileSpec struct {
PodNamespace string
PodName string
File string
}

var (
errFileSpecDoesntMatchFormat = errors.New("filespec must match the canonical format: [[namespace/]pod:]file/path")
errFileCannotBeEmpty = errors.New("filepath can not be empty")
)

func extractFileSpec(arg string) (fileSpec, error) {
i := strings.Index(arg, ":")

if i == -1 {
return fileSpec{File: arg}, nil
}
// filespec starting with a semicolon is invalid
if i == 0 {
return fileSpec{}, errFileSpecDoesntMatchFormat
}
if i == -1 {
return fileSpec{
File: newLocalPath(arg),
}, nil
}

pod, file := arg[:i], arg[i+1:]
pieces := strings.Split(pod, "/")
switch len(pieces) {
case 1:
return fileSpec{
PodName: pieces[0],
File: file,
File: newRemotePath(file),
}, nil
case 2:
return fileSpec{
PodNamespace: pieces[0],
PodName: pieces[1],
File: file,
File: newRemotePath(file),
}, nil
default:
return fileSpec{}, errFileSpecDoesntMatchFormat
Expand Down Expand Up @@ -235,16 +222,13 @@ func (o *CopyOptions) Complete(f cmdutil.Factory, cmd *cobra.Command) error {
// Validate makes sure provided values for CopyOptions are valid
func (o *CopyOptions) Validate(cmd *cobra.Command, args []string) error {
if len(args) != 2 {
return cmdutil.UsageErrorf(cmd, cpUsageStr)
return fmt.Errorf("source and destination are required")
}
return nil
}

// Run performs the execution
func (o *CopyOptions) Run(args []string) error {
if len(args) < 2 {
return fmt.Errorf("source and destination are required")
}
srcSpec, err := extractFileSpec(args[0])
if err != nil {
return err
Expand All @@ -257,6 +241,9 @@ func (o *CopyOptions) Run(args []string) error {
if len(srcSpec.PodName) != 0 && len(destSpec.PodName) != 0 {
return fmt.Errorf("one of src or dest must be a local file specification")
}
if len(srcSpec.File.String()) == 0 || len(destSpec.File.String()) == 0 {
return errors.New("filepath can not be empty")
}

if len(srcSpec.PodName) != 0 {
return o.copyFromPod(srcSpec, destSpec)
Expand All @@ -283,37 +270,32 @@ func (o *CopyOptions) checkDestinationIsDir(dest fileSpec) error {
PodName: dest.PodName,
},

Command: []string{"test", "-d", dest.File},
Command: []string{"test", "-d", dest.File.String()},
Executor: &exec.DefaultRemoteExecutor{},
}

return o.execute(options)
}

func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) error {
if len(src.File) == 0 || len(dest.File) == 0 {
return errFileCannotBeEmpty
}
if _, err := os.Stat(src.File); err != nil {
if _, err := os.Stat(src.File.String()); err != nil {
return fmt.Errorf("%s doesn't exist in local filesystem", src.File)
}
reader, writer := io.Pipe()

// strip trailing slash (if any)
if dest.File != "/" && strings.HasSuffix(string(dest.File[len(dest.File)-1]), "/") {
dest.File = dest.File[:len(dest.File)-1]
}
srcFile := src.File.(localPath)
destFile := dest.File.(remotePath)

if err := o.checkDestinationIsDir(dest); err == nil {
// If no error, dest.File was found to be a directory.
// Copy specified src into it
dest.File = dest.File + "/" + path.Base(src.File)
destFile = destFile.Join(srcFile.Base())
}

go func() {
go func(src localPath, dest remotePath, writer io.WriteCloser) {
defer writer.Close()
cmdutil.CheckErr(makeTar(src.File, dest.File, writer))
}()
cmdutil.CheckErr(makeTar(src, dest, writer))
}(srcFile, destFile, writer)
var cmdArr []string

// TODO: Improve error messages by first testing if 'tar' is present in the container?
Expand All @@ -322,9 +304,9 @@ func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) e
} else {
cmdArr = []string{"tar", "-xmf", "-"}
}
destDir := path.Dir(dest.File)
if len(destDir) > 0 {
cmdArr = append(cmdArr, "-C", destDir)
destFileDir := destFile.Dir().String()
if len(destFileDir) > 0 {
cmdArr = append(cmdArr, "-C", destFileDir)
}

options.StreamOptions = exec.StreamOptions{
Expand All @@ -345,10 +327,6 @@ func (o *CopyOptions) copyToPod(src, dest fileSpec, options *exec.ExecOptions) e
}

func (o *CopyOptions) copyFromPod(src, dest fileSpec) error {
if len(src.File) == 0 || len(dest.File) == 0 {
return errFileCannotBeEmpty
}

reader, outStream := io.Pipe()
options := &exec.ExecOptions{
StreamOptions: exec.StreamOptions{
Expand All @@ -363,57 +341,36 @@ func (o *CopyOptions) copyFromPod(src, dest fileSpec) error {
},

// TODO: Improve error messages by first testing if 'tar' is present in the container?
Command: []string{"tar", "cf", "-", src.File},
Command: []string{"tar", "cf", "-", src.File.String()},
Executor: &exec.DefaultRemoteExecutor{},
}

go func() {
defer outStream.Close()
cmdutil.CheckErr(o.execute(options))
}()
prefix := getPrefix(src.File)
prefix = path.Clean(prefix)
// 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(src, reader, dest.File, prefix)
}

// stripPathShortcuts removes any leading or trailing "../" from a given path
func stripPathShortcuts(p string) string {
newPath := path.Clean(p)
trimmed := strings.TrimPrefix(newPath, "../")

for trimmed != newPath {
newPath = trimmed
trimmed = strings.TrimPrefix(newPath, "../")
}

// trim leftover {".", ".."}
if newPath == "." || newPath == ".." {
newPath = ""
}

if len(newPath) > 0 && string(newPath[0]) == "/" {
return newPath[1:]
}
srcFile := src.File.(remotePath)
destFile := dest.File.(localPath)

return newPath
// remove extraneous path shortcuts - these could occur if a path contained extra "../"
// and attempted to navigate beyond "/" in a remote filesystem
prefix := stripPathShortcuts(srcFile.StripSlashes().Clean().String())
return o.untarAll(src.PodNamespace, src.PodName, prefix, srcFile, destFile, reader)
}

func makeTar(srcPath, destPath string, writer io.Writer) error {
func makeTar(src localPath, dest remotePath, writer io.Writer) error {
// TODO: use compression here?
tarWriter := tar.NewWriter(writer)
defer tarWriter.Close()

srcPath = path.Clean(srcPath)
destPath = path.Clean(destPath)
return recursiveTar(path.Dir(srcPath), path.Base(srcPath), path.Dir(destPath), path.Base(destPath), tarWriter)
srcPath := src.Clean()
destPath := dest.Clean()
return recursiveTar(srcPath.Dir(), srcPath.Base(), destPath.Dir(), destPath.Base(), tarWriter)
}

func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) error {
srcPath := path.Join(srcBase, srcFile)
matchedPaths, err := filepath.Glob(srcPath)
func recursiveTar(srcDir, srcFile localPath, destDir, destFile remotePath, tw *tar.Writer) error {
matchedPaths, err := srcDir.Join(srcFile).Glob()
if err != nil {
return err
}
Expand All @@ -430,13 +387,14 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e
if len(files) == 0 {
//case empty directory
hdr, _ := tar.FileInfoHeader(stat, fpath)
hdr.Name = destFile
hdr.Name = destFile.String()
if err := tw.WriteHeader(hdr); err != nil {
return err
}
}
for _, f := range files {
if err := recursiveTar(srcBase, path.Join(srcFile, f.Name()), destBase, path.Join(destFile, f.Name()), tw); err != nil {
if err := recursiveTar(srcDir, srcFile.Join(newLocalPath(f.Name())),
destDir, destFile.Join(newRemotePath(f.Name())), tw); err != nil {
return err
}
}
Expand All @@ -450,7 +408,7 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e
}

hdr.Linkname = target
hdr.Name = destFile
hdr.Name = destFile.String()
if err := tw.WriteHeader(hdr); err != nil {
return err
}
Expand All @@ -460,7 +418,7 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e
if err != nil {
return err
}
hdr.Name = destFile
hdr.Name = destFile.String()

if err := tw.WriteHeader(hdr); err != nil {
return err
Expand All @@ -481,7 +439,7 @@ func recursiveTar(srcBase, srcFile, destBase, destFile string, tw *tar.Writer) e
return nil
}

func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix string) error {
func (o *CopyOptions) untarAll(ns, pod string, prefix string, src remotePath, dest localPath, reader io.Reader) error {
symlinkWarningPrinted := false
// TODO: use compression here?
tarReader := tar.NewReader(reader)
Expand All @@ -505,34 +463,38 @@ func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix s

// basic file information
mode := header.FileInfo().Mode()
destFileName := filepath.Join(destDir, header.Name[len(prefix):])
// header.Name is a name of the REMOTE file, so we need to create
// a remotePath so that it goes through appropriate processing related
// with cleaning remote paths
destFileName := dest.Join(newRemotePath(header.Name[len(prefix):]))

if !isDestRelative(destDir, destFileName) {
if !isRelative(dest, 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 {
if err := os.MkdirAll(destFileName.Dir().String(), 0755); err != nil {
return err
}
if header.FileInfo().IsDir() {
if err := os.MkdirAll(destFileName, 0755); err != nil {
if err := os.MkdirAll(destFileName.String(), 0755); err != nil {
return err
}
continue
}

if mode&os.ModeSymlink != 0 {
if !symlinkWarningPrinted && len(o.ExecParentCmdName) > 0 {
fmt.Fprintf(o.IOStreams.ErrOut, "warning: skipping symlink: %q -> %q (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n", destFileName, header.Linkname, o.ExecParentCmdName, src.PodNamespace, src.PodName, src.File)
fmt.Fprintf(o.IOStreams.ErrOut,
"warning: skipping symlink: %q -> %q (consider using \"%s exec -n %q %q -- tar cf - %q | tar xf -\")\n",
destFileName, header.Linkname, o.ExecParentCmdName, ns, pod, src)
symlinkWarningPrinted = true
continue
}
fmt.Fprintf(o.IOStreams.ErrOut, "warning: skipping symlink: %q -> %q\n", destFileName, header.Linkname)
continue
}
outFile, err := os.Create(destFileName)
outFile, err := os.Create(destFileName.String())
if err != nil {
return err
}
Expand All @@ -548,21 +510,6 @@ func (o *CopyOptions) untarAll(src fileSpec, reader io.Reader, destDir, prefix s
return nil
}

// isDestRelative returns true if dest is pointing outside the base directory,
// false otherwise.
func isDestRelative(base, dest string) bool {
relative, err := filepath.Rel(base, dest)
if err != nil {
return false
}
return relative == "." || relative == stripPathShortcuts(relative)
}

func getPrefix(file string) string {
// tar strips the leading '/' if it's there, so we will too
return strings.TrimLeft(file, "/")
}

func (o *CopyOptions) execute(options *exec.ExecOptions) error {
if len(options.Namespace) == 0 {
options.Namespace = o.Namespace
Expand Down

0 comments on commit 24b725f

Please sign in to comment.