Name this more specifically for what it does (ValidateFilePathNoBacksteps or something)

There's tons of things this could check that it doesn't (max length, invalid chars, double slashes, etc, and naming it genetically encourages adding those types of checks which could break some callers' assumptions

Nit: volumevalidation, applies to non-PV things as well

Add a test to make sure /foo/..bar is allowed

Have to validate hostPath and mount.SubPath prior to calling filepath.Join, since it cleans/collapses backsteps

this is still outstanding... this needs to be moved above the filepath.Join line above

Good catch. Since hostPath is checked in the mounter I moved the check to the top of the conditional and only check the SubPath.

@kubernetes/sig-storage-pr-reviews is the appropriate place to error on an invalid path here or in SetUp?

this question is still outstanding

Erroring in SetUp(...) would be better, since that will automatically result in a mount failure and the generation of an event associated with the pod.

@saad-ali It seems weird to me that validation would be in SetUp and there be no actual setup logic. At that point,SetUp a misnomer.

SetUp is the function that has the mounter actually act on the configuration... the only reason it's a no-op for hostpath is that no setup or checks are currently needed or performed. If you consider a plugin like nfs, if it was given an invalid hostname, it wouldn't know until it tried to perform the mount and failed in SetUp.

While I am in favor of this validation, we need to ensure the following scenarios are handled well:

• if you already have a pod with a path like this, can its status still be updated? Can it be deleted gracefully?

@liggitt Even without the apiserver validation, the kubelet would be performing the same checks on all host volumes. I don't see any easy way around that since, if i'm not mistaken, the kubelet does not have the applied psp for the pod. The kubelet only has the security context to go off of, which does not include any relevant information that would allow the kubelet to check backsteps only for pods with a PSP that has allowed host paths set. Thoughts?

@derekwaynecarr does the kubelet still use API validation to validate pods? what does it do if it is given a pod that validation determines is invalid? we don't want pods that include backsteps in their mount paths to run, but we don't want to kubelet to fall over if a pod like that existed previously and the kubelet gets upgraded.

The function should be uppercase, as being called in other package.

I don't think it is, there is a similar method in the volume package that is getting called

Neat: put it to else block to not being executed when path is empty. Or add a return to the if body.

why not use %q instead of '%s'?

I'd rather include plugin name in the error message.

Should it be t.Fatal to stop execution if we couldn't find a plugin?

TBH, I copy/pasted this part from the other test in this file and didn't pay much attention to it. I agree, should I change both tests?

I agree, should I change both tests?

I'd change only my own code. Modifying other code makes a diff huge and hard to review.

@jhorwit2 did you ever track down whether we need to be concerned with traversals in mount.MountPath?

I don't feel like that's a concern. That's the path within the container where the volume is mounted. Escaping out of that directory only yields you directories in the container you can already access; not extra info on the host.

conceptually, that's what I would expect, but I don't know enough about the implementation to know if that is actually the case. would like an ack on that from @kubernetes/sig-storage-pr-reviews

This question is also outstanding, correct?

@jhorwit2 is right, MountPath is relative to the inside of the container so it does not matter. mount.SubPath should be checked.

according to https://golang.org/src/os/path_windows.go?s=410:485#L1 windows accepts either / or \... not really sure what to do with that.

could do strings.Split(filepath.ToSlash(targetPath), "/"), I suppose

Is there anyone from sig-windows that could comment if this is ideal and/or has any impact on existing values?

cc: @bsteciuk to take a look at this

@jhorwit2 @michmike

I haven't tested out hostPath volume mounts on Windows yet, though it seems like a reasonable strategy for splitting up the path. I did find a path that looks like it would slip through though.

According to https://en.wikipedia.org/wiki/Path_(computing)#MS-DOS.2FMicrosoft_Windows_style
C:..\File.txt is a valid path, which points to a file called File.txt located in the parent directory of the current directory on drive C:.

I don't know what the current working directory would be here, but this seems like a case that would have to be checked for.

This small snippet shows it in action, as well as show the first segment which performs the backstepping, though would not be caught by the validation.

package main

import "fmt"
import "os"
import "path/filepath" 
import "strings"

func main() {
	path := "C:..\\test.file"
	os.Create(path)
	fmt.Println(strings.Split(filepath.ToSlash(path),"/")[0])
}

The output to stdout is:
C:..

If the binary is located in C:\Users\bsteciuk\go\src\test\main.exe and I run it from C:\Users\bsteciuk\go\, then it will create the file C:\Users\bsteciuk\test.file

Edit: Now that I think about it, this validation was added to fix a concern with allowedHostPaths, which would itself likely exclude the path listed above, so this may not actually be a concern.

good catch, but you are correct, allowedHostPaths bounds the hostPath, which the subpath is joined to. I think the split and segment check covers the subpath use case.

same issue here, maybe strings.Split(filepath.ToSlash(targetPath), "/")

@jhorwit2 this change is needed to properly split on windows

@liggitt pushed

since IsAbs() is also platform-dependant, should verify !filepath.IsAbs(mount.SubPath) before calling ValidatePathNoBacksteps (it's verified in the API, but the OS could differ)

Erroring in SetUp(...) would be better, since that will automatically result in a mount failure and the generation of an event associated with the pod.

@jhorwit2 is right, MountPath is relative to the inside of the container so it does not matter. mount.SubPath should be checked.