From 96306f144a3c917a97fe27c9ad5dd89e4213f741 Mon Sep 17 00:00:00 2001 From: Tero Saarni Date: Fri, 23 Dec 2022 08:48:06 +0200 Subject: [PATCH] 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. --- pkg/volume/cephfs/cephfs.go | 2 +- pkg/volume/configmap/configmap.go | 12 +++--- pkg/volume/downwardapi/downwardapi.go | 13 +++--- pkg/volume/projected/projected.go | 12 +++--- pkg/volume/secret/secret.go | 12 +++--- pkg/volume/util/atomic_writer.go | 48 +++++++++++++++------ pkg/volume/util/atomic_writer_test.go | 62 ++++++++++++++++++++++++--- 7 files changed, 116 insertions(+), 45 deletions(-) diff --git a/pkg/volume/cephfs/cephfs.go b/pkg/volume/cephfs/cephfs.go index c4480e8447a..e6243dba120 100644 --- 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 index cdfba9480cd..7a1e5e58178 100644 --- 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 index 8338850ac42..b13e6ea6015 100644 --- a/pkg/volume/downwardapi/downwardapi.go +++ b/pkg/volume/downwardapi/downwardapi.go @@ -220,18 +220,17 @@ func (b *downwardAPIVolumeMounter) SetUpAt(dir string, mounterArgs volume.Mounte 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(b, mounterArgs.FsGroup, nil /*fsGroupChangePolicy*/, volumeutil.FSGroupCompleteHook(b.plugin, nil)) + } + err = writer.Write(data, 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/projected/projected.go b/pkg/volume/projected/projected.go index a48567b2a6a..c82b38653e3 100644 --- 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 index fa06d879303..f43f1bffa3b 100644 --- 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 index 94428f6ffc8..91ee77a9f69 100644 --- a/pkg/volume/util/atomic_writer.go +++ b/pkg/volume/util/atomic_writer.go @@ -86,11 +86,16 @@ 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 @@ -98,13 +103,19 @@ const ( // // 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: // /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,6 +197,14 @@ 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) @@ -192,7 +212,7 @@ func (w *AtomicWriter) Write(payload map[string]FileProjection) error { 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 index 653e8d0a043..d463d0ba023 100644 --- 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) + } +}