Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of --read-only-tmpfs flag #20235

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Fix handling of --read-only-tmpfs flag #20235

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/podman/common/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
createFlags.BoolVar(
&cf.ReadWriteTmpFS,
"read-only-tmpfs", cf.ReadWriteTmpFS,
"When running containers in read-only mode mount a read-write tmpfs on /run, /tmp and /var/tmp",
"When running --read-only containers mount read-write tmpfs on /dev, /dev/shm, /run, /tmp and /var/tmp",
)
requiresFlagName := "requires"
createFlags.StringSliceVar(
Expand Down
21 changes: 20 additions & 1 deletion docs/source/markdown/options/read-only-tmpfs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,23 @@
####> are applicable to all of those.
#### **--read-only-tmpfs**

If container is running in **--read-only** mode, then mount a read-write tmpfs on _/dev_, _/dev/shm_, _/run_, _/tmp_, and _/var/tmp_. The default is **true**.
When running --read-only containers, mount a read-write tmpfs on _/dev_, _/dev/shm_, _/run_, _/tmp_, and _/var/tmp_. The default is **true**.

| --read-only | --read-only-tmpfs | / | /run, /tmp, /var/tmp|
| ----------- | ----------------- | ---- | ----------------------------------- |
| true | true | r/o | r/w |
| true | false | r/o | r/o |
| false | false | r/w | r/w |
| false | true | r/w | r/w |

When **--read-only=true** and **--read-only-tmpfs=true** additional tmpfs are mounted on
the /tmp, /run, and /var/tmp directories.

When **--read-only=true** and **--read-only-tmpfs=false** /dev and /dev/shm are marked
Read/Only and no tmpfs are mounted on /tmp, /run and /var/tmp. The directories
are exposed from the underlying image, meaning they are read-only by default.
This makes the container totally read-only. No writable directories exist within
the container. In this mode writable directories need to be added via external
volumes or mounts.

By default, when **--read-only=false**, the /dev and /dev/shm are read/write, and the /tmp, /run, and /var/tmp are read/write directories from the container image.
7 changes: 5 additions & 2 deletions test/system/030-run.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1120,10 +1120,13 @@ EOF
CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch /testrw
CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm $IMAGE touch /tmp/testrw
for dir in /tmp /var/tmp /dev /dev/shm /run; do
CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm $IMAGE touch $dir/testro
CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch $dir/testro
CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false --read-only-tmpfs=true $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.

This seems wrong to me? If I ask for read-only-tmpfs, I want a read-only tmpfs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the table to the docs. --read-only=true is required in order to add the --read-only-tmpfs=true.

CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only-tmpfs=true $IMAGE touch $dir/testro

CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman 1 run --rm --read-only-tmpfs=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.

These are such confusing options. I don't have the brainpower to review right now, so just two quick points:

  • for those who care, this is a breaking change
  • for completeness, should we check all permutations of --read-only with --read-only-tmpfs?

Copy link
Member Author

Choose a reason for hiding this comment

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

The breaking change would have been to change the --read-only-tmpfs to --read-write-tmpf, which it is called internally. The issue is that if the user actually sets the constant it does it backwards, While if the user never sets it the default is to have read-write-tmpfs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Bottom line is the bug --read-only-tmpfs=false Was turning on read-only-tmpfs while --read-only-tmfs=true was turning off read-only-tmpfs.

assert "$output" =~ "touch: $dir/testro: Read-only file system"
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
done
}

Expand Down