From e2f6232258694b7c8f25681ea5eae1adf0f62108 Mon Sep 17 00:00:00 2001 From: yaroslavborbat Date: Mon, 7 Oct 2024 22:46:41 +0300 Subject: [PATCH 1/3] preserve options after remount for bind mounting Signed-off-by: yaroslavborbat --- staging/src/k8s.io/mount-utils/mount_linux.go | 36 +++++++------------ .../k8s.io/mount-utils/mount_linux_test.go | 8 ++--- test/e2e/storage/drivers/in_tree.go | 9 +++++ test/e2e/storage/utils/local.go | 7 ++-- 4 files changed, 30 insertions(+), 30 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/mount_linux.go b/staging/src/k8s.io/mount-utils/mount_linux.go index 37912c1f58a..5901114f049 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux.go +++ b/staging/src/k8s.io/mount-utils/mount_linux.go @@ -35,7 +35,6 @@ import ( "github.com/moby/sys/mountinfo" "golang.org/x/sys/unix" - inuserns "github.com/moby/sys/userns" "k8s.io/klog/v2" utilexec "k8s.io/utils/exec" ) @@ -114,7 +113,7 @@ func (mounter *Mounter) hasSystemd() bool { // Map unix.Statfs mount flags ro, nodev, noexec, nosuid, noatime, relatime, // nodiratime to mount option flag strings. -func getUserNSBindMountOptions(path string, statfs func(path string, buf *unix.Statfs_t) (err error)) ([]string, error) { +func getBindMountOptions(path string, statfs func(path string, buf *unix.Statfs_t) (err error)) ([]string, error) { var s unix.Statfs_t var mountOpts []string if err := statfs(path, &s); err != nil { @@ -137,32 +136,23 @@ func getUserNSBindMountOptions(path string, statfs func(path string, buf *unix.S return mountOpts, nil } -// Do a bind mount including the needed remount for applying the bind opts. -// If the remount fails and we are running in a user namespace -// figure out if the source filesystem has the ro, nodev, noexec, nosuid, -// noatime, relatime or nodiratime flag set and try another remount with the found flags. +// Performs a bind mount with the specified options, and then remounts +// the mount point with the same `nodev`, `nosuid`, `noexec`, `nosuid`, `noatime`, +// `relatime`, `nodiratime` options as the original mount point. func (mounter *Mounter) bindMountSensitive(mounterPath string, mountCmd string, source string, target string, fstype string, bindOpts []string, bindRemountOpts []string, bindRemountOptsSensitive []string, mountFlags []string, systemdMountRequired bool) error { - err := mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindOpts, bindRemountOptsSensitive, mountFlags, systemdMountRequired) + err := mounter.doMount(mounterPath, mountCmd, source, target, fstype, bindOpts, bindRemountOptsSensitive, mountFlags, systemdMountRequired) if err != nil { return err } - err = mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive, mountFlags, systemdMountRequired) - if inuserns.RunningInUserNS() { - if err == nil { - return nil - } - // Check if the source has ro, nodev, noexec, nosuid, noatime, relatime, - // nodiratime flag... - fixMountOpts, err := getUserNSBindMountOptions(source, unix.Statfs) - if err != nil { - return &os.PathError{Op: "statfs", Path: source, Err: err} - } - // ... and retry the mount with flags found above. - bindRemountOpts = append(bindRemountOpts, fixMountOpts...) - return mounter.doMount(mounterPath, defaultMountCommand, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive, mountFlags, systemdMountRequired) - } else { - return err + // Check if the source has ro, nodev, noexec, nosuid, noatime, relatime, + // nodiratime flag... + fixMountOpts, err := getBindMountOptions(source, unix.Statfs) + if err != nil { + return &os.PathError{Op: "statfs", Path: source, Err: err} } + // ... and retry the mount with flags found above. + bindRemountOpts = append(bindRemountOpts, fixMountOpts...) + return mounter.doMount(mounterPath, mountCmd, source, target, fstype, bindRemountOpts, bindRemountOptsSensitive, mountFlags, systemdMountRequired) } // Mount mounts source to target as fstype with given options. 'source' and 'fstype' must diff --git a/staging/src/k8s.io/mount-utils/mount_linux_test.go b/staging/src/k8s.io/mount-utils/mount_linux_test.go index 3c0842b20b3..e8c765b5c52 100644 --- a/staging/src/k8s.io/mount-utils/mount_linux_test.go +++ b/staging/src/k8s.io/mount-utils/mount_linux_test.go @@ -821,7 +821,7 @@ func mkStatfsFlags[T1 constraints.Integer, T2 constraints.Integer](orig T1, add return orig | T1(add) } -func TestGetUserNSBindMountOptions(t *testing.T) { +func TestGetBindMountOptions(t *testing.T) { var testCases = map[string]struct { flags int32 // smallest size used by any platform we care about mountoptions string @@ -843,9 +843,9 @@ func TestGetUserNSBindMountOptions(t *testing.T) { return nil } - testGetUserNSBindMountOptionsSingleCase := func(t *testing.T) { + testGetBindMountOptionsSingleCase := func(t *testing.T) { path := strings.Split(t.Name(), "/")[1] - options, _ := getUserNSBindMountOptions(path, statfsMock) + options, _ := getBindMountOptions(path, statfsMock) sort.Strings(options) optionString := strings.Join(options, ",") mountOptions := testCases[path].mountoptions @@ -855,7 +855,7 @@ func TestGetUserNSBindMountOptions(t *testing.T) { } for k := range testCases { - t.Run(k, testGetUserNSBindMountOptionsSingleCase) + t.Run(k, testGetBindMountOptionsSingleCase) } } diff --git a/test/e2e/storage/drivers/in_tree.go b/test/e2e/storage/drivers/in_tree.go index 58ec455c67b..203a69c5989 100644 --- a/test/e2e/storage/drivers/in_tree.go +++ b/test/e2e/storage/drivers/in_tree.go @@ -43,6 +43,7 @@ import ( "time" "github.com/onsi/ginkgo/v2" + v1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" storagev1 "k8s.io/api/storage/v1" @@ -1223,6 +1224,14 @@ func (l *localDriver) PrepareTest(ctx context.Context, f *framework.Framework) * framework.ExpectNoError(err) l.hostExec = utils.NewHostExec(f) + // It is recommended to mount /tmp with options noexec, nodev, nosuid. + // tmpfs on /tmp type tmpfs (rw,nosuid,nodev,noexec,relatime,seclabel,inode64) + // This prevents scripts and binaries from being executed from the /tmp directory. + // This can cause errors like "Permission denied" when executing files from `/tmp`. + // To pass the test that verifies the execution of files on a volume, we remount `/tmp` with the exec option. + remountCmd := `mount | awk '{if ($3 == "/tmp") print $0}' | grep -q "noexec" && mount -o remount,exec /tmp || true` + err = l.hostExec.IssueCommand(ctx, remountCmd, l.node) + framework.ExpectNoError(err) l.ltrMgr = utils.NewLocalResourceManager("local-driver", l.hostExec, "/tmp") // This can't be done in SkipUnsupportedTest because the test framework is not initialized yet diff --git a/test/e2e/storage/utils/local.go b/test/e2e/storage/utils/local.go index df96eac124e..1dccee34625 100644 --- a/test/e2e/storage/utils/local.go +++ b/test/e2e/storage/utils/local.go @@ -27,6 +27,7 @@ import ( "strings" "github.com/onsi/ginkgo/v2" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/kubernetes/test/e2e/framework" @@ -215,7 +216,7 @@ func (l *ltrMgr) cleanupLocalVolumeDirectory(ctx context.Context, ltr *LocalTest func (l *ltrMgr) setupLocalVolumeDirectoryLink(ctx context.Context, node *v1.Node, parameters map[string]string) *LocalTestResource { hostDir := l.getTestDir() hostDirBackend := hostDir + "-backend" - cmd := fmt.Sprintf("mkdir %s && ln -s %s %s", hostDirBackend, hostDirBackend, hostDir) + cmd := fmt.Sprintf("mkdir -p %s && ln -s %s %s", hostDirBackend, hostDirBackend, hostDir) err := l.hostExec.IssueCommand(ctx, cmd, node) framework.ExpectNoError(err) return &LocalTestResource{ @@ -235,7 +236,7 @@ func (l *ltrMgr) cleanupLocalVolumeDirectoryLink(ctx context.Context, ltr *Local func (l *ltrMgr) setupLocalVolumeDirectoryBindMounted(ctx context.Context, node *v1.Node, parameters map[string]string) *LocalTestResource { hostDir := l.getTestDir() - cmd := fmt.Sprintf("mkdir %s && mount --bind %s %s", hostDir, hostDir, hostDir) + cmd := fmt.Sprintf("mkdir -p %s && mount --bind %s %s", hostDir, hostDir, hostDir) err := l.hostExec.IssueCommand(ctx, cmd, node) framework.ExpectNoError(err) return &LocalTestResource{ @@ -255,7 +256,7 @@ func (l *ltrMgr) cleanupLocalVolumeDirectoryBindMounted(ctx context.Context, ltr func (l *ltrMgr) setupLocalVolumeDirectoryLinkBindMounted(ctx context.Context, node *v1.Node, parameters map[string]string) *LocalTestResource { hostDir := l.getTestDir() hostDirBackend := hostDir + "-backend" - cmd := fmt.Sprintf("mkdir %s && mount --bind %s %s && ln -s %s %s", hostDirBackend, hostDirBackend, hostDirBackend, hostDirBackend, hostDir) + cmd := fmt.Sprintf("mkdir -p %s && mount --bind %s %s && ln -s %s %s", hostDirBackend, hostDirBackend, hostDirBackend, hostDirBackend, hostDir) err := l.hostExec.IssueCommand(ctx, cmd, node) framework.ExpectNoError(err) return &LocalTestResource{ From 4dcddabb7e154f466c64852f658f70c09b94ff48 Mon Sep 17 00:00:00 2001 From: yaroslavborbat Date: Fri, 18 Oct 2024 18:03:45 +0300 Subject: [PATCH 2/3] update vendor Signed-off-by: yaroslavborbat --- staging/src/k8s.io/mount-utils/go.mod | 1 - staging/src/k8s.io/mount-utils/go.sum | 2 -- 2 files changed, 3 deletions(-) diff --git a/staging/src/k8s.io/mount-utils/go.mod b/staging/src/k8s.io/mount-utils/go.mod index c8796fee1b6..fae337c5ca0 100644 --- a/staging/src/k8s.io/mount-utils/go.mod +++ b/staging/src/k8s.io/mount-utils/go.mod @@ -8,7 +8,6 @@ godebug default=go1.23 require ( github.com/moby/sys/mountinfo v0.7.2 - github.com/moby/sys/userns v0.1.0 github.com/stretchr/testify v1.9.0 golang.org/x/exp v0.0.0-20240719175910-8a7402abbf56 golang.org/x/sys v0.26.0 diff --git a/staging/src/k8s.io/mount-utils/go.sum b/staging/src/k8s.io/mount-utils/go.sum index 9e77478c242..466650c5853 100644 --- a/staging/src/k8s.io/mount-utils/go.sum +++ b/staging/src/k8s.io/mount-utils/go.sum @@ -13,8 +13,6 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= github.com/moby/sys/mountinfo v0.7.2 h1:1shs6aH5s4o5H2zQLn796ADW1wMrIwHsyJ2v9KouLrg= github.com/moby/sys/mountinfo v0.7.2/go.mod h1:1YOa8w8Ih7uW0wALDUgT1dTTSBrZ+HiBLGws92L2RU4= -github.com/moby/sys/userns v0.1.0 h1:tVLXkFOxVu9A64/yh59slHVv9ahO9UIev4JZusOLG/g= -github.com/moby/sys/userns v0.1.0/go.mod h1:IHUYgu/kao6N8YZlp9Cf444ySSvCmDlmzUcYfDHOl28= github.com/pkg/diff v0.0.0-20210226163009-20ebb0f2a09e/go.mod h1:pJLUxLENpZxwdsKMEsNbx1VGcRFpLqf3715MtcvvzbA= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 h1:Jamvg5psRIccs7FGNTlIRMkT8wgtp5eCXdBlqhYGL6U= github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= From 5db88777c4f2febdaba8b964c7b3ec61e516d731 Mon Sep 17 00:00:00 2001 From: yaroslavborbat Date: Mon, 11 Nov 2024 12:22:34 +0300 Subject: [PATCH 3/3] change the CapExec value to false by default Signed-off-by: yaroslavborbat --- test/e2e/storage/drivers/in_tree.go | 51 ++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 12 deletions(-) diff --git a/test/e2e/storage/drivers/in_tree.go b/test/e2e/storage/drivers/in_tree.go index 203a69c5989..7d34574e1ea 100644 --- a/test/e2e/storage/drivers/in_tree.go +++ b/test/e2e/storage/drivers/in_tree.go @@ -1139,19 +1139,54 @@ type localVolume struct { var ( // capabilities defaultLocalVolumeCapabilities = map[storageframework.Capability]bool{ - storageframework.CapPersistence: true, - storageframework.CapFsGroup: true, - storageframework.CapBlock: false, - storageframework.CapExec: true, + storageframework.CapPersistence: true, + storageframework.CapFsGroup: true, + storageframework.CapBlock: false, + // To test CapExec, we need a volume with a filesystem. + // During end-to-end (e2e) testing, we utilize the `/tmp` directory for volume creation. + // However, best practices recommend mounting `/tmp` with the `noexec`, `nodev`, and `nosuid` parameters. + // This security measure prevents the execution of scripts and binaries within the `/tmp` directory. + // This practice, while promoting security, creates a dependency on the infrastructure configuration during e2e tests. + // This can result in "Permission Denied" errors when attempting to execute files from `/tmp`. + // To address this, we intentionally skip exec tests for certain types of LocalVolumes, such as `dir` or `dir-link`. + // This allows us to conduct comprehensive testing without relying on potentially restrictive security configurations. + storageframework.CapExec: false, storageframework.CapMultiPODs: true, storageframework.CapSingleNodeVolume: true, storageframework.CapMultiplePVsSameID: true, } localVolumeCapabitilies = map[utils.LocalVolumeType]map[storageframework.Capability]bool{ + utils.LocalVolumeTmpfs: { + storageframework.CapPersistence: true, + storageframework.CapFsGroup: true, + storageframework.CapBlock: false, + storageframework.CapExec: true, + storageframework.CapMultiPODs: true, + storageframework.CapSingleNodeVolume: true, + storageframework.CapMultiplePVsSameID: true, + }, utils.LocalVolumeBlock: { storageframework.CapPersistence: true, storageframework.CapFsGroup: true, storageframework.CapBlock: true, + storageframework.CapExec: false, + storageframework.CapMultiPODs: true, + storageframework.CapSingleNodeVolume: true, + storageframework.CapMultiplePVsSameID: true, + }, + utils.LocalVolumeBlockFS: { + storageframework.CapPersistence: true, + storageframework.CapFsGroup: true, + storageframework.CapBlock: false, + storageframework.CapExec: true, + storageframework.CapMultiPODs: true, + storageframework.CapSingleNodeVolume: true, + storageframework.CapMultiplePVsSameID: true, + }, + utils.LocalVolumeGCELocalSSD: { + storageframework.CapPersistence: true, + storageframework.CapFsGroup: true, + storageframework.CapBlock: false, storageframework.CapExec: true, storageframework.CapMultiPODs: true, storageframework.CapSingleNodeVolume: true, @@ -1224,14 +1259,6 @@ func (l *localDriver) PrepareTest(ctx context.Context, f *framework.Framework) * framework.ExpectNoError(err) l.hostExec = utils.NewHostExec(f) - // It is recommended to mount /tmp with options noexec, nodev, nosuid. - // tmpfs on /tmp type tmpfs (rw,nosuid,nodev,noexec,relatime,seclabel,inode64) - // This prevents scripts and binaries from being executed from the /tmp directory. - // This can cause errors like "Permission denied" when executing files from `/tmp`. - // To pass the test that verifies the execution of files on a volume, we remount `/tmp` with the exec option. - remountCmd := `mount | awk '{if ($3 == "/tmp") print $0}' | grep -q "noexec" && mount -o remount,exec /tmp || true` - err = l.hostExec.IssueCommand(ctx, remountCmd, l.node) - framework.ExpectNoError(err) l.ltrMgr = utils.NewLocalResourceManager("local-driver", l.hostExec, "/tmp") // This can't be done in SkipUnsupportedTest because the test framework is not initialized yet