specify cgroup ownership semantics

[frasertweedale]

cgroups v2 supports secure delegation of cgroups. Accordingly,
control over a cgroup (that is, creation of new child cgroups and
movement of processes and threads among the cgroup subtree exposed
to a container) can be safely delegated to a container. Adjusting
the ownership enables real-world use cases like systemd-based
containers fully isolated in user namespaces.

To encourage adoption of this feature, and secure implementation,
define the semantics of cgroup ownership. Changing/setting the
cgroup ownership is only allowed on cgroups v2, and the specific
files whose ownership can be change are mentioned.

In terms of current practice, this is already the behaviour of crun
(which also chown's the memory.oom.group file), and there is a pull
request for runc: opencontainers/runc#3057
(the behaviour is enabled by an annotation).

[cyphar]

Seems reasonable but using MUSTs is far too strong IMHO.

[frasertweedale]

@cyphar thanks for your comments. I'll push an update later today, or tomorrow.

[frasertweedale]

PR updated to fix a grammar issue and relax MUST to SHOULD per @cyphar's comments.

[kolyshkin]

One question: should we also require cgroupns? It is assumed with cgroup v2, but what it it is not set in config?

I have just checked that by removing cgroup ns from config.json, and the whole host /sys/fs/cgroup is mounted (for both crun and runc). Meaning we definitely need to add cgroupns to the list of conditions.

[frasertweedale]

One question: should we also require cgroupns? It is assumed with cgroup v2, but what it it is not set in config?

I have just checked that by removing cgroup ns from config.json, and the whole host /sys/fs/cgroup is mounted (for both crun and runc). Meaning we definitely need to add cgroupns to the list of conditions.

@kolyshkin good point, I agree. Should we further restrict it to a cgroupns without path specified? That is, where the runtime must create a new cgroupns? Otherwise path could be a reference to an arbitrary, pre-existing cgroupns (possibly the "root" host cgroupns).

edit I pushed an update that adds this requirement, with the empty path as I proposed.

[frasertweedale]

Are there any further comments about this PR. Any further concerns? What else needs to be done for this PR to be ready to merge?

[tianon]

Seems sane to me

@cyphar @kolyshkin willing to take another look? 😅🙏

[frasertweedale]

weekly nudge...

[kolyshkin]

LGTM (with the two commits squashed).

[frasertweedale]

@kolyshkin thank you. I squashed the commits.

[frasertweedale]

Seeking more reviews, and hopefully approvals, for this enhancement. Ping @crosbymichael, @cyphar, @dqminh, @giuseppe, @hqhq, @mrunalp, @vbatts, @vishh.

[ajwock]

I wanted to mention that there is a file named /sys/kernel/cgroup/delegate (since linux 4.15) which contains a list of delegable files for cgroups. This allows further files to be delegated in the future. My fedora 33 machine has the 3 files mentioned above as well as a new one, memory.oom.group in its /sys/kernel/cgroup/delegate file.

[frasertweedale]

Thanks for the comment. Do folks think it makes sense to update runtime-spec to instruct runtimes to read the files to be delegated from /sys/kernel/cgroup/delegate instead (possibly with the current list as fallback)?

[kolyshkin]

I guess, it should be worded like "The runtime SHOULD use the list of files from /sys/kernel/cgroup/delegate. If that file can not be used, the runtime MAY fall back to the following list" and these three files.

[ajwock]

Oh, also I'm curious- is there any reason it would be insecure for v2 cgroups to be delegated to a user other than root while running in the host namespace? Requiring a user namespace before you can manage the cgroup hierarchy adds baggage that would be less than ideal for some use cases, I'm sure.

[frasertweedale]

Oh, also I'm curious- is there any reason it would be insecure for v2 cgroups to be delegated to a user other than root while running in the host namespace? Requiring a user namespace before you can manage the cgroup hierarchy adds baggage that would be less than ideal for some use cases, I'm sure.

We could chown to process.user.uid in the namespace, instead of uid 0 unconditionally. That is, the user that will actually execute the initial process in the container. Ordinarily that would be uid 0, but it's possible to specify configs that do something different, and even to specify a config where uid 0 is unmapped in the namespace.

Specifying that behaviour would be a relaxation of the cgroup ownership semantics as currently proposed, thus backwards compatible. Is there a use case that compels us to accommodate this scenario right now? Or shall we keep it simple for now and only chown to the namespace's uid 0 (whether or not that is the uid that will execute the process, and failing if uid 0 is unmapped)?

[ajwock]

Personally, I have a use case for managing cgroups within a container but I would prefer not to add the overhead of a user namespace if it isn't necessary.

@giuseppe and others do you see any problem with allowing this?

[frasertweedale]

Personally, I have a use case for managing cgroups within a container but I would prefer not to add the overhead of a user namespace if it isn't necessary.

@giuseppe and others do you see any problem with allowing this?

The proposed behaviour doesn't require a (new) user namespace - only that the cgroup should be owned by uid 0 in the container's user namespace (which may be the host user namespace). So to admit your use case, we just need to change it to say that the cgroup should be owned by the host uid that maps to the value of process.user.uid in the container user namespace. Does anyone object to this change?

[frasertweedale]

Dear maintainers / reviewers, it would be great if we could get some more eyes on this and hopefully, approval and merge.
Ping @giuseppe @mrunalp @cyphar ?

[giuseppe]

LGTM

[dqminh]

LGTM

[frasertweedale]

Thank you to reviewers. We now have the requisite number of approvals. @cyphar has an older review with changes requested; I believe I have resolved all of the feedback.

Can this be merged now?

[frasertweedale]

Weekly bump. Could someone please merge it, or explain what prevents it?

[frasertweedale]

Weekly bump (Nov 22). Could someone please merge it, or explain what else needs to be done or is preventing this PR from being merged?

[vbatts]

@frasertweedale thank you for the weekly bumps 😬
Looks like the "MUST" that @cyphar commented on was never marked resolved. Perhaps that's why the other maintainers didn't merge? 🤷

LGTM

[frasertweedale]

@vbatts thank you so much! Thanks to all for their feedback and insights.