New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect backsteps correctly in base path detection #61080

Merged
merged 2 commits into from Mar 13, 2018

Conversation

@liggitt
Member

liggitt commented Mar 13, 2018

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

Fixes #61076

/assign @msau42 @jsafrane

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

This comment has been minimized.

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 13, 2018

@tallclair

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

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

This comment has been minimized.

@tallclair

tallclair Mar 13, 2018

Member

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

This comment has been minimized.

@ericchiang

ericchiang Mar 13, 2018

Member

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 {

This comment has been minimized.

@liggitt

liggitt Mar 13, 2018

Member

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

@@ -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)) {

This comment has been minimized.

@liggitt

liggitt Mar 13, 2018

Member

@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?

@@ -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)) {

This comment has been minimized.

@liggitt
Detect backsteps correctly in base path detection
Avoid false positives with atomic writer ..<timestamp> directories

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 13, 2018

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 13, 2018

added an e2e that tests subpath with the four atomic writer volume types

@k8s-ci-robot k8s-ci-robot removed the lgtm label Mar 13, 2018

@lorenz

This comment has been minimized.

lorenz commented Mar 13, 2018

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

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 13, 2018

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

This comment has been minimized.

Member

jsafrane commented Mar 13, 2018

/lgtm

@liggitt

This comment has been minimized.

Member

liggitt commented Mar 13, 2018

fyi @kubernetes/sig-release-misc

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 13, 2018

[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
Context("Atomic writer volumes", func() {
var err error
BeforeEach(func() {

This comment has been minimized.

@dims

dims Mar 13, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt Mar 13, 2018

Member

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

This comment has been minimized.

@dims

dims Mar 13, 2018

Member

thanks @liggitt !

@ericchiang

This comment has been minimized.

Member

ericchiang commented Mar 13, 2018

/assign @tallclair

For pkg/kubelet/OWNERS approval.

@tallclair

This comment has been minimized.

Member

tallclair commented Mar 13, 2018

/lgtm
Thanks!

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 13, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 13, 2018

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.

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

14 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation liggitt authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-merge-robot added a commit that referenced this pull request Mar 13, 2018

Merge pull request #61107 from liggitt/automated-cherry-pick-of-#61080-…
…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

@liggitt liggitt deleted the liggitt:subpath-test branch Mar 14, 2018

k8s-merge-robot added a commit that referenced this pull request Mar 14, 2018

Merge pull request #61109 from liggitt/automated-cherry-pick-of-#61080-…
…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

k8s-merge-robot added a commit that referenced this pull request Mar 14, 2018

Merge pull request #61108 from liggitt/automated-cherry-pick-of-#61080-…
…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

@zegl zegl referenced this pull request Mar 26, 2018

Closed

Upgrade to Kubernetes 1.9.6 #2651

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment