--read-write-tmpfs=false should set /dev/* tmpfs to readonly #12954

rhatdan · 2022-01-20T22:08:00Z

Change the --read-only-tmpfs option to --read-write-tmpfs since
this makes sense and the old name was doing the exact opposite.

Fixes: #12937

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

rhatdan · 2022-01-20T22:09:52Z

@giuseppe When I do this I attempt to make /dev, /dev/tty, /dev/shm, /dev/mqueue as read-only but crun blows up with

./bin/podman run -ti --read-only --read-only-tmpfs=false alpine sh
Error: OCI runtime error: crun: opening file /dev/console for writing: Read-only file system

giuseppe · 2022-01-21T08:32:30Z

/dev is still used by the OCI runtime after setting the mounts.

I've fixed an issue in crun so it works when --tty is not used, but for a read-only /dev also in the tty case, we'd need to set the ro flag later. I'll take a look at it.

I've tried with runc and it fails as well, so I think it should be optional to set /dev read-only. Could we extend --read-only-tmpfs to accept a string and have something like --read-only-tmpfs=all?

giuseppe · 2022-01-21T09:42:20Z

this seems to help with your PR: containers/crun#859

rhatdan · 2022-01-21T11:51:35Z

@kolyshkin WDYT?

rhatdan · 2022-01-21T11:52:01Z

I think if we leave any tmpfs writable, we don't solve the problem.

rhatdan · 2022-01-21T15:18:08Z

Fixes: #12937

kolyshkin · 2022-01-22T02:54:44Z

As @giuseppe mentioned earlier, runc can't work with ro /dev, but this fix is easy opencontainers/runc#3345 (and very similar to containers/crun#857).

One other thing, @rhatdan, I also find it very confusing how --read-only-tmpfs=false results in read-only /dev mounts. Maybe a separate option (--read-only-dev) or something else entirely?

rhatdan · 2022-01-22T12:07:02Z

@kolyshkin I am not crazy about adding another function, Since I believe the original goal of --read-only-tmpfs (A badly named option, which we should probably change) was to make the entire container read-only. (At least the way I think about it).

rhatdan · 2022-01-22T12:07:27Z

Changed the option to read-write-tmpfs.

rhatdan · 2022-01-27T12:22:29Z

This PR is waiting for an updated released version of runc, which will allow it to work with /dev set to read/only mode.
@kolyshkin @AkihiroSuda Any idea when that will be?

kolyshkin · 2022-01-27T21:42:41Z

This PR is waiting for an updated released version of runc, which will allow it to work with /dev set to read/only mode.
@kolyshkin @AkihiroSuda Any idea when that will be?

Depending on how soon you need it.

We can certainly release runc 1.1.1, either now (= soon) with this change alone, or a bit later, waiting for a few more fixes to pile up.

kolyshkin · 2022-01-27T21:46:57Z

This PR is waiting for an updated released version of runc,

Backport PR: opencontainers/runc#3355

rhatdan · 2022-01-28T11:46:48Z

Well since this is a post 4.0 feature, it is not needed immediately, but soon.

openshift-ci · 2022-02-28T21:20:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago · 2022-10-31T14:18:41Z

This is my fault, #16240. Can you run make docs, then git add the changed file and git commit --amend?

edsantiago

Lots of little nits, sorry for not catching them earlier.

edsantiago · 2022-11-01T11:15:45Z

docs/source/markdown/options/read-write-tmpfs.md

+####> are applicable to all of those.
+#### **--read-write-tmpfs**
+
+If container is running in --read-only mode, then mount a read-write tmpfs on /run, /tmp, and /var/tmp. When false, Podman the entire container has no tmpfs that are read/write unless specified on the command line.  Podman mounts /dev, /dev/mqueue, /dev/pts, /dev/shm as read only. The default is *true*


When false, Podman the entire container

Does not parse. I think Podman is the part that needs to go, but I'm not sure what the intention was.

Did you deliberately un-underscore the directory names? Our guidelines say:

Strings of characters or numbers can be highlighted with backticks. Paths of any kind must be highlighted.

(which suggests that backticks are preferred, underscores are ok, but leaving them unadorned is not).

edsantiago · 2022-11-01T11:16:59Z

test/system/030-run.bats

+        run_podman run --rm --read-only $IMAGE touch ${rwdir}/foo
+        run_podman run --rm --read-only --read-write-tmpfs=true $IMAGE touch ${rwdir}/foo
+	run_podman 1 run --rm --read-only --read-write-tmpfs=false $IMAGE touch ${rwdir}/foo
+	is "$output" "touch: ${rwdir}/foo: Read-only file system" "Should fail with ${rwdir}/foo read only file system"


Inconsistent indentation: some of these are tabs, some are eight spaces.

edsantiago · 2022-11-01T11:19:32Z

test/system/030-run.bats

+    # FIXME once ubuntu gets a newer crun
+    if is_ubuntu; then
+        if [ $(podman_runtime) == "crun" ]; then
+	   skip "The version of crun shipped by ubuntu is too old. Needs crun-1.4.2 or greater"


I have a find-obsolete-skips script that I run periodically. Could you add "FIXME:" to this and the above skip/Skips ? That will bring these to my attention when I run the script next time.

edsantiago · 2022-11-01T11:20:27Z

test/system/030-run.bats

+	   skip "The version of crun shipped by ubuntu is too old. Needs crun-1.4.2 or greater"
+	fi
+    fi
+    HOST=$(random_string 25)


leftover from copy/paste (hence misleading, because it's never used). Please remove.

edsantiago · 2022-11-01T11:26:21Z

cmd/podman/containers/create.go

@@ -114,6 +114,11 @@ func commonFlags(cmd *cobra.Command) error {
 	if cmd.Flags().Changed("image-volume") {
 		cliVals.ImageVolume = cmd.Flag("image-volume").Value.String()
 	}
+
+	if cmd.Flags().Changed("read-write-tmpfs") && !cliVals.ReadOnly {
+		return errors.New("--read-write-tmpfs is only used if --read-only=true")


I think this would be clearer as "...is only valid if", or "...is only meaningful when..."

edsantiago · 2022-11-01T11:37:17Z

docs/source/markdown/podman-run.1.md.in

@@ -477,7 +477,7 @@ tmpfs directories on _/run_ and _/tmp_.
 ```
 $ podman run --read-only -i -t fedora /bin/bash

-$ podman run --read-only --read-only-tmpfs=false --tmpfs /run -i -t fedora /bin/bash
+$ podman run --read-only --read-write-tmpfs=false --tmpfs /run -i -t fedora /bin/bash


(commenting on code that github does not show by default, so please click the uparrow-on-bar to see lines 473-475). Those lines read:

The best way to handle this is to mount tmpfs directories on /run and /tmp.

This looks like a holdover from a previous age: the implication is that /run and /tmp are also read-only. I haven't taken the time to go through history, but is it possible that the read-only-tmpfs option (the one that is being renamed here) was added after this was written, and that this used to be true but no longer is? Can you re-review lines 473-475 and see if they should be rewritten, maybe something like

Read-only containers still mount /run, /tmp, /var/tmp, /dev, and /dev/shm as read-write tmpfs. Use --read-write-tmpfs=false to protect even tmpfs directories, and --tmpfs=PATH to explicitly limit the read-write tmpfs filesystems.

(Idea only. That reads horribly. My documentation-writing brain is not fully engaged yet).

edsantiago · 2022-11-01T20:31:22Z

docs/source/markdown/options/read-write-tmpfs.md

+read-write tmpfs on _/run_, _/tmp_, and _/var/tmp_. When false, Podman sets
+the entire container such that no tmpfs can be written to unless specified on
+the command line.  Podman mounts _/dev_, _/dev/mqueue_, _/dev/pts_, _/dev/shm_
+as read only. The default is *true*


I find this hard to parse, and am really confused by the discrepancies between the first list of "these are tmpfs" and the second. I'm just going to hope that @TomSweeneyRedHat will offer his always-helpful input. Otherwise my (much less refined) thinking goes something like:

Only valid in combination with **--read-only**. Default: **true**. Normally a container run with **read-only** will mount the following `tmpfs` filesystems read/write: [ complete enumeration of those filesystems, possibly with a reference to where in the code that list is found ] With **--read-write-tmpfs=false**, even those filesystems are mounted read-only.

Change the --read-only-tmpfs option to --read-write-tmpfs since this makes sense and the old name was doing the exact opposite. Fixes: containers#12937 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan · 2022-12-20T18:48:59Z

Replaced by #16545

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 20, 2022

rhatdan force-pushed the tmpfs branch from 0996d89 to acbfbe8 Compare January 21, 2022 15:23

kolyshkin mentioned this pull request Jan 22, 2022

Fix working with read-only /dev opencontainers/runc#3345

Merged

rhatdan force-pushed the tmpfs branch from acbfbe8 to 6841288 Compare January 22, 2022 12:29

rhatdan changed the title ~~[WIP] --read-only-tmpfs=false should set /dev/* tmpfs to readonly~~ [WIP] --read-write-tmpfs=false should set /dev/* tmpfs to readonly Jan 22, 2022

rhatdan added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 25, 2022

rhatdan added the 4.1 label Jan 27, 2022

kolyshkin mentioned this pull request Jan 27, 2022

[1.1] Fix working with read-only /dev opencontainers/runc#3355

Merged

rhatdan force-pushed the tmpfs branch 2 times, most recently from ecc6563 to 9329fca Compare February 21, 2022 22:04

rhatdan changed the title ~~[WIP] --read-write-tmpfs=false should set /dev/* tmpfs to readonly~~ --read-write-tmpfs=false should set /dev/* tmpfs to readonly Feb 21, 2022

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 21, 2022

rhatdan force-pushed the tmpfs branch 3 times, most recently from 23268a9 to 8f10141 Compare February 28, 2022 21:20

rhatdan force-pushed the tmpfs branch 3 times, most recently from 68c331b to 28e0ef1 Compare August 16, 2022 15:01

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 3, 2022

rhatdan force-pushed the tmpfs branch from 28e0ef1 to 309e042 Compare September 5, 2022 11:14

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 5, 2022

rhatdan force-pushed the tmpfs branch from 309e042 to a934c7b Compare October 20, 2022 21:19

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Oct 20, 2022

rhatdan force-pushed the tmpfs branch 3 times, most recently from eac20a3 to 1f312b6 Compare October 31, 2022 12:48

rhatdan force-pushed the tmpfs branch from 1f312b6 to 99a34f7 Compare October 31, 2022 14:25

edsantiago requested changes Nov 1, 2022

View reviewed changes

rhatdan force-pushed the tmpfs branch 4 times, most recently from 1551a20 to 970f3b8 Compare November 1, 2022 19:32

edsantiago reviewed Nov 1, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2022

rhatdan force-pushed the tmpfs branch from 970f3b8 to 222e030 Compare November 7, 2022 11:40

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2022

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 2, 2022

--read-write-tmpfs=false should set /dev/* tmpfs to readonly

ec9640a

Change the --read-only-tmpfs option to --read-write-tmpfs since this makes sense and the old name was doing the exact opposite. Fixes: containers#12937 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan force-pushed the tmpfs branch from 222e030 to ec9640a Compare December 3, 2022 12:11

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2022

rhatdan closed this Dec 20, 2022

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 17, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--read-write-tmpfs=false should set /dev/* tmpfs to readonly #12954

--read-write-tmpfs=false should set /dev/* tmpfs to readonly #12954

rhatdan commented Jan 20, 2022 •

edited

rhatdan commented Jan 20, 2022

giuseppe commented Jan 21, 2022

giuseppe commented Jan 21, 2022

rhatdan commented Jan 21, 2022

rhatdan commented Jan 21, 2022

rhatdan commented Jan 21, 2022

kolyshkin commented Jan 22, 2022

rhatdan commented Jan 22, 2022

rhatdan commented Jan 22, 2022 •

edited

rhatdan commented Jan 27, 2022 •

edited

kolyshkin commented Jan 27, 2022

kolyshkin commented Jan 27, 2022

rhatdan commented Jan 28, 2022

openshift-ci bot commented Feb 28, 2022

edsantiago commented Oct 31, 2022

edsantiago left a comment

edsantiago Nov 1, 2022

edsantiago Nov 1, 2022

edsantiago Nov 1, 2022

edsantiago Nov 1, 2022

edsantiago Nov 1, 2022

edsantiago Nov 1, 2022

edsantiago Nov 1, 2022

rhatdan commented Dec 20, 2022

--read-write-tmpfs=false should set /dev/* tmpfs to readonly #12954

--read-write-tmpfs=false should set /dev/* tmpfs to readonly #12954

Conversation

rhatdan commented Jan 20, 2022 • edited

rhatdan commented Jan 20, 2022

giuseppe commented Jan 21, 2022

giuseppe commented Jan 21, 2022

rhatdan commented Jan 21, 2022

rhatdan commented Jan 21, 2022

rhatdan commented Jan 21, 2022

kolyshkin commented Jan 22, 2022

rhatdan commented Jan 22, 2022

rhatdan commented Jan 22, 2022 • edited

rhatdan commented Jan 27, 2022 • edited

kolyshkin commented Jan 27, 2022

kolyshkin commented Jan 27, 2022

rhatdan commented Jan 28, 2022

openshift-ci bot commented Feb 28, 2022

edsantiago commented Oct 31, 2022

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago Nov 1, 2022

Choose a reason for hiding this comment

edsantiago Nov 1, 2022

Choose a reason for hiding this comment

edsantiago Nov 1, 2022

Choose a reason for hiding this comment

edsantiago Nov 1, 2022

Choose a reason for hiding this comment

edsantiago Nov 1, 2022

Choose a reason for hiding this comment

edsantiago Nov 1, 2022

Choose a reason for hiding this comment

edsantiago Nov 1, 2022

Choose a reason for hiding this comment

rhatdan commented Dec 20, 2022

rhatdan commented Jan 20, 2022 •

edited

rhatdan commented Jan 22, 2022 •

edited

rhatdan commented Jan 27, 2022 •

edited