From b377dfba0c3b4f1784961274c5ae3f3db104d981 Mon Sep 17 00:00:00 2001 From: Mark Rossetti Date: Wed, 24 Apr 2024 11:54:14 -0700 Subject: [PATCH] Add funcs in pkg/filesystem/util that can actually set file permissiosn on Windows and update container log dir perms to 660 on Windows --- pkg/kubelet/kubelet.go | 19 ++- pkg/util/filesystem/defaultfs.go | 5 +- pkg/util/filesystem/util_unix.go | 10 ++ pkg/util/filesystem/util_windows.go | 156 +++++++++++++++++++++++ pkg/util/filesystem/util_windows_test.go | 118 +++++++++++++++++ 5 files changed, 300 insertions(+), 8 deletions(-) diff --git a/pkg/kubelet/kubelet.go b/pkg/kubelet/kubelet.go index 315fc73c70d..220011a9f42 100644 --- a/pkg/kubelet/kubelet.go +++ b/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() diff --git a/pkg/util/filesystem/defaultfs.go b/pkg/util/filesystem/defaultfs.go index 39673a95899..ef99bd3bc46 100644 --- a/pkg/util/filesystem/defaultfs.go +++ b/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 diff --git a/pkg/util/filesystem/util_unix.go b/pkg/util/filesystem/util_unix.go index 863deb0f9cc..d27d2640f33 100644 --- a/pkg/util/filesystem/util_unix.go +++ b/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) diff --git a/pkg/util/filesystem/util_windows.go b/pkg/util/filesystem/util_windows.go index 459477d36e7..5cdc586d61e 100644 --- a/pkg/util/filesystem/util_windows.go +++ b/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). diff --git a/pkg/util/filesystem/util_windows_test.go b/pkg/util/filesystem/util_windows_test.go index dc692e4949c..11702fce9c7 100644 --- a/pkg/util/filesystem/util_windows_test.go +++ b/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"))