From 338b28393566c40b9a5e79d917182375adba848c Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Sat, 10 Dec 2022 06:35:58 -0500 Subject: [PATCH] Add containers.conf read-only flag support If you are running temporary containers within podman play kube we should really be running these in read-only mode. For automotive they plan on running all of their containers in read-only temporal mode. Adding this option guarantees that the container image is not being modified during the running of the container. The containers can only write to tmpfs mounted directories. Signed-off-by: Daniel J Walsh --- DOWNLOADS.md | 2 +- cmd/podman/common/create.go | 6 ++-- cmd/podman/common/create_opts.go | 2 +- cmd/podman/containers/create.go | 2 ++ pkg/api/handlers/compat/containers_create.go | 2 +- pkg/domain/entities/pods.go | 2 +- pkg/domain/infra/abi/play.go | 16 +++++++++ pkg/specgen/generate/kube/kube.go | 2 ++ pkg/specgen/generate/storage.go | 35 ++++++++++++++++++-- pkg/specgen/specgen.go | 4 +++ pkg/specgenutil/specgen.go | 13 +++----- pkg/specgenutil/volumes.go | 23 ------------- test/system/030-run.bats | 13 ++++++++ test/system/700-play.bats | 35 ++++++++++++++++++++ 14 files changed, 117 insertions(+), 40 deletions(-) diff --git a/DOWNLOADS.md b/DOWNLOADS.md index 1140aafad6..046ff5eabb 100644 --- a/DOWNLOADS.md +++ b/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. --> diff --git a/cmd/podman/common/create.go b/cmd/podman/common/create.go index 853e37b59a..42b6d28d56 100644 --- a/cmd/podman/common/create.go +++ b/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" diff --git a/cmd/podman/common/create_opts.go b/cmd/podman/common/create_opts.go index d77df29edb..b67ee8f707 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 e15877023f..d9da303cf5 100644 --- a/cmd/podman/containers/create.go +++ b/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 diff --git a/pkg/api/handlers/compat/containers_create.go b/pkg/api/handlers/compat/containers_create.go index 234c101c81..107070f2c0 100644 --- a/pkg/api/handlers/compat/containers_create.go +++ b/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, diff --git a/pkg/domain/entities/pods.go b/pkg/domain/entities/pods.go index a70e3c8dca..36676d56d2 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 diff --git a/pkg/domain/infra/abi/play.go b/pkg/domain/infra/abi/play.go index 430ce7bd41..e3752c0a61 100644 --- a/pkg/domain/infra/abi/play.go +++ b/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, diff --git a/pkg/specgen/generate/kube/kube.go b/pkg/specgen/generate/kube/kube.go index 4ca74fc40e..29704e6149 100644 --- a/pkg/specgen/generate/kube/kube.go +++ b/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. diff --git a/pkg/specgen/generate/storage.go b/pkg/specgen/generate/storage.go index 6b131c1ddc..ffaaa0e1fb 100644 --- a/pkg/specgen/generate/storage.go +++ b/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 +} diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index 6daa4dc018..2e7078115e 100644 --- a/pkg/specgen/specgen.go +++ b/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. diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index 8c64508bef..695018f6fc 100644 --- a/pkg/specgenutil/specgen.go +++ b/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 } diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 6b6c0d91ce..c70206addf 100644 --- a/pkg/specgenutil/volumes.go +++ b/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 -} diff --git a/test/system/030-run.bats b/test/system/030-run.bats index fff16a1d59..1710083939 100644 --- a/test/system/030-run.bats +++ b/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 <$containersconf <