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

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

Closed
wants to merge 1 commit into from
Closed

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

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
4 changes: 2 additions & 2 deletions cmd/podman/common/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,8 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
"Make containers root filesystem read-only",
)
createFlags.BoolVar(
&cf.ReadOnlyTmpFS,
"read-only-tmpfs", cf.ReadOnlyTmpFS,
&cf.ReadWriteTmpFS,
"read-write-tmpfs", cf.ReadWriteTmpFS,
"When running containers in read-only mode mount a read-write tmpfs on /run, /tmp and /var/tmp",
)
requiresFlagName := "requires"
Expand Down
2 changes: 1 addition & 1 deletion cmd/podman/common/create_opts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func DefineCreateDefaults(opts *entities.ContainerCreateOptions) {
opts.MemorySwappiness = -1
opts.ImageVolume = podmanConfig.ContainersConfDefaultsRO.Engine.ImageVolumeMode
opts.Pull = policy()
opts.ReadOnlyTmpFS = true
opts.ReadWriteTmpFS = true
opts.SdNotifyMode = define.SdNotifyModeContainer
opts.StopTimeout = podmanConfig.ContainersConfDefaultsRO.Engine.StopTimeout
opts.Systemd = "true"
Expand Down
5 changes: 5 additions & 0 deletions cmd/podman/containers/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,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 valid only when --read-only=true")
}

return nil
}

Expand Down
2 changes: 2 additions & 0 deletions cmd/podman/utils/alias.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ func AliasFlags(f *pflag.FlagSet, name string) pflag.NormalizedName {
name = "os"
case "override-variant":
name = "variant"
case "read-only-tmpfs":
name = "read-write-tmpfs"
}
return pflag.NormalizedName(name)
}
Expand Down
7 changes: 0 additions & 7 deletions docs/source/markdown/options/read-only-tmpfs.md
View file
Open in desktop

This file was deleted.

11 changes: 11 additions & 0 deletions docs/source/markdown/options/read-write-tmpfs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
####> This option file is used in:
####> podman create, run
####> If you edit this file, make sure your changes
####> are applicable to all of those.
#### **--read-write-tmpfs**

If container is running in --read-only mode, when true Podman mounts a
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*
Copy link
Member

Choose a reason for hiding this comment

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

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.

2 changes: 1 addition & 1 deletion docs/source/markdown/podman-create.1.md.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ Suppress output information when pulling images

@@option read-only

@@option read-only-tmpfs
@@option read-write-tmpfs

@@option replace

Expand Down
6 changes: 3 additions & 3 deletions docs/source/markdown/podman-run.1.md.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,7 @@ Suppress output information when pulling images

@@option read-only

@@option read-only-tmpfs
@@option read-write-tmpfs

@@option replace

Expand Down Expand Up @@ -455,12 +455,12 @@ made more secure by running them in read-only mode using the **--read-only** swi
This protects the container's image from modification. By default read-only
containers can write to temporary data. Podman mounts a tmpfs on _/run_ and
_/tmp_ within the container. If the container should not write to any file
system within the container, including tmpfs, set --read-only-tmpfs=false.
system within the container, including tmpfs, set --read-write-tmpfs=false.

```
$ 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
Copy link
Member

Choose a reason for hiding this comment

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

(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).

```

### Exposing log messages from the container to the host's log
Expand Down
2 changes: 1 addition & 1 deletion pkg/api/handlers/compat/containers_create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ func cliOpts(cc handlers.CreateContainerConfig, rtc *config.Config) (*entities.C
PublishAll: cc.HostConfig.PublishAllPorts,
Quiet: false,
ReadOnly: cc.HostConfig.ReadonlyRootfs,
ReadOnlyTmpFS: true, // podman default
ReadWriteTmpFS: true, // podman default
Rm: cc.HostConfig.AutoRemove,
SecurityOpt: cc.HostConfig.SecurityOpt,
StopSignal: cc.Config.StopSignal,
Expand Down
3 changes: 2 additions & 1 deletion pkg/domain/entities/pods.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ type ContainerCreateOptions struct {
Pull string
Quiet bool
ReadOnly bool
ReadOnlyTmpFS bool
ReadWriteTmpFS bool
Restart string
Replace bool
Requires []string
Expand Down Expand Up @@ -308,6 +308,7 @@ func NewInfraContainerCreateOptions() ContainerCreateOptions {
IsInfra: true,
ImageVolume: "bind",
MemorySwappiness: -1,
ReadWriteTmpFS: true,
}
return options
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/specgen/specgen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,9 @@ type ContainerSecurityConfig struct {
// ReadOnlyFilesystem indicates that everything will be mounted
// as read-only
ReadOnlyFilesystem bool `json:"read_only_filesystem,omitempty"`
// ReadWriteTmpfs indicates that tmpfs will be mounted
// as read-only
ReadWriteTmpFS bool `json:"read_write_tmpfs,omitempty"`
// Umask is the umask the init process of the container will be run with.
Umask string `json:"umask,omitempty"`
// ProcOpts are the options used for the proc mount.
Expand Down
7 changes: 2 additions & 5 deletions pkg/specgenutil/specgen.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -585,20 +585,17 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
if !s.ReadOnlyFilesystem {
s.ReadOnlyFilesystem = c.ReadOnly
}
s.ReadWriteTmpFS = c.ReadWriteTmpFS
if len(s.ConmonPidFile) == 0 || len(c.ConmonPIDFile) != 0 {
s.ConmonPidFile = c.ConmonPIDFile
}

if len(s.DependencyContainers) == 0 || len(c.Requires) != 0 {
s.DependencyContainers = c.Requires
}

// TODO
// outside of specgen and oci though
// defaults to true, check spec/storage
// s.readonly = c.ReadOnlyTmpFS
// TODO convert to map?
// check if key=value and convert
sysmap := make(map[string]string)
for _, ctl := range c.Sysctl {
splitCtl := strings.SplitN(ctl, "=", 2)
Expand Down Expand Up @@ -676,7 +673,7 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions

// Only add read-only tmpfs mounts in case that we are read-only and the
// read-only tmpfs flag has been set.
mounts, volumes, overlayVolumes, imageVolumes, err := parseVolumes(c.Volume, c.Mount, c.TmpFS, c.ReadOnlyTmpFS && c.ReadOnly)
mounts, volumes, overlayVolumes, imageVolumes, err := parseVolumes(c.Volume, c.Mount, c.TmpFS, c.ReadWriteTmpFS && c.ReadOnly)
if err != nil {
return err
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/specgenutil/volumes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ var (
// Does not handle image volumes, init, and --volumes-from flags.
// Can also add tmpfs mounts from read-only tmpfs.
// TODO: handle options parsing/processing via containers/storage/pkg/mount
func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadOnlyTmpfs bool) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) {
func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadWriteTmpfs bool) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) {
// Get mounts from the --mounts flag.
unifiedMounts, unifiedVolumes, unifiedImageVolumes, err := Mounts(mountFlag)
if err != nil {
Expand Down Expand Up @@ -79,10 +79,10 @@ func parseVolumes(volumeFlag, mountFlag, tmpfsFlag []string, addReadOnlyTmpfs bo
}

// If requested, add tmpfs filesystems for read-only containers.
if addReadOnlyTmpfs {
readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
if addReadWriteTmpfs {
tmpfs := []string{"/tmp", "/var/tmp", "/run"}
options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
for _, dest := range readonlyTmpfs {
for _, dest := range tmpfs {
if _, ok := unifiedMounts[dest]; ok {
continue
}
Expand Down
12 changes: 12 additions & 0 deletions test/e2e/container_clone_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package integration

import (
"os"
"strings"

. "github.com/containers/podman/v4/test/utils"
. "github.com/onsi/ginkgo"
Expand Down Expand Up @@ -51,6 +52,11 @@ var _ = Describe("Podman container clone", func() {
Expect(ctrInspect).To(Exit(0))
Expect(ctrInspect.InspectContainerToJSON()[0].Name).To(ContainSubstring("-clone1"))

// FIXME when new crun reaches Ubuntu
if podmanTest.Host.Distribution == "ubuntu" &&
strings.Contains(podmanTest.OCIRuntime, "crun") {
Skip("The version of crun shipped by ubuntu is too old. Needs crun-1.4.2 or greater")
}
ctrStart := podmanTest.Podman([]string{"container", "start", clone.OutputToString()})
ctrStart.WaitWithDefaultTimeout()
Expect(ctrStart).To(Exit(0))
Expand Down Expand Up @@ -164,12 +170,18 @@ var _ = Describe("Podman container clone", func() {

It("podman container clone in a pod", func() {
SkipIfRootlessCgroupsV1("starting a container with the memory limits not supported")
// FIXME when new crun reaches Ubuntu
if podmanTest.Host.Distribution == "ubuntu" &&
strings.Contains(podmanTest.OCIRuntime, "crun") {
Skip("The version of crun shipped by ubuntu is too old. Needs crun-1.4.2 or greater")
}
run := podmanTest.Podman([]string{"run", "-dt", "--pod", "new:1234", ALPINE, "sleep", "20"})
run.WaitWithDefaultTimeout()
Expect(run).To(Exit(0))
clone := podmanTest.Podman([]string{"container", "clone", run.OutputToString()})
clone.WaitWithDefaultTimeout()
Expect(clone).To(Exit(0))

ctrStart := podmanTest.Podman([]string{"container", "start", clone.OutputToString()})
ctrStart.WaitWithDefaultTimeout()
Expect(ctrStart).To(Exit(0))
Expand Down
19 changes: 19 additions & 0 deletions test/system/030-run.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,6 +836,25 @@ EOF
is "${lines[0]}" ".*${HOST}.*"
}

@test "podman run --read-write-tmpfs" {
# 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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

fi
fi
HOST=$(random_string 25)
Copy link
Member

Choose a reason for hiding this comment

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

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

run_podman 1 run --rm --read-only $IMAGE touch /foo
is "$output" "touch: /foo: Read-only file system" "Should fail with read only file system"

for rwdir in /run /tmp /var/tmp /dev /dev/shm; do
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"
Comment on lines +851 to +854
Copy link
Member

Choose a reason for hiding this comment

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

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

done
}

@test "podman run doesn't override oom-score-adj" {
current_oom_score_adj=$(cat /proc/self/oom_score_adj)
run_podman run --rm $IMAGE cat /proc/self/oom_score_adj
Expand Down