fix paths w shortcuts when copying from pods #65189

juanvallejo · 2018-06-18T16:57:18Z

Addresses an issue where copying from a remote location containing path
shortcuts (podName:../../../tmp/foo) causes an index out of range panic.

Release note:

The "kubectl cp" command now supports path shortcuts (../) in remote paths.

cc @soltysh

soltysh

/lgtm
/approve

You might need to cherry-pick that to 1.11.

…65189-upstream-release-1.11 Automatic merge from submit-queue. Automated cherry pick of #65189: fix paths w shortcuts when copying from pods Cherry pick of #65189 on release-1.11. #65189: fix paths w shortcuts when copying from pods

liggitt · 2018-06-21T02:50:55Z

/hold
Can you describe what this is doing? It's a little hard to follow and I want to make sure this isn't reintroducing the path traversal issues with cp fixed in 1.9

juanvallejo · 2018-06-21T17:03:52Z

@liggitt

Can you describe what this is doing? It's a little hard to follow and I want to make sure this isn't reintroducing the path traversal issues with cp fixed in 1.9

This addresses a panic that occurs when a user attempts to copy from a pod, specifying a relative path containing ../:

$ kubectl cp mypod:../../../tmp/myfile ./

The panic was happening here due to the prefix being longer than the header.Name because of the path shortcuts "../../"

liggitt · 2018-06-21T17:11:53Z

pkg/kubectl/cmd/cp.go

@@ -299,6 +299,7 @@ func (o *CopyOptions) copyFromPod(src, dest fileSpec) error {
 // stripPathShortcuts removes any leading or trailing "../" from a given path
 func stripPathShortcuts(p string) string {
 	newPath := path.Clean(p)
+	newPath = strings.TrimLeft(p, "../")


TrimLeft is not correct here. I think you meant TrimPrefix. TrimLeft removes all "/" and "." characters from the front of the string. It would turn ".../.../.../test" into "test"

Updated to use strings.TrimPrefix(). Since that method only trims the first instance, I kept the call to path.Clean() above (to first resolve cases where "../foo/../foo/bar/" is given), then remove any remaining instances of "../" that occur as a prefix

liggitt · 2018-06-21T17:15:09Z

pkg/kubectl/cmd/cp_test.go

@@ -113,10 +113,14 @@ func TestGetPrefix(t *testing.T) {
 	}{
 		{
 			input:    "/foo/bar",
-			expected: "foo/bar",


this is a pretty different result, and doesn't involve any backsteps in the path... why did this change?

liggitt · 2018-06-21T17:15:41Z

pkg/kubectl/cmd/cp.go

+
+	lastIdx := -1
+	for i := len(segs) - 1; i > 0; i-- {
+		if len(segs[i]) == 0 {


can you describe the purpose of this function, and of this change?

it modified the remote path to just return the prefix without the filename, i.e. for a string "foo/bar/baz.exe" it returned "foo/bar/". I realize this changes existing behavior, particularly when attempting to copy a remote file under a different filename locally (copying remote file "/foo/bar/baz" to local file "buz" would now just create a directory "buz" and place file "baz" in it). Will undo this change

liggitt · 2018-06-21T17:17:09Z

this should not be merged as-is. it looks like this was already picked to 1.11 and merged... we should probably revert it and wait until this is ready to reintroduce. the issue this is fixing was not a regression in 1.11, correct?

juanvallejo · 2018-06-21T17:29:10Z

this should not be merged as-is. it looks like this was already picked to 1.11 and merged... we should probably revert it and wait until this is ready to reintroduce.

Okay

the issue this is fixing was not a regression in 1.11, correct?

Not a regression - just a bug that had not been caught yet

liggitt · 2018-06-21T17:42:36Z

thanks. can you open the revert against the 1.11 branch and tag sig-release and sig-cli on it?

…uts when copying from pods"

juanvallejo · 2018-06-21T20:22:17Z

thanks. can you open the revert against the 1.11 branch and tag sig-release and sig-cli on it?

#65336

juanvallejo · 2018-06-21T21:11:58Z

@liggitt thanks, comments addressed

juanvallejo · 2018-06-29T13:51:01Z

/test pull-kubernetes-e2e-gce

juanvallejo · 2018-06-29T17:28:53Z

@liggitt friendly ping

liggitt · 2018-07-03T18:38:54Z

pkg/kubectl/cmd/cp.go

+	}
+
+	// trim leftover ".."
+	newPath = strings.TrimPrefix(newPath, "..")


won't this incorrectly remove ".." from "...foo"?

add a testcase for a path containing three leading dots, since that has cropped up a few times as a mishandled case

liggitt · 2018-07-03T18:40:15Z

test/e2e/kubectl/kubectl.go

+			Testname: Kubectl, copy
+			Description: When a Pod is running, copy a known file from it to a temporary local destination.
+		*/
+		framework.ConformanceIt("should copy a file from a running Pod ", func() {


I'm not convinced this should be a conformance test. let's start with it as a normal e2e, and deal with promotion to conformance as an optional follow-up

liggitt · 2018-07-03T18:41:02Z

test/e2e/kubectl/kubectl.go

@@ -1046,6 +1048,46 @@ metadata:
 		})
 	})

+	framework.KubeDescribe("Kubectl copy", func() {
+		podYaml := substituteImageName(string(readTestFileOrDie("busybox-pod.yaml.in")))


just name this file ....yaml (no .in suffix) if it is a valid yaml file that can be created directly

soltysh · 2018-08-15T09:07:00Z

@juanvallejo I see the comments from Jordan are still not addressed,

juanvallejo · 2018-10-08T21:30:04Z

@soltysh @liggitt thanks, review comments addressed. PTAL

Addresses an issue where copying from a remote location containing path shortcuts (podName:../../../tmp/foo) causes an index out of range panic.

soltysh

/lgtm
/approve
/hold cancel

k8s-ci-robot · 2018-10-09T15:57:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/kubectl/OWNERS~~ [soltysh]
~~test/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 18, 2018

k8s-ci-robot requested review from dshulyak and mengqiy June 18, 2018 16:57

juanvallejo mentioned this pull request Jun 18, 2018

UPSTREAM: 65189: fix paths w shortcuts when copying from pods openshift/origin#20034

Merged

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch from c00442d to 212804e Compare June 18, 2018 18:01

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 18, 2018

k8s-ci-robot assigned soltysh Jun 19, 2018

soltysh approved these changes Jun 19, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 19, 2018

juanvallejo mentioned this pull request Jun 20, 2018

Automated cherry pick of #65189: fix paths w shortcuts when copying from pods #65280

Merged

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2018

liggitt reviewed Jun 21, 2018

View reviewed changes

juanvallejo added a commit to juanvallejo/kubernetes that referenced this pull request Jun 21, 2018

Revert "Automated cherry pick of kubernetes#65189: fix paths w shortc…

2e9ae72

…uts when copying from pods"

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch from 212804e to 1fa3f3e Compare June 21, 2018 21:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 21, 2018

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch from 1fa3f3e to 33dcfbc Compare June 21, 2018 21:09

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 28, 2018

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch from 122dfa5 to c37010f Compare June 28, 2018 19:27

k8s-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 28, 2018

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch 2 times, most recently from caf5bd8 to 05c25da Compare June 29, 2018 02:00

liggitt reviewed Jul 3, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2018

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch from 05c25da to f8bf95b Compare October 8, 2018 21:29

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch 2 times, most recently from b73cea9 to 3f7c7ae Compare October 9, 2018 13:29

fix paths w shortcuts when copying from pods

e55a28d

Addresses an issue where copying from a remote location containing path shortcuts (podName:../../../tmp/foo) causes an index out of range panic.

juanvallejo force-pushed the jvallejo/path-fixes-cmd-copy branch from 3f7c7ae to e55a28d Compare October 9, 2018 14:28

soltysh approved these changes Oct 9, 2018

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Oct 9, 2018

k8s-ci-robot merged commit 637ba15 into kubernetes:master Oct 9, 2018

juanvallejo deleted the jvallejo/path-fixes-cmd-copy branch October 9, 2018 17:15

juanvallejo mentioned this pull request Oct 19, 2018

UPSTREAM: 65189: fix paths w shortcuts when copying from pods openshift/origin#21315

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix paths w shortcuts when copying from pods #65189

fix paths w shortcuts when copying from pods #65189

juanvallejo commented Jun 18, 2018 •

edited

Loading

soltysh left a comment

liggitt commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

liggitt Jun 21, 2018

juanvallejo Jun 21, 2018

liggitt Jun 21, 2018

liggitt Jun 21, 2018

juanvallejo Jun 21, 2018 •

edited

Loading

liggitt commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

liggitt commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

juanvallejo commented Jun 29, 2018

juanvallejo commented Jun 29, 2018

liggitt Jul 3, 2018

liggitt Jul 3, 2018

liggitt Jul 3, 2018

liggitt Jul 3, 2018 •

edited

Loading

soltysh commented Aug 15, 2018

juanvallejo commented Oct 8, 2018

soltysh left a comment

k8s-ci-robot commented Oct 9, 2018

fix paths w shortcuts when copying from pods #65189

fix paths w shortcuts when copying from pods #65189

Conversation

juanvallejo commented Jun 18, 2018 • edited Loading

soltysh left a comment

Choose a reason for hiding this comment

liggitt commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

liggitt Jun 21, 2018

Choose a reason for hiding this comment

juanvallejo Jun 21, 2018

Choose a reason for hiding this comment

liggitt Jun 21, 2018

Choose a reason for hiding this comment

liggitt Jun 21, 2018

Choose a reason for hiding this comment

juanvallejo Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

liggitt commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

liggitt commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

juanvallejo commented Jun 21, 2018

juanvallejo commented Jun 29, 2018

juanvallejo commented Jun 29, 2018

liggitt Jul 3, 2018

Choose a reason for hiding this comment

liggitt Jul 3, 2018

Choose a reason for hiding this comment

liggitt Jul 3, 2018

Choose a reason for hiding this comment

liggitt Jul 3, 2018 • edited Loading

Choose a reason for hiding this comment

soltysh commented Aug 15, 2018

juanvallejo commented Oct 8, 2018

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 9, 2018

juanvallejo commented Jun 18, 2018 •

edited

Loading

juanvallejo Jun 21, 2018 •

edited

Loading

liggitt Jul 3, 2018 •

edited

Loading