From 970f3b8083016bd138eecd564ba3614221b70d69 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Tue, 25 Oct 2022 10:42:12 -0400 Subject: [PATCH] --read-write-tmpfs=false should set /dev/* tmpfs to readonly Change the --read-only-tmpfs option to --read-write-tmpfs since this makes sense and the old name was doing the exact opposite. Fixes: https://github.com/containers/podman/issues/12937 Signed-off-by: Daniel J Walsh --- cmd/podman/common/create.go | 4 ++-- cmd/podman/common/create_opts.go | 2 +- cmd/podman/containers/create.go | 5 +++++ cmd/podman/utils/alias.go | 2 ++ .../markdown/options/read-only-tmpfs.md | 7 ------- .../markdown/options/read-write-tmpfs.md | 11 +++++++++++ docs/source/markdown/podman-create.1.md.in | 2 +- docs/source/markdown/podman-run.1.md.in | 8 +++----- pkg/api/handlers/compat/containers_create.go | 2 +- pkg/domain/entities/pods.go | 3 ++- pkg/specgen/specgen.go | 3 +++ pkg/specgenutil/specgen.go | 7 ++----- pkg/specgenutil/volumes.go | 8 ++++---- test/e2e/container_clone_test.go | 12 ++++++++++++ test/system/030-run.bats | 19 +++++++++++++++++++ 15 files changed, 68 insertions(+), 27 deletions(-) delete mode 100644 docs/source/markdown/options/read-only-tmpfs.md create mode 100644 docs/source/markdown/options/read-write-tmpfs.md diff --git a/cmd/podman/common/create.go b/cmd/podman/common/create.go index 369c6ddd00d4..595b7ec14dc8 100644 --- a/cmd/podman/common/create.go +++ b/cmd/podman/common/create.go @@ -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" diff --git a/cmd/podman/common/create_opts.go b/cmd/podman/common/create_opts.go index d77df29edb3b..b67ee8f70725 100644 --- a/cmd/podman/common/create_opts.go +++ b/cmd/podman/common/create_opts.go @@ -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" diff --git a/cmd/podman/containers/create.go b/cmd/podman/containers/create.go index 7e5c124b091f..2cf1f875f8f8 100644 --- a/cmd/podman/containers/create.go +++ b/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 valid only when --read-only=true") + } + return nil } diff --git a/cmd/podman/utils/alias.go b/cmd/podman/utils/alias.go index f6ea5110efe0..0425e635d828 100644 --- a/cmd/podman/utils/alias.go +++ b/cmd/podman/utils/alias.go @@ -33,6 +33,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) } diff --git a/docs/source/markdown/options/read-only-tmpfs.md b/docs/source/markdown/options/read-only-tmpfs.md deleted file mode 100644 index cae57e888be3..000000000000 --- a/docs/source/markdown/options/read-only-tmpfs.md +++ /dev/null @@ -1,7 +0,0 @@ -####> 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-only-tmpfs** - -If container is running in **--read-only** mode, then mount a read-write tmpfs on _/run_, _/tmp_, and _/var/tmp_. The default is **true**. diff --git a/docs/source/markdown/options/read-write-tmpfs.md b/docs/source/markdown/options/read-write-tmpfs.md new file mode 100644 index 000000000000..28610fafd44c --- /dev/null +++ b/docs/source/markdown/options/read-write-tmpfs.md @@ -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* diff --git a/docs/source/markdown/podman-create.1.md.in b/docs/source/markdown/podman-create.1.md.in index 36d071bdbd9c..789d0ca3e5d8 100644 --- a/docs/source/markdown/podman-create.1.md.in +++ b/docs/source/markdown/podman-create.1.md.in @@ -293,7 +293,7 @@ Suppress output information when pulling images @@option read-only -@@option read-only-tmpfs +@@option read-write-tmpfs @@option replace diff --git a/docs/source/markdown/podman-run.1.md.in b/docs/source/markdown/podman-run.1.md.in index d6cea4022c92..2f5bbaec3a84 100644 --- a/docs/source/markdown/podman-run.1.md.in +++ b/docs/source/markdown/podman-run.1.md.in @@ -317,7 +317,7 @@ Suppress output information when pulling images @@option read-only -@@option read-only-tmpfs +@@option read-write-tmpfs @@option replace @@ -470,14 +470,12 @@ content. Installing packages into _/usr_, for example. In production, applications seldom need to write to the image. Container applications write to volumes if they need to write to file systems at all. Applications can be made more secure by running them in read-only mode using the **--read-only** switch. -This protects the container's image from modification. Read-only containers may -still need to write temporary data. The best way to handle this is to mount -tmpfs directories on _/run_ and _/tmp_. +This protects the container's image from modification. ``` $ 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 ``` ### Exposing log messages from the container to the host's log diff --git a/pkg/api/handlers/compat/containers_create.go b/pkg/api/handlers/compat/containers_create.go index a86b0b0d5ccd..c2b723e72a4e 100644 --- a/pkg/api/handlers/compat/containers_create.go +++ b/pkg/api/handlers/compat/containers_create.go @@ -399,7 +399,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, diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go index a059cd7b5ba4..f0bd4d42e52f 100644 --- a/pkg/domain/entities/pods.go +++ b/pkg/domain/entities/pods.go @@ -249,7 +249,7 @@ type ContainerCreateOptions struct { Pull string Quiet bool ReadOnly bool - ReadOnlyTmpFS bool + ReadWriteTmpFS bool Restart string Replace bool Requires []string @@ -303,6 +303,7 @@ func NewInfraContainerCreateOptions() ContainerCreateOptions { IsInfra: true, ImageVolume: "bind", MemorySwappiness: -1, + ReadWriteTmpFS: true, } return options } diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index 8c77df20f851..9d9991796e7c 100644 --- a/pkg/specgen/specgen.go +++ b/pkg/specgen/specgen.go @@ -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. diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index 2695d887326d..c6fb78d0c8e7 100644 --- a/pkg/specgenutil/specgen.go +++ b/pkg/specgenutil/specgen.go @@ -566,10 +566,10 @@ 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 } @@ -577,9 +577,6 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions // 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) @@ -657,7 +654,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 } diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 503d82177a4e..c2bcf5e5289b 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -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 { @@ -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 } diff --git a/test/e2e/container_clone_test.go b/test/e2e/container_clone_test.go index 29ef3bc2ab3d..17462a3757c8 100644 --- a/test/e2e/container_clone_test.go +++ b/test/e2e/container_clone_test.go @@ -2,6 +2,7 @@ package integration import ( "os" + "strings" . "github.com/containers/podman/v4/test/utils" . "github.com/onsi/ginkgo" @@ -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)) @@ -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)) diff --git a/test/system/030-run.bats b/test/system/030-run.bats index 3440508cd23a..b13f63a48606 100644 --- a/test/system/030-run.bats +++ b/test/system/030-run.bats @@ -832,6 +832,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" + fi + fi + HOST=$(random_string 25) + 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" + 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