Merge pull request #126104 from cji/5321

Add funcs in pkg/filesystem/util to set file permissions on Windows and update container log dir perms

pkg/kubelet/kubelet.go

@@ -40,8 +40,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"
@@ -1400,7 +1401,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 {
@@ -1409,17 +1410,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)
 		}
 	}
@@ -1512,6 +1513,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 Unix.
+func Chmod(name string, mode os.FileMode) error {
+	return os.Chmod(name, mode)
+}
+
+// MkdirAll is same as os.MkdirAll on Unix.
+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).

pkg/util/filesystem/util_windows_test.go

@@ -20,9 +20,12 @@ limitations under the License.
 package filesystem
 
 import (
+	"fmt"
 	"math/rand"
 	"net"
 	"os"
+	"path/filepath"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -30,6 +33,8 @@ import (
 	winio "github.com/Microsoft/go-winio"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"golang.org/x/sys/windows"
 )
 
 func TestIsUnixDomainSocketPipe(t *testing.T) {
@@ -89,6 +94,119 @@ func TestPendingUnixDomainSocket(t *testing.T) {
 	unixln.Close()
 }
 
+func TestWindowsChmod(t *testing.T) {
+	// Note: OWNER will be replaced with the actual owner SID in the test cases
+	testCases := []struct {
+		fileMode           os.FileMode
+		expectedDescriptor string
+	}{
+		{
+			fileMode:           0777,
+			expectedDescriptor: "O:OWNERG:BAD:PAI(A;OICI;FA;;;OWNER)(A;OICI;FA;;;BA)(A;OICI;FA;;;BU)",
+		},
+		{
+			fileMode:           0750,
+			expectedDescriptor: "O:OWNERG:BAD:PAI(A;OICI;FA;;;OWNER)(A;OICI;0x1200a9;;;BA)", // 0x1200a9 = GENERIC_READ | GENERIC_EXECUTE
+		},
+		{
+			fileMode:           0664,
+			expectedDescriptor: "O:OWNERG:BAD:PAI(A;OICI;0x12019f;;;OWNER)(A;OICI;0x12019f;;;BA)(A;OICI;FR;;;BU)", // 0x12019f = GENERIC_READ | GENERIC_WRITE
+		},
+	}
+
+	for _, testCase := range testCases {
+		tempDir, err := os.MkdirTemp("", "test-dir")
+		require.NoError(t, err, "Failed to create temporary directory.")
+		defer os.RemoveAll(tempDir)
+
+		// Set the file GROUP to BUILTIN\Administrators (BA) for test determinism and
+		err = setGroupInfo(tempDir, "S-1-5-32-544")
+		require.NoError(t, err, "Failed to set group for directory.")
+
+		err = Chmod(tempDir, testCase.fileMode)
+		require.NoError(t, err, "Failed to set permissions for directory.")
+
+		owner, descriptor, err := getPermissionsInfo(tempDir)
+		require.NoError(t, err, "Failed to get permissions for directory.")
+
+		expectedDescriptor := strings.ReplaceAll(testCase.expectedDescriptor, "OWNER", owner)
+
+		assert.Equal(t, expectedDescriptor, descriptor, "Unexpected DACL for directory. when setting permissions to %o", testCase.fileMode)
+	}
+}
+
+// Gets the owner and entire security descriptor of a file or directory in the SDDL format
+// https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-definition-language
+func getPermissionsInfo(path string) (string, string, error) {
+	sd, err := windows.GetNamedSecurityInfo(
+		path,
+		windows.SE_FILE_OBJECT,
+		windows.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)
+	}
+
+	owner, _, err := sd.Owner()
+	if err != nil {
+		return "", "", fmt.Errorf("Error getting owner SID for file %s: %v", path, err)
+	}
+
+	sdString := sd.String()
+
+	return owner.String(), sdString, nil
+}
+
+// Sets the GROUP of a file or a directory to the specified group
+func setGroupInfo(path, group string) error {
+	groupSID, err := windows.StringToSid(group)
+	if err != nil {
+		return fmt.Errorf("Error converting group name %s to SID: %v", group, err)
+
+	}
+
+	err = windows.SetNamedSecurityInfo(
+		path,
+		windows.SE_FILE_OBJECT,
+		windows.GROUP_SECURITY_INFORMATION,
+		nil, // owner SID
+		groupSID,
+		nil, // DACL
+		nil, //SACL
+	)
+
+	if err != nil {
+		return fmt.Errorf("Error setting group SID for file %s: %v", path, err)
+	}
+
+	return nil
+}
+
+// TestDeleteFilePermissions tests that when a folder's permissions are set to 0660, child items
+// cannot be deleted in the folder but when a folder's permissions are set to 0770, child items can be deleted.
+func TestDeleteFilePermissions(t *testing.T) {
+	tempDir, err := os.MkdirTemp("", "test-dir")
+	require.NoError(t, err, "Failed to create temporary directory.")
+
+	err = Chmod(tempDir, 0660)
+	require.NoError(t, err, "Failed to set permissions for directory to 0660.")
+
+	filePath := filepath.Join(tempDir, "test-file")
+	err = os.WriteFile(filePath, []byte("test"), 0440)
+	require.NoError(t, err, "Failed to create file in directory.")
+
+	err = os.Remove(filePath)
+	require.Error(t, err, "Expected expected error when trying to remove file in directory.")
+
+	err = Chmod(tempDir, 0770)
+	require.NoError(t, err, "Failed to set permissions for directory to 0770.")
+
+	err = os.Remove(filePath)
+	require.NoError(t, err, "Failed to remove file in directory.")
+
+	err = os.Remove(tempDir)
+	require.NoError(t, err, "Failed to remove directory.")
+}
+
 func TestAbsWithSlash(t *testing.T) {
 	// On Windows, filepath.IsAbs will not return True for paths prefixed with a slash
 	assert.True(t, IsAbs("/test"))