Fix Abs path validation on Windows (kubernetes#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 <[email protected]>

* Fix abs path validation on windows

Signed-off-by: Maksym Pavlenko <[email protected]>

* Skipp path clean check for podLogDir on windows

Signed-off-by: Maksym Pavlenko <[email protected]>

* Implement IsPathClean to validate path

Signed-off-by: Maksym Pavlenko <[email protected]>

* Add warn comment for IsAbs

Signed-off-by: Maksym Pavlenko <[email protected]>

---------

Signed-off-by: Maksym Pavlenko <[email protected]>
Co-authored-by: Claudiu Belu <[email protected]>

Author: mxpv

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))
 	}
 

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)
 		}
 

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)
 		}
 	}

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)
+}

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)
+		}
+	}
+
+}

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)
+}

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, `/`)
+}

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"))
+}