Set permissions on volume before publishing update

This change fixes a race condition that was caused by setting the file owner, group and mode non-atomically, after the updated files had been published. Users who were running non-root containers, without GID 0 permissions, and had removed read permissions from other users by setting defaultMode: 0440 or similar, were getting intermittent permission denied errors when accessing files on secret or configmap volumes or service account tokens on projected volumes during update.
kubernetes · Dec 24, 2022 · 96306f1 · 96306f1
1 parent eaa7e1c
commit 96306f1
Show file tree

Hide file tree

Showing 7 changed files with 116 additions and 45 deletions.
diff --git a/pkg/volume/cephfs/cephfs.go b/pkg/volume/cephfs/cephfs.go
@@ -368,7 +368,7 @@ func (cephfsVolume *cephfs) execFuseMount(mountpoint string) error {
 			return err
 		}
 
-		err = writer.Write(payload)
+		err = writer.Write(payload, nil /*setPerms*/)
 		if err != nil {
 			klog.Errorf("failed to write payload to dir: %v", err)
 			return err

diff --git a/pkg/volume/configmap/configmap.go b/pkg/volume/configmap/configmap.go
@@ -249,17 +249,17 @@ func (b *configMapVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA
 		return err
 	}
 
-	err = writer.Write(payload)
+	setPerms := func(_ string) error {
+		// This may be the first time writing and new files get created outside the timestamp subdirectory:
+		// change the permissions on the whole volume and not only in the timestamp directory.
+		return volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
+	}
+	err = writer.Write(payload, setPerms)
 	if err != nil {
 		klog.Errorf("Error writing payload to dir: %v", err)
 		return err
 	}
 
-	err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
-	if err != nil {
-		klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup)
-		return err
-	}
 	setupSuccess = true
 	return nil
 }

diff --git a/pkg/volume/downwardapi/downwardapi.go b/pkg/volume/downwardapi/downwardapi.go
@@ -220,15 +220,14 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, mounterArgs volume.Mounte
 		return err
 	}
 
-	err = writer.Write(data)
-	if err != nil {
-		klog.Errorf("Error writing payload to dir: %v", err)
-		return err
+	setPerms := func(_ string) error {
+		// This may be the first time writing and new files get created outside the timestamp subdirectory:
+		// change the permissions on the whole volume and not only in the timestamp directory.
+		return volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
 	}
-
-	err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
+	err = writer.Write(data, setPerms)
 	if err != nil {
-		klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup)
+		klog.Errorf("Error writing payload to dir: %v", err)
 		return err
 	}
 

diff --git a/pkg/volume/projected/projected.go b/pkg/volume/projected/projected.go
@@ -230,17 +230,17 @@ func (s *projectedVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterA
 		return err
 	}
 
-	err = writer.Write(data)
+	setPerms := func(_ string) error {
+		// This may be the first time writing and new files get created outside the timestamp subdirectory:
+		// change the permissions on the whole volume and not only in the timestamp directory.
+		return volume.SetVolumeOwnership(s, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil))
+	}
+	err = writer.Write(data, setPerms)
 	if err != nil {
 		klog.Errorf("Error writing payload to dir: %v", err)
 		return err
 	}
 
-	err = volume.SetVolumeOwnership(s, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(s.plugin, nil))
-	if err != nil {
-		klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup)
-		return err
-	}
 	setupSuccess = true
 	return nil
 }

diff --git a/pkg/volume/secret/secret.go b/pkg/volume/secret/secret.go
@@ -244,17 +244,17 @@ func (b *secretVolumeMounter) SetUpAt(dir string, mounterArgs volume.MounterArgs
 		return err
 	}
 
-	err = writer.Write(payload)
+	setPerms := func(_ string) error {
+		// This may be the first time writing and new files get created outside the timestamp subdirectory:
+		// change the permissions on the whole volume and not only in the timestamp directory.
+		return volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
+	}
+	err = writer.Write(payload, setPerms)
 	if err != nil {
 		klog.Errorf("Error writing payload to dir: %v", err)
 		return err
 	}
 
-	err = volume.SetVolumeOwnership(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil))
-	if err != nil {
-		klog.Errorf("Error applying volume ownership settings for group: %v", mounterArgs.FsGroup)
-		return err
-	}
 	setupSuccess = true
 	return nil
 }

diff --git a/pkg/volume/util/atomic_writer.go b/pkg/volume/util/atomic_writer.go
@@ -86,25 +86,36 @@ const (
 
 // Write does an atomic projection of the given payload into the writer's target
 // directory.  Input paths must not begin with '..'.
+// setPerms is an optional pointer to a function that caller can provide to set the
+// permissions of the newly created files before they are published. The function is
+// passed subPath which is the name of the timestamped directory that was created
+// under target directory.
 //
 // The Write algorithm is:
 //
 //  1. The payload is validated; if the payload is invalid, the function returns
-//     2.  The current timestamped directory is detected by reading the data directory
+//
+//  2. The current timestamped directory is detected by reading the data directory
 //     symlink
 //
 //  3. The old version of the volume is walked to determine whether any
 //     portion of the payload was deleted and is still present on disk.
 //
 //  4. The data in the current timestamped directory is compared to the projected
 //     data to determine if an update is required.
-//     5.  A new timestamped dir is created
 //
-//  6. The payload is written to the new timestamped directory
-//     7.  A symlink to the new timestamped directory ..data_tmp is created that will
-//     become the new data directory
-//     8.  The new data directory symlink is renamed to the data directory; rename is atomic
-//     9.  Symlinks and directory for new user-visible files are created (if needed).
+//  5. A new timestamped dir is created.
+//
+//  6. The payload is written to the new timestamped directory.
+//
+//  7. Permissions are set (if setPerms is not nil) on the new timestamped directory and files.
+//
+//  8. A symlink to the new timestamped directory ..data_tmp is created that will
+//     become the new data directory.
+//
+//  9. The new data directory symlink is renamed to the data directory; rename is atomic.
+//
+//  10. Symlinks and directory for new user-visible files are created (if needed).
 //
 //     For example, consider the files:
 //     <target-dir>/podName
@@ -123,9 +134,10 @@ const (
 //     linking everything else. On Windows, if a target does not exist, the created symlink
 //     will not work properly if the target ends up being a directory.
 //
-// 10.  Old paths are removed from the user-visible portion of the target directory
-// 11.  The previous timestamped directory is removed, if it exists
-func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
+//  11. Old paths are removed from the user-visible portion of the target directory.
+//
+//  12. The previous timestamped directory is removed, if it exists.
+func (w *AtomicWriter) Write(payload map[string]FileProjection, setPerms func(subPath string) error) error {
 	// (1)
 	cleanPayload, err := validatePayload(payload)
 	if err != nil {
@@ -185,14 +197,22 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 	klog.V(4).Infof("%s: performed write of new data to ts data directory: %s", w.logContext, tsDir)
 
 	// (7)
+	if setPerms != nil {
+		if err := setPerms(tsDirName); err != nil {
+			klog.Errorf("%s: error applying ownership settings: %v", w.logContext, err)
+			return err
+		}
+	}
+
+	// (8)
 	newDataDirPath := filepath.Join(w.targetDir, newDataDirName)
 	if err = os.Symlink(tsDirName, newDataDirPath); err != nil {
 		os.RemoveAll(tsDir)
 		klog.Errorf("%s: error creating symbolic link for atomic update: %v", w.logContext, err)
 		return err
 	}
 
-	// (8)
+	// (9)
 	if runtime.GOOS == "windows" {
 		os.Remove(dataDirPath)
 		err = os.Symlink(tsDirName, dataDirPath)
@@ -207,19 +227,19 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error {
 		return err
 	}
 
-	// (9)
+	// (10)
 	if err = w.createUserVisibleFiles(cleanPayload); err != nil {
 		klog.Errorf("%s: error creating visible symlinks in %s: %v", w.logContext, w.targetDir, err)
 		return err
 	}
 
-	// (10)
+	// (11)
 	if err = w.removeUserVisiblePaths(pathsToRemove); err != nil {
 		klog.Errorf("%s: error removing old visible symlinks: %v", w.logContext, err)
 		return err
 	}
 
-	// (11)
+	// (12)
 	if len(oldTsDir) > 0 {
 		if err = os.RemoveAll(oldTsPath); err != nil {
 			klog.Errorf("%s: error removing old data directory %s: %v", w.logContext, oldTsDir, err)

diff --git a/pkg/volume/util/atomic_writer_test.go b/pkg/volume/util/atomic_writer_test.go
@@ -21,6 +21,7 @@ package util
 
 import (
 	"encoding/base64"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -229,7 +230,7 @@ func TestPathsToRemove(t *testing.T) {
 		defer os.RemoveAll(targetDir)
 
 		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
-		err = writer.Write(tc.payload1)
+		err = writer.Write(tc.payload1, nil)
 		if err != nil {
 			t.Errorf("%v: unexpected error writing: %v", tc.name, err)
 			continue
@@ -397,7 +398,7 @@ IAAAAAAAsDyZDwU=`
 		defer os.RemoveAll(targetDir)
 
 		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
-		err = writer.Write(tc.payload)
+		err = writer.Write(tc.payload, nil)
 		if err != nil && tc.success {
 			t.Errorf("%v: unexpected error writing payload: %v", tc.name, err)
 			continue
@@ -574,7 +575,7 @@ func TestUpdate(t *testing.T) {
 
 		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
 
-		err = writer.Write(tc.first)
+		err = writer.Write(tc.first, nil)
 		if err != nil {
 			t.Errorf("%v: unexpected error writing: %v", tc.name, err)
 			continue
@@ -585,7 +586,7 @@ func TestUpdate(t *testing.T) {
 			continue
 		}
 
-		err = writer.Write(tc.next)
+		err = writer.Write(tc.next, nil)
 		if err != nil {
 			if tc.shouldWrite {
 				t.Errorf("%v: unexpected error writing: %v", tc.name, err)
@@ -743,7 +744,7 @@ func TestMultipleUpdates(t *testing.T) {
 		writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
 
 		for _, payload := range tc.payloads {
-			writer.Write(payload)
+			writer.Write(payload, nil)
 
 			checkVolumeContents(targetDir, tc.name, payload, t)
 		}
@@ -984,3 +985,54 @@ func TestCreateUserVisibleFiles(t *testing.T) {
 		}
 	}
 }
+
+func TestSetPerms(t *testing.T) {
+	targetDir, err := utiltesting.MkTmpdir("atomic-write")
+	if err != nil {
+		t.Fatalf("unexpected error creating tmp dir: %v", err)
+	}
+	defer os.RemoveAll(targetDir)
+
+	// Test that setPerms() is called once and with valid timestamp directory.
+	payload1 := map[string]FileProjection{
+		"foo/bar.txt": {Mode: 0644, Data: []byte("foo")},
+		"bar/zab.txt": {Mode: 0644, Data: []byte("bar")},
+	}
+
+	var setPermsCalled int
+	writer := &AtomicWriter{targetDir: targetDir, logContext: "-test-"}
+	err = writer.Write(payload1, func(subPath string) error {
+		fileInfo, err := os.Stat(filepath.Join(targetDir, subPath))
+		if err != nil {
+			t.Fatalf("unexpected error getting file info: %v", err)
+		}
+		// Ensure that given timestamp directory really exists.
+		if !fileInfo.IsDir() {
+			t.Fatalf("subPath is not a directory: %v", subPath)
+		}
+		setPermsCalled++
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("unexpected error writing: %v", err)
+	}
+	if setPermsCalled != 1 {
+		t.Fatalf("unexpected number of calls to setPerms: %v", setPermsCalled)
+	}
+
+	// Test that errors from setPerms() are propagated.
+	payload2 := map[string]FileProjection{
+		"foo/bar.txt": {Mode: 0644, Data: []byte("foo2")},
+		"bar/zab.txt": {Mode: 0644, Data: []byte("bar2")},
+	}
+
+	err = writer.Write(payload2, func(_ string) error {
+		return fmt.Errorf("error in setPerms")
+	})
+	if err == nil {
+		t.Fatalf("expected error while writing but got nil")
+	}
+	if !strings.Contains(err.Error(), "error in setPerms") {
+		t.Fatalf("unexpected error while writing: %v", err)
+	}
+}