Add funcs in pkg/filesystem/util that can actually set file permissiosn

on Windows and update container log dir perms to 660 on Windows

Author: marosset

pkg/kubelet/kubelet.go

@@ -39,8 +39,9 @@ import (
 	semconv "go.opentelemetry.io/otel/semconv/v1.12.0"
 	"go.opentelemetry.io/otel/trace"
 	"k8s.io/client-go/informers"
-
 	"k8s.io/mount-utils"
+
+	utilfs "k8s.io/kubernetes/pkg/util/filesystem"
 	netutils "k8s.io/utils/net"
 
 	v1 "k8s.io/api/core/v1"
@@ -1390,7 +1391,7 @@ func (kl *Kubelet) setupDataDirs() error {
 	if err := os.MkdirAll(kl.getRootDir(), 0750); err != nil {
 		return fmt.Errorf("error creating root directory: %v", err)
 	}
-	if err := os.MkdirAll(kl.getPodLogsDir(), 0750); err != nil {
+	if err := utilfs.MkdirAll(kl.getPodLogsDir(), 0750); err != nil {
 		return fmt.Errorf("error creating pod logs root directory %q: %w", kl.getPodLogsDir(), err)
 	}
 	if err := kl.hostutil.MakeRShared(kl.getRootDir()); err != nil {
@@ -1399,17 +1400,17 @@ func (kl *Kubelet) setupDataDirs() error {
 	if err := os.MkdirAll(kl.getPodsDir(), 0750); err != nil {
 		return fmt.Errorf("error creating pods directory: %v", err)
 	}
-	if err := os.MkdirAll(kl.getPluginsDir(), 0750); err != nil {
+	if err := utilfs.MkdirAll(kl.getPluginsDir(), 0750); err != nil {
 		return fmt.Errorf("error creating plugins directory: %v", err)
 	}
-	if err := os.MkdirAll(kl.getPluginsRegistrationDir(), 0750); err != nil {
+	if err := utilfs.MkdirAll(kl.getPluginsRegistrationDir(), 0750); err != nil {
 		return fmt.Errorf("error creating plugins registry directory: %v", err)
 	}
 	if err := os.MkdirAll(kl.getPodResourcesDir(), 0750); err != nil {
 		return fmt.Errorf("error creating podresources directory: %v", err)
 	}
 	if utilfeature.DefaultFeatureGate.Enabled(features.ContainerCheckpoint) {
-		if err := os.MkdirAll(kl.getCheckpointsDir(), 0700); err != nil {
+		if err := utilfs.MkdirAll(kl.getCheckpointsDir(), 0700); err != nil {
 			return fmt.Errorf("error creating checkpoint directory: %v", err)
 		}
 	}
@@ -1502,6 +1503,14 @@ func (kl *Kubelet) initializeModules() error {
 		}
 	}
 
+	if sysruntime.GOOS == "windows" {
+		// On Windows we should not allow other users to read the logs directory
+		// to avoid allowing non-root containers from reading the logs of other containers.
+		if err := utilfs.Chmod(ContainerLogsDir, 0750); err != nil {
+			return fmt.Errorf("failed to set permissions on directory %q: %w", ContainerLogsDir, err)
+		}
+	}
+
 	// Start the image manager.
 	kl.imageManager.Start()
 

pkg/util/filesystem/defaultfs.go

@@ -72,9 +72,8 @@ func (fs *DefaultFs) Rename(oldpath, newpath string) error {
 	return os.Rename(oldpath, newpath)
 }
 
-// MkdirAll via os.MkdirAll
 func (fs *DefaultFs) MkdirAll(path string, perm os.FileMode) error {
-	return os.MkdirAll(fs.prefix(path), perm)
+	return MkdirAll(fs.prefix(path), perm)
 }
 
 // MkdirAllWithPathCheck checks if path exists already. If not, it creates a directory
@@ -97,7 +96,7 @@ func MkdirAllWithPathCheck(path string, perm os.FileMode) error {
 		return fmt.Errorf("path %v exists but is not a directory", path)
 	}
 	// If existence of path not known, attempt to create it.
-	if err := os.MkdirAll(path, perm); err != nil {
+	if err := MkdirAll(path, perm); err != nil {
 		return err
 	}
 	return nil

pkg/util/filesystem/util_unix.go

@@ -37,6 +37,16 @@ func IsUnixDomainSocket(filePath string) (bool, error) {
 	return true, nil
 }
 
+// Chmod is the same as os.Chmod on Linux.
+func Chmod(name string, mode os.FileMode) error {
+	return os.Chmod(name, mode)
+}
+
+// MkdirAll is the same as os.MkdirAll on Linux.
+func MkdirAll(path string, perm os.FileMode) error {
+	return os.MkdirAll(path, perm)
+}
+
 // IsAbs is same as filepath.IsAbs on Unix.
 func IsAbs(path string) bool {
 	return filepath.IsAbs(path)

pkg/util/filesystem/util_windows.go

@@ -29,6 +29,8 @@ import (
 
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/klog/v2"
+
+	"golang.org/x/sys/windows"
 )
 
 const (
@@ -88,6 +90,160 @@ func IsUnixDomainSocket(filePath string) (bool, error) {
 	return true, nil
 }
 
+// On Windows os.Mkdir all doesn't set any permissions so call the Chown function below to set
+// permissions once the directory is created.
+func MkdirAll(path string, perm os.FileMode) error {
+	klog.V(6).InfoS("Function MkdirAll starts", "path", path, "perm", perm)
+	err := os.MkdirAll(path, perm)
+	if err != nil {
+		return fmt.Errorf("Error creating directory %s: %v", path, err)
+	}
+
+	err = Chmod(path, perm)
+	if err != nil {
+		return fmt.Errorf("Error setting permissions for directory %s: %v", path, err)
+	}
+
+	return nil
+}
+
+const (
+	// These aren't defined in the syscall package for Windows :(
+	USER_READ      = 0x100
+	USER_WRITE     = 0x80
+	USER_EXECUTE   = 0x40
+	GROUP_READ     = 0x20
+	GROUP_WRITE    = 0x10
+	GROUP_EXECUTE  = 0x8
+	OTHERS_READ    = 0x4
+	OTHERS_WRITE   = 0x2
+	OTHERS_EXECUTE = 0x1
+	USER_ALL       = USER_READ | USER_WRITE | USER_EXECUTE
+	GROUP_ALL      = GROUP_READ | GROUP_WRITE | GROUP_EXECUTE
+	OTHERS_ALL     = OTHERS_READ | OTHERS_WRITE | OTHERS_EXECUTE
+)
+
+// On Windows os.Chmod only sets the read-only flag on files, so we need to use Windows APIs to set the desired access on files / directories.
+// The OWNER mode will set file permissions for the file owner SID, the GROUP mode will set file permissions for the file group SID,
+// and the OTHERS mode will set file permissions for BUILTIN\Users.
+// Please note that Windows containers can be run as one of two user accounts; ContainerUser or ContainerAdministrator.
+// Containers run as ContainerAdministrator will inherit permissions from BUILTIN\Administrators,
+// while containers run as ContainerUser will inherit permissions from BUILTIN\Users.
+// Windows containers do not have the ability to run as a custom user account that is known to the host so the OTHERS group mode
+// is used to grant / deny permissions of files on the hosts to the ContainerUser account.
+func Chmod(path string, filemode os.FileMode) error {
+	klog.V(6).InfoS("Function Chmod starts", "path", path, "filemode", filemode)
+	// Get security descriptor for the file
+	sd, err := windows.GetNamedSecurityInfo(
+		path,
+		windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION|windows.OWNER_SECURITY_INFORMATION|windows.GROUP_SECURITY_INFORMATION)
+	if err != nil {
+		return fmt.Errorf("Error getting security descriptor for file %s: %v", path, err)
+	}
+
+	// Get owner SID from the security descriptor for assigning USER permissions
+	owner, _, err := sd.Owner()
+	if err != nil {
+		return fmt.Errorf("Error getting owner SID for file %s: %v", path, err)
+	}
+	ownerString := owner.String()
+
+	// Get the group SID from the security descriptor for assigning GROUP permissions
+	group, _, err := sd.Group()
+	if err != nil {
+		return fmt.Errorf("Error getting group SID for file %s: %v", path, err)
+	}
+	groupString := group.String()
+
+	mask := uint32(windows.ACCESS_MASK(filemode))
+
+	// Build a new Discretionary Access Control List (DACL) with the desired permissions using
+	//the Security Descriptor Definition Language (SDDL) format.
+	// https://learn.microsoft.com/windows/win32/secauthz/security-descriptor-definition-language
+	// the DACL is a list of Access Control Entries (ACEs) where each ACE represents the permissions (Allow or Deny) for a specific SID.
+	// Each ACE has the following format:
+	//  (AceType;AceFlags;Rights;ObjectGuid;InheritObjectGuid;AccountSid)
+	// We can leave ObjectGuid and InheritObjectGuid empty for our purposes.
+
+	dacl := "D:"
+
+	// build the owner ACE
+	dacl += "(A;OICI;"
+	if mask&USER_ALL == USER_ALL {
+		dacl += "FA"
+	} else {
+		if mask&USER_READ == USER_READ {
+			dacl += "FR"
+		}
+		if mask&USER_WRITE == USER_WRITE {
+			dacl += "FW"
+		}
+		if mask&USER_EXECUTE == USER_EXECUTE {
+			dacl += "FX"
+		}
+	}
+	dacl += ";;;" + ownerString + ")"
+
+	// Build the group ACE
+	dacl += "(A;OICI;"
+	if mask&GROUP_ALL == GROUP_ALL {
+		dacl += "FA"
+	} else {
+		if mask&GROUP_READ == GROUP_READ {
+			dacl += "FR"
+		}
+		if mask&GROUP_WRITE == GROUP_WRITE {
+			dacl += "FW"
+		}
+		if mask&GROUP_EXECUTE == GROUP_EXECUTE {
+			dacl += "FX"
+		}
+	}
+	dacl += ";;;" + groupString + ")"
+
+	// Build the others ACE
+	dacl += "(A;OICI;"
+	if mask&OTHERS_ALL == OTHERS_ALL {
+		dacl += "FA"
+	} else {
+		if mask&OTHERS_READ == OTHERS_READ {
+			dacl += "FR"
+		}
+		if mask&OTHERS_WRITE == OTHERS_WRITE {
+			dacl += "FW"
+		}
+		if mask&OTHERS_EXECUTE == OTHERS_EXECUTE {
+			dacl += "FX"
+		}
+	}
+	dacl += ";;;BU)"
+
+	klog.V(6).InfoS("Setting new DACL for path", "path", path, "dacl", dacl)
+
+	// create a new security descriptor from the DACL string
+	newSD, err := windows.SecurityDescriptorFromString(dacl)
+	if err != nil {
+		return fmt.Errorf("Error creating new security descriptor from DACL string: %v", err)
+	}
+
+	// get the DACL in binary format from the newly created security descriptor
+	newDACL, _, err := newSD.DACL()
+	if err != nil {
+		return fmt.Errorf("Error getting DACL from new security descriptor: %v", err)
+	}
+
+	// Write the new security descriptor to the file
+	return windows.SetNamedSecurityInfo(
+		path,
+		windows.SE_FILE_OBJECT,
+		windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION,
+		nil, // owner SID
+		nil, // group SID
+		newDACL,
+		nil) // SACL
+}
+
 // 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).