From d4a3de3ace5f6bc3464aa8c53c04b7e6ac62f3cd Mon Sep 17 00:00:00 2001 From: Sascha Grunert Date: Mon, 17 Apr 2023 10:23:06 +0200 Subject: [PATCH] Don't call `umask` in subscriptions This call will be done in parallel which messes up the umask on CRI-O on container creation. We now call `chmod` after directory and file creation to enforce the right permissions. Signed-off-by: Sascha Grunert --- pkg/subscriptions/subscriptions.go | 23 +++--- pkg/umask/umask.go | 58 ++++++++++++++ pkg/umask/umask_test.go | 122 +++++++++++++++++++++++++++++ 3 files changed, 191 insertions(+), 12 deletions(-) create mode 100644 pkg/umask/umask.go create mode 100644 pkg/umask/umask_test.go diff --git a/pkg/subscriptions/subscriptions.go b/pkg/subscriptions/subscriptions.go index cdece0a1c..c21f5ecc1 100644 --- a/pkg/subscriptions/subscriptions.go +++ b/pkg/subscriptions/subscriptions.go @@ -37,11 +37,13 @@ type subscriptionData struct { // saveTo saves subscription data to given directory func (s subscriptionData) saveTo(dir string) error { - path := filepath.Join(dir, s.name) - if err := os.MkdirAll(filepath.Dir(path), s.dirMode); err != nil { - return err + if err := umask.MkdirAllIgnoreUmask(dir, s.dirMode); err != nil { + return fmt.Errorf("create subscription directory: %w", err) } - return os.WriteFile(path, s.data, s.mode) + if err := umask.WriteFileIgnoreUmask(filepath.Join(dir, s.name), s.data, s.mode); err != nil { + return fmt.Errorf("write subscription data: %w", err) + } + return nil } func readAll(root, prefix string, parentMode os.FileMode) ([]subscriptionData, error) { @@ -242,13 +244,9 @@ func addSubscriptionsFromMountsFile(filePath, mountLabel, containerRunDir string return nil, err } - // Don't let the umask have any influence on the file and directory creation - oldUmask := umask.Set(0) - defer umask.Set(oldUmask) - switch mode := fileInfo.Mode(); { case mode.IsDir(): - if err = os.MkdirAll(ctrDirOrFileOnHost, mode.Perm()); err != nil { + if err = umask.MkdirAllIgnoreUmask(ctrDirOrFileOnHost, mode.Perm()); err != nil { return nil, fmt.Errorf("making container directory: %w", err) } data, err := getHostSubscriptionData(hostDirOrFile, mode.Perm()) @@ -266,10 +264,11 @@ func addSubscriptionsFromMountsFile(filePath, mountLabel, containerRunDir string return nil, err } for _, s := range data { - if err := os.MkdirAll(filepath.Dir(ctrDirOrFileOnHost), s.dirMode); err != nil { - return nil, err + dir := filepath.Dir(ctrDirOrFileOnHost) + if err := umask.MkdirAllIgnoreUmask(dir, s.dirMode); err != nil { + return nil, fmt.Errorf("create container dir: %w", err) } - if err := os.WriteFile(ctrDirOrFileOnHost, s.data, s.mode); err != nil { + if err := umask.WriteFileIgnoreUmask(ctrDirOrFileOnHost, s.data, s.mode); err != nil { return nil, fmt.Errorf("saving data to container filesystem: %w", err) } } diff --git a/pkg/umask/umask.go b/pkg/umask/umask.go new file mode 100644 index 000000000..93f1d2b3c --- /dev/null +++ b/pkg/umask/umask.go @@ -0,0 +1,58 @@ +package umask + +import ( + "fmt" + "os" + "path/filepath" +) + +// MkdirAllIgnoreUmask creates a directory by ignoring the currently set umask. +func MkdirAllIgnoreUmask(dir string, mode os.FileMode) error { + parent := dir + dirs := []string{} + + // Find all parent directories which would have been created by MkdirAll + for { + if _, err := os.Stat(parent); err == nil { + break + } else if !os.IsNotExist(err) { + return fmt.Errorf("cannot stat %s: %w", dir, err) + } + + dirs = append(dirs, parent) + newParent := filepath.Dir(parent) + + // Only possible if the root paths are not existing, which would be odd + if parent == newParent { + break + } + + parent = newParent + } + + if err := os.MkdirAll(dir, mode); err != nil { + return fmt.Errorf("create directory %s: %w", dir, err) + } + + for _, d := range dirs { + if err := os.Chmod(d, mode); err != nil { + return fmt.Errorf("chmod directory %s: %w", d, err) + } + } + + return nil +} + +// WriteFileIgnoreUmask write the provided data to the path by ignoring the +// currently set umask. +func WriteFileIgnoreUmask(path string, data []byte, mode os.FileMode) error { + if err := os.WriteFile(path, data, mode); err != nil { + return fmt.Errorf("write file: %w", err) + } + + if err := os.Chmod(path, mode); err != nil { + return fmt.Errorf("chmod file: %w", err) + } + + return nil +} diff --git a/pkg/umask/umask_test.go b/pkg/umask/umask_test.go new file mode 100644 index 000000000..ebfa7c34e --- /dev/null +++ b/pkg/umask/umask_test.go @@ -0,0 +1,122 @@ +package umask_test + +import ( + "os" + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/containers/common/pkg/umask" +) + +func TestMkdirAllIgnoreUmask(t *testing.T) { + t.Parallel() + const testMode = os.FileMode(0o744) + + for _, tc := range []struct { + name string + prepare func() string + assert func(string, error) + }{ + { + name: "success", + prepare: func() string { + dir := t.TempDir() + return filepath.Join(dir, "foo", "bar") + }, + assert: func(dir string, err error) { + assert.NoError(t, err) + + // Assert $TMPDIR/foo/bar + assert.DirExists(t, dir) + info, err := os.Stat(dir) + assert.NoError(t, err) + assert.Equal(t, testMode, info.Mode().Perm()) + + // Assert $TMPDIR/foo + dir = filepath.Dir(dir) + assert.DirExists(t, dir) + info, err = os.Stat(dir) + assert.NoError(t, err) + assert.Equal(t, testMode, info.Mode().Perm()) + }, + }, + { + name: "success no dir to create", + prepare: os.TempDir, + assert: func(dir string, err error) { + assert.NoError(t, err) + }, + }, + } { + prepare := tc.prepare + assert := tc.assert + + t.Run(tc.name, func(t *testing.T) { + old := syscall.Umask(0o077) + defer syscall.Umask(old) + + t.Parallel() + dir := prepare() + + err := umask.MkdirAllIgnoreUmask(dir, testMode) + assert(dir, err) + }) + } +} + +func TestWriteFileIgnoreUmask(t *testing.T) { + t.Parallel() + const testMode = os.FileMode(0o744) + + for _, tc := range []struct { + name string + prepare func() string + assert func(string, error) + }{ + { + name: "success", + prepare: func() string { + dir := t.TempDir() + return filepath.Join(dir, "test") + }, + assert: func(path string, err error) { + assert.NoError(t, err) + + // Assert $TMPDIR/test + assert.FileExists(t, path) + info, err := os.Stat(path) + assert.NoError(t, err) + assert.Equal(t, testMode, info.Mode().Perm()) + }, + }, + { + name: "failure path does not exist", + prepare: func() string { + path := t.TempDir() + require.NoError(t, os.RemoveAll(path)) + return filepath.Join(path, "foo") + }, + assert: func(path string, err error) { + assert.Error(t, err) + }, + }, + } { + prepare := tc.prepare + assert := tc.assert + + t.Run(tc.name, func(t *testing.T) { + old := syscall.Umask(0o077) + defer syscall.Umask(old) + + t.Parallel() + path := prepare() + + err := umask.WriteFileIgnoreUmask(path, []byte("test"), testMode) + assert(path, err) + }) + } +}