Merge pull request #94895 from kinvolk/mauricio/fix-setvolumeownership

volume: Change owner of symlinks too

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

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