From 7d293e3efadfc4f6a09fa4c2a36bf99e1bbf6816 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Thu, 17 Sep 2020 19:46:44 -0500 Subject: [PATCH 1/2] volume: Change owner of symlinks too This commit uses Lchown instead of Chown to change the owner of symlinks too. It doesn't change any behaviour. However, it could avoid some confusions as the symlinks are updated to the correct owner too. --- pkg/volume/volume_linux.go | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/pkg/volume/volume_linux.go b/pkg/volume/volume_linux.go index 17a1d3ef7385..5c46e67cd264 100644 --- a/pkg/volume/volume_linux.go +++ b/pkg/volume/volume_linux.go @@ -89,32 +89,22 @@ func legacyOwnershipChange(mounter Mounter, fsGroup *int64) error { } func changeFilePermission(filename string, fsGroup *int64, readonly bool, info os.FileInfo) error { - // chown and chmod pass through to the underlying file for symlinks. + err := os.Lchown(filename, -1, int(*fsGroup)) + if err != nil { + klog.Errorf("Lchown failed on %v: %v", filename, err) + } + + // chmod passes through to the underlying file for symlinks. // Symlinks have a mode of 777 but this really doesn't mean anything. // The permissions of the underlying file are what matter. // However, if one reads the mode of a symlink then chmods the symlink // with that mode, it changes the mode of the underlying file, overridden // the defaultMode and permissions initialized by the volume plugin, which - // is not what we want; thus, we skip chown/chmod for symlinks. + // is not what we want; thus, we skip chmod for symlinks. if info.Mode()&os.ModeSymlink != 0 { return nil } - stat, ok := info.Sys().(*syscall.Stat_t) - if !ok { - return nil - } - - if stat == nil { - klog.Errorf("Got nil stat_t for path %v while setting ownership of volume", filename) - return nil - } - - err := os.Chown(filename, int(stat.Uid), int(*fsGroup)) - if err != nil { - klog.Errorf("Chown failed on %v: %v", filename, err) - } - mask := rwMask if readonly { mask = roMask From 119040ac77a36c7c1ebad4544d601aaf48d69f88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mauricio=20V=C3=A1squez?= Date: Wed, 11 Nov 2020 08:29:32 -0500 Subject: [PATCH 2/2] volume: Add unit test for SetVolumeOwnership owners Former TestSetVolumeOwnership only checks the mode of the files. This commit adds a new TestSetVolumeOwnershipOwner that checks the ownership of the files. --- pkg/volume/volume_linux_test.go | 132 +++++++++++++++++++++++++++++++- 1 file changed, 131 insertions(+), 1 deletion(-) diff --git a/pkg/volume/volume_linux_test.go b/pkg/volume/volume_linux_test.go index c395106e29bf..fc141f50cf16 100644 --- a/pkg/volume/volume_linux_test.go +++ b/pkg/volume/volume_linux_test.go @@ -181,7 +181,7 @@ func TestSkipPermissionChange(t *testing.T) { } } -func TestSetVolumeOwnership(t *testing.T) { +func TestSetVolumeOwnershipMode(t *testing.T) { always := v1.FSGroupChangeAlways onrootMismatch := v1.FSGroupChangeOnRootMismatch expectedMask := rwMask | os.ModeSetgid | execMask @@ -350,3 +350,133 @@ func verifyDirectoryPermission(path string, readonly bool) bool { } return false } + +func TestSetVolumeOwnershipOwner(t *testing.T) { + fsGroup := int64(3000) + currentUid := os.Getuid() + if currentUid != 0 { + t.Skip("running as non-root") + } + currentGid := os.Getgid() + + tests := []struct { + description string + fsGroup *int64 + setupFunc func(path string) error + assertFunc func(path string) error + }{ + { + description: "fsGroup=nil", + fsGroup: nil, + setupFunc: func(path string) error { + filename := filepath.Join(path, "file.txt") + file, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0755) + if err != nil { + return err + } + file.Close() + return nil + }, + assertFunc: func(path string) error { + filename := filepath.Join(path, "file.txt") + if !verifyFileOwner(filename, currentUid, currentGid) { + return fmt.Errorf("invalid owner on %s", filename) + } + return nil + }, + }, + { + description: "*fsGroup=3000", + fsGroup: &fsGroup, + setupFunc: func(path string) error { + filename := filepath.Join(path, "file.txt") + file, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0755) + if err != nil { + return err + } + file.Close() + return nil + }, + assertFunc: func(path string) error { + filename := filepath.Join(path, "file.txt") + if !verifyFileOwner(filename, currentUid, int(fsGroup)) { + return fmt.Errorf("invalid owner on %s", filename) + } + return nil + }, + }, + { + description: "symlink", + fsGroup: &fsGroup, + setupFunc: func(path string) error { + filename := filepath.Join(path, "file.txt") + file, err := os.OpenFile(filename, os.O_RDWR|os.O_CREATE, 0755) + if err != nil { + return err + } + file.Close() + + symname := filepath.Join(path, "file_link.txt") + err = os.Symlink(filename, symname) + if err != nil { + return err + } + + return nil + }, + assertFunc: func(path string) error { + symname := filepath.Join(path, "file_link.txt") + if !verifyFileOwner(symname, currentUid, int(fsGroup)) { + return fmt.Errorf("invalid owner on %s", symname) + } + return nil + }, + }, + } + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + tmpDir, err := utiltesting.MkTmpdir("volume_linux_ownership") + if err != nil { + t.Fatalf("error creating temp dir: %v", err) + } + + defer os.RemoveAll(tmpDir) + + err = test.setupFunc(tmpDir) + if err != nil { + t.Errorf("for %s error running setup with: %v", test.description, err) + } + + mounter := &localFakeMounter{path: tmpDir} + always := v1.FSGroupChangeAlways + err = SetVolumeOwnership(mounter, test.fsGroup, &always, nil) + if err != nil { + t.Errorf("for %s error changing ownership with: %v", test.description, err) + } + err = test.assertFunc(tmpDir) + if err != nil { + t.Errorf("for %s error verifying permissions with: %v", test.description, err) + } + }) + } +} + +// verifyFileOwner checks if given path is owned by uid and gid. +// It returns true if it is otherwise false. +func verifyFileOwner(path string, uid, gid int) bool { + info, err := os.Lstat(path) + if err != nil { + return false + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok || stat == nil { + return false + } + + if int(stat.Uid) != uid || int(stat.Gid) != gid { + return false + } + + return true +}