Do not allow mounting to machine dir /tmp #25466
Conversation
openshift-ci
bot
added
release-note
approved
|
i'm thinking i should probably add a test that checks that |
There was a problem hiding this comment.
Makes sense, however that raises the question if we restrict /tmp why no other paths. Surely mounting over /etc, /var, /usr, /run and many more would brick the system in possibly even worse ways. I even have seen people mounting /var/lib/containers (or the home dir path) which cannot work as our virtofs mount seem to be limted to a single uid so it cannot work as universal storage path.
So it would be better to rework that to block a defined list of mounts I think.
|
Ephemeral COPR build failed. @containers/packit-build please check. |
because we are fixing bugs on specific bug reports. that said, I am fine with reworking this again. Are /tmp /etc/ /var the only ones you want restricted? How about /dev , /usr , /proc /sys /bin? |
|
How about /bin /boot /dev /etc /home /proc /root /run /sbin /sys /tmp /usr /var? |
baude
force-pushed
the
issue18230
branch
2 times, most recently
from
61c2f13 to
78762e6
Compare
| @@ -149,6 +149,11 @@ options are: | |||
| * **rw**: mount volume read/write (default) | |||
| * **security_model=[model]**: specify 9p security model (see below) | |||
|
|
|||
| Note: The following destinations are forbidden for volumes: `/bin`, `/boot`, `/dev`, `/etc`, | |||
| `/home`, `/proc`, `/root`, `/run`, `/sbin`, `/sys`, `/tmp`, `/usr`, and `/var`. Subdirectories | |||
| of these destinations are allowed. | |||
There was a problem hiding this comment.
While we do allow subdirectories, it is still not recommend, maybe it should be made clear that mounting over existing important system directories is not supported and can lead to many issues.
There was a problem hiding this comment.
with the drift of this PR, it has lost some of its meaning. In the case of /tmp, subdirs are fine as you point out but in that case I have no problem recommending it. /var on the other hand, yeah ... not a good idea imho
There was a problem hiding this comment.
i added in some text to say users need to be careful.
When certain directories, like /tmp, get mounted over, FCOS/Linux can act in unexpected ways. Added a sanity check for a list of directories think might be impacted by this. Also, moved the volume parsing earlier in the init process so we can catch problems before the expensive decompression of machine images. The following destinations are forbidden for volumes: `/bin`, `/boot`, `/dev`, `/etc`, `/home`, `/proc`, `/root`, `/run`, `/sbin`, `/sys`, `/tmp`, `/usr`, and `/var`. Subdirectories Fixes: containers#18230 Signed-off-by: Brent Baude <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, jakecorrenti, Luap99 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:
Approvers can indicate their approval by writing |
the destination machine mount overwrote /tmp. Here I have added a sanity check. I also moved the volume parsing and check earlier in the init function so that one does not have to endure the decompression and clean up of the machine image for cli parsing.
Fixes: #18230
Does this PR introduce a user-facing change?