From be4b7176dc131ea842cab6882cd4a06dbfeed12a Mon Sep 17 00:00:00 2001 From: Maksym Pavlenko Date: Wed, 10 Apr 2024 10:13:59 -0700 Subject: [PATCH] Fix Abs path validation on Windows (#124084) * Windows: Consider slash-prefixed paths as absolute filepath.IsAbs does not consider "/" or "\" as absolute paths, even though files can be addressed as such. [1][2] Currently, there are some unit tests that are failing on Windows due to this reason. [1] https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats#traditional-dos-paths [2] https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file#fully-qualified-vs-relative-paths * Add test to verify IsAbs for windows Signed-off-by: Maksym Pavlenko * Fix abs path validation on windows Signed-off-by: Maksym Pavlenko * Skipp path clean check for podLogDir on windows Signed-off-by: Maksym Pavlenko * Implement IsPathClean to validate path Signed-off-by: Maksym Pavlenko * Add warn comment for IsAbs Signed-off-by: Maksym Pavlenko --------- Signed-off-by: Maksym Pavlenko Co-authored-by: Claudiu Belu --- .../apis/config/validation/validation.go | 6 +-- pkg/kubelet/kubelet_pods.go | 7 +-- .../kubeletconfig/configfiles/configfiles.go | 2 +- pkg/util/filesystem/util.go | 27 +++++++++++ pkg/util/filesystem/util_test.go | 46 +++++++++++++++++++ pkg/util/filesystem/util_unix.go | 6 +++ pkg/util/filesystem/util_windows.go | 12 +++++ pkg/util/filesystem/util_windows_test.go | 9 ++++ 8 files changed, 108 insertions(+), 7 deletions(-) create mode 100644 pkg/util/filesystem/util.go diff --git a/pkg/kubelet/apis/config/validation/validation.go b/pkg/kubelet/apis/config/validation/validation.go index 83cf1989f8f..6fefe704f40 100644 --- a/pkg/kubelet/apis/config/validation/validation.go +++ b/pkg/kubelet/apis/config/validation/validation.go @@ -18,7 +18,6 @@ package validation import ( "fmt" - "path/filepath" "time" "unicode" @@ -33,6 +32,7 @@ import ( "k8s.io/kubernetes/pkg/features" kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config" kubetypes "k8s.io/kubernetes/pkg/kubelet/types" + utilfs "k8s.io/kubernetes/pkg/util/filesystem" utiltaints "k8s.io/kubernetes/pkg/util/taints" "k8s.io/utils/cpuset" ) @@ -293,11 +293,11 @@ func ValidateKubeletConfiguration(kc *kubeletconfig.KubeletConfiguration, featur allErrors = append(allErrors, fmt.Errorf("invalid configuration: podLogsDir was not specified")) } - if !filepath.IsAbs(kc.PodLogsDir) { + if !utilfs.IsAbs(kc.PodLogsDir) { allErrors = append(allErrors, fmt.Errorf("invalid configuration: pod logs path %q must be absolute path", kc.PodLogsDir)) } - if filepath.Clean(kc.PodLogsDir) != kc.PodLogsDir { + if !utilfs.IsPathClean(kc.PodLogsDir) { allErrors = append(allErrors, fmt.Errorf("invalid configuration: pod logs path %q must be normalized", kc.PodLogsDir)) } diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go index 8ad7be79393..b93c8a3bd18 100644 --- a/pkg/kubelet/kubelet_pods.go +++ b/pkg/kubelet/kubelet_pods.go @@ -61,6 +61,7 @@ import ( "k8s.io/kubernetes/pkg/kubelet/status" kubetypes "k8s.io/kubernetes/pkg/kubelet/types" "k8s.io/kubernetes/pkg/kubelet/util" + utilfs "k8s.io/kubernetes/pkg/util/filesystem" utilpod "k8s.io/kubernetes/pkg/util/pod" volumeutil "k8s.io/kubernetes/pkg/volume/util" "k8s.io/kubernetes/pkg/volume/util/hostutil" @@ -199,7 +200,7 @@ func (kl *Kubelet) makeBlockVolumes(pod *v1.Pod, container *v1.Container, podVol var devices []kubecontainer.DeviceInfo for _, device := range container.VolumeDevices { // check path is absolute - if !filepath.IsAbs(device.DevicePath) { + if !utilfs.IsAbs(device.DevicePath) { return nil, fmt.Errorf("error DevicePath `%s` must be an absolute path", device.DevicePath) } vol, ok := podVolumes[device.Name] @@ -280,7 +281,7 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h } if subPath != "" { - if filepath.IsAbs(subPath) { + if utilfs.IsAbs(subPath) { return nil, cleanupAction, fmt.Errorf("error SubPath `%s` must not be an absolute path", subPath) } @@ -332,7 +333,7 @@ func makeMounts(pod *v1.Pod, podDir string, container *v1.Container, hostName, h containerPath := mount.MountPath // IsAbs returns false for UNC path/SMB shares/named pipes in Windows. So check for those specifically and skip MakeAbsolutePath - if !volumeutil.IsWindowsUNCPath(runtime.GOOS, containerPath) && !filepath.IsAbs(containerPath) { + if !volumeutil.IsWindowsUNCPath(runtime.GOOS, containerPath) && !utilfs.IsAbs(containerPath) { containerPath = volumeutil.MakeAbsolutePath(runtime.GOOS, containerPath) } diff --git a/pkg/kubelet/kubeletconfig/configfiles/configfiles.go b/pkg/kubelet/kubeletconfig/configfiles/configfiles.go index 46423cb241d..4e8fd8ea021 100644 --- a/pkg/kubelet/kubeletconfig/configfiles/configfiles.go +++ b/pkg/kubelet/kubeletconfig/configfiles/configfiles.go @@ -100,7 +100,7 @@ func resolveRelativePaths(paths []*string, root string) { for _, path := range paths { // leave empty paths alone, "no path" is a valid input // do not attempt to resolve paths that are already absolute - if len(*path) > 0 && !filepath.IsAbs(*path) { + if len(*path) > 0 && !utilfs.IsAbs(*path) { *path = filepath.Join(root, *path) } } diff --git a/pkg/util/filesystem/util.go b/pkg/util/filesystem/util.go new file mode 100644 index 00000000000..d64fc37c71b --- /dev/null +++ b/pkg/util/filesystem/util.go @@ -0,0 +1,27 @@ +/* +Copyright 2024 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package filesystem + +import ( + "path/filepath" +) + +// IsPathClean will replace slashes to Separator (which is OS-specific). +// This will make sure that all slashes are the same before comparing. +func IsPathClean(path string) bool { + return filepath.ToSlash(filepath.Clean(path)) == filepath.ToSlash(path) +} diff --git a/pkg/util/filesystem/util_test.go b/pkg/util/filesystem/util_test.go index 87ec73f067a..8b8cb5363d3 100644 --- a/pkg/util/filesystem/util_test.go +++ b/pkg/util/filesystem/util_test.go @@ -19,6 +19,7 @@ package filesystem import ( "net" "os" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -84,3 +85,48 @@ func TestIsUnixDomainSocket(t *testing.T) { assert.Equal(t, result, test.expectSocket, "Unexpected result from IsUnixDomainSocket: %v for %s", result, test.label) } } + +func TestIsCleanPath(t *testing.T) { + type Case struct { + path string + expected bool + } + + // Credits https://github.com/kubernetes/kubernetes/pull/124084/files#r1557566941 + cases := []Case{ + {path: "/logs", expected: true}, + {path: "/var/lib/something", expected: true}, + {path: "var/lib/something", expected: true}, + {path: "var\\lib\\something", expected: true}, + {path: "/", expected: true}, + {path: ".", expected: true}, + {path: "/var/../something", expected: false}, + {path: "/var//lib/something", expected: false}, + {path: "/var/./lib/something", expected: false}, + } + + // Additional cases applicable on Windows + if runtime.GOOS == "windows" { + cases = append(cases, []Case{ + {path: "\\", expected: true}, + {path: "C:/var/lib/something", expected: true}, + {path: "C:\\var\\lib\\something", expected: true}, + {path: "C:/", expected: true}, + {path: "C:\\", expected: true}, + {path: "C:/var//lib/something", expected: false}, + {path: "\\var\\\\lib\\something", expected: false}, + {path: "C:\\var\\\\lib\\something", expected: false}, + {path: "C:\\var\\..\\something", expected: false}, + {path: "\\var\\.\\lib\\something", expected: false}, + {path: "C:\\var\\.\\lib\\something", expected: false}, + }...) + } + + for _, tc := range cases { + actual := IsPathClean(tc.path) + if actual != tc.expected { + t.Errorf("actual: %t, expected: %t, for path: %s\n", actual, tc.expected, tc.path) + } + } + +} diff --git a/pkg/util/filesystem/util_unix.go b/pkg/util/filesystem/util_unix.go index df887f94508..863deb0f9cc 100644 --- a/pkg/util/filesystem/util_unix.go +++ b/pkg/util/filesystem/util_unix.go @@ -22,6 +22,7 @@ package filesystem import ( "fmt" "os" + "path/filepath" ) // IsUnixDomainSocket returns whether a given file is a AF_UNIX socket file @@ -35,3 +36,8 @@ func IsUnixDomainSocket(filePath string) (bool, error) { } return true, nil } + +// IsAbs is same as filepath.IsAbs on Unix. +func IsAbs(path string) bool { + return filepath.IsAbs(path) +} diff --git a/pkg/util/filesystem/util_windows.go b/pkg/util/filesystem/util_windows.go index cd6a11ed308..459477d36e7 100644 --- a/pkg/util/filesystem/util_windows.go +++ b/pkg/util/filesystem/util_windows.go @@ -23,6 +23,8 @@ import ( "fmt" "net" "os" + "path/filepath" + "strings" "time" "k8s.io/apimachinery/pkg/util/wait" @@ -85,3 +87,13 @@ func IsUnixDomainSocket(filePath string) (bool, error) { } return true, nil } + +// IsAbs returns whether the given path is absolute or not. +// On Windows, filepath.IsAbs will not return True for paths prefixed with a slash, even +// though they can be used as absolute paths (https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats). +// +// WARN: It isn't safe to use this for API values which will propagate across systems (e.g. REST API values +// that get validated on Unix, persisted, then consumed by Windows, etc). +func IsAbs(path string) bool { + return filepath.IsAbs(path) || strings.HasPrefix(path, `\`) || strings.HasPrefix(path, `/`) +} diff --git a/pkg/util/filesystem/util_windows_test.go b/pkg/util/filesystem/util_windows_test.go index f27880b28c9..4e87c3ddc22 100644 --- a/pkg/util/filesystem/util_windows_test.go +++ b/pkg/util/filesystem/util_windows_test.go @@ -89,3 +89,12 @@ func TestPendingUnixDomainSocket(t *testing.T) { wg.Wait() unixln.Close() } + +func TestAbsWithSlash(t *testing.T) { + // On Windows, filepath.IsAbs will not return True for paths prefixed with a slash + assert.True(t, IsAbs("/test")) + assert.True(t, IsAbs("\\test")) + + assert.False(t, IsAbs("./local")) + assert.False(t, IsAbs("local")) +}