Add containers.conf read-only flag support #16545

rhatdan · 2022-11-17T22:26:42Z

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 [email protected]

rhatdan · 2022-11-17T22:27:10Z

@alexlarsson @ygalblum PTAL
@containers/podman-maintainers PTAL

TomSweeneyRedHat · 2022-11-17T22:55:58Z

docs/source/markdown/options/read-only.md

@@ -1,11 +1,11 @@
 ####> This option file is used in:
-####>   podman create, run
+####>   podman create, kube play, run
 ####> If you edit this file, make sure your changes
 ####> are applicable to all of those.


Suggest "those commands."

These are auto-generated.

docs/source/markdown/options/read-only.md

Luap99 · 2022-11-18T11:27:07Z

@rhatdan Can you squash these PR comment commits please into your actual one, they increase the commit count for no reason and if I browse the commit log they provide no clue about the change.

Luap99

Should we just use

securityContext:
      readOnlyRootFilesystem: true

https://kubernetes.io/docs/reference/kubernetes-api/workload-resources/pod-v1/
in the yaml instead of adding this as flag?

rhatdan · 2022-11-18T14:22:16Z

I think we might want to use both. The goal is to allow automotive to force Pods to be read-only, potentially similar in quadlet.
It looks like that option does not currently work in containers either.

rhatdan · 2022-11-18T14:23:28Z

Actually my code breaks this use case.

alexlarsson · 2022-11-18T17:13:21Z

Since we already have an yaml options for this, I think we should use that, and then add support for changing its default value. Then quadlet can e.g. do --default-container=securityContext.readOnlyRootFilesystem=true or something like that. This seems more flexible than adding a bunch of options with unclear semantics in cases where e.g. some containers in the pod specify the field and some don't.

alexlarsson · 2022-11-18T17:15:23Z

Similarly, we could have --default-annotation=

ygalblum · 2022-11-20T07:38:30Z

As @Luap99 and @alexlarsson wrote, since there is a field for it in the K8S schema, podman kube play should use it and not add another flag for it. Having said that, as @alexlarsson wrote, we would like to be able to set a different default value for when the value is not set.
As for the suggestion regarding --default-container since container is a structure, maybe the argument should be a JSON. This way the user does not have to repeat the parameter if she wished to set more than one default value. Moreover, if it's a JSON, we should consider allowing it to be passed as a path to a file (maybe with the a prefix of @ like in Ansible)

rhatdan · 2022-11-20T15:12:23Z

To me that is a bit of a contradiction to have the ability to override just in quadlet but not from the command line. But I am not wed to having this option. Most of the framework of this PR is still required to allow quadlet to set that default.

ygalblum · 2022-11-20T15:18:02Z

Maybe I wasn't clear. I didn't mean for these flags to be quadlet specific, nor do I see how it can be done. I think these should be podman kube play flags that callers can use. One such caller is quadlet but any user can pass them.

vrothberg · 2022-11-22T09:16:42Z

cmd/podman/kube/play.go

@@ -137,6 +138,7 @@ func playFlags(cmd *cobra.Command) {
 	flags.BoolVar(&playOptions.NoHosts, "no-hosts", false, "Do not create /etc/hosts within the pod's containers, instead use the version from the image")
 	flags.BoolVarP(&playOptions.Quiet, "quiet", "q", false, "Suppress output information when pulling images")
 	flags.BoolVar(&playOptions.TLSVerifyCLI, "tls-verify", true, "Require HTTPS and verify certificates when contacting registries")
+	flags.BoolVar(&playOptions.ReadOnlyCLI, "read-only", false, "Make all containers root filesystem read-only")


Suggested change

flags.BoolVar(&playOptions.ReadOnlyCLI, "read-only", false, "Make all containers root filesystem read-only")

flags.BoolVar(&playOptions.ReadOnlyCLI, "read-only", false, "Make all containers' root filesystems read-only")

vrothberg · 2022-11-22T09:17:35Z

docs/source/markdown/options/add-host.md

@@ -1,7 +1,7 @@
 ####> This option file is used in:
 ####>   podman build, create, pod create, run
 ####> If you edit this file, make sure your changes
-####> are applicable to all of those.
+####> are applicable to all of those files.


This change should be broken out into another commit. It's causing a lot of noise.

pkg/specgen/generate/kube/kube.go

vrothberg · 2022-11-22T09:19:52Z

pkg/domain/infra/tunnel/kube.go

@@ -66,6 +66,9 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, opts en
 		options.WithAnnotations(opts.Annotations)
 	}
 	options.WithNoHosts(opts.NoHosts).WithUserns(opts.Userns)
+	if ro := opts.ReadOnly; ro != types.OptionalBoolUndefined {
+		options.WithReadOnly(ro == types.OptionalBoolTrue)


I don't see plumbing for it on the server side.

ygalblum · 2022-11-23T07:10:27Z

Considering the fact that the Pod manifest already has a field (at the container level) to control the "read-only" attribute, do we still want to add this flag?
If we add the flag, do we disregard the value in the manifest? If we also look at the manifest, what takes precedence?

rhatdan · 2022-11-23T09:37:03Z

The manifest is primary, but if the user overrides the flag, then the flag takes precedence.

podman kube play foobar.yaml # Yaml wins
podman kube play --readonly foobar.yaml # Read Only
podman kube play --readonly=false foobar.yaml # Read Write

vrothberg

Minor nits. Some code comments may help resolve my confusion.

cmd/podman/containers/create.go

pkg/domain/infra/abi/play.go

pkg/specgen/generate/storage.go

pkg/specgenutil/specgen.go

rhatdan · 2022-12-20T23:20:52Z

@containers/podman-maintainers PTAL
@vrothberg PTAL

vrothberg

One question but LGTM

pkg/domain/infra/abi/play.go

rhatdan · 2022-12-21T11:13:54Z

@giuseppe @alexlarsson @ygalblum @flouthoc PTAL

ygalblum · 2022-12-22T08:13:18Z

pkg/specgen/generate/storage.go

+	readonlyTmpfs := []string{"/tmp", "/var/tmp", "/run"}
+	options := []string{"rw", "rprivate", "nosuid", "nodev", "tmpcopyup"}
+	for _, dest := range readonlyTmpfs {
+		for _, m := range mounts {


From the last line of the outer loop, I can see that the dest field is also used as the key in the mounts map. If so, can you replace this loop with checking if dest exists in the map?

Nice catch.

ygalblum · 2022-12-22T08:14:05Z

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_wrie_tmpfs,omitempty"`


Typo in the json name, should be read_write_tmpfs

ygalblum · 2022-12-22T08:21:33Z

pkg/specgenutil/specgen.go

-	// defaults to true, check spec/storage
-	// s.readonly = c.ReadOnlyTmpFS
+	// Only add ReadWrite tmpfs mounts iff the container is ReadOnly and
+	// the user did not disable the --read-only-tmpfs flag.


This comment is based on the assumption that ReadWriteTmpFS's default is true. But, the default value is defined somewhere else (in cmd). If for some reason, the default is changed, no one will know to change this comment and it will left with wrong information. Maybe it's better to leave the user out of this commnet

I want future maintainers to understand what this logic is doing, so I attempted to improve the comment.

vrothberg · 2022-12-22T12:33:00Z

pkg/specgenutil/specgen.go:595:58: contianer is a misspelling of container (misspell)

Happens to me all the time as well.

ygalblum · 2022-12-22T13:43:06Z

pkg/specgenutil/specgen.go

-	// defaults to true, check spec/storage
-	// s.readonly = c.ReadOnlyTmpFS
+	// Only add ReadWrite tmpfs mounts iff the container is container is
+	// being run ReadOnly and ReadWrite tmpfs are disabled,


You have container is twice

Shouldn't this be ReadWrite tmpfs is not disabled?

Yup, hopefully no more typos...

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 <[email protected]>

vrothberg

LGTM
@giuseppe PTAL

rhatdan · 2022-12-23T10:16:19Z

@flouthoc PTAL
@ygalblum PTAL

ygalblum · 2022-12-25T08:01:07Z

LGTM

flouthoc

LGTM

openshift-ci · 2022-12-25T09:11:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [flouthoc,rhatdan,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Nov 17, 2022

TomSweeneyRedHat reviewed Nov 17, 2022

View reviewed changes

docs/source/markdown/options/read-only.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Nov 17, 2022

View reviewed changes

docs/source/markdown/options/read-only.md Outdated Show resolved Hide resolved

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2022

Luap99 reviewed Nov 18, 2022

View reviewed changes

rhatdan force-pushed the read-only branch from c0a7580 to 3a914b0 Compare November 18, 2022 14:46

rhatdan force-pushed the read-only branch from 3a914b0 to 54bcf27 Compare November 18, 2022 19:18

rhatdan force-pushed the read-only branch 2 times, most recently from 40eb724 to 7d8f1e3 Compare November 21, 2022 21:19

vrothberg reviewed Nov 22, 2022

View reviewed changes

ygalblum mentioned this pull request Nov 22, 2022

Kube Play: Add argument for default pod spec #16584

Closed

rhatdan force-pushed the read-only branch 4 times, most recently from 6c28ffc to 1a9bb58 Compare November 23, 2022 00:22

rhatdan force-pushed the read-only branch 2 times, most recently from b819fc0 to c9df82e Compare November 23, 2022 10:34

ygalblum mentioned this pull request Nov 23, 2022

Varios quadlet kube defaults #16597

Closed

vrothberg reviewed Dec 20, 2022

View reviewed changes

cmd/podman/containers/create.go Outdated Show resolved Hide resolved

pkg/domain/infra/abi/play.go Show resolved Hide resolved

pkg/specgen/generate/storage.go Outdated Show resolved Hide resolved

pkg/specgenutil/specgen.go Outdated Show resolved Hide resolved

rhatdan force-pushed the read-only branch from 6433fac to e98fe8b Compare December 20, 2022 13:42

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Dec 20, 2022

rhatdan force-pushed the read-only branch 2 times, most recently from 8082c44 to ebedf12 Compare December 20, 2022 18:45

rhatdan mentioned this pull request Dec 20, 2022

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

Closed

vrothberg reviewed Dec 21, 2022

View reviewed changes

pkg/domain/infra/abi/play.go Show resolved Hide resolved

rhatdan force-pushed the read-only branch from ebedf12 to 37e891b Compare December 21, 2022 14:47

ygalblum reviewed Dec 22, 2022

View reviewed changes

rhatdan force-pushed the read-only branch 2 times, most recently from 32d92c5 to 018f8cb Compare December 22, 2022 12:05

rhatdan force-pushed the read-only branch from 018f8cb to a5ddc58 Compare December 22, 2022 13:02

ygalblum reviewed Dec 22, 2022

View reviewed changes

rhatdan force-pushed the read-only branch from a5ddc58 to 0b49f9c Compare December 22, 2022 13:46

rhatdan force-pushed the read-only branch from 0b49f9c to 338b283 Compare December 22, 2022 16:57

vrothberg approved these changes Dec 23, 2022

View reviewed changes

flouthoc approved these changes Dec 25, 2022

View reviewed changes

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Dec 25, 2022

openshift-merge-robot merged commit 4a57cfb into containers:main Dec 25, 2022

This was referenced Jan 9, 2023

placeholder issue for quay.io flakes #16973

Closed

"open /etc in the container" is now a flake #16294

Closed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 17, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2023

	flags.BoolVar(&playOptions.ReadOnlyCLI, "read-only", false, "Make all containers root filesystem read-only")
	flags.BoolVar(&playOptions.ReadOnlyCLI, "read-only", false, "Make all containers' root filesystems read-only")

Add containers.conf read-only flag support #16545

Add containers.conf read-only flag support #16545

Conversation

rhatdan commented Nov 17, 2022 • edited Loading

rhatdan commented Nov 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 commented Nov 18, 2022

Luap99 left a comment

Choose a reason for hiding this comment

rhatdan commented Nov 18, 2022

rhatdan commented Nov 18, 2022

alexlarsson commented Nov 18, 2022

alexlarsson commented Nov 18, 2022

ygalblum commented Nov 20, 2022

rhatdan commented Nov 20, 2022

ygalblum commented Nov 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygalblum commented Nov 23, 2022

rhatdan commented Nov 23, 2022

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Dec 20, 2022

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Dec 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg commented Dec 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

rhatdan commented Dec 23, 2022

ygalblum commented Dec 25, 2022

flouthoc left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 25, 2022

rhatdan commented Nov 17, 2022 •

edited

Loading