Allow bind mounts of nodev,nosuid,noexec filesystems #3805

rpluem-vf · 2023-04-03T13:58:03Z

Currently bind mounts of filesystems with nodev, nosuid, noexec options set fail in rootless mode if the same options are not set for the bind mount. For ro filesystems this was resolved by #2570 by remounting again with roset. Follow the same approach for nodev, nosuid, noexec .

This is a replacement for the now closed #3710

kolyshkin · 2023-04-04T01:19:06Z

@rpluem-vf can you please

merge the last commit into the previous one
move the test case commit to after the fix (this way we won't break git-bisect)

rpluem-vf · 2023-04-04T06:33:52Z

@rpluem-vf can you please

merge the last commit into the previous one

move the test case commit to after the fix (this way we won't break git-bisect)

@kolyshkin: Like now?

kolyshkin · 2023-04-04T17:52:57Z

@kolyshkin: Like now?

Yes, thanks!

If you can rebase one more time, CI might even become green again :)

Also, a nit in the first commit message: you have "roset" without a space, and an extra space before period.

AkihiroSuda · 2023-04-04T18:28:05Z

tests/integration/mounts_sshfs.bats

+		update_config '.linux.namespaces += [{type: "user"}]
+				| .linux.uidMappings += [{"hostID": 100000, "containerID": 0, "size": 65534}]
+				| .linux.gidMappings += [{"hostID": 100000, "containerID": 0, "size": 65534}]'
+	fi



The original test has to be kept

I moved out my check to a new bats test file (b7bb49b). Furthermore I reviewed the test again and I could simplify it and make it more like the existing test.

@kolyshkin: Like now?

Yes, thanks!

If you can rebase one more time, CI might even become green again :)

One more rebase done. After the rebase is before the next one :-)

Also, a nit in the first commit message: you have "roset" without a space, and an extra space before period.

Fixed commit message.

AkihiroSuda · 2023-04-04T18:28:47Z

I think we discussed this before in somewhere, and we wanted high-level runtimes to generate the proper config.

Originally posted by @AkihiroSuda in #3801 (comment)

kolyshkin

LGTM

rpluem-vf · 2023-04-06T03:59:38Z

LGTM

Thanks. Let me know if another final rebase is required.

rata · 2023-04-06T10:26:10Z

tests/integration/mounts_nodev_nosuid_noexec.bats

+					type: "bind",
+					source: "'"$DIR"'",
+					destination: "/mnt",
+					options: ["dev", "suid", "exec", "rprivate", "rbind", "sync"]


Can we test just using "rbind", "rprivate" too? We hit that in #3770 with COS

cc @vinayakankugoyal

Unfortunately this is not sufficient. It will not trigger a remount.

Oh, because of this:

runc/libcontainer/rootfs_linux.go

Lines 471 to 473 in 5726682

if m.Flags&^(unix.MS_REC|unix.MS_REMOUNT|unix.MS_BIND) != 0 {

// only remount if unique mount options are set

if err := remount(m, rootfs, mountFd); err != nil {

?

Then this is solving a real problem for you? You happen to hit it and the flags that were used included some that trigger a remount?

You could try "rbind", "rprivate", "sync". In order to trigger a remount an additional flag different to unix.MS_REC|unix.MS_REMOUNT|unix.MS_BIND must be present. "dev", "suid", "exec" do not meet this as they are parsed to set no flag (their value is true):

runc/libcontainer/specconv/spec_linux.go

Lines 74 to 77 in 5726682

"dev": {true, unix.MS_NODEV},

"diratime": {true, unix.MS_NODIRATIME},

"dirsync": {false, unix.MS_DIRSYNC},

"exec": {true, unix.MS_NOEXEC},

Yes, I understand that. But the only time I hit this in the wild was without such an option being set. It doesn't seem like crun needs that either.

With this patch, are you fixing an issue you encounter in a real world scenario?

@rpluem-vf probably nodev,nosuid,noexec are enough. But it would be great if @vinayakankugoyal shares what options the COS mount of /var has.

So nodev, nosuid, noexec on the source and rbind, ro on the destination (pending @vinayakankugoyal confirmation)?

How about 06f65bf that is now added?

rpluem-vf · 2023-04-13T16:48:59Z

Is there anything I can do to move this forward?

rata · 2023-04-13T18:10:55Z

@rpluem-vf maintainers review time is scarce, sadly I'm no maintainer here. I think you should ping them from time to time.

rata · 2023-04-14T10:37:15Z

@vinayakankugoyal can you test this PR to see if, without the containerd patch and this PR, it all works as expected for your use case too? It should, but just a sanity check won't hurt

rata · 2023-04-14T10:38:56Z

I had a typo when posting the last comment, cc @vinayakankugoyal again just in case it doesn't send notifications when tagging on edits.

rpluem-vf · 2023-04-18T09:13:29Z

@vinayakankugoyal can you test this PR to see if, without the containerd patch and this PR, it all works as expected for your use case too? It should, but just a sanity check won't hurt

@vinayakankugoyal : Any update?

rpluem-vf · 2023-04-18T09:14:46Z

@AkihiroSuda : Would you mind having another look? Freshly rebased and I think all things are addressed.

rata

LGTM

rata · 2023-04-18T12:37:56Z

@rpluem-vf I received an email notification that @vinayakankugoyal tested it and works fine, but github had some issues today and it seems it is not showing it here.

rpluem-vf · 2023-04-18T12:43:22Z

@rpluem-vf I received an email notification that @vinayakankugoyal tested it and works fine, but github had some issues today and it seems it is not showing it here.

Got that as well, but I thought that he probably deleted it for whatever reason.

libcontainer/rootfs_linux.go

rpluem-vf · 2023-06-19T07:11:10Z

@kolyshkin : Can you please have a look if the current state matches your expectations?

rpluem-vf · 2023-06-29T05:30:27Z

@kolyshkin / @cyphar : Can one of you please do a review?

rpluem-vf · 2023-07-10T13:02:05Z

@kolyshkin / @cyphar : Can one of you please do a review?

AkihiroSuda · 2023-07-12T07:38:09Z

Merging is blocked
Merging can be performed automatically once the requested changes are addressed.

Ping @kolyshkin

lifubang · 2023-07-15T10:20:59Z

See above.

ping @kolyshkin
#3805 (review)

kolyshkin

LGTM, thanks!

Currently bind mounts of filesystems with nodev, nosuid, noexec, noatime, relatime, strictatime, nodiratime options set fail in rootless mode if the same options are not set for the bind mount. For ro filesystems this was resolved by opencontainers#2570 by remounting again with ro set. Follow the same approach for nodev, nosuid, noexec, noatime, relatime, strictatime, nodiratime but allow to revert back to the old behaviour via the new `--no-mount-fallback` command line option. Add a testcase to verify that bind mounts of filesystems with nodev, nosuid, noexec, noatime options set work in rootless mode. Add a testcase that mounts a nodev, nosuid, noexec, noatime filesystem with a ro flag. Add two further testcases that ensure that the above testcases would fail if the `--no-mount-fallback` command line option is set. * contrib/completions/bash/runc: Add `--no-mount-fallback` command line option for bash completion. * create.go: Add `--no-mount-fallback` command line option. * restore.go: Add `--no-mount-fallback` command line option. * run.go: Add `--no-mount-fallback` command line option. * libcontainer/configs/config.go: Add `NoMountFallback` field to the `Config` struct to store the command line option value. * libcontainer/specconv/spec_linux.go: Add `NoMountFallback` field to the `CreateOpts` struct to store the command line option value and store it in the libcontainer config. * utils_linux.go: Store the command line option value in the `CreateOpts` struct. * libcontainer/rootfs_linux.go: In case that `--no-mount-fallback` is not set try to remount the bind filesystem again with the options nodev, nosuid, noexec, noatime, relatime, strictatime or nodiratime if they are set on the source filesystem. * tests/integration/mounts_sshfs.bats: Add testcases and rework sshfs setup to allow specifying different mount options depending on the test case. Signed-off-by: Ruediger Pluem <ruediger.pluem@vodafone.com>

cyphar · 2023-08-03T06:29:49Z

Sorry for not reviewing this earlier -- what was the reason for adding --no-mount-fallback? Is it only there for tests? In general we want to avoid adding arguments to runc because runc-compatible runtimes have to copy all of our cli arguments.

AkihiroSuda · 2023-08-03T06:40:16Z

Sorry for not reviewing this earlier -- what was the reason for adding --no-mount-fallback? Is it only there for tests? In general we want to avoid adding arguments to runc because runc-compatible runtimes have to copy all of our cli arguments.

Because it seemed controversial to modify the config on runc(-compatible) side.

cyphar · 2023-08-03T06:51:01Z

Hmm. Given we do this for ro, this is for rootless containers, and all of the flags are mnt_locked flags, I think it's safe to do the rewriting indiscriminately. We can discuss this in a separate PR though.

cyphar · 2023-08-06T03:25:16Z

I think more preferable behaviour would be to error out if the user explicitly requested a mount option we couldn't provide. I'll write a patch.

EDIT: #3967 is my proposed solution

testinfected · 2023-08-22T14:28:29Z

Hi,

I'm glad this has been merged to master yet I don't see any milestone set. Do you know when we can expect this to be released (1.2.0, other?).

Thanks in advance

cyphar · 2023-08-22T14:29:52Z

This is being reworked in #3967 and there are a few other PRs we need to merge before we release a 1.2.0-rc.1.

testinfected · 2023-08-22T14:32:22Z

Thanks @cyphar

rpluem-vf force-pushed the nodev_noexec_nosuid branch 5 times, most recently from 22cb767 to 2fcc2bf Compare April 3, 2023 18:49

rpluem-vf marked this pull request as ready for review April 3, 2023 19:43

rpluem-vf force-pushed the nodev_noexec_nosuid branch from 2fcc2bf to 5db5636 Compare April 4, 2023 06:16

AkihiroSuda reviewed Apr 4, 2023

View reviewed changes

rpluem-vf force-pushed the nodev_noexec_nosuid branch 2 times, most recently from 493e752 to b7bb49b Compare April 5, 2023 07:14

rpluem-vf requested a review from AkihiroSuda April 5, 2023 10:50

kolyshkin approved these changes Apr 5, 2023

View reviewed changes

rata reviewed Apr 6, 2023

View reviewed changes

rata mentioned this pull request Apr 6, 2023

evaluate noexec, nodev, and nosuid options for /etc/resolv.conf mount moby/moby#45248

Open

rpluem-vf force-pushed the nodev_noexec_nosuid branch from b7bb49b to 06f65bf Compare April 18, 2023 08:54

rata approved these changes Apr 18, 2023

View reviewed changes

kolyshkin reviewed Apr 18, 2023

View reviewed changes

libcontainer/rootfs_linux.go Outdated Show resolved Hide resolved

rpluem-vf force-pushed the nodev_noexec_nosuid branch from 66c149e to 8cb4ca7 Compare June 12, 2023 10:22

AkihiroSuda approved these changes Jun 12, 2023

View reviewed changes

AkihiroSuda requested a review from kolyshkin June 12, 2023 10:23

AkihiroSuda requested a review from cyphar June 23, 2023 12:47

AkihiroSuda requested a review from lifubang July 11, 2023 01:37

lifubang added the status/2-code-review label Jul 11, 2023

lifubang approved these changes Jul 12, 2023

View reviewed changes

lifubang added status/4-merge and removed status/2-code-review labels Jul 12, 2023

AkihiroSuda approved these changes Jul 12, 2023

View reviewed changes

rpluem-vf force-pushed the nodev_noexec_nosuid branch from 8cb4ca7 to a131dc9 Compare July 12, 2023 06:22

kolyshkin approved these changes Jul 28, 2023

View reviewed changes

kolyshkin force-pushed the nodev_noexec_nosuid branch from a131dc9 to da780e4 Compare July 28, 2023 23:32

kolyshkin enabled auto-merge July 28, 2023 23:32

kolyshkin merged commit b15a6a3 into opencontainers:main Jul 29, 2023

rpluem-vf mentioned this pull request Aug 7, 2023

rootfs: remove --no-mount-fallback and finally fix MS_REMOUNT #3967

Merged

cyphar added this to the 1.2.0 milestone Aug 22, 2023

	if m.Flags&^(unix.MS_REC\|unix.MS_REMOUNT\|unix.MS_BIND) != 0 {
	// only remount if unique mount options are set
	if err := remount(m, rootfs, mountFd); err != nil {

	"dev": {true, unix.MS_NODEV},
	"diratime": {true, unix.MS_NODIRATIME},
	"dirsync": {false, unix.MS_DIRSYNC},
	"exec": {true, unix.MS_NOEXEC},

Allow bind mounts of nodev,nosuid,noexec filesystems #3805

Allow bind mounts of nodev,nosuid,noexec filesystems #3805

Conversation

rpluem-vf commented Apr 3, 2023 • edited Loading

kolyshkin commented Apr 4, 2023

rpluem-vf commented Apr 4, 2023

kolyshkin commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Apr 4, 2023

kolyshkin left a comment

Choose a reason for hiding this comment

rpluem-vf commented Apr 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpluem-vf commented Apr 13, 2023

rata commented Apr 13, 2023

rata commented Apr 14, 2023 • edited Loading

rata commented Apr 14, 2023

rpluem-vf commented Apr 18, 2023

rpluem-vf commented Apr 18, 2023

rata left a comment

Choose a reason for hiding this comment

rata commented Apr 18, 2023

rpluem-vf commented Apr 18, 2023

rpluem-vf commented Jun 19, 2023

rpluem-vf commented Jun 29, 2023

rpluem-vf commented Jul 10, 2023

AkihiroSuda commented Jul 12, 2023

lifubang commented Jul 15, 2023

kolyshkin left a comment

Choose a reason for hiding this comment

cyphar commented Aug 3, 2023

AkihiroSuda commented Aug 3, 2023

cyphar commented Aug 3, 2023

cyphar commented Aug 6, 2023 • edited Loading

testinfected commented Aug 22, 2023 • edited Loading

cyphar commented Aug 22, 2023

testinfected commented Aug 22, 2023

rpluem-vf commented Apr 3, 2023 •

edited

Loading

rata commented Apr 14, 2023 •

edited

Loading

cyphar commented Aug 6, 2023 •

edited

Loading

testinfected commented Aug 22, 2023 •

edited

Loading