Skip to content

make /dev & /dev/shm read/only when --read-only --read-only-tmpfs=false#19422

Merged
openshift-merge-robot merged 1 commit intocontainers:mainfrom
rhatdan:read-only
Jul 31, 2023
Merged

make /dev & /dev/shm read/only when --read-only --read-only-tmpfs=false#19422
openshift-merge-robot merged 1 commit intocontainers:mainfrom
rhatdan:read-only

Conversation

@rhatdan
Copy link
Member

@rhatdan rhatdan commented Jul 29, 2023

The intention of --read-only-tmpfs=fals when in --read-only mode was to not allow any processes inside of the container to write content anywhere, unless the caller also specified a volume or a tmpfs. Having /dev and /dev/shm writable breaks this assumption.

Fixes: #12937

Does this PR introduce a user-facing change?

--read-only-tmpfs flag now effects /dev and /dev/shm as well as /run, /tmp, /var/tmp

@openshift-ci openshift-ci bot added do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None approved Indicates a PR has been approved by an approver from all required OWNERS files. release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jul 29, 2023
The intention of --read-only-tmpfs=fals when in --read-only mode was to
not allow any processes inside of the container to write content
anywhere, unless the caller also specified a volume or a tmpfs. Having
/dev and /dev/shm writable breaks this assumption.

Fixes: containers#12937

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@edsantiago PTAL

Comment on lines +1079 to +1080
CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only-tmpfs=true $IMAGE touch $dir/testro
CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch $dir/testro
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems a teeny bit wasteful to run these two in a loop, when they could be oneliners... but bash array handling isn't good enough to solve this in a human-readable way. Maybe someone has a clean solution, but if not, tests LGTM.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, rhatdan, vrothberg

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [edsantiago,rhatdan,vrothberg]

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

@rhatdan rhatdan added the lgtm Indicates that a PR is ready to be merged. label Jul 31, 2023
@openshift-merge-robot openshift-merge-robot merged commit 6b40475 into containers:main Jul 31, 2023
@github-actions 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 Oct 30, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RFE: --read-only: add sub-option to make /dev readonly as well

4 participants