Merge pull request #16545 from rhatdan/read-only

Add containers.conf read-only flag support

DOWNLOADS.md

@@ -30,7 +30,7 @@ don't take them beyond that.
 
 WARNING:  The items linked below all come from scripts in the `artifacts_task`
 map of `.cirrus.yml`.  When adding or updating any item below, please ensure it
-matches cooresponding changes in the artifacts task.
+matches corresponding changes in the artifacts task.
 
 -->
 

cmd/podman/common/create.go

@@ -377,12 +377,12 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 		)
 		createFlags.BoolVar(
 			&cf.ReadOnly,
-			"read-only", false,
+			"read-only", podmanConfig.ContainersConfDefaultsRO.Containers.ReadOnly,
 			"Make containers root filesystem read-only",
 		)
 		createFlags.BoolVar(
-			&cf.ReadOnlyTmpFS,
-			"read-only-tmpfs", cf.ReadOnlyTmpFS,
+			&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",
 		)
 		requiresFlagName := "requires"

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"

cmd/podman/containers/create.go

@@ -409,6 +409,8 @@ func createPodIfNecessary(cmd *cobra.Command, s *specgen.SpecGenerator, netOpts
 	infraOpts := entities.NewInfraContainerCreateOptions()
 	infraOpts.Net = netOpts
 	infraOpts.Quiet = true
+	infraOpts.ReadOnly = true
+	infraOpts.ReadWriteTmpFS = false
 	infraOpts.Hostname, err = cmd.Flags().GetString("hostname")
 	if err != nil {
 		return err

pkg/api/handlers/compat/containers_create.go

@@ -413,7 +413,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,

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

pkg/domain/infra/abi/play.go

@@ -68,6 +68,8 @@ func (ic *ContainerEngine) createServiceContainer(ctx context.Context, name stri
 		ImageVolume:      "bind",
 		IsInfra:          false,
 		MemorySwappiness: -1,
+		ReadOnly:         true,
+		ReadWriteTmpFS:   false,
 		// No need to spin up slirp etc.
 		Net: &entities.NetOptions{Network: specgen.Namespace{NSMode: specgen.NoNetwork}},
 	}
@@ -560,6 +562,8 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY
 		infraImage := util.DefaultContainerConfig().Engine.InfraImage
 		infraOptions := entities.NewInfraContainerCreateOptions()
 		infraOptions.Hostname = podSpec.PodSpecGen.PodBasicConfig.Hostname
+		infraOptions.ReadOnly = true
+		infraOptions.ReadWriteTmpFS = false
 		infraOptions.UserNS = options.Userns
 		podSpec.PodSpecGen.InfraImage = infraImage
 		podSpec.PodSpecGen.NoInfra = false
@@ -605,6 +609,16 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY
 		}
 	}
 
+	cfg, err := ic.Libpod.GetConfigNoCopy()
+	if err != nil {
+		return nil, nil, err
+	}
+
+	var readOnly types.OptionalBool
+	if cfg.Containers.ReadOnly {
+		readOnly = types.NewOptionalBool(cfg.Containers.ReadOnly)
+	}
+
 	ctrNames := make(map[string]string)
 	for _, initCtr := range podYAML.Spec.InitContainers {
 		// Error out if same name is used for more than one container
@@ -643,6 +657,7 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY
 			PodInfraID:         podInfraID,
 			PodName:            podName,
 			PodSecurityContext: podYAML.Spec.SecurityContext,
+			ReadOnly:           readOnly,
 			RestartPolicy:      define.RestartPolicyNo,
 			SeccompPaths:       seccompPaths,
 			SecretsManager:     secretsManager,
@@ -697,6 +712,7 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY
 			PodInfraID:         podInfraID,
 			PodName:            podName,
 			PodSecurityContext: podYAML.Spec.SecurityContext,
+			ReadOnly:           readOnly,
 			RestartPolicy:      ctrRestartPolicy,
 			SeccompPaths:       seccompPaths,
 			SecretsManager:     secretsManager,

pkg/specgen/generate/kube/kube.go

@@ -472,6 +472,8 @@ func ToSpecGen(ctx context.Context, opts *CtrSpecGenOptions) (*specgen.SpecGener
 	if ro := opts.ReadOnly; ro != itypes.OptionalBoolUndefined {
 		s.ReadOnlyFilesystem = (ro == itypes.OptionalBoolTrue)
 	}
+	// This should default to true for kubernetes yaml
+	s.ReadWriteTmpfs = true
 
 	// Make sure the container runs in a systemd unit which is
 	// stored as a label at container creation.

pkg/specgen/generate/storage.go

@@ -159,14 +159,19 @@ func finalizeMounts(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Ru
 	// Check for conflicts between named volumes and mounts
 	for dest := range baseMounts {
 		if _, ok := baseVolumes[dest]; ok {
-			return nil, nil, nil, fmt.Errorf("conflict at mount destination %v: %w", dest, specgen.ErrDuplicateDest)
+			return nil, nil, nil, fmt.Errorf("baseMounts conflict at mount destination %v: %w", dest, specgen.ErrDuplicateDest)
 		}
 	}
 	for dest := range baseVolumes {
 		if _, ok := baseMounts[dest]; ok {
-			return nil, nil, nil, fmt.Errorf("conflict at mount destination %v: %w", dest, specgen.ErrDuplicateDest)
+			return nil, nil, nil, fmt.Errorf("baseVolumes conflict at mount destination %v: %w", dest, specgen.ErrDuplicateDest)
 		}
 	}
+
+	if s.ReadWriteTmpfs {
+		baseMounts = addReadWriteTmpfsMounts(baseMounts, s.Volumes)
+	}
+
 	// Final step: maps to arrays
 	finalMounts := make([]spec.Mount, 0, len(baseMounts))
 	for _, mount := range baseMounts {
@@ -427,3 +432,29 @@ func InitFSMounts(mounts []spec.Mount) error {
 	}
 	return nil
 }
+
+func addReadWriteTmpfsMounts(mounts map[string]spec.Mount, volumes []*specgen.NamedVolume) map[string]spec.Mount {
+	readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
+	options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
+	for _, dest := range readonlyTmpfs {
+		if _, ok := mounts[dest]; ok {
+			continue
+		}
+		for _, m := range volumes {
+			if m.Dest == dest {
+				continue
+			}
+		}
+		mnt := spec.Mount{
+			Destination: dest,
+			Type:        define.TypeTmpfs,
+			Source:      define.TypeTmpfs,
+			Options:     options,
+		}
+		if dest != "/run" {
+			mnt.Options = append(mnt.Options, "noexec")
+		}
+		mounts[dest] = mnt
+	}
+	return mounts
+}

pkg/specgen/specgen.go

@@ -384,6 +384,10 @@ type ContainerSecurityConfig struct {
 	// ReadOnlyFilesystem indicates that everything will be mounted
 	// as read-only
 	ReadOnlyFilesystem bool `json:"read_only_filesystem,omitempty"`
+	// ReadWriteTmpfs indicates that when running with a ReadOnlyFilesystem
+	// mount temporary file systems
+	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.

pkg/specgenutil/specgen.go

@@ -592,10 +592,11 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
 		s.DependencyContainers = c.Requires
 	}
 
-	// TODO
-	// outside of specgen and oci though
-	// defaults to true, check spec/storage
-	// s.readonly = c.ReadOnlyTmpFS
+	// Only add ReadWrite tmpfs mounts iff the container is
+	// being run ReadOnly and ReadWriteTmpFS is not disabled,
+	// (user specifying --read-only-tmpfs=false.)
+	s.ReadWriteTmpfs = c.ReadOnly && c.ReadWriteTmpFS
+
 	//  TODO convert to map?
 	// check if key=value and convert
 	sysmap := make(map[string]string)
@@ -853,10 +854,6 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions
 		s.PasswdEntry = c.PasswdEntry
 	}
 
-	if c.ReadOnly && c.ReadOnlyTmpFS {
-		s.Mounts = addReadOnlyMounts(s.Mounts)
-	}
-
 	return nil
 }
 

pkg/specgenutil/volumes.go

@@ -703,26 +703,3 @@ func validChownFlag(flag string) (bool, error) {
 func unixPathClean(p string) string {
 	return path.Clean(p)
 }
-
-func addReadOnlyMounts(mounts []spec.Mount) []spec.Mount {
-	readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
-	options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
-	for _, dest := range readonlyTmpfs {
-		for _, m := range mounts {
-			if m.Destination == dest {
-				continue
-			}
-		}
-		mnt := spec.Mount{
-			Destination: dest,
-			Type:        define.TypeTmpfs,
-			Source:      define.TypeTmpfs,
-			Options:     options,
-		}
-		if dest != "/run" {
-			mnt.Options = append(mnt.Options, "noexec")
-		}
-		mounts = append(mounts, mnt)
-	}
-	return mounts
-}

test/system/030-run.bats

@@ -951,4 +951,17 @@ $IMAGE--c_ok" \
     run_podman stop -t 0 $cid
 }
 
+@test "podman run read-only from containers.conf" {
+    containersconf=$PODMAN_TMPDIR/containers.conf
+    cat >$containersconf <<EOF
+[containers]
+read_only=true
+EOF
+
+    CONTAINERS_CONF="$containersconf" run_podman 1 run --rm $IMAGE touch /testro
+    CONTAINERS_CONF="$containersconf" run_podman run --rm --read-only=false $IMAGE touch /testrw
+    CONTAINERS_CONF="$containersconf" run_podman run --rm $IMAGE touch /tmp/testrw
+    CONTAINERS_CONF="$containersconf" run_podman 1 run --rm --read-only-tmpfs=false $IMAGE touch /tmp/testro
+}
+
 # vim: filetype=sh

test/system/700-play.bats

@@ -238,6 +238,41 @@ EOF
     run_podman 1 container exists pod1-test3
 }
 
+@test "podman kube play read-only from containers.conf" {
+    containersconf=$PODMAN_TMPDIR/containers.conf
+    cat >$containersconf <<EOF
+[containers]
+read_only=true
+EOF
+
+    YAML=$PODMAN_TMPDIR/test.yml
+    CONTAINERS_CONF="$containersconf" run_podman create --pod new:pod1 --read-only=false --name test1 $IMAGE touch /testrw
+    CONTAINERS_CONF="$containersconf" run_podman create --pod pod1 --name test2 $IMAGE touch /testro
+    CONTAINERS_CONF="$containersconf" run_podman create --pod pod1 --name test3 $IMAGE touch /tmp/testtmp
+    CONTAINERS_CONF="$containersconf" run_podman container inspect --format '{{.HostConfig.ReadonlyRootfs}}' test1 test2 test3
+    is "$output" "false.*true.*true" "Rootfs should be read/only"
+
+    # Now generate and run kube.yaml on a machine without the defaults set
+    CONTAINERS_CONF="$containersconf" run_podman kube generate pod1 -f $YAML
+    cat $YAML
+
+    run_podman kube play --replace $YAML
+    run_podman container inspect --format '{{.HostConfig.ReadonlyRootfs}}' pod1-test1 pod1-test2 pod1-test3
+    is "$output" "false.*true.*true" "Rootfs should be read/only"
+
+    run_podman inspect --format "{{.State.ExitCode}}" pod1-test1
+    is "$output" "0" "Container / should be read/write"
+    run_podman inspect --format "{{.State.ExitCode}}" pod1-test2
+    is "$output" "1" "Container / should be read/only"
+    run_podman inspect --format "{{.State.ExitCode}}" pod1-test3
+    is "$output" "0" "/tmp in a read-only container should be read/write"
+
+    run_podman kube down - < $YAML
+    run_podman 1 container exists pod1-test1
+    run_podman 1 container exists pod1-test2
+    run_podman 1 container exists pod1-test3
+}
+
 @test "podman play with user from image" {
     TESTDIR=$PODMAN_TMPDIR/testdir
     mkdir -p $TESTDIR