Detect backsteps correctly in base path detection #61080

liggitt · 2018-03-13T05:02:27Z

Avoids false positives with atomic writer ..<timestamp> directories

Fix a regression that prevented using `subPath` volume mounts with secret, configMap, projected, and downwardAPI volumes

liggitt · 2018-03-13T05:10:02Z

@kubernetes/sig-storage-pr-reviews @kubernetes/sig-storage-bugs

liggitt · 2018-03-13T05:16:06Z

cc @tallclair

tallclair

@msau42 is OOO. I can find someone else tomorrow if @jsafrane doesn't beat me to it.

tallclair · 2018-03-13T05:22:20Z

pkg/util/mount/mount.go

@@ -325,7 +325,7 @@ func pathWithinBase(fullPath, basePath string) bool {
 	if err != nil {


This function is also implemented in Kubelet, although I'm not sure it's used anywhere? https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/util/util.go#L51

I don't see any uses of that method on master, v1.9.4, v1.8.9, or v1.7.9

$ ag 'pathWithinBase' pkg/util/mount/mount_windows_test.go 544: result := pathWithinBase(test.fullPath, test.basePath) 545: assert.Equal(t, result, test.expectedResult, "Expect result not equal with pathWithinBase(%s, %s) return: %q, expected: %q", pkg/util/mount/mount_linux_test.go 407: if pathWithinBase(test.fullPath, test.basePath) != test.expected { pkg/util/mount/mount_linux.go 732: if !pathWithinBase(evalSubPath, subpath.VolumePath) { 912: if !pathWithinBase(endDir, baseDir) { 949: if !pathWithinBase(pathname, base) { 976: if !pathWithinBase(fullExistingPath, base) { 1121: if !pathWithinBase(currentPath, base) { pkg/util/mount/mount_windows.go 329: if !pathWithinBase(currentFullPath, volumePath) { 473: if !pathWithinBase(pathname, base) { 508: if !pathWithinBase(fullExistingPath, fullBasePath) { pkg/util/mount/mount.go 322:// pathWithinBase checks if give path is within given base directory. 323:func pathWithinBase(fullPath, basePath string) bool { pkg/kubelet/util/util.go 51:func pathWithinBase(fullPath, basePath string) bool {

removed the unused function. similar faulty logic exists in pkg/util/mount/mount_windows.go lockAndCheckSubPathWithoutSymlink and findExistingPrefix

liggitt · 2018-03-13T06:14:23Z

pkg/util/mount/mount_windows.go

@@ -291,7 +291,7 @@ func lockAndCheckSubPathWithoutSymlink(volumePath, subPath string) ([]uintptr, e
 	if err != nil {
 		return []uintptr{}, fmt.Errorf("Rel(%s, %s) error: %v", volumePath, subPath, err)
 	}
-	if strings.HasPrefix(relSubPath, "..") {
+	if relSubPath == ".." || strings.HasPrefix(relSubPath, ".."+string(filepath.Separator)) {


@andyzhangx, can you verify this is correct? (does filepath.Rel always produce ..\ backstep paths on windows or would a subPath of ../... get preserved and fail this check?

liggitt · 2018-03-13T06:14:34Z

pkg/util/mount/mount_windows.go

@@ -552,7 +552,7 @@ func findExistingPrefix(base, pathname string) (string, []string, error) {
 		return base, nil, err
 	}

-	if strings.HasPrefix(rel, "..") {
+	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {


@andyzhangx same here

Avoid false positives with atomic writer ..<timestamp> directories

lorenz · 2018-03-13T12:54:13Z

Will this be released as 1.9.5 (for the 1.9 series)? Or is it better to downgrade for the time being?

liggitt · 2018-03-13T12:55:01Z

Will this be released as 1.9.5 (for the 1.9 series)?

yes, this will be cherry-picked to 1.7.x, 1.8.x, and 1.9.x

jsafrane · 2018-03-13T12:56:51Z

/lgtm

liggitt · 2018-03-13T13:18:47Z

fyi @kubernetes/sig-release-misc

k8s-github-robot · 2018-03-13T13:27:36Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@andyzhangx @jsafrane @liggitt @tallclair

Pull Request Labels

sig/release sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

dims · 2018-03-13T13:47:48Z

test/e2e/storage/subpath.go

+	Context("Atomic writer volumes", func() {
+		var err error
+
+		BeforeEach(func() {


nit - Do we need an AfterEach to cleanup the secret and configmap? How about the pods? (where do they get cleaned up?)

Do we need an AfterEach to cleanup the secret and configmap?

not really... these just hang around for the duration of the test (and if they already exist, the setup tolerates it)

where do they get cleaned up?

the entire namespace gets deleted at the end of each test, which cleans everything up

thanks @liggitt !

ericchiang · 2018-03-13T16:13:57Z

/assign @tallclair

For pkg/kubelet/OWNERS approval.

tallclair · 2018-03-13T16:53:19Z

/lgtm
Thanks!

k8s-ci-robot · 2018-03-13T16:53:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, jsafrane, liggitt, tallclair

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/kubelet/OWNERS~~ [tallclair]
~~pkg/util/mount/OWNERS~~ [jsafrane]
~~test/e2e/storage/OWNERS~~ [jsafrane,liggitt]

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

k8s-github-robot · 2018-03-13T19:26:58Z

Automatic merge from submit-queue (batch tested with PRs 60737, 60739, 61080, 60968, 60951). If you want to cherry-pick this change to another branch, please follow the instructions here.

…0-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #61080: Detect backsteps correctly in base path detection Cherry pick of #61080 on release-1.9. #61080: Detect backsteps correctly in base path detection

…0-upstream-release-1.7 Automatic merge from submit-queue. Automated cherry pick of #61080: Detect backsteps correctly in base path detection Cherry pick of #61080 on release-1.7. #61080: Detect backsteps correctly in base path detection

…0-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #61080: Detect backsteps correctly in base path detection Cherry pick of #61080 on release-1.8. #61080: Detect backsteps correctly in base path detection

k8s-ci-robot assigned jsafrane and msau42 Mar 13, 2018

k8s-ci-robot requested review from jsafrane and saad-ali March 13, 2018 05:02

liggitt force-pushed the subpath-test branch from 212ae48 to d801731 Compare March 13, 2018 05:09

liggitt added this to the v1.10 milestone Mar 13, 2018

liggitt added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 13, 2018

k8s-github-robot added the milestone/needs-approval label Mar 13, 2018

tallclair reviewed Mar 13, 2018

View reviewed changes

ericchiang added the status/approved-for-milestone label Mar 13, 2018

k8s-github-robot removed the milestone/needs-approval label Mar 13, 2018

liggitt mentioned this pull request Mar 13, 2018

"CreateContainerConfigError: failed to prepare subPath for volumeMount" error with configMap volume #61076

Closed

4 tasks

liggitt force-pushed the subpath-test branch from d801731 to 88cdab6 Compare March 13, 2018 06:11

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 Mar 13, 2018

liggitt commented Mar 13, 2018

View reviewed changes

Detect backsteps correctly in base path detection

3fafdb7

Avoid false positives with atomic writer ..<timestamp> directories

liggitt force-pushed the subpath-test branch from 88cdab6 to 3fafdb7 Compare March 13, 2018 06:23

liggitt unassigned msau42 Mar 13, 2018

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 13, 2018

Add atomic writer subpath e2e tests

806f677

liggitt force-pushed the subpath-test branch from c6476b2 to 806f677 Compare March 13, 2018 12:54

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 13, 2018

k8s-ci-robot added the sig/release Categorizes an issue or PR as relevant to SIG Release. label Mar 13, 2018

liggitt assigned tallclair Mar 13, 2018

dims reviewed Mar 13, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2018

jsafrane mentioned this pull request Mar 13, 2018

[3.9] UPSTREAM: 61045: subpath fixes openshift/origin#18957

Merged

k8s-github-robot merged commit 8313fc0 into kubernetes:master Mar 13, 2018

liggitt deleted the subpath-test branch March 14, 2018 01:04

jpbetz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Mar 14, 2018

svmaris mentioned this pull request Mar 21, 2018

Minio Fails to deploy on 1.9.4 due to ConfigMaps now being mounted ReadOnly helm/charts#4272

Closed

zegl mentioned this pull request Mar 26, 2018

Upgrade to Kubernetes 1.9.6 kubernetes/minikube#2651

Closed

primeroz mentioned this pull request Apr 4, 2018

configMap and secrets volumeMount are always mounted readOnly in 1.9.6 #62099

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect backsteps correctly in base path detection #61080

Detect backsteps correctly in base path detection #61080

liggitt commented Mar 13, 2018 •

edited

Loading

liggitt commented Mar 13, 2018

liggitt commented Mar 13, 2018

tallclair left a comment

tallclair Mar 13, 2018

ericchiang Mar 13, 2018 •

edited

Loading

liggitt Mar 13, 2018

liggitt Mar 13, 2018

liggitt Mar 13, 2018

lorenz commented Mar 13, 2018

liggitt commented Mar 13, 2018 •

edited

Loading

jsafrane commented Mar 13, 2018

liggitt commented Mar 13, 2018

k8s-github-robot commented Mar 13, 2018

dims Mar 13, 2018

liggitt Mar 13, 2018 •

edited

Loading

dims Mar 13, 2018

ericchiang commented Mar 13, 2018

tallclair commented Mar 13, 2018

k8s-ci-robot commented Mar 13, 2018

k8s-github-robot commented Mar 13, 2018

		@@ -325,7 +325,7 @@ func pathWithinBase(fullPath, basePath string) bool {
		if err != nil {

Detect backsteps correctly in base path detection #61080

Detect backsteps correctly in base path detection #61080

Conversation

liggitt commented Mar 13, 2018 • edited Loading

liggitt commented Mar 13, 2018

liggitt commented Mar 13, 2018

tallclair left a comment

Choose a reason for hiding this comment

tallclair Mar 13, 2018

Choose a reason for hiding this comment

ericchiang Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

liggitt Mar 13, 2018

Choose a reason for hiding this comment

liggitt Mar 13, 2018

Choose a reason for hiding this comment

liggitt Mar 13, 2018

Choose a reason for hiding this comment

lorenz commented Mar 13, 2018

liggitt commented Mar 13, 2018 • edited Loading

jsafrane commented Mar 13, 2018

liggitt commented Mar 13, 2018

k8s-github-robot commented Mar 13, 2018

dims Mar 13, 2018

Choose a reason for hiding this comment

liggitt Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

dims Mar 13, 2018

Choose a reason for hiding this comment

ericchiang commented Mar 13, 2018

tallclair commented Mar 13, 2018

k8s-ci-robot commented Mar 13, 2018

k8s-github-robot commented Mar 13, 2018

liggitt commented Mar 13, 2018 •

edited

Loading

ericchiang Mar 13, 2018 •

edited

Loading

liggitt commented Mar 13, 2018 •

edited

Loading

liggitt Mar 13, 2018 •

edited

Loading